BIND 10 #2521: support generic version of rdata::createRdata(text) in RRSIG, DHCID, OPT RDATA

[BIND 10 Development]

#2521: support generic version of rdata::createRdata(text) in RRSIG, DHCID, OPT
RDATA
-------------------------------------+-------------------------------------
            Reporter:  jinmei        |                        Owner:
                Type:  task          |  jinmei
            Priority:  medium        |                       Status:
           Component:  libdns++      |  reviewing
            Keywords:                |                    Milestone:
           Sensitive:  0             |  Sprint-20130514
         Sub-Project:  DNS           |                   Resolution:
Estimated Difficulty:  4             |                 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):

 (The points raised in the first round review seem to have been
 addressed unless explicitly mentioned below.)

 '''rrsig_46.cc'''

 - `signer` can be a reference, which is possibly a bit more efficient:
 {{{#!cpp
     const Name& signer = createNameFromLexer(lexer, origin);
     //const Name signer = createNameFromLexer(lexer, origin);
 }}}
 - this part has several issues:
 {{{#!cpp
     string signature_txt =
         lexer.getNextToken(MasterToken::STRING).getString();
     // RFC4034 says "Whitespace is allowed within the Base64 text."
     // So read to the end of input.
     while (true) {
         const MasterToken& token = lexer.getNextToken();
         if (token.getType() != MasterToken::STRING) {
             break;
         }
         signature_txt.append(token.getString());
     }
     lexer.ungetToken();
 }}}
   - it would ignore any error detected in getNextToken() (this also
     means we need a test case covering that scenario).
   - repeated calls to `getString()` could be a bit more efficient by
     passing a placeholder string object.  See
     `DNSKEY::constructFromLexer()` in master how we address these
     points.  For the test case mentioned in the previous bullet, see
     the "Unmatched parenthesis" test in rdata_dnskey_unittest (of
     master).  We might want to extract this pattern into a helper
     utility function and share it from multiple cases (I guess we can
     use it for DHCID too).
 - the string version of constructor is not exception safe; it has the
   same issue as the equivalent constructor of DNSKEY (see the code and
   comments).
 - as for the BIND 8-style TTL representation, the code change looks
   okay, although this documentation looks a bit awkward:
 {{{#!cpp
 /// The Original TTL field is a valid decimal representation of an
 /// unsigned 32-bit integer. Note that RFC4034 does not allow alternate
 /// textual representations of \c RRTTL such as "1H" for 3600 seconds.
 }}}
   Because this representation has always been a BIND specific
   extension and would never be "approved" by RFCs anyway. But the
   point is probably subtle, so I'm okay with not tweaking the doc
   further (at least it's not incorrect).

 '''dhcid_49.cc'''

 - createFromLexer(): same comment as RRSIG applies (the while loop has
   some issues, and we should be able to use the same pattern to
   address them)
 - As for the length check, I'd personally feel it's better to not the
   check at the DNS side.  As actually commented, the RFC rather states
   it should be considered opaque in terms of DNS, and, in fact, this
   check wouldn't help any other part of DNS specific applications (it
   doesn't make them safer, for example).  And, this check is
   susceptible to any future changes of the requirement due to DHCP
   specific reasons.  Not a strong opinion either, but if you don't
   have one I suggest removing the check.

 '''rdata_rrsig_unittest.cc'''

 - "bad signer name" case: I wonder if you actually intended to add a
   space after "isc.org":
 {{{#!cpp
     checkFromText_MissingOrigin(
                      "A 5 4 43200 "
                      "20100223214617 20100222214617 8496 isc.org"
                      "evxhlGx13mpKLVkKsjpGzycS5twtIoxOmlN14w9t5AgzGBmz"
                      "diGdLIrFabqr72af2rUq+UDBKMWXujwZTZUTws32sVldDPk/"
                      "NbuacJM25fQXfv5mO3Af7TOoow3AjMaVG9icjCW0V55WcWQU"
                      "f49t+sXKPzbipN9g+s1ZPiIyofc=");
 }}}
   In its current form there could be several reasons why it's
   rejected: name label (isc.orgevxh....) is too long, or either the
   signer or signature is considered to be missing.

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