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.

Sign up for GitHub

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

Jump to bottom

crypto/tls: QUICConn.SendSessionTicket should check Config.SessionTicketsDisabled #62032

Open
marten-seemann opened this issue Aug 15, 2023 · 2 comments
Open

crypto/tls: QUICConn.SendSessionTicket should check Config.SessionTicketsDisabled #62032

marten-seemann opened this issue Aug 15, 2023 · 2 comments
Labels
NeedsFix The path to resolution is known, but the work has not been done.
Milestone
Backlog

Comments

@marten-seemann
Copy link
Contributor

What version of Go are you using (go version)?

$ go version
go version go1.21.0 darwin/arm64

Does this issue reproduce with the latest release?

Yes

What did you do?

When QUICConn.SendSessionTicket is called even though Config.SessionTicketsDisabled is set, an error is returned ("session ticket keys unavailable").

Now one could argue that the QUIC stack do that check before calling SendSessionTicket (and that's what quic-go does), however, the QUIC stack won't have access to the tls.Config used on the connection if it is a config returned by GetConfigForClient (unless the QUIC stack wraps that callback and keeps track of the invocations, which would be quite a bit of complexity).

What did you expect to see?

Instead, it would be easy for crypto/tls to check the Config.SessionTicketsDisabled flag on the config that's in use on the connection.

API-wise, there are two options here:

  1. return nil
  2. return a sentinel error

No strong preference in either direction.

cc @neild @FiloSottile
This was referenced Aug 15, 2023
Closed
Merged
@neild
Copy link
Contributor

neild commented Aug 15, 2023

Seems reasonable to me. The TLS layer should be checking SessionTicketsDisabled, since it owns that configuration, and the QUIC layer shouldn't be left holding an uninterpretable error when tickets are disabled.

I think its safe at the moment for the QUIC layer to just ignore all errors from SendSessionTicket, but it's reasonable to want a way to identify the expected error case.

I don't have a strong opinion on whether SendSessionTicket should return nil or a distinguishable error when tickets are disabled. nil is less API surface, but doesn't give the QUIC layer any way to identify the case where a ticket wasn't sent. I don't know why the QUIC layer would need to care, though; I presume all it's going to do is ignore the error, so maybe nil is simple and sufficient.

@dmitshur dmitshur added the NeedsFix The path to resolution is known, but the work has not been done. label Aug 15, 2023
@dmitshur dmitshur added this to the Backlog milestone Aug 15, 2023
@gopherbot
Copy link

Change https://go.dev/cl/528755 mentions this issue: crypto/tls: check if quic conn can send session ticket

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Milestone
Backlog
Development

No branches or pull requests
5 participants
@neild @marten-seemann @dmitshur @gopherbot and others