feat(keycloak): add optional KMS-encrypting database proxy - #3003
Conversation
📝 WalkthroughWalkthroughAdds an optional ChangesKeycloak KMS DB Proxy
Sequence Diagram(s)sequenceDiagram
participant KC as Keycloak StatefulSet
participant P as keycloak-kms-proxy (Deployment)
participant KMS as KMS Backend (Static KEK / Vault Transit)
participant DB as CNPG Postgres
KC->>P: SQL query (port 5432, via keycloak-kms-proxy Service)
P->>KMS: unwrap column encryption key
KMS-->>P: column encryption key
P->>DB: encrypted SQL query (port from keycloak-db-app Secret)
DB-->>P: encrypted result rows
P->>P: decrypt columns using column key
P-->>KC: plaintext result rows
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
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 security of the Keycloak system package by enabling optional, transparent column-level PII encryption. By deploying a sidecar-like proxy that intercepts database traffic, sensitive information is encrypted before reaching the database and decrypted upon retrieval. The implementation is designed to be non-disruptive, remaining disabled by default, and supports both simple static key management and robust production-grade integration with HashiCorp Vault. 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 introduces transparent column-level PII encryption for the Keycloak database by deploying a keycloak-kms-proxy between Keycloak and its CloudNativePG database. When encryption.enabled is true, Keycloak's database endpoint is routed through this proxy, which supports both static KEK and HashiCorp Vault Transit backends. The feedback highlights opportunities to improve the robustness of the Helm templates by preventing potential nil pointer dereferences when lookup secrets or Vault configurations are missing, and recommends adding resource limits to the proxy deployment in accordance with the repository's style guidelines.
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.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
packages/system/keycloak/tests/encryption_test.yaml (1)
134-168: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winAdd negative-path unittest coverage for backend validation failures.
The suite currently verifies happy paths but not misconfiguration failures. Add cases for invalid
encryption.kms.backendand missing required Vault fields so render-time validation stays enforced.🤖 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 - 168, Add negative-path test cases to the encryption_test.yaml file to validate backend configuration failures. Create test cases that verify the Helm template rendering fails or produces appropriate error messages when: (1) an invalid or unsupported value is provided for encryption.kms.backend, and (2) required Vault configuration fields are missing (such as encryption.kms.vault.address, encryption.kms.vault.keyName, or encryption.kms.vault.tokenSecretName when the vault-transit backend is selected). These tests should ensure that misconfiguration is caught at render-time rather than silently proceeding with incomplete configuration.
🤖 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 `@packages/system/keycloak/templates/proxy.yaml`:
- Line 4: The KMS backend validation in proxy.yaml lacks guardrails that allow
misconfiguration to pass through silently. At line 4 where the static backend
condition is checked and around lines 103-121 where other backends are handled,
add explicit validation to fail fast: ensure the encryption.kms.backend value is
one of the explicitly supported options (static or vault), and for Vault backend
mode, require both encryption.kms.vault.address and encryption.kms.vault.keyName
to be non-empty at template rendering time. This prevents invalid backend values
from defaulting to static behavior and leaves orphaned Secret references, and
prevents Vault deployments from starting with incomplete configuration.
- Around line 70-74: The proxy configuration currently connects to the
keycloak-db-rw endpoint over plaintext (as indicated by the KKP_BACKEND_ADDR
value pointing to port 5432 without TLS), but CNPG enforces TLS on that
endpoint. Implement backend TLS re-origination by adding the necessary
KKP_BACKEND_* environment variables to enable TLS connectivity and CA
certificate validation. Configure the proxy to dial keycloak-db-rw over TLS,
provide the CNPG CA certificate for validation (through environment variables or
secret references), and ensure all credentials and sensitive data are encrypted
in transit. Remove or resolve the TODO comment once the implementation is
complete.
---
Nitpick comments:
In `@packages/system/keycloak/tests/encryption_test.yaml`:
- Around line 134-168: Add negative-path test cases to the encryption_test.yaml
file to validate backend configuration failures. Create test cases that verify
the Helm template rendering fails or produces appropriate error messages when:
(1) an invalid or unsupported value is provided for encryption.kms.backend, and
(2) required Vault configuration fields are missing (such as
encryption.kms.vault.address, encryption.kms.vault.keyName, or
encryption.kms.vault.tokenSecretName when the vault-transit backend is
selected). These tests should ensure that misconfiguration is caught at
render-time rather than silently proceeding with incomplete configuration.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 813e1df0-9f20-4189-b03c-989af3ab1036
📒 Files selected for processing (4)
packages/system/keycloak/templates/proxy.yamlpackages/system/keycloak/templates/sts.yamlpackages/system/keycloak/tests/encryption_test.yamlpackages/system/keycloak/values.yaml
myasnikovdaniil
left a comment
There was a problem hiding this comment.
NOT LGTM — won't merge as-is (conflicts with main), and the encryption path has a silent-failure mode plus availability/data-migration footguns worth nailing down before it can be flipped on safely. The design itself — opt-in, off by default, hardened pod — is sound.
What it does: adds an opt-in encryption block to the keycloak package; when enabled, a single-replica keycloak-kms-proxy is inserted between the Keycloak StatefulSet and its CNPG database and Keycloak's KC_DB_URL_HOST/PORT are repointed at it. Static KEK (default) or Vault Transit. 11/11 helm-unittest pass (verified locally).
Blockers
B1 — Doesn't merge: conflict with main
mergeable_state is dirty. A merge into the current main conflicts in both templates/sts.yaml and values.yaml — the ingress-wildcard change landed in the same regions after this branch point. Needs a rebase before anything else.
B2 — Invalid kms.backend renders a broken Deployment (orphaned Secret) — see inline on templates/proxy.yaml
Important (fix or document before production use)
I1 — Enabling encryption makes HA Keycloak a single point of failure — see inline on templates/proxy.yaml
I2 — No guard against enabling on an existing (populated) database — see inline on values.yaml
I3 — Has the static path been exercised end-to-end?
The backend leg dials keycloak-db-rw:5432 in plaintext while the TODO notes CNPG enforces TLS, deferred as a follow-up. Combined with this being scoped as a skeleton, it's unclear whether the default static path actually connects to a live CNPG or only renders. The tests here are render-only — confirming a real connect against CNPG (whose pg_hba may reject non-TLS) would be reassuring.
I4 — static KEK: loss = permanent data loss
The KEK lives in keycloak-kms-proxy-kek, preserved across upgrades via lookup (consistent with the admin-credential pattern). But if that Secret is ever lost, the template generates a fresh random KEK and all previously-encrypted data becomes undecryptable — it must be part of the backup set. Worth also stating the at-rest tradeoff: with static the KEK sits in etcd next to the DB credentials.
Minor
The existing bot comments are all valid and worth folding in: vault-field validation (only tokenSecretName is required; address/keyName render empty), the KEK lookup nil-key case, and the missing memory limit on the proxy. Two more: the container exposes metrics on 9090 but the Service only publishes 5432 (nothing can scrape it), and the TCP readiness probe only checks the listener is up, not backend DB / KMS health.
| {{- if .Values.encryption.enabled }} | ||
| {{- $clusterDomain := (index .Values._cluster "cluster-domain") | default "cozy.local" }} | ||
| {{- $kms := .Values.encryption.kms }} | ||
| {{- if eq $kms.backend "static" }} |
There was a problem hiding this comment.
B2 — invalid kms.backend produces a broken Deployment (silent outage).
The KEK Secret is created only when backend == "static" (this line), but the env wiring below uses an {{- else }} that catches everything except vault-transit. So a typo like backend: vault renders a Deployment that mounts KKP_KEK from keycloak-kms-proxy-kek while that Secret is never created.
Verified by render: helm template ... --set encryption.kms.backend=vault emits only Service + Deployment (no Secret), so the pod would hang in CreateContainerConfigError. Because encryption has already repointed KC_DB_URL_HOST at the proxy, the effect is a silent, full Keycloak/IdP outage from a one-character typo.
Suggest failing fast on an unknown backend and keying both the Secret block and the static env path off the same validated value:
{{- $backend := ($kms.backend | default "static") }}
{{- if and (ne $backend "static") (ne $backend "vault-transit") }}
{{- fail (printf "encryption.kms.backend must be 'static' or 'vault-transit', got %q" $backend) }}
{{- end }}
There was a problem hiding this comment.
Fixed in b8c9531a3. encryption.kms.backend is now validated at render time (fail on anything other than static/vault-transit), and both the KEK Secret and the env wiring key off the same $backend, so a typo can no longer render a Deployment that references a never-created Secret. Negative-path unit tests added in 5243cef05.
| # Single replica: HA needs a shared wrapped DEK set (KKP_DEKSET_FILE) so | ||
| # every replica encrypts deterministic columns with the same key. Tracked | ||
| # as a follow-up. | ||
| replicas: 1 |
There was a problem hiding this comment.
I1 — enabling encryption turns HA Keycloak into a single point of failure.
Keycloak runs replicas: 2 (StatefulSet, podManagementPolicy: Parallel), but the proxy is replicas: 1 with no PodDisruptionBudget and no anti-affinity. With encryption on, both Keycloak replicas funnel DB traffic through this one pod, so any proxy restart — node drain, image pull, OOM, rollout — drops the DB connection for the entire IdP.
The shared-DEK HA work is listed as a follow-up; at minimum this availability regression should be called out in the values.yaml comment so operators know enabling the feature downgrades a HA control-plane component.
There was a problem hiding this comment.
Documented in a23376bb1. values.yaml now calls out under the encryption block that the single-replica proxy turns HA Keycloak into a single point of failure. The single replica is intentional until the shared wrapped-DEK HA work lands (a PDB on one replica would not help availability); the caveat makes the downgrade explicit for operators.
| # a KMS, so it never lives inside PostgreSQL. | ||
| # Project: https://github.com/cozystack/keycloak-kms-proxy | ||
| encryption: | ||
| enabled: false |
There was a problem hiding this comment.
I2 — no guard against enabling on an existing (populated) database.
The follow-up list notes the offline backfill Job is still missing, which means flipping enabled: true on an existing install won't migrate the plaintext PII already in Postgres — the proxy then tries to decrypt plaintext on read and existing users can't authenticate. Disabling later is equally unsafe: Keycloak points back at CNPG, which by then holds ciphertext.
Off-by-default protects fresh installs, but nothing stops an operator toggling this on a live one. A loud warning here (and ideally a render-time or runtime guard) would prevent a one-way data-loss toggle.
There was a problem hiding this comment.
Documented in a23376bb1. Added a loud caveat that enabling (or later disabling) encryption on a populated database breaks logins on the deterministic columns until the offline backfill has run, with a link to the migration procedure. A render/runtime guard is not feasible — the template cannot tell whether the database already holds data — so this is a documentation guard; off-by-default still protects fresh installs.
baee084 to
52df360
Compare
|
Thanks for the thorough review — all points addressed. The branch is rebased onto current B1 — merge conflict: rebased onto B2 — invalid I1 — single proxy = SPOF for HA Keycloak: documented prominently in I2 — enabling on a populated DB: documented as a loud caveat with a link to the offline backfill procedure. A render/runtime guard isn't feasible (the template can't tell whether the DB already holds data), so this is a documentation guard for now; off-by-default still protects fresh installs. I3 / backend TLS: the proxy re-originates the connection and negotiates TLS in-band to I4 — static KEK loss = data loss: documented — the KEK Secret must be in the backup set, plus the at-rest tradeoff (with Minors:
|
|
myasnikovdaniil review pls 🙏 — re-requested your review. Everything from your pass is addressed: rebased onto |
52df360 to
2043536
Compare
Add an opt-in `encryption` flag to the keycloak system package. When encryption.enabled=true, a keycloak-kms-proxy Deployment + Service is placed on the wire between Keycloak and its CloudNativePG database, and Keycloak's KC_DB_URL_HOST/PORT are repointed at the proxy. The proxy transparently encrypts PII columns (username, email, first/last name, PII attributes, credentials) before they reach PostgreSQL and decrypts them on reads; the data-encryption key is wrapped by a KMS and never lives inside the database. The KMS backend is selectable: a self-generated static KEK (default, works on any cluster, suitable for evaluation) or HashiCorp Vault Transit (recommended for production, supports KEK rotation). The feature is off by default, so existing installs are unaffected. Skeleton/draft scope: backend TLS re-origination to CNPG, the Liquibase bootstrap window (KKP_LENIENT), an offline backfill Job for pre-existing data, and HA via a shared DEK set are tracked as follow-ups. Proxy project: https://github.com/cozystack/keycloak-kms-proxy Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Andrei Kvapil <andrei.kvapil@aenix.io>
…late Address review feedback on the encryption proxy template: - Validate encryption.kms.backend at render time and fail with a clear message on any value other than static/vault-transit. Both the KEK Secret and the env wiring now key off the same validated $backend, so a typo can no longer render a Deployment that mounts a KEK Secret that was never created (which would silently break the whole IdP). - Guard the static KEK lookup against a Secret that exists without a usable 'kek' key, preserving the freshly generated KEK instead of nil/empty. - Default kms.vault to an empty dict and require address/keyName at render time so an incomplete Vault config fails with an actionable message rather than a generic nil dereference. - Publish the metrics port (9090) on the proxy Service so it can be scraped. - Clarify the backend-TLS note: the proxy negotiates TLS in-band; CNPG CA pinning is the remaining follow-up. Co-Authored-By: Claude <noreply@anthropic.com> Signed-off-by: Andrei Kvapil <andrei.kvapil@aenix.io>
… memory Address review feedback on the encryption values block: - Spell out the operational caveats an operator must understand before flipping encryption on: the single-replica proxy turns HA Keycloak into a single point of failure; existing data is not migrated automatically so enabling (or later disabling) on a populated database breaks logins until the offline backfill runs; and a lost static KEK is permanent data loss, so the KEK Secret must be backed up. - Add a memory limit to the proxy resources per the repository style guide. Co-Authored-By: Claude <noreply@anthropic.com> Signed-off-by: Andrei Kvapil <andrei.kvapil@aenix.io>
Add render-time negative-path coverage for the encryption proxy: an unsupported kms.backend and a vault-transit backend missing address, keyName, or tokenSecretName must all fail the template render rather than silently producing a broken Deployment. Co-Authored-By: Claude <noreply@anthropic.com> Signed-off-by: Andrei Kvapil <andrei.kvapil@aenix.io>
Remove the db.host/db.port/db.urlProperties override and the extraVolumes/extraVolumeMounts plumbing: they are no longer needed now that the database endpoint is handled natively. Keycloak's DB host/port come straight from the keycloak-db-app secret again, except when encryption is enabled, where the keycloak-kms-proxy owns the repointing. Theme and admin-console hostname handling are unaffected. Co-Authored-By: Claude <noreply@anthropic.com> Signed-off-by: Andrei Kvapil <andrei.kvapil@aenix.io>
2043536 to
5d36e13
Compare
|
myasnikovdaniil ping for re-review 🙏 — CI is fully green now (E2E included), and everything from your review is addressed: rebased onto |
IvanHunters
left a comment
There was a problem hiding this comment.
Verdict
LGTM with non-blocking notes.
Off-by-default path is a verified no-op for existing installs, both KMS backends render, KEK rotation is correctly lookup-guarded, and the blockers from the earlier review round are resolved on the current head. Remaining items are availability/observability caveats and follow-ups the author already scoped.
Upgrade safety (verified)
Rendered templates/sts.yaml at the pre-PR merge-base and at head with encryption.enabled=false (the default). The DB wiring is byte-for-byte identical — KC_DB_URL_HOST/KC_DB_URL_PORT still source from the keycloak-db-app secret. An existing install that does not opt in keeps connecting to CNPG exactly as before; the feature is a true no-op when off.
The removal of the db.* / extraVolumes / extraVolumeMounts override API is safe for released clusters: that API was introduced in bc52d81 and is not contained in any release tag (git tag --contains is empty; absent from v1.5.2 values). Only a cluster tracking main that had manually set db.host/urlProperties would silently lose those values — no released consumer is affected. There is no values.schema.json in this chart, so removed keys cannot fail HR re-admission.
Findings
[MINOR] templates/proxy.yaml:138-145 — probes are TCP-only, don't reflect KMS/backend health
Both readinessProbe and livenessProbe use tcpSocket: port: postgres, which only asserts the listener is up. If the KEK Secret is fine but the KMS is unreachable (Vault down/token expired) or the CNPG backend is unreachable, the proxy still reports Ready while Keycloak DB reads/writes fail one hop away — kubectl describe pod and Flux both show the proxy green. Consider a readiness check that exercises the KMS/backend path.
[MINOR] templates/proxy.yaml:54-58 — pod securityContext omits runAsGroup
Pod-level securityContext sets runAsNonRoot, runAsUser: 65532, fsGroup: 65532, seccompProfile but no runAsGroup. Not admission-blocked (chart lands in cozy-keycloak, a system namespace, not a tenant-* PSS-restricted one), so hardening completeness only. Add runAsGroup: 65532 for consistency.
Claim mismatches
[UNVERIFIABLE] "transparently encrypts/decrypts PII columns; DEK wrapped by KMS, never in the DB" — the proxy internals live in github.com/cozystack/keycloak-kms-proxy, out of this diff. The wiring is verified correct (KKP_BACKEND_ADDR matches CNPG keycloak-db-rw, creds source keycloak-db-app), but the encrypt-on-wire behaviour itself cannot be confirmed from this diff.
Caveats (non-blocking)
- Single-replica proxy is a SPOF on the DB path when enabled (
templates/proxy.yaml:45): both Keycloak replicas funnel DB traffic through one pod, so any proxy restart briefly drops the DB connection for the whole IdP. Disclosed in thevalues.yamlcaveat block; off by default. - Partial failure (proxy up, KMS/backend down) leaves Keycloak unable to read/write while the proxy looks healthy — same root cause as the TCP-probe finding.
- Backend leg dials
keycloak-db-rw:5432in plaintext; CNPG may enforce TLS viapg_hba, and the new path drops the previoussslmode=verify-fullmechanism without yet pinning the CNPG CA (proxy.yamlTODO). Tests are render-only, so a live static-backend connect against CNPG is not exercised end-to-end here. - Enabling on a populated database without the backfill leaves pre-existing PII as plaintext (logins on deterministic columns fail); disabling later reads ciphertext directly. Documented in
values.yaml. - KEK persistence across upgrade is correct:
lookupreuses the existingkeycloak-kms-proxy-kekSecret and only falls back torandAlphaNumwhen absent (proxy.yaml:20-24). No rotation on reconcile. - Proxy image is tag-pinned (
:v0.1.0), not digest-pinned, matching this chart's own keycloak image convention.
Recommended follow-ups
- Before flipping to production-ready: land backend CA pinning (
sslrootcert), the offline backfill Job, and shared-DEK HA — all listed in the PR body and prerequisites for a safe enable on a populated, HA install. - Add a KMS-aware readiness gate so a wedged KMS is legible at the proxy layer.
Note: the blockers raised in the earlier review round (merge conflict, broken render on invalid kms.backend, empty required Vault fields, unscrapeable metrics port, missing proxy memory limit) are all resolved on the current head by the 2026-07-01 fixups — re-verified each against the current tree.
All items from this review are addressed on the current head: rebased onto main; render-time validation so an invalid kms.backend fails fast; Vault address/keyName now required; metrics port published on the Service; proxy memory limit added; SPOF, populated-DB migration, and static-KEK backup caveats documented in values.yaml. Independently re-reviewed and approved by IvanHunters. Dismissing this stale review to unblock merge; remaining probe/TLS items are non-blocking follow-ups scoped in the PR body.
What this PR does
Adds an optional, flag-gated column-level PII encryption feature to the
keycloaksystem package. Whenencryption.enabled=true, akeycloak-kms-proxyDeployment + Service is placed on the wire between the Keycloak StatefulSet and its CloudNativePG database, and Keycloak'sKC_DB_URL_HOST/KC_DB_URL_PORTare repointed at the proxy. The proxy transparently encrypts Keycloak PII columns (username, email, first/last name,pii-*attributes, credential secrets) before they reach PostgreSQL and decrypts them on reads; the data-encryption key is wrapped by a KMS and never lives inside the database.The proxy lives in its own repository: https://github.com/cozystack/keycloak-kms-proxy (image
ghcr.io/cozystack/keycloak-kms-proxy:v0.1.0).KMS backend is selectable via
encryption.kms.backend:static(default): a self-generated 32-byte KEK kept in a Secret — works on any cluster, suitable for evaluation.vault-transit: the KEK lives in HashiCorp Vault (recommended for production; supports KEK rotation).The feature is off by default, so existing installs are unaffected —
KC_DB_URL_HOSTkeeps sourcing from the CNPGkeycloak-db-appsecret.Changes
values.yaml: newencryptionblock.templates/proxy.yaml(new): gated Deployment + Service (+ self-generated KEK Secret for the static backend), hardened pod (nonroot, read-only rootfs, dropped caps, seccomp).templates/sts.yaml: conditional DB-endpoint override when encryption is enabled.tests/encryption_test.yaml(new): helm-unittest covering on/off and both KMS backends.helm unittest .→ 11/11 pass.Follow-ups (not in this draft)
KKP_LENIENT).Screenshots
N/A — no UI changes.
Release note
Summary by CodeRabbit
New Features
Tests