From dac711c0c32a0395ababdb791f6035a557b84dda Mon Sep 17 00:00:00 2001 From: Jim Kalafut Date: Sat, 21 Aug 2021 09:55:55 -0700 Subject: [PATCH 01/23] Guard against premature rotation --- rotation.go | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/rotation.go b/rotation.go index ba98ab2..ee69a4f 100644 --- a/rotation.go +++ b/rotation.go @@ -183,6 +183,21 @@ func (b *backend) rotateCredential(ctx context.Context, s logical.Storage) bool return false } + nextRotate := role.StaticAccount.NextRotationTime() + if time.Now().Before(nextRotate) { + b.Logger().Warn( + "unexpected early rotation request being ignored", + "last static role rotation", role.StaticAccount.LastVaultRotation, + "queued priority", time.Unix(item.Priority, 0), + ) + + item.Priority = nextRotate.Unix() + if err := b.pushItem(item); err != nil { + b.Logger().Error("unable to push item on to queue", "error", err) + } + return true + } + input := &setStaticAccountInput{ RoleName: item.Key, Role: role, From ea4631487be4a4b25b0f9de6ad15991ecaaf4fc2 Mon Sep 17 00:00:00 2001 From: Jim Kalafut Date: Sun, 22 Aug 2021 23:24:09 -0700 Subject: [PATCH 02/23] Add more logging --- backend.go | 2 ++ rotation.go | 9 ++++++++- 2 files changed, 10 insertions(+), 1 deletion(-) diff --git a/backend.go b/backend.go index 08365d9..6a3fdea 100644 --- a/backend.go +++ b/backend.go @@ -18,6 +18,8 @@ func Factory(ctx context.Context, conf *logical.BackendConfig) (logical.Backend, return nil, err } + b.Logger().Info("started openldap", "version", "v0.5.1-prem-rotation-guard") + return b, nil } diff --git a/rotation.go b/rotation.go index ee69a4f..4a303d8 100644 --- a/rotation.go +++ b/rotation.go @@ -70,6 +70,7 @@ func (b *backend) populateQueue(ctx context.Context, s logical.Storage) { if walEntry != nil { // Check walEntry last vault time if !walEntry.LastVaultRotation.IsZero() && walEntry.LastVaultRotation.Before(role.StaticAccount.LastVaultRotation) { + log.Debug("deleting outdated WAL", "WAL ID", walEntry.walID) // WAL's last vault rotation record is older than the role's data, so // delete and move on if err := framework.DeleteWAL(ctx, s, walEntry.walID); err != nil { @@ -79,6 +80,7 @@ func (b *backend) populateQueue(ctx context.Context, s logical.Storage) { log.Info("adjusting priority for Role") item.Value = walEntry.walID item.Priority = time.Now().Unix() + log.Debug("adjusted Role", "WAL ID", walEntry.walID, "priority", time.Unix(item.Priority,0)) } } @@ -183,11 +185,14 @@ func (b *backend) rotateCredential(ctx context.Context, s logical.Storage) bool return false } + // Verify times against the role. Don't use.Before() for the comparison here + // since there is a slight gap between that and the integer comparison against item.priority. nextRotate := role.StaticAccount.NextRotationTime() - if time.Now().Before(nextRotate) { + if time.Now().Unix() < nextRotate.Unix() { b.Logger().Warn( "unexpected early rotation request being ignored", "last static role rotation", role.StaticAccount.LastVaultRotation, + "next scheduled role rotation", nextRotate, "queued priority", time.Unix(item.Priority, 0), ) @@ -484,6 +489,8 @@ func (b *backend) loadStaticWALs(ctx context.Context, s logical.Storage) (map[st continue } + b.Logger().Debug("loaded WAL", "WAL ID", walID) + walEntry.walID = walID walMap[walEntry.RoleName] = walEntry } From 59eafe9920e571e60f814f2d8c64793d0530e3cb Mon Sep 17 00:00:00 2001 From: Jim Kalafut Date: Mon, 23 Aug 2021 08:00:13 -0700 Subject: [PATCH 03/23] Fix logged version number --- backend.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/backend.go b/backend.go index 6a3fdea..af13b8c 100644 --- a/backend.go +++ b/backend.go @@ -18,7 +18,7 @@ func Factory(ctx context.Context, conf *logical.BackendConfig) (logical.Backend, return nil, err } - b.Logger().Info("started openldap", "version", "v0.5.1-prem-rotation-guard") + b.Logger().Info("started openldap", "version", "v0.3.0-prem-rotation-guard") return b, nil } From fb8d6db7d595bbdc00587ec49aff5915ae9dae20 Mon Sep 17 00:00:00 2001 From: Jim Kalafut Date: Mon, 23 Aug 2021 10:42:10 -0700 Subject: [PATCH 04/23] Stop current rotation loop if we hit a premature rotation condition --- rotation.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/rotation.go b/rotation.go index 4a303d8..ac9551f 100644 --- a/rotation.go +++ b/rotation.go @@ -200,7 +200,7 @@ func (b *backend) rotateCredential(ctx context.Context, s logical.Storage) bool if err := b.pushItem(item); err != nil { b.Logger().Error("unable to push item on to queue", "error", err) } - return true + return false } input := &setStaticAccountInput{ From 61fa8a7157cd87efb09eed4bc6976c3667b388c5 Mon Sep 17 00:00:00 2001 From: Jim Kalafut Date: Mon, 23 Aug 2021 11:02:43 -0700 Subject: [PATCH 05/23] Add more WAL logging --- rotation.go | 3 +++ 1 file changed, 3 insertions(+) diff --git a/rotation.go b/rotation.go index ac9551f..6b977c2 100644 --- a/rotation.go +++ b/rotation.go @@ -363,6 +363,7 @@ func (b *backend) setStaticAccountPassword(ctx context.Context, s logical.Storag if err != nil { return output, errwrap.Wrapf("error writing WAL entry: {{err}}", err) } + b.Logger().Debug("writing WAL", "WAL ID", output.WALID) } // Update the password remotely. @@ -399,9 +400,11 @@ to %s, configure a new binddn and bindpass to restore openldap function`, pwdSto // Cleanup WAL after successfully rotating and pushing new item on to queue if err := framework.DeleteWAL(ctx, s, output.WALID); err != nil { + b.Logger().Warn("error deleting WAL", "WAL ID", output.WALID, "error", err) merr = multierror.Append(merr, 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 From 2cf0db22bb32385003d58d78b58a4e568e53e822 Mon Sep 17 00:00:00 2001 From: Jim Kalafut Date: Mon, 23 Aug 2021 12:16:34 -0700 Subject: [PATCH 06/23] More WAL logging --- rotation.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/rotation.go b/rotation.go index 6b977c2..4faab54 100644 --- a/rotation.go +++ b/rotation.go @@ -70,12 +70,12 @@ func (b *backend) populateQueue(ctx context.Context, s logical.Storage) { if walEntry != nil { // Check walEntry last vault time if !walEntry.LastVaultRotation.IsZero() && walEntry.LastVaultRotation.Before(role.StaticAccount.LastVaultRotation) { - log.Debug("deleting outdated WAL", "WAL ID", walEntry.walID) // WAL's last vault rotation record is older than the role's data, so // delete and move on if err := framework.DeleteWAL(ctx, s, walEntry.walID); err != nil { log.Warn("unable to delete WAL", "error", err, "WAL ID", walEntry.walID) } + log.Debug("deleted outdated WAL", "WAL ID", walEntry.walID) } else { log.Info("adjusting priority for Role") item.Value = walEntry.walID @@ -489,6 +489,7 @@ func (b *backend) loadStaticWALs(ctx context.Context, s logical.Storage) (map[st if err := framework.DeleteWAL(ctx, s, walEntry.walID); err != nil { b.Logger().Warn("unable to delete WAL", "error", err, "WAL ID", walEntry.walID) } + b.Logger().Debug("deleted WAL with nil role or static account", "WAL ID", walEntry.walID) continue } From 64e5a98a7f70bc4bd87b6a312e4761868df21594 Mon Sep 17 00:00:00 2001 From: Calvin Leung Huang <1883212+calvn@users.noreply.github.com> Date: Mon, 23 Aug 2021 13:47:34 -0700 Subject: [PATCH 07/23] gofmt rotation.go --- rotation.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/rotation.go b/rotation.go index 4faab54..3e5a60f 100644 --- a/rotation.go +++ b/rotation.go @@ -80,7 +80,7 @@ func (b *backend) populateQueue(ctx context.Context, s logical.Storage) { log.Info("adjusting priority for Role") item.Value = walEntry.walID item.Priority = time.Now().Unix() - log.Debug("adjusted Role", "WAL ID", walEntry.walID, "priority", time.Unix(item.Priority,0)) + log.Debug("adjusted Role", "WAL ID", walEntry.walID, "priority", time.Unix(item.Priority, 0)) } } From 5ec68fbaab19f4b725aa82e29761c8f82a9bfdd2 Mon Sep 17 00:00:00 2001 From: Tom Proctor Date: Fri, 10 Sep 2021 18:08:29 +0100 Subject: [PATCH 08/23] Fix early rotation for roles with WALs, handle multiple WALs per role --- backend.go | 2 -- rotation.go | 63 ++++++++++++++++++++++++++++++++++++++--------------- 2 files changed, 45 insertions(+), 20 deletions(-) diff --git a/backend.go b/backend.go index af13b8c..08365d9 100644 --- a/backend.go +++ b/backend.go @@ -18,8 +18,6 @@ func Factory(ctx context.Context, conf *logical.BackendConfig) (logical.Backend, return nil, err } - b.Logger().Info("started openldap", "version", "v0.3.0-prem-rotation-guard") - return b, nil } diff --git a/rotation.go b/rotation.go index 3e5a60f..5b8ecfd 100644 --- a/rotation.go +++ b/rotation.go @@ -62,7 +62,7 @@ func (b *backend) 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 @@ -72,15 +72,15 @@ func (b *backend) populateQueue(ctx context.Context, s logical.Storage) { if !walEntry.LastVaultRotation.IsZero() && 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) if err := framework.DeleteWAL(ctx, s, walEntry.walID); err != nil { log.Warn("unable to delete WAL", "error", err, "WAL ID", walEntry.walID) } - log.Debug("deleted outdated WAL", "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() - log.Debug("adjusted Role", "WAL ID", walEntry.walID, "priority", time.Unix(item.Priority, 0)) } } @@ -119,7 +119,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,13 +196,14 @@ func (b *backend) rotateCredential(ctx context.Context, s logical.Storage) bool "last static role rotation", role.StaticAccount.LastVaultRotation, "next scheduled role rotation", nextRotate, "queued priority", time.Unix(item.Priority, 0), + "WAL ID", item.Value.(string), ) item.Priority = nextRotate.Unix() if err := b.pushItem(item); err != nil { b.Logger().Error("unable to push item on to queue", "error", err) } - return false + return true } input := &setStaticAccountInput{ @@ -272,12 +275,13 @@ func (b *backend) findStaticWAL(ctx context.Context, s logical.Storage, id strin 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), - DN: data["dn"].(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), + DN: data["dn"].(string), } lvr, err := time.Parse(time.RFC3339, data["last_vault_rotation"].(string)) if err != nil { @@ -363,7 +367,7 @@ func (b *backend) setStaticAccountPassword(ctx context.Context, s logical.Storag if err != nil { return output, errwrap.Wrapf("error writing WAL entry: {{err}}", err) } - b.Logger().Debug("writing WAL", "WAL ID", output.WALID) + b.Logger().Debug("writing WAL", "role", input.RoleName, "WAL ID", output.WALID) } // Update the password remotely. @@ -400,8 +404,8 @@ to %s, configure a new binddn and bindpass to restore openldap function`, pwdSto // Cleanup WAL after successfully rotating and pushing new item on to queue if err := framework.DeleteWAL(ctx, s, output.WALID); err != nil { - b.Logger().Warn("error deleting WAL", "WAL ID", output.WALID, "error", err) 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) @@ -486,16 +490,39 @@ func (b *backend) loadStaticWALs(ctx context.Context, s logical.Storage) (map[st 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) } - b.Logger().Debug("deleted WAL with nil role or static account", "WAL ID", walEntry.walID) continue } - b.Logger().Debug("loaded WAL", "WAL ID", 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 + } + } - walEntry.walID = walID + b.Logger().Debug("loaded WAL", "WAL ID", walID) walMap[walEntry.RoleName] = walEntry } return walMap, nil From e3eb497364e2f50ad555782e2aeaa8857890ed39 Mon Sep 17 00:00:00 2001 From: Tom Proctor Date: Tue, 14 Sep 2021 13:12:22 +0100 Subject: [PATCH 09/23] Review changes: * Respect previous WAL's new password * Ensure only 1 WAL per role * Reinstate finding a WAL => rotate immediately * Remove patch check that would stop manual rotations when popping from the queue * Add a warning to manual rotation response when rotation not immediately successful * Remove re-storing of mount config when rotating unrelated roles * Discard all WALs with a previous rotation time of 0 * Remove deleted WAL IDs from queue items * Delete unused struct fields --- path_rotate.go | 11 +++- path_static_roles.go | 15 ++++-- rotation.go | 122 ++++++++++++++++++++----------------------- 3 files changed, 76 insertions(+), 72 deletions(-) diff --git a/path_rotate.go b/path_rotate.go index 669feae..633cf8a 100644 --- a/path_rotate.go +++ b/path_rotate.go @@ -149,6 +149,8 @@ func (b *backend) pathRotateRoleCredentialsUpdate(ctx context.Context, req *logi } } 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. We use pushErr here to distinguish between @@ -159,9 +161,16 @@ func (b *backend) pathRotateRoleCredentialsUpdate(ctx context.Context, req *logi return nil, pushErr } + if err != nil { + return &logical.Response{ + Warnings: []string{"unable to finish rotating credentials; retries will " + + "continue in the background but it is also safe to retry manually"}, + }, err + } + // We're not returning creds here because we do not know if its been processed // by the queue. - return nil, err + return nil, nil } // rollBackPassword uses naive exponential backoff to retry updating to an old password, diff --git a/path_static_roles.go b/path_static_roles.go index 4ff93ff..68f1eea 100644 --- a/path_static_roles.go +++ b/path_static_roles.go @@ -221,6 +221,7 @@ func (b *backend) pathStaticRoleCreateUpdate(ctx context.Context, req *logical.R // Only call setStaticAccountPassword if we're creating the role for the // first time + var item *queue.Item switch req.Operation { case logical.CreateOperation: // setStaticAccountPassword calls Storage.Put and saves the role to storage @@ -233,6 +234,9 @@ func (b *backend) pathStaticRoleCreateUpdate(ctx context.Context, req *logical.R } // 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(staticRolePath+name, role) @@ -247,17 +251,18 @@ func (b *backend) pathStaticRoleCreateUpdate(ctx context.Context, req *logical.R // the queue //TODO: Add retry logic - _, err = b.popFromRotationQueueByKey(name) + // We could be tracking a WAL ID for this role, ensure we keep it when + // it's re-added to the queue. + 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/rotation.go b/rotation.go index 5b8ecfd..39cfb8a 100644 --- a/rotation.go +++ b/rotation.go @@ -69,10 +69,20 @@ func (b *backend) populateQueue(ctx context.Context, s logical.Storage) { 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) + 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) } @@ -81,6 +91,7 @@ func (b *backend) populateQueue(ctx context.Context, s logical.Storage) { "role", item.Key, "WAL ID", walEntry.walID) item.Value = walEntry.walID + item.Priority = time.Now().Unix() } } @@ -187,25 +198,6 @@ func (b *backend) rotateCredential(ctx context.Context, s logical.Storage) bool return false } - // Verify times against the role. Don't use.Before() for the comparison here - // since there is a slight gap between that and the integer comparison against item.priority. - nextRotate := role.StaticAccount.NextRotationTime() - if time.Now().Unix() < nextRotate.Unix() { - b.Logger().Warn( - "unexpected early rotation request being ignored", - "last static role rotation", role.StaticAccount.LastVaultRotation, - "next scheduled role rotation", nextRotate, - "queued priority", time.Unix(item.Priority, 0), - "WAL ID", item.Value.(string), - ) - - item.Priority = nextRotate.Unix() - if err := b.pushItem(item); err != nil { - b.Logger().Error("unable to push item on to queue", "error", err) - } - return true - } - input := &setStaticAccountInput{ RoleName: item.Key, Role: role, @@ -214,18 +206,7 @@ func (b *backend) rotateCredential(ctx context.Context, s logical.Storage) bool // 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.setStaticAccountPassword(ctx, s, input) @@ -246,6 +227,8 @@ func (b *backend) rotateCredential(ctx context.Context, s logical.Storage) bool // 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() { @@ -293,16 +276,13 @@ func (b *backend) findStaticWAL(ctx context.Context, s logical.Storage, id strin } 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 @@ -323,8 +303,10 @@ type setStaticAccountOutput struct { // tasks must be handled outside of this method. func (b *backend) setStaticAccountPassword(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 nil, errors.New("input was empty when attempting to set credentials for static account") + return output, errors.New("input was empty when attempting to set credentials for static account") } if _, hasTimeout := ctx.Deadline(); !hasTimeout { @@ -333,35 +315,55 @@ func (b *backend) setStaticAccountPassword(ctx context.Context, s logical.Storag defer cancel() } - // Re-use WAL ID if present, otherwise PUT a new WAL - output := &setStaticAccountOutput{WALID: input.WALID} - config, err := readConfig(ctx, s) if err != nil { - return nil, err + return output, err } if config == nil { - return nil, errors.New("the config is currently unset") - } - - newPassword, err := b.GeneratePassword(ctx, config) - if err != nil { - return nil, err + return output, errors.New("the config is currently unset") } - oldPassword := input.Role.StaticAccount.Password - // Take out the backend lock since we are swapping out the connection b.Lock() defer b.Unlock() + var newPassword string + if output.WALID != "" { + wal, err := b.findStaticWAL(ctx, s, output.WALID) + if err != nil { + 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 = "" + } + } + if output.WALID == "" { + newPassword, err = b.GeneratePassword(ctx, config) + if err != nil { + return output, err + } output.WALID, err = framework.PutWAL(ctx, s, staticWALKey, &setCredentialsWAL{ RoleName: input.RoleName, Username: input.Role.StaticAccount.Username, DN: input.Role.StaticAccount.DN, NewPassword: newPassword, - OldPassword: oldPassword, + OldPassword: input.Role.StaticAccount.Password, LastVaultRotation: input.Role.StaticAccount.LastVaultRotation, }) if err != nil { @@ -372,19 +374,7 @@ func (b *backend) setStaticAccountPassword(ctx context.Context, s logical.Storag // Update the password remotely. if err := b.client.UpdatePassword(config.LDAP, input.Role.StaticAccount.DN, newPassword); err != nil { - return nil, err - } - - // Update the password locally. - if pwdStoringErr := storePassword(ctx, s, config); pwdStoringErr != nil { - // We were unable to store the new password locally. We can't continue in this state because we won't be able - // to roll any passwords, including our own to get back into a state of working. So, we need to roll back to - // the last password we successfully got into storage. - if rollbackErr := b.rollBackPassword(ctx, config, oldPassword); rollbackErr != nil { - return nil, fmt.Errorf(`unable to store new password due to %s and unable to return to previous password due -to %s, configure a new binddn and bindpass to restore openldap function`, pwdStoringErr, rollbackErr) - } - return nil, fmt.Errorf("unable to update password due to storage err: %s", pwdStoringErr) + return output, err } // Store updated role information From 30ce18530f6f4b03e375d8e6fb80ebc1cac48c7f Mon Sep 17 00:00:00 2001 From: Tom Proctor Date: Tue, 14 Sep 2021 18:11:29 +0100 Subject: [PATCH 10/23] Switch from warning to error to correct HTTP status code from 400 -> 500 --- path_rotate.go | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/path_rotate.go b/path_rotate.go index 633cf8a..c8e1329 100644 --- a/path_rotate.go +++ b/path_rotate.go @@ -162,10 +162,8 @@ func (b *backend) pathRotateRoleCredentialsUpdate(ctx context.Context, req *logi } if err != nil { - return &logical.Response{ - Warnings: []string{"unable to finish rotating credentials; retries will " + - "continue in the background but it is also safe to retry manually"}, - }, err + 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) } // We're not returning creds here because we do not know if its been processed From 2eff49f3cb40fe7cdd30c84c85326b60af4de37f Mon Sep 17 00:00:00 2001 From: Tom Proctor Date: Wed, 15 Sep 2021 13:02:47 +0100 Subject: [PATCH 11/23] * Delete WALs on failed role creation or role deletion * Take exclusive lock before reading config * Validate input before creating output struct * Comment updates --- path_rotate.go | 6 +++--- path_static_roles.go | 37 +++++++++++++++++++++++++++++++++---- rotation.go | 14 +++++++------- 3 files changed, 43 insertions(+), 14 deletions(-) diff --git a/path_rotate.go b/path_rotate.go index c8e1329..73035f0 100644 --- a/path_rotate.go +++ b/path_rotate.go @@ -24,12 +24,12 @@ func (b *backend) pathRotateCredentials() []*framework.Path { Fields: map[string]*framework.FieldSchema{}, Operations: map[logical.Operation]framework.OperationHandler{ logical.UpdateOperation: &framework.PathOperation{ - Callback: b.pathRotateCredentialsUpdate, + Callback: b.pathRotateRootCredentialsUpdate, ForwardPerformanceStandby: true, ForwardPerformanceSecondary: true, }, logical.CreateOperation: &framework.PathOperation{ - Callback: b.pathRotateCredentialsUpdate, + Callback: b.pathRotateRootCredentialsUpdate, ForwardPerformanceStandby: true, ForwardPerformanceSecondary: true, }, @@ -64,7 +64,7 @@ func (b *backend) pathRotateCredentials() []*framework.Path { } } -func (b *backend) pathRotateCredentialsUpdate(ctx context.Context, req *logical.Request, data *framework.FieldData) (*logical.Response, error) { +func (b *backend) pathRotateRootCredentialsUpdate(ctx context.Context, req *logical.Request, data *framework.FieldData) (*logical.Response, error) { if _, hasTimeout := ctx.Deadline(); !hasTimeout { var cancel func() ctx, cancel = context.WithTimeout(ctx, defaultCtxTimeout) diff --git a/path_static_roles.go b/path_static_roles.go index 68f1eea..137282c 100644 --- a/path_static_roles.go +++ b/path_static_roles.go @@ -5,6 +5,7 @@ import ( "fmt" "time" + "github.com/hashicorp/go-multierror" "github.com/hashicorp/vault/sdk/framework" "github.com/hashicorp/vault/sdk/helper/locksutil" "github.com/hashicorp/vault/sdk/logical" @@ -146,6 +147,27 @@ func (b *backend) pathStaticRoleDelete(ctx context.Context, req *logical.Request return nil, err } + walIDs, err := framework.ListWAL(ctx, req.Storage) + if err != nil { + return nil, err + } + for _, walID := range walIDs { + var merr *multierror.Error + 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() + } + return nil, nil } @@ -230,6 +252,15 @@ func (b *backend) pathStaticRoleCreateUpdate(ctx context.Context, req *logical.R Role: role, }) if err != nil { + 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() + } + } return nil, err } // guard against RotationTime not being set or zero-value @@ -248,11 +279,9 @@ func (b *backend) pathStaticRoleCreateUpdate(ctx context.Context, req *logical.R } // In case this is an update, remove any previous version of the item from - // the queue - + // the queue. The existing item could be tracking a WAL ID for this role, + // so it's important to keep the existing item rather than recreate it. //TODO: Add retry logic - // We could be tracking a WAL ID for this role, ensure we keep it when - // it's re-added to the queue. item, err = b.popFromRotationQueueByKey(name) if err != nil { return nil, err diff --git a/rotation.go b/rotation.go index 39cfb8a..02eddda 100644 --- a/rotation.go +++ b/rotation.go @@ -303,10 +303,8 @@ type setStaticAccountOutput struct { // tasks must be handled outside of this method. func (b *backend) setStaticAccountPassword(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") } if _, hasTimeout := ctx.Deadline(); !hasTimeout { @@ -315,6 +313,12 @@ func (b *backend) setStaticAccountPassword(ctx context.Context, s logical.Storag defer cancel() } + // Re-use WAL ID if present, otherwise PUT a new WAL + output := &setStaticAccountOutput{WALID: input.WALID} + + b.Lock() + defer b.Unlock() + config, err := readConfig(ctx, s) if err != nil { return output, err @@ -323,10 +327,6 @@ func (b *backend) setStaticAccountPassword(ctx context.Context, s logical.Storag return output, errors.New("the config is currently unset") } - // Take out the backend lock since we are swapping out the connection - b.Lock() - defer b.Unlock() - var newPassword string if output.WALID != "" { wal, err := b.findStaticWAL(ctx, s, output.WALID) From f89ce213b1fd959b2e5ef97a3a60ee9e06b8fb3a Mon Sep 17 00:00:00 2001 From: Tom Proctor Date: Wed, 15 Sep 2021 16:24:06 +0100 Subject: [PATCH 12/23] Fix a bug in role deletion WAL cleanup, add some WAL tests --- path_static_roles.go | 6 +- path_static_roles_test.go | 192 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 194 insertions(+), 4 deletions(-) diff --git a/path_static_roles.go b/path_static_roles.go index 137282c..f026deb 100644 --- a/path_static_roles.go +++ b/path_static_roles.go @@ -151,8 +151,8 @@ func (b *backend) pathStaticRoleDelete(ctx context.Context, req *logical.Request if err != nil { return nil, err } + var merr *multierror.Error for _, walID := range walIDs { - var merr *multierror.Error wal, err := b.findStaticWAL(ctx, req.Storage, walID) if err != nil { merr = multierror.Append(merr, err) @@ -164,11 +164,9 @@ func (b *backend) pathStaticRoleDelete(ctx context.Context, req *logical.Request merr = multierror.Append(merr, err) } } - - return nil, merr.ErrorOrNil() } - return nil, nil + return nil, merr.ErrorOrNil() } func (b *backend) pathStaticRoleRead(ctx context.Context, req *logical.Request, d *framework.FieldData) (*logical.Response, error) { diff --git a/path_static_roles_test.go b/path_static_roles_test.go index f6db655..95c4d22 100644 --- a/path_static_roles_test.go +++ b/path_static_roles_test.go @@ -2,6 +2,7 @@ package openldap import ( "context" + "fmt" "testing" "github.com/hashicorp/vault/sdk/logical" @@ -76,6 +77,15 @@ func TestRoles(t *testing.T) { if resp.Data["last_vault_rotation"] == nil { t.Fatal("expected last_vault_rotation to not be empty") } + + // Assert that we cleared the WAL ID from the queue's data in the happy path. + item, err := b.credRotationQueue.PopByKey("hashicorp") + if err != nil { + t.Fatal() + } + if item.Value != "" { + t.Fatal() + } }) t.Run("happy path with roles", func(t *testing.T) { b, storage := getBackend(false) @@ -452,3 +462,185 @@ func TestListRoles(t *testing.T) { } }) } + +func TestWALsStillTrackedAfterUpdate(t *testing.T) { + ctx := context.Background() + b, storage := getBackend(false) + defer b.Cleanup(ctx) + configureOpenLDAPMount(t, b, storage) + + _, err := b.HandleRequest(ctx, &logical.Request{ + Operation: logical.CreateOperation, + Path: staticRolePath + "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(err) + } + + generateWALFromFailedRotation(t, b, storage, "hashicorp") + requireWALs(t, storage, 1) + + _, err = b.HandleRequest(ctx, &logical.Request{ + Operation: logical.UpdateOperation, + Path: staticRolePath + "hashicorp", + Storage: storage, + Data: map[string]interface{}{ + "username": "hashicorp", + "dn": "uid=hashicorp,ou=users,dc=hashicorp,dc=com", + "rotation_period": "60s", + }, + }) + 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(true) + defer b.Cleanup(ctx) + configureOpenLDAPMount(t, b, storage) + + for i := 0; i < 3; i++ { + _, err := b.HandleRequest(ctx, &logical.Request{ + Operation: logical.CreateOperation, + Path: staticRolePath + "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(false) + defer b.Cleanup(ctx) + configureOpenLDAPMount(t, b, storage) + + // Create the roles + roleNames := []string{"hashicorp", "2"} + for _, roleName := range roleNames { + _, err := b.HandleRequest(ctx, &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": "5s", + }, + }) + if err != nil { + t.Fatal(err) + } + } + + // 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) + } + + walIDs, err := storage.List(context.Background(), "wal/") + if err != nil { + t.Fatal(err) + } + for _, walID := range walIDs { + fmt.Println(walID) + wal, err := storage.Get(ctx, "wal/"+walID) + if err != nil { + fmt.Println("error retrieving wal", walID, err) + } + fmt.Printf("%+v\n", string(wal.Value)) + } + + // 1 WAL should be cleared by the delete + requireWALs(t, storage, 1) +} + +func configureOpenLDAPMount(t *testing.T, b *backend, storage logical.Storage) { + t.Helper() + resp, err := b.HandleRequest(context.Background(), &logical.Request{ + Operation: logical.CreateOperation, + Path: configPath, + Storage: storage, + Data: map[string]interface{}{ + "binddn": "tester", + "bindpass": "pa$$w0rd", + "url": "ldap://138.91.247.105", + "certificate": validCertificate, + }, + }) + if err != nil || (resp != nil && resp.IsError()) { + t.Fatalf("err:%s resp:%#v\n", err, resp) + } +} + +func generateWALFromFailedRotation(t *testing.T, b *backend, 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, count int) { + t.Helper() + wals, err := storage.List(context.Background(), "wal/") + if err != nil { + t.Fatal(err) + } + if len(wals) != count { + t.Fatal("expected WALS", count, "got", len(wals)) + } + fmt.Println(wals) +} From 3cedec4e6c083ac48674c1fb929957aee0e36849 Mon Sep 17 00:00:00 2001 From: Tom Proctor Date: Wed, 15 Sep 2021 16:27:16 +0100 Subject: [PATCH 13/23] Delete test debug lines --- path_static_roles_test.go | 15 --------------- 1 file changed, 15 deletions(-) diff --git a/path_static_roles_test.go b/path_static_roles_test.go index 95c4d22..b955c74 100644 --- a/path_static_roles_test.go +++ b/path_static_roles_test.go @@ -2,7 +2,6 @@ package openldap import ( "context" - "fmt" "testing" "github.com/hashicorp/vault/sdk/logical" @@ -578,19 +577,6 @@ func TestWALsDeletedOnRoleDeletion(t *testing.T) { t.Fatal(err) } - walIDs, err := storage.List(context.Background(), "wal/") - if err != nil { - t.Fatal(err) - } - for _, walID := range walIDs { - fmt.Println(walID) - wal, err := storage.Get(ctx, "wal/"+walID) - if err != nil { - fmt.Println("error retrieving wal", walID, err) - } - fmt.Printf("%+v\n", string(wal.Value)) - } - // 1 WAL should be cleared by the delete requireWALs(t, storage, 1) } @@ -642,5 +628,4 @@ func requireWALs(t *testing.T, storage logical.Storage, count int) { if len(wals) != count { t.Fatal("expected WALS", count, "got", len(wals)) } - fmt.Println(wals) } From 7c7d98dccdfcfd16f10e47b061121b3d4250e1fa Mon Sep 17 00:00:00 2001 From: Tom Proctor Date: Thu, 16 Sep 2021 00:35:10 +0100 Subject: [PATCH 14/23] Fix manual rotate not respecting WAL ID, more unit testing --- backend_test.go | 2 +- path_rotate.go | 8 +++- path_static_roles_test.go | 31 ------------- rotation_test.go | 94 +++++++++++++++++++++++++++++++++++++++ 4 files changed, 101 insertions(+), 34 deletions(-) diff --git a/backend_test.go b/backend_test.go index a8f3a7d..2bae3c1 100644 --- a/backend_test.go +++ b/backend_test.go @@ -22,7 +22,7 @@ var ( func getBackend(throwsErr bool) (*backend, logical.Storage) { config := &logical.BackendConfig{ - Logger: logging.NewVaultLogger(log.Error), + Logger: logging.NewVaultLogger(log.Debug), System: &logical.StaticSystemView{ DefaultLeaseTTLVal: defaultLeaseTTLVal, diff --git a/path_rotate.go b/path_rotate.go index 73035f0..56660ac 100644 --- a/path_rotate.go +++ b/path_rotate.go @@ -133,10 +133,14 @@ func (b *backend) pathRotateRoleCredentialsUpdate(ctx context.Context, req *logi } } - resp, err := b.setStaticAccountPassword(ctx, req.Storage, &setStaticAccountInput{ + input := &setStaticAccountInput{ RoleName: name, Role: role, - }) + } + if walID, ok := item.Value.(string); ok { + input.WALID = walID + } + resp, err := b.setStaticAccountPassword(ctx, req.Storage, input) if err != nil { b.Logger().Warn("unable to rotate credentials in rotate-role", "error", err) // Update the priority to re-try this rotation and re-add the item to diff --git a/path_static_roles_test.go b/path_static_roles_test.go index b955c74..89aee95 100644 --- a/path_static_roles_test.go +++ b/path_static_roles_test.go @@ -598,34 +598,3 @@ func configureOpenLDAPMount(t *testing.T, b *backend, storage logical.Storage) { t.Fatalf("err:%s resp:%#v\n", err, resp) } } - -func generateWALFromFailedRotation(t *testing.T, b *backend, 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, count int) { - t.Helper() - wals, err := storage.List(context.Background(), "wal/") - if err != nil { - t.Fatal(err) - } - if len(wals) != count { - t.Fatal("expected WALS", count, "got", len(wals)) - } -} diff --git a/rotation_test.go b/rotation_test.go index a03121d..1fa1268 100644 --- a/rotation_test.go +++ b/rotation_test.go @@ -103,3 +103,97 @@ func TestAutoRotate(t *testing.T) { } }) } + +func TestRollsPasswordForwards(t *testing.T) { + ctx := context.Background() + b, storage := getBackend(false) + defer b.Cleanup(ctx) + configureOpenLDAPMount(t, b, storage) + + // Create the role + _, err := b.HandleRequest(ctx, &logical.Request{ + Operation: logical.CreateOperation, + Path: "static-role/hashicorp", + Storage: storage, + Data: map[string]interface{}{ + "username": "hashicorp", + "dn": "uid=hashicorp,ou=users,dc=hashicorp,dc=com", + "rotation_period": "86400s", + }, + }) + if err != nil { + t.Fatal(err) + } + + 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 != wal.OldPassword { + t.Fatal(role.StaticAccount.Password, wal.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 generateWALFromFailedRotation(t *testing.T, b *backend, 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)) + } +} From ad73e4da7452ab5484c68d78b42847f08fd7de24 Mon Sep 17 00:00:00 2001 From: Tom Proctor Date: Thu, 16 Sep 2021 13:08:38 +0100 Subject: [PATCH 15/23] Add tests for processing of stored WALs, remove bad assertion --- backend_test.go | 10 +- path_static_roles_test.go | 57 ++++-------- rotation_test.go | 190 +++++++++++++++++++++++++++++++++++--- 3 files changed, 201 insertions(+), 56 deletions(-) diff --git a/backend_test.go b/backend_test.go index 2bae3c1..f6feba5 100644 --- a/backend_test.go +++ b/backend_test.go @@ -42,7 +42,9 @@ func getBackend(throwsErr bool) (*backend, logical.Storage) { b.cancelQueue = cancel // Load queue and kickoff new periodic ticker - go b.initQueue(ictx, &logical.InitializationRequest{config.StorageView}) + b.initQueue(ictx, &logical.InitializationRequest{ + Storage: config.StorageView, + }) return b, config.StorageView } @@ -66,7 +68,7 @@ func (f *fakeLdapClient) Get(_ *client.Config, _ string) (*client.Entry, error) return client.NewEntry(entry), err } -func (f *fakeLdapClient) UpdatePassword(conf *client.Config, dn string, newPassword string) error { +func (f *fakeLdapClient) UpdatePassword(_ *client.Config, _ string, _ string) error { var err error if f.throwErrs { err = errors.New("forced error") @@ -74,7 +76,7 @@ func (f *fakeLdapClient) UpdatePassword(conf *client.Config, dn string, newPassw return err } -func (f *fakeLdapClient) UpdateRootPassword(conf *client.Config, newPassword string) error { +func (f *fakeLdapClient) UpdateRootPassword(_ *client.Config, _ string) error { var err error if f.throwErrs { err = errors.New("forced error") @@ -90,7 +92,7 @@ func (f *fakeLdapClient) Del(_ *client.Config, _ *ldap.DelRequest) error { return fmt.Errorf("not implemented") } -func (f *fakeLdapClient) Execute(conf *client.Config, entries []*ldif.Entry, continueOnError bool) (err error) { +func (f *fakeLdapClient) Execute(_ *client.Config, _ []*ldif.Entry, _ bool) (err error) { return fmt.Errorf("not implemented") } diff --git a/path_static_roles_test.go b/path_static_roles_test.go index 89aee95..735fef7 100644 --- a/path_static_roles_test.go +++ b/path_static_roles_test.go @@ -76,15 +76,6 @@ func TestRoles(t *testing.T) { if resp.Data["last_vault_rotation"] == nil { t.Fatal("expected last_vault_rotation to not be empty") } - - // Assert that we cleared the WAL ID from the queue's data in the happy path. - item, err := b.credRotationQueue.PopByKey("hashicorp") - if err != nil { - t.Fatal() - } - if item.Value != "" { - t.Fatal() - } }) t.Run("happy path with roles", func(t *testing.T) { b, storage := getBackend(false) @@ -468,31 +459,19 @@ func TestWALsStillTrackedAfterUpdate(t *testing.T) { defer b.Cleanup(ctx) configureOpenLDAPMount(t, b, storage) - _, err := b.HandleRequest(ctx, &logical.Request{ - Operation: logical.CreateOperation, - Path: staticRolePath + "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(err) - } + createRole(t, b, storage, "hashicorp") generateWALFromFailedRotation(t, b, storage, "hashicorp") requireWALs(t, storage, 1) - _, err = b.HandleRequest(ctx, &logical.Request{ + _, err := b.HandleRequest(ctx, &logical.Request{ Operation: logical.UpdateOperation, Path: staticRolePath + "hashicorp", Storage: storage, Data: map[string]interface{}{ "username": "hashicorp", "dn": "uid=hashicorp,ou=users,dc=hashicorp,dc=com", - "rotation_period": "60s", + "rotation_period": "600s", }, }) if err != nil { @@ -544,19 +523,7 @@ func TestWALsDeletedOnRoleDeletion(t *testing.T) { // Create the roles roleNames := []string{"hashicorp", "2"} for _, roleName := range roleNames { - _, err := b.HandleRequest(ctx, &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": "5s", - }, - }) - if err != nil { - t.Fatal(err) - } + createRole(t, b, storage, roleName) } // Fail to rotate the roles @@ -598,3 +565,19 @@ func configureOpenLDAPMount(t *testing.T, b *backend, storage logical.Storage) { t.Fatalf("err:%s resp:%#v\n", err, resp) } } + +func createRole(t *testing.T, b *backend, 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) + } +} diff --git a/rotation_test.go b/rotation_test.go index 1fa1268..e771a3b 100644 --- a/rotation_test.go +++ b/rotation_test.go @@ -5,7 +5,11 @@ import ( "testing" "time" + log "github.com/hashicorp/go-hclog" + "github.com/hashicorp/vault/sdk/framework" + "github.com/hashicorp/vault/sdk/helper/logging" "github.com/hashicorp/vault/sdk/logical" + "github.com/hashicorp/vault/sdk/queue" ) func TestAutoRotate(t *testing.T) { @@ -109,21 +113,7 @@ func TestRollsPasswordForwards(t *testing.T) { b, storage := getBackend(false) defer b.Cleanup(ctx) configureOpenLDAPMount(t, b, storage) - - // Create the role - _, err := b.HandleRequest(ctx, &logical.Request{ - Operation: logical.CreateOperation, - Path: "static-role/hashicorp", - Storage: storage, - Data: map[string]interface{}{ - "username": "hashicorp", - "dn": "uid=hashicorp,ou=users,dc=hashicorp,dc=com", - "rotation_period": "86400s", - }, - }) - if err != nil { - t.Fatal(err) - } + createRole(t, b, storage, "hashicorp") generateWALFromFailedRotation(t, b, storage, "hashicorp") walIDs, err := storage.List(context.Background(), "wal/") @@ -167,6 +157,176 @@ func TestRollsPasswordForwards(t *testing.T) { 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", + DN: "uid=hashicorp,ou=users,dc=hashicorp,dc=com", + NewPassword: walNewPassword, + LastVaultRotation: time.Now().Add(time.Hour), + }, + }, + { + "zero-time WAL is discarded on load", + false, + &setCredentialsWAL{ + RoleName: "hashicorp", + Username: "hashicorp", + DN: "uid=hashicorp,ou=users,dc=hashicorp,dc=com", + NewPassword: walNewPassword, + LastVaultRotation: time.Time{}, + }, + }, + { + "empty-password WAL is kept but a new password is generated", + true, + &setCredentialsWAL{ + RoleName: "hashicorp", + Username: "hashicorp", + DN: "uid=hashicorp,ou=users,dc=hashicorp,dc=com", + NewPassword: "", + LastVaultRotation: time.Now().Add(time.Hour), + }, + }, + } { + t.Run(tc.name, func(t *testing.T) { + ctx := context.Background() + config := &logical.BackendConfig{ + Logger: logging.NewVaultLogger(log.Debug), + + System: &logical.StaticSystemView{ + DefaultLeaseTTLVal: defaultLeaseTTLVal, + MaxLeaseTTLVal: maxLeaseTTLVal, + }, + 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) + configureOpenLDAPMount(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, &logical.InitializationRequest{ + Storage: config.StorageView, + }) + + 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(false) + defer b.Cleanup(ctx) + configureOpenLDAPMount(t, b, storage) + createRole(t, b, storage, "hashicorp") + + // Create 4 WALs, with a clear winner for most recent. + wal := &setCredentialsWAL{ + RoleName: "hashicorp", + Username: "hashicorp", + DN: "uid=hashicorp,ou=users,dc=hashicorp,dc=com", + 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 *backend, storage logical.Storage, roleName string) { t.Helper() // Fail to rotate the roles From 76f9f0340af79e46888cae68aaeb2f896de72954 Mon Sep 17 00:00:00 2001 From: Tom Proctor Date: Thu, 16 Sep 2021 15:40:41 +0100 Subject: [PATCH 16/23] Remove old password from WAL --- rotation.go | 3 --- rotation_test.go | 12 +++++++++--- 2 files changed, 9 insertions(+), 6 deletions(-) diff --git a/rotation.go b/rotation.go index 02eddda..ddcac2b 100644 --- a/rotation.go +++ b/rotation.go @@ -123,7 +123,6 @@ func (b *backend) runTicker(ctx context.Context, s logical.Storage) { // 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"` DN string `json:"dn"` @@ -261,7 +260,6 @@ func (b *backend) findStaticWAL(ctx context.Context, s logical.Storage, id strin walID: id, walCreatedAt: wal.CreatedAt, NewPassword: data["new_password"].(string), - OldPassword: data["old_password"].(string), RoleName: data["role_name"].(string), Username: data["username"].(string), DN: data["dn"].(string), @@ -363,7 +361,6 @@ func (b *backend) setStaticAccountPassword(ctx context.Context, s logical.Storag Username: input.Role.StaticAccount.Username, DN: input.Role.StaticAccount.DN, NewPassword: newPassword, - OldPassword: input.Role.StaticAccount.Password, LastVaultRotation: input.Role.StaticAccount.LastVaultRotation, }) if err != nil { diff --git a/rotation_test.go b/rotation_test.go index e771a3b..2f18e94 100644 --- a/rotation_test.go +++ b/rotation_test.go @@ -115,6 +115,12 @@ func TestRollsPasswordForwards(t *testing.T) { configureOpenLDAPMount(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 { @@ -127,13 +133,13 @@ func TestRollsPasswordForwards(t *testing.T) { if err != nil { t.Fatal(err) } - role, err := b.staticRole(ctx, storage, "hashicorp") + 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 != wal.OldPassword { - t.Fatal(role.StaticAccount.Password, wal.OldPassword) + if role.StaticAccount.Password != oldPassword { + t.Fatal(role.StaticAccount.Password, oldPassword) } // Trigger a retry on the rotation, it should use WAL's new password From 8dcd8405a86c13d535bcee2c7128c7731b5e810c Mon Sep 17 00:00:00 2001 From: Tom Proctor Date: Thu, 16 Sep 2021 15:42:56 +0100 Subject: [PATCH 17/23] Remove unnecessary multierror --- rotation.go | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/rotation.go b/rotation.go index ddcac2b..b3f8802 100644 --- a/rotation.go +++ b/rotation.go @@ -7,7 +7,6 @@ import ( "time" "github.com/hashicorp/errwrap" - "github.com/hashicorp/go-multierror" "github.com/hashicorp/go-secure-stdlib/base62" "github.com/hashicorp/vault/sdk/framework" "github.com/hashicorp/vault/sdk/helper/consts" @@ -300,7 +299,6 @@ type setStaticAccountOutput struct { // This method does not perform any operations on the priority queue. Those // tasks must be handled outside of this method. func (b *backend) setStaticAccountPassword(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") } @@ -391,14 +389,13 @@ func (b *backend) setStaticAccountPassword(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 } func (b *backend) GeneratePassword(ctx context.Context, cfg *config) (string, error) { From 4eac5a1ad476bd2eeeaa0698c9f655109d97e188 Mon Sep 17 00:00:00 2001 From: Tom Proctor Date: Thu, 16 Sep 2021 15:45:21 +0100 Subject: [PATCH 18/23] Distinguish between log lines --- rotation.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/rotation.go b/rotation.go index b3f8802..7ab8801 100644 --- a/rotation.go +++ b/rotation.go @@ -489,18 +489,18 @@ func (b *backend) loadStaticWALs(ctx context.Context, s logical.Storage) (map[st 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 cfbda980f3b7f8d689116e90435137375db6c89d Mon Sep 17 00:00:00 2001 From: Tom Proctor Date: Thu, 16 Sep 2021 16:41:36 +0100 Subject: [PATCH 19/23] Add missing debug logs for cleaning up WALs, tidy build.sh --- path_static_roles.go | 4 ++++ scripts/build.sh | 11 ++++------- 2 files changed, 8 insertions(+), 7 deletions(-) diff --git a/path_static_roles.go b/path_static_roles.go index f026deb..c28dc00 100644 --- a/path_static_roles.go +++ b/path_static_roles.go @@ -159,8 +159,10 @@ func (b *backend) pathStaticRoleDelete(ctx context.Context, req *logical.Request 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) merr = multierror.Append(merr, err) } } @@ -251,8 +253,10 @@ func (b *backend) pathStaticRoleCreateUpdate(ctx context.Context, req *logical.R }) 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) 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/scripts/build.sh b/scripts/build.sh index e8a8d1e..d93e852 100755 --- a/scripts/build.sh +++ b/scripts/build.sh @@ -21,10 +21,6 @@ SCRATCH="$DIR/tmp" mkdir -p "$SCRATCH/plugins" echo "--> Vault server" -echo " Writing config" -tee "$SCRATCH/vault.hcl" > /dev/null < Cleaning up" kill -INT "$VAULT_PID" + wait $VAULT_PID rm -rf "$SCRATCH" } trap cleanup EXIT From fa193fa768cd37c3e20206eea33ed57891189635 Mon Sep 17 00:00:00 2001 From: Tom Proctor Date: Thu, 16 Sep 2021 17:24:19 +0100 Subject: [PATCH 20/23] Add error to debug messages --- path_static_roles.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/path_static_roles.go b/path_static_roles.go index c28dc00..e1cf056 100644 --- a/path_static_roles.go +++ b/path_static_roles.go @@ -162,7 +162,7 @@ func (b *backend) pathStaticRoleDelete(ctx context.Context, req *logical.Request 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) + b.Logger().Debug("failed to delete WAL for deleted role", "WAL ID", walID, "error", err) merr = multierror.Append(merr, err) } } @@ -256,7 +256,7 @@ func (b *backend) pathStaticRoleCreateUpdate(ctx context.Context, req *logical.R 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) + 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)) From 78553e3263537eb168ef94f127deff5958381a42 Mon Sep 17 00:00:00 2001 From: Tom Proctor Date: Fri, 17 Sep 2021 14:36:37 +0100 Subject: [PATCH 21/23] Add last check on newPassword --- rotation.go | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/rotation.go b/rotation.go index 7ab8801..7cc5dc2 100644 --- a/rotation.go +++ b/rotation.go @@ -367,6 +367,14 @@ func (b *backend) setStaticAccountPassword(ctx context.Context, s logical.Storag b.Logger().Debug("writing WAL", "role", input.RoleName, "WAL ID", output.WALID) } + if newPassword == "" { + b.Logger().Error("newPassword was empty, re-generating based on the password policy") + newPassword, err = b.GeneratePassword(ctx, config) + if err != nil { + return output, err + } + } + // Update the password remotely. if err := b.client.UpdatePassword(config.LDAP, input.Role.StaticAccount.DN, newPassword); err != nil { return output, err From 70e4c7b5cd9d73775441dfe635aa1ec527e6eb88 Mon Sep 17 00:00:00 2001 From: Tom Proctor Date: Tue, 21 Sep 2021 13:53:53 +0100 Subject: [PATCH 22/23] Minor test improvements ported from DB PR --- path_static_roles_test.go | 31 +++++++++++++++++++++++++------ rotation_test.go | 22 ++++++++-------------- 2 files changed, 33 insertions(+), 20 deletions(-) diff --git a/path_static_roles_test.go b/path_static_roles_test.go index 735fef7..7e2437d 100644 --- a/path_static_roles_test.go +++ b/path_static_roles_test.go @@ -2,6 +2,7 @@ package openldap import ( "context" + "strings" "testing" "github.com/hashicorp/vault/sdk/logical" @@ -477,16 +478,31 @@ func TestWALsStillTrackedAfterUpdate(t *testing.T) { if err != nil { t.Fatal(err) } - requireWALs(t, storage, 1) + walIDs := requireWALs(t, storage, 1) - // Check we've still got track of it in the queue as well - item, err := b.credRotationQueue.PopByKey("hashicorp") + // 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) + } + _, err = b.HandleRequest(context.Background(), &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 wal, ok := item.Value.(string); !ok || wal == "" { - t.Fatal("should have a WAL ID in the rotation queue") + if role.StaticAccount.Password != wal.NewPassword { + t.Fatal() } + requireWALs(t, storage, 0) } func TestWALsDeletedOnRoleCreationFailed(t *testing.T) { @@ -496,7 +512,7 @@ func TestWALsDeletedOnRoleCreationFailed(t *testing.T) { configureOpenLDAPMount(t, b, storage) for i := 0; i < 3; i++ { - _, err := b.HandleRequest(ctx, &logical.Request{ + resp, err := b.HandleRequest(ctx, &logical.Request{ Operation: logical.CreateOperation, Path: staticRolePath + "hashicorp", Storage: storage, @@ -509,6 +525,9 @@ func TestWALsDeletedOnRoleCreationFailed(t *testing.T) { if err == nil { t.Fatal("expected error from OpenLDAP") } + if !strings.Contains(err.Error(), "forced error") { + t.Fatal("expected forced error message", resp, err) + } } requireWALs(t, storage, 0) diff --git a/rotation_test.go b/rotation_test.go index 2f18e94..9f08f27 100644 --- a/rotation_test.go +++ b/rotation_test.go @@ -108,7 +108,7 @@ func TestAutoRotate(t *testing.T) { }) } -func TestRollsPasswordForwards(t *testing.T) { +func TestRollsPasswordForwardsUsingWAL(t *testing.T) { ctx := context.Background() b, storage := getBackend(false) defer b.Cleanup(ctx) @@ -122,13 +122,7 @@ func TestRollsPasswordForwards(t *testing.T) { 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)) - } + walIDs := requireWALs(t, storage, 1) wal, err := b.findStaticWAL(ctx, storage, walIDs[0]) if err != nil { t.Fatal(err) @@ -236,11 +230,8 @@ func TestStoredWALsCorrectlyProcessed(t *testing.T) { // 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) - } + // 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(ictx, &logical.InitializationRequest{ @@ -353,7 +344,8 @@ func generateWALFromFailedRotation(t *testing.T, b *backend, storage logical.Sto } } -func requireWALs(t *testing.T, storage logical.Storage, expectedCount int) { +// 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 { @@ -362,4 +354,6 @@ func requireWALs(t *testing.T, storage logical.Storage, expectedCount int) { if len(wals) != expectedCount { t.Fatal("expected WALs", expectedCount, "got", len(wals)) } + + return wals } From b32b6859c277209de074a73212e2e98b07c910b6 Mon Sep 17 00:00:00 2001 From: Tom Proctor Date: Tue, 21 Sep 2021 18:10:04 +0100 Subject: [PATCH 23/23] Log line ordering --- rotation.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/rotation.go b/rotation.go index 7cc5dc2..c4189bf 100644 --- a/rotation.go +++ b/rotation.go @@ -354,6 +354,7 @@ func (b *backend) setStaticAccountPassword(ctx context.Context, s logical.Storag if err != nil { return output, err } + b.Logger().Debug("writing WAL", "role", input.RoleName, "WAL ID", output.WALID) output.WALID, err = framework.PutWAL(ctx, s, staticWALKey, &setCredentialsWAL{ RoleName: input.RoleName, Username: input.Role.StaticAccount.Username, @@ -364,7 +365,6 @@ func (b *backend) setStaticAccountPassword(ctx context.Context, s logical.Storag if err != nil { return output, errwrap.Wrapf("error writing WAL entry: {{err}}", err) } - b.Logger().Debug("writing WAL", "role", input.RoleName, "WAL ID", output.WALID) } if newPassword == "" {