BIND 10 #2387: support generic version of rdata::createRdata(text) in DNSKEY, NSEC3, NSEC3PARAM
BIND 10 Development
do-not-reply at isc.org
Thu Mar 28 22:38:04 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-20130402
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):
Replying to [comment:16 muks]:
> > '''dnskey_48.cc'''
> > - 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.
>
> I have implemented this. But I don't know of a way to test error cases
> (except data after end-of-line), as even delimited numbers show up as
> string tokens.
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=");
}}}
> > - 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.
> > - 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.
> > - 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.
> > - 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.
> > '''nsec3_50.cc'''
> > - this can be a reference:
> > {{{#!cpp
> > const string nexthash =
lexer.getNextToken(MasterToken::STRING).getString();
> > }}}
>
> I've added the reference now. (I'm still unsure about where we can use
> references when an object is returned such as the case above; I'll go
> through the C++ spec).
I've added a comment on this topic in the ticket you asked this before
(forgot the number; please search). Was that not clear enough?
> > (and the line is too long)
>
> The line was 80 columns long. There is nothing in our CodingGuidelines
> about max line length, so I assumed 80 columns. I've added this to the
> CodingGuidelines page now:
> http://bind10.isc.org/wiki/CodingGuidelines#Linelength
Actually, we use the magic number of 79 here. It's from the BIND 9
coding guidelines: http://bind10.isc.org/wiki/BIND9CodingGuidelines
I've updated the above wiki page accordingly.
(Personally, I don't care whether it's 79 or 80, but I think it's good
to be consistent with the BIND 9 style when it's basically a matter of
taste).
> > '''rdata_dnskey_unittest.cc'''
> >
> > - This case is now moot:
[...]
> > 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.
>
> These tests were not written with implementation in mind. It doesn't
They were, and I know that as it was me who wrote it:-) If you mean
whether unit tests should generally be written with specific
implementation in general, I personally think it can (blackbox-type
tests are also important depending on the context, but I don't think
unittests should also be a black box; rather sometimes we should
actively exploit the knowledge of specific implementation in unit
tests). But, in any case, now they are cleaned up, I'm fine with the
latest test code.
> > - 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.
>
> This test was actually identical (I didn't notice it before), so I've
> removed it now.
No, previously it had its own purpose. See the previous point. But
again, removing it at this stage is fine.
> > - 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.
>
> The existing code missed many test cases. I don't know what "as if it
> were developed test-driven" means here. If this is to say that
> `master` branch misses many tests and we should add them now to make up
> for it, I have done this and tested the branch for the cases I could
> think of. You are pointing some in the review, which will be added too.
I don't remember what I meant by that after some period since I made
the comment:-) But I guess what I intended by "as if..." was to write
tests for these cases and actually confirm they fail (either first see
an exception when we'd use EXPECT_THROW, or temporarily disable the
corresponding code), and then confirm the latest version actually
makes it pass.
> If this statement is implying that new code in this branch was not
> tested (with covered tests) as it was being written, there seems to be a
> misunderstanding. Is this gathered from the commit sequence?
At least this was not my intent. I didn't check the history
commit-by-commit that closely:-)
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.
'''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.
- 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);
}}}
'''rdata_dnskey_unittest.cc'''
- we should probably test extra trailing garbage (new line, comment)
for the string version. same for NSEC3 and NSEC3PARAM
- 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).
- do we still need createFromLexer? Same for NSEC3.
- toWireBuffer: mostly out of scope of this ticket, but adding a test
case is easy here, so I've committed my suggested test.
'''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.
--
Ticket URL: <http://bind10.isc.org/ticket/2387#comment:17>
BIND 10 Development <http://bind10.isc.org>
BIND 10 Development
More information about the bind10-tickets
mailing list