Patch expiration fix over from ENT (#11650) (#11654)

* Patch expiration fix over from ENT

* Rename changelog

Co-authored-by: Scott Miller <[email protected]>

changelog/11650.txt

@@ -0,0 +1,3 @@
+```release-note:bug
+core: correct logic for renewal of leases nearing their expiration time.
+```

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")
 		}
 

vault/expiration.go

@@ -389,6 +389,7 @@ func (m *ExpirationManager) Tidy(ctx context.Context) error {
 			} else {
 				tokenCache[le.ClientToken] = true
 			}
+
 			goto REVOKE_CHECK
 		} else {
 			if isValid {
@@ -604,6 +605,7 @@ func (m *ExpirationManager) processRestore(ctx context.Context, leaseID string)
 	if err != nil {
 		return err
 	}
+
 	return nil
 }
 
@@ -747,7 +749,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
 }
 
@@ -1459,15 +1467,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.
@@ -2061,7 +2067,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

vault/expiration_test.go

@@ -625,6 +625,7 @@ func TestExpiration_RegisterAuth_NoLease(t *testing.T) {
 
 	auth := &logical.Auth{
 		ClientToken: root.ID,
+		Policies:    []string{"root"},
 	}
 
 	te := &logical.TokenEntry{
@@ -1416,7 +1417,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{}
@@ -1493,6 +1493,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)
 

vault/token_store_test.go

@@ -3709,7 +3709,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()) {
@@ -3766,7 +3766,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()) {