Subject: Re: [Last-Call] Genart last call review of draft-ietf-ippm-stamp-option-tlv-06

From: Greg Mirsky <[email-masked]>
Date: Thu, 2 Jul 2020 12:22:04 -0700

Hi Dan,
thank you for your review, detailed questions, and helpful suggestions.
Please find my answers and notes below tagged GIM>>.

Regards,
Greg

On Mon, Jun 29, 2020 at 8:02 AM Dan Romascanu via Datatracker <
[email-masked]> wrote:

> Reviewer: Dan Romascanu
> Review result: Ready with Issues
>
> 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
>
> <https://trac.ietf.org/trac/gen/wiki/GenArtfaq>.
>
> Document: draft-ietf-ippm-stamp-option-tlv-06
> Reviewer: Dan Romascanu
> Review Date: 2020-06-29
> IETF LC End Date: 2020-07-06
> IESG Telechat date: Not scheduled for a telechat
>
> Summary: Ready with issues
>
> This is a clear, well-written document. There are a few minor issues that
> would
> benefit from clarifications and possible edits before approval.
>
> Major issues:
>
> Minor issues:
>
> 1. Section 3. Is there any recommended strategy to generate SSIDs? Are
> these
> supposed to be generated sequentially? Randomly? How soon is the 16 -bit
> space
> supposed to wrap-up? Some clarification would be useful I believe.
>
GIM>> Because test sessions, in general, will be performed for different
periods of time, implementation will need to manage the pool of available
identifiers. I agree, the initial allocation may use sequential ascending
increment by one method, but at some point, it will be
"get-the-next-available number". I propose to update the text as follows:
OLD TEXT:
   A STAMP
   Session-Sender MAY generate a locally unique STAMP Session Identifier
   (SSID).  SSID is two octets long non-zero unsigned integer.
NEW TEXT:
   A STAMP
   Session-Sender MAY generate a locally unique STAMP Session Identifier
   (SSID).  SSID is two octets long non-zero unsigned integer. SSID
generation
   policy is implementation-specific. For example, sequentially ascending
   incremented by one method could be used for the initial allocation of
SSID.
   Because of test sessions lasting different time an implementation that
uses
   SSID MUST monitor the pool of available identifiers. An implementation
   SHOULD NOT assign the same identifier to different STAMP test sessions.

>
> 2. Section 4.5 - how is the value Session-Sender Tx counter (S_TxC)
> determined
> by the sender?
>
GIM>> The value of S_TxC is the current value of the transmitted in-profile
packets. Would the following update (also addressing the #3) make it
clearer?
OLD TEXT:
    o  Session-Sender Tx counter (S_TxC) is four octets long field.

   o  Session-Reflector Rx counter (R_RxC) is four octets long field.
      MUST be zeroed by the Session-Sender and filled by the Session-
      Reflector.

   o  Session-Reflector Tx counter (R_TxC) is four octets long field.
      MUST be zeroed by the Session-Sender and filled by the Session-
      Reflector.
NEW TEXT:
   o  Session-Sender Tx counter (S_TxC) is four octets long field.  The
      Session-Reflector MUST set its value equal to the number of the
      transmitted in-profile packets.

   o  Session-Reflector Rx counter (R_RxC) is four octets long field.
      MUST be zeroed by the Session-Sender on transmit and ingored by
      the Session-Reflector on receipt.  The Session-Reflector MUST fill
      it with the value of in-profile packets received.

   o  Session-Reflector Tx counter (R_TxC) is four octets long field.
      MUST be zeroed by the Session-Sender and ignored by the Session-
      Reflector on receipt.  The Session-Reflector MUST fill with the
      value of the transmitted in-profile packets.

>
> 3. Section 4.5 - (R_RxC) and (R_TxC) MUST be zeroed by the Session-Sender
> - Is
> this verified at reception? What happens if a Session-Reflector detects a
> non-zero value in one of these fields?
>
GIM>> Please let us know if the update above addresses your concern.

>
> 4. Section 4.6 - it seems that understanding [TS23501] is needed to
> properly
> implement this section and interpret the content of the TLV. Should not
> this
> reference be Normative rather than Informative?
>
GIM>> Agreed and moved it to the list of Normative References

>
> 5. Section 5.2 - as the values for Synchronization Sources in Table 4
> refer to
> 'this document', it seems to be necessary to include in this document
> references to the documents that define the respective terms / sources
>
GIM>> The only convenient place for references I see is in the Acronyms
section. Would you suggest another section in the document? Besides the
location, some of the listed sources of synchronization do not have a
standard specification, e.g. BITS/SSU, or the specification is not easily
available, e.g., Russian government's GLONASS. Some systems, like LORAL-C,
are in the process of being decommissioned and only a few LORAL
transmitters remain operational. Would adding references to NTP and PTP in
the Acronyms section be acceptable?
   BITS Building Integrated Timing Supply

   CoS Class of Service

   DSCP Differentiated Services Code Point

   ECN Explicit Congestion Notification

   GLONASS Global Orbiting Navigation Satellite System

   GPS Global Positioning System [GPS]

   HMAC Hashed Message Authentication Code

   LORAN-C Long Range Navigation System Version C

   MBZ Must Be Zero

   NTP Network Time Protocol [RFC5905]

   PMF Performance Measurement Function

   PTP Precision Time Protocol [IEEE.1588.2008]

   TLV Type-Length-Value

   SSID STAMP Session Identifier

   SSU Synchronization Supply Unit

   STAMP Simple Two-way Active Measurement Protocol

>
> 6. Section 6 - Security Considerations: Is not sending of test packets to a
> reflector that does not support SSID a potential sourse for DoS attacks?

GIM>> A Session-Reflector that does not support SSID would transmit
reflected test packet with SSID field zeroed. The local to the
Session-Sender policy will control whether the Session-Sender stops or
continues the test session.

> Same
> question about sending packets with unsupported TLV types. How do
> Reflectors
> protect against such situations? As such attacks would be beyond STAMP base
> specifications, it may be good to discuss these.
>
GIM>> A Session-Reflector that does not support STAMP extensions is not
expected to compare the value in the Length field of the UDP header and the
length of the STAMP base packet. Hence the Session-Reflector will transmit
the base STAMP packet. It is the local policy on the Session-Sender
(similar to the handling of SSID == 0 situation) that will control the
Sender's behavior. I think the text might be appended to the second
paragraph of Section 4. The updated paragraph is below:
   A STAMP node, whether Session-Sender or Session-Reflector, receiving
   a test packet MUST determine whether the packet is a base STAMP
   packet or includes one or more TLVs.  The node MUST compare the value
   in the Length field of the UDP header and the length of the base
   STAMP test packet in the mode, unauthenticated or authenticated based
   on the configuration of the particular STAMP test session.  If the
   difference between the two values is larger than the length of UDP
   header, then the test packet includes one or more STAMP TLVs that
   immediately follow the base STAMP test packet.  A Session-Reflector
   that does not support STAMP extensions is not expected to compare the
   value in the Length field of the UDP header and the length of the
   STAMP base packet.  Hence the Session-Reflector will transmit the
   base STAMP packet.  It is the local policy on the Session-Sender
   (similar to the handling of SSID == 0 scenario described in
   Section 3) that will control the sender's behavior.

>
> Nits/editorial comments:
>
> 1. Section 2.1 - it's more convenient for future users of the document if
> acronyms were listed in alphabetical order
>
GIM>> Agree. Done (please check it above).

>
> 2. Sections 4.6, 4.7 - inconsistent use of capitalization:
>
>  Reserved - ... must be zeroed on transmission
>       and ignored on receipt.
>
> It's a 'must' in 4.6, and a 'MUST' in 4.7
>
GIM>> Thank you for pointing it out. I've found two cases of "must" that
changed to normative-style.

---

From: Dan Romascanu <[email-masked]>
Date: Thu, 2 Jul 2020 22:52:19 +0300

Hi Greg,

Thank you for the answer and for addressing my comments. All your
explanations are clear and satisfactory, and the proposed edits are fine
with me.

Regards,

Dan

On Thu, Jul 2, 2020 at 10:22 PM Greg Mirsky <[email-masked]> wrote:

> GIM>> The value of S_TxC is the current value of the transmitted
> in-profile packets. Would the following update (also addressing the #3)
> make it clearer?
> OLD TEXT:

---

From: Greg Mirsky <[email-masked]>
Date: Thu, 2 Jul 2020 16:26:29 -0700

Hi Dan,
I greatly appreciate your kind consideration of the prosed updates and the
most expedient response. Again, thank you for your comments that helped to
improve the document.

Kind regards,
Greg

On Thu, Jul 2, 2020 at 12:52 PM Dan Romascanu <[email-masked]> wrote:

>>> This is a clear, well-written document. There are a few minor issues
>>> that would
>>> 3. Section 4.5 - (R_RxC) and (R_TxC) MUST be zeroed by the
>>> Session-Sender - Is

---

From: Alissa Cooper <[email-masked]>
Date: Wed, 15 Jul 2020 10:51:10 -0400

Dan, thanks for your review. Greg, thanks for responding to Dan’s review. I entered a No Objection ballot.

Alissa

> On Jul 2, 2020, at 7:26 PM, Greg Mirsky <[email-masked]> wrote:
> 
> Hi Dan,
> I greatly appreciate your kind consideration of the prosed updates and the most expedient response. Again, thank you for your comments that helped to improve the document.
> 
> Kind regards,
> Greg
> 
> On Thu, Jul 2, 2020 at 12:52 PM Dan Romascanu <[email-masked] <mailto:[email-masked]>> wrote:
> Hi Greg, 
> 
> Thank you for the answer and for addressing my comments. All your explanations are clear and satisfactory, and the proposed edits are fine with me. 
> 
> Regards,
> 
> Dan
> 
> 
> On Thu, Jul 2, 2020 at 10:22 PM Greg Mirsky <[email-masked] <mailto:[email-masked]>> wrote:
> Hi Dan,
> thank you for your review, detailed questions, and helpful suggestions. Please find my answers and notes below tagged GIM>>.
> 
> Regards,
> Greg
> 
> On Mon, Jun 29, 2020 at 8:02 AM Dan Romascanu via Datatracker <[email-masked] <mailto:[email-masked]>> wrote:
> Reviewer: Dan Romascanu
> <https://trac.ietf.org/trac/gen/wiki/GenArtfaq <https://trac.ietf.org/trac/gen/wiki/GenArtfaq>>.
> 
> This is a clear, well-written document. There are a few minor issues that would
> 1. Section 3. Is there any recommended strategy to generate SSIDs? Are these
> supposed to be generated sequentially? Randomly? How soon is the 16 -bit space
> GIM>> Because test sessions, in general, will be performed for different periods of time, implementation will need to manage the pool of available identifiers. I agree, the initial allocation may use sequential ascending increment by one method, but at some point, it will be "get-the-next-available number". I propose to update the text as follows:
> OLD TEXT:
>    A STAMP
>    (SSID).  SSID is two octets long non-zero unsigned integer. SSID generation
>    incremented by one method could be used for the initial allocation of SSID.
>    Because of test sessions lasting different time an implementation that uses
> 2. Section 4.5 - how is the value Session-Sender Tx counter (S_TxC) determined
> by the sender?
> GIM>> The value of S_TxC is the current value of the transmitted in-profile packets. Would the following update (also addressing the #3) make it clearer?
> OLD TEXT:
>       transmitted in-profile packets..
> 
> 3. Section 4.5 - (R_RxC) and (R_TxC) MUST be zeroed by the Session-Sender - Is
> 4. Section 4.6 - it seems that understanding [TS23501] is needed to properly
> implement this section and interpret the content of the TLV. Should not this
> 5. Section 5.2 - as the values for Synchronization Sources in Table 4 refer to
> GIM>> The only convenient place for references I see is in the Acronyms section. Would you suggest another section in the document? Besides the location, some of the listed sources of synchronization do not have a standard specification, e.g. BITS/SSU, or the specification is not easily available, e.g., Russian government's GLONASS. Some systems, like LORAL-C, are in the process of being decommissioned and only a few LORAL transmitters remain operational. Would adding references to NTP and PTP in the Acronyms section be acceptable?
> GIM>> A Session-Reflector that does not support SSID would transmit reflected test packet with SSID field zeroed. The local to the Session-Sender policy will control whether the Session-Sender stops or continues the test session.
> Same
> question about sending packets with unsupported TLV types. How do Reflectors
> GIM>> A Session-Reflector that does not support STAMP extensions is not expected to compare the value in the Length field of the UDP header and the length of the STAMP base packet. Hence the Session-Reflector will transmit the base STAMP packet. It is the local policy on the Session-Sender (similar to the handling of SSID == 0 situation) that will control the Sender's behavior. I think the text might be appended to the second paragraph of Section 4. The updated paragraph is below:
> GIM>> Thank you for pointing it out. I've found two cases of "must" that changed to normative-style. 
> _______________________________________________
> ippm mailing list
> [email-masked]
> https://www.ietf.org/mailman/listinfo/ippm

---

From: Benjamin Kaduk <[email-masked]>
Date: Fri, 17 Jul 2020 15:39:13 -0700

Hi again Greg :)

Reading Dan's review reminded me of one other point (inline)...

>    SHOULD NOT assign the same identifier to different STAMP test sessions.

I would actually recommend against mentioning the "sequential increment"
strategy.  There's some justification for why in
draft-gont-numeric-ids-sec-considerations (and more in the references),
which I just completed my AD Evaluation of with intent to AD sponsor as a
BCP.

Thanks,

Ben

---

From: Greg Mirsky <[email-masked]>
Date: Fri, 17 Jul 2020 16:55:55 -0700

Hi Ben,
thank you for the reference, very helpful. As you've noticed, this method
mentioned as an example. Would you suggest referencing another technique?
As I understood, Dan's comment was not specific to the sequential increment
allocation policy but to provide some guidance to an implementor.

Regards,
Greg

On Fri, Jul 17, 2020 at 3:39 PM Benjamin Kaduk <[email-masked]> wrote:

> that

---

From: Dan Romascanu <[email-masked]>
Date: Sat, 18 Jul 2020 09:37:08 +0300

Greg's understanding of my comment is correct.

Regards,

Dan

---

From: Benjamin Kaduk <[email-masked]>
Date: Mon, 20 Jul 2020 18:48:09 -0700

Thanks Dan.

Greg, my recommendation would be to refer to the appopriate section of
https://tools.ietf.org/html/draft-gont-numeric-ids-generation-03 for the
needs in question.  My understanding is that STAMP needs only unique (not
ordered) session IDs, and that furthermore, an occasional accidental
collision would not be catastrophic, in which case we can use the
"Uniqueness (soft failure)" characterization of Section 7.1 of the linked
document.

-Ben

---

From: Greg Mirsky <[email-masked]>
Date: Mon, 20 Jul 2020 19:01:06 -0700

Hi Ben,
thank you for the suggestions. Would the Informational reference to
draft-gont-numeric-ids-generation be reasonable?

Regards,
Greg

---

From: Greg Mirsky <[email-masked]>
Date: Mon, 20 Jul 2020 19:54:53 -0700

Hi Dan and Ben,
the updated text is below. I greatly appreciate your feedback.

NEW TEXT:
    A STAMP
   Session-Sender MAY generate a locally unique STAMP Session Identifier
   (SSID).  The SSID is a two-octet-long non-zero unsigned integer.
   SSID generation policy is implementation-specific.
   [I-D.gont-numeric-ids-generation] thoroughly analyzes common
   algorithms for identifier generation and their vulnerabilities.  For
   example, an implementation can use algorithms described in
   Section 7.1 of [I-D.gont-numeric-ids-generation].  An implementation
   MUST NOT assign the same identifier to different STAMP test sessions.

Regards,
Greg