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

[Benjamin Kaduk <[e-mail]>]

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.

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.


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

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.

Section 4.2.4

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

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.

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.

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

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.