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      |
-------------------------------------+-------------------------------------

Comment (by jinmei):

 Thanks for the prompt review!  I really didn't expect it to be that
 fast:-)

 Replying to [comment:9 stephen]:
 >
 > '''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?

 Yeah, this is one of the points I was not sure.  I chose
 getCryptoAlgorithm() to make it clear that it would be used for
 libcryptolink stuff.  But since that might also be clear from the
 return type, I'm fine with changing it to getAlgorithm() if you prefer
 it.

 > (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].)

 I've tried to clarify that with added comments (commit daf48e8),
 please check.

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

 This is another thing I was not sure.  I had checked the BIND 9
 implementation (it has its own "buffer" module for 48-bit in/out), and
 had found that TSIG is the only use case for the 48-bit interface.  So
 I hesitated to make the generic class more complicated only for one
 single use case.  I'm okay with adding it, but I'd personally hold off
 until we see at least one other use case.  And, in any case, I'd defer
 it to a separate ticket.

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

 You mean the following two, for example?
 {{{
     uint16_t getCode() const { return (code_); }

     bool equals(const TSIGError& other) const
     { return (code_ == other.code_); }
 }}}

 Basically, I (personally) follow the rule that
 - if everything can fit in a single line (up to 79 columns), do it
 - otherwise, create a new line

 (which I think is consistent with the BIND 9 coding guideline:
 http://bind10.isc.org/wiki/BIND9CodingGuidelines)

 But at the same time I love consistency:-) So, if you strongly want to
 see consistent line-break rule in this file, I don't mind changing it.

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

 Hmm...it didn't happen for me and I'm not sure why (I guess we both
 use Mac.  Maybe this is because I mainly use clang++?), but anyway,
 that would break building lib/util/unittests itself because in
 util/Makefile.am unittests is built before io.  I don't have an answer
 to your "unrelated" question (you should ask Jerry I guess), but
 that's probably related to this point.

 My suggestion at the moment is to have dns/tests link libutil_io.la
 and create a separate ticket to resolve the dependency problem in a
 cleaner way.  The current structure seems to be so fragile and I think
 we need some fundamental cleanup.  (This suggestion is not yet
 committed).

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

 Okay, I've added some comments (commit 78bd2b7)

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

 Fair enough, and that (about TSIGRecord) was actually what I thought.

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