-
Notifications
You must be signed in to change notification settings - Fork 17.8k
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
crypto/tls: advanced sessions APIs #60105
Comments
What's the motivation for allowing WrapSession to do its own encryption of the session ticket? Users may need to add additional data into the session ticket, but when would they need to encrypt it? This seems like it'd be safer if users could set SessionState.Extra but the server always handled encoding and encrypting the state into a ticket.
A map is convenient for the user here, but is an allocation-heavy way of providing extra data to include in the ticket. I'm also a bit dubious about serialization order of map entries; I can't think of any concrete issues at the moment that might be caused by the serialization of this section being unstable, but it doesn't feel right. String keys are also an inefficient way of encoding data on the wire. What about making this a plain []byte? It's slightly less convenient for the user if they want to encode a more complex structure, but avoids all these issues. If the user doesn't care about perfect efficiency, they can gob-encode a map.
Is the idea that persistent storage of a session will retrieve a session with something like this?
If so, "AddSession" is a bit of an odd name; "add" sounds like something I can call multiple times. Also, I wonder if this should be a constructor returning a *ClientSessionState:
What's an identity? Why isn't it a field of |
That way the application can implement "session IDs" which are just a legacy name for "I'm not encrypting this, I am putting it in a database and giving you back its lookup ID". They are especially useful when doing 0-RTT because you can prevent replays by removing the entry from the db the first time it's retrieved.
Oh I should have documented the intention here. The idea was that a wrapper implementation would put its data under its identifier, so that you could nest wrappers without conflicting. But maybe that's overthinking this, and it would be just fine to say "if Extra is not nil, you need a way to preserve it across a round-trip". Let's do that.
You can call it multiple times if doing PSK because each PSK only works with a certain hash and protocol version. Also, you can just try multiple PSKs if for some reason you have multiple, maybe if you're rotating them. The ordinary case for resumption will have only one though, so maybe the constructor is a useful helper?
The identity is the opaque bytes the client sends to the server to make it understand what session/PSK it's resuming/using. It can be an encrypted ticket, a session ID, or just a name. It's logically not part of the session state, it's the "name" of the session state, and only the client stores it, while we use |
This proposal has been added to the active column of the proposals project |
This is a lot of new API, but my understanding is that @FiloSottile, @neild, and @marten-seemann are all in favor of it. Have all remaining concerns been addressed? |
There are some TODOs in the proposed API which need to be resolved. I'm guessing that if When resuming a QUIC session, we want to verify that the client's remembered transport parameters match the server's current transport parameters. The server would do that by putting the parameters (or a hash of the parameters) in Is there a way to determine which |
Change https://go.dev/cl/496820 mentions this issue: |
Change https://go.dev/cl/496817 mentions this issue: |
Change https://go.dev/cl/496819 mentions this issue: |
Change https://go.dev/cl/496822 mentions this issue: |
Change https://go.dev/cl/496821 mentions this issue: |
Change https://go.dev/cl/496818 mentions this issue: |
Mailed the CL stack, including the changes suggested above. (The last could two are lacking tests, but I'll add them today or tomorrow.) In the interest of limiting the complexity and API surface added late in the cycle, I didn't implement external PSKs and TLS 1.2 Session IDs. We know the API can be extended to support them, and there's no rush in doing so. I had to change the WrapSession and UnwrapSession Config method names to EncryptTicket and DecryptTicket since they would clash with the Config fields.
The TODO about forcing PSK use is only relevant for external PSKs, since for resumption you can always fallback to a full handshake, but I expect the answer will be "VerifyConnection aborts if DidResume is false". The TODO about the ClientSessionCache key I think doesn't actually matter: simple clients don't do earlyData, and QUIC stacks will know the cache key better than crypto/tls and probably ignore it and make ClientSessionCache a couple of closures.
No, I think we should do lice GetConfigForClient: non-nil error aborts, (nil, nil) discards and continues. I am not a fan of errors getting dropped on the floor.
Correct!
Good question. I exposed SessionState.Extra as ConnectionState.Session. Does that work? |
Extremely minor nit, but for completeness sake you should probably include NewResumptionState in the API diff above. |
Here's the updated API diff. Note that I used ResumptionState instead of ResumptionSession, to rhyme with ClientSessionState and SessionState.
|
I can confirm that this API and the implementation work for the QUIC use case:
My quic-go branch also contains tests to make sure this works recursively: The application using quic-go can use the |
We talked with @marten-seemann (see also here), and the late ConnectionState.Session addition is not working as well as intended: a middleware like quic-go can't strip its additional data from it, clashing with applications that expect to use it like in crypto/tls. The good news is that we don't need it for Go 1.21. It's only needed for #46718 and as @neild pointed out to disambiguate between multiple tickets, but currently crypto/tls only supports sending one ticket per connection (and even on the server where it has to accept multiple, only the first can be used for 0-RTT by spec). I suggest we go ahead with this downscoped API (#60105 (comment) without ConnectionState.Session) for Go 1.21, which is sufficient for quic-go, and leave the rest for later. |
Based on the discussion above, this proposal seems like a likely accept. |
Ever since session ticket key rotation was introduced in CL 9072, we've been including a prefix in every ticket to identify what key it's encrypted with. It's a small privacy gain, but the cost of trial decryptions is also small, especially since the first key is probably the most frequently used. Also reissue tickets on every resumption so that the next connection can't be linked to all the previous ones. Again the privacy gain is small but the performance cost is small and it comes with a reduction in complexity. For #60105 Change-Id: I852f297162d2b79a3d9bf61f6171e8ce94b2537a Reviewed-on: https://go-review.googlesource.com/c/go/+/496817 Reviewed-by: Damien Neil <[email protected]> Reviewed-by: Matthew Dempsky <[email protected]> Run-TryBot: Damien Neil <[email protected]> TryBot-Result: Gopher Robot <[email protected]>
This change by itself is useless, because the application has no way to access or provide SessionStates to crypto/tls, but they will be provided in following CLs. For #60105 Change-Id: I8d5de79b1eda0a778420134cf6f346246a1bb296 Reviewed-on: https://go-review.googlesource.com/c/go/+/496818 Reviewed-by: Marten Seemann <[email protected]> TryBot-Result: Gopher Robot <[email protected]> Reviewed-by: Damien Neil <[email protected]> Reviewed-by: Matthew Dempsky <[email protected]> Run-TryBot: Filippo Valsorda <[email protected]>
Another internal change, that allows exposing the new APIs easily in following CLs. For #60105 Change-Id: I9c61b9f6e9d29af633f952444f514bcbbe82fe4e Reviewed-on: https://go-review.googlesource.com/c/go/+/496819 Reviewed-by: Matthew Dempsky <[email protected]> TryBot-Result: Gopher Robot <[email protected]> Reviewed-by: Damien Neil <[email protected]> Run-TryBot: Filippo Valsorda <[email protected]>
…tate For #60105 Fixes #25351 Change-Id: Iffd658f2663cfc47b48157824226ed6c0260a59e Reviewed-on: https://go-review.googlesource.com/c/go/+/496820 TryBot-Result: Gopher Robot <[email protected]> Reviewed-by: Damien Neil <[email protected]> Reviewed-by: Matthew Dempsky <[email protected]> Run-TryBot: Filippo Valsorda <[email protected]> Reviewed-by: Marten Seemann <[email protected]>
There was a bug in TestResumption: the first ExpiredSessionTicket was inserting a ticket far in the future, so the second ExpiredSessionTicket wasn't actually supposed to fail. However, there was a bug in checkForResumption->sendSessionTicket, too: if a session was not resumed because it was too old, its createdAt was still persisted in the next ticket. The two bugs used to cancel each other out. For #60105 Fixes #19199 Change-Id: Ic9b2aab943dcbf0de62b8758a6195319dc286e2f Reviewed-on: https://go-review.googlesource.com/c/go/+/496821 Run-TryBot: Filippo Valsorda <[email protected]> TryBot-Result: Gopher Robot <[email protected]> Reviewed-by: Roland Shoemaker <[email protected]> Reviewed-by: Damien Neil <[email protected]>
This can be used by applications to store additional data in a session. Fixes #57753 For #60105 Change-Id: Ib42387ad64750fa8dbbdf51de5e9c86378bef0ee Reviewed-on: https://go-review.googlesource.com/c/go/+/496822 Run-TryBot: Filippo Valsorda <[email protected]> Reviewed-by: Marten Seemann <[email protected]> Reviewed-by: Damien Neil <[email protected]> TryBot-Result: Gopher Robot <[email protected]> Reviewed-by: Matthew Dempsky <[email protected]>
After playing around with this API in quic-go for the last week, and writing some tests where the application stores random data und one that uses session IDs, here's what I've found: First of all, it works: The QUIC stack can store the QUIC transport parameters (and other connection parameters, such as the RTT) in the session ticket. That's great news! I'd like to propose a small change to the API though: However, the QUIC stack might not be the only one that's interested in using that slice to store additional information. For example, the HTTP/3 layer also needs to store certain values in the session ticket (see Section 7.2.4.2 of RFC 9114). Other implementations running on top of QUIC will probably want to do the same, and there could be multiple layers that want to do that. This is pretty inconvenient, since at the time the What if we made it more explicit that this is a stack that every layer in the application can push to (on type SessionState struct {
// not exported
// PushExtra would append to this slice.
// PopExtra would pop the last element of this slice.
extra [][]byte
}
// PushExtra adds some extra information to the session state.
// This information is ignored by crypto/tls, but is encoded by [SessionState.Bytes]
// and parsed by [ParseSessionState].
//
// This allows [Config.UnwrapSession]/[Config.WrapSession] and
// [ClientSessionCache] wrappers to store and retrieve additional data.
func (SessionState) PushExtra([]byte)
// PopExtra removes extra information from the session state.
func (SessionState) PopExtra() (_ []byte, ok bool) Any layer of the application (incl. the QUIC stack) would call |
Going to move this to accepted, and filed a separate issue for @marten-seemann's latest comment, #60539. |
No change in consensus, so accepted. 🎉 |
Change https://go.dev/cl/501303 mentions this issue: |
Change https://go.dev/cl/501675 mentions this issue: |
For #60105 For #44886 Change-Id: I8f6cfc4490535979ee8c0d8381c03b03c9c7b9a0 Reviewed-on: https://go-review.googlesource.com/c/go/+/501303 TryBot-Result: Gopher Robot <[email protected]> Reviewed-by: Ian Lance Taylor <[email protected]> Reviewed-by: Roland Shoemaker <[email protected]> Run-TryBot: Damien Neil <[email protected]>
Fixes #60539 Updates #60105 Change-Id: I7b567cc1d0901891ed97d29591db935cd487cc71 Reviewed-on: https://go-review.googlesource.com/c/go/+/501675 Auto-Submit: Filippo Valsorda <[email protected]> Run-TryBot: Filippo Valsorda <[email protected]> Reviewed-by: Roland Shoemaker <[email protected]> TryBot-Result: Gopher Robot <[email protected]> Reviewed-by: Damien Neil <[email protected]>
@FiloSottile can we disable this in golang 1.21 with some flag ? This causes an issue with large threads |
Currently, we support TLS 1.2 and TLS 1.3 session resumption with very little flexibility: applications can only store sessions in memory on the client side, and can only provide session ticket encryption keys on the server side.
This has a few limitations: we don't support external PSKs, we don't support session IDs on the server side, we don't support storing sessions offline on the client side. Moreover, complex applications like a QUIC stack need extra control over session storage to implement features like 0-RTT.
We propose an advanced sessions API that addresses all of the above.
Although we don't implement the spec itself, this API enables an implementation of RFC 9258 PSK importers either externally or in the future in the standard library. On the client side
ClientSessionState.AddSession
is called for each ipskx (one for each KDF supported by the supported cipher suites), and on the server side the identity is parsed inConfig.UnwrapSession
and the correct ipskx is derived and returned. We'll need to add aImported
bool toSessionState
to request the use of theimp binder
derivation path. (Should we add that already even if we don't add RFC 9258 helpers yet? Should we add RFC 9258 helpers?)This was designed with @marten-seemann, although note that I changed
ClientSessionState
: instead of exposing a single session as fields, I added methods so that multiple PSKs can be provisioned for use with different hashes./cc @golang/security @golang/proposal-review @davidben (if he has opinions)
The text was updated successfully, but these errors were encountered: