Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Bug]: DNS CNAME record is always queried, even when the query for an A record already succeeded #48048

Closed
4 of 8 tasks
georglauterbach opened this issue Sep 15, 2024 · 3 comments · Fixed by #48675
Closed
4 of 8 tasks
Labels
0. Needs triage Pending check for reproducibility or if it fits our roadmap 29-feedback bug

Comments

@georglauterbach
Copy link
Contributor

georglauterbach commented Sep 15, 2024

⚠️ This issue respects the following points: ⚠️

Bug description

I am looking at this code:

$dnsTypes = \defined('AF_INET6') || @inet_pton('::1')
? [DNS_A, DNS_AAAA, DNS_CNAME]
: [DNS_A, DNS_CNAME];
foreach ($dnsTypes as $dnsType) {
if ($this->negativeDnsCache->isNegativeCached($target, $dnsType)) {
continue;
}
$dnsResponses = $this->dnsGetRecord($target, $dnsType);
$canHaveCnameRecord = true;
if ($dnsResponses !== false && count($dnsResponses) > 0) {
foreach ($dnsResponses as $dnsResponse) {
if (isset($dnsResponse['ip'])) {
$targetIps[] = $dnsResponse['ip'];
$canHaveCnameRecord = false;
} elseif (isset($dnsResponse['ipv6'])) {
$targetIps[] = $dnsResponse['ipv6'];
$canHaveCnameRecord = false;
} elseif (isset($dnsResponse['target']) && $canHaveCnameRecord) {
$targetIps = array_merge($targetIps, $this->dnsResolve($dnsResponse['target'], $recursionCount));
$canHaveCnameRecord = true;
}
}
} elseif ($dnsNegativeTtl !== null) {
$this->negativeDnsCache->setNegativeCacheForDnsType($target, $dnsType, $dnsNegativeTtl);
}
}
return $targetIps;

Important

I am by no means a PHP expert, please tell me if I am mistaken.

To me, the way the code is constructed would indicate that CNAME is always queried, even when A succeeded. This aligns with what I am observing in my DNS server logs:

[INFO] 10.10.0.38:42508 - 60413 "CNAME IN updates.nextcloud.com. udp 50 false 1200" NOERROR qr,rd,ra 116 0.039573751s
[INFO] 10.10.0.38:46316 - 26341 "CNAME IN updates.nextcloud.com.cloud.svc.cluster. udp 68 false 1200" NXDOMAIN qr,aa,rd,ra 132 0.000151319s
[INFO] 10.10.0.38:42074 - 34897 "CNAME IN updates.nextcloud.com.cluster. udp 58 false 1200" NXDOMAIN qr,aa,rd,ra 122 0.000110026s
[INFO] 10.10.0.38:54477 - 22629 "CNAME IN apps.nextcloud.com.cloud.svc.cluster. udp 65 false 1200" NXDOMAIN qr,aa,rd,ra 129 0.000151308s
[INFO] 10.10.0.38:49188 - 36978 "CNAME IN www.nextcloud.com.cloud.svc.cluster. udp 64 false 1200" NXDOMAIN qr,aa,rd,ra 128 0.000266391s
[INFO] 10.10.0.38:48706 - 44842 "CNAME IN www.nextcloud.com.cluster. udp 54 false 1200" NXDOMAIN qr,aa,rd,ra 118 0.000134442s
[INFO] 10.10.0.38:40937 - 47170 "CNAME IN nextcloud.com. udp 42 false 1200" NOERROR qr,rd,ra 108 0.040937649s
[INFO] 10.10.0.38:57557 - 28949 "CNAME IN nextcloud.com.svc.cluster. udp 54 false 1200" NXDOMAIN qr,aa,rd,ra 118 0.000163416s

Wouldn't it make more sense to skip CNAME when canHaveCnameRecord is false, and we should set canHaveCnameRecord outside the loop?

Steps to reproduce

  1. Run NC
  2. Look at DNS server logs

Expected behavior

When canHaveCnameRecord is false, we should not query for CNAME.

Nextcloud Server version

29

Operating system

Debian/Ubuntu

PHP engine version

PHP 8.3

Web server

Apache (supported)

Database engine version

PostgreSQL

Is this bug present after an update or on a fresh install?

None

Are you using the Nextcloud Server Encryption module?

None

What user-backends are you using?

  • Default user-backend (database)
  • LDAP/ Active Directory
  • SSO - SAML
  • Other

Configuration report

I will deliver upon request; I do not think the config is important for this issue.

List of activated Apps

I will deliver upon request; I do not think the config is important for this issue.

Nextcloud Signing status

I will deliver upon request; I do not think the config is important for this issue.

Nextcloud Logs

I will deliver upon request; I do not think the config is important for this issue.

Additional info

This is not a showstopper in any way; I just think the behavior is neither expected nor desirable.

@georglauterbach georglauterbach added 0. Needs triage Pending check for reproducibility or if it fits our roadmap bug labels Sep 15, 2024
@georglauterbach
Copy link
Contributor Author

georglauterbach commented Oct 12, 2024

@solracsf any progress or update on this? I usually cannot afford to keep issues open for a long time; otherwise I lose the overview. The fix for this should also take not more than 5 minutes, for me maybe 20 considering I do not know PHP.

@solracsf
Copy link
Member

any progress or update on this

If there isn't any PR attached, no. Feel free to submit one to be reviwed 👍

@georglauterbach
Copy link
Contributor Author

@solracsf please take a look at #48675 :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
0. Needs triage Pending check for reproducibility or if it fits our roadmap 29-feedback bug
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants