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: Check that the NSEC covers an ENT when looking for NODATA #5808

Merged
merged 2 commits into from Oct 16, 2017

Conversation

Projects
None yet
2 participants
@rgacogne
Member

rgacogne commented Oct 11, 2017

Short description

Otherwise we might consider that a NSEC record covers a name when it does not.

Checklist

I have:

  • read the CONTRIBUTING.md document
  • compiled and tested this code
  • included documentation (including possible behaviour changes)
  • documented the code
  • added or modified regression test(s)
  • added or modified unit test(s)
rec: Check that the NSEC covers an ENT when looking for NODATA
Otherwise we might consider that a NSEC record covers a name when it
does not.

@rgacogne rgacogne added this to the rec-4.1.0 milestone Oct 11, 2017

@rgacogne rgacogne requested a review from pieterlexis Oct 11, 2017

rec: The NSEC next name should be different to prove an ENT
While it's not an issue in the current code because we checked
earlier that the NSEC covered the name, it might prevent an issue
if we reuse nsecProvesENT() later.
@pieterlexis

From a skim it looks good. We should add some regression tests alongside the unit tests (can be in a later PR)

// FIXME: Add ENT support
// FIXME: Make usable for non-DS records and hook up to validateRecords (or another place)
dState getDenial(const cspmap_t &validrrsets, const DNSName& qname, const uint16_t qtype, bool referralToUnsigned)
dState getDenial(const cspmap_t &validrrsets, const DNSName& qname, const uint16_t qtype, bool referralToUnsigned, bool wantsNoDataProof)

This comment has been minimized.

@pieterlexis

pieterlexis Oct 16, 2017

Member

I am not a fan of telling getDenial what we are looking for. I think this function should always return the correct dState. Although this might be a project for later.

@pieterlexis

pieterlexis Oct 16, 2017

Member

I am not a fan of telling getDenial what we are looking for. I think this function should always return the correct dState. Although this might be a project for later.

This comment has been minimized.

@rgacogne

rgacogne Oct 16, 2017

Member

I completely agree, but we will need to refactor our handling of DNSSEC denials for that.

@rgacogne

rgacogne Oct 16, 2017

Member

I completely agree, but we will need to refactor our handling of DNSSEC denials for that.

@rgacogne rgacogne merged commit 2874160 into PowerDNS:master Oct 16, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@rgacogne rgacogne deleted the rgacogne:rec-nsec-ent branch Oct 16, 2017

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