Skip to content

Commit

Permalink
Identity: check NextSigningKey existence during key rotation (#13298) (
Browse files Browse the repository at this point in the history
…#13303)

* oidc: fix key rotation panic

* refactor and update unit tests

* add changelog
  • Loading branch information
fairclothjm committed Nov 29, 2021
1 parent 90e1bca commit 5f98886
Show file tree
Hide file tree
Showing 3 changed files with 40 additions and 24 deletions.
3 changes: 3 additions & 0 deletions changelog/13298.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
```release-note:bug
identity: Fixes a panic in the OIDC key rotation due to a missing nil check.
```
55 changes: 34 additions & 21 deletions vault/identity_store_oidc.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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{})
Expand Down Expand Up @@ -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 {
Expand All @@ -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 {
Expand Down
6 changes: 3 additions & 3 deletions vault/identity_store_oidc_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -921,23 +921,23 @@ func TestOIDC_PeriodicFunc(t *testing.T) {
}
}{
{
// don't set NextSigningKey to ensure its non-existence can be handled
&namedKey{
name: "test-key",
Algorithm: "RS256",
VerificationTTL: 1 * cyclePeriod,
RotationPeriod: 1 * cyclePeriod,
KeyRing: nil,
SigningKey: jwk,
NextSigningKey: jwk,
NextRotation: time.Now(),
},
[]struct {
cycle int
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},
Expand Down

0 comments on commit 5f98886

Please sign in to comment.