> On 25 Jun 2019, at 10:07, internet-dra...@ietf.org wrote: > > > A New Internet-Draft is available from the on-line Internet-Drafts > directories. > This draft is a work item of the Domain Name System Operations WG of the IETF. > > Title : Secret Key Transaction Authentication for DNS (TSIG) > Authors : Francis Dupont > Stephen Morris > Paul Vixie > Donald E. Eastlake 3rd > Olafur Gudmundsson > Brian Wellington > Filename : draft-ietf-dnsop-rfc2845bis-04.txt > Pages : 26 > Date : 2019-06-25
Back in March, Martin Hoffman did a comprehensive review of the RFC2845bis draft and made a number of very good suggestions for improvements to its readability. I've edited the draft to take account of his comments, something that has had a significant effect on its structure. For this reason, I'd appreciate if reviewers could cross-check against RFCs 2845 and 4635 to ensure that nothing got lost in the process. Martin's email - and a description of the changes made in response to it - are below. Stephen > 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? Accepted. The introduction section has been rewritten to include a background section, a protocol overview and finally the reasons why the document has been written. > > 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. As this is a replacement for RFC 2845, I think explicitly listing the numbers is important as it clearly indicates that this document is responsible for defining them. > > 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. Accepted and so it appears before any detailed protocol description. > > 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. Accepted. The section has been included in the description of the TSIG RR. > > 4.3. TSIG Record Format > > o The "NAME" is normally called the "owner". I'm not sure about this - this is really a key name and not a domain name in the normal sense. > > 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. Accepted. > o "Octet stream" for a field sounds wrong; "octet string" would be more > appropriate but is also misleading, maybe "octet sequence"? I've used "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. The choice of a "Fudge" value is a bit subjective, so I think it should remain in the security consideration section. > > 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 ...) I hope that the restructuring of the document has made this clearer... > > 4.4. Example > > o I am not sure this section provides any value in its current form. Agreed, I've removed the example. > > > 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. I agree. My first attempt at restructuring reduced the "Protocol Operation" to three paragraphs, but the more I thought about it, the more I realised that it more or less repeated what was in the "Protocol Details" section. I've since removed the "Protocol Operation" section - everything is now in "Protocol Details". > > 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. This has been made clearer. > > 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. This has been explicitly stated. > 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. Accepted and changed. > > 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. Accepted and changed. > > 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. The text has been restructured, so I hope this is clearer. > > 5.4.2. TSIG Variables > > o A short intro what these are and why we need them would be nice. I've added the sentence "Adding this data provides further protection against an attempt to interfere with the message." > > 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? I don't think so, as the reference referred to talks about avoiding other message types, so this has been changed to refer to message type 00 only. > > 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. The components are now described in the order in which they appear. > > 5.5. Component Padding > > o This can be merged into the introduction to 5.4; it does talk about > network byte order already. Done. > > 6.1. TSIG Generation on Requests > > o The client must also remember the key used, not only the MAC. Good point, accepted. > > 6.2. TSIG on Answers > > o Should this rather be "TSIG Generation on Answers" for consistency? > Same goes for 6.3 and 6.4. The sections have been renamed to be more consistent. > > 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. I think this is perhaps unnecessary as the first paragraph explicitly states that a signed error message MUST NOT be sent back if the MAC fails to validate. > > 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. Done. > > 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? This is now explicitly stated. > > 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. This is now explicitly stated. > > 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? Suggestion accepted. > > 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. Suggestion accepted. > > 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? Good question: thoughts? > > o What Fudge should the server use in its BADTIME response? I would presume that the Fudge field is not used when verifying the error response so is irrelevant. However, is should be specified. Thoughts? > > 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? It has to happen some time, and the order is now explicitly defined in section 5.2. > > 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". Added a mention that the MAC should be compared with that received. > > 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. This is an implementation issue and does not really form part f this document. > > 6.6.1. Key Error Handling > > o I don’t understand this section at all. I've tried to rephrased it, but I think it is clear enough. > > 7. Algorithms and Identifiers > > o There is now two paragraphs listing what is mandatory and optional and > then there is also a table. The two paragraphs form an introduction to the 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? Sentence removed. > o Can the table be expanded to include pointers to where the algorithms > are defined? Probably, but not done in this iteration. > > 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. Now added to the paragraphs before 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. I've removed the bit about unknown/unimplemented algorithms which I think was the cause of the comment. > > Kind regards, > Martin > > _______________________________________________ > DNSOP mailing list > DNSOP@ietf.org > https://www.ietf.org/mailman/listinfo/dnsop _______________________________________________ DNSOP mailing list DNSOP@ietf.org https://www.ietf.org/mailman/listinfo/dnsop
