feat(mcp): add internal OIDC issuer for MCP authentication#2457
Open
feat(mcp): add internal OIDC issuer for MCP authentication#2457
Conversation
Replace the MCP server's dependency on the main app's external BYO OIDC IdP with a Tracecat-owned internal OIDC issuer. The issuer authenticates users via existing Tracecat session cookies and issues short-lived JWT access tokens scoped to a single organization. Endpoints mounted at /api/mcp-oidc: - /.well-known/openid-configuration and /.well-known/jwks.json - /authorize (auth-code + PKCE flow with session resolution) - /authorize/resume (post-login/org-selection continuation) - /token (code exchange with PKCE, IP binding, rate limiting) - /userinfo (Bearer token introspection) Security hardening: - Ed25519 signing keys derived from USER_AUTH_SECRET via HKDF - Fernet-encrypted Redis storage for auth codes and resume transactions - Auth code and resume transaction IP binding - PKCE S256 only (plain rejected), one-time-use codes - Constant-time client secret comparison, Content-Type enforcement - Per-IP rate limiting on /token, JTI tracking for revocation support - Redirect URI strict allow-list (single MCP callback) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Replace get_platform_oidc_config() with the internal issuer's derived client credentials and discovery URL. This removes the MCP server's dependency on OIDC_ISSUER, OIDC_CLIENT_ID, and OIDC_CLIENT_SECRET environment variables. Key changes in tracecat/mcp/auth.py: - _create_oidc_auth() now uses INTERNAL_CLIENT_ID and HKDF-derived secret - _required_scopes simplified to [openid, profile, email] (no offline_access) - _extract_upstream_claims() decodes internal issuer JWT directly - OIDCProxy config_url uses internal API URL for server-to-server comms Mount the mcp_oidc_router in tracecat/api/app.py (excluded from OpenAPI schema since these endpoints are only consumed by the FastMCP proxy). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Add /mcp-auth/continue and /mcp-auth/select-org pages for the internal OIDC issuer's login redirect flow: - /mcp-auth/continue: Redirects to sign-in if no session, or resumes the OIDC authorize flow if already authenticated - /mcp-auth/select-org: Org picker for platform superadmins in multi-tenant mode, sets tracecat-org-id cookie before resuming Add test cases confirming /mcp-auth/* paths are allowed by sanitizeReturnUrl() (not blocked by the /auth prefix filter). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Replace OIDC_ISSUER, OIDC_CLIENT_ID, OIDC_CLIENT_SECRET env vars on the MCP service with USER_AUTH_SECRET, TRACECAT__API_URL, and TRACECAT__PUBLIC_API_URL for the internal OIDC issuer. Docker Compose (dev, local, production): - Add USER_AUTH_SECRET, TRACECAT__API_URL, TRACECAT__PUBLIC_API_URL - Remove OIDC_ISSUER, OIDC_CLIENT_ID, OIDC_CLIENT_SECRET, OIDC_SCOPES - Add api service as a dependency (MCP needs issuer discovery at startup) Fargate ECS: - Replace OIDC_ISSUER/OIDC_SCOPES with TRACECAT__API_URL/PUBLIC_API_URL - Replace oidc_client_id/oidc_client_secret secrets with user_auth_secret Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
topher-lo
temporarily deployed
to
internal-registry-ci
GitHub Actions
Inactive
topher-lo
temporarily deployed
to
internal-registry-ci
GitHub Actions
Inactive
|
✅ No security or compliance issues detected. Reviewed everything up to 0ce97eb. Security Overview
Detected Code ChangesThe diff is too large to display a summary of code changes. |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
5 issues found across 17 files
Confidence score: 2/5
- High risk security posture:
USER_AUTH_SECRETcan be empty intracecat/mcp/oidc/signing.pyandtracecat/mcp/oidc/config.py, yielding predictable keys/secrets that are attacker-reproducible. - Concurrency and token handling issues could cause user-facing auth failures:
tracecat/mcp/oidc/storage.pyallows double redemption of auth codes, and malformedid_tokencan 500 intracecat/mcp/auth.py. - Given multiple high-severity issues with clear auth impact, this is risky to merge without fixes.
- Pay close attention to
tracecat/mcp/oidc/signing.py,tracecat/mcp/oidc/config.py,tracecat/mcp/oidc/storage.py,tracecat/mcp/oidc/endpoints.py,tracecat/mcp/auth.py- authentication correctness and security edge cases.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="tracecat/mcp/oidc/storage.py">
<violation number="1" location="tracecat/mcp/oidc/storage.py:117">
P1: Non-atomic get-then-delete allows an auth code to be redeemed twice under concurrent requests, violating the one-time-use guarantee (RFC 6749 §10.5). The same pattern also affects `load_and_delete_resume_transaction`. Use Redis `GETDEL` (available in redis-py 7.1.0) instead of separate `get` + `delete` calls to make this truly atomic.</violation>
</file>
<file name="tracecat/mcp/oidc/endpoints.py">
<violation number="1" location="tracecat/mcp/oidc/endpoints.py:267">
P2: `state` is not URL-encoded before being interpolated into the redirect URL. If the OAuth client sends a `state` containing `&`, `=`, or `#`, the redirect URL is malformed and the callback will fail or lose the state value. Use `urllib.parse.urlencode` to construct the query string safely.</violation>
</file>
<file name="tracecat/mcp/auth.py">
<violation number="1" location="tracecat/mcp/auth.py:784">
P2: Malformed `id_token` from the internal issuer will raise an unhandled exception (e.g. `IndexError`, `ValueError`) from `_decode_unverified_id_token_claims`, bypassing the fallback path and causing a 500 on the token endpoint. Wrap the id_token decoding block in a try/except so it gracefully falls through to the `_resolve_idp_email` fallback.</violation>
</file>
<file name="tracecat/mcp/oidc/signing.py">
<violation number="1" location="tracecat/mcp/oidc/signing.py:43">
P0: Deriving the Ed25519 signing key from `USER_AUTH_SECRET` without validating it is non-empty means a missing or empty env var silently produces a predictable, attacker-reproducible key. Add a guard to raise early if the secret is blank.</violation>
</file>
<file name="tracecat/mcp/oidc/config.py">
<violation number="1" location="tracecat/mcp/oidc/config.py:30">
P1: No guard against empty `USER_AUTH_SECRET` — HKDF will silently derive a predictable, publicly-known client secret from the empty string default. Add an early check to fail loudly rather than running with a trivially guessable secret.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
tracecat/mcp/oidc/signing.py
Outdated
| keypair, so all replicas share one signing identity without key | ||
| distribution. | ||
| """ | ||
| seed = _derive_seed(USER_AUTH_SECRET) |
Contributor
There was a problem hiding this comment.
P0: Deriving the Ed25519 signing key from USER_AUTH_SECRET without validating it is non-empty means a missing or empty env var silently produces a predictable, attacker-reproducible key. Add a guard to raise early if the secret is blank.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At tracecat/mcp/oidc/signing.py, line 43:
<comment>Deriving the Ed25519 signing key from `USER_AUTH_SECRET` without validating it is non-empty means a missing or empty env var silently produces a predictable, attacker-reproducible key. Add a guard to raise early if the secret is blank.</comment>
<file context>
@@ -0,0 +1,144 @@
+ keypair, so all replicas share one signing identity without key
+ distribution.
+ """
+ seed = _derive_seed(USER_AUTH_SECRET)
+ return Ed25519PrivateKey.from_private_bytes(seed)
+
</file context>
| """ | ||
| store = _get_store() | ||
| key = _code_key(code) | ||
| raw = await store.get(key) |
Contributor
There was a problem hiding this comment.
P1: Non-atomic get-then-delete allows an auth code to be redeemed twice under concurrent requests, violating the one-time-use guarantee (RFC 6749 §10.5). The same pattern also affects load_and_delete_resume_transaction. Use Redis GETDEL (available in redis-py 7.1.0) instead of separate get + delete calls to make this truly atomic.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At tracecat/mcp/oidc/storage.py, line 117:
<comment>Non-atomic get-then-delete allows an auth code to be redeemed twice under concurrent requests, violating the one-time-use guarantee (RFC 6749 §10.5). The same pattern also affects `load_and_delete_resume_transaction`. Use Redis `GETDEL` (available in redis-py 7.1.0) instead of separate `get` + `delete` calls to make this truly atomic.</comment>
<file context>
@@ -0,0 +1,187 @@
+ """
+ store = _get_store()
+ key = _code_key(code)
+ raw = await store.get(key)
+ if raw is None:
+ return None
</file context>
| length=32, | ||
| salt=b"tracecat-mcp-oidc-client-secret-v1", | ||
| info=b"client-secret", | ||
| ).derive(USER_AUTH_SECRET.encode("utf-8")) |
Contributor
There was a problem hiding this comment.
P1: No guard against empty USER_AUTH_SECRET — HKDF will silently derive a predictable, publicly-known client secret from the empty string default. Add an early check to fail loudly rather than running with a trivially guessable secret.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At tracecat/mcp/oidc/config.py, line 30:
<comment>No guard against empty `USER_AUTH_SECRET` — HKDF will silently derive a predictable, publicly-known client secret from the empty string default. Add an early check to fail loudly rather than running with a trivially guessable secret.</comment>
<file context>
@@ -0,0 +1,73 @@
+ length=32,
+ salt=b"tracecat-mcp-oidc-client-secret-v1",
+ info=b"client-secret",
+ ).derive(USER_AUTH_SECRET.encode("utf-8"))
+ return base64.urlsafe_b64encode(secret_bytes).decode("ascii")
+
</file context>
| client_ip=_get_client_ip(request), | ||
| ) | ||
|
|
||
| redirect_url = f"{redirect_uri}?code={code}&state={state}" |
Contributor
There was a problem hiding this comment.
P2: state is not URL-encoded before being interpolated into the redirect URL. If the OAuth client sends a state containing &, =, or #, the redirect URL is malformed and the callback will fail or lose the state value. Use urllib.parse.urlencode to construct the query string safely.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At tracecat/mcp/oidc/endpoints.py, line 267:
<comment>`state` is not URL-encoded before being interpolated into the redirect URL. If the OAuth client sends a `state` containing `&`, `=`, or `#`, the redirect URL is malformed and the callback will fail or lose the state value. Use `urllib.parse.urlencode` to construct the query string safely.</comment>
<file context>
@@ -0,0 +1,565 @@
+ client_ip=_get_client_ip(request),
+ )
+
+ redirect_url = f"{redirect_uri}?code={code}&state={state}"
+ return RedirectResponse(redirect_url, status_code=302)
+
</file context>
Comment on lines
+784
to
+786
| if isinstance(id_token, str) and id_token: | ||
| claims = _decode_unverified_id_token_claims(id_token) | ||
| if email := _normalize_email_claim(claims.get("email")): |
Contributor
There was a problem hiding this comment.
P2: Malformed id_token from the internal issuer will raise an unhandled exception (e.g. IndexError, ValueError) from _decode_unverified_id_token_claims, bypassing the fallback path and causing a 500 on the token endpoint. Wrap the id_token decoding block in a try/except so it gracefully falls through to the _resolve_idp_email fallback.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At tracecat/mcp/auth.py, line 784:
<comment>Malformed `id_token` from the internal issuer will raise an unhandled exception (e.g. `IndexError`, `ValueError`) from `_decode_unverified_id_token_claims`, bypassing the fallback path and causing a 500 on the token endpoint. Wrap the id_token decoding block in a try/except so it gracefully falls through to the `_resolve_idp_email` fallback.</comment>
<file context>
@@ -775,40 +774,38 @@ async def _retry_without_refresh_scope(
+ decode the id_token to extract claims.
+ """
+ id_token = idp_tokens.get("id_token")
+ if isinstance(id_token, str) and id_token:
+ claims = _decode_unverified_id_token_claims(id_token)
+ if email := _normalize_email_claim(claims.get("email")):
</file context>
Suggested change
| if isinstance(id_token, str) and id_token: | |
| claims = _decode_unverified_id_token_claims(id_token) | |
| if email := _normalize_email_claim(claims.get("email")): | |
| if isinstance(id_token, str) and id_token: | |
| try: | |
| claims = _decode_unverified_id_token_claims(id_token) | |
| except Exception: | |
| claims = {} | |
| if email := _normalize_email_claim(claims.get("email")): |
Contributor
|
Found 1 test failure on Blacksmith runners: Failure
|
Fix 5 issues found during E2E QA with Codex and Claude Code: - Switch signing from EdDSA to ES256 for FastMCP JWTVerifier compat - Make `resource` param optional (FastMCP doesn't forward RFC 8707) - Use request origin for discovery token/userinfo/jwks endpoints - Remove IP binding on token endpoint (container IPs always differ) - Fix Caddy routing so /mcp-auth/* goes to UI not MCP container Add comprehensive test suite (93 tests): - Fix broken tests in test_mcp_auth.py (internal issuer mocks) - New test_mcp_oidc_signing.py (10 tests: key derivation, JWT ops) - New test_mcp_oidc_storage.py (10 tests: Redis store, rate limit) - New test_mcp_oidc_endpoints.py (29 tests: all OIDC endpoints) - New test_mcp_oidc_flow.py (3 tests: E2E authorize-code-PKCE) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
topher-lo
temporarily deployed
to
internal-registry-ci
GitHub Actions
Inactive
topher-lo
had a problem deploying
to
internal-registry-ci
GitHub Actions
Failure
Contributor
Author
QA Results: Internal OIDC Issuer E2E TestingTested the full MCP OAuth flow end-to-end with Codex CLI and Claude Code. Found and fixed 5 compatibility issues. Issues Found & Fixed
Known Gotchas for Future Reference
Test Coverage Added (93 tests)
|
Contributor
There was a problem hiding this comment.
2 issues found across 9 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="tests/unit/test_mcp_oidc_endpoints.py">
<violation number="1" location="tests/unit/test_mcp_oidc_endpoints.py:60">
P2: `_make_auth_code_data` ignores the provided `verifier` and generates an unrelated PKCE challenge, producing mismatched verifier/challenge pairs.</violation>
</file>
<file name="tracecat/mcp/oidc/endpoints.py">
<violation number="1" location="tracecat/mcp/oidc/endpoints.py:136">
P2: The `jwks()` docstring still references "Ed25519" but the signing algorithm was changed to ECDSA P-256 (ES256). This is misleading and should be updated to match.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| **kwargs, | ||
| ) -> AuthCodeData: | ||
| if verifier and not challenge: | ||
| _, challenge = verifier, None |
Contributor
There was a problem hiding this comment.
P2: _make_auth_code_data ignores the provided verifier and generates an unrelated PKCE challenge, producing mismatched verifier/challenge pairs.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At tests/unit/test_mcp_oidc_endpoints.py, line 60:
<comment>`_make_auth_code_data` ignores the provided `verifier` and generates an unrelated PKCE challenge, producing mismatched verifier/challenge pairs.</comment>
<file context>
@@ -0,0 +1,802 @@
+ **kwargs,
+) -> AuthCodeData:
+ if verifier and not challenge:
+ _, challenge = verifier, None
+ if not challenge:
+ _, challenge = _pkce_pair()
</file context>
| "jwks_uri": f"{request_issuer}/.well-known/jwks.json", | ||
| "response_types_supported": ["code"], | ||
| "subject_types_supported": ["public"], | ||
| "id_token_signing_alg_values_supported": ["ES256"], |
Contributor
There was a problem hiding this comment.
P2: The jwks() docstring still references "Ed25519" but the signing algorithm was changed to ECDSA P-256 (ES256). This is misleading and should be updated to match.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At tracecat/mcp/oidc/endpoints.py, line 136:
<comment>The `jwks()` docstring still references "Ed25519" but the signing algorithm was changed to ECDSA P-256 (ES256). This is misleading and should be updated to match.</comment>
<file context>
@@ -112,18 +112,28 @@ def _parse_basic_auth(authorization: str) -> tuple[str, str] | None:
"response_types_supported": ["code"],
"subject_types_supported": ["public"],
- "id_token_signing_alg_values_supported": ["EdDSA"],
+ "id_token_signing_alg_values_supported": ["ES256"],
"scopes_supported": ["openid", "profile", "email"],
"token_endpoint_auth_methods_supported": [
</file context>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
/api/mcp-oidc) that replaces the MCP server's dependency on an external BYO OIDC IdPtracecat/mcp/auth.pyto use the internal issuer's derived credentials and discovery URL, removing the need forOIDC_ISSUER,OIDC_CLIENT_ID, andOIDC_CLIENT_SECRETon the MCP service/mcp-auth/continueand/mcp-auth/select-orgfrontend routes for the login redirect and superadmin org picker flowsUSER_AUTH_SECRETandTRACECAT__API_URLinstead of OIDC credentialsSecurity hardening
USER_AUTH_SECRETvia HKDF/token/token, JTI tracking for future revocationTest plan
OIDC_ISSUERenv var/mcp-auth/continueredirects to sign-in and resumes correctly/mcp-auth/select-orgsets org cookie and resumes for superadmins🤖 Generated with Claude Code
Summary by cubic
Adds a Tracecat-owned internal OIDC issuer for MCP auth and rewires the MCP server to use it, removing the external OIDC dependency. Also fixes FastMCP compatibility (ES256 signing, optional resource, dynamic discovery base) and updates routing.
New Features
/api/mcp-oidc(discovery, JWKS,authorize+authorize/resume,tokenwith PKCE S256,userinfo; endpoints resolve against the request origin).OIDC_*env needed on the MCP service./mcp-auth/continue(login resume) and/mcp-auth/select-org(superadmin org picker).USER_AUTH_SECRET(HKDF), Fernet-encrypted Redis for codes/transactions, one‑time codes, constant‑time secret checks, per‑IP rate limits, strict redirect allow‑list.resourceis optional,/tokenno longer enforces IP binding, and Caddy routing ensures/mcp-auth/*goes to the UI.Migration
OIDC_ISSUER,OIDC_CLIENT_ID,OIDC_CLIENT_SECRET,OIDC_SCOPES. AddUSER_AUTH_SECRET,TRACECAT__API_URL,TRACECAT__PUBLIC_API_URLfor MCP.apia dependency of MCP; in ECS, passUSER_AUTH_SECRETand API URLs instead of OIDC secrets./mcp-auth/select-org(sets the org cookie).Written for commit 0ce97eb. Summary will update on new commits.