From 0cb178e430f3919d42c15dab1bca47d2832c81b8 Mon Sep 17 00:00:00 2001 From: JM Faircloth Date: Fri, 11 Aug 2023 14:12:44 -0500 Subject: [PATCH 1/2] adv ttl mgmt: add rotation_window field --- builtin/logical/database/path_roles.go | 43 ++++++++++++++++++--- builtin/logical/database/path_roles_test.go | 21 +++++++++- builtin/logical/database/rotation.go | 3 ++ 3 files changed, 59 insertions(+), 8 deletions(-) diff --git a/builtin/logical/database/path_roles.go b/builtin/logical/database/path_roles.go index 6beb34b40a1e..f9a164d9a78b 100644 --- a/builtin/logical/database/path_roles.go +++ b/builtin/logical/database/path_roles.go @@ -197,12 +197,18 @@ func staticFields() map[string]*framework.FieldSchema { Type: framework.TypeDurationSecond, Description: `Period for automatic credential rotation of the given username. Not valid unless used with - "username".`, + "username". Mutually exclusive with "rotation_schedule."`, }, "rotation_schedule": { Type: framework.TypeString, Description: `Schedule for automatic credential rotation of the - given username.`, + given username. Mutually exclusive with "rotation_period."`, + }, + "rotation_window": { + Type: framework.TypeDurationSecond, + Description: `The window of time in which rotations are allowed to + occur starting from a given "rotation_schedule". Requires "rotation_schedule" + to be specified`, }, "rotation_statements": { Type: framework.TypeStringSlice, @@ -299,11 +305,17 @@ func (b *databaseBackend) pathStaticRoleRead(ctx context.Context, req *logical.R if role.StaticAccount != nil { data["username"] = role.StaticAccount.Username data["rotation_statements"] = role.Statements.Rotation - data["rotation_period"] = role.StaticAccount.RotationPeriod.Seconds() if !role.StaticAccount.LastVaultRotation.IsZero() { data["last_vault_rotation"] = role.StaticAccount.LastVaultRotation } - data["rotation_schedule"] = role.StaticAccount.RotationSchedule + + // only return one of the mutually exclusive fields in the response + if role.StaticAccount.RotationPeriod != 0 { + data["rotation_period"] = role.StaticAccount.RotationPeriod.Seconds() + } else if role.StaticAccount.RotationSchedule != "" { + data["rotation_schedule"] = role.StaticAccount.RotationSchedule + data["rotation_window"] = role.StaticAccount.RotationWindow.Seconds() + } } if len(role.CredentialConfig) > 0 { @@ -547,6 +559,7 @@ func (b *databaseBackend) pathStaticRoleCreateUpdate(ctx context.Context, req *l rotationPeriodSecondsRaw, rotationPeriodOk := data.GetOk("rotation_period") rotationSchedule := data.Get("rotation_schedule").(string) rotationScheduleOk := rotationSchedule != "" + rotationWindowSecondsRaw, rotationWindowOk := data.GetOk("rotation_window") if rotationScheduleOk && rotationPeriodOk { return logical.ErrorResponse("mutually exclusive fields rotation_period and rotation_schedule were both specified; only one of them can be provided"), nil @@ -565,6 +578,9 @@ func (b *databaseBackend) pathStaticRoleCreateUpdate(ctx context.Context, req *l role.StaticAccount.RotationPeriod = time.Duration(rotationPeriodSeconds) * time.Second // Unset rotation schedule if rotation period is set role.StaticAccount.RotationSchedule = "" + if rotationWindowOk { + return logical.ErrorResponse("rotation_window is invalid with use of rotation_period"), nil + } } if rotationScheduleOk { @@ -581,6 +597,13 @@ func (b *databaseBackend) pathStaticRoleCreateUpdate(ctx context.Context, req *l role.StaticAccount.Schedule = *sched // Unset rotation period if rotation schedule is set role.StaticAccount.RotationPeriod = 0 + if rotationWindowOk { + rotationWindowSeconds := rotationWindowSecondsRaw.(int) + if rotationWindowSeconds < minRotationWindowSeconds { + return logical.ErrorResponse(fmt.Sprintf("rotation_window must be %d seconds or more", minRotationWindowSeconds)), nil + } + role.StaticAccount.RotationWindow = time.Duration(rotationWindowSeconds) * time.Second + } } if rotationStmtsRaw, ok := data.GetOk("rotation_statements"); ok { @@ -654,10 +677,12 @@ func (b *databaseBackend) pathStaticRoleCreateUpdate(ctx context.Context, req *l } } - if rotationPeriodOk { + _, rotationPeriodChanged := data.Raw["rotation_period"] + _, rotationScheduleChanged := data.Raw["rotation_schedule"] + if rotationPeriodChanged { b.logger.Debug("init priority for RotationPeriod", "lvr", lvr, "next", lvr.Add(role.StaticAccount.RotationPeriod)) item.Priority = lvr.Add(role.StaticAccount.RotationPeriod).Unix() - } else { + } else if rotationScheduleChanged { next := role.StaticAccount.Schedule.Next(lvr) b.logger.Debug("init priority for Schedule", "lvr", lvr) b.logger.Debug("init priority for Schedule", "next", next) @@ -783,6 +808,12 @@ type staticAccount struct { // day. RotationSchedule string `json:"rotation_schedule"` + // RotationWindow is number in seconds in which rotations are allowed to + // occur starting from a given rotation_schedule. + RotationWindow time.Duration `json:"rotation_window"` + + // Schedule holds the parsed "chron style" string representing the allowed + // schedule for each rotation. Schedule cron.SpecSchedule `json:"schedule"` // RevokeUser is a boolean flag to indicate if Vault should revoke the diff --git a/builtin/logical/database/path_roles_test.go b/builtin/logical/database/path_roles_test.go index f012df753546..8f4050f86b06 100644 --- a/builtin/logical/database/path_roles_test.go +++ b/builtin/logical/database/path_roles_test.go @@ -266,12 +266,29 @@ func TestBackend_StaticRole_Config(t *testing.T) { "rotation_period": float64(5400), }, }, - "missing rotation period": { + "missing required fields": { account: map[string]interface{}{ "username": dbUser, }, path: "plugin-role-test", - err: errors.New("rotation_period is required to create static accounts"), + err: errors.New("one of rotation_schedule or rotation_period must be provided to create a static account"), + }, + "rotation window invalid with rotation_period": { + account: map[string]interface{}{ + "username": dbUser, + "rotation_window": "3600s", + }, + path: "plugin-role-test", + err: errors.New("one of rotation_schedule or rotation_period must be provided to create a static account"), + }, + "rotation window invalid without rotation_schedule": { + account: map[string]interface{}{ + "username": dbUser, + "rotation_period": "5400s", + "rotation_window": "3600s", + }, + path: "disallowed-role", + err: errors.New("rotation_window is invalid with use of rotation_period"), }, "disallowed role config": { account: map[string]interface{}{ diff --git a/builtin/logical/database/rotation.go b/builtin/logical/database/rotation.go index 5e361959aec1..e64fce06366f 100644 --- a/builtin/logical/database/rotation.go +++ b/builtin/logical/database/rotation.go @@ -22,6 +22,9 @@ const ( // Default interval to check the queue for items needing rotation defaultQueueTickSeconds = 5 + // Minimum allowed value for rotation_window + minRotationWindowSeconds = 3600 + // Config key to set an alternate interval queueTickIntervalKey = "rotation_queue_tick_interval" From 1264817a7afc74dc5647f9d1089580b6fc00325b Mon Sep 17 00:00:00 2001 From: JM Faircloth Date: Fri, 11 Aug 2023 16:13:52 -0500 Subject: [PATCH 2/2] do some rotation_window validation and add unit tests --- builtin/logical/database/backend.go | 4 +- builtin/logical/database/path_roles.go | 7 ++- builtin/logical/database/path_roles_test.go | 66 ++++++++++++++++++--- 3 files changed, 66 insertions(+), 11 deletions(-) diff --git a/builtin/logical/database/backend.go b/builtin/logical/database/backend.go index 644d829348ba..2a4f3fcf5a9c 100644 --- a/builtin/logical/database/backend.go +++ b/builtin/logical/database/backend.go @@ -129,8 +129,10 @@ func Backend(conf *logical.BackendConfig) *databaseBackend { b.queueCtx, b.cancelQueueCtx = context.WithCancel(context.Background()) b.roleLocks = locksutil.CreateLocks() + // TODO(JM): don't allow seconds in production, this is helpful for + // development/testing though parser := cron.NewParser( - cron.Second | cron.Minute | cron.Hour | cron.Dom | cron.Month | cron.DowOptional | cron.Descriptor, + cron.Second | cron.Minute | cron.Hour | cron.Dom | cron.Month | cron.DowOptional, ) b.scheduleParser = parser return &b diff --git a/builtin/logical/database/path_roles.go b/builtin/logical/database/path_roles.go index f9a164d9a78b..fa5512708593 100644 --- a/builtin/logical/database/path_roles.go +++ b/builtin/logical/database/path_roles.go @@ -314,7 +314,10 @@ func (b *databaseBackend) pathStaticRoleRead(ctx context.Context, req *logical.R data["rotation_period"] = role.StaticAccount.RotationPeriod.Seconds() } else if role.StaticAccount.RotationSchedule != "" { data["rotation_schedule"] = role.StaticAccount.RotationSchedule - data["rotation_window"] = role.StaticAccount.RotationWindow.Seconds() + // rotation_window is only valid with rotation_schedule + if role.StaticAccount.RotationWindow != 0 { + data["rotation_window"] = role.StaticAccount.RotationWindow.Seconds() + } } } @@ -578,6 +581,7 @@ func (b *databaseBackend) pathStaticRoleCreateUpdate(ctx context.Context, req *l role.StaticAccount.RotationPeriod = time.Duration(rotationPeriodSeconds) * time.Second // Unset rotation schedule if rotation period is set role.StaticAccount.RotationSchedule = "" + if rotationWindowOk { return logical.ErrorResponse("rotation_window is invalid with use of rotation_period"), nil } @@ -597,6 +601,7 @@ func (b *databaseBackend) pathStaticRoleCreateUpdate(ctx context.Context, req *l role.StaticAccount.Schedule = *sched // Unset rotation period if rotation schedule is set role.StaticAccount.RotationPeriod = 0 + if rotationWindowOk { rotationWindowSeconds := rotationWindowSecondsRaw.(int) if rotationWindowSeconds < minRotationWindowSeconds { diff --git a/builtin/logical/database/path_roles_test.go b/builtin/logical/database/path_roles_test.go index 8f4050f86b06..26d420828c27 100644 --- a/builtin/logical/database/path_roles_test.go +++ b/builtin/logical/database/path_roles_test.go @@ -7,6 +7,7 @@ import ( "context" "encoding/json" "errors" + "fmt" "strings" "testing" "time" @@ -254,6 +255,8 @@ func TestBackend_StaticRole_Config(t *testing.T) { path string expected map[string]interface{} err error + // use this field to check partial error strings, otherwise use err + errContains string }{ "basic": { account: map[string]interface{}{ @@ -276,19 +279,52 @@ func TestBackend_StaticRole_Config(t *testing.T) { "rotation window invalid with rotation_period": { account: map[string]interface{}{ "username": dbUser, + "rotation_period": "5400s", "rotation_window": "3600s", }, + path: "disallowed-role", + err: errors.New("rotation_window is invalid with use of rotation_period"), + }, + "happy path for rotation_schedule": { + account: map[string]interface{}{ + "username": dbUser, + "rotation_schedule": "* * * * *", + }, path: "plugin-role-test", - err: errors.New("one of rotation_schedule or rotation_period must be provided to create a static account"), + expected: map[string]interface{}{ + "username": dbUser, + "rotation_schedule": "* * * * *", + }, }, - "rotation window invalid without rotation_schedule": { + "happy path for rotation_schedule and rotation_window": { account: map[string]interface{}{ - "username": dbUser, - "rotation_period": "5400s", - "rotation_window": "3600s", + "username": dbUser, + "rotation_schedule": "* * * * *", + "rotation_window": "3600s", }, - path: "disallowed-role", - err: errors.New("rotation_window is invalid with use of rotation_period"), + path: "plugin-role-test", + expected: map[string]interface{}{ + "username": dbUser, + "rotation_schedule": "* * * * *", + "rotation_window": float64(3600), + }, + }, + "error parsing rotation_schedule": { + account: map[string]interface{}{ + "username": dbUser, + "rotation_schedule": "foo", + }, + path: "plugin-role-test", + errContains: "could not parse rotation_schedule", + }, + "rotation_window invalid": { + account: map[string]interface{}{ + "username": dbUser, + "rotation_schedule": "* * * * *", + "rotation_window": "59s", + }, + path: "plugin-role-test", + err: errors.New(fmt.Sprintf("rotation_window must be %d seconds or more", minRotationWindowSeconds)), }, "disallowed role config": { account: map[string]interface{}{ @@ -322,7 +358,12 @@ func TestBackend_StaticRole_Config(t *testing.T) { } resp, err = b.HandleRequest(namespace.RootContext(nil), req) - if err != nil || (resp != nil && resp.IsError()) { + if tc.errContains != "" { + if !strings.Contains(resp.Error().Error(), tc.errContains) { + t.Fatalf("expected err message: (%s), got (%s), response error: (%s)", tc.err, err, resp.Error()) + } + return + } else if err != nil || (resp != nil && resp.IsError()) { if tc.err == nil { t.Fatalf("err:%s resp:%#v\n", err, resp) } @@ -358,7 +399,14 @@ func TestBackend_StaticRole_Config(t *testing.T) { expected := tc.expected actual := make(map[string]interface{}) - dataKeys := []string{"username", "password", "last_vault_rotation", "rotation_period"} + dataKeys := []string{ + "username", + "password", + "last_vault_rotation", + "rotation_period", + "rotation_schedule", + "rotation_window", + } for _, key := range dataKeys { if v, ok := resp.Data[key]; ok { actual[key] = v