Identity: prepublish jwt signing keys #12414
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
fairclothjm
requested review from
vinay-gopalan,
calvn,
austingebauer and
a team
and removed request for
a team
calvn
reviewed
Aug 31, 2021
vault/identity_store_oidc.go
Outdated
| key.KeyRing = append(key.KeyRing, &expireableKey{KeyID: signingKey.Public().KeyID}) | ||
| key.KeyRing = append(key.KeyRing, &expireableKey{ | ||
| KeyID: signingKey.Public().KeyID, | ||
| ExpireAt: now.Add(key.RotationPeriod).Add(key.VerificationTTL), |
There was a problem hiding this comment.
IIUC, this means that we're calculating the ExpireAt by factoring the rotation period early on instead of just appending the VerificationTTL upon rotation like it currently is. What happens if there's an update on the named key's rotation_period after the key has been created (i.e. by an update)?
austingebauer
approved these changes
Sep 9, 2021
changelog/12414.txt
Outdated
| @@ -0,0 +1,3 @@ | |||
| ```release-note:bug | |||
There was a problem hiding this comment.
nit: I wonder if this is an enhancement instead of a bug? I think the implementation before functioned as designed.
There was a problem hiding this comment.
I agree with that
jartek
pushed a commit
to jartek/vault
that referenced
this pull request
Sep 11, 2021
* pre-publish new signing keys for `rotation_period` of time before using * Work In Progress: Prepublish JWKS and even cache control * remove comments * use math/rand instead of math/big * update tests * remove debug comment * refactor cache control logic into func * don't set expiry when create/update key * update cachecontrol name in oidccache for test * fix bug in periodicfunc test case * add changelog * remove confusing comment * add logging and comments * update change log from bug to improvement Co-authored-by: Ian Ferguson <ian.ferguson@datadoghq.com>
|
thanks for picking this up! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Addresses #11438 which reported that OIDC key rotations happen up to 60 seconds after they are scheduled. This becomes an issue because the
Cache-Controlheader returned on calls toidentity/oidc/.well-known/keyswill returnCache-Control: no-storefor the 0 to 60 seconds between the scheduled rotation time and when rotation happens. This causes a thundering herd of server requests to update their JWKS keys.This solution updates the Cache-Control logic to return a random value between 0 and the lowest rotation_period and validation_ttl of all keys on the vault cluster, to smooth out the rate servers call Vault for JWKS keys and avoid the thundering herd issues.