Re: [tram] Review of draft-ietf-tram-stun-pmtud-03

[Brandon Williams <[e-mail]>]

Looks good to me.

--Brandon

On 02/20/2017 02:17 PM, Gonzalo Salgueiro (gsalguei) wrote:
> Thanks for the re-review, Brandon.  We posted a new version (-05) that
> hopefully addresses all your points below.  Please confirm.
>
> Responses inline...
>
>> On Feb 20, 2017, at 12:27 PM, Brandon Williams
>> <[e-mail] <mailto:[e-mail]>> wrote:
>>
>> Hi Marc,
>>
>> Thanks. It looks like most of my comments are resolved.
>>
>> After re-reading, I've still got a few comments, at least one of which
>> I see that I missed on the previous pass. My new comments are below.
>> Nothing is in-line. Let me know if anything is unclear.
>>
>> --Brandon
>>
>> Section 4
>>
>> I suggest moving the first paragraph "A client MUST NOT ..." to section
>> 4.1.1, since it's about forming/sending a message. Seems like it would
>> be a good last paragraph in that section.
>
> Fixed.
>
>> Section 4.1.1 para 2
>>
>> s/transctaion/transaction/
>
> Fixed.
>
>> Section 4.1.1 para 3
>>
>> My question about FINGERPRINT was meant to indicate that the text
>> should say why it's needed. I see you've added that explanation
>> elsewhere. Maybe add the same here too.
>
> Fixed.
>
>> Section 4.2.1 para 1
>>
>> s/The server/The client/
>
> Fixed.
>
>> Section 4.2.5
>>
>> I still find "the same as" in para 1 confusing here since it's not
>> applied to any header fields. Maybe "similar to”?
>
> Fixed.
>
>> Also, I found para 2 a little difficult to read. How about:
>>
>>  For each STUN Probe Indication or Request, the server retrieves the
>>  STUN FINGERPRINT value. For all other packets, the server calculates
>>  the checksum as described above. It puts these FINGERPRINT and
>>  checksum values in a chronologically ordered list that is sent back
>>  in the Report Response.
>
> Fixed.
>
>> Section 5.2
>>
>> I'm still uncertain about this section. The receiver behavior makes
>> sense, but that's not really what I was questioning. It's not clear
>> what you intend for the original probe sender whose support for PMTUD
>> is being assumed by the receiver. How did that first machine know that
>> it was safe to send its probe? This section seems to imply that it
>> injects probes as a way to tell the receiver that it supports the
>> protocol, though you're right, it doesn't actually say that so I think
>> I just misunderstood your intention.
>>
>> So, how does a machine that doesn't know whether the receiver supports
>> the PMTUD protocol know whether it's safe to send a probe when it
>> hasn't received one from the other party? Is the idea that one side or
>> the other must receive this information via signaling or some other means?
>
> We believe this is answered in Section 5.1. In fact, Section 5.1. states
> when the client can send the probe, whereas Section 5.2 states when the
> server can become a client and send to the original client.
>
>> I see the paragraph in section 4 that I previously missed which states
>> the client MUST NOT send if it doesn't know the other party supports
>> it. Maybe section 5.2 could say something like "In cases where only
>> one endpoint (the initial client) knows that its peer endpoint (the
>> initial server) supports PMTUD, the initial client sends a Probe
>> Request or Indication to the initial server." That seems a little
>> clunky, but I think you'll get the point of my suggestion.
>
> We updated the text, not with your exact proposed text, but with text
> that blended a bit better with the existing text and (hopefully)
> addressed the source of your uncertainty.
>
>>
>> Section 6.1 para 2
>>
>> Maybe you could indicate that section 4.2.5 and 4.2.6 describe two
>> methods for acquiring and formatting the identifiers that could be used
>> for this purpose. It's clear to me that this is the case, but the
>> current text seems to imply that no options are provided by this
>> document.
>
> Fixed.
>
> Cheers,
>
> Gonzalo
>
>
>>
>>
>> On 02/16/2017 02:52 PM, Marc Petit-Huguenin wrote:
>>> Hi Brandon,
>>>
>>> Thanks again for the review, and sorry for the delay in responding.
>>>
>>> See my comments inline.  A new version will be published in a few
>>> minutes, please have a look too.
>>>
>>> Thanks.
>>>
>>> On 12/27/2016 11:27 AM, Brandon Williams wrote:
>>>> Hi all,
>>>>
>>>> Apologies for taking so long to complete a review of this one.
>>>>
>>>> Although my list of notes is long, there's nothing really
>>>> substantive here. All of my comments are just suggestions to improve
>>>> the document's clarity. Incorporate or discard as you see fit.
>>>>
>>>> Please let me know if any of the following is unclear.
>>>>
>>>> --Brandon
>>>>
>>>>
>>>> 1. Introduction
>>>>
>>>> I find the second paragraph a tiny bit difficult to read. Here's a
>>>> suggestion.
>>>>
>>>> OLD:
>>>>
>>>>  This document only describes how probing mechanisms are implemented
>>>>  with Session Traversal Utilities for NAT (STUN). The algorithm to
>>>>  find the Path MTU is described in [RFC4821].
>>>>
>>>> NEW:
>>>>
>>>>  This document describes probing mechanisms for Session Traversal
>>>>  Utilities for NAT (STUN) for use in implementing the Path MTU
>>>>  Discovery algorithm described in [RFC4821].
>>>
>>> Your text change a bit the meaning I was trying to convey in the
>>> original text.  What about something like the following text:
>>>
>>> New:
>>>
>>>   Not all UDP-based implement the Path
>>>   MTU discovery mechanism described in [RFC4821].  These protocols can
>>>   make use of the probing mechanisms described in this document instead
>>>   of designing their own adhoc extension.  These probing mechanisms are
>>>   implemented with Session Traversal Utilities for NAT (STUN), but
>>>   their usage is not limited to STUN-based protocols.
>>>
>>>>
>>>>
>>>> I also find the fourth paragraph a tiny bit difficult to read.
>>>>
>>>> OLD:
>>>>
>>>>  Additional network characteristics like the network path (using the
>>>>  STUN Traceroute mechanism described in
>>>>  [I-D.martinsen-tram-stuntrace]) and bandwidth availability (using the
>>>>  mechanism described in [I-D.martinsen-tram-turnbandwidthprobe]) can
>>>>  be discovered using complementary techniques.
>>>>
>>>> NEW:
>>>>
>>>>  Complimentary techniques can be used to discover addition network
>>>>  characteristics, such as the network path (using the STUN Traceroute
>>>>  mechanism described in [I-D.martinsen-tram-stuntrace]) and bandwidth
>>>>  availability (using the mechanism described in
>>>>  [I-D.martinsen-tram-turnbandwidthprobe]).
>>>
>>> Done.
>>>
>>>>
>>>> 2. Overview of Operations
>>>>
>>>> In paragraph three, the first "it" in the paragraph should be expanded.
>>>>
>>>> OLD: ... then it initiates Probe transactions, ...
>>>>
>>>> NEW: ... then the Client initiates Probe transacations, ...
>>>
>>> Done.
>>>
>>>>
>>>>
>>>> Same comment for the next paragraph.
>>>>
>>>> OLD: ... then it sends Probe Indications of various sizes ...
>>>>
>>>> NEW: ... then the Client sends Probe Indications of various sizes ...
>>>
>>> Done.
>>>
>>>>
>>>>
>>>> 4. Probing Mechanisms
>>>>
>>>> In paragraph six, I think the SHOULD requirement about retransmissions
>>>> belongs in section 4.1.1, which already describes this requirement in
>>>> other terms ("the Rc parameter ... is set to 3."). This part of section
>>>> 4 should stick to providing the MUST/MAY requirements for which of the
>>>> mechanisms are to be implemented.
>>>
>>> Sentence removed.
>>>
>>>>
>>>> Additionally, I think that paragraph six could be moved to be text
>>>> for the currently empty section 4.1. Simple Probing Mechanism. In
>>>> that case, paragraphs 7-10 could be moved to be text for the
>>>> currently empty section 4.2. Complete Probing Mechanism. I think
>>>> moving the high-level descriptions of the two mechanisms into their
>>>> respective sections improves the document flow.
>>>
>>> Done.
>>>
>>>>
>>>> Finally, in the last paragraph of the section, it states "... the
>>>> client adds a sequence number in front of each UDP packet sent."
>>>> I think you mean "... the client prepends the UDP data with a
>>>> header that provides a sequence number." At least, this is what the
>>>> description in the later section indicates to me.
>>>
>>> Done.
>>>
>>>>
>>>>
>>>> 4.1.1. Sending a Probe Request
>>>>
>>>> This section should indicate the STUN method type used for a Probe
>>>> Request (prefered) or forward reference section 8.1.
>>>
>>> Done, using your preference.
>>>
>>>>
>>>> Regarding authentication, why would an implementation authenticate
>>>> the Probe transaction? Is doing so recommended? If so, why? This is
>>>> discussed later in security considerations, but it was unclear to me
>>>> at this point in the document.
>>>
>>> Rewrote the text with a bit more explanations.
>>>
>>>>
>>>> Regarding the FINGERPRINT attribute, why is it required?
>>>
>>> The STUN messages need to be sent together with the packets of the
>>> the other protocol, which can be anything, so the FINGERPRINT helps
>>> differentiate STUN packets.
>>>
>>>>
>>>>
>>>> 4.1.2. Receiving a Probe Request
>>>>
>>>> Here again, why is the FINGERPRINT attribute required?
>>>
>>> See above.
>>>
>>>>
>>>>
>>>> 4.1.3. Receiving a Probe Response
>>>>
>>>> OLD: If the Probe transactions times out ...
>>>>
>>>> NEW: If the Probe transaction times out ...
>>>
>>> Done.
>>>
>>>>
>>>>
>>>> 4.2.1. Sending the Probe Indications and Report Request
>>>>
>>>> Why MUST the Probe Indication be authenticated, when this is not
>>>> required (only allowed) for a Probe Request? Here again, it's
>>>> discussed later, but was unclear to me here.
>>>
>>> Same than above, clarified the text.
>>>
>>>>
>>>> Also, same question as above about FINGERPRINT here.
>>>
>>> See above.
>>>
>>>>
>>>> This section should indicate the STUN method type used for a
>>>> Report Request (prefered) or forward reference section 8.1.
>>>>
>>>
>>> Done, using your preference.
>>>
>>>>
>>>> 4.2.3. Receiving a Probe Indication and Report Request
>>>>
>>>> I suggest that you forward reference the section that describes
>>>> construction of the IDENTIFIERS attribute in the last paragraph.
>>>
>>> Done.
>>>
>>>>
>>>>
>>>> 4.2.4. Receiving a Report Response
>>>>
>>>> The description of Probe Failure is a bit incomplete. It should
>>>> include a little more detail.
>>>>
>>>> OLD:
>>>>
>>>>  If the Probe Indication identifier cannot be found in the Report
>>>>  Response, this is interpreted as a Probe Failure, as defined in
>>>>  [RFC4821] Section 7.5.
>>>>
>>>> NEW:
>>>>
>>>>  If the Probe Indication identifier cannot be found in the Report
>>>>  Response but identifiers for other packets sent before or after
>>>>  the Probe Indication can all be found, this is interpreted as a
>>>>  Probe Failure as defined in [RFC4821] Section 7.5.
>>>
>>> Done.
>>>
>>>>
>>>>
>>>> 4.2.5. Using Checksums as Packet Identifiers
>>>>
>>>> The second paragraph states "The algorithm used to calculate the
>>>> checksum is the same as the algorithm used for the FINGERPRINT
>>>> attribute." I find this description a bit unclear, since the
>>>> description of the FINGERPRINT calculation in RFC5389 discusses
>>>> the STUN header fields, etc. Here, you're just talking about
>>>> generating a CRC-32 value XORed with 0x5354554e. It might be
>>>> more clear to just say that.
>>>
>>> Done.
>>>
>>>>
>>>>
>>>> 5.2. Implicit Probe Support Signaling Mechanism
>>>>
>>>> I'm a little concerned about the idea of just injecting probes
>>>> as a way to figure out whether the receiver supports them. It's
>>>> hard to know for every possible protocol that this will work and
>>>> will not break the protocol. Maybe add a cautionary note.
>>>
>>> No, the idea is not to inject probes to detect support.  Here we know
>>> that the receiver will support receiving probes, because it acted as
>>> a probe sender.  The sender of a probe implicitly signals that it can
>>> itself receive and process probes.
>>>
>>> Section 4 states that all implementations of this specification MUST
>>> implement the server side of both probing mechanisms, and this is how
>>> the implicit signaling mechanism can work.
>>>
>>>>
>>>>
>>>> 6.1. IDENTIFIERS
>>>>
>>>> Sections 4.2.5. and 4.2.6. each describe 32 bit identifiers. I
>>>> think section 6.1. should say a little more than just "contents
>>>> are opaque". I suggest that you describe specifically what the
>>>> attribute value will look like if one of the two 32 bit schemes
>>>> is used and declare that other protocols may generate different
>>>> values.
>>>>
>>>
>>> All right, that was the difficult part.  When trying to do what you
>>> suggested, I discovered that there was no mechanism to put the Probe
>>> packets in the list in the IDENTIFIERS attribute.  So the text in
>>> both section was rewritten to add that possibility.  The sections
>>> also contain the description of the content of the IDENTIFIERS attribute.
>>>
>>>>
>>>> 6.2. PMTUD-SUPPORTED
>>>>
>>>> For clarity, I suggest something a little more like the value
>>>> description of DONT-FRAGMENT from RFC5766.
>>>>
>>>> OLD: This attribute is empty.
>>>>
>>>> NEW:
>>>>
>>>>  This attribute has no value part and thus the attribute
>>>>  length field is 0.
>>>>
>>>
>>> Done.
>>>
>>
>> --
>> Brandon Williams; Chief Architect
>> Cloud Networking; Akamai Technologies Inc.
>>
>> _______________________________________________
>> tram mailing list
>> [e-mail] <mailto:[e-mail]>
>> https://www.ietf.org/mailman/listinfo/tram
>> <https://urldefense.proofpoint.com/v2/url?u=https-3A__www.ietf.org_mailman_listinfo_tram&d=DwMGaQ&c=96ZbZZcaMF4w0F4jpN6LZg&r=bwZ-nnRGWmcGKRIuadq6-NSnsgwbBVUJa4mZfmEIBXg&m=119_atpYkoATgcaHTtCqVlTio6bj6SnMszldKzR78W4&s=LfZktBcJfpC7O9e21qX1vopnfrr_5MJVbDd2qRo9wfE&e=>
>

-- 
Brandon Williams; Chief Architect
Cloud Networking; Akamai Technologies Inc.