Repository: coreos/dex
Pull request #830 (2017-03-01 20:47:58)

server: fix expiry detection for verification keys
Cherry pick of https://github.com/coreos/dex/pull/829

cc @rithujohn191 


Patch:
diff --git a/server/rotation.go b/server/rotation.go
index 8516e9bd..fb790c62 100644
--- a/server/rotation.go
+++ b/server/rotation.go
@@ -22,8 +22,9 @@ type rotationStrategy struct {
 	// Time between rotations.
 	rotationFrequency time.Duration
 
-	// After being rotated how long can a key validate signatues?
-	verifyFor time.Duration
+	// After being rotated how long should the key be kept around for validating
+	// signatues?
+	idTokenValidFor time.Duration
 
 	// Keys are always RSA keys. Though cryptopasta recommends ECDSA keys, not every
 	// client may support these (e.g. github.com/coreos/go-oidc/oidc).
@@ -35,17 +36,17 @@ func staticRotationStrategy(key *rsa.PrivateKey) rotationStrategy {
 	return rotationStrategy{
 		// Setting these values to 100 years is easier than having a flag indicating no rotation.
 		rotationFrequency: time.Hour * 8760 * 100,
-		verifyFor:         time.Hour * 8760 * 100,
+		idTokenValidFor:   time.Hour * 8760 * 100,
 		key:               func() (*rsa.PrivateKey, error) { return key, nil },
 	}
 }
 
 // defaultRotationStrategy returns a strategy which rotates keys every provided period,
 // holding onto the public parts for some specified amount of time.
-func defaultRotationStrategy(rotationFrequency, verifyFor time.Duration) rotationStrategy {
+func defaultRotationStrategy(rotationFrequency, idTokenValidFor time.Duration) rotationStrategy {
 	return rotationStrategy{
 		rotationFrequency: rotationFrequency,
-		verifyFor:         verifyFor,
+		idTokenValidFor:   idTokenValidFor,
 		key: func() (*rsa.PrivateKey, error) {
 			return rsa.GenerateKey(rand.Reader, 2048)
 		},
@@ -128,11 +129,14 @@ func (k keyRotater) rotate() error {
 			return storage.Keys{}, errors.New("keys already rotated")
 		}
 
-		// Remove expired verification keys.
-		i := 0
+		expired := func(key storage.VerificationKey) bool {
+			return tNow.After(key.Expiry)
+		}
 
+		// Remove any verification keys that have expired.
+		i := 0
 		for _, key := range keys.VerificationKeys {
-			if !key.Expiry.After(tNow) {
+			if !expired(key) {
 				keys.VerificationKeys[i] = key
 				i++
 			}
@@ -140,10 +144,15 @@ func (k keyRotater) rotate() error {
 		keys.VerificationKeys = keys.VerificationKeys[:i]
 
 		if keys.SigningKeyPub != nil {
-			// Move current signing key to a verification only key.
+			// Move current signing key to a verification only key, throwing
+			// away the private part.
 			verificationKey := storage.VerificationKey{
 				PublicKey: keys.SigningKeyPub,
-				Expiry:    tNow.Add(k.strategy.verifyFor),
+				// After demoting the signing key, keep the token around for at least
+				// the amount of time an ID Token is valid for. This ensures the
+				// verification key won't expire until all ID Tokens it's signed
+				// expired as well.
+				Expiry: tNow.Add(k.strategy.idTokenValidFor),
 			}
 			keys.VerificationKeys = append(keys.VerificationKeys, verificationKey)
 		}
diff --git a/storage/kubernetes/client.go b/storage/kubernetes/client.go
index 55e319d7..dc238a9c 100644
--- a/storage/kubernetes/client.go
+++ b/storage/kubernetes/client.go
@@ -72,10 +72,9 @@ func idToName(s string, h func() hash.Hash) string {
 }
 
 func offlineTokenName(userID string, connID string, h func() hash.Hash) string {
-	hash := h()
-	hash.Write([]byte(userID))
-	hash.Write([]byte(connID))
-	return strings.TrimRight(encoding.EncodeToString(hash.Sum(nil)), "=")
+	h().Write([]byte(userID))
+	h().Write([]byte(connID))
+	return strings.TrimRight(encoding.EncodeToString(h().Sum(nil)), "=")
 }
 
 func (c *client) urlFor(apiVersion, namespace, resource, name string) string {
@@ -136,9 +135,6 @@ func checkHTTPErr(r *http.Response, validStatusCodes ...int) error {
 	if r.StatusCode == http.StatusNotFound {
 		return storage.ErrNotFound
 	}
-	if r.Request.Method == "POST" && r.StatusCode == http.StatusConflict {
-		return storage.ErrAlreadyExists
-	}
 
 	var url, method string
 	if r.Request != nil {
diff --git a/storage/kubernetes/storage.go b/storage/kubernetes/storage.go
index 788d08b1..421b74c9 100644
--- a/storage/kubernetes/storage.go
+++ b/storage/kubernetes/storage.go
@@ -3,6 +3,7 @@ package kubernetes
 import (
 	"errors"
 	"fmt"
+	"net/http"
 	"strings"
 	"time"
 
@@ -30,7 +31,7 @@ const (
 	resourceRefreshToken    = "refreshtokens"
 	resourceKeys            = "signingkeies" // Kubernetes attempts to pluralize.
 	resourcePassword        = "passwords"
-	resourceOfflineSessions = "offlinesessionses" // Again attempts to pluralize.
+	resourceOfflineSessions = "offlinesessions"
 )
 
 // Config values for the Kubernetes storage type.
@@ -41,19 +42,15 @@ type Config struct {
 
 // Open returns a storage using Kubernetes third party resource.
 func (c *Config) Open(logger logrus.FieldLogger) (storage.Storage, error) {
-	cli, err := c.open(logger, false)
+	cli, err := c.open(logger)
 	if err != nil {
 		return nil, err
 	}
 	return cli, nil
 }
 
-// open returns a kubernetes client, initializing the third party resources used
-// by dex.
-//
-// errOnTPRs controls if errors creating the resources cause this method to return
-// immediately (used during testing), or if the client will asynchronously retry.
-func (c *Config) open(logger logrus.FieldLogger, errOnTPRs bool) (*client, error) {
+// open returns a client with no garbage collection.
+func (c *Config) open(logger logrus.FieldLogger) (*client, error) {
 	if c.InCluster && (c.KubeConfigFile != "") {
 		return nil, errors.New("cannot specify both 'inCluster' and 'kubeConfigFile'")
 	}
@@ -83,18 +80,16 @@ func (c *Config) open(logger logrus.FieldLogger, errOnTPRs bool) (*client, error
 
 	ctx, cancel := context.WithCancel(context.Background())
 
-	if !cli.createThirdPartyResources() {
-		if errOnTPRs {
-			return nil, fmt.Errorf("failed creating third party resources")
-		}
-
-		// Try to synchronously create the third party resources once. This doesn't mean
-		// they'll immediately be available, but ensures that the client will actually try
-		// once.
+	// Try to synchronously create the third party resources once. This doesn't mean
+	// they'll immediately be available, but ensures that the client will actually try
+	// once.
+	if err := cli.createThirdPartyResources(); err != nil {
 		logger.Errorf("failed creating third party resources: %v", err)
 		go func() {
 			for {
-				if cli.createThirdPartyResources() {
+				if err := cli.createThirdPartyResources(); err != nil {
+					logger.Errorf("failed creating third party resources: %v", err)
+				} else {
 					return
 				}
 
@@ -113,33 +108,27 @@ func (c *Config) open(logger logrus.FieldLogger, errOnTPRs bool) (*client, error
 }
 
 // createThirdPartyResources attempts to create the third party resources dex
-// requires or identifies that they're already enabled. It logs all errors,
-// returning true if the third party resources were created successfully.
+// requires or identifies that they're already enabled.
 //
 // Creating a third party resource does not mean that they'll be immediately available.
 //
 // TODO(ericchiang): Provide an option to wait for the third party resources
 // to actually be available.
-func (cli *client) createThirdPartyResources() (ok bool) {
-	ok = true
+func (cli *client) createThirdPartyResources() error {
 	for _, r := range thirdPartyResources {
 		err := cli.postResource("extensions/v1beta1", "", "thirdpartyresources", r)
 		if err != nil {
-			switch err {
-			case storage.ErrAlreadyExists:
-				cli.logger.Errorf("third party resource already created %s", r.ObjectMeta.Name)
-			case storage.ErrNotFound:
-				cli.logger.Errorf("third party resources not found, please enable API group extensions/v1beta1")
-				ok = false
-			default:
-				cli.logger.Errorf("creating third party resource %s: %v", r.ObjectMeta.Name, err)
-				ok = false
+			if e, ok := err.(httpError); ok {
+				if e.StatusCode() == http.StatusConflict {
+					cli.logger.Errorf("third party resource already created %q", r.ObjectMeta.Name)
+					continue
+				}
 			}
-			continue
+			return err
 		}
-		cli.logger.Errorf("create third party resource %s", r.ObjectMeta.Name)
+		cli.logger.Errorf("create third party resource %q", r.ObjectMeta.Name)
 	}
-	return ok
+	return nil
 }
 
 func (cli *client) Close() error {
diff --git a/storage/kubernetes/types.go b/storage/kubernetes/types.go
index c362452f..81dc01c3 100644
--- a/storage/kubernetes/types.go
+++ b/storage/kubernetes/types.go
@@ -502,14 +502,9 @@ func (cli *client) fromStorageOfflineSessions(o storage.OfflineSessions) Offline
 }
 
 func toStorageOfflineSessions(o OfflineSessions) storage.OfflineSessions {
-	s := storage.OfflineSessions{
+	return storage.OfflineSessions{
 		UserID:  o.UserID,
 		ConnID:  o.ConnID,
 		Refresh: o.Refresh,
 	}
-	if s.Refresh == nil {
-		// Server code assumes this will be non-nil.
-		s.Refresh = make(map[string]*storage.RefreshTokenRef)
-	}
-	return s
 }


Test patch:
diff --git a/server/rotation_test.go b/server/rotation_test.go
index abb4e431..a792d7b6 100644
--- a/server/rotation_test.go
+++ b/server/rotation_test.go
@@ -1,1 +1,101 @@
 package server
+
+import (
+	"os"
+	"sort"
+	"testing"
+	"time"
+
+	"github.com/Sirupsen/logrus"
+	"github.com/coreos/dex/storage"
+	"github.com/coreos/dex/storage/memory"
+)
+
+func signingKeyID(t *testing.T, s storage.Storage) string {
+	keys, err := s.GetKeys()
+	if err != nil {
+		t.Fatal(err)
+	}
+	return keys.SigningKey.KeyID
+}
+
+func verificationKeyIDs(t *testing.T, s storage.Storage) (ids []string) {
+	keys, err := s.GetKeys()
+	if err != nil {
+		t.Fatal(err)
+	}
+	for _, key := range keys.VerificationKeys {
+		ids = append(ids, key.PublicKey.KeyID)
+	}
+	return ids
+}
+
+// slicesEq compare two string slices without modifying the ordering
+// of the slices.
+func slicesEq(s1, s2 []string) bool {
+	if len(s1) != len(s2) {
+		return false
+	}
+
+	cp := func(s []string) []string {
+		c := make([]string, len(s))
+		copy(c, s)
+		return c
+	}
+
+	cp1 := cp(s1)
+	cp2 := cp(s2)
+	sort.Strings(cp1)
+	sort.Strings(cp2)
+
+	for i, el := range cp1 {
+		if el != cp2[i] {
+			return false
+		}
+	}
+	return true
+}
+
+func TestKeyRotater(t *testing.T) {
+	now := time.Now()
+
+	delta := time.Millisecond
+	rotationFrequency := time.Second * 5
+	validFor := time.Second * 21
+
+	// Only the last 5 verification keys are expected to be kept around.
+	maxVerificationKeys := 5
+
+	l := &logrus.Logger{
+		Out:       os.Stderr,
+		Formatter: &logrus.TextFormatter{DisableColors: true},
+		Level:     logrus.DebugLevel,
+	}
+
+	r := &keyRotater{
+		Storage:  memory.New(l),
+		strategy: defaultRotationStrategy(rotationFrequency, validFor),
+		now:      func() time.Time { return now },
+		logger:   l,
+	}
+
+	var expVerificationKeys []string
+
+	for i := 0; i < 10; i++ {
+		now = now.Add(rotationFrequency + delta)
+		if err := r.rotate(); err != nil {
+			t.Fatal(err)
+		}
+
+		got := verificationKeyIDs(t, r.Storage)
+
+		if !slicesEq(expVerificationKeys, got) {
+			t.Errorf("after %d rotation, expected varification keys %q, got %q", i+1, expVerificationKeys, got)
+		}
+
+		expVerificationKeys = append(expVerificationKeys, signingKeyID(t, r.Storage))
+		if n := len(expVerificationKeys); n > maxVerificationKeys {
+			expVerificationKeys = expVerificationKeys[n-maxVerificationKeys:]
+		}
+	}
+}
diff --git a/storage/kubernetes/client_test.go b/storage/kubernetes/client_test.go
index 5793d7d1..01b4eb7a 100644
--- a/storage/kubernetes/client_test.go
+++ b/storage/kubernetes/client_test.go
@@ -29,19 +29,6 @@ func TestIDToName(t *testing.T) {
 	wg.Wait()
 }
 
-func TestOfflineTokenName(t *testing.T) {
-	h := func() hash.Hash { return fnv.New64() }
-
-	userID1 := "john"
-	userID2 := "jane"
-
-	id1 := offlineTokenName(userID1, "local", h)
-	id2 := offlineTokenName(userID2, "local", h)
-	if id1 == id2 {
-		t.Errorf("expected offlineTokenName to produce different hashes")
-	}
-}
-
 func TestNamespaceFromServiceAccountJWT(t *testing.T) {
 	namespace, err := namespaceFromServiceAccountJWT(serviceAccountToken)
 	if err != nil {
diff --git a/storage/kubernetes/storage_test.go b/storage/kubernetes/storage_test.go
index 2addd12e..dae641e8 100644
--- a/storage/kubernetes/storage_test.go
+++ b/storage/kubernetes/storage_test.go
@@ -28,7 +28,7 @@ func loadClient(t *testing.T) *client {
 		Formatter: &logrus.TextFormatter{DisableColors: true},
 		Level:     logrus.DebugLevel,
 	}
-	s, err := config.open(logger, true)
+	s, err := config.open(logger)
 	if err != nil {
 		t.Fatal(err)
 	}