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:10 stephen]:
 > > 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.

 So, are you suggesting we should make the buffer local to helper
 methods, or are you okaying the current code?  I'm okay with either
 way.

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

 Hmm, I see.  But if we use the free function version of writeUint16 we
 need to update the intermediate 8 bytes separately, which would make
 the code look rather complicated (relatively).  Would you still
 suggest changing the code that way?  (If so, I'm okay with that.)

 As a related note, I saw comments in io_utilities.h saying that "It
 should really be moved into a separate library."  I'm not sure what it
 means, but does this perhaps mean this function version of
 readUint16() may be removed in future?

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

 Hmm, actually I also previously (not for this task) thought about
 having a separate header file to define numeric protocol constants
 such as Name::MAX_WIRE/MAX_LABELS, Message::DEFAULT_MAX_UDPSIZE.  I
 was not sure if such a separate header file was really useful and
 still am not so sure, but if you like I'll create a separate ticket
 for that (at least for considering it).  It's a different question
 whether we want to publicly define constants for things like the size
 of DNS header section or offsets to particular fields such as QID or
 ARCOUNT...I personally think they are too minor to be defined
 publicly.

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

 I added some more explanation (143b2c6).  Hopefully this addressed
 your concern.

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