-
Notifications
You must be signed in to change notification settings - Fork 444
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
recursor: respect DO bit in incoming queries #2196
recursor: respect DO bit in incoming queries #2196
Conversation
hm, this is definitely possible. There's a lot of special logic in place for DNSSEC on the authority, it's possible we're doing something incorrect there. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we add some basic tests here?
I've updated the PR. now section 3.2.1 of RFC4035 is fully covered. all the tests in ferrous-systems/dnssec-tests#53 associated to that RFC section pass
$ delv -p 1053 @127.0.0.1 +rtrace www.example.com.
;; fetch: www.example.com/A
;; fetch: com/DS
;; no valid RRSIG resolving 'com/DS/IN': 127.0.0.1#1053
;; broken trust chain resolving 'www.example.com/A/IN': 127.0.0.1#1053
;; resolution failed: broken trust chain
$ delv -p 1053 @127.0.0.1 +rtrace www.example.com.
;; fetch: www.example.com/A
;; fetch: example.com/DNSKEY
;; fetch: example.com/DS
;; fetch: com/DS
;; no valid RRSIG resolving 'com/DS/IN': 127.0.0.1#1053
;; broken trust chain resolving 'example.com/DS/IN': 127.0.0.1#1053
;; broken trust chain resolving 'example.com/DNSKEY/IN': 127.0.0.1#1053
;; broken trust chain resolving 'www.example.com/A/IN': 127.0.0.1#1053
;; resolution failed: broken trust chain (this fails because the query
$ delv @1.1.1.1 +rtrace www.example.com.
;; fetch: www.example.com/A
;; fetch: example.com/DNSKEY
;; fetch: example.com/DS
;; fetch: com/DNSKEY
;; fetch: com/DS
;; fetch: ./DNSKEY
; fully validated |
What is |
|
it would be possible to do some unit testing around the behavior of the there are comprehensive end-to-end tests for these RFC requirements in the conformance test suite ( ferrous-systems/dnssec-tests#53 and ferrous-systems/dnssec-tests#53 ). it would be less work to just rely on those I fixed the CI failure (clippy warning) and can squash the commits if desired |
Would be nice to squash the clippy commit into the originating commit. Otherwise it looks like these commits are at least somewhat logically independent? |
when the recursor is "security-aware" -- that is the "dnssec" feature is enabled -- as per RFC 4035 section 3.2.1
84c58f6
to
48a2531
Compare
and make security-awareness opt-in
last two commits implement the build-pattern configuration API for I used the could not read config /etc/named.toml: Error { kind: TomlDecode(Error { kind: Custom, line: Some(0), col: 0, at: Some(0), message: "unknown field `security-aware`, expected one of `roots`, `ns_cache_size`, `record_cache_size`, `security_aware`", key: ["zones", "stores"] }) } which looks quite useful to me, although it could be slightly better formatted instead of showing the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At the moment this implements the first half of RFC 4035, section 3.2.1: the DO bit is set in the queries the recursive resolver sends on behalf of the client. On the wire, I can see that the name servers it communicates with include extra DNSSEC records in their response to the resolver. However, the
Recursor
removes those extra DNSSEC records before answering the client. That stripping of DNSSEC records does the opposite of the second half of what the RFC dictates (when the DO bit is set).AFAICT, the stripping of those DNSSEC records happens here. That function caches all the received records but only responds with the ones that exactly match the query and as DNSSEC records like RRSIG were not included in the original query it discards them.
I still have to figure out how to best avoid stripping so I'm leaving this as a draft PR for now.
P.S. I'm checking conformance with the tests in ferrous-systems/dnssec-tests#53
cc #2193