design-proposal: SecretRef for external credentials in managed app specs#37
design-proposal: SecretRef for external credentials in managed app specs#37IvanHunters wants to merge 1 commit into
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a design proposal for using SecretRef to reference external credentials in managed application specs, preventing plaintext credentials from leaking into the custom resource specs. The review feedback suggests resolving a naming contradiction between the text and the example, addressing a potential visibility and UX challenge for tenants managing manually created Secrets, and clarifying the behavior when a referenced Secret does not exist yet to align with the render-time lookup limitations.
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.
|
|
||
| A credential field is expressed one of two ways depending on whether the engine can generate the value: | ||
|
|
||
| - **Generatable credential (engine user password).** The field accepts either the inline literal (unchanged) or a `valueFrom.secretKeyRef`. Exactly one; render `fail`s if both are set, matching the OIDC precedent (`oidc-authn-config.yaml:80-84`). Omitting both keeps today's auto-generation. |
There was a problem hiding this comment.
There is a slight contradiction here: the text mentions valueFrom.secretKeyRef, but the example directly below (line 125) uses passwordSecretRef, and Open Question #2 (line 215) explicitly lists this naming choice as unresolved.
We should update this text to refer to the flat passwordSecretRef structure or clarify that the naming is still under discussion to keep the proposal consistent.
| - **Generatable credential (engine user password).** The field accepts either the inline literal (unchanged) or a `valueFrom.secretKeyRef`. Exactly one; render `fail`s if both are set, matching the OIDC precedent (`oidc-authn-config.yaml:80-84`). Omitting both keeps today's auto-generation. | |
| - **Generatable credential (engine user password).** The field accepts either the inline literal (unchanged) or a passwordSecretRef (or similar flat reference). Exactly one; render fails if both are set, matching the OIDC precedent (oidc-authn-config.yaml:80-84). Omitting both keeps today's auto-generation. |
| ## Security | ||
|
|
||
| - **Primary win: the credential leaves the CR spec.** The value is no longer readable by `use`-tier subjects via `get <app>`. This is the whole point, and it closes the leak class where a credential set inline on a CR is readable by anyone who can `get` that CR. | ||
| - **Trust boundary of the referenced Secret.** It stays a same-namespace, tenant-owned Secret read at runtime by the operator/pod. It is deliberately **not** labelled `internal.cozystack.io/tenantresource` and **not** added to `spec.secrets`, so it is not projected outward as a TenantSecret — a private input must not become a tenant-visible output. Reviewers should confirm no conversion PR adds the referenced Secret to `spec.secrets`. |
There was a problem hiding this comment.
This design highlights an interesting visibility/UX challenge for the tenant:
If tenant ServiceAccounts only have access to the virtual core.cozystack.io/tenantsecrets resource (and not raw core/v1 Secrets), and a Secret is only visible via tenantsecrets if it has the internal.cozystack.io/tenantresource label, then a manually created Secret that is deliberately not labeled will be completely invisible to the tenant after creation.
This means the tenant won't be able to list, view, or verify the existence of their own pre-shared keys or passwords via the dashboard or standard CLI access, making troubleshooting and rotation very difficult.
We should consider adding this to the Open Questions section to determine how tenant-created input Secrets can be made visible/manageable to the tenant without violating the trust boundary or exposing them to other unauthorized tiers.
| ## Failure and edge cases | ||
|
|
||
| - Both inline and `secretRef` set → render `fail` with a clear message (OIDC precedent). | ||
| - `secretRef` names a Secret that does not exist yet → for a generatable credential, treated as "not supplied" only if inline/generation is also absent; for a required external secret, the app reports `NotReady` and does not start with an empty credential. It must not crash-loop or render an empty literal. |
There was a problem hiding this comment.
The phrasing here is somewhat confusing:
secretRefnames a Secret that does not exist yet → for a generatable credential, treated as "not supplied" only if inline/generation is also absent;
Since the chart does not perform lookup at render time to check for Secret existence (as established in Section 2), if a secretRef is specified in the values, the chart must always render the reference to it. It cannot dynamically fall back to auto-generation if the Secret is missing.
We should clarify this to state that if secretRef is provided, the reference is always used, and the application will wait for the Secret to be created (failing closed/remaining NotReady), regardless of whether the credential is normally generatable.
| - `secretRef` names a Secret that does not exist yet → for a generatable credential, treated as "not supplied" only if inline/generation is also absent; for a required external secret, the app reports `NotReady` and does not start with an empty credential. It must not crash-loop or render an empty literal. | |
| - secretRef names a Secret that does not exist yet → the chart always renders the reference to the specified Secret. The application will remain NotReady (fail-closed) until the tenant creates the Secret, regardless of whether the credential is normally generatable. |
📝 WalkthroughWalkthroughThis change adds a design proposal for SecretRef-based external credentials, including credential schemas, ChangesExternal credential SecretRef
Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
a0206d0 to
52d7fc3
Compare
Signed-off-by: IvanHunters <xorokhotnikov@gmail.com>
52d7fc3 to
78347fb
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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 `@design-proposals/external-credential-secretref/README.md`:
- Around line 95-101: Align the “defined two-phase bootstrap” goal with the
documented missing-Secret behavior: either narrow the fail-closed claim to an
absent reference field, or specify an admission/readiness mechanism that detects
missing referenced Secret objects and keys. Update both occurrences of this goal
so they consistently describe the implemented behavior, including the
user-scoped credential case.
- Around line 163-168: Update cozy-lib.credentials.secret to accept explicit
reference/skip state alongside inlineValueOrEmpty, and exclude referenced fields
from lookup, generation, and rendered release credentials. Update callers to
pass this state for every SecretRef-backed field, preserving inline generation
and reuse for non-referenced fields, and add coverage confirming referenced
fields are never generated.
- Around line 170-175: Update the “Two-phase bootstrap and RBAC” section to make
the tenant Secret write-path decision and a successful end-to-end bootstrap test
explicit prerequisites for rollout. Ensure adoption of reference-only consumers,
including site-gateway, is gated until tenants can create the referenced Secret
through the approved path and the full flow is validated.
- Around line 148-159: Update the “Rendering: reference-passing, never
content-copy” section to guarantee only that credentials are passed by
reference, not that rotation is uniformly observed live. Remove the claim that
rotation and late creation are always observed natively, and document
engine-specific restart, reconcile, or re-apply requirements for env-based and
SQL-applied credentials, consistent with the edge-case section.
- Around line 113-146: The proposal leaves the reference field naming ambiguous
between passwordSecretRef and the valueFrom.secretKeyRef envelope. Choose one
canonical field shape for generatable credentials, then update the shared
SecretKeyRef schema, examples, and helper signature references to use it
consistently; do not revisit the already-defined class-2/reference-shape split.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 07604371-d84c-48a4-882a-8e288f1aae96
📒 Files selected for processing (1)
design-proposals/external-credential-secretref/README.md
| ## Goals | ||
|
|
||
| - A single value-schema type and rendering convention for "this credential is a reference to a tenant-namespace Secret, not a literal", reusable by any managed-application chart. | ||
| - The referenced value never appears in the `apps.cozystack.io` CR spec, and the chart never copies it into a chart-owned Secret; it is consumed by the operator/pod at runtime by `{name, key}`. | ||
| - Additive and backward-compatible: existing inline fields keep working; existing clusters that set inline passwords are unaffected until they opt in. | ||
| - A `cozy-lib` helper that both (a) centralises the generate-or-reuse idiom currently duplicated across eight charts and (b) resolves the inline-or-reference choice into the operator's native reference field, so every engine wires it identically. | ||
| - A defined two-phase bootstrap (tenant creates the Secret, then references it) with fail-closed behaviour when the referenced Secret is absent. |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Align the fail-closed goal with missing-Secret behavior.
The goal promises fail-closed behavior when the referenced Secret is absent, but the design deliberately emits the reference without checking whether the Secret or key exists. For user-scoped credentials, the app can remain Ready while only the operator object is stuck. Either narrow the goal to a missing reference field, or define an admission/readiness mechanism for missing Secret objects and keys.
Also applies to: 197-203
🤖 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 `@design-proposals/external-credential-secretref/README.md` around lines 95 -
101, Align the “defined two-phase bootstrap” goal with the documented
missing-Secret behavior: either narrow the fail-closed claim to an absent
reference field, or specify an admission/readiness mechanism that detects
missing referenced Secret objects and keys. Update both occurrences of this goal
so they consistently describe the implemented behavior, including the
user-scoped credential case.
| ### 1. The reference type | ||
|
|
||
| A shared value-schema `@typedef` mirroring the Kubernetes `SecretKeySelector` shape tenants already know from `env[].valueFrom`: | ||
|
|
||
| ```yaml | ||
| ## @typedef {struct} SecretKeyRef - Reference to a key in an existing Secret in the tenant (release) namespace. | ||
| ## @field {string} name - Name of the Secret. Must exist in the application's own namespace. | ||
| ## @field {string} key - Key within the Secret whose value is the credential. | ||
| ``` | ||
|
|
||
| A credential field is expressed one of two ways depending on whether the engine can generate the value: | ||
|
|
||
| - **Generatable credential (engine user password).** The field accepts either the inline literal (unchanged) or a `valueFrom.secretKeyRef`. Exactly one; render `fail`s if both are set, matching the OIDC precedent (`oidc-authn-config.yaml:80-84`). Omitting both keeps today's auto-generation. | ||
|
|
||
| ```yaml | ||
| users: | ||
| app1: | ||
| # password: hackme # inline (still valid; discouraged) | ||
| passwordSecretRef: # OR reference | ||
| name: app1-db-password | ||
| key: password | ||
| ``` | ||
|
|
||
| - **Non-generatable external secret (PSK, BGP-MD5, integration token).** Reference-only. There is no inline form, because inline is exactly the plaintext-in-spec anti-pattern this removes. If a required reference is absent, the chart renders `fail`, so the HelmRelease — and therefore the app CR `Ready` condition — goes not-Ready with a clear message; it never renders an empty literal. See §4 for two-phase-bootstrap timing and Failure and edge cases for the missing-Secret-after-reference case. | ||
|
|
||
| ```yaml | ||
| site: | ||
| ipsec: | ||
| pskSecretRef: | ||
| name: peer-frankfurt-psk | ||
| key: psk | ||
| ``` | ||
|
|
||
| Because some operators take a name-only reference with a fixed key layout (class 2 in Context), each engine's schema documents the required Secret shape — `{name, key}` where the operator accepts a key selector, or `name` plus a mandated key set (e.g. `username`+`password`, or a `kubernetes.io/basic-auth`-typed Secret) where it does not. |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
set -euo pipefail
printf '\n== File outline ==\n'
ast-grep outline design-proposals/external-credential-secretref/README.md --view expanded || true
printf '\n== Relevant excerpts ==\n'
sed -n '1,240p' design-proposals/external-credential-secretref/README.md | cat -n
printf '\n== Search for related terms ==\n'
rg -n "valueFrom\.secretKeyRef|SecretKeyRef|credentials\.ref|name-only|class 2|flat-vs|pskSecretRef|passwordSecretRef|secretKeyRef" design-proposals/external-credential-secretref/README.md -n || trueRepository: cozystack/community
Length of output: 38945
🏁 Script executed:
set -euo pipefail
printf '\n== Find defining sections ==\n'
rg -n "credentials\.ref|class 2|name-only|SecretKeyRef|SecretKeySelector|secretKeyRef|flat|envelope" design-proposals/external-credential-secretref -S || true
printf '\n== Other related docs ==\n'
git ls-files | rg 'external-credential-secretref|credential|secretref|secretRef|secretKeyRef|oidc-authn-config|external-credential' || trueRepository: cozystack/community
Length of output: 8251
Lock down the field name here. The class-2/reference-shape split is already covered; the remaining open choice is passwordSecretRef: {name, key} vs. the full valueFrom.secretKeyRef envelope. Pick one so the shared schema and helper signature stay stable.
🧰 Tools
🪛 LanguageTool
[style] ~136-~136: Consider an alternative for the overused word “exactly”.
Context: ...re is no inline form, because inline is exactly the plaintext-in-spec anti-pattern this...
(EXACTLY_PRECISELY)
🤖 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 `@design-proposals/external-credential-secretref/README.md` around lines 113 -
146, The proposal leaves the reference field naming ambiguous between
passwordSecretRef and the valueFrom.secretKeyRef envelope. Choose one canonical
field shape for generatable credentials, then update the shared SecretKeyRef
schema, examples, and helper signature references to use it consistently; do not
revisit the already-defined class-2/reference-shape split.
| ### 2. Rendering: reference-passing, never content-copy | ||
|
|
||
| The chart resolves the choice into the operator's existing native reference field. It never reads the referenced Secret at render time (`lookup` is Flux-invisible, §Context) and never copies its content into a chart-owned Secret (`valuesFrom` is pinned, §Context). The value reaches the workload only at runtime, through the operator or a pod `secretKeyRef`, where rotation and late creation are observed natively. | ||
|
|
||
| Concretely, per credential the chart computes `(secretName, secretKey)`: | ||
|
|
||
| - reference set → `secretName, secretKey = ref.name, ref.key` (the tenant's Secret); | ||
| - inline set → the chart materialises `<release>-credentials` as today and uses `<release>-credentials, <field>`; | ||
| - neither, generatable → auto-generate into `<release>-credentials` as today, use that; | ||
| - neither, required-external → render `fail`, so the HelmRelease and the app CR `Ready` go not-Ready with a clear message. | ||
|
|
||
| then fills the operator CR's native field with `{secretName, secretKey}`. This is exactly what `kubernetes` OIDC does at `cluster.yaml:318-323`, generalised. |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Do not promise uniform live rotation.
This section says runtime reference passing means rotation is observed natively, but the edge-case section correctly notes that env-based and SQL-applied credentials do not rotate live. Make the contract guarantee only reference passing; document restart, reconcile, or re-apply requirements per engine.
Also applies to: 203-203
🧰 Tools
🪛 LanguageTool
[style] ~159-~159: Consider an alternative for the overused word “exactly”.
Context: ...with {secretName, secretKey}. This is exactly what kubernetes OIDC does at `cluster...
(EXACTLY_PRECISELY)
🤖 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 `@design-proposals/external-credential-secretref/README.md` around lines 148 -
159, Update the “Rendering: reference-passing, never content-copy” section to
guarantee only that credentials are passed by reference, not that rotation is
uniformly observed live. Remove the claim that rotation and late creation are
always observed natively, and document engine-specific restart, reconcile, or
re-apply requirements for env-based and SQL-applied credentials, consistent with
the edge-case section.
| `cozy-lib` has no credential helper today (`packages/library/cozy-lib/templates/` — only `tls.caCertSecret` is secret-adjacent). Two helpers move the duplicated logic into the library: | ||
|
|
||
| - `cozy-lib.credentials.secret` — the idempotent generate-or-reuse renderer: given `(namespace, secretName, dict-of-field→inlineValueOrEmpty, $)`, it `lookup`s the existing Secret, reuses present keys, generates (`randAlphaNum`) the missing ones, and renders the `<release>-credentials` Secret. This is the eight-chart inline idiom, factored once. It fails closed via `cozy-lib.checkInput` when `$` is not passed, like every other helper. | ||
| - `cozy-lib.credentials.ref` — the resolver from §2: given `(inlineValue, secretKeyRef, defaultName, defaultKey)`, returns the reference to splice into the operator CR — a `{name, key}` map for key-selectable operators (class 1), or a bare `name` for name-only operators (class 2), for which the helper also validates that the caller declared the operator's required key layout. Renders nothing; pure selection. | ||
|
|
||
| An engine chart then reads: call `cozy-lib.credentials.ref` per credential, drop the result into the operator's `secretKeyRef`/`importCredentialsSecret`/`auth.secretPath` field, and call `cozy-lib.credentials.secret` once for the inline/generated fields. No chart re-implements generation or reference selection. |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Make referenced fields explicit inputs to the Secret helper.
credentials.secret accepts field → inlineValueOrEmpty, which cannot distinguish “omitted, so generate” from “set by SecretRef, so do not create a chart-owned copy.” A referenced field could therefore generate an unused <release>-credentials value while the operator points at the tenant Secret, violating the no-copy contract and making rollback behavior ambiguous. Pass reference state or an explicit skip set into the helper, and test that referenced fields are never generated.
🤖 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 `@design-proposals/external-credential-secretref/README.md` around lines 163 -
168, Update cozy-lib.credentials.secret to accept explicit reference/skip state
alongside inlineValueOrEmpty, and exclude referenced fields from lookup,
generation, and rendered release credentials. Update callers to pass this state
for every SecretRef-backed field, preserving inline generation and reuse for
non-referenced fields, and add coverage confirming referenced fields are never
generated.
| ### 4. Two-phase bootstrap and RBAC | ||
|
|
||
| The referenced Secret lives in the application's own (tenant) namespace and is created by the tenant **before** the reference resolves. The credential value is read by the operator/pod at runtime from that namespace — the same-namespace `secretKeyRef` the operators already use — so no cross-namespace RBAC is introduced and the chart adds nothing to `spec.secrets` (the Secret is a tenant-owned input, not a chart-owned tenant-projected output; §Context, lineage webhook). | ||
|
|
||
| The open dependency is the tenant's ability to **create** that Secret. Tenant ServiceAccounts today get `get/list/watch` on `core.cozystack.io/tenantsecrets` and no verbs on raw `core/v1` Secrets (`clusterroles.yaml:45-51`). Whether the tenant `use`/`admin` tier can create a Secret in its namespace, or whether the write must go through a TenantSecret create path, is unresolved and tracked in Open Questions — it is a prerequisite for this contract to be usable end-to-end. | ||
|
|
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Gate rollout on the tenant Secret write path.
Reference-only credentials cannot work end-to-end while tenant Secret creation remains unresolved. The rollout currently schedules site-gateway adoption despite identifying this as a hard prerequisite. Make the write-path decision and an end-to-end bootstrap test explicit gates before adopting reference-only consumers.
Also applies to: 214-219
🤖 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 `@design-proposals/external-credential-secretref/README.md` around lines 170 -
175, Update the “Two-phase bootstrap and RBAC” section to make the tenant Secret
write-path decision and a successful end-to-end bootstrap test explicit
prerequisites for rollout. Ensure adoption of reference-only consumers,
including site-gateway, is gated until tenants can create the referenced Secret
through the approved path and the full flow is validated.
1 participant
What
Adds a design proposal under
design-proposals/external-credential-secretref/for a uniform way to carry an external-system credential in a managed-application spec by reference to a Secret, instead of as a literal in the CR spec.Why
An inline credential (
spec.users[].passwordon postgres/mariadb/rabbitmq/clickhouse) stays in theapps.cozystack.ioCR spec, which is readable by everyuse-tier subject through the dynamic API — so a value the platform keeps in a Secret for the operator is simultaneously exposed onkubectl get <app>. External secrets with no auto-generation fallback (IPsec PSK, BGP-MD5, integration tokens) have no option today except that inline, plaintext-in-spec form.The model
A
{name, key}reference (KubernetessecretKeyRefshape) that the chart wires into the backing operator's existing native secret reference. The value never enters the CR spec and is never copied into a chart-owned Secret; it is read by the operator/pod at runtime. Generatable credentials become inline-or-reference (auto-generation stays the default); non-generatable external secrets are reference-only. Acozy-libhelper centralises the generate-or-reuse idiom currently duplicated across seven charts and resolves the inline-vs-reference choice.The proposal generalises two precedents already in the tree (the
kubernetesOIDCsecretRefand the backups3CredentialsSecret.name). It is additive and backward-compatible: no migration, noSecret.typechange, no CRD-required-field addition.Related
cozystack/community#30(site-to-site gateway) consumes the reference-only form for IPsec PSK / BGP-MD5.design-proposals/unified-tls-pki(opposite direction: this brings credentials inward, that deliversca.crtoutward).Status: Draft — opening for direction review, ahead of the
cozy-libhelper and the per-engine conversions. One open prerequisite is called out in the proposal: whether a tenant can create the referenced Secret in its namespace today.Summary by CodeRabbit