Opened 8 years ago

Closed 8 years ago

Last modified 8 years ago

#1307 closed task (complete)

auth NSEC support: Handle NXDOMAIN case

Reported by: jinmei Owned by: jinmei
Priority: medium Milestone: Sprint-20111025
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: 0 Internal?: no

Description

This is subtask 3 of #1244. Depend on #1305.

Subtickets

Change History (10)

comment:1 Changed 8 years ago by jinmei

  • Owner set to jinmei
  • Status changed from new to accepted
  • Summary changed from auth NSEC support: Handle NXDOMAIN case. to auth NSEC support: Handle NXDOMAIN case

comment:2 Changed 8 years ago by jinmei

trac1307 is ready for review.

Notes:

  • the first commit is a dependency merge (trac1305, already merged to trunk) so should be ignored
  • I needed to made some not directly related changes: one for a test utility (9895253) and one for the NSEC rdata class (03265ef). they are independent changes but necessary for this task
  • I also made some cleanups that are not related to the topic of this task at all. They are noted so in the commit logs. I hope these changes are acceptable

The main code logic is implemented in Query::addNXDOMAINProof(). It's
not trivial, but I believe with the comment its correctness should be clear.
The changes to the test are all about various cases of this processing.

comment:3 Changed 8 years ago by jinmei

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

comment:4 Changed 8 years ago by jelte

  • Owner changed from UnAssigned to jelte

comment:5 follow-up: Changed 8 years ago by jelte

  • Owner changed from jelte to jinmei

(I've removed a little bit of whitespace in query.cc)

Not really relevant to this ticket, but just wondering, why are
the exceptions in query.cc structs and not classes? I know there
isn't that much of a difference really, but still.

Would we want to ensure and recheck on the query.cc level that the
nsecs we add actually do cover the queried name (and later, in the
noerror/nodata response, the type)? (if so, this could be a separate
ticket)

But the code looks ok, and can be merged

comment:6 in reply to: ↑ 5 Changed 8 years ago by jinmei

Replying to jelte:

Thanks for the prompt review.

(I've removed a little bit of whitespace in query.cc)

Ack, thanks.

Not really relevant to this ticket, but just wondering, why are
the exceptions in query.cc structs and not classes? I know there
isn't that much of a difference really, but still.

Ah...well I don't know why in this case:-) I simply copied other Query
exception "structs" without even noticing they are structs and renamed
it.

Personally, when I intentionally use struct instead of class I mean
that it's just a set of values rather than realizing some abstraction
(and so all member variables are generally public, the default of
struct, and as a consequence of that I don't expect any inter
dependency between them). I guess it's similar to what Stroustrup
says in the C++ book: "I usually prefer to use struct for classes that
have all data public. I think of such classes as 'not quite proper
types, just data structures'".

For exception types, I'd actually use class, but for consistency I'll
leave it for now. (In fact, I suspect we are not so consistent about
when to use struct instead of class throughout the source code).

Would we want to ensure and recheck on the query.cc level that the
nsecs we add actually do cover the queried name (and later, in the
noerror/nodata response, the type)? (if so, this could be a separate
ticket)

I don't think so. DataSourceClient::find() requires it should meet
the requirement when it returns NXDOMAIN with an RRset. I still added
some validation for the things that a legitimate find() implementation
shouldn't break, but that's basically limited to the case where the
auth server could crash due to a buggy data source implementation. If
the server results in a bogus NSEC due to a buggy data source (client)
implementation, it's embarrassing, but I'd let it happen and then fix
the lower level implementation. (Actually I felt even the validation
I added for NSEC may be too much - for example, the case of a buggy
implementation returning an empty RRset isn't specific to NSEC. Maybe
we'll loosen the check later.)

But the code looks ok, and can be merged

Okay, will do, thanks.

comment:7 Changed 8 years ago by jinmei

merge done, closing ticket.

comment:8 Changed 8 years ago by jinmei

  • Resolution set to complete
  • Status changed from reviewing to closed

comment:9 Changed 8 years ago by jinmei

  • Estimated Difficulty changed from 0 to 5

comment:10 Changed 8 years ago by jinmei

added time estimate retroactively.

Note: See TracTickets for help on using tickets.