issue-448 add globber support; improve cache performance; and allow w…

…ildcard matcher to operate within host portion of URL to ensure immune to injection type patterns in the path

Signed-off-by: Thomas Haines <[email protected]>

go.mod

@@ -13,7 +13,7 @@ require (
 	github.com/felixge/httpsnoop v1.0.1
 	github.com/ghodss/yaml v1.0.0
 	github.com/go-sql-driver/mysql v1.5.0
-	github.com/golang/groupcache v0.0.0-20190702054246-869f871628b6
+	github.com/golang/groupcache v0.0.0-20190702054246-869f871628b6 // indirect
 	github.com/golang/protobuf v1.3.2
 	github.com/google/uuid v1.1.1 // indirect
 	github.com/gorilla/handlers v1.4.2

server/oauth2.go

@@ -566,9 +566,14 @@ func validateRedirectURI(client storage.Client, wildcardCache *lru.ARCCache, red
 				// exact match is valid
 				return true
 			}
-			if wildcardMatch(uri, redirectURI, wildcardCache) {
-				// found wildcard match
-				return true
+
+			if urlRequested, err := url.Parse(redirectURI); err == nil && urlRequested != nil {
+				// validly-formed url
+				clobberMatcher := getClobberHostMatcher(uri, wildcardCache)
+				if clobberMatcher.HostMatcher != nil && matchesClobber(clobberMatcher, urlRequested) {
+					// clobber match success
+					return true
+				}
 			}
 		}
 		return false
@@ -594,92 +599,141 @@ func validateRedirectURI(client storage.Client, wildcardCache *lru.ARCCache, red
 	return err == nil && host == "localhost"
 }
 
-func wildcardMatch(knownRedirectUri string, requestedRedirectUri string, wildcardCache *lru.ARCCache) bool {
-	if wildcardCache == nil {
+func matchesClobber(clobberMatcher ClientRedirectClobberMatcher, urlRequested *url.URL) bool {
+	if !clobberMatcher.HostMatcher.MatchString(urlRequested.Host) {
+		// failed host match
 		return false
 	}
-	regExStr, ok := wildcardCache.Get(knownRedirectUri)
-	if ok && regExStr != nil {
-		// cache hit
-		return matchByRegEx(fmt.Sprintf("%s", regExStr), requestedRedirectUri)
-	} else {
-		// cache miss - calc and add to cache
-		calcResult := calculateWildcardRedirect(knownRedirectUri)
-		wildcardCache.Add(knownRedirectUri, calcResult)
-		return matchByRegEx(calcResult, requestedRedirectUri)
-	}
-}
-
-func matchByRegEx(regexStrVal string, knownRedirectUri string) bool {
-	if len(regexStrVal) == 0 {
+	if clobberMatcher.ClientRedirectURLSections.Scheme != urlRequested.Scheme {
+		// failed scheme match
 		return false
 	}
-	if re, err := regexp.Compile(regexStrVal); err == nil {
-		return re.MatchString(knownRedirectUri)
-	} else {
-		// issue with regex
+	if clobberMatcher.ClientRedirectURLSections.RawPath != urlRequested.Path {
+		// failed path match
 		return false
 	}
+	return true
 }
 
-func calculateWildcardRedirect(uri string) string {
-	empty := ""
-	if len(uri) < 4 {
-		// empty or without protocol cannot be wildcard domain
-		return empty
-	}
-	re, err := regexp.Compile("\\*")
-	if err != nil {
-		// bad pattern
-		return empty
+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 ClobberURLSectional struct {
+	Scheme   string     // scheme: `http` or `https`
+	HostPair *HostSplit // leading from top-level to subsequent subdomains
+	RawPath  string     // path: requires leading slash; and includes fragments and query params
+}
+
+type ClientRedirectClobberMatcher struct {
+	ClientRedirectURL string
+	ClientRedirectURLSections *ClobberURLSectional
+	HostMatcher *regexp.Regexp
+}
+
+// Returns a clobber/wildcard subdomain matcher struct.  The `ClientRedirectURLSections`
+// and `ClientRedirectURLSections` pointers will be nil if clientRedirectURI does not meet
+// the requirements to perform as a clobber/wildcard matcher.  ClientRedirectURL is always set.
+func createClientRedirectClobberMatcher(clientRedirectURI string) ClientRedirectClobberMatcher {
+
+	splitResult := splitClobberHttpRedirectUrl(clientRedirectURI)
+	if splitResult == nil || splitResult.HostPair == nil {
+		// does not meet basic wildcard usage requirements regarding scheme or path
+		return  ClientRedirectClobberMatcher{
+			ClientRedirectURL: clientRedirectURI,
+		}
 	}
 
-	var wildcardCharCount = 0
-	starRes := re.FindAllString(uri, 3)
-	if starRes != nil {
-		wildcardCharCount = len(starRes)
+	clobberMatcher := ClientRedirectClobberMatcher{
+		ClientRedirectURL: clientRedirectURI,
+		ClientRedirectURLSections: splitResult,
 	}
 
-	if wildcardCharCount != 1 {
-		// only one wildcard is allowed
-		// zero wildcards also a non-match as wildcard definition
-		return empty
+	// first replace double-asterisk globber with .+
+	var subdomain = strings.ReplaceAll(regexp.QuoteMeta(splitResult.HostPair.SubDomainPrefix), "\\*\\*", ".+")
+	subdomain = strings.ReplaceAll(subdomain, "\\*", "[^.]+")
+
+	hostNameRegExStr := "^" + subdomain + "\\." + splitResult.HostPair.HostSuffix + "$"
+
+	hostNameRegEx, err := regexp.Compile(hostNameRegExStr)
+	if err != nil {
+		// bad pattern - unexpected
+		return clobberMatcher
 	}
 
-	schemaRe, err := regexp.Compile("^[hH][tT]{2}[pP][sS]?://(.+?)(:\\d+)?/.*")
+	clobberMatcher.HostMatcher = hostNameRegEx
+	return clobberMatcher
+}
+
+// This method is similar to url.Parse stdlib, expect that it does not validate a url, it is
+// used to 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 string) *ClobberURLSectional {
+	schemaRe, err := regexp.Compile("^([hH][tT]{2}[pP][sS]?)://([^/]+?)(:\\d+)?(/.*)$")
 	if err != nil {
-		return empty // bad regex pattern
+		return nil // bad regex pattern
 	}
-	splitSchemaRes := schemaRe.FindStringSubmatch(uri)
-	if splitSchemaRes == nil {
+	splitRes := schemaRe.FindStringSubmatch(clientRedirectUrlSpec)
+	if splitRes == nil {
 		// not using http or https
-		return empty
+		return nil
 	}
-	hostPortion := splitSchemaRes[1]
 
-	// optional prefix/postfix ensure wildcard in first bracket, and 2 or more domains under root TLD
-	hostSplitRe, err := regexp.Compile("^([^.]*\\*[^.]*)(\\..+\\..+)$")
-	if err != nil {
-		return empty // bad regex pattern
+	hostPortion := splitRes[2]
+	if !strings.Contains(hostPortion, "*") {
+		// for efficiency, check wildcard before DomainComponents
+		return nil
 	}
 
-	if !hostSplitRe.MatchString(hostPortion) {
-		return empty // no match, cannot be valid wildcard domain
+	if strings.Contains(hostPortion, "***") {
+		// more than wildcards chars are not permitted
+		return nil
 	}
 
-	// all checks & rules passed, created regex representing wildcard match
-	regexStr := strings.ReplaceAll(regexp.QuoteMeta(uri), "\\*", "[^.]+")
+	// port := splitRes[3]
 
-	regExStr := "^" + regexStr + "$"
+	// sub-domain portion is first portion (greedy)
+	hostSplit, err := regexp.Compile("^(.+)\\.([^.]*[A-Za-z][^.]*\\.[^.]*[A-Za-z][^.]*[^.])$")
+	if hostSplit == nil {
+		// unexpected issue with regex
+		return nil
+	}
+	hostRes := hostSplit.FindStringSubmatch(hostPortion)
+	if hostRes == nil {
+		// does not conform to requirements of a subdomain spec followed by two higher order domains
+		return nil
+	}
 
-	// check valid regex before saving to cache
-	_, err = regexp.Compile(regExStr)
-	if err != nil {
-		// bad regex pattern (do not cache)
-		return empty
+	return &ClobberURLSectional {
+		Scheme: splitRes[1],
+		HostPair: &HostSplit{
+			HostSuffix: hostRes[2],
+			SubDomainPrefix: hostRes[1],
+		},
+		RawPath: splitRes[4],
+	}
+}
+
+func getClobberHostMatcher(clientRedirectUri string, wildcardCache *lru.ARCCache) ClientRedirectClobberMatcher {
+	if wildcardCache == nil {
+		// unexpected error condition where cache is unavailable
+		return ClientRedirectClobberMatcher {
+			ClientRedirectURL: clientRedirectUri,
+		}
 	}
 
-	return regExStr
+	if redirectMatcherStruct, ok := wildcardCache.Get(clientRedirectUri); ok {
+		// cache hit
+		return redirectMatcherStruct.(ClientRedirectClobberMatcher)
+	} else {
+		// 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 {

server/oauth2_test.go

@@ -11,6 +11,7 @@ import (
 	"testing"
 
 	"github.com/hashicorp/golang-lru"
+	"github.com/stretchr/testify/assert"
 	"gopkg.in/square/go-jose.v2"
 
 	"github.com/dexidp/dex/storage"
@@ -251,6 +252,76 @@ 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
+		expectScheme    string
+		expectPath      string
+		expectHostSplit *HostSplit
+	}{
+		{
+			redirectURI:  "http://*.example.com/%23267#222",
+			expectValid:  true,
+			expectScheme: "http",
+			expectHostSplit: &HostSplit{
+				HostSuffix: "example.com",
+				SubDomainPrefix: "*",
+			},
+			expectPath:   "/%23267#222",
+		},
+		{
+			// 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
+		},
+		{
+			redirectURI: "ftp://*.example.com/",
+			expectValid: false, // wrong schema
+		},
+		{
+			// 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 {
+		result := splitClobberHttpRedirectUrl(test.redirectURI)
+		if result == nil {
+			tAssert.False(test.expectValid, "Valid result expected")
+		} else {
+			tAssert.Equal(test.expectScheme,result.Scheme, "scheme should match")
+			tAssert.Equal(test.expectPath,result.RawPath, "path should match")
+			tAssert.Equal(test.expectHostSplit,result.HostPair, "DomainComponents should match")
+
+			fullResult := createClientRedirectClobberMatcher(test.redirectURI)
+			tAssert.True(fullResult.HostMatcher != nil)
+		}
+	}
+}
+
+
 func TestValidRedirectURI(t *testing.T) {
 	tests := []struct {
 		client      storage.Client
@@ -309,13 +380,27 @@ func TestValidRedirectURI(t *testing.T) {
 			wantValid:   false,
 		},
 		{
-			// wildcard must be located in the subdomain furthest from the root domain.
-			//  https://abc.*.foo.com not ok.
+			// 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:   false,
+			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://a**.foo.com/bar"},
+			},
+			redirectURI: "http://abc.123.foo.com/bar",
+			wantValid:   true,
 		},
 		{
 			// wildcard may be prefixed and/or suffixed with additional valid hostname characters
@@ -391,33 +476,17 @@ func TestValidRedirectURI(t *testing.T) {
 	maxCacheSize := 3
 	wildcardCache, _ := lru.NewARC(maxCacheSize)
 
-	if maxCacheSize >= len(tests) {
-		t.Errorf("cache tests pre-condition is that mac cache size is less than test suite size" +
-			" cacheMax=%d testItemsSize=%d", maxCacheSize, len(tests))
-	}
-
-	var firstWildcardCache = ""
-	for i, test := range tests {
+	for _, test := range tests {
 		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)
 		}
-
-		// expect redirectURI last used should be stored in cache
-		if !test.client.Public && len(test.client.RedirectURIs) > 0 && test.redirectURI != test.client.RedirectURIs[0] {
-			if !wildcardCache.Contains(test.client.RedirectURIs[0]) {
-				t.Errorf("test[%d] should contain redirectURI as cache key %s", i, test.client.RedirectURIs[0])
-			}
-			if len(firstWildcardCache) == 0 {
-				firstWildcardCache = test.client.RedirectURIs[0]
-			}
-		}
 	}
 
-	if wildcardCache.Contains(firstWildcardCache) {
+	if wildcardCache.Len() == 0 {
 		// check eviction policed
-		t.Errorf("first redirectURI should have been evicted from cache %s", tests[0].client.RedirectURIs[0])
+		t.Errorf("cache should be in use")
 	}
 }