From 2d934c9d4eb73b65ad809af2ba1dacfda8ee6b96 Mon Sep 17 00:00:00 2001 From: JM Faircloth Date: Thu, 3 Aug 2023 11:02:36 -0500 Subject: [PATCH 01/21] add rotation_schedule field to db backend --- builtin/logical/database/path_roles.go | 41 ++++++++++++++++++++++---- go.mod | 1 + go.sum | 2 ++ 3 files changed, 39 insertions(+), 5 deletions(-) diff --git a/builtin/logical/database/path_roles.go b/builtin/logical/database/path_roles.go index 31708415af6b..8ffa5fe85014 100644 --- a/builtin/logical/database/path_roles.go +++ b/builtin/logical/database/path_roles.go @@ -17,6 +17,7 @@ import ( "github.com/hashicorp/vault/sdk/helper/locksutil" "github.com/hashicorp/vault/sdk/logical" "github.com/hashicorp/vault/sdk/queue" + "github.com/robfig/cron" ) func pathListRoles(b *databaseBackend) []*framework.Path { @@ -197,6 +198,11 @@ func staticFields() map[string]*framework.FieldSchema { Description: `Period for automatic credential rotation of the given username. Not valid unless used with "username".`, + }, + "rotation_schedule": { + Type: framework.TypeString, + Description: `Schedule for automatic credential rotation of the + given username.`, }, "rotation_statements": { Type: framework.TypeStringSlice, @@ -297,6 +303,7 @@ func (b *databaseBackend) pathStaticRoleRead(ctx context.Context, req *logical.R if !role.StaticAccount.LastVaultRotation.IsZero() { data["last_vault_rotation"] = role.StaticAccount.LastVaultRotation } + data["rotation_schedule"] = role.StaticAccount.RotationSchedule } if len(role.CredentialConfig) > 0 { @@ -480,6 +487,7 @@ func (b *databaseBackend) pathRoleCreateUpdate(ctx context.Context, req *logical } func (b *databaseBackend) pathStaticRoleCreateUpdate(ctx context.Context, req *logical.Request, data *framework.FieldData) (*logical.Response, error) { + response := &logical.Response{} name := data.Get("name").(string) if name == "" { return logical.ErrorResponse("empty role name attribute given"), nil @@ -536,12 +544,17 @@ func (b *databaseBackend) pathStaticRoleCreateUpdate(ctx context.Context, req *l } role.StaticAccount.Username = username - // If it's a Create operation, both username and rotation_period must be included - rotationPeriodSecondsRaw, ok := data.GetOk("rotation_period") - if !ok && createRole { - return logical.ErrorResponse("rotation_period is required to create static accounts"), nil + rotationPeriodSecondsRaw, rotationPeriodOk := data.GetOk("rotation_period") + rotationSchedule := data.Get("rotation_schedule").(string) + rotationScheduleOk := rotationSchedule != "" + + if rotationScheduleOk && rotationPeriodOk { + response.AddWarning("mutually exclusive fields rotation_period and rotation_schedule were both specified; rotation_period takes priority") + } else if createRole && (!rotationScheduleOk && !rotationPeriodOk) { + return logical.ErrorResponse("one of rotation_schedule or rotation_period must be provided to create a static account"), nil } - if ok { + + if rotationPeriodOk { rotationPeriodSeconds := rotationPeriodSecondsRaw.(int) if rotationPeriodSeconds < defaultQueueTickSeconds { // If rotation frequency is specified, and this is an update, the value @@ -551,6 +564,15 @@ func (b *databaseBackend) pathStaticRoleCreateUpdate(ctx context.Context, req *l } role.StaticAccount.RotationPeriod = time.Duration(rotationPeriodSeconds) * time.Second } + if rotationScheduleOk { + // we will use cront.Parse to validate the input but disregard the + // parsed result for now. + _, err := cron.Parse(rotationSchedule) + if err != nil { + return logical.ErrorResponse("could not parse rotation_schedule", "error", err), nil + } + role.StaticAccount.RotationSchedule = rotationSchedule + } if rotationStmtsRaw, ok := data.GetOk("rotation_statements"); ok { role.Statements.Rotation = rotationStmtsRaw.([]string) @@ -630,6 +652,9 @@ func (b *databaseBackend) pathStaticRoleCreateUpdate(ctx context.Context, req *l return nil, err } + if response.IsError() { + return response, nil + } return nil, nil } @@ -735,6 +760,12 @@ type staticAccount struct { // determine if a password needs to be rotated RotationPeriod time.Duration `json:"rotation_period"` + // RotationSchedule is a "chron style" string representing the allowed + // schedule for each rotation. + // e.g. "1 0 * * *" would rotate at one minute past midnight (00:01) every + // day. + RotationSchedule string `json:"rotation_scedule"` + // RevokeUser is a boolean flag to indicate if Vault should revoke the // database user when the role is deleted RevokeUserOnDelete bool `json:"revoke_user_on_delete"` diff --git a/go.mod b/go.mod index 4a9a71d5a4f0..951a5d891ea6 100644 --- a/go.mod +++ b/go.mod @@ -459,6 +459,7 @@ require ( github.com/prometheus/client_model v0.3.0 // indirect github.com/prometheus/procfs v0.8.0 // indirect github.com/renier/xmlrpc v0.0.0-20170708154548-ce4a1a486c03 // indirect + github.com/robfig/cron v1.2.0 // indirect github.com/rogpeppe/go-internal v1.10.0 // indirect github.com/sergi/go-diff v1.1.0 // indirect github.com/shopspring/decimal v1.3.1 // indirect diff --git a/go.sum b/go.sum index d1c5fb985f43..86fdfafeda2c 100644 --- a/go.sum +++ b/go.sum @@ -2574,6 +2574,8 @@ github.com/rcrowley/go-metrics v0.0.0-20200313005456-10cdbea86bc0/go.mod h1:bCqn github.com/remyoudompheng/bigfft v0.0.0-20200410134404-eec4a21b6bb0/go.mod h1:qqbHyh8v60DhA7CoWK5oRCqLrMHRGoxYCSS9EjAz6Eo= github.com/renier/xmlrpc v0.0.0-20170708154548-ce4a1a486c03 h1:Wdi9nwnhFNAlseAOekn6B5G/+GMtks9UKbvRU/CMM/o= github.com/renier/xmlrpc v0.0.0-20170708154548-ce4a1a486c03/go.mod h1:gRAiPF5C5Nd0eyyRdqIu9qTiFSoZzpTq727b5B8fkkU= +github.com/robfig/cron v1.2.0 h1:ZjScXvvxeQ63Dbyxy76Fj3AT3Ut0aKsyd2/tl3DTMuQ= +github.com/robfig/cron v1.2.0/go.mod h1:JGuDeoQd7Z6yL4zQhZ3OPEVHB7fL6Ka6skscFHfmt2k= github.com/rogpeppe/fastuuid v0.0.0-20150106093220-6724a57986af/go.mod h1:XWv6SoW27p1b0cqNHllgS5HIMJraePCO15w5zCzIWYg= github.com/rogpeppe/fastuuid v1.2.0/go.mod h1:jVj6XXZzXRy/MSR5jhDC/2q6DgLz+nrA6LYCDYWNEvQ= github.com/rogpeppe/go-internal v1.1.0/go.mod h1:M8bDsm7K2OlrFYOpmOWEs/qY81heoFRclV5y23lUDJ4= From 5511ed44104425fb646c783f6475200a2cdfaa9d Mon Sep 17 00:00:00 2001 From: JM Faircloth Date: Fri, 4 Aug 2023 11:01:20 -0500 Subject: [PATCH 02/21] add cron schedule field --- builtin/logical/database/path_roles.go | 17 +++++++++++++---- go.mod | 1 + go.sum | 2 ++ 3 files changed, 16 insertions(+), 4 deletions(-) diff --git a/builtin/logical/database/path_roles.go b/builtin/logical/database/path_roles.go index 8ffa5fe85014..9811870c25b4 100644 --- a/builtin/logical/database/path_roles.go +++ b/builtin/logical/database/path_roles.go @@ -17,7 +17,7 @@ import ( "github.com/hashicorp/vault/sdk/helper/locksutil" "github.com/hashicorp/vault/sdk/logical" "github.com/hashicorp/vault/sdk/queue" - "github.com/robfig/cron" + "github.com/robfig/cron/v3" ) func pathListRoles(b *databaseBackend) []*framework.Path { @@ -565,13 +565,13 @@ func (b *databaseBackend) pathStaticRoleCreateUpdate(ctx context.Context, req *l role.StaticAccount.RotationPeriod = time.Duration(rotationPeriodSeconds) * time.Second } if rotationScheduleOk { - // we will use cront.Parse to validate the input but disregard the - // parsed result for now. - _, err := cron.Parse(rotationSchedule) + // TODO(JM): validate this isn't less than defaultQueueTickSeconds? + schedule, err := cron.ParseStandard(rotationSchedule) if err != nil { return logical.ErrorResponse("could not parse rotation_schedule", "error", err), nil } role.StaticAccount.RotationSchedule = rotationSchedule + role.StaticAccount.schedule = &schedule } if rotationStmtsRaw, ok := data.GetOk("rotation_statements"); ok { @@ -760,17 +760,26 @@ type staticAccount struct { // determine if a password needs to be rotated RotationPeriod time.Duration `json:"rotation_period"` + // NextVaultRotation represents the next time Vault should rotate the password + NextVaultRotation time.Time `json:"next_vault_rotation"` + // RotationSchedule is a "chron style" string representing the allowed // schedule for each rotation. // e.g. "1 0 * * *" would rotate at one minute past midnight (00:01) every // day. RotationSchedule string `json:"rotation_scedule"` + schedule *cron.Schedule + // RevokeUser is a boolean flag to indicate if Vault should revoke the // database user when the role is deleted RevokeUserOnDelete bool `json:"revoke_user_on_delete"` } +func (s *staticAccount) Schedule() cron.Schedule { + return *s.schedule +} + // NextRotationTime calculates the next rotation by adding the Rotation Period // to the last known vault rotation func (s *staticAccount) NextRotationTime() time.Time { diff --git a/go.mod b/go.mod index 951a5d891ea6..6a1bd93f6d48 100644 --- a/go.mod +++ b/go.mod @@ -460,6 +460,7 @@ require ( github.com/prometheus/procfs v0.8.0 // indirect github.com/renier/xmlrpc v0.0.0-20170708154548-ce4a1a486c03 // indirect github.com/robfig/cron v1.2.0 // indirect + github.com/robfig/cron/v3 v3.0.0 // indirect github.com/rogpeppe/go-internal v1.10.0 // indirect github.com/sergi/go-diff v1.1.0 // indirect github.com/shopspring/decimal v1.3.1 // indirect diff --git a/go.sum b/go.sum index 86fdfafeda2c..93a1c7b53ff0 100644 --- a/go.sum +++ b/go.sum @@ -2576,6 +2576,8 @@ github.com/renier/xmlrpc v0.0.0-20170708154548-ce4a1a486c03 h1:Wdi9nwnhFNAlseAOe github.com/renier/xmlrpc v0.0.0-20170708154548-ce4a1a486c03/go.mod h1:gRAiPF5C5Nd0eyyRdqIu9qTiFSoZzpTq727b5B8fkkU= github.com/robfig/cron v1.2.0 h1:ZjScXvvxeQ63Dbyxy76Fj3AT3Ut0aKsyd2/tl3DTMuQ= github.com/robfig/cron v1.2.0/go.mod h1:JGuDeoQd7Z6yL4zQhZ3OPEVHB7fL6Ka6skscFHfmt2k= +github.com/robfig/cron/v3 v3.0.0 h1:kQ6Cb7aHOHTSzNVNEhmp8EcWKLb4CbiMW9h9VyIhO4E= +github.com/robfig/cron/v3 v3.0.0/go.mod h1:eQICP3HwyT7UooqI/z+Ov+PtYAWygg1TEWWzGIFLtro= github.com/rogpeppe/fastuuid v0.0.0-20150106093220-6724a57986af/go.mod h1:XWv6SoW27p1b0cqNHllgS5HIMJraePCO15w5zCzIWYg= github.com/rogpeppe/fastuuid v1.2.0/go.mod h1:jVj6XXZzXRy/MSR5jhDC/2q6DgLz+nrA6LYCDYWNEvQ= github.com/rogpeppe/go-internal v1.1.0/go.mod h1:M8bDsm7K2OlrFYOpmOWEs/qY81heoFRclV5y23lUDJ4= From 6b95acfbd8ae4a19311b680955c32cfb3aad00e9 Mon Sep 17 00:00:00 2001 From: JM Faircloth Date: Fri, 4 Aug 2023 15:15:40 -0500 Subject: [PATCH 03/21] use priority queue with scheduled rotation types --- builtin/logical/database/backend.go | 8 ++++++++ builtin/logical/database/path_roles.go | 12 ++++++++++-- builtin/logical/database/rotation.go | 17 +++++++++++++++-- 3 files changed, 33 insertions(+), 4 deletions(-) diff --git a/builtin/logical/database/backend.go b/builtin/logical/database/backend.go index 6836370933fc..644d829348ba 100644 --- a/builtin/logical/database/backend.go +++ b/builtin/logical/database/backend.go @@ -25,6 +25,7 @@ import ( "github.com/hashicorp/vault/sdk/helper/locksutil" "github.com/hashicorp/vault/sdk/logical" "github.com/hashicorp/vault/sdk/queue" + "github.com/robfig/cron/v3" ) const ( @@ -127,6 +128,11 @@ func Backend(conf *logical.BackendConfig) *databaseBackend { b.connections = syncmap.NewSyncMap[string, *dbPluginInstance]() b.queueCtx, b.cancelQueueCtx = context.WithCancel(context.Background()) b.roleLocks = locksutil.CreateLocks() + + parser := cron.NewParser( + cron.Second | cron.Minute | cron.Hour | cron.Dom | cron.Month | cron.DowOptional | cron.Descriptor, + ) + b.scheduleParser = parser return &b } @@ -176,6 +182,8 @@ type databaseBackend struct { // the running gauge collection process gaugeCollectionProcess *metricsutil.GaugeCollectionProcess gaugeCollectionProcessStop sync.Once + + scheduleParser cron.Parser } func (b *databaseBackend) DatabaseConfig(ctx context.Context, s logical.Storage, name string) (*DatabaseConfig, error) { diff --git a/builtin/logical/database/path_roles.go b/builtin/logical/database/path_roles.go index 9811870c25b4..27dafe9dfd6c 100644 --- a/builtin/logical/database/path_roles.go +++ b/builtin/logical/database/path_roles.go @@ -566,7 +566,7 @@ func (b *databaseBackend) pathStaticRoleCreateUpdate(ctx context.Context, req *l } if rotationScheduleOk { // TODO(JM): validate this isn't less than defaultQueueTickSeconds? - schedule, err := cron.ParseStandard(rotationSchedule) + schedule, err := b.scheduleParser.Parse(rotationSchedule) if err != nil { return logical.ErrorResponse("could not parse rotation_schedule", "error", err), nil } @@ -645,7 +645,15 @@ func (b *databaseBackend) pathStaticRoleCreateUpdate(ctx context.Context, req *l } } - item.Priority = lvr.Add(role.StaticAccount.RotationPeriod).Unix() + if rotationPeriodOk { + b.logger.Debug("init priority for RotationPeriod", "lvr", lvr, "next", lvr.Add(role.StaticAccount.RotationPeriod)) + item.Priority = lvr.Add(role.StaticAccount.RotationPeriod).Unix() + } else { + next := role.StaticAccount.Schedule().Next(lvr) + b.logger.Debug("init priority for Schedule", "lvr", lvr) + b.logger.Debug("init priority for Schedule", "next", next) + item.Priority = role.StaticAccount.Schedule().Next(lvr).Unix() + } // Add their rotation to the queue if err := b.pushItem(item); err != nil { diff --git a/builtin/logical/database/rotation.go b/builtin/logical/database/rotation.go index ec8209b80e6b..5e361959aec1 100644 --- a/builtin/logical/database/rotation.go +++ b/builtin/logical/database/rotation.go @@ -264,8 +264,21 @@ func (b *databaseBackend) rotateCredential(ctx context.Context, s logical.Storag } // Update priority and push updated Item to the queue - nextRotation := lvr.Add(role.StaticAccount.RotationPeriod) - item.Priority = nextRotation.Unix() + if role.StaticAccount.RotationSchedule != "" { + schedule, err := b.scheduleParser.Parse(role.StaticAccount.RotationSchedule) + if err != nil { + b.logger.Error("could not parse rotation_schedule", "error", err) + return true + } + next := schedule.Next(lvr) + b.logger.Debug("update priority for Schedule", "lvr", lvr) + b.logger.Debug("update priority for Schedule", "next", next) + item.Priority = schedule.Next(lvr).Unix() + } else { + b.logger.Debug("update priority for RotationPeriod", "lvr", lvr, "next", lvr.Add(role.StaticAccount.RotationPeriod)) + item.Priority = lvr.Add(role.StaticAccount.RotationPeriod).Unix() + } + if err := b.pushItem(item); err != nil { logger.Warn("unable to push item on to queue", "error", err) } From 417466c83bb48586ef21d869c6da5dad027dbfc5 Mon Sep 17 00:00:00 2001 From: JM Faircloth Date: Fri, 4 Aug 2023 15:45:45 -0500 Subject: [PATCH 04/21] allow marshalling of cron schedule type --- builtin/logical/database/path_roles.go | 19 ++++++++++--------- 1 file changed, 10 insertions(+), 9 deletions(-) diff --git a/builtin/logical/database/path_roles.go b/builtin/logical/database/path_roles.go index 27dafe9dfd6c..952ece2d9cba 100644 --- a/builtin/logical/database/path_roles.go +++ b/builtin/logical/database/path_roles.go @@ -564,6 +564,7 @@ func (b *databaseBackend) pathStaticRoleCreateUpdate(ctx context.Context, req *l } role.StaticAccount.RotationPeriod = time.Duration(rotationPeriodSeconds) * time.Second } + if rotationScheduleOk { // TODO(JM): validate this isn't less than defaultQueueTickSeconds? schedule, err := b.scheduleParser.Parse(rotationSchedule) @@ -571,7 +572,11 @@ func (b *databaseBackend) pathStaticRoleCreateUpdate(ctx context.Context, req *l return logical.ErrorResponse("could not parse rotation_schedule", "error", err), nil } role.StaticAccount.RotationSchedule = rotationSchedule - role.StaticAccount.schedule = &schedule + sched, ok := schedule.(*cron.SpecSchedule) + if !ok { + return logical.ErrorResponse("could not parse rotation_schedule"), nil + } + role.StaticAccount.Schedule = *sched } if rotationStmtsRaw, ok := data.GetOk("rotation_statements"); ok { @@ -649,10 +654,10 @@ func (b *databaseBackend) pathStaticRoleCreateUpdate(ctx context.Context, req *l b.logger.Debug("init priority for RotationPeriod", "lvr", lvr, "next", lvr.Add(role.StaticAccount.RotationPeriod)) item.Priority = lvr.Add(role.StaticAccount.RotationPeriod).Unix() } else { - next := role.StaticAccount.Schedule().Next(lvr) + next := role.StaticAccount.Schedule.Next(lvr) b.logger.Debug("init priority for Schedule", "lvr", lvr) b.logger.Debug("init priority for Schedule", "next", next) - item.Priority = role.StaticAccount.Schedule().Next(lvr).Unix() + item.Priority = role.StaticAccount.Schedule.Next(lvr).Unix() } // Add their rotation to the queue @@ -775,19 +780,15 @@ type staticAccount struct { // schedule for each rotation. // e.g. "1 0 * * *" would rotate at one minute past midnight (00:01) every // day. - RotationSchedule string `json:"rotation_scedule"` + RotationSchedule string `json:"rotation_schedule"` - schedule *cron.Schedule + Schedule cron.SpecSchedule `json:"schedule"` // RevokeUser is a boolean flag to indicate if Vault should revoke the // database user when the role is deleted RevokeUserOnDelete bool `json:"revoke_user_on_delete"` } -func (s *staticAccount) Schedule() cron.Schedule { - return *s.schedule -} - // NextRotationTime calculates the next rotation by adding the Rotation Period // to the last known vault rotation func (s *staticAccount) NextRotationTime() time.Time { From 4779495e42ae3f8a78ea8a44a8dc02af81217b64 Mon Sep 17 00:00:00 2001 From: JM Faircloth Date: Fri, 4 Aug 2023 16:08:02 -0500 Subject: [PATCH 05/21] return warning on use of mutually exclusive fields --- builtin/logical/database/path_roles.go | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/builtin/logical/database/path_roles.go b/builtin/logical/database/path_roles.go index 952ece2d9cba..9ece84ba3f8e 100644 --- a/builtin/logical/database/path_roles.go +++ b/builtin/logical/database/path_roles.go @@ -549,6 +549,7 @@ func (b *databaseBackend) pathStaticRoleCreateUpdate(ctx context.Context, req *l rotationScheduleOk := rotationSchedule != "" if rotationScheduleOk && rotationPeriodOk { + // TODO(JM): handle mutual exclusion response.AddWarning("mutually exclusive fields rotation_period and rotation_schedule were both specified; rotation_period takes priority") } else if createRole && (!rotationScheduleOk && !rotationPeriodOk) { return logical.ErrorResponse("one of rotation_schedule or rotation_period must be provided to create a static account"), nil @@ -665,10 +666,7 @@ func (b *databaseBackend) pathStaticRoleCreateUpdate(ctx context.Context, req *l return nil, err } - if response.IsError() { - return response, nil - } - return nil, nil + return response, nil } type roleEntry struct { From acb5ebb92eefe5cc0c4ae04fc8af42d7db452120 Mon Sep 17 00:00:00 2001 From: Milena Zlaticanin <60530402+Zlaticanin@users.noreply.github.com> Date: Fri, 11 Aug 2023 14:21:25 -0700 Subject: [PATCH 06/21] handle mutual exclusion of rotation fields (#22306) * handle mutual exclusion of rotation fields * fix import --- builtin/logical/database/path_roles.go | 7 +++++-- go.mod | 3 +-- go.sum | 6 ++---- 3 files changed, 8 insertions(+), 8 deletions(-) diff --git a/builtin/logical/database/path_roles.go b/builtin/logical/database/path_roles.go index 9ece84ba3f8e..6beb34b40a1e 100644 --- a/builtin/logical/database/path_roles.go +++ b/builtin/logical/database/path_roles.go @@ -549,8 +549,7 @@ func (b *databaseBackend) pathStaticRoleCreateUpdate(ctx context.Context, req *l rotationScheduleOk := rotationSchedule != "" if rotationScheduleOk && rotationPeriodOk { - // TODO(JM): handle mutual exclusion - response.AddWarning("mutually exclusive fields rotation_period and rotation_schedule were both specified; rotation_period takes priority") + return logical.ErrorResponse("mutually exclusive fields rotation_period and rotation_schedule were both specified; only one of them can be provided"), nil } else if createRole && (!rotationScheduleOk && !rotationPeriodOk) { return logical.ErrorResponse("one of rotation_schedule or rotation_period must be provided to create a static account"), nil } @@ -564,6 +563,8 @@ func (b *databaseBackend) pathStaticRoleCreateUpdate(ctx context.Context, req *l return logical.ErrorResponse(fmt.Sprintf("rotation_period must be %d seconds or more", defaultQueueTickSeconds)), nil } role.StaticAccount.RotationPeriod = time.Duration(rotationPeriodSeconds) * time.Second + // Unset rotation schedule if rotation period is set + role.StaticAccount.RotationSchedule = "" } if rotationScheduleOk { @@ -578,6 +579,8 @@ func (b *databaseBackend) pathStaticRoleCreateUpdate(ctx context.Context, req *l return logical.ErrorResponse("could not parse rotation_schedule"), nil } role.StaticAccount.Schedule = *sched + // Unset rotation period if rotation schedule is set + role.StaticAccount.RotationPeriod = 0 } if rotationStmtsRaw, ok := data.GetOk("rotation_statements"); ok { diff --git a/go.mod b/go.mod index 6a1bd93f6d48..5b733ca2fd1b 100644 --- a/go.mod +++ b/go.mod @@ -190,6 +190,7 @@ require ( github.com/prometheus/client_golang v1.14.0 github.com/prometheus/common v0.37.0 github.com/rboyer/safeio v0.2.1 + github.com/robfig/cron/v3 v3.0.1 github.com/ryanuber/columnize v2.1.0+incompatible github.com/ryanuber/go-glob v1.0.0 github.com/sasha-s/go-deadlock v0.2.0 @@ -459,8 +460,6 @@ require ( github.com/prometheus/client_model v0.3.0 // indirect github.com/prometheus/procfs v0.8.0 // indirect github.com/renier/xmlrpc v0.0.0-20170708154548-ce4a1a486c03 // indirect - github.com/robfig/cron v1.2.0 // indirect - github.com/robfig/cron/v3 v3.0.0 // indirect github.com/rogpeppe/go-internal v1.10.0 // indirect github.com/sergi/go-diff v1.1.0 // indirect github.com/shopspring/decimal v1.3.1 // indirect diff --git a/go.sum b/go.sum index 93a1c7b53ff0..fdb6dd7346a6 100644 --- a/go.sum +++ b/go.sum @@ -2574,10 +2574,8 @@ github.com/rcrowley/go-metrics v0.0.0-20200313005456-10cdbea86bc0/go.mod h1:bCqn github.com/remyoudompheng/bigfft v0.0.0-20200410134404-eec4a21b6bb0/go.mod h1:qqbHyh8v60DhA7CoWK5oRCqLrMHRGoxYCSS9EjAz6Eo= github.com/renier/xmlrpc v0.0.0-20170708154548-ce4a1a486c03 h1:Wdi9nwnhFNAlseAOekn6B5G/+GMtks9UKbvRU/CMM/o= github.com/renier/xmlrpc v0.0.0-20170708154548-ce4a1a486c03/go.mod h1:gRAiPF5C5Nd0eyyRdqIu9qTiFSoZzpTq727b5B8fkkU= -github.com/robfig/cron v1.2.0 h1:ZjScXvvxeQ63Dbyxy76Fj3AT3Ut0aKsyd2/tl3DTMuQ= -github.com/robfig/cron v1.2.0/go.mod h1:JGuDeoQd7Z6yL4zQhZ3OPEVHB7fL6Ka6skscFHfmt2k= -github.com/robfig/cron/v3 v3.0.0 h1:kQ6Cb7aHOHTSzNVNEhmp8EcWKLb4CbiMW9h9VyIhO4E= -github.com/robfig/cron/v3 v3.0.0/go.mod h1:eQICP3HwyT7UooqI/z+Ov+PtYAWygg1TEWWzGIFLtro= +github.com/robfig/cron/v3 v3.0.1 h1:WdRxkvbJztn8LMz/QEvLN5sBU+xKpSqwwUO1Pjr4qDs= +github.com/robfig/cron/v3 v3.0.1/go.mod h1:eQICP3HwyT7UooqI/z+Ov+PtYAWygg1TEWWzGIFLtro= github.com/rogpeppe/fastuuid v0.0.0-20150106093220-6724a57986af/go.mod h1:XWv6SoW27p1b0cqNHllgS5HIMJraePCO15w5zCzIWYg= github.com/rogpeppe/fastuuid v1.2.0/go.mod h1:jVj6XXZzXRy/MSR5jhDC/2q6DgLz+nrA6LYCDYWNEvQ= github.com/rogpeppe/go-internal v1.1.0/go.mod h1:M8bDsm7K2OlrFYOpmOWEs/qY81heoFRclV5y23lUDJ4= From 7ceac551648f7526c7fdaa7e211e35a2b6e46f9f Mon Sep 17 00:00:00 2001 From: John-Michael Faircloth Date: Fri, 11 Aug 2023 19:34:07 -0500 Subject: [PATCH 07/21] adv ttl mgmt: add rotation_window field (#22303) * adv ttl mgmt: add rotation_window field * do some rotation_window validation and add unit tests --- builtin/logical/database/backend.go | 4 +- builtin/logical/database/path_roles.go | 48 ++++++++++++-- builtin/logical/database/path_roles_test.go | 73 +++++++++++++++++++-- builtin/logical/database/rotation.go | 3 + 4 files changed, 117 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 6beb34b40a1e..fa5512708593 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,20 @@ 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 + // rotation_window is only valid with rotation_schedule + if role.StaticAccount.RotationWindow != 0 { + data["rotation_window"] = role.StaticAccount.RotationWindow.Seconds() + } + } } if len(role.CredentialConfig) > 0 { @@ -547,6 +562,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 +581,10 @@ 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 +601,14 @@ 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 +682,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 +813,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..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{}{ @@ -266,12 +269,62 @@ 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_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", + expected: map[string]interface{}{ + "username": dbUser, + "rotation_schedule": "* * * * *", + }, + }, + "happy path for rotation_schedule and rotation_window": { + account: map[string]interface{}{ + "username": dbUser, + "rotation_schedule": "* * * * *", + "rotation_window": "3600s", + }, + 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{}{ @@ -305,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) } @@ -341,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 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 6aedaaf7c3f8e4c56985b8b68733f1fddb36ffa9 Mon Sep 17 00:00:00 2001 From: John-Michael Faircloth Date: Tue, 15 Aug 2023 17:05:32 -0500 Subject: [PATCH 08/21] adv ttl mgmt: Ensure initialization sets appropriate rotation schedule (#22341) * general cleanup and refactor rotation type checks * make NextRotationTime account for the rotation type * add comments --- builtin/logical/database/path_roles.go | 51 ++++++++++++++++++-------- builtin/logical/database/rotation.go | 27 ++++++++------ 2 files changed, 50 insertions(+), 28 deletions(-) diff --git a/builtin/logical/database/path_roles.go b/builtin/logical/database/path_roles.go index fa5512708593..54148e14fbe8 100644 --- a/builtin/logical/database/path_roles.go +++ b/builtin/logical/database/path_roles.go @@ -310,9 +310,9 @@ func (b *databaseBackend) pathStaticRoleRead(ctx context.Context, req *logical.R } // only return one of the mutually exclusive fields in the response - if role.StaticAccount.RotationPeriod != 0 { + if role.StaticAccount.UsesRotationPeriod() { data["rotation_period"] = role.StaticAccount.RotationPeriod.Seconds() - } else if role.StaticAccount.RotationSchedule != "" { + } else if role.StaticAccount.UsesRotationSchedule() { data["rotation_schedule"] = role.StaticAccount.RotationSchedule // rotation_window is only valid with rotation_schedule if role.StaticAccount.RotationWindow != 0 { @@ -579,16 +579,18 @@ func (b *databaseBackend) pathStaticRoleCreateUpdate(ctx context.Context, req *l return logical.ErrorResponse(fmt.Sprintf("rotation_period must be %d seconds or more", defaultQueueTickSeconds)), nil } 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 } + + // Unset rotation schedule and window if rotation period is set since + // these are mutually exclusive + role.StaticAccount.RotationSchedule = "" + role.StaticAccount.RotationWindow = 0 } if rotationScheduleOk { - // TODO(JM): validate this isn't less than defaultQueueTickSeconds? schedule, err := b.scheduleParser.Parse(rotationSchedule) if err != nil { return logical.ErrorResponse("could not parse rotation_schedule", "error", err), nil @@ -599,8 +601,6 @@ func (b *databaseBackend) pathStaticRoleCreateUpdate(ctx context.Context, req *l return logical.ErrorResponse("could not parse rotation_schedule"), nil } role.StaticAccount.Schedule = *sched - // Unset rotation period if rotation schedule is set - role.StaticAccount.RotationPeriod = 0 if rotationWindowOk { rotationWindowSeconds := rotationWindowSecondsRaw.(int) @@ -609,6 +609,10 @@ func (b *databaseBackend) pathStaticRoleCreateUpdate(ctx context.Context, req *l } role.StaticAccount.RotationWindow = time.Duration(rotationWindowSeconds) * time.Second } + + // Unset rotation period if rotation schedule is set since these are + // mutually exclusive + role.StaticAccount.RotationPeriod = 0 } if rotationStmtsRaw, ok := data.GetOk("rotation_statements"); ok { @@ -689,8 +693,7 @@ func (b *databaseBackend) pathStaticRoleCreateUpdate(ctx context.Context, req *l item.Priority = lvr.Add(role.StaticAccount.RotationPeriod).Unix() } 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) + b.logger.Debug("init priority for Schedule", "lvr", lvr, "next", next) item.Priority = role.StaticAccount.Schedule.Next(lvr).Unix() } @@ -804,9 +807,6 @@ type staticAccount struct { // determine if a password needs to be rotated RotationPeriod time.Duration `json:"rotation_period"` - // NextVaultRotation represents the next time Vault should rotate the password - NextVaultRotation time.Time `json:"next_vault_rotation"` - // RotationSchedule is a "chron style" string representing the allowed // schedule for each rotation. // e.g. "1 0 * * *" would rotate at one minute past midnight (00:01) every @@ -826,14 +826,33 @@ type staticAccount struct { RevokeUserOnDelete bool `json:"revoke_user_on_delete"` } -// NextRotationTime calculates the next rotation by adding the Rotation Period -// to the last known vault rotation +// NextRotationTime calculates the next rotation for period and schedule-based +// rotations. +// +// Period-based expiries are calculated by adding the Rotation Period to the +// last known vault rotation. Schedule-based expiries are calculated by +// querying for the next schedule expiry since the last known vault rotation. func (s *staticAccount) NextRotationTime() time.Time { - return s.LastVaultRotation.Add(s.RotationPeriod) + if s.UsesRotationPeriod() { + return s.LastVaultRotation.Add(s.RotationPeriod) + } + return s.Schedule.Next(s.LastVaultRotation) +} + +// UsesRotationSchedule returns true if the given static account has been +// configured to rotate credentials on a schedule (i.e. NOT on a rotation period). +func (s *staticAccount) UsesRotationSchedule() bool { + return s.RotationSchedule != "" && s.RotationPeriod == 0 +} + +// UsesRotationPeriod returns true if the given static account has been +// configured to rotate credentials on a period (i.e. NOT on a rotation schedule). +func (s *staticAccount) UsesRotationPeriod() bool { + return s.RotationPeriod != 0 && s.RotationSchedule == "" } // CredentialTTL calculates the approximate time remaining until the credential is -// no longer valid. This is approximate because the periodic rotation is only +// no longer valid. This is approximate because the rotation expiry is only // checked approximately every 5 seconds, and each rotation can take a small // amount of time to process. This can result in a negative TTL time while the // rotation function processes the Static Role and performs the rotation. If the diff --git a/builtin/logical/database/rotation.go b/builtin/logical/database/rotation.go index e64fce06366f..b063bff2c9ea 100644 --- a/builtin/logical/database/rotation.go +++ b/builtin/logical/database/rotation.go @@ -94,6 +94,11 @@ func (b *databaseBackend) populateQueue(ctx context.Context, s logical.Storage) log.Warn("unable to delete WAL", "error", err, "WAL ID", walEntry.walID) } } 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() @@ -220,6 +225,8 @@ func (b *databaseBackend) rotateCredential(ctx context.Context, s logical.Storag // 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 err := b.pushItem(item); err != nil { logger.Error("unable to push item on to queue", "error", err) @@ -267,19 +274,15 @@ func (b *databaseBackend) rotateCredential(ctx context.Context, s logical.Storag } // Update priority and push updated Item to the queue - if role.StaticAccount.RotationSchedule != "" { - schedule, err := b.scheduleParser.Parse(role.StaticAccount.RotationSchedule) - if err != nil { - b.logger.Error("could not parse rotation_schedule", "error", err) - return true - } - next := schedule.Next(lvr) - b.logger.Debug("update priority for Schedule", "lvr", lvr) - b.logger.Debug("update priority for Schedule", "next", next) - item.Priority = schedule.Next(lvr).Unix() - } else { - b.logger.Debug("update priority for RotationPeriod", "lvr", lvr, "next", lvr.Add(role.StaticAccount.RotationPeriod)) + 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") } if err := b.pushItem(item); err != nil { From 7a542fce4ac4fca52dc8dfd218958dc9c2414425 Mon Sep 17 00:00:00 2001 From: Milena Zlaticanin <60530402+Zlaticanin@users.noreply.github.com> Date: Wed, 16 Aug 2023 13:01:18 -0700 Subject: [PATCH 09/21] add unit tests to handle mutual exclusion (#22352) * add unit tests to handle mutual exclusion * revert rotation_test.go and add missing test case to path_roles_test.go --- builtin/logical/database/path_roles_test.go | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/builtin/logical/database/path_roles_test.go b/builtin/logical/database/path_roles_test.go index 26d420828c27..7dadf1d460ac 100644 --- a/builtin/logical/database/path_roles_test.go +++ b/builtin/logical/database/path_roles_test.go @@ -276,6 +276,15 @@ func TestBackend_StaticRole_Config(t *testing.T) { path: "plugin-role-test", err: errors.New("one of rotation_schedule or rotation_period must be provided to create a static account"), }, + "rotation_period with rotation_schedule": { + account: map[string]interface{}{ + "username": dbUser, + "rotation_period": "5400s", + "rotation_schedule": "* * * * *", + }, + path: "plugin-role-test", + err: errors.New("mutually exclusive fields rotation_period and rotation_schedule were both specified; only one of them can be provided"), + }, "rotation window invalid with rotation_period": { account: map[string]interface{}{ "username": dbUser, From c2705528ac36a95798efc330db8aa68dcf89e8a6 Mon Sep 17 00:00:00 2001 From: John-Michael Faircloth Date: Wed, 16 Aug 2023 15:25:39 -0500 Subject: [PATCH 10/21] adv ttl mgmt: add tests for init queue (#22376) --- builtin/logical/database/path_roles_test.go | 16 +++++ builtin/logical/database/rotation_test.go | 66 ++++++++++++++++----- 2 files changed, 67 insertions(+), 15 deletions(-) diff --git a/builtin/logical/database/path_roles_test.go b/builtin/logical/database/path_roles_test.go index 7dadf1d460ac..b4f496fb1f97 100644 --- a/builtin/logical/database/path_roles_test.go +++ b/builtin/logical/database/path_roles_test.go @@ -922,6 +922,22 @@ func createRole(t *testing.T, b *databaseBackend, storage logical.Storage, mockD } } +func createRoleWithData(t *testing.T, b *databaseBackend, s logical.Storage, mockDB *mockNewDatabase, roleName string, data map[string]interface{}) { + 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: s, + Data: data, + }) + if err != nil || (resp != nil && resp.IsError()) { + t.Fatal(resp, err) + } +} + const testRoleStaticCreate = ` CREATE ROLE "{{name}}" WITH LOGIN diff --git a/builtin/logical/database/rotation_test.go b/builtin/logical/database/rotation_test.go index db4df16d2a27..2c17337c693c 100644 --- a/builtin/logical/database/rotation_test.go +++ b/builtin/logical/database/rotation_test.go @@ -36,7 +36,7 @@ const ( dbUserDefaultPassword = "password" ) -func TestBackend_StaticRole_Rotate_basic(t *testing.T) { +func TestBackend_StaticRole_Rotation_basic(t *testing.T) { cluster, sys := getCluster(t) defer cluster.Cleanup() @@ -184,7 +184,7 @@ func TestBackend_StaticRole_Rotate_basic(t *testing.T) { // Sanity check to make sure we don't allow an attempt of rotating credentials // for non-static accounts, which doesn't make sense anyway, but doesn't hurt to // verify we return an error -func TestBackend_StaticRole_Rotate_NonStaticError(t *testing.T) { +func TestBackend_StaticRole_Rotation_NonStaticError(t *testing.T) { cluster, sys := getCluster(t) defer cluster.Cleanup() @@ -288,7 +288,7 @@ func TestBackend_StaticRole_Rotate_NonStaticError(t *testing.T) { } } -func TestBackend_StaticRole_Revoke_user(t *testing.T) { +func TestBackend_StaticRole_Rotation_Revoke_user(t *testing.T) { cluster, sys := getCluster(t) defer cluster.Cleanup() @@ -466,7 +466,7 @@ func verifyPgConn(t *testing.T, username, password, connURL string) { // WAL testing // // First scenario, WAL contains a role name that does not exist. -func TestBackend_Static_QueueWAL_discard_role_not_found(t *testing.T) { +func TestBackend_StaticRole_Rotation_QueueWAL_discard_role_not_found(t *testing.T) { cluster, sys := getCluster(t) defer cluster.Cleanup() @@ -507,7 +507,7 @@ func TestBackend_Static_QueueWAL_discard_role_not_found(t *testing.T) { // Second scenario, WAL contains a role name that does exist, but the role's // LastVaultRotation is greater than the WAL has -func TestBackend_Static_QueueWAL_discard_role_newer_rotation_date(t *testing.T) { +func TestBackend_StaticRole_Rotation_QueueWAL_discard_role_newer_rotation_date(t *testing.T) { cluster, sys := getCluster(t) defer cluster.Cleanup() @@ -695,7 +695,7 @@ func assertWALCount(t *testing.T, s logical.Storage, expected int, key string) { type userCreator func(t *testing.T, username, password string) -func TestBackend_StaticRole_Rotations_PostgreSQL(t *testing.T) { +func TestBackend_StaticRole_Rotation_PostgreSQL(t *testing.T) { cleanup, connURL := postgreshelper.PrepareTestContainer(t, "13.4-buster") defer cleanup() uc := userCreator(func(t *testing.T, username, password string) { @@ -707,7 +707,7 @@ func TestBackend_StaticRole_Rotations_PostgreSQL(t *testing.T) { }) } -func TestBackend_StaticRole_Rotations_MongoDB(t *testing.T) { +func TestBackend_StaticRole_Rotation_MongoDB(t *testing.T) { cleanup, connURL := mongodb.PrepareTestContainerWithDatabase(t, "5.0.10", "vaulttestdb") defer cleanup() @@ -720,7 +720,7 @@ func TestBackend_StaticRole_Rotations_MongoDB(t *testing.T) { }) } -func TestBackend_StaticRole_Rotations_MongoDBAtlas(t *testing.T) { +func TestBackend_StaticRole_Rotation_MongoDBAtlas(t *testing.T) { // To get the project ID, connect to cloud.mongodb.com, go to the vault-test project and // look at Project Settings. projID := os.Getenv("VAULT_MONGODBATLAS_PROJECT_ID") @@ -944,7 +944,7 @@ type createUserCommand struct { } // Demonstrates a bug fix for the credential rotation not releasing locks -func TestBackend_StaticRole_LockRegression(t *testing.T) { +func TestBackend_StaticRole_Rotation_LockRegression(t *testing.T) { cluster, sys := getCluster(t) defer cluster.Cleanup() @@ -1023,7 +1023,7 @@ func TestBackend_StaticRole_LockRegression(t *testing.T) { } } -func TestBackend_StaticRole_Rotate_Invalid_Role(t *testing.T) { +func TestBackend_StaticRole_Rotation_Invalid_Role(t *testing.T) { cluster, sys := getCluster(t) defer cluster.Cleanup() @@ -1160,10 +1160,18 @@ func TestRollsPasswordForwardsUsingWAL(t *testing.T) { func TestStoredWALsCorrectlyProcessed(t *testing.T) { const walNewPassword = "new-password-from-wal" + + rotationPeriodData := map[string]interface{}{ + "username": "hashicorp", + "db_name": "mockv5", + "rotation_period": "86400s", + } + for _, tc := range []struct { name string shouldRotate bool wal *setCredentialsWAL + data map[string]interface{} }{ { "WAL is kept and used for roll forward", @@ -1174,6 +1182,7 @@ func TestStoredWALsCorrectlyProcessed(t *testing.T) { NewPassword: walNewPassword, LastVaultRotation: time.Now().Add(time.Hour), }, + rotationPeriodData, }, { "zero-time WAL is discarded on load", @@ -1184,9 +1193,10 @@ func TestStoredWALsCorrectlyProcessed(t *testing.T) { NewPassword: walNewPassword, LastVaultRotation: time.Time{}, }, + rotationPeriodData, }, { - "empty-password WAL is kept but a new password is generated", + "rotation_period empty-password WAL is kept but a new password is generated", true, &setCredentialsWAL{ RoleName: "hashicorp", @@ -1194,6 +1204,22 @@ func TestStoredWALsCorrectlyProcessed(t *testing.T) { NewPassword: "", LastVaultRotation: time.Now().Add(time.Hour), }, + rotationPeriodData, + }, + { + "rotation_schedule empty-password WAL is kept but a new password is generated", + true, + &setCredentialsWAL{ + RoleName: "hashicorp", + Username: "hashicorp", + NewPassword: "", + LastVaultRotation: time.Now().Add(time.Hour), + }, + map[string]interface{}{ + "username": "hashicorp", + "db_name": "mockv5", + "rotation_schedule": "*/10 * * * * *", + }, }, } { t.Run(tc.name, func(t *testing.T) { @@ -1209,7 +1235,7 @@ func TestStoredWALsCorrectlyProcessed(t *testing.T) { } b.credRotationQueue = queue.New() configureDBMount(t, config.StorageView) - createRole(t, b, config.StorageView, mockDB, "hashicorp") + createRoleWithData(t, b, config.StorageView, mockDB, tc.wal.RoleName, tc.data) role, err := b.StaticRole(ctx, config.StorageView, "hashicorp") if err != nil { t.Fatal(err) @@ -1247,6 +1273,7 @@ func TestStoredWALsCorrectlyProcessed(t *testing.T) { t.Fatal(err) } + nextRotationTime := role.StaticAccount.NextRotationTime() if tc.shouldRotate { if tc.wal.NewPassword != "" { // Should use WAL's new_password field @@ -1262,11 +1289,11 @@ func TestStoredWALsCorrectlyProcessed(t *testing.T) { t.Fatal() } } + // Ensure the role was not promoted for early rotation + assertPriorityUnchanged(t, item.Priority, nextRotationTime) } else { // Ensure the role was not promoted for early rotation - if item.Priority < time.Now().Add(time.Hour).Unix() { - t.Fatal("priority should be for about a week away, but was", item.Priority) - } + assertPriorityUnchanged(t, item.Priority, nextRotationTime) if role.StaticAccount.Password != initialPassword { t.Fatal("password should not have been rotated yet") } @@ -1447,3 +1474,12 @@ func newBoolPtr(b bool) *bool { v := b return &v } + +// assertPriorityUnchanged is a helper to verify that the priority is the +// expected value for a given rotation time +func assertPriorityUnchanged(t *testing.T, priority int64, nextRotationTime time.Time) { + t.Helper() + if priority != nextRotationTime.Unix() { + t.Fatalf("expected next rotation at %s, but got %s", nextRotationTime, time.Unix(priority, 0).String()) + } +} From f23ff91a9fe2ea5295b9dff602cca480b280c863 Mon Sep 17 00:00:00 2001 From: Milena Zlaticanin <60530402+Zlaticanin@users.noreply.github.com> Date: Thu, 17 Aug 2023 12:37:37 -0700 Subject: [PATCH 11/21] Vault 18908/handle manual rotation (#22389) * support manual rotation for schedule based roles * update description and naming --- builtin/logical/database/path_roles.go | 9 +++++++++ builtin/logical/database/path_rotate_credentials.go | 2 +- 2 files changed, 10 insertions(+), 1 deletion(-) diff --git a/builtin/logical/database/path_roles.go b/builtin/logical/database/path_roles.go index 54148e14fbe8..56e7ecea10ea 100644 --- a/builtin/logical/database/path_roles.go +++ b/builtin/logical/database/path_roles.go @@ -839,6 +839,15 @@ func (s *staticAccount) NextRotationTime() time.Time { return s.Schedule.Next(s.LastVaultRotation) } +// NextRotationTimeFromInput calculates the next rotation time for period and +// schedule-based roles based on the input. +func (s *staticAccount) NextRotationTimeFromInput(input time.Time) time.Time { + if s.UsesRotationPeriod() { + return input.Add(s.RotationPeriod) + } + return s.Schedule.Next(input) +} + // UsesRotationSchedule returns true if the given static account has been // configured to rotate credentials on a schedule (i.e. NOT on a rotation period). func (s *staticAccount) UsesRotationSchedule() bool { diff --git a/builtin/logical/database/path_rotate_credentials.go b/builtin/logical/database/path_rotate_credentials.go index 72ce644b8926..f8ae66485e4d 100644 --- a/builtin/logical/database/path_rotate_credentials.go +++ b/builtin/logical/database/path_rotate_credentials.go @@ -224,7 +224,7 @@ func (b *databaseBackend) pathRotateRoleCredentialsUpdate() framework.OperationF item.Value = resp.WALID } } else { - item.Priority = resp.RotationTime.Add(role.StaticAccount.RotationPeriod).Unix() + item.Priority = role.StaticAccount.NextRotationTimeFromInput(resp.RotationTime).Unix() // Clear any stored WAL ID as we must have successfully deleted our WAL to get here. item.Value = "" } From 17946703f3c2e75c362027aff22b80ec00e0ec3e Mon Sep 17 00:00:00 2001 From: John-Michael Faircloth Date: Fri, 18 Aug 2023 16:17:01 -0500 Subject: [PATCH 12/21] adv ttl mgmt: consider rotation window (#22448) * consider rotation window ensure rotations only occur within a rotation window for schedule-based rotations * use helper method to set priority in rotateCredential * fix bug with priority check * remove test for now * add and remove comments --- builtin/logical/database/path_roles.go | 37 ++++++++++++++++++++++++++ builtin/logical/database/rotation.go | 22 +++------------ 2 files changed, 41 insertions(+), 18 deletions(-) diff --git a/builtin/logical/database/path_roles.go b/builtin/logical/database/path_roles.go index 56e7ecea10ea..5366ac711c0c 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 <= 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/rotation.go b/builtin/logical/database/rotation.go index b063bff2c9ea..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() @@ -223,11 +220,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) } @@ -274,16 +268,7 @@ func (b *databaseBackend) rotateCredential(ctx context.Context, s logical.Storag } // 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) @@ -509,6 +494,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 519eacf59865d252b785821b3d794905257334cc Mon Sep 17 00:00:00 2001 From: Milena Zlaticanin <60530402+Zlaticanin@users.noreply.github.com> Date: Fri, 18 Aug 2023 14:22:54 -0700 Subject: [PATCH 13/21] add unit tests for manual rotation (#22453) --- builtin/logical/database/rotation_test.go | 225 ++++++++++++++-------- 1 file changed, 142 insertions(+), 83 deletions(-) diff --git a/builtin/logical/database/rotation_test.go b/builtin/logical/database/rotation_test.go index 2c17337c693c..4e7b1dbfba77 100644 --- a/builtin/logical/database/rotation_test.go +++ b/builtin/logical/database/rotation_test.go @@ -82,103 +82,162 @@ func TestBackend_StaticRole_Rotation_basic(t *testing.T) { t.Fatalf("err:%s resp:%#v\n", err, resp) } - data = map[string]interface{}{ - "name": "plugin-role-test", - "db_name": "plugin-test", - "rotation_statements": testRoleStaticUpdate, - "username": dbUser, - "rotation_period": "5400s", + testCases := map[string]struct { + account map[string]interface{} + path string + expected map[string]interface{} + waitTime time.Duration + }{ + "basic with rotation_period": { + account: map[string]interface{}{ + "username": dbUser, + "rotation_period": "5400s", + }, + path: "plugin-role-test-1", + expected: map[string]interface{}{ + "username": dbUser, + "rotation_period": float64(5400), + }, + }, + "rotation_schedule is set and expires": { + account: map[string]interface{}{ + "username": dbUser, + "rotation_schedule": "*/10 * * * * *", + }, + path: "plugin-role-test-2", + expected: map[string]interface{}{ + "username": dbUser, + "rotation_schedule": "*/10 * * * * *", + }, + waitTime: 20 * time.Second, + }, } - req = &logical.Request{ - Operation: logical.CreateOperation, - Path: "static-roles/plugin-role-test", - Storage: config.StorageView, - Data: data, - } + for name, tc := range testCases { + t.Run(name, func(t *testing.T) { + data = map[string]interface{}{ + "name": "plugin-role-test", + "db_name": "plugin-test", + "rotation_statements": testRoleStaticUpdate, + "username": dbUser, + } - resp, err = b.HandleRequest(namespace.RootContext(nil), req) - if err != nil || (resp != nil && resp.IsError()) { - t.Fatalf("err:%s resp:%#v\n", err, resp) - } + for k, v := range tc.account { + data[k] = v + } - // Read the creds - data = map[string]interface{}{} - req = &logical.Request{ - Operation: logical.ReadOperation, - Path: "static-creds/plugin-role-test", - Storage: config.StorageView, - Data: data, - } + req = &logical.Request{ + Operation: logical.CreateOperation, + Path: "static-roles/" + tc.path, + Storage: config.StorageView, + Data: data, + } - resp, err = b.HandleRequest(namespace.RootContext(nil), req) - if err != nil || (resp != nil && resp.IsError()) { - t.Fatalf("err:%s resp:%#v\n", err, resp) - } + resp, err = b.HandleRequest(namespace.RootContext(nil), req) + if err != nil || (resp != nil && resp.IsError()) { + t.Fatalf("err:%s resp:%#v\n", err, resp) + } - username := resp.Data["username"].(string) - password := resp.Data["password"].(string) - if username == "" || password == "" { - t.Fatalf("empty username (%s) or password (%s)", username, password) - } + // Read the creds + data = map[string]interface{}{} + req = &logical.Request{ + Operation: logical.ReadOperation, + Path: "static-creds/" + tc.path, + Storage: config.StorageView, + Data: data, + } - // Verify username/password - verifyPgConn(t, dbUser, password, connURL) + resp, err = b.HandleRequest(namespace.RootContext(nil), req) + if err != nil || (resp != nil && resp.IsError()) { + t.Fatalf("err:%s resp:%#v\n", err, resp) + } - // Re-read the creds, verifying they aren't changing on read - data = map[string]interface{}{} - req = &logical.Request{ - Operation: logical.ReadOperation, - Path: "static-creds/plugin-role-test", - Storage: config.StorageView, - Data: data, - } - resp, err = b.HandleRequest(namespace.RootContext(nil), req) - if err != nil || (resp != nil && resp.IsError()) { - t.Fatalf("err:%s resp:%#v\n", err, resp) - } + username := resp.Data["username"].(string) + password := resp.Data["password"].(string) + if username == "" || password == "" { + t.Fatalf("empty username (%s) or password (%s)", username, password) + } - if username != resp.Data["username"].(string) || password != resp.Data["password"].(string) { - t.Fatal("expected re-read username/password to match, but didn't") - } + // Verify username/password + verifyPgConn(t, dbUser, password, connURL) - // Trigger rotation - data = map[string]interface{}{"name": "plugin-role-test"} - req = &logical.Request{ - Operation: logical.UpdateOperation, - Path: "rotate-role/plugin-role-test", - Storage: config.StorageView, - Data: data, - } - resp, err = b.HandleRequest(namespace.RootContext(nil), req) - if err != nil || (resp != nil && resp.IsError()) { - t.Fatalf("err:%s resp:%#v\n", err, resp) - } + // Re-read the creds, verifying they aren't changing on read + data = map[string]interface{}{} + req = &logical.Request{ + Operation: logical.ReadOperation, + Path: "static-creds/" + tc.path, + Storage: config.StorageView, + Data: data, + } + resp, err = b.HandleRequest(namespace.RootContext(nil), req) + if err != nil || (resp != nil && resp.IsError()) { + t.Fatalf("err:%s resp:%#v\n", err, resp) + } - if resp != nil { - t.Fatalf("Expected empty response from rotate-role: (%#v)", resp) - } + if username != resp.Data["username"].(string) || password != resp.Data["password"].(string) { + t.Fatal("expected re-read username/password to match, but didn't") + } - // Re-Read the creds - data = map[string]interface{}{} - req = &logical.Request{ - Operation: logical.ReadOperation, - Path: "static-creds/plugin-role-test", - Storage: config.StorageView, - Data: data, - } - resp, err = b.HandleRequest(namespace.RootContext(nil), req) - if err != nil || (resp != nil && resp.IsError()) { - t.Fatalf("err:%s resp:%#v\n", err, resp) - } + // Trigger rotation + data = map[string]interface{}{"name": "plugin-role-test"} + req = &logical.Request{ + Operation: logical.UpdateOperation, + Path: "rotate-role/" + tc.path, + Storage: config.StorageView, + Data: data, + } + resp, err = b.HandleRequest(namespace.RootContext(nil), req) + if err != nil || (resp != nil && resp.IsError()) { + t.Fatalf("err:%s resp:%#v\n", err, resp) + } - newPassword := resp.Data["password"].(string) - if password == newPassword { - t.Fatalf("expected passwords to differ, got (%s)", newPassword) - } + if resp != nil { + t.Fatalf("Expected empty response from rotate-role: (%#v)", resp) + } - // Verify new username/password - verifyPgConn(t, username, newPassword, connURL) + // Re-Read the creds + data = map[string]interface{}{} + req = &logical.Request{ + Operation: logical.ReadOperation, + Path: "static-creds/" + tc.path, + Storage: config.StorageView, + Data: data, + } + resp, err = b.HandleRequest(namespace.RootContext(nil), req) + if err != nil || (resp != nil && resp.IsError()) { + t.Fatalf("err:%s resp:%#v\n", err, resp) + } + + newPassword := resp.Data["password"].(string) + if password == newPassword { + t.Fatalf("expected passwords to differ, got (%s)", newPassword) + } + + // Verify new username/password + verifyPgConn(t, username, newPassword, connURL) + + if tc.waitTime > 0 { + time.Sleep(tc.waitTime) + // Re-Read the creds after schedule expiration + data = map[string]interface{}{} + req = &logical.Request{ + Operation: logical.ReadOperation, + Path: "static-creds/" + tc.path, + Storage: config.StorageView, + Data: data, + } + resp, err = b.HandleRequest(namespace.RootContext(nil), req) + if err != nil || (resp != nil && resp.IsError()) { + t.Fatalf("err:%s resp:%#v\n", err, resp) + } + + checkPassword := resp.Data["password"].(string) + if newPassword == checkPassword { + t.Fatalf("expected passwords to differ, got (%s)", checkPassword) + } + } + }) + } } // Sanity check to make sure we don't allow an attempt of rotating credentials From 38c3d769ed24d807378599649791206dc87c8be8 Mon Sep 17 00:00:00 2001 From: JM Faircloth Date: Mon, 21 Aug 2023 12:31:08 -0500 Subject: [PATCH 14/21] adv ttl mgmt: add tests for rotation_window --- builtin/logical/database/path_roles_test.go | 101 ++++++++++++++++++++ 1 file changed, 101 insertions(+) diff --git a/builtin/logical/database/path_roles_test.go b/builtin/logical/database/path_roles_test.go index b4f496fb1f97..0895975fd7c5 100644 --- a/builtin/logical/database/path_roles_test.go +++ b/builtin/logical/database/path_roles_test.go @@ -17,6 +17,7 @@ import ( 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/robfig/cron/v3" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/mock" ) @@ -902,6 +903,106 @@ func TestWALsDeletedOnRoleDeletion(t *testing.T) { requireWALs(t, storage, 1) } +func TestIsInsideRotationWindow(t *testing.T) { + rotationPeriodData := map[string]interface{}{ + "username": "hashicorp", + "db_name": "mockv5", + "rotation_period": "86400s", + } + + for _, tc := range []struct { + name string + expected bool + data map[string]interface{} + now time.Time + timeModifier func(t time.Time) time.Time + }{ + { + "always returns true for rotation_period type", + true, + rotationPeriodData, + time.Now(), + nil, + }, + { + "always returns true for rotation_schedule when no rotation_window set", + true, + map[string]interface{}{ + "username": "hashicorp", + "db_name": "mockv5", + "rotation_schedule": "0 0 */2 * * *", + }, + time.Now(), + nil, + }, + { + "returns true for rotation_schedule when inside rotation_window", + true, + map[string]interface{}{ + "username": "hashicorp", + "db_name": "mockv5", + "rotation_schedule": "0 0 */2 * * *", + "rotation_window": "3600s", + }, + time.Now(), + func(t time.Time) time.Time { + // set current time just inside window + return t.Add(-3640 * time.Second) + }, + }, + { + "returns false for rotation_schedule when outside rotation_window", + false, + map[string]interface{}{ + "username": "hashicorp", + "db_name": "mockv5", + "rotation_schedule": "0 0 */2 * * *", + "rotation_window": "3600s", + }, + time.Now(), + func(t time.Time) time.Time { + // set current time just outside window + return t.Add(-3560 * time.Second) + }, + }, + } { + t.Run(tc.name, func(t *testing.T) { + ctx := context.Background() + b, s, mockDB := getBackend(t) + defer b.Cleanup(ctx) + configureDBMount(t, s) + + testTime := tc.now + if tc.data["rotation_schedule"] != nil && tc.timeModifier != nil { + rotationSchedule := tc.data["rotation_schedule"].(string) + schedule, err := b.scheduleParser.Parse(rotationSchedule) + if err != nil { + t.Fatalf("could not parse rotation_schedule: %s", err) + } + sched, ok := schedule.(*cron.SpecSchedule) + if !ok { + t.Fatalf("could not parse rotation_schedule") + } + next1 := sched.Next(tc.now) // the next rotation time we expect + next2 := sched.Next(next1) // the next rotation time after that + testTime = tc.timeModifier(next2) + } + + createRoleWithData(t, b, s, mockDB, "test-role", tc.data) + role, err := b.StaticRole(ctx, s, "test-role") + if err != nil { + t.Fatal(err) + } + + // testTime := sched.Next(time.Now()).tc.timeModifier() + isInsideWindow := role.StaticAccount.IsInsideRotationWindow(testTime) + if tc.expected != isInsideWindow { + t.Fatalf("expected %t, got %t", tc.expected, isInsideWindow) + } + }) + } +} + func createRole(t *testing.T, b *databaseBackend, storage logical.Storage, mockDB *mockNewDatabase, roleName string) { t.Helper() mockDB.On("UpdateUser", mock.Anything, mock.Anything). From d725e4ab8c364b3c750beb49546cb41cf5179f6a Mon Sep 17 00:00:00 2001 From: John-Michael Faircloth Date: Mon, 21 Aug 2023 14:52:46 -0500 Subject: [PATCH 15/21] adv ttl mgmt: refactor window tests (#22472) --- builtin/logical/database/path_roles_test.go | 19 +++++-------------- 1 file changed, 5 insertions(+), 14 deletions(-) diff --git a/builtin/logical/database/path_roles_test.go b/builtin/logical/database/path_roles_test.go index 0895975fd7c5..3032bb46b612 100644 --- a/builtin/logical/database/path_roles_test.go +++ b/builtin/logical/database/path_roles_test.go @@ -904,12 +904,6 @@ func TestWALsDeletedOnRoleDeletion(t *testing.T) { } func TestIsInsideRotationWindow(t *testing.T) { - rotationPeriodData := map[string]interface{}{ - "username": "hashicorp", - "db_name": "mockv5", - "rotation_period": "86400s", - } - for _, tc := range []struct { name string expected bool @@ -920,7 +914,9 @@ func TestIsInsideRotationWindow(t *testing.T) { { "always returns true for rotation_period type", true, - rotationPeriodData, + map[string]interface{}{ + "rotation_period": "86400s", + }, time.Now(), nil, }, @@ -928,8 +924,6 @@ func TestIsInsideRotationWindow(t *testing.T) { "always returns true for rotation_schedule when no rotation_window set", true, map[string]interface{}{ - "username": "hashicorp", - "db_name": "mockv5", "rotation_schedule": "0 0 */2 * * *", }, time.Now(), @@ -939,8 +933,6 @@ func TestIsInsideRotationWindow(t *testing.T) { "returns true for rotation_schedule when inside rotation_window", true, map[string]interface{}{ - "username": "hashicorp", - "db_name": "mockv5", "rotation_schedule": "0 0 */2 * * *", "rotation_window": "3600s", }, @@ -954,8 +946,6 @@ func TestIsInsideRotationWindow(t *testing.T) { "returns false for rotation_schedule when outside rotation_window", false, map[string]interface{}{ - "username": "hashicorp", - "db_name": "mockv5", "rotation_schedule": "0 0 */2 * * *", "rotation_window": "3600s", }, @@ -988,13 +978,14 @@ func TestIsInsideRotationWindow(t *testing.T) { testTime = tc.timeModifier(next2) } + tc.data["username"] = "hashicorp" + tc.data["db_name"] = "mockv5" createRoleWithData(t, b, s, mockDB, "test-role", tc.data) role, err := b.StaticRole(ctx, s, "test-role") if err != nil { t.Fatal(err) } - // testTime := sched.Next(time.Now()).tc.timeModifier() isInsideWindow := role.StaticAccount.IsInsideRotationWindow(testTime) if tc.expected != isInsideWindow { t.Fatalf("expected %t, got %t", tc.expected, isInsideWindow) From fdeb7052d6d554f44b8e79d81f140ffd83adad11 Mon Sep 17 00:00:00 2001 From: Milena Zlaticanin <60530402+Zlaticanin@users.noreply.github.com> Date: Mon, 21 Aug 2023 14:31:29 -0700 Subject: [PATCH 16/21] Handle GET static-creds endpoint (#22476) * update read static-creds endpoint to include correct resp data * return rotation_window if set * update --- builtin/logical/database/path_creds_create.go | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/builtin/logical/database/path_creds_create.go b/builtin/logical/database/path_creds_create.go index d3e95876b501..9e1b6f2fb308 100644 --- a/builtin/logical/database/path_creds_create.go +++ b/builtin/logical/database/path_creds_create.go @@ -249,10 +249,18 @@ func (b *databaseBackend) pathStaticCredsRead() framework.OperationFunc { respData := map[string]interface{}{ "username": role.StaticAccount.Username, "ttl": role.StaticAccount.CredentialTTL().Seconds(), - "rotation_period": role.StaticAccount.RotationPeriod.Seconds(), "last_vault_rotation": role.StaticAccount.LastVaultRotation, } + if role.StaticAccount.UsesRotationPeriod() { + respData["rotation_period"] = role.StaticAccount.RotationPeriod.Seconds() + } else if role.StaticAccount.UsesRotationSchedule() { + respData["rotation_schedule"] = role.StaticAccount.RotationSchedule + if role.StaticAccount.RotationWindow.Seconds() != 0 { + respData["rotation_window"] = role.StaticAccount.RotationWindow.Seconds() + } + } + switch role.CredentialType { case v5.CredentialTypePassword: respData["password"] = role.StaticAccount.Password From 552e804a46014ed4313b19f1bc4040314d78764b Mon Sep 17 00:00:00 2001 From: JM Faircloth Date: Mon, 21 Aug 2023 16:51:08 -0500 Subject: [PATCH 17/21] add changelog --- changelog/22484.txt | 4 ++++ 1 file changed, 4 insertions(+) create mode 100644 changelog/22484.txt diff --git a/changelog/22484.txt b/changelog/22484.txt new file mode 100644 index 000000000000..6992e7c2fa56 --- /dev/null +++ b/changelog/22484.txt @@ -0,0 +1,4 @@ +```release-note:feature +**Database Static Role Advanced TTL Management**: Adds the ability to rotate +static roles on a defined schedule. +``` From 4fa0f547b38e5cce4ec80089efd2af0df4ab7d81 Mon Sep 17 00:00:00 2001 From: Milena Zlaticanin <60530402+Zlaticanin@users.noreply.github.com> Date: Tue, 22 Aug 2023 12:08:58 -0700 Subject: [PATCH 18/21] add unit test for static-creds read endpoint (#22505) --- builtin/logical/database/path_roles_test.go | 180 ++++++++++++++++++++ 1 file changed, 180 insertions(+) diff --git a/builtin/logical/database/path_roles_test.go b/builtin/logical/database/path_roles_test.go index 3032bb46b612..4cc142d783c9 100644 --- a/builtin/logical/database/path_roles_test.go +++ b/builtin/logical/database/path_roles_test.go @@ -463,6 +463,186 @@ func TestBackend_StaticRole_Config(t *testing.T) { } } +func TestBackend_StaticRole_ReadCreds(t *testing.T) { + cluster, sys := getCluster(t) + defer cluster.Cleanup() + + config := logical.TestBackendConfig() + config.StorageView = &logical.InmemStorage{} + config.System = sys + + lb, err := Factory(context.Background(), config) + if err != nil { + t.Fatal(err) + } + b, ok := lb.(*databaseBackend) + if !ok { + t.Fatal("could not convert to db backend") + } + defer b.Cleanup(context.Background()) + + cleanup, connURL := postgreshelper.PrepareTestContainer(t, "") + defer cleanup() + + // create the database user + createTestPGUser(t, connURL, dbUser, dbUserDefaultPassword, testRoleStaticCreate) + + verifyPgConn(t, dbUser, dbUserDefaultPassword, connURL) + + // Configure a connection + data := map[string]interface{}{ + "connection_url": connURL, + "plugin_name": "postgresql-database-plugin", + "verify_connection": false, + "allowed_roles": []string{"*"}, + "name": "plugin-test", + } + + req := &logical.Request{ + Operation: logical.UpdateOperation, + Path: "config/plugin-test", + Storage: config.StorageView, + Data: data, + } + resp, err := b.HandleRequest(namespace.RootContext(nil), req) + if err != nil || (resp != nil && resp.IsError()) { + t.Fatalf("err:%s resp:%#v\n", err, resp) + } + + testCases := map[string]struct { + account map[string]interface{} + path string + expected map[string]interface{} + }{ + "happy path for rotation_period": { + account: map[string]interface{}{ + "username": dbUser, + "rotation_period": "5400s", + }, + path: "plugin-role-test", + expected: map[string]interface{}{ + "username": dbUser, + "rotation_period": float64(5400), + }, + }, + "happy path for rotation_schedule": { + account: map[string]interface{}{ + "username": dbUser, + "rotation_schedule": "* * * * *", + }, + path: "plugin-role-test", + expected: map[string]interface{}{ + "username": dbUser, + "rotation_schedule": "* * * * *", + }, + }, + "happy path for rotation_schedule and rotation_window": { + account: map[string]interface{}{ + "username": dbUser, + "rotation_schedule": "* * * * *", + "rotation_window": "3600s", + }, + path: "plugin-role-test", + expected: map[string]interface{}{ + "username": dbUser, + "rotation_schedule": "* * * * *", + "rotation_window": float64(3600), + }, + }, + } + + for name, tc := range testCases { + t.Run(name, func(t *testing.T) { + data = map[string]interface{}{ + "name": "plugin-role-test", + "db_name": "plugin-test", + "rotation_statements": testRoleStaticUpdate, + "username": dbUser, + } + + for k, v := range tc.account { + data[k] = v + } + + req = &logical.Request{ + Operation: logical.CreateOperation, + Path: "static-roles/plugin-role-test", + Storage: config.StorageView, + Data: data, + } + + resp, err = b.HandleRequest(namespace.RootContext(nil), req) + if err != nil || (resp != nil && resp.IsError()) { + t.Fatalf("err:%s resp:%#v\n", err, resp) + } + + // Read the creds + data = map[string]interface{}{} + req = &logical.Request{ + Operation: logical.ReadOperation, + Path: "static-creds/plugin-role-test", + Storage: config.StorageView, + Data: data, + } + + resp, err = b.HandleRequest(namespace.RootContext(nil), req) + if err != nil || (resp != nil && resp.IsError()) { + t.Fatalf("err:%s resp:%#v\n", err, resp) + } + + expected := tc.expected + actual := make(map[string]interface{}) + dataKeys := []string{ + "username", + "password", + "last_vault_rotation", + "rotation_period", + "rotation_schedule", + "rotation_window", + "ttl", + } + for _, key := range dataKeys { + if v, ok := resp.Data[key]; ok { + actual[key] = v + } + } + + if len(tc.expected) > 0 { + // verify a password is returned, but we don't care what it's value is + if actual["password"] == "" { + t.Fatalf("expected result to contain password, but none found") + } + if actual["ttl"] == "" { + t.Fatalf("expected result to contain ttl, but none found") + } + if v, ok := actual["last_vault_rotation"].(time.Time); !ok { + t.Fatalf("expected last_vault_rotation to be set to time.Time type, got: %#v", v) + } + + // delete these values before the comparison, since we can't know them in + // advance + delete(actual, "password") + delete(actual, "ttl") + delete(actual, "last_vault_rotation") + if diff := deep.Equal(expected, actual); diff != nil { + t.Fatal(diff) + } + } + + // Delete role for next run + req = &logical.Request{ + Operation: logical.DeleteOperation, + Path: "static-roles/plugin-role-test", + Storage: config.StorageView, + } + resp, err = b.HandleRequest(namespace.RootContext(nil), req) + if err != nil || (resp != nil && resp.IsError()) { + t.Fatalf("err:%s resp:%#v\n", err, resp) + } + }) + } +} + func TestBackend_StaticRole_Updates(t *testing.T) { cluster, sys := getCluster(t) defer cluster.Cleanup() From aee50beff7bc90c51ec548a9f3754db7a2d99667 Mon Sep 17 00:00:00 2001 From: Zlaticanin Date: Wed, 23 Aug 2023 12:45:54 -0700 Subject: [PATCH 19/21] Add the ability to set seconds in cron schedule for testing purposes --- builtin/logical/database/backend.go | 8 +++----- builtin/logical/database/path_roles_test.go | 3 +++ builtin/logical/database/rotation_test.go | 6 ++++++ 3 files changed, 12 insertions(+), 5 deletions(-) diff --git a/builtin/logical/database/backend.go b/builtin/logical/database/backend.go index 2a4f3fcf5a9c..0c279ec71c05 100644 --- a/builtin/logical/database/backend.go +++ b/builtin/logical/database/backend.go @@ -36,6 +36,8 @@ const ( minRootCredRollbackAge = 1 * time.Minute ) +var scheduleOptions = cron.Minute | cron.Hour | cron.Dom | cron.Month | cron.Dow + type dbPluginInstance struct { sync.RWMutex database databaseVersionWrapper @@ -129,11 +131,7 @@ 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, - ) + parser := cron.NewParser(scheduleOptions) b.scheduleParser = parser return &b } diff --git a/builtin/logical/database/path_roles_test.go b/builtin/logical/database/path_roles_test.go index 4cc142d783c9..74c72ae594e3 100644 --- a/builtin/logical/database/path_roles_test.go +++ b/builtin/logical/database/path_roles_test.go @@ -1084,6 +1084,9 @@ func TestWALsDeletedOnRoleDeletion(t *testing.T) { } func TestIsInsideRotationWindow(t *testing.T) { + // We allow seconds to be set for testing purposes, but it's not to be used in production + scheduleOptions = cron.Second | cron.Minute | cron.Hour | cron.Dom | cron.Month | cron.Dow + for _, tc := range []struct { name string expected bool diff --git a/builtin/logical/database/rotation_test.go b/builtin/logical/database/rotation_test.go index 4e7b1dbfba77..93bed36e2909 100644 --- a/builtin/logical/database/rotation_test.go +++ b/builtin/logical/database/rotation_test.go @@ -14,6 +14,8 @@ import ( "testing" "time" + "github.com/robfig/cron/v3" + "github.com/Sectorbob/mlab-ns2/gae/ns/digest" "github.com/hashicorp/vault/helper/namespace" "github.com/hashicorp/vault/helper/testhelpers/mongodb" @@ -37,6 +39,9 @@ const ( ) func TestBackend_StaticRole_Rotation_basic(t *testing.T) { + // We allow seconds to be set for testing purposes, but it's not to be used in production + scheduleOptions = cron.Second | cron.Minute | cron.Hour | cron.Dom | cron.Month | cron.Dow + cluster, sys := getCluster(t) defer cluster.Cleanup() @@ -1218,6 +1223,7 @@ func TestRollsPasswordForwardsUsingWAL(t *testing.T) { } func TestStoredWALsCorrectlyProcessed(t *testing.T) { + scheduleOptions = cron.Second | cron.Minute | cron.Hour | cron.Dom | cron.Month | cron.Dow const walNewPassword = "new-password-from-wal" rotationPeriodData := map[string]interface{}{ From 324d13ec8f09ed605c0da5714233224c0e6b799a Mon Sep 17 00:00:00 2001 From: Zlaticanin Date: Thu, 24 Aug 2023 17:14:48 -0700 Subject: [PATCH 20/21] update test so we don't use global var --- builtin/logical/database/backend.go | 24 +++++++++++--- builtin/logical/database/path_roles.go | 8 ++--- builtin/logical/database/path_roles_test.go | 35 ++++++++++++--------- builtin/logical/database/rotation_test.go | 16 +++++----- 4 files changed, 50 insertions(+), 33 deletions(-) diff --git a/builtin/logical/database/backend.go b/builtin/logical/database/backend.go index 0c279ec71c05..1c5e936108f5 100644 --- a/builtin/logical/database/backend.go +++ b/builtin/logical/database/backend.go @@ -34,10 +34,9 @@ const ( databaseRolePath = "role/" databaseStaticRolePath = "static-role/" minRootCredRollbackAge = 1 * time.Minute + scheduleOptionsDefault = cron.Minute | cron.Hour | cron.Dom | cron.Month | cron.Dow ) -var scheduleOptions = cron.Minute | cron.Hour | cron.Dom | cron.Month | cron.Dow - type dbPluginInstance struct { sync.RWMutex database databaseVersionWrapper @@ -131,8 +130,6 @@ func Backend(conf *logical.BackendConfig) *databaseBackend { b.queueCtx, b.cancelQueueCtx = context.WithCancel(context.Background()) b.roleLocks = locksutil.CreateLocks() - parser := cron.NewParser(scheduleOptions) - b.scheduleParser = parser return &b } @@ -183,7 +180,24 @@ type databaseBackend struct { gaugeCollectionProcess *metricsutil.GaugeCollectionProcess gaugeCollectionProcessStop sync.Once - scheduleParser cron.Parser + scheduleOptionsOverride cron.ParseOption +} + +func (b *databaseBackend) ParseSchedule(rotationSchedule string) (*cron.SpecSchedule, error) { + scheduleOptions := scheduleOptionsDefault + if b.scheduleOptionsOverride != 0 { + scheduleOptions = b.scheduleOptionsOverride + } + parser := cron.NewParser(scheduleOptions) + schedule, err := parser.Parse(rotationSchedule) + if err != nil { + return nil, err + } + sched, ok := schedule.(*cron.SpecSchedule) + if !ok { + return nil, fmt.Errorf("invalid rotation schedule") + } + return sched, nil } func (b *databaseBackend) DatabaseConfig(ctx context.Context, s logical.Storage, name string) (*DatabaseConfig, error) { diff --git a/builtin/logical/database/path_roles.go b/builtin/logical/database/path_roles.go index 5366ac711c0c..43417f000a74 100644 --- a/builtin/logical/database/path_roles.go +++ b/builtin/logical/database/path_roles.go @@ -591,16 +591,12 @@ func (b *databaseBackend) pathStaticRoleCreateUpdate(ctx context.Context, req *l } if rotationScheduleOk { - schedule, err := b.scheduleParser.Parse(rotationSchedule) + parsedSchedule, err := b.ParseSchedule(rotationSchedule) if err != nil { return logical.ErrorResponse("could not parse rotation_schedule", "error", err), nil } role.StaticAccount.RotationSchedule = rotationSchedule - sched, ok := schedule.(*cron.SpecSchedule) - if !ok { - return logical.ErrorResponse("could not parse rotation_schedule"), nil - } - role.StaticAccount.Schedule = *sched + role.StaticAccount.Schedule = *parsedSchedule if rotationWindowOk { rotationWindowSeconds := rotationWindowSecondsRaw.(int) diff --git a/builtin/logical/database/path_roles_test.go b/builtin/logical/database/path_roles_test.go index 74c72ae594e3..672a9a96e696 100644 --- a/builtin/logical/database/path_roles_test.go +++ b/builtin/logical/database/path_roles_test.go @@ -252,10 +252,11 @@ func TestBackend_StaticRole_Config(t *testing.T) { // Test static role creation scenarios. Uses a map, so there is no guaranteed // 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 + account map[string]interface{} + path string + expected map[string]interface{} + scheduleOptions cron.ParseOption + err error // use this field to check partial error strings, otherwise use err errContains string }{ @@ -344,6 +345,19 @@ func TestBackend_StaticRole_Config(t *testing.T) { path: "disallowed-role", err: errors.New("\"disallowed-role\" is not an allowed role"), }, + "fails to parse cronSpec with seconds": { + account: map[string]interface{}{ + "username": dbUser, + "rotation_schedule": "*/10 * * * * *", + }, + path: "plugin-role-test-1", + expected: map[string]interface{}{ + "username": dbUser, + "rotation_schedule": "*/10 * * * * *", + }, + scheduleOptions: scheduleOptionsDefault, + errContains: "could not parse rotation_schedule", + }, } for name, tc := range testCases { @@ -1084,9 +1098,6 @@ func TestWALsDeletedOnRoleDeletion(t *testing.T) { } func TestIsInsideRotationWindow(t *testing.T) { - // We allow seconds to be set for testing purposes, but it's not to be used in production - scheduleOptions = cron.Second | cron.Minute | cron.Hour | cron.Dom | cron.Month | cron.Dow - for _, tc := range []struct { name string expected bool @@ -1148,16 +1159,12 @@ func TestIsInsideRotationWindow(t *testing.T) { testTime := tc.now if tc.data["rotation_schedule"] != nil && tc.timeModifier != nil { rotationSchedule := tc.data["rotation_schedule"].(string) - schedule, err := b.scheduleParser.Parse(rotationSchedule) + schedule, err := b.ParseSchedule(rotationSchedule) if err != nil { t.Fatalf("could not parse rotation_schedule: %s", err) } - sched, ok := schedule.(*cron.SpecSchedule) - if !ok { - t.Fatalf("could not parse rotation_schedule") - } - next1 := sched.Next(tc.now) // the next rotation time we expect - next2 := sched.Next(next1) // the next rotation time after that + next1 := schedule.Next(tc.now) // the next rotation time we expect + next2 := schedule.Next(next1) // the next rotation time after that testTime = tc.timeModifier(next2) } diff --git a/builtin/logical/database/rotation_test.go b/builtin/logical/database/rotation_test.go index 93bed36e2909..4ee3c6d775f0 100644 --- a/builtin/logical/database/rotation_test.go +++ b/builtin/logical/database/rotation_test.go @@ -14,8 +14,6 @@ import ( "testing" "time" - "github.com/robfig/cron/v3" - "github.com/Sectorbob/mlab-ns2/gae/ns/digest" "github.com/hashicorp/vault/helper/namespace" "github.com/hashicorp/vault/helper/testhelpers/mongodb" @@ -27,6 +25,7 @@ import ( "github.com/hashicorp/vault/sdk/logical" "github.com/hashicorp/vault/sdk/queue" _ "github.com/jackc/pgx/v4/stdlib" + "github.com/robfig/cron/v3" "github.com/stretchr/testify/mock" mongodbatlasapi "go.mongodb.org/atlas/mongodbatlas" "go.mongodb.org/mongo-driver/mongo" @@ -34,14 +33,12 @@ import ( ) const ( - dbUser = "vaultstatictest" - dbUserDefaultPassword = "password" + dbUser = "vaultstatictest" + dbUserDefaultPassword = "password" + testScheduleOptionsSeconds = cron.Second | cron.Minute | cron.Hour | cron.Dom | cron.Month | cron.Dow ) func TestBackend_StaticRole_Rotation_basic(t *testing.T) { - // We allow seconds to be set for testing purposes, but it's not to be used in production - scheduleOptions = cron.Second | cron.Minute | cron.Hour | cron.Dom | cron.Month | cron.Dow - cluster, sys := getCluster(t) defer cluster.Cleanup() @@ -59,6 +56,8 @@ func TestBackend_StaticRole_Rotation_basic(t *testing.T) { } defer b.Cleanup(context.Background()) + b.scheduleOptionsOverride = testScheduleOptionsSeconds + cleanup, connURL := postgreshelper.PrepareTestContainer(t, "") defer cleanup() @@ -1223,7 +1222,6 @@ func TestRollsPasswordForwardsUsingWAL(t *testing.T) { } func TestStoredWALsCorrectlyProcessed(t *testing.T) { - scheduleOptions = cron.Second | cron.Minute | cron.Hour | cron.Dom | cron.Month | cron.Dow const walNewPassword = "new-password-from-wal" rotationPeriodData := map[string]interface{}{ @@ -1299,6 +1297,7 @@ func TestStoredWALsCorrectlyProcessed(t *testing.T) { t.Fatal(err) } b.credRotationQueue = queue.New() + b.scheduleOptionsOverride = testScheduleOptionsSeconds configureDBMount(t, config.StorageView) createRoleWithData(t, b, config.StorageView, mockDB, tc.wal.RoleName, tc.data) role, err := b.StaticRole(ctx, config.StorageView, "hashicorp") @@ -1459,6 +1458,7 @@ func getBackend(t *testing.T) (*databaseBackend, logical.Storage, *mockNewDataba if err := b.Setup(context.Background(), config); err != nil { t.Fatal(err) } + b.scheduleOptionsOverride = testScheduleOptionsSeconds b.credRotationQueue = queue.New() b.populateQueue(context.Background(), config.StorageView) From 6318db4449c6aae30741b3e694a456e1416079dd Mon Sep 17 00:00:00 2001 From: Zlaticanin Date: Fri, 25 Aug 2023 09:14:33 -0700 Subject: [PATCH 21/21] update with suggestions --- builtin/logical/database/backend.go | 1 + builtin/logical/database/path_roles_test.go | 19 ++++++------------- 2 files changed, 7 insertions(+), 13 deletions(-) diff --git a/builtin/logical/database/backend.go b/builtin/logical/database/backend.go index 1c5e936108f5..4c97935ce584 100644 --- a/builtin/logical/database/backend.go +++ b/builtin/logical/database/backend.go @@ -180,6 +180,7 @@ type databaseBackend struct { gaugeCollectionProcess *metricsutil.GaugeCollectionProcess gaugeCollectionProcessStop sync.Once + // scheduleOptionsOverride is used by tests to set a custom ParseOption with seconds enabled scheduleOptionsOverride cron.ParseOption } diff --git a/builtin/logical/database/path_roles_test.go b/builtin/logical/database/path_roles_test.go index 672a9a96e696..bb7b5b93a90e 100644 --- a/builtin/logical/database/path_roles_test.go +++ b/builtin/logical/database/path_roles_test.go @@ -17,7 +17,6 @@ import ( 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/robfig/cron/v3" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/mock" ) @@ -252,11 +251,10 @@ func TestBackend_StaticRole_Config(t *testing.T) { // Test static role creation scenarios. Uses a map, so there is no guaranteed // ordering, so each case cleans up by deleting the role testCases := map[string]struct { - account map[string]interface{} - path string - expected map[string]interface{} - scheduleOptions cron.ParseOption - err error + account map[string]interface{} + path string + expected map[string]interface{} + err error // use this field to check partial error strings, otherwise use err errContains string }{ @@ -350,13 +348,8 @@ func TestBackend_StaticRole_Config(t *testing.T) { "username": dbUser, "rotation_schedule": "*/10 * * * * *", }, - path: "plugin-role-test-1", - expected: map[string]interface{}{ - "username": dbUser, - "rotation_schedule": "*/10 * * * * *", - }, - scheduleOptions: scheduleOptionsDefault, - errContains: "could not parse rotation_schedule", + path: "plugin-role-test-1", + errContains: "could not parse rotation_schedule", }, }