Skip to content

fix: harden the requests to the provider and the JWKS cache - #293

Open
me-cedric wants to merge 5 commits into
sevensolutions:mainfrom
me-cedric:fix/oidc/provider-requests
Open

fix: harden the requests to the provider and the JWKS cache#293
me-cedric wants to merge 5 commits into
sevensolutions:mainfrom
me-cedric:fix/oidc/provider-requests

Conversation

@me-cedric

Copy link
Copy Markdown
Contributor

Split out of #290, which is now a draft. Four independent fixes to how the plugin talks to the provider, plus the CI change that makes the first one's test meaningful. Each is its own commit, reviewable on its own.

fix: take the read lock when looking up a JWKS key

JwksHandler carries an RWMutex, but only the reload path ever took it. getRsaKey/getEcdsaKey walked RsaKeys/EcdsaKeys unlocked, so validating a token while the JWKS cache was being replaced was a data race. Reproducible with go test -race via the added test.

ci: run the tests of every package with the race detector

go test in ./src only builds that one package, so the tests under src/oidc, src/rules and src/utils have never been executed — by CI or by task test:unit. Now go test -race ./..., which is also what makes the JWKS test above show the problem it guards against.

This is in the same PR as the JWKS fix on purpose: without it the new test compiles but proves nothing.

fix: give the requests to the provider a timeout

httpClient had no Timeout, so a provider that accepts a connection and then never answers holds a traefik goroutine indefinitely — discovery, token exchange, refresh, introspection and userinfo all go through it. 30s.

fix: check the status code of the introspection response

introspectToken decoded the response body whatever the status code was, so an error payload from the provider was read as an introspection result. A body that happens to carry "active": true would then decide the outcome.

fix: keep the refresh request in sync with the auth code exchange

renewToken had drifted apart from exchangeAuthCode in two ways:

  • it sent resources, while RFC 8707 (and the auth code exchange) use resource, so RequestedResources were dropped on every refresh and the renewed token could come back with the wrong audience
  • it never sent the client assertion, so a provider configured with ClientJwtPrivateKey rejected every refresh

Upgrade note

Refresh requests now carry resource when RequestedResources is set. A provider that allows it on the auth code grant but not on the refresh grant would start rejecting refreshes.

Test plan

  • New tests: JWKS lookup under -race, introspection with a non-200, and the refresh request's form values.
  • The same code passed go test -race ./..., go vet ./..., gofmt -l ., govulncheck ./... and task test:e2e (19/19 against Keycloak, so through Yaegi and not just the Go compiler) as part of fix: security and reliability pass over cookies, token renewal and logout #290. CI runs the unit tests on this branch.

AI usage

Per AI_POLICY.md: written with Claude Opus 5 (Claude Code), then reviewed line by line. The CI gap was found by an adversarial review pass over the diff — the JWKS test was passing for the wrong reason. I can explain and defend every line here.

JwksHandler already carries an RWMutex but only the reload path ever took
it. Key lookups walked RsaKeys/EcdsaKeys unlocked, so a token validation
running while the JWKS cache was being replaced was a data race.

Reproducible with `go test -race` via the new test.
`go test` in ./src only builds the src package, so the tests under
src/oidc, src/rules and src/utils were never executed by CI or by
`task test:unit`. The JWKS locking test added here also only shows the
problem it guards against under -race.
The http client talking to the IDP had no timeout, so a provider that
accepts a connection and then never answers holds on to a traefik goroutine
for as long as it likes. Discovery, token exchange, refresh, introspection
and userinfo all go through it.
The response body was decoded whatever the status code was, so an error
payload from the provider was read as an introspection result. A body that
happens to carry "active" would then decide the outcome.
renewToken had drifted apart from exchangeAuthCode in two ways:

- it sent `resources`, while RFC 8707 (and the auth code exchange) use
  `resource`, so RequestedResources were dropped on every refresh and the
  renewed token could come back with the wrong audience
- it never sent the client assertion, so a provider configured with
  ClientJwtPrivateKey rejected every refresh
@me-cedric

Copy link
Copy Markdown
Contributor Author

The workflow runs on this PR are sitting at action_required — GitHub gates Actions on PRs from forks until you approve them, so there's no signal here yet.

I ran both workflows on this exact commit inside my own fork to check. Run Go tests and E2E Tests both pass, and the CI change in this PR is visible in the output — every package builds and runs now, where before only src did:

go test -race ./...
ok  github.com/sevensolutions/traefik-oidc-auth/src         1.894s
ok  github.com/sevensolutions/traefik-oidc-auth/src/oidc    1.063s
ok  github.com/sevensolutions/traefik-oidc-auth/src/rules   1.016s
ok  github.com/sevensolutions/traefik-oidc-auth/src/utils   1.013s

That src/oidc line is the one that matters here: it's where the JWKS locking test lives, and under -race it's what actually exercises the fix.

@sevensolutions

Copy link
Copy Markdown
Owner

Refresh requests now carry resource when RequestedResources is set. A provider that allows it on the auth code grant but not on the refresh grant would start rejecting refreshes.

Can we detect that somehow?

@me-cedric

Copy link
Copy Markdown
Contributor Author

At runtime we can. A provider that refuses the resource on the refresh grant is supposed to answer with invalid_target. So renewToken could catch that one error code, drop resource, and retry once. It already reads the error body, it just throws it away today.

It's a narrow risk tho. The token endpoint must ignore parameters it doesn't know, so only a provider that actually implements resource indicators can break here. And the failure ends in a re-login, not a lockout. Today's behaviour is arguably worse: resources is ignored by everyone, so refreshed tokens silently come back with the wrong audience.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants