Re: [ippm] Benjamin Kaduk's Discuss on draft-ietf-ippm-stamp-option-tlv-07: (with DISCUSS and COMMENT) addressing COMMENTS

[Benjamin Kaduk <[e-mail]>]

Hi Greg,

On Sat, Jul 18, 2020 at 01:33:00PM -0700, Greg Mirsky wrote:
> Hi Benjamin,
> I hope my proposal to split DISCUSS and COMMENTS resolutions is acceptable
> to you (I'll make sure that nothing gets lost in the meantime). Please find
> my answers to the COMMENTS section of your review below tagged GIM>>.
> Attached is the diff reflecting all the updates applied in the working
> version. Please note that the format of the Location TLV is WIP.

Understood about the Location TLV, and thank you for reopening that part of
the design..
Inline...

> Regards,
> Greg
> 
> On Tue, Jul 14, 2020 at 7:30 PM Benjamin Kaduk via Datatracker <
> [e-mail]> wrote:
> 
> > Benjamin Kaduk has entered the following ballot position for
> > draft-ietf-ippm-stamp-option-tlv-07: Discuss
> >
> > When responding, please keep the subject line intact and reply to all
> > email addresses included in the To and CC lines. (Feel free to cut this
> > introductory paragraph, however.)
> >
> >
> > Please refer to https://www.ietf.org/iesg/statement/discuss-criteria.html
> > for more information about IESG DISCUSS and COMMENT positions.
> >
> >
> > The document, along with other ballot positions, can be found here:
> > https://datatracker.ietf.org/doc/draft-ietf-ippm-stamp-option-tlv/
> >
> >
> >
> > ----------------------------------------------------------------------
> > DISCUSS:
> > ----------------------------------------------------------------------
> >
> > I support Roman's discusses and am happy to see the ongoing discussion
> > thereof.
> >
> > (1) I think there's a conflict between this document and RFC 8762 with
> > respect to the behavior of pure RFC 8762 implementations that receive
> > packets longer than the base packet for the given operational mode.
> >
> > RFC 8762 says (Section 4.3):
> >
> > % The Session-Reflector receives the STAMP-Test packet and verifies it. If
> > % the base STAMP-Test packet is validated, the Session-Reflector that
> > % supports this specification prepares and transmits the reflected test
> > % packet symmetric to the packet received from the Session-Sender copying
> > % the content beyond the size of the base STAMP packet (see Section 4.2).
> >
> > But Section 4 of this document says:
> >
> >                                                    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.  [...]
> >
> > Does "will transmit the base STAMP packet" mean something other than
> > "with the exact length of the base packet [for the given operational
> > mode]"?
> >
> > (2) As I remarked on (then-) draft-ietf-ippm-stamp, I think we need to
> > require some level of cryptographic protection whenever control
> > information is included in a Session-Sender's test packet.  That is,
> > that a Session-Reflector MUST NOT act on control information received in
> > unauthenticated packets, and specifically, that the HMAC TLV must be
> > used, since the base authenticated STAMP packet's HMAC does not cover
> > the options.
> >
> > (3) The secdir reviewer's question about dealing with 6-to-4 gateways
> > seems to have not gotten a response.  Specifically, the requirement that
> > "[t]he Session-Reflector MUST validate the Length value against the
> > address family of the transport encapsulating the STAMP test packet"
> > seems to require the protocol to fail when sender and reflector use
> > different address families, or perhaps to require the sender to use
> > trial and error to determine which address family is used by the
> > reflector.  Some clarification on the intended operation in such
> > scenarios seems appropriate.
> >
> > (4) The ability for a Session-Sender to (MUST-level!) control the DSCP
> > codepoint used by packets generated by a Session-Reflector feels like it
> > opens up significant risk in site-local (security-relevant) policy.  That
> > is, the interpretation of the DSCP codepoints is to large extent
> > site-specific, and allowing a nominally external system to set any/all
> > possible values, without a chance for site policy to be applied and
> > block the use of potentially disruptive DSCP values.  So I think we need
> > to modify the "MUST set", perhaps requiring that either the requested
> > DSCP value is used or the entire TLV/packet/whatever is rejected.
> >
> > (5) If we're not going to remedy the severability of authenticated
> > options from authenticated base packets (which would be my preferred
> > resolution), we need to document that weakness in the security
> > considerations.
> >
> >
> > ----------------------------------------------------------------------
> > COMMENT:
> > ----------------------------------------------------------------------
> >
> > (side note) It seems slightly gratuitous to group the "base" (with
> > SSID) packet formats by authenticated/unauthenticated and then by
> > sender/reflector, when RFC 8762 groups by sender/reflector and then by
> > authenticated/unauthenticated.  But probably not worth churning the
> > document at this point to address it...
> >
> GIM>> As you've said, it would make documents look consistent but if you
> can accept the current way of presenting information, much appreciate your
> understanding.

To confirm: I can accept it (this is the non-blocking COMMENT section,
after all ;)
> >
> > I support Erik Kline's discuss point about the L-bit error-case guidance
> > (and note that Section 4.7 also seems to be saying to zero specific
> > fields vs. the earlier guidance to echo the entire rest of the packet).
> >
> GIM>> I'm looking to hearing from Erik on the proposed resolution. Yes, the
> text in Section 4.7 is ambiguous. Will s/set the L flag/set the L flag to
> 1/  fix the issue?

I don't think so.  The question is not about what happens to the L bit, but
rather what happens for the "rest of the packet" (which, I realized after
balloting, is actually a bit unclear itself -- is "rest of the packet" just
"bits later in the packet body" or does processing order come into play,
e.g., processing the HMAC TLV first).

> >
> > Section 1
> >
> >    Simple Two-way Active Measurement Protocol (STAMP) [RFC8762] supports
> >    the use of optional extensions that use Type-Length-Value (TLV)
> >    encoding.  Such extensions enhance the STAMP base functions, such as
> >
> > How about a "this document specifies" in there somewhere?  TLVs are not
> > mentioned in RFC 8762...
> >
> GIM>> Agree and thank you for the suggested text. Would the following be
> acceptable:
> OLD TEXT:
>    Simple Two-way Active Measurement Protocol (STAMP) [RFC8762] supports
>    the use of optional extensions that use Type-Length-Value (TLV)
>    encoding.
> NEW TEXT:
>    Simple Two-way Active Measurement Protocol (STAMP) [RFC8762] defined
>    the STAMP base functionalities.  This document specifies the use of
>    optional extensions that use Type-Length-Value (TLV) encoding.

+1

> >
> > Section 3
> >
> >    An implementation of the STAMP Session-Reflector that supports this
> >    specification SHOULD identify a STAMP Session using the SSID in
> >    combination with elements of the usual 4-tuple for the session.
> >
> > It's slightly surprising that this is only a SHOULD.
> >
> GIM>> Similar comment came from another IESG reviewer. The working version
> of the draft states:
>     An implementation of the STAMP Session-Reflector that supports this
>    specification MUST identify a STAMP Session using the SSID in
>    combination with elements of the usual 4-tuple for the session.
> >
> >
> >    A STAMP Session-Reflector that does not support this specification
> >    will return the zeroed SSID field in the reflected STAMP test packet.
> >    The Session-Sender MAY stop the session if it receives a zeroed SSID
> >    field.  An implementation of a Session-Sender MUST support control of
> >    its behavior in such a scenario.  [...]
> >
> > This feels like it's somewhat misaligned, in that the MAY would indicate
> > that the choice is at the implementation's discretion, but the MUST
> > indicates that it's at the operator's discretion.
> >
> GIM>> Thank you for sharing your concern. Our intention was to require an
> implementation to provide the control over the Session-Sender's behavior in
> this situation and give an operator option to chose one. Would re-wording
> s/The Session-Sender MAY stop/The Session-Sender MAY be configured by an
> operator to stop/ make it clearer?

That seems like an improvement, yes.  I suspect that a more drastic
rewording would yield further improvement, but am not coalescing on a
particular suggestion right now.
 
> >
> >
> > Should the TLVs field be included in Figures 3 and 4 (as it is in
> > Figures 1 and 2)?
> >
> GIM>> Of course, Done.
> 
> >
> > Section 4
> >
> >    optional field in the STAMP test packet.  Multiple TLVs MAY be placed
> >    in a STAMP test packet.  A TLV MAY be enclosed in a TLV.  TLVs have a
> >
> > Would it be appropriate to instead say something like "Additional TLVs
> > may be enclosed within a given TLV, subject to the semantics of the
> > (outer) TLV in question"?
> >
> GIM>> Thank you for the suggested text. Took it in.
> 
> >
> >    The format of the STAMP TLV Flags displayed in Figure 6 and the
> >    location of flags is according to Section 5.2.
> >
> > (side note): it's often nice to give a short "mnemonic" expansion for
> > the bit names, to help people remember what they do.  E.g., 'U' could be
> > "unimplemented" (or "unrecognized"), 'A' could be "authentic", etc.
> > (assuming that the boolean sense of the interpretation makes sense, of
> > course).  ('L' confuses me a bit, as "length-error" is not quite a
> > generic enough description to cover all flavors of malformed contents.)
> >
> GIM>> Thank you for the suggestion to re-name the L flag to M (Malformed).
> I'll note that the A flag also renamed to I(Integrity). Would this manner
> give the expanded name (in addition to the IANA section) help a reader:
>    o  U (Unrecognized) - is a one-bit flag.  ...
> 
>    o  M (Malformed) is a one-bit flag.  ...
> 
>    o  I (Integrity) - a one-bit flag.  ...

Yes, putting the expanded name in parentheses would be a big help!

> >
> >    o  U - a one-bit flag.  A Session-Sender MUST set the U flag to 0
> >       before transmitting an extended STAMP test packet.  A Session-
> >       Reflector MUST set the U flag to 1 if the Session-Reflector has
> >       not understood the TLV.
> >
> > This seems kind of problematic, in that the Session-Sender can only rely
> > on this ("set to 1") behavior if it already knows as a precondition that
> > the Session-Reflector supports this document.  (IIUC, a pure RFC 8762
> > Session-Reflector would just blindly copy any part of the packet after
> > the base format into the reflected packet, i.e., leave the bit set at
> > zero.)  Wouldn't it be more robust if the semantics were to set the bit
> > to 1 if it actively was understood?  (Yes, this would require a more
> > complicated specification for recipient behavior when the U flag is set,
> > since it would differ between Sender and Reflector, but that seems
> > easier to control for than lack of robustness in the face of mixed
> > deployment.)
> >
> GIM>> Thank you for the idea. As noted earlier, the U flag now required to
> be set by a Session-Sender to 1. Rest of the operations with the U flag
> remain the same:
>    o  U (Unrecognized) - is a one-bit flag.  A Session-Sender MUST set
>       the U flag to 1 before transmitting an extended STAMP test packet.
>       A Session-Reflector MUST set the U flag to 1 if the Session-
>       Reflector has not understood the TLV.  Otherwise, the Session-
>       Reflector MUST set the U flag in the reflected packet to 0.

Hmm, I'm not sure this really helps very much.  Let me try to step back a
little.  A Session-Sender that implements this spec and is sending a given
TLV could be talking to any of (1) a stock 8762 Session-Reflector, (2) a
Session-Reflector that implements TLVs but not the one in question, or (3)
a Session-Reflector that implements the TLV in question.  If we assume that
the Session-Sender knows (whether from out-of-band means, SSID inspection,
or otherwise) that its Session-Reflector supports any TLVs at all (and that
we're not likely to bother sending TLVs to a Session-Reflector that doesn't
support them), then we only have to care about cases (2) and (3), and
exactly how distinguish between them doesn't matter, because we assume that
we fully control all the behavior.  What I'm concerned about is the risk of
a "false positive" -- that we think we know the Session-Reflector supports
TLVs but it actually does not.  With both this formulation and the previous
one, in the case of such a "false positive" we detect that the we are in
case (3), because the 8762 behavior is to reflect the whole packet, i.e.,
keep the U flag set to 1.  It seems like it might be safer with respect to
random errors if we detect this situation as case (2), since case (2) means
that the peer does not support the functionality in question, which is the
case for a base 8762 implementation.

If the WG does not feel that this risk is worth considering (e.g., due to
perceived infallibility of the control plane), then perhaps it doesn't
matter, but applying a general approach to defining safe systems would have
us detect case (1) as case (2) and treat the peer as not supporting (any)
TLVs.

> 
> >
> > Section 4.1
> >
> >    o  Extra Padding - a pseudo-random sequence of bits.  The field MAY
> >       be filled with all zeros.
> >
> > "All zeros" is not a "pseudo-random sequence of bits".  I suggest
> > "typically, a pseudo-random sequence of bits" instead.
> >
> GIM>> We've received a comment from another reviewer and the edited text
> now reads as:
>    o  Extra Padding - SHOULD be filled by a sequence of a pseudo-random
>       numbers.  The field MAY be filled with all zeros.  An
>       implementation MUST control the type of filling of the Extra
>       Padding field.
> Had the change made the text clearer?

Yes, thanks.

> >
> > Section 4.2
> >
> >    STAMP Session-Senders MAY include the Location TLV to request
> >    information from the Session-Reflector.  The Session-Sender SHOULD
> >    NOT fill any information fields except for STAMP TLV Flags, Type, and
> >    Length.  [...]
> >
> > Just to check: "NOT fill any information" means "set all bits to zero",
> > right?
> >
> GIM>> Correct. Would you suggest to explicitly require zeroing all other
> fields?

>From a general point of view, it's better to explicitly say what to fill
the fields with, since in some implementation languages not doing anything
means sending the contents of uninitialized memory, which is treated as a
security vulnerability.

> >
> > Also, to check: this "to request information" implies that this is
> > "optional control information" that, per my Discuss point, "MUST NOT be
> > acted on unless authenticated"?  So, we would want to (in addition to my
> > previous point) say that the fields would be set to all zeros in the
> > reflected packet if the initial packet was unauthenticated.
> >
> GIM>> Would changing the text to "to query information" remove the
> "optional control information" interpretation? Will add text that

That helps, yes.

> unreported fields in the reflected packet MUST be set to all zeroes.
> In the DISCUSS thread, two options for more secure use of the Location TLV
> - authentication and local policies. Following new text is for the latter:
>    The Session-Reflector that received an extended STAMP packet with the
>    Location TLV MUST include the Location TLV in the reflected packet.
>    Based on the local policy, the Session-Reflector MAY some fields
>    unreported by filling them with zeroes.  An implementation of the
>    stateful Session-Reflector MUST provide control for managing such
>    policies.

In combination with the above, this sounds good -- thanks!

> >
> >    packet.  If the Length field's value is invalid, the Session-
> >    Reflector MUST zero all fields and MUST NOT return any information to
> >    the Session-Sender.  The Session-Reflector MUST ignore all other
> >    fields of the received Location TLV.
> >
> > nit(?): Grammatically, framing these requirements as standalone
> > sentences seems to imply that they are independent requirements that
> > apply separately.  That, in turn, would say that the Session-Reflector
> > MUST ignore everything but the Length, always, which is of course absurd
> > ... but it might be worth clarifying that this requirement is also
> > conditional on an invalid Length field.
> >
> GIM>> Following on the comment from another IESG reviewer this text was
> modified and now reads as:
>   The Session-Reflector MUST validate the Length value against
>    the address family of the transport encapsulating the STAMP test
>    packet.  If the Length field's value is invalid, the Session-
>    Reflector follows the procedure defined in Section 4 for a malformed
>    TLV.
> Would you agree with this update?

Yes.

> >
> >    o  Source MAC - 6-octet-long field.  The Session-Reflector MUST copy
> >       the Source MAC of the received STAMP packet into this field.
> >
> > (nit) In my mind, a bare "MAC" here evaluates to Message Authentication
> > Code, i.e., the authorization HMAC, which is of course 16 octets, not 6.
> > So I suggest including the word "Address" to disambiguate.
> >
> GIM>> Done.
> 
> >
> >    o  Destination IP Address - IPv4 or IPv6 destination address of the
> >       packet received by the STAMP Session-Reflector.
> >    [...]
> >
> > (side note): I note that at least in 2008, there were deployed NATs that
> > rewrote binary payloads containing the NAT's public IP address,
> > necessitating the creation of the XOR-MAPPED-ADDRESS attribute to
> > successfully convey the un-NAT-mangled address to the peer
> > (https://tools.ietf.org/html/rfc5389#section-15.2).  Depending on how
> > reliable the address information (source or destination) needs to be for
> > STAMP, this may be a relevant consideration.
> >
> GIM>> Thank you for the reference. Will discuss this among the authors.
> 
> >
> >    ports will indicate if there is a NAT router on the path.  It allows
> >    the Session-Sender to identify the IP address of the Session-
> >    Reflector behind the NAT, and detect changes in the NAT mapping that
> >    could cause sending the STAMP packets to the wrong Session-Reflector.
> >
> > How does this allow the Session-Sender to detect changes in the NAT
> > mapping that would cause this?
> 
> GIM>> My understanding of the operational case is that the Location TLV is
> sparsely included in a test packet. The change in Source IP Address, Source
> UDP port values would be the indication of changed mapping.
> 
> 
> > Wouldn't a change in the NAT mapping
> > that sends packets to the wrong Session-Reflector result in a failure to
> > look up the session on the Reflector and a lack of any reflected packets
> > coming back to the Session-Sender?
> >
> GIM>> That probably depends on how the "wrong" Session-Reflector is
> configured. I think that there are legitimate scenarios when such
> Session-Reflector sends the reflected test packet to the Session-Sender.

Hmm, okay.  Thanks for clarifying.

> >
> > Section 4.3
> >
> > Is just the granularity of "NTP" or "PTP" as time synchronization
> > information sufficient to be useful?  Is the assumption that one would
> > then go and query, via a different mechanism, (e.g.) what stratum of NTP
> > source is used, etc.?
> >
> GIM>> The quality of the clock synchronization is only one element when
> considering the accuracy of the collected timestamp. The method of
> obtaining the timestamp might have a larger impact on accuracy than the
> quality of the clock synchronization. I think that if the TLV is defined as
> variable-length, it will allow extend it in the future, if need to be.

That's true (and I see you're adding the sub-TLVs in the editor's copy).

> >
> >    The STAMP Session-Sender MAY include the Timestamp Information TLV to
> >    request information from the Session-Reflector.  The Session-Sender
> >
> > [ditto re "optional control information" and authentication]
> >
> GIM>> Would using "query for information" remove the stigma of "control
> information"? AFAIK, all the variants of the Echo query do that - query for
> information.

I don't think I'm allowed to complain if all traffic ends up getting
authenticated due to us saying that echo requests are control commands :)
But yes, "query for information" would help some.
> >
> >    SHOULD NOT fill any information fields except for STAMP TLV Flags,
> >    Type, and Length.  The Session-Reflector MUST validate the Length
> >
> > [ditto re "NOT fill in"]
> >
> GIM>> Added, "all other fields MUST be filled with zeroes".
> 
> >
> >    value of the TLV.  If the value of the Length field is invalid, the
> >
> > I know that we expect the Session-Reflector to have relatively little
> > logic, but the Session-Sender should probably still behave defensively
> > and also do this kind of validation (not just here; throughout the
> > document) in case it receives attacker-supplied input.
> >
> GIM>> This text was updated as the result of responding to other comments.
> The working version of the text as follows:
>    If the value of the Length field is invalid, the
>    Session-Reflector follows the procedure defined in Section 4 for a
>    malformed TLV.
> Is the updated text acceptable?

This still seems to only talk about the Session-Reflector.  I want the
Session-Sender to also be checking for malformed packets.

> 
> >
> > Section 4.4
> >
> > Could we get some more guidance on how the Session-Sender should set all
> > these fields, specifically the DSCP2 that is "the received value in the
> > DSCP field at the Session-Reflector in the forward direction", that in
> > general the sender cannot know at the time it constructs the packet?
> >
> GIM>> The motivation for this TLV came from the field experience with
> multi-service backhauling. I imagine that one sequence of DSCP2 values can
> be used in the Service activation testing and a somewhat different sequence
> would be used in the production network. We don't have any general
> recommendations to design the sequence or select the particular value. As
> too often "It depends".

I was just looking for "set the fields to zero that will be filled in but
not interpreted by the Session-Reflector, e.g., DSCP2.

> >
> >    drops for lower service packets are at a normal level.  Using a CoS
> >    TLV in STAMP testing helps to troubleshoot the existing problem and
> >    also verify whether DiffServ policies are processing CoS as required
> >    by the configuration.
> >
> > It's not immediately obvious to me how we get from "using a CoS TLV in
> > STAMP" to "verify whether DiffServ policies are processing CoS as
> > required by the configuration", though I'm willing to believe it's the
> > case.
> >
> GIM>> This TLV extends the functionality defined in RFC 7750
> <https://datatracker.ietf.org/doc/rfc7750/>. To the best of my
> understanding, there's at least one independent implementation of that RFC.
> 
> >
> > Section 4.5
> >
> > While I can perhaps accept that the specifics of "in-profile" are going
> > to be site-specific, I would greatly appreciate some greater clarity on
> > what type of thing it's supposed to be doing and at what scope (in time
> > and, e.g., STAMP session) it is going to be defined/configured.
> >
> GIM>> In response to another comment the working version now equates
> "in-profile packets" to a distinctive data flow:
>    The Direct Measurement TLV enables collection of the number of in-
>    profile packets, i.e., packets that form a specific data flow, that
>    had been transmitted and received by the Session-Sender and Session-
>    Reflector, respectively.
> Would the updated text be acceptable?

Yes, thanks.
> >
> >    test packet.  The Session-Sender MUST zero the R_RxC and R_TxC fields
> >    before the transmission of the STAMP test packet.  If the received
> >
> > (This is redundant with the per-field definitions.)
> >
> GIM>> Thank you for catching this. Removed the sentence.
> 
> >
> > Section 4.6
> >
> >    o  ID (Access ID) - four-bit-long field that identifies the access
> >       network, e.g., 3GPP (Radio Access Technologies specified by 3GPP)
> >       or Non-3GPP (accesses that are not specified by 3GPP) [TS23501].
> >
> > Does the [TS23501] reference really apply to the "Non-3GPP" case
> > (whether or not in addition to the 3GPP case)?
> >
> GIM>> Yes, it does refer to all access networks other than defined in 3GPP,
> e.g., WiFi, fixed broadband, as Non-3GPP.
> 
> >
> >       The value is one of those listed below:
> >
> >       *  1 - 3GPP Network
> >
> >       *  2 - Non-3GPP Network
> >
> >       All other values are invalid and the TLV that contains it MUST be
> >       discarded.
> >
> > So there's no interest in future expansion here or need for a registry?
> >
> GIM>> This TLV follows the current Rev.16 specification of the PMF. 3GPP
> might change, add values in the future. In the earlier version, that was
> another sub-registry. But due to too few values defined and uncertainty of
> the real need for new values, decided not to use IANA.
> 
> >
> >    three seconds.  An implementation MUST provide control of the
> >    retransmission timer value and the number of retransmissions.
> >
> > Is the retransmission timer defined to have a fixed value (i.e., no
> > back-off)?
> >
> GIM>> The text follows  TS23501 that defined the PMF.
> 
> >
> > Is the retransmission a blind retransmission, i.e., using the original
> > Return Code even if local conditions have changed?
> >
> GIM>> Excellent question, thank you! Will get back.
> 
> >
> > Section 4.7
> >
> >    in the reflected packet.  If the Session-Reflector is in stateless
> >    mode (defined in Section 4.2 [RFC8762]), it MUST zero the Sequence
> >
> > Looks like this definition is just in the toplevel Section 4 of RFC 8762.
> >
> GIM>> I think that the field's naming identical to the field in the base
> STAMP packet may be the source of some confusion. Would naming the field

Perhaps, but I'm just searching for the word "stateless" in RFC 8762 and
finding it in (toplevel) Section 4 and not again until 4.3.1.  So if the
intent is to refer to the definition of "stateless mode", it seems pretty
clear that Section 4.2 is not the right reference.  If the intent is to
refer to something else (actually in Section 4.2 of RFC8762), then I have
no idea what that something else is.

> Previous Sequence Number make it clearer while being consistent with the
> definition:

This renaming is probably worth doing, regardless of the above.

>    o  Sequence Number - four-octet-long field indicating the sequence
>       number of the last packet reflected in the same STAMP-test
>       session.  Since the Session-Reflector runs in the stateful mode
>       (defined in Section 4.2 [RFC8762]), it is the Session-Reflector's
>       Sequence Number of the previous reflected packet.
> 
> >
> >    o  Follow-up Timestamp - eight-octet-long field, with the format
> >       indicated by the Z flag of the Error Estimate field of the packet
> >       transmitted by a Session-Reflector, as described in Section 4.1
> >       [RFC8762].  It carries the timestamp when the reflected packet
> >       with the specified sequence number was sent.
> >
> > We should probably be extremely clear on whether this is the Z flag of
> > the current/containing packet or the one indicated by the Sequence
> > Number field.
> >
> GIM>> It seems very unlikely that a STAMP node will change timestamp
> encoding in the course of the test session. I agree that clarification
> would be helpful:
>    o  Follow-up Timestamp - eight-octet-long field, with the format
>       indicated by the Z flag of the Error Estimate field of the STAMP
>       base packet, which is contained in this reflected test packet
>       transmitted by a Session-Reflector, as described in Section 4.2.1
>       [RFC8762].  It carries the timestamp when the reflected packet
>       with the specified sequence number was sent.
> 
> >
> > Section 4.8
> >
> > (I'm happy to see the discussion with Roman about the key-management and
> > algorithm-agility questions, noting that we have BCPs 107 and 201 to
> > give guidance for those cases, respectively.)
> >
> >    The STAMP authenticated mode protects the integrity of data collected
> >    in the STAMP base packet.  STAMP extensions are designed to provide
> >    valuable information about the condition of a network, and protecting
> >    the integrity of that data is also essential.  The keyed Hashed
> >    Message Authentication Code (HMAC) TLV MUST be included in a STAMP
> >    test packet in the authenticated mode, excluding when the only TLV
> >    present is Extra Padding TLV.  The HMAC TLV MUST follow all TLVs
> >
> > (editorial?) I think I can convince myself to read this with two different
> > causalities -- either the HMAC TLV is only allowed to appear in
> > authenticated-mode packets (but can also appear in unauthenticated-mode
> > packets where the only other TLV is the Extra Padding TLV); or in the
> > case when you are sending [this-document] packets in authenticated mode,
> > you MUST also have an HMAC TLV, unless you're sending unauthenticated
> > extended packets where the only TLV present is the Extra Padding TLV.
> > The first interpretation doesn't really make much sense, and the second
> > one is pretty consistent with the follow-up note about "HMAC TLV MAY be
> > used [...] in unauthenticated mode".  But I would still suggest
> > rewording for clarity, perhpas "all authenticated STAMP packets
> > compatible with this specification MUST additionally authenticate the
> > option TLVs by including the HMAC TLV, with the sole exception of when
> > there is only one TLV present and it is the Extended Padding TLV".
> >
> GIM>> Many thanks for this detailed analysis and the most helpful text.
> Indeed, the goal was and is that HMAC TLV MUST be used in the authenticated
> STAMP mode. I've used your text to have the following update:
> OLD TEXT:
>    The keyed Hashed
>    Message Authentication Code (HMAC) TLV MUST be included in a STAMP
>    test packet in the authenticated mode, excluding the case where the
>    only TLV present is the Extra Padding TLV.
> NEW TEXT:
>    All authenticated
>    STAMP base packets (per Section 4.2.2 and Section 4.3.2 [RFC8762])
>    compatible with this specification MUST additionally authenticate the
>    option TLVs by including the keyed Hashed Message Authentication Code
>    (HMAC) TLV, with the sole exception of when there is only one TLV
>    present, and it is the Extended Padding TLV.

Thanks!

> >
> >    fully applicable to the use of the HMAC TLV.  HMAC is calculated as
> >    defined in [RFC2104] over text as the concatenation of all preceding
> >    TLVs.  [...]
> >
> > As the secdir reviewer noted, this makes the options severable from the
> > base packet (and separately replayable, the "mix and match" nature).
> > >From a cryptographic point of view it's much preferred to include at
> > least some of the base packet (ideally all, though that of course makes
> > precise egress timestamping hard) as HMAC input, to bind the two
> > together.  Even including just the sequence number(s) would be a big
> > win.
> >
> GIM>> Thank you for the detailed explanation of the issue. Would the
> updated procedure described below be acceptable:
>    HMAC is calculated as
>    defined in [RFC2104] over text as the concatenation of the Sequence
>    Number field of the base STAMP packet and all preceding TLVs.

That would be acceptable, yes; thank you.

> >
> >    packet.  The Session-Reflector MUST copy the remainder of the
> >    extended STAMP test packet into the reflected packet.  The Session-
> >    Reflector MUST set the A flag in the copy of the HMAC TLV in the
> >    reflected packet to 1 before transmitting the reflected test packet.
> >
> > This phrasing is a bit weird -- is "the remainder" of the packet all the
> > other TLVs, or just the stuff after the HMAC TLV?  We're supposed to
> > process HMAC first, after all, but that's only partially clear from the
> > rest of the section/document...
> >
> GIM>> Thank you for pointing to this. Indeed, none of TLVs are in the
> reflected packet when HMAC verification fails.
> Does re-wording make it right:
>    The Session-
>    Reflector MUST copy the TLVs from the received STAMP test packet into
>    the reflected packet.

(IIRC the question of whether or not to set the 'A' flag in all of them is
under discussion elsewhere.  The main change here is fine, though.)

> >
> > Section 5.1
> >
> >    This document defines the following new values in the STAMP Extension
> >    TLV range of the STAMP TLV Type registry:
> >
> > This appears to be the only instance of "STAMP Extension TLV range" in
> > the document; is this perhaps meant to refer to the "IETF Review" or
> > 1-175 range?
> >
> GIM>> Thank you for pointing this out. Will use the "IETF Review".
> 
> >
> > Section 5.4
> >
> > Do we feel a need to provide any kind of definition for "HW Assist", "SW
> > local", and/or "Control plane"?
> >
> GIM>> That is the best classification that could produce. Strictly
> speaking, even SW local does use HW assist to obtain the time stamp. Would
> much appreciate your suggestions on how to make it clearer, a bit more
> formal.

I'm not coming up with anything that I'm happy with, sorry.

> >
> > Section 6
> >
> > This would be a fine place to reiterate that both Sender and Reflector
> > should have parsers written defensively to protect against malformed
> > (i.e., bad TLV length) input); as I noted previously, the current text
> > really only suggests that the reflector should do so, but the sender is
> > not immune.
> >
> GIM>> Propose a new text in the Security Considerations section:
>    To protect against a malformed TLV an implementation of a Session-
>    Sender and Session-Reflector MUST:
>    o  check the setting of the M flag;
>    o  validate the Length field value.

Thanks!

> >
> > If we leave the U-flag semantics unchanged, we also need to document the
> > potential ambiguity between a reflector that blindly reflects the whole
> > message and one that actually understands all the received TLVs.
> >
> GIM>> As mentioned above, the initial setting of the U flag by a
> Session-Sender now is to 1.
> 
> >
> > There's also some interesting considerations about the state of the 'A'
> > bit not itself being covered by the response's HMAC TLV (if it is only
> > set in the HMAC TLV vs. all TLVs).  I would strongly recommend having
> > Section 4.8 require that the A flag be set in *all* TLVs if
> > authentication fails, so that this information is covered by the
> > response's HMAC TLV.  (In the degenerate case of only an HMAC TLV it's
> > still unprotected, but the HMAC itself is over the empty message in that
> > case so it's not terribly interesting to mess with.)  Also, the
> > definition of the A flag in Section 4 could probably be tightened up,
> > whichever way we land on this point -- right now it suggests that it
> > might be set on more than just the HMAC TLV but is not very clear.
> > (Interestingly, later on in Section 4 we do seem to say that you have to
> > *check* the A flag in every TLV.)
> >
> GIM>> After re-reading the definition of the I flag (former A flag) I see
> that it can be interpreted as being set in all TLVs when HMAC verification
> fails. I've updated the text in Section 4.8 to make the processing of the
> failed verification consistent with the definition:
> If the HMAC TLV is present in the extended STAMP test packet,
>    e.g., in the authenticated mode, HMAC MUST be verified before using
>    any data in the included STAMP TLVs.  If HMAC verification by the
>    Session-Reflector fails, then the Session-Reflector MUST stop
>    processing the received extended STAMP test packet.  The Session-
>    Reflector MUST copy the TLVs from the received STAMP test packet into
>    the reflected packet.  The Session-Reflector MUST set the I flag in
>    each TLV copied over into the reflected packet to 1 before
>    transmitting the reflected test packet.  If the Session-Sender
>    receives the extended STAMP test packet with I flag set to 1, then

Maybe "set to 1 in any TLV"?  There could be more than on I flag, in
general, I think.

>    the Session-Sender MUST stop processing TLVs in the reflected test
>    packet.  If HMAC verification by the Session-Sender fails, then the
>    Session-Sender MUST stop processing TLVs in the reflected extended
>    STAMP packet.
> 
> Does it work now?

I think so, yes.

> 
> >
> > We might also consider referencing RFC 2474 for the DSCP security
> > considerations.
> >
> GIM>> I propose a short new paragraph appended to the security section:
>    Monitoring and optional control of DSCP does not appear to introduce
>    any additional security threat to hosts that communicate with STAMP
>    as defined in [RFC8762].  As this specification defined the mechanism
>    to test DSCP mapping, this document inherits all the security
>    considerations discussed in [RFC2474].
>  And RFC 2474 listed as Normative reference. What do you think?

It has a good overall structure, but I'm not entirely sure that it's
correct.  Specifically, adding "optional control of DSCP" might actually be
a new thing here -- as far as I know, RFC 2474 envisioned it being set
solely as a local policy (albeit perhaps informed by the inbound value with
intent to provide continuity of packet policy).  The semantics of the -07
require setting the supplied DSCP value without any sanity-checking, which
could cause network instability in certain cases (e.g., with a low-ish
capacity network and a DSCP value that strongly deprioritizes other
traffic).

> >
> > Section 9.1
> >
> > It's not entirely clear that RFC 5357 needs to be normative; we say we
> > can be compatible with lite-mode and that the extended padding is
> > similar, but those are just facts and you don't need to know anything
> > from 5357 to implement this document.
> >
> GIM>> I agree with your reasoning. Moved it to the Informational list.
> 
> >
> > Similarly, I'm not sure why [TS23501] is listed as normative.
> >
> GIM>> The Access Report TLV is based on 3GPP specification of the PMF. On
> the other hand, the document provides all the necessary information from
> that spec. I've moved to the Informational list in the working version.
> 
> >
> > Section 9.2
> >
> > On the other hand, you do actually need to read RFC 2104 to implement
> > the HMAC TLV, which makes it a normative reference (similarly for 4868)!
> >
> GIM>> Thank you. Moved it up.

Thanks for these and all the updates!

[trimmed the diff, but I read through it and have no further comments since
you gave the discliamer that Location TLV is still WIP]

-Ben