[ntpwg] [Gen-art] Review: draft-ietf-ntp-autokey-05.txt

[Brian Haberman]

Joel,
      Thanks for the review.  At this point, I will not have time to 
look at these comments until later this week or possibly early next 
week.  When I do, I will provide responses to your comments.

Thanks,
Brian

Joel M. Halpern wrote:
> I have been selected as the General Area Review Team (Gen-ART)
> reviewer for this draft (for background on Gen-ART, please see
> http://www.alvestrand.no/ietf/gen/art/gen-art-FAQ.html).
> 
> Please resolve these comments along with any other Last Call comments
> you may receive.
> 
> Document: draft-ietf-ntp-autokey-05.txt
>     Network Time Protocol Version 4 Autokey Specification
> Reviewer: Joel M. Halpern
> Review Date: 5-June-2009
> IETF LC End Date: 12-June-2009
> IESG Telechat date: N/A
> 
> Summary: While nearly ready for publication as an Informational RFC, 
> this document has some issues that need to be addressed before 
> publication.  The issues are described by priority below.
> 
> Major issues:
>     There appears to be some confusion in the protocol description as to 
> which field actually tells you what message you are handling, and which 
> field tells you that this is NTP Autokey.  In teh text in section 10, it 
> says that the "Field Type" field is set to 2, and represents a version 
> number.  Further, it says that the 6-bit Code field specifies the 
> request or response.  Unfortunately, all of the subsections of section 
> 10 carefully and consistently refer to having different "Field Type" 
> values, not different Code values.
>     Section 4 of this document is explicit that MD5 is the (singular) 
> hash algorithm for generating message signatures and autokeys.  I was 
> concerned about lack of algorithm agility.  Section 11.1 however 
> includes a 16 bit Digest / Signature NID.
> 1) There is no description of how this is used / negotiated
> 2) There is no explanation of the relationship between this and the 
> earlier assertion of MD5
> 3) While the text mentions that this is defined in "the OpenSSL 
> library", there is no reference, and particularly no reference to any 
> documentation to see what values are defined.  Since this ought to be 
> implementable by someone who does not live in the OpenSSH world, a 
> reference is necessary.
> 4) There is no mention of what happens if the value proposed by the host 
> and the values supported by the server do not match.
> 
> Moderate issue:
> 
>     The IETF does not normally use "reference implementations." 
> Therefore, it would be helpful if early in this document there were text 
> describing what the reference implementation is, how it is found, and 
> what the relationship of that reference implementation to this RFC is.
> 
>     I believe that the wording on hierarchy in section 5, paragraph 3 is 
> exactly the opposite of what is shown in the figure.  (And what is shown 
> in the figure matches what I would expect.)  The text says:
>    Secure groups can be configured as hierarchies where a TH of one
>    group can be a client of one or more other groups operating at a
>    lower stratum.
> But in fact, what I think happens is that a client of one group operates 
> as the TH of a group operating at a lower stratum.
> 
>     Section 10, the paragraph that begins "[T]he extension field parser 
> initializes a pointer..." has some sort of problem.  After reading this 
> several times, I am pretty sure that "the length" by which the pointer 
> is increment is the length in the extension header, not the length 
> computed by subtracting the NTP header from the packet length.  If so, 
> please insert test (probably after the "greater than 20" check and 
> before the "not multiple of 4 check") that describes something like "The 
> extension header is found immediately after the NTP header.  The length 
> of the extension header is checked to ensure that this length plus the 
> 20 bytes for the MAC header does not exceed the calculated remaining 
> octets of the packet."
> 
> Minor issues:
> 
>     In figure 5, it would probably help the reader if the groups and 
> hosts were not named identically.  (Even just calling the groups 
> Alice-Group and Carol-Group would help.)
> 
>    In section 5, in the description of "[t]he steps in hiking the 
> certificate trails...", in step 1, in the second sentence, could you 
> please add language to make it clear that what the server is "exchanging 
> host names and negotiating..." with is the server from whom it is 
> getting information.  It took rereading the list to realize that this 
> was not about the servers initiating communication with the clients who 
> use them.  (Yes, I know that such initiation is impossible.  Which is 
> why an extra couple of words would help.)  Part of this is because 
> common terminology, and later usage in this document, is that the 
> machine in this role (the "server" who is starting to exchange host 
> names ...) is called the client, if I have parsed the protocol correctly.
> 
>     Is there any way section 8 could be moved earlier in the document? 
>  One of the problems reading the document is that various parts early in 
> the document assume properties of the system which have not been 
> described yet.
> 
>     I am not sure if this is an issue that should be described in the 
> security considerations section, or whether I am missing something that 
> helps.  There are multiple references to the need for Identity 
> mechanisms.  But those mechanisms are not part of this draft.  If I read 
> things properly, if identity verification is not used then a 
> man-in-the-middle can step into the stream, and assuming he can convince 
> clients (possibly by packet misdirection) to talk to him, he can produce 
> an apparently valid certificate chain and simply lie about the time. 
> (Yes, there is mention earlier of other aspects of NTP that moderate his 
> potential impact.  But if we did not care about this, then this document 
> would not exist.)
> 
> Nits/editorial comments:
>     It appears that section 11.6 (Security Considerations) is supposed 
> to be a top-level sections, with 11.7 and 11.8 as sub-sections of that.
> 
> 
> 
>