diff --git a/CHANGELOG.md b/CHANGELOG.md index 14d60f09..128c2a56 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,7 @@ - [#347](https://github.com/pusher/oauth2_proxy/pull/347) Update keycloak provider configuration documentation - [#325](https://github.com/pusher/oauth2_proxy/pull/325) dist.sh: use sha256sum (@syscll) - [#179](https://github.com/pusher/oauth2_proxy/pull/179) Add Nextcloud provider (@Ramblurr) +- [#280](https://github.com/pusher/oauth2_proxy/pull/280) whitelisted redirect domains: add support for whitelisting specific ports or allowing wildcard ports (@kamaln7) - [#351](https://github.com/pusher/oauth2_proxy/pull/351) Add DigitalOcean Auth provider (@kamaln7) # v4.1.0 diff --git a/docs/configuration/configuration.md b/docs/configuration/configuration.md index 8ff64c18..7f4a7bdc 100644 --- a/docs/configuration/configuration.md +++ b/docs/configuration/configuration.md @@ -112,7 +112,7 @@ An example [oauth2_proxy.cfg]({{ site.gitweb }}/contrib/oauth2_proxy.cfg.example | `-version` | n/a | print version string | | | `-whitelist-domain` | string \| list | allowed domains for redirection after authentication. Prefix domain with a `.` to allow subdomains (eg `.example.com`) | | -Note, when using the `whitelist-domain` option, any domain prefixed with a `.` will allow any subdomain of the specified domain as a valid redirect URL. +Note: when using the `whitelist-domain` option, any domain prefixed with 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 URL's 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:*`. See below for provider specific options diff --git a/oauthproxy.go b/oauthproxy.go index 72ab580b..3fa7a223 100644 --- a/oauthproxy.go +++ b/oauthproxy.go @@ -514,6 +514,43 @@ func (p *OAuthProxy) GetRedirect(req *http.Request) (redirect string, err error) return } +// splitHostPort separates host and port. If the port is not valid, it returns +// the entire input as host, and it doesn't check the validity of the host. +// Unlike net.SplitHostPort, but per RFC 3986, it requires ports to be numeric. +// *** taken from net/url, modified validOptionalPort() to accept ":*" +func splitHostPort(hostport string) (host, port string) { + host = hostport + + colon := strings.LastIndexByte(host, ':') + if colon != -1 && validOptionalPort(host[colon:]) { + host, port = host[:colon], host[colon+1:] + } + + if strings.HasPrefix(host, "[") && strings.HasSuffix(host, "]") { + host = host[1 : len(host)-1] + } + + return +} + +// validOptionalPort reports whether port is either an empty string +// or matches /^:\d*$/ +// *** taken from net/url, modified to accept ":*" +func validOptionalPort(port string) bool { + if port == "" || port == ":*" { + return true + } + if port[0] != ':' { + return false + } + for _, b := range port[1:] { + if b < '0' || b > '9' { + return false + } + } + return true +} + // IsValidRedirect checks whether the redirect URL is whitelisted func (p *OAuthProxy) IsValidRedirect(redirect string) bool { switch { @@ -524,11 +561,28 @@ func (p *OAuthProxy) IsValidRedirect(redirect string) bool { if err != nil { return false } + redirectHostname := redirectURL.Hostname() + for _, domain := range p.whitelistDomains { - if (redirectURL.Host == domain) || (strings.HasPrefix(domain, ".") && strings.HasSuffix(redirectURL.Host, domain)) { - return true + domainHostname, domainPort := splitHostPort(strings.TrimLeft(domain, ".")) + if domainHostname == "" { + continue + } + + if (redirectHostname == domainHostname) || (strings.HasPrefix(domain, ".") && strings.HasSuffix(redirectHostname, domainHostname)) { + // the domain names match, now validate the ports + // if the whitelisted domain's port is '*', allow all ports + // if the whitelisted domain contains a specific port, only allow that port + // if the whitelisted domain doesn't contain a port at all, only allow empty redirect ports ie http and https + redirectPort := redirectURL.Port() + if (domainPort == "*") || + (domainPort == redirectPort) || + (domainPort == "" && redirectPort == "") { + return true + } } } + return false default: return false diff --git a/oauthproxy_test.go b/oauthproxy_test.go index ee850419..abe449d9 100644 --- a/oauthproxy_test.go +++ b/oauthproxy_test.go @@ -182,49 +182,158 @@ func TestIsValidRedirect(t *testing.T) { opts.ClientSecret = "fgkdsgj" opts.CookieSecret = "ljgiogbj" // Should match domains that are exactly foo.bar and any subdomain of bar.foo - opts.WhitelistDomains = []string{"foo.bar", ".bar.foo"} + opts.WhitelistDomains = []string{ + "foo.bar", + ".bar.foo", + "port.bar:8080", + ".sub.port.bar:8080", + "anyport.bar:*", + ".sub.anyport.bar:*", + } opts.Validate() proxy := NewOAuthProxy(opts, func(string) bool { return true }) - noRD := proxy.IsValidRedirect("") - assert.Equal(t, false, noRD) + testCases := []struct { + Desc, Redirect string + ExpectedResult bool + }{ + { + Desc: "noRD", + Redirect: "", + ExpectedResult: false, + }, + { + Desc: "singleSlash", + Redirect: "/redirect", + ExpectedResult: true, + }, + { + Desc: "doubleSlash", + Redirect: "//redirect", + ExpectedResult: false, + }, + { + Desc: "validHTTP", + Redirect: "http://foo.bar/redirect", + ExpectedResult: true, + }, + { + Desc: "validHTTPS", + Redirect: "https://foo.bar/redirect", + ExpectedResult: true, + }, + { + Desc: "invalidHTTPSubdomain", + Redirect: "http://baz.foo.bar/redirect", + ExpectedResult: false, + }, + { + Desc: "invalidHTTPSSubdomain", + Redirect: "https://baz.foo.bar/redirect", + ExpectedResult: false, + }, + { + Desc: "validHTTPSubdomain", + Redirect: "http://baz.bar.foo/redirect", + ExpectedResult: true, + }, + { + Desc: "validHTTPSSubdomain", + Redirect: "https://baz.bar.foo/redirect", + ExpectedResult: true, + }, + { + Desc: "validHTTPDomain", + Redirect: "http://bar.foo/redirect", + ExpectedResult: true, + }, + { + Desc: "invalidHTTP1", + Redirect: "http://foo.bar.evil.corp/redirect", + ExpectedResult: false, + }, + { + Desc: "invalidHTTPS1", + Redirect: "https://foo.bar.evil.corp/redirect", + ExpectedResult: false, + }, + { + Desc: "invalidHTTP2", + Redirect: "http://evil.corp/redirect?rd=foo.bar", + ExpectedResult: false, + }, + { + Desc: "invalidHTTPS2", + Redirect: "https://evil.corp/redirect?rd=foo.bar", + ExpectedResult: false, + }, + { + Desc: "invalidPort", + Redirect: "https://evil.corp:3838/redirect", + ExpectedResult: false, + }, + { + Desc: "invalidEmptyPort", + Redirect: "http://foo.bar:3838/redirect", + ExpectedResult: false, + }, + { + Desc: "invalidEmptyPortSubdomain", + Redirect: "http://baz.bar.foo:3838/redirect", + ExpectedResult: false, + }, + { + Desc: "validSpecificPort", + Redirect: "http://port.bar:8080/redirect", + ExpectedResult: true, + }, + { + Desc: "invalidSpecificPort", + Redirect: "http://port.bar:3838/redirect", + ExpectedResult: false, + }, + { + Desc: "validSpecificPortSubdomain", + Redirect: "http://foo.sub.port.bar:8080/redirect", + ExpectedResult: true, + }, + { + Desc: "invalidSpecificPortSubdomain", + Redirect: "http://foo.sub.port.bar:3838/redirect", + ExpectedResult: false, + }, + { + Desc: "validAnyPort1", + Redirect: "http://anyport.bar:8080/redirect", + ExpectedResult: true, + }, + { + Desc: "validAnyPort2", + Redirect: "http://anyport.bar:8081/redirect", + ExpectedResult: true, + }, + { + Desc: "validAnyPortSubdomain1", + Redirect: "http://a.sub.anyport.bar:8080/redirect", + ExpectedResult: true, + }, + { + Desc: "validAnyPortSubdomain2", + Redirect: "http://a.sub.anyport.bar:8081/redirect", + ExpectedResult: true, + }, + } - singleSlash := proxy.IsValidRedirect("/redirect") - assert.Equal(t, true, singleSlash) + for _, tc := range testCases { + t.Run(tc.Desc, func(t *testing.T) { + result := proxy.IsValidRedirect(tc.Redirect) - doubleSlash := proxy.IsValidRedirect("//redirect") - assert.Equal(t, false, doubleSlash) - - validHTTP := proxy.IsValidRedirect("http://foo.bar/redirect") - assert.Equal(t, true, validHTTP) - - validHTTPS := proxy.IsValidRedirect("https://foo.bar/redirect") - assert.Equal(t, true, validHTTPS) - - invalidHTTPSubdomain := proxy.IsValidRedirect("http://baz.foo.bar/redirect") - assert.Equal(t, false, invalidHTTPSubdomain) - - invalidHTTPSSubdomain := proxy.IsValidRedirect("https://baz.foo.bar/redirect") - assert.Equal(t, false, invalidHTTPSSubdomain) - - validHTTPSubdomain := proxy.IsValidRedirect("http://baz.bar.foo/redirect") - assert.Equal(t, true, validHTTPSubdomain) - - validHTTPSSubdomain := proxy.IsValidRedirect("https://baz.bar.foo/redirect") - assert.Equal(t, true, validHTTPSSubdomain) - - invalidHTTP1 := proxy.IsValidRedirect("http://foo.bar.evil.corp/redirect") - assert.Equal(t, false, invalidHTTP1) - - invalidHTTPS1 := proxy.IsValidRedirect("https://foo.bar.evil.corp/redirect") - assert.Equal(t, false, invalidHTTPS1) - - invalidHTTP2 := proxy.IsValidRedirect("http://evil.corp/redirect?rd=foo.bar") - assert.Equal(t, false, invalidHTTP2) - - invalidHTTPS2 := proxy.IsValidRedirect("https://evil.corp/redirect?rd=foo.bar") - assert.Equal(t, false, invalidHTTPS2) + if result != tc.ExpectedResult { + t.Errorf("expected %t got %t", tc.ExpectedResult, result) + } + }) + } } type TestProvider struct {