Subject: [Gen-art] Gen-ART Last Call review of draft-ietf-behave-ipfix-nat-logging-06

From: Paul Kyzivat <[email-masked]>
Date: Mon, 1 Feb 2016 15:53:08 -0500

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-behave-ipfix-nat-logging-06
Reviewer: Paul Kyzivat
Review Date:
IETF LC End Date: 2016-02-12
IESG Telechat date:

Summary:

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

Major: 3
Minor: 5
Nits:  1

(Note: I've used Major for anything that is ambiguous, and Minor for 
things that are just unclear.)

(1) Major:

Section 5.2 starts with "The templates could contain a subset of the 
Information Elements(IEs) shown in Table 1 depending upon the event 
being logged."

This is not a normative statement. It isn't clear what is normative 
regarding the use and content of templates. If I understand RFC7011, a 
NAT device can use any number of templates, and those templates can 
reference any defined IE. Is *this* document intended to *restrict* the 
form and number of templates used for logging NAT devices? Or is it 
simply suggesting some templates that may be modified as desired to fit 
the needs of a particular NAT device device?

These templates do not have any Information Element that uniquely 
identifies to the IPFIX collector that this template is being used. So 
how does the collector know that the particular event is intended to 
follow the definitions in this draft, rather than simply some 
proprietary template? Absent that, how do normative statements of what 
must be in the template mean anything?

(2) Major:

As I understand it, Section 5.3 is defining valid values for the 
"natEvent" IE. That Information Element is already defined in the IANA 
registry, with values 1-3 that seem to correspond in semantics (but not 
name) to the first three values in Table 2. So I gather the intent is 
for Table 2 to replace what is currently in the registry for natEvent.

But I find nothing in the IANA Considerations section that calls for 
updating that entry in the registry. The IANA Considerations section 
needs to request a revision to the definition of this element.

(3) Major:

Section 5.6 says: "Depending on the implementation and configuration 
various IE's specified can be included or ignored."

What is the normative intent of this statement? Is it defining what is 
meant by the "Mandatory" field in the tables? (I.e., that in the 
templates it sends the NAT device MUST include fields with Mandatory=Yes 
but MAY omit fields with Mandatory=No.) This should be revised to make 
the normative behavior clearer.

(4) Minor:

The first sentence of Section 3 says: "This document focuses exclusively 
on the specification of IPFIX IE's." But this statement appears to be 
false. A large part of the document (Section 5.6) specifies Templates. 
It appears to be an important aspect of the document that goes beyond 
specifying just IEs. So the statement should be expanded.

(5) Minor:

The first paragraph of Section 5 has a reference to Section 4.1, but 
there is no such section in the document. Did this mean to refer to 
Table 2 in section 5.3?

(6) Minor:

Section 5.4 defines a set of values that are clearly intended to be 
conveyed in some IE. It calls them "sub event types for the Quota 
exceeded or Limits reached event". It does not name the IE. By 
examination of the templates I finally figured out that these are values 
for the "natLimitEvent" IE. This needs to be specified. (It is mentioned 
in the IANA considerations, but that is too late to help the reader.)

Also, is this list of values intended to be extensible? Should the list 
be in the IANA registry, with Expert Review for additions to it? Or is 
it expected that this RFC will need to be revised to extend the list? 
This needs to be spelled out.

(7) Minor:

Section 5.5 has an analogous issue to the one above above about section 
5.4, except that it pertains to the "natThresholdEvent" IE.

(8) Minor:

Section 9 appears to be normative, since it uses 2119 language. But it 
appears at a position in the document (after Acknowledgements and IANA 
Considerations, before Security Considerations), where I would normally 
not expect to find normative language.

Perhaps this is intended to be more of an appendix. If so, then the 
normative language should be removed, and it ought to be formatted as an 
appendix.

If it is intended to be normative, then I suggest that it be moved ahead 
of the Acknowledgements.

(9) Nit:

Running IDNits returns a number of issues, mostly regarding references. 
Please check these.

---

From: Paul Kyzivat <[email-masked]>
Date: Tue, 23 Feb 2016 15:40:56 -0500

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:

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.

---

From: Tal Mizrahi <[email-masked]>
Date: Thu, 25 Feb 2016 06:37:01 +0000

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'.

>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."

>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

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.

>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:
>
>[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.

Best regards,
Tal.

>-----Original Message-----
>From: Paul Kyzivat [mailto:[email-masked]]
>Sent: Tuesday, February 23, 2016 10:41 PM
>To: [email-masked]
>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
>This draft is on the right track but has open issues, described in the review.
>
>Major: 2
>Minor: 3
>Nits:  1
>
>(1) Major:
>
>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:
>
>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
>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:
>
>[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.
>
>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.
>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:
>
>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.

---

From: Paul Kyzivat <[email-masked]>
Date: Thu, 25 Feb 2016 12:03:42 -0500

Tal,

Please see inline.

> "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…

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?

>    the other extension fields in this case MUST be at least 16 octets

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

> 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

---

From: Tal Mizrahi <[email-masked]>
Date: Sun, 28 Feb 2016 06:31:20 +0000

Hi Paul,

>Inserting this for those that don't will deny them of the time they are looking
>for.
>

I believe that the Checksum Complement is most useful in LANs and in locally administered environments, where both client and server are managed by the same administrator. These are the cases where you need a high clock accuracy, and you will want to get the extra juice from hardware timestamping + Checksum Complement.
I believe it is less likely that the Checksum Complement will be used by a client that connects to a random NTP server.

Having said that, I agree that this should be reflected in the draft, and thus 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 unknown extension fields. It is noted that existing implementations that discard packets with unknown extension fields cannot interoperate with transmitters that use the Checksum Complement. 

It should be noted that when hardware-based timestamping is used, it will likely be used at both ends, and thus both hosts that take part in the protocol will support the functionality described in this memo. If only one of the hosts uses hardware-based timestamping, then the Checksum Complement can only be used if it is known that the peer host can accept the Checksum Complement."

>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.)

Actually, [NTP-Ext] uses the exact text from the erratum (with some additional text in the middle).

Best regards,
Tal.

>Sent: Thursday, February 25, 2016 7:04 PM
>To: Tal Mizrahi; [email-masked]
>Cc: General Area Review Team; Brian Haberman ([email-masked]);
>Suresh Krishnan; Karen ODonoghue ([email-masked])
>Subject: Re: [Gen-art] Gen-ART Last Call review of draft-ietf-ntp-checksum-
>trailer-04
>
>Tal,
>
>Please see inline.
>
>> 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:
>>>
>>> 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."
>
>Inserting this for those that don't will deny them of the time they are looking
>for.
>
>>> understand. I found this logic, and the references, to be confusing
>>> and seemingly
>>> contradictory:
>>>
>>> In [NTP-Ext] I find:
>>>
>>>     ... However, a MAC is
>> 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:
>>
>>> 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:
>>>
>>> IMO this draft should not progress until this is specified
>>> *somewhere*, probably in [NTP-Ext].
>>>
>>> backward compatible alternative. But this comment isn't a formal part
>>> of this review.]
>>
>>
>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
>
>>> 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>.
>>>
>>> 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:
>>>
>>> understand. I found this logic, and the references, to be confusing
>>> and seemingly
>>> contradictory:
>>>
>>> In [NTP-Ext] I find:
>>>
>>>     ... However, a MAC is
>>> 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:
>>>
>>> IMO this draft should not progress until this is specified
>>> *somewhere*, probably in [NTP-Ext].
>>>
>>> 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.
>>>
>generated or used.
>>> 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:
>>>
>>> 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.
>>
>>

---

From: Paul Kyzivat <[email-masked]>
Date: Sun, 28 Feb 2016 16:37:54 -0500

Hi Tal,

> It should be noted that when hardware-based timestamping is used, it will likely be used at both ends, and thus both hosts that take part in the protocol will support the functionality described in…

OK. That seems like a reasonable way to deal with it.

> Actually, [NTP-Ext] uses the exact text from the erratum (with some additional text in the middle).

What *isn't* in [NTP-Ext] are the notes from the erratum - notably the 
2nd one. ISTM that would be very helpful to readers.

(But my review of *this* document isn't conditioned on dealing with that.)

Do you plan to have a revised version out soon? So far this has been a 
LC review. I'm also on the hook to to a telechat review. There is an 
amazingly short gap (two days) between end of LC and the telchat. (I've 
never seen it so tight. I wonder if people will have time to review a 
revised document for the telechat?

If there is no revised document from the LC before the telechat then I 
will simply send my same review again as the telechat review, along with 
a note that we have agreed on some changes. If you plan on getting a new 
revision out I will try to update my review accordingly.

	Thanks,
	Paul