-
Notifications
You must be signed in to change notification settings - Fork 4.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
secrets/database: advanced TTL management for static roles #22484
Changes from all commits
2d934c9
5511ed4
6b95acf
417466c
4779495
acb5ebb
7ceac55
6aedaaf
7a542fc
c270552
f23ff91
1794670
519eacf
38c3d76
d725e4a
fdeb705
552e804
4fa0f54
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -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,13 @@ func Backend(conf *logical.BackendConfig) *databaseBackend { | |||||
b.connections = syncmap.NewSyncMap[string, *dbPluginInstance]() | ||||||
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, | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
) | ||||||
b.scheduleParser = parser | ||||||
return &b | ||||||
} | ||||||
|
||||||
|
@@ -176,6 +184,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) { | ||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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/v3" | ||
) | ||
|
||
func pathListRoles(b *databaseBackend) []*framework.Path { | ||
|
@@ -196,7 +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. 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, | ||
|
@@ -293,10 +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 | ||
} | ||
|
||
// only return one of the mutually exclusive fields in the response | ||
if role.StaticAccount.UsesRotationPeriod() { | ||
data["rotation_period"] = role.StaticAccount.RotationPeriod.Seconds() | ||
} else if role.StaticAccount.UsesRotationSchedule() { | ||
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 { | ||
|
@@ -480,6 +502,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 +559,18 @@ 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) | ||
vinay-gopalan marked this conversation as resolved.
Show resolved
Hide resolved
|
||
rotationScheduleOk := rotationSchedule != "" | ||
Comment on lines
+563
to
+564
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: I might be missing something but curious if there is a reason There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this line was added a bit later when we realized we needed the boolean. Will update, thanks! |
||
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 | ||
} 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 | ||
|
@@ -550,6 +579,40 @@ 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 | ||
|
||
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 { | ||
schedule, err := b.scheduleParser.Parse(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 | ||
|
||
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 | ||
} | ||
|
||
// 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 { | ||
|
@@ -623,14 +686,23 @@ func (b *databaseBackend) pathStaticRoleCreateUpdate(ctx context.Context, req *l | |
} | ||
} | ||
|
||
item.Priority = lvr.Add(role.StaticAccount.RotationPeriod).Unix() | ||
_, rotationPeriodChanged := data.Raw["rotation_period"] | ||
_, rotationScheduleChanged := data.Raw["rotation_schedule"] | ||
Comment on lines
+689
to
+690
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Curious if there is a difference between these values from There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nope, good catch! |
||
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 if rotationScheduleChanged { | ||
next := role.StaticAccount.Schedule.Next(lvr) | ||
b.logger.Debug("init priority for Schedule", "lvr", lvr, "next", next) | ||
item.Priority = role.StaticAccount.Schedule.Next(lvr).Unix() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: any good reason to not use |
||
} | ||
|
||
// Add their rotation to the queue | ||
if err := b.pushItem(item); err != nil { | ||
return nil, err | ||
} | ||
|
||
return nil, nil | ||
return response, nil | ||
} | ||
|
||
type roleEntry struct { | ||
|
@@ -730,24 +802,103 @@ 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 | ||
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_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 | ||
// database user when the role is deleted | ||
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) | ||
} | ||
|
||
// 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 { | ||
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 == "" | ||
} | ||
|
||
// 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 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 | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are we going to remove supporting seconds altogether or are we going to add some safeguards so they can still be run in our CI tests but users can't add them in production? From the tests set up below, I can definitely see us keeping seconds around just for our testing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, also just saw Milena's PR to only allow seconds in testing, so this comment should be resolved when that PR is merged 😅