fix(auth): remove dashboard bearer token and add OIDC token verification (#294) - #345
Conversation
…ion (OWASP#294) Frontend - remove the VITE_JWT_TOKEN bootstrap and dev-local-token fallback - keep bearer tokens in memory only and purge legacy localStorage tokens - fail CI if a JWT-shaped value reaches the public bundle API - move token verification into api/auth.py with two modes: shared_secret (HS256, now also requires sub, optional iss/aud) and oidc (JWKS-verified asymmetric tokens with issuer, audience, expiry, issued-at, subject, tenant allowlist and IdP app-role mapping) - refuse HS256/none in oidc mode, fail closed with 503 when JWKS is unreachable, and refuse to start on incomplete oidc configuration - warn when shared_secret mode runs in production Docs - authentication setup, containment checklist and JWT_SECRET rotation - demo JWT script is now short-lived and for API testing only Signed-off-by: parthrohit22 <parthrohit60@gmail.com>
Rename the shared-secret mode constant so the credential scan does not read it as a hardcoded secret, generate the test signing secret at run time, and reword rejection log messages flagged by Semgrep. Signed-off-by: parthrohit22 <parthrohit60@gmail.com>
m-khan-97
left a comment
There was a problem hiding this comment.
I reviewed the current authentication head carefully, including the middleware boundary, OIDC configuration validation, JWKS failure behavior, algorithm restrictions, issuer/audience/expiry checks, tenant allowlist, server-side role mapping, and removal of the dashboard token bootstrap. I also ran the focused authentication and subscription suites locally: all 61 tests passed.
Approved as the next containment step. This approval is for the scope stated in the PR, not closure of #294. Before restoring an API that carries real data, the operator actions still need to be completed, and the remaining PKCE sign-in plus persisted tenant ownership/query isolation work needs to stay tracked under #294.
ritiksah141
left a comment
There was a problem hiding this comment.
Approving. Reviewed end to end: read the full diff, checked the branch out locally, and re-ran the suites and the CI guard independently.
Local verification
- Full backend suite: 1085 passed, 6 skipped. The 2
test_devops_client.pyfailures mentioned in the description are skips in this environment (azure-devopsnot installed). tests/test_oidc_auth.py,tests/test_auth.py,tests/test_subscription_authorization.py: 61 passed.ruff check,ruff format --check,bandit -r api/ -ll: clean.- Frontend
api.test.mjs(26),aiApi.test.mjs,usePageData.test.mjs: pass. - Reproduced the canary build in both directions: building with the canary
VITE_JWT_TOKENset passes the guard, and the guard grep catches a JWT-shaped value planted infrontend/dist, so the gate is proven non-vacuous (jobworking-directory: frontendmakes thedistpath resolve correctly).
Security review notes
- Algorithm confusion is blocked twice: the unverified-header
algallowlist check happens before key lookup, andjwt.decodepinsalgorithmsagain. Startup refuses symmetric algorithms, a non-HTTPS JWKS URL, and missing issuer/audience/JWKS. - Confirmed against the pinned PyJWT 2.13.0 that
PyJWKClient.get_signing_key_from_jwtreads onlykidand never honors a token-suppliedjku, so there is no token-controlled key injection path. The client always fetches the configured HTTPS URL. - Fails closed as described: unreachable JWKS returns 503, an identity with no mapped role returns 403, a self-asserted
roleclaim is ignored in oidc mode, and tenant rejection returns the generic "Invalid token" without leaking claim details. g.useris not read anywhere else inapi/, so the principal shape change is safe.subis newly required in shared_secret mode:generate_demo_jwt.pyand the conftest fixture already include it, so existing flows do not break.
Nits (optional, non-blocking)
_get_jwks_clientbuildsPyJWKClientwith its defaulttimeout=30. If the IdP endpoint hangs rather than refusing connections, every request can tie up a worker for up to 30s before returning 503, which could exhaust workers during an IdP outage. Consider passing a shortertimeout(5-10s).- The
cache_keys=Trueper-kid LRU has no TTL: a resolved key is trusted until process restart or eviction, and the 300s JWKS-set lifespan only applies to unknown kids. Low risk with Entra since kid values are certificate thumbprints and are not reused, but emergency key revocation would require a restart. Worth one line in the docs. - Production startup still requires a strong
JWT_SECRET(_resolve_jwt_secretruns regardless of mode) even though oidc mode never uses it for verification. It fails closed with a clear message, butdocs/security/authentication.mddoes not mention this, and an operator who deletesJWT_SECRETafter switching to oidc will hit a startup failure. A doc line would prevent the confusion. .env.exampleomitsOIDC_ALGORITHMSandOIDC_CLOCK_SKEW_SECONDS(both are documented inauthentication.md).- Behavior note: shared_secret mode now accepts tokens carrying an
audclaim whenJWT_AUDIENCEis unset (verify_aud: False), where PyJWT's default previously rejected them. Defensible for this mode since trust anchors on the signature, just a subtle loosening to be aware of.
Solid containment step for #294: the credential is out of the public bundle behind a proven CI guard, tokens are memory-only with a legacy purge, and the OIDC verifier is fail-closed with strong test coverage.
What does this PR do?
Removes the bearer token built into the dashboard and adds OIDC token verification (JWKS signature, issuer, audience, tenant, IdP app roles) to the API. This is the second containment step for #294, after #320 (token expiry, roles, subscription allowlist).
Type of change
Changes
Frontend: no credential in public JS
VITE_JWT_TOKENbootstrap and thedev-local-tokenfallback fromApp.jsx. Anything in aVITE_*variable is compiled into the public bundle.api.jsnow keeps the token in memory only (setToken,getToken,clearToken) and deletes any legacylocalStorage.jwt_tokenon load without using it.aiApi.jsreads from the same in-memory store.VITE_JWT_TOKENand fails if a JWT-shaped value, the canary ordev-local-tokenappears infrontend/dist. I checked it both ways: the guard catches the canary on currentdevcode and passes with this change. The canary is assembled at run time, so Gitleaks doesn't flag the workflow file.API: durable token boundary (
api/auth.py)The auth mode is set with
OPENSHIELD_AUTH_MODE:shared_secret(default, unchanged for local/CI)oidc(new, enterprise)JWT_SECRETOIDC_JWKS_URL(5 min key cache)exp,sub(new);iss/audwhenJWT_ISSUER/JWT_AUDIENCEare setexp,iat,iss,aud,subtidmust be inOIDC_ALLOWED_TENANTSwhen setroleclaimrolesby default, e.g.OpenShield.Operator), mapped viaOIDC_ROLE_MAP; a self-assertedroleclaim is ignoredalg: nonetokens are refused inoidcmode before key lookup, so a token minted withJWT_SECRETcan never pass as an IdP token.503403oidcmode is missing issuer, audience or JWKS, uses a non-HTTPS JWKS URL, or configures a symmetric algorithmshared_secretmode runs in production.app.pynow calls the verifier. The viewer-can't-write gate and all existing error messages are unchanged.Docs and scripts
docs/security/authentication.md, covering:JWT_SECRET, restrict scope, review logs, verify, restore)generate_demo_jwt.pynow defaults to a 1h viewer token, is for API testing only, and no longer tells anyone to set it asVITE_JWT_TOKEN.api-reference.md,FRONTEND_API_TESTING.md,API_ENDPOINTS.txt,scripts/README.md,.env.exampleandCHANGELOG.md.Behavior change to note
A dashboard deployment that relied on
VITE_JWT_TOKENwill stop sending a token. Reads then work only against an API running withOPENSHIELD_PUBLIC_DEMO=true(non-sensitive data). This is intentional, per #294's containment step "RemoveVITE_JWT_TOKENand the automaticdev-local-tokenbootstrap". The public API is still suspended (#243).Operator actions (not code, can't be done from a PR)
VITE_JWT_TOKENfrom the Vercel environment and rotateJWT_SECRETon the API, following the checklist indocs/security/authentication.md.OPENSHIELD_AUTH_MODE=oidc.Testing
tests/test_oidc_auth.py: 34 new tests using a throwaway RSA key and a stub JWKS, with no network access. They cover:roleclaim ignoredkid, an HS256 token,alg: none, and a garbage tokensub,iss/audchecks, and RS256 refusedoidcmode, wrong tenant → 401tests/test_auth.pyandtests/test_subscription_authorization.pypass unchanged.test_devops_client.pyare local-only (azure-devopsisn't installed on my machine) and also fail on unmodifieddev.npm run lint,api.test.mjs(26, including a new memory-only/legacy-purge test),aiApi.test.mjs,usePageData.test.mjs, a11y and i18n checks, andnpm run buildwith the bundle guard.ruff check,ruff format --checkandbandit -r api/ -llare clean.Related issue
Partially addresses #294. Still open under that issue:
Checklist
Signed-off-bytrailerfix/description