fixes #3673 purge expired revocations, fix revocation bugs (#3679)

- fixes RevokeToken double-write: adds return after JWTID-keyed revocation
    save, preventing fallthrough write with raw JWT string as unreachable key
  - fixes TerminateSession key mismatch: stores revocation by identityId alone,
    matching the Subject-based lookup in ValidateAccessToken
  - fixes RevocationDelete sync action: passes DataState_Delete instead of
    DataState_Create so routers evict the entry from their data model
  - adds RevocationManager.DeleteExpired: batch-deletes expired revocations in
    batches of 500 until none remain
  - adds RevocationEnforcer: periodic policy runner (every 1 minute) that calls
    DeleteExpired and records metrics

before test fixes

Author: andrewpmartinez

controller/env/appenv.go

@@ -229,7 +229,7 @@ func (ae *AppEnv) ValidateAccessToken(token string) (*common.AccessClaims, error
 		return nil, err
 	}
 
-	if revocation != nil && revocation.CreatedAt.After(accessClaims.IssuedAt.AsTime()) {
+	if revocation != nil && revocation.CreatedAt.Truncate(time.Second).After(accessClaims.IssuedAt.AsTime()) {
 		return nil, errors.New("access token has been revoked by identity")
 	}
 

controller/env/security_ctx.go

@@ -26,6 +26,7 @@ import (
 
 	"github.com/openziti/edge-api/rest_model"
 	"github.com/openziti/foundation/v2/errorz"
+	"github.com/openziti/storage/boltz"
 	"github.com/openziti/ziti/v2/common"
 	"github.com/openziti/ziti/v2/controller/model"
 	"github.com/openziti/ziti/v2/controller/models"
@@ -396,6 +397,28 @@ func (ctx *SecurityCtx) resolveOidcSession(securityToken *common.SecurityToken)
 		return
 	}
 
+	// Check if this specific token has been revoked by JWTID.
+	tokenRevocation, err := ctx.env.GetManagers().Revocation.Read(claims.JWTID)
+	if err != nil && !boltz.IsErrNotFoundErr(err) {
+		ctx.setApiSessionError(errorz.NewUnauthorizedOidcInvalid())
+		return
+	}
+	if tokenRevocation != nil {
+		ctx.setApiSessionError(errorz.NewUnauthorizedOidcInvalid())
+		return
+	}
+
+	// Check if the issuing identity has been terminated via a high-water-mark revocation.
+	identityRevocation, err := ctx.env.GetManagers().Revocation.Read(claims.Subject)
+	if err != nil && !boltz.IsErrNotFoundErr(err) {
+		ctx.setApiSessionError(errorz.NewUnauthorizedOidcInvalid())
+		return
+	}
+	if identityRevocation != nil && identityRevocation.CreatedAt.Truncate(time.Second).After(claims.IssuedAt.AsTime()) {
+		ctx.setApiSessionError(errorz.NewUnauthorizedOidcExpired())
+		return
+	}
+
 	ctx.resolvedIdentity = identity
 	ctx.resolvedAuthPolicy = authPolicy
 

controller/internal/policy/revocation_enforcer.go

@@ -0,0 +1,71 @@
+/*
+	Copyright NetFoundry Inc.
+
+	Licensed under the Apache License, Version 2.0 (the "License");
+	you may not use this file except in compliance with the License.
+	You may obtain a copy of the License at
+
+	https://www.apache.org/licenses/LICENSE-2.0
+
+	Unless required by applicable law or agreed to in writing, software
+	distributed under the License is distributed on an "AS IS" BASIS,
+	WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+	See the License for the specific language governing permissions and
+	limitations under the License.
+*/
+
+package policy
+
+import (
+	"time"
+
+	"github.com/michaelquigley/pfxlog"
+	"github.com/openziti/ziti/v2/common/runner"
+	"github.com/openziti/ziti/v2/controller/change"
+	"github.com/openziti/ziti/v2/controller/env"
+)
+
+const (
+	RevocationEnforcerRun    = "revocation.enforcer.run"
+	RevocationEnforcerDelete = "revocation.enforcer.delete"
+	RevocationEnforcerSource = "revocation.enforcer"
+)
+
+// RevocationEnforcer periodically purges expired revocation records from the
+// controller database and router data models.
+type RevocationEnforcer struct {
+	appEnv *env.AppEnv
+	*runner.BaseOperation
+}
+
+// NewRevocationEnforcer creates a RevocationEnforcer that runs at the given frequency.
+func NewRevocationEnforcer(appEnv *env.AppEnv, frequency time.Duration) *RevocationEnforcer {
+	return &RevocationEnforcer{
+		appEnv:        appEnv,
+		BaseOperation: runner.NewBaseOperation("RevocationEnforcer", frequency),
+	}
+}
+
+// Run deletes all expired revocation entries.
+func (e *RevocationEnforcer) Run() error {
+	startTime := time.Now()
+
+	defer func() {
+		e.appEnv.GetMetricsRegistry().Timer(RevocationEnforcerRun).UpdateSince(startTime)
+	}()
+
+	ctx := change.New().SetSourceType(RevocationEnforcerSource).SetChangeAuthorType(change.AuthorTypeController)
+
+	total, err := e.appEnv.GetManagers().Revocation.DeleteExpired(ctx)
+	if err != nil {
+		pfxlog.Logger().WithError(err).Error("failed to delete expired revocations")
+		return nil
+	}
+
+	if total > 0 {
+		pfxlog.Logger().Debugf("removed %d expired revocations", total)
+		e.appEnv.GetMetricsRegistry().Meter(RevocationEnforcerDelete).Mark(int64(total))
+	}
+
+	return nil
+}

controller/model/revocation_manager.go

@@ -17,6 +17,9 @@
 package model
 
 import (
+	"fmt"
+	"time"
+
 	"github.com/openziti/storage/boltz"
 	"github.com/openziti/ziti/v2/common/pb/edge_cmd_pb"
 	"github.com/openziti/ziti/v2/controller/change"
@@ -59,6 +62,41 @@ func (self *RevocationManager) NewModelEntity() *Revocation {
 	return &Revocation{}
 }
 
+const revocationDeleteBatchSize = 500
+
+// DeleteExpired deletes all revocations whose ExpiresAt is in the past, working
+// in batches of revocationDeleteBatchSize until no expired entries remain.
+// Returns the total number of entries deleted.
+func (self *RevocationManager) DeleteExpired(ctx *change.Context) (int, error) {
+	query := fmt.Sprintf(`expiresAt < datetime(%s) limit %d`, time.Now().UTC().Format(time.RFC3339), revocationDeleteBatchSize)
+	total := 0
+	for {
+		result, err := self.BaseList(query)
+		if err != nil {
+			return total, err
+		}
+
+		ids := make([]string, 0, len(result.GetEntities()))
+		for _, entity := range result.GetEntities() {
+			ids = append(ids, entity.GetId())
+		}
+
+		if len(ids) == 0 {
+			break
+		}
+
+		if err = self.deleteEntityBatch(ids, ctx); err != nil {
+			return total, err
+		}
+		total += len(ids)
+
+		if len(ids) < revocationDeleteBatchSize {
+			break
+		}
+	}
+	return total, nil
+}
+
 func (self *RevocationManager) Read(id string) (*Revocation, error) {
 	modelEntity := &Revocation{}
 	if err := self.readEntity(id, modelEntity); err != nil {

controller/oidc_auth/storage.go

@@ -830,7 +830,7 @@ func (s *HybridStorage) TokenRequestByRefreshToken(_ context.Context, refreshTok
 // TerminateSession implements the op.Storage interface
 func (s *HybridStorage) TerminateSession(_ context.Context, identityId string, clientID string) error {
 	now := time.Now()
-	return s.saveRevocation(NewRevocation(identityId+","+clientID, now.Add(s.config.MaxTokenDuration())))
+	return s.saveRevocation(NewRevocation(identityId, now.Add(s.config.MaxTokenDuration())))
 }
 
 // GetRefreshTokenInfo implements the op.Storage interface
@@ -856,6 +856,7 @@ func (s *HybridStorage) RevokeToken(_ context.Context, tokenIDOrToken string, _
 			return oidc.ErrServerError()
 		}
 
+		return nil
 	}
 
 	revocation := NewRevocation(tokenIDOrToken, time.Now().Add(s.config.MaxTokenDuration()))

controller/server/controller.go

@@ -47,10 +47,11 @@ type Controller struct {
 }
 
 const (
-	policyMinFreq     = 1 * time.Second
-	policyMaxFreq     = 1 * time.Hour
-	policyAppWanFreq  = 1 * time.Second
-	policySessionFreq = 5 * time.Second
+	policyMinFreq          = 1 * time.Second
+	policyMaxFreq          = 1 * time.Hour
+	policyAppWanFreq       = 1 * time.Second
+	policySessionFreq      = 5 * time.Second
+	policyRevocationFreq   = 1 * time.Minute
 )
 
 func NewController(host env.HostController) (*Controller, error) {
@@ -202,6 +203,14 @@ func (c *Controller) Initialize() {
 
 	}
 
+	revocationEnforcer := policy.NewRevocationEnforcer(c.AppEnv, policyRevocationFreq)
+	if err := c.policyEngine.AddOperation(revocationEnforcer); err != nil {
+		log.WithField("cause", err).
+			WithField("enforcerName", revocationEnforcer.GetName()).
+			WithField("enforcerId", revocationEnforcer.GetId()).
+			Errorf("could not add revocation enforcer")
+	}
+
 	if err := c.AppEnv.GetStores().EventualEventer.Start(c.AppEnv.GetHostController().GetCloseNotifyChannel()); err != nil {
 		log.WithError(err).Panic("could not start EventualEventer")
 	}

controller/sync_strats/sync_instant.go

@@ -1837,7 +1837,7 @@ func (strategy *InstantStrategy) RevocationUpdate(index uint64, revocation *db.R
 }
 
 func (strategy *InstantStrategy) RevocationDelete(index uint64, revocation *db.Revocation) {
-	strategy.handleRevocation(index, edge_ctrl_pb.DataState_Create, revocation)
+	strategy.handleRevocation(index, edge_ctrl_pb.DataState_Delete, revocation)
 }
 
 func (strategy *InstantStrategy) handlePublicKey(index uint64, action edge_ctrl_pb.DataState_Action, publicKey *edge_ctrl_pb.DataState_PublicKey) {

tests/api_client_management.go

@@ -597,6 +597,22 @@ func (helper *ManagementHelperClient) PatchIdentity(id string, patch *rest_model
 	return helper.GetIdentity(id)
 }
 
+// AuthenticateOidc sets the client to use OIDC, authenticates with the given
+// credentials, and returns the OIDC API session. Returns an error if
+// authentication fails or the session is not an OIDC session.
+func (helper *ManagementHelperClient) AuthenticateOidc(creds edgeApis.Credentials) (*edgeApis.ApiSessionOidc, error) {
+	helper.SetUseOidc(true)
+	apiSession, err := helper.Authenticate(creds, nil)
+	if err != nil {
+		return nil, err
+	}
+	oidcSession, ok := apiSession.(*edgeApis.ApiSessionOidc)
+	if !ok {
+		return nil, fmt.Errorf("expected *edge_apis.ApiSessionOidc, got %T", apiSession)
+	}
+	return oidcSession, nil
+}
+
 func (helper *ManagementHelperClient) GetCurrentApiSessionDetail() (*rest_model.CurrentAPISessionDetail, error) {
 	resp, err := helper.GetCurrentApiSessionDetailResponse()