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

Add state and nonce verification in OIDC #345

Merged
merged 2 commits into from
May 27, 2024

Conversation

oneonestar
Copy link
Member

Description

Fix #339
Depends on #343
Add state and nonce verification and integration test in OIDC.

Implementation details

State

Ref: https://developers.google.com/identity/openid-connect/openid-connect#server-flow
Correct implementation of OIDC:

  1. Create an anti-forgery state token (implemented by this PR using OidcCookie)
  2. Send an authentication request to Google
  3. Confirm the anti-forgery state token (implemented by this PR using state)
  4. Exchange code for access token and ID token
  5. Obtain user information from the ID token
  6. Authenticate the user

Nonce

Ref: https://openid.net/specs/openid-connect-core-1_0-17_orig.html#NonceNotes

The nonce parameter value needs to include per-session state and be unguessable to attackers. One method to achieve this for Web Server Clients is to store a cryptographically random value as an HttpOnly session cookie and use a cryptographic hash of the value as the nonce parameter. In that case, the nonce in the returned ID Token is compared to the hash of the session cookie to detect ID Token replay by third parties.

Implemented by this PR using nonce, hashNonce() and OidcCookie.

Release notes

( ) Release notes are required, with the following suggested text:

* 

@cla-bot cla-bot bot added the cla-signed label May 15, 2024
@oneonestar oneonestar force-pushed the star/oidc_add_state_nonce branch 2 times, most recently from 4d35148 to 09ddc86 Compare May 20, 2024 13:54
Copy link
Contributor

@willmostly willmostly left a comment

Choose a reason for hiding this comment

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

Minor comments. By my analysis this implements the logic referenced from https://developers.google.com/identity/openid-connect/openid-connect#server-flow, so I think this can be merged once comments are addressed.

Copy link
Member

@mosabua mosabua left a comment

Choose a reason for hiding this comment

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

Thats quite the complex setup for functionality and testing. From my perspective this looks good though, since OIDC just is tricky.

@mosabua mosabua merged commit d83215f into trinodb:main May 27, 2024
2 checks passed
@github-actions github-actions bot added this to the 10 milestone May 27, 2024
@oneonestar oneonestar deleted the star/oidc_add_state_nonce branch May 28, 2024 01:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

OIDC login implementation isn't up to standard
3 participants