oauth2: add allow_failed_matcher for unauthenticated pass-through on auth failure

[mchen391]

Commit Message:

This PR adds an allow_failed_matcher field to the OAuth2 filter configuration, enabling requests to proceed to upstream as unauthenticated when OAuth validation fails (missing, invalid, or expired credentials) and the request matches the configured matchers.

This enables graceful degradation patterns where services can handle both authenticated and unauthenticated requests, similar to the JWT filter's allow_missing_or_failed functionality.

When triggered:

• All OAuth cookies are stripped from the request
• Context headers x-envoy-oauth-status: failed and x-envoy-oauth-failure-reason are added
• Request continues to upstream as unauthenticated

Matcher evaluation order: pass_through_matcher > allow_failed_matcher > deny_redirect_matcher > default behavior.

Signed-off-by: Mo Chen carterchen.m@gmail.com

Additional Description:

Use case: An API that returns personalized results for authenticated requests but falls back to generic results for unauthenticated requests.

Example configuration:

allow_failed_matcher:
  - name: ":path"
    string_match:
      prefix: "/api/public"

Risk Level: Medium

Testing:
Added unit tests covering multiple scenarios (no cookies, invalid refresh token, async/sync failures, OAuth callback path, matcher precedence). All existing OAuth2 tests pass. Verified backward compatibility.

Docs Changes: Updated proto documentation for allow_failed_matcher field. Added release notes to changelogs/current.yaml.

Release Notes: Added to changelogs/current.yaml

Platform Specific Features: N/A

[Optional Runtime guard:] N/A - Config-guarded feature (optional proto field)
[Optional Fixes #Issue] Fixes #36523
[Optional API Considerations:]

• New optional field allow_failed_matcher (field 27) to OAuth2Config proto
• Backward compatible - no behavior change when not configured
• Uses existing HeaderMatcher type, follows same pattern as other matchers in OAuth2Config

[repokitteh-read-only]

Hi @mchen391, welcome and thank you for your contribution.

We will try to review your Pull Request as quickly as possible.

In the meantime, please take a look at the contribution guidelines if you have not done so already.

🐱

Caused by: #43027 was opened by mchen391.

see: more, trace.

[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 @markdroth
CC @envoyproxy/api-watchers: FYI only for changes made to (api/envoy/|docs/root/api-docs/).

🐱

Caused by: #43027 was opened by mchen391.

see: more, trace.

[tyxia]

Hi @zhaohuabing, I saw you contributed a lot to this area in the past. Could we borrow your expertise to review this PR

cc @wbpcode as code owner as well

Thanks!

[zhaohuabing]

Use case: An API that returns personalized results for authenticated requests but falls back to generic results for unauthenticated requests.

Can't this be achieved with Custom Response?

Here is an example using Envoy Gateway BackendTrafficPolicy.

[mchen391]

@zhaohuabing Unless the local reply is able to forward the request to upstream, no unfortunately. Imagine the homepage of a website - it will render normally for anonymous users, but will render tailored content for logged-in users too. Or imagine a search engine API that gives anonymous users result (likely from DB) but also gives tailored result to users with credentials. As a result, we need the request that does not carry any oauth credentials to still forward to upstream and let upstream handles the request.

[zhaohuabing]

I think we could redirect unauthenticated requests. Since the OAuth2 filter is already fairly complex, it might be better to handle this in a more generalized way rather than adding another knob to the OAuth2 filter.

https://github.com/zhaohuabing/playground/blob/main/gateway/oidc-keycloak/response-override-redirect.yaml

[mchen391]

I think we could redirect unauthenticated requests. Since the OAuth2 filter is already fairly complex, it might be better to handle this in a more generalized way rather than adding another knob to the OAuth2 filter.

https://github.com/zhaohuabing/playground/blob/main/gateway/oidc-keycloak/response-override-redirect.yaml

@zhaohuabing Sorry I still don't think this would solve the problem. In this case we do NOT want to show the anonymous user an error page, but instead want to show them a normal content but just not tailored for them since they are anonymous.

Just imagine the google homepage - you will be able to load google and use its search engine without login completely fine, but can also do that in a logged-in state, with maybe some tailored search result sorting if you search in logged-in state.

I don't see a clean way to achieve that with redirects. One thing I can think of is to make a duplicate set of all the APIs that are supposed to be optional auth (could work with or without auth), one require auth and one does not require, and make the former redirect to the latter if auth failed. You could imagine that there are going to tons of endpoints that need to be duplicated and maintained separately, which I don't think is ideal or maintenable.

In fact I think the Envoy jwt_authn filter's allow_missing_or_failed option is also designed with this requirement in mind. https://www.envoyproxy.io/docs/envoy/latest/api-v3/extensions/filters/http/jwt_authn/v3/config.proto#envoy-v3-api-field-extensions-filters-http-jwt-authn-v3-jwtrequirement-allow-missing-or-failed

[zhaohuabing]

Thanks for the clarification — I don’t think you necessarily need a full duplicate setup, but it's true that you’d still need some separate routing or handling logic for unauthenticated vs authenticated requests, which would require changes on the application side and isn’t ideal. In that case, having a pass-through option is really helpful. So this approach seems reasonable to me.

[markdroth]

/lgtm api

[mchen391]

@zhaohuabing, sorry for the ping, but I could use another look at the PR. Changes have been made to the header sanitization per the suggestions, though there are a few other comments I think are worth discussing. Thank you!

[theon73]

Friendly bump on this in case anyone has had a chance to look since the last ping.
I’m helping cover while @mchen391 is away. Feedback appreciated, thanks.

[botengyao]

friendly ping @zhaohuabing

[zhaohuabing]

Hi folks, Sorry for the delay — I’ll review this sometime this week.

[mchen391]

Sorry. I think we need to re-evaluate the whole implementation. Note, the onFailure may be called directly in sometimes. Then, call the continueDecoding() in the failure callback will result in unexpected behavior. We must to take it into account.

cc @zhaohuabing

Ok in fact when investigating this issue I found that not only onFailure but also onSuccess can be called directly inside send(), so calling continueDecoding in onSuccess is not safe either.

I've pushed a fix which I think is the most straightforward and easy solution on top of my head. In short, onSuccess and onFailure will now check if it is called before the async request is dispatched (should not call continueDecoding), or after it is dispatched (should call continueDecoding). Please refer to the two comments I left on the code for more details.

Happy to learn and discuss if y'all have any better and cleaner solution to this. Thank you for bringing this up!

cc @zhaohuabing @wbpcode

[wbpcode]

Ok in fact when investigating this issue I found that not only onFailure but also onSuccess can be called directly inside send(), so calling continueDecoding in onSuccess is not safe either.

This should never happen in practice 🤔 ?

[mchen391]

Ok in fact when investigating this issue I found that not only onFailure but also onSuccess can be called directly inside send(), so calling continueDecoding in onSuccess is not safe either.

This should never happen in practice 🤔 ?

I'm not that familiar with the entire infra but there's actually a test around calling onSuccess() for unhealthy upstream (here). Based on my investigation (with claude):

• send() will call onSuccess() with response status being 5xx for unhealthy upstream
• send() will call onFailure() if network is completely unreachable

Either way, I made onSuccess() safe to call in both sync/async context as well as onFailure() just in case it ever hits that pattern.

[wbpcode]

I'm not that familiar with the entire infra but there's actually a test around calling onSuccess() for unhealthy upstream (here). Based on my investigation (with claude):

This should be a test error. The onSuccess() should never be called synchronously

[wbpcode]

LGTM overall to me now. Thanks for the update.

[wbpcode]

/wait

[mchen391]

@wbpcode thanks for another review, please let me know if there's anything I need to do to move forward! Thank you!

[markdroth]

/lgtm api

[wbpcode]

LGTM. Thanks for this great feature.

[mchen391]

/retest

[mchen391]

CI failed due to test coverage low. Added a few new tests in filter_test

[mchen391]

/retest