I have nothing useful to add crypto-wise, but will note that "-Wrapper" is a pretty gross suffix.

Oh, yeah, for the record I don't like the name and have no attachment to it. Just couldn't come up with anything better.

Sealer/Seal/Unseal could be an alternative. It's very close to crypto.AEAD, not sure if it's a good thing.

Also: returning nil, nil from Wrap/Seal should just skip sending the ticket.

Also: the implementation might need to surface a need to re-encrypt the ticket (only meaningful in 1.2) and a ticket lifetime hint (only meaningful in 1.3). But not sure where to put them.

If it's of any interest looking at some code in the wild, here's how Caddy does key rotation currently: https://github.com/mholt/caddy/blob/06873175bf9141531e5de2e6b061f2ba94014edc/caddytls/crypto.go#L259

And here's how it's initialized: https://github.com/mholt/caddy/blob/06873175bf9141531e5de2e6b061f2ba94014edc/caddyhttp/httpserver/server.go#L256

/cc @agl

I definitely have a use case for this API so I'd like to offer my thoughts.

As for the name I agree that Wrapper/Wrap/Unwrap is an awful naming scheme. It also seems confusing, what does 'wrapping' actually mean in this context. It sounds more like you're asking for some sort of binary framing to be added. Sealer/Seal/Unseal is much better and also clearer as to the intended purpose - to cryptographically seal the opaque data. Perhaps Sealer/Seal/Open is best though, both to be more consistent with cipher.AEAD and because it seems more idiomatic.

I think the ticket lifetime (nit: it's not actually a hint any more in TLS 1.3 is it?) should be on the *tls.Config. This API should strictly concern itself with cryptographically sealing and unsealing of the opaque ticket data. If it's necessary to select a different lifetime for different clients (although I can't actually foresee a use case) that could be done easily with the existing GetConfigForClient hook.

I also understand there to be overlap between the owners of crypto/tls and BoringSSL, in which case it seems wise to keep things at least broadly consistent with the BoringSSL API (see https://boringssl-review.googlesource.com/14144 & https://boringssl-review.googlesource.com/14205).

I actually queried the inability to renew a ticket in that first change:

There isn't anyway to set |*out_renew_ticket| from the new |SSL_TICKET_AEAD_METHOD| API. This seems like an omission. Is there any rational for this?

and @davidben stated that this was very much intentional:

This is intentional. Ticket renewal in TLS 1.3 is implicit, and it is useless in TLS 1.2. As a server, one cannot safely deploy it because older SChannels will break. As a client, it must not extend the short session lifetime because the key does not change. (BoringSSL clamps the lifetime of a renewed ticket to the original session. I certainly hope other clients do the same.)

I'd argue very strongly that TLS 1.2 ticket renewal should not be included in this API at all. [Although, it should go in separate issue, I actually want to suggest that ticket renewal be removed from crypto/tls entirely, for the reasons above].

The one real problem I have with this proposal is the Clone method. It feels needlessly messy to me, I can imagine that almost every implementation would just end up doing:

func (s *Sealer) Clone() SessionTicketWrapper { return s }

Instead of a Clone method, perhaps the SessionTicketWrapperFixed and SessionTicketWrapperManual types could be special cased with type assertions to maintain both backward comparability and a clean API. Or perhaps those two types could be omitted and just be special cased internally to crypto/tls?

Actually, I was also slightly confused by the addition of *ConnectionState and *ClientHelloInfo to the API (the question @Bren2010 asked in #17432 (comment)). I'm still not entirely sure what the purpose of them is, although I can see some potential value in having them. (Barring some use case that Cloudflare has come across), I do think they can be removed without losing that functionality.

I'd like to propose a simpler API, just providing the following two methods:

type SessionTicketSealer interface {
    Seal(content []byte) (ticket []byte, err error)
    Open(ticket []byte) (content []byte, success bool)
}

I think it's nicer that the two methods are more symmetric as well.

Perhaps the success return value should be eliminated as well, just returning nil seems more sensible. It doesn't make any sense to return ([]byte, false), and returning (nil, true) would clearly be a bug.

That would just leave the following API, which seems cleaner:

type SessionTicketSealer interface {
    Seal(content []byte) (ticket []byte, err error)
    Open(ticket []byte) (content []byte)
}

If there was a need to act upon the *ClientHelloInfo, that could be done by switching the SessionTicketSealer in the GetConfigForClient hook, which provides a *ClientHelloInfo.

Hell, you could even do something like this if you really wanted to:

type sealer struct { chi *tls.ClientHelloInfo }

func main() {
    // ...
    config.GetConfigForClient = func(chi *tls.ClientHelloInfo) (*tls.Config, error) {
        config := config.Clone()
        config.SessionTicketSealer = &sealer{chi}
        return config, nil
    }
    // ...
}

func (s *sealer) Seal(content []byte) (ticket []byte, err error) {
    // make decisions based upon s.chi here
    return ...
}

func (s *sealer) Open(ticket []byte) (content []byte, success bool) {
    // make decisions based upon s.chi here
    return ...
}

With the advent of TLS 1.3, stateful storage of Session IDs and encryption of session tickets are merging. We should try to make a common API out of both, so similarly to #25228, TBD in 1.3.

Change https://go.dev/cl/496821 mentions this issue: crypto/tls: add WrapSession and UnwrapSession