Grant CIMD clients the server's allowed audiences - #6490
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #6490 +/- ##
==========================================
- Coverage 78.19% 78.18% -0.01%
==========================================
Files 769 769
Lines 75078 75087 +9
==========================================
+ Hits 58707 58709 +2
- Misses 16366 16373 +7
Partials 5 5 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
jhrozek
left a comment
There was a problem hiding this comment.
Reviewed with security, OAuth-correctness, code-quality, and architecture passes — no blocking issues.
This correctly mirrors the DCR audience fix from #3796 for the CIMD path. Confirmed the actual audience gate is the resource-parameter validation in token.go, not client.GetAudience(), so granting the full AllowedAudiences list here doesn't widen what tokens get issued — it just unblocks fosite's refresh-grant self-check, which was the actual bug. Test coverage is solid, including a regression test that drives fosite's real DefaultAudienceMatchingStrategy.
Two minor non-blocking nits for a follow-up if you want them:
CIMDDecoratorConfig's doc comment says "prevents silent swaps of the two adjacent[]stringfields" — there are three now.buildFositeClientis up to 6 params (4[]string) — might be worth a struct given the file already uses one for the same reason elsewhere, but not required.
CIMD-resolved clients were built with an empty audience list, so fosite's DefaultAudienceMatchingStrategy rejected every refresh_token grant with "has not been whitelisted by the OAuth 2.0 Client". The authorization_code grant was unaffected because that path never validates audience, so the failure only surfaced on refresh, roughly an access-token lifetime after the first sign-in. CIMDDecoratorConfig now carries AllowedAudiences, threaded from the server's own AllowedAudiences at construction, and buildFositeClient grants that list to every resolved client, mirroring how DCR clients already inherit the server's AllowedAudiences (stacklok#3796). Fixes: stacklok#6489
Since stacklok#6284 the decorator persists every resolved CIMD client so Redis session rehydration can find it. A refresh_token grant on a rehydrated session is therefore checked against the persisted row's audience list rather than the in-memory client's. The decorator-level regression test now asserts the persisted snapshot carries the configured audiences as well.
buildFositeClient had grown to six parameters, four of them []string, so a swapped pair at a call site would compile and misbehave silently. A fositeClientParams struct names every input at the call site, for the same reason CIMDDecoratorConfig is a struct. The CIMDDecoratorConfig comment now counts the three adjacent []string fields it guards.
8fd8a0f to
d83cbbe
Compare
|
Rebased over #6284 (both changed the same |
|
The one red leg (Tests / Test Go Code) is |
Summary
CIMD-resolved clients are built with an empty
Audiencelist, while the token handler grants the session audience from theresourceparameter (or defaults it toAllowedAudiences[0]). fosite's refresh handler validates the granted audience against the client's own list (DefaultAudienceMatchingStrategy), so every refresh_token grant from a CIMD client fails withinvalid_request("Requested audience ... has not been whitelisted by the OAuth 2.0 Client"): sign-in works, refresh never does, and every CIMD client (ChatGPT connectors, Claude Code) has to re-authorize interactively once per access-token lifespan. #3796 fixed the identical failure for DCR clients by registering them with the server'sAllowedAudiences; the CIMD path never received that fix.CIMDDecoratorConfiggainsAllowedAudiences, threaded from the server's ownAllowedAudiencesat the construction site inserver_impl.go.buildFositeClientgrants that list to every resolved client instead ofnil, mirroring the DCR shape from Set audience on DCR clients for refresh token support #3796; its inputs now travel in afositeClientParamsstruct (review nit).fetch()path plusfosite.DefaultAudienceMatchingStrategyexactly as the refresh handler calls it — it fails at the pre-fix behavior with fosite's verbatim production error and passes with the fix. The decorator test also asserts that the client row Persist resolved CIMD clients so Redis session rehydration finds them #6284 persists for Redis session rehydration carries the same audiences, since a refresh on a rehydrated session is checked against that row.Fixes #6489
Type of change
Test plan
task test)task lint-fix)go test ./pkg/authserver/...(every package this change touches, including the HTTP-level integration tests) is green locally; the fulltask testmatrix incl.-raceis left to CI (cgo unavailable on the dev machine).golangci-lint run ./pkg/authserver/...reports 0 issues.Does this introduce a user-facing change?
Yes: clients registered via Client ID Metadata Documents can now use the refresh_token grant; previously every CIMD refresh failed with
invalid_requestand clients had to re-authorize each access-token lifespan.Special notes for reviewers
fosite.DefaultAudienceMatchingStrategydirectly, mirroringhandler/oauth2/flow_refresh.go's call verbatim; a full HTTP-level CIMD refresh flow would need a CIMD-document test server wired into the integration harness, which felt disproportionate for a fix this size — happy to add it if you'd prefer.buildFositeClientcall: the audience is threaded into the client it persists, and the test above covers that row.