[Gen-art] Gen-ART Last Call review of draft-ietf-ntp-checksum-trailer-04

[Paul Kyzivat <[e-mail]>]

I am the assigned Gen-ART reviewer for this draft. The General Area 
Review Team (Gen-ART) reviews all IETF documents being processed by the 
IESG for the IETF Chair. Please treat these comments just like any other 
last call comments. For more information, please see the FAQ at 
<http://wiki.tools.ietf.org/area/gen/trac/wiki/GenArtfaq>.

Document: draft-ietf-ntp-checksum-trailer-04
Reviewer: Paul Kyzivat
Review Date: 2016-02-23
IETF LC End Date: 2016-03-01
IESG Telechat date: 2016-03-03

Summary:

This draft is on the right track but has open issues, described in the 
review.

Major: 2
Minor: 3
Nits:  1

(1) Major:

Section 3.3 references [NTP-Ext] (draft-ietf-ntp-extension-field) as 
justification for why existing implementations won't have any trouble 
with this extension. But some (most?) existing implementations won't 
comply with [NTP-Ext], but rather only with RFC5905. This section 
doesn't explain how *those* implementations will behave. ISTM there is 
some possibility that they won't behave well.

(2) Major:

Section 3.2 says:

      This length guarantees that the host that receives the packet
      parses it correctly, whether the packet includes a MAC or not.
      [NTP-Ext] provides further details about the length of an
      extension field in the absence of a MAC.

I am not reviewing [NTP-Ext], but I consulted it in an attempt to 
understand. I found this logic, and the references, to be confusing and 
seemingly contradictory:

In [NTP-Ext] I find:

    ... However, a MAC is
    not mandatory after an extension field; an NTPv4 packet can include
    one or more extension fields without including a MAC (Section 7.5 of
    [RFC5905]).

While section 7.5 of [RFC5905] contradicts that:

    In NTPv4, one or more extension fields can be inserted after the
    header and before the MAC, which is always present when an extension
    field is present.

The text that allows extension fields without a MAC is an update to 
RFC5905 in section 3 of [NTP-Ext]. But that isn't very clear about how 
one is expected to decipher this. I *think* the intended logic is:

    If the packet length is more that 24 bytes in excess of the size
    of a packet with no extensions or MAC, then it must contain at
    least one extension. In that case, process extensions until the
    remaining size of the packet is less or equal to 24. Then, if bytes
    remain process them as a MAC.

IMO this draft should not progress until this is specified *somewhere*, 
probably in [NTP-Ext].

[IMO this technique is a horrible hack. I hope there was no better 
backward compatible alternative. But this comment isn't a formal part of 
this review.]

(3) Minor:

Section 3.2 says:

The extension field includes 22 octets of padding. This field
SHOULD be set to 0, and SHOULD be ignored by the recipient.

In my experience SHOULD is dangerous when not explained. I would 
recommend either changing it to MUST or providing an explanation of 
conditions that would justify violating the SHOULD.

(4) Minor:

While section 3.2 carefully defines the format of the Checksum 
Complement option, it never specifies how the content of that field is 
generated or used. An example of that calculation is shown in appendix 
A, but that isn't normative. A normative specification of the content of 
the field is needed. IIUC it is to simply be ignored by the recipient. 
It would be good to say that too.

(4) Minor:

This document has a normative reference to RFC5905 and an informative 
reference to [NTP-Ext]. But [NTP-Ext] has normative updates to RFC5905 
and this document has dependencies on those changes, so it needs a 
normative reference to that document.

(6) Nit/Question:

In section 3.1, IIUC the timestamp engine will need to know that the 
Checksum Complement field is present before it updates the packet. Is it 
assumed that engine will assume that to be so, and will not be invoked 
unless it is? It might be helpful to say something about this. If not, I 
don't see how it can work.