]> Cypherpunks repositories - gostls13.git/commitdiff
crypto/tls: rotate session ticket keys
authorKatie Hockman <katie@golang.org>
Tue, 28 Apr 2020 21:47:27 +0000 (17:47 -0400)
committerKatie Hockman <katie@golang.org>
Fri, 8 May 2020 02:00:35 +0000 (02:00 +0000)
Automatically rotate session ticket keys for servers
that don't already have sessionTicketKeys and that
haven't called SetSessionTicketKeys.

Now, session ticket keys will be rotated every 24 hours
with a lifetime of 7 days. This adds a small performance
cost to existing clients that don't provide a session
ticket encrypted with a fresh enough session ticket key,
which would require a full handshake.

Updates #25256

Change-Id: I15b46af7a82aab9a108bceb706bbf66243a1510f
Reviewed-on: https://go-review.googlesource.com/c/go/+/230679
Run-TryBot: Katie Hockman <katie@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Filippo Valsorda <filippo@golang.org>
src/crypto/tls/common.go
src/crypto/tls/conn.go
src/crypto/tls/handshake_client_test.go
src/crypto/tls/handshake_server.go
src/crypto/tls/handshake_server_test.go
src/crypto/tls/ticket.go

index 9bd7005fc1ab2d4fca44ef4d980213df2a20ad86..90846a3659ceaefe5c77069ad55bdb8d2e505167 100644 (file)
@@ -501,15 +501,10 @@ type Config struct {
        // If GetConfigForClient is nil, the Config passed to Server() will be
        // used for all connections.
        //
-       // Uniquely for the fields in the returned Config, session ticket keys
-       // will be duplicated from the original Config if not set.
-       // Specifically, if SetSessionTicketKeys was called on the original
-       // config but not on the returned config then the ticket keys from the
-       // original config will be copied into the new config before use.
-       // Otherwise, if SessionTicketKey was set in the original config but
-       // not in the returned config then it will be copied into the returned
-       // config before use. If neither of those cases applies then the key
-       // material from the returned config will be used for session tickets.
+       // If SessionTicketKey was explicitly set on the returned Config, or if
+       // SetSessionTicketKeys was called on the returned Config, those keys will
+       // be used. Otherwise, the original Config keys will be used (and possibly
+       // rotated if they are automatically managed).
        GetConfigForClient func(*ClientHelloInfo) (*Config, error)
 
        // VerifyPeerCertificate, if not nil, is called after normal
@@ -579,10 +574,10 @@ type Config struct {
        // See RFC 5077 and the PSK mode of RFC 8446. If zero, it will be filled
        // with random data before the first server handshake.
        //
-       // If multiple servers are terminating connections for the same host
-       // they should all have the same SessionTicketKey. If the
-       // SessionTicketKey leaks, previously recorded and future TLS
-       // connections using that key might be compromised.
+       // Deprecated: if this field is left at zero, session ticket keys will be
+       // automatically rotated every day and dropped after seven days. For
+       // customizing the rotation schedule or synchronizing servers that are
+       // terminating connections for the same host, use SetSessionTicketKeys.
        SessionTicketKey [32]byte
 
        // ClientSessionCache is a cache of ClientSessionState entries for TLS
@@ -622,20 +617,32 @@ type Config struct {
        // used for debugging.
        KeyLogWriter io.Writer
 
-       serverInitOnce sync.Once // guards calling (*Config).serverInit
-
-       // mutex protects sessionTicketKeys.
+       // mutex protects sessionTicketKeys and autoSessionTicketKeys.
        mutex sync.RWMutex
-       // sessionTicketKeys contains zero or more ticket keys. If the length
-       // is zero, SessionTicketsDisabled must be true. The first key is used
-       // for new tickets and any subsequent keys can be used to decrypt old
-       // tickets.
+       // sessionTicketKeys contains zero or more ticket keys. If set, it means the
+       // the keys were set with SessionTicketKey or SetSessionTicketKeys. The
+       // first key is used for new tickets and any subsequent keys can be used to
+       // decrypt old tickets. The slice contents are not protected by the mutex
+       // and are immutable.
        sessionTicketKeys []ticketKey
+       // autoSessionTicketKeys is like sessionTicketKeys but is owned by the
+       // auto-rotation logic. See Config.ticketKeys.
+       autoSessionTicketKeys []ticketKey
 }
 
-// ticketKeyNameLen is the number of bytes of identifier that is prepended to
-// an encrypted session ticket in order to identify the key used to encrypt it.
-const ticketKeyNameLen = 16
+const (
+       // ticketKeyNameLen is the number of bytes of identifier that is prepended to
+       // an encrypted session ticket in order to identify the key used to encrypt it.
+       ticketKeyNameLen = 16
+
+       // ticketKeyLifetime is how long a ticket key remains valid and can be used to
+       // resume a client connection.
+       ticketKeyLifetime = 7 * 24 * time.Hour // 7 days
+
+       // ticketKeyRotation is how often the server should rotate the session ticket key
+       // that is used for new tickets.
+       ticketKeyRotation = 24 * time.Hour
+)
 
 // ticketKey is the internal representation of a session ticket key.
 type ticketKey struct {
@@ -644,16 +651,19 @@ type ticketKey struct {
        keyName [ticketKeyNameLen]byte
        aesKey  [16]byte
        hmacKey [16]byte
+       // created is the time at which this ticket key was created. See Config.ticketKeys.
+       created time.Time
 }
 
 // ticketKeyFromBytes converts from the external representation of a session
 // ticket key to a ticketKey. Externally, session ticket keys are 32 random
 // bytes and this function expands that into sufficient name and key material.
-func ticketKeyFromBytes(b [32]byte) (key ticketKey) {
+func (c *Config) ticketKeyFromBytes(b [32]byte) (key ticketKey) {
        hashed := sha512.Sum512(b[:])
        copy(key.keyName[:], hashed[:ticketKeyNameLen])
        copy(key.aesKey[:], hashed[ticketKeyNameLen:ticketKeyNameLen+16])
        copy(key.hmacKey[:], hashed[ticketKeyNameLen+16:ticketKeyNameLen+32])
+       key.created = c.time()
        return key
 }
 
@@ -664,15 +674,8 @@ const maxSessionTicketLifetime = 7 * 24 * time.Hour
 // Clone returns a shallow clone of c. It is safe to clone a Config that is
 // being used concurrently by a TLS client or server.
 func (c *Config) Clone() *Config {
-       // Running serverInit ensures that it's safe to read
-       // SessionTicketsDisabled.
-       c.serverInitOnce.Do(func() { c.serverInit(nil) })
-
-       var sessionTicketKeys []ticketKey
        c.mutex.RLock()
-       sessionTicketKeys = c.sessionTicketKeys
-       c.mutex.RUnlock()
-
+       defer c.mutex.RUnlock()
        return &Config{
                Rand:                        c.Rand,
                Time:                        c.Time,
@@ -699,58 +702,122 @@ func (c *Config) Clone() *Config {
                DynamicRecordSizingDisabled: c.DynamicRecordSizingDisabled,
                Renegotiation:               c.Renegotiation,
                KeyLogWriter:                c.KeyLogWriter,
-               sessionTicketKeys:           sessionTicketKeys,
+               sessionTicketKeys:           c.sessionTicketKeys,
+               autoSessionTicketKeys:       c.autoSessionTicketKeys,
        }
 }
 
-// serverInit is run under c.serverInitOnce to do initialization of c. If c was
-// returned by a GetConfigForClient callback then the argument should be the
-// Config that was passed to Server, otherwise it should be nil.
-func (c *Config) serverInit(originalConfig *Config) {
-       if c.SessionTicketsDisabled || len(c.ticketKeys()) != 0 {
+// deprecatedSessionTicketKey is set as the prefix of SessionTicketKey if it was
+// randomized for backwards compatibility but is not in use.
+var deprecatedSessionTicketKey = []byte("DEPRECATED")
+
+// initLegacySessionTicketKeyRLocked ensures the legacy SessionTicketKey field is
+// randomized if empty, and that sessionTicketKeys is populated from it otherwise.
+func (c *Config) initLegacySessionTicketKeyRLocked() {
+       // Don't write if SessionTicketKey is already defined as our deprecated string,
+       // or if it is defined by the user but sessionTicketKeys is already set.
+       if c.SessionTicketKey != [32]byte{} &&
+               (bytes.HasPrefix(c.SessionTicketKey[:], deprecatedSessionTicketKey) || len(c.sessionTicketKeys) > 0) {
                return
        }
 
-       alreadySet := false
-       for _, b := range c.SessionTicketKey {
-               if b != 0 {
-                       alreadySet = true
-                       break
+       // We need to write some data, so get an exclusive lock and re-check any conditions.
+       c.mutex.RUnlock()
+       defer c.mutex.RLock()
+       c.mutex.Lock()
+       defer c.mutex.Unlock()
+       if c.SessionTicketKey == [32]byte{} {
+               if _, err := io.ReadFull(c.rand(), c.SessionTicketKey[:]); err != nil {
+                       panic(fmt.Sprintf("tls: unable to generate random session ticket key: %v", err))
                }
+               // Write the deprecated prefix at the beginning so we know we created
+               // it. This key with the DEPRECATED prefix isn't used as an actual
+               // session ticket key, and is only randomized in case the application
+               // reuses it for some reason.
+               copy(c.SessionTicketKey[:], deprecatedSessionTicketKey)
+       } else if !bytes.HasPrefix(c.SessionTicketKey[:], deprecatedSessionTicketKey) && len(c.sessionTicketKeys) == 0 {
+               c.sessionTicketKeys = []ticketKey{c.ticketKeyFromBytes(c.SessionTicketKey)}
        }
 
-       if !alreadySet {
-               if originalConfig != nil {
-                       copy(c.SessionTicketKey[:], originalConfig.SessionTicketKey[:])
-               } else if _, err := io.ReadFull(c.rand(), c.SessionTicketKey[:]); err != nil {
-                       c.SessionTicketsDisabled = true
-                       return
+}
+
+// ticketKeys returns the ticketKeys for this connection.
+// If configForClient has explicitly set keys, those will
+// be returned. Otherwise, the keys on c will be used and
+// may be rotated if auto-managed.
+// During rotation, any expired session ticket keys are deleted from
+// c.sessionTicketKeys. If the session ticket key that is currently
+// encrypting tickets (ie. the first ticketKey in c.sessionTicketKeys)
+// is not fresh, then a new session ticket key will be
+// created and prepended to c.sessionTicketKeys.
+func (c *Config) ticketKeys(configForClient *Config) []ticketKey {
+       // If the ConfigForClient callback returned a Config with explicitly set
+       // keys, use those, otherwise just use the original Config.
+       if configForClient != nil {
+               configForClient.mutex.RLock()
+               if configForClient.SessionTicketsDisabled {
+                       return nil
+               }
+               configForClient.initLegacySessionTicketKeyRLocked()
+               if len(configForClient.sessionTicketKeys) != 0 {
+                       ret := configForClient.sessionTicketKeys
+                       configForClient.mutex.RUnlock()
+                       return ret
                }
+               configForClient.mutex.RUnlock()
        }
 
-       if originalConfig != nil {
-               originalConfig.mutex.RLock()
-               c.sessionTicketKeys = originalConfig.sessionTicketKeys
-               originalConfig.mutex.RUnlock()
-       } else {
-               c.sessionTicketKeys = []ticketKey{ticketKeyFromBytes(c.SessionTicketKey)}
+       c.mutex.RLock()
+       defer c.mutex.RUnlock()
+       if c.SessionTicketsDisabled {
+               return nil
+       }
+       c.initLegacySessionTicketKeyRLocked()
+       if len(c.sessionTicketKeys) != 0 {
+               return c.sessionTicketKeys
+       }
+       // Fast path for the common case where the key is fresh enough.
+       if len(c.autoSessionTicketKeys) > 0 && c.time().Sub(c.autoSessionTicketKeys[0].created) < ticketKeyRotation {
+               return c.autoSessionTicketKeys
        }
-}
 
-func (c *Config) ticketKeys() []ticketKey {
-       c.mutex.RLock()
-       // c.sessionTicketKeys is constant once created. SetSessionTicketKeys
-       // will only update it by replacing it with a new value.
-       ret := c.sessionTicketKeys
+       // autoSessionTicketKeys are managed by auto-rotation.
        c.mutex.RUnlock()
-       return ret
+       defer c.mutex.RLock()
+       c.mutex.Lock()
+       defer c.mutex.Unlock()
+       // Re-check the condition in case it changed since obtaining the new lock.
+       if len(c.autoSessionTicketKeys) == 0 || c.time().Sub(c.autoSessionTicketKeys[0].created) >= ticketKeyRotation {
+               var newKey [32]byte
+               if _, err := io.ReadFull(c.rand(), newKey[:]); err != nil {
+                       panic(fmt.Sprintf("unable to generate random session ticket key: %v", err))
+               }
+               valid := make([]ticketKey, 0, len(c.autoSessionTicketKeys)+1)
+               valid = append(valid, c.ticketKeyFromBytes(newKey))
+               for _, k := range c.autoSessionTicketKeys {
+                       // While rotating the current key, also remove any expired ones.
+                       if c.time().Sub(k.created) < ticketKeyLifetime {
+                               valid = append(valid, k)
+                       }
+               }
+               c.autoSessionTicketKeys = valid
+       }
+       return c.autoSessionTicketKeys
 }
 
-// SetSessionTicketKeys updates the session ticket keys for a server. The first
-// key will be used when creating new tickets, while all keys can be used for
-// decrypting tickets. It is safe to call this function while the server is
-// running in order to rotate the session ticket keys. The function will panic
-// if keys is empty.
+// SetSessionTicketKeys updates the session ticket keys for a server.
+//
+// The first key will be used when creating new tickets, while all keys can be
+// used for decrypting tickets. It is safe to call this function while the
+// server is running in order to rotate the session ticket keys. The function
+// will panic if keys is empty.
+//
+// Calling this function will turn off automatic session ticket key rotation.
+//
+// If multiple servers are terminating connections for the same host they should
+// all have the same session ticket keys. If the session ticket keys leaks,
+// previously recorded and future TLS connections using those keys might be
+// compromised.
 func (c *Config) SetSessionTicketKeys(keys [][32]byte) {
        if len(keys) == 0 {
                panic("tls: keys must have at least one key")
@@ -758,7 +825,7 @@ func (c *Config) SetSessionTicketKeys(keys [][32]byte) {
 
        newKeys := make([]ticketKey, len(keys))
        for i, bytes := range keys {
-               newKeys[i] = ticketKeyFromBytes(bytes)
+               newKeys[i] = c.ticketKeyFromBytes(bytes)
        }
 
        c.mutex.Lock()
index bf2111cb97437ec0e69df18a6feb806ebcb2728b..d759986bb94a2c75c9791322908ba2c0d8b07f94 100644 (file)
@@ -62,6 +62,11 @@ type Conn struct {
        // NewSessionTicket messages. nil if config.SessionTicketsDisabled.
        resumptionSecret []byte
 
+       // ticketKeys is the set of active session ticket keys for this
+       // connection. The first one is used to encrypt new tickets and
+       // all are tried to decrypt tickets.
+       ticketKeys []ticketKey
+
        // clientFinishedIsFirst is true if the client sent the first Finished
        // message during the most recent handshake. This is recorded because
        // the first transmitted Finished message is the tls-unique
index f7c0a8e045ae8a9d614e72c9a92b7e1edc9addcc..cd387dcc6cb1c28766156b8c9a4344c637e4ee4f 100644 (file)
@@ -937,6 +937,21 @@ func testResumption(t *testing.T, version uint16) {
                t.Fatal("ticket didn't change after resumption")
        }
 
+       // An old session ticket can resume, but the server will provide a ticket encrypted with a fresh key.
+       serverConfig.Time = func() time.Time { return time.Now().Add(24*time.Hour + time.Minute) }
+       testResumeState("ResumeWithOldTicket", true)
+       if bytes.Equal(ticket[:ticketKeyNameLen], getTicket()[:ticketKeyNameLen]) {
+               t.Fatal("old first ticket matches the fresh one")
+       }
+
+       // Now the session tickey key is expired, so a full handshake should occur.
+       serverConfig.Time = func() time.Time { return time.Now().Add(24*8*time.Hour + time.Minute) }
+       testResumeState("ResumeWithExpiredTicket", false)
+       if bytes.Equal(ticket, getTicket()) {
+               t.Fatal("expired first ticket matches the fresh one")
+       }
+
+       serverConfig.Time = func() time.Time { return time.Now() } // reset the time back
        key1 := randomKey()
        serverConfig.SetSessionTicketKeys([][32]byte{key1})
 
index d227c043e619177fc87ab3e40cec9dc9a0ace113..4885c69568d439b8ac5d35e99f558df716ca0d1f 100644 (file)
@@ -37,10 +37,6 @@ type serverHandshakeState struct {
 
 // serverHandshake performs a TLS handshake as a server.
 func (c *Conn) serverHandshake() error {
-       // If this is the first server handshake, we generate a random key to
-       // encrypt the tickets with.
-       c.config.serverInitOnce.Do(func() { c.config.serverInit(nil) })
-
        clientHello, err := c.readClientHello()
        if err != nil {
                return err
@@ -143,16 +139,18 @@ func (c *Conn) readClientHello() (*clientHelloMsg, error) {
                return nil, unexpectedMessageError(clientHello, msg)
        }
 
+       var configForClient *Config
+       originalConfig := c.config
        if c.config.GetConfigForClient != nil {
                chi := clientHelloInfo(c, clientHello)
-               if newConfig, err := c.config.GetConfigForClient(chi); err != nil {
+               if configForClient, err = c.config.GetConfigForClient(chi); err != nil {
                        c.sendAlert(alertInternalError)
                        return nil, err
-               } else if newConfig != nil {
-                       newConfig.serverInitOnce.Do(func() { newConfig.serverInit(c.config) })
-                       c.config = newConfig
+               } else if configForClient != nil {
+                       c.config = configForClient
                }
        }
+       c.ticketKeys = originalConfig.ticketKeys(configForClient)
 
        clientVersions := clientHello.supportedVersions
        if len(clientHello.supportedVersions) == 0 {
index 61f0ca2bf7e26f26263b91db803adbb4b2376d13..a7a532431296f32714a96239f3c8167eeadabc21 100644 (file)
@@ -1496,12 +1496,8 @@ var getConfigForClientTests = []struct {
                },
                "",
                func(config *Config) error {
-                       // The value of SessionTicketKey should have been
-                       // duplicated into the per-connection Config.
-                       for i := range config.SessionTicketKey {
-                               if b := config.SessionTicketKey[i]; b != byte(i) {
-                                       return fmt.Errorf("SessionTicketKey was not duplicated from original Config: byte %d has value %d", i, b)
-                               }
+                       if config.SessionTicketKey == [32]byte{} {
+                               return fmt.Errorf("expected SessionTicketKey to be set")
                        }
                        return nil
                },
@@ -1522,10 +1518,8 @@ var getConfigForClientTests = []struct {
                },
                "",
                func(config *Config) error {
-                       // The session ticket keys should have been duplicated
-                       // into the per-connection Config.
-                       if l := len(config.sessionTicketKeys); l != 1 {
-                               return fmt.Errorf("got len(sessionTicketKeys) == %d, wanted 1", l)
+                       if config.SessionTicketKey == [32]byte{} {
+                               return fmt.Errorf("expected SessionTicketKey to be set")
                        }
                        return nil
                },
index 222333aec18a6f44001baeea50d0bca6d560915f..38b01fc25ca4f0860c8ae4508c5e2bcf4bf21eb2 100644 (file)
@@ -118,6 +118,10 @@ func (m *sessionStateTLS13) unmarshal(data []byte) bool {
 }
 
 func (c *Conn) encryptTicket(state []byte) ([]byte, error) {
+       if len(c.ticketKeys) == 0 {
+               return nil, errors.New("tls: internal error: session ticket keys unavailable")
+       }
+
        encrypted := make([]byte, ticketKeyNameLen+aes.BlockSize+len(state)+sha256.Size)
        keyName := encrypted[:ticketKeyNameLen]
        iv := encrypted[ticketKeyNameLen : ticketKeyNameLen+aes.BlockSize]
@@ -126,7 +130,7 @@ func (c *Conn) encryptTicket(state []byte) ([]byte, error) {
        if _, err := io.ReadFull(c.config.rand(), iv); err != nil {
                return nil, err
        }
-       key := c.config.ticketKeys()[0]
+       key := c.ticketKeys[0]
        copy(keyName, key.keyName[:])
        block, err := aes.NewCipher(key.aesKey[:])
        if err != nil {
@@ -151,19 +155,17 @@ func (c *Conn) decryptTicket(encrypted []byte) (plaintext []byte, usedOldKey boo
        macBytes := encrypted[len(encrypted)-sha256.Size:]
        ciphertext := encrypted[ticketKeyNameLen+aes.BlockSize : len(encrypted)-sha256.Size]
 
-       keys := c.config.ticketKeys()
        keyIndex := -1
-       for i, candidateKey := range keys {
+       for i, candidateKey := range c.ticketKeys {
                if bytes.Equal(keyName, candidateKey.keyName[:]) {
                        keyIndex = i
                        break
                }
        }
-
        if keyIndex == -1 {
                return nil, false
        }
-       key := &keys[keyIndex]
+       key := &c.ticketKeys[keyIndex]
 
        mac := hmac.New(sha256.New, key.hmacKey[:])
        mac.Write(encrypted[:len(encrypted)-sha256.Size])