feat(helm): add secretEnv to avoid API keys in ConfigMap#730
feat(helm): add secretEnv to avoid API keys in ConfigMap#730thepagent merged 6 commits intoopenabdev:mainfrom
Conversation
OpenAB PR ScreeningThis is auto-generated by the OpenAB project-screening flow for context collection and reviewer handoff.
Screening report## IntentPR #730 is trying to stop Helm-rendered agent API keys from being exposed in the OpenAB ConfigMap. The concrete operator-visible problem is that current FeatThis is a feature for the Helm chart with a security-hardening effect. Behavioral change:
Who It ServesPrimary beneficiaries:
Secondary beneficiaries:
Rewritten PromptImplement Helm chart support for agent-scoped secret environment variables. Add an Ensure secret values are never rendered into the ConfigMap. Preserve existing Merge PitchThis is worth advancing because it fixes a real deployment security footgun without changing OpenAB runtime semantics. The proposed flow uses Kubernetes-native secret references and aligns with common Helm chart expectations. Risk profile is moderate-low: the change is confined to Helm templates, but reviewer attention should focus on backwards compatibility, duplicate Best-Practice ComparisonRelevant OpenClaw principles:
Relevant Hermes Agent principles:
The strongest best-practice alignment is with explicit secret routing and avoiding persistent plaintext secret material in broadly readable configuration artifacts. Implementation OptionsOption 1: Conservative template-only support Accept the PR’s core approach: add Pros:
Cons:
Option 2: Balanced chart feature with validation and docs Keep the same implementation model, but add chart documentation, Pros:
Cons:
Option 3: Ambitious secret-management pass Broaden the chart’s environment handling into a unified model supporting plain env, secret refs, config map refs, existing secret names, and optional generated Secret resources. Pros:
Cons:
Comparison Table
RecommendationAdvance this PR using Option 2 as the target shape. The underlying idea is sound and should move forward: secrets should not be rendered into ConfigMaps, and Kubernetes Suggested sequencing:
|
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
Thanks for the thorough review! I've addressed all the blocking issues and nits: 🔴 Deduplication — Added 🔴 Required field validation — Added 🔴 🟡 🟡 Rename — |
This comment has been minimized.
This comment has been minimized.
EKS Helm Test — PR #730
|
Closes openabdev#729 Adds agents.<name>.secretEnv so API keys stay in Kubernetes Secrets instead of being rendered as plaintext in the ConfigMap. Deployment renders each entry as valueFrom.secretKeyRef; ConfigMap merges the key names into inherit_env so the openab process passes them to agent subprocesses. Secret values never appear in the ConfigMap. Validation: - fail if any secretEnv entry is missing name, secretName, or secretKey - fail if the same key appears in both env and secretEnv (collision guard) - uniq applied to merged inherit_env list to prevent duplicate entries values.yaml: added secretEnv: [] with usage comments to all agent blocks. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
58e2097 to
79fc867
Compare
This comment has been minimized.
This comment has been minimized.
Consolidate the validation-only range block and the render range block into a single loop so validation and rendering happen together. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
This comment has been minimized.
This comment has been minimized.
Covers ConfigMap inherit_env merge, dedup with inheritEnv, no plaintext rendering, env-conflict guard, and Deployment valueFrom.secretKeyRef rendering. Field-validation guards are tested via the Deployment template (helm-unittest detects fail() correctly there). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
helm test — Proof of Testing on Live EKS Cluster
1. New helm unit tests addedTwo new test files added to
Full suite run — 85/85 passing (zero regressions): ``` PASS adapter enablement gating Charts: 1 passed, 1 total secretEnv test scenarios covered:
2. Live EKS deployment —
|
|
This is a strong improvement from a security and operability standpoint. Moving sensitive agent environment variables out of the ConfigMap and into Secret-backed env injection makes the Helm chart much safer by default while keeping the workflow straightforward. I’d be happy to see this merged. |
This comment has been minimized.
This comment has been minimized.
Extract the duplicated `if not (and .name .secretName .secretKey)` guard from configmap.yaml and deployment.yaml into a shared named template `openab.validateSecretEnv` in _helpers.tpl; both callers now use a single `include` instead of repeating the same fail block. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
Thanks for the review feedback — addressed in the latest commit. Extracted the duplicated |
This comment has been minimized.
This comment has been minimized.
Replace single order-sensitive regex with three independent assertions: each key must appear in inherit_env, and GEMINI_API_KEY must not appear twice. This removes the implicit reliance on uniq preserving insertion order while still verifying the deduplication invariant. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
Thanks for the thorough final review! Addressed the remaining nit: 🟡 Test order-sensitivity — The dedup test
This removes the implicit reliance on 🟡 Double validation call — Left as-is per reviewer guidance ("Not worth changing"). Belt-and-suspenders is appropriate for a security feature — if either template is rendered in isolation (e.g. All 85 helm-unittest tests pass with the updated assertion. |
This comment has been minimized.
This comment has been minimized.
…p.yaml deployment.yaml already calls the same validation helper, so the configmap.yaml call was redundant (validation ran twice on every render). Keep it in deployment.yaml only — fail-fast on the template that actually renders the secretKeyRef entries.
🟢 LGTM — Clean, well-scoped Helm enhancement with good safety guardsWhat problem does it solve? API keys stored in How? Three-part approach:
Considered alternatives? The chart already has Review Details🟢 INFO — Done well
🟡 NIT — Non-blocking suggestions
Verdict: Ready for maintainer review. No blocking issues found. |
Summary
Closes #729
Problem
[agent].envvalues are rendered directly into the ConfigMap as plaintext, exposing API keys to anyone withkubectl get configmapaccess:Solution
Add a
secretEnvfield that renders asvalueFrom.secretKeyRefin the Deployment and automatically adds the key name toinherit_envin config.toml:Deployment (secret value, not plaintext):
ConfigMap (no secret value):
The secret flows: k8s Secret → Deployment env → openab process →
inherit_env→ agent subprocess. The ConfigMap stays clean.Changes
charts/openab/templates/deployment.yaml— rendersecretEnvasvalueFrom.secretKeyRefcharts/openab/templates/configmap.yaml— mergesecretEnvkey names intoinherit_envTesting
helm template test ./charts/openab \ --set agents.gemini.secretEnv[0].name=GEMINI_API_KEY \ --set agents.gemini.secretEnv[0].secretName=openab-secrets \ --set agents.gemini.secretEnv[0].secretKey=GEMINI_API_KEYVerified ConfigMap contains
inherit_env = ["GEMINI_API_KEY"]and Deployment containsvalueFrom.secretKeyRef.Discord Discussion
https://discord.com/channels/1491295327620169908/1491365157010542652