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.

Sign up for GitHub

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

Jump to bottom

Add an option to disable nonce in the OAuth2 filter #37049

Closed
zhaohuabing opened this issue Nov 8, 2024 · 3 comments · Fixed by #37473
Closed

Add an option to disable nonce in the OAuth2 filter #37049

zhaohuabing opened this issue Nov 8, 2024 · 3 comments · Fixed by #37473
Assignees
@zhaohuabing
Labels
area/oauth enhancement Feature requests. Not bugs or questions. no stalebot Disables stalebot from closing an issue

Comments

@zhaohuabing
Copy link
Member

cognito login page doesn't handle state value properly and the nonce doesn't stay in the state but gets raised directly to the url parameter and when coming back to callback nonce has disappeared from the state. In their documentation they do not support urlencoded json state values (and I guess this extends to any format, json just so common that it was mentioned).

xref: #36871
@zhaohuabing zhaohuabing added enhancement Feature requests. Not bugs or questions. triage Issue requires triage labels Nov 8, 2024
@zhaohuabing
Copy link
Member Author

/assign

@zhaohuabing zhaohuabing mentioned this issue Nov 8, 2024
Closed
@KBaichoo
Copy link
Contributor

KBaichoo commented Nov 8, 2024

cc @derekargueta

@KBaichoo KBaichoo added area/oauth and removed triage Issue requires triage labels Nov 8, 2024
@zhaohuabing zhaohuabing mentioned this issue Dec 4, 2024
Merged
@github-actions GitHub Actions
Copy link

github-actions bot commented Dec 9, 2024

This issue has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in the next 7 days unless it is tagged "help wanted" or "no stalebot" or other activity occurs. Thank you for your contributions.

@github-actions github-actions bot added the stale stalebot believes this issue/PR has not been touched recently label Dec 9, 2024
@KBaichoo KBaichoo added no stalebot Disables stalebot from closing an issue and removed stale stalebot believes this issue/PR has not been touched recently labels Dec 9, 2024
zhaohuabing added a commit to zhaohuabing/envoy that referenced this issue Dec 13, 2024
@zhaohuabing @wbpcode @phlax
Fix: oauth2 state encoding (envoyproxy#37473)
a7df0d1 
Commit Message:
* Changes the state pramater to a json object to store the user state
before redirecting the user request to the auth server. Currently, the
json object has two fields: the original url and a nonce for csrf
prevention.
* Changes the state econding to
[Base64URL](https://datatracker.ietf.org/doc/html/rfc4648#section-5) to
fix [ AWS cognito doesn't support url encoded state
value](https://docs.aws.amazon.com/cognito/latest/developerguide/authorization-endpoint.html#get-authorize-request-parameters)
* Change the nonce from a plain timestamp to a random number to enhance
its security robustness. This addresses the "1. Generation" in this
issue envoyproxy#36276
* Small refacor: rename variables related to token_secret to hmac_secret
to improve clarity and consistency.

Example:
original state:
`{"url":"https://localhost:8080/login","nonce":"IPOom6PfIoFS+MmiV04aTJai8vUYlzyO5zUgT2G8mZA="}`
base64url encoded state:
`eyJ1cmwiOiJodHRwczovL2xvY2FsaG9zdDo4MDgwL2xvZ2luIiwibm9uY2UiOiJJUE9vbTZQZklvRlMrTW1pVjA0YVRKYWk4dlVZbHp5TzV6VWdUMkc4bVpBPSJ9`

Additional Description: The nonce in the [state parameter
](https://datatracker.ietf.org/doc/html/rfc6749#section-10.12)is used
for csrf prevention and is applicable for both oauth2 and oidc. Please
note that the OIDC spec defines a seperate [nonce
parameter](https://openid.net/specs/openid-connect-core-1_0.html#AuthRequest)
,which is specifically designed to prevent replay attacks and is unique
to OIDC.

More discussion about state and nonce parameters and be found in this
comment:
envoyproxy#37050 (comment)

Risk Level:
Testing: Unit test and integration test
Docs Changes:
Release Notes: Yes
Platform Specific Features:
[Optional Runtime guard:] A runtime gurad
"envoy.reloadable_features.oauth2_enable_state_nonce" has been added for
the new nonce in the state parameter.
[Optional Fixes envoyproxy#37049 envoyproxy#36871]
[Optional Fixes commit #PR or SHA]
[Optional Deprecated:]
[Optional [API
Considerations](https://github.com/envoyproxy/envoy/blob/main/api/review_checklist.md):]

Related Envoy Gateway issue:
envoyproxy/gateway#4625

I've tested this PR against AWS cognito using Envoy Gateway
SecurityPolicy, and it worked.

cc @missBerg @arkodg

---------

Signed-off-by: Huabing Zhao <[email protected]>
Signed-off-by: code <[email protected]>
Co-authored-by: code <[email protected]>
Co-authored-by: phlax <[email protected]>
(cherry picked from commit 0c6a1d2)
Signed-off-by: Huabing Zhao <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@zhaohuabing zhaohuabing

Labels
area/oauth enhancement Feature requests. Not bugs or questions. no stalebot Disables stalebot from closing an issue
Projects
None yet
Milestone
No milestone
2 participants
@zhaohuabing @KBaichoo