Re: [Gen-art] Genart last call review of draft-ietf-ippm-stamp-option-tlv-06

[Greg Mirsky <[e-mail]>]

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 <
[e-mail]> 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.