Subject: [quicwg/base-drafts] forbid key discarding before receiving a packet protected with new keys (#4079)

From: Marten Seemann <[email-masked]>
Date: Tue, 08 Sep 2020 04:13:35 -0700

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

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

-- Commit Summary --

  * forbid key discarding before receiving a packet protected with new keys

-- File Changes --

    M draft-ietf-quic-tls.md (28)

-- Patch Links --

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

---

From: Marten Seemann <[email-masked]>
Date: Tue, 08 Sep 2020 04:17:15 -0700

@marten-seemann pushed 1 commit.

64ed3d99411adfcb48e264e037d17857d08ef6ce  forbid key discarding before receiving a packet protected with new keys

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/4079/files/411e65ce5c9a80c03a8413baa0a2c2aafddc4a59..64ed3d99411adfcb48e264e037d17857d08ef6ce

---

From: Marten Seemann <[email-masked]>
Date: Tue, 08 Sep 2020 04:25:02 -0700

8c2eadfbfd376e86e812a72432c4a1d98e4f9629  forbid key discarding before receiving a packet protected with new keys

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/4079/files/64ed3d99411adfcb48e264e037d17857d08ef6ce..8c2eadfbfd376e86e812a72432c4a1d98e4f9629

---

From: Kazuho Oku <[email-masked]>
Date: Tue, 08 Sep 2020 05:01:47 -0700

@kazuho commented on this pull request.

Thank you for working on the PR. I like the changes to section 6.5. Regarding changes to section 6.1, please see my comments below.

> @@ -1496,10 +1496,11 @@ The endpoint that initiates a key update also updates the keys that it uses for
 receiving packets.  These keys will be needed to process packets the peer sends
 after updating.
 
-An endpoint SHOULD retain old keys so that packets sent by its peer prior to
-receiving the key update can be processed.  Discarding old keys too early can
-cause delayed packets to be discarded.  Discarding packets will be interpreted
-as packet loss by the peer and could adversely affect performance.
+An endpoint MUST retain old keys until it has successfully unprotected a packet
+sent using the new keys.  An endpoint SHOULD NOT discard old keys immediately
+after unprotecting a packet sent using the new keys.  Discarding old keys too
+early can cause delayed packets to be discarded.  Discarding packets will be
+interpreted as packet loss by the peer and could adversely affect performance.

Regarding the first sentence, I think SHOULD works too, but I agree that MUST is better here.

I do not think we need the second sentence, as we state that an endpoint "SHOULD retain no more than 3 PTO" down below.

I think the crux of the problem is that the last two sentences that are left unmodified. As correctly pointing out by the issue, the outcome of discarding the keys too early is more than just a performance degradation. Doing so might kill the connection. We should state that clearly, and then the choice of the keyword in the first sentence becomes more of an editorial choice.

-- 
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/4079#pullrequestreview-484061184

---

From: Mike Bishop <[email-masked]>
Date: Tue, 08 Sep 2020 13:56:09 -0700

@MikeBishop commented on this pull request.

I'm a bit uncomfortable tying the trigger to the peer's own key update, even though they're required to update in step.  Would it make more sense to say you don't discard until you've received an acknowledgement for a packet sent with the new keys?  That already has to be tracked for the maximum frequency of key updates, so it would make sense to hang this mechanism on it as well.  If you know the peer has received a packet from you in the new phase, they'll be updating soon enough, but you know they have read keys which is the real issue.

> @@ -1617,9 +1619,9 @@ after receiving an acknowledgment that confirms that the previous key update was
 received.  Failing to allow sufficient time could lead to packets being
 discarded.
 
-An endpoint SHOULD retain old read keys for no more than three times the PTO.
-After this period, old read keys and their corresponding secrets SHOULD be
-discarded.
+An endpoint SHOULD retain old read keys for no more than three times the PTO
+afer having received a packet protected using the new keys. After this period,

```suggestion
after having received a packet protected using the new keys. After this period,
```

It *is* a performance issue provided the peer keeps sending.  You just lose some reordered packets and have spurious detection of some extra packet loss.  Where the connection hard-fails is when the peer never updates / learns about the update because the initiator stops sending.

-- 
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/4079#pullrequestreview-484485061

---

From: ianswett <[email-masked]>
Date: Tue, 08 Sep 2020 15:20:07 -0700

@ianswett pushed 1 commit.

1bad2789c8045b6cc43678e4b602a38c2adec9c3  Update draft-ietf-quic-tls.md

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/4079/files/3064d6b9cfa4e4f79fdbb9fae7f884079f3504f4..1bad2789c8045b6cc43678e4b602a38c2adec9c3

---

From: Kazuho Oku <[email-masked]>
Date: Tue, 08 Sep 2020 15:45:15 -0700

I'm not sure if I agree.

As @marten-seemann points out, an endpoint that only receives data (but does not send any) might decide to do a key update. If that endpoint drops the old receive key before any of the packets _it_ sent reaches the peer, the connection will be stuck.

That is because all the ack-eliciting packets involved will be the ones that are sent by the peer using the old key, which the endpoint has dropped that key.

-- 
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/4079#discussion_r485235463

---

From: Martin Thomson <[email-masked]>
Date: Tue, 08 Sep 2020 17:06:50 -0700

@martinthomson approved this pull request.

This is a good change.  The MUST regarding retention is already (strongly) implied, so it's not a normative change.  But we definitely benefit from this being direct.  I just have a tiny editorial suggestion.

SHOULD NOT isn't a thing.  you want

```suggestion
sent using the new keys.  An endpoint SHOULD retain old keys for some time
```

-- 
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/4079#pullrequestreview-484529391

---

From: Marten Seemann <[email-masked]>
Date: Tue, 08 Sep 2020 20:15:13 -0700

4898bcdbe025da21c27db41352948f361deeb3f2  Apply suggestions from code review

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/4079/files/1bad2789c8045b6cc43678e4b602a38c2adec9c3..4898bcdbe025da21c27db41352948f361deeb3f2

---

From: Marten Seemann <[email-masked]>
Date: Tue, 08 Sep 2020 20:20:55 -0700

@marten-seemann commented on this pull request.

So here's what happens when you initiate a key update from key phase N to N+1: You start using N+1 for encrypting packets. As it's not guaranteed that those packets are received by the peer, it's not safe to drop keys for key phase N yet. You only know that the peer received a packet protected with N+1 keys once you successfully unprotect a packet encrypted with N+1 keys (as the peer is required to update its keys immediately once it receives your N+1 packet).
>From this point on, it is **safe** to drop key N. However, it's not recommended, as there might be reordering, so keeping the key for a bit longer allows you to process delayed packets. At this point, this is just a performance optimization.

-- 
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/4079#discussion_r485313351

---

From: Martin Thomson <[email-masked]>
Date: Tue, 08 Sep 2020 20:54:30 -0700

@martinthomson commented on this pull request.

Yes, it's safe to drop key N when you receive packets protected with key N+1.  And it's true that keeping keys for a short while helps avoid dropping reordered packets.

However, it it generally safer to only drop key N when you receive an acknowledgment for a packet sent with N+1 keys.  Because the other side might have updated on its own.  That means that the N+1 packet you received isn't any indication that they received your key update.  This distinction doesn't matter for dropping keys, but it does matter for determining whether you can initiate an update to N+2 keys later.

-- 
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/4079#discussion_r485321502

---

From: Marten Seemann <[email-masked]>
Date: Tue, 08 Sep 2020 21:37:21 -0700

> However, it it generally safer to only drop key N when you receive an acknowledgment for a packet sent with N+1 keys.

That's correct. In my mental model (and in my implementation), these are two separate conditions though. I can start the timer to drop the keys before I'm allowed to update to N+2.

Do you think we need to make any changes to this text here?

-- 
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/4079#discussion_r485332383

---

From: Martin Thomson <[email-masked]>
Date: Tue, 08 Sep 2020 21:42:05 -0700

I think that it's OK to have two events, and technically the drop events are bound to use of keys.  In that regard, I'm comfortable with the text you have.  I don't think we need to rely exclusively on acknowledgments for driving 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/4079#discussion_r485333549

---

From: ianswett <[email-masked]>
Date: Wed, 09 Sep 2020 16:12:42 -0700

@ianswett approved this pull request.

This seems like what we intended to me.

-- 
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/4079#pullrequestreview-485462972

---

From: Martin Thomson <[email-masked]>
Date: Wed, 09 Sep 2020 18:51:31 -0700

Merged #4079 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/4079#event-3748528637