Updated ICMP PMTU section #1412

igorlord · 2018-06-05T02:54:37Z

Per request of the editors, here is a draft PR for ICMP PMTU section.
I am deliberately not including text for #1243 here. (That can be a subsequent PR.)

ianswett

This looks good, but I'd like someone who's more of an expert in ICMP to read it, so I'm adding @martinduke

ianswett · 2018-06-09T15:56:44Z

draft-ietf-quic-transport.md

+When an ICMP "Unreachable" message is received after the handshake, the QUIC
+endpoint should send a PATH_CHALLENGE frame ({{frame-path-challenge}}).  Sending
+PATH_CHALLENGE frames on the same path due to ICMP "Unreachable" messages should
+be rate limited.


To what? How about one outstanding PATH_CHALLENGE at once?

I'll rewrite this completely.

ianswett · 2018-06-09T16:11:06Z

draft-ietf-quic-transport.md

+the protocol implementation to off-path attacks and requires mitigations.
+
+Even ICMP messages without an on-path proof SHOULD undergo some validation, such
+as:

 * Set the IPv4 Don't Fragment (DF) bit on a small proportion of packets, so that


I thought we were recommending setting DF on all packets?

I also wondered ...

That was one of the original proposals. But I agree -- this is not the best idea. I'll revise.

martinthomson

Thanks for doing this Igor.

This is a lot of new words, but generally good. I think that we probably need to do something to break this up a little.

I also think that implementations will not be able to save anything of the IP ID, and won't be inclined to save anything from the packet. So we might want to suggest a mode switch when a packet is received.

martinthomson · 2018-06-11T16:20:04Z

draft-ietf-quic-transport.md

-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}})


"classical" isn't a useful description of the mechanism. If you need to distinguish this between PLPMTUD, talk about using ICMP feedback.

How strongly do you feel against word "classical"? It is used in other RFCs for the purpose I am using it here. See https://tools.ietf.org/html/rfc4821 and https://tools.ietf.org/html/rfc5320.

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

I think the context does make it clear, but @martinduke's suggestion seems reasonable.

martinthomson · 2018-06-11T16:20:34Z

draft-ietf-quic-transport.md

+possible without the ICMPv6 packet exceeding the minimum IPv6 MTU", and
+{{!RFC1812}}, which states that ICMPv4 error messages "SHOULD contain as much of
+the original datagram as possible without the length of the ICMP datagram
+exceeding 576 bytes".  ICMP messages without an on-path prooff are sent in


typo: prooff

martinthomson · 2018-06-11T16:37:18Z

draft-ietf-quic-transport.md

-mitigate this risk. For instance, an application could:
+The minimum required validation of ICMP messages with an on-path proof invoves
+verifying that the message was sent by this endpoint with at least 1-2^32
+probability and it is still outstanding (not acknowledged and not deemed lost).


How does one make this determination?

You are right. This needs revising.

martinthomson · 2018-06-11T16:40:06Z

draft-ietf-quic-transport.md

+  and can therefore be identified as spurious.
+
+* Store IP ID field of the sent datagrams to validate that ICMP message is
+  refering to an outstanding packet.


More IPv4 advice, which should be called out as such. I don't think that senders will remember this if it is not predictable.

martinthomson · 2018-06-11T16:40:34Z

draft-ietf-quic-transport.md

+unless then would cause a reduction to a Path MTU value smaller than 1280
+octets.
+
+If during a handshake a client receives an ICMP TPB message that requests it to


martinthomson · 2018-06-11T16:41:49Z

draft-ietf-quic-transport.md

+reduce Path MTU to a value smaller than 1280 octets, then:
+
+* If the client has another IP address for the server to try, the client should
+  restart the connection to another IP.


martinthomson · 2018-06-11T16:43:07Z

draft-ietf-quic-transport.md

+octets.
+
+If during a handshake a client receives an ICMP TPB message that requests it to
+reduce Path MTU to a value smaller than 1280 octets, then:


I think that this disagrees with the conclusion of the working group - we agreed that a PMTU of 1280 was necessary to support QUIC and that the response to a reduction in size was to ignore the signal. If the signal is true, this causes the connection to fail, but that's OK.

This is specifically done during the handshake. The language here is trying to stay in agreement with the decision that PMTU < 1280 does not support QUIC. Which ones of the statements is the concern:

to switch IPs (but stick w/ QUIC), if multiple IPs are available for the server?

to switch protocols, if we have an on-path validation of the signal?

Would you rather see all PMTU<1280 signals ignored (and not switch to TCP, if available, for example)?

martinthomson · 2018-06-11T16:44:07Z

draft-ietf-quic-transport.md


-* Any reduction in PMTU due to a report contained in an ICMP packet is
+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


Where did this 1392 come from?

Somewhat arbitrary, examining various tunneling protocols out there to see what seems like a conservative-enough number that is not too small to be significantly concerned with degraded performance. This number would certainly need a wg consensus.

The goal was to make sure that in the common case (no attack), we can switch to a workable PMTU immediately.

Of course, if the wg believes that 1280 is "large enough" to use it immediately, this would simplify things.

martinthomson · 2018-06-11T16:45:47Z

draft-ietf-quic-transport.md

+a Path MTU smaller than 1280 octets (see {{icmp-pmtu}}).
+
+When an ICMP "Unreachable" message is received after the handshake, the QUIC
+endpoint should send a PATH_CHALLENGE frame ({{frame-path-challenge}}).  Sending


SHOULD?

If "SHOULD", how does an endpoint decide to do this (or not). If it is purely a rate-limiting thing, then maybe different words would help.

Fixing and rewriting this.

martinduke

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

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.
The added clarification around handshake/non-handshake, and the explicit call for some validation of QUIC headers, if available, is valuable.
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.

martinduke · 2018-06-18T20:30:25Z

draft-ietf-quic-transport.md

-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.

martinduke · 2018-06-18T20:34:27Z

draft-ietf-quic-transport.md

-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.

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.

I'd say that not just the QUIC header but, more importantly, the QUIC encrypted section, since it has more entropy.

But, I agree, instead of coming up with metrics, we should just leave the specifics undefined. I like your wording. Will adapt and incorporate.

(As for an 8-byte proof, it is sufficient for TCP but not QUIC, since 8 bytes only cover two UDP ports, length and checksum. Hence this entire discussion.)

martinduke · 2018-06-18T20:42:33Z

draft-ietf-quic-transport.md

-* 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.

The language in this section says that validation is a SHOULD. But if that validation fails, discarding is a MUST. (And if the validation is not done, since it is not mandatory, then the signal SHOULD be treated as a signal without an on-path proof.)

Do you still think this is a problem?

@janaiyengar is suggesting that not doing (or not being able to do) a validation means it is a ~~SHOULD~~ MUST to treat the signal as not having an on-path proof. It makes sense to me.

martinduke · 2018-06-18T20:43:35Z

draft-ietf-quic-transport.md


+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...

martinduke · 2018-06-18T20:47:35Z

draft-ietf-quic-transport.md

+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


martinduke · 2018-06-18T20:47:43Z

draft-ietf-quic-transport.md


+* 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


martinduke · 2018-06-18T20:49:27Z

draft-ietf-quic-transport.md


-### 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?

PLPMTUD take time to do its work (it is loss/timeout-based). The purpose of this discussion is to identify conditions, when you can trust ICMP immediately and not have to resort to PLPMTUD (especially during handshake).

martinduke · 2018-06-18T20:52:28Z

draft-ietf-quic-transport.md

+
+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.

Yes, this would need to be reconfirmed. I do believe that we should encourage implementations to use the best available signal, including ICMP, especially when we have reasons to trust it. Performance is important for QUIC, and it is double-important for handshake, and ICMP works much faster than PLPMTUD.

Re-reading PLPMTUD, the draft says explicitly that it is an extension to the classical (ICMP-based) PMTU discovery process and its prescriptions are to be used in cases where ICMP-based PMTU discovery cannot be done. So say that PLPMTUD is a SHOULD is effectively saying that ICMP is a SHOULD (as much as practicable).

martinduke · 2018-06-18T20:52:56Z

draft-ietf-quic-transport.md

+
+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.

martinduke · 2018-06-18T21:00:52Z

draft-ietf-quic-transport.md


-* 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?

LOL. Mostly because I received comments that we recommend DF to be set on all packets, and this is going against that recommendation.

(But being pedantic, that standard was for on-path proof, while this discussion is for validation packets w/o on-path proof.)

More importantly, the entire standard thing is gone now. I will further revise it to use your latest feedback as to the wording (ie. "use the data from the packet" instead of prescribing standards or which data to use).

janaiyengar

Thanks for getting this going, @igorlord ! A few comments, mostly editorial.

janaiyengar · 2018-06-18T23:09:32Z

draft-ietf-quic-transport.md

-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}})


I think the context does make it clear, but @martinduke's suggestion seems reasonable.

janaiyengar · 2018-06-18T23:09:43Z

draft-ietf-quic-transport.md

@@ -3187,40 +3187,76 @@ QUIC packets on the affected path.  This could result in termination of the
 connection if an alternative path cannot be found.


-### IPv4 PMTU Discovery {#v4-pmtud}
+### Classical ICMP-based path MTU discovery {#icmp-pmtu}


ditto. Also, Capitalize This Section Title.

janaiyengar · 2018-06-18T23:12:14Z

draft-ietf-quic-transport.md

+
+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
+an on-path proof are the messages sent in accordance with {{!ICMPv6=RFC4443}},


the messages -> messages

janaiyengar · 2018-06-18T23:28:45Z

draft-ietf-quic-transport.md

+
+The minimum required validation of ICMP messages with an on-path proof invoves
+verifying that the message was sent by a network element that has observed a
+QUIC packet that is still outstanding (not acknowledged and not deemed lost).


This sentence is a bit hard to follow. Also, it's possible that the ICMP message was received after the sender deemed it lost, since the sender may have been too earnest in marking it lost. Perhaps remove the requirement that it not be marked as lost yet? How about the following: "To validate an ICMP message containing on-path proof, the proof MUST be from a QUIC packet that is not yet acknowledged."

janaiyengar · 2018-06-18T23:31:57Z

draft-ietf-quic-transport.md

+The minimum required validation of ICMP messages with an on-path proof invoves
+verifying that the message was sent by a network element that has observed a
+QUIC packet that is still outstanding (not acknowledged and not deemed lost).
+To perform the on-path validation, the sender would need to remember at least 8


To perform the on-path validation -> To validate a received ICMP message

janaiyengar · 2018-06-18T23:33:04Z

draft-ietf-quic-transport.md

+validation, it SHOULD treat the packet as an ICMP message without an on-path
+proof.
+
+As noted in {{?RFC5927}}, using ICMP messages without an on-path proof exposes


an on-path proof -> on-path proof

janaiyengar · 2018-06-18T23:34:01Z

draft-ietf-quic-transport.md

+proof.
+
+As noted in {{?RFC5927}}, using ICMP messages without an on-path proof exposes
+the protocol implementation to off-path attacks and requires mitigations.


the protocol implementation -> an endpoint

janaiyengar · 2018-06-18T23:34:21Z

draft-ietf-quic-transport.md

+As noted in {{?RFC5927}}, using ICMP messages without an on-path proof exposes
+the protocol implementation to off-path attacks and requires mitigations.
+
+ICMP messages without an on-path proof SHOULD undergo as much validation as


an on-path proof -> on-path proof

janaiyengar · 2018-06-18T23:52:26Z

draft-ietf-quic-transport.md

+ICMP messages without an on-path proof SHOULD undergo as much validation as
+possible.  For example, such validation could include storing IP ID fields of
+sent datagrams to validate that received ICMP messages are refering to
+outstanding packets.


This paragraph seems quite redundant. I would replace it with one sentence with recommendation for what state a sender can store to validate proof in ICMP messages.

janaiyengar · 2018-06-18T23:54:39Z

draft-ietf-quic-transport.md

+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


then -> they

igorlord · 2018-06-26T04:25:21Z

I've made updates per feedback received. There are two points of discussion left (that I know of):

This is promoting ICMP processing to a SHOULD. I think this is a correct thing to do, since PLPMTUD claims itself to be an extension of ICMP-based PMTUD process. Of course, implementations w/o access to ICMP would not be able to implement this SHOULD, but this is ok albeit unfortunate -- the very definition of a SHOULD (skipping on MAY is not "unfortunate").
I am using a mostly made up number (1392) as "conservative enough to be believed w/o on-path proof". It was a compromise between "small enough for most Internet tunnels" and "not that small that someone would go into troubles of maliciously reducing PMTU of the victim to that number". This may need a discussion.

* DF is always set, * better response to ICMP Unreachable after handshake

…nse to ICMP Unreachable.

ianswett · 2018-12-07T19:21:07Z

@igorlord This hasn't been updated in a while and I think PR #2036 may have covered some of the items you were working on. Can you either update this PR or close it as being now obsolete?

igorlord · 2018-12-10T04:37:49Z

@ianswett, yes, much has changed since. Let's close this PR. Instead, I've posted a simpler #2108 and #2109 that may need a short discussion.

ianswett reviewed Jun 9, 2018

View reviewed changes

ianswett requested a review from martinduke June 9, 2018 16:13

martinthomson reviewed Jun 11, 2018

View reviewed changes

MikeBishop added design An issue that affects the design of the protocol; resolution requires consensus. -transport labels Jun 11, 2018

martinduke suggested changes Jun 18, 2018

View reviewed changes

igorlord force-pushed the icmp branch from de84e82 to f2a764b Compare June 18, 2018 21:37

janaiyengar reviewed Jun 18, 2018

View reviewed changes

igorlord force-pushed the icmp branch from f2a764b to cf0d320 Compare June 22, 2018 15:33

igorlord force-pushed the icmp branch from cf0d320 to 37f87e7 Compare June 29, 2018 20:00

igorlord added 3 commits July 6, 2018 17:25

Updated ICMP PMTU section

70981f3

Updated ICMP PMTU section: incorporating feedback

0c03315

* DF is always set, * better response to ICMP Unreachable after handshake

Incorporating further feedback on PMTU and ICMP. Fixed server's respo…

6d98148

…nse to ICMP Unreachable.

igorlord force-pushed the icmp branch from 37f87e7 to 6d98148 Compare July 6, 2018 21:26

igorlord closed this Dec 10, 2018


		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


		* 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


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

Updated ICMP PMTU section #1412

Updated ICMP PMTU section #1412

Conversation

igorlord commented Jun 5, 2018

ianswett left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

martinthomson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

martinduke left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

igorlord Jun 18, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

janaiyengar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

igorlord commented Jun 26, 2018

ianswett commented Dec 7, 2018

igorlord commented Dec 10, 2018

igorlord Jun 18, 2018 •

edited

Loading