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
_______________________________________________
DNSOP mailing list
DNSOP@ietf.org
https://www.ietf.org/mailman/listinfo/dnsop
