HYPERFLEET-1288 - feat: support multiple JWT issuers with per-issuer config (DRAFT)#285
HYPERFLEET-1288 - feat: support multiple JWT issuers with per-issuer config (DRAFT)#285pnguyen44 wants to merge 1 commit into
Conversation
|
[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 |
📝 WalkthroughWalkthroughJWT auth now uses Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error)
✅ Passed checks (10 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
Comment |
Risk Score: 5 —
|
| Signal | Detail | Points |
|---|---|---|
| PR size | 1711 lines (>500) | +2 |
| Sensitive paths | cmd/ | +2 |
| Test coverage | Missing tests for: cmd/hyperfleet-api/environments cmd/hyperfleet-api/server pkg/validation test | +1 |
Computed by hyperfleet-risk-scorer
|
/retest |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
pkg/auth/auth_middleware.go (1)
35-53: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winSilently discarded identity-resolution error on the non-mutating path.
When
CallerIdentityFromRequestreturns a non-emptyerron a read request (e.g. malformed/oversized/pattern-rejected identity header or claim), execution falls through to Line 53 and serves the request anonymously with the error dropped — no log, no comment. That masks misconfiguration and rejected identity input at a security boundary. Make the degradation explicit (log at Warn + comment) rather than swallowing it.As per path instructions: "Log-and-continue MUST be intentional degradation with a comment, not a missing return."
Proposed change
if isMutatingMethod(r.Method) { msg := "Caller identity is required for mutating requests but could not be resolved" if err != nil { msg = fmt.Sprintf("Unable to resolve caller identity: %s", err) } handleError(ctx, w, r, errors.CodeAuthNoCredentials, msg) return } + // Non-mutating requests may proceed unauthenticated; surface (don't swallow) + // any identity-resolution error for observability. + if err != nil { + logger.WithError(ctx, err).Warn("caller identity resolution failed on non-mutating request; proceeding anonymously") + } next.ServeHTTP(w, r)🤖 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/auth_middleware.go` around lines 35 - 53, The non-mutating branch in auth_middleware.go is silently swallowing a non-nil error from CallerIdentityFromRequest when identity is empty, so make the fallback explicit in that path. In authMiddleware, before calling next.ServeHTTP for read requests, add a Warn-level log that records the CallerIdentityFromRequest failure and add a brief comment explaining this intentional anonymous degradation; keep mutating requests using handleError as they do now.Source: Path instructions
pkg/auth/jwt_handler.go (1)
191-212: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winReject non-HTTPS
server.jwt.configs[].jwk_cert_urlin config validation
validate:"omitempty,url"acceptshttp://, andbuildKeyfuncwill fetch JWKS over that URL. That enables cleartext key retrieval and key substitution/token forgery (CWE-319, CWE-345). Enforcehttps://before this reachespkg/auth/jwt_handler.go.🤖 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 191 - 212, The JWT config validation currently allows non-HTTPS jwk_cert_url values, which lets buildKeyfunc in jwt_handler.go fetch JWKS over cleartext. Tighten validation for server.jwt.configs[].jwk_cert_url so only https:// URLs are accepted, and reject any http:// or non-HTTPS scheme before buildKeyfunc can call keyfunc.NewDefaultCtx or jwkset.NewHTTPClient.
🧹 Nitpick comments (6)
charts/values.schema.json (1)
151-188: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winSchema doesn't enforce a JWK source or exclusivity for it.
configs[]items only requireissuer_url. An operator can--set-jsonan issuer with neitherjwk_cert_urlnorjwk_cert_file(or with both), and this passeshelm template --validate/schema checks. Signature verification then depends entirely on Go-side startup validation catching it — if that check has a gap, JWTHandler may build a keyfunc with no key material (CWE-347: Improper Verification of Cryptographic Signature). Enforce it at the schema level too so bad configs fail before deployment, not at pod crash-loop.🔒 Proposed fix: require exactly one JWK source
"items": { "type": "object", "properties": { "issuer_url": { "type": "string", "description": "OIDC issuer URL for token validation (required)" }, "jwk_cert_url": { "type": "string", "description": "URL to fetch JWK certificates from" }, "jwk_cert_file": { "type": "string", "description": "Path to a local JWK certificate file" }, ... }, - "required": ["issuer_url"] + "required": ["issuer_url"], + "oneOf": [ + { "required": ["jwk_cert_url"], "not": { "required": ["jwk_cert_file"] } }, + { "required": ["jwk_cert_file"], "not": { "required": ["jwk_cert_url"] } } + ] }🤖 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 `@charts/values.schema.json` around lines 151 - 188, The configs[] item schema only requires issuer_url and does not enforce a JWK source, so update values.schema.json to make jwt/JWK configuration valid only when exactly one of jwk_cert_url or jwk_cert_file is provided. Add schema-level validation for the item object under configs to require one and only one JWK source, so invalid combinations are rejected by Helm before deployment. Use the existing issuer_url, jwk_cert_url, jwk_cert_file, and configs item definition to place the constraint.docs/authentication.md (1)
176-186: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winDocument forbidden header values for
header/identity_header.Per PR objectives, validation now rejects
Cookie/Set-Cookie(and other collisions) for JWT source/identity headers — this prevents Bearer-style auth from being confused with cookie-based auth (CWE-352 relevant: cookies auto-attach cross-origin, unlike explicit headers). The field reference table and per-issuer examples don't mention this restriction, so operators will hit an undocumented startup validation error.📝 Suggested table note
| `header` | No | `Authorization` | HTTP header to read the JWT from | | `audience` | No | `""` (any) | Expected `aud` claim; skipped if empty | | `identity_claim` | No | `email` | JWT claim used as audit identity | | `identity_claim_pattern` | No | `""` (none) | Regex the identity value must match; non-matching requests get 401 | | `identity_header` | No | `""` (disabled) | HTTP header that overrides the JWT claim for audit identity (gateway-set only) | + +> `header` and `identity_header` must not be `Cookie` or `Set-Cookie` (and cannot collide with each other) — configuration validation rejects these at startup.Also applies to: 234-249
🤖 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 `@docs/authentication.md` around lines 176 - 186, Update the authentication docs to mention that `header` and `identity_header` must not use forbidden values like `Cookie` or `Set-Cookie`, and note that other header collisions are rejected by startup validation. Add this restriction to the field reference table for `header`/`identity_header` and to the per-issuer examples so readers of `authentication.md` can see the constraint before configuring JWT source headers.docs/config.md (2)
292-294: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winHeading level skips from h2 to h4.
#### Caller Identityfollows## Advanced Configurationwith no h3 in between (the<summary>tag isn't a real heading), violating MD001.📝 Proposed fix
-#### Caller Identity +### Caller Identity🤖 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 `@docs/config.md` around lines 292 - 294, The heading hierarchy is skipping a level in the configuration docs: the existing Caller Identity section is using a fourth-level heading immediately after a second-level section. Update the markdown heading in the Caller Identity section to the appropriate third-level heading so it follows the surrounding structure and satisfies MD001, keeping the section name and link target consistent.Source: Linters/SAST tools
502-507: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winValidation Rules section omits
server.jwt.configs.The rewritten Server validation bullets (port, timeouts) don't mention the new JWT issuer validation (required
issuer_url, one ofjwk_cert_url/jwk_cert_file, forbidden header values, identity pattern rules) even though this is the most significant breaking/validated surface in this release.📝 Suggested addition
**Server**: - `server.port`: 1-65535 - `server.timeouts.read`: ≥ 1s - `server.timeouts.write`: ≥ 1s +- `server.jwt.configs`: required non-empty when `server.jwt.enabled=true`; see [Issuer configuration reference](authentication.md#issuer-configuration-reference) for per-field validation rules🤖 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 `@docs/config.md` around lines 502 - 507, The Server validation rules section is missing the new `server.jwt.configs` requirements, so update the Server bullets to include the JWT validation surface alongside `server.port` and timeouts. Add the key constraints from the `server.jwt.configs` schema: each config must have `issuer_url`, must provide exactly one of `jwk_cert_url` or `jwk_cert_file`, must reject forbidden header values, and must enforce the identity pattern rules. Keep the wording concise but explicit so readers can see the full set of Server validation requirements at a glance.pkg/config/server_test.go (1)
106-139: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd negative coverage for the two untested security controls.
Validate()enforces (a) forbidden JWT source header (cfg.Header=Cookie/Set-Cookie) and (b)headervsidentity_headercollision. Neither branch is exercised, so a regression that drops either check would pass CI. Error paths of security validators must be tested (TEST-02).Suggested subtests
t.Run("forbidden JWT source header fails", func(t *testing.T) { RegisterTestingT(t) cfg := JWTConfig{Enabled: true, Configs: []JWTIssuerConfig{{ IssuerURL: "https://issuer.example.com", JWKCertURL: "https://keys.example.com", Header: "Cookie", }}} err := cfg.Validate() Expect(err).To(HaveOccurred()) Expect(err.Error()).To(ContainSubstring("not allowed as a JWT source")) }) t.Run("header equal to identity_header fails", func(t *testing.T) { RegisterTestingT(t) cfg := JWTConfig{Enabled: true, Configs: []JWTIssuerConfig{{ IssuerURL: "https://issuer.example.com", JWKCertURL: "https://keys.example.com", Header: "X-Token", IdentityHeader: "x-token", }}} err := cfg.Validate() Expect(err).To(HaveOccurred()) Expect(err.Error()).To(ContainSubstring("must differ from header")) })🤖 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_test.go` around lines 106 - 139, Add negative test coverage in TestJWTConfig_ValidateIdentityHeader for the security checks in JWTConfig.Validate(). Create subtests that exercise the forbidden JWT source header path by setting JWTIssuerConfig.Header to Cookie or Set-Cookie and asserting validation fails with the expected “not allowed as a JWT source” message, and another subtest that sets Header and IdentityHeader to the same value (case-insensitive) and asserts validation fails with the “must differ from header” message. Keep using the existing base helper and validate through cfg.Validate() so the branches in JWTConfig and JWTIssuerConfig are directly covered.Source: Path instructions
pkg/config/loader.go (1)
305-307: 🔒 Security & Privacy | 🔵 Trivial | 💤 Low valueEnv/CLI override removal shifts JWT trust entirely to the YAML file (CWE-732). Since
server.jwt.configscan no longer be set/overridden via env vars or flags, issuer URLs, JWK sources, and identity settings live only in the config file. Ensure deployments mount this file read-only with restrictive ownership/permissions and document the loss of env-var override for these security-sensitive fields (path instruction: "Environment variable overrides documented").Also applies to: 375-376
🤖 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 305 - 307, The JWT config override path was removed, so the security-sensitive settings under server.jwt.configs now come only from the YAML file. Update the config-loading/docs flow around l.bindEnv and the server.jwt.enabled handling to clearly document that issuer URLs, JWK sources, and identity settings are no longer env/CLI overridable, and add deployment guidance that this file must be mounted read-only with restrictive ownership and permissions. Also update the affected documentation section for environment variable overrides so users know these fields must be managed in the config file only.Source: Path instructions
🤖 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 `@test/helper.go`:
- Around line 179-182: The default identity header is only being set in
startAPIServer, which can make IdentityHeaderName() return an empty value when
configs are read through other paths. Extract the "X-HyperFleet-Identity"
literal into a shared constant and apply it during config defaulting so both
startAPIServer and IdentityHeaderName() resolve the same value. Make sure the
default is set in the shared config setup used by
helper.Env()/environments.Environment() rather than only at server startup.
---
Outside diff comments:
In `@pkg/auth/auth_middleware.go`:
- Around line 35-53: The non-mutating branch in auth_middleware.go is silently
swallowing a non-nil error from CallerIdentityFromRequest when identity is
empty, so make the fallback explicit in that path. In authMiddleware, before
calling next.ServeHTTP for read requests, add a Warn-level log that records the
CallerIdentityFromRequest failure and add a brief comment explaining this
intentional anonymous degradation; keep mutating requests using handleError as
they do now.
In `@pkg/auth/jwt_handler.go`:
- Around line 191-212: The JWT config validation currently allows non-HTTPS
jwk_cert_url values, which lets buildKeyfunc in jwt_handler.go fetch JWKS over
cleartext. Tighten validation for server.jwt.configs[].jwk_cert_url so only
https:// URLs are accepted, and reject any http:// or non-HTTPS scheme before
buildKeyfunc can call keyfunc.NewDefaultCtx or jwkset.NewHTTPClient.
---
Nitpick comments:
In `@charts/values.schema.json`:
- Around line 151-188: The configs[] item schema only requires issuer_url and
does not enforce a JWK source, so update values.schema.json to make jwt/JWK
configuration valid only when exactly one of jwk_cert_url or jwk_cert_file is
provided. Add schema-level validation for the item object under configs to
require one and only one JWK source, so invalid combinations are rejected by
Helm before deployment. Use the existing issuer_url, jwk_cert_url,
jwk_cert_file, and configs item definition to place the constraint.
In `@docs/authentication.md`:
- Around line 176-186: Update the authentication docs to mention that `header`
and `identity_header` must not use forbidden values like `Cookie` or
`Set-Cookie`, and note that other header collisions are rejected by startup
validation. Add this restriction to the field reference table for
`header`/`identity_header` and to the per-issuer examples so readers of
`authentication.md` can see the constraint before configuring JWT source
headers.
In `@docs/config.md`:
- Around line 292-294: The heading hierarchy is skipping a level in the
configuration docs: the existing Caller Identity section is using a fourth-level
heading immediately after a second-level section. Update the markdown heading in
the Caller Identity section to the appropriate third-level heading so it follows
the surrounding structure and satisfies MD001, keeping the section name and link
target consistent.
- Around line 502-507: The Server validation rules section is missing the new
`server.jwt.configs` requirements, so update the Server bullets to include the
JWT validation surface alongside `server.port` and timeouts. Add the key
constraints from the `server.jwt.configs` schema: each config must have
`issuer_url`, must provide exactly one of `jwk_cert_url` or `jwk_cert_file`,
must reject forbidden header values, and must enforce the identity pattern
rules. Keep the wording concise but explicit so readers can see the full set of
Server validation requirements at a glance.
In `@pkg/config/loader.go`:
- Around line 305-307: The JWT config override path was removed, so the
security-sensitive settings under server.jwt.configs now come only from the YAML
file. Update the config-loading/docs flow around l.bindEnv and the
server.jwt.enabled handling to clearly document that issuer URLs, JWK sources,
and identity settings are no longer env/CLI overridable, and add deployment
guidance that this file must be mounted read-only with restrictive ownership and
permissions. Also update the affected documentation section for environment
variable overrides so users know these fields must be managed in the config file
only.
In `@pkg/config/server_test.go`:
- Around line 106-139: Add negative test coverage in
TestJWTConfig_ValidateIdentityHeader for the security checks in
JWTConfig.Validate(). Create subtests that exercise the forbidden JWT source
header path by setting JWTIssuerConfig.Header to Cookie or Set-Cookie and
asserting validation fails with the expected “not allowed as a JWT source”
message, and another subtest that sets Header and IdentityHeader to the same
value (case-insensitive) and asserts validation fails with the “must differ from
header” message. Keep using the existing base helper and validate through
cfg.Validate() so the branches in JWTConfig and JWTIssuerConfig are directly
covered.
🪄 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: 8d30c17b-6936-4d05-8b9f-61aae07b9860
📒 Files selected for processing (29)
charts/README.mdcharts/templates/NOTES.txtcharts/templates/configmap.yamlcharts/values.schema.jsoncharts/values.yamlcmd/hyperfleet-api/server/api_server.gocmd/hyperfleet-api/server/routes.goconfigs/config.yaml.exampledocs/api-operator-guide.mddocs/authentication.mddocs/config.mddocs/deployment.mdpkg/auth/auth_middleware.gopkg/auth/context.gopkg/auth/identity.gopkg/auth/identity_test.gopkg/auth/jwt_handler.gopkg/auth/jwt_handler_test.gopkg/config/dump.gopkg/config/dump_test.gopkg/config/flags.gopkg/config/helpers_test.gopkg/config/loader.gopkg/config/loader_test.gopkg/config/server.gopkg/config/server_test.gopkg/validation/identity_header.gotest/helper.gotest/integration/integration_test.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)
- test/integration/integration_test.go
- pkg/config/flags.go
pnguyen44
changed the title
pnguyen44
force-pushed
the
HYPERFLEET-1288
branch
from
5606b4c to
3dd95f8
Compare
|
/test presubmits-integration |
pnguyen44
force-pushed
the
HYPERFLEET-1288
branch
from
3dd95f8 to
1c5cba2
Compare
1 participant
Summary
HYPERFLEET-1288
The API can now authenticate requests from multiple identity providers. Each issuer is configured independently with its own JWK source, token header, audience, and identity resolution settings. This enables scenarios like accepting tokens from both an API gateway (e.g. Keycloak) and GCP service accounts without proxying through a single IdP.
Breaking change: the flat
server.jwt.*/server.jwk.*/server.identity_headerfields are replaced by aserver.jwt.configs[]list. Existing deployments must migrate to the new format.Changes
server.jwt.configs[]listcreated_by) is now per-issuer, not globalTest Plan
make test-allpassesmake lintpassesmake test-helm(if applicable)