-
Notifications
You must be signed in to change notification settings - Fork 4.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Identity: prepublish jwt signing keys #12414
Conversation
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a few comments to get started. Still looking through this and wrapping my head around the approach. Will give another pass shortly. Ty!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍
changelog/12414.txt
Outdated
@@ -0,0 +1,3 @@ | |||
```release-note:bug |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with that
thanks for picking this up! |
Addresses #11438 which reported that OIDC key rotations happen up to 60 seconds after they are scheduled. This becomes an issue because the
Cache-Control
header returned on calls toidentity/oidc/.well-known/keys
will returnCache-Control: no-store
for 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.