fix(auth): validate Okta JWT kid and harden key-fetch client#354
Merged
Conversation
7085c1e to
36df55f
Compare
MackOverflow
previously approved these changes
Jun 18, 2026
MackOverflow
left a comment
Collaborator
There was a problem hiding this comment.
This is a well-scoped, correctly implemented security fix with thorough tests and clear documentation. The vulnerability (unsanitized kid in URL construction + no-timeout HTTP client) is real and the remediation is appropriate. Nice catch Cam.
Two nits:
TestOktaKeyValidKidFetchesAndParses could additionally assert that gotPath == "/"+kid when the base URL has no trailing slash (edge case: double-slash if TokenKeyUrl already ends in /). Currently the test sets ts.URL + "/" so it only tests the slash-appended case.
Consider a test case for a kid of exactly 200 chars (boundary) and exactly 201 chars — the current invalid list uses 201 and 500, but 200 (valid boundary) isn't in the valid list.
oktaKey read kid from the unverified token header and concatenated it into the
key-fetch URL, then fetched it with http.DefaultClient (no timeout, follows
redirects) and used the returned PEM as the verification key. An attacker-shaped
kid could reshape the outbound request and, worst case, return a key that
verifies a forged token.
- Validate kid against ^[A-Za-z0-9_-]{1,200}$ before any cache/config/network
access; reject otherwise. This is what makes the existing TokenKeyUrl+kid
concatenation safe (no traversal, host injection, scheme, or encoded tricks).
- Route both the Okta per-kid fetch and the Entra JWKS fetch through a dedicated
client with a 10s timeout that refuses to follow redirects, so a slow endpoint
cannot pin a goroutine and a 3xx cannot bounce the fetch off-host.
- Capture the previously-ignored http.NewRequest error.
Tests: kid allowlist (valid vs traversal/host-injection/scheme/oversized),
rejection before any fetch, and a positive path that a valid kid fetches exactly
TokenKeyUrl+kid and parses the ECDSA key.
Refs CMS-Enterprise/ztmf-misc#240
36df55f to
1da8ad0
Compare
danielbowne
added a commit
that referenced
this pull request
Jun 23, 2026
#351) (#361) ## Summary Routine refresh of the `impl` branch from `main` so the experimental environment does not drift from production code. Merge (not rebase), per the impl-sync policy, because `impl` is a shared, deployed branch. ## What main brought in The bulk of main's recent history (OpDiv-scoped RBAC, dual-IdP scaffolding, pgxpool, OpenAPI autogeneration, the N+1 users-list fix) was already present on impl from the previous sync, so the net new content here is small: - Scores diff endpoint to compare two data calls (#356) - Okta JWT `kid` validation and hardened key-fetch client (#354) - Route `/login/*` through CloudFront to the ALB for Entra login (#351) - The enrichment OpDiv gate as it landed on main (#359), reconciled with impl's own copy (#358) ## Conflict resolution - **Makefile** - kept impl's per-environment port scaffolding (`ZTMF_ENV` detection, parametrized `TEST_DB_PORT`). Reverting to main's hardcoded ports would break side-by-side worktree runs. - **fismasystems.go / scores.go / openapi.yaml** - took main's canonical versions (main's `FindFismaSystemByUUID` adds an explicit `LIMIT 1`; scores-diff is additive). OpenAPI regenerated from annotations afterward, no drift. ## Environment safety - Entra remains dormant on impl: there is no impl tfvars override, so `entra_enabled` stays at its default of false. Main's #350 only enabled it in dev. - No new migrations introduced by this sync beyond what is already merged on main. ## Testing Full local suite on the impl port set (unit, integration, E2E/Emberfall). --------- Co-authored-by: cameron testerman <11036339+voidspooks@users.noreply.github.com> Co-authored-by: MackOverflow <114018913+MackOverflow@users.noreply.github.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.
Hardens JWT verification key fetching against an unsanitized-input flaw in the Okta path.
Problem
oktaKey()took thekidfrom the (necessarily unverified) token header and concatenated it directly into the key-fetch URL, then fetched it withhttp.DefaultClient(no timeout, follows redirects) and used the returned PEM as the verification key. Becausekidselects the key, it is read before the signature is checked - so it is untrusted input being used to shape an outbound request and choose a trust anchor. The Entra (RS256) path was already safe (fixed JWKS URL,kidas a map key); this brings the Okta path to the same standard.Change
kidagainst^[A-Za-z0-9_-]{1,200}$before any cache, config, or network access; reject otherwise. The allowlist (URL-safe characters only, matching real Okta/ALB key ids) is what makes the existingTokenKeyUrl + kidconcatenation safe - no path traversal, host/userinfo injection, scheme, or encoded tricks can reshape the request.kidfetch and the Entra JWKS fetch through a dedicatedhttp.Clientwith a 10s timeout that refuses to follow redirects, so a slow/hung endpoint cannot pin a request goroutine and a 3xx cannot bounce the fetch to an unexpected host.http.NewRequesterror.Behavior is unchanged for legitimate tokens: a valid
kidstill fetches exactlyTokenKeyUrl + kidand parses the ECDSA key.Test plan
make test-full(unit + Emberfall E2E) passes 117/0kidallowlist (valid vs traversal/host-injection/scheme/oversized), rejection before any outbound fetch, and a positive path (validkid-> correct URL -> ECDSA parse)go build,go vet,gofmtcleanRefs CMS-Enterprise/ztmf-misc#240