Conversation
Signed-off-by: Sebastian Łaskawiec <[email protected]>
Signed-off-by: Sebastian Łaskawiec <[email protected]>
…iance Signed-off-by: Sebastian Łaskawiec <[email protected]>
…ainer Signed-off-by: Sebastian Łaskawiec <[email protected]>
…torePaths and k8s CA Signed-off-by: Sebastian Łaskawiec <[email protected]>
…lidation (core-604) Signed-off-by: Sebastian Łaskawiec <[email protected]>
|
@codex review |
Greptile SummaryThis PR routes Keycloak away from its upstream
Confidence Score: 3/5Not safe to merge standalone — the chart depends on a BCFKS truststore that the referenced configImage (0.28.0) does not produce; deploying today would leave Keycloak without a system truststore and break all outgoing TLS connections. The chart changes are logically sound and the new test coverage is thorough, but they are coupled to a companion PR (uds-identity-config#878) that has not yet been merged. The current sync.sh in identity-config:0.28.0 neither reads TRUSTSTORE_PATHS nor creates keycloak-truststore.bcfks — it still copies certs into conf/truststores, which is exactly what this PR aims to stop. Until the companion lands and configImage is bumped, any deployment using this chart will start Keycloak with a JAVA_OPTS_APPEND pointing at a file that does not exist. src/keycloak/chart/values.yaml — configImage must be bumped to a version that includes the BCFKS truststore builder from the companion PR before this PR can be merged.
|
| Filename | Overview |
|---|---|
| src/keycloak/chart/templates/statefulset.yaml | Core chart changes: removes --truststore-paths, adds --https-key-store-type/trust-store-type=bcfks and --truststore-kubernetes-enabled=false unconditionally, injects JAVA_OPTS_APPEND with JVM truststore properties always, and passes TRUSTSTORE_PATHS env to the uds-config init container. Has a minor comment typo (bcfjks) and the BCFKS truststore password is hardcoded in plaintext. |
| src/keycloak/chart/values.yaml | configImage still references identity-config:0.28.0 which does not support BCFKS truststore building; truststorePaths comment updated to reflect new BCFKS-centric behavior. |
| src/keycloak/chart/tests/kc_truststore_paths_test.yaml | Test suite renamed and significantly expanded to cover BCFKS truststore behavior: verifies --truststore-paths is absent, --truststore-kubernetes-enabled=false is present, TRUSTSTORE_PATHS env is passed to init container, extraVolumeMounts are forwarded, and JAVA_OPTS_APPEND contains the correct JVM truststore properties. |
| src/keycloak/chart/tests/kc_fips_mode_test.yaml | Adds tests for BCFKS key/trust store type args and verifies the migration init container command removes both .p12 and .bcfks truststores. |
| src/keycloak/chart/tests/kc_fips_jce_security_test.yaml | Updated tests to reflect that JAVA_OPTS_APPEND is now always injected (not conditional on cgr.dev); fips-sunjce.security path still only appears for cgr.dev images; fail-render guard now triggers for all images, not only cgr.dev. |
Sequence Diagram
%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
participant MIG as remove-generated-truststore (optional)
participant IC as uds-config init container
participant KC as Keycloak container
participant JVM as JVM / TLS stack
MIG->>MIG: rm keycloak-truststore.p12 + .bcfks
IC->>IC: Read TRUSTSTORE_PATHS env var
IC->>IC: build_bcfks.sh assembles BCFKS from DoD CAs + extra paths
IC->>KC: Write /opt/keycloak/data/keycloak-truststore.bcfks
IC->>KC: Leave conf/truststores/ empty (no TruststoreBuilder trigger)
KC->>KC: "Start with --https-key-store-type=bcfks, --https-trust-store-type=bcfks, --truststore-kubernetes-enabled=false"
KC->>JVM: "JAVA_OPTS_APPEND sets javax.net.ssl.trustStore=*.bcfks + trustStoreType=BCFKS"
JVM->>JVM: Outgoing TLS validated against BCFKS system truststore
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
sequenceDiagram
participant MIG as remove-generated-truststore (optional)
participant IC as uds-config init container
participant KC as Keycloak container
participant JVM as JVM / TLS stack
MIG->>MIG: rm keycloak-truststore.p12 + .bcfks
IC->>IC: Read TRUSTSTORE_PATHS env var
IC->>IC: build_bcfks.sh assembles BCFKS from DoD CAs + extra paths
IC->>KC: Write /opt/keycloak/data/keycloak-truststore.bcfks
IC->>KC: Leave conf/truststores/ empty (no TruststoreBuilder trigger)
KC->>KC: "Start with --https-key-store-type=bcfks, --https-trust-store-type=bcfks, --truststore-kubernetes-enabled=false"
KC->>JVM: "JAVA_OPTS_APPEND sets javax.net.ssl.trustStore=*.bcfks + trustStoreType=BCFKS"
JVM->>JVM: Outgoing TLS validated against BCFKS system truststore
Comments Outside Diff (1)
-
src/keycloak/chart/values.yaml, line 13 (link)configImagemust be bumped before this PR is merge-readyThe chart injects
TRUSTSTORE_PATHSinto theuds-configinit container and expects it to produce/opt/keycloak/data/keycloak-truststore.bcfks. However, the currentsync.shinuds-identity-config:0.28.0does neither — it ignoresTRUSTSTORE_PATHSentirely and still runscp -fv certs/* /opt/keycloak/conf/truststores(which populatesconf/truststoresand triggers Quarkus auto-discovery →TruststoreBuilder→ PKCS12, the exact problem this PR intends to fix). Deploying with the current image will leave the truststore file absent; the JVM will error out on any outgoing TLS connection and Keycloak will likely fail startup.
Reviews (1): Last reviewed commit: "fix(keycloak): align BCFKS truststore cl..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull request overview
This PR updates the Keycloak Helm chart to avoid Keycloak’s upstream truststore generation (PKCS12) and instead run with a BCFKS system truststore suitable for FIPS deployments.
Changes:
- Removes reliance on
--truststore-pathsby passingtruststorePathsinto theuds-configinit container viaTRUSTSTORE_PATHS. - Forces BCFKS store types and disables Kubernetes CA auto-inclusion to prevent Keycloak’s
TruststoreBuilderfrom producing PKCS12. - Updates Helm unit tests to validate the new startup args/env behavior and truststore cleanup behavior.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/keycloak/chart/values.yaml | Updates truststorePaths documentation to reflect BCFKS truststore build flow. |
| src/keycloak/chart/tests/kc_truststore_paths_test.yaml | Reworks tests to assert --truststore-paths is not rendered and that TRUSTSTORE_PATHS/JAVA_OPTS_APPEND are set appropriately. |
| src/keycloak/chart/tests/kc_fips_mode_test.yaml | Adds assertions that BCFKS key/trust store types are always present and cleanup removes both .p12 and .bcfks. |
| src/keycloak/chart/tests/kc_fips_jce_security_test.yaml | Adjusts tests to validate the JCE override is (not) included within the now-injected JAVA_OPTS_APPEND. |
| src/keycloak/chart/templates/statefulset.yaml | Implements the runtime behavior changes: env/args updates, init-container env wiring, and truststore cleanup expansion. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ad6f70766d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…mentation - Fix 'bcfjks' typo in comments by removing script name coupling (Comments 1,3,4) - Create dedicated Secret for truststore password and consume via secretKeyRef (Comment 2) - Add KC_TRUSTSTORE_PASSWORD env var to both init and main containers - Replace hardcoded truststore password with $(KC_TRUSTSTORE_PASSWORD) expansion - Move remove-generated-truststore init container before uds-config to avoid deleting rebuilt truststore (Comment 7) - Update test assertions for reordered init containers and env var expansion - Clarify values.yaml documentation that truststorePaths entries can be files or directories
slaskawi
changed the title
Status
Dependencies
This issue is a workaround for: keycloak/keycloak#50110
Features from this PR will be used in defenseunicorns/uds-identity-config#878
Summary
This Pull Request switches the PKCS12 default truststore, which has been confirmed with Chainguard as non-FIPS compliant, to BCFJKS.
Here are some highlights of the changes:
--truststore-paths(would triggerTruststoreBuilder→ PKCS12)--truststore-kubernetes-enabled=false(prevents k8s CA auto-include from triggeringTruststoreBuilder)--https-key-store-type=bcfksand--https-trust-store-type=bcfksunconditionallyJAVA_OPTS_APPENDpointingjavax.net.ssl.trustStoreat/opt/keycloak/data/keycloak-truststore.bcfks(BCFKS, passwordkeycloakchangeit)uds-configinit container (sync.sh) copies the pre-built BCFKS truststore into/opt/keycloak/data/— withconf/truststoresleft empty so Quarkus auto-discovery never callsTruststoreBuilderkeycloak-truststore.p12andkeycloak-truststore.bcfkson redeployRelated tickets
Manual verification
TBD