diff --git a/builtin/logical/database/path_roles.go b/builtin/logical/database/path_roles.go index dc3f1d1fc3..639de2662b 100644 --- a/builtin/logical/database/path_roles.go +++ b/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 { diff --git a/builtin/logical/database/path_roles_test.go b/builtin/logical/database/path_roles_test.go index d1fe247873..8283b95939 100644 --- a/builtin/logical/database/path_roles_test.go +++ b/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 diff --git a/builtin/logical/database/rotation.go b/builtin/logical/database/rotation.go index 804771b670..a8e0f813a0 100644 --- a/builtin/logical/database/rotation.go +++ b/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(), diff --git a/builtin/logical/database/rotation_test.go b/builtin/logical/database/rotation_test.go index e6d4104771..7f0c9fcbe9 100644 --- a/builtin/logical/database/rotation_test.go +++ b/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). diff --git a/changelog/30320.txt b/changelog/30320.txt new file mode 100644 index 0000000000..3ed6ac4048 --- /dev/null +++ b/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. +``` \ No newline at end of file