Subject: Re: [quicwg/base-drafts] Discard Initial keys as soon as possible (#2045)

From: janaiyengar <[email-masked]>
Date: Mon, 03 Dec 2018 20:35:24 -0800

janaiyengar commented on this pull request.

> @@ -3592,6 +3594,21 @@ and will contain a CRYPTO frame with an offset matching the size of the CRYPTO
 frame sent in the first Initial packet.  Cryptographic handshake messages
 subsequent to the first do not need to fit within a single UDP datagram.
 
+
+### Abandoning Initial Packets {#discard-initial}
+
+A client stops both sending and accepting Initial packets when it sends its
+first Handshake packet.  A server stops sending and accepting Initial packets
+when it receives its first Handshake packet.  Though packets might still be in
+flight or awaiting acknowledgment, no further Initial packets need to be
+exchanged beyond this point.  Initial packet protection keys are discarded (see
+Section 4.10 of {{QUIC-TLS}}) along with any loss recovery and congestion
+control state (see Sections 5.3.1.2 and 6.9 of {{QUIC-RECOVERY}}).

Good points. I'd say the conservative approach is the one @martinthomson suggests. I wonder if it might be better to be more explicit about it though, but I'll save that for another PR I think.

-- 
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/2045#discussion_r238527338

---

From: Martin Thomson <[email-masked]>
Date: Tue, 04 Dec 2018 11:28:30 -0800

@martinthomson pushed 1 commit.

5069203066a8a5302f60cbfe215d940dd4c4f9d8  Tweaks based on review feedback

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/2045/files/98feded4a7630c7c0b6cfd47066d671706d1c25b..5069203066a8a5302f60cbfe215d940dd4c4f9d8

---

From: janaiyengar <[email-masked]>
Date: Tue, 11 Dec 2018 15:30:50 -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/2045#pullrequestreview-183953983

---

From: Martin Thomson <[email-masked]>
Date: Wed, 12 Dec 2018 00:40:41 +0000 (UTC)

Just a note of warning.  @ianswett is going to review this within 24 hours and I expect to merge it shortly afterwards.

-- 
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/2045#issuecomment-446417743

---

From: ekr <[email-masked]>
Date: Tue, 11 Dec 2018 18:30:08 -0800

I continue to be unconvinced this is the right answer, and I'd prefer you not merge it. If you elect to do so in any case, I am going to want to talk about it in Tokyo.

-- 
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/2045#issuecomment-446438724

---

From: ianswett <[email-masked]>
Date: Tue, 11 Dec 2018 18:31:25 -0800

@ekr Clarification question: Do you think this is an issue we should just not address in QUIC v1 or there are other solutions not in this PR?

-- 
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/2045#issuecomment-446438934

---

From: ekr <[email-masked]>
Date: Tue, 11 Dec 2018 18:35:08 -0800

I'm not sure we need to address it in QUICv1 at all. Rather than piecemeal
fixes, I'd like to start with a clear agreement about what the proposed
threat model is for man on the side attacks. Once we've got consensus on
that, we can then fix all the attacks that are implicated by that threat
model.

On Tue, Dec 11, 2018 at 6:31 PM ianswett <[email-masked]> wrote:

> @ekr <https://github.com/ekr> Clarification question: Do you think this
> is an issue we should just not address in QUIC v1 or there are other
> solutions not in this PR?
>
> —
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub
> <https://github.com/quicwg/base-drafts/pull/2045#issuecomment-446438934>,
> or mute the thread
> <https://github.com/notifications/unsubscribe-auth/ABD1oaOoTZCrPO2f3w3mUJkZ69GWrMf7ks5u4Gp8gaJpZM4YwALg>
> .
>

-- 
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/2045#issuecomment-446439591

---

From: Martin Thomson <[email-masked]>
Date: Tue, 11 Dec 2018 19:28:32 -0800

I think that there is a principled position that can be used to justify this change and not (for example) #2076.  Taking that to the list is the right answer though.

I don't know how to avoid having this delay -17.  I guess we'll just have to see.

-- 
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/2045#issuecomment-446448947

---

From: ianswett <[email-masked]>
Date: Tue, 11 Dec 2018 19:32:09 -0800

ianswett commented on this pull request.

Thanks Martin, I think this is now fairly close.

+

This draft uses packet processing, and accepting for things like "accepting" new connections, so I think processing is better here and on 3601

> @@ -1059,6 +1084,15 @@ A sender that does not use pacing SHOULD reset its congestion window to the
 minimum of the current congestion window and the initial congestion window.
 This recommendation is based on Section 4.1 of {{?RFC5681}}.
 
+## In-Flight Packet Accounting
+
+When keys for an packet number space are discarded (see {{QUIC-TLS}}), any
+packets sent with those keys are removed from the count of bytes in flight.  No

clarification: any in-flight packets sent with those keys

> @@ -439,6 +456,14 @@ Either packet indicates that the Initial was received but not processed.
 Neither packet can be treated as an acknowledgment for the Initial, but they MAY
 be used to improve the RTT estimate.
 
+#### Discarding Initial State {#discard-initial}
+
+As described in Section 4.10 of {{QUIC-TLS}}, endpoints stop sending and

I think it makes more sense to reference the transport draft, now that it has the required text.  Also, it's a bit odd to have the recovery draft have a direct dependency on TLS.

Most of this paragraph seems to apply to the Handshake encryption level as well, correct?  

> @@ -1059,6 +1084,15 @@ A sender that does not use pacing SHOULD reset its congestion window to the
 minimum of the current congestion window and the initial congestion window.
 This recommendation is based on Section 4.1 of {{?RFC5681}}.
 
+## In-Flight Packet Accounting

I would call this section something else like "Discarding Keys" or "Abandoning Encryption Levels"

> @@ -439,6 +456,14 @@ Either packet indicates that the Initial was received but not processed.
 Neither packet can be treated as an acknowledgment for the Initial, but they MAY
 be used to improve the RTT estimate.
 
+#### Discarding Initial State {#discard-initial}
+
+As described in Section 4.10 of {{QUIC-TLS}}, endpoints stop sending and
+receiving Initial packets once they start exchanging Handshake packets.  At this
+point, all loss recovery state for the Initial packet number space is also
+discarded.  After discarding state, new Initial packets will not be sent and no

```suggestion
discarded without declaring them acknowledged or lost.  After discarding state, new Initial packets will not be sent.
```

-- 
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/2045#pullrequestreview-183998679

---

From: Martin Thomson <[email-masked]>
Date: Tue, 11 Dec 2018 19:48:05 -0800

79e16843a1f092a07efa6e72a3c40842a1f83a12  Ian's review

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/2045/files/5069203066a8a5302f60cbfe215d940dd4c4f9d8..79e16843a1f092a07efa6e72a3c40842a1f83a12

---

From: ianswett <[email-masked]>
Date: Wed, 12 Dec 2018 10:55:14 +0000 (UTC)

ianswett approved this pull request.

I'm not sure if we want to do this, and if so if we want to do it in time for draft 17, but I think the text now looks fine.

-- 
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/2045#pullrequestreview-184117842

---

From: ianswett <[email-masked]>
Date: Wed, 12 Dec 2018 06:19:24 -0800

One thought.  There are two parts of this PR.
 1) What to do when you drop keys, which the spec currently requires in some cases and others may want to do(implicit acking).
 2) Add the requirement that all implementations support implicit acking.

We really need the first part, even if we discuss the second part further.

-- 
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/2045#issuecomment-446603293

---

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

I've been avoiding using "implicit ACK" to describe this, because it's at a different layer.  ACKs deal with packets, but this is really looking at the packet number space.

The parts of this that relate to what to do with those dangling packets from an old packet number space are only really necessary here because we are moving the point where state is discarded earlier.  For other packet number spaces, they are long quiescent by the time any discarding happens, so the advice we'd provide would be largely academic.

-- 
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/2045#issuecomment-446775661

---

From: Christian Huitema <[email-masked]>
Date: Wed, 12 Dec 2018 23:09:07 +0000 (UTC)

I am looking at the arguments on this thread, and I think EKR and Kazuho are making very similar points:

1) The current state is that packet injection attacks are possible until 3 RTT after handshake is complete, or about 4 RTT from the time the Client Hello is sent.

2) This PR (#2045) would reduce the window from 4 RTT to 1 RTT.

3) But it is reasonably easy for attackers to inject packets within 0.5 RTT from the point the Client Hello is sent.

4) Thus we get a piecemeal defense that adds some complexity and risk but does not actually solve the problem.

I feel sad.

-- 
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/2045#issuecomment-446778962

---

From: Martin Thomson <[email-masked]>
Date: Wed, 12 Dec 2018 19:53:47 -0800

Yep, that seems like a reasonable summation.  But I hold hopes for the out-of-band configuration that you and Kazuho are working on.  I don't think that it is worth holding v1 for though.  Especially since it imposes some deployment constraints that could be onerous.

-- 
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/2045#issuecomment-446833996

---

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

357f53bd6de9d7b33838f427efe35e72f1853a60  Merge branch 'master' into abandon-initial

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/2045/files/79e16843a1f092a07efa6e72a3c40842a1f83a12..357f53bd6de9d7b33838f427efe35e72f1853a60

---

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

Merged #2045 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/2045#event-2034346760