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

[Benjamin Kaduk <[e-mail]>]

Hi Felipe,

Thanks for following up -- it looks like I filed away the original mail
without responding to it, somehow(!)
The Discuss point's resolution is fine; I just have one more question
(inline).

-Ben

On Wed, Jun 26, 2019 at 04:01:58AM +0000, Felipe Garrido (fegarrid) wrote:
> Hi Benjamin,
> 
> Just following up on my previous email. Let me know if the below response satisfies your comments.
> 
> Thanks,
> -Felipe
> 
> From: "Felipe Garrido (fegarrid)" <[e-mail]>
> Date: Tuesday, May 21, 2019 at 11:59 AM
> To: "[e-mail]" <[e-mail]>, "[e-mail]" <[e-mail]>
> Cc: "[e-mail]" <[e-mail]>, "[e-mail]" <[e-mail]>, "[e-mail]" <[e-mail]>, "[e-mail]" <[e-mail]>
> Subject: Benjamin Kaduk's Discuss on draft-ietf-tram-stun-pmtud-10: (with DISCUSS and COMMENT)
> 
> 
> 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.”

I still don't understand what "the selection process specified in Section
5.2 of [RFC4821]" is -- can you point me to the text from RFC 4821
describing the process in question?

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