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

rec: NS from the cache could be a forwarder #10643

Merged

Conversation

omoerbeek
Copy link
Member

Take that into acount when determining dont-query status. Should fix #10638.
Only if it;s coming from the auth domain map pierceDontQuery wil lbe set.

A workarounf is to add !<forwarder-ip> to dont-query

Short description

Checklist

I have:

  • read the CONTRIBUTING.md document
  • compiled this code
  • tested this code
  • included documentation (including possible behaviour changes)
  • documented the code
  • added or modified regression test(s)
  • added or modified unit test(s)

…ies to.

Take that into acount when determining dont-query status. Should fix PowerDNS#10638.
@omoerbeek omoerbeek added this to the rec-4.6.0 milestone Aug 11, 2021
pdns/syncres.cc Outdated
s_dontqueries++;
return true;
} else {
LOG(prefix<<qname<<": sending query to " << remoteIP.toString() << ", blocked by 'dont-query' but a forwarding/auth case" << endl);
Copy link
Member

Choose a reason for hiding this comment

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

It looks like it would also prevent dont-query from applying to a more specific NS. For example, if we have a non-recursive forwarder for 'local.', learn a more specific NS ns.dummy->192.0.2.1 for subzone.local. via that forwarder, do we want to query it?
I gather we don't, so perhaps it would make sense to pass tns to throttledOrBlocked() so it knows if the nameserver comes from a auth zone or a forward, instead of doing a new auth/forward lookup?

Copy link
Member Author

Choose a reason for hiding this comment

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

That sounds sensible indeed.

Copy link
Member Author

@omoerbeek omoerbeek Aug 17, 2021

Choose a reason for hiding this comment

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

Looking closer, I must be missing something The NS we are contacting comes from the cache in the case in #10638. Se we're not able to see from tns if this was a forwarder or auth case: the name is filled in. That's more or less the who point of doing the lookup again using qname (the name being queried) .

And taking your example.
We have

subzone.local NS ns.dummy
ns.dummy A 192.0.2.1

in the cache. Now assume name www.subzone.local is being queried, so the NS record from the cache is used, since it is more specific than the forward.
The name being queried is covered by the local. forward, so I think it should be allowed? After all the NS record we learned came from the forwarder and it applies to a forwarded (sub)domain.

We might need to extend the test to also check if the remoteIP (form the cache) matches one for the forward addresses listed for the name.

Copy link
Member

Choose a reason for hiding this comment

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

The name being queried is covered by the local. forward, so I think it should be allowed?

OK, that's exactly the point I'm not sure about. I'm 100% fine with not applying dont-query to servers we explicitely forward to, but I'm not sure we should also not apply it to the name servers we learn from these forwarders. If we do, I'm afraid we lose the protection provided by dont-query as soon as we forward to at least one not fully trusted server, in a transitive way. If we decide to do that I feel we should document it clearly, at the very least.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think my last suggestion: both the name should be in forward and the NS IP we're contacting should be one of the IPs being forwarded to is good. AFAKS it brings back the original dont-query override semantics that the code had before looking at the cache for NS.

Copy link
Member Author

Choose a reason for hiding this comment

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

With

;; ANSWER SECTION:
test.moerbeek.net.      60      IN      NS      ns2.test.moerbeek.net.
test.moerbeek.net.      60      IN      NS      ns1.test.moerbeek.net.

;; ADDITIONAL SECTION:
ns1.test.moerbeek.net.  60      IN      A       192.168.178.3
ns2.test.moerbeek.net.  60      IN      A       192.168.178.4

and a forwarder clause:

forward-zones=moerbeek.net=192.168.178.3

The code I'm about to commit now logs (after making sure the NS set is in the cache)

Aug 18 09:21:14 [2] a.test.moerbeek.net: We have NS in cache for 'test.moerbeek.net' (flawedNSSet=0)
Aug 18 09:21:14 [2] a.test.moerbeek.net authOrForwDomain: moerbeek.net nsFromCacheDomain: test.moerbeek.net isPartof: 0
Aug 18 09:21:14 [2] a.test.moerbeek.net: using NS from cache
Aug 18 09:21:14 [2] a.test.moerbeek.net: Cache consultations done, have 2 NS to contact
Aug 18 09:21:14 [2] a.test.moerbeek.net.: Nameservers: ns2.test.moerbeek.net(0.00ms), ns1.test.moerbeek.net(0.00ms)
Aug 18 09:21:14 [2] a.test.moerbeek.net: Trying to resolve NS 'ns2.test.moerbeek.net' (1/2)
Aug 18 09:21:14 [2] Nameserver ns2.test.moerbeek.net IPs: 192.168.178.4(0.00ms)
Aug 18 09:21:14 [2] a.test.moerbeek.net: Resolved 'test.moerbeek.net' NS ns2.test.moerbeek.net to: 192.168.178.4
Aug 18 09:21:14 [2] a.test.moerbeek.net: Trying IP 192.168.178.4:53, asking 'a.test.moerbeek.net|A'
Aug 18 09:21:14 [2] a.test.moerbeek.net: not sending query to 192.168.178.4, blocked by 'dont-query' setting
Aug 18 09:21:14 [2] a.test.moerbeek.net: Trying to resolve NS 'ns1.test.moerbeek.net' (2/2)
Aug 18 09:21:14 [2] Nameserver ns1.test.moerbeek.net IPs: 192.168.178.3(0.00ms)
Aug 18 09:21:14 [2] a.test.moerbeek.net: Resolved 'test.moerbeek.net' NS ns1.test.moerbeek.net to: 192.168.178.3
Aug 18 09:21:14 [2] a.test.moerbeek.net: Trying IP 192.168.178.3:53, asking 'a.test.moerbeek.net|A'
Aug 18 09:21:14 [2] a.test.moerbeek.net: sending query to 192.168.178.3, blocked by 'dont-query' but a forwarding/auth case
Aug 18 09:21:14 [2] a.test.moerbeek.net: Got 2 answers from ns1.test.moerbeek.net (192.168.178.3), rcode=0 (No Error), aa=1, in 1ms

So .3 is accepted, but .4 not.

Copy link
Member

@rgacogne rgacogne left a comment

Choose a reason for hiding this comment

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

That looks good, perhaps we could write a few lines about the exact behaviour in the documentation?

@omoerbeek
Copy link
Member Author

Done, there's was already a line in the docs for dont-query, I extended it.

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

Successfully merging this pull request may close these issues.

Change in behaviour for dont-query between pdns-recursor versions 4.3 and 4.4
2 participants