Re: [quicwg/base-drafts] Updated ICMP PMTU section (#1412)

[martinduke <[e-mail]>]

martinduke requested changes on this pull request.

There a bunch of nits, but I have three higher-level comments:

1) There are no MUSTs in the ICMP stuff because you don't always control that in user space. We reached consensus before that PLPMTUD was a SHOULD and all ICMP stuff was a MAY; we should socialize this in the whole group if we upend that now. You've slipped in some SHOULDs that muddy those waters.

2) The added clarification around handshake/non-handshake, and the explicit call for some validation of QUIC headers, if available, is valuable.

3) A big part of this PR is a rewrite of how to handle it when ICMP gives you only 8 bytes. I found the original text to be more clear and concise than the new text, but perhaps we can discuss what the motivation for this rewrite is.

> @@ -3157,9 +3157,9 @@ header, protected payload, and any authentication fields.
 All QUIC packets SHOULD be sized to fit within the estimated PMTU to avoid IP
 fragmentation or packet drops. To optimize bandwidth efficiency, endpoints
 SHOULD use Packetization Layer PMTU Discovery ({{!PLPMTUD=RFC4821}}).  Endpoints
-MAY use PMTU Discovery ({{!PMTUDv4=RFC1191}}, {{!PMTUDv6=RFC8201}}) for
-detecting the PMTU, setting the PMTU appropriately, and storing the result of
-previous PMTU determinations.
+MAY use classical PMTU Discovery ({{!PMTUDv4=RFC1191}}, {{!PMTUDv6=RFC8201}})

How about "ICMP-based"? This eliminates all ambiguity and saves people the trouble of clicking through references to know what we're talking about.

>  
-Traditional ICMP-based path MTU discovery in IPv4 {{!PMTUDv4}} is potentially
-vulnerable to off-path attacks that successfully guess the IP/port 4-tuple and
-reduce the MTU to a bandwidth-inefficient value. TCP connections mitigate this
-risk by using the (at minimum) 8 bytes of transport header echoed in the ICMP
-message to validate the TCP sequence number as valid for the current
-connection. However, as QUIC operates over UDP, in IPv4 the echoed information
-could consist only of the IP and UDP headers, which usually has insufficient
-entropy to mitigate off-path attacks.
+ICMP error messages used in classical Path MTU discovery may be classified into
+messages with an on-path proof and without an on-path proof.  ICMP messages with

I disagree with the statement that some ICMP messages have no proof.  The 8-byte trailer is sufficient as a proof for TCP, and even if the QUIC case there is enough entropy if we store the right things.

>  
-* Any reduction in PMTU due to a report contained in an ICMP packet is
-provisional until QUIC's loss detection algorithm determines that the packet is
-actually lost.
+Any ICMP messages that fail validation MUST be discarded.

The problem with MUSTs in this section is that some user space implementations may not have any control over ICMP handling logic.

>  
+Any reduction in Path MTU due to a report contained in an ICMP Packet Too Big
+message (PTB) provisional until QUIC's loss detection algorithm determines that

... SHOULD be provisional...

>  
-### Special Considerations for Packetization Layer PMTU Discovery
+#### ICMP PTB During Handshake  {#icmp-ptb-during-handshake}
+
+It is important that any problems are detected quickly during the connection
+handshake, because the client may be able to mitigate them by switching to
+alternative IP addresses or protocols.  Hence, an endpoint SHOULD reduce Path
+MTU in response to an ICMP PTB message during a handshake, unless then would
+cause a reduction to a Path MTU value smaller than 1280 octets.
+
+If a client receives an ICMP PTB message that requests it to reduce Path MTU to
+a value smaller than 1280 octets during a handshake, then:
+
+* If the client has another IP address for the server to try, the client should

SHOULD

>  
+* Otherwise, if the client can fail over to another protocol, and the ICMP
+  packet has on-path validation, the client should retry connecting with another

SHOULD

>  
-### Special Considerations for Packetization Layer PMTU Discovery
+#### ICMP PTB During Handshake  {#icmp-ptb-during-handshake}

I like how you separate the handshake issues here, but these also apply to PLPMTUD, yes?

>  
-### Special Considerations for Packetization Layer PMTU Discovery
+#### ICMP PTB During Handshake  {#icmp-ptb-during-handshake}
+
+It is important that any problems are detected quickly during the connection
+handshake, because the client may be able to mitigate them by switching to
+alternative IP addresses or protocols.  Hence, an endpoint SHOULD reduce Path

This is a significant change. The spec suggests ("SHOULD") that PLPMTUD be used. ICMP-based PMTUD is strictly a MAY.

>  
-The PADDING frame provides a useful option for PMTU probe packets. PADDING
+#### ICMP PTB After Handshake   {#icmp-ptb-after-handshake}
+
+If an ICMP PTB message is received after handshake, and the claimed Path MTU is
+at least 1280 octets for messages with on-path validation or 1392 for messages
+without on-path validations, the Path MTU SHOULD be set accordingly.  Otherwise,

Same comment: we're elevating ICMP response from a MAY to a SHOULD.

>  
-Traditional ICMP-based path MTU discovery in IPv4 {{!PMTUDv4}} is potentially
-vulnerable to off-path attacks that successfully guess the IP/port 4-tuple and
-reduce the MTU to a bandwidth-inefficient value. TCP connections mitigate this
-risk by using the (at minimum) 8 bytes of transport header echoed in the ICMP
-message to validate the TCP sequence number as valid for the current
-connection. However, as QUIC operates over UDP, in IPv4 the echoed information
-could consist only of the IP and UDP headers, which usually has insufficient
-entropy to mitigate off-path attacks.
+ICMP error messages used in classical Path MTU discovery may be classified into
+messages with an on-path proof and without an on-path proof.  ICMP messages with

Zooming out a bit, I think the original text of this section is a bit clearer about what the real problem is. I do think it could use a paragraph like "endpoints SHOULD use fields from the QUIC header, if provided in the ICMP message, to validate that a QUIC packet in flight might have generated the ICMP message." And leave the rest unchanged.

>  
-* Set the IPv4 Don't Fragment (DF) bit on a small proportion of packets, so that
-most invalid ICMP messages arrive when there are no DF packets outstanding, and
-can therefore be identified as spurious.

I guess you deleted this because it didn't meet your 1/2^32 standard?

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/quicwg/base-drafts/pull/1412#pullrequestreview-129725968