Summary: Has 4 DISCUSSes. Needs 3 more YES or NO OBJECTION positions to pass.
I was going to report the same thing as Adam, but will just say that I support his Discuss. 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.
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. 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. Section 4.2.4 nit: I think that all instances of "the Probe Indication" should be replaced with "a Probe Indication", in this section. 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. 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. 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". 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.
Section 4.1.x and 4.2.x Please specify how this simple probing mechanism will work with IPv6. It shouldn't be too difficult to do (cleanup references to the DF bit, use Type 2 "Packet Too Big" to identify failures etc.). Similar treatment will be required for the complete probing mechanism as well.
[Updated because I forgot one point] Based on the transport review provided by Gorry (Thanks!), please clarify the applicability (as you claim the "usage is not limited to STUN-based protocols") and the relation to draft-ietf-tsvwg-datagram-plpmtud. Further, more discussion on impact of reordering, loss, and congestion control is probably needed (also see Gorry's review). This document should also consider IPv6 (rfc8201). Regarding sec 4.2.5, as mentioned by Gorry, it is probably not possible to use the UPD checksum because it might change on the path. And finally, I have two questions on sec 4.2.6.: 1) I don't quite understand how the identifier "shim" is used. I guess this would be needed on all UDP packet and it should be negotiated/indicated between the client and the server. How is that done? 2) Also why does the sequence number needs to be "monotonically incremented by one for each packet sent". I think all you need is a unique number. So you actually don't need a sequence number but that an easy implementation to get a unique number. I would like to see this clarified because adding a sequence number might not always be the best choice.
Sec 4.2.2 says: "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. 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" However, RFC4821 seems to only see the second case as an failure.
This seems like an interesting technique that warrants collection of operational experience. From a process perspective, I think we have a bit of an issue, unless I've overlooked something relevant. This is proposed as a Standards-Track document, but it relies on the use of the PADDING attribute defined in RFC 5780. RFC 5780 is Experimental, so this is a formal downref. And RFC 5780 does not appear in the downref registry , nor did the IETF last call  include a request that the IETF community consider allowing such a refernce. From a practical perspective, the mechanism described in this document seems like the kind of thing that it would be useful to gather operational experience with prior to putting it on the standards track. I have some operational concerns (described below) that I think could be either proven out or dispelled by experimental deployment of the technology. My recommendation is to recategorize this mechanism as experimental, adding some text about the desire to gather operational experience. For avoidance of doubt: My DISCUSS is only on the process issue, and I'll happily clear regardless of how this issue is rationalized (e.g., either by running IETF last call again, by reclassifying this mechanism as experimental, or perhaps some novel solution that I may not have thought of). Everything else is merely a recommendation. ____  https://datatracker.ietf.org/doc/downref/  https://www.ietf.org/mail-archive/web/tram/current/msg02609.html
In the general case, STUN servers aren't aware of the signaling protocol that is in use. For example, when a TURN server is use with RTP and RTCP with a session set up via SIP, there is no requirement that the TURN server itself have any inherent knowledge of SIP or RTP or RTCP. From that perspective, the following text in section 4.2 is a bit confusing and/or problematic: Some application layer protocols may already have a way of identifying each individual UDP packet, in which case these identifiers SHOULD be used in the IDENTIFIERS attribute of the Report Response. It seems odd that I would have to teach my TURN server about the protocols I'm using it with just so that it can identify the packets. This behavior, combined with the requirement that all behavior be symmetrical ("As a result of the fact that all endpoints implementing this specification are both clients and servers") leads me to believe that perhaps the use cases that drove this mechanism are tightly scoped to direct peer-to-peer uses of ICE, while the other common uses of STUN (e.g., public TURN servers used for symmetric NAT traversal) were given no consideration. If that was intentional, then I think the abstract and introduction need to clearly describe the scenarios the mechanism was defined for; and, more importantly, clarify that it does not work for the general case, including STUN servers used for NAT traversal. I suspect that, once this mechanism begins to be deployed, the foregoing limitations will cause operational difficulties, which may in turn suggest changes to the mechanism that is currently defined, hence my suggestion above to recharacterize the mechanism as experimental. --------------------------------------------------------------------------- §4: > The Probing mechanism is used to discover the Path MTU in one > direction only, from the client to the server. Nit: "...only: from..." > Two Probing mechanisms are described, a Simple Probing mechanism and Nit: "...described: a Simple..." > a more complete mechanism that can converge quicker and find an Nit: "...converge more quickly..." > appropriate PMTU in the presence of congestion. Additionally, the Nit: Please expand "PMTU" on first use. --------------------------------------------------------------------------- §4.2.5: > 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]). The location of the citation in here implies that the XOR'ing described is part of V.42. Given that 0x53545554E is ASCII for "STUN," I'm pretty sure that's not part of the underlying CRC. Would suggest reworking as: algorithm used for the FINGERPRINT attribute (i.e., the CRC-32 calculated per the algorithm defined in [ITU.V42.2002], such has subsequently been XOR'ed with 32-bit value 0x5354554e).
I support Adam's DISCUSS. I will go a bit further to say that, even if a new IETF LC occurs, I would be skeptical that the dependency on PADDING in a standards track protocol is appropriate unless people are willing to argue that RFC 5780 has become mature enough that it could reasonably be promoted to standards track. Another alternative might be to re-describe PADDING in this draft, as it is used in the context of the draft. I don't normally love that sort of duplication, but it might be appropriate here. Other comments: §2: "It is not intended as a replacement for [RFC4821]": I find this comment confusing. Are other sections in the document intended to replace some or all of 4821? §4: "The probing mechanism is used to discover the Path MTU in one direction only...": Can this mechanism not be used bidirectionally, with reciprocal client-server roles? §4.1.2: "The server MUST add the FINGERPRINT attribute...": Is this a new requirement for PMTUD, or a generic STUN requirement? If the latter, it should not be stated normatively. (Same comment for §4.2.1) §4.2.1: "If the authentication mechanism permits it, then the Indication MUST be authenticated": Is that intended to imply it's okay to use authentication mechanisms that don't allow this?
I am looking forward to the resolution of procedural DISCUSS raised by Adam.
Rich version of this review at: https://mozphab-ietf.devsvcdev.mozaws.net/D4528 IMPORTANT S 4.2.6. > It could have been possible to use the checksum generated in the UDP > checksum for this, but this value is generally not accessible to > applications. Also, sometimes the checksum is not calculated or is > off-loaded to network hardware. > > 4.2.6. Using Sequence Numbers as Packet Identifiers I don't understand how as an endpoint I know which method I use. S 4.2.6. > > 4.2.6. Using Sequence Numbers as Packet Identifiers > > When using sequence numbers, a small header similar to the TURN > ChannelData header is added in front of all packets that are not a > STUN Probe Indication or Request. The sequence number is how would this interact with ICE, where you send Binding Indidcations. COMMENTS S 2. > Probing mechanism (as described in Section 4.2). The selection of > which Probing Mechanism to use is dependent on performance and > security and complexity trade-offs. > > If the Simple Probing mechanism is chosen, then the Client initiates > Probe transactions, as shown in Figure 1, which increase in size Why does this use probe and not binding-request? Then you wouldn't have a constraint on knowing the other side supported it. S 2. > security and complexity trade-offs. > > If the Simple Probing mechanism is chosen, then the Client initiates > Probe transactions, as shown in Figure 1, which increase in size > until transactions timeout, indicating that the Path MTU has been > exceeded. It then uses that information to update the Path MTU. Most of the MTU mechanisms I know of start big and go small. See, for instance: https://tools.ietf.org/html/rfc4821#section-7.2 S 4.1.2. > [RFC5389]. > > The server then creates a Probe Response. The server MUST add the > FINGERPRINT attribute so the STUN messages are disambiguated from the > other protocol packets. The server then sends the response to the > client. I note that this doesn't let you measure PMTU in the opposite direction. S 4.1.3. > client. > > 4.1.3. Receiving a Probe Response > > A client receiving a Probe Response MUST process it as specified in > [RFC5389]. If a response is received this is interpreted as a Probe 5389 doesn't describe Probe, so you should lay out what this means. S 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. When is this useful? Only when you want to use simple probing? S 7. > The Simple Probing mechanism may be used without authentication > because this usage by itself cannot trigger an amplification attack > as the Probe Response is smaller than the Probe Request. An > unauthenticated Simple Probing mechanism cannot be used in > conjunction with the Implicit Probing Support Signaling mechanism in > order to prevent amplification attacks. I don't understand this last sentence. It can't be used? Doesn't the previous sentence imply you can?
I support Adam's DISCUSS, and believe that Ben's proposed alternative ("re-describe PADDING in this draft") is a viable way forward.