From ededcaf05f6f1875b435c3c8c2fd472639ff36f7 Mon Sep 17 00:00:00 2001 From: JM Faircloth Date: Thu, 17 Aug 2023 14:05:56 -0500 Subject: [PATCH 1/5] consider rotation window ensure rotations only occur within a rotation window for schedule-based rotations --- builtin/logical/database/path_roles.go | 37 +++++++++++++++++++++ builtin/logical/database/path_roles_test.go | 19 +++++++++++ builtin/logical/database/rotation.go | 12 ++++--- 3 files changed, 63 insertions(+), 5 deletions(-) diff --git a/builtin/logical/database/path_roles.go b/builtin/logical/database/path_roles.go index 56e7ecea10ea..62349170b690 100644 --- a/builtin/logical/database/path_roles.go +++ b/builtin/logical/database/path_roles.go @@ -802,6 +802,10 @@ type staticAccount struct { // LastVaultRotation represents the last time Vault rotated the password LastVaultRotation time.Time `json:"last_vault_rotation"` + // NextVaultRotation represents the next time Vault is expected to rotate + // the password + NextVaultRotation time.Time `json:"next_vault_rotation"` + // RotationPeriod is number in seconds between each rotation, effectively a // "time to live". This value is compared to the LastVaultRotation to // determine if a password needs to be rotated @@ -860,6 +864,39 @@ func (s *staticAccount) UsesRotationPeriod() bool { return s.RotationPeriod != 0 && s.RotationSchedule == "" } +// IsInsideRotationWindow returns true if the current time t is within a given +// static account's rotation window. +// +// Returns true if the rotation window is not set. In this case, the rotation +// window is effectively the span of time between two consecutive rotation +// schedules and we should not prevent rotation. +func (s *staticAccount) IsInsideRotationWindow(t time.Time) bool { + if s.UsesRotationSchedule() && s.RotationWindow != 0 { + return t.Before(s.NextVaultRotation.Add(s.RotationWindow)) + } + return true +} + +// ShouldRotate returns true if a given static account should have its +// credentials rotated. +// +// This will return true when the priority is less than the current Unix time. +// If this static account is schedule-based with a rotation window, this method +// will return false if now is outside the rotation window. +func (s *staticAccount) ShouldRotate(priority int64) bool { + now := time.Now() + return priority < now.Unix() && s.IsInsideRotationWindow(now) +} + +// SetNextVaultRotation +func (s *staticAccount) SetNextVaultRotation(t time.Time) { + if s.UsesRotationPeriod() { + s.NextVaultRotation = t.Add(s.RotationPeriod) + } else { + s.NextVaultRotation = s.Schedule.Next(t) + } +} + // CredentialTTL calculates the approximate time remaining until the credential is // no longer valid. This is approximate because the rotation expiry is only // checked approximately every 5 seconds, and each rotation can take a small diff --git a/builtin/logical/database/path_roles_test.go b/builtin/logical/database/path_roles_test.go index b4f496fb1f97..ed820ecb6aa9 100644 --- a/builtin/logical/database/path_roles_test.go +++ b/builtin/logical/database/path_roles_test.go @@ -902,6 +902,25 @@ func TestWALsDeletedOnRoleDeletion(t *testing.T) { requireWALs(t, storage, 1) } +func TestIsInsideRotationWindow(t *testing.T) { + ctx := context.Background() + b, storage, mockDB := getBackend(t) + defer b.Cleanup(ctx) + configureDBMount(t, storage) + + data := map[string]interface{}{ + "username": "hashicorp", + "db_name": "mockv5", + "rotation_schedule": "0 0 */2 * * *", + "rotation_window": "1h", + } + createRoleWithData(t, b, storage, mockDB, "hashicorp", data) + role, err := b.StaticRole(ctx, storage, "hashicorp") + if err != nil { + t.Fatal(err) + } +} + func createRole(t *testing.T, b *databaseBackend, storage logical.Storage, mockDB *mockNewDatabase, roleName string) { t.Helper() mockDB.On("UpdateUser", mock.Anything, mock.Anything). diff --git a/builtin/logical/database/rotation.go b/builtin/logical/database/rotation.go index b063bff2c9ea..e12670f60980 100644 --- a/builtin/logical/database/rotation.go +++ b/builtin/logical/database/rotation.go @@ -223,11 +223,8 @@ func (b *databaseBackend) rotateCredential(ctx context.Context, s logical.Storag logger = logger.With("database", role.DBName) - // If "now" is less than the Item priority, then this item does not need to - // be rotated - // TODO(JM): ensure we don't process schedule-based rotations - // outside the rotation_window - if time.Now().Unix() < item.Priority { + if !role.StaticAccount.ShouldRotate(item.Priority) { + // do not rotate now, push item back onto queue to be rotated later if err := b.pushItem(item); err != nil { logger.Error("unable to push item on to queue", "error", err) } @@ -272,6 +269,10 @@ func (b *databaseBackend) rotateCredential(ctx context.Context, s logical.Storag if lvr.IsZero() { lvr = time.Now() } + role.StaticAccount.SetNextVaultRotation(lvr) + logger.Debug("update NextVaultRotation", "next", role.StaticAccount.NextVaultRotation) + + // TODO(JM): use helper method to set Priority instead of if/else? // Update priority and push updated Item to the queue if role.StaticAccount.UsesRotationSchedule() { @@ -509,6 +510,7 @@ func (b *databaseBackend) setStaticAccount(ctx context.Context, s logical.Storag // lvr is the known LastVaultRotation lvr := time.Now() input.Role.StaticAccount.LastVaultRotation = lvr + input.Role.StaticAccount.SetNextVaultRotation(lvr) output.RotationTime = lvr entry, err := logical.StorageEntryJSON(databaseStaticRolePath+input.RoleName, input.Role) From defdc6a1a48324837b6cc1642d80357de3307621 Mon Sep 17 00:00:00 2001 From: JM Faircloth Date: Fri, 18 Aug 2023 09:29:27 -0500 Subject: [PATCH 2/5] use helper method to set priority in rotateCredential --- builtin/logical/database/rotation.go | 18 ++---------------- 1 file changed, 2 insertions(+), 16 deletions(-) diff --git a/builtin/logical/database/rotation.go b/builtin/logical/database/rotation.go index e12670f60980..b751a28f5d94 100644 --- a/builtin/logical/database/rotation.go +++ b/builtin/logical/database/rotation.go @@ -269,22 +269,8 @@ func (b *databaseBackend) rotateCredential(ctx context.Context, s logical.Storag if lvr.IsZero() { lvr = time.Now() } - role.StaticAccount.SetNextVaultRotation(lvr) - logger.Debug("update NextVaultRotation", "next", role.StaticAccount.NextVaultRotation) - - // TODO(JM): use helper method to set Priority instead of if/else? - - // Update priority and push updated Item to the queue - if role.StaticAccount.UsesRotationSchedule() { - next := role.StaticAccount.Schedule.Next(lvr) - logger.Debug("update priority for Schedule", "next", next) - item.Priority = role.StaticAccount.Schedule.Next(lvr).Unix() - } else if role.StaticAccount.UsesRotationPeriod() { - logger.Debug("update priority for RotationPeriod", "lvr", lvr, "next", lvr.Add(role.StaticAccount.RotationPeriod)) - item.Priority = lvr.Add(role.StaticAccount.RotationPeriod).Unix() - } else { - logger.Error("rotation has not been properly configured") - } + + item.Priority = role.StaticAccount.NextRotationTimeFromInput(lvr).Unix() if err := b.pushItem(item); err != nil { logger.Warn("unable to push item on to queue", "error", err) From ea204682855acaafbea2be9bf8e89bb687e7db1f Mon Sep 17 00:00:00 2001 From: JM Faircloth Date: Fri, 18 Aug 2023 10:31:05 -0500 Subject: [PATCH 3/5] fix bug with priority check --- builtin/logical/database/path_roles.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/builtin/logical/database/path_roles.go b/builtin/logical/database/path_roles.go index 62349170b690..5366ac711c0c 100644 --- a/builtin/logical/database/path_roles.go +++ b/builtin/logical/database/path_roles.go @@ -880,12 +880,12 @@ func (s *staticAccount) IsInsideRotationWindow(t time.Time) bool { // ShouldRotate returns true if a given static account should have its // credentials rotated. // -// This will return true when the priority is less than the current Unix time. -// If this static account is schedule-based with a rotation window, this method -// will return false if now is outside the rotation window. +// This will return true when the priority <= the current Unix time. If this +// static account is schedule-based with a rotation window, this method will +// return false if now is outside the rotation window. func (s *staticAccount) ShouldRotate(priority int64) bool { now := time.Now() - return priority < now.Unix() && s.IsInsideRotationWindow(now) + return priority <= now.Unix() && s.IsInsideRotationWindow(now) } // SetNextVaultRotation From d1b43073341a44aa0b0d7b6c8f9f779b9f09d6c5 Mon Sep 17 00:00:00 2001 From: JM Faircloth Date: Fri, 18 Aug 2023 14:44:44 -0500 Subject: [PATCH 4/5] remove test for now --- builtin/logical/database/path_roles_test.go | 19 ------------------- 1 file changed, 19 deletions(-) diff --git a/builtin/logical/database/path_roles_test.go b/builtin/logical/database/path_roles_test.go index ed820ecb6aa9..b4f496fb1f97 100644 --- a/builtin/logical/database/path_roles_test.go +++ b/builtin/logical/database/path_roles_test.go @@ -902,25 +902,6 @@ func TestWALsDeletedOnRoleDeletion(t *testing.T) { requireWALs(t, storage, 1) } -func TestIsInsideRotationWindow(t *testing.T) { - ctx := context.Background() - b, storage, mockDB := getBackend(t) - defer b.Cleanup(ctx) - configureDBMount(t, storage) - - data := map[string]interface{}{ - "username": "hashicorp", - "db_name": "mockv5", - "rotation_schedule": "0 0 */2 * * *", - "rotation_window": "1h", - } - createRoleWithData(t, b, storage, mockDB, "hashicorp", data) - role, err := b.StaticRole(ctx, storage, "hashicorp") - if err != nil { - t.Fatal(err) - } -} - func createRole(t *testing.T, b *databaseBackend, storage logical.Storage, mockDB *mockNewDatabase, roleName string) { t.Helper() mockDB.On("UpdateUser", mock.Anything, mock.Anything). From 48ec0a6dcfb344270ee57797b6e003764b625bcd Mon Sep 17 00:00:00 2001 From: JM Faircloth Date: Fri, 18 Aug 2023 16:07:37 -0500 Subject: [PATCH 5/5] add and remove comments --- builtin/logical/database/rotation.go | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/builtin/logical/database/rotation.go b/builtin/logical/database/rotation.go index b751a28f5d94..090678bcba22 100644 --- a/builtin/logical/database/rotation.go +++ b/builtin/logical/database/rotation.go @@ -96,9 +96,6 @@ func (b *databaseBackend) populateQueue(ctx context.Context, s logical.Storage) } else { // previous rotation attempt was interrupted, so we set the // Priority as highest to be processed immediately - - // TODO(JM): ensure we don't process schedule-based rotations - // outside the rotation_window log.Info("found WAL for role", "role", item.Key, "WAL ID", walEntry.walID) item.Value = walEntry.walID item.Priority = time.Now().Unix() @@ -270,6 +267,7 @@ func (b *databaseBackend) rotateCredential(ctx context.Context, s logical.Storag lvr = time.Now() } + // Update priority and push updated Item to the queue item.Priority = role.StaticAccount.NextRotationTimeFromInput(lvr).Unix() if err := b.pushItem(item); err != nil {