From 32152e10fdb78f751289ad82a77a2ac0599ebceb Mon Sep 17 00:00:00 2001
From: John-Michael Faircloth <fairclothjm@users.noreply.github.com>
Date: Mon, 29 Nov 2021 15:10:58 -0600
Subject: [PATCH] Identity: check NextSigningKey existence during key rotation
 (#13298)

* oidc: fix key rotation panic

* refactor and update unit tests

* add changelog
---
 changelog/13298.txt               |  3 ++
 vault/identity_store_oidc.go      | 55 +++++++++++++++++++------------
 vault/identity_store_oidc_test.go |  6 ++--
 3 files changed, 40 insertions(+), 24 deletions(-)
 create mode 100644 changelog/13298.txt

diff --git a/changelog/13298.txt b/changelog/13298.txt
new file mode 100644
index 000000000..893ab1281
--- /dev/null
+++ b/changelog/13298.txt
@@ -0,0 +1,3 @@
+```release-note:bug
+identity: Fixes a panic in the OIDC key rotation due to a missing nil check.
+```
diff --git a/vault/identity_store_oidc.go b/vault/identity_store_oidc.go
index e9dbd1824..751fc84c5 100644
--- a/vault/identity_store_oidc.go
+++ b/vault/identity_store_oidc.go
@@ -561,18 +561,10 @@ func (i *IdentityStore) pathOIDCCreateUpdateKey(ctx context.Context, req *logica
 		}
 		i.Logger().Debug("generated OIDC public key to sign JWTs", "key_id", signingKey.Public().KeyID)
 
-		nextSigningKey, err := generateKeys(key.Algorithm)
+		err = key.generateAndSetNextKey(ctx, i.Logger(), req.Storage)
 		if err != nil {
 			return nil, err
 		}
-
-		key.NextSigningKey = nextSigningKey
-		key.KeyRing = append(key.KeyRing, &expireableKey{KeyID: nextSigningKey.Public().KeyID})
-
-		if err := saveOIDCPublicKey(ctx, req.Storage, nextSigningKey.Public()); err != nil {
-			return nil, err
-		}
-		i.Logger().Debug("generated OIDC public key for future use", "key_id", nextSigningKey.Public().KeyID)
 	}
 
 	if err := i.oidcCache.Flush(ns); err != nil {
@@ -1021,6 +1013,24 @@ func mergeJSONTemplates(logger hclog.Logger, output map[string]interface{}, temp
 	return nil
 }
 
+// generateAndSetNextKey will generate new signing and public key pairs and set
+// them as the NextSigningKey.
+func (k *namedKey) generateAndSetNextKey(ctx context.Context, logger hclog.Logger, s logical.Storage) error {
+	signingKey, err := generateKeys(k.Algorithm)
+	if err != nil {
+		return err
+	}
+
+	k.NextSigningKey = signingKey
+	k.KeyRing = append(k.KeyRing, &expireableKey{KeyID: signingKey.Public().KeyID})
+
+	if err := saveOIDCPublicKey(ctx, s, signingKey.Public()); err != nil {
+		return err
+	}
+	logger.Debug("generated OIDC public key for future use", "key_id", signingKey.Public().KeyID)
+	return nil
+}
+
 func (k *namedKey) signPayload(payload []byte) (string, error) {
 	signingKey := jose.SigningKey{Key: k.SigningKey, Algorithm: jose.SignatureAlgorithm(k.Algorithm)}
 	signer, err := jose.NewSigner(signingKey, &jose.SignerOptions{})
@@ -1477,16 +1487,6 @@ func (k *namedKey) rotate(ctx context.Context, logger hclog.Logger, s logical.St
 		verificationTTL = overrideVerificationTTL
 	}
 
-	// generate new key
-	nextSigningKey, err := generateKeys(k.Algorithm)
-	if err != nil {
-		return err
-	}
-	if err := saveOIDCPublicKey(ctx, s, nextSigningKey.Public()); err != nil {
-		return err
-	}
-	logger.Debug("generated OIDC public key for future use", "key_id", nextSigningKey.Public().KeyID)
-
 	now := time.Now()
 	// set the previous public key's expiry time
 	for _, key := range k.KeyRing {
@@ -1496,11 +1496,24 @@ func (k *namedKey) rotate(ctx context.Context, logger hclog.Logger, s logical.St
 		}
 	}
 
-	k.KeyRing = append(k.KeyRing, &expireableKey{KeyID: nextSigningKey.KeyID})
+	if k.NextSigningKey == nil {
+		// keys will not have a NextSigningKey if they were generated before
+		// vault 1.9
+		err := k.generateAndSetNextKey(ctx, logger, s)
+		if err != nil {
+			return err
+		}
+	}
+	// do the rotation
 	k.SigningKey = k.NextSigningKey
-	k.NextSigningKey = nextSigningKey
 	k.NextRotation = now.Add(k.RotationPeriod)
 
+	// now that we have rotated, generate a new NextSigningKey
+	err := k.generateAndSetNextKey(ctx, logger, s)
+	if err != nil {
+		return err
+	}
+
 	// store named key (it was modified when rotate was called on it)
 	entry, err := logical.StorageEntryJSON(namedKeyConfigPath+k.name, k)
 	if err != nil {
diff --git a/vault/identity_store_oidc_test.go b/vault/identity_store_oidc_test.go
index ba63a940f..d52ae7a14 100644
--- a/vault/identity_store_oidc_test.go
+++ b/vault/identity_store_oidc_test.go
@@ -921,6 +921,7 @@ func TestOIDC_PeriodicFunc(t *testing.T) {
 		}
 	}{
 		{
+			// don't set NextSigningKey to ensure its non-existence can be handled
 			&namedKey{
 				name:            "test-key",
 				Algorithm:       "RS256",
@@ -928,7 +929,6 @@ func TestOIDC_PeriodicFunc(t *testing.T) {
 				RotationPeriod:  1 * cyclePeriod,
 				KeyRing:         nil,
 				SigningKey:      jwk,
-				NextSigningKey:  jwk,
 				NextRotation:    time.Now(),
 			},
 			[]struct {
@@ -936,8 +936,8 @@ func TestOIDC_PeriodicFunc(t *testing.T) {
 				numKeys       int
 				numPublicKeys int
 			}{
-				{1, 1, 1},
-				{2, 2, 2},
+				{1, 2, 2},
+				{2, 3, 3},
 				{3, 3, 3},
 				{4, 3, 3},
 				{5, 3, 3},