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      |
-------------------------------------+-------------------------------------
Changes (by stephen):

 * owner:  stephen => jinmei


Comment:

 Reviewing differences between 34c2d5b (first commit after branch trac813
 has been merged in) and latest revision (b734c55).


 '''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?

 '''src/lib/dns/tests/tsig_unittest.cc'''[[BR]]
 ... and associated testdata files.  Only checked that the tests seemed
 reasonable - did not check the numbers in the tests.


 '''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.)

 Very comprehensive and instructive header for the verify() method - thank-
 you.

 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?

 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?

 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.


 '''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?

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