nsupdate: fix zone lookup #5818

n0p90 · 2023-01-11T22:10:22Z

The SOA record for an existing zone is returned as an answer RR and not as an authority RR. It is returned as an authority for subdomains of a zone.

$ dig -t SOA example.com
;; ANSWER SECTION:
example.com.	3530	IN	SOA	ns.icann.org. noc.dns.icann.org. 2022091184 7200 3600 1209600 3600

$ dig -t SOA www.example.com
;; AUTHORITY SECTION:
example.com.	3600	IN	SOA	ns.icann.org. noc.dns.icann.org. 2022091184 7200 3600 1209600 3600

SUMMARY

Fixes issue #5817

ISSUE TYPE

Bugfix Pull Request

COMPONENT NAME

nsupdate

ansibullbot · 2023-01-11T22:15:25Z

cc @nerzhul
click here for bot help

felixfontein

Thanks for your contribution! Could you please add a changelog fragment? Thanks.

n0p90 · 2023-01-13T23:04:43Z

Thanks for your contribution! Could you please add a changelog fragment? Thanks.

Done + rebased on current main. Thanks for the review.

changelogs/fragments/5818-nsupdate-fix-zone-lookup.yml

felixfontein · 2023-01-14T17:00:18Z

plugins/modules/nsupdate.py

@@ -270,7 +270,7 @@ def lookup_zone(self):
                self.module.fail_json(msg='Zone lookup failure: \'%s\' will not respond to queries regarding \'%s\'.' % (
                    self.module.params['server'], self.module.params['record']))
            try:
-                zone = lookup.authority[0].name
+                zone = lookup.answer[0].name


One problem with checking answer is that this could also be a CNAME.

I tested this with some DNS servers and found several behaviors:

response for SOA on a known CNAME record is empty

response for SOA on a known CNAME record contains the CNAME record

response for SOA on a known CNAME record contains the CNAME record as the first answer, and SOA of the DNS name the CNAME points to as the second answer

(Interestingly I got 1 and 2 for the same DNS name with some time inbetween. Whatever...)

Simply using lookup.answer[0].name here seems wrong.

Also, another point: if lookup.authority[0] is present, that one should be used directly IMO, to avoid having to crawl up the DNS hierarchy. So how about:

First try lookup.authority[0].name

If that results in IndexError, try lookup.answer[0]

If that is a SOA entry, use lookup.answer[0].name

(And keep the if name == zone check.)

WDYT?

One problem with checking answer is that this could also be a CNAME.

I tested this with some DNS servers and found several behaviors:

1. response for SOA on a known CNAME record is empty 2. response for SOA on a known CNAME record contains the CNAME record 3. response for SOA on a known CNAME record contains the CNAME record as the first answer, and SOA of the DNS name the CNAME points to as the second answer

(Interestingly I got 1 and 2 for the same DNS name with some time inbetween. Whatever...)

I'm not sure I understand what you tested above. Did you do a query of type SOA or did you do a query of type A? Because I don't think you should get an RR of type CNAME in the answer section if you did an SOA query.

EDIT: I've now also observed the same behavior with CNAME, I'll add a test of the RR type from the Answer section instead of assuming they are of type SOA.

Simply using lookup.answer[0].name here seems wrong.

The code does queries of type SOA, and iterate through each subdomain. So when it ends up doing a query for a zone which exists (which must have a SOA record), then it will find the zone in which to insert the record.

Also, another point: if lookup.authority[0] is present, that one should be used directly IMO, to avoid having to crawl up the DNS hierarchy. So how about:

1. First try `lookup.authority[0].name` 2. If that results in `IndexError`, try `lookup.answer[0]` 3. If that is a SOA entry, use `lookup.answer[0].name`

(And keep the if name == zone check.)

WDYT?

I've updated the code to shortcut the subdomain traversal in case the SOA record name in the Authority section is a subdomain of the record we want to insert. I've tried to keep the code consistent with dnspython dns.resolver.zone_for_name() code.

I've not tested this code as I don't currently have access to work infrastructure, but I can test it next week if needed.

I'm not sure I understand what you tested above. Did you do a query of type SOA or did you do a query of type A? Because I don't think you should get an RR of type CNAME in the answer section if you did an SOA query.

Please ignore the above comment. I just got now an answer with a CNAME in the Answer section when doing a query of type SOA on an existing CNAME. I had previously done the same query several times but only got the Authority section in the response.

So I'll indeed have to add a check of the Answer RR type before returning its name. Thanks a lot for catching this!

I'll update this PR with this additional check.

Please ignore the above comment. I just got now an answer with a CNAME in the Answer section when doing a query of type SOA on an existing CNAME. I had previously done the same query several times but only got the Authority section in the response.

For me it was the other way around ;) I was pretty surprised as I also expected that this won't happen... I really wonder why it sometimes goes this way and sometimes the other. After trying it again now for a few different CNAMEs I know, the behavior seems to be (for me) that I get the CNAME answer on the first try, and no CNAME for the next tries for the same DNS name. But... 🤷

I can't get response as 'authority', only as 'answer'... Besides, even if we query for a CNAME record, first item in response array is CNAME and the second response array item is actually SOA...

>>> name = 'www.ya.ru' >>> query = dns.message.make_query(name, dns.rdatatype.SOA); >>> q = dns.query.udp(query, '93.158.134.1'); >>> for rr in q.answer: ... print("RR.NAME %s returned as RR.RDTYPE %s" % (rr.name, rr.rdtype)); ... RR.NAME www.ya.ru. returned as RR.RDTYPE 5 RR.NAME ya.ru. returned as RR.RDTYPE 6 >>> print(int(dns.rdatatype.CNAME)) 5 >>> print(int(dns.rdatatype.SOA)) 6 >>> print(q.answer[0]) www.ya.ru. 600 IN CNAME ya.ru. >>> print(q.answer[1]) ya.ru. 3600 IN SOA ns1.yandex.ru. sysadmin.yandex.ru. 2023011100 900 600 2592000 900

so we can/could just walk answer section looking for dns.rdatatype.SOA == 6 response and return it ? btw, in case of CNAME, current commit code check for rr.name == name would fail, since 'ya.ru != www.ya.ru'

I can't get response as 'authority', only as 'answer'...

It apparently (also) depends on the resolver used, for example if you do dig -t SOA www.google.com @1.1.1.1 or dig -t SOA www.google.com @8.8.8.8 you should see the SOA in 'authority'. When not adding @ I don't see it.

so we can/could just walk answer section looking for dns.rdatatype.SOA == 6 response?

The current version of the PR does that.

The current version of the PR does that.

(As mention on IRC) You are right, it does not do that, but the current version is correct IMO, since otherwise we would end up potentially returning the zone of the CNAME target, and not of the CNAME itself. In your example these are identical, but for example if we query cname.example.com which happens to be a CNAME to foo.example.org, and the CNAME answer contains the SOA entry for example.org, we do not want to use that one, but instead continue querying example.com (as the parent of cname.example.com).

The SOA record for an existing zone is returned as an answer RR and not as an authority RR. It can be returned as an authority RR for subdomains of a zone. $ dig -t SOA example.com ;; ANSWER SECTION: example.com. 3530 IN SOA ns.icann.org. noc.dns.icann.org. 2022091184 7200 3600 1209600 3600 $ dig -t SOA www.example.com ;; AUTHORITY SECTION: example.com. 3600 IN SOA ns.icann.org. noc.dns.icann.org. 2022091184 7200 3600 1209600 3600

n0p90 · 2023-01-14T23:53:28Z

The new version of this PR has not been tested yet. I'll confirm if it is working next week once I'll have access to work infra.

felixfontein · 2023-01-15T13:18:22Z

The new version of this PR has not been tested yet. I'll confirm if it is working next week once I'll have access to work infra.

The code looks good to me, but I don't want to merge it before you had a chance to test it :)

n0p90 · 2023-01-15T23:17:38Z

The code looks good to me, but I don't want to merge it before you had a chance to test it :)

I've now tested this patch and can confirm it works as expected. The 1st version was indeed failing when updating an existing CNAME record, because the name of the CNAME was used as a zone name, but no such zone existed. Current version works correctly.

patchback · 2023-01-17T20:03:42Z

Backport to stable-5: 💚 backport PR created

✅ Backport PR branch: patchback/backports/stable-5/5ad703ac64026f88a401880b52fb25dab49237bc/pr-5818

Backported as #5852

🤖 @patchback
I'm built with octomachinery and
my source is open — https://github.com/sanitizers/patchback-github-app.

The SOA record for an existing zone is returned as an answer RR and not as an authority RR. It can be returned as an authority RR for subdomains of a zone. $ dig -t SOA example.com ;; ANSWER SECTION: example.com. 3530 IN SOA ns.icann.org. noc.dns.icann.org. 2022091184 7200 3600 1209600 3600 $ dig -t SOA www.example.com ;; AUTHORITY SECTION: example.com. 3600 IN SOA ns.icann.org. noc.dns.icann.org. 2022091184 7200 3600 1209600 3600 (cherry picked from commit 5ad703a)

patchback · 2023-01-17T20:03:51Z

Backport to stable-6: 💚 backport PR created

✅ Backport PR branch: patchback/backports/stable-6/5ad703ac64026f88a401880b52fb25dab49237bc/pr-5818

Backported as #5853

🤖 @patchback
I'm built with octomachinery and
my source is open — https://github.com/sanitizers/patchback-github-app.

The SOA record for an existing zone is returned as an answer RR and not as an authority RR. It can be returned as an authority RR for subdomains of a zone. $ dig -t SOA example.com ;; ANSWER SECTION: example.com. 3530 IN SOA ns.icann.org. noc.dns.icann.org. 2022091184 7200 3600 1209600 3600 $ dig -t SOA www.example.com ;; AUTHORITY SECTION: example.com. 3600 IN SOA ns.icann.org. noc.dns.icann.org. 2022091184 7200 3600 1209600 3600 (cherry picked from commit 5ad703a)

felixfontein · 2023-01-17T20:04:40Z

@n0p90 thanks for your contribution!
@mator thanks for your tests/review!

nsupdate: fix zone lookup (#5818) The SOA record for an existing zone is returned as an answer RR and not as an authority RR. It can be returned as an authority RR for subdomains of a zone. $ dig -t SOA example.com ;; ANSWER SECTION: example.com. 3530 IN SOA ns.icann.org. noc.dns.icann.org. 2022091184 7200 3600 1209600 3600 $ dig -t SOA www.example.com ;; AUTHORITY SECTION: example.com. 3600 IN SOA ns.icann.org. noc.dns.icann.org. 2022091184 7200 3600 1209600 3600 (cherry picked from commit 5ad703a) Co-authored-by: n0p90 <[email protected]>

n0p90 · 2023-01-19T01:15:52Z

@n0p90 thanks for your contribution!
@mator thanks for your tests/review!

Thanks a lot for the review and the help with debugging + finding a proper fix.

ansibullbot added bug This issue/PR relates to a bug module module net_tools new_contributor Help guide this first time contributor plugins plugin (any type) small_patch Hopefully easy to review labels Jan 11, 2023

felixfontein added check-before-release PR will be looked at again shortly before release and merged if possible. backport-5 labels Jan 12, 2023

felixfontein reviewed Jan 12, 2023

View reviewed changes

ansibullbot removed the small_patch Hopefully easy to review label Jan 13, 2023

felixfontein reviewed Jan 14, 2023

View reviewed changes

felixfontein approved these changes Jan 17, 2023

View reviewed changes

felixfontein removed the check-before-release PR will be looked at again shortly before release and merged if possible. label Jan 17, 2023

felixfontein merged commit 5ad703a into ansible-collections:main Jan 17, 2023

patchback bot mentioned this pull request Jan 17, 2023

[PR #5818/5ad703ac backport][stable-5] nsupdate: fix zone lookup #5852

Merged

patchback bot mentioned this pull request Jan 17, 2023

[PR #5818/5ad703ac backport][stable-6] nsupdate: fix zone lookup #5853

Merged

n0p90 deleted the fix-nsupdate-authoritative branch January 19, 2023 01:10

n0p90 mentioned this pull request Jan 19, 2023

nsupdate: zone lookup failure #5817

Closed

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

nsupdate: fix zone lookup #5818

nsupdate: fix zone lookup #5818

n0p90 commented Jan 11, 2023

ansibullbot commented Jan 11, 2023

felixfontein left a comment

n0p90 commented Jan 13, 2023

felixfontein Jan 14, 2023

n0p90 Jan 14, 2023 •

edited

Loading

n0p90 Jan 14, 2023

felixfontein Jan 15, 2023

mator Jan 15, 2023 •

edited

Loading

felixfontein Jan 15, 2023 •

edited

Loading

felixfontein Jan 15, 2023

n0p90 commented Jan 14, 2023

felixfontein commented Jan 15, 2023

n0p90 commented Jan 15, 2023

patchback bot commented Jan 17, 2023 •

edited

Loading

patchback bot commented Jan 17, 2023 •

edited

Loading

felixfontein commented Jan 17, 2023

n0p90 commented Jan 19, 2023

nsupdate: fix zone lookup #5818

nsupdate: fix zone lookup #5818

Conversation

n0p90 commented Jan 11, 2023

SUMMARY

ISSUE TYPE

COMPONENT NAME

ansibullbot commented Jan 11, 2023

felixfontein left a comment

Choose a reason for hiding this comment

n0p90 commented Jan 13, 2023

felixfontein Jan 14, 2023

Choose a reason for hiding this comment

n0p90 Jan 14, 2023 • edited Loading

Choose a reason for hiding this comment

n0p90 Jan 14, 2023

Choose a reason for hiding this comment

felixfontein Jan 15, 2023

Choose a reason for hiding this comment

mator Jan 15, 2023 • edited Loading

Choose a reason for hiding this comment

felixfontein Jan 15, 2023 • edited Loading

Choose a reason for hiding this comment

felixfontein Jan 15, 2023

Choose a reason for hiding this comment

n0p90 commented Jan 14, 2023

felixfontein commented Jan 15, 2023

n0p90 commented Jan 15, 2023

patchback bot commented Jan 17, 2023 • edited Loading

Backport to stable-5: 💚 backport PR created

patchback bot commented Jan 17, 2023 • edited Loading

Backport to stable-6: 💚 backport PR created

felixfontein commented Jan 17, 2023

n0p90 commented Jan 19, 2023

n0p90 Jan 14, 2023 •

edited

Loading

mator Jan 15, 2023 •

edited

Loading

felixfontein Jan 15, 2023 •

edited

Loading

patchback bot commented Jan 17, 2023 •

edited

Loading

patchback bot commented Jan 17, 2023 •

edited

Loading