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

[Paul Kyzivat <[e-mail]>]

Tal,

Please see inline.

On 2/25/16 1:37 AM, Tal Mizrahi wrote:
> Hi Paul,
>
> Many thanks for the thorough review.
>
> Please find my responses to the major comments below. The next version of the draft will address these comments, as well as the ones you specified as 'minor'.
>
>
>> (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.
>
> I agree that Section 3.3. should be clarified. An existing implementation (whether it complies to [NTP-Ext] or not) that receives a Checksum Complement has two options: (i) ignore it, or (ii) drop the packet. While (i) is interoperable with the current draft, (ii) is not. I suggest the following text for Section 3.3:
>
> "The behavior defined in this document does not impose new requirements on the reception of NTP packets beyond the requirements defined in [NTP-Ext]. Note that, as defined in [NTP-Ext], a host that receives an NTP message with an unknown extension field SHOULD ignore the extension field and MAY drop the packet if policy requires it. Thus, transmitters and intermediate nodes that support the Checksum Complement can transparently interoperate with receivers that are not Checksum-Complement-compliant, as long as these receivers ignore the Checksum Complement extension field. It is noted that existing implementations that discard packets with unknown extension fields cannot interoperate with transmitters that use the Checksum Complement."

Isn't this serious? IIUC in many (most?) cases the transmitters and 
intermediate nodes won't know if the receiver supports [NTP-Ext]. 
Inserting this for those that don't will deny them of the time they are 
looking for.

Shouldn't there be some discussion of how a decision can be made about 
when to use this in order to avoid breaking receivers?

>> (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.
>>
>
> Please note that this paragraph in RFC5905 was corrected by erratum 3627 (https://www.rfc-editor.org/errata_search.php?rfc=5905), and the corrected text is:
>
> In NTPv4, one or more extension fields can be inserted after the
> header and before the MAC, if a MAC is present. If a MAC is not
> present, one or more extension fields can be inserted after the
> header, according to the following rules:
> o If the packet includes a single extension field, the length of the
>    extension  field MUST be at least 7 words, i.e., at least 28 octets.
> o If the packet includes more than one extension field, the length of
>    the last extension field MUST be at least 28 octets. The length of
>    the other extension fields in this case MUST be at least 16 octets
>    each.

I missed the errata. I agree that it covers the issue.

>> 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.]
>
>
> I agree that this logic is far from intuitive, and is defined for the sake of backward compatibility.
> The text in [NTP-Ext] is:
>
>     If a MAC is not present, one or more extension fields can be inserted
>     after the header, according to the following rules:
>
>     o  If the packet includes a single extension field, the length of the
>        extension field MUST be at least 7 words, i.e., at least 28
>        octets.
>
>     o  If the packet includes more than one extension field, the length
>        of the last extension field MUST be at least 28 octets. The length
>        of the other extension fields in this case MUST be at least 16
>        octets each.
>
>
> I believe the current text in [NTP-Ext] is correct, and consistent with erratum 3627.
> Please let us know if you think this is not the case.

I think so. However the erratam also has exactly the text I was hoping 
to find describing how extensions and the mac are parsed out of the 
packet by the receiver, and I don't see that in [NTP-Ext]. In principle 
it is not necessary (it is an exercise for the reader to figure out how 
to do this) but having it ensures that developers get it right.

Is there a reason why [NTP-Ext] doesn't also pick that up from the 
erratam? (Of course that has no bearing on *this* draft.)

	Thanks,
	Paul

>> -----Original Message-----
>> From: Paul Kyzivat [mailto:[e-mail]]
>> Sent: Tuesday, February 23, 2016 10:41 PM
>> To: [e-mail]
>> Cc: General Area Review Team
>> Subject: [Gen-art] Gen-ART Last Call review of draft-ietf-ntp-checksum-trailer-
>> 04
>>
>> 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.
>
>