Subject: [quicwg/base-drafts] Fixed handshake key deadlock issue (#3093)

From: martinduke <[email-masked]>
Date: Tue, 15 Oct 2019 00:46:07 -0700

Along with some other PRs, I believe this fixes #2863.

Fundamentally, there are four ways to solve the problem of one endpoint throwing away the handshake keys while the other has no 1-RTT data.

1) Keep the handshake keys forever
2) Make the endpoint start a 1-RTT PTO timer whether or not it has data to send
3) Require each endpoint to send a retransmittable frame soon after the handshake is complete; or
4) Make the endpoint wait until it receives an ack-eliciting 1-RTT packet, in addition to a 1-RTT ack.

I believe that the discard keys group might have a plurality for #4. The added condition makes sure that the peer&#39;s PTO machinery has started and will have little effect on most applications, including HTTP3.

This PR is a concrete statement of the fourth option.
You can view, comment on, or merge this pull request online at:

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

-- Commit Summary --

  * Fixed handshake key deadlock issue

-- File Changes --

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

-- Patch Links --

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

---

From: Martin Thomson <[email-masked]>
Date: Tue, 15 Oct 2019 16:29:39 -0700

I don't think that this fixes the problem.  What we had discussed was requiring that some ack-eliciting 1-RTT data is sent after handshake completion.  With that, the existing definition of "confirmed" is adequate.

-- 
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/3093#issuecomment-542446237

---

From: Marten Seemann <[email-masked]>
Date: Wed, 16 Oct 2019 12:03:54 -0700

<img width="669" alt="2863" src="https://user-images.githubusercontent.com/1478487/66950326-ff98f980-f00c-11e9-93bb-84587ddd805d.png">

-- 
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/3093#issuecomment-542846395

---

From: Kazuho Oku <[email-masked]>
Date: Wed, 16 Oct 2019 12:09:09 -0700

Note: at the moment, recovery states "When a PTO timer expires, a sender MUST send at least one ack-eliciting packet as a probe, unless there is no data available to send."

We need remove the "unless".

-- 
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/3093#issuecomment-542848391

---

From: martinduke <[email-masked]>
Date: Wed, 16 Oct 2019 12:12:06 -0700

@martinthomson I believe discussion in Cupertino resolved your concern.

I believe @marten-seemann's chart has been addressed by discussion in Cupertino, pending further analysis that shows a different problem.

-- 
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/3093#issuecomment-542849528

---

From: Martin Thomson <[email-masked]>
Date: Wed, 16 Oct 2019 12:12:37 -0700

Belay that last comment from me.   The friendly amendment here that we need is to the text on discarding Handshake keys that says "no sooner than handshake confirmed" rather than requiring Handshake keys to be discarded at that point.

-- 
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/3093#issuecomment-542849717

---

From: martinduke <[email-masked]>
Date: Wed, 16 Oct 2019 13:57:51 -0700

@marten-seemann has a fairly silly corner case that breaks this:
1) the ack of the CFIN is lost; the server sends no 1-RTT packet.
2) the client acks and sends a PING
3) the server acks it.
4) the client sends ACK + PADDING (which is technically legal). This is sufficient for the server to discard state.
5) The client has not started the 1RTT PTO timer, and we are going to fail the connection.

This is fixable by disallowing ACK+PADDING in response to a non-ack-eliciting packet. At the moment 13.2.1 of quic-transport does not explicitly forbid this. IMO this is an oversight and we should change 13.2.1.

-- 
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/3093#issuecomment-542888691

---

From: Marten Seemann <[email-masked]>
Date: Wed, 16 Oct 2019 14:03:01 -0700

In my opinion, what this corner case shows is that option 4 is pretty brittle. Disallowing ACK  +PADDING seems to work to fix this particular corner case, but it doesn't guarantee that there isn't any other corner case that will lead to a deadlock.

-- 
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/3093#issuecomment-542890574

---

From: ianswett <[email-masked]>
Date: Wed, 16 Oct 2019 14:12:16 -0700

ACK+PADDING packets aren't ack-eliciting, so the server shouldn't discard keys at step 4.

-- 
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/3093#issuecomment-542893752

---

From: Marten Seemann <[email-masked]>
Date: Wed, 16 Oct 2019 14:14:51 -0700

@ianswett The server already received a PING in 1-RTT packets in step 2.

-- 
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/3093#issuecomment-542894610

---

From: martinduke <[email-masked]>
Date: Wed, 16 Oct 2019 14:14:58 -0700

@ianswett it has received an ack-eliciting packet in step 2 and a 1-RTT ack in 4. This is also fixable by changing the condition to "ack of an ack-eliciting packet"

-- 
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/3093#issuecomment-542894647

---

From: Marten Seemann <[email-masked]>
Date: Wed, 16 Oct 2019 14:17:23 -0700

@martinduke The ACK sent in step 4 is an acknowledgement for an ack-eliciting packet.

-- 
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/3093#issuecomment-542895443

---

From: martinduke <[email-masked]>
Date: Wed, 16 Oct 2019 14:19:14 -0700

@marten-seemann No it isn't, it's an ack of an ack.

-- 
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/3093#issuecomment-542896105

---

From: Martin Thomson <[email-masked]>
Date: Mon, 21 Oct 2019 17:20:16 -0700

As we concluded at the meeting, we're going to keep Handshake keys indefinitely.

FWIW, I don't believe that it is possible to find a confirmation step that doesn't involve both peers actively driving the state machine.  Otherwise, peers might not reach the same state within a reasonable time.  This change still has problems (more below) and only adds delay to the confirmed state by making the requirements more complex.

Here's an example of how this fix breaks.  I'm sure that there are more:

![image](https://user-images.githubusercontent.com/67641/67252125-5a26cf80-f4bd-11e9-915c-29e8bb33a7be.png)

<details><summary>web sequence diagram code</summary>
```
title Bad scenario

Client->Server: Initial
Client->Server: 0-RTT (ack-eliciting)
Server->Client: Initial + Handshake
Server->Client: 1-RTT (ACK for 0-RTT)

Client->Server: Handshake
Server-->Client: ACK (lost)

Client->Server: 1-RTT (ack-eliciting + ACK for 0.5-RTT)
Server->Client: 1-RTT (ACK for 1-RTT)
note right of Server
   The server now believes that 
   the handshake is confirmed
end note

note left of Client
   The client isn't confirmed
end note

note over Client,Server
   Considerable time can now elapse
end note

Client->Server: Handshake (retransmission)
note over Server: drop garbage
Client->Server: Handshake (retransmission)
note over Server: drop garbage ...
```
</details>

-- 
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/3093#issuecomment-544760798

---

From: Martin Thomson <[email-masked]>
Date: Mon, 21 Oct 2019 17:20:17 -0700

Closed #3093.

-- 
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/3093#event-2731650483

---

From: ianswett <[email-masked]>
Date: Mon, 21 Oct 2019 18:28:39 -0700

Thanks for the diagram, and I'm happy to stick with the approach of keeping the keys forever.

If we change nothing about the existing text, what you describe occurs, but as I suggested elsewhere, if we are going to delay dropping the Handshake keys(somewhat or indefinitely), I think we should update the text of transport or recovery to be clear that you can stop retransmitting any outstanding data in Handshake once you've received a 1-RTT ACK.

-- 
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/3093#issuecomment-544772965

---

From: martinduke <[email-masked]>
Date: Mon, 21 Oct 2019 19:44:27 -0700

This diagram is incorrect. There is an ack from the client of 0.5rtt data,
but the ack-eliciting packet is not present in the diagram. If it were, the
client would have discarded its handshake keys and there is no deadlock.

On Mon, Oct 21, 2019, 6:28 PM ianswett <[email-masked]> wrote:

> If we change nothing about the existing text, what you describe occurs,
> but as I suggested elsewhere, if we are going to delay dropping the
> Handshake keys(somewhat or indefinitely), I think we should update the text
> of transport or recovery to be clear that you can stop retransmitting any
> outstanding data in Handshake once you've received a 1-RTT ACK.
>
> —
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub
> <https://github.com/quicwg/base-drafts/pull/3093?email_source=notifications&email_token=AF2EYELRDBHBIVQPFLAFXO3QPZJMRA5CNFSM4JAYS242YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEB4JGZI#issuecomment-544772965>,
> or unsubscribe
> <https://github.com/notifications/unsubscribe-auth/AF2EYEOCVAYBG6U65TEXL2LQPZJMRANCNFSM4JAYS24Q>
> .
>

-- 
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/3093#issuecomment-544784960

---

From: Martin Thomson <[email-masked]>
Date: Mon, 21 Oct 2019 19:46:47 -0700

@martinduke, the diagram is correct.  There is no ack-eliciting packet in 0.5-RTT.  The ACK of the 0.5-RTT data is bundled with the other ack-eliciting frames in the packet, that's all.

-- 
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/3093#issuecomment-544785322

---

From: martinduke <[email-masked]>
Date: Tue, 22 Oct 2019 03:34:53 +0000 (UTC)

@martinthomson Ah, I see that now. Yeah, I think we need an explicit signal, which I would prefer over keeping the keys forever.

-- 
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/3093#issuecomment-544792621

---

From: MikkelFJ <[email-masked]>
Date: Mon, 21 Oct 2019 23:38:26 -0700

I don't see the problem with an explicit signal. The handshake is uniquely complex already and it helps isolate that complexity. You could even say that if you can drive a handshake to completion within a timeout period, the connection is broken. An explicit signal helps deal with cases where one endpoint otherwise has nothing to send. Now it has.

-- 
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/3093#issuecomment-544825620