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

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

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