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

Add API route config #1760

Merged
merged 7 commits into from
Sep 11, 2022
Merged

Conversation

segfault16
Copy link
Contributor

@segfault16 segfault16 commented Aug 17, 2022

Description

In addition to requests with Accept header application/json return 401 instead of 302 to login page on requests matching API paths regex.

Motivation and Context

Several APIs like grpcweb or graphql don't use application/json. See #551

How Has This Been Tested?

Extended unit tests

Checklist:

  • My change requires a change to the documentation or CHANGELOG.
  • I have updated the documentation/CHANGELOG accordingly.
  • I have created a feature (non-master) branch for my PR.

In addition to requests with Accept header `application/json` return 401 instead of 302 to login page on requests matching API paths regex.
@segfault16 segfault16 requested a review from a team as a code owner August 17, 2022 06:57
@segfault16 segfault16 mentioned this pull request Aug 19, 2022
3 tasks
Copy link
Member

@JoelSpeed JoelSpeed left a comment

Choose a reason for hiding this comment

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

This looks great, couple of nits but otherwise LGTM, thanks for putting this together


if tc.shouldRedirect {
assert.Equal(t, 302, rw.Code)
// assert.Equal(t, "Allowed Request", rw.Body.String())
Copy link
Member

Choose a reason for hiding this comment

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

Do we need this line?

@@ -96,6 +96,8 @@ func Validate(o *options.Options) error {
})
}

msgs = append(msgs, validateApiRoutes(o)...)
Copy link
Member

Choose a reason for hiding this comment

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

Can you move this up to the top of the function, I don't think we need to have it down here right?

JoelSpeed
JoelSpeed previously approved these changes Sep 11, 2022
Copy link
Member

@JoelSpeed JoelSpeed left a comment

Choose a reason for hiding this comment

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

LGTM thanks

@JoelSpeed
Copy link
Member

Please resolve the linter issues

@JoelSpeed JoelSpeed dismissed their stale review September 11, 2022 13:59

Linting has failed

@JoelSpeed JoelSpeed merged commit 965fab4 into oauth2-proxy:master Sep 11, 2022
msaf1980 pushed a commit to msaf1980/oauth2-proxy that referenced this pull request Sep 27, 2022
* Add API route config

In addition to requests with Accept header `application/json` return 401 instead of 302 to login page on requests matching API paths regex.

* Update changelog

* Refactor

* Remove unnecessary comment

* Reorder checks

* Lint Api -> API

Co-authored-by: Sebastian Halder <[email protected]>
@beezerk23
Copy link

Hey, is there a plan on when to release this in some patch version? Im using the docker image and not sure how to use the master branch directly. Any help is highly appreciated

@JoelSpeed
Copy link
Member

Release should come soon, need to find the time to get the release together

@johnswarbrick-napier
Copy link

johnswarbrick-napier commented Nov 9, 2022

Hi @segfault16, please could you confirm what the format of the --api-route configuration option is?

I have a number of paths that I want to return a 401 Unauthorized if a bearer token is not present instead of issuing a 302 redirect to the OIDC provider.

I'm applying the following in my yaml:

spec:
  containers:
    - args:
      - --api-route="^(/api|admin/_api)"

And the parameters are confirmed in the logs:

[2022/11/09 20:37:46] [oauthproxy.go:496] API route - Path: ^(/api|admin/_api)

But when I try with a curl I'm still getting the 302 redirect instead of a 401:

curl -i https://<domain>/admin/_api

HTTP/1.1 302 Moved Temporarily
Date: Wed, 09 Nov 2022 20:40:28 GMT
Content-Type: text/html
Content-Length: 138
Connection: keep-alive
Location: https://<domain>/oauth2/start?rd=%2Fadmin%2F_api

I've tried various variations on the structure of the path_regex but it doesn't seem to want to match.

@segfault16
Copy link
Contributor Author

@johnswarbrick-napier Your regex doesn't match. You probably mean this: ^(/api|/admin/_api)
Notice the leading /.

@johnswarbrick-napier
Copy link

johnswarbrick-napier commented Nov 10, 2022

Sorry, that was a typo in my comment.

spec:
  containers:
    - args:
      - --api-route="^(/api|/admin/_api)"
[2022/11/10 13:14:17] [oauthproxy.go:496] API route - Path: ^(/api|/admin/_api)
curl -i https://<domain>/admin/_api

HTTP/1.1 302 Moved Temporarily
Date: Thu, 10 Nov 2022 13:15:53 GMT
Content-Type: text/html
Content-Length: 138
Connection: keep-alive
Location: https://<domain>/oauth2/start?rd=%2Fadmin%2F_api

I tried simplifying it with a single path and these variants but still get the 302 redirection:

[2022/11/10 13:17:35] [oauthproxy.go:496] API route - Path: ^/admin/_api
[2022/11/10 13:19:31] [oauthproxy.go:496] API route - Path: .*/admin/_api
[2022/11/10 13:24:32] [oauthproxy.go:496] API route - Path: /admin/_api

@johnswarbrick-napier
Copy link

Hi @segfault16. Any thoughts on why even a simple path in --api-route doesn't seem to result in a change from 301 Redirect to 401 Unauthorized? I put some examples in my previous comment.

I assume the path regex isn't matching but I cannot figure out why.

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

Successfully merging this pull request may close these issues.

4 participants