From 2b2241571e5cdc4aee8f6022f8c1946dff537958 Mon Sep 17 00:00:00 2001 From: Calvin Leung Huang <1883212+calvn@users.noreply.github.com> Date: Wed, 22 Sep 2021 08:57:57 -0700 Subject: [PATCH] Port: Premature Rotation For autorotate (#12563) (#12605) * port of ldap fix for early cred rotation * some more porting * another couple lines to port * final commits before report * remove deadlock * needs testing * updates * Sync with OpenLDAP PR * Update the update error handling for items not found in the queue * WIP unit tests * Need to configure DB mount correctly, with db type mockv5 * Need to find a way to inject errors into that mock db * throw error on role creation failure * do not swallow error on role creation * comment out wip tests and add in a test for disallowed role * Use newly generated password in WAL Co-authored-by: Michael Golowka <72365+pcman312@users.noreply.github.com> * return err on popFromRotationQueueByKey error; cleanup on setStaticAccount * test: fix TestPlugin_lifecycle * Uncomment and fix unit tests * Use mock database plugin to inject errors * Tidy test code to rely less on code internals where possible * Some stronger test assertions * Undo logging updates * Add changelog * Remove ticker and background threads from WAL tests * Keep pre-existing API behaviour of allowing update static role to act as a create * Switch test back to update operation * Revert my revert, and fix some test bugs * Fix TestBackend_StaticRole_LockRegression * clean up defer on TestPlugin_lifecycle * unwrap reqs on cleanup * setStaticAccount: don't hold a write lock * TestStoredWALsCorrectlyProcessed: set replication state to unknown Co-authored-by: Tom Proctor Co-authored-by: Michael Golowka <72365+pcman312@users.noreply.github.com> Co-authored-by: Calvin Leung Huang <1883212+calvn@users.noreply.github.com> Co-authored-by: Hridoy Roy Co-authored-by: Tom Proctor Co-authored-by: Michael Golowka <72365+pcman312@users.noreply.github.com> --- builtin/logical/database/path_roles.go | 68 +++- builtin/logical/database/path_roles_test.go | 151 +++++++- .../database/path_rotate_credentials.go | 17 +- builtin/logical/database/rotation.go | 139 +++++--- builtin/logical/database/rotation_test.go | 326 +++++++++++++++++- .../logical/database/versioning_large_test.go | 10 +- changelog/12563.txt | 3 + 7 files changed, 634 insertions(+), 80 deletions(-) create mode 100644 changelog/12563.txt diff --git a/builtin/logical/database/path_roles.go b/builtin/logical/database/path_roles.go index dcaeb23432f61..32f30082f9c43 100644 --- a/builtin/logical/database/path_roles.go +++ b/builtin/logical/database/path_roles.go @@ -6,6 +6,7 @@ import ( "strings" "time" + "github.com/hashicorp/go-multierror" v4 "github.com/hashicorp/vault/sdk/database/dbplugin" "github.com/hashicorp/vault/sdk/framework" "github.com/hashicorp/vault/sdk/helper/locksutil" @@ -16,7 +17,7 @@ import ( func pathListRoles(b *databaseBackend) []*framework.Path { return []*framework.Path{ - &framework.Path{ + { Pattern: "roles/?$", Callbacks: map[logical.Operation]framework.OperationFunc{ @@ -26,7 +27,7 @@ func pathListRoles(b *databaseBackend) []*framework.Path { HelpSynopsis: pathRoleHelpSyn, HelpDescription: pathRoleHelpDesc, }, - &framework.Path{ + { Pattern: "static-roles/?$", Callbacks: map[logical.Operation]framework.OperationFunc{ @@ -41,7 +42,7 @@ func pathListRoles(b *databaseBackend) []*framework.Path { func pathRoles(b *databaseBackend) []*framework.Path { return []*framework.Path{ - &framework.Path{ + { Pattern: "roles/" + framework.GenericNameRegex("name"), Fields: fieldsForType(databaseRolePath), ExistenceCheck: b.pathRoleExistenceCheck, @@ -56,7 +57,7 @@ func pathRoles(b *databaseBackend) []*framework.Path { HelpDescription: pathRoleHelpDesc, }, - &framework.Path{ + { Pattern: "static-roles/" + framework.GenericNameRegex("name"), Fields: fieldsForType(databaseStaticRolePath), ExistenceCheck: b.pathStaticRoleExistenceCheck, @@ -215,7 +216,28 @@ func (b *databaseBackend) pathStaticRoleDelete(ctx context.Context, req *logical return nil, err } - return nil, nil + walIDs, err := framework.ListWAL(ctx, req.Storage) + if err != nil { + return nil, err + } + var merr *multierror.Error + for _, walID := range walIDs { + wal, err := b.findStaticWAL(ctx, req.Storage, walID) + if err != nil { + merr = multierror.Append(merr, err) + continue + } + if wal != nil && name == wal.RoleName { + b.Logger().Debug("deleting WAL for deleted role", "WAL ID", walID, "role", name) + err = framework.DeleteWAL(ctx, req.Storage, walID) + if err != nil { + b.Logger().Debug("failed to delete WAL for deleted role", "WAL ID", walID, "error", err) + merr = multierror.Append(merr, err) + } + } + } + + return nil, merr.ErrorOrNil() } func (b *databaseBackend) pathStaticRoleRead(ctx context.Context, req *logical.Request, d *framework.FieldData) (*logical.Response, error) { @@ -482,19 +504,34 @@ func (b *databaseBackend) pathStaticRoleCreateUpdate(ctx context.Context, req *l // Only call setStaticAccount if we're creating the role for the // first time + var item *queue.Item switch req.Operation { case logical.CreateOperation: // setStaticAccount calls Storage.Put and saves the role to storage resp, err := b.setStaticAccount(ctx, req.Storage, &setStaticAccountInput{ - RoleName: name, - Role: role, - CreateUser: createRole, + RoleName: name, + Role: role, }) if err != nil { + if resp != nil && resp.WALID != "" { + b.Logger().Debug("deleting WAL for failed role creation", "WAL ID", resp.WALID, "role", name) + walDeleteErr := framework.DeleteWAL(ctx, req.Storage, resp.WALID) + if walDeleteErr != nil { + b.Logger().Debug("failed to delete WAL for failed role creation", "WAL ID", resp.WALID, "error", walDeleteErr) + var merr *multierror.Error + merr = multierror.Append(merr, err) + merr = multierror.Append(merr, fmt.Errorf("failed to clean up WAL from failed role creation: %w", walDeleteErr)) + err = merr.ErrorOrNil() + } + } + return nil, err } // guard against RotationTime not being set or zero-value lvr = resp.RotationTime + item = &queue.Item{ + Key: name, + } case logical.UpdateOperation: // store updated Role entry, err := logical.StorageEntryJSON(databaseStaticRolePath+name, role) @@ -504,17 +541,16 @@ func (b *databaseBackend) pathStaticRoleCreateUpdate(ctx context.Context, req *l if err := req.Storage.Put(ctx, entry); err != nil { return nil, err } - - // In case this is an update, remove any previous version of the item from - // the queue - b.popFromRotationQueueByKey(name) + item, err = b.popFromRotationQueueByKey(name) + if err != nil { + return nil, err + } } + item.Priority = lvr.Add(role.StaticAccount.RotationPeriod).Unix() + // Add their rotation to the queue - if err := b.pushItem(&queue.Item{ - Key: name, - Priority: lvr.Add(role.StaticAccount.RotationPeriod).Unix(), - }); err != nil { + if err := b.pushItem(item); err != nil { return nil, err } diff --git a/builtin/logical/database/path_roles_test.go b/builtin/logical/database/path_roles_test.go index f4b6d56b6be76..a2c87ec7f480f 100644 --- a/builtin/logical/database/path_roles_test.go +++ b/builtin/logical/database/path_roles_test.go @@ -3,13 +3,16 @@ package database import ( "context" "errors" + "strings" "testing" "time" "github.com/go-test/deep" "github.com/hashicorp/vault/helper/namespace" postgreshelper "github.com/hashicorp/vault/helper/testhelpers/postgresql" + v5 "github.com/hashicorp/vault/sdk/database/dbplugin/v5" "github.com/hashicorp/vault/sdk/logical" + "github.com/stretchr/testify/mock" ) var dataKeys = []string{"username", "password", "last_vault_rotation", "rotation_period"} @@ -43,7 +46,7 @@ func TestBackend_StaticRole_Config(t *testing.T) { "connection_url": connURL, "plugin_name": "postgresql-database-plugin", "verify_connection": false, - "allowed_roles": []string{"*"}, + "allowed_roles": []string{"plugin-role-test"}, "name": "plugin-test", } req := &logical.Request{ @@ -61,6 +64,7 @@ func TestBackend_StaticRole_Config(t *testing.T) { // ordering, so each case cleans up by deleting the role testCases := map[string]struct { account map[string]interface{} + path string expected map[string]interface{} err error }{ @@ -69,6 +73,7 @@ func TestBackend_StaticRole_Config(t *testing.T) { "username": dbUser, "rotation_period": "5400s", }, + path: "plugin-role-test", expected: map[string]interface{}{ "username": dbUser, "rotation_period": float64(5400), @@ -78,7 +83,16 @@ func TestBackend_StaticRole_Config(t *testing.T) { account: map[string]interface{}{ "username": dbUser, }, - err: errors.New("rotation_period is required to create static accounts"), + path: "plugin-role-test", + err: errors.New("rotation_period is required to create static accounts"), + }, + "disallowed role config": { + account: map[string]interface{}{ + "username": dbUser, + "rotation_period": "5400s", + }, + path: "disallowed-role", + err: errors.New("\"disallowed-role\" is not an allowed role"), }, } @@ -94,9 +108,11 @@ func TestBackend_StaticRole_Config(t *testing.T) { data[k] = v } + path := "static-roles/" + tc.path + req := &logical.Request{ Operation: logical.CreateOperation, - Path: "static-roles/plugin-role-test", + Path: path, Storage: config.StorageView, Data: data, } @@ -510,6 +526,135 @@ func TestBackend_StaticRole_Role_name_check(t *testing.T) { } } +func TestWALsStillTrackedAfterUpdate(t *testing.T) { + ctx := context.Background() + b, storage, mockDB := getBackend(t) + defer b.Cleanup(ctx) + configureDBMount(t, storage) + + createRole(t, b, storage, mockDB, "hashicorp") + + generateWALFromFailedRotation(t, b, storage, mockDB, "hashicorp") + requireWALs(t, storage, 1) + + resp, err := b.HandleRequest(ctx, &logical.Request{ + Operation: logical.UpdateOperation, + Path: "static-roles/hashicorp", + Storage: storage, + Data: map[string]interface{}{ + "username": "hashicorp", + "dn": "uid=hashicorp,ou=users,dc=hashicorp,dc=com", + "rotation_period": "600s", + }, + }) + if err != nil || (resp != nil && resp.IsError()) { + t.Fatal(resp, err) + } + walIDs := requireWALs(t, storage, 1) + + // Now when we trigger a manual rotate, it should use the WAL's new password + // which will tell us that the in-memory structure still kept track of the + // WAL in addition to it still being in storage. + wal, err := b.findStaticWAL(ctx, storage, walIDs[0]) + if err != nil { + t.Fatal(err) + } + rotateRole(t, b, storage, mockDB, "hashicorp") + role, err := b.StaticRole(ctx, storage, "hashicorp") + if err != nil { + t.Fatal(err) + } + if role.StaticAccount.Password != wal.NewPassword { + t.Fatal() + } + requireWALs(t, storage, 0) +} + +func TestWALsDeletedOnRoleCreationFailed(t *testing.T) { + ctx := context.Background() + b, storage, mockDB := getBackend(t) + defer b.Cleanup(ctx) + configureDBMount(t, storage) + + for i := 0; i < 3; i++ { + mockDB.On("UpdateUser", mock.Anything, mock.Anything). + Return(v5.UpdateUserResponse{}, errors.New("forced error")). + Once() + resp, err := b.HandleRequest(ctx, &logical.Request{ + Operation: logical.CreateOperation, + Path: "static-roles/hashicorp", + Storage: storage, + Data: map[string]interface{}{ + "username": "hashicorp", + "db_name": "mockv5", + "rotation_period": "5s", + }, + }) + if err == nil { + t.Fatal("expected error from DB") + } + if !strings.Contains(err.Error(), "forced error") { + t.Fatal("expected forced error message", resp, err) + } + } + + requireWALs(t, storage, 0) +} + +func TestWALsDeletedOnRoleDeletion(t *testing.T) { + ctx := context.Background() + b, storage, mockDB := getBackend(t) + defer b.Cleanup(ctx) + configureDBMount(t, storage) + + // Create the roles + roleNames := []string{"hashicorp", "2"} + for _, roleName := range roleNames { + createRole(t, b, storage, mockDB, roleName) + } + + // Fail to rotate the roles + for _, roleName := range roleNames { + generateWALFromFailedRotation(t, b, storage, mockDB, roleName) + } + + // Should have 2 WALs hanging around + requireWALs(t, storage, 2) + + // Delete one of the static roles + resp, err := b.HandleRequest(ctx, &logical.Request{ + Operation: logical.DeleteOperation, + Path: "static-roles/hashicorp", + Storage: storage, + }) + if err != nil || (resp != nil && resp.IsError()) { + t.Fatal(resp, err) + } + + // 1 WAL should be cleared by the delete + requireWALs(t, storage, 1) +} + +func createRole(t *testing.T, b *databaseBackend, storage logical.Storage, mockDB *mockNewDatabase, roleName string) { + t.Helper() + mockDB.On("UpdateUser", mock.Anything, mock.Anything). + Return(v5.UpdateUserResponse{}, nil). + Once() + resp, err := b.HandleRequest(context.Background(), &logical.Request{ + Operation: logical.CreateOperation, + Path: "static-roles/" + roleName, + Storage: storage, + Data: map[string]interface{}{ + "username": roleName, + "db_name": "mockv5", + "rotation_period": "86400s", + }, + }) + if err != nil || (resp != nil && resp.IsError()) { + t.Fatal(resp, err) + } +} + const testRoleStaticCreate = ` CREATE ROLE "{{name}}" WITH LOGIN diff --git a/builtin/logical/database/path_rotate_credentials.go b/builtin/logical/database/path_rotate_credentials.go index 6faee56f34cd7..8cc1b6b491a23 100644 --- a/builtin/logical/database/path_rotate_credentials.go +++ b/builtin/logical/database/path_rotate_credentials.go @@ -169,10 +169,14 @@ func (b *databaseBackend) pathRotateRoleCredentialsUpdate() framework.OperationF } } - resp, err := b.setStaticAccount(ctx, req.Storage, &setStaticAccountInput{ + input := &setStaticAccountInput{ RoleName: name, Role: role, - }) + } + if walID, ok := item.Value.(string); ok { + input.WALID = walID + } + resp, err := b.setStaticAccount(ctx, req.Storage, input) // if err is not nil, we need to attempt to update the priority and place // this item back on the queue. The err should still be returned at the end // of this method. @@ -188,6 +192,8 @@ func (b *databaseBackend) pathRotateRoleCredentialsUpdate() framework.OperationF } } else { item.Priority = resp.RotationTime.Add(role.StaticAccount.RotationPeriod).Unix() + // Clear any stored WAL ID as we must have successfully deleted our WAL to get here. + item.Value = "" } // Add their rotation to the queue @@ -195,8 +201,13 @@ func (b *databaseBackend) pathRotateRoleCredentialsUpdate() framework.OperationF return nil, err } + if err != nil { + return nil, fmt.Errorf("unable to finish rotating credentials; retries will "+ + "continue in the background but it is also safe to retry manually: %w", err) + } + // return any err from the setStaticAccount call - return nil, err + return nil, nil } } diff --git a/builtin/logical/database/rotation.go b/builtin/logical/database/rotation.go index 0f61b819d5036..61161784a36bb 100644 --- a/builtin/logical/database/rotation.go +++ b/builtin/logical/database/rotation.go @@ -8,8 +8,6 @@ import ( "time" "github.com/hashicorp/errwrap" - "github.com/hashicorp/go-multierror" - v4 "github.com/hashicorp/vault/sdk/database/dbplugin" v5 "github.com/hashicorp/vault/sdk/database/dbplugin/v5" "github.com/hashicorp/vault/sdk/framework" "github.com/hashicorp/vault/sdk/helper/consts" @@ -67,21 +65,34 @@ func (b *databaseBackend) populateQueue(ctx context.Context, s logical.Storage) item := queue.Item{ Key: roleName, - Priority: role.StaticAccount.LastVaultRotation.Add(role.StaticAccount.RotationPeriod).Unix(), + Priority: role.StaticAccount.NextRotationTime().Unix(), } // Check if role name is in map walEntry := walMap[roleName] if walEntry != nil { // Check walEntry last vault time - if !walEntry.LastVaultRotation.IsZero() && walEntry.LastVaultRotation.Before(role.StaticAccount.LastVaultRotation) { + if walEntry.LastVaultRotation.IsZero() { + // A WAL's last Vault rotation can only ever be 0 for a role that + // was never successfully created. So we know this WAL couldn't + // have been created for this role we just retrieved from storage. + // i.e. it must be a hangover from a previous attempt at creating + // a role with the same name + log.Debug("deleting WAL with zero last rotation time", "WAL ID", walEntry.walID, "created", walEntry.walCreatedAt) + if err := framework.DeleteWAL(ctx, s, walEntry.walID); err != nil { + log.Warn("unable to delete zero-time WAL", "error", err, "WAL ID", walEntry.walID) + } + } else if walEntry.LastVaultRotation.Before(role.StaticAccount.LastVaultRotation) { // WAL's last vault rotation record is older than the role's data, so // delete and move on + log.Debug("deleting outdated WAL", "WAL ID", walEntry.walID, "created", walEntry.walCreatedAt) if err := framework.DeleteWAL(ctx, s, walEntry.walID); err != nil { log.Warn("unable to delete WAL", "error", err, "WAL ID", walEntry.walID) } } else { - log.Info("adjusting priority for Role") + log.Info("found WAL for role", + "role", item.Key, + "WAL ID", walEntry.walID) item.Value = walEntry.walID item.Priority = time.Now().Unix() } @@ -115,13 +126,14 @@ func (b *databaseBackend) runTicker(ctx context.Context, queueTickInterval time. // credential setting or rotation in the event of partial failure. type setCredentialsWAL struct { NewPassword string `json:"new_password"` - OldPassword string `json:"old_password"` RoleName string `json:"role_name"` Username string `json:"username"` LastVaultRotation time.Time `json:"last_vault_rotation"` - walID string + // Private fields which will not be included in json.Marshal/Unmarshal. + walID string + walCreatedAt int64 // Unix time at which the WAL was created. } // rotateCredentials sets a new password for a static account. This method is @@ -195,18 +207,7 @@ func (b *databaseBackend) rotateCredential(ctx context.Context, s logical.Storag // If there is a WAL entry related to this Role, the corresponding WAL ID // should be stored in the Item's Value field. if walID, ok := item.Value.(string); ok { - walEntry, err := b.findStaticWAL(ctx, s, walID) - if err != nil { - b.logger.Error("error finding static WAL", "error", err) - item.Priority = time.Now().Add(10 * time.Second).Unix() - if err := b.pushItem(item); err != nil { - b.logger.Error("unable to push item on to queue", "error", err) - } - } - if walEntry != nil && walEntry.NewPassword != "" { - input.Password = walEntry.NewPassword - input.WALID = walID - } + input.WALID = walID } resp, err := b.setStaticAccount(ctx, s, input) @@ -227,6 +228,8 @@ func (b *databaseBackend) rotateCredential(ctx context.Context, s logical.Storag // Go to next item return true } + // Clear any stored WAL ID as we must have successfully deleted our WAL to get here. + item.Value = "" lvr := resp.RotationTime if lvr.IsZero() { @@ -256,11 +259,11 @@ func (b *databaseBackend) findStaticWAL(ctx context.Context, s logical.Storage, data := wal.Data.(map[string]interface{}) walEntry := setCredentialsWAL{ - walID: id, - NewPassword: data["new_password"].(string), - OldPassword: data["old_password"].(string), - RoleName: data["role_name"].(string), - Username: data["username"].(string), + walID: id, + walCreatedAt: wal.CreatedAt, + NewPassword: data["new_password"].(string), + RoleName: data["role_name"].(string), + Username: data["username"].(string), } lvr, err := time.Parse(time.RFC3339, data["last_vault_rotation"].(string)) if err != nil { @@ -272,16 +275,13 @@ func (b *databaseBackend) findStaticWAL(ctx context.Context, s logical.Storage, } type setStaticAccountInput struct { - RoleName string - Role *roleEntry - Password string - CreateUser bool - WALID string + RoleName string + Role *roleEntry + WALID string } type setStaticAccountOutput struct { RotationTime time.Time - Password string // Optional return field, in the event WAL was created and not destroyed // during the operation WALID string @@ -301,7 +301,6 @@ type setStaticAccountOutput struct { // This method does not perform any operations on the priority queue. Those // tasks must be handled outside of this method. func (b *databaseBackend) setStaticAccount(ctx context.Context, s logical.Storage, input *setStaticAccountInput) (*setStaticAccountOutput, error) { - var merr error if input == nil || input.Role == nil || input.RoleName == "" { return nil, errors.New("input was empty when attempting to set credentials for static account") } @@ -312,6 +311,9 @@ func (b *databaseBackend) setStaticAccount(ctx context.Context, s logical.Storag if err != nil { return output, err } + if dbConfig == nil { + return output, errors.New("the config is currently unset") + } // If role name isn't in the database's allowed roles, send back a // permission denied. @@ -331,31 +333,47 @@ func (b *databaseBackend) setStaticAccount(ctx context.Context, s logical.Storag // Use password from input if available. This happens if we're restoring from // a WAL item or processing the rotation queue with an item that has a WAL // associated with it - newPassword := input.Password - if newPassword == "" { - newPassword, err = dbi.database.GeneratePassword(ctx, b.System(), dbConfig.PasswordPolicy) + var newPassword string + if output.WALID != "" { + wal, err := b.findStaticWAL(ctx, s, output.WALID) if err != nil { - return output, err + return output, errwrap.Wrapf("error retrieving WAL entry: {{err}}", err) } - } - output.Password = newPassword - config := v4.StaticUserConfig{ - Username: input.Role.StaticAccount.Username, - Password: newPassword, + switch { + case wal != nil && wal.NewPassword != "": + newPassword = wal.NewPassword + default: + if wal == nil { + b.Logger().Error("expected role to have WAL, but WAL not found in storage", "role", input.RoleName, "WAL ID", output.WALID) + } else { + b.Logger().Error("expected WAL to have a new password set, but empty", "role", input.RoleName, "WAL ID", output.WALID) + err = framework.DeleteWAL(ctx, s, output.WALID) + if err != nil { + b.Logger().Warn("failed to delete WAL with no new password", "error", err, "WAL ID", output.WALID) + } + } + // If there's anything wrong with the WAL in storage, we'll need + // to generate a fresh WAL and password + output.WALID = "" + } } if output.WALID == "" { + newPassword, err = dbi.database.GeneratePassword(ctx, b.System(), dbConfig.PasswordPolicy) + if err != nil { + return output, err + } output.WALID, err = framework.PutWAL(ctx, s, staticWALKey, &setCredentialsWAL{ RoleName: input.RoleName, - Username: config.Username, - NewPassword: config.Password, - OldPassword: input.Role.StaticAccount.Password, + Username: input.Role.StaticAccount.Username, + NewPassword: newPassword, LastVaultRotation: input.Role.StaticAccount.LastVaultRotation, }) if err != nil { return output, errwrap.Wrapf("error writing WAL entry: {{err}}", err) } + b.Logger().Debug("writing WAL", "role", input.RoleName, "WAL ID", output.WALID) } updateReq := v5.UpdateUserRequest{ @@ -390,12 +408,13 @@ func (b *databaseBackend) setStaticAccount(ctx context.Context, s logical.Storag // Cleanup WAL after successfully rotating and pushing new item on to queue if err := framework.DeleteWAL(ctx, s, output.WALID); err != nil { - merr = multierror.Append(merr, err) - return output, merr + b.Logger().Warn("error deleting WAL", "WAL ID", output.WALID, "error", err) + return output, err } + b.Logger().Debug("deleted WAL", "WAL ID", output.WALID) // The WAL has been deleted, return new setStaticAccountOutput without it - return &setStaticAccountOutput{RotationTime: lvr}, merr + return &setStaticAccountOutput{RotationTime: lvr}, nil } // initQueue preforms the necessary checks and initializations needed to perform @@ -494,13 +513,39 @@ func (b *databaseBackend) loadStaticWALs(ctx context.Context, s logical.Storage) continue } if role == nil || role.StaticAccount == nil { + b.Logger().Debug("deleting WAL with nil role or static account", "WAL ID", walEntry.walID) if err := framework.DeleteWAL(ctx, s, walEntry.walID); err != nil { b.Logger().Warn("unable to delete WAL", "error", err, "WAL ID", walEntry.walID) } continue } - walEntry.walID = walID + if existingWALEntry, exists := walMap[walEntry.RoleName]; exists { + b.Logger().Debug("multiple WALs detected for role", "role", walEntry.RoleName, + "loaded WAL ID", existingWALEntry.walID, "created at", existingWALEntry.walCreatedAt, "last vault rotation", existingWALEntry.LastVaultRotation, + "candidate WAL ID", walEntry.walID, "created at", walEntry.walCreatedAt, "last vault rotation", walEntry.LastVaultRotation) + + if walEntry.walCreatedAt > existingWALEntry.walCreatedAt { + // If the existing WAL is older, delete it from storage and fall + // through to inserting our current WAL into the map. + b.Logger().Debug("deleting stale loaded WAL", "WAL ID", existingWALEntry.walID) + err = framework.DeleteWAL(ctx, s, existingWALEntry.walID) + if err != nil { + b.Logger().Warn("unable to delete loaded WAL", "error", err, "WAL ID", existingWALEntry.walID) + } + } else { + // If we already have a more recent WAL entry in the map, delete + // this one and continue onto the next WAL. + b.Logger().Debug("deleting stale candidate WAL", "WAL ID", walEntry.walID) + err = framework.DeleteWAL(ctx, s, walID) + if err != nil { + b.Logger().Warn("unable to delete candidate WAL", "error", err, "WAL ID", walEntry.walID) + } + continue + } + } + + b.Logger().Debug("loaded WAL", "WAL ID", walID) walMap[walEntry.RoleName] = walEntry } return walMap, nil diff --git a/builtin/logical/database/rotation_test.go b/builtin/logical/database/rotation_test.go index 28314c8fe2b0e..2031bdd943121 100644 --- a/builtin/logical/database/rotation_test.go +++ b/builtin/logical/database/rotation_test.go @@ -3,6 +3,8 @@ package database import ( "context" "database/sql" + "errors" + "fmt" "log" "os" "strings" @@ -13,11 +15,15 @@ import ( "github.com/hashicorp/vault/helper/namespace" "github.com/hashicorp/vault/helper/testhelpers/mongodb" postgreshelper "github.com/hashicorp/vault/helper/testhelpers/postgresql" + v5 "github.com/hashicorp/vault/sdk/database/dbplugin/v5" "github.com/hashicorp/vault/sdk/framework" + "github.com/hashicorp/vault/sdk/helper/consts" "github.com/hashicorp/vault/sdk/helper/dbtxn" "github.com/hashicorp/vault/sdk/logical" + "github.com/hashicorp/vault/sdk/queue" "github.com/lib/pq" mongodbatlasapi "github.com/mongodb/go-client-mongodb-atlas/mongodbatlas" + "github.com/stretchr/testify/mock" "go.mongodb.org/mongo-driver/mongo" "go.mongodb.org/mongo-driver/mongo/options" ) @@ -974,15 +980,25 @@ func TestBackend_StaticRole_LockRegression(t *testing.T) { } createTestPGUser(t, connURL, dbUser, dbUserDefaultPassword, testRoleStaticCreate) - for i := 0; i < 25; i++ { - data := map[string]interface{}{ - "name": "plugin-role-test", - "db_name": "plugin-test", - "rotation_statements": testRoleStaticUpdate, - "username": dbUser, - "rotation_period": "7s", - } + data = map[string]interface{}{ + "name": "plugin-role-test", + "db_name": "plugin-test", + "rotation_statements": testRoleStaticUpdate, + "username": dbUser, + "rotation_period": "7s", + } + req = &logical.Request{ + Operation: logical.CreateOperation, + Path: "static-roles/plugin-role-test", + Storage: config.StorageView, + Data: data, + } + resp, err = b.HandleRequest(namespace.RootContext(nil), req) + if err != nil || (resp != nil && resp.IsError()) { + t.Fatalf("err:%s resp:%#v\n", err, resp) + } + for i := 0; i < 25; i++ { req = &logical.Request{ Operation: logical.UpdateOperation, Path: "static-roles/plugin-role-test", @@ -1094,6 +1110,300 @@ func TestBackend_StaticRole_Rotate_Invalid_Role(t *testing.T) { } } +func TestRollsPasswordForwardsUsingWAL(t *testing.T) { + ctx := context.Background() + b, storage, mockDB := getBackend(t) + defer b.Cleanup(ctx) + configureDBMount(t, storage) + createRole(t, b, storage, mockDB, "hashicorp") + + role, err := b.StaticRole(ctx, storage, "hashicorp") + if err != nil { + t.Fatal(err) + } + oldPassword := role.StaticAccount.Password + + generateWALFromFailedRotation(t, b, storage, mockDB, "hashicorp") + + walIDs := requireWALs(t, storage, 1) + wal, err := b.findStaticWAL(ctx, storage, walIDs[0]) + if err != nil { + t.Fatal(err) + } + role, err = b.StaticRole(ctx, storage, "hashicorp") + if err != nil { + t.Fatal(err) + } + // Role's password should still be the WAL's old password + if role.StaticAccount.Password != oldPassword { + t.Fatal(role.StaticAccount.Password, oldPassword) + } + + rotateRole(t, b, storage, mockDB, "hashicorp") + + role, err = b.StaticRole(ctx, storage, "hashicorp") + if err != nil { + t.Fatal(err) + } + if role.StaticAccount.Password != wal.NewPassword { + t.Fatal("role password", role.StaticAccount.Password, "WAL new password", wal.NewPassword) + } + // WAL should be cleared by the successful rotate + requireWALs(t, storage, 0) +} + +func TestStoredWALsCorrectlyProcessed(t *testing.T) { + const walNewPassword = "new-password-from-wal" + for _, tc := range []struct { + name string + shouldRotate bool + wal *setCredentialsWAL + }{ + { + "WAL is kept and used for roll forward", + true, + &setCredentialsWAL{ + RoleName: "hashicorp", + Username: "hashicorp", + NewPassword: walNewPassword, + LastVaultRotation: time.Now().Add(time.Hour), + }, + }, + { + "zero-time WAL is discarded on load", + false, + &setCredentialsWAL{ + RoleName: "hashicorp", + Username: "hashicorp", + NewPassword: walNewPassword, + LastVaultRotation: time.Time{}, + }, + }, + { + "empty-password WAL is kept but a new password is generated", + true, + &setCredentialsWAL{ + RoleName: "hashicorp", + Username: "hashicorp", + NewPassword: "", + LastVaultRotation: time.Now().Add(time.Hour), + }, + }, + } { + t.Run(tc.name, func(t *testing.T) { + ctx := context.Background() + config := logical.TestBackendConfig() + storage := &logical.InmemStorage{} + config.StorageView = storage + b := Backend(config) + defer b.Cleanup(ctx) + mockDB := setupMockDB(b) + if err := b.Setup(ctx, config); err != nil { + t.Fatal(err) + } + b.credRotationQueue = queue.New() + configureDBMount(t, config.StorageView) + createRole(t, b, config.StorageView, mockDB, "hashicorp") + role, err := b.StaticRole(ctx, config.StorageView, "hashicorp") + if err != nil { + t.Fatal(err) + } + initialPassword := role.StaticAccount.Password + + // Set up a WAL for our test case + framework.PutWAL(ctx, config.StorageView, staticWALKey, tc.wal) + requireWALs(t, config.StorageView, 1) + // Reset the rotation queue to simulate startup memory state + b.credRotationQueue = queue.New() + + // Now finish the startup process by populating the queue, which should discard the WAL + b.initQueue(ctx, config, consts.ReplicationUnknown) + + if tc.shouldRotate { + requireWALs(t, storage, 1) + } else { + requireWALs(t, storage, 0) + } + + // Run one tick + mockDB.On("UpdateUser", mock.Anything, mock.Anything). + Return(v5.UpdateUserResponse{}, nil). + Once() + b.rotateCredentials(ctx, storage) + requireWALs(t, storage, 0) + + role, err = b.StaticRole(ctx, storage, "hashicorp") + if err != nil { + t.Fatal(err) + } + item, err := b.popFromRotationQueueByKey("hashicorp") + if err != nil { + t.Fatal(err) + } + + if tc.shouldRotate { + if tc.wal.NewPassword != "" { + // Should use WAL's new_password field + if role.StaticAccount.Password != walNewPassword { + t.Fatal() + } + } else { + // Should rotate but ignore WAL's new_password field + if role.StaticAccount.Password == initialPassword { + t.Fatal() + } + if role.StaticAccount.Password == walNewPassword { + t.Fatal() + } + } + } else { + // Ensure the role was not promoted for early rotation + if item.Priority < time.Now().Add(time.Hour).Unix() { + t.Fatal("priority should be for about a week away, but was", item.Priority) + } + if role.StaticAccount.Password != initialPassword { + t.Fatal("password should not have been rotated yet") + } + } + }) + } +} + +func TestDeletesOlderWALsOnLoad(t *testing.T) { + ctx := context.Background() + b, storage, mockDB := getBackend(t) + defer b.Cleanup(ctx) + configureDBMount(t, storage) + createRole(t, b, storage, mockDB, "hashicorp") + + // Create 4 WALs, with a clear winner for most recent. + wal := &setCredentialsWAL{ + RoleName: "hashicorp", + Username: "hashicorp", + NewPassword: "some-new-password", + LastVaultRotation: time.Now(), + } + for i := 0; i < 3; i++ { + _, err := framework.PutWAL(ctx, storage, staticWALKey, wal) + if err != nil { + t.Fatal(err) + } + } + time.Sleep(2 * time.Second) + // We expect this WAL to have the latest createdAt timestamp + walID, err := framework.PutWAL(ctx, storage, staticWALKey, wal) + if err != nil { + t.Fatal(err) + } + requireWALs(t, storage, 4) + + walMap, err := b.loadStaticWALs(ctx, storage) + if err != nil { + t.Fatal(err) + } + if len(walMap) != 1 || walMap["hashicorp"] == nil || walMap["hashicorp"].walID != walID { + t.Fatal() + } + requireWALs(t, storage, 1) +} + +func generateWALFromFailedRotation(t *testing.T, b *databaseBackend, storage logical.Storage, mockDB *mockNewDatabase, roleName string) { + t.Helper() + mockDB.On("UpdateUser", mock.Anything, mock.Anything). + Return(v5.UpdateUserResponse{}, errors.New("forced error")). + Once() + _, err := b.HandleRequest(context.Background(), &logical.Request{ + Operation: logical.UpdateOperation, + Path: "rotate-role/" + roleName, + Storage: storage, + }) + if err == nil { + t.Fatal("expected error") + } +} + +func rotateRole(t *testing.T, b *databaseBackend, storage logical.Storage, mockDB *mockNewDatabase, roleName string) { + t.Helper() + mockDB.On("UpdateUser", mock.Anything, mock.Anything). + Return(v5.UpdateUserResponse{}, nil). + Once() + resp, err := b.HandleRequest(context.Background(), &logical.Request{ + Operation: logical.UpdateOperation, + Path: "rotate-role/" + roleName, + Storage: storage, + }) + if err != nil || (resp != nil && resp.IsError()) { + t.Fatal(resp, err) + } +} + +// returns a slice of the WAL IDs in storage +func requireWALs(t *testing.T, storage logical.Storage, expectedCount int) []string { + t.Helper() + wals, err := storage.List(context.Background(), "wal/") + if err != nil { + t.Fatal(err) + } + if len(wals) != expectedCount { + t.Fatal("expected WALs", expectedCount, "got", len(wals)) + } + + return wals +} + +func getBackend(t *testing.T) (*databaseBackend, logical.Storage, *mockNewDatabase) { + t.Helper() + config := logical.TestBackendConfig() + config.StorageView = &logical.InmemStorage{} + // Create and init the backend ourselves instead of using a Factory because + // the factory function kicks off threads that cause racy tests. + b := Backend(config) + if err := b.Setup(context.Background(), config); err != nil { + t.Fatal(err) + } + b.credRotationQueue = queue.New() + b.populateQueue(context.Background(), config.StorageView) + + mockDB := setupMockDB(b) + + return b, config.StorageView, mockDB +} + +func setupMockDB(b *databaseBackend) *mockNewDatabase { + mockDB := &mockNewDatabase{} + mockDB.On("Initialize", mock.Anything, mock.Anything).Return(v5.InitializeResponse{}, nil) + mockDB.On("Close").Return(nil) + dbw := databaseVersionWrapper{ + v5: mockDB, + } + + dbi := &dbPluginInstance{ + database: dbw, + id: "foo-id", + name: "mockV5", + } + b.connections["mockv5"] = dbi + + return mockDB +} + +// configureDBMount puts config directly into storage to avoid the DB engine's +// plugin init code paths, allowing us to use a manually populated mock DB object. +func configureDBMount(t *testing.T, storage logical.Storage) { + t.Helper() + entry, err := logical.StorageEntryJSON(fmt.Sprintf("config/mockv5"), &DatabaseConfig{ + AllowedRoles: []string{"*"}, + }) + if err != nil { + t.Fatal(err) + } + + err = storage.Put(context.Background(), entry) + if err != nil { + t.Fatal(err) + } +} + // capturePasswords captures the current passwords at the time of calling, and // returns a map of username / passwords building off of the input map func capturePasswords(t *testing.T, b logical.Backend, config *logical.BackendConfig, testCases []string, pws map[string][]string) map[string][]string { diff --git a/builtin/logical/database/versioning_large_test.go b/builtin/logical/database/versioning_large_test.go index 7b3cc82eee930..db2941308507e 100644 --- a/builtin/logical/database/versioning_large_test.go +++ b/builtin/logical/database/versioning_large_test.go @@ -81,8 +81,12 @@ func TestPlugin_lifecycle(t *testing.T) { for name, test := range tests { t.Run(name, func(t *testing.T) { - cleanupReqs := []*logical.Request{} - defer cleanup(t, b, cleanupReqs) + var cleanupReqs []*logical.Request + defer func() { + // Do not defer cleanup directly so that we can populate the + // slice before the function gets executed. + cleanup(t, b, cleanupReqs) + }() // ///////////////////////////////////////////////////////////////// // Configure @@ -173,7 +177,7 @@ func TestPlugin_lifecycle(t *testing.T) { // Create static role staticRoleName := "static-role" req = &logical.Request{ - Operation: logical.UpdateOperation, + Operation: logical.CreateOperation, Path: fmt.Sprintf("static-roles/%s", staticRoleName), Storage: config.StorageView, Data: map[string]interface{}{ diff --git a/changelog/12563.txt b/changelog/12563.txt new file mode 100644 index 0000000000000..9298e82460be3 --- /dev/null +++ b/changelog/12563.txt @@ -0,0 +1,3 @@ +```release-note:bug +secrets/db: Fix bug where Vault can rotate static role passwords early during start up under certain conditions. +```