Skip to content

Conversation

@bobcallaway
Copy link
Member

Summary:

This change hardens the OIDC authentication process by implementing a cryptographically secure nonce (number used once) in the WebOidcClient.

Key Changes:

  • Nonce Generation: Introduces a generateNonce() method that creates a random, URL-safe string using SecureRandom.
  • Request Binding: Updates the authorization flow to include this nonce in the initial authorization request URL.
  • Validation: Adds strict validation logic to verify that the nonce claim in the returned ID token matches the original nonce sent by the client.
  • Testing: Adds WebOidcClientNonceTest to verify that tokens with correct nonces are accepted and tokens with mismatched or missing nonces are rejected.

Security Benefit:

This mechanism is a critical defense against Replay Attacks. By binding the ID token to the specific client request that initiated it, we ensure that an attacker cannot capture a valid ID token from a previous session and "replay" it to impersonate a user. If the nonces do not match, the token is immediately rejected.

Copy link
Member

@loosebazooka loosebazooka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks fine, gonna run some tests locally just to double check the web flow still works.

@loosebazooka
Copy link
Member

ignore the example test failures. also please run ./gradlew spApp to apply formatting

@loosebazooka
Copy link
Member

yeah locally weboidc seems to pass with google and github accounts.

Signed-off-by: Bob Callaway <[email protected]>
Copy link
Member

@jku jku left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this should work -- nonce is optional but should always be returned in the response if it is included in the request.

That said state is marked as recommended for the authorization code flow and seems to be for a similar purpose so this could use state instead. The result seems about the same but maybe that requires less work since AuthorizationCodeRequestUrl has a setState() constructor chain method (I can't quite see how that all works so could be wrong and it's not easier at all)

@loosebazooka loosebazooka merged commit a0693e6 into sigstore:main Jan 21, 2026
15 of 21 checks passed
@loosebazooka
Copy link
Member

I'm going to be migrating off the google-http-client to either something pluggable, or just apache-http-client5, so will revisit this a bit.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants