Fix NxDomain and NoData responses #286
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@Darkspirit, this is an initial pass at a fix for the incorrect negative response. I still need some tests for this case, but otherwise, I think this is correct. |
bluejekyll
commented
Nov 13, 2017
| // see https://tools.ietf.org/html/rfc2308 for proper response construct | ||
| match records { | ||
| AuthLookup::NoName => response.set_response_code(ResponseCode::NXDomain), | ||
| AuthLookup::NameExists => response.set_response_code(ResponseCode::NoError), |
There was a problem hiding this comment.
This is where the actual fix is.
| // records in a list except when there are a lot of records. But this makes indexed lookups by name+type | ||
| // always return empty sets. This is only important in the negative case, where other DNS authorities | ||
| // generally return NoError and no results when other types exist at the same name. bah. | ||
| if result.is_empty() { |
There was a problem hiding this comment.
This is where, IMO, the stupidity of the spec is detected, for proper response handling later...
Codecov Report
@@ Coverage Diff @@
## master #286 +/- ##
========================================
Coverage ? 87.1%
========================================
Files ? 115
Lines ? 12015
Branches ? 0
========================================
Hits ? 10466
Misses ? 1549
Partials ? 0
Continue to review full report at Codecov.
|
|
@Darkspirit, I think this is ready for review. After this, I don't think we'll see any issues with the NxDomain/NoData issues. |
Before: http://dnsviz.net/d/dev.terrax.net/Wgujmg/dnssec/ + SSLLabs: "Unable to resolve domain name" nftables.conf (nft is very new to me. I made this to redirect port 53 to 8053, so dns and web server don't need to run as root. I am unsure whether this is acceptable or something dirty.) config.toml zone file A direct request works: But my freshly restarted local unbound don't want to answer this (SERVFAIL): unbound.log
|
|
Can you remind of what unbound is doing in the picture here? Is it just forwarding on to trust-dns? The clock skew is probably not something I can fix with backdated dates to the current day. I think we already have a bug open for that. |
Unbound doesn't use any public resolver. It seems to know the root zone + TLD. Somehow like this:
Edit: I added some dashes to unbound's log to seperate things a little bit, but don't know whether they are positioned correctly or not. Edit2: I am confused, but there are multiple blocks of: and 2a01:4f8:c0c:2c12::50 is trustdns |
(and repeat) So it's asking for "_tcp.dev.terrax.net. A IN" at some point
https://github.com/k0nsl/unbound/blob/master/validator/validator.c#L982
|
|
I'm confused by the NxDomain that unbound is getting. Do you have the logs from trust-dns? |
trustdns screenlog (Is this correct? It doesn't really show the request and requesting IP.): dig unbound.log |
That is very suspicious. NSEC records shouldn't be in the zone while it's being signed. They should be produced afterward. I'm going to load the zone file you posted and see if I can reproduce that behavior. EDIT: nevermind, the NSEC records are being inserted into the zone dynamically and then the signing occurs. So this output is correct. There's also something else funny going on here. In the logs from trust-dns I don't see any requests for the TLSA record itself. I need to clean up the logging configuration, that debug output is including too much from library dependencies. |
|
Ok, I've added some better logs on requests. I should have had those there before. Now we'll actually be able to see the requests coming into the server. The logs are still very noisy on DEBUG, but these will be issue on the INFO level, that should be enough to gain better visibility into the requests being made by unbound, i.e. DEBUG shouldn't be necessary. Btw, I took your configs, and added them to one of my test zones, and I couldn't repro any issues on lookup of TLSA. I'll keep looking. |
|
I just noticed in your key configuration,: |
trustdns dig unbound unbound.conf |
|
PowerDNS (NOERROR) TRust-DNS (always the same request: bad packet, NXDOMAIN, bad packet, NXDOMAIN. Other requests (like AAAA or DNSKEY from dev.terrax.net) to TRust-DNS seem to be always fine.) |
|
Thank you for your patience in trying to resolve this. If this doesn't fix it, I'll need to replicate your setup locally to try and replicate the issues you're seeing. This is strange: The dig query you specified was So I expect an NXDomain for But I've pushed a change to change the NSEC response to NoError/NoData. Question, when you make the |
does not directly belong to
Test 1: Test 2: Thanks! I will test your new patches later.
I wouldn't query that (and have not). But let's see what PowerDNS would tell us in such a situation: |
Part A) dig via unbound works now.
trustdns
Part B) Those bad packets.
Edit: |
|
Thanks for the details. It looks like we're making good progress. bad-packet, here are some interesting things: It looks like NSEC is most likely the cause. It should only be returned with 2017-11-17T22:21:58.121866886+00:00 INFO trust_dns_server::server::server_future:215 request: 10795 src: [2003:c4:bf23:b900:7c04:5f85:3ec0:5f31]:48766 type: Query op_code: Query dnssec: true name: _tcp.dev.terrax.net. type: A class: IN
2017-11-17T22:21:58.121908835+00:00 INFO trust_dns_server::authority::catalog:280 request: 10795 found authority: dev.terrax.net.
2017-11-17T22:21:58.121984360+00:00 INFO trust_dns_server::authority::catalog:303 request: 10795 supported_algs:
2017-11-17T22:21:58.122041925+00:00 INFO trust_dns_server::authority::catalog:345 request: 10795 non-existent adding nsecs: 2
2017-11-17T22:21:58.122100628+00:00 INFO trust_dns_server::server::server_future:226 request: 10795 response_code: No Error answers: 0 name_servers: 4 additionals: 0
2017-11-17T22:21:58.122169023+00:00 INFO trust_dns_server::server::request_stream:115 request: 10795 sending message len: 461This is the initial But what's interesting is that Can you explain the difference between the $ dig A _tcp.dev.terrax.net @dev.h.terrax.net +dnssec ### FAILS
...
darkspirit@darkspirit:~$ dig A _tcp.dev.terrax.net @dev.h.terrax.net +dnssec ### SUCCEEDS
...
darkspirit@darkspirit:~$ dig A _tcp.dev.terrax.net @dev.h.terrax.net +dnssec ### SUCCEEDS
...
darkspirit@darkspirit:~$ dig A _tcp.dev.terrax.net @dev.h.terrax.net +dnssec ### FAILS
...This is strange... I'll build a new compatibility test with trust-dns and dig to validate that locally. A badly constructed RR or packet will be annoying to track down without this. |
In #286 (comment) Part B (the dig test)
I don't see anything bad. There is This unbound log belongs to #286 (comment) Part A:
I just executed the same dig command multiple times, behind one another. Some results were good, some were bad. bad:
= The same here: Some results were good, some were bad. So UDP (EDNS?) shouldn't be the cause(?). |
|
Maybe I found something. (Or it's something completely different.)
dig via unbound unbound |
Yup, this is what I expect to see. The 2 nsec records should be 1 NSEC and 1 RRSIG for it's signature. What's strange is that you get success and failure, same number of bytes.
Well, it does show that unbound is also getting something from trust-dns it's not expecting. I'm trying to reproduce. I've finally gotten this reproduced. I should be able to track this down now. |
|
Ok, the only difference I've found so far is the order of the DAU and DHU fields supported algorithms. I'm going to review the RFC, and see if there's a defined order on these fields, and also order them consistently to test... I've tracked down the issue. It's not related to the DAU/DHU fields. It looks like it's in the name servers section. Possibly a bug related to the interaction of canonical name form vs. non-canonical as it relates to label compression. |
|
Ok, I finally tracked this down, for posterity, it's the NSEC record encoding: vs. trust-dns has no problem parsing these differences correctly. But there must be something about the order effecting some of the other parsers out there. It's nice to finally have tracked this down where this is coming from. I'm working on a fix. Ok, I have a fix locally, but I need to cleanup a ton of debug code. It's too bad that other systems expect a stable order on this stuff, but not unexpected. The implementation details are here: https://tools.ietf.org/html/rfc4034#section-4.1.2 Specifically: The way that I'm calculating that is not necessarily the most efficient, by using a HashMap such that order doesn't matter. Switching to a BTreeMap fixes the issue, the critical section is here: and it's subsequently encoded here: I won't have time to clean this up right now. I'll try and get something published later today. |
|
Thanks for your patience and help in resolving this, @Darkspirit! |
|
Ok, @Darkspirit, this recent change should fix the issue we were seeing (Got delayed on getting this in). |
|
All issues are fixed. Thank you! 🎉 |
|
Great news! I’ll merge into master after double checking the code a Little’s later. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
fixes: #53