From cb7d216d84506d5dd23c8b2afa32b09b763b8e02 Mon Sep 17 00:00:00 2001 From: Thomas Haines Date: Thu, 20 Aug 2020 00:11:27 +0800 Subject: [PATCH] Improve testing and used more mature and concurrency-safe LRU cache from hashicorp Signed-off-by: Thomas Haines --- go.mod | 1 + go.sum | 2 ++ server/oauth2.go | 8 ++++---- server/oauth2_test.go | 41 ++++++++++++++++++++++++++++++++++++++--- server/server.go | 13 ++++++++++--- 5 files changed, 55 insertions(+), 10 deletions(-) diff --git a/go.mod b/go.mod index edd4a00f45..2b740e45d0 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 7886ad6403..c2921eb4b6 100644 --- a/server/oauth2.go +++ b/server/oauth2.go @@ -21,10 +21,10 @@ import ( "strings" "time" - "github.com/golang/groupcache/lru" - 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" @@ -559,7 +559,7 @@ func (s *Server) validateCrossClientTrust(clientID, peerID string) (trusted bool return false, nil } -func validateRedirectURI(client storage.Client, wildcardCache *lru.Cache, 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 { @@ -594,7 +594,7 @@ func validateRedirectURI(client storage.Client, wildcardCache *lru.Cache, redire return err == nil && host == "localhost" } -func wildcardMatch(knownRedirectUri string, requestedRedirectUri string, wildcardCache *lru.Cache) bool { +func wildcardMatch(knownRedirectUri string, requestedRedirectUri string, wildcardCache *lru.ARCCache) bool { if wildcardCache == nil { return false } diff --git a/server/oauth2_test.go b/server/oauth2_test.go index a528af6794..dbfb8bbf3e 100644 --- a/server/oauth2_test.go +++ b/server/oauth2_test.go @@ -10,7 +10,7 @@ import ( "strings" "testing" - "github.com/golang/groupcache/lru" + "github.com/hashicorp/golang-lru" "gopkg.in/square/go-jose.v2" "github.com/dexidp/dex/storage" @@ -286,6 +286,20 @@ func TestValidRedirectURI(t *testing.T) { 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, + }, { // URL must use http: or https: protocol client: storage.Client{ @@ -374,15 +388,36 @@ func TestValidRedirectURI(t *testing.T) { }, } - wildcardCache := lru.New(2) + maxCacheSize := 3 + wildcardCache, _ := lru.NewARC(maxCacheSize) - for _, test := range tests { + 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 { 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) { + // check eviction policed + t.Errorf("first redirectURI should have been evicted from cache %s", tests[0].client.RedirectURIs[0]) } } diff --git a/server/server.go b/server/server.go index 7ec512680c..2783289930 100644 --- a/server/server.go +++ b/server/server.go @@ -15,12 +15,13 @@ import ( "time" "github.com/felixge/httpsnoop" - "github.com/golang/groupcache/lru" "github.com/gorilla/handlers" "github.com/gorilla/mux" "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,7 +164,7 @@ type Server struct { logger log.Logger // LRU cache of regex matchers for a given permitted redirect URI defined in Client - wildcardMatcherCache *lru.Cache + wildcardMatcherCache *lru.ARCCache } // NewServer constructs a server from the provided config. @@ -216,6 +217,12 @@ 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), @@ -229,7 +236,7 @@ func newServer(ctx context.Context, c Config, rotationStrategy rotationStrategy) templates: tmpls, passwordConnector: c.PasswordConnector, logger: c.Logger, - wildcardMatcherCache: lru.New(500), + wildcardMatcherCache: arcCache, } // Retrieves connector objects in backend storage. This list includes the static connectors