Skip to content
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

Allow empty nonce from ID Tokens issued from Refresh Tokens #509

Open
1 of 2 tasks
jkroepke opened this issue Jan 5, 2024 · 9 comments
Open
1 of 2 tasks

Allow empty nonce from ID Tokens issued from Refresh Tokens #509

jkroepke opened this issue Jan 5, 2024 · 9 comments

Comments

@jkroepke
Copy link
Contributor

jkroepke commented Jan 5, 2024

Preflight Checklist

  • I could not find a solution in the existing issues, docs, nor discussions
  • I have joined the ZITADEL chat

Describe your problem

I have the issue that ID Token from Microsoft Entra ID issued via an Refresh Token may have a nonce claim. According to OIDC Spec, such token should not contain an nonce claim (The clarification on SPEC introduced 14 months ago; https://bitbucket.org/openid/connect/pull-requests/341)

The IDToken verifier includes an non optional claim which fails in my scenria, if I to a RefreshToken against Microsoft Entra ID I got this error:

expected "" but was "H1HIv-[redacted]-PneVxio4"

Describe your ideal solution

Since ID Tokens request by an RefreshToken should not contains a nonce anyways (technically, it's not possible to define one from client site and its defined in SPEC), I prefer to disable the nonce validation in such cases.

Version

3.8.1

Environment

Self-hosted

Additional Context

I'm aware that Microsoft should fix the logic in Entra ID, but I feel this will not happen in next moths...

@hifabienne
Copy link
Member

Thank you for sharing your idea.
If there is a significant demand from customers/community, we will carefully consider implementing the feature.
Currently, the issue will be added to our product backlog.

Meanwhile, if you're interested in implementing it yourself, we also welcome pull requests.

@muhlemmer
Copy link
Collaborator

muhlemmer commented Jan 5, 2024

The standard is a bit fishy on this.

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

Disable is indeed the easy solution, but not necessary the correct one. If we want to be 100% correct we should verify the nonce against the original ID Token, if it was returned with the new ID token. So in case of Entra ID you could work-around the issue by constructing the RP with the WithNonce verrifier option, passed to WithVerifierOpts. Then, when calling RefreshTokens use context Values to set and obtain the Nonce from the original token.

@jkroepke
Copy link
Contributor Author

jkroepke commented Jan 5, 2024

The standard also says (Section 12.2)

12.2. Successful Refresh Response
Upon successful validation of the Refresh Token, the response body is the Token Response of Section 3.1.3.3 except that it might not contain an id_token.
...
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

My problem is according the standard, its compliant to omit the nonce claim, too. In this case, I need a verifier which accepts both: an empty string and the nonce claim from the original ID Token.

In my case, I support any OIDC provider and I have different results from Entra ID. While on my test, the nonce is empty, other users experience that error that nonce is non-empty.

Would it be an option to check the nonce only, if its non-empty? That would be 100% correct, because it says check nonce only, if present.

@jkroepke jkroepke changed the title Disable nonce validation on ID Tokens issued from Refresh Tokens Allow empty nonce from ID Tokens issued from Refresh Tokens Jan 7, 2024
@jkroepke
Copy link
Contributor Author

jkroepke commented Jan 7, 2024

I also tried something like this:

    ctx = context.WithValue(ctx, types.CtxNonce{}, "value")
	newTokens, err := rp.RefreshTokens[*oidc.IDTokenClaims](ctx, relyingParty, refreshToken, "", "")
	if errors.Is(err, oidc.ErrNonceInvalid) {
		ctx = context.WithValue(ctx, types.CtxNonce{}, "")
		newTokens, err = rp.RefreshTokens[*oidc.IDTokenClaims](ctx, relyingParty, refreshToken, "", "")
	}

but it fails, if the Provider has a one time usage policy for refresh tokens.

@jkroepke
Copy link
Contributor Author

jkroepke commented Feb 1, 2024

@muhlemmer what did you think about empty nonce value is fine on refresh?

@muhlemmer
Copy link
Collaborator

Sorry lost track of this issue. With the above arguments you gave I agree that we can allow an empty nonce. Do you want to send a PR for that?

@muhlemmer muhlemmer added the auth label Feb 2, 2024
@jkroepke
Copy link
Contributor Author

jkroepke commented Feb 2, 2024

With the above arguments you gave I agree that we can allow an empty nonce

Its spec compliant. If you read (Section 12.2):

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

which I interpret: claims from refresh response should be empty/non present. If not empty, the values should be equal to the initial value.

If you would agree here, I would open a PR

@muhlemmer
Copy link
Collaborator

Yes, I wrote I agree ;)

@jkroepke
Copy link
Contributor Author

jkroepke commented Feb 8, 2024

I looked into it and since the ValidToken function does not know the context of the token source (comming from CodeExchange or Refresh), I have no clue how I could integrate a condition without changing the signature

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: 📨 Product Backlog
Development

No branches or pull requests

3 participants