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

api(oidc): adds support in OIDC api to optionally deny redirects #5455

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

sam-burrell
Copy link
Contributor

@sam-burrell sam-burrell commented Mar 10, 2025

What this PR does / why we need it:
Adds support in the OIDC api to optionally deny redirects that match a header/:path/:method.

The intention is to then implement this with the underlying envoy oauth2 configuration deny_redirect_matcher
This is useful for ajax/machine paths that should not be directed to the authentication flow.

I have the changes ready for a second pull request to implement this if accepted.

Which issue(s) this PR fixes:
Could be used as fix for? 2496

Release Notes: No

@sam-burrell sam-burrell requested a review from a team as a code owner March 10, 2025 10:47
Copy link

codecov bot commented Mar 10, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 65.27%. Comparing base (d8d3ca4) to head (8549b31).

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #5455      +/-   ##
==========================================
+ Coverage   65.24%   65.27%   +0.03%     
==========================================
  Files         213      213              
  Lines       33988    33988              
==========================================
+ Hits        22175    22186      +11     
+ Misses      10485    10475      -10     
+ Partials     1328     1327       -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@sam-burrell
Copy link
Contributor Author

/retest

@zhaohuabing
Copy link
Member

@sam-burrell Thanks for adding this useful feature. Overall, the API looks good. I just have a few nits.

@sam-burrell
Copy link
Contributor Author

Thanks @zhaohuabing for the feedback. I've made some (hopeful) improvements!

@sam-burrell
Copy link
Contributor Author

/retest

@sam-burrell
Copy link
Contributor Author

Do I wait for this to be merged before I do a pull request for the actual underlying code change?

@arkodg
Copy link
Contributor

arkodg commented Mar 13, 2025

@sam-burrell will this make #5142 redundant for the machine case ?

@zhaohuabing
Copy link
Member

@sam-burrell will this make #5142 redundant for the machine case ?

@arkodg I think they're used for different purposes:

They may be used together for non-human requests.

@sam-burrell
Copy link
Contributor Author

I agree with @zhaohuabing this is a slightly different usecase. This is mainly for non human requests

…irects based on string matching headers/paths/methods...

Signed-off-by: sam-burrell <sam.burrell@gmail.com>
Signed-off-by: sam-burrell <sam.burrell@gmail.com>
Signed-off-by: sam-burrell <sam.burrell@gmail.com>
@arkodg
Copy link
Contributor

arkodg commented Mar 15, 2025

hey @sam-burrell had a chat with @zhaohuabing offline, who mentioned that this could be solved with #5142 + a jwt section in the SecurityPolicy authenticating the token.
Would that work for your use case ?

@sam-burrell
Copy link
Contributor Author

Thanks for your thoughts all.

Thinking about this #5142 could work if we are able to make different httproutes for the same hostname.

We would like to deny redirects by path.
The use case I am considering is the following

https://app.foo/api -> we want 401 (no redirect)
https://app.foo/* -> we want redirects to normal login flow

Would #5142 work for this?

@zhaohuabing
Copy link
Member

zhaohuabing commented Mar 20, 2025

Thought again, #5142 may not be able to solve this use case. @arkodg @sam-burrell

Let's say all AJAX requests have a header is-ajax-request, and the "normal" user requests don't have this header, and we want to avoid redirection for AJAX requests.

How could we achieve this with passThroughAuthHeader ?

This configuration only allows requests with Authorization header passthrough without OIDC, it won't work for AJAX requests without tokens.

    oidc:
      provider:
        issuer: "https://oauth.foo.com"
        authorizationEndpoint: "https://oauth.foo.com/oauth2/v2/auth"
        tokenEndpoint: "https://oauth.foo.com/token"
      clientID: "client.oauth.foo.com"
      clientSecret:
        name: "client-secret-1"
      redirectURL: "https://www.example.com/oauth2/callback"
      passThroughAuthHeader: true
    jwt:
      providers:
      - name: example1
        issuer: https://oauth.foo.com
        remoteJWKS:
          uri: https://oauth.foo.com/jwt/public-key/jwks.json

This configuration can skip OIDC for AJAX request without token, but the requests with token will be blocked since the token is not in the is-ajax-request header.

   oidc:
      provider:
        issuer: "https://oauth.foo.com"
        authorizationEndpoint: "https://oauth.foo.com/oauth2/v2/auth"
        tokenEndpoint: "https://oauth.foo.com/token"
      clientID: "client.oauth.foo.com"
      clientSecret:
        name: "client-secret-1"
      redirectURL: "https://www.example.com/oauth2/callback"
      passThroughHeaders: true
    jwt:
      providers:
      - name: example1
        issuer: https://oauth.foo.com
        remoteJWKS:
          uri: https://oauth.foo.com/jwt/public-key/jwks.json
        extractFrom:
          headers:
          - name: is-ajax-request

If we change the API of passThroughAuthHeader from bool to a list of headers, this may work:

   oidc:
      provider:
        issuer: "https://oauth.foo.com"
        authorizationEndpoint: "https://oauth.foo.com/oauth2/v2/auth"
        tokenEndpoint: "https://oauth.foo.com/token"
      clientID: "client.oauth.foo.com"
      clientSecret:
        name: "client-secret-1"
      redirectURL: "https://www.example.com/oauth2/callback"
      passThroughAuthHeader:
      - Authorization
      - is-ajax-request
    jwt:
      providers:
      - name: example1
        issuer: https://oauth.foo.com
        remoteJWKS:
          uri: https://oauth.foo.com/jwt/public-key/jwks.json
        extractFrom:
          headers:
          - name: Authorization

Compared with this approach, I prefer a standalone DenyRedirectMatcher API for the followings reasons:

  • The current passThroughAuthHeader bool API is more intuitive and avoids duplication within OIDC and JWT sections.
  • DenyRedirectionMatcher can work without JWT.

@sam-burrell
Copy link
Contributor Author

@zhaohuabing yup agreed, I guess this is the reason DenyRedirectMatcher is there in the underlying envoy oauth2 configuration

// This behavior can be useful for AJAX or machine requests.
// +optional
// +notImplementedHide
DenyRedirectMatcher []OIDCDenyRedirectMatcher `json:"denyRedirectMatcher,omitempty"`
Copy link
Contributor

Choose a reason for hiding this comment

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

can we make this look more Gateway API like ?
one suggestion

denyRedirect:
  headers:
  - name: foo
     value: bar

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants