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

[Dan Romascanu <[e-mail]>]

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