[tram] Benjamin Kaduk's Discuss on draft-ietf-tram-stun-pmtud-10: (with DISCUSS and COMMENT)

["Felipe Garrido (fegarrid)" <[e-mail]>]

Hi Benjamin,

Apologies for the delay in responding, the current authors are having scheduling conflicts and have added me to address the current concerns. Please see my responses inline.

thanks
-Felipe


Benjamin Kaduk has entered the following ballot position for
draft-ietf-tram-stun-pmtud-10: 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-tram-stun-pmtud/



----------------------------------------------------------------------
DISCUSS:
----------------------------------------------------------------------

I was going to report the same thing as Adam, but will just say that I support his Discuss.
[FG]: I’ll be addressing this Discuss in Adam’s feedback.

I also have one other (also minor and easy to resolve) Discuss point:  Section 4.2.6 needs
to state what the Length field is measuring the length of.
[FG]: Agree that this is required. Adding the following text to Section 4.2.6.
“The Length field specifies the length in bytes of the sequence number and application data fields.”

----------------------------------------------------------------------
COMMENT:
----------------------------------------------------------------------

I understand that this document inherently has to be incomplete and "vague",
since the procedure specified within is only meaningful in the context of a
STUN usage or other protocol.  But in general it seems like there could be
greater clarity even within the constraints that we must work under.  My
points are probably less interesting than the ones Adam raised already, though.
The only general observation in this space that I can offer is that some parts of
the text read as if only the Probe packets are going to be monitored for the
report (but this is clearly not the case given the document as a whole).

Section 4.2

  The Complete Probing mechanism is implemented by sending one or more
  Probe Indications with a PADDING attribute over UDP with the DF bit
  set in the IP header followed by a Report Request to the same server.
  A router on the path to the server can reject this Indication with an
  ICMP message or drop it.

nit: I don't think "this" is the right word; perhaps "each" would be
better.


[FG]: Agree, updates will be made.

Section 4.2.3

  A server supporting this specification will keep the identifiers of
  all packets received in a chronologically ordered list.  The packets
  that are to be associated to a list are selected according to
  Section 5.2 of [RFC4821].  [...]

4821 doesn't talk about "list"s at all, and in fact the indicated section
seems to be talking more about where to store a PMTU value after it has
been determined, rather than what packets to be considering for a report.
So I'm pretty confused about what this sentence is trying to say.


[FG]: Agree. Updated wording to make the statement easier to read.
“The selection process specified in Section 5.2 of [RFC4821] is to be used to determine whether a packet is added with a list.”

Section 4.2.4

nit: I think that all instances of "the Probe Indication" should be
replaced with "a Probe Indication", in this section.


[FG]: Agree, updates will be made.

Section 4.2.5

  When using a checksum as a packet identifier, the client calculates
  the checksum for each packet sent over UDP that is not a STUN Probe
  Indication or Request and keeps this checksum in a chronologically
  ordered list.  The client also keeps the checksum of the STUN Probe
  Indication or Request sent in that same chronologically ordered list.
  The algorithm used to calculate the checksum is similar to the
  algorithm used for the FINGERPRINT attribute (i.e., the CRC-32 of the
  payload XOR'ed with the 32-bit value 0x5354554e [ITU.V42.2002]).

(editorial) It's pretty confusing to start out with the split between STUN
and non-STUN messages, only later to clarify that this is because the
FINGERPRINT is used for STUN messages.  So maybe:

 When using a checksum as a packet identifier, the client keeps a
 chronologically ordered list of the packets it transmits, along with an
 associated checksum value.  For STUN Probe Indication or Request packets,
 the associated checksum value is the FINGERPRINT value from the packet; for
 other packets a checksum value is computed using a similar algorithm to the
 FINGERPRINT calculation.


[FG]: Agree with changing of the language. It doesn’t change the content and easier to read.

Section 4.2.6

  When using sequence numbers, a small header similar to the TURN
  ChannelData header [...]

Probably want an informative reference for this header.


[FG]: Agree, updates will be made to reference.
Section 6.2

6.2.  PMTUD-SUPPORTED

  The PMTUD-SUPPORTED attribute indicates that its sender supports this
  specification.  This attribute has no value part and thus the
  attribute length field is 0.

"this specification" is not sufficiently detailed to interoperate, so I
think this needs to be qualified as more like "supports this mechanism, as
incorporated into the STUN usage or protocol being used".


[FG]: Agree, updates will be made.

Section 7

The contents of the PADDING do not seem to be specified anywhere, so it
could in theory be used as a side channel to convey other information,
which has some potential privacy considerations.  Nowadays we tend to ask
for the value of the padding bytes to be deterministic (but validation
remains optional); I forget if there are STUN-specific considerations that
would discourage just setting them all to zero.


[FG]: Agree.  Adding language to state contents of PADDING.
“The padding bits MUST be set to zero on sending and MUST be ignored by the receiver.”