Opened 8 years ago

Closed 8 years ago

Last modified 8 years ago

#1696 closed task (complete)

lettuce test for NSEC3 responses

Reported by: jinmei Owned by: jelte
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:
Estimated Difficulty: 6 Add Hours to Ticket: 0
Total Hours: 9.75 Internal?: no

Description (last modified by jinmei)

or checking complete responses from b10-auth in various types of
queries in general.

My suggestion is to use rfc5155-example.zone.signed (available in
src/lib/testutils/testdata of trac1695 branch) and check example
cases described in appendix B of RFC5155. If it's easy add some
more cases not covered in the examples.

Subtickets

Change History (19)

comment:1 Changed 8 years ago by jinmei

  • Description modified (diff)

comment:2 Changed 8 years ago by jelte

  • Estimated Difficulty changed from 0 to 6

comment:3 Changed 8 years ago by jreed

We need tasks for in-memory and for sqlite3 data source backends.

comment:4 Changed 8 years ago by jelte

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

comment:5 Changed 8 years ago by jelte

  • Priority changed from major to critical

comment:6 Changed 8 years ago by jelte

  • Owner set to jelte
  • Status changed from new to assigned

comment:7 Changed 8 years ago by jelte

  • Owner changed from jelte to UnAssigned
  • Status changed from assigned to reviewing

Ready for review.

Apart from the tests themselves, I made three changes to the querying.py terrain file;

  • added an option for dnssec queries
  • added parsing (and checks) for edns results (do flag)
  • updated response section checker to be case insensitive and order-independent

As I mailed, one check failed, see ticket #1701, the relevant part of the check has been disabled until that ticket is resolved.

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

comment:8 Changed 8 years ago by jinmei

  • Owner changed from UnAssigned to jinmei

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

general

  • considering future extensions, is it easy to share the same scenario for in-memory and sqlite3 (or other database-based) data sources? not requiring it be addressed in this ticket, though.
  • if it's not too hard, I'd like to add tests for uncovered cases (I suspect there are some) of RFC5155 Sections 7.2.2 through 7.2.8. We may find some other bugs like #1701.

nsec3_auth.config

  • Do we need an explicit Boss (components) configuration?

terrain/querying.py

  • I'd say 'response message' instead of 'response packet':
    # (flags and edns_flags are both one string with all flags, in the order
    # in which they appear in the response packet.)
    
  • check_last_query_section: I'm afraid it's technically not correct to convert all inputs to lower-cased:
        # replace whitespace of any length by one space
        response_string = re.sub("[ \t]+", " ", response_string)
        expect = re.sub("[ \t]+", " ", step.multiline)
    
    e.g., RDATA of TXT RR should be considered case sensitive. But, assuming (or hoping) we'll make these checks more reliable in future, for now I'm okay with keeping this probably with some warning comments.

comment:10 Changed 8 years ago by jinmei

  • Owner changed from jinmei to jelte

comment:11 in reply to: ↑ 9 ; follow-up: Changed 8 years ago by jelte

  • Owner changed from jelte to jinmei
  • Total Hours changed from 0 to 8

Replying to jinmei:

general

  • considering future extensions, is it easy to share the same scenario for in-memory and sqlite3 (or other database-based) data sources? not requiring it be addressed in this ticket, though.

I think we could use Outlines to 'loop' over a set of configurations if we want, but I haven't tried it out yet.

http://lettuce.it/tutorial/scenario-outlines.html#tutorial-scenario-outlines

  • if it's not too hard, I'd like to add tests for uncovered cases (I suspect there are some) of RFC5155 Sections 7.2.2 through 7.2.8. We may find some other bugs like #1701.

Right, I've come up with 5 new tests:

  • 2 cases where one NSEC3 covers multiple parts of the proof (closest enc, and wildcard match) both of these failed so are commented out for now (nsec3 is added twice
  • 2 cases for querying for wildcard name itself (either the NODATA or the wildcard record itself)
  • Case for a direct query of an NSEC3 record (which should result in denial of existence)

As discussed on jabber, the first two failed, and are commented out. The last three succeed.

If you have any other cases I missed, let me know :)

nsec3_auth.config

  • Do we need an explicit Boss (components) configuration?

We don't *need* it, but when setting it up, I just disabled all modules we don't need (no need to start up xfrin, xfrout, etc. for each and every case)

terrain/querying.py

  • I'd say 'response message' instead of 'response packet':
    # (flags and edns_flags are both one string with all flags, in the order
    # in which they appear in the response packet.)
    

ok, done

  • check_last_query_section: I'm afraid it's technically not correct to convert all inputs to lower-cased:
        # replace whitespace of any length by one space
        response_string = re.sub("[ \t]+", " ", response_string)
        expect = re.sub("[ \t]+", " ", step.multiline)
    
    e.g., RDATA of TXT RR should be considered case sensitive. But, assuming (or hoping) we'll make these checks more reliable in future, for now I'm okay with keeping this probably with some warning comments.

True, it was a choice between not taking the examples from the rfc as literally as possible, or possibly missing things such as capitalization of TXT rdatas. I chose the latter. It's not too hard to do the former (i still had to edit them in some places, uppercasing hex and baseX output would also not be too much work).

If we decide we do need it, we can remove the lowercasing, and update the data in the scenario's, or we can even make all three conversions optional on a per-scenario basis.

Added a comment to the docstring of the step.

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

Replying to jelte:

Replying to jinmei:

general

  • considering future extensions, is it easy to share the same scenario for in-memory and sqlite3 (or other database-based) data sources? not requiring it be addressed in this ticket, though.

I think we could use Outlines to 'loop' over a set of configurations if we want, but I haven't tried it out yet.

http://lettuce.it/tutorial/scenario-outlines.html#tutorial-scenario-outlines

  • if it's not too hard, I'd like to add tests for uncovered cases (I suspect there are some) of RFC5155 Sections 7.2.2 through 7.2.8. We may find some other bugs like #1701.

Right, I've come up with 5 new tests:

  • 2 cases where one NSEC3 covers multiple parts of the proof (closest enc, and wildcard match)

This one seems to be using the same query name as the previous (non
wildcard) test. Is that correct?

    #Scenario: 7.2.2 other; Name Error where one NSEC3 covers multiple parts of proof (wildcard)

If you have any other cases I missed, let me know :)

  • Case for Section 7.2.4: no data, type DS (there's no test case for it, right?)
  • run-time collision, if possible (but I guess it's probably unrealistic)
  • check_last_query_section: I'm afraid it's technically not correct to convert all inputs to lower-cased:

Added a comment to the docstring of the step.

The last sentence didn't parse for me:

    currently the checks are always case insensitive. Should we checks do
    need to be case sensitive, we can either remove it or make it optional.

Do you mean, "Should the checks do need..." or something?

comment:13 Changed 8 years ago by jinmei

  • Owner changed from jinmei to jelte

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

  • Owner changed from jelte to jinmei

Right, I've come up with 5 new tests:

  • 2 cases where one NSEC3 covers multiple parts of the proof (closest enc, and wildcard match)

This one seems to be using the same query name as the previous (non
wildcard) test. Is that correct?

    #Scenario: 7.2.2 other; Name Error where one NSEC3 covers multiple parts of proof (wildcard)

ack, should've been a.w.example, fixed (the intention is that it is the 'other' NSEC3 that is added twice here)

If you have any other cases I missed, let me know :)

  • Case for Section 7.2.4: no data, type DS (there's no test case for it, right?)

added two cases; DS for just some name that exists in the zone, and direct DS query for an opt-out delegation.

  • run-time collision, if possible (but I guess it's probably unrealistic)

quite :) should we come across a collision we can of course add it, but unless we already know of one, they tend to be quite hard to find :)

  • check_last_query_section: I'm afraid it's technically not correct to convert all inputs to lower-cased:

Added a comment to the docstring of the step.

The last sentence didn't parse for me:

    currently the checks are always case insensitive. Should we checks do
    need to be case sensitive, we can either remove it or make it optional.

Do you mean, "Should the checks do need..." or something?

Actually I meant 'should we decide the checks do need'. Amended (as well as an extra comment that some tests need updating then).

comment:15 in reply to: ↑ 14 Changed 8 years ago by jinmei

The revised branch looks okay. Please feel free to merge.

comment:16 Changed 8 years ago by jinmei

  • Owner changed from jinmei to jelte

comment:17 Changed 8 years ago by jinmei

#1701 has been merged. When you merge this branch, please re-enable
the disabled test that should now succeed.

comment:18 Changed 8 years ago by jelte

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

Thanks! Merged and re-enabled the wildcard response test. Closing ticket.

comment:19 Changed 8 years ago by jinmei

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