This reverts commit 9c61c49ec2.
The original fix broke nginx deployments using `auth_request`. When `/oauth2/auth` returns 302,
nginx's `auth_request` module treats this as an internal error:
[error] auth request unexpected status: 302 while sending to client
nginx then returns **500 Internal Server Error** to the browser.
> If the subrequest returns a 2xx response code, the access is allowed. If it returns 401 or 403,
> the access is denied with the corresponding error code. Any other response code returned by the
> subrequest is considered an error.
https://nginx.org/en/docs/http/ngx_http_auth_request_module.html
The nginx `auth_request` module has strict semantics (non-negotiable):
| Subrequest status | nginx behavior |
|---|---|
| 2xx | Allow request |
| 401 / 403 | Deny → trigger `error_page` |
| **Any other status** | **Internal error → 500** |
The `/oauth2/auth` endpoint is used as a **policy oracle** (yes/no decision),
not as a browser-facing endpoint. It cannot return redirects.
Any nginx deployment with:
- `skip-provider-button=true`
- Using `auth_request` directive
Will receive 500 errors instead of the expected authentication flow.
The correct fix for #334 is a **documentation update**, not a code change:
```nginx
error_page 401 = @oauth2_signin;
location @oauth2_signin {
return 302 /oauth2/sign_in?rd=$scheme://$host$request_uri;
}
```
This keeps `/oauth2/auth` as a pure 401/2xx oracle and lets nginx perform the proper 302 redirect to the browser.
- Original Issue: #334
- Regression introduced in PR: #3309
Signed-off-by: Stefan Markmann <stefan@markmann.net>
Signed-off-by: Jan Larwig <jan@larwig.com>
Co-authored-by: Jan Larwig <jan@larwig.com>
This commit is contained in:
Stefan Markmann
GitHub
parent
7bf586c898
commit
cf5d34acf6
|
|
@ -8,6 +8,8 @@
|
|||
|
||||
## Changes since v7.14.1
|
||||
|
||||
- [#3314](https://github.com/oauth2-proxy/oauth2-proxy/pull/3314) revert: fix: skip provider button auth only redirect (#3309)
|
||||
|
||||
# V7.14.1
|
||||
|
||||
## Release Highlights
|
||||
|
|
|
|||
|
|
@ -214,7 +214,7 @@ Provider specific options can be found on their respective subpages.
|
|||
| flag: `--skip-auth-regex`<br/>toml: `skip_auth_regex` | string \| list | (DEPRECATED for `--skip-auth-route`) bypass authentication for requests paths that match (may be given multiple times) | |
|
||||
| flag: `--skip-auth-route`<br/>toml: `skip_auth_routes` | string \| list | bypass authentication for requests that match the method & path. Format: method=path_regex OR method!=path_regex. For all methods: path_regex OR !=path_regex | |
|
||||
| flag: `--skip-jwt-bearer-tokens`<br/>toml: `skip_jwt_bearer_tokens` | bool | will skip requests that have verified JWT bearer tokens (the token must have [`aud`](https://en.wikipedia.org/wiki/JSON_Web_Token#Standard_fields) that matches this client id or one of the extras from `extra-jwt-issuers`) | false |
|
||||
| flag: `--skip-provider-button`<br/>toml: `skip_provider_button` | bool | will skip sign-in-page to directly reach the next step: oauth/start. When enabled, the `/oauth2/auth` endpoint returns 302 redirects instead of 401, ensuring compatibility with nginx `auth_request` and similar reverse proxy authentication architectures. | false |
|
||||
| flag: `--skip-provider-button`<br/>toml: `skip_provider_button` | bool | will skip sign-in-page to directly reach the next step: oauth/start | false |
|
||||
| flag: `--ssl-insecure-skip-verify`<br/>toml: `ssl_insecure_skip_verify` | bool | skip validation of certificates presented when using HTTPS providers | false |
|
||||
| flag: `--trusted-ip`<br/>toml: `trusted_ips` | string \| list | list of IPs or CIDR ranges to allow to bypass authentication (may be given multiple times). When combined with `--reverse-proxy` and optionally `--real-client-ip-header` this will evaluate the trust of the IP stored in an HTTP header by a reverse proxy rather than the layer-3/4 remote address. WARNING: trusting IPs has inherent security flaws, especially when obtaining the IP address from an HTTP header (reverse-proxy mode). Use this option only if you understand the risks and how to manage them. | |
|
||||
| flag: `--whitelist-domain`<br/>toml: `whitelist_domains` | string \| list | allowed domains for redirection after authentication. Prefix domain with a `.` or a `*.` to allow subdomains (e.g. `.example.com`, `*.example.com`) [^2] | |
|
||||
|
|
|
|||
|
|
@ -214,7 +214,7 @@ Provider specific options can be found on their respective subpages.
|
|||
| flag: `--skip-auth-regex`<br/>toml: `skip_auth_regex` | string \| list | (DEPRECATED for `--skip-auth-route`) bypass authentication for requests paths that match (may be given multiple times) | |
|
||||
| flag: `--skip-auth-route`<br/>toml: `skip_auth_routes` | string \| list | bypass authentication for requests that match the method & path. Format: method=path_regex OR method!=path_regex. For all methods: path_regex OR !=path_regex | |
|
||||
| flag: `--skip-jwt-bearer-tokens`<br/>toml: `skip_jwt_bearer_tokens` | bool | will skip requests that have verified JWT bearer tokens (the token must have [`aud`](https://en.wikipedia.org/wiki/JSON_Web_Token#Standard_fields) that matches this client id or one of the extras from `extra-jwt-issuers`) | false |
|
||||
| flag: `--skip-provider-button`<br/>toml: `skip_provider_button` | bool | will skip sign-in-page to directly reach the next step: oauth/start. When enabled, the `/oauth2/auth` endpoint returns 302 redirects instead of 401, ensuring compatibility with nginx `auth_request` and similar reverse proxy authentication architectures. | false |
|
||||
| flag: `--skip-provider-button`<br/>toml: `skip_provider_button` | bool | will skip sign-in-page to directly reach the next step: oauth/start | false |
|
||||
| flag: `--ssl-insecure-skip-verify`<br/>toml: `ssl_insecure_skip_verify` | bool | skip validation of certificates presented when using HTTPS providers | false |
|
||||
| flag: `--trusted-ip`<br/>toml: `trusted_ips` | string \| list | list of IPs or CIDR ranges to allow to bypass authentication (may be given multiple times). When combined with `--reverse-proxy` and optionally `--real-client-ip-header` this will evaluate the trust of the IP stored in an HTTP header by a reverse proxy rather than the layer-3/4 remote address. WARNING: trusting IPs has inherent security flaws, especially when obtaining the IP address from an HTTP header (reverse-proxy mode). Use this option only if you understand the risks and how to manage them. | |
|
||||
| flag: `--whitelist-domain`<br/>toml: `whitelist_domains` | string \| list | allowed domains for redirection after authentication. Prefix domain with a `.` or a `*.` to allow subdomains (e.g. `.example.com`, `*.example.com`) [^2] | |
|
||||
|
|
|
|||
|
|
@ -988,15 +988,7 @@ func (p *OAuthProxy) enrichSessionState(ctx context.Context, s *sessionsapi.Sess
|
|||
// and optional authorization).
|
||||
func (p *OAuthProxy) AuthOnly(rw http.ResponseWriter, req *http.Request) {
|
||||
session, err := p.getAuthenticatedSession(rw, req)
|
||||
// If SkipProviderButton is enabled and user needs login, redirect directly
|
||||
// to OAuth provider instead of returning 401. This allows nginx auth_request
|
||||
// to pass through the 302 redirect to the browser, bypassing error_page
|
||||
// handling which can break redirect flows.
|
||||
// See: https://github.com/oauth2-proxy/oauth2-proxy/issues/334
|
||||
if p.SkipProviderButton && err == ErrNeedsLogin {
|
||||
p.doOAuthStart(rw, req, nil)
|
||||
return
|
||||
} else if err != nil {
|
||||
if err != nil {
|
||||
http.Error(rw, http.StatusText(http.StatusUnauthorized), http.StatusUnauthorized)
|
||||
return
|
||||
}
|
||||
|
|
|
|||
|
|
@ -834,20 +834,8 @@ func NewProcessCookieTest(opts ProcessCookieTestOpts, modifiers ...OptionsModifi
|
|||
return nil, err
|
||||
}
|
||||
testProvider := &TestProvider{
|
||||
ProviderData: &providers.ProviderData{
|
||||
ProviderName: "Test Provider",
|
||||
LoginURL: &url.URL{
|
||||
Scheme: "http",
|
||||
Host: "localhost",
|
||||
Path: "/oauth/authorize",
|
||||
},
|
||||
RedeemURL: &url.URL{
|
||||
Scheme: "http",
|
||||
Host: "localhost",
|
||||
Path: "/oauth/token",
|
||||
},
|
||||
},
|
||||
ValidToken: opts.providerValidateCookieResponse,
|
||||
ProviderData: &providers.ProviderData{},
|
||||
ValidToken: opts.providerValidateCookieResponse,
|
||||
}
|
||||
|
||||
groups := pcTest.opts.Providers[0].AllowedGroups
|
||||
|
|
@ -1124,35 +1112,6 @@ func TestAuthOnlyEndpointUnauthorizedOnNoCookieSetError(t *testing.T) {
|
|||
assert.Equal(t, "Unauthorized\n", string(bodyBytes))
|
||||
}
|
||||
|
||||
// TestAuthOnlyEndpointRedirectWithSkipProviderButton tests that when SkipProviderButton
|
||||
// is true and no session exists, the /auth endpoint should return a 302 redirect
|
||||
// to the OAuth provider instead of 401. This is important for nginx auth_request
|
||||
// architecture where 401 triggers error_page handling which can break the redirect flow.
|
||||
// See: https://github.com/oauth2-proxy/oauth2-proxy/issues/334
|
||||
func TestAuthOnlyEndpointRedirectWithSkipProviderButton(t *testing.T) {
|
||||
test, err := NewAuthOnlyEndpointTest("", func(opts *options.Options) {
|
||||
opts.SkipProviderButton = true
|
||||
})
|
||||
if err != nil {
|
||||
t.Fatal(err)
|
||||
}
|
||||
|
||||
test.proxy.ServeHTTP(test.rw, test.req)
|
||||
|
||||
// With SkipProviderButton=true and no session, should return 302 redirect
|
||||
// to OAuth provider, NOT 401 Unauthorized
|
||||
assert.Equal(t, http.StatusFound, test.rw.Code)
|
||||
location := test.rw.Header().Get("Location")
|
||||
assert.NotEmpty(t, location, "Expected Location header for redirect")
|
||||
|
||||
// Verify the redirect points to the OAuth provider's authorize endpoint
|
||||
// and contains key OAuth parameters
|
||||
assert.Contains(t, location, "/oauth/authorize", "Expected redirect to OAuth authorize endpoint")
|
||||
assert.Contains(t, location, "client_id=", "Expected client_id in redirect URL")
|
||||
assert.Contains(t, location, "redirect_uri=", "Expected redirect_uri in redirect URL")
|
||||
assert.Contains(t, location, "state=", "Expected state parameter in redirect URL")
|
||||
}
|
||||
|
||||
func TestAuthOnlyEndpointUnauthorizedOnExpiration(t *testing.T) {
|
||||
test, err := NewAuthOnlyEndpointTest("", func(opts *options.Options) {
|
||||
opts.Cookie.Expire = time.Duration(24) * time.Hour
|
||||
|
|
|
|||
Loading…
Reference in New Issue