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

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

Ben, 

I agree with the improvement but would rather use the same terminology as in rfc4821. 
It' would be, 

The packets that are to be associated to a given flow's identifier are selected according to Section 5.2 of [RFC4821]."

Thanks,
-Felipe

On 8/19/19, 7:50 PM, "Benjamin Kaduk" <[e-mail]> wrote:

    Hi Felipe,
    
    The new wording is at least enough for me to come up with what seems like a
    plausible guess for what the identifier (and algorithm thereto) is supposed
    to be, but I do still feel like I'm guessing.  If it's the path identifier
    used to associate stored (PL)PMTU state across different flows (which is my
    guess), then maybe "associated to a given path's identifier" would be a
    further improvement?
    
    Thanks,
    
    Ben
    
    On Mon, Aug 19, 2019 at 02:13:13PM +0000, Felipe Garrido (fegarrid) wrote:
    > Hi Ben, 
    > 
    > Do you agree with the latest wording changes? A new draft version has been published with them.
    > 
    > Thanks,
    > -Felipe
    > 
    > On 7/8/19, 10:43 AM, "tram on behalf of Felipe Garrido (fegarrid)" <[e-mail] on behalf of [e-mail]> wrote:
    > 
    >     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.”
    >         > 
    >         > 
    >         > 
    >         > 
    >         > 
    >         
    >     
    >     _______________________________________________
    >     tram mailing list
    >     [e-mail]
    >     https://www.ietf.org/mailman/listinfo/tram
    >     
    >