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

proposal: crypto/tls: SessionTicketWrapper and Forward Secrecy by default #19199

Open
FiloSottile opened this issue Feb 20, 2017 · 8 comments

Comments

Projects
None yet
8 participants
@FiloSottile
Copy link
Member

commented Feb 20, 2017

By default a Go TLS server does not offer any Forward Secrecy in respect to the Session Ticket key, as that is fixed for the lifetime of the server. (And Session Tickets in 1.2 are awful: sent before the ChangeCipherSpec on every connection, with the ephemeral key material of the current connection.) Moreover, there is no way to abstract away the keys a-la-crypto.Signer. Finally, there are already too many ways to specify keys. This proposal tries to address all these concerns.

The main interface would be SessionTicketWrapper:

// A SessionTicketWrapper provides a way to securely incapsulate
// session state for storage on the client. All methods are safe for
// concurrent use.
type SessionTicketWrapper interface {
    // Wrap returns a session ticket value that can be later passed to Unwrap
    // to recover the content, usually by encrypting it. The ticket will be sent
    // to the client to be stored, and will be sent back in plaintext, so it can
    // be read and modified by an attacker.
    Wrap(cs *ConnectionState, content []byte) (ticket []byte, err error)

    // Unwrap returns a session ticket contents. The ticket can't be safely
    // assumed to have been generated by Wrap. 
    // If unable to unwrap the ticket, the connection will proceed with a
    // complete handshake.
    Unwrap(chi *ClientHelloInfo, ticket []byte) (content []byte, success bool)

    // Clone returns a new SessionTicketWrapper to be used in a
    // new config. It can be either a copy or a reference to the
    // existing one.
    Clone() SessionTicketWrapper
}

A simple tls.Config attribute would be the main hook.

type Config struct {
    // SessionTicketWrapper, if not nil, is used to wrap and unwrap
    // session tickets, instead of SessionTicketKey.
    SessionTicketWrapper SessionTicketWrapper
}

Three standard implementations would be offered, providing behind the scenes all current behaviors and the new default.

// SessionTicketWrapperFixed provides a SessionTicketWrapper using
// a fixed encryption key.
type SessionTicketWrapperFixed struct {}

SessionTicketWrapperFixed is instantiated under the hood when Config.SessionTicketKey is set.

// SessionTicketWrapperManual provides a SessionTicketWrapper using
// a set of encryptions keys. The first one is used to encrypt new tickets,
// and the others are tried to decrypt existing ones. Keys are rotated manually
// by calling the SetSessionTicketKeys method.
type SessionTicketWrapperManual struct {}

SessionTicketWrapperManual is instantiated under the hood when Config.SetSessionTicketKeys is called.

// SessionTicketWrapperRotating provides a SessionTicketWrapper using
// a set of random encryption keys that are rotated automatically.
// The latest key is used to encrypt new tickets and older ones are tried
// for decryption of existing ones. Connections gain forward secrecy and
// tickets stop working after RotateEvery * MaxKeys elapses.
type SessionTicketWrapperRotating struct {
    // RotateEvery is the period after which a new key is generated.
    // If zero, it defaults to 3 hours.
    RotateEvery time.Duration
    // MaxKeys is the maximum number of keys that will be kept in
    // memory to attempt decryption. If zero, it defaults to 8.
    MaxKeys int
}

SessionTicketWrapperRotating would be the default for a server that does not set any property, making the default have a 24-hours FS/resumption cliff.

On Clone and GetConfigForClient, SessionTicketWrapper.Clone is called. The Fixed and Manual implementations make deep copies, so that operations on the new ones don't affect the original, maintaining current behavior.

By very strict reading, there might be a Go 1 violation: a server that starts without SessionTicketKey, does a handshake, then copies the SessionTicketKey expecting it to be the fixed key will fail. IMHO it's not a significant breakage and worth it for FS.

/cc @agl

Supersedes #17432

@bradfitz

This comment has been minimized.

Copy link
Member

commented Feb 20, 2017

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

@FiloSottile

This comment has been minimized.

Copy link
Member Author

commented Feb 21, 2017

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.

@FiloSottile

This comment has been minimized.

Copy link
Member Author

commented Feb 22, 2017

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.

@mholt

This comment has been minimized.

Copy link

commented Feb 23, 2017

@rsc

This comment has been minimized.

Copy link
Contributor

commented Feb 27, 2017

/cc @agl

@gopherbot gopherbot added this to the Proposal milestone Mar 20, 2017

@gopherbot gopherbot added the Proposal label Mar 20, 2017

@rsc rsc changed the title proposal: crypto/tls.SessionTicketWrapper and Forward Secrecy by default proposal: crypto/tls: SessionTicketWrapper and Forward Secrecy by default Apr 3, 2017

@tmthrgd

This comment has been minimized.

Copy link
Contributor

commented Apr 15, 2017

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?

@tmthrgd

This comment has been minimized.

Copy link
Contributor

commented Apr 15, 2017

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 ...
}
@FiloSottile

This comment has been minimized.

Copy link
Member Author

commented May 4, 2018

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.