-
Notifications
You must be signed in to change notification settings - Fork 432
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
DnssecDnsHandle
: check RRSIG validity as per RFC4035
#2213
base: main
Are you sure you want to change the base?
Conversation
for better type safety. its fields are not meant to be mutated after fully constructed as `dns_class` comes from the (response) records themselves, `verify_rrsets` no longer needs a `Query` argument
including inception and expiration times closes hickory-dns#2209
rebased and ready for review |
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.
LGTM, but I'm a little wary of adding this without much in the way of test coverage. Is there something minimal we can do now? If not, what's the plan to cover this stuff soon?
// TODO: combine this with crate::rr::RecordSet? | ||
#[derive(Debug)] | ||
pub(super) struct Rrset<'r> { | ||
name: Name, |
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.
If only someone implemented the restrictions RFC!
we plan to add tests that use "incorrect" dnssec records to test code paths like these but it's a bit involved because in some cases we need to re-generate rrsig records if we modify the record that the rrsig covers. the dns-test supports mutation of unsigned zone files but still does not update signatures and other dnssec records. in this particular case of inception / expiration time we might be able to just set the clock of the container back / forward in time to generate dnssec records with the desired timestamps without having to mutate and resign zone files. I can have a look into that |
this PR adds the validity checks specified in section 5.3.1 of RFC4035, including the time check that was reported as missing in issue #2209
these new checks overlap with some of the existing ones but I opted for not removing the existing ones because I do not know how well the existing code is covered by tests.
I checked that I did not break this query:
It would be good to check that expired signatures are rejected. There are plans to add those kind of tests to the conformance test suite (ferrous-systems/dnssec-tests#61) but given that
Recursor
does not useDnssecDnsHandle
those tests won't exercise the code paths added here. I'm looking into reusing some of the existing code inDnssecDnsHandle
to implement DNSSEC validation inRecursor
(then conformance tests will cover some ofDnssecDnsHandle
code) but that's still ways off time-wise.Another way to check the code here end to end would be to modify
util/src/bin/resolve.rs
to do DNSSEC validation (e.g. via a new CLI flag) then it would behave just likedelv
. That updatedresolve
could be comparatively againstdelv
using thedns-test
framework, that is without relying on external services / the internet.I'm leaving this in draft state because the implementation will be cleaner when #2211 is implemented.