Opened 8 years ago

Closed 8 years ago

#1587 closed task (complete)

auth::Query NSEC3 support: cleanup

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

Description (last modified by jinmei)

Once all of #1580, #1581, #1582, #1583, #1584, and #1585 are
completed, we should check any obvious code duplicate and unify them.

Depend on these tickets.

We already noted in #1585 several specific cases where duplicate code
should be unified. These should be addressed in this ticket.

Subtickets

Change History (19)

comment:1 Changed 8 years ago by jelte

  • Estimated Difficulty changed from 0 to 5

comment:2 Changed 8 years ago by jinmei

  • Milestone changed from Year 3 Task Backlog to Next-Sprint-Proposed

comment:3 Changed 8 years ago by jinmei

  • Milestone changed from Next-Sprint-Proposed to Sprint-20120207

comment:4 Changed 8 years ago by jinmei

  • Description modified (diff)

comment:5 Changed 8 years ago by jelte

Another task for this ticket: we don't consistently check the results from findNSEC3(); in most cases we check 'matched', but not always. Also, we don't always check if next_proof is set when it should be. So if we would have a datasource that does not return the correct data, we might get errors like 'NULL RRset is given to addRRset()", which with a simple check could be 'no next_proof in result of findRRset() for NXDOMAIN case", for example. (Note: in both cases this will result in SERVFAIL, so there is no behavioural change, but IMO if we want to log this it's better to produce a helpful message).

One example is the result of findNSEC3 in addNXDOMAINProofByNSEC3(), added in #1580; here neither 'matched' nor 'next_proof is not empty' are checked. But it's probably easiest to just walk through all the calls to findNSEC3()

We should at least make the checking consistent (either check everywhere, or don't check and assume the datasource returns correct data, i personally prefer the former at this time) Perhaps this can be done with a helper function, but i'm not sure; this may lose to much context to produce informative errors.

Also, we should probably check for run-time collisions (see 5155 section 7.2.9). But this is the same check for next_proof i think.

Last edited 8 years ago by jelte (previous) (diff)

comment:6 Changed 8 years ago by jelte

Oh, and it is probably also a good idea to do a coverage check, i think we missed a few code paths in our test cases

comment:7 Changed 8 years ago by jelte

  • Priority changed from major to critical

comment:8 Changed 8 years ago by jinmei

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

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

trac1587 is ready for review.

I summarized how the NSEC3 cases call findNSEC3() and use the results
in a table, and decided to categorize the cases into two cases,
corresponding to recursive/non recursive mode of findNSEC3(). I
introduced separate private methods for these 2 cases and implemented
consistent validation policy there.

Of course, this is not the only reasonable way to unify the cases.
For example, we could probably even consolidate all cases into a
single super clever method. But in my subjective impression based on
the summary table that would be rather complicated. I hope my result
is at least reasonably understandable.

I'm not answering each point noted in the ticket, but I believe I
covered all major concerns in this branch.

I've also performed coverage check. It was actually quite good, and
at least I didn't find any uncovered code related to NSEC3 changes.
The coverage report is available here:
http://bind10.isc.org/~jinmei/cov-1587/cov/bin/auth/query.cc.gcov.html

Since we are adding a new user-visible feature, I think we should
provide a summarized changelog entry. This is the proposal:

384.?	[func]		haikuo, jelte, jinmei, kevin, vorner
	b10-auth now supports NSEC3-signed zones in the in-memory data
	source.
	(Trac #1580-#1585, #1587, and other related changes to the
	in-memory data source)

comment:10 Changed 8 years ago by jinmei

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

comment:11 Changed 8 years ago by kevin_tes

  • Owner changed from UnAssigned to kevin_tes

comment:12 follow-up: Changed 8 years ago by kevin_tes

Hello,all seems good to me.

For one thing I am not sure. In this method:
+ uint8_t
+ Query::addClosestEncloserProof(ZoneFinder?& finder, const Name& name,
+ bool exact_ok, bool add_closest)

For opt-out,should we valid the opt-out flag been set? If not, I do not think we need "bool exact_ok" this parameter.

comment:13 Changed 8 years ago by kevin_tes

  • Owner changed from kevin_tes to jinmei

comment:14 in reply to: ↑ 9 Changed 8 years ago by kevin_tes

Replying to jinmei:

trac1587 is ready for review.

I summarized how the NSEC3 cases call findNSEC3() and use the results
in a table, and decided to categorize the cases into two cases,
corresponding to recursive/non recursive mode of findNSEC3(). I
introduced separate private methods for these 2 cases and implemented
consistent validation policy there.

Of course, this is not the only reasonable way to unify the cases.
For example, we could probably even consolidate all cases into a
single super clever method. But in my subjective impression based on
the summary table that would be rather complicated. I hope my result
is at least reasonably understandable.

I'm not answering each point noted in the ticket, but I believe I
covered all major concerns in this branch.

I've also performed coverage check. It was actually quite good, and
at least I didn't find any uncovered code related to NSEC3 changes.
The coverage report is available here:
http://bind10.isc.org/~jinmei/cov-1587/cov/bin/auth/query.cc.gcov.html

Since we are adding a new user-visible feature, I think we should
provide a summarized changelog entry. This is the proposal:

384.?	[func]		haikuo, jelte, jinmei, kevin, vorner
	b10-auth now supports NSEC3-signed zones in the in-memory data
	source.
	(Trac #1580-#1585, #1587, and other related changes to the
	in-memory data source)

This may be better,_

384.?	[func]	jinmei, jelte, vorner, haikuo, kevin
 	b10-auth now supports NSEC3-signed zones in the in-memory data
 	source.
 	(Trac #1580-#1585, #1587, and other related changes to the
 	in-memory data source)

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

Replying to kevin_tes:

Thanks for the prompt review.

Hello,all seems good to me.

For one thing I am not sure. In this method:
+ uint8_t
+ Query::addClosestEncloserProof(ZoneFinder?& finder, const Name& name,
+ bool exact_ok, bool add_closest)

For opt-out,should we valid the opt-out flag been set? If not, I do not think we need "bool exact_ok" this parameter.

I'm not sure if I understand the concern, but if you mean whether it's
okay if we skip checking the NSEC3 optout bit when optout is allowed
(exact_ok = true) and we get both closest and next NSEC3 (in which
case the "next" NSEC3 should have the optout bit set), the policy here
is to basically trust the zone signer, and if it's a broken zone we'll
let the validator detect the error. This is documented in the header
file description. This policy itself could be discussed if you don't
like it, but it was derived from the pre-refactoring implementation;
this branch doesn't change that semantics. So, if you want to
continue the discussion I'd suggest doing it separately from this
ticket (so we can close it sooner).

As for the changelog, I simply listed the developer in alphabetical
order, which I think is quite normal in cases like this. You don't
have to be too humble:-) but if you really want to reorder them, I'm
okay with that, too.

comment:16 Changed 8 years ago by jinmei

  • Owner changed from jinmei to kevin_tes

comment:17 in reply to: ↑ 15 ; follow-up: Changed 8 years ago by kevin_tes

  • Owner changed from kevin_tes to jinmei

Replying to jinmei:

Replying to kevin_tes:

Thanks for the prompt review.

Hello,all seems good to me.

For one thing I am not sure. In this method:
+ uint8_t
+ Query::addClosestEncloserProof(ZoneFinder?& finder, const Name& name,
+ bool exact_ok, bool add_closest)

For opt-out,should we valid the opt-out flag been set? If not, I do not think we need "bool exact_ok" this parameter.

I'm not sure if I understand the concern, but if you mean whether it's
okay if we skip checking the NSEC3 optout bit when optout is allowed
(exact_ok = true) and we get both closest and next NSEC3 (in which
case the "next" NSEC3 should have the optout bit set), the policy here
is to basically trust the zone signer, and if it's a broken zone we'll
let the validator detect the error. This is documented in the header
file description. This policy itself could be discussed if you don't
like it, but it was derived from the pre-refactoring implementation;

Oh, it is ok to me if it was derived from the pre-refactoring implementation.

this branch doesn't change that semantics. So, if you want to
continue the discussion I'd suggest doing it separately from this
ticket (so we can close it sooner).

As for the changelog, I simply listed the developer in alphabetical
order, which I think is quite normal in cases like this. You don't
have to be too humble:-) but if you really want to reorder them, I'm
okay with that, too.

yeah,i really want to reorder them,please.

All is ok to me, i think it can be merged. Thanks.

comment:18 in reply to: ↑ 17 Changed 8 years ago by jinmei

Replying to kevin_tes:

yeah,i really want to reorder them,please.

All is ok to me, i think it can be merged. Thanks.

Okay, merge done (reordered the changelog), closing.

Thanks.

comment:19 Changed 8 years ago by jinmei

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