Subject: [quicwg/base-drafts] Endpoints MAY (not SHOULD) ignore IPv4 ICMP PTB messages (#2109)

From: Igor Lubashev <[email-masked]>
Date: Sun, 09 Dec 2018 20:31:23 -0800

The majority of IPv4 ICMP messages still include only 8 bytes past the original IP header.  We should not recommend implementations to ignore them.  Even though these ICMP messages could be spoofed, we already require ignoring ICMP messages that try to reduce PMTU to a value smaller than 1280.  So the damage from spoofed ICMP messages is minimal, while ignoring them may cause problems from slow PMTU discovery to broken connections.
You can view, comment on, or merge this pull request online at:

  https://github.com/quicwg/base-drafts/pull/2109

-- Commit Summary --

  * Endpoints MAY (not SHOULD) ignore IPv4 ICMP PTB messages

-- File Changes --

    M draft-ietf-quic-transport.md (6)

-- Patch Links --

https://github.com/quicwg/base-drafts/pull/2109.patch
https://github.com/quicwg/base-drafts/pull/2109.diff

-- 
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/2109

---

From: Martin Thomson <[email-masked]>
Date: Mon, 10 Dec 2018 17:05:41 -0800

I think that the "SHOULD" is still appropriate here.  We have text that defines validation in a way that doesn't exclusively depend on having more than 8 bytes to rely on.

-- 
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/2109#issuecomment-446034111

---

From: Igor Lubashev <[email-masked]>
Date: Mon, 10 Dec 2018 17:48:32 -0800

> We have text that defines validation in a way that doesn't exclusively depend on having more than 8 bytes to rely on.

There is a difference between "failed validation" and "do not / cannot perform validation".  This sentence applies only to the latter case.  The argument is that if an implementation cannot perform validation (for whatever reason, including the limitations of the platform or limitations of the implementation itself), it is better to honor ICMP PTB (up to the 1280 limit) than to risk driving packets into a black hole.

-- 
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/2109#issuecomment-446042173

---

From: Martin Thomson <[email-masked]>
Date: Mon, 10 Dec 2018 18:04:14 -0800

The SHOULD is better there though - it allows for the case where validation is impossible, but encourages implementations not to follow that path.  Maybe that's wishful thinking, but that's always part of this.

-- 
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/2109#issuecomment-446045146

---

From: Igor Lubashev <[email-masked]>
Date: Mon, 10 Dec 2018 19:37:36 -0800

I wholeheartedly share the desire to encourage more robust implementations.  Ironically, this is exactly why I am concerned about this SHOULD.

I see this SHOULD not as _"encourag[ing] implementations not to follow that path"_ but providing a dangerous recommendation (with no disclaimer) to implementations who already found themselves on that path for whatever reason.

We see _many_ broken PMTU discovery implementations on the Internet, because PMTUD is hard, and many implementers are not well-informed and/or under time pressure. I am worried that an implementer will implement no 8-byte-compatible validations, read this SHOULD, drop the packet, and not think too hard about it, because he is "just following the RFC recommendation".

I see that we are both approaching this with the same goal but are coming to different conclusions. Maybe it is worthwhile to take it to the list for more visibility?

-- 
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/2109#issuecomment-446060951

---

From: Martin Thomson <[email-masked]>
Date: Mon, 10 Dec 2018 21:09:23 -0800

If this is just about the 8 byte thing, then maybe the right thing to do is carve that out separately to the other validation points.  But yeah, list.

-- 
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/2109#issuecomment-446074453

---

From: Gorry Fairhurst <[email-masked]>
Date: Wed, 12 Dec 2018 01:20:11 -0800

The current recommendations for all transports are covered by:
https://tools.ietf.org/html/draft-ietf-tsvwg-datagram-plpmtud
Section 3, requirement 4 currently describes the TSVWG work on DPLPMTUD.

-- 
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/2109#issuecomment-446517826

---

From: Igor Lubashev <[email-masked]>
Date: Wed, 12 Dec 2018 10:35:53 -0800

DPLPMTUD is not a requirement for all QUIC implementations.  The SHOULD in question is written as a requirement for all.

Looking at DPLPMTUD, however, I am realizing that it is possible that the question is moot.  DPLPMTUD draft (#3.4) considers doing anything at all (like simply matching to an existing connection using IP addresses) sufficient for validation (it says that doing more for validation is a SHOULD, implying that just matching IPs is still acceptable).  Since doing anything less is impractical when processing PTB messages, any implementation that uses PTB messages is implicitly validating them.  If so, I'd be happy with some text making it explicit that just matching on IPs and, maybe, UDP ports is sufficient to consider PTB validated.

-- 
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/2109#issuecomment-446695165

---

From: Martin Thomson <[email-masked]>
Date: Wed, 12 Dec 2018 22:22:44 -0800

If we step back a little let's list what what we would like to have happen:

1. if the PMTU is being forced below 1280, ignore it
2. validate the packet, using whatever means you like (this is likely a policy decision) and throw out packets that don't pass the test
3. adjust the PMTU 

Based on that, I think that we can make the test simpler.

FWIW, I don't agree that matching addresses is sufficient, but I'm happy to leave that to endpoints to each decide for themselves.

-- 
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/2109#issuecomment-446856053

---

From: Igor Lubashev <[email-masked]>
Date: Thu, 13 Dec 2018 08:43:04 -0800

@martinthomson, yes, I am happy with this one.

Right now we also have step 2.5: "If the means you used in step 2 did not include X, we recommended that you throw out the packet."

So I think we need to either specify what X is explicitly or get rid of this step 2.5.

-- 
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/2109#issuecomment-447037525

---

From: Gorry Fairhurst <[email-masked]>
Date: Thu, 13 Dec 2018 08:51:34 -0800

I am not sure I agree or not. It is not clear what the issue you see is to me.

Gorry

> On 13 Dec 2018, at 16:43, Igor Lubashev <[email-masked]> wrote:
> 
> —
> You are receiving this because you commented.
> Reply to this email directly, view it on GitHub, or mute the thread.

-- 
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/2109#issuecomment-447040540

---

From: Igor Lubashev <[email-masked]>
Date: Thu, 13 Dec 2018 12:37:09 -0800

Ok, I've pushed an update that simplifies and clarifies ICMP PTB validation along the lines discussed.

(I'll create a separate PR for disallowing ICMP PTB that increase PMTU -- something that got missed here I believe.)

-- 
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/2109#issuecomment-447112161

---

From: Martin Thomson <[email-masked]>
Date: Thu, 13 Dec 2018 15:39:51 -0800

martinthomson approved this pull request.

I'm OK with this (it deletes text, how can I resist).  But I want to understand @gorryfair's concerns better.

-As a part of ICMP validation, QUIC endpoints SHOULD validate that connection ID
-information corresponds to an active session.
+ICMP message validation SHOULD include matching IP addresses and UDP ports

```suggestion
ICMP message validation MUST include matching IP addresses and UDP ports
```

-- 
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/2109#pullrequestreview-184918199

---

From: Gorry Fairhurst <[email-masked]>
Date: Fri, 14 Dec 2018 00:31:24 -0800

On 13/12/2018, 20:37, Igor Lubashev wrote:
> (I'll create a separate PR for disallowing ICMP PTB that increase PMTU 
> -- something that got missed here I believe.)
>
> —
> Reply to this email directly, view it on GitHub 
> <https://github.com/quicwg/base-drafts/pull/2109#issuecomment-447112161>, 
> or mute the thread 
> <https://github.com/notifications/unsubscribe-auth/AHC1kkABid6XIPKIMh5-50x-BGyU02ehks5u4rp2gaJpZM4ZKoYb>.
>
What does the text say?

Gorry

-- 
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/2109#issuecomment-447253057

---

From: Igor Lubashev <[email-masked]>
Date: Fri, 14 Dec 2018 06:48:02 -0800

@gorryfair, the text for this PR has been updated.  Click on "[Files changed](https://github.com/quicwg/base-drafts/pull/2109/files)" to see it.

The followup PR for PMTU increase is #2161.

-- 
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/2109#issuecomment-447346731

---

From: Igor Lubashev <[email-masked]>
Date: Fri, 14 Dec 2018 12:09:00 -0800

@igorlord pushed 1 commit.

ad6122da9447057fab23d283f541ed503a40915f  Validation MUST match IPs and ports.

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/2109/files/4534794a000d30427a543c1c6ecda4aac381e30b..ad6122da9447057fab23d283f541ed503a40915f

---

From: ianswett <[email-masked]>
Date: Tue, 18 Dec 2018 15:34:48 -0800

ianswett approved this pull request.

-- 
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/2109#pullrequestreview-186331771

---

From: Martin Thomson <[email-masked]>
Date: Tue, 18 Dec 2018 15:35:15 -0800

@martinthomson pushed 1 commit.

382d294a4ba61d4d59c895adb8d7edecdd148246  Merge branch 'master' into pmtu-may-ignore-ipv4-icmp

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/2109/files/ad6122da9447057fab23d283f541ed503a40915f..382d294a4ba61d4d59c895adb8d7edecdd148246

---

From: janaiyengar <[email-masked]>
Date: Tue, 18 Dec 2018 15:39:33 -0800

janaiyengar approved this pull request.

-- 
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/2109#pullrequestreview-186332940

---

From: Martin Thomson <[email-masked]>
Date: Tue, 18 Dec 2018 23:39:43 +0000 (UTC)

Merged #2109 into master.

-- 
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/2109#event-2034389918