feat(proxy): add optional OIDC access token audience validation - #3466
Conversation
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | 89 |
🟢 Coverage 74.60% diff coverage · +0.12% coverage variation
Metric Results Coverage variation ✅ +0.12% coverage variation (-1.00%) Diff coverage ✅ 74.60% diff coverage Coverage variation details
Coverable lines Covered lines Coverage Common ancestor commit (92560db) 88625 21142 23.86% Head commit (72d2047) 88660 (+35) 21258 (+116) 23.98% (+0.12%) Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch:
<coverage of head commit> - <coverage of common ancestor commit>Diff coverage details
Coverable lines Covered lines Diff coverage Pull request (#3466) 63 47 74.60% Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified:
<covered lines added or modified>/<coverable lines added or modified> * 100%
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
|
Do you think a small performance benchmark for the cache-hit path would be useful here, given that the access token now needs to be verified even on cache hits? |
|
I did a small performance investigation of the audience-enabled cache-hit path. The additional cost is dominated by the required JWT signature verification. Audience-enabled cache hits were around 99 µs/op compared to around 8 µs/op without per-hit verification. I looked into avoiding repeated verification through policy-scoped caching, but that would weaken the current behavior around JWKS key removal/replacement and would also require broader cache/logout invalidation changes, so I kept that out of this PR. I did find one small safe optimization in the existing verification path: avoiding the second full JWT parse after successful verification. In controlled serial benchmarks this reduced audience-enabled cache-hit time by about 5–7% and saves 1,088 B / 22 allocations per hit, while leaving the audience-disabled path unchanged. Full affected package tests and race tests pass. |
5847b9e to
bf738a1
Compare
rhafer
left a comment
rhafer
left a comment
There was a problem hiding this comment.
@zerox80 Thanks a lot for you efforts!
Would you mind submitting the fix(config): correct pending version annotations commit as a separate PR and remove it from this one?
We could fast-track that one, as it is straight-forward. The rest needs a more thorough review.
bf738a1 to
1947e48
Compare
done |
rhafer
left a comment
rhafer
left a comment
There was a problem hiding this comment.
Thanks again. Here's round of feedback after a quick look through the code.
61a0018 to
fd6affe
Compare
dc39e8d to
5707906
Compare
rhafer
left a comment
rhafer
left a comment
There was a problem hiding this comment.
Just a few smaller comments after some testing.
Also: Please rebase this once more on the latest main branch to get the latest CI fixes.
|
|
||
| func newOIDCAuthenticator(logger log.Logger, cfg *config.Config, userInfoCache store.Store, httpClient *http.Client) *middleware.OIDCAuthenticator { | ||
| if cfg.OIDC.Issuer != "" && len(cfg.OIDC.Audiences) == 0 { | ||
| logger.Warn().Msg("OIDC access token audience validation is disabled. Configure PROXY_OIDC_AUDIENCES to enable it; this is recommended for production.") |
There was a problem hiding this comment.
We should only log this if AccessTokenVerifyMethod != "none'. Otherwise this is misleading.
| if strings.TrimSpace(audience) == "" { | ||
| return RegClaimsWithSID{}, jwt.MapClaims{}, errors.New("access token audiences must not contain empty or whitespace-only entries") | ||
| } | ||
| } |
There was a problem hiding this comment.
This config validation should IMO happen in NewOIDCClient, so that we have a chance to fail early (NewOIDCClient would obviously need a separate error return value for that), instead of failing every single request.
| [audience validation requirement in RFC 9068, Section 4](https://www.rfc-editor.org/rfc/rfc9068.html#name-validating-jwt-access-token): | ||
| a resource server following that JWT access token profile must reject tokens | ||
| whose audience does not identify the resource server. | ||
|
|
There was a problem hiding this comment.
I think we should add a few words/links about:
- how the builtin IDP does (not) handle the audience claim. (Basically it just hardcodes the
audclaim to the client id of the authenticated client) - how properly setup audiences in keycloak (a pointer to: https://www.keycloak.org/docs/latest/server_admin/#audience-support)
| "go-micro.dev/v4/store" | ||
| ) | ||
|
|
||
| func newOIDCAuthenticator(logger log.Logger, cfg *config.Config, userInfoCache store.Store, httpClient *http.Client) *middleware.OIDCAuthenticator { |
There was a problem hiding this comment.
Nitpick: please move this function to server.go. I think it not needed to have it in a separate file.
5707906 to
05a8e79
Compare
Co-authored-by: Ralf Haferkamp <ralf@h4kamp.de>
github-project-automation
Bot
moved this from In Progress
to Done
in OpenCloud Team Board
2 participants
Description
Add optional OIDC access token audience validation through
PROXY_OIDC_AUDIENCESoroidc.audiencesinproxy.yaml. The list defaults to empty for compatibility. When configured, a token must contain at least one exact, case-sensitive match in itsaudclaim. Missing, empty, malformed and nonmatching audiences are rejected during token verification.Token verification, including the audience check, runs on Userinfo cache misses. Cache hits reuse unexpired cached claims without verifying the token again or requesting Userinfo. Existing entries in a shared or persistent cache can therefore remain accepted under the previous audience configuration until they expire. The documentation explains clearing the Userinfo cache after updating all proxy instances when a policy change must take effect immediately.
Audience configuration is validated when creating the OIDC client, so invalid settings fail during startup. The proxy logs an audience warning only when OIDC and JWT verification are enabled and the audience list is empty. The README includes ENV and YAML examples, the built-in IDP's client-ID audience behavior, Keycloak setup guidance and a recommendation to enable the check in production.
The token parsing optimization is in #3530 and can be reviewed independently.
Related Issue
Fixes #3456
Motivation and Context
When an IdP serves several applications, OpenCloud can currently accept an otherwise valid access token issued for another application. This gives administrators an explicit way to restrict tokens to OpenCloud without breaking existing IdP configurations by default.
How Has This Been Tested?
Tested on Ubuntu WSL with Go 1.25.9 and GCC, using the vendored dependencies.
go test -mod=vendor -race -count=1 ./pkg/oidc/... ./services/proxy/...passed after the constructor validation changes and rebase onto main.git diff --checkpassed.The tests use locally generated keys and an HTTP test IdP with discovery, JWKS and Userinfo endpoints. They cover both
skip_user_infosettings, audience matching and rejection, cached Userinfo reuse, expired or corrupt cache entries, logout invalidation, ENV/YAML precedence, rejection of invalid configuration during client setup and the startup warning, including its suppression with verification set to none. A middleware test verifies that cache misses call token verification once and cache hits skip it. Signature, issuer,nbf, token expiry and AD FS issuer checks are also covered. No external IdP is needed.Types of changes
Checklist
HTTP integration tests are included in the Go suite. The Behat acceptance suite was not extended.
GPT 6 Astra was used as a supporting tool for the implementation and tests.