BIND 10 #812: TSIG: Signing messages

[BIND 10 Development]

#812: TSIG: Signing messages
-------------------------------------+-------------------------------------
                   Reporter:         |                 Owner:  jinmei
  stephen                            |                Status:  reviewing
                       Type:         |             Milestone:
  enhancement                        |  Sprint-20110503
                   Priority:         |            Resolution:
  blocker                            |             Sensitive:  0
                  Component:         |           Sub-Project:  DNS
  Unclassified                       |  Estimated Difficulty:  8.0
                   Keywords:         |           Total Hours:  0
            Defect Severity:  N/A    |
Feature Depending on Ticket:  tsig   |
        Add Hours to Ticket:  0      |
                  Internal?:  0      |
-------------------------------------+-------------------------------------
Changes (by stephen):

 * owner:  stephen => jinmei


Comment:

 > first two commits can (or even should) be ignored. it simply
 incorporates a dependent branch (781) and makes minimal change to make the
 tree buildable.
 Ignored :-)

 > next three commits (adfd101..ad13479) are not directly related to TSIG
 signing, but make some adjustments to the existing TSIGKey class for the
 use of crypto API with TSIG. These are a separate chunk of changes.

 '''src/lib/dns/tsigkey.{cc,h}'''[[BR]]
 The method that returns the hash algorithm name is called
 getAlgorithmName(), but the (new) method that returns the hash algorithm
 itself is called getCryptoAlgorithm().  Wouldn't getAlgorithm() be more
 consistent?


 (Review of differences between ad13479 and fd45c40)

 '''src/lib/dns/tsig.{cc,h}'''[[BR]]
 In TSIGContext::sign(), suggest adding a comment to the effect that TSIG
 uses a 48-bit time before the statement masking gettimeofdayWrapper with a
 constant. (It's not clear why the value is masked unless you really study
 [http://tools.ietf.org/html/rfc2845 RFC 2845].)

 In TSIGContext::sign(), the 48-bit time is written using writeUint16().
 followed by writeUint32().  For completeness (and for possible extensions
 in the future), is there a case for adding writeUint48() and writeUint64()
 (and the corresponding read functions) to the library?


 '''src/lib/dns/tsigerror.{cc,h}'''[[BR]]
 Inconsistent layout in the header file: the body of some methods is on the
 line after the function signature; for other methods it is on the same
 line.

 '''src/lib/util/unittests/Makefile.am'''[[BR]]
 Need to add src/lib/util/io/libutil_io.la to the link command line (else
 the dns tests fail to link).  As an aside (and not related to this
 change), is there any reason why there is a separate util/io library
 rather than the "io" code being included in the main util library?

 '''src/lib/util/unittests/newhook.{cc,h}'''[[BR]]
 I think that the ability to do a memory allocation failure test is a
 useful addition to our test suite.  In the comments in the code, I suggest
 emphasising that "throw_size_on_new" is the exact size that causes an
 exception to be thrown rather than the minimum size at which an exception
 is thrown.  The comments mention memory allocation failure, which suggests
 that the number is a limit above which an allocation request fails.

 '''Comments on the Design'''
 TSIGContext: the decision to make this class independent from Message
 seems sound.

 TSIGRecord: I admit that my first impulse would have been to derive the
 class from AbstractRRSet and implement restrictions on setTTL().  However,
 I accept the argument that TSIG is a very special RR type and so will only
 be used in special circumstances.  If circumstances change, we should be
 prepared to re-visit this decision.

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