Fix unsafe secrets#59
Conversation
📝 WalkthroughOverviewThis PR refactors secret handling in the Helm charts for Architecture ChangesThe PR introduces a multi-stage secret flow:
New Components
Deployment Changes
Configuration Changes
PostgreSQL Secret ManagementThe bundled development PostgreSQL password is now stored in a dedicated Kubernetes secret ( Helm Chart ChangesThe Development vs. Production FlexibilityThe changes support multiple deployment modes:
WalkthroughThe platform chart now provisions an OpenBao-backed secret store, syncs Sequence Diagram(s)sequenceDiagram
participant Helm as Helm chart
participant ESO as External Secrets Operator
participant OpenBao as OpenBao
participant K8s as Kubernetes
participant API as asdlc-api Deployment
Helm->>K8s: apply ClusterSecretStore and ExternalSecret
ESO->>OpenBao: read KV v2 keys
ESO->>K8s: write asdlc-api-secrets
API->>K8s: read secretKeyRef values and key mounts
sequenceDiagram
participant Job as task-signing-key Job script
participant Kubectl as kubectl
participant K8s as Kubernetes
Job->>K8s: check for the task-signing key Secret
Job->>Job: generate RSA 2048 private key when missing
Job->>Kubectl: create Secret manifest
Kubectl->>K8s: apply the Secret
Suggested reviewers
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
deployments/helm-charts/wso2-agentic-engineer-bundle/templates/task-signing-key-job.yaml (1)
16-31: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winUse namespace-scoped RBAC for the key generator.
The job only manages a Secret in
{{ .Release.Namespace }}, so aRole/RoleBindingis sufficient and keeps the permission scope aligned with the job.Proposed RBAC scope reduction
-kind: ClusterRole +kind: Role metadata: name: wso2-ae-key-generator + namespace: {{ .Release.Namespace }} @@ -kind: ClusterRoleBinding +kind: RoleBinding @@ roleRef: apiGroup: rbac.authorization.k8s.io - kind: ClusterRole + kind: Role name: wso2-ae-key-generatorAlso applies to: 33-52
🤖 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 `@deployments/helm-charts/wso2-agentic-engineer-bundle/templates/task-signing-key-job.yaml` around lines 16 - 31, The RBAC for the key generator is overly broad because it uses a ClusterRole for a Secret that lives only in the release namespace. Update the task-signing-key-job template to use namespace-scoped RBAC by replacing the ClusterRole/ClusterRoleBinding with a Role/RoleBinding tied to {{ .Release.Namespace }}, and keep the secret permissions limited to the existing key generator job objects. Use the existing task-signing-key-job resource names and annotations to locate the related RBAC sections that need to be changed together.
🤖 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
`@deployments/helm-charts/wso2-ae-platform/templates/asdlc-api/cluster-secret-store.yaml`:
- Around line 14-17: The rendered store type is inconsistent between this
template and the consumers in external-secret.yaml and push-secret.yaml. Update
the cluster-secret-store template to either always use ClusterSecretStore by
pinning the kind in .Values.externalSecret.secretStoreRef.kind across the chart,
or branch the lookup/create logic in this template based on the configured kind
so it only provisions the matching SecretStore or ClusterSecretStore. Reference
the cluster-secret-store.yaml template and the secretStoreRef.kind usage in the
external-secret and push-secret templates while making the change.
In
`@deployments/helm-charts/wso2-ae-platform/templates/asdlc-api/deployment.yaml`:
- Around line 223-252: The Secret templates for asdlc-api are still sourcing
private key contents from Helm values, which should not be rendered into
manifests or release metadata. Update the Secret handling in the deployment
template to stop using .Values.taskSigning.privateKeyPem and
.Values.github.appPrivateKeyPem in the Secret definitions, and instead rely on
generated secrets or references to pre-existing secrets managed outside Helm.
Keep the existing Secret names and surrounding conditional structure in
deployment.yaml, but remove any values-based private key material from the
rendered output.
- Around line 213-238: The asdlc-api Deployment always mounts
asdlc-api-task-signing-key, but wso2-ae-platform only creates that Secret when
.Values.taskSigning.privateKeyPem is set, so direct installs can fail. Add a
pre-install Helm hook Job in the wso2-ae-platform chart, alongside the existing
taskSigning secret logic, that generates and creates asdlc-api-task-signing-key
when .Values.taskSigning.privateKeyPem is absent, matching the standalone safety
already handled in the bundle chart.
In
`@deployments/helm-charts/wso2-ae-platform/templates/asdlc-api/seed-secret.yaml`:
- Around line 11-18: The seed Secret is incorrectly skipped when .Values.secrets
is empty, which breaks the databaseURL secret flow. Update the seed-secret.yaml
gating logic around the secret rendering so the asdlc-api-seed resource is
always created when databaseURL is part of the chain, even with the default
secrets: {} value. Keep the existing secret iteration logic in sync with the
resource template and make sure the databaseURL entry can still be published by
push-secret.yaml and consumed by the deployment via asdlc-api-secrets.
In `@deployments/helm-charts/wso2-ae-platform/templates/postgres/secret.yaml`:
- Around line 14-15: The PostgreSQL secret template is still sourcing the
password from chart values via the secret rendering path, so it remains baked
into Helm-managed manifests. Update the secret-handling flow in the postgres
Secret template to stop templating .Values.postgres.auth.password directly:
either require an existing/external Secret reference or generate a one-time
local password during install and persist it outside chart values. Keep the
change centered around the postgres secret template and the postgres.auth values
contract so the password is no longer shipped from default values.
In
`@deployments/helm-charts/wso2-agentic-engineer-bundle/templates/task-signing-key-job.yaml`:
- Around line 100-105: The reuse logic in the task signing key job only checks
whether the Secret exists, so it can exit even when the expected
task-signing.pem entry is missing. Update the Secret validation in the
task-signing-key job script to verify the key data exists before reusing it, and
only exit early when both the Secret and task-signing.pem are present; otherwise
continue with key generation so the mounted file is always available.
In `@deployments/helm-charts/wso2-agentic-engineer-bundle/values.yaml`:
- Around line 158-160: The githubClientSecret entry is currently treated like a
seedable value even though it is empty, which can cause an empty secret to be
persisted and block later updates. Update the values handling for
githubClientSecret in the seed/push flow so empty values are skipped or marked
as externally pre-seeded, and ensure the logic around the seed/push templates
does not apply IfNotExists semantics to this field.
---
Outside diff comments:
In
`@deployments/helm-charts/wso2-agentic-engineer-bundle/templates/task-signing-key-job.yaml`:
- Around line 16-31: The RBAC for the key generator is overly broad because it
uses a ClusterRole for a Secret that lives only in the release namespace. Update
the task-signing-key-job template to use namespace-scoped RBAC by replacing the
ClusterRole/ClusterRoleBinding with a Role/RoleBinding tied to {{
.Release.Namespace }}, and keep the secret permissions limited to the existing
key generator job objects. Use the existing task-signing-key-job resource names
and annotations to locate the related RBAC sections that need to be changed
together.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b609c06d-bdc0-4a86-81d6-1bad05404946
📒 Files selected for processing (10)
deployments/helm-charts/wso2-ae-platform/templates/asdlc-api/cluster-secret-store.yamldeployments/helm-charts/wso2-ae-platform/templates/asdlc-api/deployment.yamldeployments/helm-charts/wso2-ae-platform/templates/asdlc-api/external-secret.yamldeployments/helm-charts/wso2-ae-platform/templates/asdlc-api/push-secret.yamldeployments/helm-charts/wso2-ae-platform/templates/asdlc-api/seed-secret.yamldeployments/helm-charts/wso2-ae-platform/templates/postgres/secret.yamldeployments/helm-charts/wso2-ae-platform/templates/postgres/statefulset.yamldeployments/helm-charts/wso2-ae-platform/values.yamldeployments/helm-charts/wso2-agentic-engineer-bundle/templates/task-signing-key-job.yamldeployments/helm-charts/wso2-agentic-engineer-bundle/values.yaml
| {{- $storeName := .Values.externalSecret.secretStoreRef.name | default "default" }} | ||
| {{- $existing := lookup "external-secrets.io/v1" "ClusterSecretStore" "" $storeName }} | ||
| {{- if not $existing }} | ||
| apiVersion: external-secrets.io/v1 |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Keep secretStoreRef.kind consistent with the rendered store type.
Lines 14-17 always look up/create a ClusterSecretStore, but external-secret.yaml and push-secret.yaml read .Values.externalSecret.secretStoreRef.kind. If that value is switched to SecretStore, the chart will still provision the cluster-scoped resource while the consumers reference a namespaced one. Either pin the kind to ClusterSecretStore everywhere or branch this template on the configured kind.
🧰 Tools
🪛 YAMLlint (1.37.1)
[error] 14-14: syntax error: expected the node content, but found '-'
(syntax)
🤖 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
`@deployments/helm-charts/wso2-ae-platform/templates/asdlc-api/cluster-secret-store.yaml`
around lines 14 - 17, The rendered store type is inconsistent between this
template and the consumers in external-secret.yaml and push-secret.yaml. Update
the cluster-secret-store template to either always use ClusterSecretStore by
pinning the kind in .Values.externalSecret.secretStoreRef.kind across the chart,
or branch the lookup/create logic in this template based on the configured kind
so it only provisions the matching SecretStore or ClusterSecretStore. Reference
the cluster-secret-store.yaml template and the secretStoreRef.kind usage in the
external-secret and push-secret templates while making the change.
| - match: | ||
| secretKey: databaseURL | ||
| remoteRef: | ||
| remoteKey: {{ .Values.externalSecret.keys.databaseURL }} | ||
| property: "value" |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
databaseURL cannot rotate after the first install.
Lines 88-92 always publish databaseURL through the IfNotExists PushSecret. If postgres.url or the bundled DB credentials change later, OpenBao keeps the old value and asdlc-api continues consuming a stale connection string from asdlc-api-secrets. Give databaseURL an overwrite path, or derive it without a one-time seed.
| {{- $hasSecrets := false }} | ||
| {{- range $k, $v := .Values.secrets }} | ||
| {{- $policy := $v.forceDefaultOverride | default "" | toString }} | ||
| {{- if ne $policy "false" }} | ||
| {{- $hasSecrets = true }} | ||
| {{- end }} | ||
| {{- end }} | ||
| {{- if $hasSecrets }} |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Always render the seed Secret while databaseURL is part of the flow.
Lines 11-18 gate the whole resource only on .Values.secrets, but Lines 29-33 always define databaseURL, push-secret.yaml always tries to publish it, and the deployment reads it from asdlc-api-secrets. With the new default secrets: {}, asdlc-api-seed is never created, so the database secret chain stops before ESO can populate the runtime Secret.
🧰 Tools
🪛 YAMLlint (1.37.1)
[error] 11-11: syntax error: expected the node content, but found '-'
(syntax)
🤖 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
`@deployments/helm-charts/wso2-ae-platform/templates/asdlc-api/seed-secret.yaml`
around lines 11 - 18, The seed Secret is incorrectly skipped when
.Values.secrets is empty, which breaks the databaseURL secret flow. Update the
seed-secret.yaml gating logic around the secret rendering so the asdlc-api-seed
resource is always created when databaseURL is part of the chain, even with the
default secrets: {} value. Keep the existing secret iteration logic in sync with
the resource template and make sure the databaseURL entry can still be published
by push-secret.yaml and consumed by the deployment via asdlc-api-secrets.
| stringData: | ||
| password: {{ .Values.postgres.auth.password | quote }} |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
This still renders the PostgreSQL password from chart values.
Moving the password into a Secret removes it from the pod spec, but templating .Values.postgres.auth.password here still keeps the value in Helm-managed manifests, and the current values contract still ships a repo-known default. To fully meet the secret-handling goal, generate this once for local installs or require an existing/external secret instead of rendering the password from chart values.
🤖 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 `@deployments/helm-charts/wso2-ae-platform/templates/postgres/secret.yaml`
around lines 14 - 15, The PostgreSQL secret template is still sourcing the
password from chart values via the secret rendering path, so it remains baked
into Helm-managed manifests. Update the secret-handling flow in the postgres
Secret template to stop templating .Values.postgres.auth.password directly:
either require an existing/external Secret reference or generate a one-time
local password during install and persist it outside chart values. Keep the
change centered around the postgres secret template and the postgres.auth values
contract so the password is no longer shipped from default values.
| echo "Checking if task signing key Secret already exists..." | ||
| if kubectl get secret "$SECRET_NAME" -n {{ .Release.Namespace }} >/dev/null 2>&1; then | ||
| echo "Task signing key Secret already exists (reusing existing key)" | ||
| echo "This preserves JWT verification across helm upgrades and pod restarts" | ||
| exit 0 | ||
| fi |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Validate the expected key before reusing the Secret.
The current check skips generation when the Secret object exists, even if task-signing.pem is missing. Check for that key before exiting so the mounted file is always present.
Proposed idempotency check
echo "Checking if task signing key Secret already exists..."
if kubectl get secret "$SECRET_NAME" -n {{ .Release.Namespace }} >/dev/null 2>&1; then
- echo "Task signing key Secret already exists (reusing existing key)"
- echo "This preserves JWT verification across helm upgrades and pod restarts"
- exit 0
+ if kubectl get secret "$SECRET_NAME" -n {{ .Release.Namespace }} -o jsonpath='{.data.task-signing\.pem}' | grep -q .; then
+ echo "Task signing key Secret already exists (reusing existing key)"
+ echo "This preserves JWT verification across helm upgrades and pod restarts"
+ exit 0
+ fi
+ echo "Task signing key Secret exists but task-signing.pem is missing; regenerating"
fi📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| echo "Checking if task signing key Secret already exists..." | |
| if kubectl get secret "$SECRET_NAME" -n {{ .Release.Namespace }} >/dev/null 2>&1; then | |
| echo "Task signing key Secret already exists (reusing existing key)" | |
| echo "This preserves JWT verification across helm upgrades and pod restarts" | |
| exit 0 | |
| fi | |
| echo "Checking if task signing key Secret already exists..." | |
| if kubectl get secret "$SECRET_NAME" -n {{ .Release.Namespace }} >/dev/null 2>&1; then | |
| if kubectl get secret "$SECRET_NAME" -n {{ .Release.Namespace }} -o jsonpath='{.data.task-signing\.pem}' | grep -q .; then | |
| echo "Task signing key Secret already exists (reusing existing key)" | |
| echo "This preserves JWT verification across helm upgrades and pod restarts" | |
| exit 0 | |
| fi | |
| echo "Task signing key Secret exists but task-signing.pem is missing; regenerating" | |
| fi |
🤖 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
`@deployments/helm-charts/wso2-agentic-engineer-bundle/templates/task-signing-key-job.yaml`
around lines 100 - 105, The reuse logic in the task signing key job only checks
whether the Secret exists, so it can exit even when the expected
task-signing.pem entry is missing. Update the Secret validation in the
task-signing-key job script to verify the key data exists before reusing it, and
only exit early when both the Secret and task-signing.pem are present; otherwise
continue with key generation so the mounted file is always available.
| githubClientSecret: | ||
| value: "" | ||
| forceDefaultOverride: "" |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Do not seed an empty GitHub client secret with IfNotExists.
The seed/push templates include non-false entries, so this can persist an empty value on first install and later non-force updates will not replace it. Mark this entry as externally pre-seeded or skip empty values in the seed/push flow.
🤖 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 `@deployments/helm-charts/wso2-agentic-engineer-bundle/values.yaml` around
lines 158 - 160, The githubClientSecret entry is currently treated like a
seedable value even though it is empty, which can cause an empty secret to be
persisted and block later updates. Update the values handling for
githubClientSecret in the seed/push flow so empty values are skipped or marked
as externally pre-seeded, and ensure the logic around the seed/push templates
does not apply IfNotExists semantics to this field.
1 participant
Purpose
Helm chart secrets were being passed as plaintext
value:fields in pod specs and embedded in Helm release history, violating basic secret hygiene.Resolves #56
Goals
value:in any pod spec or Helm release manifestApproach
Introduces an ESO-based secret management pipeline using OpenBao as the secret backend:
values.yaml (secrets.*.value)
→ K8s Secret "asdlc-api-seed" (staging area)
→ PushSecret → OpenBao (ESO pushes values in)
→ ExternalSecret (ESO reads back every 15s)
→ K8s Secret "asdlc-api-secrets"
→ pod env vars via secretKeyRef
Per-secret
forceDefaultOverridepolicy — each secret inwso2-ae-platform.secretsnow takes an object withvalueandforceDefaultOverride:forceDefaultOverride""(default)IfNotExists) — seeds on first install, never overwrites on upgrade"true"helm install/upgrade"false"Dev pattern: leave
forceDefaultOverride: ""— values are seeded into OpenBao on first install and never touched on upgrade. Values appear in Helm release history, which is acceptable for non-productionuse.
Production pattern: pre-seed each secret path in OpenBao before
helm install, then setforceDefaultOverride: "false"so the chart never writes to the store and values never appear in release history.Additional fixes:
OPENBAO_TOKENremoved fromdeployment.yaml— dead code (BFF never reads it)POSTGRES_PASSWORDfor the bundled dev StatefulSet now usessecretKeyRefvia a dedicatedasdlc-db-credentialsK8s Secret--set-file)ClusterSecretStorecreation skipped if one already exists in the cluster (idempotent installs)