Repository: coreos/go-oidc
Pull request #259 (2020-07-17 06:07:05)

*: remove Cache-Control checks from key rotation strategy
This removes the dependency on github.com/pquerna/cachecontrol and
brings the package more inline with the strategy from the OpenID Spec
for rotation.

https://openid.net/specs/openid-connect-core-1_0.html#RotateSigKeys

Updates https://github.com/coreos/go-oidc/issues/232


Patch:
diff --git a/go.mod b/go.mod
index 50be11f..f5ee5ae 100644
--- a/go.mod
+++ b/go.mod
@@ -4,7 +4,6 @@ go 1.14
 
 require (
 	github.com/google/go-cmp v0.4.0 // indirect
-	github.com/pquerna/cachecontrol v0.0.0-20180517163645-1555304b9b35
 	github.com/stretchr/testify v1.5.1 // indirect
 	golang.org/x/net v0.0.0-20200505041828-1ed23360d12c
 	golang.org/x/oauth2 v0.0.0-20200107190931-bf48bf16ab8d
diff --git a/go.sum b/go.sum
index 660d665..66c0767 100644
--- a/go.sum
+++ b/go.sum
@@ -7,8 +7,6 @@ github.com/google/go-cmp v0.4.0 h1:xsAVV57WRhGj6kEIi8ReJzQlHHqcBYCElAvkovg3B/4=
 github.com/google/go-cmp v0.4.0/go.mod h1:v8dTdLbMG2kIc/vJvl+f65V22dbkXbowE6jgT/gNBxE=
 github.com/pmezard/go-difflib v1.0.0 h1:4DBwDE0NGyQoBHbLQYPwSUPoCMWR5BEzIk/f1lZbAQM=
 github.com/pmezard/go-difflib v1.0.0/go.mod h1:iKH77koFhYxTK1pcRnkKkqfTogsbg7gZNVY4sRDYZ/4=
-github.com/pquerna/cachecontrol v0.0.0-20180517163645-1555304b9b35 h1:J9b7z+QKAmPf4YLrFg6oQUotqHQeUNWwkvo7jZp1GLU=
-github.com/pquerna/cachecontrol v0.0.0-20180517163645-1555304b9b35/go.mod h1:prYjPmNq4d1NPVmpShWobRqXY3q7Vp+80DqgxxUrUIA=
 github.com/stretchr/objx v0.1.0/go.mod h1:HFkY916IF+rwdDfMAkV7OtwuqBVzrE8GR6GFx+wExME=
 github.com/stretchr/testify v1.5.1 h1:nOGnQDM7FYENwehXlg/kFVnos3rEvtKTjRvOWSzb6H4=
 github.com/stretchr/testify v1.5.1/go.mod h1:5W2xD1RspED5o8YsWQXVCued0rvSQ+mT+I5cxcmMvtA=
diff --git a/oidc/jwks.go b/oidc/jwks.go
index e6a82c8..ce737f7 100644
--- a/oidc/jwks.go
+++ b/oidc/jwks.go
@@ -9,20 +9,9 @@ import (
 	"sync"
 	"time"
 
-	"github.com/pquerna/cachecontrol"
 	jose "gopkg.in/square/go-jose.v2"
 )
 
-// keysExpiryDelta is the allowed clock skew between a client and the OpenID Connect
-// server.
-//
-// When keys expire, they are valid for this amount of time after.
-//
-// If the keys have not expired, and an ID Token claims it was signed by a key not in
-// the cache, if and only if the keys expire in this amount of time, the keys will be
-// updated.
-const keysExpiryDelta = 30 * time.Second
-
 // NewRemoteKeySet returns a KeySet that can validate JSON web tokens by using HTTP
 // GETs to fetch JSON web token sets hosted at a remote URL. This is automatically
 // used by NewProvider using the URLs returned by OpenID Connect discovery, but is
@@ -56,9 +45,8 @@ type remoteKeySet struct {
 	// multiple goroutines to wait for its result.
 	inflight *inflight
 
-	// A set of cached keys and their expiry.
+	// A set of cached keys.
 	cachedKeys []jose.JSONWebKey
-	expiry     time.Time
 }
 
 // inflight is used to wait on some in-flight request from multiple goroutines.
@@ -109,9 +97,7 @@ func (r *remoteKeySet) verify(ctx context.Context, jws *jose.JSONWebSignature) (
 		break
 	}
 
-	keys, expiry := r.keysFromCache()
-
-	// Don't check expiry yet. This optimizes for when the provider is unavailable.
+	keys := r.keysFromCache()
 	for _, key := range keys {
 		if keyID == "" || key.KeyID == keyID {
 			if payload, err := jws.Verify(&key); err == nil {
@@ -120,11 +106,10 @@ func (r *remoteKeySet) verify(ctx context.Context, jws *jose.JSONWebSignature) (
 		}
 	}
 
-	if !r.now().Add(keysExpiryDelta).After(expiry) {
-		// Keys haven't expired, don't refresh.
-		return nil, errors.New("failed to verify id token signature")
-	}
-
+	// If the kid doesn't match, check for new keys from the remote. This is the
+	// strategy recommended by the spec.
+	//
+	// https://openid.net/specs/openid-connect-core-1_0.html#RotateSigKeys
 	keys, err := r.keysFromRemote(ctx)
 	if err != nil {
 		return nil, fmt.Errorf("fetching keys %v", err)
@@ -140,10 +125,10 @@ func (r *remoteKeySet) verify(ctx context.Context, jws *jose.JSONWebSignature) (
 	return nil, errors.New("failed to verify id token signature")
 }
 
-func (r *remoteKeySet) keysFromCache() (keys []jose.JSONWebKey, expiry time.Time) {
+func (r *remoteKeySet) keysFromCache() (keys []jose.JSONWebKey) {
 	r.mu.Lock()
 	defer r.mu.Unlock()
-	return r.cachedKeys, r.expiry
+	return r.cachedKeys
 }
 
 // keysFromRemote syncs the key set from the remote set, records the values in the
@@ -160,7 +145,7 @@ func (r *remoteKeySet) keysFromRemote(ctx context.Context) ([]jose.JSONWebKey, e
 		// once the goroutine is done.
 		go func() {
 			// Sync keys and finish inflight when that's done.
-			keys, expiry, err := r.updateKeys()
+			keys, err := r.updateKeys()
 
 			r.inflight.done(keys, err)
 
@@ -171,7 +156,6 @@ func (r *remoteKeySet) keysFromRemote(ctx context.Context) ([]jose.JSONWebKey, e
 
 			if err == nil {
 				r.cachedKeys = keys
-				r.expiry = expiry
 			}
 
 			// Free inflight so a different request can run.
@@ -189,40 +173,31 @@ func (r *remoteKeySet) keysFromRemote(ctx context.Context) ([]jose.JSONWebKey, e
 	}
 }
 
-func (r *remoteKeySet) updateKeys() ([]jose.JSONWebKey, time.Time, error) {
+func (r *remoteKeySet) updateKeys() ([]jose.JSONWebKey, error) {
 	req, err := http.NewRequest("GET", r.jwksURL, nil)
 	if err != nil {
-		return nil, time.Time{}, fmt.Errorf("oidc: can't create request: %v", err)
+		return nil, fmt.Errorf("oidc: can't create request: %v", err)
 	}
 
 	resp, err := doRequest(r.ctx, req)
 	if err != nil {
-		return nil, time.Time{}, fmt.Errorf("oidc: get keys failed %v", err)
+		return nil, fmt.Errorf("oidc: get keys failed %v", err)
 	}
 	defer resp.Body.Close()
 
 	body, err := ioutil.ReadAll(resp.Body)
 	if err != nil {
-		return nil, time.Time{}, fmt.Errorf("unable to read response body: %v", err)
+		return nil, fmt.Errorf("unable to read response body: %v", err)
 	}
 
 	if resp.StatusCode != http.StatusOK {
-		return nil, time.Time{}, fmt.Errorf("oidc: get keys failed: %s %s", resp.Status, body)
+		return nil, fmt.Errorf("oidc: get keys failed: %s %s", resp.Status, body)
 	}
 
 	var keySet jose.JSONWebKeySet
 	err = unmarshalResp(resp, body, &keySet)
 	if err != nil {
-		return nil, time.Time{}, fmt.Errorf("oidc: failed to decode keys: %v %s", err, body)
-	}
-
-	// If the server doesn't provide cache control headers, assume the
-	// keys expire immediately.
-	expiry := r.now()
-
-	_, e, err := cachecontrol.CachableResponse(req, resp, cachecontrol.Options{})
-	if err == nil && e.After(expiry) {
-		expiry = e
+		return nil, fmt.Errorf("oidc: failed to decode keys: %v %s", err, body)
 	}
-	return keySet.Keys, expiry, nil
+	return keySet.Keys, nil
 }


Test patch:
diff --git a/oidc/jwks_test.go b/oidc/jwks_test.go
index 6226117..4103e4b 100644
--- a/oidc/jwks_test.go
+++ b/oidc/jwks_test.go
@@ -172,7 +172,7 @@ func testKeyVerify(t *testing.T, good, bad *signingKey, verification ...*signing
 	}
 }
 
-func TestCacheControl(t *testing.T) {
+func TestRotation(t *testing.T) {
 	ctx, cancel := context.WithCancel(context.Background())
 	defer cancel()
 
@@ -220,16 +220,6 @@ func TestCacheControl(t *testing.T) {
 		Keys: []jose.JSONWebKey{key1.jwk(), key2.jwk()},
 	}
 
-	if _, err := rks.verify(ctx, jws1); err != nil {
-		t.Errorf("failed to verify valid signature: %v", err)
-	}
-	if _, err := rks.verify(ctx, jws2); err == nil {
-		t.Errorf("incorrectly verified signature, still within cache limit")
-	}
-
-	// Move time forward. Remote key set should not query the remote server.
-	now = now.Add(time.Duration(cacheForSeconds) * time.Second)
-
 	if _, err := rks.verify(ctx, jws1); err != nil {
 		t.Errorf("failed to verify valid signature: %v", err)
 	}
@@ -237,9 +227,8 @@ func TestCacheControl(t *testing.T) {
 		t.Errorf("failed to verify valid signature: %v", err)
 	}
 
-	// Kill server and move time forward again. Keys should still verify.
+	// Kill server. Keys should still be cached.
 	s.Close()
-	now = now.Add(time.Duration(cacheForSeconds) * time.Second)
 
 	if _, err := rks.verify(ctx, jws1); err != nil {
 		t.Errorf("failed to verify valid signature: %v", err)