[ARVADOS] created: 2.1.0-290-g969441a09

Git user git at public.arvados.org
Tue Jan 26 04:59:43 UTC 2021


        at  969441a091ce3aa1eb7a9525d3ab85f24fbd8fdd (commit)


commit 969441a091ce3aa1eb7a9525d3ab85f24fbd8fdd
Author: Tom Clegg <tom at curii.com>
Date:   Mon Jan 25 23:58:15 2021 -0500

    16669: Test oidcTokenAuthorizer cache.
    
    Arvados-DCO-1.1-Signed-off-by: Tom Clegg <tom at curii.com>

diff --git a/lib/controller/localdb/login_oidc.go b/lib/controller/localdb/login_oidc.go
index 1fa4da776..a5fe45181 100644
--- a/lib/controller/localdb/login_oidc.go
+++ b/lib/controller/localdb/login_oidc.go
@@ -37,10 +37,11 @@ import (
 	"google.golang.org/api/people/v1"
 )
 
-const (
+var (
 	tokenCacheSize        = 1000
 	tokenCacheNegativeTTL = time.Minute * 5
 	tokenCacheTTL         = time.Minute * 10
+	tokenCacheRaceWindow  = time.Minute
 )
 
 type oidcLoginController struct {
@@ -363,8 +364,9 @@ func (ta *oidcTokenAuthorizer) WrapCalls(origFunc api.RoutableFunc) api.Routable
 			return origFunc(ctx, opts)
 		}
 		// Check each token in the incoming request. If any
-		// are OAuth2 access tokens, swap them out for Arvados
-		// tokens.
+		// are valid OAuth2 access tokens, insert/update them
+		// in the database so RailsAPI's auth code accepts
+		// them.
 		for _, tok := range creds.Tokens {
 			err = ta.registerToken(ctx, tok)
 			if err != nil {
@@ -463,7 +465,7 @@ func (ta *oidcTokenAuthorizer) registerToken(ctx context.Context, tok string) er
 	// Expiry time for our token is one minute longer than our
 	// cache TTL, so we don't pass it through to RailsAPI just as
 	// it's expiring.
-	exp := time.Now().UTC().Add(tokenCacheTTL + time.Minute)
+	exp := time.Now().UTC().Add(tokenCacheTTL + tokenCacheRaceWindow)
 
 	var aca arvados.APIClientAuthorization
 	if updating {
diff --git a/lib/controller/localdb/login_oidc_test.go b/lib/controller/localdb/login_oidc_test.go
index 9bc6f90ea..e157b73fc 100644
--- a/lib/controller/localdb/login_oidc_test.go
+++ b/lib/controller/localdb/login_oidc_test.go
@@ -7,8 +7,11 @@ package localdb
 import (
 	"bytes"
 	"context"
+	"crypto/hmac"
+	"crypto/sha256"
 	"encoding/json"
 	"fmt"
+	"io"
 	"net/http"
 	"net/http/httptest"
 	"net/url"
@@ -23,6 +26,7 @@ import (
 	"git.arvados.org/arvados.git/sdk/go/arvadostest"
 	"git.arvados.org/arvados.git/sdk/go/auth"
 	"git.arvados.org/arvados.git/sdk/go/ctxlog"
+	"github.com/jmoiron/sqlx"
 	check "gopkg.in/check.v1"
 )
 
@@ -194,6 +198,62 @@ func (s *OIDCLoginSuite) TestGoogleLogin_PeopleAPIError(c *check.C) {
 	c.Check(resp.RedirectLocation, check.Equals, "")
 }
 
+func (s *OIDCLoginSuite) TestOIDCAuthorizer(c *check.C) {
+	s.cluster.Login.Google.Enable = false
+	s.cluster.Login.OpenIDConnect.Enable = true
+	json.Unmarshal([]byte(fmt.Sprintf("%q", s.fakeProvider.Issuer.URL)), &s.cluster.Login.OpenIDConnect.Issuer)
+	s.cluster.Login.OpenIDConnect.ClientID = "oidc#client#id"
+	s.cluster.Login.OpenIDConnect.ClientSecret = "oidc#client#secret"
+	s.fakeProvider.ValidClientID = "oidc#client#id"
+	s.fakeProvider.ValidClientSecret = "oidc#client#secret"
+	db := arvadostest.DB(c, s.cluster)
+
+	tokenCacheTTL = time.Millisecond
+	tokenCacheRaceWindow = time.Millisecond
+
+	oidcAuthorizer := OIDCAccessTokenAuthorizer(s.cluster, func(context.Context) (*sqlx.DB, error) { return db, nil })
+	accessToken := s.fakeProvider.ValidAccessToken()
+
+	mac := hmac.New(sha256.New, []byte(s.cluster.SystemRootToken))
+	io.WriteString(mac, accessToken)
+	hmac := fmt.Sprintf("%x", mac.Sum(nil))
+
+	cleanup := func() {
+		_, err := db.Exec(`delete from api_client_authorizations where api_token=$1`, hmac)
+		c.Check(err, check.IsNil)
+	}
+	cleanup()
+	defer cleanup()
+
+	ctx := auth.NewContext(context.Background(), &auth.Credentials{Tokens: []string{accessToken}})
+	var exp1 time.Time
+	oidcAuthorizer.WrapCalls(func(ctx context.Context, opts interface{}) (interface{}, error) {
+		creds, ok := auth.FromContext(ctx)
+		c.Assert(ok, check.Equals, true)
+		c.Assert(creds.Tokens, check.HasLen, 1)
+		c.Check(creds.Tokens[0], check.Equals, accessToken)
+
+		err := db.QueryRowContext(ctx, `select expires_at at time zone 'UTC' from api_client_authorizations where api_token=$1`, hmac).Scan(&exp1)
+		c.Check(err, check.IsNil)
+		c.Check(exp1.Sub(time.Now()) > -time.Second, check.Equals, true)
+		c.Check(exp1.Sub(time.Now()) < time.Second, check.Equals, true)
+		return nil, nil
+	})(ctx, nil)
+
+	// If the token is used again after the in-memory cache
+	// expires, oidcAuthorizer must re-checks the token and update
+	// the expires_at value in the database.
+	time.Sleep(3 * time.Millisecond)
+	oidcAuthorizer.WrapCalls(func(ctx context.Context, opts interface{}) (interface{}, error) {
+		var exp time.Time
+		err := db.QueryRowContext(ctx, `select expires_at at time zone 'UTC' from api_client_authorizations where api_token=$1`, hmac).Scan(&exp)
+		c.Check(err, check.IsNil)
+		c.Check(exp.Sub(exp1) > 0, check.Equals, true)
+		c.Check(exp.Sub(exp1) < time.Second, check.Equals, true)
+		return nil, nil
+	})(ctx, nil)
+}
+
 func (s *OIDCLoginSuite) TestGenericOIDCLogin(c *check.C) {
 	s.cluster.Login.Google.Enable = false
 	s.cluster.Login.OpenIDConnect.Enable = true

commit f0d3eae5fc05aaad38a2998627c59637e3ef606c
Author: Tom Clegg <tom at curii.com>
Date:   Mon Jan 25 16:01:51 2021 -0500

    16669: Fix cache bug.
    
    When caching a valid token we were updating the database with
    exp=now+10m but updating the cache with exp="". When checking the
    cache, exp="" indicates we don't need to re-check the access token and
    extend the database entry's expiry field -- so we never did, we just
    passed through the HMAC, which RailsAPI rejected based on the expiry
    time in the database row.
    
    Arvados-DCO-1.1-Signed-off-by: Tom Clegg <tom at curii.com>

diff --git a/lib/controller/localdb/login_oidc.go b/lib/controller/localdb/login_oidc.go
index b99a1c2aa..1fa4da776 100644
--- a/lib/controller/localdb/login_oidc.go
+++ b/lib/controller/localdb/login_oidc.go
@@ -488,6 +488,7 @@ func (ta *oidcTokenAuthorizer) registerToken(ctx context.Context, tok string) er
 	if err != nil {
 		return err
 	}
+	aca.ExpiresAt = exp.Format(time.RFC3339Nano)
 	ta.cache.Add(tok, aca)
 	return nil
 }

-----------------------------------------------------------------------


hooks/post-receive
-- 




More information about the arvados-commits mailing list