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-20130423
         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):

 '''changelog'''

 I'd leave it to you, but I personally think it's sufficient if we
 provide a single entry when we complete the set of rdata updates
 (probably at the point of completing #2522).

 '''opt_41.cc'''

 - I don't think we need to include lexer_util.h for this file
 - I'd clarify that the constructors *always* throw.  The current
   documentation could read as if it throws only if it cannot be
   constructed (and it succeeds for other cases).

 '''rrsig_46.cc'''

 - I made a few minor style type of cleanups.
 - createFromLexer: maybe a matter of taste, but I think it's better to
   catch errors as  soon as possible.  So it would look:
 {{{#!cpp
     const RRType covered_type(lexer.getNextToken(MasterToken::STRING).
                               getString());
     const uint32_t algorithm = lexer.getNextToken(MasterToken::NUMBER).
         getNumber();
     if (algorithm > 0xff) {
         isc_throw(InvalidRdataText, "RRSIG algorithm out of range");
     }
 ...
     impl_ = new RRSIGImpl(covered_type, algorithm, labels, originalttl,
                           timeexpire, timeinception,
                           static_cast<uint16_t>(tag), signer, signature);
 }}}
   (by converting the string to RRType first we can catch bad RR type
   representation sooner)
 - I'm not sure if we want to allow "1H" etc for the original TTL
   field:
 {{{#!cpp
     const uint32_t originalttl =
 RRTTL(lexer.getNextToken(MasterToken::STRING).getString()).getValue();
 }}}
   At least BIND 9 doesn't allow this form for this field.  See
   https://bugs.isc.org/Ticket/Display.html?id=33305
   I'm personally inclined to follow BIND 9, whether it was an
   intentional decision, although this is probably too minor and can be
   implementation specific.  In either case we need some more
   documentation: if we don't allow it, explain it and it's different
   from, e.g., SOA and explain the rationale; if we allow it, explain
   it's different from BIND 9, explain why (and also update
   doc/differences.txt).
 - For documentation, I'd also refer to RFC4034, saying the allowed
   format basically conforms to the RFC.

 '''dhcid_49.cc'''

 - I made one minor style type of cleanups.

 - createFromLexer: this revised version seems to have a regression
   that space-separated multiple base64 chunks aren't recognized.
   Assuming I'm right, this also means tests are not sufficient.

 - createFromLexer: not due to this branch, but I'm not sure if we need
   to have this check:
 {{{#!cpp
     // RFC4701 states DNS software should consider the RDATA section to
     // be opaque, but there must be at least three bytes in the data:
     // < 2 octets >    Identifier type code
     // < 1 octet >     Digest type code
 }}}
   At least BIND 9 doesn't have this check.

 '''unit tests general'''

 I suggest covering both "from string" and "with lexer" versions in a
 unified way and more comprehensively.  See, for example, the fromText
 test of rdata_dnskey_unittest in master.

 '''rdata_rrsig_unittest'''

 - more test cases seem to be needed: non number is used for a number
   field; bad form of RR type (separately from the "missing field"
   case); bad signer name; bad expiration/inception cases should be
   separated (otherwise the latter one is effectively untested); if we
   allow additional form for original TTL it should be tested; multi
   line input should be tested; space in base64 should be tested; and
   probably more.

 - I personally think we don't need createFromLexer test any more (it
   was needed while we used a workaround wrapper) and we should clean
   it up.

 '''rdata_dhcid_unittest'''

 - as noted above we need more tests.

 '''rrcollator_unittest'''

 - why the space here?
 {{{#!diff
 -                                "12345 example.com. FAKE\n")),
 +                                "12345 example.com. FAKE ")),
          sig_rdata2_(createRdata(RRType::RRSIG(), rrclass_,
                                  "NS 5 3 3600 20000101000000
 20000201000000 "
 -                                "12345 example.com. FAKE\n"))
 +                                "12345 example.com. FAKE "))
 }}}

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