fix(security): enforce JWT aud validation per route; M2M gets own audience (#84) - #94
Open
EM-SubhamDas wants to merge 1 commit into
Open
fix(security): enforce JWT aud validation per route; M2M gets own audience (#84)#94EM-SubhamDas wants to merge 1 commit into
EM-SubhamDas wants to merge 1 commit into
Conversation
…ience (#84) The aud claim was written on every token but never validated, and M2M client_credentials tokens shared the user-token audience. Token-type boundaries were therefore unenforced: a machine or API-key token was accepted on user self-service routes (/auth/me, /auth/otp/*), where no user exists behind it. - Add AudienceAPI/M2M/Management/Agent constants; sentinel errors ErrUnexpectedAudience and ErrNoAudienceAllowed - Add VerifyForAudience(ctx, token, allowed...); Verify() is now AudienceAPI-only, plus VerifyM2M(); require exactly one audience - issueTokenPair -> AudienceAPI, IssueServiceToken -> AudienceM2M, SAML JIT login -> AudienceAPI - JWTRequired(jwtSvc, allowedAudiences...) is audience-parameterised; adminGroup accepts {API, Management, M2M} so machine clients keep working, while JWTRenew user routes accept AudienceAPI only - Harden: pin HS256 exactly (was HMAC family) and enforce iss - Add emc_auth_token_audience_rejections_total{audience,route}, wired in both JWTRequired and JWTRenew - Tests: 8 audience cases using real SignManagement/SignAgent tokens, a 9-case middleware token x route matrix, fail-closed and metric guards Deploying invalidates tokens minted before restart (old aud); access tokens live 15 minutes, so impact is one forced re-login for users active in that window. Refresh cookies are unaffected. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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
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
The
aud(audience) claim was written on every JWT but never validated, and M2Mclient_credentialstokens shared the same audience value as user login tokens. Token-type boundaries were therefore unenforced at runtime — a machine or API-key-derived token was accepted on user self-service routes (/auth/me,/auth/otp/*) where no user exists behind the token. This PR enforces audience per route group, so machine and management tokens keep working on the admin API but are refused on user-facing routes.Closes #84
Type of change
Phase / Plan reference
Issue #84 — no roadmap plan document. Hardening of Phase 2 (Auth Engine), contributes to Phase 7 (Testing & Hardening).
Changes made
AudienceAPI/AudienceM2M/AudienceManagement/AudienceAgentconstants, replacing scattered string literals; add sentinel errorsErrUnexpectedAudienceandErrNoAudienceAllowedVerifyForAudience(ctx, token, allowed...)as the single verification funnel;Verify()becomesAudienceAPI-only, plus newVerifyM2M(); require exactly one audience so a multi-valuedaudcannot satisfy a route by carrying one acceptable value among othersissueTokenPair→AudienceAPI;IssueServiceToken→AudienceM2M; SAML JIT login →AudienceAPI(not in the issue text, but it shared the same literal and would have broken)JWTRequired(jwtSvc, allowedAudiences...)is now audience-parameterised.adminGroupaccepts{API, Management, M2M}so machine clients keep working;JWTRenewuser routes acceptAudienceAPIonlyHS256(was HMAC family) and enforce theissclaim, which was set on every token but never checkedemc_auth_token_audience_rejections_total{audience, route}, wired in bothJWTRequiredandJWTRenewjwt_test.gousing realSignManagement/SignAgenttokens rather than synthetic stand-ins; newmiddleware/jwt_test.gowith a 9-case token × route matrix, a fail-closed test, and a metric regression guardDeviation from the issue's stated plan — please read
The issue's acceptance criterion said "
Verify()rejects any token whoseaudis notAudienceAPI", assuming M2M tokens had no legitimate consumer. That assumption is wrong for this codebase, and implementing it literally would have returned401to every machine integration using the admin API:IssueServiceToken's own doc: "Scopes are loaded from theoauth_clients.scopescolumn so downstream permission checks receive the correct grants."auditAdminApp(the shared admin audit helper): "Service tokens carry the public client_id in the UserID claim, which is not a users.id…"Machine-to-machine access to the admin API is the Auth0
client_credentialsManagement API pattern — the parity feature this issue is labelled for. Audience is therefore enforced per route group rather than as one global rule, which meets the security objective without the regression.Security considerations
ErrNoAudienceAllowed)On audit events: a Prometheus counter was chosen over audit rows. These rejections occur on unauthenticated requests, so writing an audit row per rejection would give an attacker control over write volume. The counter gives operators the same signal without that amplification. A well-behaved client never triggers it, so any sustained non-zero rate is worth alerting on:
The HTTP response stays a generic
401 {"code":"token_invalid"}on purpose — a caller must not be able to probe which token types a route accepts.Test plan
go build ./...passes (alsogo vetandgofmtclean)go test ./... -p 1 -count=1passes for all packages touched here/meverified, plus SAML path reviewed)Note on the suite: run with
-p 1(existing test-isolation limitation). 8 failures inemail_flows_test.go/user_management_test.gowere confirmed pre-existing by running the identical tests on untouchedmasterin a throwaway worktree — same 8 failures. Root cause is local dev-DB schema drift (missingfailed_login_attempts,blocked_at,breach_notified_at; the migration file is correct but the DB records it as applied). CI builds a fresh DB from migrations, so these should not appear here.Manual verification highlights (all observed on a running server):
emc-auth-api)/auth/me,/tenantsemc-auth-m2m)/users(itsusers:readscope)/auth/me,/auth/otp/statustoken_invalid← previously 200/tenants/auth/metoken_invalid← previously 200/auth/meRollout
Deploy as a single restart — this ships as one self-hosted binary, so the issue's suggested staged 15-minute rollout (which assumes a multi-instance fleet) does not apply.
Tokens minted before the restart carry the old
aud: "emc-auth-server", which no verify path accepts. Access tokens live 15 minutes, so the impact is one forced re-login for users active in that window. Refresh-token cookies are unaffected (opaque, DB-hashed, not JWTs), so no data is lost.Checklist
feat/issue-84-jwt-audience-validation)fmt.Printlnor debug statements left inReviewer notes — pre-existing CI risks
Two items may show red through no fault of this PR, both already tracked as deferred follow-ups:
pgx v5.6.0(sanitize.SanitizeSQL). Predates this work; needs its own chore PR to bump to ≥ v5.9.2.TestSQLInjection_LoginPassword— flaky hard 3s timing threshold that trips on slower machines against Login's deliberate bcrypt-cost-12 anti-timing padding. Payloads are correctly rejected.Follow-ups identified (not in this PR)
audhere is not Auth0'saudience. Ours carries the token type; Auth0's names the target API. Per-API audience is a future feature — needed once tenant apps verify tokens themselves, since JWT libraries validateaudautomatically while nothing validates ourapp_idclaim. Recommended migration: move token type to agtyclaim and freeaud. Do not make token-type audiences tenant-configurable — a tenant able to claimemc-auth-apire-opens this issue.SignAgentmintsaud=emc-auth-agenttokens that nothing verifies. Not an exposure — no verify path allows that audience — but the feature is half-built.