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: Detect zone cuts by asking for DS instead of NS #5716

Merged
merged 5 commits into from Sep 28, 2017

Conversation

Projects
None yet
3 participants
@rgacogne
Member

rgacogne commented Sep 20, 2017

Short description

Since there is so many broken DNS servers handing NS queries incorrectly, move to DS queries for figuring out zone cuts since we only care as long as the zone is Secure (including with a TA).
Please review this carefully before merging.
Fixes #5681.

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)

rgacogne added some commits Sep 12, 2017

@rgacogne rgacogne added the rec label Sep 20, 2017

@rgacogne rgacogne added this to the rec-4.1.0 milestone Sep 20, 2017

@rgacogne rgacogne requested a review from pieterlexis Sep 20, 2017

rgacogne added some commits Sep 21, 2017

@pieterlexis

2 questions, good otherwise (as @Habbie tested this)

@@ -1387,8 +1392,21 @@ vState SyncRes::getDSRecords(const DNSName& zone, dsmap_t& ds, bool taOnly, unsi
}
}
if (ds.empty()) {
if (rcode == RCode::NoError && ds.empty()) {
if (foundCut) {

This comment has been minimized.

@pieterlexis

pieterlexis Sep 28, 2017

Member

(just spit balling here) is getDSRecords() ever called in a way there we won't need to know the denialstate? I mean that if we have 'false' DS records we should check this regardless of the fact that we want to know if we are at a zonecut?

@pieterlexis

pieterlexis Sep 28, 2017

Member

(just spit balling here) is getDSRecords() ever called in a way there we won't need to know the denialstate? I mean that if we have 'false' DS records we should check this regardless of the fact that we want to know if we are at a zonecut?

owner name regardless of type.
*/
if (nsec3->d_set.count(QType::NS) && !nsec3->d_set.count(QType::SOA) &&
getSigner(v.second.signatures).countLabels() < v.first.first.countLabels()) {

This comment has been minimized.

@pieterlexis

pieterlexis Sep 28, 2017

Member

should we check for v.first.first.isPartOf(getSigner(v.second.signatures)) here?

@pieterlexis

pieterlexis Sep 28, 2017

Member

should we check for v.first.first.isPartOf(getSigner(v.second.signatures)) here?

@Habbie Habbie merged commit 91d779c into PowerDNS:master Sep 28, 2017

1 check passed

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

@rgacogne rgacogne deleted the rgacogne:rec-cut-ds branch Sep 29, 2017

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