From a671890555c78de61cef44ec1a47fe114ee766ee Mon Sep 17 00:00:00 2001 From: Brian Kassouf Date: Tue, 18 May 2021 16:52:34 -0700 Subject: [PATCH] Patch expiration fix over from ENT (#11650) (#11652) * Patch expiration fix over from ENT * Rename changelog Co-authored-by: Scott Miller --- changelog/11650.txt | 3 + sdk/framework/lease.go | 2 +- vault/expiration.go | 35 +++-- vault/expiration_test.go | 138 +++++++++++++++++- vault/token_store_test.go | 4 +- .../hashicorp/vault/sdk/framework/lease.go | 2 +- 6 files changed, 168 insertions(+), 16 deletions(-) create mode 100644 changelog/11650.txt diff --git a/changelog/11650.txt b/changelog/11650.txt new file mode 100644 index 000000000000..75029f94a090 --- /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 f5c68b841d0f..4d0240fbe7fd 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 360b8ca07f50..deac8b862e11 100644 --- a/vault/expiration.go +++ b/vault/expiration.go @@ -578,6 +578,7 @@ func (m *ExpirationManager) Tidy(ctx context.Context) error { } else { tokenCache[le.ClientToken] = true } + goto REVOKE_CHECK } else { if isValid { @@ -793,6 +794,7 @@ func (m *ExpirationManager) processRestore(ctx context.Context, leaseID string) if err != nil { return err } + return nil } @@ -954,7 +956,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 } @@ -1673,15 +1681,13 @@ func (m *ExpirationManager) updatePendingInternal(le *leaseEntry) { // Check for an existing timer info, ok := m.pending.Load(le.LeaseID) - 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. @@ -2360,7 +2366,14 @@ 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 +} + +func (le *leaseEntry) isIncorrectlyNonExpiring() bool { + return le.ExpireTime.IsZero() && !le.nonexpiringToken() } // decodeLeaseEntry is used to reverse encode and return a new entry diff --git a/vault/expiration_test.go b/vault/expiration_test.go index 2665c227242e..895108ac0876 100644 --- a/vault/expiration_test.go +++ b/vault/expiration_test.go @@ -820,6 +820,7 @@ func TestExpiration_RegisterAuth_NoLease(t *testing.T) { auth := &logical.Auth{ ClientToken: root.ID, + Policies: []string{"root"}, } te := &logical.TokenEntry{ @@ -1611,7 +1612,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{} @@ -1688,6 +1688,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 7643927c03f9..e6ca297a6da9 100644 --- a/vault/token_store_test.go +++ b/vault/token_store_test.go @@ -3761,7 +3761,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()) { @@ -3818,7 +3818,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 f5c68b841d0f..4d0240fbe7fd 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") }