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

[Greg Mirsky <[e-mail]>]

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.


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

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

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

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

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

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

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

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