feat(keycloak): add Vault Kubernetes and AppRole auth for the encryption proxy - #3239
Conversation
The keycloak-kms-proxy Vault Transit backend only supported a static token. Add an encryption.kms.vault.auth switch — "token" (default, unchanged) or "approle" — wiring KKP_VAULT_ROLE_ID / KKP_VAULT_SECRET_ID / KKP_VAULT_APPROLE_MOUNT from a Secret. With AppRole the proxy logs in and re-authenticates on demand, so no long-lived token is stored. Requires a keycloak-kms-proxy image with AppRole support (> v0.1.0). Assisted-By: Claude AI Signed-off-by: Kirill Ilin <stitch14@yandex.ru>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis PR updates the Keycloak KMS proxy chart for multi-mode Vault auth, adds a dedicated proxy ServiceAccount, introduces replica and DEK-set configuration, and expands encryption coverage in tests. ChangesVault auth and encryption wiring for KMS proxy
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Helm as Helm template
participant ProxyTpl as proxy.yaml
participant ServiceAccount as keycloak-kms-proxy
participant Pod as Proxy Pod
Helm->>ProxyTpl: render encryption settings
ProxyTpl->>ProxyTpl: derive vaultAuth and k8sAuth
ProxyTpl->>ServiceAccount: render ServiceAccount
ProxyTpl->>Pod: set serviceAccountName
ProxyTpl->>Pod: set automountServiceAccountToken
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
The notContains checks matched whole array elements, so a name-only content never equaled the valueFrom entry and the assertion could never fail. Add any:true so "AppRole stores no static token" and, symmetrically, "token mode renders no AppRole vars" are actually verified (confirmed by mutation-testing the template). Assisted-By: Claude AI Signed-off-by: Kirill Ilin <stitch14@yandex.ru>
Extend encryption.kms.vault.auth with "kubernetes" (recommended): the proxy logs in with its own ServiceAccount token, so no credential is stored in a Secret. "approle" and "token" remain. Render-time validation requires kubernetes.role and rejects unknown methods; helm-unittest covers the new path and the mutual absence of the other modes' env vars. Assisted-By: Claude AI Signed-off-by: Kirill Ilin <stitch14@yandex.ru>
Address review: the proxy pod ran under the namespace default ServiceAccount, which the keycloak StatefulSet also uses. Binding a Vault Kubernetes-auth role to default would hand the keycloak pod itself Transit access, defeating the isolation the proxy exists to provide. Add a dedicated keycloak-kms-proxy ServiceAccount, run the proxy pod under it, and mount its token only in the kubernetes auth mode (off otherwise, to minimise the credential surface). Docs name the SA to bind the Vault role to. Assisted-By: Claude AI Signed-off-by: Kirill Ilin <stitch14@yandex.ru>
…ount Lock in the SA-object token toggle (off by default, on only for kubernetes auth), complementing the pod-level assertions. Assisted-By: Claude AI Signed-off-by: Kirill Ilin <stitch14@yandex.ru>
v0.2.0 is the first release carrying the Kubernetes/AppRole auth modes this chart wires, so encryption.kms.vault.auth is now usable with the default image. Assisted-By: Claude AI Signed-off-by: Kirill Ilin <stitch14@yandex.ru>
Summary of ChangesHello, 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 enhances the keycloak-kms-proxy integration by introducing flexible Vault authentication options. By supporting Kubernetes and AppRole auth, users can now avoid storing long-lived Vault tokens in Kubernetes Secrets. The changes include infrastructure updates to use a dedicated ServiceAccount and rigorous testing to ensure secure and correct configuration across all supported modes. Highlights
New Features🧠 You can now enable Memory (public preview) to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe 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 by creating a comment using either
Customization To customize the 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 Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counterproductive. You can react with 👍 and 👎 on Gemini (@gemini-code-assist) comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request updates the Keycloak KMS proxy to version v0.2.0 and introduces support for Vault approle and kubernetes authentication methods alongside the existing token auth. It also creates a dedicated ServiceAccount for the proxy, mounting its token only when Kubernetes authentication is enabled, and adds extensive unit tests to validate these configurations. The reviewer feedback suggests quoting dynamic Helm template values (such as secret names and keys) to ensure correct YAML parsing.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| - name: KKP_VAULT_ROLE_ID | ||
| valueFrom: | ||
| secretKeyRef: | ||
| name: {{ $secretName }} | ||
| key: {{ $appRole.roleIdKey | default "role-id" }} | ||
| - name: KKP_VAULT_SECRET_ID | ||
| valueFrom: | ||
| secretKeyRef: | ||
| name: {{ $secretName }} | ||
| key: {{ $appRole.secretIdKey | default "secret-id" }} |
There was a problem hiding this comment.
It is a best practice in Helm templates to quote dynamic values (like secret names and keys) to ensure they are always treated as valid YAML strings and to prevent any parsing issues.
- name: KKP_VAULT_ROLE_ID
valueFrom:
secretKeyRef:
name: {{ $secretName | quote }}
key: {{ $appRole.roleIdKey | default "role-id" | quote }}
- name: KKP_VAULT_SECRET_ID
valueFrom:
secretKeyRef:
name: {{ $secretName | quote }}
key: {{ $appRole.secretIdKey | default "secret-id" | quote }}| valueFrom: | ||
| secretKeyRef: | ||
| name: {{ required "encryption.kms.vault.tokenSecretName is required when kms.backend=vault-transit" $vault.tokenSecretName }} | ||
| name: {{ required "encryption.kms.vault.tokenSecretName is required when vault.auth=token" $vault.tokenSecretName }} |
There was a problem hiding this comment.
Quote the dynamic secret name to ensure it is parsed correctly as a string in YAML.
name: {{ required "encryption.kms.vault.tokenSecretName is required when vault.auth=token" $vault.tokenSecretName | quote }}There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/system/keycloak/tests/encryption_test.yaml (1)
154-160: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a positive assertion for
KKP_VAULT_TOKENin the token-auth test.The test verifies
KKP_VAULT_ADDRis present andKKP_VAULT_ROLE_IDis absent, but never confirms thatKKP_VAULT_TOKENis actually rendered with the correctsecretKeyRef(name: keycloak-vault-token,key: token). Adding this assertion would close the loop on the token auth path.✨ Suggested addition
asserts: - contains: path: spec.template.spec.containers[0].env content: name: KKP_VAULT_ADDR value: https://vault.vault.svc:8200 + - contains: + path: spec.template.spec.containers[0].env + content: + name: KKP_VAULT_TOKEN + valueFrom: + secretKeyRef: + name: keycloak-vault-token + key: token # Token auth must not leak AppRole env vars. any:true matches on the # element name alone (helm-unittest otherwise deep-equals whole elements). - notContains:🤖 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 `@packages/system/keycloak/tests/encryption_test.yaml` around lines 154 - 160, The token-auth test in encryption_test.yaml only checks that KKP_VAULT_ROLE_ID is absent, but it never verifies that KKP_VAULT_TOKEN is rendered correctly. Add a positive assertion in the same token-auth test to check the env entry for KKP_VAULT_TOKEN in spec.template.spec.containers[0].env, and ensure its value comes from the expected secretKeyRef with name keycloak-vault-token and key token. Use the existing encryption_test.yaml token-auth block and the KKP_VAULT_TOKEN env symbol to locate the right place.
🤖 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.
Nitpick comments:
In `@packages/system/keycloak/tests/encryption_test.yaml`:
- Around line 154-160: The token-auth test in encryption_test.yaml only checks
that KKP_VAULT_ROLE_ID is absent, but it never verifies that KKP_VAULT_TOKEN is
rendered correctly. Add a positive assertion in the same token-auth test to
check the env entry for KKP_VAULT_TOKEN in spec.template.spec.containers[0].env,
and ensure its value comes from the expected secretKeyRef with name
keycloak-vault-token and key token. Use the existing encryption_test.yaml
token-auth block and the KKP_VAULT_TOKEN env symbol to locate the right place.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 9eaa81cf-e186-48b7-b311-061cf5af1506
📒 Files selected for processing (3)
packages/system/keycloak/templates/proxy.yamlpackages/system/keycloak/tests/encryption_test.yamlpackages/system/keycloak/values.yaml
…roxy Wire encryption.deksetSecretName -> KKP_DEKSET_FILE (mounted read-only) and make encryption.replicas configurable. Without a dekset the proxy mints an ephemeral DEK, which only works on a fresh database and a single replica. A shared wrapped DEK set is required to migrate an existing populated Keycloak (the proxy must reuse the DEK the backfill wrapped rows with) and to run more than one replica; a guard rejects replicas>1 without one. Assisted-By: Claude AI Signed-off-by: Kirill Ilin <stitch14@yandex.ru>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/system/keycloak/tests/encryption_test.yaml (1)
134-160: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winStrengthen env var isolation assertions across auth mode tests.
The PR objective states tests should "verify that unrelated environment variables are not rendered," but each auth mode test is missing some
notContainsassertions for env vars belonging to other modes:
- Token auth test (line 134-160): asserts
KKP_VAULT_ROLE_IDis absent but doesn't assertKKP_VAULT_TOKENis present, nor thatKKP_VAULT_SECRET_ID,KKP_VAULT_KUBERNETES_ROLE, orKKP_VAULT_APPROLE_MOUNTare absent.- AppRole test (line 232-276): asserts
KKP_VAULT_TOKENis absent but doesn't assertKKP_VAULT_KUBERNETES_ROLEorKKP_VAULT_KUBERNETES_MOUNTare absent.- Kubernetes test (line 293-329): asserts
KKP_VAULT_TOKENandKKP_VAULT_ROLE_IDare absent but doesn't assertKKP_VAULT_SECRET_IDorKKP_VAULT_APPROLE_MOUNTare absent.Adding these assertions is low-cost and closes the gap on the stated objective.
♻️ Suggested additions
# Token auth test (after line 160) + - notContains: + path: spec.template.spec.containers[0].env + any: true + content: + name: KKP_VAULT_SECRET_ID + - notContains: + path: spec.template.spec.containers[0].env + any: true + content: + name: KKP_VAULT_KUBERNETES_ROLE + - notContains: + path: spec.template.spec.containers[0].env + any: true + content: + name: KKP_VAULT_APPROLE_MOUNT + - contains: + path: spec.template.spec.containers[0].env + content: + name: KKP_VAULT_TOKEN + valueFrom: + secretKeyRef: + name: keycloak-vault-token + key: token # AppRole test (after line 276) + - notContains: + path: spec.template.spec.containers[0].env + any: true + content: + name: KKP_VAULT_KUBERNETES_ROLE + - notContains: + path: spec.template.spec.containers[0].env + any: true + content: + name: KKP_VAULT_KUBERNETES_MOUNT # Kubernetes test (after line 329) + - notContains: + path: spec.template.spec.containers[0].env + any: true + content: + name: KKP_VAULT_SECRET_ID + - notContains: + path: spec.template.spec.containers[0].env + any: true + content: + name: KKP_VAULT_APPROLE_MOUNTAlso applies to: 232-276, 293-329
🤖 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 `@packages/system/keycloak/tests/encryption_test.yaml` around lines 134 - 160, Strengthen the Vault auth mode coverage in the encryption tests by updating the token, AppRole, and Kubernetes cases in encryption_test.yaml to explicitly assert both presence of the mode-specific env var and absence of env vars from the other modes. In the Vault Transit/token-auth test, keep the KKP_VAULT_ADDR check, add an assertion that KKP_VAULT_TOKEN is rendered, and add notContains checks for KKP_VAULT_SECRET_ID, KKP_VAULT_KUBERNETES_ROLE, and KKP_VAULT_APPROLE_MOUNT alongside the existing KKP_VAULT_ROLE_ID exclusion. In the AppRole and Kubernetes test blocks, add the missing notContains assertions for the other modes’ env vars so each auth path only renders its own variables.
🤖 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.
Nitpick comments:
In `@packages/system/keycloak/tests/encryption_test.yaml`:
- Around line 134-160: Strengthen the Vault auth mode coverage in the encryption
tests by updating the token, AppRole, and Kubernetes cases in
encryption_test.yaml to explicitly assert both presence of the mode-specific env
var and absence of env vars from the other modes. In the Vault
Transit/token-auth test, keep the KKP_VAULT_ADDR check, add an assertion that
KKP_VAULT_TOKEN is rendered, and add notContains checks for KKP_VAULT_SECRET_ID,
KKP_VAULT_KUBERNETES_ROLE, and KKP_VAULT_APPROLE_MOUNT alongside the existing
KKP_VAULT_ROLE_ID exclusion. In the AppRole and Kubernetes test blocks, add the
missing notContains assertions for the other modes’ env vars so each auth path
only renders its own variables.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 1fdcf925-ae57-4b1d-9398-da13ecd3a95a
📒 Files selected for processing (3)
packages/system/keycloak/templates/proxy.yamlpackages/system/keycloak/tests/encryption_test.yamlpackages/system/keycloak/values.yaml
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/system/keycloak/templates/proxy.yaml
Assert KKP_DEKSET_FILE honors a non-default deksetSecretKey, closing the last uncovered branch of the dekset wiring. Assisted-By: Claude AI Signed-off-by: Kirill Ilin <stitch14@yandex.ru>
The release workflow publishes the OCI tag as 0.2.0 (docker/metadata-action strips the leading v from the git tag), so the chart default of v0.2.0 does not exist and pulls fail. Pin the real tag. Signed-off-by: Kirill Ilin <stitch14@yandex.ru>
Assisted-By: Claude AI Signed-off-by: Kirill Ilin <stitch14@yandex.ru>
VerdictLGTM with non-blocking notes The change is coherent, needed (the old proxy was token-auth-only, single-replica-only, and could not migrate a populated DB), off-by-default, well covered by helm-unittest (45 tests pass), and renders correctly for every auth mode; only minor documentation/convention debt remains. Findings[MINOR]
Claim mismatches[PARTIAL] "Uses Caveats
|
What this PR does
Makes the optional
keycloak-kms-proxyPII-encryption feature production-usable.Vault auth — new
encryption.kms.vault.authswitch:kubernetes— recommended. The proxy logs in with its own dedicated ServiceAccount (keycloak-kms-proxy; bind the Vault role tosystem:serviceaccount:<ns>:keycloak-kms-proxy), so no credential is stored in a Secret.approle—KKP_VAULT_ROLE_ID/KKP_VAULT_SECRET_IDfromencryption.kms.vault.appRole.secretName.token(default, unchanged) — Vault token fromtokenSecretName.The proxy pod runs under a dedicated ServiceAccount (never the namespace default), with its token mounted only for Kubernetes auth.
Shared DEK set —
encryption.deksetSecretName→KKP_DEKSET_FILE(mounted read-only), andencryption.replicasis now configurable. Without a dekset the proxy mints an ephemeral DEK (only safe on a fresh, empty database, single replica). A shared wrapped DEK set — minted by the proxy's backfill tool — is required to migrate an existing populated Keycloak and to run HA; a guard rejectsreplicas > 1without one.Render-time validation requires each mode's settings and rejects unknown values. helm-unittest covers all auth modes, the ServiceAccount/token toggle, and the dekset/replicas paths. Uses
keycloak-kms-proxyv0.2.0.Release note
Summary by CodeRabbit