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:

 > 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.
 Given that the buffer is explicitly cleared at the start of each of those
 methods, I'm relaxed about the scope issue.  (In a past project I've had
 to do something similar to overcome performance limitations.)

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

 Looking at TSIGContextImpl::digestDNSMessage() in light of this, it does
 occur to me that there is no need to declare an !OutputBuffer object to
 hold the DNS message header (with the associated overhead of dynamic
 memory allocation): the data written to the buffer is a fixed length
 message header, so this could be constructed in an automatically allocated
 uint8_t array.  (A stand-alone version of writeUint16() can be found in
 src/lib/util/io_utilities.h)

 > 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.
 OK, although I would suggest that it would still be useful to have header
 file that defines these values.  This file - reflecting values enshrined
 in RFCs - would be distinct from the Message class.

 > 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...
 If you were to include this text - explaining exactly why a BADKEY is
 generated and the need for the received algorithm name - in the header
 comments of the TSIGKey constructor, that would answer my comment.

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