Skip to content

fix(proxy): revoke all cached tokens during backchannel logout - #3477

Open
zerox80 wants to merge 7 commits into
opencloud-eu:mainfrom
zerox80:codex/oidc-session-logout
Open

fix(proxy): revoke all cached tokens during backchannel logout#3477
zerox80 wants to merge 7 commits into
opencloud-eu:mainfrom
zerox80:codex/oidc-session-logout

Conversation

@zerox80

@zerox80 zerox80 commented Sep 7, 2026

Copy link
Copy Markdown
Contributor

Description

When a session receives a refreshed access token, its cache lookup currently replaces the previous token's hash. Backchannel logout can therefore leave older cached tokens usable. Deleting cached claims also allows a locally verified JWT to authenticate again when PROXY_OIDC_SKIP_USER_INFO is enabled.

Register each accepted token separately and retain revocations independently of cached claims. Logout invalidates every tracked token matching the requested subject or session. Authentication checks revocation before using a token and after registering new claims, so a concurrent claims write cannot restore a logged-out token. Notification failures no longer prevent invalidation.

Preserve per-record expiry across memory, Redis, and NATS using a dedicated OIDC cache namespace. Import unexpired legacy claims on startup and clean up expired records. Empty legacy NATS buckets require no migration. Physically purge NATS delete markers within the configured table and through the observed revision, preserving concurrent writes. This prevents a bucket-wide TTL from dropping revocations before their tokens expire while keeping expired state from accumulating.

Commit sequence

  1. Support per-token logout records. Introduce compatible lookup and revocation keys, support multiple tokens per session, and add the matching unit tests.
  2. Preserve OIDC cache expiry across storage backends. Add the cache wrapper, legacy migration, startup wiring, cleanup, and memory/Redis/NATS tests.
  3. Revoke every tracked token on backchannel logout. Connect authentication and logout to the new records, with HTTP and concurrency regression tests.
  4. Document logout persistence and cache upgrades. Explain operational behavior and add the changelog entry.
  5. Accept empty NATS caches during migration. Handle new, expired, and marker-only buckets without hiding connection failures; include native NATS regression tests.
  6. Remove NATS delete markers. Add scoped cleanup bounded by the observed revision, using the cache authentication and TLS settings, with storage and concurrency regression tests.

Tests are included with the behavior they validate. The first four commits split the original patch; the final two address the NATS startup and cleanup review findings.

Related Issue

No issue linked.

Motivation and Context

Backchannel logout must invalidate all tokens already accepted for the affected session, including tokens issued before a refresh and tokens verified without a userinfo request.

Revocations are retained until the verified token expiry. Tokens without a verified expiry, including migrated legacy entries, retain logout state indefinitely. All proxy instances must use the updated code and the same persistent store to share revocations; memory-backed state is lost on restart. The proxy documentation explains the cache namespace and upgrade behavior.

How Has This Been Tested?

Test environment: Ubuntu under WSL, Go 1.25.9, vendored dependencies, Redis 7.0.15, and the vendored NATS server.

  • Six HTTP regression cases fail on the unmodified base with HTTP 200 instead of HTTP 401, and pass with the fix. They cover subject-only, session-only, and combined logout, with userinfo enabled and disabled.
  • Additional tests cover concurrent claims writes, storage failures, repeated logout, unrelated sessions, disabled claims caching, absent token expiry, legacy migration, expiration cleanup, and more than 1,000 cache entries.
  • Tests with temporary Redis and NATS instances verify cache compatibility and shared revocations. NATS tests confirm that revocations survive the legacy bucket TTL.
  • New NATS regressions reproduce both review findings before the fixes. They verify empty-cache startup, actual stream message counts after cleanup, concurrent key reuse, isolation between tables and buckets, authenticated maintenance, retry after failure, and cancellation.
  • Proxy and OIDC tests pass, including race checks. Production identity providers and Redis Sentinel failover are not covered by these tests.
go test -mod=vendor -p 4 -short -timeout 3m -v ./services/proxy/... ./pkg/oidc/...
go test -mod=vendor -p 4 -race -short -timeout 3m -v ./services/proxy/pkg/command ./services/proxy/pkg/middleware ./services/proxy/pkg/staticroutes/... ./pkg/oidc/...
go test -mod=vendor -p 4 -race -short -timeout 3m -v ./services/proxy/... ./pkg/oidc/...

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Technical debt
  • Tests only (no source changes)

Checklist

  • Code changes
  • Unit tests added
  • Acceptance tests added
  • Documentation added

@codacy-production

codacy-production Bot commented Sep 7, 2026

Copy link
Copy Markdown

Not up to standards ⛔

🔴 Issues 1 critical

Alerts:
⚠ 1 issue (≤ 0 issues of at least minor severity)

Results:
1 new issue

Category Results
Security 1 critical

View in Codacy

🟢 Metrics 283 complexity

Metric Results
Complexity 283

View in Codacy

NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.

Add token-specific session keys and separate revocation keys. Keep the legacy
subject/session key format readable while allowing multiple tokens to belong
to one session. Match both subject and session when both are supplied and
avoid truncating logout lookups after 1,000 cache entries.

Include unit tests for key compatibility, multiple tokens, and subject
isolation. Authentication starts using these records in the logout fix.
Store absolute record expiry with the value so Redis and NATS retain the
same lifetime information as memory stores. Use a dedicated OIDC namespace
without a bucket-wide TTL, migrate legacy cached tokens before startup,
and clean up expired records with the proxy context.

Keep logout state when claims caching is disabled. Add expiry, migration,
cleanup, and memory/Redis/NATS backend tests alongside the cache changes.
Register tokens synchronously before caching claims and check revocation
before authentication and after registration. Revoke every token matching
the logout subject or session independently of its cached claims, preserving
the original token expiry. Notification failures must not skip invalidation.

Add HTTP regressions for refreshes and userinfo bypass, concurrent claims
writes, disabled claims caching, missing expiry, and storage failures.
Document the dedicated OIDC namespace, legacy cache migration, shared-store
requirements, expiry cleanup, and retention for tokens without verified
expiry. Add the changelog entry for the complete backchannel logout fix.
@zerox80
zerox80 force-pushed the codex/oidc-session-logout branch from 32eaef5 to 6c4da8b Compare September 7, 2026 07:46
@zerox80

zerox80 commented Sep 7, 2026

Copy link
Copy Markdown
Contributor Author

Could you mark this finding as a false positive and rerun Codacy? It occurs in an integration test: the executable name is fixed to redis-server, arguments are passed without a shell, and the socket path comes from t.TempDir(). No untrusted request input reaches this call.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant