fix(helm): Generate strong Temporal payload encryption key - #4453
fix(helm): Generate strong Temporal payload encryption key#4453kfelternv wants to merge 2 commits into
Conversation
|
Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually. Contributors can view more details about this message here. |
Summary by CodeRabbit
WalkthroughThe Helm chart resolves the Temporal encryption key from configured values, an existing Kubernetes Secret, or generated 32-byte random data. Upgrade behavior, failure cases, and disabled Secret creation are covered by Helm tests. ChangesTemporal encryption key handling
Estimated code review effort: 2 (Simple) | ~10 minutes Sequence Diagram(s)sequenceDiagram
participant HelmValues
participant SecretsTemplate
participant KubernetesSecret
HelmValues->>SecretsTemplate: provide configured key and create setting
SecretsTemplate->>KubernetesSecret: read existing key during upgrade
KubernetesSecret-->>SecretsTemplate: return decoded key
SecretsTemplate-->>KubernetesSecret: render resolved Secret or fail
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
4bbc7a3 to
a31f289
Compare
What changedFresh NICo REST installs now generate the Temporal payload encryption key from 32 random bytes. Explicit values remain supported, Secret creation can still be disabled, and upgrades reuse the existing key. Scenario and setupTested commit VerificationStep 1: Generate a 32-byte key for a fresh installWhy this step exists: It verifies the empty default value reaches the random-key path and creates a usable Kubernetes Secret. Runnable command: kubectl create namespace temporal-key-fresh
helm template temporal-key-pr4453 helm/rest/nico-rest \
--namespace temporal-key-fresh \
--set nico-rest-api.config.keycloak.enabled=true \
--show-only charts/nico-rest-common/templates/secrets.yaml |
kubectl apply -f -
kubectl -n temporal-key-fresh get secret temporal-encryption-key \
-o jsonpath='{.data.temporal-encryption-key}' |
base64 -D | base64 -D | wc -cObserved result: Why this proves the behavior: Decoding the Kubernetes Secret value and then the value returned by Helm's Step 2: Generate a different key for another fresh installWhy this step exists: It verifies fresh installs do not reuse a fixed or deterministic default. Runnable command: FRESH_ONE_HASH=$(kubectl -n temporal-key-fresh get secret temporal-encryption-key \
-o jsonpath='{.data.temporal-encryption-key}' | shasum -a 256 | awk '{print $1}')
kubectl create namespace temporal-key-fresh-two
helm template temporal-key-pr4453 helm/rest/nico-rest \
--namespace temporal-key-fresh-two \
--set nico-rest-api.config.keycloak.enabled=true \
--show-only charts/nico-rest-common/templates/secrets.yaml |
kubectl apply -f -
FRESH_TWO_HASH=$(kubectl -n temporal-key-fresh-two get secret temporal-encryption-key \
-o jsonpath='{.data.temporal-encryption-key}' | shasum -a 256 | awk '{print $1}')
if [ "$FRESH_ONE_HASH" != "$FRESH_TWO_HASH" ]; then
printf '%s\n' 'fresh key hashes differ'
else
printf '%s\n' 'fresh key hashes matched unexpectedly'
false
fiObserved result: Why this proves the behavior: Two independent fresh renders stored different key values. Step 3: Honor an explicitly configured keyWhy this step exists: It verifies operators can still provide a specific value. Runnable command: kubectl create namespace temporal-key-explicit
helm template temporal-key-pr4453 helm/rest/nico-rest \
--namespace temporal-key-explicit \
--set nico-rest-api.config.keycloak.enabled=true \
--set-string nico-rest-common.secrets.temporalEncryptionKey.value=configured-key \
--show-only charts/nico-rest-common/templates/secrets.yaml |
kubectl apply -f -
kubectl -n temporal-key-explicit get secret temporal-encryption-key \
-o jsonpath='{.data.temporal-encryption-key}' | base64 -DObserved result: Why this proves the behavior: The stored Secret contains the exact configured value instead of a generated replacement. Step 4: Keep Secret creation disabled when requestedWhy this step exists: It verifies the existing Runnable command: helm template temporal-key-pr4453 helm/rest/nico-rest \
--namespace temporal-key-disabled \
--set nico-rest-api.config.keycloak.enabled=true \
--set nico-rest-common.secrets.create=false \
--show-only charts/nico-rest-common/templates/secrets.yaml |
awk '$0 == "kind: Secret" {count++} END {print count+0}'Observed result: Why this proves the behavior: The common Secret template rendered zero Secret objects when creation was disabled. Step 5: Reuse the existing key during an upgradeWhy this step exists: It verifies the live-cluster Runnable command: UPGRADE_BEFORE=$(kubectl -n temporal-key-fresh get secret temporal-encryption-key \
-o jsonpath='{.data.temporal-encryption-key}' | shasum -a 256 | awk '{print $1}')
helm template temporal-key-pr4453 helm/rest/nico-rest \
--dry-run=server \
--is-upgrade \
--namespace temporal-key-fresh \
--set nico-rest-api.config.keycloak.enabled=true \
--show-only charts/nico-rest-common/templates/secrets.yaml |
kubectl apply -f -
UPGRADE_AFTER=$(kubectl -n temporal-key-fresh get secret temporal-encryption-key \
-o jsonpath='{.data.temporal-encryption-key}' | shasum -a 256 | awk '{print $1}')
if [ "$UPGRADE_BEFORE" = "$UPGRADE_AFTER" ]; then
printf '%s\n' 'upgrade preserved key hash'
else
printf '%s\n' 'upgrade changed key hash unexpectedly'
false
fiObserved result: Why this proves the behavior: The server-connected upgrade render read the existing Secret, and the stored key hash remained identical after applying that render. |
![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: a31f289da2
ℹ️ 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".
🔐 TruffleHog Secret Scan✅ No secrets or credentials found! Your code has been scanned for 700+ types of secrets and credentials. All clear! 🎉 🕐 Last updated: 2026-07-31 22:23:04 UTC | Commit: a31f289 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
helm/rest/nico-rest/charts/nico-rest-common/tests/secrets_test.yaml (1)
8-13: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winAdd regression coverage for Secret reuse and upgrade failure.
This test checks only the generated value shape. It does not exercise the existing-Secret lookup branch or verify that an upgrade fails instead of generating a replacement key when the Secret is unavailable.
Add tests for both cases to protect the key-persistence contract.
🤖 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 `@helm/rest/nico-rest/charts/nico-rest-common/tests/secrets_test.yaml` around lines 8 - 13, Add regression tests alongside the existing temporal-encryption-key test to cover both Secret reuse when an existing Secret is found and upgrade failure when that Secret is unavailable. Exercise the chart’s existing-Secret lookup path, assert the reused key remains unchanged, and verify an upgrade returns an error rather than generating a replacement key.
🤖 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 `@helm/rest/nico-rest/charts/nico-rest-common/templates/secrets.yaml`:
- Around line 5-14: The Temporal encryption key resolution in secrets.yaml must
generate a random key only during installation; on upgrades or rollbacks, fail
when neither the configured value nor the existing Secret key is available.
Preserve reuse of an existing Secret key, using Helm release state to
distinguish installs from upgrades. Update secrets_test.yaml to cover existing
Secret reuse and failure for an upgrade with a missing Secret.
In `@helm/rest/nico-rest/charts/nico-rest-common/values.yaml`:
- Around line 19-21: Update the comment above value in the Temporal payload
encryption configuration to identify value: "" as the chart default and document
resolution precedence: a configured value overrides Secret reuse and random
generation, while an empty value reuses the existing Secret on upgrade or
generates 32 random bytes on install; state that secrets.create: false disables
this resolution.
---
Nitpick comments:
In `@helm/rest/nico-rest/charts/nico-rest-common/tests/secrets_test.yaml`:
- Around line 8-13: Add regression tests alongside the existing
temporal-encryption-key test to cover both Secret reuse when an existing Secret
is found and upgrade failure when that Secret is unavailable. Exercise the
chart’s existing-Secret lookup path, assert the reused key remains unchanged,
and verify an upgrade returns an error rather than generating a replacement key.
🪄 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: Enterprise
Run ID: fde99b05-9f56-4d13-a8fd-04948ca1a7c3
📒 Files selected for processing (3)
helm/rest/nico-rest/charts/nico-rest-common/templates/secrets.yamlhelm/rest/nico-rest/charts/nico-rest-common/tests/secrets_test.yamlhelm/rest/nico-rest/charts/nico-rest-common/values.yaml
There was a problem hiding this comment.
🧹 Nitpick comments (1)
helm/rest/nico-rest/charts/nico-rest-common/templates/secrets.yaml (1)
7-12: 🗄️ Data Integrity & Integration | 🔵 TrivialDo not use this
lookuppath with client-side renderers.Client-side renderers cannot read the existing Secret. On install, this generates a new key; on upgrade, it fails. Use server-side rendering with
getaccess to the Secret, or use an externally managed Secret withsecrets.create=false.🤖 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 `@helm/rest/nico-rest/charts/nico-rest-common/templates/secrets.yaml` around lines 7 - 12, The temporal-encryption-key lookup in the secrets template is incompatible with client-side rendering. Update the secret-generation flow around $existingSecret and $existingKey to use server-side rendering with get access, or support an externally managed Secret when secrets.create=false, while preserving reuse of the existing key and generation only for installs without one.Sources: Path instructions, MCP tools
🤖 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 `@helm/rest/nico-rest/charts/nico-rest-common/templates/secrets.yaml`:
- Around line 7-12: The temporal-encryption-key lookup in the secrets template
is incompatible with client-side rendering. Update the secret-generation flow
around $existingSecret and $existingKey to use server-side rendering with get
access, or support an externally managed Secret when secrets.create=false, while
preserving reuse of the existing key and generation only for installs without
one.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: f2280dcb-4084-4e49-9d61-a7c55c7e49cc
📒 Files selected for processing (3)
helm/rest/nico-rest/charts/nico-rest-common/templates/secrets.yamlhelm/rest/nico-rest/charts/nico-rest-common/tests/secrets_test.yamlhelm/rest/nico-rest/charts/nico-rest-common/values.yaml
🚧 Files skipped from review as they are similar to previous changes (1)
- helm/rest/nico-rest/charts/nico-rest-common/values.yaml
1 participant
Fresh NICo REST installs currently use a fixed Temporal payload encryption key. This change generates the key from 32 random bytes while preserving existing Secrets on upgrades and honoring explicit values.
Related issues
None.
Type of Change
Breaking Changes
Testing
Additional Notes
Existing installations keep their current key during upgrades so encrypted Temporal payloads remain readable.