Subject: [quicwg/base-drafts] Close in response to invalid CONNECTION_CLOSE (#3230)

From: Martin Thomson <[email-masked]>
Date: Mon, 11 Nov 2019 21:40:57 -0800

The looping case was a little unclear.  This should make it a tiny bit
clearer.  As Roni observes, we can&#39;t really let invalid CONNECTION_CLOSE
through without some sort of special handling.  This proposes that.

Unfortunately, this is a little more than an editorial fix.  So I&#39;m
going to ask that the issue be promoted.

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

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

-- Commit Summary --

  * Close in response to invalid CONNECTION_CLOSE

-- File Changes --

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

-- Patch Links --

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

---

From: Marten Seemann <[email-masked]>
Date: Mon, 11 Nov 2019 21:44:39 -0800

marten-seemann requested changes on this pull request.

> @@ -2709,8 +2710,10 @@ frame risks a peer missing the first such packet.  The only mechanism available
 to an endpoint that continues to receive data for a terminated connection is to
 use the stateless reset process ({{stateless-reset}}).
 
-An endpoint that receives an invalid CONNECTION_CLOSE frame MUST NOT signal the
-existence of the error to its peer.
+An endpoint that receives an invalid CONNECTION_CLOSE frame MUST treat the

What is an invalid CONNECTION_CLOSE frame?

-- 
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/3230#pullrequestreview-315304778

---

From: Martin Thomson <[email-masked]>
Date: Mon, 11 Nov 2019 21:45:59 -0800

martinthomson commented on this pull request.

One that has an reason phrase that overruns the packet would do it.  I can't see any other reason.

-- 
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/3230#discussion_r345025722

---

From: Marten Seemann <[email-masked]>
Date: Mon, 11 Nov 2019 21:47:54 -0800

marten-seemann commented on this pull request.

I don't think we need any extra rules for this special case. Just do what you'd normally do: close the connection with a FRAME_ENCODING_ERROR.

-- 
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/3230#discussion_r345026064

---

From: Marten Seemann <[email-masked]>
Date: Mon, 11 Nov 2019 21:49:36 -0800

> @@ -2397,12 +2397,13 @@ transmission of CONNECTION_CLOSE frames to validated addresses or drop packets
 without response if the response would be more than three times larger than the
 received packet.
 
-After receiving a CONNECTION_CLOSE frame, endpoints enter the draining state.
-An endpoint that receives a CONNECTION_CLOSE frame MAY send a single packet
-containing a CONNECTION_CLOSE frame before entering the draining state, using a
-CONNECTION_CLOSE frame and a NO_ERROR code if appropriate.  An endpoint MUST NOT
-send further packets, which could result in a constant exchange of
-CONNECTION_CLOSE frames until the closing period on either peer ended.
+After receiving a CONNECTION_CLOSE frame, or an invalid packet that appears to
+contain a CONNECTION_CLOSE frame, endpoints enter the draining state.  An

Looks like Github swallowed my previous comment on this line:
How would you even parse an invalid packet? What does it mean to "appear to contain" a frame?

-- 
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/3230#pullrequestreview-315305981

---

From: Martin Thomson <[email-masked]>
Date: Mon, 11 Nov 2019 21:50:09 -0800

I see, you think that it is enough to rely on rate limiting of the CONNECTION_CLOSE packet (c.f., #3095).  That would also work.

-- 
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/3230#discussion_r345026471

---

From: MikkelFJ <[email-masked]>
Date: Tue, 12 Nov 2019 05:09:33 -0800

mikkelfj commented on this pull request.

-An endpoint that receives an invalid CONNECTION_CLOSE frame MUST NOT signal the
-existence of the error to its peer.
+An endpoint that receives an invalid CONNECTION_CLOSE frame MUST treat the
+message as being equivalent to a CONNECTION_CLOSE with the INTERNAL_ERROR error
+code.  The endpoint MAY send a single CONNECTION_CLOSE in response, but it MUST
+then enter the draining period; see {{draining}}.

Would INTERNAL_ERROR not suggest a locally induced inconsistent state as opposed to an observed remote inconsistent state? If so, I think it is important to make a distinction since it affects who is to blame and consequently where to debug.

-- 
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/3230#pullrequestreview-315519376

---

From: Mike Bishop <[email-masked]>
Date: Tue, 12 Nov 2019 10:03:01 -0800

MikeBishop commented on this pull request.

A packet might "appear to contain a frame" if it contains the truncated beginning of a frame.

-- 
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/3230#discussion_r345359295

---

From: ianswett <[email-masked]>
Date: Tue, 12 Nov 2019 12:50:35 -0800

ianswett commented on this pull request.

"or an invalid packet that appears to contain a CONNECTION_CLOSE_ frame," is a pretty big category of potential issues.  Does this need to be called out as separate from the general invalid packet case?

-- 
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/3230#discussion_r345436922

---

From: Martin Thomson <[email-masked]>
Date: Tue, 12 Nov 2019 13:21:57 -0800

I think that we'll just go with #3231 and avoid having to worry about this definition.

-- 
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/3230#discussion_r345451126

---

From: Martin Thomson <[email-masked]>
Date: Thu, 28 Nov 2019 17:03:02 -0800

Closed #3230 via #3231.

-- 
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/3230#event-2840091184