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

Move crypto to stream 0 #247

Closed
wants to merge 5 commits into from
Closed

Move crypto to stream 0 #247

wants to merge 5 commits into from

Conversation

MikeBishop
Copy link
Contributor

In #246, we agreed that the crypto stream and only the crypto stream would be exempt from connection-level flow control. It's also exempt from the congestion controller and has special rules about retransmissions. Is there a logical reason to have stream-level flow control on a stream that is a control channel?

If not, let's make it clear that the crypto stream is special. Make it Stream 0, the stream that can't be given any stream-level flow control credit because it's a reserved value. The change is relatively modest, and seems like a good fit to our special snowflake stream.

@martinthomson
Copy link
Member

The big change here is that stream-level flow control doesn't apply to TLS - it becomes impossible to give stream 0 stream-level credit. Even with the exemption from the connection-level flow control, stream 0 was previously still subject to stream-level flow control.

The change is safe because stream 0 carries mostly cleartext data and WINDOW_UPDATE was previously treated with some suspicion anyway. I'd like to confirm that this is acceptable with others before we merge this.

@ianswett
Copy link
Contributor

To clarify, "this" is exempting stream 0 from stream level flow control as well? If so, I think so, but I'd like to hear if others have concerns.

That conveniently still allows, stream 0 to be used in the WINDOW_UPDATE frame to indicate the connection's flow control window.

@marten-seemann
Copy link
Contributor

If stream 0 isn't controlled by stream-level flow control, this opens up a DoS. A peer introduce a gap in stream data and then just send arbitrary data with a higher offset. The receiver can't even verify if this data is a valid TLS message, because the gap would prevent reading from that stream.

@ianswett
Copy link
Contributor

In reality, I don't think it's a DoS, because the receiver has an expected max size for a handshake message. A CHLO is always 1 packet and a HRR(aka reject) is multiple. If one side receives data way outside the expected range, they can just terminate the handshake. In the case of the server, if you're only doing stateless rejects, all CHLO's better have a stream offset of 0.

Also, if the argument is that control messages(ie: public resets, flow control updates, etc) can be processed immediately, and that's why they don't count towards flow control, then I think the same argument applies for the crypto stream, modulo any concerns about the size of a reject/etc.

@marten-seemann
Copy link
Contributor

What about data received after the handshake? Terminating the connection if data beyond a certain offset is received sounds a lot like an implicit flow control window, just without the option to increase it.

@ianswett
Copy link
Contributor

I can't think of anything the client sends post-handshake, at least in QUIC crypto and TLS.

I'd like to think about it as imposing some sort of validity constraints on the crypto stream which are specific to the crypto being done. ie: As long as you're doing the crypto the peer expects, then all should go well. If you deviate, then given how important crypto is to QUIC, the connection is killed.

But tell me if I'm crazy.

@marten-seemann
Copy link
Contributor

@ianswett: Using TLS, there's the session ticket, but with reasonable limits this will probably not be a problem (I'm not sure if that's true for extremely long-lived connections though).
I was thinking about other handshake protocols that might have different requirements. But maybe I'm taking the generalization too far.

@martinthomson
Copy link
Member

If we look at the crypto handshake in the abstract, the only two functions that are needed are tickets and source address tokens. Both of these have to be less than 1280 octets. If we went the way that this PR suggests, I'd be happy to write advice about how much of that to accept. Maybe even require that a server not produce more than a small multiple of this limit without receiving any acknowledgment. That would allow a client to police this.

The other thing that is somewhat unbounded is client certificates. That too is probably manageable on the same basis.

@mcmanus
Copy link
Contributor

mcmanus commented Jan 30, 2017

I was going to mention client certs too - but I think I would rather have this PR and manage it heuristically than worry about the complications of flow control on a control stream. I favor merging.

@janaiyengar
Copy link
Contributor

I don't think you're simplifying things by removing flow control on the crypto stream. Is there a reason, besides textual simplicity, for doing this? I don't think it makes implementation any easier, but may make it harder, since there's basically no way for the implementation to send more than X bytes, and the receiver must enforce this, separate from any stream-level flow control logic (since that logic will be tied to sending WINDOW_UPDATE frames.) We'll also need to specify a required min window offset that MUST be supported on the crypto stream. This seems like a largely unnecessary limit and special case.

I'm not sure I see what real benefit it brings... I'm inclined towards keeping this as Stream 1 with stream-level flow control applied to it.

@ianswett
Copy link
Contributor

ianswett commented Feb 2, 2017

I'm now leaning towards the fact this is another special case, and special cases are bad unless there's a good reason for them.

In normal operation, we expect this to not matter, and Marten made some fine arguments about how lack of flow control could cause issues without some careful implementation choices, so I lean towards keeping stream flow control for the crypto stream. As such, I now also prefer it stay on stream 1, otherwise the window update frame has to change.

@martinthomson
Copy link
Member

I think that we are leaning towards not accepting this change.

@mnot, @larseggert, am I correct here? and how should we proceed now?

@mnot
Copy link
Member

mnot commented Feb 8, 2017

@MikeBishop any thoughts / change of mind?

@MikeBishop
Copy link
Contributor Author

If stream flow control is staying, then of course it's not stream zero. There's also #248, which questions the exemption from congestion control. If we're going the route of saying that the crypto stream is still a regular stream (with all the controls that entails), then stream 1 makes the most sense.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants