Opened 8 years ago

Closed 8 years ago

Last modified 8 years ago

#1310 closed task (fixed)

auth NSEC support: Handle WILDCARD_NXRRSET case

Reported by: jinmei Owned by: kevin_tes
Priority: medium Milestone: Sprint-20111206
Component: b10-auth Version:
Keywords: Cc:
CVSS Scoring: Parent Tickets:
Sensitive: no Defect Severity: N/A
Sub-Project: DNS Feature Depending on Ticket: NSEC
Estimated Difficulty: 3 Add Hours to Ticket: 1
Total Hours: 0 Internal?: no

Description

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

Subtickets

Change History (24)

comment:1 Changed 8 years ago by jelte

  • Add Hours to Ticket changed from 0 to 3

comment:2 Changed 8 years ago by jelte

  • Add Hours to Ticket 3 deleted
  • Estimated Difficulty changed from 0 to 3

comment:3 Changed 8 years ago by jelte

  • Milestone changed from Sprint-20111025 to Sprint-20111108

comment:4 Changed 8 years ago by kevin_tes

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

comment:5 Changed 8 years ago by jelte

  • Milestone changed from Sprint-20111108 to Sprint-20111122

comment:6 Changed 8 years ago by jinmei

I've noticed a branch named "trac1310" had been pushed to the
central repository. Is this branch expected to be reviewed?

If so, please change the status of this ticket to "review to Unassigned"
with a comment about something like "this is ready for review".

comment:8 Changed 8 years ago by kevin_tes

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

comment:9 Changed 8 years ago by kevin_tes

this is ready for review

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

handle the WILDCARD_NXRRSET.
In this case,if zone is secure and support NSEC,
an NSEC RR proving that there is no exact match for QNAME,
an NSEC RR proving that the zone contains no RRsets that
would match <QNAME,QTYPE>,via wildcard name expansion,
should add those to the authority section.
Some times one NSEC RR can do the same thing.
In case of this,the respone should remove the duplicate one.

I only test the one NSEC RR can do the same thing( that to say: the respone should remove the duplicate one.)

Because:i found that in the query_unittest.cc:
ZoneFinder::FindResult?
MockZoneFinder::find():
when it goes down to wildcard branch:

if ((options & NO_WILDCARD) == 0) {

const Name wild_suffix("wild.example.com");

I am not sure why fixed this domain("wild.example.com") here.

If added new unit test not using this domain,this branch should be overrided, this may affect other unit test.

But used this domain("wild.example.com"), i can not write unit test about two NSEC RRs.

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

Replying to kevin_tes:

Because:i found that in the query_unittest.cc:
ZoneFinder::FindResult?
MockZoneFinder::find():
when it goes down to wildcard branch:

if ((options & NO_WILDCARD) == 0) {

const Name wild_suffix("wild.example.com");

I am not sure why fixed this domain("wild.example.com") here.

It's just for the convenience of the test at that point. It was
expected to be extended to support other cases.

comment:12 Changed 8 years ago by stephen

  • Owner changed from UnAssigned to stephen

comment:13 follow-up: Changed 8 years ago by stephen

  • Owner changed from stephen to kevin_tes

src/bin/auth/query.cc
Indentation of addWildcardNxrrsetProof() uses tabs - it should use spaces.

The first block of comments in addWildcardNxrrsetProof() do not apply to the immediately following statements, but to the block of statements after that - the one starting:

const ZoneFinder::FindResult fresult =

As far as I can determine, the first "if" block:

if (nsec->getRdataCount() == 0) {

is a check that the the first NSEC RR in 3.1.3.4 - the one that proves there are no RRsets of the STYPE requested at the name that matched <SNAME, SCLASS> via wildcard expansion - is present in the "nsec" RRset. The reason for this check should be commented.

Adding the NSEC RRsets to the Authority section is done via the construct:

if names are equal {
   add fresult.rrset
} else {
   add nsec
   add fresult.rrset
}

The "add fresult rrset" is common to both branches of the "if" statement and so can be outside it.

If the above change is made, then part of the processing in addWildcardNxrrsetProof() is identical to the processing in addWildcardProof(). It suggests that the two could be combined: add a second argument to addWildcardProof() that defaults to an empty ConstRRsetPtr. The code would then:

  • add the standard wildcard NSEC record
  • if "nsec" is not null:
    • test if it contains any NSEC records; if not, thrown an exception
    • add to the Authority section if the name is different from the standard wildcard record

src/bin/auth/query.h
The header for addWildcardNxrrsetProof() should include Doxygen tags describing the arguments.

Although not really part of this ticket, while addressing the above comment the opportunity should be taken to add Doxygen tags to the headers of the other "addXxx()" methods.

The headers of these methods should be expanded to do a better description as to how the methods are used. It took quite a bit of searching (and reading RFC 4035) to find out how the logic worked (i.e. that addWildcardNxrrsetProof() depends on the fact that ZoneFinder::find() in this case - Wildcard No Data - returns the NSEC that covers the interval where the empty non-terminal lives, but not the NSEC for the wildcard demanded by RFC 4035, thus requiring the addition of the wildcard NSEC).

src/bin/auth/tests/query_unittest.cc
There should be two tests: one for the case that returns two NSEC records, and one that returns a single NSEC record (to prove that the duplicate NSEC is not included in the Authority section).

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

Replying to stephen:
Thanks for the review.

src/bin/auth/query.cc
Indentation of addWildcardNxrrsetProof() uses tabs - it should use spaces.

I set my vim editor that a tabs equals four spaces,is it ok? Otherwise i 'll change it.

The first block of comments in addWildcardNxrrsetProof() do not apply to the immediately following statements, but to the block of statements after that - the one starting:

const ZoneFinder::FindResult fresult =

As far as I can determine, the first "if" block:

if (nsec->getRdataCount() == 0) {

is a check that the the first NSEC RR in 3.1.3.4 - the one that proves there are no RRsets of the STYPE requested at the name that matched <SNAME, SCLASS> via wildcard expansion - is present in the "nsec" RRset. The reason for this check should be commented.

good advice, i'll add it.

Adding the NSEC RRsets to the Authority section is done via the construct:

if names are equal {
   add fresult.rrset
} else {
   add nsec
   add fresult.rrset
}

The "add fresult rrset" is common to both branches of the "if" statement and so can be outside it.

If the above change is made, then part of the processing in addWildcardNxrrsetProof() is identical to the processing in addWildcardProof(). It suggests that the two could be combined: add a second argument to addWildcardProof() that defaults to an empty ConstRRsetPtr. The code would then:

  • add the standard wildcard NSEC record
  • if "nsec" is not null:
    • test if it contains any NSEC records; if not, thrown an exception
    • add to the Authority section if the name is different from the standard wildcard record

good advice, i'll changed it.

src/bin/auth/query.h
The header for addWildcardNxrrsetProof() should include Doxygen tags describing the arguments.

Although not really part of this ticket, while addressing the above comment the opportunity should be taken to add Doxygen tags to the headers of the other "addXxx()" methods.

The headers of these methods should be expanded to do a better description as to how the methods are used. It took quite a bit of searching (and reading RFC 4035) to find out how the logic worked (i.e. that addWildcardNxrrsetProof() depends on the fact that ZoneFinder::find() in this case - Wildcard No Data - returns the NSEC that covers the interval where the empty non-terminal lives, but not the NSEC for the wildcard demanded by RFC 4035, thus requiring the addition of the wildcard NSEC).

good advice,i'll fix it.

src/bin/auth/tests/query_unittest.cc
There should be two tests: one for the case that returns two NSEC records, and one that returns a single NSEC record (to prove that the duplicate NSEC is not included in the Authority section).

see my comment http://bind10.isc.org/ticket/1310?replyto=11#comment

but i'll fixed it.

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

comment:15 Changed 8 years ago by kevin_tes

This has been done based on comments,please review it.

comment:16 Changed 8 years ago by kevin_tes

  • Owner changed from kevin_tes to stephen

comment:17 follow-up: Changed 8 years ago by stephen

  • Owner changed from stephen to kevin_tes

src/bin/auth/query.cc
Indentation of addWildcardNxrrsetProof() uses tabs - it should use spaces.

The problem is there are tab characters in the file - each of these needs to be replaced with four spaces.

I set my vim editor that a tabs equals four spaces,is it ok? Otherwise i 'll change it.

As well as "set tabstop=4", you also need to add "set expandtab" to your .vimrc file to get the tab key to insert up to four spaces in the file instead of a tab character.

src/bin/auth/query.h
Tab characters in the file.

src/bin/auth/query.cc
Tab characters in the file.

The comment in the new text (wildcardNxrrsetWithNEC) is the same as the comment in the test above it - what are the different conditions that the tests are testing?

comment:18 Changed 8 years ago by stephen

  • Add Hours to Ticket set to 1

comment:19 in reply to: ↑ 17 Changed 8 years ago by kevin_tes

stephen,thanks very much for the review.
I get a lot from the comments!

Replying to stephen:

src/bin/auth/query.cc
Indentation of addWildcardNxrrsetProof() uses tabs - it should use spaces.

The problem is there are tab characters in the file - each of these needs to be replaced with four spaces.

I set my vim editor that a tabs equals four spaces,is it ok? Otherwise i 'll change it.

As well as "set tabstop=4", you also need to add "set expandtab" to your .vimrc file to get the tab key to insert up to four spaces in the file instead of a tab character.

Thanks for notice,

src/bin/auth/query.h
Tab characters in the file.

src/bin/auth/query.cc
Tab characters in the file.

Have changed the 'tab' to 'space' by using
:set ts=4
:set expandtab
:%retab!

The comment in the new text (wildcardNxrrsetWithNEC) is the same as the comment in the test above it - what are the different conditions that the tests are testing?

I added the comment for wildcardNxrrsetWithNEC,

please recheck it!

comment:20 Changed 8 years ago by kevin_tes

  • Owner changed from kevin_tes to stephen

comment:21 Changed 8 years ago by stephen

  • Owner changed from stephen to kevin_tes

Reviewed commit 97cf501e33b45c373aa12a3cb8ae76909d3522bc

All OK, please merge.

comment:22 Changed 8 years ago by kevin_tes

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

comment:23 Changed 8 years ago by kevin_tes

Thanks, merged.

comment:24 Changed 8 years ago by shane

  • Feature Depending on Ticket set to NSEC
Note: See TracTickets for help on using tickets.