Re: [ippm] Genart last call review of draft-ietf-ippm-stamp-07

[[e-mail] (Dale R. Worley)]

> From: Greg Mirsky <[e-mail]>
> 
> Hi Dale,
> hope this friendly reminder finds you well. I much appreciate your
> consideration of the responses to your helpful comments.

My apologies for the delay.

All of this looks good.  Although there are a couple of minor points
that I've added below.

On Mon, Sep 30, 2019 at 5:17 PM Greg Mirsky <[e-mail]> wrote:

> Hi Dale,
> thank you for your review, comments, and suggestions. Please find answers
> and notes in-line under the GIM>> tag.
> Attached, please find the diff between -07 and the current working
> version, as well as, the working version of the draft.
>
> Regards,
> Greg
>
> On Tue, Sep 3, 2019 at 6:01 PM Dale Worley via Datatracker <
> [e-mail]> wrote:
>
>>   ----------------------------------------------------------------------
>> 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-07
>> Reviewer:  Dale R. Worley
>> Review Date:  2019-09-03
>> IETF LC End Date:  2019-09-03
>> IESG Telechat date:  2019-09-19
>>
>> Summary:
>>
>>        This draft is on the right track but has open issues, described
>>        in the review.
>>
>> Minor technical issue:
>>
>> 4.4.  Interoperability with TWAMP Light
>>
>> The consequences of using a TWAMP Session-Sender with a STAMP
>> Session-Reflector aren't adequately described.  I suspect that the WG
>> understands the technical solution, but the text does not make that
>> clear.
>>
>>    If the Server Octets field is present in the TWAMP
>>    Session-Sender packet, STAMP Session-Reflector will not copy the
>>    content starting from the Server Octets field but will transmit the
>>    reflected packet of equal size.
>>
>> The final phrase appears to not be true.  In both unauthenticated and
>> authenticated mode, the size of the reflected packet is fixed, and so
>> in this case, it will not have equal size with the incoming packet.
>> Also, there is no requirement that a STAMP Session-Reflector even
>> accept incoming packets that are not of the standard length.
>>
> GIM>> Thank you for catching this issue. When the work on STAMP
> specification started, this draft also included extensions, such as Extra
> Padding TLV. Over the course of our work, all extensions were moved to the
> separate draft. Would the following update (including the previous
> paragraph in the section) address your concern:
> OLD TEXT:
>    In the latter scenario, if a TWAMP Light Session-Sender does not
>    support the use of UDP port 862, the test management system MUST set
>    STAMP Session-Reflector to use UDP port number as defined in
>    Section 4.  If the TWAMP Light Session-Sender includes Packet Padding
>    field in its transmitted packet, the STAMP Session-Reflector will
>    return the reflected packet of the symmetrical size if the size of
>    the received test packet is larger than the size of the STAMP base
>    packet.  The Session-Reflector MUST be set to use the default format
>    for its timestamps, NTP.
>
>    STAMP does not support the Reflect Octets capability defined in
>    [RFC6038].  If the Server Octets field is present in the TWAMP
>    Session-Sender packet, STAMP Session-Reflector will not copy the
>    content starting from the Server Octets field but will transmit the
>    reflected packet of equal size.
> NEW TEXT:
>    In the latter scenario, if a TWAMP Light Session-Sender does not
>    support the use of UDP port 862, the test management system MUST set
>    STAMP Session-Reflector to use UDP port number as defined in
>    Section 4.  The Session-Reflector MUST be set to use the default
>    format for its timestamps, NTP.
>
>    A STAMP Session-Reflector that supports this specification would
>    transmit the base packet (Figure 5) regardless of the size of the
>    Padding field in the packet received from TWAMP Session-Sender.
>    Also, STAMP does not support the Reflect Octets capability defined in
>    [RFC6038].  If the Server Octets field is present in the TWAMP
>    Session-Sender packet, STAMP Session-Reflector will not copy the
>    content starting from the Server Octets field and will transmit the
>    reflected packet, as displayed in Figure 5.

That looks good.

>> It seems that there is an unwritten requirement that a STAMP
>> Session-Reflector accept longer packets than are expected for its
>> configured operation mode and simply ignore the trailing excess
>> octets.
>>
>> There are also some interactions with how the HMAC is verified for
>> over-length incoming authenticated packets -- how does a STAMP
>> Session-Reflector authenticate an incoming packet that is longer than
>> the STAMP format?
>>
> GIM>> Authentication in TWAMP and STAMP use different algorithms
> (HMAC-SHA-256 in STAMP vs. HMAC-SHA-1 in TWAMP as inherited from OWAMP
> [RFC4656]), we don't have the requirement to provide interoperability in
> Authenticated mode. Will update the first paragraph of the interoperability
> section to explicitly state the scope:
> OLD TEXT:
>    One of the essential requirements to STAMP is the ability to
>    interwork with a TWAMP Light device.
> NEW TEXT:
>    One of the essential requirements to STAMP is the ability to
>    interwork with a TWAMP Light device.  Because STAMP and TWAMP use
>    different algorithms in Authenticated mode (HMAC-SHA-256 vs. HMAC-
>    SHA-1), interoperability is only considered for Unauthenticated mode.
>

That looks good.

>> Editorial issues:
>>
>> 1.  Introduction
>>
>>    and inherit separation of control (vendor-specific configuration or
>>    orchestration) and test functions.
>>
>> Is "inherit" intended to be "inherent"?
>>
> GIM>> Thank you. That was the intention. Done.
>>
>>    One of such is Performance
>>    Measurement from IP Edge to Customer Equipment using TWAMP Light from
>>    Broadband Forum [BBF.TR-390] used as the reference TWAMP Light that,
>>    according to [RFC8545], includes sub-set of TWAMP-Test functions in
>>    combination with other applications that provide, for example,
>>    control and security.
>>
>> This sentence is awkward because of the length of the title in the
>> middle of the sentence.  Perhaps
>>
>>    One of such is [BBF.TR-390] (Performance Measurement from IP Edge
>>    to Customer Equipment using TWAMP Light from the Broadband Forum),
>>    a the reference TWAMP Light, that includes a sub-set of TWAMP-Test
>>    functions along with control and security functions.
>>
> GIM>> Thank you for the proposed text. While addressing other comments the
> text has been transformed to the following:
> CURRENT TEXT:
>    Recent work on IP Edge to Customer Equipment using TWAMP Light from
>    Broadband Forum [BBF.TR-390] demonstrated that interoperability among
>    implementations of TWAMP Light is challenged because the composition
>    and operation of TWAMP Light were not sufficiently specified in
>    [RFC5357].  According to [RFC8545], TWAMP Light includes sub-set of
>    TWAMP-Test functions to provide comprehensive solution requires
>    support by other applications that provide, for example, control and
>    security.
> Hope the current version is clearer and more readable.

I would change "interoperability among implementations of TWAMP Light
is challenged" to "interoperability among TWAMP Light implementations
is poor", or perhaps "... is difficult".

In the last sentence, I think there are words missing from "to provide
comprehensive solution requires support"; perhaps "to provide a
comprhenesive soultion with support"?

>>
>> --
>>
>>    This document defines an active performance measurement test protocol
>>
>> This sentence doesn't explicitly say that STAMP is connected to TWAMP,
>> despite that the preceding sentence does explicitly say that
>> BBF.TR-390 is connected to TWAMP.  Perhaps, "This document defines
>> another such mechanism ..."  Perhaps also precede with a paragraph
>> break for clarity.
>>
> GIM>> Introduction section now has three paragraphs and the last refers to
> common extensions. Do you feel that the current version made the
> relationship between TWAMP-Test and STAMP clearer:
> CURRENT TEXT:
>    This document defines an active performance measurement test
>    protocol, Simple Two-way Active Measurement Protocol (STAMP), that
>    enables measurement of both one-way and round-trip performance
>    metrics like delay, delay variation, and packet loss.  Some TWAMP
>    extensions, e.g., [RFC7750] are supported by the extensions to STAMP
>    base specification in [I-D.ietf-ippm-stamp-option-tlv].

That looks good.

>> 2.1.  Terminology
>>
>>    AES Advanced Encryption Standard
>>    CBC Cipher Block Chaining
>>    ECB Electronic Cookbook
>>    KEK Key-encryption Key
>>
>> These four terms are not used in the document.
>>
> GIM>> Thank you, removed.
>
>>
>> 3.  Softwarization of Performance Measurement
>>
>>    The configuration and management of the STAMP Session-
>>    Sender, Session-Reflector, and management of the STAMP sessions can
>>    be achieved through various means.
>>
>> You should probably revise to add "... sessions are outside the scope
>> of this document and can be ...".
>>
> GIM>> Thank you for the suggested text. Agreed and added.
>
>>
>>    Command Line Interface, OSS/BSS
>>    (operations support system/business support system as a combination
>>    of two systems used to support a range of telecommunication services)
>>    using SNMP or controllers in Software-Defined Networking using
>>    Netconf/YANG are but a few examples.
>>
>> This sentence is long and awkward in a number of ways.  One version
>> that I think is clearer is:
>>
>>     A few examples are:  Command Line Interface, telecommunication
>>     services' OSS/BSS systems, SNMP, and Netconf/YANG-based SDN
>>     controllers.
>>
> GIM>> Many thanks for the proposal. The text is denser and informative.
>
>
>>
>> 4.  Theory of Operation
>>
>> About half of the text in section 4 is about port assignments, but
>> that isn't really part of the "Theory of Operation".  I think the
>> exposition would be easier to read if the text about port assignments
>> was extracted and put into a separate subsection (probably placed
>> between 4 and 4.1).
>>
> GIM>> I've re-arranged Section 4. Please let me know if the flow of
> information is clear and makes sense.

Yes, that reads better.

>> 4.1.1.  Session-Sender Packet Format in Unauthenticated Mode
>>
>>    o  Sequence Number is four octets long field.  For each new session
>>       its value starts at zero and is incremented with each transmitted
>>       packet.
>>
>> There is no definition of what a STAMP session is in this document.
>>
>> The idea is more or less obvious, but it would be useful to call out
>> its primary properties in section 4 "Theory of Operation".  It seems
>> like the primary properties are that a session is between one
>> Session-Sender and one Session-Reflector and a session contains a
>> potentially unlimited number of packets sent between the two.
>>
> GIM>> Magnus had similar comment and in course of our discussion we've
> agreed on the following definition of a STAMP session, now part of Section
> 3:
>    In this document, a measurement session also referred to as
>    STAMP session, is the bi-directional packet flow between one specific
>    Session-Sender and one particular Session-Reflector for a time
>    duration.

That looks good.

>>    o  Timestamp is eight octets long field.  STAMP node MUST support
>>       Network Time Protocol (NTP) version 4 64-bit timestamp format
>>       [RFC5905], the format used in [RFC5357].  STAMP node MAY support
>>       IEEE 1588v2 Precision Time Protocol truncated 64-bit timestamp
>>       format [IEEE.1588.2008], the format used in [RFC8186].
>>
>> The specification of how the time format for a particular packet is
>> chosen is weak.  I think you want to add to the above paragraph that
>> the choice of format is determined either by configuration or the Z
>> bit in the Error Estimate field.
>>
> GIM>> In regard to the format of a timestamp, STAMP specification follows
> the idea of RFC 8186 where extension to negotiate timestamp format for
> TWAMP-Control and the new interpretation of Z flag were defined. For STAMP,
> as defined for TWAMP, the value of the Z flag in the Error Estimate field
> is only to reflect the format used by the node.

In that case, I think you want to add to the definition of the
timestamp field that the format it uses is part of the configuration
of the session.  In any situation, it must be clear how the receiver
of a STAMP packet determines how to interpret the Timestamp value.

>> 4.1.2.  Session-Sender Packet Format in Authenticated Mode
>>
>>    Also, MBZ fields are used to align the packet on 16 octets
>>    boundary.
>>
>> You can't align the packet itself using a field within the packet.
>> You want to say "are used to align the fields within the packet on 16
>> octets boundaries."  Or perhaps "to make the packet length a multiple
>> of 16 octets."  Similarly in 4.2.1 and 4.2.2.
>>
> GIM>> Thank you for the suggested text. I've used the second option.
>
>>
>> 4.2.  Session-Reflector Behavior and Packet Format
>>
>>    Two modes of STAMP Session-Reflector characterize the expected
>>    behavior and, consequently, performance metrics that can be measured:
>>
>>    o  Stateless - STAMP Session-Reflector does not maintain test state
>>       and will reflect the received sequence number without
>>       modification.  As a result, only round-trip packet loss can be
>>       calculated while the reflector is operating in stateless mode.
>>
>>    o  Stateful - STAMP Session-Reflector maintains test state thus
>>       enabling the ability to determine forward loss, gaps recognized in
>>       the received sequence number.  As a result, both near-end
>>       (forward) and far-end (backward) packet loss can be computed.
>>       That implies that the STAMP Session-Reflector MUST keep a state
>>       for each accepted STAMP-test session, uniquely identifying STAMP-
>>       test packets to one such session instance, and enabling adding a
>>       sequence number in the test reply that is individually incremented
>>       on a per-session basis.
>>
>> This seems important enough -- the mode determines what data STAMP
>> can measure -- to be promoted to part of section 4, "Theory of
>> Operation".
>>
> GIM>> I agree. Moved the text to Section 4 and re-named this section to
> Session-Reflector Packet Format

That looks good, except that the first words of para 4 are
"STAMP supports two modes:", just after para 2 and 3 define two
*other* modes.  So I'd change para 4 to start "STAMP supports two
authentication modes:".

>> 4.3.  Integrity and Confidentiality Protection in STAMP
>>
>>    To provide integrity protection, each STAMP message is being
>>    authenticated by adding Hashed Message Authentication Code (HMAC).
>>
>> Of course, this is only regarding authenticated mode.  So you want to
>> phrase this "Authenticated mode provides integrity protection to each
>> STAMP message by adding ...".
>>
> GIM>> Again, thank you for the suggestion.
>
>>
>> 4.4.  Interoperability with TWAMP Light
>>
>>    For example, a TWAMP Light
>>    Session-Reflector may not support the use of UDP port 862 as defined
>>    in [RFC8545].  Thus STAMP Session-Sender MAY use port numbers as
>>    defined in Section 4.
>>
>> The connection between these two sentences is unclear.  I think it
>> means:
>>
>>    For example, a TWAMP Light
>>    Session-Reflector may not support the use of UDP port 862 as specified
>>    in [RFC8545].  Thus Section 4 permits a STAMP Session-Sender to use
>>    alternative ports.
>>
> GIM>> Great, thank you!
>
>>
>> --
>>
>>    The
>>    Session-Sender SHOULD use the default format for its timestamps -
>>    NTP.  And it MAY use PTPv2 timestamp format
>>
>> The first sentence could be made more specific.  But the second
>> sentence seems to be redundant -- SHOULD is never mandatory, so you
>> don't have to add a MAY.  So perhaps,
>>
>>    When interoperating with a TWAMP Light Session-Reflector, the
>>    Session-Sender SHOULD use the default format for its timestamps -
>>    NTP.
>>
> GIM>> We've reviewed these requirements and had agreed that they are not
> needed. The STAMP Session-Sender is required to be able to interpret both
> formats, NTP and PTP2, and the selection is local decision that has no
> impact on the ability to interwork with TWAMP Light Session-Reflector.
> Hence, these two sentences were removed.
>
>>
>> --
>>
>>    In the latter scenario, if a TWAMP Light Session-Sender does not
>>    support the use of UDP port 862, the test management system MUST set
>>    STAMP Session-Reflector to use UDP port number as defined in
>>    Section 4.
>>
>> The phrase "to use UDP port number as defined in Section 4" isn't
>> clear, since second 4 doesn't define what UDP port number should be
>> used in this situation.  I think the meaning is "the test management
>> system MUST set STAMP Session-Reflector to use an acceptable UDP port
>> number, as permitted by Section 4".
>>
> GIM>> Thank you, accepted the text you've proposed.
>
>>
>> 6.  Security Considerations
>>
>>    STAMP test packets can be transmitted with the destination UDP port
>>    number from the User Ports range, as defined in Section 4, that is
>>    already or will be assigned by IANA.
>>
>> This sentence seems to be more part of the port-number discussion (see
>> comments on section 4) than security considerations -- unless the
>> usage of port numbers is a topic in security.
>>
>> Additionally, it appears that section 4 requires that a STAMP service
>> can be configured to sent packets to any port within the User Ports
>> range, regardless of whether IANA assigns a User Port specifically to
>> STAMP.  With this consideration, the phrase "that is already or will
>> be assigned by IANA" seems to be either redundant or too narrow.
>>
> GIM>> This text has been moved to the Operational Considerations section.
> In the process of re-arraging we've removed the characterization "already
> or will be assigned by IANA". Please let me know if the current version of
> the text is acceptable.

This looks good.

Dale