From e119466233eeb1b195a4145160f61ce71de981d1 Mon Sep 17 00:00:00 2001 From: Jim Date: Thu, 10 Sep 2020 16:37:45 -0400 Subject: [PATCH] stop oplogging tokens and allow for a time skew (#343) --- internal/authtoken/repository.go | 52 +++++++++------------------ internal/authtoken/repository_test.go | 16 ++++++--- 2 files changed, 28 insertions(+), 40 deletions(-) diff --git a/internal/authtoken/repository.go b/internal/authtoken/repository.go index bd6ce0fbbc..8aed35a467 100644 --- a/internal/authtoken/repository.go +++ b/internal/authtoken/repository.go @@ -12,7 +12,6 @@ import ( "github.com/hashicorp/boundary/internal/db/timestamp" "github.com/hashicorp/boundary/internal/iam" "github.com/hashicorp/boundary/internal/kms" - "github.com/hashicorp/boundary/internal/oplog" ) // TODO (ICU-406): Make these fields configurable. @@ -20,6 +19,7 @@ var ( lastAccessedUpdateDuration = 10 * time.Minute maxStaleness = 24 * time.Hour maxTokenDuration = 7 * 24 * time.Hour + timeSkew = time.Duration(0) ) // A Repository stores and retrieves the persistent types in the authtoken @@ -86,10 +86,6 @@ func (r *Repository) CreateAuthToken(ctx context.Context, withIamUser *iam.User, } at.Token = token - oplogWrapper, err := r.kms.GetWrapper(ctx, withIamUser.GetScopeId(), kms.KeyPurposeOplog) - if err != nil { - return nil, fmt.Errorf("create: unable to get oplog wrapper: %w", err) - } databaseWrapper, err := r.kms.GetWrapper(ctx, withIamUser.GetScopeId(), kms.KeyPurposeDatabase) if err != nil { return nil, fmt.Errorf("create: unable to get database wrapper: %w", err) @@ -122,12 +118,12 @@ func (r *Repository) CreateAuthToken(ctx context.Context, withIamUser *iam.User, at.AuthMethodId = acct.GetAuthMethodId() at.IamUserId = acct.GetIamUserId() - metadata := newAuthTokenMetadata(at, oplog.OpType_OP_TYPE_CREATE) newAuthToken = at.toWritableAuthToken() if err := newAuthToken.encrypt(ctx, databaseWrapper); err != nil { return err } - if err := w.Create(ctx, newAuthToken, db.WithOplog(oplogWrapper, metadata)); err != nil { + // tokens are not replicated, so they don't need oplog entries. + if err := w.Create(ctx, newAuthToken); err != nil { return err } newAuthToken.CtToken = nil @@ -211,28 +207,28 @@ func (r *Repository) ValidateToken(ctx context.Context, id, token string, opt .. return nil, fmt.Errorf("validate token: last accessed time : %w", err) } - oplogWrapper, err := r.kms.GetWrapper(ctx, retAT.GetScopeId(), kms.KeyPurposeOplog) - if err != nil { - return nil, fmt.Errorf("validate token: unable to get oplog wrapper: %w", err) - } - now := time.Now() - sinceLastAccessed := now.Sub(lastAccessed) - if now.After(exp) || sinceLastAccessed > maxStaleness { + sinceLastAccessed := now.Sub(lastAccessed) + timeSkew + // TODO (jimlambrt 9/2020) - investigate the need for the timeSkew and see + // if it can be eliminated. + if now.After(exp.Add(-timeSkew)) || sinceLastAccessed >= maxStaleness { // If the token has expired or has become too stale, delete it from the DB. _, err = r.writer.DoTx( ctx, db.StdRetryCnt, db.ExpBackoff{}, func(_ db.Reader, w db.Writer) error { - metadata := newAuthTokenMetadata(retAT, oplog.OpType_OP_TYPE_DELETE) delAt := retAT.toWritableAuthToken() - if _, err := w.Delete(ctx, delAt, db.WithOplog(oplogWrapper, metadata)); err != nil { + // tokens are not replicated, so they don't need oplog entries. + if _, err := w.Delete(ctx, delAt); err != nil { return fmt.Errorf("validate token: delete auth token: %w", err) } retAT = nil return nil }) + if err != nil { + return nil, err + } return nil, nil } @@ -251,16 +247,15 @@ func (r *Repository) ValidateToken(ctx context.Context, id, token string, opt .. db.StdRetryCnt, db.ExpBackoff{}, func(_ db.Reader, w db.Writer) error { - metadata := newAuthTokenMetadata(retAT, oplog.OpType_OP_TYPE_UPDATE) at := retAT.toWritableAuthToken() // Setting the ApproximateLastAccessTime to null through using the null mask allows a defined db's - // trigger to set ApproximateLastAccessTime to the commit timestamp. + // trigger to set ApproximateLastAccessTime to the commit + // timestamp. Tokens are not replicated, so they don't need oplog entries. rowsUpdated, err := w.Update( ctx, at, nil, []string{"ApproximateLastAccessTime"}, - db.WithOplog(oplogWrapper, metadata), ) if err == nil && rowsUpdated > 1 { return db.ErrMultipleRecords @@ -317,20 +312,15 @@ func (r *Repository) DeleteAuthToken(ctx context.Context, id string, opt ...Opti return db.NoRowsAffected, nil } - oplogWrapper, err := r.kms.GetWrapper(ctx, at.GetScopeId(), kms.KeyPurposeOplog) - if err != nil { - return db.NoRowsAffected, fmt.Errorf("delete: unable to get oplog wrapper: %w", err) - } - var rowsDeleted int _, err = r.writer.DoTx( ctx, db.StdRetryCnt, db.ExpBackoff{}, func(_ db.Reader, w db.Writer) error { - metadata := newAuthTokenMetadata(at, oplog.OpType_OP_TYPE_DELETE) deleteAT := at.toWritableAuthToken() - rowsDeleted, err = w.Delete(ctx, deleteAT, db.WithOplog(oplogWrapper, metadata)) + // tokens are not replicated, so they don't need oplog entries. + rowsDeleted, err = w.Delete(ctx, deleteAT) if err == nil && rowsDeleted > 1 { return db.ErrMultipleRecords } @@ -351,13 +341,3 @@ func allocAuthToken() *AuthToken { } return fresh } - -func newAuthTokenMetadata(at *AuthToken, op oplog.OpType) oplog.Metadata { - metadata := oplog.Metadata{ - "scope-id": []string{at.GetScopeId()}, - "resource-public-id": []string{at.GetPublicId()}, - "resource-type": []string{"auth token"}, - "op-type": []string{op.String()}, - } - return metadata -} diff --git a/internal/authtoken/repository_test.go b/internal/authtoken/repository_test.go index 90d734db93..05723e8399 100644 --- a/internal/authtoken/repository_test.go +++ b/internal/authtoken/repository_test.go @@ -211,7 +211,8 @@ func TestRepository_CreateAuthToken(t *testing.T) { assert.Equal(tt.authAcctId, got.GetAuthAccountId()) assert.Equal(got.CreateTime, got.UpdateTime) assert.Equal(got.CreateTime, got.ApproximateLastAccessTime) - assert.NoError(db.TestVerifyOplog(t, rw, got.GetPublicId(), db.WithOperation(oplog.OpType_OP_TYPE_CREATE))) + // We should find no oplog since tokens are not replicated, so they don't need oplog entries. + assert.Error(db.TestVerifyOplog(t, rw, got.GetPublicId(), db.WithOperation(oplog.OpType_OP_TYPE_CREATE))) }) } } @@ -294,6 +295,7 @@ func TestRepository_LookupAuthToken(t *testing.T) { func TestRepository_ValidateToken(t *testing.T) { conn, _ := db.TestSetup(t, "postgres") lastAccessedUpdateDuration = 0 + timeSkew = 20 * time.Millisecond rw := db.New(conn) wrapper := db.TestWrapper(t) @@ -360,6 +362,7 @@ func TestRepository_ValidateToken(t *testing.T) { t.Run(tt.name, func(t *testing.T) { assert, require := assert.New(t), require.New(t) got, err := repo.ValidateToken(context.Background(), tt.id, tt.token) + if tt.wantErr != nil { assert.Truef(errors.Is(err, tt.wantErr), "want err: %q got: %q", tt.wantErr, err) return @@ -396,9 +399,11 @@ func TestRepository_ValidateToken(t *testing.T) { require.NoError(err) assert.True(preTime2.After(preTime1), "First updated time %q was not after the creation time %q", preTime2, preTime1) - assert.NoError(db.TestVerifyOplog(t, rw, got.GetPublicId(), db.WithOperation(oplog.OpType_OP_TYPE_UPDATE))) + // We should find no oplog since tokens are not replicated, so they don't need oplog entries. + assert.Error(db.TestVerifyOplog(t, rw, got.GetPublicId(), db.WithOperation(oplog.OpType_OP_TYPE_UPDATE))) got3, err := repo.ValidateToken(context.Background(), tt.id, tt.token) + require.NoError(err) preTime3, err := ptypes.Timestamp(got3.GetApproximateLastAccessTime().GetTimestamp()) require.NoError(err) assert.True(preTime3.Equal(preTime2), "The 3rd timestamp %q was not equal to the second time %q", preTime3, preTime2) @@ -461,6 +466,7 @@ func TestRepository_ValidateToken_expired(t *testing.T) { maxStaleness = tt.staleDuration maxTokenDuration = tt.expirationDuration + timeSkew = 20 * time.Millisecond ctx := context.Background() at, err := repo.CreateAuthToken(ctx, iamUser, baseAT.GetAuthAccountId()) @@ -472,7 +478,8 @@ func TestRepository_ValidateToken_expired(t *testing.T) { if tt.wantReturned { assert.NotNil(got) } else { - assert.NoError(db.TestVerifyOplog(t, rw, at.GetPublicId(), db.WithOperation(oplog.OpType_OP_TYPE_DELETE))) + // We should find no oplog since tokens are not replicated, so they don't need oplog entries. + assert.Error(db.TestVerifyOplog(t, rw, at.GetPublicId(), db.WithOperation(oplog.OpType_OP_TYPE_DELETE))) assert.Nil(got) } @@ -535,7 +542,8 @@ func TestRepository_DeleteAuthToken(t *testing.T) { assert.NoError(err) assert.Equal(tt.want, got, "row count") if tt.want != 0 { - assert.NoError(db.TestVerifyOplog(t, rw, tt.id, db.WithOperation(oplog.OpType_OP_TYPE_DELETE))) + // We should find no oplog since tokens are not replicated, so they don't need oplog entries. + assert.Error(db.TestVerifyOplog(t, rw, tt.id, db.WithOperation(oplog.OpType_OP_TYPE_DELETE))) } }) }