Core handling of TTLs #4230
Core handling of TTLs #4230
Conversation
| if te.TTL == 0 && !strutil.StrListContains(te.Policies, "root") { | ||
| te.TTL = sysView.DefaultLeaseTTL() | ||
| // Only calculate a TTL if you are A) periodic, B) have a TTL, C) do not have a TTL and are not a root token | ||
| if periodToUse > 0 || te.TTL > 0 || (te.TTL == 0 && !strutil.StrListContains(te.Policies, "root")) { |
There was a problem hiding this comment.
Given that only root tokens should not have a TTL, would it be safer here to instead only not run calculateTTL if it's root?
There was a problem hiding this comment.
The old logic would still enforce periodic on root tokens, I just kept it that way.
vault/ttl.go
Outdated
| } | ||
|
|
||
| // We cannot go past this time | ||
| maxValidTime = startTime.Add(maxTTL) |
There was a problem hiding this comment.
This switch can be cleaned up a bit if you initialize maxValidTime := startTime.Add(maxTTL) and then have the switch like:
switch {
case period > 0:
...
case increment > 0:
ttl = increment
case backendTTL > 0:
ttl = backendTTL
default:
ttl = sysView.DefaultLeaseTTL()
}
There was a problem hiding this comment.
The only problem with this is that maxTTL does not limit the period value. I implemented it this way and it ended up about the same.
vault/expiration.go
Outdated
| @@ -1056,7 +1066,7 @@ func (m *ExpirationManager) revokeEntry(le *leaseEntry) error { | |||
| } | |||
|
|
|||
| // renewEntry is used to attempt renew of an internal entry | |||
| func (m *ExpirationManager) renewEntry(le *leaseEntry) (*logical.Response, error) { | |||
| func (m *ExpirationManager) renewEntry(le *leaseEntry, estimatedTTL time.Duration) (*logical.Response, error) { | |||
There was a problem hiding this comment.
This isn't being used... is it just not implemented yet?
There was a problem hiding this comment.
Fixed in 65e9e1b
vault/expiration.go
Outdated
| @@ -1069,7 +1079,7 @@ func (m *ExpirationManager) renewEntry(le *leaseEntry) (*logical.Response, error | |||
|
|
|||
| // renewAuthEntry is used to attempt renew of an auth entry. Only the token | |||
| // store should get the actual token ID intact. | |||
| func (m *ExpirationManager) renewAuthEntry(req *logical.Request, le *leaseEntry) (*logical.Response, error) { | |||
| func (m *ExpirationManager) renewAuthEntry(req *logical.Request, le *leaseEntry, estimatedTTL time.Duration) (*logical.Response, error) { | |||
| @@ -111,11 +111,6 @@ func (b *backend) pathLoginRenew(ctx context.Context, req *logical.Request, d *f | |||
| return nil, fmt.Errorf("policies have changed, not renewing") | |||
| } | |||
|
|
|||
| resp, err = framework.LeaseExtend(0, 0, b.System())(ctx, req, d) | |||
There was a problem hiding this comment.
I think the request.Auth is not carried over to the response in this case.
There was a problem hiding this comment.
This is fixed in 2317f5e
logical/framework/lease.go
Outdated
| } | ||
| } | ||
|
|
||
| // CalculateTTL takes all the user-specifie, backend, and system inputs and calculates |
| // Make sure we increase the VALID UNTIL endpoint for this user. | ||
| if expireTime := resp.Secret.ExpirationTime(); !expireTime.IsZero() { | ||
| // Make sure we increase the VALID UNTIL endpoint for this user. This value is estimated and does not | ||
| // take into account any backend specific values. These value will be calculated by core and will only |
| } | ||
| if ttl > 0 { | ||
| expireTime := time.Now().Add(ttl) | ||
| // Adding a small buffer since the TTL will be calculated again afeter this call |
logical/framework/lease.go
Outdated
| // cap the increment to whatever is left | ||
| if maxValidTime.Before(proposedExpiration) { | ||
| increment = maxValidTime.Sub(now) | ||
| // cap the increment to whatever is left, with a small buffer due to |
There was a problem hiding this comment.
The comment says that there will be a small buffer, but we aren't adding any buffer.
vault/expiration.go
Outdated
| @@ -739,56 +755,40 @@ func (m *ExpirationManager) RenewToken(req *logical.Request, source string, toke | |||
| return logical.ErrorResponse(err.Error()), logical.ErrInvalidRequest | |||
| } | |||
|
|
|||
| sysView := m.router.MatchingSystemView(le.Path) | |||
There was a problem hiding this comment.
Can we move this to be just above CalculateTTL?
| } | ||
| } | ||
| return f(ctx, req, d) | ||
| req.Auth.Period = te.Period |
There was a problem hiding this comment.
Is there a reason for not doing req.Auth.TTL = te.TTL here?
There was a problem hiding this comment.
The original implementation did not take into account the token entry TTL in calculating a new TTL. I think the token entry TTL is just the original TTL and we just use the mount configured values when calculating new TTLs.
This PR moves the responsibility of TTL handling from the backends into core. This should ensure a standard way for calculating and validating TTL values. The backends only have to pass back the specific request configured TTL and period and core will determine the TTL to put on the lease.
LeaseExtend has also been converted to work in a backwards compatible way. Additionally, an auth backend (AppRole) and secret backend (AWS) have been implemented using the new method. Once the core logic is reviewed, the rest of the backends and plugins will get updated although the LeaseExtend semantics will still work.