oauth2: add allow_failed_matcher for unauthenticated pass-through on auth failure#43027
oauth2: add allow_failed_matcher for unauthenticated pass-through on auth failure#43027mchen391 wants to merge 4 commits intoenvoyproxy:mainfrom
Conversation
…auth failure 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 feature enables graceful degradation patterns where services can handle both authenticated and unauthenticated requests, similar to the JWT filter's allow_missing_or_failed functionality. Implementation details: - Added allow_failed_matcher field to OAuth2Config proto (field 27) - The matcher is evaluated in the request flow after pass_through_matcher (which skips validation entirely) and before deny_redirect_matcher - OAuth validation is always attempted (unlike pass_through_matcher which skips it) - When OAuth validation fails AND the request matches allow_failed_matcher, all OAuth cookies are stripped from the request before forwarding to upstream - Context headers added: x-envoy-oauth-status: failed and x-envoy-oauth-failure-reason - Added new statistic oauth_allow_failed_passthrough to track when this path is taken - Updated filter logic in filter.cc and oauth_client.cc to handle the new matcher - Added comprehensive unit tests in filter_test.cc and oauth_test.cc The implementation follows the same architectural pattern as other matchers in the OAuth2 filter and maintains backward compatibility (optional field with no behavior change when not configured). Signed-off-by: Mo Chen <carterchen.m@gmail.com>
|
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. |
|
CC @envoyproxy/api-shepherds: Your approval is needed for changes made to |
| return absl::OkStatus(); | ||
| } else { | ||
| parent_->sendUnauthorizedResponse("Token endpoint cluster not found"); | ||
| return absl::UnavailableError("Token endpoint cluster not found"); |
There was a problem hiding this comment.
Although this error is thrown inside oauth_client, it is not an error thrown from the async context and as a result it cannot call parent_->asyncOnUnauthorized. To allow this sync failure and forward the request to upstream, it needs to go back to filter.cc and let it return FilterHeadersStatus::Continue.
|
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! |
Can't this be achieved with Custom Response? Here is an example using Envoy Gateway BackendTrafficPolicy. |
|
@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.
|
|
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. |
@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 |
|
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. |
|
/lgtm api |
| // | ||
| // Note: If a request matches pass_through_matcher, it bypasses OAuth validation and this matcher won't be evaluated. | ||
| // This matcher takes precedence over deny_redirect_matcher. | ||
| repeated config.route.v3.HeaderMatcher allow_failed_matcher = 27; |
There was a problem hiding this comment.
Should this matcher take precedence over deny_redirect_matcher?
deny_redirect_matcher won't redirect matching requests to the oauth server, which happens before auth fails/succeeds.
There was a problem hiding this comment.
Correct me if I'm wrong, but I think the filter works in this way:
- First of all, if
pass_through_matchermatches, skip the entire filter - Do auth on OAuth cookies, refresh token if needed. If auth succeeded, forward to upstream
- If auth failed (could be missing cookies, refresh failure, etc), it will either
a. redirect to OAuth server to start a new OAuth, or
b. Ifdeny_redirect_matchermatches, do not redirect and reject with a 401.
As a result, deny_redirect_matcher check happens after auth, and is determining what is the behavior when auth failed. I think this new allow_failed_matcher is in fact defining a new behavior after a & b:
c. If allow_failed_matcher matches, allow this auth failure and forward to upstream
So it's in fact in parallel to deny_redirect_matcher. Technically it's ok for either of them to take precedes over the other one, but I feel like c happens before b makes more sense. Please let me know if you think other way though!
| } | ||
|
|
||
| void OAuth2Filter::continueAsUnauthorized(const std::string& failure_reason) { | ||
| removeOAuthTokenCookies(*request_headers_); |
There was a problem hiding this comment.
unauthenticated requests shouldn’t have those cookies?
There was a problem hiding this comment.
Unauthenticated requests could be:
- A request without any oauth cookies
- A request with expired oauth cookies, especially an expired refresh token, which cannot be used to obtain a new access token any more
The clean up of the token cookies are for the 2nd case.
| bool OAuth2Filter::canRedirectToOAuthServer(Http::RequestHeaderMap& headers) const { | ||
| for (const auto& matcher : config_->allowFailedMatchers()) { | ||
| if (matcher->matchesHeaders(headers)) { | ||
| ENVOY_LOG(debug, "allow_failed_matcher matched, will not redirect"); |
There was a problem hiding this comment.
request matching allowFailedMatchers should be directed to the OAuth2 server to start the oauth flow.
There was a problem hiding this comment.
I actually intentionally let requests matching allowFailedMatchers to never initiate a new OAuth on its own by the filter. Basically if the filter fail to authenticate the request (that matches allowFailedMatchers) or any reasons, it should forward to upstream and let upstream to decide what to do with the unauthenticated request.
If we redirect to OAuth server to start a new oauth flow for these requests, the filter is still enforcing authentication.
This commit addresses a security vulnerability where malicious clients could inject x-envoy-oauth-status and x-envoy-oauth-failure-reason headers that might confuse downstream services about the authentication state of requests. Changes: - Sanitize OAuth status headers (x-envoy-oauth-status and x-envoy-oauth-failure-reason) for all incoming requests in decodeHeaders(), following the same pattern as Authorization header sanitization - Change from addCopy() to setCopy() when setting these headers in continueAsUnauthorized() to ensure headers are replaced rather than appended, providing defense-in-depth - Add test coverage for header injection scenarios in both authenticated and allow_failed code paths The sanitization ensures that only the OAuth2 filter itself can set these headers, preventing header injection attacks at system boundaries. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com> Signed-off-by: Mo Chen <carterchen.m@gmail.com>
# Conflicts: # changelogs/current.yaml Signed-off-by: Mo Chen <carterchen.m@gmail.com>
The previous merge incorrectly reordered changelog entries alphabetically, causing them to appear as additions in the PR diff. This commit fixes the merge by: - Keeping all main branch entries in their exact original order - Appending the oauth2 allow_failed_matcher entry at the end This minimizes diff noise and makes the PR review clearer. Signed-off-by: Mo Chen <carterchen.m@gmail.com>
|
@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! |
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:
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:
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:]