Subject: [quicwg/base-drafts] Specify behavior for post-handshake CRYPTO messages (#2524)

From: Nick Harper <[email-masked]>
Date: Fri, 15 Mar 2019 16:54:35 -0700

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

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

-- Commit Summary --

  * Specify behavior for post-handshake CRYPTO messages

-- File Changes --

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

-- Patch Links --

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

---

From: ianswett <[email-masked]>
Date: Fri, 15 Mar 2019 19:03:12 -0700

ianswett commented on this pull request.

> @@ -1232,6 +1232,19 @@ handshake data start from zero in each packet number space.
 Endpoints MUST explicitly negotiate an application protocol.  This avoids
 situations where there is a disagreement about the protocol that is in use.
 
+Implementations need to maintain a buffer of CRYPTO data received out of order.
+Because there is no flow control of CRYPTO frames, and endpoint could
+potentially force its peer to buffer an unbounded amount of data.
+Implementations MUST support buffering at least 4096 bytes of data.
+
+Once the handshake completes, if an endpoint is unable to buffer all data in a
+CRYPTO frame, it MAY discard all subsequent CRYPTO frames, or it MAY close the
+connection with an INTERNAL_ERROR code. In the case where an endpoint chooses to

If we're going to close the connection, can we pick an error code from the CRYPTO_ERROR section?  "CRYPTO_ERROR (0x1XX):"

-- 
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/2524#pullrequestreview-215296528

---

From: Martin Thomson <[email-masked]>
Date: Sat, 16 Mar 2019 15:34:08 -0700

martinthomson requested changes on this pull request.

Thanks for writing this up Nick.

+Implementations need to maintain a buffer of CRYPTO data received out of order.
+Because there is no flow control of CRYPTO frames, and endpoint could
+potentially force its peer to buffer an unbounded amount of data.
+Implementations MUST support buffering at least 4096 bytes of data.
+
+Once the handshake completes, if an endpoint is unable to buffer all data in a
+CRYPTO frame, it MAY discard all subsequent CRYPTO frames, or it MAY close the
+connection with an INTERNAL_ERROR code. In the case where an endpoint chooses to
+discard all subsequent CRYPTO frames, the packets containing these CRYPTO frames
+are still ACKed. An endpoint in this condition MAY tear down state for its

```suggestion
are still ackknowledged. An endpoint in this condition MAY tear down state for its
```

```suggestion
connection with an INTERNAL_ERROR code. If an endpoint chooses to
```

+Implementations need to maintain a buffer of CRYPTO data received out of order.
+Because there is no flow control of CRYPTO frames, and endpoint could
+potentially force its peer to buffer an unbounded amount of data.
+Implementations MUST support buffering at least 4096 bytes of data.
+
+Once the handshake completes, if an endpoint is unable to buffer all data in a
+CRYPTO frame, it MAY discard all subsequent CRYPTO frames, or it MAY close the
+connection with an INTERNAL_ERROR code. In the case where an endpoint chooses to
+discard all subsequent CRYPTO frames, the packets containing these CRYPTO frames
+are still ACKed. An endpoint in this condition MAY tear down state for its
+cryptographic handshake, keeping only the material needed for the current
+encryption keys and any future key updates.

I would drop the last sentence here.  It's an optimization that is sufficiently obvious.

I think that this needs a new section heading.  Section 7.4 "After Handshake Completion" perhaps, after the transport parameters, could be what it is, but it might be better to say "Cryptographic Message Buffering" and discuss the handshake a little (more below).

Does this requirement apply during the handshake?  Perhaps document some recommendations.

Being unable to buffer CRYPTO frames during the handshake leads to connection failure. Endpoints MAY choose to allow more data to be buffered during the handshake and reduce buffering once the handshake completes.  A larger limit during the handshake could allow for larger keys or credentials to be exchanged.

-- 
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/2524#pullrequestreview-215332111

---

From: Nick Harper <[email-masked]>
Date: Mon, 18 Mar 2019 13:27:28 -0700

nharper commented on this pull request.

The CRYPTO_ERRORs are TLS alerts (which are one byte) converted into QUIC error codes, so we can't pick one of those. I've created a new one CRYPTO_BUFFER_EXCEEDED.

I moved this to the end of section 7 in a new subsection.

Done.

Done.

Dropped.

Yes, it applies during the handshake.

I've updated the text to discuss buffer sizing and separate out pre- vs post-handshake completion behavior.

-- 
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/2524#discussion_r266606030

---

From: Martin Thomson <[email-masked]>
Date: Mon, 18 Mar 2019 15:20:28 -0700

martinthomson approved this pull request.

I think that this is good.  Thanks for doing the hard work Nick.

I believe that this issue can be resolved without chair involvement as it is a conclusion from the Tokyo meeting.

> +## Cryptographic Message Buffering
+
+Implementations need to maintain a buffer of CRYPTO data received out of order.
+Because there is no flow control of CRYPTO frames, an endpoint could
+potentially force its peer to buffer an unbounded amount of data.
+
+Implementations MUST support buffering at least 4096 bytes of data received in
+CRYPTO frames out of order. Endpoints MAY choose to allow more data to be
+buffered during the handshake. A larger limit during the handshake could allow
+for larger keys or credentials to be exchanged. An endpoint's buffer size does
+not need to remain constant during the life of the connection.
+
+Being unable to buffer CRYPTO frames during the handshake leads to a connection
+failure. If an endpoint's buffer is exceeded during the handshake, it can expand
+its buffer temporarily to complete the handshake. If an endpoint cannot expand
+its buffer, it MAY close the connection with a CRYPTO_BUFFER_EXCEEDED error

```suggestion
its buffer, it MUST close the connection with a CRYPTO_BUFFER_EXCEEDED error
```

I don't see any other recourse, do you?

> +CRYPTO frames out of order. Endpoints MAY choose to allow more data to be
+buffered during the handshake. A larger limit during the handshake could allow
+for larger keys or credentials to be exchanged. An endpoint's buffer size does
+not need to remain constant during the life of the connection.
+
+Being unable to buffer CRYPTO frames during the handshake leads to a connection
+failure. If an endpoint's buffer is exceeded during the handshake, it can expand
+its buffer temporarily to complete the handshake. If an endpoint cannot expand
+its buffer, it MAY close the connection with a CRYPTO_BUFFER_EXCEEDED error
+code.
+
+Once the handshake completes, if an endpoint is unable to buffer all data in a
+CRYPTO frame, it MAY discard all subsequent CRYPTO frames, or it MAY close the
+connection with an CRYPTO_BUFFER_EXCEEDED error code. If an endpoint chooses to
+discard all subsequent CRYPTO frames, the packets containing these CRYPTO frames
+are still acknowledged.

```suggestion
MUST be acknowledged.
```

-- 
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/2524#pullrequestreview-215880600

---

From: Nick Harper <[email-masked]>
Date: Mon, 18 Mar 2019 15:36:30 -0700

@nharper pushed 1 commit.

4ae94ebdb9fc7ac35db090ab362c1c10fc9e15c3  Change some things to MUST

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/2524/files/7ec36cb2d514f944890f4d970ac12426ff71ab0f..4ae94ebdb9fc7ac35db090ab362c1c10fc9e15c3

---

From: Nick Harper <[email-masked]>
Date: Mon, 18 Mar 2019 15:36:40 -0700

+

I was thinking it could grow its buffer, but I've already preconditioned with it not doing that, so a MUST here makes sense.

-- 
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/2524#discussion_r266668048

---

From: Marten Seemann <[email-masked]>
Date: Mon, 18 Mar 2019 15:37:03 -0700

I’d prefer to always close the connection instead of silently discarding the frames. The problem with discarding frames is that while it doesn’t really hurt with “normal” TLS 1.3, there might be TLS extensions which require the reliable delivery of post-Handshake messages.
Furthermore, acknowledging a packet means that a peer has processsed all the frames. Silently dropping frames seems like an awkward exception to that rule. 

-- 
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/2524#issuecomment-474128186

---

From: Ryan Hamilton <[email-masked]>
Date: Mon, 18 Mar 2019 20:45:28 -0700

@marten-seemann Good point. That seems like a good principle to apply 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/2524#issuecomment-474187343

---

From: Nick Harper <[email-masked]>
Date: Mon, 18 Mar 2019 23:08:44 -0700

We discussed this in Tokyo ([minutes](https://github.com/quicwg/wg-materials/blob/master/interim-19-01/minutes.md#flow-control-for-post-handshake-crypto-messages-1834)), and there are basically 3 options here:

- Implement proper flow control for CRYPTO frames
- Specify a buffer size (50k is the example from the Tokyo discussion)
- Drop CRYPTO frames if an endpoint has to buffer too many

My sense of the room was that the 3rd option was the preferred option for now.

When we have TLS extensions where we care about reliable delivery of post-handshake messages, that sounds like a good time to add proper flow control. I don't like picking a fixed buffer size for the same reason as using timers for discarding keys. If we pick a buffer size now, how do we know that it will be big enough for whatever future TLS extensions might get used with QUIC (or future key exchanges)?

I don't think that this breaks a rule of acknowledging packets means that the peer has processed all frames in that packet. For example, if an endpoint receives a packet containing stream data that is a retransmission of previously received data, that endpoint in effect is dropping that STREAM frame. Here, the CRYPTO frame is being processed (and the endpoint is immediately choosing to do nothing based on 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/2524#issuecomment-474210701

---

From: ianswett <[email-masked]>
Date: Tue, 19 Mar 2019 14:19:53 +0000 (UTC)

As @martinthomson said, this was discussed in Tokyo and I believe included in the recent consensus call on the list, so I'd prefer to land this and gain some implementation experience, even if I might personally prefer a different solution.  As @nharper states, there are 3 options, and the WG has repeatedly said no to flow control and a number of people didn't like a fixed limit on the entire connection, so we ended up with the 3rd option.

-- 
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/2524#issuecomment-474391329

---

From: ianswett <[email-masked]>
Date: Tue, 19 Mar 2019 14:20:56 +0000 (UTC)

ianswett 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/2524#pullrequestreview-216186382

---

From: Marten Seemann <[email-masked]>
Date: Tue, 19 Mar 2019 07:36:45 -0700

> I don't think that this breaks a rule of acknowledging packets means that the peer has processed all frames in that packet. For example, if an endpoint receives a packet containing stream data that is a retransmission of previously received data, that endpoint in effect is dropping that STREAM frame. 

I don't think this is a fair characterization. For the sender, it doesn't really matter if it was this copy of the data, or an earlier STREAM frame containing exactly the same data. The ACK signals that the data was delivered to the application, and that's all that matters.

> Here, the CRYPTO frame is being processed (and the endpoint is immediately choosing to do nothing based on it).

Exactly the opposite. Processing the CRYPTO frame would mean to deliver it to the TLS stack. Here it's dropped one layer below that. Everywhere else in the spec, we use "processing" in the sense that the data has been acted upon, not in the sense that the frame was merely parsed.

-- 
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/2524#issuecomment-474398859

---

From: Nick Banks <[email-masked]>
Date: Tue, 19 Mar 2019 07:57:59 -0700

nibanks approved this pull request.

I think this is definitely in a gray area. Generally, I view the action of sending an ACK for a packet as saying "I've received the data you sent me, and I don't need that data any more. Don't retransmit it." It's really up to the receiver to decide what that means. The sender can then always safely assume the data doesn't need to be retransmitted. It can't know when (or if) the data ever gets delivered to the next layer; whether that's the application sitting on top of a stream or TLS on top of the crypto stream. Only by receiving data from that next layer in response can the original sender know for sure.

-- 
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/2524#pullrequestreview-216212690

---

From: MikkelFJ <[email-masked]>
Date: Tue, 19 Mar 2019 08:26:55 -0700

Is this buffering covering the internal TLS state engine, or is that separate?

-- 
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/2524#issuecomment-474423298

---

From: Nick Harper <[email-masked]>
Date: Tue, 19 Mar 2019 17:46:02 +0000 (UTC)

> Is this buffering covering the internal TLS state engine, or is that separate?

This is buffering in the QUIC layer (specifically buffering out of order data that can't be delivered to the TLS stack) - the TLS stack probably has its own internal buffers, but that's opaque to QUIC.

-- 
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/2524#issuecomment-474491576

---

From: Martin Thomson <[email-masked]>
Date: Tue, 19 Mar 2019 16:18:02 -0700

As @nharper says, the TLS stack will be expecting data to be delivered in order.  If it can't keep up, or data arrives waaay out of order, that is when the transport needs to decide what to do.  (That is why this text belongs in the transport doc and not the TLS one, in case people were wondering.)

-- 
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/2524#issuecomment-474621315