db: consider possibility of NextVaultRotation being unset on queue population (VAULT-35639) (#30320)

* consider possibility of NextVaultRotation being nil on queue population

* move test

* add changelog

* fix reference to nil, and improve debug log

* use helper function to write static roles to storage

* add password check in test

* fix godoc

* fix changelog and add remediation debug line

* force ticker to run, and make sure credential doesnt rotate

* add another edge case

* fix godoc

* check ttl is less in test

* check error case and if resp is nil

* make check on ttl more robust

builtin/logical/database/path_roles.go

@@ -763,6 +763,14 @@ func (b *databaseBackend) pathStaticRoleCreateUpdate(ctx context.Context, req *l
 			Key: name,
 		}
 	case logical.UpdateOperation:
+		// if lastVaultRotation is zero, the role had `skip_import_rotation` set
+		if lastVaultRotation.IsZero() {
+			lastVaultRotation = time.Now()
+		}
+
+		// Ensure that NextVaultRotation is recalculated in case the rotation period changed
+		role.StaticAccount.SetNextVaultRotation(lastVaultRotation)
+
 		// store updated Role
 		err := b.StoreStaticRole(ctx, req.Storage, role)
 		if err != nil {

builtin/logical/database/path_roles_test.go

@@ -18,6 +18,7 @@ import (
 	"github.com/hashicorp/vault/sdk/logical"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/mock"
+	"github.com/stretchr/testify/require"
 )
 
 func TestBackend_Roles_CredentialTypes(t *testing.T) {
@@ -1352,6 +1353,57 @@ func TestIsInsideRotationWindow(t *testing.T) {
 	}
 }
 
+// TestStaticRoleTTLAfterUpdate tests that a static roles
+// TTL is properly updated after updating rotation period
+// This addresses a bug in which NextVaultRotation was not
+// set on update
+func TestStaticRoleTTLAfterUpdate(t *testing.T) {
+	ctx := context.Background()
+	b, storage, mockDB := getBackend(t)
+	defer b.Cleanup(ctx)
+	configureDBMount(t, storage)
+
+	roleName := "hashicorp"
+	data := map[string]interface{}{
+		"username":        "hashicorp",
+		"db_name":         "mockv5",
+		"rotation_period": "10m",
+	}
+
+	createRoleWithData(t, b, storage, mockDB, roleName, data)
+	// read credential
+	resp := readStaticCred(t, b, storage, mockDB, roleName)
+	var initialTTL float64
+	if v, ok := resp.Data["ttl"]; !ok || v == nil {
+		require.FailNow(t, "initial ttl should be set")
+	} else {
+		initialTTL, ok = v.(float64)
+		if !ok {
+			require.FailNow(t, "expected ttl to be an integer")
+		}
+	}
+
+	updateStaticRoleWithData(t, b, storage, mockDB, roleName, map[string]interface{}{
+		"username":        "hashicorp",
+		"db_name":         "mockv5",
+		"rotation_period": "20m",
+	})
+
+	resp = readStaticCred(t, b, storage, mockDB, roleName)
+	var updatedTTL float64
+	if v, ok := resp.Data["ttl"]; !ok || v == nil {
+		require.FailNow(t, "expected ttl to be set after update")
+	} else {
+		updatedTTL, ok = v.(float64)
+		if !ok {
+			require.FailNow(t, "expected ttl to be a float64 after update")
+		}
+	}
+
+	require.Greaterf(t, updatedTTL, initialTTL, "expected ttl to be greater than %f, actual value: %f",
+		initialTTL, updatedTTL)
+}
+
 func createRole(t *testing.T, b *databaseBackend, storage logical.Storage, mockDB *mockNewDatabase, roleName string) {
 	t.Helper()
 	mockDB.On("UpdateUser", mock.Anything, mock.Anything).
@@ -1404,6 +1456,31 @@ func readStaticCred(t *testing.T, b *databaseBackend, s logical.Storage, mockDB
 	return resp
 }
 
+func updateStaticRoleWithData(t *testing.T, b *databaseBackend, storage logical.Storage, mockDB *mockNewDatabase, roleName string, d map[string]interface{}) {
+	t.Helper()
+
+	mockDB.On("UpdateUser", mock.Anything, mock.Anything).
+		Return(v5.UpdateUserResponse{}, nil).
+		Once()
+
+	req := &logical.Request{
+		Operation: logical.UpdateOperation,
+		Path:      "static-roles/" + roleName,
+		Storage:   storage,
+		Data:      d,
+	}
+
+	resp, err := b.HandleRequest(context.Background(), req)
+	assert.NoError(t, err, "unexpected error")
+	if resp != nil {
+		assert.NoError(t, resp.Error(), "unexpected error in response")
+	}
+
+	if t.Failed() {
+		require.FailNow(t, "failed to update static role: %s", roleName)
+	}
+}
+
 const testRoleStaticCreate = `
 CREATE ROLE "{{name}}" WITH
   LOGIN

builtin/logical/database/rotation.go

@@ -64,6 +64,25 @@ func (b *databaseBackend) populateQueue(ctx context.Context, s logical.Storage)
 			continue
 		}
 
+		// If an account's NextVaultRotation period is zero time (time.Time{}), it means that the
+		// role was created before we added the `NextVaultRotation` field. In this
+		// case, we need to calculate the next rotation time based on the
+		// LastVaultRotation and the RotationPeriod. However, if the role was
+		// created with skip_import_rotation set, we need to use the current time
+		// instead of LastVaultRotation because LastVaultRotation is 0
+		if role.StaticAccount.NextVaultRotation.IsZero() {
+			log.Debug("NextVaultRotation unset (zero time). Role may predate field", roleName)
+			if role.StaticAccount.LastVaultRotation.IsZero() {
+				log.Debug("Setting NextVaultRotation based on current time", roleName)
+				role.StaticAccount.SetNextVaultRotation(time.Now())
+			} else {
+				log.Debug("Setting NextVaultRotation based on LastVaultRotation", roleName)
+				role.StaticAccount.SetNextVaultRotation(role.StaticAccount.LastVaultRotation)
+			}
+
+			b.StoreStaticRole(ctx, s, role)
+		}
+
 		item := queue.Item{
 			Key:      roleName,
 			Priority: role.StaticAccount.NextRotationTime().Unix(),

builtin/logical/database/rotation_test.go

@@ -1620,6 +1620,59 @@ func TestDeletesOlderWALsOnLoad(t *testing.T) {
 	requireWALs(t, storage, 1)
 }
 
+// TestStaticRoleNextVaultRotationOnRestart verifies that a static role created prior to Vault 1.15.0
+// (when roles were created without NextVaultRotation set) is automatically assigned a valid
+// NextVaultRotation when loaded from storage and the rotation queue is repopulated.
+func TestStaticRoleNextVaultRotationOnRestart(t *testing.T) {
+	ctx := context.Background()
+	b, storage, mockDB := getBackend(t)
+	defer b.Cleanup(ctx)
+	configureDBMount(t, storage)
+
+	roleName := "hashicorp"
+	data := map[string]interface{}{
+		"username":        "hashicorp",
+		"db_name":         "mockv5",
+		"rotation_period": "10m",
+	}
+
+	createRoleWithData(t, b, storage, mockDB, roleName, data)
+	item, err := b.credRotationQueue.Pop()
+	require.NoError(t, err)
+	firstPriority := item.Priority
+	role, err := b.StaticRole(context.Background(), storage, roleName)
+	firstPassword := role.StaticAccount.Password
+	require.NoError(t, err)
+
+	// force NextVaultRotation to zero to simulate roles before 1.15.0
+	role.StaticAccount.NextVaultRotation = time.Time{}
+	entry, err := logical.StorageEntryJSON(databaseStaticRolePath+roleName, role)
+	require.NoError(t, err)
+	if err := storage.Put(context.Background(), entry); err != nil {
+		t.Fatal("failed to write role to storage", err)
+	}
+
+	// Confirm that NextVaultRotation is nil
+	role, err = b.StaticRole(ctx, storage, roleName)
+	require.NoError(t, err)
+	require.Equal(t, role.StaticAccount.NextVaultRotation, time.Time{})
+
+	// Repopulate queue to simulate restart
+	b.populateQueue(ctx, storage)
+
+	success := b.rotateCredential(t.Context(), storage)
+	require.False(t, success, "expected rotation to fail")
+
+	role, err = b.StaticRole(ctx, storage, roleName)
+	require.NoError(t, err)
+	require.Equal(t, role.StaticAccount.Password, firstPassword)
+	item, err = b.credRotationQueue.Pop()
+	require.NoError(t, err)
+	newPriority := item.Priority
+	require.Equal(t, role.StaticAccount.NextVaultRotation.Unix(), newPriority) // Confirm NextVaultRotation and priority are equal
+	require.Equal(t, newPriority, firstPriority)                               // confirm that priority has not changed
+}
+
 func generateWALFromFailedRotation(t *testing.T, b *databaseBackend, storage logical.Storage, mockDB *mockNewDatabase, roleName string) {
 	t.Helper()
 	mockDB.On("UpdateUser", mock.Anything, mock.Anything).

changelog/30320.txt

@@ -0,0 +1,3 @@
+```release-note:bug
+database: Prevent static roles created in versions prior to 1.15.0 from rotating on backend restart.
+```