diff --git a/go.mod b/go.mod index 5046ceba72..068426a3dd 100644 --- a/go.mod +++ b/go.mod @@ -20,6 +20,7 @@ require ( github.com/gorilla/mux v1.7.3 github.com/gorilla/websocket v1.4.0 // indirect github.com/grpc-ecosystem/go-grpc-prometheus v1.2.0 + github.com/hashicorp/golang-lru v0.5.4 github.com/konsorten/go-windows-terminal-sequences v1.0.2 // indirect github.com/kylelemons/godebug v1.1.0 github.com/lib/pq v1.3.0 diff --git a/go.sum b/go.sum index 54abd1b102..43e3c76484 100644 --- a/go.sum +++ b/go.sum @@ -154,6 +154,8 @@ github.com/hashicorp/go-multierror v0.0.0-20161216184304-ed905158d874/go.mod h1: github.com/hashicorp/golang-lru v0.5.0/go.mod h1:/m3WP610KZHVQ1SGc6re/UDhFvYD7pJ4Ao+sR/qLZy8= github.com/hashicorp/golang-lru v0.5.1 h1:0hERBMJE1eitiLkihrMvRVBYAkpHzc/J3QdDN+dAcgU= github.com/hashicorp/golang-lru v0.5.1/go.mod h1:/m3WP610KZHVQ1SGc6re/UDhFvYD7pJ4Ao+sR/qLZy8= +github.com/hashicorp/golang-lru v0.5.4 h1:YDjusn29QI/Das2iO9M0BHnIbxPeyuCHsjMW+lJfyTc= +github.com/hashicorp/golang-lru v0.5.4/go.mod h1:iADmTwqILo4mZ8BN3D2Q6+9jd8WM5uGBxy+E8yxSoD4= github.com/hashicorp/hcl v1.0.0/go.mod h1:E5yfLk+7swimpb2L/Alb/PJmXilQ/rhwaUYs4T20WEQ= github.com/hpcloud/tail v1.0.0 h1:nfCOvKYfkgYP8hkirhJocXT2+zOD8yUNjXaWfTlyFKI= github.com/hpcloud/tail v1.0.0/go.mod h1:ab1qPbhIpdTxEkNHXyeSf5vhxWSCs/tWer42PpOxQnU= diff --git a/server/oauth2.go b/server/oauth2.go index 79e54c3297..364fcdaf3b 100644 --- a/server/oauth2.go +++ b/server/oauth2.go @@ -16,12 +16,15 @@ import ( "net" "net/http" "net/url" + "regexp" "strconv" "strings" "time" jose "gopkg.in/square/go-jose.v2" + lru "github.com/hashicorp/golang-lru" + "github.com/dexidp/dex/connector" "github.com/dexidp/dex/server/internal" "github.com/dexidp/dex/storage" @@ -440,7 +443,7 @@ func (s *Server) parseAuthorizationRequest(r *http.Request) (*storage.AuthReques } } - if !validateRedirectURI(client, redirectURI) { + if !validateRedirectURI(client, s.wildcardMatcherCache, redirectURI) { description := fmt.Sprintf("Unregistered redirect_uri (%q).", redirectURI) return nil, &authErr{"", "", errInvalidRequest, description} } @@ -587,12 +590,23 @@ func (s *Server) validateCrossClientTrust(clientID, peerID string) (trusted bool return false, nil } -func validateRedirectURI(client storage.Client, redirectURI string) bool { +func validateRedirectURI(client storage.Client, wildcardCache *lru.ARCCache, redirectURI string) bool { if !client.Public { for _, uri := range client.RedirectURIs { if redirectURI == uri { + // exact match is valid return true } + + if urlRequested, err := url.Parse(redirectURI); err == nil && urlRequested != nil { + // validly-formed url + clobberMatcher := getClobberHostMatcher(uri, wildcardCache) + validClobber := clobberMatcher.HostMatcher != nil && clobberMatcher.ClientRedirectURL != nil + if validClobber && matchesClobber(clobberMatcher, urlRequested) { + // clobber match success + return true + } + } } return false } @@ -606,7 +620,8 @@ func validateRedirectURI(client storage.Client, redirectURI string) bool { if err != nil { return false } - if u.Scheme != "http" { + // public clients should use http or https (#1300) + if u.Scheme != "http" && u.Scheme != "https" { return false } if u.Host == "localhost" { @@ -616,6 +631,108 @@ func validateRedirectURI(client storage.Client, redirectURI string) bool { return err == nil && host == "localhost" } +func matchesClobber(clobberMatcher ClientRedirectClobberMatcher, urlRequested *url.URL) bool { + if clobberMatcher.ClientRedirectURL.Path != urlRequested.Path { + // failed path match + return false + } + if clobberMatcher.ClientRedirectURL.Scheme != urlRequested.Scheme { + // failed scheme match + return false + } + if !clobberMatcher.HostMatcher.MatchString(urlRequested.Host) { + // failed host match + return false + } + return true +} + +type HostSplit struct { + HostSuffix string // e.g. host:port of two closest root domains e.g. example.com:8888 + SubDomainPrefix string // e.g. dom.abc (from dom.abc.example.com) +} + +type ClientRedirectClobberMatcher struct { + ClientRedirectURL *url.URL + HostMatcher *regexp.Regexp +} + +// Returns a clobber/wildcard subdomain matcher struct +func createClientRedirectClobberMatcher(clientRedirectURI string) ClientRedirectClobberMatcher { + redirectURL, _ := url.Parse(clientRedirectURI) + if redirectURL == nil { + return ClientRedirectClobberMatcher{} + } + splitResult := splitClobberHTTPRedirectURL(redirectURL) + if splitResult == nil { + // not wildcard or invalid + return ClientRedirectClobberMatcher{} + } + + // first replace double-asterisk globber with .+ + var subdomain = strings.ReplaceAll(regexp.QuoteMeta(splitResult.SubDomainPrefix), "\\*\\*", ".+") + subdomain = strings.ReplaceAll(subdomain, "\\*", "[^.]+") + + hostNameRegExStr := "^" + subdomain + "\\." + regexp.QuoteMeta(splitResult.HostSuffix) + "$" + + hostNameRegEx, err := regexp.Compile(hostNameRegExStr) + if err != nil { + // bad pattern - unexpected + return ClientRedirectClobberMatcher{} + } + + return ClientRedirectClobberMatcher{ + ClientRedirectURL: redirectURL, + HostMatcher: hostNameRegEx, + } +} + +// extract the scheme, host and path for wildcard checks. +// If the scheme is not http or https, or does not specify an absolute path beginning with '/' +// the parser will return nil. +func splitClobberHTTPRedirectURL(clientRedirectURLSpec *url.URL) *HostSplit { + host := clientRedirectURLSpec.Host + if !strings.Contains(host, "*") { + // for efficiency, check wildcard before DomainComponents + return nil + } + + if strings.Contains(host, "***") { + // a maximum of 2 '*' characters are permitted in sequence + return nil + } + + // sub-domain portion is first portion (greedy) + hostSplit := regexp.MustCompile(`^(.+)\.([^.]*[A-Za-z][^.]*\.[^.]*[A-Za-z][^.]+)$`) + hostRes := hostSplit.FindStringSubmatch(host) + if hostRes == nil { + // does not conform to requirements of a subdomain spec followed by two higher order domains + return nil + } + + return &HostSplit{ + HostSuffix: hostRes[2], + SubDomainPrefix: hostRes[1], + } +} + +func getClobberHostMatcher(clientRedirectURI string, wildcardCache *lru.ARCCache) ClientRedirectClobberMatcher { + if wildcardCache == nil { + // unexpected error condition where cache is unavailable + return ClientRedirectClobberMatcher{} + } + + if redirectMatcherStruct, ok := wildcardCache.Get(clientRedirectURI); ok { + // cache hit + return redirectMatcherStruct.(ClientRedirectClobberMatcher) + } + // cache miss - calc and add to cache + calcResult := createClientRedirectClobberMatcher(clientRedirectURI) + wildcardCache.Add(clientRedirectURI, calcResult) + // return cached result + return calcResult +} + func validateConnectorID(connectors []storage.Connector, connectorID string) bool { for _, c := range connectors { if c.ID == connectorID { diff --git a/server/oauth2_test.go b/server/oauth2_test.go index 8db9ea5929..8ed50d1257 100644 --- a/server/oauth2_test.go +++ b/server/oauth2_test.go @@ -10,6 +10,8 @@ import ( "strings" "testing" + lru "github.com/hashicorp/golang-lru" + "github.com/stretchr/testify/assert" "gopkg.in/square/go-jose.v2" "github.com/dexidp/dex/storage" @@ -322,6 +324,69 @@ func TestAccessTokenHash(t *testing.T) { } } +func TestSplitHttpRedirectUrl(t *testing.T) { + // if strings.Index(clientRedirectUrlSpec, "*") == -1 { + // no wildcard characters to consider + // return nil + //} + + tests := []struct { + redirectURI string + expectValid bool + expectHostSplit *HostSplit + }{ + { + redirectURI: "http://*.example.com/%23267#222", + expectValid: true, + expectHostSplit: &HostSplit{ + HostSuffix: "example.com", + SubDomainPrefix: "*", + }, + }, + { + // ip addresses cannot + redirectURI: "http://1.2.3.4/", + expectValid: false, + }, + { + // cannot act on top-level domain + redirectURI: "http://*.com/", + expectValid: false, + }, + { + // no wildcard + redirectURI: "http://example.com/", + expectValid: false, + }, + { + redirectURI: "http://1.2.3.4", + expectValid: false, // no trailing slash + }, + { + // ipv6 address not used for wildcard (dns only) + redirectURI: "https://[fdda:5cc1:23:4::1f]/foo", + expectValid: false, + }, + } + + tAssert := assert.New(t) + for _, test := range tests { + testRedirectURL, err := url.Parse(test.redirectURI) + tAssert.Nilf(err, "Invalid test url %v", test.redirectURI) + tAssert.NotNilf(testRedirectURL, "Invalid test url %v", test.redirectURI) + result := splitClobberHTTPRedirectURL(testRedirectURL) + tAssert.Equal(test.expectValid, result != nil, "Valid result expected for %v", test.redirectURI) + if test.expectHostSplit != nil { + tAssert.Equal(result, test.expectHostSplit, "Host split validation failed for %v", test.redirectURI) + } + if result != nil { + fullResult := createClientRedirectClobberMatcher(test.redirectURI) + tAssert.True(fullResult.HostMatcher != nil) + tAssert.Equal(fullResult.ClientRedirectURL, testRedirectURL) + } + } +} + func TestValidRedirectURI(t *testing.T) { tests := []struct { client storage.Client @@ -335,11 +400,198 @@ func TestValidRedirectURI(t *testing.T) { redirectURI: "http://foo.com/bar", wantValid: true, }, + { + // invalid schema + client: storage.Client{ + RedirectURIs: []string{"http://foo.com/bar"}, + }, + redirectURI: "https://foo.com/bar", + wantValid: false, + }, + { + // invalid schema with pattern + client: storage.Client{ + RedirectURIs: []string{"http://*.foo.com/bar"}, + }, + redirectURI: "https://test.foo.com/bar", + wantValid: false, + }, + { + // check https is valid protocol + client: storage.Client{ + RedirectURIs: []string{"https://foo.com/bar/baz"}, + }, + redirectURI: "https://foo.com/bar/baz", + wantValid: true, + }, { client: storage.Client{ RedirectURIs: []string{"http://foo.com/bar"}, }, redirectURI: "http://foo.com/bar/baz", + wantValid: false, + }, + { + // invalid path with pattern + client: storage.Client{ + RedirectURIs: []string{"http://*.foo.com/bar"}, + }, + redirectURI: "http://test.foo.com/baz", + wantValid: false, + }, + { + client: storage.Client{ + RedirectURIs: []string{"http://*.foo.com/bar"}, + }, + redirectURI: "http://abc.foo.com/bar", + wantValid: true, + }, + { + client: storage.Client{ + RedirectURIs: []string{"http://abc.foo.com/bar"}, + }, + redirectURI: "http://abc.foo.com/bar", + wantValid: true, + }, + { + client: storage.Client{ + RedirectURIs: []string{"http://b*.foo.com/bar"}, + }, + redirectURI: "http://abc.foo.com/bar", + wantValid: false, + }, + { + client: storage.Client{ + RedirectURIs: []string{"http://b*.foo.com/bar"}, + }, + redirectURI: "http://b.foo.com/bar", + wantValid: false, + }, + { + client: storage.Client{ + RedirectURIs: []string{"http://b*.foo.com/bar"}, + }, + redirectURI: "http://bar.foo.com/bar", + wantValid: true, + }, + { + // URL must use http: or https: protocol + client: storage.Client{ + RedirectURIs: []string{"unknown://*.foo.com/bar"}, + }, + redirectURI: "unknown://abc.foo.com/bar", + wantValid: true, + }, + { + // wildcard can be located in subdomain as long as not in top two domains + // https://abc.*.foo.com ok. + client: storage.Client{ + RedirectURIs: []string{"http://abc.*.foo.com/bar"}, + }, + redirectURI: "http://abc.123.foo.com/bar", + wantValid: true, + }, + { + client: storage.Client{ + RedirectURIs: []string{"http://**.foo.com/bar"}, + }, + redirectURI: "http://abc.123.foo.com/bar", + wantValid: true, + }, + { + client: storage.Client{ + RedirectURIs: []string{"http://**.foo.com/bar"}, + }, + redirectURI: "http://test.foo.com/bar", + wantValid: true, + }, + { + client: storage.Client{ + RedirectURIs: []string{"http://a**.foo.com/bar"}, + }, + redirectURI: "http://abc.123.foo.com/bar", + wantValid: true, + }, + { + client: storage.Client{ + RedirectURIs: []string{"http://a**.foo.com/bar"}, + }, + redirectURI: "http://a.foo.com/bar", + wantValid: false, + }, + { + // wildcard may be prefixed and/or suffixed with additional valid hostname characters + // https://pre-*-post.foo.com will work + client: storage.Client{ + RedirectURIs: []string{"http://pre-*-post.foo.com/bar"}, + }, + redirectURI: "http://pre-dinner-post.foo.com/bar", + wantValid: true, + }, + { + // valid wildcard will not match a URL more than one subdomain level in place of the wildcard + // https://*.foo.com will not work with https://123.abc.foo.com. + client: storage.Client{ + RedirectURIs: []string{"https://*.foo.com/"}, + }, + redirectURI: "https://123.abc.foo.com/", + wantValid: false, + }, + { + // check escaping + client: storage.Client{ + RedirectURIs: []string{"https://*.foo.com/"}, + }, + redirectURI: "https://abc.foo0com/", + wantValid: false, + }, + { + // with port + client: storage.Client{ + RedirectURIs: []string{"http://*.sld.com:6000/path"}, + }, + redirectURI: "http://test.sld.com:6000/path", + wantValid: true, + }, + { + // negative logic with port + client: storage.Client{ + RedirectURIs: []string{"http://*.sld.com:6000/path"}, + }, + redirectURI: "http://test.sld.com/path", + wantValid: false, + }, + { + // wildcard must be located in a subdomain within a hostname component. http://*.com is not permitted. + client: storage.Client{ + RedirectURIs: []string{"http://*.*.com/bar"}, + }, + redirectURI: "http://test.foo.com/bar", + wantValid: false, + }, + { + // partial wildcard in SLD is prohibited + client: storage.Client{ + RedirectURIs: []string{"http://*.foo*.com/bar"}, + }, + redirectURI: "http://test.foo.com/bar", + wantValid: false, + }, + { + // wildcard in TLD is prohibited + client: storage.Client{ + RedirectURIs: []string{"http://*.example.*/bar"}, + }, + redirectURI: "http://test.foo.com/bar", + wantValid: false, + }, + { + // partial wildcard in TLD is prohibited + client: storage.Client{ + RedirectURIs: []string{"http://*.example.io*/bar"}, + }, + redirectURI: "http://test.foo.com/bar", + wantValid: false, }, { client: storage.Client{ @@ -362,6 +614,14 @@ func TestValidRedirectURI(t *testing.T) { redirectURI: "http://localhost:991/bar", wantValid: true, }, + { + // github.com/dexidp/dex/issues/1300 allow public redirect URLs + client: storage.Client{ + Public: true, + }, + redirectURI: "https://localhost", + wantValid: true, + }, { client: storage.Client{ Public: true, @@ -377,13 +637,22 @@ func TestValidRedirectURI(t *testing.T) { wantValid: false, }, } + + maxCacheSize := 3 + wildcardCache, _ := lru.NewARC(maxCacheSize) + for _, test := range tests { - got := validateRedirectURI(test.client, test.redirectURI) + got := validateRedirectURI(test.client, wildcardCache, test.redirectURI) if got != test.wantValid { t.Errorf("client=%#v, redirectURI=%q, wanted valid=%t, got=%t", test.client, test.redirectURI, test.wantValid, got) } } + + if wildcardCache.Len() == 0 { + // check eviction policed + t.Errorf("cache should be in use") + } } func TestStorageKeySet(t *testing.T) { diff --git a/server/server.go b/server/server.go index c37b4fdc5e..b48f30814b 100644 --- a/server/server.go +++ b/server/server.go @@ -21,6 +21,8 @@ import ( "github.com/prometheus/client_golang/prometheus" "golang.org/x/crypto/bcrypt" + lru "github.com/hashicorp/golang-lru" + "github.com/dexidp/dex/connector" "github.com/dexidp/dex/connector/atlassiancrowd" "github.com/dexidp/dex/connector/authproxy" @@ -163,6 +165,9 @@ type Server struct { deviceRequestsValidFor time.Duration logger log.Logger + + // LRU cache of regex matchers for a given permitted redirect URI defined in Client + wildcardMatcherCache *lru.ARCCache } // NewServer constructs a server from the provided config. @@ -222,6 +227,11 @@ func newServer(ctx context.Context, c Config, rotationStrategy rotationStrategy) now = time.Now } + arcCache, err := lru.NewARC(500) + if err != nil { + c.Logger.Infof("ARC LRU case failed to load, wildcard redirect URIs disabled: %v", err) + } + s := &Server{ issuerURL: *issuerURL, connectors: make(map[string]Connector), @@ -236,6 +246,7 @@ func newServer(ctx context.Context, c Config, rotationStrategy rotationStrategy) templates: tmpls, passwordConnector: c.PasswordConnector, logger: c.Logger, + wildcardMatcherCache: arcCache, } // Retrieves connector objects in backend storage. This list includes the static connectors @@ -547,15 +558,15 @@ func (s *Server) OpenConnector(conn storage.Connector) (Connector, error) { } } - connector := Connector{ + connectorInstance := Connector{ ResourceVersion: conn.ResourceVersion, Connector: c, } s.mu.Lock() - s.connectors[conn.ID] = connector + s.connectors[conn.ID] = connectorInstance s.mu.Unlock() - return connector, nil + return connectorInstance, nil } // getConnector retrieves the connector object with the given id from the storage