From b90bbd20848f2721ae96f5904132d352c3770618 Mon Sep 17 00:00:00 2001 From: HridoyRoy Date: Tue, 14 Sep 2021 22:53:48 -0700 Subject: [PATCH 01/28] port of ldap fix for early cred rotation --- builtin/logical/database/path_roles.go | 23 +-- .../database/path_rotate_credentials.go | 9 +- builtin/logical/database/rotation.go | 134 ++++++++++++------ 3 files changed, 114 insertions(+), 52 deletions(-) diff --git a/builtin/logical/database/path_roles.go b/builtin/logical/database/path_roles.go index fd272dd1e42d6..61b635f30f272 100644 --- a/builtin/logical/database/path_roles.go +++ b/builtin/logical/database/path_roles.go @@ -482,19 +482,22 @@ 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 { 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 +507,15 @@ 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_rotate_credentials.go b/builtin/logical/database/path_rotate_credentials.go index 5774ea8634600..6f4ae62b651ff 100644 --- a/builtin/logical/database/path_rotate_credentials.go +++ b/builtin/logical/database/path_rotate_credentials.go @@ -188,6 +188,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 +197,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 25652e8b545fd..12df9745837b5 100644 --- a/builtin/logical/database/rotation.go +++ b/builtin/logical/database/rotation.go @@ -7,6 +7,7 @@ import ( "strconv" "time" + "github.com/hashicorp/errwrap" "github.com/hashicorp/go-multierror" "github.com/hashicorp/go-secure-stdlib/strutil" v4 "github.com/hashicorp/vault/sdk/database/dbplugin" @@ -66,21 +67,33 @@ 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() } @@ -120,7 +133,9 @@ type setCredentialsWAL struct { 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 @@ -194,18 +209,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) @@ -226,6 +230,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() { @@ -255,11 +261,12 @@ 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), + OldPassword: data["old_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 { @@ -271,16 +278,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,16 +305,19 @@ type setStaticAccountOutput struct { // 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") - } // Re-use WAL ID if present, otherwise PUT a new WAL output := &setStaticAccountOutput{WALID: input.WALID} + if input == nil || input.Role == nil || input.RoleName == "" { + return output, errors.New("input was empty when attempting to set credentials for static account") + } dbConfig, err := b.DatabaseConfig(ctx, s, input.Role.DBName) 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. @@ -327,17 +334,31 @@ func (b *databaseBackend) setStaticAccount(ctx context.Context, s logical.Storag dbi.RLock() defer dbi.RUnlock() - // 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) + } + + 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 = "" } } - output.Password = newPassword config := v4.StaticUserConfig{ Username: input.Role.StaticAccount.Username, @@ -345,6 +366,10 @@ func (b *databaseBackend) setStaticAccount(ctx context.Context, s logical.Storag } 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, @@ -355,6 +380,7 @@ func (b *databaseBackend) setStaticAccount(ctx context.Context, s logical.Storag if err != nil { return output, fmt.Errorf("error writing WAL entry: %w", err) } + b.Logger().Debug("writing WAL", "role", input.RoleName, "WAL ID", output.WALID) } updateReq := v5.UpdateUserRequest{ @@ -390,8 +416,10 @@ 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) + b.Logger().Warn("error deleting WAL", "WAL ID", output.WALID, "error", err) return output, merr } + b.Logger().Debug("deleted WAL", "WAL ID", output.WALID) // The WAL has been deleted, return new setStaticAccountOutput without it return &setStaticAccountOutput{RotationTime: lvr}, merr @@ -493,13 +521,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 WAL", "WAL ID", existingWALEntry.walID) + err = framework.DeleteWAL(ctx, s, existingWALEntry.walID) + if err != nil { + b.Logger().Warn("unable to delete 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 WAL", "WAL ID", walEntry.walID) + err = framework.DeleteWAL(ctx, s, walID) + if err != nil { + b.Logger().Warn("unable to delete WAL", "error", err, "WAL ID", walEntry.walID) + } + continue + } + } + + b.Logger().Debug("loaded WAL", "WAL ID", walID) walMap[walEntry.RoleName] = walEntry } return walMap, nil From b4fa49a29bfef48619f2776d8df75f850ff94c2b Mon Sep 17 00:00:00 2001 From: HridoyRoy Date: Wed, 15 Sep 2021 09:33:55 -0700 Subject: [PATCH 02/28] some more porting --- builtin/logical/database/path_roles.go | 32 ++++++++++++++++++++++++-- 1 file changed, 30 insertions(+), 2 deletions(-) diff --git a/builtin/logical/database/path_roles.go b/builtin/logical/database/path_roles.go index 61b635f30f272..e17810ad6d0cc 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" "github.com/hashicorp/go-secure-stdlib/strutil" v4 "github.com/hashicorp/vault/sdk/database/dbplugin" "github.com/hashicorp/vault/sdk/framework" @@ -215,7 +216,26 @@ 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 { + err = framework.DeleteWAL(ctx, req.Storage, walID) + if err != nil { + 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) { @@ -491,7 +511,15 @@ func (b *databaseBackend) pathStaticRoleCreateUpdate(ctx context.Context, req *l Role: role, }) if err != nil { - return nil, err + if resp != nil && resp.WALID != "" { + walDeleteErr := framework.DeleteWAL(ctx, req.Storage, resp.WALID) + if walDeleteErr != nil { + 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() + } + } } // guard against RotationTime not being set or zero-value lvr = resp.RotationTime From f60322741e9795964be0dea6c91a519bc9534341 Mon Sep 17 00:00:00 2001 From: HridoyRoy Date: Wed, 15 Sep 2021 09:47:01 -0700 Subject: [PATCH 03/28] another couple lines to port --- builtin/logical/database/rotation.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/builtin/logical/database/rotation.go b/builtin/logical/database/rotation.go index 12df9745837b5..f65c7df2b7cec 100644 --- a/builtin/logical/database/rotation.go +++ b/builtin/logical/database/rotation.go @@ -310,6 +310,8 @@ func (b *databaseBackend) setStaticAccount(ctx context.Context, s logical.Storag if input == nil || input.Role == nil || input.RoleName == "" { return output, errors.New("input was empty when attempting to set credentials for static account") } + b.Lock() + defer b.Unlock() dbConfig, err := b.DatabaseConfig(ctx, s, input.Role.DBName) if err != nil { From ade88b4357c997a95692038b7c6d7bbb94188a3b Mon Sep 17 00:00:00 2001 From: HridoyRoy Date: Wed, 15 Sep 2021 10:01:23 -0700 Subject: [PATCH 04/28] final commits before report --- builtin/logical/database/path_roles.go | 5 +++-- builtin/logical/database/rotation.go | 18 ++++++++++++++---- 2 files changed, 17 insertions(+), 6 deletions(-) diff --git a/builtin/logical/database/path_roles.go b/builtin/logical/database/path_roles.go index e17810ad6d0cc..52e51f29105e3 100644 --- a/builtin/logical/database/path_roles.go +++ b/builtin/logical/database/path_roles.go @@ -507,8 +507,9 @@ func (b *databaseBackend) pathStaticRoleCreateUpdate(ctx context.Context, req *l 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, + RoleName: name, + Role: role, + CreateUser: createRole, }) if err != nil { if resp != nil && resp.WALID != "" { diff --git a/builtin/logical/database/rotation.go b/builtin/logical/database/rotation.go index f65c7df2b7cec..a1d8c42400b27 100644 --- a/builtin/logical/database/rotation.go +++ b/builtin/logical/database/rotation.go @@ -278,9 +278,11 @@ func (b *databaseBackend) findStaticWAL(ctx context.Context, s logical.Storage, } type setStaticAccountInput struct { - RoleName string - Role *roleEntry - WALID string + RoleName string + Role *roleEntry + CreateUser bool + WALID string + Password string } type setStaticAccountOutput struct { @@ -336,7 +338,15 @@ func (b *databaseBackend) setStaticAccount(ctx context.Context, s logical.Storag dbi.RLock() defer dbi.RUnlock() - var newPassword string + // ROY + // var newPassword string + // 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) + } if output.WALID != "" { wal, err := b.findStaticWAL(ctx, s, output.WALID) if err != nil { From cd4b897e5dcd7b26a86a0968cebf0f3337445c86 Mon Sep 17 00:00:00 2001 From: HridoyRoy Date: Wed, 15 Sep 2021 12:35:16 -0700 Subject: [PATCH 05/28] remove deadlock --- builtin/logical/database/rotation.go | 2 -- 1 file changed, 2 deletions(-) diff --git a/builtin/logical/database/rotation.go b/builtin/logical/database/rotation.go index a1d8c42400b27..dd89d202eeb4e 100644 --- a/builtin/logical/database/rotation.go +++ b/builtin/logical/database/rotation.go @@ -312,8 +312,6 @@ func (b *databaseBackend) setStaticAccount(ctx context.Context, s logical.Storag if input == nil || input.Role == nil || input.RoleName == "" { return output, errors.New("input was empty when attempting to set credentials for static account") } - b.Lock() - defer b.Unlock() dbConfig, err := b.DatabaseConfig(ctx, s, input.Role.DBName) if err != nil { From 61882c17f3ee796ecca676ec4e76b7e15232bb2a Mon Sep 17 00:00:00 2001 From: HridoyRoy Date: Wed, 15 Sep 2021 13:00:38 -0700 Subject: [PATCH 06/28] needs testing --- builtin/logical/database/path_roles.go | 11 +++++++++-- builtin/logical/database/rotation.go | 2 -- 2 files changed, 9 insertions(+), 4 deletions(-) diff --git a/builtin/logical/database/path_roles.go b/builtin/logical/database/path_roles.go index 52e51f29105e3..7a1bbfa3db2c0 100644 --- a/builtin/logical/database/path_roles.go +++ b/builtin/logical/database/path_roles.go @@ -537,8 +537,15 @@ func (b *databaseBackend) pathStaticRoleCreateUpdate(ctx context.Context, req *l return nil, err } item, err = b.popFromRotationQueueByKey(name) - if err != nil { - return nil, err + if err != nil || item == nil { + // in this case we want to push a new role into the rotation queue + if err := b.pushItem(&queue.Item{ + Key: name, + Priority: lvr.Add(role.StaticAccount.RotationPeriod).Unix(), + }); err != nil { + return nil, err + } + return nil, nil } } item.Priority = lvr.Add(role.StaticAccount.RotationPeriod).Unix() diff --git a/builtin/logical/database/rotation.go b/builtin/logical/database/rotation.go index dd89d202eeb4e..d112ce877f8a7 100644 --- a/builtin/logical/database/rotation.go +++ b/builtin/logical/database/rotation.go @@ -336,8 +336,6 @@ func (b *databaseBackend) setStaticAccount(ctx context.Context, s logical.Storag dbi.RLock() defer dbi.RUnlock() - // ROY - // var newPassword string // 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 From 73fc6841b8c9f81075609aa1f1bd54ff3050d1f6 Mon Sep 17 00:00:00 2001 From: HridoyRoy Date: Thu, 16 Sep 2021 07:14:08 -0700 Subject: [PATCH 07/28] updates --- builtin/logical/database/path_roles.go | 1 + builtin/logical/database/rotation.go | 9 +++------ 2 files changed, 4 insertions(+), 6 deletions(-) diff --git a/builtin/logical/database/path_roles.go b/builtin/logical/database/path_roles.go index 7a1bbfa3db2c0..94a361ed55cdb 100644 --- a/builtin/logical/database/path_roles.go +++ b/builtin/logical/database/path_roles.go @@ -547,6 +547,7 @@ func (b *databaseBackend) pathStaticRoleCreateUpdate(ctx context.Context, req *l } return nil, nil } + } item.Priority = lvr.Add(role.StaticAccount.RotationPeriod).Unix() diff --git a/builtin/logical/database/rotation.go b/builtin/logical/database/rotation.go index d112ce877f8a7..3d0478cc2c150 100644 --- a/builtin/logical/database/rotation.go +++ b/builtin/logical/database/rotation.go @@ -333,16 +333,13 @@ func (b *databaseBackend) setStaticAccount(ctx context.Context, s logical.Storag return output, err } - dbi.RLock() - defer dbi.RUnlock() + dbi.Lock() + defer dbi.Unlock() // 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 { From 01eae5d5b25a8c4734225da9c69f4465e87ecca3 Mon Sep 17 00:00:00 2001 From: Tom Proctor Date: Thu, 16 Sep 2021 17:29:42 +0100 Subject: [PATCH 08/28] Sync with OpenLDAP PR --- builtin/logical/database/path_roles.go | 9 +++-- .../database/path_rotate_credentials.go | 8 +++-- builtin/logical/database/rotation.go | 35 ++++++++----------- 3 files changed, 26 insertions(+), 26 deletions(-) diff --git a/builtin/logical/database/path_roles.go b/builtin/logical/database/path_roles.go index 94a361ed55cdb..e41f968bca23d 100644 --- a/builtin/logical/database/path_roles.go +++ b/builtin/logical/database/path_roles.go @@ -228,8 +228,10 @@ func (b *databaseBackend) pathStaticRoleDelete(ctx context.Context, req *logical 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) } } @@ -507,14 +509,15 @@ func (b *databaseBackend) pathStaticRoleCreateUpdate(ctx context.Context, req *l 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)) diff --git a/builtin/logical/database/path_rotate_credentials.go b/builtin/logical/database/path_rotate_credentials.go index 6f4ae62b651ff..040784a731fa1 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. diff --git a/builtin/logical/database/rotation.go b/builtin/logical/database/rotation.go index 3d0478cc2c150..f3d990de652e5 100644 --- a/builtin/logical/database/rotation.go +++ b/builtin/logical/database/rotation.go @@ -8,7 +8,6 @@ import ( "time" "github.com/hashicorp/errwrap" - "github.com/hashicorp/go-multierror" "github.com/hashicorp/go-secure-stdlib/strutil" v4 "github.com/hashicorp/vault/sdk/database/dbplugin" v5 "github.com/hashicorp/vault/sdk/database/dbplugin/v5" @@ -67,7 +66,8 @@ func (b *databaseBackend) populateQueue(ctx context.Context, s logical.Storage) item := queue.Item{ Key: roleName, - Priority: role.StaticAccount.NextRotationTime().Unix()} + Priority: role.StaticAccount.NextRotationTime().Unix(), + } // Check if role name is in map walEntry := walMap[roleName] @@ -127,7 +127,6 @@ 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"` @@ -264,7 +263,6 @@ func (b *databaseBackend) findStaticWAL(ctx context.Context, s logical.Storage, walID: id, walCreatedAt: wal.CreatedAt, NewPassword: data["new_password"].(string), - OldPassword: data["old_password"].(string), RoleName: data["role_name"].(string), Username: data["username"].(string), } @@ -278,11 +276,9 @@ func (b *databaseBackend) findStaticWAL(ctx context.Context, s logical.Storage, } type setStaticAccountInput struct { - RoleName string - Role *roleEntry - CreateUser bool - WALID string - Password string + RoleName string + Role *roleEntry + WALID string } type setStaticAccountOutput struct { @@ -306,12 +302,11 @@ 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 - // Re-use WAL ID if present, otherwise PUT a new WAL - output := &setStaticAccountOutput{WALID: input.WALID} if input == nil || input.Role == nil || input.RoleName == "" { - return output, errors.New("input was empty when attempting to set credentials for static account") + return nil, errors.New("input was empty when attempting to set credentials for static account") } + // Re-use WAL ID if present, otherwise PUT a new WAL + output := &setStaticAccountOutput{WALID: input.WALID} dbConfig, err := b.DatabaseConfig(ctx, s, input.Role.DBName) if err != nil { @@ -379,7 +374,6 @@ func (b *databaseBackend) setStaticAccount(ctx context.Context, s logical.Storag RoleName: input.RoleName, Username: config.Username, NewPassword: config.Password, - OldPassword: input.Role.StaticAccount.Password, LastVaultRotation: input.Role.StaticAccount.LastVaultRotation, }) if err != nil { @@ -420,14 +414,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) b.Logger().Warn("error deleting WAL", "WAL ID", output.WALID, "error", err) - return output, merr + 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 @@ -541,18 +534,18 @@ func (b *databaseBackend) loadStaticWALs(ctx context.Context, s logical.Storage) 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 WAL", "WAL ID", existingWALEntry.walID) + 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 WAL", "error", err, "WAL ID", existingWALEntry.walID) + 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 WAL", "WAL ID", walEntry.walID) + 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 WAL", "error", err, "WAL ID", walEntry.walID) + b.Logger().Warn("unable to delete candidate WAL", "error", err, "WAL ID", walEntry.walID) } continue } From 67564170dcd0c11d9db2357777943d14704fb70e Mon Sep 17 00:00:00 2001 From: Tom Proctor Date: Fri, 17 Sep 2021 18:34:00 +0100 Subject: [PATCH 09/28] Update the update error handling for items not found in the queue --- builtin/logical/database/path_roles.go | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/builtin/logical/database/path_roles.go b/builtin/logical/database/path_roles.go index e41f968bca23d..4ce5a39a32c02 100644 --- a/builtin/logical/database/path_roles.go +++ b/builtin/logical/database/path_roles.go @@ -541,14 +541,11 @@ func (b *databaseBackend) pathStaticRoleCreateUpdate(ctx context.Context, req *l } item, err = b.popFromRotationQueueByKey(name) if err != nil || item == nil { - // in this case we want to push a new role into the rotation queue - if err := b.pushItem(&queue.Item{ + b.Logger().Warn("expected role to exist in rotation queue but none found", "role", name, "error", err) + item = &queue.Item{ Key: name, Priority: lvr.Add(role.StaticAccount.RotationPeriod).Unix(), - }); err != nil { - return nil, err } - return nil, nil } } From 584c65cde088af949fa5d98cbefd7c1a2edfcb79 Mon Sep 17 00:00:00 2001 From: Tom Proctor Date: Fri, 17 Sep 2021 18:49:07 +0100 Subject: [PATCH 10/28] 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 --- builtin/logical/database/path_roles_test.go | 128 +++++++++ builtin/logical/database/rotation_test.go | 284 ++++++++++++++++++++ 2 files changed, 412 insertions(+) diff --git a/builtin/logical/database/path_roles_test.go b/builtin/logical/database/path_roles_test.go index 9a2081c9863d1..94969951a2d66 100644 --- a/builtin/logical/database/path_roles_test.go +++ b/builtin/logical/database/path_roles_test.go @@ -516,6 +516,134 @@ func TestBackend_StaticRole_Role_name_check(t *testing.T) { } } +func TestWALsStillTrackedAfterUpdate(t *testing.T) { + ctx := context.Background() + b, storage := getBackend(t, false) + defer b.Cleanup(ctx) + configureDBMount(t, b, storage) + + createRole(t, b, storage, "hashicorp") + + generateWALFromFailedRotation(t, b, storage, "hashicorp") + requireWALs(t, storage, 1) + + _, 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 { + t.Fatal(err) + } + requireWALs(t, storage, 1) + + // Check we've still got track of it in the queue as well + item, err := b.credRotationQueue.PopByKey("hashicorp") + if err != nil { + t.Fatal(err) + } + if wal, ok := item.Value.(string); !ok || wal == "" { + t.Fatal("should have a WAL ID in the rotation queue") + } +} + +func TestWALsDeletedOnRoleCreationFailed(t *testing.T) { + ctx := context.Background() + b, storage := getBackend(t, true) + defer b.Cleanup(ctx) + configureDBMount(t, b, storage) + + for i := 0; i < 3; i++ { + _, err := b.HandleRequest(ctx, &logical.Request{ + Operation: logical.CreateOperation, + Path: "static-roles/hashicorp", + Storage: storage, + Data: map[string]interface{}{ + "username": "hashicorp", + "dn": "uid=hashicorp,ou=users,dc=hashicorp,dc=com", + "rotation_period": "5s", + }, + }) + if err == nil { + t.Fatal("expected error from OpenLDAP") + } + } + + requireWALs(t, storage, 0) +} + +func TestWALsDeletedOnRoleDeletion(t *testing.T) { + ctx := context.Background() + b, storage := getBackend(t, false) + defer b.Cleanup(ctx) + configureDBMount(t, b, storage) + + // Create the roles + roleNames := []string{"hashicorp", "2"} + for _, roleName := range roleNames { + createRole(t, b, storage, roleName) + } + + // Fail to rotate the roles + for _, roleName := range roleNames { + generateWALFromFailedRotation(t, b, storage, roleName) + } + + // Should have 2 WALs hanging around + requireWALs(t, storage, 2) + + // Delete one of the static roles + _, err := b.HandleRequest(ctx, &logical.Request{ + Operation: logical.DeleteOperation, + Path: "static-role/hashicorp", + Storage: storage, + }) + if err != nil { + t.Fatal(err) + } + + // 1 WAL should be cleared by the delete + requireWALs(t, storage, 1) +} + +func configureDBMount(t *testing.T, b *databaseBackend, storage logical.Storage) { + t.Helper() + resp, err := b.HandleRequest(context.Background(), &logical.Request{ + Operation: logical.CreateOperation, + Path: "config", + Storage: storage, + Data: map[string]interface{}{ + "binddn": "tester", + "bindpass": "pa$$w0rd", + "url": "ldap://138.91.247.105", + }, + }) + if err != nil || (resp != nil && resp.IsError()) { + t.Fatalf("err:%s resp:%#v\n", err, resp) + } +} + +func createRole(t *testing.T, b *databaseBackend, storage logical.Storage, roleName string) { + _, err := b.HandleRequest(context.Background(), &logical.Request{ + Operation: logical.CreateOperation, + Path: "static-role/" + roleName, + Storage: storage, + Data: map[string]interface{}{ + "username": roleName, + "dn": "uid=hashicorp,ou=users,dc=hashicorp,dc=com", + "rotation_period": "86400s", + }, + }) + if err != nil { + t.Fatal(err) + } +} + const testRoleStaticCreate = ` CREATE ROLE "{{name}}" WITH LOGIN diff --git a/builtin/logical/database/rotation_test.go b/builtin/logical/database/rotation_test.go index 28314c8fe2b0e..20bbc884a7c0f 100644 --- a/builtin/logical/database/rotation_test.go +++ b/builtin/logical/database/rotation_test.go @@ -10,12 +10,16 @@ import ( "time" "github.com/Sectorbob/mlab-ns2/gae/ns/digest" + "github.com/hashicorp/go-hclog" "github.com/hashicorp/vault/helper/namespace" "github.com/hashicorp/vault/helper/testhelpers/mongodb" postgreshelper "github.com/hashicorp/vault/helper/testhelpers/postgresql" "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/helper/logging" "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" "go.mongodb.org/mongo-driver/mongo" @@ -1094,6 +1098,286 @@ func TestBackend_StaticRole_Rotate_Invalid_Role(t *testing.T) { } } +func TestRollsPasswordForwards(t *testing.T) { + ctx := context.Background() + b, storage := getBackend(t, false) + defer b.Cleanup(ctx) + configureDBMount(t, b, storage) + createRole(t, b, storage, "hashicorp") + + role, err := b.StaticRole(ctx, storage, "hashicorp") + if err != nil { + t.Fatal(err) + } + oldPassword := role.StaticAccount.Password + + generateWALFromFailedRotation(t, b, storage, "hashicorp") + walIDs, err := storage.List(context.Background(), "wal/") + if err != nil { + t.Fatal(err) + } + if len(walIDs) != 1 { + t.Fatal("expected 1 WAL, got", len(walIDs)) + } + 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) + } + + // Trigger a retry on the rotation, it should use WAL's new password + _, err = b.HandleRequest(ctx, &logical.Request{ + Operation: logical.UpdateOperation, + Path: "rotate-role/hashicorp", + Storage: storage, + }) + if err != nil { + t.Fatal(err) + } + + role, err = b.StaticRole(ctx, storage, "hashicorp") + if err != nil { + t.Fatal(err) + } + if role.StaticAccount.Password != wal.NewPassword { + t.Fatal(role.StaticAccount.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.BackendConfig{ + Logger: logging.NewVaultLogger(hclog.Debug), + + System: &logical.StaticSystemView{ + DefaultLeaseTTLVal: time.Hour * 12, + MaxLeaseTTLVal: time.Hour * 24, + }, + StorageView: &logical.InmemStorage{}, + } + + b := Backend(&fakeLdapClient{throwErrs: false}) + b.Setup(context.Background(), config) + + b.credRotationQueue = queue.New() + initCtx := context.Background() + ictx, cancel := context.WithCancel(initCtx) + b.cancelQueue = cancel + + defer b.Cleanup(ctx) + configureDBMount(t, b, config.StorageView) + createRole(t, b, config.StorageView, "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) + // Remove the role from the rotation queue to simulate startup memory state + _, err = b.popFromRotationQueueByKey("hashicorp") + if err != nil { + t.Fatal(err) + } + + // Now finish the startup process by populating the queue, which should discard the WAL + b.initQueue(ictx, config, consts.ReplicationDRPrimary) + + if tc.shouldRotate { + requireWALs(t, config.StorageView, 1) + } else { + requireWALs(t, config.StorageView, 0) + } + + // Run one tick + b.rotateCredentials(ctx, config.StorageView) + requireWALs(t, config.StorageView, 0) + + role, err = b.StaticRole(ctx, config.StorageView, "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 := getBackend(t, false) + defer b.Cleanup(ctx) + configureDBMount(t, b, storage) + createRole(t, b, storage, "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, roleName string) { + t.Helper() + // Fail to rotate the roles + ldapClient := b.client.(*fakeLdapClient) + originalValue := ldapClient.throwErrs + ldapClient.throwErrs = true + defer func() { + ldapClient.throwErrs = originalValue + }() + + _, err := b.HandleRequest(context.Background(), &logical.Request{ + Operation: logical.UpdateOperation, + Path: "rotate-role/" + roleName, + Storage: storage, + }) + if err == nil { + t.Fatal("expected error") + } +} + +func requireWALs(t *testing.T, storage logical.Storage, expectedCount int) { + 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)) + } +} + +func getBackend(t *testing.T, throwsErr bool) (*databaseBackend, logical.Storage) { + t.Helper() + config := &logical.BackendConfig{ + Logger: logging.NewVaultLogger(hclog.Debug), + + System: &logical.StaticSystemView{ + DefaultLeaseTTLVal: time.Hour * 12, + MaxLeaseTTLVal: time.Hour * 24, + }, + StorageView: &logical.InmemStorage{}, + } + + b := Backend(config) + if err := b.Setup(context.Background(), config); err != nil { + t.Fatal(err) + } + + b.credRotationQueue = queue.New() + // Create a context with a cancel method for processing any WAL entries and + // populating the queue + initCtx := context.Background() + ictx, cancel := context.WithCancel(initCtx) + b.cancelQueue = cancel + + // Load queue and kickoff new periodic ticker + b.initQueue(ictx, config, consts.ReplicationPerformancePrimary) + + return b, config.StorageView +} + // 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 { From a5de7c36020bd80f8683bd4ff5f1b4eb66daf397 Mon Sep 17 00:00:00 2001 From: HridoyRoy Date: Fri, 17 Sep 2021 12:46:37 -0700 Subject: [PATCH 11/28] throw error on role creation failure --- builtin/logical/database/path_roles.go | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/builtin/logical/database/path_roles.go b/builtin/logical/database/path_roles.go index 94a361ed55cdb..7cb2a208eed35 100644 --- a/builtin/logical/database/path_roles.go +++ b/builtin/logical/database/path_roles.go @@ -520,6 +520,10 @@ func (b *databaseBackend) pathStaticRoleCreateUpdate(ctx context.Context, req *l merr = multierror.Append(merr, fmt.Errorf("failed to clean up WAL from failed role creation: %w", walDeleteErr)) err = merr.ErrorOrNil() } + } else { + // There is no walID, but role creation failed. In this case we must simply + // return the error. + return nil, err } } // guard against RotationTime not being set or zero-value From b1c8a06e10c2b35379d16241c0f8f2c542d117cc Mon Sep 17 00:00:00 2001 From: HridoyRoy Date: Fri, 17 Sep 2021 12:50:27 -0700 Subject: [PATCH 12/28] do not swallow error on role creation --- builtin/logical/database/path_roles.go | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/builtin/logical/database/path_roles.go b/builtin/logical/database/path_roles.go index 7cb2a208eed35..1f7417c89c1ff 100644 --- a/builtin/logical/database/path_roles.go +++ b/builtin/logical/database/path_roles.go @@ -520,11 +520,8 @@ func (b *databaseBackend) pathStaticRoleCreateUpdate(ctx context.Context, req *l merr = multierror.Append(merr, fmt.Errorf("failed to clean up WAL from failed role creation: %w", walDeleteErr)) err = merr.ErrorOrNil() } - } else { - // There is no walID, but role creation failed. In this case we must simply - // return the error. - return nil, err } + return nil, err } // guard against RotationTime not being set or zero-value lvr = resp.RotationTime From a85bda79f152e310a70ee6111e366a4107aedf47 Mon Sep 17 00:00:00 2001 From: HridoyRoy Date: Fri, 17 Sep 2021 13:19:14 -0700 Subject: [PATCH 13/28] comment out wip tests and add in a test for disallowed role --- builtin/logical/database/path_roles_test.go | 207 +++++---- builtin/logical/database/rotation_test.go | 476 ++++++++++---------- 2 files changed, 348 insertions(+), 335 deletions(-) diff --git a/builtin/logical/database/path_roles_test.go b/builtin/logical/database/path_roles_test.go index 94969951a2d66..9686ecd2314d1 100644 --- a/builtin/logical/database/path_roles_test.go +++ b/builtin/logical/database/path_roles_test.go @@ -43,7 +43,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 +61,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 +70,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 +80,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 +105,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, } @@ -516,100 +529,100 @@ func TestBackend_StaticRole_Role_name_check(t *testing.T) { } } -func TestWALsStillTrackedAfterUpdate(t *testing.T) { - ctx := context.Background() - b, storage := getBackend(t, false) - defer b.Cleanup(ctx) - configureDBMount(t, b, storage) - - createRole(t, b, storage, "hashicorp") - - generateWALFromFailedRotation(t, b, storage, "hashicorp") - requireWALs(t, storage, 1) - - _, 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 { - t.Fatal(err) - } - requireWALs(t, storage, 1) - - // Check we've still got track of it in the queue as well - item, err := b.credRotationQueue.PopByKey("hashicorp") - if err != nil { - t.Fatal(err) - } - if wal, ok := item.Value.(string); !ok || wal == "" { - t.Fatal("should have a WAL ID in the rotation queue") - } -} - -func TestWALsDeletedOnRoleCreationFailed(t *testing.T) { - ctx := context.Background() - b, storage := getBackend(t, true) - defer b.Cleanup(ctx) - configureDBMount(t, b, storage) - - for i := 0; i < 3; i++ { - _, err := b.HandleRequest(ctx, &logical.Request{ - Operation: logical.CreateOperation, - Path: "static-roles/hashicorp", - Storage: storage, - Data: map[string]interface{}{ - "username": "hashicorp", - "dn": "uid=hashicorp,ou=users,dc=hashicorp,dc=com", - "rotation_period": "5s", - }, - }) - if err == nil { - t.Fatal("expected error from OpenLDAP") - } - } - - requireWALs(t, storage, 0) -} - -func TestWALsDeletedOnRoleDeletion(t *testing.T) { - ctx := context.Background() - b, storage := getBackend(t, false) - defer b.Cleanup(ctx) - configureDBMount(t, b, storage) - - // Create the roles - roleNames := []string{"hashicorp", "2"} - for _, roleName := range roleNames { - createRole(t, b, storage, roleName) - } - - // Fail to rotate the roles - for _, roleName := range roleNames { - generateWALFromFailedRotation(t, b, storage, roleName) - } - - // Should have 2 WALs hanging around - requireWALs(t, storage, 2) - - // Delete one of the static roles - _, err := b.HandleRequest(ctx, &logical.Request{ - Operation: logical.DeleteOperation, - Path: "static-role/hashicorp", - Storage: storage, - }) - if err != nil { - t.Fatal(err) - } - - // 1 WAL should be cleared by the delete - requireWALs(t, storage, 1) -} +// func TestWALsStillTrackedAfterUpdate(t *testing.T) { +// ctx := context.Background() +// b, storage := getBackend(t, false) +// defer b.Cleanup(ctx) +// configureDBMount(t, b, storage) + +// createRole(t, b, storage, "hashicorp") + +// generateWALFromFailedRotation(t, b, storage, "hashicorp") +// requireWALs(t, storage, 1) + +// _, 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 { +// t.Fatal(err) +// } +// requireWALs(t, storage, 1) + +// // Check we've still got track of it in the queue as well +// item, err := b.credRotationQueue.PopByKey("hashicorp") +// if err != nil { +// t.Fatal(err) +// } +// if wal, ok := item.Value.(string); !ok || wal == "" { +// t.Fatal("should have a WAL ID in the rotation queue") +// } +// } + +// func TestWALsDeletedOnRoleCreationFailed(t *testing.T) { +// ctx := context.Background() +// b, storage := getBackend(t, true) +// defer b.Cleanup(ctx) +// configureDBMount(t, b, storage) + +// for i := 0; i < 3; i++ { +// _, err := b.HandleRequest(ctx, &logical.Request{ +// Operation: logical.CreateOperation, +// Path: "static-roles/hashicorp", +// Storage: storage, +// Data: map[string]interface{}{ +// "username": "hashicorp", +// "dn": "uid=hashicorp,ou=users,dc=hashicorp,dc=com", +// "rotation_period": "5s", +// }, +// }) +// if err == nil { +// t.Fatal("expected error from OpenLDAP") +// } +// } + +// requireWALs(t, storage, 0) +// } + +// func TestWALsDeletedOnRoleDeletion(t *testing.T) { +// ctx := context.Background() +// b, storage := getBackend(t, false) +// defer b.Cleanup(ctx) +// configureDBMount(t, b, storage) + +// // Create the roles +// roleNames := []string{"hashicorp", "2"} +// for _, roleName := range roleNames { +// createRole(t, b, storage, roleName) +// } + +// // Fail to rotate the roles +// for _, roleName := range roleNames { +// generateWALFromFailedRotation(t, b, storage, roleName) +// } + +// // Should have 2 WALs hanging around +// requireWALs(t, storage, 2) + +// // Delete one of the static roles +// _, err := b.HandleRequest(ctx, &logical.Request{ +// Operation: logical.DeleteOperation, +// Path: "static-role/hashicorp", +// Storage: storage, +// }) +// if err != nil { +// t.Fatal(err) +// } + +// // 1 WAL should be cleared by the delete +// requireWALs(t, storage, 1) +// } func configureDBMount(t *testing.T, b *databaseBackend, storage logical.Storage) { t.Helper() diff --git a/builtin/logical/database/rotation_test.go b/builtin/logical/database/rotation_test.go index 20bbc884a7c0f..2ee15f587beb9 100644 --- a/builtin/logical/database/rotation_test.go +++ b/builtin/logical/database/rotation_test.go @@ -1098,244 +1098,244 @@ func TestBackend_StaticRole_Rotate_Invalid_Role(t *testing.T) { } } -func TestRollsPasswordForwards(t *testing.T) { - ctx := context.Background() - b, storage := getBackend(t, false) - defer b.Cleanup(ctx) - configureDBMount(t, b, storage) - createRole(t, b, storage, "hashicorp") - - role, err := b.StaticRole(ctx, storage, "hashicorp") - if err != nil { - t.Fatal(err) - } - oldPassword := role.StaticAccount.Password - - generateWALFromFailedRotation(t, b, storage, "hashicorp") - walIDs, err := storage.List(context.Background(), "wal/") - if err != nil { - t.Fatal(err) - } - if len(walIDs) != 1 { - t.Fatal("expected 1 WAL, got", len(walIDs)) - } - 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) - } - - // Trigger a retry on the rotation, it should use WAL's new password - _, err = b.HandleRequest(ctx, &logical.Request{ - Operation: logical.UpdateOperation, - Path: "rotate-role/hashicorp", - Storage: storage, - }) - if err != nil { - t.Fatal(err) - } - - role, err = b.StaticRole(ctx, storage, "hashicorp") - if err != nil { - t.Fatal(err) - } - if role.StaticAccount.Password != wal.NewPassword { - t.Fatal(role.StaticAccount.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.BackendConfig{ - Logger: logging.NewVaultLogger(hclog.Debug), - - System: &logical.StaticSystemView{ - DefaultLeaseTTLVal: time.Hour * 12, - MaxLeaseTTLVal: time.Hour * 24, - }, - StorageView: &logical.InmemStorage{}, - } - - b := Backend(&fakeLdapClient{throwErrs: false}) - b.Setup(context.Background(), config) - - b.credRotationQueue = queue.New() - initCtx := context.Background() - ictx, cancel := context.WithCancel(initCtx) - b.cancelQueue = cancel - - defer b.Cleanup(ctx) - configureDBMount(t, b, config.StorageView) - createRole(t, b, config.StorageView, "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) - // Remove the role from the rotation queue to simulate startup memory state - _, err = b.popFromRotationQueueByKey("hashicorp") - if err != nil { - t.Fatal(err) - } - - // Now finish the startup process by populating the queue, which should discard the WAL - b.initQueue(ictx, config, consts.ReplicationDRPrimary) - - if tc.shouldRotate { - requireWALs(t, config.StorageView, 1) - } else { - requireWALs(t, config.StorageView, 0) - } - - // Run one tick - b.rotateCredentials(ctx, config.StorageView) - requireWALs(t, config.StorageView, 0) - - role, err = b.StaticRole(ctx, config.StorageView, "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 := getBackend(t, false) - defer b.Cleanup(ctx) - configureDBMount(t, b, storage) - createRole(t, b, storage, "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, roleName string) { - t.Helper() - // Fail to rotate the roles - ldapClient := b.client.(*fakeLdapClient) - originalValue := ldapClient.throwErrs - ldapClient.throwErrs = true - defer func() { - ldapClient.throwErrs = originalValue - }() - - _, err := b.HandleRequest(context.Background(), &logical.Request{ - Operation: logical.UpdateOperation, - Path: "rotate-role/" + roleName, - Storage: storage, - }) - if err == nil { - t.Fatal("expected error") - } -} +// func TestRollsPasswordForwards(t *testing.T) { +// ctx := context.Background() +// b, storage := getBackend(t, false) +// defer b.Cleanup(ctx) +// configureDBMount(t, b, storage) +// createRole(t, b, storage, "hashicorp") + +// role, err := b.StaticRole(ctx, storage, "hashicorp") +// if err != nil { +// t.Fatal(err) +// } +// oldPassword := role.StaticAccount.Password + +// generateWALFromFailedRotation(t, b, storage, "hashicorp") +// walIDs, err := storage.List(context.Background(), "wal/") +// if err != nil { +// t.Fatal(err) +// } +// if len(walIDs) != 1 { +// t.Fatal("expected 1 WAL, got", len(walIDs)) +// } +// 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) +// } + +// // Trigger a retry on the rotation, it should use WAL's new password +// _, err = b.HandleRequest(ctx, &logical.Request{ +// Operation: logical.UpdateOperation, +// Path: "rotate-role/hashicorp", +// Storage: storage, +// }) +// if err != nil { +// t.Fatal(err) +// } + +// role, err = b.StaticRole(ctx, storage, "hashicorp") +// if err != nil { +// t.Fatal(err) +// } +// if role.StaticAccount.Password != wal.NewPassword { +// t.Fatal(role.StaticAccount.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.BackendConfig{ +// Logger: logging.NewVaultLogger(hclog.Debug), + +// System: &logical.StaticSystemView{ +// DefaultLeaseTTLVal: time.Hour * 12, +// MaxLeaseTTLVal: time.Hour * 24, +// }, +// StorageView: &logical.InmemStorage{}, +// } + +// b := Backend(&fakeLdapClient{throwErrs: false}) +// b.Setup(context.Background(), config) + +// b.credRotationQueue = queue.New() +// initCtx := context.Background() +// ictx, cancel := context.WithCancel(initCtx) +// b.cancelQueue = cancel + +// defer b.Cleanup(ctx) +// configureDBMount(t, b, config.StorageView) +// createRole(t, b, config.StorageView, "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) +// // Remove the role from the rotation queue to simulate startup memory state +// _, err = b.popFromRotationQueueByKey("hashicorp") +// if err != nil { +// t.Fatal(err) +// } + +// // Now finish the startup process by populating the queue, which should discard the WAL +// b.initQueue(ictx, config, consts.ReplicationDRPrimary) + +// if tc.shouldRotate { +// requireWALs(t, config.StorageView, 1) +// } else { +// requireWALs(t, config.StorageView, 0) +// } + +// // Run one tick +// b.rotateCredentials(ctx, config.StorageView) +// requireWALs(t, config.StorageView, 0) + +// role, err = b.StaticRole(ctx, config.StorageView, "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 := getBackend(t, false) +// defer b.Cleanup(ctx) +// configureDBMount(t, b, storage) +// createRole(t, b, storage, "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, roleName string) { +// t.Helper() +// // Fail to rotate the roles +// ldapClient := b.client.(*fakeLdapClient) +// originalValue := ldapClient.throwErrs +// ldapClient.throwErrs = true +// defer func() { +// ldapClient.throwErrs = originalValue +// }() + +// _, err := b.HandleRequest(context.Background(), &logical.Request{ +// Operation: logical.UpdateOperation, +// Path: "rotate-role/" + roleName, +// Storage: storage, +// }) +// if err == nil { +// t.Fatal("expected error") +// } +// } func requireWALs(t *testing.T, storage logical.Storage, expectedCount int) { t.Helper() From b07aa5328b7905baf7fa66040de6170ac115dbfe Mon Sep 17 00:00:00 2001 From: Hridoy Roy Date: Fri, 17 Sep 2021 14:55:12 -0700 Subject: [PATCH 14/28] Use newly generated password in WAL Co-authored-by: Michael Golowka <72365+pcman312@users.noreply.github.com> --- builtin/logical/database/rotation.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/builtin/logical/database/rotation.go b/builtin/logical/database/rotation.go index f3d990de652e5..91bef0316b750 100644 --- a/builtin/logical/database/rotation.go +++ b/builtin/logical/database/rotation.go @@ -373,7 +373,7 @@ func (b *databaseBackend) setStaticAccount(ctx context.Context, s logical.Storag output.WALID, err = framework.PutWAL(ctx, s, staticWALKey, &setCredentialsWAL{ RoleName: input.RoleName, Username: config.Username, - NewPassword: config.Password, + NewPassword: newPassword, LastVaultRotation: input.Role.StaticAccount.LastVaultRotation, }) if err != nil { From 76492975121e9352f2caff1156704799d99bf92e Mon Sep 17 00:00:00 2001 From: Calvin Leung Huang <1883212+calvn@users.noreply.github.com> Date: Mon, 20 Sep 2021 15:16:41 -0700 Subject: [PATCH 15/28] return err on popFromRotationQueueByKey error; cleanup on setStaticAccount --- builtin/logical/database/path_roles.go | 8 ++------ builtin/logical/database/rotation.go | 8 +------- 2 files changed, 3 insertions(+), 13 deletions(-) diff --git a/builtin/logical/database/path_roles.go b/builtin/logical/database/path_roles.go index 0e4ca5d6c73b8..8c5f9e0980316 100644 --- a/builtin/logical/database/path_roles.go +++ b/builtin/logical/database/path_roles.go @@ -541,12 +541,8 @@ func (b *databaseBackend) pathStaticRoleCreateUpdate(ctx context.Context, req *l return nil, err } item, err = b.popFromRotationQueueByKey(name) - if err != nil || item == nil { - b.Logger().Warn("expected role to exist in rotation queue but none found", "role", name, "error", err) - item = &queue.Item{ - Key: name, - Priority: lvr.Add(role.StaticAccount.RotationPeriod).Unix(), - } + if err != nil { + return nil, err } } diff --git a/builtin/logical/database/rotation.go b/builtin/logical/database/rotation.go index 91bef0316b750..0598a4adf3ad1 100644 --- a/builtin/logical/database/rotation.go +++ b/builtin/logical/database/rotation.go @@ -9,7 +9,6 @@ import ( "github.com/hashicorp/errwrap" "github.com/hashicorp/go-secure-stdlib/strutil" - 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" @@ -360,11 +359,6 @@ func (b *databaseBackend) setStaticAccount(ctx context.Context, s logical.Storag } } - config := v4.StaticUserConfig{ - Username: input.Role.StaticAccount.Username, - Password: newPassword, - } - if output.WALID == "" { newPassword, err = dbi.database.GeneratePassword(ctx, b.System(), dbConfig.PasswordPolicy) if err != nil { @@ -372,7 +366,7 @@ func (b *databaseBackend) setStaticAccount(ctx context.Context, s logical.Storag } output.WALID, err = framework.PutWAL(ctx, s, staticWALKey, &setCredentialsWAL{ RoleName: input.RoleName, - Username: config.Username, + Username: input.Role.StaticAccount.Username, NewPassword: newPassword, LastVaultRotation: input.Role.StaticAccount.LastVaultRotation, }) From 747e0ee0ad92d42b12c92f8280794dbffc25dcf3 Mon Sep 17 00:00:00 2001 From: Calvin Leung Huang <1883212+calvn@users.noreply.github.com> Date: Mon, 20 Sep 2021 15:41:21 -0700 Subject: [PATCH 16/28] test: fix TestPlugin_lifecycle --- builtin/logical/database/versioning_large_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/builtin/logical/database/versioning_large_test.go b/builtin/logical/database/versioning_large_test.go index 7b3cc82eee930..6d0931114cb29 100644 --- a/builtin/logical/database/versioning_large_test.go +++ b/builtin/logical/database/versioning_large_test.go @@ -173,7 +173,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{}{ From 9cd86cb6d08f05b4ee75caf3a4e090f32c02d4f9 Mon Sep 17 00:00:00 2001 From: Tom Proctor Date: Tue, 21 Sep 2021 12:35:59 +0100 Subject: [PATCH 17/28] 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 --- builtin/logical/database/path_roles.go | 1 + builtin/logical/database/path_roles_test.go | 224 ++++---- builtin/logical/database/rotation.go | 10 +- builtin/logical/database/rotation_test.go | 540 ++++++++++---------- 4 files changed, 401 insertions(+), 374 deletions(-) diff --git a/builtin/logical/database/path_roles.go b/builtin/logical/database/path_roles.go index 8c5f9e0980316..760f78c6fed71 100644 --- a/builtin/logical/database/path_roles.go +++ b/builtin/logical/database/path_roles.go @@ -524,6 +524,7 @@ func (b *databaseBackend) pathStaticRoleCreateUpdate(ctx context.Context, req *l err = merr.ErrorOrNil() } } + return nil, err } // guard against RotationTime not being set or zero-value diff --git a/builtin/logical/database/path_roles_test.go b/builtin/logical/database/path_roles_test.go index 9686ecd2314d1..b66f85d5475fe 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"} @@ -529,131 +532,132 @@ func TestBackend_StaticRole_Role_name_check(t *testing.T) { } } -// func TestWALsStillTrackedAfterUpdate(t *testing.T) { -// ctx := context.Background() -// b, storage := getBackend(t, false) -// defer b.Cleanup(ctx) -// configureDBMount(t, b, storage) - -// createRole(t, b, storage, "hashicorp") - -// generateWALFromFailedRotation(t, b, storage, "hashicorp") -// requireWALs(t, storage, 1) - -// _, 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 { -// t.Fatal(err) -// } -// requireWALs(t, storage, 1) - -// // Check we've still got track of it in the queue as well -// item, err := b.credRotationQueue.PopByKey("hashicorp") -// if err != nil { -// t.Fatal(err) -// } -// if wal, ok := item.Value.(string); !ok || wal == "" { -// t.Fatal("should have a WAL ID in the rotation queue") -// } -// } - -// func TestWALsDeletedOnRoleCreationFailed(t *testing.T) { -// ctx := context.Background() -// b, storage := getBackend(t, true) -// defer b.Cleanup(ctx) -// configureDBMount(t, b, storage) - -// for i := 0; i < 3; i++ { -// _, err := b.HandleRequest(ctx, &logical.Request{ -// Operation: logical.CreateOperation, -// Path: "static-roles/hashicorp", -// Storage: storage, -// Data: map[string]interface{}{ -// "username": "hashicorp", -// "dn": "uid=hashicorp,ou=users,dc=hashicorp,dc=com", -// "rotation_period": "5s", -// }, -// }) -// if err == nil { -// t.Fatal("expected error from OpenLDAP") -// } -// } - -// requireWALs(t, storage, 0) -// } - -// func TestWALsDeletedOnRoleDeletion(t *testing.T) { -// ctx := context.Background() -// b, storage := getBackend(t, false) -// defer b.Cleanup(ctx) -// configureDBMount(t, b, storage) - -// // Create the roles -// roleNames := []string{"hashicorp", "2"} -// for _, roleName := range roleNames { -// createRole(t, b, storage, roleName) -// } - -// // Fail to rotate the roles -// for _, roleName := range roleNames { -// generateWALFromFailedRotation(t, b, storage, roleName) -// } - -// // Should have 2 WALs hanging around -// requireWALs(t, storage, 2) - -// // Delete one of the static roles -// _, err := b.HandleRequest(ctx, &logical.Request{ -// Operation: logical.DeleteOperation, -// Path: "static-role/hashicorp", -// Storage: storage, -// }) -// if err != nil { -// t.Fatal(err) -// } - -// // 1 WAL should be cleared by the delete -// requireWALs(t, storage, 1) -// } - -func configureDBMount(t *testing.T, b *databaseBackend, storage logical.Storage) { - t.Helper() - resp, err := b.HandleRequest(context.Background(), &logical.Request{ - Operation: logical.CreateOperation, - Path: "config", +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{}{ - "binddn": "tester", - "bindpass": "pa$$w0rd", - "url": "ldap://138.91.247.105", + "username": "hashicorp", + "dn": "uid=hashicorp,ou=users,dc=hashicorp,dc=com", + "rotation_period": "600s", }, }) if err != nil || (resp != nil && resp.IsError()) { - t.Fatalf("err:%s resp:%#v\n", err, resp) + 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 createRole(t *testing.T, b *databaseBackend, storage logical.Storage, roleName string) { - _, err := b.HandleRequest(context.Background(), &logical.Request{ +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-role/" + roleName, + Path: "static-roles/" + roleName, Storage: storage, Data: map[string]interface{}{ "username": roleName, - "dn": "uid=hashicorp,ou=users,dc=hashicorp,dc=com", + "db_name": "mockv5", "rotation_period": "86400s", }, }) - if err != nil { - t.Fatal(err) + if err != nil || (resp != nil && resp.IsError()) { + t.Fatal(resp, err) } } diff --git a/builtin/logical/database/rotation.go b/builtin/logical/database/rotation.go index 0598a4adf3ad1..fddfff3f0be58 100644 --- a/builtin/logical/database/rotation.go +++ b/builtin/logical/database/rotation.go @@ -451,7 +451,13 @@ func (b *databaseBackend) initQueue(ctx context.Context, conf *logical.BackendCo walID, err := framework.PutWAL(ctx, conf.StorageView, staticWALKey, &setCredentialsWAL{RoleName: "vault-readonlytest"}) if walID != "" { - defer framework.DeleteWAL(ctx, conf.StorageView, walID) + defer func() { + b.logger.Debug("deleting readonly test WAL") + err := framework.DeleteWAL(ctx, conf.StorageView, walID) + if err != nil { + b.logger.Debug("failed to clean up readonly test WAL", "error", err) + } + }() } switch { case err == nil: @@ -513,7 +519,7 @@ 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) + b.Logger().Debug("deleting WAL with nil role or static account", "WAL ID", walEntry.walID, "WAL", walEntry) if err := framework.DeleteWAL(ctx, s, walEntry.walID); err != nil { b.Logger().Warn("unable to delete WAL", "error", err, "WAL ID", walEntry.walID) } diff --git a/builtin/logical/database/rotation_test.go b/builtin/logical/database/rotation_test.go index 2ee15f587beb9..f3d8a1231cfba 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" @@ -10,18 +12,18 @@ import ( "time" "github.com/Sectorbob/mlab-ns2/gae/ns/digest" - "github.com/hashicorp/go-hclog" "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/helper/logging" "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" ) @@ -1098,246 +1100,235 @@ func TestBackend_StaticRole_Rotate_Invalid_Role(t *testing.T) { } } -// func TestRollsPasswordForwards(t *testing.T) { -// ctx := context.Background() -// b, storage := getBackend(t, false) -// defer b.Cleanup(ctx) -// configureDBMount(t, b, storage) -// createRole(t, b, storage, "hashicorp") - -// role, err := b.StaticRole(ctx, storage, "hashicorp") -// if err != nil { -// t.Fatal(err) -// } -// oldPassword := role.StaticAccount.Password - -// generateWALFromFailedRotation(t, b, storage, "hashicorp") -// walIDs, err := storage.List(context.Background(), "wal/") -// if err != nil { -// t.Fatal(err) -// } -// if len(walIDs) != 1 { -// t.Fatal("expected 1 WAL, got", len(walIDs)) -// } -// 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) -// } - -// // Trigger a retry on the rotation, it should use WAL's new password -// _, err = b.HandleRequest(ctx, &logical.Request{ -// Operation: logical.UpdateOperation, -// Path: "rotate-role/hashicorp", -// Storage: storage, -// }) -// if err != nil { -// t.Fatal(err) -// } - -// role, err = b.StaticRole(ctx, storage, "hashicorp") -// if err != nil { -// t.Fatal(err) -// } -// if role.StaticAccount.Password != wal.NewPassword { -// t.Fatal(role.StaticAccount.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.BackendConfig{ -// Logger: logging.NewVaultLogger(hclog.Debug), - -// System: &logical.StaticSystemView{ -// DefaultLeaseTTLVal: time.Hour * 12, -// MaxLeaseTTLVal: time.Hour * 24, -// }, -// StorageView: &logical.InmemStorage{}, -// } - -// b := Backend(&fakeLdapClient{throwErrs: false}) -// b.Setup(context.Background(), config) - -// b.credRotationQueue = queue.New() -// initCtx := context.Background() -// ictx, cancel := context.WithCancel(initCtx) -// b.cancelQueue = cancel - -// defer b.Cleanup(ctx) -// configureDBMount(t, b, config.StorageView) -// createRole(t, b, config.StorageView, "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) -// // Remove the role from the rotation queue to simulate startup memory state -// _, err = b.popFromRotationQueueByKey("hashicorp") -// if err != nil { -// t.Fatal(err) -// } - -// // Now finish the startup process by populating the queue, which should discard the WAL -// b.initQueue(ictx, config, consts.ReplicationDRPrimary) - -// if tc.shouldRotate { -// requireWALs(t, config.StorageView, 1) -// } else { -// requireWALs(t, config.StorageView, 0) -// } - -// // Run one tick -// b.rotateCredentials(ctx, config.StorageView) -// requireWALs(t, config.StorageView, 0) - -// role, err = b.StaticRole(ctx, config.StorageView, "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 := getBackend(t, false) -// defer b.Cleanup(ctx) -// configureDBMount(t, b, storage) -// createRole(t, b, storage, "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, roleName string) { -// t.Helper() -// // Fail to rotate the roles -// ldapClient := b.client.(*fakeLdapClient) -// originalValue := ldapClient.throwErrs -// ldapClient.throwErrs = true -// defer func() { -// ldapClient.throwErrs = originalValue -// }() - -// _, err := b.HandleRequest(context.Background(), &logical.Request{ -// Operation: logical.UpdateOperation, -// Path: "rotate-role/" + roleName, -// Storage: storage, -// }) -// if err == nil { -// t.Fatal("expected error") -// } -// } - -func requireWALs(t *testing.T, storage logical.Storage, expectedCount int) { +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.ReplicationDRPrimary) + + 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 { @@ -1346,36 +1337,61 @@ func requireWALs(t *testing.T, storage logical.Storage, expectedCount int) { if len(wals) != expectedCount { t.Fatal("expected WALs", expectedCount, "got", len(wals)) } + + return wals } -func getBackend(t *testing.T, throwsErr bool) (*databaseBackend, logical.Storage) { +func getBackend(t *testing.T) (*databaseBackend, logical.Storage, *mockNewDatabase) { t.Helper() - config := &logical.BackendConfig{ - Logger: logging.NewVaultLogger(hclog.Debug), + config := logical.TestBackendConfig() + config.StorageView = &logical.InmemStorage{} + lb, err := Factory(context.Background(), config) + if err != nil { + t.Fatal(err) + } + b, ok := lb.(*databaseBackend) + if !ok { + t.Fatal("could not convert to database backend") + } - System: &logical.StaticSystemView{ - DefaultLeaseTTLVal: time.Hour * 12, - MaxLeaseTTLVal: time.Hour * 24, - }, - StorageView: &logical.InmemStorage{}, + 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, } - b := Backend(config) - if err := b.Setup(context.Background(), config); err != nil { - t.Fatal(err) + dbi := &dbPluginInstance{ + database: dbw, + id: "foo-id", + name: "mockV5", } + b.connections["mockv5"] = dbi - b.credRotationQueue = queue.New() - // Create a context with a cancel method for processing any WAL entries and - // populating the queue - initCtx := context.Background() - ictx, cancel := context.WithCancel(initCtx) - b.cancelQueue = cancel + return mockDB +} - // Load queue and kickoff new periodic ticker - b.initQueue(ictx, config, consts.ReplicationPerformancePrimary) +// 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) + } - return b, config.StorageView + err = storage.Put(context.Background(), entry) + if err != nil { + t.Fatal(err) + } } // capturePasswords captures the current passwords at the time of calling, and From 63c3d0c83c70ce2dcd55062cb963a9fa3612da8d Mon Sep 17 00:00:00 2001 From: Tom Proctor Date: Tue, 21 Sep 2021 12:48:43 +0100 Subject: [PATCH 18/28] Undo logging updates --- builtin/logical/database/rotation.go | 10 ++-------- 1 file changed, 2 insertions(+), 8 deletions(-) diff --git a/builtin/logical/database/rotation.go b/builtin/logical/database/rotation.go index fddfff3f0be58..0598a4adf3ad1 100644 --- a/builtin/logical/database/rotation.go +++ b/builtin/logical/database/rotation.go @@ -451,13 +451,7 @@ func (b *databaseBackend) initQueue(ctx context.Context, conf *logical.BackendCo walID, err := framework.PutWAL(ctx, conf.StorageView, staticWALKey, &setCredentialsWAL{RoleName: "vault-readonlytest"}) if walID != "" { - defer func() { - b.logger.Debug("deleting readonly test WAL") - err := framework.DeleteWAL(ctx, conf.StorageView, walID) - if err != nil { - b.logger.Debug("failed to clean up readonly test WAL", "error", err) - } - }() + defer framework.DeleteWAL(ctx, conf.StorageView, walID) } switch { case err == nil: @@ -519,7 +513,7 @@ 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, "WAL", walEntry) + 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) } From 45af1604a82d239721c10fe191d8f9cbc4dbca95 Mon Sep 17 00:00:00 2001 From: Tom Proctor Date: Tue, 21 Sep 2021 12:56:27 +0100 Subject: [PATCH 19/28] Add changelog --- changelog/12563.txt | 3 +++ 1 file changed, 3 insertions(+) create mode 100644 changelog/12563.txt 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. +``` From 0c0122661aa4174bebacf94905795f65c67d7480 Mon Sep 17 00:00:00 2001 From: Tom Proctor Date: Tue, 21 Sep 2021 13:17:25 +0100 Subject: [PATCH 20/28] Remove ticker and background threads from WAL tests --- builtin/logical/database/rotation_test.go | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/builtin/logical/database/rotation_test.go b/builtin/logical/database/rotation_test.go index f3d8a1231cfba..313c535bea671 100644 --- a/builtin/logical/database/rotation_test.go +++ b/builtin/logical/database/rotation_test.go @@ -1345,14 +1345,14 @@ func getBackend(t *testing.T) (*databaseBackend, logical.Storage, *mockNewDataba t.Helper() config := logical.TestBackendConfig() config.StorageView = &logical.InmemStorage{} - lb, err := Factory(context.Background(), config) - if err != nil { + // 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, ok := lb.(*databaseBackend) - if !ok { - t.Fatal("could not convert to database backend") - } + b.credRotationQueue = queue.New() + b.populateQueue(context.Background(), config.StorageView) mockDB := setupMockDB(b) From 166fe7da230426d29226b1b8d4a172d58a258f85 Mon Sep 17 00:00:00 2001 From: Tom Proctor Date: Tue, 21 Sep 2021 13:29:15 +0100 Subject: [PATCH 21/28] Keep pre-existing API behaviour of allowing update static role to act as a create --- builtin/logical/database/path_roles.go | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/builtin/logical/database/path_roles.go b/builtin/logical/database/path_roles.go index 760f78c6fed71..72b2279063a1c 100644 --- a/builtin/logical/database/path_roles.go +++ b/builtin/logical/database/path_roles.go @@ -543,10 +543,13 @@ func (b *databaseBackend) pathStaticRoleCreateUpdate(ctx context.Context, req *l } item, err = b.popFromRotationQueueByKey(name) if err != nil { - return nil, err + // This update operation is actually performing a create. + item = &queue.Item{ + Key: name, + } } - } + item.Priority = lvr.Add(role.StaticAccount.RotationPeriod).Unix() // Add their rotation to the queue From 5b88f40d54fd2913bd462ba6f947d69844fee746 Mon Sep 17 00:00:00 2001 From: Tom Proctor Date: Tue, 21 Sep 2021 14:11:02 +0100 Subject: [PATCH 22/28] Switch test back to update operation --- builtin/logical/database/versioning_large_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/builtin/logical/database/versioning_large_test.go b/builtin/logical/database/versioning_large_test.go index 6d0931114cb29..7b3cc82eee930 100644 --- a/builtin/logical/database/versioning_large_test.go +++ b/builtin/logical/database/versioning_large_test.go @@ -173,7 +173,7 @@ func TestPlugin_lifecycle(t *testing.T) { // Create static role staticRoleName := "static-role" req = &logical.Request{ - Operation: logical.CreateOperation, + Operation: logical.UpdateOperation, Path: fmt.Sprintf("static-roles/%s", staticRoleName), Storage: config.StorageView, Data: map[string]interface{}{ From a6765ffa8b69f93c4df87380be1216463a4c67db Mon Sep 17 00:00:00 2001 From: Tom Proctor Date: Tue, 21 Sep 2021 16:37:19 +0100 Subject: [PATCH 23/28] Revert my revert, and fix some test bugs --- builtin/logical/database/path_roles.go | 5 +---- builtin/logical/database/versioning_large_test.go | 10 +++++----- 2 files changed, 6 insertions(+), 9 deletions(-) diff --git a/builtin/logical/database/path_roles.go b/builtin/logical/database/path_roles.go index 72b2279063a1c..6c0d901836826 100644 --- a/builtin/logical/database/path_roles.go +++ b/builtin/logical/database/path_roles.go @@ -543,10 +543,7 @@ func (b *databaseBackend) pathStaticRoleCreateUpdate(ctx context.Context, req *l } item, err = b.popFromRotationQueueByKey(name) if err != nil { - // This update operation is actually performing a create. - item = &queue.Item{ - Key: name, - } + return nil, err } } diff --git a/builtin/logical/database/versioning_large_test.go b/builtin/logical/database/versioning_large_test.go index 7b3cc82eee930..02e74e1fed909 100644 --- a/builtin/logical/database/versioning_large_test.go +++ b/builtin/logical/database/versioning_large_test.go @@ -82,7 +82,7 @@ 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) + defer cleanup(t, b, &cleanupReqs) // ///////////////////////////////////////////////////////////////// // Configure @@ -173,7 +173,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{}{ @@ -213,13 +213,13 @@ func TestPlugin_lifecycle(t *testing.T) { } } -func cleanup(t *testing.T, b *databaseBackend, reqs []*logical.Request) { +func cleanup(t *testing.T, b *databaseBackend, reqs *[]*logical.Request) { ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) defer cancel() // Go in stack order so it works similar to defer - for i := len(reqs) - 1; i >= 0; i-- { - req := reqs[i] + for i := len(*reqs) - 1; i >= 0; i-- { + req := (*reqs)[i] resp, err := b.HandleRequest(ctx, req) if err != nil { t.Fatalf("Error cleaning up: %s", err) From 52137e69591b8f0bc571eaf03d33b1d8ad643eac Mon Sep 17 00:00:00 2001 From: Tom Proctor Date: Tue, 21 Sep 2021 17:28:29 +0100 Subject: [PATCH 24/28] Fix TestBackend_StaticRole_LockRegression --- builtin/logical/database/rotation_test.go | 26 ++++++++++++++++------- 1 file changed, 18 insertions(+), 8 deletions(-) diff --git a/builtin/logical/database/rotation_test.go b/builtin/logical/database/rotation_test.go index 313c535bea671..35dcf2fb9acdd 100644 --- a/builtin/logical/database/rotation_test.go +++ b/builtin/logical/database/rotation_test.go @@ -980,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", From 83e9387a725ba88a266eb4c28a2655382b4afd5f Mon Sep 17 00:00:00 2001 From: Calvin Leung Huang <1883212+calvn@users.noreply.github.com> Date: Tue, 21 Sep 2021 15:04:03 -0700 Subject: [PATCH 25/28] clean up defer on TestPlugin_lifecycle --- builtin/logical/database/versioning_large_test.go | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/builtin/logical/database/versioning_large_test.go b/builtin/logical/database/versioning_large_test.go index 02e74e1fed909..2f7277fd826cf 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 @@ -213,13 +217,13 @@ func TestPlugin_lifecycle(t *testing.T) { } } -func cleanup(t *testing.T, b *databaseBackend, reqs *[]*logical.Request) { +func cleanup(t *testing.T, b *databaseBackend, reqs []*logical.Request) { ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) defer cancel() // Go in stack order so it works similar to defer - for i := len(*reqs) - 1; i >= 0; i-- { - req := (*reqs)[i] + for i := len(reqs) - 1; i >= 0; i-- { + req := (reqs)[i] resp, err := b.HandleRequest(ctx, req) if err != nil { t.Fatalf("Error cleaning up: %s", err) From 7c29a761a1c3e89c3add1e81e271cc93a043d733 Mon Sep 17 00:00:00 2001 From: Calvin Leung Huang <1883212+calvn@users.noreply.github.com> Date: Tue, 21 Sep 2021 15:06:30 -0700 Subject: [PATCH 26/28] unwrap reqs on cleanup --- builtin/logical/database/versioning_large_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/builtin/logical/database/versioning_large_test.go b/builtin/logical/database/versioning_large_test.go index 2f7277fd826cf..db2941308507e 100644 --- a/builtin/logical/database/versioning_large_test.go +++ b/builtin/logical/database/versioning_large_test.go @@ -223,7 +223,7 @@ func cleanup(t *testing.T, b *databaseBackend, reqs []*logical.Request) { // Go in stack order so it works similar to defer for i := len(reqs) - 1; i >= 0; i-- { - req := (reqs)[i] + req := reqs[i] resp, err := b.HandleRequest(ctx, req) if err != nil { t.Fatalf("Error cleaning up: %s", err) From dec64cd46314921189dedd032cc060af08e6efee Mon Sep 17 00:00:00 2001 From: Calvin Leung Huang <1883212+calvn@users.noreply.github.com> Date: Tue, 21 Sep 2021 16:55:10 -0700 Subject: [PATCH 27/28] setStaticAccount: don't hold a write lock --- builtin/logical/database/rotation.go | 4 ++-- builtin/logical/database/rotation_test.go | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/builtin/logical/database/rotation.go b/builtin/logical/database/rotation.go index 0598a4adf3ad1..3e732ff2e7877 100644 --- a/builtin/logical/database/rotation.go +++ b/builtin/logical/database/rotation.go @@ -327,8 +327,8 @@ func (b *databaseBackend) setStaticAccount(ctx context.Context, s logical.Storag return output, err } - dbi.Lock() - defer dbi.Unlock() + dbi.RLock() + defer dbi.RUnlock() // 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 diff --git a/builtin/logical/database/rotation_test.go b/builtin/logical/database/rotation_test.go index 35dcf2fb9acdd..c275e84a7ddcb 100644 --- a/builtin/logical/database/rotation_test.go +++ b/builtin/logical/database/rotation_test.go @@ -1217,7 +1217,7 @@ func TestStoredWALsCorrectlyProcessed(t *testing.T) { b.credRotationQueue = queue.New() // Now finish the startup process by populating the queue, which should discard the WAL - b.initQueue(ctx, config, consts.ReplicationDRPrimary) + b.initQueue(ctx, config, consts.ReplicationPerformancePrimary) if tc.shouldRotate { requireWALs(t, storage, 1) From 9e0c96291cf5864773d51897c1e0a5c4537d3fc9 Mon Sep 17 00:00:00 2001 From: Calvin Leung Huang <1883212+calvn@users.noreply.github.com> Date: Tue, 21 Sep 2021 17:06:51 -0700 Subject: [PATCH 28/28] TestStoredWALsCorrectlyProcessed: set replication state to unknown --- builtin/logical/database/rotation_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/builtin/logical/database/rotation_test.go b/builtin/logical/database/rotation_test.go index c275e84a7ddcb..2031bdd943121 100644 --- a/builtin/logical/database/rotation_test.go +++ b/builtin/logical/database/rotation_test.go @@ -1217,7 +1217,7 @@ func TestStoredWALsCorrectlyProcessed(t *testing.T) { b.credRotationQueue = queue.New() // Now finish the startup process by populating the queue, which should discard the WAL - b.initQueue(ctx, config, consts.ReplicationPerformancePrimary) + b.initQueue(ctx, config, consts.ReplicationUnknown) if tc.shouldRotate { requireWALs(t, storage, 1)