Opened 7 years ago

Closed 7 years ago

#2124 closed defect (fixed)

RFC 6594 for SSHFP

Reported by: vorner Owned by: muks
Priority: medium Milestone: Sprint-20120731
Component: libdns++ Version:
Keywords: Cc:
CVSS Scoring: Parent Tickets:
Sensitive: no Defect Severity: N/A
Sub-Project: DNS Feature Depending on Ticket:
Estimated Difficulty: 2 Add Hours to Ticket: 0
Total Hours: 0 Internal?: no

Description

The mentioned RFC defines new algorithms and key types for SSHFP. Our SSHFP reject these as out-of-range (which is annoying to me, as I happen to have the ECDSA public keys with few of my hosts). The solution for us is simple ‒ relax the checks, there's no special handling based on the algorithm or key type.

Anyway, as there may be more key types in the future, should we have the checks at all?

As it is simple and annoying, I put it to next-sprint-proposed.

Subtickets

Change History (27)

comment:1 Changed 7 years ago by shane

Or perhaps a midway solution, where we check the types we know but ignore others?

comment:2 follow-up: Changed 7 years ago by vorner

What do you mean, by ignore? Reject, or accept?

The (frustrating) problem I see is, because the code doesn't know the type exists, it rejects it. But if the check is just removed, everything works fine. I'm OK with allowing just the known ones, but once there's a new type, we'll need to extend the check again so the type accepts a valid key it is able to handle, just unwilling. I kind of don't see the advantage of having the check.

comment:3 in reply to: ↑ 2 Changed 7 years ago by shane

Replying to vorner:

What do you mean, by ignore? Reject, or accept?

The (frustrating) problem I see is, because the code doesn't know the type exists, it rejects it. But if the check is just removed, everything works fine. I'm OK with allowing just the known ones, but once there's a new type, we'll need to extend the check again so the type accepts a valid key it is able to handle, just unwilling. I kind of don't see the advantage of having the check.

I meant accept.

Basically, if we know about a given type, then we can check it, if we don't we just assume that it is correct.

comment:4 Changed 7 years ago by vorner

Well, that is kind of the same here. The SSHFP looks like:

<Key type> <Fingerprint type> <Hex data>

We don't do any checking of the hex data, as it can be mostly anything and it would be correct (I think). We check the key type and fingerprint type (both are represented as number in the input) are in range of known types. That's it. So if it is unknown, we can either reject or accept it, but there are no more check we would do.

comment:5 Changed 7 years ago by shane

Ah... from that point of view it makes no sense, and sure, we should probably just remove the check.

comment:6 Changed 7 years ago by jinmei

BIND 9 accepts any u_int8 integer (i.e. doesn't do any check in this context). I think we should do the same.

comment:7 Changed 7 years ago by jelte

  • Milestone changed from Next-Sprint-Proposed to Sprint-20120731

comment:8 Changed 7 years ago by muks

  • Owner changed from UnAssigned to muks
  • Status changed from new to assigned

Picking

comment:9 Changed 7 years ago by muks

  • Owner changed from muks to UnAssigned
  • Status changed from assigned to reviewing

Up for review.

comment:10 Changed 7 years ago by jinmei

  • Owner changed from UnAssigned to jinmei

comment:11 follow-up: Changed 7 years ago by jinmei

  • I suspect it now allows >255 textual values. We need test cases for them, and hey should be rejected correctly.
  • I'd add tests for the "from wire" cases (against some uncommon algorithms/types)
  • We need a changelog for this fix.
  • I made some (unrelated) style cleanups.
  • I'd make lines like this shorter:
        EXPECT_NO_THROW(const generic::SSHFP rdata_sshfp("0 1 123456789abcdef67890123456789abcdef67890"));
    
    So they won't be too long or we won't have to fold them. For the purpose of the test, the fingerprint should be as simple as "1234" etc.
  • I'm not sure if this change is a good one:
    -    EXPECT_THROW(const generic::SSHFP rdata_sshfp("1 2 foo bar"), InvalidRdataText);
    +    EXPECT_THROW(const generic::SSHFP rdata_sshfp("1 2 foo bar"), isc::BadValue);
    
    In some sense it makes sense as it correctly catches what the test intends to catch (the bad part is "foo bar", and the hex decoder throws BadValue?). But from the caller's point of view, it should be more convenient and consistent if any bogus textual input generally results in InvalidRdataText. In that sense I guess the better fix is to catch BadValue from decodeHex() and convert it to InvalidRdataText.

comment:12 Changed 7 years ago by jinmei

  • Owner changed from jinmei to muks

comment:13 in reply to: ↑ 11 ; follow-up: Changed 7 years ago by muks

  • Owner changed from muks to jinmei

Replying to jinmei:

  • I suspect it now allows >255 textual values. We need test cases for them, and hey should be rejected correctly.

True. Added. :)

  • I'd add tests for the "from wire" cases (against some uncommon algorithms/types)

Done. :)

  • We need a changelog for this fix.

How does this look:

+XXX.   [bug]           muks
+       SSHFP's algorithm and fingerprint type checks have been relaxed
+       such that they will accept any values in [1,255]. This is so that
+       future algorithm and fingerprint types are accomodated.
+       (Trac #2124, git ...)
+
  • I made some (unrelated) style cleanups.
  • I'd make lines like this shorter:
        EXPECT_NO_THROW(const generic::SSHFP rdata_sshfp("0 1 123456789abcdef67890123456789abcdef67890"));
    
    So they won't be too long or we won't have to fold them. For the purpose of the test, the fingerprint should be as simple as "1234" etc.

Done. :)

  • I'm not sure if this change is a good one:
    -    EXPECT_THROW(const generic::SSHFP rdata_sshfp("1 2 foo bar"), InvalidRdataText);
    +    EXPECT_THROW(const generic::SSHFP rdata_sshfp("1 2 foo bar"), isc::BadValue);
    
    In some sense it makes sense as it correctly catches what the test intends to catch (the bad part is "foo bar", and the hex decoder throws BadValue?). But from the caller's point of view, it should be more convenient and consistent if any bogus textual input generally results in InvalidRdataText. In that sense I guess the better fix is to catch BadValue from decodeHex() and convert it to InvalidRdataText.

Fixed. :)

comment:14 in reply to: ↑ 13 ; follow-up: Changed 7 years ago by jinmei

Replying to muks:

I don't think we should reject 0 for algorithm and type. The RFC
doesn't require it, and BIND 9 (which we generally try to be
compatible with if there's specific reason not to do so) doesn't
reject them.

  • I'd add tests for the "from wire" cases (against some uncommon algorithms/types)

Done. :)

Probably beyond the scope of this task, but now that you've deleted
"TBD": we still need some more tests, e.g., short data.

Also, you don't necessarily have to explicitly specify parameters of
test data if you simply use the default values. From a quick look you
can omit many of them.

  • We need a changelog for this fix.

How does this look:

+XXX.   [bug]           muks
+       SSHFP's algorithm and fingerprint type checks have been relaxed
+       such that they will accept any values in [1,255]. This is so that
+       future algorithm and fingerprint types are accomodated.
+       (Trac #2124, git ...)
+

Looks okay, except that it would need to be adjusted if we allow
values of 0.

comment:15 Changed 7 years ago by jinmei

  • Owner changed from jinmei to muks

comment:16 in reply to: ↑ 14 ; follow-up: Changed 7 years ago by muks

  • Owner changed from muks to jinmei

Replying to jinmei:

I don't think we should reject 0 for algorithm and type. The RFC
doesn't require it, and BIND 9 (which we generally try to be
compatible with if there's specific reason not to do so) doesn't
reject them.

The RFC marked it as reserved (and given 1-255 available for use), I didn't think it would get used. I have relaxed the check now to allow 0 also.

  • I'd add tests for the "from wire" cases (against some uncommon algorithms/types)

Done. :)

Probably beyond the scope of this task, but now that you've deleted
"TBD": we still need some more tests, e.g., short data.

A short fingerprint data test has been added.

Also, you don't necessarily have to explicitly specify parameters of
test data if you simply use the default values. From a quick look you
can omit many of them.

Updated.

  • We need a changelog for this fix.

How does this look:

+XXX.   [bug]           muks
+       SSHFP's algorithm and fingerprint type checks have been relaxed
+       such that they will accept any values in [1,255]. This is so that
+       future algorithm and fingerprint types are accomodated.
+       (Trac #2124, git ...)
+

Looks okay, except that it would need to be adjusted if we allow
values of 0.

Nod. It'll be modified when it's committed.

Last edited 7 years ago by muks (previous) (diff)

comment:17 in reply to: ↑ 16 ; follow-up: Changed 7 years ago by jinmei

Replying to muks:

Probably beyond the scope of this task, but now that you've deleted
"TBD": we still need some more tests, e.g., short data.

A short fingerprint data test has been added.

Ah, sorry, I was not clear enough. What I actually meant was
something like the case where the actual data length is shorter than
the given RDATA length (consider, e.g, what if rdata_len = 65535 or
the buffer is even empty, etc. )

And, talking about these I also wonder what if the fingerprint is
empty, both from string and wire cases. BIND 9 seems to allow that.
If we accept it, we'll also make sure that it can be handled correctly
in towire and totext.

comment:18 Changed 7 years ago by jinmei

  • Owner changed from jinmei to muks

comment:19 in reply to: ↑ 17 Changed 7 years ago by muks

  • Owner changed from muks to jinmei

Replying to jinmei:

Replying to muks:

Probably beyond the scope of this task, but now that you've deleted
"TBD": we still need some more tests, e.g., short data.

A short fingerprint data test has been added.

Ah, sorry, I was not clear enough. What I actually meant was
something like the case where the actual data length is shorter than
the given RDATA length (consider, e.g, what if rdata_len = 65535 or
the buffer is even empty, etc. )

And, talking about these I also wonder what if the fingerprint is
empty, both from string and wire cases. BIND 9 seems to allow that.
If we accept it, we'll also make sure that it can be handled correctly
in towire and totext.

Some more tests and some missing unit tests have been added now. Code has been changed appropriately.

comment:20 follow-up: Changed 7 years ago by jinmei

Not yet looking into it, but some tests failed:

Rdata_SSHFP_Test.emptyFingerprintFromWire
rdata_sshfp_unittest.cc:191: Failure
Value of: rdf.getSSHFPAlgorithmNumber()
  Actual: \0
Expected: 4
rdata_sshfp_unittest.cc:192: Failure
Value of: rdf.getSSHFPFingerprintType()
  Actual: \0
Expected: 9
rdata_sshfp_unittest.cc:193: Failure
Value of: rdf.getFingerprintLen()
  Actual: 3458755740794340403
Expected: 0
[  FAILED  ] Rdata_SSHFP_Test.emptyFingerprintFromWire (2 ms)

comment:21 in reply to: ↑ 20 ; follow-up: Changed 7 years ago by jinmei

Replying to jinmei:

Not yet looking into it, but some tests failed:

Identified the issue and fixed it. One more thing: we cannot use
vector[0] if its size is 0 (try replacing it with at(0) and see
what happens).

I'll review the rest of the code later.

comment:22 Changed 7 years ago by jinmei

  • Owner changed from jinmei to muks

comment:23 in reply to: ↑ 21 Changed 7 years ago by muks

  • Owner changed from muks to jinmei

Replying to jinmei:

Replying to jinmei:

Not yet looking into it, but some tests failed:

Identified the issue and fixed it.

Eek.. that was a bad one.

One more thing: we cannot use
vector[0] if its size is 0 (try replacing it with at(0) and see
what happens).

I've fixed the code for it. Also added some missing tests of existing code.

comment:24 follow-up: Changed 7 years ago by jinmei

I have one last thing to discuss. (Unlike the "from text" case) I
wouldn't catch buffer exception in the from wire constructor at this
level:

    try {
...
    } catch (const isc::util::InvalidBufferPosition& e) {
        isc_throw(InvalidRdataLength,
                  "SSHFP record shorter than RDATA len: " << e.what());
    }

because in the case of from wire we normally use try-catch for a
higher level like "Message::fromWire", and since from wire
construction is relatively performance sensitive, and depending on the
implementation of the exception mechanism simply doing try-catch can
be expensive, and since (as far as I understand it) we don't catch
these errors within RDATA implementations for other types.

And I have one minor suggestion: in SSHFP::compare() the condition
could be a bit simplified:

    if (cmplen > 0) {
        const int cmp = memcmp(&fingerprint_[0], &other_sshfp.fingerprint_[0],
                               cmplen);
        if (cmp != 0) {
            return (cmp);
        }
    }
    return ((this_len == other_len)
            ? 0 : (this_len < other_len) ? -1 : 1);

then we can reduce the number of the ugly nested ?:. In cases like
this I'd even avoid using ?: completely (I personally often use ?: if
by doing so we can make a variable const, but in this case there's no
such advantage).

But all of these are pretty minor. I'd leave it to you.

Finally, I've made a few editorial/style changes. Some may need
discussions but to safe time I've directly committed these. Please
check.

And really finally: I think we should be able to use the generic .spec
file with the generator script for the added test data. Then we
wouldn't have the style issues (basically copy-paste problems) I fixed
in my commits. But we've already spent too much time for this
supposedly simple ticket, so I don't suggest changing them at this
stage.

comment:25 Changed 7 years ago by jinmei

  • Owner changed from jinmei to muks

comment:26 in reply to: ↑ 24 Changed 7 years ago by muks

Replying to jinmei:

I have one last thing to discuss. (Unlike the "from text" case) I
wouldn't catch buffer exception in the from wire constructor at this
level:

    try {
...
    } catch (const isc::util::InvalidBufferPosition& e) {
        isc_throw(InvalidRdataLength,
                  "SSHFP record shorter than RDATA len: " << e.what());
    }

because in the case of from wire we normally use try-catch for a
higher level like "Message::fromWire", and since from wire
construction is relatively performance sensitive, and depending on the
implementation of the exception mechanism simply doing try-catch can
be expensive, and since (as far as I understand it) we don't catch
these errors within RDATA implementations for other types.

Done. :)

And I have one minor suggestion: in SSHFP::compare() the condition
could be a bit simplified:

    if (cmplen > 0) {
        const int cmp = memcmp(&fingerprint_[0], &other_sshfp.fingerprint_[0],
                               cmplen);
        if (cmp != 0) {
            return (cmp);
        }
    }
    return ((this_len == other_len)
            ? 0 : (this_len < other_len) ? -1 : 1);

then we can reduce the number of the ugly nested ?:. In cases like
this I'd even avoid using ?: completely (I personally often use ?: if
by doing so we can make a variable const, but in this case there's no
such advantage).

Done. :)

But all of these are pretty minor. I'd leave it to you.

Finally, I've made a few editorial/style changes. Some may need
discussions but to safe time I've directly committed these. Please
check.

The .size() to .empty() change looks fine, and I agree.

And really finally: I think we should be able to use the generic .spec
file with the generator script for the added test data. Then we
wouldn't have the style issues (basically copy-paste problems) I fixed
in my commits. But we've already spent too much time for this
supposedly simple ticket, so I don't suggest changing them at this
stage.

I'll leave them as raw wiredata, as it doesn't hurt. There are other tests that don't use .spec files as well.

comment:27 Changed 7 years ago by muks

  • Resolution set to fixed
  • Status changed from reviewing to closed

Merged to master in commit 49e6644811a7ad09e1326f20dd73ab43116dfd21:

* 2c86895 [2124] Don't catch InvalidBufferPosition when parsing wiredata
* 9b88b6b [2124] Rewrite code to avoid duplication
* 5a7c29f [2124] some editorial fixes to test data comments
* d2355e5 [2124] prefer empty() over size() > 0 (see the previous commit)
* a09cb3f [2124] some suggested changes: - use !empty() instead of size() > 0
* 4017593 [2124] Rename SSHFP getter methods
* d71f7e9 [2124] Add another toWire() test
* 73080d9 [2124] Don't add trailing space in toText() if fingerprint is empty
* 595b447 [2124] Don't access fingerprint_ data when size is 0
* 26401e4 [2124] don't keep a reference to a temporary object.
* 31477bf [2124] Test toWire() with empty fingerprint data
* 3b82a4f [2124] Handle empty SSHFP fingerprints
* 16cd39d [2124] Add wiredata tests where the record is shorter than rdata len indicates
* d75a744 [2124] Add another wiredata test with short fingerprint data
* 6efceee [2124] Remove default values from wiredata spec files
* 67872ac [2124] Allow algorithm=0 and fingerprint-type=0
* 5e7af00 [2124] Add some more wire data tests
* 25e3471 [2124] Add more tests (check >255)
* 63c345f [2124] Catch isc::BadValue and throw InvalidRdataText instead
* 2d714ad [2124] Shorten rdata
* d0d70f2 [2124] Check that algorithm and fingerprint are in the range [1,255]
* 045ede2 [2124] Indent code according to our coding style
* d448a7e [2124] trivial style fixes: folded long lines; brace position
* 50d012e [2124] Relax algorithm and fingerprint type checks for SSHFP

Resolving as fixed. Thank you for the reviews Jinmei.

Note: See TracTickets for help on using tickets.