oidc: check for nil signing key on rotation

[fairclothjm]

Description

Check for a nil signing key before performing a rotation and before signing payloads. This is to prevent panics in the event that a nil signing key was written to storage.

If the current signing key is nil during rotation, we will skip setting the ExpireAt and allow the next signing key to be rotated into the current. If the signing key is nil during sign payload, we will return an error.

Background

A panic can occur for any key which was created while being on a version < v1.9.0 but was rotated while being on v1.9.0.

Repro steps:

• start vault with persistent storage
• create keys/roles with Vault < v1.9.0
• upgrade vault to v1.9.0
• manually rotate the key or wait for an auto rotation
• once key rotation triggers, a panic will occur
  • this was fixed in Identity: check NextSigningKey existence during key rotation #13298
• restarting vault v1.9.0 should result in panics on rotation
• upgrade vault to v1.9.1 or greater
• restart vault with same persistent storage backend
• once key rotation triggers, a panic will occur
  • this is being fix in the current PR

[fairclothjm]

Logs

T14:05:11.152-0600 [DEBUG] : rotating OIDC key: key=key1
T14:05:11.152-0600 [DEBUG] : nil signing key detected on rotation
T14:05:11.306-0600 [DEBUG] : generated OIDC public key to sign JWTs: key_id=aa2c1f85-32cf-3645-1a52-7cd7e0983ccf
T14:05:11.459-0600 [DEBUG] : generated OIDC public key for future use: key_id=3d9c57ba-9384-ffd9-6714-2ac56060a286
T14:05:11.537-0600 [DEBUG] : rotated OIDC public key, now using: key_id=f8e1c269-94a1-28b7-e26a-2b020c6b980e
T14:05:11.695-0600 [DEBUG] : deleted OIDC public key: key_id=6d428db6-3c32-ddfa-0e48-f4b616a4a42f


T14:07:11.147-0600 [DEBUG] : rotating OIDC key: key=key1
T14:07:11.557-0600 [DEBUG] : generated OIDC public key for future use: key_id=bfc5304b-d810-cdba-7dd3-40081cb12b19
T14:07:11.635-0600 [DEBUG] : rotated OIDC public key, now using: key_id=3d9c57ba-9384-ffd9-6714-2ac56060a286
T14:07:11.795-0600 [DEBUG] : deleted OIDC public key: key_id=aa2c1f85-32cf-3645-1a52-7cd7e0983ccf

[calvn]

Mostly minor nit, but otherwise looks good!

[calvn]

NIt: cycle could be an int since it's just used as a counter length reference. You could have something like:

for i := 0; i < cycle; i++ {
  currentCycle := cycle + 1
  ...
}

The for loop above could be reworked to be something similar as well.

[calvn]

Nit: I wonder if it would be a bit more obvious that the cases are based on different toggles if we had a test helper that generated the named keys with setSigningKey and setNextSigningKey` as params (which would also remove them from the testSet struct):

namedKey: testGenerateNamedKey("test-key-nil-signing-key", false, true)

[fairclothjm]

I think the namedKey would need to be removed from the testSet instead since we need to call generateAndSetKey with the storage for each test set. I can create an issue to track cleaning up these tests a bit.

[austingebauer]

LGTM 👍