diff --git a/changelog/11650.txt b/changelog/11650.txt new file mode 100644 index 000000000..75029f94a --- /dev/null +++ b/changelog/11650.txt @@ -0,0 +1,3 @@ +```release-note:bug +core: correct logic for renewal of leases nearing their expiration time. +``` diff --git a/sdk/framework/lease.go b/sdk/framework/lease.go index f5c68b841..4d0240fbe 100644 --- a/sdk/framework/lease.go +++ b/sdk/framework/lease.go @@ -91,7 +91,7 @@ func CalculateTTL(sysView logical.SystemView, increment, backendTTL, period, bac // If we are past the max TTL, we shouldn't be in this function...but // fast path out if we are - if maxValidTTL < 0 { + if maxValidTTL <= 0 { return 0, nil, fmt.Errorf("past the max TTL, cannot renew") } diff --git a/vault/expiration.go b/vault/expiration.go index cee48ae15..a76b4b0c1 100644 --- a/vault/expiration.go +++ b/vault/expiration.go @@ -629,6 +629,7 @@ func (m *ExpirationManager) Tidy(ctx context.Context) error { } else { tokenCache[le.ClientToken] = true } + goto REVOKE_CHECK } else { if isValid { @@ -844,6 +845,7 @@ func (m *ExpirationManager) processRestore(ctx context.Context, leaseID string) if err != nil { return err } + return nil } @@ -1001,7 +1003,13 @@ func (m *ExpirationManager) revokeCommon(ctx context.Context, leaseID string, fo if m.logger.IsInfo() && !skipToken && m.logLeaseExpirations { m.logger.Info("revoked lease", "lease_id", leaseID) } - + if m.logger.IsWarn() && !skipToken && le.isIncorrectlyNonExpiring() { + var accessor string + if le.Auth != nil { + accessor = le.Auth.Accessor + } + m.logger.Warn("finished revoking incorrectly non-expiring lease", "leaseID", le.LeaseID, "accessor", accessor) + } return nil } @@ -1727,15 +1735,14 @@ func (m *ExpirationManager) updatePendingInternal(le *leaseEntry) { info, leaseInPending := m.pending.Load(le.LeaseID) var pending pendingInfo - if le.ExpireTime.IsZero() { - if le.nonexpiringToken() { - // Store this in the nonexpiring map instead of pending. - // There does not appear to be any cases where a token that had - // a nonzero can be can be assigned a zero TTL, but we can handle that - // anyway by falling through to the next check. - pending.cachedLeaseInfo = m.inMemoryLeaseInfo(le) - m.nonexpiring.Store(le.LeaseID, pending) - } + + if le.ExpireTime.IsZero() && le.nonexpiringToken() { + // Store this in the nonexpiring map instead of pending. + // There does not appear to be any cases where a token that had + // a nonzero can be can be assigned a zero TTL, but that can be + // handled by the next check + pending.cachedLeaseInfo = m.inMemoryLeaseInfo(le) + m.nonexpiring.Store(le.LeaseID, pending) // if the timer happened to exist, stop the time and delete it from the // pending timers. @@ -2482,7 +2489,10 @@ func (le *leaseEntry) nonexpiringToken() bool { if le.Auth == nil { return false } - return !le.Auth.LeaseEnabled() + // Note that at this time the only non-expiring tokens are root tokens, this test is more involved as it is trying + // to catch tokens created by the VAULT-1949 non-expiring tokens bug and ensure they become expiring. + return !le.Auth.LeaseEnabled() && len(le.Auth.Policies) == 1 && le.Auth.Policies[0] == "root" && le.namespace != nil && + le.namespace.ID == namespace.RootNamespaceID } // TODO maybe lock RevokeErr once this goes in: https://github.com/hashicorp/vault/pull/11122 @@ -2490,6 +2500,10 @@ func (le *leaseEntry) isIrrevocable() bool { return le.RevokeErr != "" } +func (le *leaseEntry) isIncorrectlyNonExpiring() bool { + return le.ExpireTime.IsZero() && !le.nonexpiringToken() +} + // decodeLeaseEntry is used to reverse encode and return a new entry func decodeLeaseEntry(buf []byte) (*leaseEntry, error) { out := new(leaseEntry) diff --git a/vault/expiration_test.go b/vault/expiration_test.go index 2447332da..055021444 100644 --- a/vault/expiration_test.go +++ b/vault/expiration_test.go @@ -917,6 +917,7 @@ func TestExpiration_RegisterAuth_NoLease(t *testing.T) { auth := &logical.Auth{ ClientToken: root.ID, + Policies: []string{"root"}, } te := &logical.TokenEntry{ @@ -1706,7 +1707,6 @@ func TestExpiration_Renew_NotRenewable(t *testing.T) { t.Fatalf("Bad: %#v", noop.Requests) } } - func TestExpiration_Renew_RevokeOnExpire(t *testing.T) { exp := mockExpiration(t) noop := &NoopBackend{} @@ -1783,6 +1783,142 @@ func TestExpiration_Renew_RevokeOnExpire(t *testing.T) { } } +func TestExpiration_Renew_FinalSecond(t *testing.T) { + exp := mockExpiration(t) + noop := &NoopBackend{} + _, barrier, _ := mockBarrier(t) + view := NewBarrierView(barrier, "logical/") + meUUID, err := uuid.GenerateUUID() + if err != nil { + t.Fatal(err) + } + err = exp.router.Mount(noop, "prod/aws/", &MountEntry{Path: "prod/aws/", Type: "noop", UUID: meUUID, Accessor: "noop-accessor", namespace: namespace.RootNamespace}, view) + if err != nil { + t.Fatal(err) + } + + req := &logical.Request{ + Operation: logical.ReadOperation, + Path: "prod/aws/foo", + ClientToken: "foobar", + } + req.SetTokenEntry(&logical.TokenEntry{ID: "foobar", NamespaceID: "root"}) + resp := &logical.Response{ + Secret: &logical.Secret{ + LeaseOptions: logical.LeaseOptions{ + TTL: 2 * time.Second, + Renewable: true, + }, + }, + Data: map[string]interface{}{ + "access_key": "xyz", + "secret_key": "abcd", + }, + } + + ctx := namespace.RootContext(nil) + id, err := exp.Register(ctx, req, resp) + if err != nil { + t.Fatalf("err: %v", err) + } + le, err := exp.loadEntry(ctx, id) + if err != nil { + t.Fatalf("err: %v", err) + } + // Give it an auth section to emulate the real world bug + le.Auth = &logical.Auth{ + LeaseOptions: logical.LeaseOptions{ + Renewable: true, + }, + } + exp.persistEntry(ctx, le) + + noop.Response = &logical.Response{ + Secret: &logical.Secret{ + LeaseOptions: logical.LeaseOptions{ + TTL: 2 * time.Second, + MaxTTL: 2 * time.Second, + }, + }, + Data: map[string]interface{}{ + "access_key": "123", + "secret_key": "abcd", + }, + } + + time.Sleep(1000 * time.Millisecond) + _, err = exp.Renew(ctx, id, 0) + if err != nil { + t.Fatalf("err: %v", err) + } + + if _, ok := exp.nonexpiring.Load(id); ok { + t.Fatalf("expirable lease became nonexpiring") + } +} + +func TestExpiration_Renew_FinalSecond_Lease(t *testing.T) { + exp := mockExpiration(t) + noop := &NoopBackend{} + _, barrier, _ := mockBarrier(t) + view := NewBarrierView(barrier, "logical/") + meUUID, err := uuid.GenerateUUID() + if err != nil { + t.Fatal(err) + } + err = exp.router.Mount(noop, "prod/aws/", &MountEntry{Path: "prod/aws/", Type: "noop", UUID: meUUID, Accessor: "noop-accessor", namespace: namespace.RootNamespace}, view) + if err != nil { + t.Fatal(err) + } + + req := &logical.Request{ + Operation: logical.ReadOperation, + Path: "prod/aws/foo", + ClientToken: "foobar", + } + req.SetTokenEntry(&logical.TokenEntry{ID: "foobar", NamespaceID: "root"}) + resp := &logical.Response{ + Secret: &logical.Secret{ + LeaseOptions: logical.LeaseOptions{ + TTL: 2 * time.Second, + Renewable: true, + }, + LeaseID: "abcde", + }, + Data: map[string]interface{}{ + "access_key": "xyz", + "secret_key": "abcd", + }, + } + + ctx := namespace.RootContext(nil) + id, err := exp.Register(ctx, req, resp) + if err != nil { + t.Fatalf("err: %v", err) + } + le, err := exp.loadEntry(ctx, id) + if err != nil { + t.Fatalf("err: %v", err) + } + // Give it an auth section to emulate the real world bug + le.Auth = &logical.Auth{ + LeaseOptions: logical.LeaseOptions{ + Renewable: true, + }, + } + exp.persistEntry(ctx, le) + + time.Sleep(1000 * time.Millisecond) + _, err = exp.Renew(ctx, id, 0) + if err != nil { + t.Fatalf("err: %v", err) + } + + if _, ok := exp.nonexpiring.Load(id); ok { + t.Fatalf("expirable lease became nonexpiring") + } +} + func TestExpiration_revokeEntry(t *testing.T) { exp := mockExpiration(t) diff --git a/vault/token_store_test.go b/vault/token_store_test.go index 31ce20e73..3fcbb4966 100644 --- a/vault/token_store_test.go +++ b/vault/token_store_test.go @@ -3788,7 +3788,7 @@ func TestTokenStore_RolePeriod(t *testing.T) { req.Operation = logical.UpdateOperation req.Path = "auth/token/renew-self" req.Data = map[string]interface{}{ - "increment": 1, + "increment": 2, } resp, err = core.HandleRequest(namespace.RootContext(nil), req) if err != nil || (resp != nil && resp.IsError()) { @@ -3845,7 +3845,7 @@ func TestTokenStore_RolePeriod(t *testing.T) { req.Operation = logical.UpdateOperation req.Path = "auth/token/renew-self" req.Data = map[string]interface{}{ - "increment": 1, + "increment": 2, } resp, err = core.HandleRequest(namespace.RootContext(nil), req) if err != nil || (resp != nil && resp.IsError()) { diff --git a/vendor/github.com/hashicorp/vault/sdk/framework/lease.go b/vendor/github.com/hashicorp/vault/sdk/framework/lease.go index f5c68b841..4d0240fbe 100644 --- a/vendor/github.com/hashicorp/vault/sdk/framework/lease.go +++ b/vendor/github.com/hashicorp/vault/sdk/framework/lease.go @@ -91,7 +91,7 @@ func CalculateTTL(sysView logical.SystemView, increment, backendTTL, period, bac // If we are past the max TTL, we shouldn't be in this function...but // fast path out if we are - if maxValidTTL < 0 { + if maxValidTTL <= 0 { return 0, nil, fmt.Errorf("past the max TTL, cannot renew") }