From bc022fbfd1bc37ba61c6d647e8bfbe799a18d302 Mon Sep 17 00:00:00 2001 From: Jan Brezina Date: Sat, 20 Jan 2024 20:08:30 +0100 Subject: [PATCH] Add possibility to encode the state param as UrlEncodedBase64 (#2312) * Add possibility to encode the state param as UrlEncodedBase64 * Update CHANGELOG.md * Update oauthproxy.go Co-authored-by: Jan Larwig --------- Co-authored-by: Jan Larwig --- CHANGELOG.md | 1 + docs/docs/configuration/overview.md | 1 + oauthproxy.go | 30 +++++++++++++++++++++-------- oauthproxy_test.go | 25 +++++++++++++++++++++++- pkg/apis/options/options.go | 2 ++ 5 files changed, 50 insertions(+), 9 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 6fbc9775..cc06be3f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,6 +12,7 @@ - [#2269](https://github.com/oauth2-proxy/oauth2-proxy/pull/2269) Added Azure China (and other air gaped cloud) support (@mblaschke) - [#2237](https://github.com/oauth2-proxy/oauth2-proxy/pull/2237) adds an option to append CA certificates (@emsixteeen) - [#2128](https://github.com/oauth2-proxy/oauth2-proxy/pull/2128) Update dependencies (@vllvll) +- [#2239](https://github.com/oauth2-proxy/oauth2-proxy/pull/2312) Add possibility to encode the state param as UrlEncodedBase64 (@brezinajn) - [#2274](https://github.com/oauth2-proxy/oauth2-proxy/pull/2274) Upgrade golang.org/x/net to v0.17.0 (@pierluigilenoci) - [#2278](https://github.com/oauth2-proxy/oauth2-proxy/pull/2278) Improve the Nginx auth_request example (@akunzai) - [#2282](https://github.com/oauth2-proxy/oauth2-proxy/pull/2282) Fixed checking Google Groups membership using Google Application Credentials (@kvanzuijlen) diff --git a/docs/docs/configuration/overview.md b/docs/docs/configuration/overview.md index 544b2bf5..746d15cf 100644 --- a/docs/docs/configuration/overview.md +++ b/docs/docs/configuration/overview.md @@ -207,6 +207,7 @@ An example [oauth2-proxy.cfg](https://github.com/oauth2-proxy/oauth2-proxy/blob/ | `--version` | n/a | print version string | | | `--whitelist-domain` | string \| list | allowed domains for redirection after authentication. Prefix domain with a `.` or a `*.` to allow subdomains (e.g. `.example.com`, `*.example.com`) \[[2](#footnote2)\] | | | `--trusted-ip` | 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. | | +| `--encode-state` | bool | encode the state parameter as UrlEncodedBase64 | false | > ###### 1. Only these providers support `--cookie-refresh`: GitLab, Google and OIDC {#footnote1} > ###### 2. When using the `whitelist-domain` option, any domain prefixed with a `.` or a `*.` will allow any subdomain of the specified domain as a valid redirect URL. By default, only empty ports are allowed. This translates to allowing the default port of the URLs protocol (80 for HTTP, 443 for HTTPS, etc.) since browsers omit them. To allow only a specific port, add it to the whitelisted domain: `example.com:8080`. To allow any port, use `*`: `example.com:*`. {#footnote2} diff --git a/oauthproxy.go b/oauthproxy.go index 40a622eb..bf4aaf92 100644 --- a/oauthproxy.go +++ b/oauthproxy.go @@ -3,6 +3,7 @@ package main import ( "context" "embed" + "encoding/base64" "encoding/json" "errors" "fmt" @@ -110,6 +111,8 @@ type OAuthProxy struct { serveMux *mux.Router redirectValidator redirect.Validator appDirector redirect.AppDirector + + encodeState bool } // NewOAuthProxy creates a new instance of OAuthProxy from the options provided @@ -239,6 +242,7 @@ func NewOAuthProxy(opts *options.Options, validator func(string) bool) (*OAuthPr upstreamProxy: upstreamProxy, redirectValidator: redirectValidator, appDirector: appDirector, + encodeState: opts.EncodeState, } p.buildServeMux(opts.ProxyPrefix) @@ -796,7 +800,7 @@ func (p *OAuthProxy) doOAuthStart(rw http.ResponseWriter, req *http.Request, ove callbackRedirect := p.getOAuthRedirectURI(req) loginURL := p.provider.GetLoginURL( callbackRedirect, - encodeState(csrf.HashOAuthState(), appRedirect), + encodeState(csrf.HashOAuthState(), appRedirect, p.encodeState), csrf.HashOIDCNonce(), extraParams, ) @@ -854,7 +858,7 @@ func (p *OAuthProxy) OAuthCallback(rw http.ResponseWriter, req *http.Request) { csrf.ClearCookie(rw, req) - nonce, appRedirect, err := decodeState(req) + nonce, appRedirect, err := decodeState(req.Form.Get("state"), p.encodeState) if err != nil { logger.Errorf("Error while parsing OAuth2 state: %v", err) p.ErrorPage(rw, req, http.StatusInternalServerError, err.Error()) @@ -1194,18 +1198,28 @@ func checkAllowedEmails(req *http.Request, s *sessionsapi.SessionState) bool { // encodedState builds the OAuth state param out of our nonce and // original application redirect -func encodeState(nonce string, redirect string) string { - return fmt.Sprintf("%v:%v", nonce, redirect) +func encodeState(nonce string, redirect string, encode bool) string { + rawString := fmt.Sprintf("%v:%v", nonce, redirect) + if encode { + return base64.RawURLEncoding.EncodeToString([]byte(rawString)) + } + return rawString } // decodeState splits the reflected OAuth state response back into // the nonce and original application redirect -func decodeState(req *http.Request) (string, string, error) { - state := strings.SplitN(req.Form.Get("state"), ":", 2) - if len(state) != 2 { +func decodeState(state string, encode bool) (string, string, error) { + toParse := state + if encode { + decoded, _ := base64.RawURLEncoding.DecodeString(state) + toParse = string(decoded) + } + + parsedState := strings.SplitN(toParse, ":", 2) + if len(parsedState) != 2 { return "", "", errors.New("invalid length") } - return state[0], state[1], nil + return parsedState[0], parsedState[1], nil } // addHeadersForProxying adds the appropriate headers the request / response for proxying diff --git a/oauthproxy_test.go b/oauthproxy_test.go index a27db6e2..772543f3 100644 --- a/oauthproxy_test.go +++ b/oauthproxy_test.go @@ -413,7 +413,7 @@ func (patTest *PassAccessTokenTest) getCallbackEndpoint() (httpCode int, cookie http.MethodGet, fmt.Sprintf( "/oauth2/callback?code=callback_code&state=%s", - encodeState(csrf.HashOAuthState(), "%2F"), + encodeState(csrf.HashOAuthState(), "%2F", false), ), strings.NewReader(""), ) @@ -3288,6 +3288,29 @@ func TestAuthOnlyAllowedEmailDomains(t *testing.T) { } } +func TestStateEncodesCorrectly(t *testing.T) { + state := "some_state_to_test" + nonce := "some_nonce_to_test" + + encodedResult := encodeState(nonce, state, true) + assert.Equal(t, "c29tZV9ub25jZV90b190ZXN0OnNvbWVfc3RhdGVfdG9fdGVzdA", encodedResult) + + notEncodedResult := encodeState(nonce, state, false) + assert.Equal(t, "some_nonce_to_test:some_state_to_test", notEncodedResult) +} + +func TestStateDecodesCorrectly(t *testing.T) { + nonce, redirect, _ := decodeState("c29tZV9ub25jZV90b190ZXN0OnNvbWVfc3RhdGVfdG9fdGVzdA", true) + + assert.Equal(t, "some_nonce_to_test", nonce) + assert.Equal(t, "some_state_to_test", redirect) + + nonce2, redirect2, _ := decodeState("some_nonce_to_test:some_state_to_test", false) + + assert.Equal(t, "some_nonce_to_test", nonce2) + assert.Equal(t, "some_state_to_test", redirect2) +} + func TestAuthOnlyAllowedEmails(t *testing.T) { testCases := []struct { name string diff --git a/pkg/apis/options/options.go b/pkg/apis/options/options.go index 49474c92..77ba341c 100644 --- a/pkg/apis/options/options.go +++ b/pkg/apis/options/options.go @@ -61,6 +61,7 @@ type Options struct { SSLInsecureSkipVerify bool `flag:"ssl-insecure-skip-verify" cfg:"ssl_insecure_skip_verify"` SkipAuthPreflight bool `flag:"skip-auth-preflight" cfg:"skip_auth_preflight"` ForceJSONErrors bool `flag:"force-json-errors" cfg:"force_json_errors"` + EncodeState bool `flag:"encode-state" cfg:"encode_state"` AllowQuerySemicolons bool `flag:"allow-query-semicolons" cfg:"allow_query_semicolons"` SignatureKey string `flag:"signature-key" cfg:"signature_key"` @@ -128,6 +129,7 @@ func NewFlagSet() *pflag.FlagSet { flagSet.Bool("ssl-insecure-skip-verify", false, "skip validation of certificates presented when using HTTPS providers") flagSet.Bool("skip-jwt-bearer-tokens", false, "will skip requests that have verified JWT bearer tokens (default false)") flagSet.Bool("force-json-errors", false, "will force JSON errors instead of HTTP error pages or redirects") + flagSet.Bool("encode-state", false, "will encode oauth state with base64") flagSet.Bool("allow-query-semicolons", false, "allow the use of semicolons in query args") flagSet.StringSlice("extra-jwt-issuers", []string{}, "if skip-jwt-bearer-tokens is set, a list of extra JWT issuer=audience pairs (where the issuer URL has a .well-known/openid-configuration or a .well-known/jwks.json)")