Fix early rotation for roles with WALs, ensure <=1 WAL per role (#28)

* Add more WAL logging
* Fix early rotation for roles with WALs, handle multiple WALs per role
* Respect previous WAL's new password
* Ensure only 1 WAL per role
* 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
* Switch from warning to error to correct HTTP status code from 400 -> 500
* Delete WALs on failed role creation or role deletion
* Take exclusive lock before reading config
* Fix manual rotate not respecting WAL ID
* Add tests for processing of stored WALs
* Remove unnecessary multierror
* Add last check on newPassword

backend_test.go

@@ -20,7 +20,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,
@@ -40,7 +40,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
 }
@@ -62,15 +64,15 @@ 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")
 	}
 	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")

path_roles.go

@@ -2,8 +2,10 @@ package openldap
 
 import (
 	"context"
+	"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"
@@ -145,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) {
@@ -220,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
@@ -228,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)
@@ -243,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
 	}
 

path_roles_test.go

@@ -2,6 +2,7 @@ package openldap
 
 import (
 	"context"
+	"strings"
 	"testing"
 
 	"github.com/hashicorp/vault/sdk/logical"
@@ -452,3 +453,150 @@ 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)
+
+	createRole(t, b, storage, "hashicorp")
+
+	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": "600s",
+		},
+	})
+	if err != nil {
+		t.Fatal(err)
+	}
+	walIDs := requireWALs(t, storage, 1)
+
+	// Now when we trigger a manual rotate, it should use the WAL's new password
+	// which will tell us that the in-memory structure still kept track of the
+	// WAL in addition to it still being in storage.
+	wal, err := b.findStaticWAL(ctx, storage, walIDs[0])
+	if err != nil {
+		t.Fatal(err)
+	}
+	_, 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 role.StaticAccount.Password != wal.NewPassword {
+		t.Fatal()
+	}
+	requireWALs(t, storage, 0)
+}
+
+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++ {
+		resp, 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")
+		}
+		if !strings.Contains(err.Error(), "forced error") {
+			t.Fatal("expected forced error message", resp, err)
+		}
+	}
+
+	requireWALs(t, storage, 0)
+}
+
+func TestWALsDeletedOnRoleDeletion(t *testing.T) {
+	ctx := context.Background()
+	b, storage := getBackend(false)
+	defer b.Cleanup(ctx)
+	configureOpenLDAPMount(t, b, storage)
+
+	// Create the roles
+	roleNames := []string{"hashicorp", "2"}
+	for _, roleName := range roleNames {
+		createRole(t, b, storage, roleName)
+	}
+
+	// Fail to rotate the roles
+	for _, roleName := range roleNames {
+		generateWALFromFailedRotation(t, b, storage, roleName)
+	}
+
+	// Should have 2 WALs hanging around
+	requireWALs(t, storage, 2)
+
+	// Delete one of the static roles
+	_, err := b.HandleRequest(ctx, &logical.Request{
+		Operation: logical.DeleteOperation,
+		Path:      "static-role/hashicorp",
+		Storage:   storage,
+	})
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	// 1 WAL should be cleared by the delete
+	requireWALs(t, storage, 1)
+}
+
+func 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 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)
+	}
+}

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,