Skip to content

fix(DnsPinning): Ensure to always lookup based on FQDN#59147

Merged
kesselb merged 1 commit intomasterfrom
fix/use-fqdn-for-dns-pinning
Mar 23, 2026
Merged

fix(DnsPinning): Ensure to always lookup based on FQDN#59147
kesselb merged 1 commit intomasterfrom
fix/use-fqdn-for-dns-pinning

Conversation

@DerDreschner
Copy link
Contributor

@DerDreschner DerDreschner commented Mar 21, 2026

Summary

After upgrading my private server from Debian 12 to Debian 13, I've experienced the same issue mentioned in #56489 which resulted in the app store to be totally unusable. Upgrading the Nextcloud version worked without any issues tho.

The investigation got the following results:

  • The main issue is the DNS Pinning
  • When looking up github.com, it tried to resolve A, AAAA and CNAME records
  • It only got an A and AAAA record back
  • The A record had the correct host and IP, but the AAAA record didn't. It was the result for github.com.dreschner.net
  • cURL tries to connect via the (incorrectly) provided IPv6 address first
  • The certificate doesn't match the expected hostname and resulting in the error 60

For reproducing the issue on command line level, you can use the following command on a server with IPv6 connectivity:

curl -v https://github.com/nextcloud-releases/integration_giphy/releases/download/v2.2.0/integration_giphy-v2.2.0.tar.gz --resolve 'github.com:443:140.82.121.4,2a0a:4cc0:c1:71d6:1876:e0ff:fe86:4038'

The reason is that we don't use FQDNs for resolving the DNS records and GitHub doesn't provide an AAAA record. Therefore, the program behind dns_get_records() looks for the host as subdomain of the local domain name. This works as I have a wildcard AAAA record set-up.

The fix is easy: just add a . at the end to make the requested domain name a FQDN and prevent the lookup under the local domain name when no record is being found in the first lookup.

Checklist

AI (if applicable)

  • The content of this PR was partly or fully generated using AI

@DerDreschner DerDreschner requested a review from a team as a code owner March 21, 2026 19:08
@DerDreschner DerDreschner requested review from ArtificialOwl, icewind1991, provokateurin and salmart-dev and removed request for a team March 21, 2026 19:08
@DerDreschner DerDreschner added bug 3. to review Waiting for reviews labels Mar 21, 2026
@DerDreschner
Copy link
Contributor Author

/backport to stable32

@DerDreschner DerDreschner force-pushed the fix/use-fqdn-for-dns-pinning branch from 31e56ed to d413f74 Compare March 21, 2026 19:25
@DerDreschner
Copy link
Contributor Author

/backport to stable33

@DerDreschner DerDreschner force-pushed the fix/use-fqdn-for-dns-pinning branch 2 times, most recently from b6f77cb to aa45c07 Compare March 21, 2026 19:39
@DerDreschner DerDreschner enabled auto-merge March 21, 2026 19:44
Copy link
Member

@provokateurin provokateurin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Uh that is quite something 😅

@DerDreschner DerDreschner force-pushed the fix/use-fqdn-for-dns-pinning branch from aa45c07 to 2d3b281 Compare March 22, 2026 18:54
Comment on lines +34 to +35
* @param string $hostname
* @return string
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: @param/@return phpdoc comments are redundant with the type hints

Copy link
Contributor Author

@DerDreschner DerDreschner Mar 22, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

True, I have to figure out how to get PhpStorm to not do that when adding phpdoc comments.

@DerDreschner DerDreschner force-pushed the fix/use-fqdn-for-dns-pinning branch from 2d3b281 to f728b49 Compare March 22, 2026 19:34
Signed-off-by: David Dreschner <david.dreschner@nextcloud.com>
@DerDreschner DerDreschner force-pushed the fix/use-fqdn-for-dns-pinning branch from f728b49 to 5bc0ba6 Compare March 22, 2026 19:35
@DerDreschner DerDreschner disabled auto-merge March 22, 2026 19:40
@ChristophWurst ChristophWurst added 4. to release Ready to be released and/or waiting for tests to finish and removed 3. to review Waiting for reviews labels Mar 23, 2026
@kesselb kesselb merged commit 8032ad8 into master Mar 23, 2026
174 checks passed
@kesselb kesselb deleted the fix/use-fqdn-for-dns-pinning branch March 23, 2026 15:36
@DerDreschner
Copy link
Contributor Author

I've realized that my fix has an unexpected side effect. It totally prevents the usage of relative domains, as all domains are being treated as FQDNs from a DNS point of view. That's totally fine for the app store - but a bit unexpected for other developers and might break some apps, as it's usually possible to use both relative and absolute domains on any HTTP request. As most apps use our implementation, I lean on fixing it before someone might complain about it (and hence merging it into stable33 and stable32).

I can think of two ways to tackle that:

  1. Put the list of IANA root zones into the resources folder (probably cache it for performance reasons) and check against that before marking the domain as an FQDN.
  2. Rewrite the DNS pinning: Let cURL resolve the domain name, lookup the SOA record and then cache the result and inject it into any further request for the specified TTL.

The suggested change here to discard the record if it doesn't match the requested host would have the same unexpected side effect as my fix does. So, that solution is not possible, unfortunately. Also, as the example with github.com.dreschner.net demonstrates, we're unable to just check if the domain is likely a hostname (has no dot in it).

Any thoughts on this? @kesselb @provokateurin @ChristophWurst

@ChristophWurst
Copy link
Member

Unfortunately I absolutely lack expertise in this area. I don't know.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

4. to release Ready to be released and/or waiting for tests to finish bug

Projects

None yet

4 participants