feat(jwt): add jwt.issuer to enforce the iss claim on both issuerUrl and jwksUrl - #657
feat(jwt): add jwt.issuer to enforce the iss claim on both issuerUrl and jwksUrl#657Aman-Cool wants to merge 4 commits into
jwt.issuer to enforce the iss claim on both issuerUrl and jwksUrl#657Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughJWT authentication now accepts an optional issuer, passes it through OIDC and JWKS verifier construction, conditionally validates the JWT ChangesJWT issuer verification
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related issues
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
404da2b to
e8daf32
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
pkg/evaluators/identity/jwt.go (1)
37-39: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAvoid using a global variable for configuration pointers.
jwksTokenVerifierConfigis a global variable holding a pointer tooidc.Config. Passing a shared pointer to external library functions can be risky if the library or an internal function mutates the configuration. It is safer to return the struct from a function.♻️ Proposed refactor
-// jwksTokenVerifierConfig is used when only a raw jwksUrl is configured. There is no -// expected issuer in that API shape, so the issuer check must always be skipped. -var jwksTokenVerifierConfig = &oidc.Config{SkipClientIDCheck: true, SkipIssuerCheck: true} +// jwksTokenVerifierConfig returns the verifier config for the jwksUrl path. +// There is no expected issuer in that API shape, so the issuer check must always be skipped. +func jwksTokenVerifierConfig() *oidc.Config { + return &oidc.Config{SkipClientIDCheck: true, SkipIssuerCheck: true} +}Then, apply this update where the configuration is used on line 233:
- verifier := oidc.NewVerifier("", v.jwks, jwksTokenVerifierConfig) + verifier := oidc.NewVerifier("", v.jwks, jwksTokenVerifierConfig())🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/evaluators/identity/jwt.go` around lines 37 - 39, Replace the global pointer jwksTokenVerifierConfig with a function that returns a fresh oidc.Config value or pointer containing SkipClientIDCheck and SkipIssuerCheck set to true. Update the configuration use near the existing jwksTokenVerifierConfig reference to call this factory, ensuring each external library invocation receives an independent configuration instance.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@controllers/auth_config_controller.go`:
- Around line 384-386: Change the issuer-claim log in translateAuthConfig to
emit at verbosity V(1) instead of Info/V(0). In
controllers/auth_config_controller.go lines 384-386, update that logger call; in
controllers/auth_config_controller_test.go lines 391-393, configure the funcr
test logger with Verbosity: 1 so the V(1) message remains captured.
---
Nitpick comments:
In `@pkg/evaluators/identity/jwt.go`:
- Around line 37-39: Replace the global pointer jwksTokenVerifierConfig with a
function that returns a fresh oidc.Config value or pointer containing
SkipClientIDCheck and SkipIssuerCheck set to true. Update the configuration use
near the existing jwksTokenVerifierConfig reference to call this factory,
ensuring each external library invocation receives an independent configuration
instance.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: eedf2a9a-45cc-4959-bbb8-6eafb31034a0
📒 Files selected for processing (11)
api/v1beta3/auth_config_types.gocontrollers/auth_config_controller.gocontrollers/auth_config_controller_test.godocs/features.mdinstall/crd/authorino.kuadrant.io_authconfigs.yamlinstall/manifests.yamlpkg/evaluators/identity/jwt.gopkg/evaluators/identity/jwt_issuer_test.gopkg/evaluators/identity/jwt_test.gopkg/evaluators/metadata/user_info_test.gopkg/service/auth_pipeline_test.go
|
@guicassolato, Following the thread from #656..., while wiring up The interesting part: Authorino already validates this exactly one evaluator over; So the natural follow-up feels like an That's a slightly different shape than the |
This comment was marked as duplicate.
This comment was marked as duplicate.
guicassolato
left a comment
guicassolato
left a comment
There was a problem hiding this comment.
@Aman-Cool, I would like to challenge the API shape a little bit before this merges – since CRD fields are hard to change once released.
Leaving for your consideration a few comments throughout the code explaining the idea.
| func oidcTokenVerifierConfig(checkIssuerClaim bool) *oidc.Config { | ||
| return &oidc.Config{SkipClientIDCheck: true, SkipIssuerCheck: !checkIssuerClaim} | ||
| } | ||
|
|
||
| // jwksTokenVerifierConfig is used when only a raw jwksUrl is configured. There is no | ||
| // expected issuer in that API shape, so the issuer check must always be skipped. | ||
| var jwksTokenVerifierConfig = &oidc.Config{SkipClientIDCheck: true, SkipIssuerCheck: true} |
There was a problem hiding this comment.
These two functions could probably be simplified to a oidcConfig(checkIssuerClaim bool) *oidc.Config method of JWTVerifier. While oidcProviderVerifier can pass v.checkIssuerClaim in the call, jwksVerifier will pass a static false.
Or, depending on API redesign proposal, oidcConfig(issue string) *oidc.Config. Then, both verifiers would pass v.issuer in the call.
WDYT?
There was a problem hiding this comment.
+1, and the second option is the one I'd take. Once both verifiers carry an issuer, they collapse into a single oidcConfig(issuer string) with each just passing v.issuer. Best part: handing jwksVerifier an issuer of its own is the very thing that kills the silent no-op you flag below.., so this isn't just tidier, it's the same fix wearing a different hat.
| // Enable it to have Authorino reject, at the authentication phase, any token whose issuer differs from the configured issuerUrl. | ||
| // +optional | ||
| // +kubebuilder:default:=false | ||
| CheckIssuerClaim bool `json:"checkIssuerClaim,omitempty"` |
There was a problem hiding this comment.
A boolean checkIssuerClaim that implicitly derives its comparison value from issuerUrl has two problems:
- It doesn't work when
issuerUrland the token'sisslegitimately differ. A common deployment pattern is fetching the OIDC discovery document from within the cluster (http://keycloak.keycloak.svc.cluster.local:8080/realms/demo) while the IdP stamps tokens with the external URL (https://keycloak.example.com/realms/demo). EnablingcheckIssuerClaimhere would reject perfectly valid tokens — the user enables it expecting protection and gets false rejections with no recourse. - It doesn't extend to
jwksUrl. The field is silently ignored on thejwksUrlpath, which creates an inconsistency. A user who setscheckIssuerClaim: truewithjwksUrlwould believe they have issuer enforcement when they don't.
Alternative: a separate issuer field. Instead of a boolean, consider an explicit issuer string field — presence implies enforcement, absence preserves current behavior:
# cluster-local discovery, external issuer in tokens
jwt:
issuerUrl: "http://keycloak.keycloak.svc.cluster.local:8080/realms/demo"
issuer: "https://keycloak.example.com/realms/demo"
# jwksUrl — works the same way
jwt:
jwksUrl: "https://example.com/.well-known/jwks.json"
issuer: "https://example.com"This also extends naturally to other claim checks in the future (e.g., audiences: [...] for the aud claim) without accumulating booleans.
There was a problem hiding this comment.
You got me on this one😳, checkIssuerClaim: true with jwksUrl really is a silent no-op today, and a user would absolutely think they're covered when they aren't. The string fixes it cleanly, because the jwks path can enforce a plain issuer directly.
On #1 I went and ran it before agreeing: today's NewProvider(issuerUrl) actually rejects that internal/external split at construction (issuer did not match), so it fails at reconcile rather than false-rejecting tokens, and oidc.InsecureIssuerURLContext(issuer) turns out to be the tidy way to make the split work while still checking iss. So... sold: absence = no check, presence = enforce, one field on both paths, and audiences []string slots in right beside it later. I'll rework to this.
| if !identity.Jwt.CheckIssuerClaim { | ||
| log.FromContext(ctxWithLogger).Info("JWT authentication with issuerUrl does not verify the token issuer claim by default; set checkIssuerClaim to true or enforce the issuer via an authorization rule", "authentication", identityCfgName, "issuerUrl", identity.Jwt.IssuerUrl) | ||
| } | ||
| jwtVerifier = identity_evaluators.NewOIDCProviderVerifier(ctx, identity.Jwt.IssuerUrl, identity.Jwt.CheckIssuerClaim, identity.Jwt.TTL, identity.Jwt.Timeout) |
There was a problem hiding this comment.
With the issuer-as-a-string-field approach described above, this call would pass the explicit expected issuer value (or empty string for "don't check") rather than a boolean. That naturally resolves the question of what to compare against — the caller decides, not the OIDC discovery document.
There was a problem hiding this comment.
Yep... this just forwards the issuer string (or "" for "don't check") and lets the verifier decide, so the controller stays out of the discovery-vs-token weeds. Small bonus I didn't see coming: the "unsafe default" log now fires the same way for both issuerUrl and jwksUrl when issuer is empty, whereas the boolean could only ever warn on the OIDC path.
| defer v.mu.RUnlock() | ||
|
|
||
| idToken, err := provider.Verifier(tokenVerifierConfig).Verify(ctx, rawIDToken) | ||
| idToken, err := provider.Verifier(oidcTokenVerifierConfig(v.checkIssuerClaim)).Verify(ctx, rawIDToken) |
There was a problem hiding this comment.
nit: oidcTokenVerifierConfig(…) allocates a new *oidc.Config on every Verify() call (i.e., every inbound request). Since the config is immutable after construction, consider precomputing and storing the config on the oidcProviderVerifier struct.
There was a problem hiding this comment.
agreed. I'll resolve the config once at construction and hang it on the struct; it's immutable and independent of the provider, so the periodic JWKS/OIDC refresh doesn't care, and Verify() stops allocating on every request.
|
@guicassolato, Really glad you pushed on this.., "CRD fields are forever" is exactly the right reason to slow down here, and honestly your shape is better than mine🧐. I went through each note and even ran the go-oidc bits I wasn't 100% sure about, and I'm sold on the |
e8daf32 to
5121bae
Compare
Aman-Cool
changed the title
checkIssuerClaim to enforce the iss claim on the issuerUrl pathjwt.issuer to enforce the iss claim on both issuerUrl and jwksUrl
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/evaluators/identity/jwt.go (1)
222-248: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valuePrecompute the
oidc.IDTokenVerifierforjwksVerifier.Since
jwks,issuer, andconfigdo not change over the lifecycle of ajwksVerifier, you can construct and store the*oidc.IDTokenVerifieron the struct during initialization. This mirrors the caching ofv.configinoidcProviderVerifierand avoids allocating a new verifier struct on every inbound request.♻️ Proposed refactor
type jwksVerifier struct { - jwks oidc.KeySet - issuer string - config *oidc.Config + verifier *oidc.IDTokenVerifier } func NewJwksVerifier(ctx gocontext.Context, jwksUrl string, issuer string, timeout *int) JWTVerifier { // Create HTTP client with timeout and trace propagation. // Use Background context for request lifecycle (to avoid cancellation from reconciliation), // but propagate trace context from caller's ctx for observability. httpClient := httputil.NewClientWithTracing(ctx, timeout) jwkCtx := oidc.ClientContext(gocontext.Background(), httpClient) + jwks := oidc.NewRemoteKeySet(jwkCtx, jwksUrl) return &jwksVerifier{ - jwks: oidc.NewRemoteKeySet(jwkCtx, jwksUrl), - issuer: issuer, - config: oidcConfig(issuer), + verifier: oidc.NewVerifier(issuer, jwks, oidcConfig(issuer)), } } func (v *jwksVerifier) Verify(ctx gocontext.Context, rawIDToken string) (*oidc.IDToken, error) { - verifier := oidc.NewVerifier(v.issuer, v.jwks, v.config) - if verifier == nil { + if v.verifier == nil { return nil, errors.New(msg_jwksVerifierFailedToCreate) } - return verifier.Verify(ctx, rawIDToken) + return v.verifier.Verify(ctx, rawIDToken) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/evaluators/identity/jwt.go` around lines 222 - 248, Update jwksVerifier to store a precomputed *oidc.IDTokenVerifier, initialize it once in NewJwksVerifier from the configured issuer, JWKS, and config, and have Verify reuse that stored verifier instead of calling oidc.NewVerifier per request. Preserve the existing nil-verifier error behavior during initialization or verification as appropriate.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@pkg/evaluators/identity/jwt.go`:
- Around line 222-248: Update jwksVerifier to store a precomputed
*oidc.IDTokenVerifier, initialize it once in NewJwksVerifier from the configured
issuer, JWKS, and config, and have Verify reuse that stored verifier instead of
calling oidc.NewVerifier per request. Preserve the existing nil-verifier error
behavior during initialization or verification as appropriate.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: cf170e64-0142-454b-ab00-6b1a5cc04d5a
📒 Files selected for processing (11)
api/v1beta3/auth_config_types.gocontrollers/auth_config_controller.gocontrollers/auth_config_controller_test.godocs/features.mdinstall/crd/authorino.kuadrant.io_authconfigs.yamlinstall/manifests.yamlpkg/evaluators/identity/jwt.gopkg/evaluators/identity/jwt_issuer_test.gopkg/evaluators/identity/jwt_test.gopkg/evaluators/metadata/user_info_test.gopkg/service/auth_pipeline_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- pkg/service/auth_pipeline_test.go
- install/manifests.yaml
5121bae to
9bedbca
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/evaluators/identity/jwt.go (1)
133-133: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winRemove unnecessary RLock to prevent blocking background refreshes.
Holding
v.mu.RLock()across the potentially blockingVerifycall (which involves cryptographic checks and lazy JWKS HTTP fetches) can starve the background OIDC refresher that requiresv.mu.Lock()ingetOpenIdProvider().Because
provideris safely retrieved as a local variable beforehand, andv.configis immutable, this lock is not protecting any shared mutable state and can be safely removed to reduce contention.♻️ Proposed refactor
- v.mu.RLock() - defer v.mu.RUnlock() - idToken, err := provider.Verifier(v.config).Verify(ctx, rawIDToken)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/evaluators/identity/jwt.go` at line 133, Remove the RLock/RUnlock scope surrounding the Verify call in the JWT evaluation flow, using the already retrieved local provider directly. Keep the Verify invocation and immutable v.config access unchanged, ensuring no read lock is held during cryptographic verification or JWKS fetching.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@pkg/evaluators/identity/jwt.go`:
- Line 133: Remove the RLock/RUnlock scope surrounding the Verify call in the
JWT evaluation flow, using the already retrieved local provider directly. Keep
the Verify invocation and immutable v.config access unchanged, ensuring no read
lock is held during cryptographic verification or JWKS fetching.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 14582f94-2346-441b-952c-aca56f45b5f5
📒 Files selected for processing (11)
api/v1beta3/auth_config_types.gocontrollers/auth_config_controller.gocontrollers/auth_config_controller_test.godocs/features.mdinstall/crd/authorino.kuadrant.io_authconfigs.yamlinstall/manifests.yamlpkg/evaluators/identity/jwt.gopkg/evaluators/identity/jwt_issuer_test.gopkg/evaluators/identity/jwt_test.gopkg/evaluators/metadata/user_info_test.gopkg/service/auth_pipeline_test.go
🚧 Files skipped from review as they are similar to previous changes (7)
- pkg/evaluators/metadata/user_info_test.go
- docs/features.md
- controllers/auth_config_controller_test.go
- install/crd/authorino.kuadrant.io_authconfigs.yaml
- controllers/auth_config_controller.go
- pkg/evaluators/identity/jwt_issuer_test.go
- pkg/evaluators/identity/jwt_test.go
9bedbca to
1edd598
Compare
…and jwksUrl JWT authentication verifies the token signature and expiry but does not verify the "iss" (issuer) claim. Where distinct issuers share signing keys (multi-tenant IdPs, or Festival Wristbands issued by more than one AuthConfig backed by the same signing-key Secret), a token minted for one issuer would satisfy another issuer's config on signature alone. This adds a new optional `authentication.jwt.issuer` field holding the expected value of the `iss` claim. When set, Authorino rejects at the authentication phase any token whose `iss` does not equal it; when omitted (the default) the claim is not verified and existing AuthConfigs behave identically. The field applies to both the issuerUrl and jwksUrl paths. An explicit issuer string is used instead of a boolean so that: it enforces on the jwksUrl path too (which has no discovered issuer); the expected value is chosen by the operator rather than implicitly derived from discovery; and it extends naturally to further claim checks (e.g. audiences) without accumulating booleans. When the configured issuer differs from issuerUrl, discovery and JWKS are still fetched from issuerUrl while the issuer is pinned via oidc.InsecureIssuerURLContext, supporting setups where the OIDC discovery endpoint is reached at a different URL than the issuer stamped into tokens (e.g. cluster-internal discovery vs external issuer). At reconciliation an INFO log is emitted, uniformly for issuerUrl and jwksUrl, when issuer is omitted, and docs/features.md documents the field. Tests cover both paths (foreign issuer rejected when set, accepted when unset, matching issuer accepted, and the internal/external split enforced). Verified with `make test` and a full `make e2e` against real Keycloak (issuer unset, tokens still authenticate). Signed-off-by: Aman_Cool <aman017102007@gmail.com>
1edd598 to
f461a21
Compare
|
@guicassolato, Reworked the whole thing around your suggestion. The boolean is gone... it's now a single Tested end to end: unit tests for both paths: matching/foreign/unset plus the internal/external split; passing under The |
guicassolato
left a comment
guicassolato
left a comment
There was a problem hiding this comment.
Thanks for putting this together @Aman-Cool! It's really an excellent work! 🏅
Leaving here just a few nits, but it looks like we're very close to getting this one in.
Meanwhile, for the people interested in verifying the PR manually, these are the steps I followed roughly:
Verification steps
make local-setup FF=1 DEPLOY_KEYCLOAK=1
kubectl port-forward deployment/keycloak 8080:8080 2>&1 >/dev/null &
kubectl port-forward deployment/envoy 8000:8000 2>&1 >/dev/null &issuerUrl = internal, issuer = ∅, token → external ⇒ 200
kubectl apply -f -<<EOF
apiVersion: authorino.kuadrant.io/v1beta3
kind: AuthConfig
metadata:
name: talker-api-protection
spec:
hosts:
- talker-api.127.0.0.1.nip.io
authentication:
"keycloak-kuadrant-realm":
jwt:
issuerUrl: http://keycloak.default.svc.cluster.local:8080/realms/kuadrant
EOFACCESS_TOKEN=$(curl http://keycloak.127.0.0.1.nip.io:8080/realms/kuadrant/protocol/openid-connect/token -s -d 'grant_type=password' -d 'client_id=demo' -d 'username=john' -d 'password=p' -d 'scope=openid' | jq -r .access_token)curl -H "Authorization: Bearer $ACCESS_TOKEN" http://talker-api.127.0.0.1.nip.io:8000 -i
# 200issuerUrl = internal, issuer = external, token → external ⇒ 200
kubectl apply -f -<<EOF
apiVersion: authorino.kuadrant.io/v1beta3
kind: AuthConfig
metadata:
name: talker-api-protection
spec:
hosts:
- talker-api.127.0.0.1.nip.io
authentication:
"keycloak-kuadrant-realm":
jwt:
issuerUrl: http://keycloak.default.svc.cluster.local:8080/realms/kuadrant
issuer: http://keycloak.127.0.0.1.nip.io:8080/realms/kuadrant
EOFcurl -H "Authorization: Bearer $ACCESS_TOKEN" http://talker-api.127.0.0.1.nip.io:8000 -i
# 200issuerUrl = internal, issuer = internal, token → external ⇒ 401
kubectl apply -f -<<EOF
apiVersion: authorino.kuadrant.io/v1beta3
kind: AuthConfig
metadata:
name: talker-api-protection
spec:
hosts:
- talker-api.127.0.0.1.nip.io
authentication:
"keycloak-kuadrant-realm":
jwt:
issuerUrl: http://keycloak.default.svc.cluster.local:8080/realms/kuadrant
issuer: http://keycloak.default.svc.cluster.local:8080/realms/kuadrant
EOFcurl -H "Authorization: Bearer $ACCESS_TOKEN" http://talker-api.127.0.0.1.nip.io:8000 -i
# 401issuerUrl = internal, issuer = internal, token → internal ⇒ 200
ACCESS_TOKEN=$(kubectl run token --attach --rm --restart=Never -q --image=curlimages/curl -- http://keycloak.default.svc.cluster.local:8080/realms/kuadrant/protocol/openid-connect/token -s -d 'grant_type=password' -d 'client_id=demo' -d 'username=john' -d 'password=p' -d 'scope=openid' | jq -r .access_token)curl -H "Authorization: Bearer $ACCESS_TOKEN" http://talker-api.127.0.0.1.nip.io:8000 -i
# 200jwksUrl = internal, issuer = ∅, token → external ⇒ 200
kubectl apply -f -<<EOF
apiVersion: authorino.kuadrant.io/v1beta3
kind: AuthConfig
metadata:
name: talker-api-protection
spec:
hosts:
- talker-api.127.0.0.1.nip.io
authentication:
"keycloak-kuadrant-realm":
jwt:
jwksUrl: http://keycloak.default.svc.cluster.local:8080/realms/kuadrant/protocol/openid-connect/certs
EOFACCESS_TOKEN=$(curl http://keycloak.127.0.0.1.nip.io:8080/realms/kuadrant/protocol/openid-connect/token -s -d 'grant_type=password' -d 'client_id=demo' -d 'username=john' -d 'password=p' -d 'scope=openid' | jq -r .access_token)curl -H "Authorization: Bearer $ACCESS_TOKEN" http://talker-api.127.0.0.1.nip.io:8000 -i
# 200jwksUrl = internal, issuer = external, token → external ⇒ 200
kubectl apply -f -<<EOF
apiVersion: authorino.kuadrant.io/v1beta3
kind: AuthConfig
metadata:
name: talker-api-protection
spec:
hosts:
- talker-api.127.0.0.1.nip.io
authentication:
"keycloak-kuadrant-realm":
jwt:
jwksUrl: http://keycloak.default.svc.cluster.local:8080/realms/kuadrant/protocol/openid-connect/certs
issuer: http://keycloak.127.0.0.1.nip.io:8080/realms/kuadrant
EOFcurl -H "Authorization: Bearer $ACCESS_TOKEN" http://talker-api.127.0.0.1.nip.io:8000 -i
# 200jwksUrl = internal, issuer = internal, token → external ⇒ 401
kubectl apply -f -<<EOF
apiVersion: authorino.kuadrant.io/v1beta3
kind: AuthConfig
metadata:
name: talker-api-protection
spec:
hosts:
- talker-api.127.0.0.1.nip.io
authentication:
"keycloak-kuadrant-realm":
jwt:
jwksUrl: http://keycloak.default.svc.cluster.local:8080/realms/kuadrant/protocol/openid-connect/certs
issuer: http://keycloak.default.svc.cluster.local:8080/realms/kuadrant
EOFcurl -H "Authorization: Bearer $ACCESS_TOKEN" http://talker-api.127.0.0.1.nip.io:8000 -i
# 401jwksUrl = internal, issuer = external, token → internal ⇒ 200
ACCESS_TOKEN=$(kubectl run token --attach --rm --restart=Never -q --image=curlimages/curl -- http://keycloak.default.svc.cluster.local:8080/realms/kuadrant/protocol/openid-connect/token -s -d 'grant_type=password' -d 'client_id=demo' -d 'username=john' -d 'password=p' -d 'scope=openid' | jq -r .access_token)curl -H "Authorization: Bearer $ACCESS_TOKEN" http://talker-api.127.0.0.1.nip.io:8000 -i
# 200| ) | ||
|
|
||
| const ( | ||
| issuerTestServerHost = "127.0.0.1:9007" |
There was a problem hiding this comment.
I'm afraid this port number may conflict with our OPA unit tests 😞.
In fact, github.com/kuadrant/authorino/pkg/auth/httptest.NewHttpServerMock is basically a wrapper of Go's net/http/httptest that takes as input a network address (to bind the listener to) and a HTTP path → HTTP response data map that dictates the behaviour of the handler. We used this approach in the past to multiplex the requests essentially based on the port number instead of dynamic server URLs.
More recently though, we've been favouring using net/http/httptest.NewServer directly instead. It binds the listener to :0 (OS-assigned ephemeral port), thus preventing port conflicts. E.g.:
authorino/pkg/evaluators/metadata/generic_http_test.go
Lines 207 to 213 in 58fecc6
Your newIdPMockWithIssuer func would probably change to something like this:
func newIdPMockWithIssuer(advertisedIssuer, jwks string) *gohttptest.Server {
var server *gohttptest.Server
server = gohttptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
switch r.URL.Path {
case "/.well-known/openid-configuration":
fmt.Fprintf(w, `{"issuer":%q,"jwks_uri":"%s/certs"}`, advertisedIssuer, server.URL)
case "/certs":
w.Header().Set("Content-Type", "application/json")
fmt.Fprint(w, jwks)
}
}))
return server
}Of course, the tests would have to pass server.URL as the issuerUrl instead of the constant trustedIssuer.
There was a problem hiding this comment.
good catch, and it's a real one: pkg/evaluators/authorization/opa_test.go:24 already binds 127.0.0.1:9007, so with packages running in parallel this would have flaked in CI rather than locally. Switching to httptest.NewServer with the OS-assigned port as you suggested, following the generic_http_test.go precedent, so the tests stop depending on a hand-maintained port registry entirely.
There was a problem hiding this comment.
Continuing in the thread wrt #657 (comment):
Nice catch, @Aman-Cool! Indeed, I hadn't tested that snipped. It was something of the top of my head. Thanks for spotting the limitation.
I guess option 1 is reasonable. The func would then probably go:
func newIdPMockWithIssuer(advertisedIssuer, jwks string) *gohttptest.Server {
var server *gohttptest.Server
server = gohttptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
iss := advertisedIssuer
if iss == "" {
iss = server.URL // self-advertise
}
switch r.URL.Path {
case "/.well-known/openid-configuration":
fmt.Fprintf(w, `{"issuer":%q,"jwks_uri":"%s/certs"}`, iss, server.URL)
case "/certs":
w.Header().Set("Content-Type", "application/json")
fmt.Fprint(w, jwks)
}
}))
return server
}Then, called as follows:
- Common case (self-advertises):
newIdPMockWithIssuer("", jwks) - Mismatch test only:
newIdPMockWithIssuer(externalIssuer, jwks)
Again, I haven't tested it, but make sense?
BTW, since we won't need the trustedIssuer constant anymore, if you want to name the func just newIdPMock, it works for me. But it's your call.
|
|
||
| // issuerUrl path, issuer set: a token signed by a key in the configured provider's JWKS | ||
| // but whose `iss` names a different issuer must NOT authenticate. | ||
| func TestOIDCProviderVerifier_IssuerSet_RejectsForeignIssuer(t *testing.T) { |
There was a problem hiding this comment.
Do you mind moving the tests in this file into pkg/evaluators/identity/jwt_test.go please?
There was a problem hiding this comment.
will do. I'll keep the existing tests in that file on their current oidcServerHost setup rather than converting them too, so the diff stays scoped to this PR; happy to migrate them in a follow-up if you'd like the whole file on ephemeral ports.
|
Thanks for the review @guicassolato😄 applied both doc/comment suggestions. |
|
@guicassolato..,in your proposed signature, Options I see:
I'd lean towards (1) since it keeps a single helper and the mismatch case stays explicit at the call site, but it's your test conventions... let me know which you prefer and I'll push the refactor. |
Co-authored-by: Guilherme Cassolato <guicassolato@gmail.com> Signed-off-by: Aman_Cool <aman017102007@gmail.com>
Co-authored-by: Guilherme Cassolato <guicassolato@gmail.com> Signed-off-by: Aman_Cool <aman017102007@gmail.com>
Move the issuer-enforcement tests and their helpers from jwt_issuer_test.go into jwt_test.go, as requested in review, keeping all JWT identity tests in a single file. Signed-off-by: Aman_Cool <aman017102007@gmail.com>
2d89a63 to
d62d2e1
Compare
|
Done: moved the issuer tests into Still open: the |
|
✅ Action performedReview finished.
|
|
@coderabbitai , have a look |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '--- changed files ---'
git diff --name-status "$(git merge-base HEAD origin/main)" HEAD 2>/dev/null || git diff --name-status HEAD~1 HEAD
echo '--- relevant outlines ---'
for f in pkg/evaluators/identity/jwt.go pkg/evaluators/identity/jwt_test.go controllers/auth_config_controller.go; do
if [ -f "$f" ]; then
echo "### $f"
ast-grep outline "$f" --items all --type function,method 2>/dev/null || true
fi
done
echo '--- issuer helper and its call sites ---'
rg -n -C 8 'newIdPMockWithIssuer|NewOIDCProviderVerifier|NewJwksVerifier|InsecureIssuerURLContext|oidcConfig' \
pkg/evaluators/identity/jwt.go pkg/evaluators/identity/jwt_test.go controllers/auth_config_controller.go \
2>/dev/null || true
echo '--- focused diff ---'
git diff "$(git merge-base HEAD origin/main)" HEAD -- \
pkg/evaluators/identity/jwt.go \
pkg/evaluators/identity/jwt_test.go \
controllers/auth_config_controller.go \
api/v1beta3/auth_config_types.go \
2>/dev/null || git diff HEAD~1 HEAD -- \
pkg/evaluators/identity/jwt.go \
pkg/evaluators/identity/jwt_test.go \
controllers/auth_config_controller.go \
api/v1beta3/auth_config_types.goLength of output: 57139 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '--- issuer test constants and package imports ---'
sed -n '1,80p' pkg/evaluators/identity/jwt_test.go
sed -n '260,450p' pkg/evaluators/identity/jwt_test.go
echo '--- custom HTTP mock definition and usage conventions ---'
rg -n -C 6 'func NewHttpServerMock|type HttpServerMock|NewHttpServerMock\(' \
--glob '*.go' .
echo '--- dynamic standard httptest server usage in test files ---'
rg -n -C 4 'net/http/httptest|httptest\.NewServer\(' --glob '*_test.go' .
echo '--- all production constructor call sites ---'
rg -n -C 3 'NewOIDCProviderVerifier\s*\(|NewJwksVerifier\s*\(' \
--glob '*.go' -g '!**/*_test.go' .
echo '--- locking/provider refresh implementation ---'
sed -n '90,225p' pkg/evaluators/identity/jwt.goLength of output: 65869
For the remaining helper question, I recommend option 1: make an empty The current helper at Line 319 still binds the new tests to func newIdPMockWithIssuer(advertisedIssuer, jwks string) *httptest.Server {
var server *httptest.Server
server = httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
switch r.URL.Path {
case "/.well-known/openid-configuration":
issuer := advertisedIssuer
if issuer == "" {
issuer = server.URL
}
w.Header().Set("Content-Type", "application/json")
_, _ = fmt.Fprintf(w, `{"issuer":%q,"jwks_uri":%q}`, issuer, server.URL+"/certs")
case "/certs":
w.Header().Set("Content-Type", "application/json")
_, _ = w.Write([]byte(jwks))
default:
http.NotFound(w, r)
}
}))
return server
}Then derive the discovery URL from |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/evaluators/identity/jwt_test.go`:
- Around line 265-266: Update the issuer mock helper and related test setup to
bind the server on loopback port 0, obtain its assigned listener address, and
return the resolved issuer URL from the helper. Replace uses of the fixed
trustedIssuer value in verifier configuration with that returned URL, including
the additional referenced setup.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: a633a3c9-10a7-43cb-91e0-d1299b4a9a07
📒 Files selected for processing (2)
api/v1beta3/auth_config_types.gopkg/evaluators/identity/jwt_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- api/v1beta3/auth_config_types.go
| issuerTestServerHost = "127.0.0.1:9007" | ||
| trustedIssuer = "http://" + issuerTestServerHost |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Use an ephemeral port for the issuer mock.
Binding every run to 127.0.0.1:9007 can fail with “address already in use” when tests or local processes overlap. Bind to loopback port 0, return the resolved issuer URL from the helper, and use it in verifier setup.
Also applies to: 320-320
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pkg/evaluators/identity/jwt_test.go` around lines 265 - 266, Update the
issuer mock helper and related test setup to bind the server on loopback port 0,
obtain its assigned listener address, and return the resolved issuer URL from
the helper. Replace uses of the fixed trustedIssuer value in verifier
configuration with that returned URL, including the additional referenced setup.
2 participants
Implements #656.
Adds an optional
jwt.issuerfield to JWT authentication.., the expected value of theissclaim. Leave it unset (the default) and nothing changes on upgrade; set it and Authorino rejects, right at the authentication phase, any token whoseissdoesn't equal it, with no separate authorization rule needed.Following the review, this uses an explicit
issuerstring rather than a boolean, so it: enforces on thejwksUrlpath too (not justissuerUrl); lets the operator choose the expected value rather than deriving it from discovery; and supports the cluster-internal-discovery / external-issuer split viaoidc.InsecureIssuerURLContext(discovery/JWKS still fetched fromissuerUrl,isspinned toissuer). It also leaves room foraudiences: [...](theaudclaim) later without piling on booleans.Also adds an INFO log at reconciliation... uniformly for both
issuerUrlandjwksUrl.., whenissueris omitted, plus a note in the docs.Tests cover both paths (foreign issuer rejected when set, accepted when unset, matching issuer accepted, and the internal/external split enforced). Verified with
make testand a fullmake e2eagainst real Keycloak; normal tokens still authenticate.The
aud-claim / audience validation is intentionally left out to keep this focused; tracked as a follow-up.Summary by CodeRabbit
New Features
issclaim does not match the configured issuer.Documentation
Bug Fixes