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

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

Hi Ben, 

Totally agree now that I've re-read it. 

Here's the new proposed wording. 

"The packets that are to be associated to an identifier are selected according to Section 5.2 of [RFC4821]."

Here's the full text. 

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 an identifier are selected 
according to Section 5.2 of [RFC4821]."

Thanks,
-Felipe

On 6/27/19, 12:00 AM, "Benjamin Kaduk" <[e-mail]> wrote:

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