HYPERFLEET-1237 - feat: support multiple JWT issuer configurations#246
HYPERFLEET-1237 - feat: support multiple JWT issuer configurations#246rh-amarin wants to merge 2 commits into
Conversation
…ation Introduces an optional regex pattern (`identity_claim_pattern`) that the resolved JWT identity value must match. This allows operators to restrict which identities can perform mutations — for example, enforcing email-like values or allowing only a fixed set of service account names. - Add `IdentityClaimPattern` field to `JWTConfig` and `CallerIdentityConfig` - Compile pattern once at startup in `NewCallerIdentityMiddleware`; invalid regex prevents server from starting - Pass compiled `*regexp.Regexp` to `CallerIdentityFromRequest` to avoid per-request recompilation - Wire `IdentityClaimPattern` from config through routes - Bind `server.jwt.identity_claim_pattern` env var and `--server-jwt-identity-claim-pattern` flag - Add unit and middleware-level tests covering match, reject, no-pattern, and invalid-pattern cases - Document new setting in `docs/authentication.md` and `docs/config.md`
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
📝 WalkthroughSummary by CodeRabbit
WalkthroughJWT configuration is refactored from a single-issuer top-level structure to a multi-issuer array of Sequence Diagram(s)sequenceDiagram
participant Client
participant JWTHandler
participant Issuer1Parser
participant Issuer2Parser
participant IdentityMW
participant Next
Client->>JWTHandler: GET /api/... + Bearer Token
JWTHandler->>Issuer1Parser: Validate token with issuer 1
alt Issuer1 validation fails
Issuer1Parser-->>JWTHandler: Error
JWTHandler->>Issuer2Parser: Validate token with issuer 2
alt Issuer2 validation succeeds
Issuer2Parser-->>JWTHandler: Parsed JWT + matched issuer config
else Issuer2 fails
Issuer2Parser-->>JWTHandler: Error
JWTHandler-->>Client: 401 Invalid or expired JWT
end
else Issuer1 validation succeeds
Issuer1Parser-->>JWTHandler: Parsed JWT + matched issuer config
end
JWTHandler->>JWTHandler: SetMatchedIdentityConfig(ctx, issuer config, pattern)
JWTHandler->>IdentityMW: ServeHTTP with context
IdentityMW->>IdentityMW: GetMatchedIdentityConfig(ctx)
IdentityMW->>IdentityMW: Compile JWT claim → identity
alt Pattern set and mismatch
IdentityMW-->>Client: 401 Identity claim does not match pattern
else Pattern matches or not set
IdentityMW->>Next: ServeHTTP with username in context
Next-->>Client: 200 OK
end
🎯 4 (Complex) | ⏱️ ~45 minutes Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 2 warnings)
✅ Passed checks (8 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
Comment |
Risk Score: 5 —
|
| Signal | Detail | Points |
|---|---|---|
| PR size | 978 lines (>500) | +2 |
| Sensitive paths | cmd/ | +2 |
| Test coverage | Missing tests for: cmd/hyperfleet-api/environments cmd/hyperfleet-api/server test | +1 |
Computed by hyperfleet-risk-scorer
Replace single-issuer JWT scalars with an ordered list of issuer configs. A request is accepted if its token validates against any configured issuer, enabling multi-IdP deployments without forking the binary. - Add JWTIssuerConfig struct with per-issuer issuer_url, audience, jwk_cert_url, jwk_cert_file, identity_claim, identity_claim_pattern, header fields; JWTConfig now holds Configs []JWTIssuerConfig - Redesign JWTHandler: try each compiledIssuer in order; on first success store matched identity config in context via SetMatchedIdentityConfig - Update CallerIdentityMiddleware to read per-request identity config from context (JWT path) with static config as non-JWT fallback - Remove removed scalar fields from config flags, env bindings, and tests - Fix integration test wiring: OverrideConfig sets JWT.Configs[0]; StartJWKCertServerMock and CreateJWTString read from Configs[0] - Update docs/authentication.md and docs/config.md for new config shape Co-Authored-By: Claude <noreply@anthropic.com>
rh-amarin
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (2)
pkg/auth/jwt_handler_test.go (1)
224-233: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winIsolate unknown-issuer rejection from signature rejection (CWE-287, CWE-290).
This subtest uses a new key (
unknownKey), so rejection can occur before issuer-claim enforcement is evaluated. Use a configured key (for examplekeyA) with an unknownissto prove issuer validation is enforced independently.Suggested test change
t.Run("token from unknown issuer rejected", func(t *testing.T) { RegisterTestingT(t) - unknownKey, err := rsa.GenerateKey(rand.Reader, 2048) - Expect(err).NotTo(HaveOccurred()) - token := signToken(t, unknownKey, jwt.MapClaims{ + token := signToken(t, keyA, jwt.MapClaims{ "iss": "https://idp-c.example.com", "exp": time.Now().Add(time.Hour).Unix(), }) rr := serve(handler, "/protected", "Bearer "+token) Expect(rr.Code).To(Equal(http.StatusUnauthorized)) })As per path instructions, “Error paths SHOULD be tested, not just happy paths.”
🤖 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/auth/jwt_handler_test.go` around lines 224 - 233, The test conflates signature validation failure (due to using an unknown key) with issuer validation failure, making it impossible to verify that issuer validation is being enforced independently. Replace the use of the newly generated unknownKey in the signToken call with a configured key like keyA, while keeping the unknown issuer claim set to "https://idp-c.example.com". This ensures the signature validation passes but the issuer validation fails, proving that issuer-claim enforcement is checked independently of signature verification.Source: Path instructions
pkg/config/server.go (1)
81-95: 🔒 Security & Privacy | 🔵 TrivialConfig validation defers key source checks to handler startup — confirm this is intentional layering, not a gap.
JWTConfig.Validate() checks only
IssuerURLandIdentityClaim, deferring JWK source validation to NewJWTHandler at startup (buildKeyfunc line 196-197: "at least one of KeysFile or KeysURL must be provided"). The startup path is fail-fast (check() → os.Exit(1)) before the server listens, so an issuer without a key source cannot reach production. IdentityClaimPattern regex is also compiled/validated at handler startup (buildCompiledIssuer), not config load time.This validation layering is suboptimal from a defense-in-depth perspective — config validation should catch missing key sources rather than deferring to handler initialization — but the security boundary holds: the process exits before listening if a key source is absent. Confirm in code review whether this intentional split is documented or acceptable for your deployment model.
Audience is conditionally enforced (jwt.WithAudience appended only if set); no cross-service token confusion window exists if
Audienceis required in your configs.🤖 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/config/server.go` around lines 81 - 95, The JWTConfig.Validate() method should perform comprehensive validation of all required JWT configuration fields at config load time, rather than deferring validation to handler startup. Currently, the Validate method only checks IssuerURL and IdentityClaim, but defers KeysFile/KeysURL presence validation to NewJWTHandler (buildKeyfunc function) and IdentityClaimPattern regex compilation to buildCompiledIssuer. Move the key source validation logic (ensuring at least one of KeysFile or KeysURL is provided) from the handler initialization path into the JWTConfig.Validate() method for each config in the Configs slice, and add regex compilation validation for IdentityClaimPattern if it is set. This ensures all critical JWT configuration issues are caught at config load time, providing proper defense-in-depth validation layering.
🤖 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 `@cmd/hyperfleet-api/server/routes.go`:
- Around line 89-94: The callerIdentityMW middleware with an empty
CallerIdentityConfig is being installed unconditionally, which breaks no-auth
mode by returning 401 errors on all mutating requests when no identity config
exists in context. Only install the NewCallerIdentityMiddleware and the
callerIdentityMW.ResolveCallerIdentity middleware if JWT is actually enabled or
a real identity source exists. Gate the middleware creation and installation
behind a condition that checks whether JWT is configured, rather than
unconditionally installing it with a fallback empty config.
In `@pkg/auth/jwt_handler.go`:
- Around line 121-138: The header value ic.Header is being stored in the
compiledIssuer struct's identityCfg without validation, which allows it to
bypass the per-issuer identity claim pattern gate. Add validation of ic.Header
before storing it in the HeaderName field of the CallerIdentityConfig within the
compiledIssuer return statement. Apply the same forbidden-header validation
logic that is used in NewCallerIdentityMiddleware to ensure headers are properly
validated at this system boundary before being stored in the configuration.
- Around line 171-189: Preserve the expired token error classification when
iterating through issuers. Currently in the loop that attempts to parse the
token with each issuer, the `lastErr` variable is unconditionally overwritten
with each failed attempt, which causes an earlier jwt.ErrTokenExpired error to
be lost if a subsequent issuer fails with a different error. Before assigning
the new error to lastErr after issuer.parser.Parse fails, check if lastErr is
already set to jwt.ErrTokenExpired and skip the assignment if so, ensuring that
the expired token classification is preserved for the final error handling logic
that checks for jwt.ErrTokenExpired.
In `@pkg/config/loader.go`:
- Around line 306-311: The documentation advertises
`HYPERFLEET_SERVER_JWT_CONFIGS` as a supported environment variable override for
JWT configurations, but the implementation does not actually handle this
environment variable. The config loader does not bind `server.jwt.configs` in
the `bindAllEnvVars()` function around line 306-311, nor does it include this
key in the `handleJSONArrayEnvVars()` function (lines 235-275) which is required
to properly parse JSON array environment variables. You must resolve this
inconsistency by either: 1) removing `HYPERFLEET_SERVER_JWT_CONFIGS` from the
documentation (config.md and authentication.md) since JWT configs are currently
YAML-only as noted in the comment, OR 2) adding the `server.jwt.configs` key
mapping to the `handleJSONArrayEnvVars()` function to actually implement support
for the JSON environment variable. Choose the approach that aligns with the
intended design and implement it consistently across both code and
documentation.
In `@test/helper.go`:
- Around line 159-160: The code at the index assignment
`helper.Env().Config.Server.JWT.Configs[0] = jwkURL` accesses the Configs slice
without checking if it contains any elements, which can cause an index out of
range panic if the slice is empty. Add a guard check using
`len(helper.Env().Config.Server.JWT.Configs) > 0` before this line, following
the same pattern used by other helper methods in the file like
IdentityHeaderName, StartJWKCertServerMock, and CreateJWTString. Only assign the
jwkURL if the Configs slice has at least one element.
---
Nitpick comments:
In `@pkg/auth/jwt_handler_test.go`:
- Around line 224-233: The test conflates signature validation failure (due to
using an unknown key) with issuer validation failure, making it impossible to
verify that issuer validation is being enforced independently. Replace the use
of the newly generated unknownKey in the signToken call with a configured key
like keyA, while keeping the unknown issuer claim set to
"https://idp-c.example.com". This ensures the signature validation passes but
the issuer validation fails, proving that issuer-claim enforcement is checked
independently of signature verification.
In `@pkg/config/server.go`:
- Around line 81-95: The JWTConfig.Validate() method should perform
comprehensive validation of all required JWT configuration fields at config load
time, rather than deferring validation to handler startup. Currently, the
Validate method only checks IssuerURL and IdentityClaim, but defers
KeysFile/KeysURL presence validation to NewJWTHandler (buildKeyfunc function)
and IdentityClaimPattern regex compilation to buildCompiledIssuer. Move the key
source validation logic (ensuring at least one of KeysFile or KeysURL is
provided) from the handler initialization path into the JWTConfig.Validate()
method for each config in the Configs slice, and add regex compilation
validation for IdentityClaimPattern if it is set. This ensures all critical JWT
configuration issues are caught at config load time, providing proper
defense-in-depth validation layering.
🪄 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: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 7d6df354-9c2f-4c9f-ade0-ca9194723da4
📒 Files selected for processing (17)
cmd/hyperfleet-api/environments/e_integration_testing.gocmd/hyperfleet-api/server/api_server.gocmd/hyperfleet-api/server/routes.godocs/authentication.mddocs/config.mdpkg/auth/auth_middleware.gopkg/auth/context.gopkg/auth/jwt_handler.gopkg/auth/jwt_handler_test.gopkg/config/dump.gopkg/config/flags.gopkg/config/helpers_test.gopkg/config/loader.gopkg/config/loader_test.gopkg/config/server.gopkg/config/server_test.gotest/helper.go
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
openshift-hyperfleet/architecture(manual)openshift-hyperfleet/hyperfleet-api(manual)openshift-hyperfleet/hyperfleet-sentinel(manual)openshift-hyperfleet/hyperfleet-adapter(manual)openshift-hyperfleet/hyperfleet-broker(manual)
💤 Files with no reviewable changes (2)
- pkg/config/helpers_test.go
- pkg/config/loader_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/auth/auth_middleware.go
| // Identity config is resolved per-request from context when JWT is enabled | ||
| // (JWTHandler stores the matched issuer's config). An empty static config is used | ||
| // as the fallback for non-JWT paths (dev/no-auth mode). | ||
| callerIdentityMW, mwErr := auth.NewCallerIdentityMiddleware(auth.CallerIdentityConfig{}) | ||
| check(mwErr, "Unable to create caller identity middleware") | ||
| apiV1Router.Use(callerIdentityMW.ResolveCallerIdentity) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Do not install an empty identity resolver in no-auth mode.
With JWT disabled, no matched identity config is ever placed in context. This middleware then falls back to the empty config on Line 92, cannot resolve an identity, and returns 401 for every mutating request. That breaks run-no-auth. CWE/CVE: not applicable.
Gate the middleware unless a real identity source exists
- // Identity config is resolved per-request from context when JWT is enabled
- // (JWTHandler stores the matched issuer's config). An empty static config is used
- // as the fallback for non-JWT paths (dev/no-auth mode).
- callerIdentityMW, mwErr := auth.NewCallerIdentityMiddleware(auth.CallerIdentityConfig{})
- check(mwErr, "Unable to create caller identity middleware")
- apiV1Router.Use(callerIdentityMW.ResolveCallerIdentity)
+ if env().Config.Server.JWT.Enabled {
+ // Identity config is resolved per-request from context when JWTHandler stores
+ // the matched issuer's config.
+ callerIdentityMW, mwErr := auth.NewCallerIdentityMiddleware(auth.CallerIdentityConfig{})
+ check(mwErr, "Unable to create caller identity middleware")
+ apiV1Router.Use(callerIdentityMW.ResolveCallerIdentity)
+ }As per coding guidelines, make run-no-auth must “Start server without auth.”
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Identity config is resolved per-request from context when JWT is enabled | |
| // (JWTHandler stores the matched issuer's config). An empty static config is used | |
| // as the fallback for non-JWT paths (dev/no-auth mode). | |
| callerIdentityMW, mwErr := auth.NewCallerIdentityMiddleware(auth.CallerIdentityConfig{}) | |
| check(mwErr, "Unable to create caller identity middleware") | |
| apiV1Router.Use(callerIdentityMW.ResolveCallerIdentity) | |
| if env().Config.Server.JWT.Enabled { | |
| // Identity config is resolved per-request from context when JWTHandler stores | |
| // the matched issuer's config. | |
| callerIdentityMW, mwErr := auth.NewCallerIdentityMiddleware(auth.CallerIdentityConfig{}) | |
| check(mwErr, "Unable to create caller identity middleware") | |
| apiV1Router.Use(callerIdentityMW.ResolveCallerIdentity) | |
| } |
🤖 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 `@cmd/hyperfleet-api/server/routes.go` around lines 89 - 94, The
callerIdentityMW middleware with an empty CallerIdentityConfig is being
installed unconditionally, which breaks no-auth mode by returning 401 errors on
all mutating requests when no identity config exists in context. Only install
the NewCallerIdentityMiddleware and the callerIdentityMW.ResolveCallerIdentity
middleware if JWT is actually enabled or a real identity source exists. Gate the
middleware creation and installation behind a condition that checks whether JWT
is configured, rather than unconditionally installing it with a fallback empty
config.
Source: Coding guidelines
| var compiledPattern *regexp.Regexp | ||
| if ic.IdentityClaimPattern != "" { | ||
| compiledPattern, err = regexp.Compile(ic.IdentityClaimPattern) | ||
| if err != nil { | ||
| return compiledIssuer{}, fmt.Errorf("identity_claim_pattern %q is not a valid regex: %w", | ||
| ic.IdentityClaimPattern, err) | ||
| } | ||
| } | ||
|
|
||
| // JWTHandler validates JWT tokens on incoming requests. Call Close() during | ||
| // shutdown to stop the background JWKS refresh goroutine. | ||
| type JWTHandler struct { | ||
| keyfunc keyfunc.Keyfunc | ||
| next http.Handler | ||
| parser *jwt.Parser | ||
| cancel context.CancelFunc | ||
| publicPatterns []*regexp.Regexp | ||
| return compiledIssuer{ | ||
| parser: jwt.NewParser(parserOpts...), | ||
| keyfunc: kf, | ||
| compiledPattern: compiledPattern, | ||
| identityCfg: CallerIdentityConfig{ | ||
| JWTIdentityClaim: ic.IdentityClaim, | ||
| IdentityClaimPattern: ic.IdentityClaimPattern, | ||
| HeaderName: ic.Header, | ||
| }, |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Validate per-issuer identity headers before storing them.
Line 137 copies ic.Header into the matched identity config, but the only forbidden-header check is in NewCallerIdentityMiddleware, which routes.go now calls with an empty config. CallerIdentityFromRequest also resolves HeaderName before the JWT claim and returns before applying IdentityClaimPattern, so a configured header can bypass the per-issuer claim-pattern gate. CWE-287.
Security hardening direction
func buildCompiledIssuer(ctx context.Context, ic JWTIssuerHandlerConfig) (compiledIssuer, error) {
kf, err := buildKeyfunc(ctx, ic)
if err != nil {
return compiledIssuer{}, fmt.Errorf("failed to build keyfunc: %w", err)
}
+
+ if ic.Header != "" && validation.IsForbiddenIdentityHeaderName(ic.Header) {
+ return compiledIssuer{}, fmt.Errorf("identity header name %q is not allowed", ic.Header)
+ }Also apply IdentityClaimPattern to the final resolved identity, or disallow header precedence when IdentityClaimPattern is configured.
As per path instructions, Security (SEC-01 to SEC-03): “Validate input at system boundaries (HTTP handlers, CLI parsers, webhook receivers).”
🤖 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/auth/jwt_handler.go` around lines 121 - 138, The header value ic.Header
is being stored in the compiledIssuer struct's identityCfg without validation,
which allows it to bypass the per-issuer identity claim pattern gate. Add
validation of ic.Header before storing it in the HeaderName field of the
CallerIdentityConfig within the compiledIssuer return statement. Apply the same
forbidden-header validation logic that is used in NewCallerIdentityMiddleware to
ensure headers are properly validated at this system boundary before being
stored in the configuration.
Source: Path instructions
| var lastErr error | ||
| for _, issuer := range h.issuers { | ||
| token, err := issuer.parser.Parse(tokenString, issuer.keyfunc.Keyfunc) | ||
| if err != nil { | ||
| lastErr = err | ||
| continue | ||
| } | ||
| ctx := SetJWTTokenContext(r.Context(), token) | ||
| ctx = SetMatchedIdentityConfig(ctx, issuer.identityCfg, issuer.compiledPattern) | ||
| h.next.ServeHTTP(w, r.WithContext(ctx)) | ||
| return | ||
| } | ||
|
|
||
| ctx := SetJWTTokenContext(r.Context(), token) | ||
| h.next.ServeHTTP(w, r.WithContext(ctx)) | ||
| logger.WithError(r.Context(), lastErr).Warn("JWT validation failed against all configured issuers") | ||
| if errors.Is(lastErr, jwt.ErrTokenExpired) { | ||
| handleError(r.Context(), w, r, hferrors.CodeAuthExpiredToken, "JWT token has expired") | ||
| } else { | ||
| handleError(r.Context(), w, r, hferrors.CodeAuthInvalidCredentials, "invalid or expired JWT token") | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Preserve expired-token classification across issuer attempts.
lastErr is overwritten by later issuer failures. An expired token for issuer A can return HYPERFLEET-AUT-002 instead of HYPERFLEET-AUT-003 if issuer B is attempted afterward and fails with a non-expiry error. CWE/CVE: not applicable.
Order-independent error classification
var lastErr error
+ expired := false
for _, issuer := range h.issuers {
token, err := issuer.parser.Parse(tokenString, issuer.keyfunc.Keyfunc)
if err != nil {
+ if errors.Is(err, jwt.ErrTokenExpired) {
+ expired = true
+ }
lastErr = err
continue
}
@@
}
logger.WithError(r.Context(), lastErr).Warn("JWT validation failed against all configured issuers")
- if errors.Is(lastErr, jwt.ErrTokenExpired) {
+ if expired {
handleError(r.Context(), w, r, hferrors.CodeAuthExpiredToken, "JWT token has expired")
} else {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| var lastErr error | |
| for _, issuer := range h.issuers { | |
| token, err := issuer.parser.Parse(tokenString, issuer.keyfunc.Keyfunc) | |
| if err != nil { | |
| lastErr = err | |
| continue | |
| } | |
| ctx := SetJWTTokenContext(r.Context(), token) | |
| ctx = SetMatchedIdentityConfig(ctx, issuer.identityCfg, issuer.compiledPattern) | |
| h.next.ServeHTTP(w, r.WithContext(ctx)) | |
| return | |
| } | |
| ctx := SetJWTTokenContext(r.Context(), token) | |
| h.next.ServeHTTP(w, r.WithContext(ctx)) | |
| logger.WithError(r.Context(), lastErr).Warn("JWT validation failed against all configured issuers") | |
| if errors.Is(lastErr, jwt.ErrTokenExpired) { | |
| handleError(r.Context(), w, r, hferrors.CodeAuthExpiredToken, "JWT token has expired") | |
| } else { | |
| handleError(r.Context(), w, r, hferrors.CodeAuthInvalidCredentials, "invalid or expired JWT token") | |
| } | |
| var lastErr error | |
| expired := false | |
| for _, issuer := range h.issuers { | |
| token, err := issuer.parser.Parse(tokenString, issuer.keyfunc.Keyfunc) | |
| if err != nil { | |
| if errors.Is(err, jwt.ErrTokenExpired) { | |
| expired = true | |
| } | |
| lastErr = err | |
| continue | |
| } | |
| ctx := SetJWTTokenContext(r.Context(), token) | |
| ctx = SetMatchedIdentityConfig(ctx, issuer.identityCfg, issuer.compiledPattern) | |
| h.next.ServeHTTP(w, r.WithContext(ctx)) | |
| return | |
| } | |
| logger.WithError(r.Context(), lastErr).Warn("JWT validation failed against all configured issuers") | |
| if expired { | |
| handleError(r.Context(), w, r, hferrors.CodeAuthExpiredToken, "JWT token has expired") | |
| } else { | |
| handleError(r.Context(), w, r, hferrors.CodeAuthInvalidCredentials, "invalid or expired JWT token") | |
| } |
🤖 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/auth/jwt_handler.go` around lines 171 - 189, Preserve the expired token
error classification when iterating through issuers. Currently in the loop that
attempts to parse the token with each issuer, the `lastErr` variable is
unconditionally overwritten with each failed attempt, which causes an earlier
jwt.ErrTokenExpired error to be lost if a subsequent issuer fails with a
different error. Before assigning the new error to lastErr after
issuer.parser.Parse fails, check if lastErr is already set to
jwt.ErrTokenExpired and skip the assignment if so, ensuring that the expired
token classification is preserved for the final error handling logic that checks
for jwt.ErrTokenExpired.
| l.bindEnv("server.tls.cert_file") | ||
| l.bindEnv("server.tls.key_file") | ||
| l.bindEnv("server.jwt.enabled") | ||
| l.bindEnv("server.jwt.issuer_url") | ||
| l.bindEnv("server.jwt.audience") | ||
| l.bindEnv("server.jwt.identity_claim") | ||
| l.bindEnv("server.identity_header") | ||
| l.bindEnv("server.jwk.cert_file") | ||
| l.bindEnv("server.jwk.cert_url") | ||
| // Note: server.jwt.configs is a slice of structs configured via YAML config file. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Is HYPERFLEET_SERVER_JWT_CONFIGS bound/honored anywhere, or is config truly YAML-only?
rg -nP -C2 'JWT_CONFIGS|jwt\.configs|AutomaticEnv' pkg/config
echo "----- docs claims -----"
rg -nP -C2 'JWT_CONFIGS|jwt\.configs|identity_header|JWT_IDENTITY_CLAIM' docsRepository: openshift-hyperfleet/hyperfleet-api
Length of output: 6480
🏁 Script executed:
#!/bin/bash
# 1. Show full bindAllEnvVars() method to see what is/isn't bound
sed -n '295,350p' pkg/config/loader.go
echo "--- CHECK: is server.jwt.identity_claim ever bound? ---"
rg -n 'server\.jwt\.identity_claim|bindEnv.*identity' pkg/config/
echo "--- CHECK: what calls the JSON array parsing code at lines 260-264? ---"
rg -n 'jwt.*Configs\|configs.*\[\]' pkg/config/loader.go | head -20Repository: openshift-hyperfleet/hyperfleet-api
Length of output: 2272
🏁 Script executed:
#!/bin/bash
# 2. Check if server.jwt.configs binding exists or if it's AutomaticEnv only
echo "--- Full search for jwt.configs binding/handling ---"
rg -n 'jwt\.configs|JwtConfigs' pkg/config/
echo "--- Check Viper binding/Get calls for jwt.configs ---"
ast-grep run --pattern 'viper.Bind$_("server.jwt.configs")' --lang go pkg/config/Repository: openshift-hyperfleet/hyperfleet-api
Length of output: 694
🏁 Script executed:
#!/bin/bash
# 3. Check git history — were identity_claim/identity_header ever in loader.go?
echo "--- Git log for pkg/config/loader.go (recent commits) ---"
git log --oneline -20 -- pkg/config/loader.go
echo "--- Git show: what was in bindAllEnvVars before? ---"
git log -p --follow -S 'server.jwt.identity_claim' -- pkg/config/loader.go | head -100Repository: openshift-hyperfleet/hyperfleet-api
Length of output: 359
🏁 Script executed:
#!/bin/bash
# Show the context around the JSON parsing code to find what function uses it
sed -n '250,280p' pkg/config/loader.goRepository: openshift-hyperfleet/hyperfleet-api
Length of output: 1467
🏁 Script executed:
#!/bin/bash
# Find function containing lines 260-264 JSON parsing
sed -n '200,320p' pkg/config/loader.go | grep -B 40 'Always set the parsed JSON array value'Repository: openshift-hyperfleet/hyperfleet-api
Length of output: 1792
🏁 Script executed:
#!/bin/bash
# Search for where jwt.configs is actually parsed/read from env
rg -B5 -A5 'jwt.*Configs.*\[\]|Viper.*jwt' pkg/config/Repository: openshift-hyperfleet/hyperfleet-api
Length of output: 173
🏁 Script executed:
#!/bin/bash
# Check if server.jwt.configs is in handleJSONArrayEnvVars mappings
sed -n '235,275p' pkg/config/loader.go | grep -A 20 'jsonArrayMappings'Repository: openshift-hyperfleet/hyperfleet-api
Length of output: 1277
🏁 Script executed:
#!/bin/bash
# Search for where server.jwt.configs env var is actually handled
echo "=== Search for JWT_CONFIGS handling in Load() or elsewhere ==="
rg -n 'jwt\.configs|JWT_CONFIGS' pkg/config/loader.go
echo "=== Check if Load() calls handleJSONArrayEnvVars ==="
rg -B10 'handleJSONArrayEnvVars' pkg/config/loader.go | head -30Repository: openshift-hyperfleet/hyperfleet-api
Length of output: 1369
🏁 Script executed:
#!/bin/bash
# Check where server.jwt.configs is actually populated — config file only?
echo "=== How jwt.configs is set in server.go Validate() ==="
sed -n '80,100p' pkg/config/server.go
echo "=== Full JWT struct definition ==="
ast-grep outline pkg/config/server.go | grep -A 30 'JWT\|Jwt'Repository: openshift-hyperfleet/hyperfleet-api
Length of output: 1676
Environment variable documentation inconsistency: HYPERFLEET_SERVER_JWT_CONFIGS is advertised in docs but not implemented.
The docs (config.md:394, authentication.md:240) document HYPERFLEET_SERVER_JWT_CONFIGS as a supported JSON environment variable override for JWT issuer configurations. However, the code does not handle this: server.jwt.configs is neither bound in bindAllEnvVars() (line 311) nor included in handleJSONArrayEnvVars() (lines 235–275), which is the only place JSON array env vars are parsed. Without explicit handling, relying on AutomaticEnv() alone would fail to parse the JSON array correctly (Viper treats commas as CSV delimiters, not JSON syntax).
Either remove the env var from docs or add server.jwt.configs to the handleJSONArrayEnvVars() mapping to actually support it. The comment at line 311 is correct: JWT configs are currently YAML-only.
(Note: the original comment referenced removal of server.jwt.identity_claim / server.identity_header env bindings, but these keys do not exist in the codebase — no prior binding to remove.)
🤖 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/config/loader.go` around lines 306 - 311, The documentation advertises
`HYPERFLEET_SERVER_JWT_CONFIGS` as a supported environment variable override for
JWT configurations, but the implementation does not actually handle this
environment variable. The config loader does not bind `server.jwt.configs` in
the `bindAllEnvVars()` function around line 306-311, nor does it include this
key in the `handleJSONArrayEnvVars()` function (lines 235-275) which is required
to properly parse JSON array environment variables. You must resolve this
inconsistency by either: 1) removing `HYPERFLEET_SERVER_JWT_CONFIGS` from the
documentation (config.md and authentication.md) since JWT configs are currently
YAML-only as noted in the comment, OR 2) adding the `server.jwt.configs` key
mapping to the `handleJSONArrayEnvVars()` function to actually implement support
for the JSON environment variable. Choose the approach that aligns with the
intended design and implement it consistently across both code and
documentation.
Source: Path instructions
| // Configure JWK certificate URL for the first (and only) integration test issuer | ||
| helper.Env().Config.Server.JWT.Configs[0].JWKCertURL = jwkURL |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Unguarded Configs[0] index — panic on empty slice (CWE-129).
startAPIServer indexes Server.JWT.Configs[0] with no length check, while every sibling helper in this file (IdentityHeaderName L385, StartJWKCertServerMock L393, CreateJWTString L596) guards with len(cfgs) > 0. Any test path that constructs a helper without populating Configs (e.g. JWT disabled, or a future fixture change) triggers index out of range and crashes the test binary instead of failing cleanly.
Proposed guard
- // Configure JWK certificate URL for the first (and only) integration test issuer
- helper.Env().Config.Server.JWT.Configs[0].JWKCertURL = jwkURL
+ // Configure JWK certificate URL for the first (and only) integration test issuer
+ if cfgs := helper.Env().Config.Server.JWT.Configs; len(cfgs) > 0 {
+ cfgs[0].JWKCertURL = jwkURL
+ }As per path instructions (**/*.go): "Flag nil/bounds access without guards."
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Configure JWK certificate URL for the first (and only) integration test issuer | |
| helper.Env().Config.Server.JWT.Configs[0].JWKCertURL = jwkURL | |
| // Configure JWK certificate URL for the first (and only) integration test issuer | |
| if cfgs := helper.Env().Config.Server.JWT.Configs; len(cfgs) > 0 { | |
| cfgs[0].JWKCertURL = jwkURL | |
| } |
🤖 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 `@test/helper.go` around lines 159 - 160, The code at the index assignment
`helper.Env().Config.Server.JWT.Configs[0] = jwkURL` accesses the Configs slice
without checking if it contains any elements, which can cause an index out of
range panic if the slice is empty. Add a guard check using
`len(helper.Env().Config.Server.JWT.Configs) > 0` before this line, following
the same pattern used by other helper methods in the file like
IdentityHeaderName, StartJWKCertServerMock, and CreateJWTString. Only assign the
jwkURL if the Configs slice has at least one element.
Source: Path instructions
1 participant
Summary
issuer_url,audience,identity_claim,identity_claim_pattern, plus the globalidentity_headerandjwk.*) with an ordered listserver.jwt.configs. A request is accepted if its token validates against any configured issuer — enabling multi-IdP deployments (e.g. RH SSO + Google) without forking the binary.JWTHandlernow compiles each issuer at startup (keyfunc, parser, identity pattern) and tries them in sequence; the first match stores the matched issuer's identity config in the request context forCallerIdentityMiddlewareto consume.issuer_url,audience,jwk_cert_url,jwk_cert_file,identity_claim,identity_claim_pattern,header.Config shape change
Before:
After:
Test plan
make verifypasses (vet + gofmt)make test— all 1241 unit tests pass, including newTestJWTHandler_MultiIssuer(token accepted by issuer A, token accepted by issuer B, unknown issuer rejected, matched identity config stored in context)make test-integration— auth integration tests against a real DB🤖 Generated with Claude Code