chore(core): Use p12 store directly with keycloak #2622
Conversation
github-actions
bot
added
comp:ci
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary of Changes
Hello @dmihalcik-virtru, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
This pull request updates Keycloak configurations to use PKCS#12 (P12) keystores instead of the older Java Keystore (JKS) format. This change aligns with modern Java practices, as JKS support is being deprecated since Java 9. The modifications involve updating configuration files, build scripts, and test data to reflect this new certificate store format.
Highlights
- Keycloak Configuration Migration: I've updated the docker-compose.yaml and oauth_test.go files to configure Keycloak to use PKCS#12 (.p12) files for its truststore and HTTPS key store, moving away from the deprecated JKS format.
- Certificate Generation Script Updates: I've modified the init-temp-keys.cmd and init-temp-keys.sh scripts to directly generate PKCS#12 files using openssl pkcs12 and removed the keytool commands that previously converted P12 to JKS. This simplifies the certificate generation process.
- Updated Test Certificates: I've refreshed the test certificate and key files (keycloak-ca.pem, keycloak-ca.srl, localhost.crt, localhost.key, sampleuser.crt, sampleuser.key) within the sdk/auth/oauth/testdata directory to reflect the new P12-centric setup.
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments or fill out our survey to provide feedback.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Old JKS fades, PKCS#12 now takes hold, Keycloak's trust renewed.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Code Review
This pull request is a good step towards modernizing the Keycloak configuration by using a PKCS#12 store directly instead of the deprecated JKS format. The changes to the initialization scripts and the main configuration files are mostly correct. However, I've identified a few inconsistencies in docker-compose.yaml and sdk/auth/oauth/oauth_test.go where some outdated or redundant settings for passwords and keystore files remain. Additionally, a critical test data file, testdata/new-ca.p12, appears to be missing from the changes, which would likely cause tests to fail. My review comments provide specific details to help resolve these issues.
| "--spi-truststore-file-hostname-verification-policy=ANY", | ||
| }, | ||
| Files: []tc.ContainerFile{ | ||
| {HostFilePath: "testdata/new-ca.jks", ContainerFilePath: "/truststore/truststore.jks", FileMode: int64(0o777)}, | ||
| {HostFilePath: "testdata/new-ca.p12", ContainerFilePath: "/truststore/truststore.p12", FileMode: int64(0o777)}, |
There was a problem hiding this comment.
The test setup now references testdata/new-ca.p12, but this file has not been added to the repository in this pull request. The old testdata/new-ca.jks file seems to be replaced by it. Please generate and add testdata/new-ca.p12 and remove testdata/new-ca.jks from version control.
| {HostFilePath: "testdata/localhost.crt", ContainerFilePath: "/etc/x509/tls/localhost.crt", FileMode: int64(0o777)}, | ||
| {HostFilePath: "testdata/localhost.key", ContainerFilePath: "/etc/x509/tls/localhost.key", FileMode: int64(0o777)}, | ||
| }, | ||
| Env: map[string]string{ | ||
| "KEYCLOAK_ADMIN": "admin", | ||
| "KEYCLOAK_ADMIN_PASSWORD": "admin", | ||
| "KC_HTTPS_KEY_STORE_PASSWORD": "password", | ||
| "KC_HTTPS_KEY_STORE_FILE": "/truststore/truststore.jks", | ||
| "KC_HTTPS_KEY_STORE_FILE": "/truststore/truststore.p12", |
There was a problem hiding this comment.
This test configuration has some inconsistencies that should be addressed:
- The
-Djavax.net.ssl.trustStorePassword=passwordon line 548 is incorrect as the new.p12truststore is passwordless and should be removed. - The
KC_HTTPS_KEY_STORE_PASSWORDon line 561 should be removed for consistency withdocker-compose.yamland because the p12 file is passwordless. - The
KC_HTTPS_KEY_STORE_FILEon this line is redundant asKC_HTTPS_CERTIFICATE_FILEandKC_HTTPS_CERTIFICATE_KEY_FILEtake precedence. It should also be removed.
Please update the test configuration to be consistent and correct.
| image: keycloak/keycloak:25.0 | ||
| restart: always | ||
| command: | ||
| - "start-dev" | ||
| - "--verbose" | ||
| - "-Djavax.net.ssl.trustStorePassword=password" | ||
| - "-Djavax.net.ssl.HostnameVerifier=AllowAll" | ||
| - "-Djavax.net.ssl.trustStore=/truststore/truststore.jks" | ||
| - "--truststore-paths=/truststore/truststore.p12" |
There was a problem hiding this comment.
With the switch to --truststore-paths, the Java system property -Djavax.net.ssl.trustStorePassword=password on line 15 is now incorrect. The new ca.p12 truststore is created with an empty password, so this property should be removed.
| @@ -37,8 +37,7 @@ | |||
| #KC_HOSTNAME_URL: http://localhost:8888/auth | |||
| KC_FEATURES: "preview,token-exchange" | |||
| KC_HEALTH_ENABLED: "true" | |||
| KC_HTTPS_KEY_STORE_PASSWORD: "password" | |||
| KC_HTTPS_KEY_STORE_FILE: "/truststore/truststore.jks" | |||
| KC_HTTPS_KEY_STORE_FILE: "/truststore/truststore.p12" | |||
There was a problem hiding this comment.
The KC_HTTPS_KEY_STORE_FILE setting is redundant because KC_HTTPS_CERTIFICATE_FILE and KC_HTTPS_CERTIFICATE_KEY_FILE are also configured and take precedence. It's also confusing as it points to the truststore file (ca.p12). To avoid confusion and simplify the configuration, this line should be removed.
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Standard Benchmark Metrics Skipped or FailedBulk Benchmark Results
TDF3 Benchmark Results:
NANOTDF Benchmark Results:
|
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Standard Benchmark Metrics Skipped or FailedBulk Benchmark Results
TDF3 Benchmark Results:
NANOTDF Benchmark Results:
|
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Standard Benchmark Metrics Skipped or FailedBulk Benchmark Results
TDF3 Benchmark Results:
NANOTDF Benchmark Results:
|
Proposed Changes
Checklist
Testing Instructions