fix: use up-to-date kid in JWT header when refreshing
#3942
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| @@ -1021,6 +1021,32 @@ func (h *Handler) oauth2TokenExchange(w http.ResponseWriter, r *http.Request) { | |||
| } | |||
| } | |||
|
|
|||
| if accessRequest.GetGrantTypes().ExactOne(string(fosite.GrantTypeRefreshToken)) { | |||
There was a problem hiding this comment.
Maybe a short comment explaining why we do this here?
There was a problem hiding this comment.
| if accessRequest.GetGrantTypes().ExactOne(string(fosite.GrantTypeRefreshToken)) { | |
| // When refreshing tokens, we want to ensure to use the latest key-id available for signing the | |
| // potentially JWT-formatted tokens. | |
| if accessRequest.GetGrantTypes().ExactOne(string(fosite.GrantTypeRefreshToken)) { |
| @@ -1021,6 +1021,32 @@ func (h *Handler) oauth2TokenExchange(w http.ResponseWriter, r *http.Request) { | |||
| } | |||
| } | |||
|
|
|||
| if accessRequest.GetGrantTypes().ExactOne(string(fosite.GrantTypeRefreshToken)) { | |||
There was a problem hiding this comment.
| if accessRequest.GetGrantTypes().ExactOne(string(fosite.GrantTypeRefreshToken)) { | |
| // When refreshing tokens, we want to ensure to use the latest key-id available for signing the | |
| // potentially JWT-formatted tokens. | |
| if accessRequest.GetGrantTypes().ExactOne(string(fosite.GrantTypeRefreshToken)) { |
| } | ||
| } | ||
|
|
||
| openIDKeyID, err := h.r.OpenIDJWTStrategy().GetPublicKeyID(ctx) |
There was a problem hiding this comment.
Can you please add an if condition to only fetch this key if the flow is an openid flow. I think it should be enough to check if GrantedScope contains openid. This will reduce DB load in cases where we don't have an OpenID flow.
|
It may be that this can be solved more simply by letting fosite handle this, now that ory/fosite#799 is released. I put up a PR to see about that. I kept the E2E tests and the commit authorship, but added myself as a co-author. Instead of adding go code to set the right |
|
I agree that #3973 is the better approach now that this is solved in fosite! |
When you rotate any of the JWT signing keys (
hydra.jwt.access-token/hydra.openid.id-token, Hydra will (as expected) sign all new JWTs with the new key - including any JWTs from a refresh token grant flow. However, for any refresh token chain that existed before the key rotation Hydra will incorrectly use thekidof the old key in the JWT header. This effectively breaks/poisons all issued refresh tokens on key rotation since all future access/id tokens they provide will be invalid. See the new test for a reproducible example.I consider this to be a critical bug.
Related issue(s)
#3573, #3719
Checklist
introduces a new feature.
contributing code guidelines.
vulnerability. If this pull request addresses a security vulnerability, I
confirm that I got the approval (please contact
[email protected]) from the maintainers to push
the changes.
works.
Further Comments
The underlying problem is that the JWT signing mechanism doesn't control the
kidin the JWT header - it's picked separately and stored inSession. So ideally, we would either instruct Fosite which key to use or respect the key that Fosite chooses. As pointed out in #3573, if we do not set the KIDs Fosite will use the correctkid. However, because we include the KIDs in the token hook upfront before we sign the JWTs it would be a breaking change to omit it. It seems non-trivial (for me at least) to refactor the codebase so the KIDs used in the actual JWT signing code is the source of truth for the KIDs provided in the token hook - mostly because the hook must run before the signing.Therefore, I've opted for being consistent with how it's done for the initial authorization/token exchange. On the start of every refresh token grant we set the KIDs in the
Sessionto correspond to the newest keys. This is not a perfect solution, but is a huge improvement over the current situation without making big changes. It goes from "guaranteed" breaking of all JWTs from existing refresh tokens to "very unlikely". Given the severity of this bug, I think it makes sense to first make a smaller fix and then follow up later with larger structural improvements.What do you think?