Simple Two-Way Active Measurement Protocol Optional Extensions
draft-ietf-ippm-stamp-option-tlv-10

[ general ]

* I sill find the access ID being a boolean for 3GPP or not being a bit
  odd, but not super important I guess.

[ section 4 ]

*  "All multibyte fields in the defined in this specification TLVs are in
   network byte order."

   -->

   "... fields defined in this spec..."?

Thanks for addressing my DISCUSS point.  I've left my original (non-blocking) comments here for the record.

--

I'll open with my usual complaint about having more than five authors, which is the recommended maximum.

Abstract:

* "... in addition to ones supported by the STAMP base specification." -- I think you could remove this clause; it's enough to say you're defining extension(s).

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." -- Why is this only a SHOULD?  When might one legitimately deviate from this requirement?

Section 4.3:

* "The Session-Sender SHOULD NOT fill any information fields except for STAMP TLV Flags, Type, and Length." -- When might I deviate from that advice, as SHOULD NOT allows?

Section 4.5:

* "The definition of "in-profile packet" is outside the scope of this document and is left to the test operators to determine." -- This isn't my domain, to be sure, but I'm curious about what "in-profile" might mean.  Would it be possible to include an example?

Section 4.6:

* Same question for "access network".

Section 4.8:

* "... excluding when ..." -- suggest instead "excluding the case where"

Thank you for responding to the SECDIR review (and thank you to Charlie Kaufman for doing it).

Thanks for addressing my DISCUSS and COMMENT points.  As noted in the thread, I'd also recommend the following: 

** Section 4.2.  Section 4.5 of RFC8762 already has helpful guidance word on confidentiality (and it cite it in the Security Considerations generically).  I woul suggest you reiterate that guidance in particular for this TLV because of the information it carries.

Thank you for the work put into this document. 

Please find below a couple of non-blocking COMMENTs (and I would appreciate a reply to my COMMENTs).

I hope that this helps to improve the document,

Regards,

-éric

== COMMENTS ==

-- Section 4 --
The text "A system that has received a STAMP test packet with extension TLVs MUST validate each TLV:" is unclear whether it is Session-Sender or Session-Reflector or both. Clarification welcome.

The A-bit is linked to HMAC and looks like "Authentication" while section 4.8 says HMAC is only for integrity. Hence, why not using "I-bit" rather than "A-bit" (cosmetic issue)

-- Section 4.1 --
For the 'Extra Padding' field, "a pseudo-random sequence of bits.  The field MAY be filled with all zeros.". Is it 'bits' or 'octets' ? (the latter most probably). I also have hard time to reconciliate "pseudo-random" with "MAY be filled with 0"; this seems like an oxymoron to me.

-- Section 4.2 --
How to handle cases where the MAC address is not 6-bytes long ? AFAIK IEEE 802.15.4 uses 16 or 64-bit addresses and future MAC layers could use different MAC addresses length. What should be the MAC address field value when there is no associated MAC address?

-- Section 4.8
What is the expected behavior of a Session-Sender when receiving a STAMP packet whose HMAC verification fails ?

I must also say that I am a little puzzled by the Session-Reflector behavior when HMAC verification fails: logging and replying anyway seems opening an avenue for STAMP service theft or DoS. Or am I missing something ?

-- Section 5.3 --
In table 5, is there a reason why Galileo is not listed ?

I support Murray's DISCUSS and Roman's first DISCUSS point.

(1) Figures 1 and 2 are labeled "an example" -- but they are not examples.

(2) §3: "If the test session is not stopped, the Session-Sender, can, for example, send a base STAMP packet [RFC8762]."  Just to make sure I understand: if the Session-Reflector doesn't support this specification, then a packet with or without an SSID will look the same to them, right?  If so, then there's no need for the Session-Sender to change its packets -- may be worth a mention.

(3) §4: "the TLV is malformed, i.e., the Length field value of the fixed-size TLV is not equal to the value defined for the particular type".  The Length can also be incorrect for variable-size TLVs -- so perhaps change to "the TLV is malformed, i.e., the Length field value is not valid for the particular type"

(4) §4: "MUST try to process"  "Do or do not. There is no try." - Yoda. ;-)  Seriously...consider deleting this sentence as skipping the TLV should already result in processing the next one, if present.

(5) The Reference column in tables 1/4/6/8 mixes references and assignment policies.  The whole registry should reference this document...so the column should really point at the assignment policies.

(6) Table 2: The new registry is being defined in this document, so you can go ahead and assign values -- no need to wait for IANA.

(7) Nits:

s/SSID/the SSID/g

s/vendor's the Structure/vendor's Structure

s/extend STAMP/extend the STAMP

s/is Extra/is the Extra

s/one-octet-long Type field, and two-octet-long/a one-octet-long Type field, and a two-octet-long

s/equals length/equals the length

Thank you for addressing my Discuss (and Comment!) points.

I support Ben's discuss regarding check the length of the STAMP packet to determine whether it is a base or extended STAMP packet.

Comments:
Thank you for this document.  By and large I found this document fairly easy to read and understand, but have comments on a few areas:

I found the document slightly confusing in that the extended stamp packets are defined under the "STAMP Test Session Identifier" section.  I would have found the document to be more coherent if the new extended packet structure was pulled into its own section, perhaps with "STAMP Test Session Identifier" as a sub-section.

Perhaps it might be helpful to explain why the MBZ sections are where they are - I presume to still conform with the structure of the base STAMP packet formats for session-reflectors that do not understand the new format.

In section 4:  TLV Extensions to STAMP

   A system that has received a STAMP test packet with extension TLVs
   MUST validate each TLV:

      If the U flag is set, the STAMP system MUST skip the processing of
      the TLV.  The implementation MUST try to process the next TLV if
      present in the extended STAMP packet.

      If the L flag is set, the STAMP system MUST stop processing the
      remainder of the extended STAMP packet.

      If the A flag is set, the STAMP system MUST discard all TLVs and
      MUST stop processing the remainder of the extended STAMP packet.

I was initially surprised that different behavior was specified for the handling for the U, L and A flags given that the sender sets these to zero.  I presume that the aim here is just to have commonality between the Session Sender and Session Reflectors?  It might help clarify whether this section applies to both sender and reflector.

In section 4.4:

   o  DSCP2 - The received value in the DSCP field at the Session-
      Reflector in the forward direction.

   o  ECN - The received value in the ECN field at the Session-Reflector
      in the forward direction.
      
These are the only two cases of forward direction.  Would it be better to describe this as "ingress of the Session-Reflector", e.g. similar to the description in the previous section?

4.5.  Direct Measurement TLV

   o  Session-Reflector Rx counter (R_RxC) is a four-octet-long field.
      MUST be zeroed by the Session-Sender on transmit and ignored by
      the Session-Reflector on receipt.  The Session-Reflector MUST fill
      it with the value of in-profile packets received.
      
"in-profile packets received" is unclear to me.

   o  Session-Reflector Tx counter (R_TxC) is a four-octet-long field.
      MUST be zeroed by the Session-Sender and ignored by the Session-
      Reflector on receipt.  The Session-Reflector MUST fill it with the
      value of the transmitted in-profile packets.
      
Presumably the Session-Reflector needs to be configured via an out of band mechanism to specify how many in-profile packets are transmitted, and what those packets are.  Further text in the beginning of section 4.5 may be helpful here.

4.6.  Access Report TLV

This section seems to describe more than just a TLV given that it seems to put additional constraints on the implementation (e.g. use of a retransmission timer).  Possibly the document would have been more clear to split the definition of the access report handling separately (e.g. as a separate section earlier in the document) from the Access Report TLV definition.

Regards,
Rob