Opened 8 years ago

Closed 8 years ago

#1573 closed task (fixed)

auth::Query needs to return DS for secure delegation

Reported by: jinmei Owned by: jelte
Priority: high Milestone: Sprint-20120207
Component: b10-auth Version:
Keywords: Cc:
CVSS Scoring: Parent Tickets:
Sensitive: no Defect Severity: N/A
Sub-Project: DNS Feature Depending on Ticket:
Estimated Difficulty: 5 Add Hours to Ticket: 0
Total Hours: 7.5 Internal?: no

Description

We need to update the ZoneFinder::DELEGATION case of
auth::Query::process() so that if DNSSEC is requested it will find the
DS of the delegated zone and (if found) include it in the response.

Subtickets

Change History (16)

comment:1 Changed 8 years ago by jelte

  • Estimated Difficulty changed from 0 to 5

comment:2 Changed 8 years ago by jelte

  • Milestone changed from Next-Sprint-Proposed to Sprint-20120207
  • Priority changed from major to critical

comment:3 Changed 8 years ago by jelte

  • Owner set to jelte
  • Status changed from new to assigned

comment:4 Changed 8 years ago by jelte

  • Owner changed from jelte to UnAssigned
  • Status changed from assigned to reviewing
  • Total Hours changed from 0 to 4

Ready for review. Changes itself are quite simple, test was a bit harder (but mostly because of trying to figure out how they worked).

comment:5 Changed 8 years ago by jinmei

  • Owner changed from UnAssigned to jinmei

comment:6 follow-up: Changed 8 years ago by jinmei

First, I made a couple of trivial editorial fixes.

query.cc

  • I'd suggest rename parameter name for ZoneFinder from "zone" to "finder", partly for consistency, and partly because it's more appropriate in terms of what it is.
  • If DS isn't found (and the zone is signed with NSEC) we need to add NSEC (maybe it was not clear from the ticket description, if so, sorry about that). I cannot find normative text about this in RFC4035 (it only provides an example in an appendix), but that's how BIND 9 works, and apparently so do all root servers. Note also that the header file documentation would also have to be updated.
  • We should also probably handle a pathological case where find(DS) results in neither SUCCESS or NXRRSET (return SERVFAIL?)

unittest

  • This comment doesn't seem to be very correct. At least the answer section is empty, and I don't think anything is omitted in this case:
        // find match rrset, omit additional data which has already been provided
        // in the answer section from the additional.
    
  • We also need to test the case when DS isn't found (insecure delegation)
  • We also need to test the pathological case where find(DS) results in neither SUCCESS or NXRRSET.
  • As for the check for the additional section, we could at least reduce the amount of hardcoding a bit:
                      (string("glue.delegation.example.com. 3600 IN RRSIG ") +
                       getCommonRRSIGText("A") + "\n" +
                       string(ns_addrs_txt) +
                       string("glue.delegation.example.com. 3600 IN RRSIG ") +
                       getCommonRRSIGText("AAAA") + "\n" +
                       string("noglue.example.com. 3600 IN RRSIG ") +
                       getCommonRRSIGText("A")).c_str());
    
    (there's one tricky point, though: we need to place ns_addrs_txt between the glue A RRSIG and AAAA RRSIG; otherwise masterLoad() would be confused. However, I believe #1614 will solve this issue too)
  • Not really related to this branch, but I noticed one oddity in the additional section: glue RRs should normally not have RRSIGs. But it's probably okay for the Query class to include them if it happens to be passed such odd RRSIGs.

other

maybe we need a changelog for this?

comment:7 Changed 8 years ago by jinmei

  • Owner changed from jinmei to jelte

comment:8 in reply to: ↑ 6 ; follow-up: Changed 8 years ago by jelte

  • Owner changed from jelte to jinmei

Replying to jinmei:

First, I made a couple of trivial editorial fixes.

query.cc

  • I'd suggest rename parameter name for ZoneFinder from "zone" to "finder", partly for consistency, and partly because it's more appropriate in terms of what it is.

ok

  • If DS isn't found (and the zone is signed with NSEC) we need to add NSEC (maybe it was not clear from the ticket description, if so, sorry about that). I cannot find normative text about this in RFC4035 (it only provides an example in an appendix), but that's how BIND 9 works, and apparently so do all root servers. Note also that the header file documentation would also have to be updated.

doh, of course. Actually it is mentioned in section 3.1.4

abstracted the handling of NXRRSET response for this, to avoid duplicate code.

  • We should also probably handle a pathological case where find(DS) results in neither SUCCESS or NXRRSET (return SERVFAIL?)

check

unittest

Snipped your comments here, I radically changed the additions; instead of overloading the existing delegation.example.com. I decided it would be cleaner to add a few more delegations specifically for these tests; signed-delegation, unsigned-delegation, and bad-signed-delegation (which errors when looking for the DS record), with out-of-zone NS targets, so the response does not get cluttered with other info.

  • Not really related to this branch, but I noticed one oddity in the additional section: glue RRs should normally not have RRSIGs. But it's probably okay for the Query class to include them if it happens to be passed such odd RRSIGs.

probably better than to trip over it.

other

maybe we need a changelog for this?

Ack.

[func] jelte
The in-memory datasource now correctly includes DS records (or the denial of its existence if NSEC is used) when returning a delegation from a signed zone.
(Trac 1573, git ###)

comment:9 in reply to: ↑ 8 ; follow-up: Changed 8 years ago by jinmei

Replying to jelte:

I made some other editorial fixes.

The revised code now looks mostly okay. I have a few minor points:

query.cc

  • If we name addNXRRsetDenial with the postfix of "Denial", maybe we should be consistent for addNXDOMAINProof, addWildcardProof, and addWildcardNXRRSETProof?

query_unittest.cc

  • I'd add one-line comment before this to explain what will follow.
    const char* const signed_delegation_txt =
        "signed-delegation.example.com. 3600 IN NS ns.example.net.\n";
    

maybe we need a changelog for this?

Ack.

[func] jelte
The in-memory datasource now correctly includes DS records (or the denial of its existence if NSEC is used) when returning a delegation from a signed zone.
(Trac 1573, git ###)

The auth::Query class is not intended to be in-memory only, so I'd say
something like

The new query handling module of b10-auth (currently only used with the
in-memory data source) now correctly...

Also, we might say this is a "bug" (fix), but I'd leave it to you.

comment:10 Changed 8 years ago by jinmei

  • Owner changed from jinmei to jelte

comment:11 in reply to: ↑ 9 ; follow-up: Changed 8 years ago by jelte

  • Owner changed from jelte to jinmei

Replying to jinmei:

query.cc

  • If we name addNXRRsetDenial with the postfix of "Denial", maybe we should be consistent for addNXDOMAINProof, addWildcardProof, and addWildcardNXRRSETProof?

I've renamed addNXRRset to Proof to be consistent with the existent ones.

query_unittest.cc

  • I'd add one-line comment before this to explain what will follow.
    const char* const signed_delegation_txt =
        "signed-delegation.example.com. 3600 IN NS ns.example.net.\n";
    

Added one for each type of delegation I added.

maybe we need a changelog for this?

Ack.

[func] jelte
The in-memory datasource now correctly includes DS records (or the denial of its existence if NSEC is used) when returning a delegation from a signed zone.
(Trac 1573, git ###)

The auth::Query class is not intended to be in-memory only, so I'd say
something like

The new query handling module of b10-auth (currently only used with the
in-memory data source) now correctly...

Ah doh, of course.

Also, we might say this is a "bug" (fix), but I'd leave it to you.

Ok.

[bug] jelte
The new query handling module of b10-auth (currently only used with the
in-memory data source) now correctly includes the DS record (or the denial of its existence if NSEC is used).

comment:12 in reply to: ↑ 11 Changed 8 years ago by jinmei

Replying to jelte:

Also, we might say this is a "bug" (fix), but I'd leave it to you.

Ok.

[bug] jelte
The new query handling module of b10-auth (currently only used with the
in-memory data source) now correctly includes the DS record (or the denial of its existence if NSEC is used).

"...when returning a delegation from a signed zone." (or something
like that) would be still necessary to clarify it's not about DS query
(not DS attached with delegation).

Other parts look okay. Please feel free to merge.

comment:13 Changed 8 years ago by jinmei

  • Owner changed from jinmei to jelte

comment:14 Changed 8 years ago by jinmei

  • Total Hours changed from 4 to 6.5

comment:15 Changed 8 years ago by jelte

  • Total Hours changed from 6.5 to 7.5

Thanks, merged, closing ticket.

comment:16 Changed 8 years ago by jelte

  • Resolution set to fixed
  • Status changed from reviewing to closed
Note: See TracTickets for help on using tickets.