Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Flow control for post-handshake CRYPTO messages #1834

Closed
vasilvv opened this issue Oct 4, 2018 · 13 comments · Fixed by #2524
Closed

Flow control for post-handshake CRYPTO messages #1834

vasilvv opened this issue Oct 4, 2018 · 13 comments · Fixed by #2524
Assignees
Labels
-tls design An issue that affects the design of the protocol; resolution requires consensus. has-consensus An issue that the Chairs have determined has consensus, by canvassing the mailing list.

Comments

@vasilvv
Copy link
Contributor

vasilvv commented Oct 4, 2018

Consider the following situation. The client sends a 0-RTT handshake. The server accepts the handshake, installs the 1-RTT keys and sends its ServerHello...Finished flight. The client can now send a bunch of post-handshake messages to the server which the server cannot process until it receives ClientFinished, so it has to buffer them. We should bound that buffer somehow.

@davidben
Copy link

davidben commented Oct 4, 2018

(Client post-handshake messages don't really exist today, but they might in the future, e.g., with https://tools.ietf.org/html/draft-wood-tls-ticketrequests-00.)

Another incarnation of this problem is if the client receives a huge window of CRYPTO frames into the future but not the first byte, so it can't process anything yet. With STREAM frames, we have flow control. With CRYPTO frames during the handshake, we know the handshake itself is bounded. Post-handshake, we don't know a priori the server won't send lots and lots of tickets.

Of course, realistically, it won't send that many in a row, so one could get away with just picking a bound. But this is much more of an ad-hoc bound than the handshake. It also may send lots of tickets over the course of a connection, so the bound is in how far ahead it can get, rather than the handshake which can impose a stronger total bound. This then starts looking an awful lot like flow control...

@kazuho
Copy link
Member

kazuho commented Oct 5, 2018

@vasilvv

The client can now send a bunch of post-handshake messages to the server which the server cannot process until it receives ClientFinished, so it has to buffer them. We should bound that buffer somehow.

Isn't the expected behavior of the server to drop the 1-RTT packets containing the post-handshake messages until it receives the ClientFinished (that activates the 1-RTT read key)?

To put it another way, my understanding is that you can have any size of buffer you want to better cope with packet reordering, but that happens per-packet rather than per-frame (because it's not just the post-handshake messages that cannot be trusted prior to receiving ClientFinished), and that the buffer can be arbitrary sized because you can drop it to trigger a retransmit.

Am I missing something?

@kazuho
Copy link
Member

kazuho commented Oct 5, 2018

@davidben

Another incarnation of this problem is if the client receives a huge window of CRYPTO frames into the future but not the first byte, so it can't process anything yet. With STREAM frames, we have flow control. With CRYPTO frames during the handshake, we know the handshake itself is bounded. Post-handshake, we don't know a priori the server won't send lots and lots of tickets.

I agree that the problem exists.

However, in my view, the memory exhaustion issue does not get resolved just by having a window for the CRYTO frames. My understanding that most TLS stacks buffers the octets of a handshake message until it receives a complete one. That means that the TLS stack is expected to buffer at most 16MB of junk even if we introduce window to the CRYPTO frames.

Considering that, I think it would be preferable to at first discuss introducing bounds for the TLS messages in the TLS working group, then consider how to apply that to QUIC. Until then, each QUIC stack or TLS stack can (continue to) have its own internal limit.

@davidben
Copy link

davidben commented Oct 5, 2018

I don't think that's an accurate reduction.

TLS stacks usually impose ad-hoc limits tighter than the maximum 2^24, sometimes as a function of the maximum certificate size the caller configures. It's unfortunate that it's ad-hoc but different consumers do indeed tolerate different certificate sizes. It's plausible to do that analysis per-message and to do it per-flight since the set of messages in a flight is one of a few fixed patterns.

Post-handshake is very different. The pattern of messages is no longer fixed. Rather it is a property of how the application uses TLS. Indeed the QUIC spec even calls out applications choosing to send tickets at various points during the connection. Thus TLS-level limits, ad-hoc or spec'd, are not sufficient in this case. QUIC needs to do its share of the work.

(There's a reason I dislike post-handshake messages so much. :-) People always forget this complexity.)

@davidben
Copy link

davidben commented Oct 5, 2018

Re the client Finished case, I guess that pulls in the mess of stuff around there. The spec is a little unclear what a server does in between server and client Finished. In particular, if doing 0-RTT the server needs to at least be willing to send half-RTT data. This text suggests that, since there's a binder, processing out of order is okay. (Unless I'm reading it wrong?)
https://tools.ietf.org/html/draft-ietf-quic-tls-15#section-5.7

I agree that, if you believe the server just shouldn't process 1RTT packets before client Finished at all, the buffering issue is solved. Though it does leave it unable to read the ACKs to half-RTT data which is differently weird. :-)

I think the general post-handshake issue is probably the more isolated instance. They're the same problem in terms of buffering, but one has a confounding factor around whether it's actually an issue.

@kazuho
Copy link
Member

kazuho commented Oct 5, 2018

@davidben

Re the client Finished case, I guess that pulls in the mess of stuff around there. The spec is a little unclear what a server does in between server and client Finished. In particular, if doing 0-RTT the server needs to at least be willing to send half-RTT data. This text suggests that, since there's a binder, processing out of order is okay. (Unless I'm reading it wrong?)
https://tools.ietf.org/html/draft-ietf-quic-tls-15#section-5.7

Oh I missed it. Thank you for pointing that out.

I agree that, if you believe the server just shouldn't process 1RTT packets before client Finished at all, the buffering issue is solved.

I had assumed that TLS stacks always expose the 1-RTT read key to the QUIC stacks acting as a server when it receives ClientFinished. I'd be curious to know if there are TLS stacks that takes an alternative approach.

Though it does leave it unable to read the ACKs to half-RTT data which is differently weird. :-)

I assume that issue is unrelated to 0-RTT, right?

IIUC the advise to the QUIC stacks is that until you know that the peer can process a packet, you might want to coalesce the packet you send with another packet that uses a lower-epoch key; then you can have a weak confirmation that the packet has reached the peer. Having such signal is beneficial to not give false information to the congestion controller though the loss recovery code needs to assume that the packet might have been lost, because a stack is allowed to drop a packet that is was unable to decrypt.

I think the general post-handshake issue is probably the more isolated instance. They're the same problem in terms of buffering, but one has a confounding factor around whether it's actually an issue.

I agree. I think it is preferable to fix the issue.

Maybe I should rephrase my argument to that there should be limits in both layers (i.e. limit on the post-handshake message size in the TLS layer, and the window size in the QUIC layer), because having limit only on one layer does not prevent excessive buffering on the other layer. The two limits could either be a shared single value, two values shared using one extension, or two values shared using different mechanisms.

@martinthomson martinthomson added design An issue that affects the design of the protocol; resolution requires consensus. -tls labels Oct 16, 2018
@martinthomson
Copy link
Member

I'm very much inclined to leave this "unfixed", but to leave a warning that sending large post-handshake messages risks connection failure.

I had assumed that TLS stacks always expose the 1-RTT read key to the QUIC stacks acting as a server when it receives ClientFinished. I'd be curious to know if there are TLS stacks that takes an alternative approach.

That was what I had assumed too, until I thought through the nasty interactions with partial Handshake flights and 0-RTT. That is, the TLS stack makes secrets available at the point that it would normally be installing them for its own use. In that way, a server wouldn't install the 1-RTT read keys until it had processed the finished from the client, exactly as you say.

My current patch (which is still under review) releases secrets as soon as possible to avoid the problem where the server can't read Handshake acknowledgements because it is receiving 0-RTT. The main consequence of that is that you need a separate API where the TLS stack tells the QUIC stack what it is willing to send and receive (well, I had a ton of fun fixing tests as well, but that's just because we were a little lazy with certain things in the past).

@davidben
Copy link

davidben commented Oct 25, 2018

Right, this issue originally came up as we were thinking through key availability vs key install times. The problem is, as you note, it's weird to allow sending data you cannot read ACKs for. (Conversely, it's weird, perhaps fatally so, to allow receiving data you cannot send ACKs for.) Though it's not the only scenario. The other side may send a burst of tickets.

I'm very much inclined to leave this "unfixed", but to leave a warning that sending large post-handshake messages risks connection failure.

The point is that's not a sufficient warning. This is either a problem or a nitpick depending on the solution.

Sending an overly large message, in general, risks connection failure. TLS stacks bound the size there based on some ad-hoc analysis that no sane NewSessionTicket (or other message) will ever exceed such-and-such bytes based on what goes in it. TLS stacks typically buffer up a message before processing, so it makes sense that the TLS stack is responsible for buffering it.

But that's distinct from sending many post-handshake messages. TLS over QUIC must also account for that due to the lack of flow control. TLS over TCP does not have this problem because TCP provides flow control. If I try to send a million 100-byte TLS messages over TCP, I'll quickly run into TCP's flow control.

QUIC, however, doesn't have such flow control for CRYPTO frames. Instead it relies on a limit from the crypto protocol:
https://tools.ietf.org/html/draft-ietf-quic-transport-15#section-10.4

Although the TLS stack natively bounds a single message rather than total data, it is easy to compute a total limit during the handshake because there are only a fixed handful of messages. Thus the advice there works.... except for post-handshake messages. Thus the issue.

Realistically, it's probably fine to handwaive and say:

  • The bound is not total data ever sent at the encryption level, but amount of data ahead of the read position.
  • Pick some ad-hoc limit for post-handshake stuff, enough to fit a bunch of NSTs.
  • Yeah, don't send too many NSTs in a row. You can send a bunch of them over the lifetime of the connection, but space them out a bit so you're sure the receiver isn't waiting on the first byte, having buffered the rest up.
  • Probably you won't end up in a situation where the first NST packet keeps getting lost and as you queue up more and more NSTs over the lifetime of the connection. That would be strange.

But it also smells weird, so perhaps proper flow control (toss post-hs stuff onto stream 0?) would be better instead?

@kazuho
Copy link
Member

kazuho commented Oct 26, 2018

I had assumed that TLS stacks always expose the 1-RTT read key to the QUIC stacks acting as a server when it receives ClientFinished. I'd be curious to know if there are TLS stacks that takes an alternative approach.

That was what I had assumed too, until I thought through the nasty interactions with partial Handshake flights and 0-RTT. That is, the TLS stack makes secrets available at the point that it would normally be installing them for its own use. In that way, a server wouldn't install the 1-RTT read keys until it had processed the finished from the client, exactly as you say.

Thank you for pointing that out. I forgot about that exception. So while that is the only exception picotls has in terms of key installation timing, I agree that it's merely about having one exception or two...

But it also smells weird, so perhaps proper flow control (toss post-hs stuff onto stream 0?) would be better instead?

I am totally fine with handwaving, but in case we are to have flow control, I'd prefer having flow control for CRYPTO streams in all epochs. They could all use a fixed-size window that would be defined in the specification. I prefer that because IMO having flow control for every stream (i.e. all the ordinary streams + all the 4 CRYPTO streams) is a simplification.

@siyengar
Copy link

siyengar commented Oct 30, 2018

FWIW we had a similar discussion about stream 0 flow control in #1252 and during the stream 0 design team discussions.

@mnot mnot added this to Crypto in Transport / Recovery / TLS Jan 10, 2019
@martinthomson
Copy link
Member

The problem here is that CRYPTO is not flow controlled. Endpoints can cause arbitrary buffering of CRYPTO frame data.

This has very little to do with the handshake (though @siyengar notes that we have a similar concern for CRYPTO in Initial and Handshake, that is bounded by the handshake timers).

Proposal is to allow endpoints to maintain a buffer of any size they choose. If they receive a TLS message that does not fit into that buffer, they can drop all subsequent TLS data. The effect of this is to disable TLS from that point onward. Currently, we don't depend on this frame for anything other that NewSessionTicket, for which loss has no real consequence.

We will assign a minimum buffer size that implementations are required to support, probably something small like 4k. A higher limit is possible. If an endpoint finds that this buffer is overrun they MAY discard all CRYPTO frames with higher offsets, or they MAY drop the connection with a NEW_ERROR code.

@martinthomson martinthomson self-assigned this Jan 30, 2019
@mnot
Copy link
Member

mnot commented Feb 8, 2019

In Tokyo:

MT: I am writing up this proposal and I’ll take to the token to drive this to conclusion.

@mnot mnot removed the yaec! label Feb 12, 2019
@mnot mnot added this to Design Issues in Late Stage Processing Feb 27, 2019
@janaiyengar janaiyengar added the proposal-ready An issue which has a proposal that is believed to be ready for a consensus call. label Apr 16, 2019
@janaiyengar janaiyengar moved this from Design Issues to Consensus Emerging in Late Stage Processing Apr 16, 2019
Late Stage Processing automation moved this from Consensus Emerging to Text Incorporated Apr 23, 2019
@mnot
Copy link
Member

mnot commented Apr 24, 2019

Please don't close issues until AFTER consensus is declared (not just called).

@mnot mnot reopened this Apr 24, 2019
Late Stage Processing automation moved this from Text Incorporated to Triage Apr 24, 2019
@mnot mnot moved this from Triage to Consensus Emerging in Late Stage Processing Apr 24, 2019
@mnot mnot added has-consensus An issue that the Chairs have determined has consensus, by canvassing the mailing list. and removed proposal-ready An issue which has a proposal that is believed to be ready for a consensus call. labels May 2, 2019
@mnot mnot moved this from Consensus Emerging to Consensus Declared in Late Stage Processing May 2, 2019
@mnot mnot moved this from Consensus Declared to Text Incorporated in Late Stage Processing May 7, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
-tls design An issue that affects the design of the protocol; resolution requires consensus. has-consensus An issue that the Chairs have determined has consensus, by canvassing the mailing list.
Projects
Late Stage Processing
  
Issue Handled
Development

Successfully merging a pull request may close this issue.

8 participants