Summary: Needs a YES. Has a DISCUSS. Needs 3 more YES or NO OBJECTION positions to pass.
[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.
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?
Thank you for addressing my Discuss point! I'll retain the note that I supported Adam's Discuss, for clarity, but trim the old comments.
Thanks for addressing my DISCUSS point.
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.
Thanks for addressing my DISCUSS. I'm keeping the one substantive comment below, as it is still applicable to the -13 version of the document. 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.