OAuth2: add a nonce to the state parameter

[zhaohuabing]

Commit Message: This PR adds a nonce to the state parameter of the oauth2 requests to mitigate the risk of CSRF.
Additional Description:
Risk Level: low
Testing: Unit and Integration test
Docs Changes: API docs: add a new oauth_nonce to the OAuth2Credentials.CookieNames proto API.
Release Notes: Yes
Platform Specific Features: No
[Optional Runtime guard:]
[Optional Fixes #Issue] Implement #35232 #29526
[Optional Fixes commit #PR or SHA]
[Optional Deprecated:]
[Optional API Considerations:]

The diagram below shows how nonce works within the oauth2 flow, with the red annotations highlighting the changes introduced in this PR.

[repokitteh-read-only]

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to (api/envoy/|docs/root/api-docs/).
envoyproxy/api-shepherds assignee is @wbpcode
CC @envoyproxy/api-watchers: FYI only for changes made to (api/envoy/|docs/root/api-docs/).

🐱

Caused by: #35919 was synchronize by zhaohuabing.

see: more, trace.

[phlax]

@zhaohuabing would you be able to fix the example first?

[zhaohuabing]

@zhaohuabing would you be able to fix the example first?

@phlax
Yes, I can, but some of the verifications in the example need to be temporarily disabled. This is because the content of the state parameter has changed, and the current OAuth2 filter's verification of the state differs from the new implementation. Once this PR is merged, I will re-enable those verifications in the examples.

[phlax]

hmm - was just looking - i guess my main concern is that the PR lands and breaks both repos

not sure if resolution is trivial - ie assuming the nonce is not predictable - might require some hackery on the responds_with_header fun

[zhaohuabing]

hmm - was just looking - i guess my main concern is that the PR lands and breaks both repos

not sure if resolution is trivial - ie assuming the nonce is not predictable - might require some hackery on the responds_with_header fun

Yes, the nonce is not predictable. But we can change the verification to just check the presence of the state, not the value of the nonce, like what we currently do to the auth code.

If this approach is acceptable, I will raise a PR to the example repository to fix the example accordingly.

[phlax]

could you raise the PR now, so its ready to go?

this will also need to be resolved for the fix to work - #35954

[repokitteh-read-only]

CC @envoyproxy/dependency-shepherds: Your approval is needed for changes made to (bazel/.*repos.*\.bzl)|(bazel/dependency_imports\.bzl)|(api/bazel/.*\.bzl)|(.*/requirements\.txt)|(.*\.patch).
envoyproxy/dependency-shepherds assignee is @phlax

🐱

Caused by: #35919 was synchronize by zhaohuabing.

see: more, trace.

[wbpcode]

Thanks for the contribution. A comment is added. I am not familiar to oauth2. So, may be we need to defer this code review to others. Or I need to take more time to read the oauth2 first.

/wait

[wbpcode]

if there a way to disable this new feature? Or is this has any possible side effect?

[zhaohuabing]

Enabling this by default enhances the security posture of the OAuth2 filter. This practice is also recommended by the OAuth2 spec: https://datatracker.ietf.org/doc/html/rfc6819#section-5.3.5. So my preference is that we enable this since this release and don't provide an option to disable it unless there is a particular reason asking for it.

There are no side effects, as the nonce is transparent to both the end users and the applications.

[wbpcode]

got it. Thanks.

[wbpcode]

/lgtm api

[zhaohuabing]

Thanks for the contribution. A comment is added. I am not familiar to oauth2. So, may be we need to defer this code review to others. Or I need to take more time to read the oauth2 first.

/wait

Hi @derekargueta @mattklein123 could you please help review this PR when you have a moment? I noticed that you're listed as owners of the OAuth2 filter in the CODEOWNERS file. Thank you so much for your time and assistance!