Token respose does not always include nonce

[Darkrael]

While upgrading to Keycloak version >25 , i've noticed, that exchanging the refresh token for a new access token and extracting the claims, the token verification fails due to the nonce not being included in refresh request responses anymore. This is due to Keycloak now following the OpenID Connect Core 1.0 specification recommendation to not include the nonce in a refresh request: https://www.keycloak.org/docs/26.0.4/upgrading/#nonce-claim-is-only-added-to-the-id-token. This can be "fixed" by adding the Nonce backwards compatible to the Keycloak configuration for the client, but i think it should be possible to get the claims without checking the nonce, which i think is not possible at the moment unless i've overlooked something.

[ramosbugs]

Hey @Darkrael, thanks for reporting this issue. The relevant portion of the spec is:

it SHOULD NOT have a nonce Claim, even when the ID Token issued at the time of the original authentication contained nonce; however, if it is present, its value MUST be the same as in the ID Token issued at the time of the original authentication

This crate's IdToken::claims method accepts a generic NonceVerifier trait when verifying the ID token and extracting the claims. I think it probably makes sense to add an IgnoreNonceForTokenRefresh struct to this crate that implements NonceVerifier and ignores the nonce. Users can pass this struct when reading ID tokens returned via a token refresh. Does that sound reasonable?

[Darkrael]

This would certainly allow users to handle refreshes correctly, but it would not be immediatly clear to people that this should be used. It would make more sense to me that this would be the "default" behavior. However, if i understand the code correctly, there is no difference between a token gained through refresh and a token gained through the initial authentication flow, where the nonce must be given. Maybe the IdToken could have a property to store if it was gained through refresh or through the initial authentication flow. The "default" verifier (meaning the one provided by the Client) could then take this into account when verifying the token and ignore an empty nonce for refresh token. Since i don't know the code at all apart from quickly scrolling though, i don't know if this would be feasable to implement. Otherwise the suggested solution together with a small note in the claims function to use the other verifier for refresh tokens should be good.

[ramosbugs]

if i understand the code correctly, there is no difference between a token gained through refresh and a token gained through the initial authentication flow, where the nonce must be given.

that's right. the provenance of the ID token isn't stored currently. I'll have to think about the best way to represent this in the type system, but that will probably be a more involved change.

I think for now, I'll introduce the IgnoreNonceForTokenRefresh nonce verifier and mention in the IdToken::claims docs to use it for ID tokens that came from a token refresh. I may also add an example.

[ramosbugs]

oh, I forgot NonceVerifier is also implemented for FnOnce(Option<&Nonce>) -> Result<(), String>. this means users can just do:

id_token.claims(&id_token_verifier, |_| Ok(()))

In that case, I think I'll just mention this in the docs for that method instead of adding a dedicated struct, which might be more likely to be misused for non-refresh tokens (a security vulnerability).

[jakmeier]

that's right. the provenance of the ID token isn't stored currently. I'll have to think about the best way to represent this in the type system, but that will probably be a more involved change.

@ramosbugs , thanks for your work! Just checking, do you plan to handle this inside openidconnect, or is this something that library users should take care of as shown in your last comment? I don't think it has been updated in the docs, yet. So, I'm not sure what the plan is here and I did hit this and had to debug a bit until I found this issue.