[DNSOP] Review of draft-ieft-dnsop-rfc2845bis-03

[Martin Hoffmann <[e-mail]>]

Hi,

the following is a review of draft-ieft-dnsop-rfc2845bis-03. As an
implementer, let me say how much I appreciate this being a full revision on
RFC 2845 instead of just an update for the issues discovered. I strongly
believe that keeping the full specification in a single place by writing
revisions instead of a stream of updates greatly improves the quality of
software since it avoids people missing important implementation experience.

As someone who coincidentally is just in the middle of implementing TSIG
specifically, I have to admit I found RFC 2845 to be somewhat difficult to
read and understand. As such, I had hoped that the bis would attempt to
improve the overall clarity of the document. I have pointed out the
places that I believe to benefit from re-writing below and I am very much
willing to help with doing that.

Now on to some concrete points, in order of appearance.

1. Introduction

   o  The second paragraph seems misplaced. It describes why there is this
      particular revision. Shouldn’t that be towards the end of the
      introduction?

   o  Third paragraph: Give a short explanation what "the protocol described
      in [RFC 3007]" actually is?


3. New Assigned Numbers

   o  Is this necessary here? The RRTYPE is mentioned right after; the error
      codes might be better listed in 4.3 together with the error field.


4. TSIG RR Format

   o  It would be nice to have a section before this one that describing the
      overall protocol operation in more general terms so that a reader isn’t
      thrown in at the deep end.

4.2 TSIG Calculation

   o  This sections seems a bit unnecessary here. For one, it doesn’t
      actually talk about TSIG calculation. The noting that the record gets
      discarded can be merged into 4.1 or left for later. The note about
      network byte order is better added with the record data description
      below.

4.3. TSIG Record Format

   o  The "NAME" is normally called the "owner".

   o  Would it be possible to spell out things like "uint48_t" as "unsigned
      48 bit integer". Not everyone speaks C anymore. And even then,
      "uint48_t" isn’t a thing.

   o  "Octet stream" for a field sounds wrong; "octet string" would be more
      appropriate but is also misleading, maybe "octet sequence"?

   o  The guidance for choosing a value for "fudge" is currently buried in
      the security considerations. Perhaps it should be given right here
      instead.

   o  MAC. A little more on what that is what be nice. It should also say
      that how it is calculated depends on the particular message and refer
      to the section with the details for that. (This is what had me most
      fooled in RFC 2845 where these details are in a section called "TSIG
      Variables and Coverage", but we will come to that ...)

4.4. Example

   o  I am not sure this section provides any value in its current form.


5. Protocol Operation

   o  The delimitation between sections 5 and 6 isn’t quite clear. It would
      be nice to merge them and give it structure pretty much along the
      lines of what section 6 does now.

5.1. Effects of Adding TSIG to Outgoing Messages

   o  "If the TSIG record cannot be added without the message to truncate."
      It isn’t immediately quite clear that this is only relevant for a
      server responding. This should move to 6.2. and 6.6.

   o  There should also be a note here that when creating TSIGs, there must
      be only one and it must be the last record. While this can be inferred
      from 5.2., it should perhaps be explicit here.

5.3.  Time Values Used in TSIG Calculations

   o  This should move into 5.4 as a sub-section since it actually describes
      one of the things used for digest calculation.

5.4.  TSIG Variables and Coverage

   o  This section should be renamed to something like "MAC Computation". It
      should be made clear that the things described are the components of
      input. It might be worthwhile to mention the various types of digests
      here already.

5.4.1.  DNS Message

   o  The first sentence is specific to digest creating and ignores that the
      calculation also happens for digest verification. The second sentence
      then suddenly talks about something that happens in verification only.

5.4.2.  TSIG Variables

   o  A short intro what these are and why we need them would be nice.

   o  For canonicalisation of label types, the reference for normal labels
      would be more helpful if it were to to section 6.1 of RFC 4034. Do we
      really need to mention label type 01?

5.4.3.  Request MAC

   o  The request MAC goes first in the actual digest but is described here
      last. Also, there is mention of "Prior MAC" later on. That should be
      merged in here.

5.5.  Component Padding

   o  This can be merged into the introduction to 5.4; it does talk about
      network byte order already.

6.1.  TSIG Generation on Requests

   o  The client must also remember the key used, not only the MAC.

6.2.  TSIG on Answers

   o  Should this rather be "TSIG Generation on Answers" for consistency?
      Same goes for 6.3 and 6.4.

6.3.  TSIG on TSIG Error Returns

   o  The digest components include "Request MAC (if the request MAC
      validated)". That seems incredibly vague. After all, the client needs
      to know if it was included or not in order to validate the MAC. So it
      should be spelled out when and when not a Request MAC is included.
      
6.4.  TSIG on Zone Transfer Over a TCP Connection

   o  That the last digest component here is "TSIG Timers" and not "TSIG
      Variables" totally ran by me. This should probably be called out
      explicitly.

6.5.  Server TSIG checks

   o  I am missing here guidance on what to do if the TSIG record is broken.
      It does say I have to include a TSIG if there is one in the request,
      but what if it is so broken that I can’t even synthesise a meaningful
      one? Is a FORMERR without TSIG okay? Do I throw a broken TSIG back?
      Drop the request on the floor?

6.5.1.  Key Check and Error Handling

   o  Section 8 contains some guidance with regards to unacceptable
      algorithms that might better be placed here. When implementing, I was
      wondering what to do if the algorithm is wrong.

6.5.2.1.  Specifying Truncation

   o  All mention of shortened digests should probably be called "MAC
      Truncation" everywhere to avoid confusion with DNS message truncation.
      Or perhaps a better term could be found altogether?

   o  I would flip items 3 and 4. This would allow item for to simply begin
      with "Otherwise" and not have an awkward forward reference.

6.5.3.  Time Check and Error Handling

   o  An actual protocol question: What is the point of the caching the last
      Time Signed per key and rejecting earlier messages? What about
      reordering of messages as can happen with UDP?

   o  What Fudge should the server use in its BADTIME response?

6.5.4.  Truncation Check and Error Handling

   o  Is this happening this late on purpose? Or could this be another item
      in 6.5.4.? If it is on purpose, perhaps add a note why?

6.6.  Client Processing of Answer

   o  "The client then extracts the TSIG, adjusts the ARCOUNT,
      and calculates the MAC in the same way as the server," here add "and
      compares it to the MAC included in the TSIG".

   o  Speaking of which: it would be good to mention the need to compare
      MACs in constant time. Because of the digest being constructed from
      multiple parts, it isn’t always practical to leave all of this
      business to the underlying crypto library but rather do the
      calculation and comparison manually.

6.6.1.  Key Error Handling

   o  I don’t understand this section at all.

7.  Algorithms and Identifiers

   o  There is now two paragraphs listing what is mandatory and optional and
      then there is also a table.
      
   o  Why is the "current HMAC-MD5.SIG-ALG.REG.INT [...] included [...] for
      convenience"? Since it is mandatory, isn’t its presence kind of
      important?

   o  Can the table be expanded to include pointers to where the algorithms
      are defined?

   o  The note about SHA-1 truncated to 96 bits should be part of the
      description of the algorithms above. It is easy to overlook trailing
      under the table.

8.  TSIG Truncation Policy

   o  The first paragraphs isn’t really about truncation but rather about
      keys and algorithms and stuff. It is probably better placed
      in the protocol description.

Kind regards,
Martin