fix: use opaque dataplane subject ids - #5708
Conversation
|
Hi @gandhipratik203 — thanks for pushing the opaque-subject migration through the publisher, token minting, and Streamable HTTP paths. I reviewed
I reproduced both cases independently. The four focused pytest invocations listed in the PR pass locally, and the full CI pytest run itself passed ( Could you update both identity-resolution paths and add regression tests using the actual UUID-only session payload and a newly minted UUID-sub API token through |
|
Thanks, fixed in This resolves UUID-only session subjects before session team scope, fixes UUID-sub API tokens in Validated with the focused auth/scoping/transport pytest suites plus ruff and |
There was a problem hiding this comment.
Blocking — reverse-proxy ownership still compares UUID to email
This is separate from the resolved require_auth issue: authentication now succeeds, but the downstream ownership identities differ. New API tokens carry the opaque UUID in sub and the signed email in user.email. Reverse-proxy WebSocket authentication goes through get_current_user() and stores the session owner as the resolved email, while the HTTP session endpoints receive the raw payload from require_auth() and _get_user_from_credentials() still selects sub first. For the same non-admin API token, /sessions therefore hides its own session and the disconnect/request/SSE paths return 403 because <uuid> != <email>.
I reproduced this with a UUID-sub payload containing user.email: _get_user_from_credentials() returned the UUID, followed by 403 Not authorized against a session owned by that email.
Would you update the reverse-proxy credential extraction to use the canonical signed-email helper (for example, get_jwt_user_email_from_payload()) and add a regression test covering UUID sub + nested user.email against an email-owned session?
|
Thanks Luca, fixed in Reverse-proxy credential/session-owner extraction now uses the canonical signed-email helper, so UUID Validated with: |
msureshkumar88
left a comment
There was a problem hiding this comment.
Nice fix for the core problem in #5462 — minting the API-token sub from EmailUser.id and keying the Redis UserConfig publisher off that same UUID is the right approach, and the test coverage on the touched files (UUID-with-metadata, UUID-without-metadata, unknown-UUID rejection, reverse-proxy ownership, publisher keying) is solid.
A few things worth resolving before merge — the PR introduces a new canonical pattern for resolving email from a JWT payload (auth_context.get_jwt_user_email_from_payload / resolve_jwt_user_email_from_payload), but a couple of call sites that are just as security-relevant as the ones you did update still run the old logic:
Blocking:
mcpgateway/middleware/csrf_middleware.py(~line 149) — the fallback identity resolution still doespayload.get("sub") or payload.get("email") or ..., which picks the raw UUID before checking signeduser.email. The primary path a few lines up binds CSRF tokens touser.email, so a UUID-sub token hitting the fallback branch could get a CSRF mismatch. Worth swapping toget_jwt_user_email_from_payload()for consistency with the rest of the PR.mcpgateway/auth.py—get_current_user()(the dependency behind Admin UI cookie sessions, CSRF issuance, logout, password-change, and MCPinitialize/ping/websocket) andget_user_email_from_token()still have their own independent UUID→email resolution that doesn't check signeduser.emailmetadata first, and on an unresolved UUID one of them keeps the raw UUID asemailrather than failing closed like the rest of this PR does. Given how central this path is, could these be migrated to the new helper in the same PR rather than left as follow-up?mcpgateway/middleware/token_usage_middleware.py(~line 232) — the new JTI-fallback lookup doesexcept Exception: user_email = Nonewith no log line, unlike the rest of the file. A real DB error here becomes indistinguishable from "token not found" during an incident. Alogger.debug/warningwould help.
Worth a look, not blocking:
verify_credentials._enforce_revocation_and_active_user: whenrequire_user_in_db=Falseand a UUID sub can't be resolved, the function returns before ever checkingis_active— under that config the active-user gate is silently skipped for unverifiable UUID identities. Might be intentional given the existing dev-mode leniency, just flagging.- The Redis TTL cutover for legacy email-sub tokens (documented in the PR body) is a reasonable tradeoff, but a log line when an old-format token is seen post-cutover would make the rollout easier to observe/debug.
Happy to take another pass once these land — the core direction here is good.
|
One more thing worth calling out explicitly: this is a breaking change for existing deployments, not just an internal refactor. Pre-existing API tokens carry This is already disclosed in the PR description's "Migration / Compatibility" section, which is great, but there's no code-level signal for it — no warning log when a legacy email-sub token is used post-cutover, and no dual-publish/migration window to soften the transition. Right now the only way an operator finds out a token needs rotating is via the 400s in production. Given the blast radius (every pre-existing API token routed through the dataplane), could we either:
Not asking to block on a full migration shim — just want to make sure this doesn't surprise anyone in production. |
|
Dug into the actual publisher/config code to size the breaking-change risk more precisely — wanted to correct/sharpen my earlier comment rather than leave a vague "this could break things." There are two separate breaking-change vectors here, with very different blast radii: Vector A — JWT Vector B — Redis For the subset of deployments that do set Suggested mitigation for vector B — dual-publish during a deprecation window, no Rust-side changes needed since the dataplane treats # get_data_from_db() already has both id and email per user_row
return {
key: self._build_user_data(user_email, ...)
for user_email, teams in user_teams_map.items()
for key in (
[user_subject_key_by_email[user_email], user_email]
if settings.dataplane_publisher_dual_key_compat
else [user_subject_key_by_email[user_email]]
)
}Add a For vector A — this reinforces the earlier ask to migrate For both — a rate-limited log line when a legacy email-sub token is seen post-cutover on a dataplane-routed request would turn "users report 400s" into "ops sees a list of tokens that still need rotation." Given |
lucarlig
left a comment
There was a problem hiding this comment.
Thanks — the UUID-sub authentication, token-scoping, and reverse-proxy ownership fixes now look good, and the focused regression suites are green.
Blocking documentation follow-up
Could you update the JWT documentation and examples that still describe sub as the user email? New API tokens now use EmailUser.id (UUID) in sub, retain the human identity in signed user.email, and continue accepting legacy email-sub tokens.
At minimum, the following are stale:
docs/docs/architecture/security-features.mdstill shows"sub": user.emailas the email-auth token shape.mcpgateway/auth.pydocumentssubas “The user unique identifier (email).”
Please also check adjacent user-facing token examples for the same assumption and clarify the UUID-sub versus legacy email-sub behavior. This keeps the documented JWT contract aligned with the implementation.
|
@msureshkumar88 Thanks for flagging the legacy email-sub observability point. Since the dataplane has not been released or deployed yet, there is no live legacy dataplane token population or production cutover to observe. We can omit a post-cutover warning/log in this PR; the documented rotation note is sufficient for pre-release fixtures and local environments. We can revisit rollout-specific telemetry when the dataplane is introduced into a live environment. |
|
Fair enough on the telemetry — agreed it's not worth carrying for a WIP-flagged feature. Two small corrections/asks and then I'm happy:
On vector A — I re-checked and withdraw most of it: |
|
Thanks for the review. I pushed the follow-up changes. What changed:
|
e53565e to
04d350e
Compare
msureshkumar88
left a comment
There was a problem hiding this comment.
Thanks for pushing this through the review rounds — the direction (opaque EmailUser.id subject, signed user.email metadata, and the centralized auth_context.get_jwt_user_email_from_payload/resolve_jwt_user_email_from_payload resolver) is the right shape, and the fail-closed behavior on unresolvable UUID subjects is a real security improvement over the previous "keep the UUID as email" fallback.
Two things on the current head commit (04d350ec6) look like they'd block merge on their own:
Blocking
-
CI is currently red —
pylint (mcpgateway)fails on a new cyclic import.mcpgateway/api/v1/__init__.py:1: R0401: Cyclic import (mcpgateway.auth -> mcpgateway.auth_context).auth_context.pyalready has a module-levelfrom mcpgateway.auth import normalize_token_teams(pre-existing, one-directional onmain). This PR adds the reverse edge via the new function-local imports inauth.py(from mcpgateway.auth_context import resolve_jwt_user_email_from_payload/get_jwt_user_email_from_payload) — pylint's import-graph check tracks these regardless of scope, so# pylint: disable=import-outside-topleveldoesn't suppress it. Could we close the cycle instead of accepting it — e.g. dropauth_context.py's dependency onnormalize_token_teamsfromauth.py, or relocate the new helpers so the dependency only flows one direction? -
.secrets.baselineneeds a regen. The pre-commitdetect-secretshook is failing on CI (pure line-number drift from the diff shifting other files, no new secrets) — just needsmake detect-secrets-scanand the updated baseline committed.
Worth considering (non-blocking, feature is still default-off)
verify_credentials.py::require_admin_auth(~line 1677) still has its own independent UUID-detection logic (hand-rolled regex + directEmailUser.idquery) rather than using the newresolve_jwt_user_email_from_payloadhelper. I didn't find a live bug here — the regex fallback resolves correctly — but it's now the last unconsolidated "is this sub a UUID" implementation in the codebase, and that's exactly the pattern that produced most of the blocking findings earlier in this review. Might be worth folding onto the canonical helper while it's fresh.- No log line yet when a legacy email-sub token is seen post-cutover — agreed this doesn't need to hold up this PR since
dataplane_publisherships default-off, but flagging it as something to land before that flag is promoted toward default-on, given the ~130s global cutover once it's enabled. - A dual-key Redis publish window (UUID + email, gated by a
dataplane_publisher_dual_key_compat-style setting) was discussed earlier in the thread as a way to soften that cutover — same "before wider rollout" caveat as above, not this PR. - Could the rotation-step guidance from the PR description's "Migration / Compatibility" section also get mirrored into
docs/docs/manage/upgrade.md? Right now it only lives in the PR body, which won't be discoverable post-merge.
One more note not really actionable by you: the last LGTM review is showing as DISMISSED against the current head (a test-only commit landed after it), so there's no standing approval on 04d350ec6 right now — probably just needs a re-look once the CI items above are sorted.
Happy to take another pass once the pylint/secrets-baseline items are in.
|
Updated the PR with the CI fixes:
Local checks pass: focused auth/token tests, pylint, and |
62d447e to
cb6fb6c
Compare
|
Re-reviewed the current head ( Both previously-blocking items are resolved:
Ran the PR's full stated unit-test list locally (11 files) — all pass. One thing worth a conscious decision before merge (not blocking, but flagging clearly):
Worth considering: gate the new Suggestions (non-blocking):
Minor:
No Alembic migration needed here and correctly none was added — |
|
Thanks, updated this to make the compatibility scope explicit. The docs now state that newly issued token-catalog API tokens use opaque UUID I also added the explicit fallback assertion in |
msureshkumar88
left a comment
There was a problem hiding this comment.
Ran a full live end-to-end validation against a real running gateway (not just the unit suites) to close out the open review rounds. Summary below — everything checked out, approving.
Environment
Isolated stack, nothing shared with any other deployment:
- Gateway:
uv run uvicorn mcpgateway.main:app --host 127.0.0.1 --port 14444 - Redis: fresh
redis:latestcontainer, port16379 - DB: fresh sqlite file (throwaway)
- Config:
DATAPLANE_PUBLISHER=true,DATAPLANE_PUBLISHER_INTERVAL_SECONDS=3(fast publish cycle),AUTH_REQUIRED=true,PUBLIC_REGISTRATION_ENABLED=true,MCPGATEWAY_ADMIN_API_ENABLED=true,PLUGINS_ENABLED=false,JWT_SECRET_KEY/AUTH_ENCRYPTION_SECRETas generated 32+ char test secrets
What was checked and what came back
1. Core claim shape — Registered a real user, minted a token through POST /tokens. Decoded payload:
{"sub": "4d379145-8871-4d04-a4b2-0ae9c52c6516",
"user": {"email": "e2e-pr5708-user@example.com", "is_admin": false},
"teams": ["a32331cebb19407d9557f9f94dd3e0e3"]}sub is the opaque EmailUser.id UUID, user.email carries the human identity. ✅
2. Round 1 (session/API-token scoping through require_auth) — GET /admin/well-known with the UUID-sub token → 200 (previously 401'd here pre-fix). ✅
3. Team-scoping, positive + negative control — Token sees a server owned by its own team; a second unrelated user/team's server is correctly excluded from the same token's GET /servers. Confirms real filtering, not open visibility. ✅
4. Round 2 (reverse-proxy ownership) — GET /v1/reverse-proxy/sessions executes cleanly on a UUID-sub payload (no crash in _get_user_from_credentials/get_jwt_user_email_from_payload). Couldn't get a live WebSocket session established in this from-scratch config (both session and API tokens 403'd identically at the ASGI layer with no app-level log — looks like an environment/config gate, not something this PR introduced, since it hits the pre-existing session-token path too). Falling back to tests/unit/mcpgateway/routers/test_reverse_proxy.py -q: 80/80 passed. ✅ (flagging this as unit-test corroboration rather than full live coverage, in the interest of transparency)
5. Backward compatibility — Hand-signed a legacy sub=<email> JWT with the same secret, hit a plain authenticated route → 200, log confirms ✓ Authenticated user: e2e-pr5708-user@example.com. Legacy tokens keep working as documented. ✅
6. Dataplane publisher UUID-keying — After ~15s (5 publish cycles), Redis holds only UUID-keyed UserConfig entries, no email-keyed ones. ✅
7. Cyclic import / secrets baseline (round 5) — pylint --enable=cyclic-import on auth.py/auth_context.py/api/v1/__init__.py scores clean, no R0401 — confirmed auth_context.py no longer imports from auth.py at all (only the reverse edge remains). .secrets.baseline diff is timestamp-only. ✅
8. Regression sweep — test_token_catalog_service.py, test_token_scoping.py, test_dataplane_publisher.py, test_streamablehttp_transport.py, test_reverse_proxy.py, test_csrf_middleware.py, test_csrf_fixes.py, plus test_auth.py and a broader -k "auth or csrf or admin" sweep across tests/unit/mcpgateway/ — all green, only expected environment-conditional skips (no Postgres test DB, plugins/UI disabled in this harness). ✅
Teardown confirmed clean — no containers, ports, or processes left behind from this run.
One non-blocking note
The current head has a merge conflict against main (.env.example, admin.py CSRF cookie naming, admin_ui/llmModels.js) — unrelated to this PR's own changes, main has just moved on. Worth a rebase before merge, but not something I'd hold the review on.
Approving — the opaque-subject migration, the fail-closed UUID resolution, and the backward-compat path all hold up under live verification, and nothing in the surrounding auth/CSRF/admin code broke.
Signed-off-by: Pratik Gandhi <gandhipratik203@gmail.com>
Signed-off-by: Pratik Gandhi <gandhipratik203@gmail.com>
Signed-off-by: Pratik Gandhi <gandhipratik203@gmail.com>
Signed-off-by: Pratik Gandhi <gandhipratik203@gmail.com>
Signed-off-by: Pratik Gandhi <gandhipratik203@gmail.com>
Signed-off-by: Pratik Gandhi <gandhipratik203@gmail.com>
Signed-off-by: Pratik Gandhi <gandhipratik203@gmail.com>
Signed-off-by: Pratik Gandhi <gandhipratik203@gmail.com>
Signed-off-by: Pratik Gandhi <gandhipratik203@gmail.com>
Signed-off-by: Pratik Gandhi <gandhipratik203@gmail.com>
Signed-off-by: Pratik Gandhi <gandhipratik203@gmail.com>
8dc023b to
5fc641e
Compare
…sport interrogate --fail-under 100 blocked merge queue; sibling copies in auth.py, verify_credentials.py, and token_scoping.py already had it. Signed-off-by: Suresh Kumar Moharajan <suresh.kumar.m@ibm.com>
Summary
EmailUser.idinstead of email addresses.EmailUser.idwhile retaining signeduser.emailmetadata.Notes
This is the control-plane portion of the dataplane subject migration. Part of #5462.
Matching Rust dataplane/demo fixture updates are open in contextforge-org/contextforge-data-plane#61.
Migration / Compatibility
Redis user configs are now keyed by the opaque user UUID instead of email. Existing email-keyed Redis entries are not migrated; they expire through the normal dataplane publisher TTL.
Session tokens already use UUID subjects, so they continue to match UUID-keyed configs.
Pre-existing API tokens minted before this change may still carry email subjects. Those tokens must be rotated or recreated during rollout. After the old email-keyed Redis entry expires, an email-sub token no longer matches a published config and dataplane requests can fail with HTTP 400:
Problem occurred retrieving the configurationuntil the token is replaced.No Rust dataplane fallback is added because the dataplane treats JWT
subas an opaque lookup key and does not own email-to-UUID identity mapping.Validation Performed
Python unit coverage:
uv run pytest tests/unit/mcpgateway/services/test_token_catalog_service.py \ tests/unit/mcpgateway/middleware/test_token_scoping.py \ tests/unit/mcpgateway/services/test_dataplane_publisher.py -q uv run pytest tests/unit/mcpgateway/transports/test_streamablehttp_transport.py -q uv run pytest tests/unit/mcpgateway/test_auth_context_email_precedence.py \ tests/unit/mcpgateway/middleware/test_token_usage_middleware.py \ tests/unit/mcpgateway/middleware/test_token_scoping.py -q uv run pytest tests/unit/mcpgateway/test_auth.py -k "uuid_sub or api_token" -q uv run ruff check mcpgateway/services/dataplane_publisher.py \ mcpgateway/services/token_catalog_service.py \ mcpgateway/middleware/token_scoping.py \ mcpgateway/auth_context.py \ mcpgateway/transports/streamablehttp_transport.py \ mcpgateway/middleware/token_usage_middleware.py \ tests/unit/mcpgateway/services/test_dataplane_publisher.py \ tests/unit/mcpgateway/services/test_token_catalog_service.py \ tests/unit/mcpgateway/middleware/test_token_scoping.py \ tests/unit/mcpgateway/test_auth_context_email_precedence.py \ tests/unit/mcpgateway/transports/test_streamablehttp_transport.py \ tests/unit/mcpgateway/middleware/test_token_usage_middleware.py git diff --checkCross-repo dataplane smoke test:
cd ../contextforge-gateway-rs docker compose -f docker/docker-compose-local.yaml up -d redis gateway-one11111111-1111-1111-1111-111111111111c0ffee00f001f00lf00ldeadbeefdeadhttp://127.0.0.1:5555/mcpsub:11111111-1111-1111-1111-111111111111user.email:admin@example.comResults and confirmation points:
The smoke script stdout confirmed the Python-side Redis write and MCP responses:
The smoke script also asserted that no email-keyed Redis config existed for
admin@example.com, so the published key was UUID-only.The Rust dataplane process logs confirmed that the JWT
subwas used as the Redis lookup key and that the config loaded successfully:So the result was confirmed in two places:
tools/listcompleted.Semi-automated cross-repo smoke script
This is opt-in/manual validation. It uses one Redis container, one sample backend container (
gateway-one), and one local Rust dataplane process.Start the Rust-side services:
In another terminal, from this Python repo branch:
Expected output:
Cleanup:
Live ContextForge protocol/RBAC validation:
A fresh clone of this PR branch was built and run on
test-vm-cf.Environment:
issue-5462-dataplane-uuid-subjectshttp://127.0.0.1:8080x-contextforge-mcp-transport-mounted: python)mcpgateway/mcpgateway:latestbuilt from this PR branchCommands:
Result:
Result:
Note: the first protocol run exposed stale/incomplete test-stack setup:
fast-time-*tools were missing from the live catalog. After rerunning theregister_fast_timesetup job, the catalog contained bothfast-time-*andfast-test-*tools, and the full protocol suite passed.Single-instance Playwright token smoke
Ran against the rebased PR head
5fc641e791ontest-vm-cf, using a singlemake devContextForge instance athttp://localhost:8000.Setup:
v22.23.1/ npm10.9.8on the VM so the admin UI bundle could be built.make js-build.make dev.GET /healthreturned200.Command shape:
Result:
This covers API token create/list/update/revoke, using newly generated API tokens as bearer tokens, scoped-token permission enforcement, revocation enforcement, and the admin UI token revoke flow.