BIND 10 #2387: support generic version of rdata::createRdata(text) in DNSKEY, NSEC3, NSEC3PARAM
BIND 10 Development
do-not-reply at isc.org
Wed Apr 3 12:56:44 UTC 2013
#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-20130423
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
-------------------------------------+-------------------------------------
Changes (by muks):
* owner: muks => jinmei
Comment:
Hi Jinmei
Replying to [comment:17 jinmei]:
> See, e.g, end of Rdata_IN_AAAA_Test::createFromText. We can use a
> similar pattern in this case, e.g:
>
> {{{#!cpp
> generic::DNSKEY("(1 1 1 YmFiYWJhYmE=");
> }}}
Such tests are added now.
> > > - not directly related to this ticket, but what's the rationale of
> > > this?
> > > {{{#!cpp
> > > if (keydatastr.size() == 0) {
> > > isc_throw(InvalidRdataText, "Missing DNSKEY digest");
> > > }
> > > }}}
> >
> > This is to check that there is some key data that was read before
> > end-of-input was reached.
>
> This is mostly obvious from the code. My question was why we should
> reject it if there's no data. RFC seems to be silent. BIND 9 seems
> to accept such cases. What we should do could be debatable, but since
> this field is algorithm dependent and our implementation doesn't
> reject unknown algorithms, I'd be inclined to be lenient here.
DNSKEY now accepts RDATA with missing digest.
> > > - ditto, but this one too:
> > > {{{#!cpp
> > > if (algorithm == 1 && keydata.size() < 3) {
> > > isc_throw(InvalidRdataLength, "DNSKEY keydata too short");
> > > }
> > > }}}
> >
> > This code was not introduced by this branch, but was present in the
> > previous implementation. It is to support RSA/MD5 tag extraction:
> > http://tools.ietf.org/html/rfc4034#appendix-B.1
> >
> > See `DNSKEY::getTag()`. I've updated the code with a comment pointing
to
> > the RFC.
>
> Hmm, I see, but on a closer look this behavior still seems to be
> questionable. First, if we read the spec carefully, the key size
> should actually be even larger than 3 bytes, because the key tag comes
> from the "modulus" subfield of the public key field of the RDATA, and
> there should at least be one byte of "exponent length" subfield before
> "modulus".
>
> Regarding the computation of key tag, it seems to be different from
> what BIND 9 does (or for that matter what ldns does). BIND 9's
> behavior also seems to be questionable to me (I've reported it; see
> BIND 9 RT 33033), though. But at least these implementations do not
> reject such DNSKEYs.
>
> This can probably be an implementation dependent behavior, since it's
> quite unlikely to happen in a valid real-world scenario. But since
> checking the size against "3" isn't really correct anyway as explained
> above, and the additional special case for algorithm 1 (which
> should be 'not recommended' today) is mostly unnecessary overhead,
> I'd be inclined to do any special case handling in getTag() (where we
> need a special case per algorithm anyway). In getTag(), we could
> either be compatible with BIND 9 or could throw if it's too short.
For now I've removed these checks during construction, and moved the
check to the `getTag()` method. Now we expect at least 4 octets long
digest. We can revisit BIND 9 vs. BIND 10 when BIND 9 code is updated by
RT 33033.
> > > - and why don't we do the same check for the "from wire" case if we
> > > need to do it?
> >
> > I don't know why this was not done previous to this branch. This
branch
> > only touched the `MasterLexer` related parts of the code, so it didn't
> > modify the wire cases. I've added code towards this now.
>
> See above.
Updated.
> > > - 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.
> >
> > The content of the public key field is dependent on the choice of
> > algorithm. I guess it's safe to assume that the public key should
always
> > be non-empty. So I've added a check that the rest of the rdata is at
> > least 1 byte long, after reading the initial fields.
>
> See above.
Updated.
[snip]
> Now, comments on the revised branch:
>
> '''nsec_bitmap.cc'''
>
> - buildBitmapsFromLexer: this check shouldn't be needed:
> {{{#!cpp
> if (token.getType() != MasterToken::STRING) {
> isc_throw(InvalidRdataText,
> "Non-string token found when parsing key data");
> }
> }}}
> because of this:
> {{{#!cpp
> const MasterToken& token =
> lexer.getNextToken(MasterToken::STRING, true);
> }}}
> This version of getNextToken() ensures it returns an expected type
> of token (or EOF/EOL in this case) and throws an exception
> otherwise. See also the discussion on dnskey_48.cc above.
This check is now removed.
> '''dnskey_48.cc'''
>
> - constructFromLexer: this should be unnecessary:
> {{{#!cpp
> if (token.getType() != MasterToken::STRING) {
> isc_throw(InvalidRdataText,
> "Non-string token found when parsing key data");
> }
> }}}
> and same comments as buildBitmapsFromLexer applies.
This is also removed.
> - constructFromLexer: keydata_substr should better be defined outside
> of the `while` loop because the whole point is to skip
> re-initialization and possible allocation every time
> {{{#!cpp
> std::string keydata_str;
> std::string keydata_substr;
> while (true) {
> ...
> //std::string keydata_substr;
> token.getString(keydata_substr);
> }}}
This has been moved out.
> '''rdata_dnskey_unittest.cc'''
>
> - we should probably test extra trailing garbage (new line, comment)
> for the string version. same for NSEC3 and NSEC3PARAM
This is tested. Note that because of how we read it using the lexer, the
string constructor will accept a trailing comment, but will throw if any
actual extra garbage RDATA comes afterwards.
> - If this means to test operator=, I suspect it doesn't do it:
> {{{#!cpp
> TEST_F(Rdata_DNSKEY_Test, assign) {
> generic::DNSKEY rdata_dnskey2 = rdata_dnskey;
> EXPECT_EQ(0, rdata_dnskey.compare(rdata_dnskey2));
> }
> }}}
> (I guess it would use the copy constructor).
Nod. This has been updated too.
> - do we still need createFromLexer? Same for NSEC3.
I have left minimal tests in place, which check them at a slightly
different API layer.
> - toWireBuffer: mostly out of scope of this ticket, but adding a test
> case is easy here, so I've committed my suggested test.
Nod. Thank you.
>
> '''testdata'''
>
> In general, I'd suggest revising util/python/gen_wiredata (in this
> case so that it supports DNSKEY) specify the test data in the
> "spec file" format, and let the script convert it to .wire. Handmade
> wire data such as rdata_dnskey_empty_keydata_fromWire often contains
> errors or other garbage (due to naive copy-pasting) and generally
> difficult to maintain. Also, extending gen_wiredata is generally good
> in that we can use the extended feature for later tests.
This has been added, and the wire data has also been updated to use spec
files.
--
Ticket URL: <http://bind10.isc.org/ticket/2387#comment:20>
BIND 10 Development <http://bind10.isc.org>
BIND 10 Development
More information about the bind10-tickets
mailing list