HYPERFLEET-1340 - feat: support custom CA certificates for JWKS endpoint TLS#292
HYPERFLEET-1340 - feat: support custom CA certificates for JWKS endpoint TLS#292pnguyen44 wants to merge 2 commits 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 |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Central YAML (base), Organization UI (inherited) Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (2)
🔗 Linked repositories identifiedCodeRabbit considers these linked repositories for cross-repo context during reviews:
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds optional custom CA certificate support for TLS JWKS retrieval. JWT configuration validates that Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant JWTHandler
participant CAFile
participant JWKSClient
participant JWKS_Endpoint
JWTHandler->>CAFile: Read CA certificate PEM
JWTHandler->>JWKSClient: Configure TLS roots and JWKS storage
JWKSClient->>JWKS_Endpoint: Fetch JWKS over TLS
JWKS_Endpoint-->>JWKSClient: Return JWKS keys
JWKSClient-->>JWTHandler: Provide signing keys
🚥 Pre-merge checks | ✅ 10 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (10 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
pkg/auth/jwt_handler.go (2)
175-261: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
buildKeyfuncexceeds decomposition threshold; CA-storage logic duplicated across branches.The function spans ~86 lines with branching on
hasFile/hasURL/CA-presence in two places (196-222 and 235-247) that each independently decide between CA-aware storage and a plain/memory fallback. Extracting a shared helper (e.g.remoteJWKSStorage(ctx, url, caFile string) (jwkset.Storage, error)) would remove the duplication and shrinkbuildKeyfuncbelow the flagged size/branch threshold.♻️ Sketch of consolidated helper
func remoteJWKSStorage(ctx context.Context, url, caFile string) (jwkset.Storage, error) { if caFile == "" { return jwkset.NewMemoryStorage(), nil } return newStorageWithCA(ctx, url, caFile) }Both branches would then call
remoteJWKSStorage(ctx, issuer.JWKCertURL, issuer.JWKCertCAFile)instead of duplicating theif issuer.JWKCertCAFile != ""check.As per path instructions, "Functions >50 lines or >5 branching paths — flag for decomposition."
🤖 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 175 - 261, Decompose buildKeyfunc by extracting the repeated CA-aware remote storage selection into a helper such as remoteJWKSStorage(ctx, url, caFile string) (jwkset.Storage, error). Have it return newStorageWithCA when caFile is set and an appropriate memory storage otherwise, then use it in both URL-only and combined file/URL branches while preserving existing error context and behavior.Source: Path instructions
285-306: 🔒 Security & Privacy | 🔵 Trivial | 💤 Low valueSplit
buildKeyfuncinto smaller helpers. It mixes source selection, CA-backed storage setup, and keyfunc construction in one 80+ line function, and the URL+CA wiring is duplicated across branches.🤖 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 285 - 306, Refactor buildKeyfunc into focused helpers: isolate key-source selection, CA-backed HTTP client/storage setup, and final keyfunc construction, while preserving existing behavior. Consolidate duplicated URL-and-CA wiring through a shared helper that uses buildHTTPClientWithCA, and have buildKeyfunc orchestrate these helpers with clear error propagation.
🤖 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 `@charts/values.schema.json`:
- Around line 166-169: The Helm schema currently allows jwk_cert_ca_file without
its required jwk_cert_url. Update the relevant schema object containing
jwk_cert_ca_file with an item-level if/then rule: if jwk_cert_ca_file is
present, then require jwk_cert_url, matching the validation enforced by server
configuration.
In `@docs/authentication.md`:
- Line 181: Update the `jwk_cert_ca_file` entry in the authentication
configuration table to state that it is valid only when `jwk_cert_url` is
configured, rejected when used only with `jwk_cert_file`, and must reference an
existing PEM file available at the configured path inside the API container.
In `@pkg/config/dump.go`:
- Around line 82-83: Handle the ignored error in formatIssuers by replacing the
fmt.Fprintf call with a non-error-returning write, or propagate its error
through formatIssuers and DumpConfig; ensure issuer output cannot be silently
incomplete.
In `@scripts/test-helm.sh`:
- Around line 149-151: Update the CA path assertion in the test script to use
fixed-string matching rather than regular-expression matching. Modify
assert_contains or add a helper using grep -Fq --, and ensure the assertion for
/var/run/secrets/kubernetes.io/serviceaccount/ca.crt invokes that fixed-string
behavior.
---
Nitpick comments:
In `@pkg/auth/jwt_handler.go`:
- Around line 175-261: Decompose buildKeyfunc by extracting the repeated
CA-aware remote storage selection into a helper such as remoteJWKSStorage(ctx,
url, caFile string) (jwkset.Storage, error). Have it return newStorageWithCA
when caFile is set and an appropriate memory storage otherwise, then use it in
both URL-only and combined file/URL branches while preserving existing error
context and behavior.
- Around line 285-306: Refactor buildKeyfunc into focused helpers: isolate
key-source selection, CA-backed HTTP client/storage setup, and final keyfunc
construction, while preserving existing behavior. Consolidate duplicated
URL-and-CA wiring through a shared helper that uses buildHTTPClientWithCA, and
have buildKeyfunc orchestrate these helpers with clear error propagation.
🪄 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: 517d74d0-134e-477e-b723-4c96ff92e902
📒 Files selected for processing (12)
charts/templates/configmap.yamlcharts/values.schema.jsoncharts/values.yamlconfigs/config.yaml.exampledocs/authentication.mdpkg/auth/jwt_handler.gopkg/auth/jwt_handler_test.gopkg/config/dump.gopkg/config/dump_test.gopkg/config/server.gopkg/config/server_test.goscripts/test-helm.sh
🔗 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)
Risk Score: 1 —
|
| Signal | Detail | Points |
|---|---|---|
| PR size | 403 lines (>200) | +1 |
| Sensitive paths | none | +0 |
| Test coverage | Tests cover changed packages | +0 |
Computed by hyperfleet-risk-scorer
pnguyen44
force-pushed
the
HYPERFLEET-1340
branch
from
47a1bac to
48db2a4
Compare
|
/retest |
|
@pnguyen44: The following tests failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
2 participants
Summary
HYPERFLEET-1340
When deployed inside a Kubernetes cluster, the JWT handler cannot fetch JWKS from
https://kubernetes.default.svc.cluster.local/openid/v1/jwksbecause Go's default HTTP client does not trust the cluster CA. This adds ajwk_cert_ca_fileconfig field that lets operators point to a custom CA certificate (e.g./var/run/secrets/kubernetes.io/serviceaccount/ca.crt), enabling TLS-verified JWKS fetches without resorting to the static-file workaround.Changes
jwk_cert_ca_filetoJWTIssuerConfigwith config validation ensuring it is only set whenjwk_cert_urlis also presentjwk_cert_ca_fileis set,buildKeyfuncconstructs a custom HTTP client with a TLS config that trusts the specified CA, used for both URL-only and combined (file + URL) JWKS configurationsjwk_cert_ca_filewhen present, with matching values schema and documentation updatesTest plan
make test-allpassesmake lintpassesmake test-helm(if applicable)