BIND 10 #2387: support generic version of rdata::createRdata(text) in DNSKEY, NSEC3, NSEC3PARAM

[BIND 10 Development]

#2387: support generic version of rdata::createRdata(text) in DNSKEY, NSEC3,
NSEC3PARAM
-------------------------------------+-------------------------------------
            Reporter:  jinmei        |                        Owner:
                Type:  task          |  jinmei
            Priority:  medium        |                       Status:
           Component:  libdns++      |  reviewing
            Keywords:                |                    Milestone:
           Sensitive:  0             |  Sprint-20130319
         Sub-Project:  DNS           |                   Resolution:
Estimated Difficulty:  5             |                 CVSS Scoring:
         Total Hours:  0             |              Defect Severity:  N/A
                                     |  Feature Depending on Ticket:
                                     |  loadzone-ng
                                     |          Add Hours to Ticket:  0
                                     |                    Internal?:  0
-------------------------------------+-------------------------------------

Comment (by jinmei):

 '''nsec3param_common.h'''

 - This doesn't seem to be very correct:
 {{{#!cpp
 /// \exception MasterLexer::LexerError There was a failure reading a
 /// field from the MasterLexer.
 }}}
   "failure" sounds like a low level (quite unexpected) error such as
   an I/O error, but `LexerError` is more expected to be thrown against
   syntax errors (such as a string given when a number is expected or
   an unexpected end of input).

 '''dnskey_48.cc'''
 - the doxygen doc for the constructors share some amount of text.  I'd
   unify them, e.g., by referring from one to the other.
 - is this good?
 {{{#!cpp
     while (true) {
         const MasterToken& token = lexer.getNextToken();
         if (token.getType() != MasterToken::STRING) {
             break;
         }
         keydatastr.append(token.getString());
     }
 }}}
   This way, even if the lexer founds a syntax error, it exits from the
   loop just like the normal case and the error detection is left to
   the caller.  It seems to me better if we keep extracting strings
   exclusively until we see end of line/input and catch any
   intermediate error at the exact point.  BTW, I found nsec_bitmap.cc
   has the same issue.
 - probably marginal, but it'd be slightly more efficient if we use the
   other version of `getString()`
 {{{#!cpp
     string keydatastr, keydata_token;
     while (true) {
         const MasterToken& token =
             lexer.getNextToken(MasterToken::STRING, true);
         if (token.getType() != MasterToken::STRING) {
             break;
         }
         token.getString(keydata_token);
         keydatastr.append(keydata_token);
     }
 }}}
   (and while it's out of scope of this ticket, the same point applies
   to buildBitmapsFromLexer() regarding `type_str`).
 - not directly related to this ticket, but what's the rationale of
   this?
 {{{#!cpp
     if (keydatastr.size() == 0) {
         isc_throw(InvalidRdataText, "Missing DNSKEY digest");
     }
 }}}
 - ditto, but this one too:
 {{{#!cpp
     if (algorithm == 1 && keydata.size() < 3) {
         isc_throw(InvalidRdataLength, "DNSKEY keydata too short");
     }
 }}}
 - and why don't we do the same check for the "from wire" case if we
   need to do it?
 - and, if we allow empty key this code is not really correct:
 {{{#!cpp
     buffer.readData(&keydata[0], rdata_len);
 }}}
   because accessing `[0]` is invalid for an empty vector.

 '''nsec3_50.cc'''
 - redundant doc (see dnskey comments)
 - this can be a reference:
 {{{#!cpp
     const string nexthash =
 lexer.getNextToken(MasterToken::STRING).getString();
 }}}
  (and the line is too long)
 - this can be a reference:
 {{{#!cpp
     const MasterToken token = lexer.getNextToken();
 }}}
 - is it possible to avoid doing `ungetToken()` here:
 {{{#!cpp
     if ((token.getType() == MasterToken::END_OF_LINE) ||
         (token.getType() == MasterToken::END_OF_FILE)) {
          empty_bitmap = true;
     }

     lexer.ungetToken();
 }}}
   by unifying it in `buildBitmapsFromLexer`?  `ungetToken` for a
   string is a bit expensive because you'd need to copy it twice.

 '''nsec3param_51.cc'''
 - redundant doc (see dnskey comments)

 '''masterload_unittest.cc'''

 Is that intentional there are two '#' signs here?
 {{{
 // doesn't support them. This test should be fixed and re-enabled by
 // ##2381, or deleted.
 }}}

 '''tests in general'''

 It looks suboptimal that string and lexer versions have different sets
 of tests (e.g., badText and createFromLexer for DNSKEY).  They cause
 duplicates/redundancy, as well as incomplete cases.  I'd generally
 unify these cases.  See, for example, `Rdata_IN_A_Test::createFromText`.

 '''rdata_dnskey_unittest.cc'''

 - This case is now moot:
 {{{#!cpp
     // Missing algorithm
     EXPECT_THROW(generic::DNSKEY("257 3 5BEAAEFTd"), InvalidRdataText);
 }}}
   This test was specifically crafted for this code:
 {{{#!cpp
     unsigned int flags, protocol, algorithm;
     stringbuf keydatabuf;

     iss >> flags >> protocol >> algorithm >> &keydatabuf;
 }}}
   which would incorrectly accept the above invalid input.  It doesn't
   make much sense to use the same test case because our lexer is never
   confused with token boundary this way.  If we really want to check
   the "missing algorithm" case, we should at least make the base64
   part valid; otherwise it may not be really clear if the
   implementation correctly detects the algorithm is missing.
 - Likewise, this one:
 {{{#!cpp
     // How about this?  It's even more confusing for the parser because
     // it could be ambiguous '51 EAAA' vs '5 1EAA..'. But we first parse
     // the other fields before we parse the public key, so we reject
     // this.
     EXPECT_THROW(generic::DNSKEY("257 3 51EAAEFTd"), InvalidRdataText);
 }}}
   "how about this?" part doesn't make sense any more, and it's not
   really confusing at all (the input is obviously invalid).  And, it's
   not even clear whether we need two cases like this.
 - cases where a non number is used for a number field are missing.
   checking spaces in base64 also seems to be missing.  there may be
   some other missed cases.  Please check corner cases as if it were
   developed test-driven.

 '''rdata_nsec3_unittest.cc'''

 - number vs non-number cases seem to be missing.  there may be some
   other.

 '''rdata_nsec3param_unittest.cc'''

 - number vs non-number cases seem to be missing.  bad hex case is
   missing.  there may be some other.

-- 
Ticket URL: <http://bind10.isc.org/ticket/2387#comment:14>
BIND 10 Development <http://bind10.isc.org>
BIND 10 Development