Skip to content

Commit

Permalink
Review changes:
Browse files Browse the repository at this point in the history 
* 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
  • Loading branch information
@tomhjp
tomhjp committed Sep 14, 2021
1 parent 8910f05 commit 19f32ab
Show file tree
Hide file tree
Showing 3 changed files with 76 additions and 72 deletions.
15 changes: 10 additions & 5 deletions path_roles.go
View file
Expand Up @@ -220,6 +220,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
Expand All @@ -232,6 +233,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)
Expand All @@ -246,17 +250,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
}

Expand Down
11 changes: 10 additions & 1 deletion path_rotate.go
View file
Expand Up @@ -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
Expand All @@ -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,
Expand Down
122 changes: 56 additions & 66 deletions rotation.go
View file
Expand Up @@ -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)
}
Expand All @@ -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()
}
}

Expand Down Expand Up @@ -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,
Expand All @@ -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)
Expand All @@ -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() {
Expand Down Expand Up @@ -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
Expand All @@ -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 {
Expand All @@ -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 {
Expand All @@ -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
Expand Down

0 comments on commit 19f32ab

Please sign in to comment.