From 7239afb0f10b709349c01e47a97eba60ebd24b43 Mon Sep 17 00:00:00 2001 From: Calvin Leung Huang <1883212+calvn@users.noreply.github.com> Date: Tue, 21 Sep 2021 13:24:03 -0700 Subject: [PATCH 1/3] dep: update vault-plugin-secrets-openldap to v0.4.1 --- go.mod | 2 +- go.sum | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/go.mod b/go.mod index 5a972b80b756..8c6fcfdface6 100644 --- a/go.mod +++ b/go.mod @@ -99,7 +99,7 @@ require ( github.com/hashicorp/vault-plugin-secrets-gcpkms v0.8.0 github.com/hashicorp/vault-plugin-secrets-kv v0.8.0 github.com/hashicorp/vault-plugin-secrets-mongodbatlas v0.3.0 - github.com/hashicorp/vault-plugin-secrets-openldap v0.4.0 + github.com/hashicorp/vault-plugin-secrets-openldap v0.4.1 github.com/hashicorp/vault-plugin-secrets-terraform v0.1.0 github.com/hashicorp/vault/api v1.0.5-0.20210210214158-405eced08457 github.com/hashicorp/vault/sdk v0.2.1-0.20210823231532-aa9a6c2f8af5 diff --git a/go.sum b/go.sum index c86e8fc9c046..422944f85778 100644 --- a/go.sum +++ b/go.sum @@ -714,8 +714,8 @@ github.com/hashicorp/vault-plugin-secrets-kv v0.8.0 h1:9AWMN1+n4z6p/YX6d5/gaD5Qu github.com/hashicorp/vault-plugin-secrets-kv v0.8.0/go.mod h1:B/Cybh5aVF7LNAMHwVBxY8t7r2eL0C6HVGgTyP4nKK4= github.com/hashicorp/vault-plugin-secrets-mongodbatlas v0.3.0 h1:fge/+aIH0yrTPpfpkuCoQ8wV1gUcr3t8J9T5IFnbtZo= github.com/hashicorp/vault-plugin-secrets-mongodbatlas v0.3.0/go.mod h1:4mdgPqlkO+vfFX1cFAWcxkeqz6JAtZgKxL/67q/58Oo= -github.com/hashicorp/vault-plugin-secrets-openldap v0.4.0 h1:av7AhykZLA/lSQpxStGP+bGdNNuAEhAejZdBVrzw3p0= -github.com/hashicorp/vault-plugin-secrets-openldap v0.4.0/go.mod h1:GiFI8Bxwx3+fn0A3SyVp9XdYQhm3cOgN8GzwKxyJ9So= +github.com/hashicorp/vault-plugin-secrets-openldap v0.4.1 h1:BIHKR9Vusk3OrB9B89v8xL5k8klBVJDi2tvM9l5+1YE= +github.com/hashicorp/vault-plugin-secrets-openldap v0.4.1/go.mod h1:GiFI8Bxwx3+fn0A3SyVp9XdYQhm3cOgN8GzwKxyJ9So= github.com/hashicorp/vault-plugin-secrets-terraform v0.1.0 h1:g+r6TKJsD2aM0kUNWByuL4ffZTbZH/xO/sqDwTltOu0= github.com/hashicorp/vault-plugin-secrets-terraform v0.1.0/go.mod h1:7r/0t51X/ZtSRh/TjBk7gCm1CUMk50aqLAx811OsGQ8= github.com/hashicorp/vic v1.5.1-0.20190403131502-bbfe86ec9443 h1:O/pT5C1Q3mVXMyuqg7yuAWUg/jMZR1/0QTzTRdNR6Uw= From 7f6fed14b27ca0198de74a02e607bbddeb9cdcce Mon Sep 17 00:00:00 2001 From: Calvin Leung Huang <1883212+calvn@users.noreply.github.com> Date: Tue, 21 Sep 2021 14:02:53 -0700 Subject: [PATCH 2/3] add changelog entry --- changelog/12598.txt | 3 +++ 1 file changed, 3 insertions(+) create mode 100644 changelog/12598.txt diff --git a/changelog/12598.txt b/changelog/12598.txt new file mode 100644 index 000000000000..45d34e9a67e3 --- /dev/null +++ b/changelog/12598.txt @@ -0,0 +1,3 @@ +```release-note:bug +secrets/openldap: Fix bug where Vault can rotate static role passwords early during start up under certain conditions. [#28](https://github.com/hashicorp/vault-plugin-secrets-openldap/pull/28) +``` \ No newline at end of file From 0c5c772820d0809c80eafedb4f549e3b29b09922 Mon Sep 17 00:00:00 2001 From: Calvin Leung Huang <1883212+calvn@users.noreply.github.com> Date: Tue, 21 Sep 2021 14:11:51 -0700 Subject: [PATCH 3/3] go mod vendor and go mod tidy --- .../path_rotate.go | 23 ++- .../path_static_roles.go | 52 +++++- .../vault-plugin-secrets-openldap/rotation.go | 165 +++++++++++------- vendor/modules.txt | 2 +- 4 files changed, 167 insertions(+), 75 deletions(-) diff --git a/vendor/github.com/hashicorp/vault-plugin-secrets-openldap/path_rotate.go b/vendor/github.com/hashicorp/vault-plugin-secrets-openldap/path_rotate.go index 669feaee38d2..56660acc4c2e 100644 --- a/vendor/github.com/hashicorp/vault-plugin-secrets-openldap/path_rotate.go +++ b/vendor/github.com/hashicorp/vault-plugin-secrets-openldap/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) @@ -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 @@ -149,6 +153,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 +165,14 @@ func (b *backend) pathRotateRoleCredentialsUpdate(ctx context.Context, req *logi return nil, pushErr } + 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) + } + // 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/vendor/github.com/hashicorp/vault-plugin-secrets-openldap/path_static_roles.go b/vendor/github.com/hashicorp/vault-plugin-secrets-openldap/path_static_roles.go index 4ff93ff22c63..e1cf056dab47 100644 --- a/vendor/github.com/hashicorp/vault-plugin-secrets-openldap/path_static_roles.go +++ b/vendor/github.com/hashicorp/vault-plugin-secrets-openldap/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,7 +147,28 @@ func (b *backend) pathStaticRoleDelete(ctx context.Context, req *logical.Request return nil, err } - return nil, nil + walIDs, err := framework.ListWAL(ctx, req.Storage) + if err != nil { + return nil, err + } + var merr *multierror.Error + for _, walID := range walIDs { + wal, err := b.findStaticWAL(ctx, req.Storage, walID) + if err != nil { + merr = multierror.Append(merr, err) + continue + } + if wal != nil && name == wal.RoleName { + b.Logger().Debug("deleting WAL for deleted role", "WAL ID", walID, "role", name) + err = framework.DeleteWAL(ctx, req.Storage, walID) + if err != nil { + b.Logger().Debug("failed to delete WAL for deleted role", "WAL ID", walID, "error", err) + merr = multierror.Append(merr, err) + } + } + } + + return nil, merr.ErrorOrNil() } func (b *backend) pathStaticRoleRead(ctx context.Context, req *logical.Request, d *framework.FieldData) (*logical.Response, error) { @@ -221,6 +243,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 @@ -229,10 +252,24 @@ func (b *backend) pathStaticRoleCreateUpdate(ctx context.Context, req *logical.R Role: role, }) if err != nil { + if resp != nil && resp.WALID != "" { + b.Logger().Debug("deleting WAL for failed role creation", "WAL ID", resp.WALID, "role", name) + walDeleteErr := framework.DeleteWAL(ctx, req.Storage, resp.WALID) + if walDeleteErr != nil { + b.Logger().Debug("failed to delete WAL for failed role creation", "WAL ID", resp.WALID, "error", walDeleteErr) + var merr *multierror.Error + merr = multierror.Append(merr, err) + merr = multierror.Append(merr, fmt.Errorf("failed to clean up WAL from failed role creation: %w", walDeleteErr)) + err = merr.ErrorOrNil() + } + } return nil, err } // guard against RotationTime not being set or zero-value lvr = resp.RotationTime + item = &queue.Item{ + Key: name, + } case logical.UpdateOperation: // store updated Role entry, err := logical.StorageEntryJSON(staticRolePath+name, role) @@ -244,20 +281,19 @@ 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 - _, err = 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/vendor/github.com/hashicorp/vault-plugin-secrets-openldap/rotation.go b/vendor/github.com/hashicorp/vault-plugin-secrets-openldap/rotation.go index 3139f827ffc5..04268d01d3e6 100644 --- a/vendor/github.com/hashicorp/vault-plugin-secrets-openldap/rotation.go +++ b/vendor/github.com/hashicorp/vault-plugin-secrets-openldap/rotation.go @@ -7,7 +7,6 @@ import ( "time" "github.com/hashicorp/errwrap" - "github.com/hashicorp/go-multierror" "github.com/hashicorp/vault/sdk/framework" "github.com/hashicorp/vault/sdk/helper/base62" "github.com/hashicorp/vault/sdk/helper/consts" @@ -62,21 +61,34 @@ 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 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() } @@ -110,14 +122,15 @@ 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"` 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 @@ -191,18 +204,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) @@ -223,6 +225,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() { @@ -252,12 +256,12 @@ 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), + 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 { @@ -269,16 +273,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 @@ -298,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") } @@ -312,32 +312,54 @@ func (b *backend) setStaticAccountPassword(ctx context.Context, s logical.Storag // 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 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 + 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) + } - // Take out the backend lock since we are swapping out the connection - b.Lock() - defer b.Unlock() + 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 + } + 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, DN: input.Role.StaticAccount.DN, NewPassword: newPassword, - OldPassword: oldPassword, LastVaultRotation: input.Role.StaticAccount.LastVaultRotation, }) if err != nil { @@ -345,21 +367,17 @@ 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 + 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 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) + // Update the password remotely. + if err := b.client.UpdatePassword(config.LDAP, input.Role.StaticAccount.DN, newPassword); err != nil { + return output, err } // Store updated role information @@ -379,12 +397,13 @@ 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 { - merr = multierror.Append(merr, err) - return output, merr + b.Logger().Warn("error deleting WAL", "WAL ID", output.WALID, "error", err) + return output, err } + b.Logger().Debug("deleted WAL", "WAL ID", output.WALID) // The WAL has been deleted, return new setStaticAccountOutput without it - return &setStaticAccountOutput{RotationTime: lvr}, merr + return &setStaticAccountOutput{RotationTime: lvr}, nil } func (b *backend) GeneratePassword(ctx context.Context, cfg *config) (string, error) { @@ -463,13 +482,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) } continue } - walEntry.walID = walID + if existingWALEntry, exists := walMap[walEntry.RoleName]; exists { + b.Logger().Debug("multiple WALs detected for role", "role", walEntry.RoleName, + "loaded WAL ID", existingWALEntry.walID, "created at", existingWALEntry.walCreatedAt, "last vault rotation", existingWALEntry.LastVaultRotation, + "candidate WAL ID", walEntry.walID, "created at", walEntry.walCreatedAt, "last vault rotation", walEntry.LastVaultRotation) + + if walEntry.walCreatedAt > existingWALEntry.walCreatedAt { + // If the existing WAL is older, delete it from storage and fall + // through to inserting our current WAL into the map. + b.Logger().Debug("deleting stale loaded WAL", "WAL ID", existingWALEntry.walID) + err = framework.DeleteWAL(ctx, s, existingWALEntry.walID) + if err != nil { + b.Logger().Warn("unable to delete loaded WAL", "error", err, "WAL ID", existingWALEntry.walID) + } + } else { + // If we already have a more recent WAL entry in the map, delete + // this one and continue onto the next WAL. + b.Logger().Debug("deleting stale candidate WAL", "WAL ID", walEntry.walID) + err = framework.DeleteWAL(ctx, s, walID) + if err != nil { + b.Logger().Warn("unable to delete candidate WAL", "error", err, "WAL ID", walEntry.walID) + } + continue + } + } + + b.Logger().Debug("loaded WAL", "WAL ID", walID) walMap[walEntry.RoleName] = walEntry } return walMap, nil diff --git a/vendor/modules.txt b/vendor/modules.txt index 7c0842084769..36bfc3b0ff33 100644 --- a/vendor/modules.txt +++ b/vendor/modules.txt @@ -695,7 +695,7 @@ github.com/hashicorp/vault-plugin-secrets-kv # github.com/hashicorp/vault-plugin-secrets-mongodbatlas v0.3.0 ## explicit github.com/hashicorp/vault-plugin-secrets-mongodbatlas -# github.com/hashicorp/vault-plugin-secrets-openldap v0.4.0 +# github.com/hashicorp/vault-plugin-secrets-openldap v0.4.1 ## explicit github.com/hashicorp/vault-plugin-secrets-openldap github.com/hashicorp/vault-plugin-secrets-openldap/client