From 65212b69064a52584911b79e68afa027a1de50f8 Mon Sep 17 00:00:00 2001 From: Todd Date: Mon, 25 Sep 2023 16:16:43 -0700 Subject: [PATCH] Remove expired auth tokens from cache (#3768) * Remove expired auth tokens from cache --- internal/cmd/commands/search/search_test.go | 7 ++-- internal/daemon/cache/repository_refresh.go | 2 +- .../daemon/cache/repository_refresh_test.go | 37 +++++++++-------- internal/daemon/cache/repository_token.go | 26 +++++++----- .../daemon/cache/repository_token_test.go | 40 ++++++++++++++----- internal/daemon/cache/schema.sql | 3 +- 6 files changed, 76 insertions(+), 39 deletions(-) diff --git a/internal/cmd/commands/search/search_test.go b/internal/cmd/commands/search/search_test.go index e50bd247feb..afc41d48625 100644 --- a/internal/cmd/commands/search/search_test.go +++ b/internal/cmd/commands/search/search_test.go @@ -50,9 +50,10 @@ func (r *testCommander) ReadTokenFromKeyring(k, a string) *authtokens.AuthToken func TestSearch(t *testing.T) { ctx := context.Background() at := &authtokens.AuthToken{ - Id: "at_1", - UserId: "user_1", - Token: "at_1_token", + Id: "at_1", + UserId: "user_1", + Token: "at_1_token", + ExpirationTime: time.Now().Add(time.Minute), } cmd := &testCommander{t: t, at: at} diff --git a/internal/daemon/cache/repository_refresh.go b/internal/daemon/cache/repository_refresh.go index 42781392ab9..1d070523b8b 100644 --- a/internal/daemon/cache/repository_refresh.go +++ b/internal/daemon/cache/repository_refresh.go @@ -102,7 +102,7 @@ func (r *Repository) cleanAndPickAuthTokens(ctx context.Context, u *user) (map[A // targets from a boundary address. func (r *Repository) Refresh(ctx context.Context, opt ...Option) error { const op = "cache.(Repository).Refresh" - if err := r.cleanOrphanedAuthTokens(ctx); err != nil { + if err := r.cleanExpiredOrOrphanedAuthTokens(ctx); err != nil { return errors.Wrap(ctx, err, op) } diff --git a/internal/daemon/cache/repository_refresh_test.go b/internal/daemon/cache/repository_refresh_test.go index 981a4395261..2fb41b19abe 100644 --- a/internal/daemon/cache/repository_refresh_test.go +++ b/internal/daemon/cache/repository_refresh_test.go @@ -9,6 +9,7 @@ import ( "fmt" "sync" "testing" + "time" "github.com/hashicorp/boundary/api/authtokens" "github.com/hashicorp/boundary/api/sessions" @@ -36,26 +37,30 @@ func TestCleanAndPickTokens(t *testing.T) { boundaryAddr := "address" u1 := &user{Id: "u1", Address: boundaryAddr} at1a := &authtokens.AuthToken{ - Id: "at_1a", - Token: "at_1a_token", - UserId: u1.Id, + Id: "at_1a", + Token: "at_1a_token", + UserId: u1.Id, + ExpirationTime: time.Now().Add(time.Minute), } at1b := &authtokens.AuthToken{ - Id: "at_1b", - Token: "at_1b_token", - UserId: u1.Id, + Id: "at_1b", + Token: "at_1b_token", + UserId: u1.Id, + ExpirationTime: time.Now().Add(time.Minute), } keyringOnlyUser := &user{Id: "keyringUser", Address: boundaryAddr} keyringAuthToken1 := &authtokens.AuthToken{ - Id: "at_2a", - Token: "at_2a_token", - UserId: keyringOnlyUser.Id, + Id: "at_2a", + Token: "at_2a_token", + UserId: keyringOnlyUser.Id, + ExpirationTime: time.Now().Add(time.Minute), } keyringAuthToken2 := &authtokens.AuthToken{ - Id: "at_2b", - Token: "at_2b_token", - UserId: keyringOnlyUser.Id, + Id: "at_2b", + Token: "at_2b_token", + UserId: keyringOnlyUser.Id, + ExpirationTime: time.Now().Add(time.Minute), } boundaryAuthTokens := []*authtokens.AuthToken{at1a, keyringAuthToken1, at1b, keyringAuthToken2} atMap := make(map[ringToken]*authtokens.AuthToken) @@ -149,10 +154,10 @@ func TestRefresh(t *testing.T) { boundaryAddr := "address" u := &user{Id: "u1", Address: boundaryAddr} at := &authtokens.AuthToken{ - Id: "at_1", - Token: "at_1_token", - UserId: u.Id, - AuthMethodId: "am_1", + Id: "at_1", + Token: "at_1_token", + UserId: u.Id, + ExpirationTime: time.Now().Add(time.Minute), } boundaryAuthTokens := []*authtokens.AuthToken{at} diff --git a/internal/daemon/cache/repository_token.go b/internal/daemon/cache/repository_token.go index c01d69f2a40..681247858ab 100644 --- a/internal/daemon/cache/repository_token.go +++ b/internal/daemon/cache/repository_token.go @@ -62,10 +62,11 @@ func upsertUserAndAuthToken(ctx context.Context, reader db.Reader, writer db.Wri Id: at.Id, UserId: at.UserId, LastAccessedTime: time.Now(), + ExpirationTime: at.ExpirationTime, } onConflict := &db.OnConflict{ Target: db.Columns{"id"}, - Action: db.SetColumns([]string{"last_accessed_time"}), + Action: db.SetColumns([]string{"last_accessed_time", "expiration_time"}), } if err := writer.Create(ctx, st, db.WithOnConflict(onConflict)); err != nil { return errors.Wrap(ctx, err, op) @@ -303,7 +304,7 @@ func (r *Repository) deleteKeyringToken(ctx context.Context, kt KeyringToken) er switch n { case 1: - if err := cleanOrphanedAuthTokens(ctx, writer, r.idToKeyringlessAuthToken); err != nil { + if err := cleanExpiredOrOrphanedAuthTokens(ctx, writer, r.idToKeyringlessAuthToken); err != nil { return errors.Wrap(ctx, err, op) } return nil @@ -319,12 +320,12 @@ func (r *Repository) deleteKeyringToken(ctx context.Context, kt KeyringToken) er return nil } -// cleanAuthTokens removes all tokens which are older than the staleness limit +// cleanExpiredOrOrphanedAuthTokens removes all tokens which are older than the staleness limit // or does not have either a keyring or keyringless reference to it. -func (r *Repository) cleanOrphanedAuthTokens(ctx context.Context) error { - const op = "cache.Repository.cleanOrphanedAuthTokens" +func (r *Repository) cleanExpiredOrOrphanedAuthTokens(ctx context.Context) error { + const op = "cache.Repository.cleanExpiredOrOrphanedAuthTokens" _, err := r.rw.DoTx(ctx, db.StdRetryCnt, db.ExpBackoff{}, func(reader db.Reader, writer db.Writer) error { - if err := cleanOrphanedAuthTokens(ctx, writer, r.idToKeyringlessAuthToken); err != nil { + if err := cleanExpiredOrOrphanedAuthTokens(ctx, writer, r.idToKeyringlessAuthToken); err != nil { return errors.Wrap(ctx, err, op) } return nil @@ -332,10 +333,10 @@ func (r *Repository) cleanOrphanedAuthTokens(ctx context.Context) error { return err } -// cleanAuthTokens removes all tokens which are older than the staleness limit +// cleanExpiredOrOrphanedAuthTokens removes all tokens which are older than the staleness limit // or does not have either a keyring or keyringless reference to it. -func cleanOrphanedAuthTokens(ctx context.Context, writer db.Writer, idToKeyringlessAuthToken *sync.Map) error { - const op = "cache.cleanAuthTokens" +func cleanExpiredOrOrphanedAuthTokens(ctx context.Context, writer db.Writer, idToKeyringlessAuthToken *sync.Map) error { + const op = "cache.cleanExpiredOrOrphanedAuthTokens" switch { // TODO: Add check here to see if a transaction is in flight. case util.IsNil(writer): @@ -354,6 +355,12 @@ func cleanOrphanedAuthTokens(ctx context.Context, writer db.Writer, idToKeyringl delete from auth_token where last_accessed_time < @last_accessed_time + or + -- sqlite stores expiration_time as a string in a format that might not match + -- what is being used by current_timestamp. Using datetime() makes the + -- formats match which allow the string comparison performed here to work + -- the same as a time comparison. + datetime(expiration_time) < current_timestamp or %s ` @@ -497,6 +504,7 @@ type AuthToken struct { Id string `gorm:"primaryKey"` UserId string `gorm:"default:null"` LastAccessedTime time.Time `gorm:"default:(strftime('%Y-%m-%d %H:%M:%f','now'))"` + ExpirationTime time.Time } func (*AuthToken) TableName() string { diff --git a/internal/daemon/cache/repository_token_test.go b/internal/daemon/cache/repository_token_test.go index 4ccdff4291d..299ba93e151 100644 --- a/internal/daemon/cache/repository_token_test.go +++ b/internal/daemon/cache/repository_token_test.go @@ -618,9 +618,10 @@ func TestRepository_RemoveStaleTokens(t *testing.T) { Address: "address", } at1 := &authtokens.AuthToken{ - Id: "at_1", - Token: "at_1_token", - UserId: u.Id, + Id: "at_1", + Token: "at_1_token", + UserId: u.Id, + ExpirationTime: time.Now().Add(1 * time.Minute), } kt1 := KeyringToken{ TokenName: "t1", @@ -628,25 +629,40 @@ func TestRepository_RemoveStaleTokens(t *testing.T) { AuthTokenId: at1.Id, } at2 := &authtokens.AuthToken{ - Id: "at_2", - Token: "at_2_token", - UserId: u.Id, + Id: "at_2", + Token: "at_2_token", + UserId: u.Id, + ExpirationTime: time.Now().Add(1 * time.Minute), } kt2 := KeyringToken{ TokenName: "t2", KeyringType: "k2", AuthTokenId: "at_2", } + // this auth token expired a minute ago + at3 := &authtokens.AuthToken{ + Id: "at_3", + Token: "at_3_token", + UserId: u.Id, + ExpirationTime: time.Now().Add(-(1 * time.Minute)), + } + kt3 := KeyringToken{ + TokenName: "t3", + KeyringType: "k3", + AuthTokenId: "at_3", + } - boundaryAuthTokens := []*authtokens.AuthToken{at1, at2} + boundaryAuthTokens := []*authtokens.AuthToken{at1, at2, at3} atMap := map[ringToken]*authtokens.AuthToken{ {kt1.KeyringType, kt1.TokenName}: at1, {kt2.KeyringType, kt2.TokenName}: at2, + {kt3.KeyringType, kt3.TokenName}: at3, } r, err := NewRepository(ctx, s, &sync.Map{}, mapBasedAuthTokenKeyringLookup(atMap), sliceBasedAuthTokenBoundaryReader(boundaryAuthTokens)) require.NoError(t, err) assert.NoError(t, r.AddKeyringToken(ctx, u.Address, kt1)) assert.NoError(t, r.AddKeyringToken(ctx, u.Address, kt2)) + assert.NoError(t, r.AddKeyringToken(ctx, u.Address, kt3)) staleTime := time.Now().Add(-(tokenStalenessLimit + 1*time.Hour)) freshTime := time.Now().Add(-(tokenStalenessLimit - 1*time.Hour)) @@ -657,6 +673,12 @@ func TestRepository_RemoveStaleTokens(t *testing.T) { } _, err = r.rw.Update(ctx, freshAt, []string{"LastAccessedTime"}, nil) require.NoError(t, err) + anotherFreshAt := &AuthToken{ + Id: at3.Id, + LastAccessedTime: freshTime, + } + _, err = r.rw.Update(ctx, anotherFreshAt, []string{"LastAccessedTime"}, nil) + require.NoError(t, err) staleAt := &AuthToken{ Id: at2.Id, @@ -667,9 +689,9 @@ func TestRepository_RemoveStaleTokens(t *testing.T) { lAt, err := r.listTokens(ctx, u) assert.NoError(t, err) - assert.Len(t, lAt, 2) + assert.Len(t, lAt, 3) - assert.NoError(t, r.cleanOrphanedAuthTokens(ctx)) + assert.NoError(t, r.cleanExpiredOrOrphanedAuthTokens(ctx)) lAt, err = r.listTokens(ctx, u) assert.NoError(t, err) diff --git a/internal/daemon/cache/schema.sql b/internal/daemon/cache/schema.sql index 94acdcc8de6..d6de11fd75f 100644 --- a/internal/daemon/cache/schema.sql +++ b/internal/daemon/cache/schema.sql @@ -25,7 +25,8 @@ create table if not exists auth_token ( -- the last time this the auth token was used on this machine to access -- boundary outside of the context of the cache. last_accessed_time timestamp not null - default (strftime('%Y-%m-%d %H:%M:%f','now')) + default (strftime('%Y-%m-%d %H:%M:%f','now')), + expiration_time timestamp not null ); -- *delete_orphaned_users triggers delete a user when it no longer has any