Opened 8 years ago

Closed 8 years ago

#1244 closed task (complete)

NSEC support in isc::auth::Query

Reported by: vorner Owned by: UnAssigned
Priority: medium Milestone: Sprint-20111220
Component: b10-auth Version:
Keywords: Cc:
CVSS Scoring: Parent Tickets:
Sensitive: no Defect Severity: N/A
Sub-Project: DNS Feature Depending on Ticket: Datasrc refactoring
Estimated Difficulty: 1 Add Hours to Ticket: 0
Total Hours: 0.33 Internal?: no

Description

This is continuation of #1177. The code in #1177 hopefully implements the needed support from database backends. But the query needs to take the NSEC data and put them into answer. Also, it needs to accomodate the new result statuses (WILDCARD, WILDCARD_NXRRSET) and find additional NSEC records if more than one is needed.

It should also consider the case when the backend doesn't support DNSSEC and throws from findPreviousName.

Subtickets

Change History (13)

comment:1 Changed 8 years ago by jelte

  • Milestone changed from Sprint-20110927 to Sprint-20111011

comment:2 Changed 8 years ago by jelte

  • Estimated Difficulty changed from 0 to 6

comment:3 Changed 8 years ago by jelte

  • Milestone changed from Sprint-20111011 to Sprint-20111025

comment:4 Changed 8 years ago by jinmei

  • Owner set to jinmei
  • Status changed from new to accepted

comment:5 Changed 8 years ago by jinmei

I've looked into BIND 9's NSEC implementation referring to Section
3.1.3 of RFC4035 to see how we can handle this task. I propose
the following breakdown.

  1. Prepare additional (and updating) primitives in the underlying data source.
    • We need to handle CNAME + wildcard case separately from non wildcard CNAME. This is a bit ad hoc, but I propose adding a new result status, WILDCARD_CNAME.
    • DNAME + wildcard is also tricky, but as described in rfc2672bis, this usage is now discouraged. My suggestion is to leave this case "undefined", at least for now. (But checking what happens in tests would be a good idea anyway)
    • It looks like we should treat empty (non terminal) wildcard case from other WILDCARD_NXRRSET. I propose adding a new result status, WILDCARD_EMPTY.
    • in ZoneFinder::find(), we need another option "NO_WILDCARD", which disables wildcard matching.

The rest of the subtasks are to update auth::Query per the result code.

  1. Handle normal NXRRSET case. This is the easiest one. This case corresponds to Section 3.1.3.1 and normal (non wildcard) empty non terminal case of Section 3.1.3.2 of RFC4035. Just add the returned NSEC to the authority section, and done.
  1. Handle NXDOMAIN case. This case corresponds to Section 3.1.3.2 of RFC4035. find() returns the first NSEC of 3.1.3.2. We should then perform another find() with the NO_WILDCARD option for the best possible wildcard name. See BIND 9's bin/named/query.c:query_addwildcardproof() function to identify that wildcard name. Then perform find() with NO_WILDCARD again, which should result in NXDOMAIN, and add the returned NSEC to the authority section.
  1. Handle WILDCARD_EMPTY case. This case corresponds to Section 3.1.3.2 of RFC4035 for the empty wildcard case. find() returns the *second* NSEC of 3.1.3.2. So we should fetch the first NSEC that proves there's no exact matching name for the query name. Perform find() with NO_WILDCARD, which should result in NXDOMAIN, and add the returned NSEC.
  1. Handle WILDCARD and WILDCARD_CNAME cases. This case corresponds to Section 3.1.3.3 of RFC4035. In this case we should add an NSEC that proves there's no exact matching name for the query name. The algorithm is the same as case #4.
  1. Handle WILDCARD_NXRRSET case. This case corresponds to Section 3.1.3.4 of RFC4036. find() returns the first NSEC of 3.1.3.4. So we should add an NSEC that proves there's no exact or closer non-wildcard match. It should actually be equal to the result of find() with NO_WILDCARD for the original query name.

Subtask 2 has no dependency. Subtasks 3-6 depend on task subtask 1,
but 3-6 themselves are independent from any others.

This ticket is now mostly a meta ticket, but if we parallelize the sub
tasks, there will be some duplicate code (basically, the developers
should watch each other's work to minimize the redundancy), so after
finishing all, in this ticket we should probably do some
unification/cleanup.

I'm going to create tickets for the subtasks, and release this ticket.

p.s. In the above description I may be wrong in some details. Please
do not blindly trust the descrption. Whoever takes this task should
read the protocol spec carefully, and should also refer to the
behavior of other implementations (and the implementation itself).

comment:6 Changed 8 years ago by jinmei

  • Owner changed from jinmei to UnAssigned
  • Status changed from accepted to assigned

Subtasks created: #1305, #1306, #1307, #1308, #1309, #1310.

comment:7 Changed 8 years ago by jelte

  • Estimated Difficulty changed from 6 to 1

Decided to put the estimated difficulty in the subtickets, and change this one to 1, as this is now only a metaticket. (not zero so that it won't show up in the estimations list, and perhaps it will need a bit of verification to see everything comes together)

comment:8 Changed 8 years ago by jelte

  • Milestone changed from Sprint-20111025 to Sprint-20111108

comment:9 Changed 8 years ago by jelte

  • Milestone changed from Sprint-20111108 to Sprint-20111122

comment:10 Changed 8 years ago by jinmei

Assuming #1308 is mostly ready, I think we can also close this
meta ticket.

I've looked at the resulting code by merging trac1308 to master,
and haven't found a point that require obvious refactoring (e.g. due
to code duplicate). Some of the addXXXProof variants share common
logic, but each is slightly different from others, and since they are
basically quite compact, I think it's okay to leave them as they are.

I've made a few trivial cleanups and for that purpose created a
separate branch (trac1244). One, possibly not so trivial, cleanup is
the change of the method name: from addWildcardNxrrsetProof to
addWildcardNXRRSETProof. I did it so that it would be consistent
with addNXDOMAINProof (character case policy should better consistent
between NXDOMAIN and NXRRSET, and since they are kind of acronyms I
think all-upper-case is a better choice).

I've put this ticket to the review queue just in case someone notices
something wrong, but since it's quite trivial I basically plan to
merge the cleanup changes and close this ticket (after #1308 is
closed) unless I hear something soon.

comment:11 Changed 8 years ago by jinmei

  • Status changed from assigned to reviewing

comment:12 Changed 8 years ago by jinmei

I've not heard an objection, so merged the cleanups. Closing ticket.

comment:13 Changed 8 years ago by jinmei

  • Resolution set to complete
  • Status changed from reviewing to closed
  • Total Hours changed from 0 to 0.33
Note: See TracTickets for help on using tickets.