Port: Premature Rotation For autorotate (#12563) (#12605)

* port of ldap fix for early cred rotation

* some more porting

* another couple lines to port

* final commits before report

* remove deadlock

* needs testing

* updates

* Sync with OpenLDAP PR

* Update the update error handling for items not found in the queue

* WIP unit tests
* Need to configure DB mount correctly, with db type mockv5
* Need to find a way to inject errors into that mock db

* throw error on role creation failure

* do not swallow error on role creation

* comment out wip tests and add in a test for disallowed role

* Use newly generated password in WAL

Co-authored-by: Michael Golowka <72365+pcman312@users.noreply.github.com>

* return err on popFromRotationQueueByKey error; cleanup on setStaticAccount

* test: fix TestPlugin_lifecycle

* Uncomment and fix unit tests
* Use mock database plugin to inject errors
* Tidy test code to rely less on code internals where possible
* Some stronger test assertions

* Undo logging updates

* Add changelog

* Remove ticker and background threads from WAL tests

* Keep pre-existing API behaviour of allowing update static role to act as a create

* Switch test back to update operation

* Revert my revert, and fix some test bugs

* Fix TestBackend_StaticRole_LockRegression

* clean up defer on TestPlugin_lifecycle

* unwrap reqs on cleanup

* setStaticAccount: don't hold a write lock

* TestStoredWALsCorrectlyProcessed: set replication state to unknown

Co-authored-by: Tom Proctor <tomhjp@users.noreply.github.com>
Co-authored-by: Michael Golowka <72365+pcman312@users.noreply.github.com>
Co-authored-by: Calvin Leung Huang <1883212+calvn@users.noreply.github.com>

Co-authored-by: Hridoy Roy <roy@hashicorp.com>
Co-authored-by: Tom Proctor <tomhjp@users.noreply.github.com>
Co-authored-by: Michael Golowka <72365+pcman312@users.noreply.github.com>

builtin/logical/database/path_roles.go

@@ -6,6 +6,7 @@ import (
 	"strings"
 	"time"
 
+	"github.com/hashicorp/go-multierror"
 	v4 "github.com/hashicorp/vault/sdk/database/dbplugin"
 	"github.com/hashicorp/vault/sdk/framework"
 	"github.com/hashicorp/vault/sdk/helper/locksutil"
@@ -16,7 +17,7 @@ import (
 
 func pathListRoles(b *databaseBackend) []*framework.Path {
 	return []*framework.Path{
-		&framework.Path{
+		{
 			Pattern: "roles/?$",
 
 			Callbacks: map[logical.Operation]framework.OperationFunc{
@@ -26,7 +27,7 @@ func pathListRoles(b *databaseBackend) []*framework.Path {
 			HelpSynopsis:    pathRoleHelpSyn,
 			HelpDescription: pathRoleHelpDesc,
 		},
-		&framework.Path{
+		{
 			Pattern: "static-roles/?$",
 
 			Callbacks: map[logical.Operation]framework.OperationFunc{
@@ -41,7 +42,7 @@ func pathListRoles(b *databaseBackend) []*framework.Path {
 
 func pathRoles(b *databaseBackend) []*framework.Path {
 	return []*framework.Path{
-		&framework.Path{
+		{
 			Pattern:        "roles/" + framework.GenericNameRegex("name"),
 			Fields:         fieldsForType(databaseRolePath),
 			ExistenceCheck: b.pathRoleExistenceCheck,
@@ -56,7 +57,7 @@ func pathRoles(b *databaseBackend) []*framework.Path {
 			HelpDescription: pathRoleHelpDesc,
 		},
 
-		&framework.Path{
+		{
 			Pattern:        "static-roles/" + framework.GenericNameRegex("name"),
 			Fields:         fieldsForType(databaseStaticRolePath),
 			ExistenceCheck: b.pathStaticRoleExistenceCheck,
@@ -215,7 +216,28 @@ func (b *databaseBackend) pathStaticRoleDelete(ctx context.Context, req *logical
 		return nil, err
 	}
 
-	return nil, nil
+	walIDs, err := framework.ListWAL(ctx, req.Storage)
+	if err != nil {
+		return nil, err
+	}
+	var merr *multierror.Error
+	for _, walID := range walIDs {
+		wal, err := b.findStaticWAL(ctx, req.Storage, walID)
+		if err != nil {
+			merr = multierror.Append(merr, err)
+			continue
+		}
+		if wal != nil && name == wal.RoleName {
+			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 *databaseBackend) pathStaticRoleRead(ctx context.Context, req *logical.Request, d *framework.FieldData) (*logical.Response, error) {
@@ -482,19 +504,34 @@ func (b *databaseBackend) pathStaticRoleCreateUpdate(ctx context.Context, req *l
 
 	// Only call setStaticAccount if we're creating the role for the
 	// first time
+	var item *queue.Item
 	switch req.Operation {
 	case logical.CreateOperation:
 		// setStaticAccount calls Storage.Put and saves the role to storage
 		resp, err := b.setStaticAccount(ctx, req.Storage, &setStaticAccountInput{
-			RoleName:   name,
-			Role:       role,
-			CreateUser: createRole,
+			RoleName: name,
+			Role:     role,
 		})
 		if err != nil {
+			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(databaseStaticRolePath+name, role)
@@ -504,17 +541,16 @@ func (b *databaseBackend) pathStaticRoleCreateUpdate(ctx context.Context, req *l
 		if err := req.Storage.Put(ctx, entry); err != nil {
 			return nil, err
 		}
-
-		// In case this is an update, remove any previous version of the item from
-		// the queue
-		b.popFromRotationQueueByKey(name)
+		item, err = b.popFromRotationQueueByKey(name)
+		if err != nil {
+			return nil, err
+		}
 	}
 
+	item.Priority = lvr.Add(role.StaticAccount.RotationPeriod).Unix()
+
 	// Add their rotation to the queue
-	if err := b.pushItem(&queue.Item{
-		Key:      name,
-		Priority: lvr.Add(role.StaticAccount.RotationPeriod).Unix(),
-	}); err != nil {
+	if err := b.pushItem(item); err != nil {
 		return nil, err
 	}
 

builtin/logical/database/path_roles_test.go

@@ -3,13 +3,16 @@ package database
 import (
 	"context"
 	"errors"
+	"strings"
 	"testing"
 	"time"
 
 	"github.com/go-test/deep"
 	"github.com/hashicorp/vault/helper/namespace"
 	postgreshelper "github.com/hashicorp/vault/helper/testhelpers/postgresql"
+	v5 "github.com/hashicorp/vault/sdk/database/dbplugin/v5"
 	"github.com/hashicorp/vault/sdk/logical"
+	"github.com/stretchr/testify/mock"
 )
 
 var dataKeys = []string{"username", "password", "last_vault_rotation", "rotation_period"}
@@ -43,7 +46,7 @@ func TestBackend_StaticRole_Config(t *testing.T) {
 		"connection_url":    connURL,
 		"plugin_name":       "postgresql-database-plugin",
 		"verify_connection": false,
-		"allowed_roles":     []string{"*"},
+		"allowed_roles":     []string{"plugin-role-test"},
 		"name":              "plugin-test",
 	}
 	req := &logical.Request{
@@ -61,6 +64,7 @@ func TestBackend_StaticRole_Config(t *testing.T) {
 	// ordering, so each case cleans up by deleting the role
 	testCases := map[string]struct {
 		account  map[string]interface{}
+		path     string
 		expected map[string]interface{}
 		err      error
 	}{
@@ -69,6 +73,7 @@ func TestBackend_StaticRole_Config(t *testing.T) {
 				"username":        dbUser,
 				"rotation_period": "5400s",
 			},
+			path: "plugin-role-test",
 			expected: map[string]interface{}{
 				"username":        dbUser,
 				"rotation_period": float64(5400),
@@ -78,7 +83,16 @@ func TestBackend_StaticRole_Config(t *testing.T) {
 			account: map[string]interface{}{
 				"username": dbUser,
 			},
-			err: errors.New("rotation_period is required to create static accounts"),
+			path: "plugin-role-test",
+			err:  errors.New("rotation_period is required to create static accounts"),
+		},
+		"disallowed role config": {
+			account: map[string]interface{}{
+				"username":        dbUser,
+				"rotation_period": "5400s",
+			},
+			path: "disallowed-role",
+			err:  errors.New("\"disallowed-role\" is not an allowed role"),
 		},
 	}
 
@@ -94,9 +108,11 @@ func TestBackend_StaticRole_Config(t *testing.T) {
 				data[k] = v
 			}
 
+			path := "static-roles/" + tc.path
+
 			req := &logical.Request{
 				Operation: logical.CreateOperation,
-				Path:      "static-roles/plugin-role-test",
+				Path:      path,
 				Storage:   config.StorageView,
 				Data:      data,
 			}
@@ -510,6 +526,135 @@ func TestBackend_StaticRole_Role_name_check(t *testing.T) {
 	}
 }
 
+func TestWALsStillTrackedAfterUpdate(t *testing.T) {
+	ctx := context.Background()
+	b, storage, mockDB := getBackend(t)
+	defer b.Cleanup(ctx)
+	configureDBMount(t, storage)
+
+	createRole(t, b, storage, mockDB, "hashicorp")
+
+	generateWALFromFailedRotation(t, b, storage, mockDB, "hashicorp")
+	requireWALs(t, storage, 1)
+
+	resp, err := b.HandleRequest(ctx, &logical.Request{
+		Operation: logical.UpdateOperation,
+		Path:      "static-roles/hashicorp",
+		Storage:   storage,
+		Data: map[string]interface{}{
+			"username":        "hashicorp",
+			"dn":              "uid=hashicorp,ou=users,dc=hashicorp,dc=com",
+			"rotation_period": "600s",
+		},
+	})
+	if err != nil || (resp != nil && resp.IsError()) {
+		t.Fatal(resp, err)
+	}
+	walIDs := requireWALs(t, storage, 1)
+
+	// Now when we trigger a manual rotate, it should use the WAL's new password
+	// which will tell us that the in-memory structure still kept track of the
+	// WAL in addition to it still being in storage.
+	wal, err := b.findStaticWAL(ctx, storage, walIDs[0])
+	if err != nil {
+		t.Fatal(err)
+	}
+	rotateRole(t, b, storage, mockDB, "hashicorp")
+	role, err := b.StaticRole(ctx, storage, "hashicorp")
+	if err != nil {
+		t.Fatal(err)
+	}
+	if role.StaticAccount.Password != wal.NewPassword {
+		t.Fatal()
+	}
+	requireWALs(t, storage, 0)
+}
+
+func TestWALsDeletedOnRoleCreationFailed(t *testing.T) {
+	ctx := context.Background()
+	b, storage, mockDB := getBackend(t)
+	defer b.Cleanup(ctx)
+	configureDBMount(t, storage)
+
+	for i := 0; i < 3; i++ {
+		mockDB.On("UpdateUser", mock.Anything, mock.Anything).
+			Return(v5.UpdateUserResponse{}, errors.New("forced error")).
+			Once()
+		resp, err := b.HandleRequest(ctx, &logical.Request{
+			Operation: logical.CreateOperation,
+			Path:      "static-roles/hashicorp",
+			Storage:   storage,
+			Data: map[string]interface{}{
+				"username":        "hashicorp",
+				"db_name":         "mockv5",
+				"rotation_period": "5s",
+			},
+		})
+		if err == nil {
+			t.Fatal("expected error from DB")
+		}
+		if !strings.Contains(err.Error(), "forced error") {
+			t.Fatal("expected forced error message", resp, err)
+		}
+	}
+
+	requireWALs(t, storage, 0)
+}
+
+func TestWALsDeletedOnRoleDeletion(t *testing.T) {
+	ctx := context.Background()
+	b, storage, mockDB := getBackend(t)
+	defer b.Cleanup(ctx)
+	configureDBMount(t, storage)
+
+	// Create the roles
+	roleNames := []string{"hashicorp", "2"}
+	for _, roleName := range roleNames {
+		createRole(t, b, storage, mockDB, roleName)
+	}
+
+	// Fail to rotate the roles
+	for _, roleName := range roleNames {
+		generateWALFromFailedRotation(t, b, storage, mockDB, roleName)
+	}
+
+	// Should have 2 WALs hanging around
+	requireWALs(t, storage, 2)
+
+	// Delete one of the static roles
+	resp, err := b.HandleRequest(ctx, &logical.Request{
+		Operation: logical.DeleteOperation,
+		Path:      "static-roles/hashicorp",
+		Storage:   storage,
+	})
+	if err != nil || (resp != nil && resp.IsError()) {
+		t.Fatal(resp, err)
+	}
+
+	// 1 WAL should be cleared by the delete
+	requireWALs(t, storage, 1)
+}
+
+func createRole(t *testing.T, b *databaseBackend, storage logical.Storage, mockDB *mockNewDatabase, roleName string) {
+	t.Helper()
+	mockDB.On("UpdateUser", mock.Anything, mock.Anything).
+		Return(v5.UpdateUserResponse{}, nil).
+		Once()
+	resp, err := b.HandleRequest(context.Background(), &logical.Request{
+		Operation: logical.CreateOperation,
+		Path:      "static-roles/" + roleName,
+		Storage:   storage,
+		Data: map[string]interface{}{
+			"username":        roleName,
+			"db_name":         "mockv5",
+			"rotation_period": "86400s",
+		},
+	})
+	if err != nil || (resp != nil && resp.IsError()) {
+		t.Fatal(resp, err)
+	}
+}
+
 const testRoleStaticCreate = `
 CREATE ROLE "{{name}}" WITH
   LOGIN

builtin/logical/database/path_rotate_credentials.go

@@ -169,10 +169,14 @@ func (b *databaseBackend) pathRotateRoleCredentialsUpdate() framework.OperationF
 			}
 		}
 
-		resp, err := b.setStaticAccount(ctx, req.Storage, &setStaticAccountInput{
+		input := &setStaticAccountInput{
 			RoleName: name,
 			Role:     role,
-		})
+		}
+		if walID, ok := item.Value.(string); ok {
+			input.WALID = walID
+		}
+		resp, err := b.setStaticAccount(ctx, req.Storage, input)
 		// if err is not nil, we need to attempt to update the priority and place
 		// this item back on the queue. The err should still be returned at the end
 		// of this method.
@@ -188,15 +192,22 @@ func (b *databaseBackend) pathRotateRoleCredentialsUpdate() framework.OperationF
 			}
 		} else {
 			item.Priority = resp.RotationTime.Add(role.StaticAccount.RotationPeriod).Unix()
+			// Clear any stored WAL ID as we must have successfully deleted our WAL to get here.
+			item.Value = ""
 		}
 
 		// Add their rotation to the queue
 		if err := b.pushItem(item); err != nil {
 			return nil, err
 		}
 
+		if err != nil {
+			return nil, fmt.Errorf("unable to finish rotating credentials; retries will "+
+				"continue in the background but it is also safe to retry manually: %w", err)
+		}
+
 		// return any err from the setStaticAccount call
-		return nil, err
+		return nil, nil
 	}
 }