Refactor DNS check #96
Conversation
| if len(in.Ns) > 0 { | ||
| for _, r := range in.Ns { | ||
| rr, ok := r.(*dns.NS) | ||
| if ok && recursionsCnt < preCheckDNSMaxRecursions { |
There was a problem hiding this comment.
Guess we should query all NS at some point, right? Instead of artificially limit the number to five.
|
@xenolf Right now we only check the first NS in the record. The MaxRecursion actually limits the recursion depth and protects us from an infinite loop due to misconfigured DNS zones. |
|
We also better add a test case before merging this 😄 |
|
Are you going to add that to your PR or should I add it later? :) Thanks for you hard work on this issue! 👏 |
|
I will update this with the check for all authoritative NS. If you could add a test when thats done? 😏 |
|
😆 Yeah I will. Them bloody tests 😜 |
|
@xenolf So the changes for querying all authoritative nameservers are in. This is a bit of a rewrite, so please review. Tested with CloudFlare DNS. |
|
Added some unit tests FYI. |
| // waitFor polls the given function 'f', once per second, up to 'timeout' seconds. | ||
| func waitFor(timeout int, f func() (bool, error)) error { | ||
| // waitFor polls the given function 'f', once every 'interval' seconds, up to 'timeout' seconds. | ||
| func waitFor(timeout, interval int, f func() (bool, error)) error { |
There was a problem hiding this comment.
I'm just thinking out loud here... but maybe we should add a util.go for stuff like this?
There was a problem hiding this comment.
That was on my mind too! 😄 But maybe we can do the code re-organization later in a separate patch?
There was a problem hiding this comment.
Yeah, I agree... out of scope of this PR :)
There was a problem hiding this comment.
Do you have any other stuff on your mind that ought to move to the util.go?
There was a problem hiding this comment.
Yes, limitReader for example. I'm also thinking about crypto.go as there are multiple functions in there which are mere utility functions.
|
@willglynn If you will, please test your scenario against the updated PR. Thanks! |
* Implements a (semi-)recursive query mechanism to find all authoritative nameservers for the zone enclosing the domain. This is necessary because Google Public DNS does sometimes not return any NS at all or those of a different zone (e.g. when the domain is a CNAME pointing elsewhere). * All authoritative nameservers are then queried for the TXT record to make sure that it has been completely propagated. * The waitFor utility function now takes an interval parameter and returns the last seen error on timeout.
|
I should be able to check in on this tomorrow. Who knows; I might even be able to contribute tests 😄 |
|
Awesome 👌 😋 |
|
I don't have immediate feedback, but I have been collecting scenarios for testing. (CNAMEs make for all sorts of fun.) |
|
The iterative DNS lookup used in this PR proved overkill, since the objective (getting a definite list of authoritative NS for a domain) can actually very well be accomplished by plain old and cheap recursive queries to a public DNS resolver. 😬 😅 @willglynn The API has not changed so any tests you might have prepared can be applied to the new PR. |
Fixes #95
recursiveiterative lookup mechanism to find all authoritativenameservers for the zone enclosing the domain.
This is necessary because a direct query for NS records at a recursive DNS server (e.g. Google Public DNS) or at the zone's primary nameserver will often yield an incomplete list of authoritative nameservers or even those of a another zone (e.g.
when the domain is a CNAME pointing to another zone).
sure that it has been completely propagated.
the last seen error on timeout.