BIND 10 #893: TSIG verify, part 2

[BIND 10 Development]

#893: TSIG verify, part 2
-------------------------------------+-------------------------------------
                   Reporter:         |                 Owner:  jinmei
  jinmei                             |                Status:  reviewing
                       Type:         |             Milestone:
  enhancement                        |  Sprint-20110517
                   Priority:  major  |            Resolution:
                  Component:         |             Sensitive:  0
  DNSPacket API                      |           Sub-Project:  DNS
                   Keywords:         |  Estimated Difficulty:  3.0
            Defect Severity:  N/A    |           Total Hours:  0
Feature Depending on Ticket:         |
        Add Hours to Ticket:  0      |
                  Internal?:  0      |
-------------------------------------+-------------------------------------

Comment (by jinmei):

 Replying to [comment:7 stephen]:
 > Reviewing differences between 34c2d5b (first commit after branch trac813
 has been merged in) and latest revision (b734c55).

 Thanks for the review!

 '''src/lib/cryptolink/crypto_hmac.cc'''[[BR]]
 > verify(): This function is passed a pointer to the signature and the
 length of the signature.  I'm a bit uncertain about the temporary change
 which forces the length to a minimum value - can we guarantee that the
 signature passed in is that length?

 Ah, good point.  My original change was wrong and I've fixed it in
 commit d53bbaf.

 > '''src/lib/dns/tsig.{cc,h}'''[[BR]]
 > In the example in the constructor header, it could be clearer if it were
 explicitly mentioned that "ctx" was an object of type TSIGContext.
 >
 > Also in the example, suggest that the comment in the third branch be
 "Other error: discard response keep waiting with the same ctx for another
 one".  (Probably just me, but it was only after reading RFC2845 that I
 realised why there was this other branch.)

 Okay, I've updated the doc (in a slightly different way for the first
 point) in commit 9bd9fa8.

 > digestXxxx(): The comments say that buffer is just workspace and not
 used outside the digestXxxx methods.  Rather than declare it in the caller
 and pass it between methods, why not declare it as a class member?

 I actually considered that option, but didn't choose that because I
 thought it would be better to localize the scope of a mutable variable
 as much as possible.  This buffer is essentially local to sign/verify
 methods (or, as commented, even local to each helper method) and
 doesn't have to be valid over the lifetime of a single call to sign()
 or verify().  Since keeping a variable with mutable states for a
 longer term can easily be a source of bugs, I decided not to have it
 as a member variable.

 I admit in this sense the current implementation may look an
 incomplete.  Again, as commented we could make the buffer local to
 each helper method, and it may look a "premature optimization" to
 share the same variable for "efficiency reasons" especially when it's
 expected to be used during crypto operation (which would generally be
 more expensive than creating/destructing buffer objects) anyway.

 So, if you like, I'm okay with making the buffer a local variable of
 the helper methods.

 > digestPreviousMac(): previous_digest_.size() is a size_type and is
 converted to uint16_t (risking truncation).  Should we check that there is
 no truncation - or at least have an assert() to catch any such instance in
 testing?

 Okay, I've tried to make it clearer with assert() and changing some
 part of function parameter type when possible (commit 4787c28).

 > MESSAGE_HEADER_LEN is defined as 12.  I suggest that this - and related
 constants (e.g. offset of QID in the message, offset of the answer count
 etc.) - be defined in the Message class.  This gives one place for related
 constants.

 Actually, I intentionally avoided the dependency on the Message class
 even though I knew it was one possible approach.  I know it's a
 duplicate, which should generally be avoided, but at the same time I
 don't want to make the tsig implementation depend on a higher level
 notion of Message in terms of decoupling independent classes.  In this
 case, the necessary knowledge about the DNS message is minimal (as
 commented) and is quite unlikely to change, so I thought the
 disadvantage of duplicate is acceptable.

 > '''src/lib/dns/tsigkey.{cc,h}'''[[BR]]
 > TSIGKey constructor: I'm not clear under what circumstances an invalid
 algorithm name would be useful, even specified with an empty secret key.
 Why not prohibit any unknown algorithm name?

 Because we need to return a TSIG RR with BADKEY when we receive a TSIG
 with an "unknown" algorithm name.  That returned TSIG will have to
 include the unknown algorithm name, and in order to use the same
 interface for verify:

 {{{
         // "algorithm" may be unknown to the server, in which case the
         // context initialized with a BADKEY error.
         TSIGContext ctx(tsig->getName(), tsig->getRdata().getAlgorithm(),
                         keyring);

         // this will result in BADKEY
         ctx.verify(tsig, data, data_len);

         // this will send a response with a TSIG RR indicating BADKEY
         // and the algorithm name field being the "unknown" one.
         message.toWire(renderer, ctx);

         // (this works for the case of a valid key and sig, too)
 }}}

 I needed to allow TSIGKey to be constructed with an "unknown"
 algorithm.

 We could achieve this goal by introducing more branches based on error
 codes (i.e., by making TSIGContext implementation more complicated),
 so if you think the change to TSIGKey is too dangerous, we can discuss
 which one is "less worse".

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