Opened 8 years ago

Closed 8 years ago

Last modified 8 years ago

#1580 closed task (fixed)

auth::Query NSEC3 support: Name Error case

Reported by: jinmei Owned by: kevin_tes
Priority: medium Milestone: Sprint-20120221
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: 4 Add Hours to Ticket: 0
Total Hours: 2 Internal?: no

Description (last modified by jinmei)

(updated based on #1431 discussion)

This task implements RFC5155 7.2.2 (and 7.2.9) and
update ZoneFinder::NXDOMAIN case of Query::process():

  • call findNSEC3(qname, recursive=true). It should return the closest encloser proof. If next_proof is null, it means a run-time collision (or the zone is otherwise broken), so we should return SERVFAIL as described in RFC5155 7.2.9.
  • construct the possible best wildcard name from the closest provable encloser and call findNSEC3(recursive = false) for it. It will return NSEC3 covering the wildcard name.
  • add the returned NSEC3s to the authority section

This task depends on #1431.

Subtickets

Change History (17)

comment:1 Changed 8 years ago by jinmei

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

comment:2 Changed 8 years ago by jinmei

  • Description modified (diff)

comment:3 Changed 8 years ago by jelte

  • Estimated Difficulty changed from 0 to 4

comment:4 Changed 8 years ago by jelte

  • Milestone changed from Year 3 Task Backlog to Sprint-20120207

comment:5 Changed 8 years ago by jinmei

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

comment:6 Changed 8 years ago by jinmei

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

comment:7 Changed 8 years ago by kevin_tes

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

comment:8 Changed 8 years ago by kevin_tes

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

This ticket should be reviewed now.

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

Initial feedback:

  • I've made trivial editorial cleanups. In particular, redundant white spaces at end of lines should be easily identifiable if (e.g) using colored output of git. I'd suggest you perform this level of checks before asking for review.
  • The code seems naively copies comments from the NSEC counterpart, many of which are not applicable to the NSEC3 case. Please go through the comments and make sure they make sense in the context.
  • in addNSEC3NXDOMAINProof, the case where next_proof is NULL must be explicitly rejected (and tested)
  • related to the previous point, tests are insufficient. They should cover failure cases.
  • This is not correct:
        const Name wildname(Name("*").concatenate(qname_.split(1)));
    
    Consider the case where the wildcard is *.example.com and qname is a.b.example.com.

comment:10 Changed 8 years ago by jinmei

  • Owner changed from UnAssigned to kevin_tes

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

Replying to jinmei:

Initial feedback:

  • I've made trivial editorial cleanups. In particular, redundant white spaces at end of lines should be easily identifiable if (e.g) using colored output of git. I'd suggest you perform this level of checks before asking for review.

Thanks very much for the remind,will do that from then on.

  • The code seems naively copies comments from the NSEC counterpart, many of which are not applicable to the NSEC3 case. Please go through the comments and make sure they make sense in the context.

Ok, check and amend will be done.

  • in addNSEC3NXDOMAINProof, the case where next_proof is NULL must be explicitly rejected (and tested)

Codes and unittest will be added.

  • related to the previous point, tests are insufficient. They should cover failure cases.

More tests will be done.

  • This is not correct:
        const Name wildname(Name("*").concatenate(qname_.split(1)));
    
    Consider the case where the wildcard is *.example.com and qname is a.b.example.com.

Sorry, consideration seems not so thoroughly, those codes will be modified.

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

  • Owner changed from kevin_tes to jinmei

Changed has been done,please review.

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

Replying to kevin_tes:

Changed has been done,please review.

  • Some comments still seem incorrect, and some seem redundant (header part comments seem to repeat what's commented in the method body).
  • On second thought, I'd (at least for now) let Message::addRRset() throw if the next proof is null. As we discussed in the biweekly call, we should probably keep the query logic simple for bogus return values due to broken data source implementation as long as it doesn't crash.
  • Also as we discussed, duplicate name check is probably better deferred to later cleanup.
  • The case of wildcard NSEC3 is a matching NSEC3 should be checked and rejected (and tested)

Normally I give the branch back to the original author with comments
(and I don't want to "steal" other's work), but since the deadline is
coming, I've directly committed suggested changes to these points.
Before doing that, I first merged master to the branch and resolved
conflicts. That would make the final merge smoother. The latest
commit of mine is not related to the above points, and is rather
cleanup. I've provided some detailed commit logs to explain the
intent of each change.

If you agree with the comments above and the changes, please simply
merge it to master. Of course, you may not agree with some of them or
may find a bug in it. In that case please continue the discussion.

comment:14 Changed 8 years ago by jinmei

  • Owner changed from jinmei to kevin_tes

comment:15 Changed 8 years ago by jelte

I also looked at it, and had some additional comments. However, I think these can be deferred to #1587 (added a comment there), and this can be merged.

If you want to do this, please go ahead. Otherwise I'll merge it later today.

comment:16 Changed 8 years ago by kevin_tes

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

Ok,the ticket was closed, and merged.

comment:17 Changed 8 years ago by jinmei

  • Total Hours changed from 0 to 2
Note: See TracTickets for help on using tickets.