Subject: [quicwg/base-drafts] Don't store or retransmit PATH_RESPOSNE frames, avoid buffering (#2729)

From: Eric Kinnear <[email-masked]>
Date: Mon, 20 May 2019 09:31:22 -0700

Mention in the PATH_RESPONSE section that we must not store or retransmit them.

Closes #2724.
You can view, comment on, or merge this pull request online at:

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

-- Commit Summary --

  * #2724: Don't store or retransmit PATH_RESPOSNE frames, avoid buffering too many

-- File Changes --

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

-- Patch Links --

https://github.com/quicwg/base-drafts/pull/2729.patch
https://github.com/quicwg/base-drafts/pull/2729.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/2729

---

From: Martin Thomson <[email-masked]>
Date: Tue, 21 May 2019 00:33:30 -0700

martinthomson commented on this pull request.

Did you want to base this on the other PR? Merge conflicts suck. 

> @@ -1722,6 +1722,10 @@ it can associate the peer's response with the corresponding PATH_CHALLENGE.
 
 On receiving a PATH_CHALLENGE frame, an endpoint MUST respond immediately by
 echoing the data contained in the PATH_CHALLENGE frame in a PATH_RESPONSE frame.
+An endpoint MUST NOT store or retransmit a PATH_RESPONSE frame, as a new

```suggestion

An endpoint MUST NOT store or retransmit a PATH_RESPONSE frame, as a new
```

> @@ -1722,6 +1722,10 @@ it can associate the peer's response with the corresponding PATH_CHALLENGE.
 
 On receiving a PATH_CHALLENGE frame, an endpoint MUST respond immediately by
 echoing the data contained in the PATH_CHALLENGE frame in a PATH_RESPONSE frame.
+An endpoint MUST NOT store or retransmit a PATH_RESPONSE frame, as a new
+PATH_CHALLENGE frame will be sent if another PATH_RESPONSE frame is needed.  An
+endpoint MAY choose to limit the number of pending PATH_RESPONSE frames

I think that this amounts to ignoring PATH_CHALLENGE. It might be better to say that instead. 

-- 
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/2729#pullrequestreview-239867076

---

From: Eric Kinnear <[email-masked]>
Date: Tue, 21 May 2019 02:40:48 -0700

@erickinnear pushed 1 commit.

e27a1c68f3e85eb79bb352a9e6b36b7d99bacc62  Update draft-ietf-quic-transport.md

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/2729/files/6b587ac89ed89afc48026a391ab52da6f0c8c1e0..e27a1c68f3e85eb79bb352a9e6b36b7d99bacc62

---

From: ianswett <[email-masked]>
Date: Tue, 21 May 2019 03:40:30 -0700

ianswett commented on this pull request.

It'd actually be slightly better to respond to the later ones, if possible.  That being said, if you're in this state, it's likely a bug in the peer or an attack.

-- 
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/2729#discussion_r285961239

---

From: Eric Kinnear <[email-masked]>
Date: Tue, 28 May 2019 15:10:26 -0700

52957ecb8dbc1740819661a2a87564addbc821ea  Clarify the MUST, unless and indicate that the destination CID is sufficient for rate limiting

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/2729/files/e27a1c68f3e85eb79bb352a9e6b36b7d99bacc62..52957ecb8dbc1740819661a2a87564addbc821ea

---

From: Eric Kinnear <[email-masked]>
Date: Tue, 28 May 2019 15:12:04 -0700

erickinnear commented on this pull request.

Clarified in 52957ec. I didn't want to get into which one to respond to on a given CID, since I don't think it makes a huge difference and it makes it a bit more confusing.

-- 
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/2729#discussion_r288323152

---

From: Rui Paulo <[email-masked]>
Date: Tue, 28 May 2019 16:57:37 -0700

rpaulo commented on this pull request.

> @@ -1721,7 +1721,12 @@ it can associate the peer's response with the corresponding PATH_CHALLENGE.
 ## Path Validation Responses
 
 On receiving a PATH_CHALLENGE frame, an endpoint MUST respond immediately by
-echoing the data contained in the PATH_CHALLENGE frame in a PATH_RESPONSE frame.
+echoing the data contained in the PATH_CHALLENGE frame in a PATH_RESPONSE frame,
+unless it has PATH_RESPONSE frames buffered for the same destination connection
+ID and wishes to limit memory consumtion, for example when congestion limited.

This example is inconsistent with the advice of just using one congestion controller so we might want to skip it.

-- 
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/2729#pullrequestreview-242976595

---

From: Eric Kinnear <[email-masked]>
Date: Tue, 28 May 2019 17:00:43 -0700

I don't know that it's entirely inconsistent, but sure can remove it.

-- 
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/2729#discussion_r288347829

---

From: Eric Kinnear <[email-masked]>
Date: Tue, 28 May 2019 17:01:19 -0700

d0d147a3a3db426ddba63ce139d0555fa8f26019  Remove example

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/2729/files/52957ecb8dbc1740819661a2a87564addbc821ea..d0d147a3a3db426ddba63ce139d0555fa8f26019

---

From: Eric Kinnear <[email-masked]>
Date: Tue, 28 May 2019 17:01:31 -0700

Removed in d0d147a

-- 
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/2729#discussion_r288347972

---

From: Martin Thomson <[email-masked]>
Date: Tue, 28 May 2019 17:52:42 -0700

martinthomson approved this pull request.

+ID and wishes to limit memory consumtion.

```suggestion
ID and wishes to limit memory consumption.
```

-- 
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/2729#pullrequestreview-242995151

---

From: Eric Kinnear <[email-masked]>
Date: Tue, 28 May 2019 17:53:45 -0700

Oof. Good catch!

-- 
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/2729#discussion_r288357042

---

From: Eric Kinnear <[email-masked]>
Date: Tue, 28 May 2019 17:53:49 -0700

ac8b8ef220b82e693fc5144e43463dc1b98fd1dc  Update draft-ietf-quic-transport.md

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/2729/files/d0d147a3a3db426ddba63ce139d0555fa8f26019..ac8b8ef220b82e693fc5144e43463dc1b98fd1dc

---

From: MikkelFJ <[email-masked]>
Date: Tue, 28 May 2019 22:18:23 -0700

mikkelfj commented on this pull request.

 On receiving a PATH_CHALLENGE frame, an endpoint MUST respond immediately by
-echoing the data contained in the PATH_CHALLENGE frame in a PATH_RESPONSE frame.
+echoing the data contained in the PATH_CHALLENGE frame in a PATH_RESPONSE frame,
+unless it has PATH_RESPONSE frames buffered for the same destination connection
+ID and wishes to limit memory consumption.
+
+An endpoint MUST NOT store or retransmit a PATH_RESPONSE frame, as a new
+PATH_CHALLENGE frame will be sent if another PATH_RESPONSE frame is needed.

I don't understand this: "unless it has PATH_RESPONSE frame buffered" and later "MUST NOT store"? why would you buffer a response and then not send it, and why would you buffer if you are not allowed to?  And why would it limit memory to buffer and why would it use less memory when choosing not to send the buffered data?

Do you mean "unless a PATH_RESPONSE has already been sent to the same destination connection ID?"

-- 
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/2729#pullrequestreview-243042649

---

From: Eric Kinnear <[email-masked]>
Date: Tue, 28 May 2019 23:33:02 -0700

I think the original intent here for “store” was in terms of (frames in) packets that are waiting to be acked, not things that might be kept around for a very short time to satisfy pacing and/or congestion limits on their way onto the network. Some of that should be cleaned up soon.

You should send multiple path responses in response to multiple path challenges, even on the same connection ID - unless you’re getting attacked with them at such a high rate that you can’t get them out the door quickly enough and you need to limit your memory consumption while doing that. 

-- 
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/2729#discussion_r288411344

---

From: Kazuho Oku <[email-masked]>
Date: Tue, 28 May 2019 23:42:27 -0700

kazuho commented on this pull request.

Maybe something like: "An endpoint MUST NOT send more than one PATH_RESPONSE frame in response to one PATH_REQUEST frame, as a new ..."

-- 
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/2729#discussion_r288413637

---

From: MikkelFJ <[email-masked]>
Date: Tue, 28 May 2019 23:43:14 -0700

Why do we bother with ACK on challenge / response? And is that an attack vector, given what you just said?

-- 
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/2729#discussion_r288413833

---

From: Dmitri Tikhonov <[email-masked]>
Date: Wed, 29 May 2019 05:50:12 -0700

dtikhonov commented on this pull request.

> You should send multiple path responses in response to multiple path challenges, even on the same connection ID

This seems unjustified.  Let's say an endpoint receives 100 _PATH_CHALLENGE_ frames in a single packet.  Is it to reply with 100 _PATH_RESPONSE_ frames?

-- 
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/2729#discussion_r288547484

---

From: David Schinazi <[email-masked]>
Date: Wed, 29 May 2019 06:26:53 -0700

I would personally recommend dropping the  "unless" clause and only adding the new MUST NOT statement in this PR. The attack of sending many path challenges is no worse than sending other types of frames (see discussion in #2129 and #2241). I worry that this "unless" is vague and will cause some implementations to not respond to path challenges in non-attack cases which will harm performance. One mitigation that makes more sense is to say that you MUST NOT send multiple PATH_CHALLENGE frames in a packet, and receiving multiple of them triggers a PROTOCOL_VIOLATION.

-- 
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/2729#issuecomment-496935980

---

From: Jana Iyengar <[email-masked]>
Date: Thu, 30 May 2019 22:57:21 +0000 (UTC)

janaiyengar commented on this pull request.

The MUST NOT store 

I don't think this addition is necessary.  QUIC does not require or suggest that PATH_RESPONSE be buffered, so an implementation would simply send or queue subsequent PATH_RESPONSE frames, as it should. Memory considerations are much larger with data, and I would argue that memory pressure due to too many PATH_CHALLENGEs isn't interesting.

I prefer @kazuho's rewording here: "And endpoint MUST NOT send more than one PATH_RESPONSE in response to a PATH_CHALLENGE frame, as a new ..."

-- 
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/2729#pullrequestreview-244067541