feat(controller): engine-agnostic CA extraction for the tenant trust anchor#3299
feat(controller): engine-agnostic CA extraction for the tenant trust anchor#3299Aleksei Sviridkin (lexfrei) wants to merge 6 commits into
Conversation
github-actions
Bot
added
area/uncategorized
📝 WalkthroughWalkthroughAdds a CA source contract to ChangesCA certificate extraction
Estimated code review effort: 5 (Critical) | ~90+ minutes Sequence Diagram(s)sequenceDiagram
participant HelmRelease
participant CACertReconciler
participant ApplicationDefinition
participant OperatorSecret
participant TenantCASecret
HelmRelease->>CACertReconciler: reconcile application release
CACertReconciler->>ApplicationDefinition: read caCert source declaration
CACertReconciler->>OperatorSecret: read declared or labelled CA data
CACertReconciler->>CACertReconciler: validate PEM and retain ca.crt
CACertReconciler->>TenantCASecret: create or update key-free projection
Possibly related issues
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 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 |
github-actions
Bot
added
the
size/XXL
dosubot
Bot
added
the
area/platform
Aleksei Sviridkin (lexfrei)
removed
the
area/uncategorized
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 implements a secure, engine-agnostic framework for distributing CA certificates to tenants. By decoupling the trust anchor from the underlying operator-managed secrets, the platform prevents the accidental exposure of private key material. The solution provides a robust, fail-closed extraction path that supports both label-based and explicitly declared sources, ensuring consistent and secure trust anchor delivery across various managed services. 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. Ignored Files
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
|
![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 CA-extraction controller (cacert) that projects the TLS trust anchor of managed applications into a canonical, key-free Secret (<release>-tenant-ca) in the release namespace, allowing tenants to securely read CA certificates without exposing private keys. It updates the ApplicationDefinition CRD and CRD schema to support authoritative caCert declarations, configures the Postgres application definition to use this new mechanism, and updates the cozy-lib library chart helper to align with the new canonical naming and labelling. Additionally, it introduces a ValidatingAdmissionPolicy to restrict writes to CA-source Secrets to cert-manager only, updates controller RBAC permissions, and adds comprehensive unit and integration tests. I have no feedback to provide as there are no review comments to assess.
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.
![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 `@internal/controller/cacert/reconciler.go`:
- Around line 565-576: Treat any source Secret already occupying the canonical
projection name as a collision, regardless of whether it contains private key
material: update reconcileSourceAtCanonicalName to emit the required collision
warning and requeue instead of silently succeeding. Update
internal/controller/cacert/reconciler_test.go lines 524-550 to assert the
collision warning and resulting behavior rather than a successful no-op.
- Around line 885-895: Validate the namespace parsed from SourceRefAnnotation
against hr.Namespace before calling r.Reader.Get in the existing source-lookup
block. Reject or ignore references where ns differs from hr.Namespace, ensuring
no cross-namespace Secret read occurs while preserving same-namespace lookup
behavior.
- Around line 495-502: Update the reconciliation path around the src == nil
branch and pruneProjection so the persisted source mode remains available when
the declared Secret disappears, and pass the current ApplicationDefinition into
pruning to distinguish a removed declaration from a temporarily absent source.
Ensure removing spec.caCert withdraws the previously projected trust anchor, and
add a regression test covering source disappearance before declaration removal.
- Around line 960-968: Update projectionData to fully decode and parse every PEM
block as a certificate, rejecting malformed or truncated payloads, trailing
bytes, and non-certificate blocks while preserving private-key rejection. In
internal/controller/cacert/reconciler.go lines 960-968, replace header matching
with complete validation; in internal/controller/cacert/reconciler_test.go lines
66-67, use valid certificate fixtures; and in
internal/controller/cacert/reconciler_test.go lines 1383-1398, add malformed,
trailing-data, and non-certificate-block cases.
- Around line 1085-1089: Update the rendered Secret name validation after
strings.TrimSpace in the surrounding name-rendering function to reject values
that are not valid DNS1123 subdomains, using the existing
CACertDeclarationInvalid error/result convention instead of the generic error.
Preserve the empty-name check and return the validated name only when it
satisfies the Kubernetes naming rules.
🪄 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: 8f836b39-3c2b-4df5-a46a-93f85ee43477
📒 Files selected for processing (16)
api/v1alpha1/applicationdefinitions_types.goapi/v1alpha1/zz_generated.deepcopy.gocmd/cozystack-controller/main.gointernal/controller/cacert/postgres_wiring_test.gointernal/controller/cacert/reconciler.gointernal/controller/cacert/reconciler_test.gopackages/library/cozy-lib/templates/_tls.tplpackages/system/application-definition-crd/definition/cozystack.io_applicationdefinitions.yamlpackages/system/cozystack-basics/templates/publish-ca-cert-writer-policy.yamlpackages/system/cozystack-basics/tests/publish-ca-cert-writer-policy_test.yamlpackages/system/cozystack-controller/Makefilepackages/system/cozystack-controller/templates/rbac.yamlpackages/system/cozystack-controller/tests/rbac_test.yamlpackages/system/postgres-rd/cozyrds/postgres.yamlpackages/tests/cozy-lib-tests/templates/tests/tls-cacert.yamlpackages/tests/cozy-lib-tests/tests/tls_cacert_test.yaml
| if src == nil { | ||
| // Nothing resolved: no Secret opted in through the label, and the | ||
| // ApplicationDefinition declares no source. | ||
| // | ||
| // That is NOT yet enough to withdraw the trust anchor, because it conflates | ||
| // two opposite situations — the release genuinely stopped publishing, or its | ||
| // source is merely absent this instant. pruneProjection tells them apart. | ||
| return r.pruneProjection(ctx, hr) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Do not let a vanished source hide removal of a declaration.
If the declared Secret disappears before spec.caCert is removed, pruneProjection sees the recorded source as absent and retains the projection forever. Persist the source mode and pass the current definition into pruning so removing a declaration definitively withdraws its old trust anchor. Add a regression test for this ordering.
🤖 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 `@internal/controller/cacert/reconciler.go` around lines 495 - 502, Update the
reconciliation path around the src == nil branch and pruneProjection so the
persisted source mode remains available when the declared Secret disappears, and
pass the current ApplicationDefinition into pruning to distinguish a removed
declaration from a temporarily absent source. Ensure removing spec.caCert
withdraws the previously projected trust anchor, and add a regression test
covering source disappearance before declaration removal.
| func (r *Reconciler) reconcileSourceAtCanonicalName(src *source, target string) (ctrl.Result, error) { | ||
| for key, value := range src.secret.Data { | ||
| if containsPrivateKey(string(value)) { | ||
| r.warn(src.secret, reasonCanonicalNameOccupied, | ||
| "the Secret %q occupies the canonical trust-anchor name and carries private key material under %q; no trust anchor is published for this release until that Secret is renamed", | ||
| target, key) | ||
| return ctrl.Result{RequeueAfter: resyncInterval}, nil | ||
| } | ||
| } | ||
| // Key-free and already canonical: the engine publishes its own trust | ||
| // anchor. Leave it alone. | ||
| return ctrl.Result{}, nil |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Treat every foreign Secret at the canonical name as a collision.
The key-free branch silently accepts an object lacking the tenant-CA label, controller ownership, sanitization, and garbage collection, so it does not satisfy the canonical projection contract or emit the required collision warning.
internal/controller/cacert/reconciler.go#L565-L576: refuse and warn when the source itself occupies the canonical projection name.internal/controller/cacert/reconciler_test.go#L524-L550: expect a collision warning rather than a silent successful no-op.
📍 Affects 2 files
internal/controller/cacert/reconciler.go#L565-L576(this comment)internal/controller/cacert/reconciler_test.go#L524-L550
🤖 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 `@internal/controller/cacert/reconciler.go` around lines 565 - 576, Treat any
source Secret already occupying the canonical projection name as a collision,
regardless of whether it contains private key material: update
reconcileSourceAtCanonicalName to emit the required collision warning and
requeue instead of silently succeeding. Update
internal/controller/cacert/reconciler_test.go lines 524-550 to assert the
collision warning and resulting behavior rather than a successful no-op.
| if ns, name, ok := splitSourceRef(existing.Annotations[SourceRefAnnotation]); ok { | ||
| recorded := &corev1.Secret{} | ||
| err := r.Reader.Get(ctx, types.NamespacedName{Namespace: ns, Name: name}, recorded) | ||
| switch { | ||
| case apierrors.IsNotFound(err): | ||
| // MERELY ABSENT. Hold the trust anchor and wait for the source to come | ||
| // back — fast while the release is young, then on the ordinary resync. | ||
| wait := missingSourceWait(hr) | ||
| logger.Info("the CA source is gone; keeping the published trust anchor and waiting for it to return", | ||
| "secret", hr.Namespace+"/"+target, "source", ns+"/"+name, "retryIn", wait) | ||
| return ctrl.Result{RequeueAfter: wait}, nil |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Constrain recorded source lookups to the release namespace.
SourceRefAnnotation is mutable, but its namespace is used directly with the cluster-wide API reader. A forged cross-namespace reference creates a Secret-existence oracle and can suppress withdrawal when the target is absent. Reject references where ns != hr.Namespace before performing the read.
🧰 Tools
🪛 ast-grep (0.44.1)
[warning] 892-893: A log/format call (log.Print/Printf/Println, the Fatal/Panic variants, fmt.Sprintf, or a structured logger's Info/Warn/Error/Debug method) is given a message built by concatenating a string literal with a non-literal value such as request data. Unsanitized, attacker-controlled input written to logs enables log forging / CRLF injection: an attacker can inject newlines to spoof log entries or break log parsers. Do not concatenate raw input into the log message; pass it as a separate structured field/argument (e.g. 'log.Printf("user: %s", user)' or 'logger.Info("login", "user", user)') and strip or escape newline characters first.
Context: logger.Info("the CA source is gone; keeping the published trust anchor and waiting for it to return",
"secret", hr.Namespace+"/"+target, "source", ns+"/"+name, "retryIn", wait)
Note: [CWE-117] Improper Output Neutralization for Logs.
(log-injection-request-data-concat-go)
🤖 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 `@internal/controller/cacert/reconciler.go` around lines 885 - 895, Validate
the namespace parsed from SourceRefAnnotation against hr.Namespace before
calling r.Reader.Get in the existing source-lookup block. Reject or ignore
references where ns differs from hr.Namespace, ensuring no cross-namespace
Secret read occurs while preserving same-namespace lookup behavior.
| name := strings.TrimSpace(buf.String()) | ||
| if name == "" { | ||
| return "", errors.New("rendered to an empty name") | ||
| } | ||
| return name, nil |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map relevant structure first
ast-grep outline internal/controller/cacert/reconciler.go --view expanded || true
# Show the target area with line numbers
sed -n '1020,1115p' internal/controller/cacert/reconciler.go
# Search for name validation / declaration invalid errors in this controller package
rg -n "CACertDeclarationInvalid|rendered to an empty name|dns subdomain|IsDNS1123Subdomain|IsValidSecret|secret name|NotFound" internal/controller/cacert -SRepository: cozystack/cozystack
Length of output: 9711
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '560,740p' internal/controller/cacert/reconciler.go
printf '\n---\n'
sed -n '840,920p' internal/controller/cacert/reconciler.go
printf '\n--- tests ---\n'
sed -n '150,240p' internal/controller/cacert/reconciler_test.goRepository: cozystack/cozystack
Length of output: 15206
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find any explicit validation of Secret names in this controller package or repo.
rg -n "IsDNS1123Subdomain|IsValidSecretName|Validate.*Secret|validation.*secret|CACertDeclarationInvalid|unusable caCert declaration" . -SRepository: cozystack/cozystack
Length of output: 50375
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the renderSourceName call path and the handling of its errors.
nl -ba internal/controller/cacert/reconciler.go | sed -n '600,740p'
printf '\n---\n'
nl -ba internal/controller/cacert/reconciler.go | sed -n '860,920p'
printf '\n---\n'
nl -ba internal/controller/cacert/reconciler_test.go | sed -n '150,240p'
printf '\n---\n'
rg -n "renderSourceName\(" internal/controller/cacert/reconciler.go internal/controller/cacert/reconciler_test.go -n -SRepository: cozystack/cozystack
Length of output: 197
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show the surrounding implementation of declaredSource and upsertProjection.
nl -ba internal/controller/cacert/reconciler.go | sed -n '640,730p'
printf '\n---\n'
nl -ba internal/controller/cacert/reconciler.go | sed -n '700,760p'
printf '\n---\n'
nl -ba internal/controller/cacert/reconciler.go | sed -n '860,920p'Repository: cozystack/cozystack
Length of output: 197
🏁 Script executed:
#!/bin/bash
set -euo pipefail
nl -ba internal/controller/cacert/reconciler.go | sed -n '640,740p'
printf '\n---\n'
nl -ba internal/controller/cacert/reconciler.go | sed -n '860,920p'Repository: cozystack/cozystack
Length of output: 197
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "IsDNS1123Subdomain|IsValidSecretName|Validate.*DNS|validation\.Is" internal/controller/cacert . -SRepository: cozystack/cozystack
Length of output: 869
Validate the rendered Secret name before using it. strings.TrimSpace only catches the empty case; a template can still render a value that is not a valid DNS1123 subdomain, which will fail later in reconciliation. Return CACertDeclarationInvalid here instead.
🤖 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 `@internal/controller/cacert/reconciler.go` around lines 1085 - 1089, Update
the rendered Secret name validation after strings.TrimSpace in the surrounding
name-rendering function to reject values that are not valid DNS1123 subdomains,
using the existing CACertDeclarationInvalid error/result convention instead of
the generic error. Preserve the empty-name check and return the validated name
only when it satisfies the Kubernetes naming rules.
myasnikovdaniil
left a comment
myasnikovdaniil
left a comment
There was a problem hiding this comment.
Reviewed the whole controller, the VAP, the RBAC delta, the cozy-lib guard, and the tests as the tree rather than the diff, and traced both source legs end to end against the tenant read path. The security story holds: tenants read trust anchors only through the label-gated, read-only tenantsecrets API and cannot write raw Secrets, the label leg is admission-gated to cert-manager, and the declared leg stays authoritative over the forgeable label leg — so the change fails closed on every tenant-influenceable path, is namespace-confined with no cross-tenant vector, and is unusually well tested (forged-label override, forged-projection healing, non-Opaque collision, recreate-rehome, poisoned rotation, prune discrimination).
Approving — everything below is non-blocking. The two follow-ups most worth doing are the x509-parse hardening of the write-path guard (it currently validates by header substring only) and tightening the prune/canonical-name corner, where a retired anchor can linger if a declaration is removed while its source is momentarily absent. Two nits I did not inline: selectorsDigest keeps only the low 64 bits of the SHA-256 (reconciler.go:952), and applicationDefinition returns the first kind match without flagging duplicate kinds (reconciler.go:696) — both harmless with platform-authored input.
| if containsPrivateKey(pem) { | ||
| return nil, errPrivateKey | ||
| } | ||
| if !certificateHeader.MatchString(pem) { |
There was a problem hiding this comment.
Non-blocking, defense-in-depth. This guard validates the trust anchor by header substring — BEGIN CERTIFICATE present and no BEGIN … PRIVATE KEY — but never decodes the PEM or runs x509.ParseCertificate, so certificate armor wrapped around non-certificate bytes (e.g. headerless DER) passes and is copied verbatim. It is not reachable from a shipped path: both legs' sources come from trusted writers (the operator-created <release>-ca, or cert-manager, which the writer VAP gates) and a tenant can write neither. But since this is the stated fail-closed boundary, worth making the check match the claim: decode with encoding/pem and assert every block is a CERTIFICATE that x509.ParseCertificate accepts, with no trailing remainder. The mirrored chart guard in _tls.tpl shares the limitation.
| } | ||
| // Key-free and already canonical: the engine publishes its own trust | ||
| // anchor. Leave it alone. | ||
| return ctrl.Result{}, nil |
There was a problem hiding this comment.
Two small asymmetries on the key-free canonical branch, both misconfig-only (no shipped engine declares <release>-tenant-ca as its source). It returns ctrl.Result{} with no RequeueAfter, so on the unwatched name-driven leg a self-source that later gains key material is never re-examined, whereas every other path here requeues on resyncInterval; and unlike the shared write path it does not assert the single-ca.crt/Opaque contract. It stamps no tenant-ca/tenantresource label, so it grants no tenant visibility on its own — but return ctrl.Result{RequeueAfter: resyncInterval} plus a warning on extra keys would make this corner symmetric with the rest.
| err := r.Reader.Get(ctx, types.NamespacedName{Namespace: ns, Name: name}, recorded) | ||
| switch { | ||
| case apierrors.IsNotFound(err): | ||
| // MERELY ABSENT. Hold the trust anchor and wait for the source to come |
There was a problem hiding this comment.
Correctness edge worth a look. If a declared source disappears and spec.caCert is then removed, resolveSource falls through to the empty label leg and returns nil, so pruneProjection runs — but because the recorded source is still absent it lands in this MERELY ABSENT branch and holds+requeues the projection indefinitely, even though removing the declaration is a definitive opt-out. The retired anchor stays tenant-readable until the source name reappears or the HelmRelease is deleted. The absent-vs-opt-out discriminator keys off the source Secret's existence, which cannot tell 'source rotating' apart from 'declaration withdrawn while the source happens to be gone'. Same-namespace and low-severity, but the withdrawal never fires.
| # platform controller's own ServiceAccount; no tenant RBAC is widened. | ||
| - apiGroups: [""] | ||
| resources: ["secrets"] | ||
| verbs: ["get", "list", "watch", "create", "update", "patch", "delete"] |
There was a problem hiding this comment.
This widens the controller ServiceAccount from cluster-wide Secret read (already covered by the '*'/'*': get,list,watch catch-all) to cluster-wide create/update/patch/delete. It is genuinely required — projections land in tenant namespaces and RBAC cannot scope a verb to a name suffix — but it enlarges the blast radius if this SA is compromised, now co-located with cozystack.io/*: *. Non-blocking; worth a threat-model line, or later splitting the projection writes onto a dedicated SA that holds only the Secret verbs.
| failurePolicy is Fail: a Secret whose write cannot be evaluated is denied, | ||
| not admitted — fail-closed, as a trust-anchor control must be. | ||
| */}} | ||
| {{- $certManagerWriter := "system:serviceaccount:cozy-cert-manager:cert-manager" }} |
There was a problem hiding this comment.
Two non-blocking operational notes on this policy. The allowed writer is pinned as a literal, so a cert-manager repackage/rename must move it in lockstep; it fails closed (denies the new identity, killing the label leg) rather than open, and since postgres uses the declared leg the policy gates nothing live yet, so the drift would only surface when the first label-leg engine converges — deriving the identity from the cert-manager package values at render time would remove the seam. Separately, failurePolicy: Fail + [Deny] with no namespace/object selector means a Velero or kubectl restore of a labelled cert-manager Secret during DR is denied (the restorer is not cert-manager); cert-manager re-mints so it is recoverable, but it is worth a runbook note.
The trust-anchor contract is a label on the CA Secret, but two engines cannot honour it. CloudNativePG builds its PKI Secrets without applying spec.inheritedMetadata, so the chart's labels never reach <release>-ca, and Percona Server for MongoDB exposes no secretTemplate at all and reconciles an out-of-band patch away. Add an optional ApplicationDefinition.spec.caCert declaring, per engine, the name of the Secret its operator creates and the key inside it that holds the certificate. Registering an engine is then data, not code. The declaration only names an object the platform already reads; nothing a tenant can read is widened by it, and a labelled Secret still takes precedence where an engine can label its own output. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la>
Every engine in the catalog issues a per-release CA, but the object that holds ca.crt almost always holds a private key next to it, so any RBAC path that hands a tenant the trust anchor hands over key material too. Add a controller that publishes, for each application release, a canonical Opaque Secret <release>-tenant-ca holding only ca.crt. That name is claimed by none of the operators the platform ships, which matters: <release>-ca-cert, the obvious candidate, is already taken and means opposite things across engines — a key-free CA on one, a key-BEARING Secret on another — so projecting onto it could only be done by overwriting live key material. The certificate is found two ways. A Secret labelled internal.cozystack.io/publish-ca-cert is how a chart opts in, and it is watched, so a rotation propagates at once. For the engines whose operator owns the CA Secret and will not label it, the application's caCert declaration names that Secret, which is read by name and re-read on a resync. The two legs differ only in how the source is found — extraction, sanitization, ownership and collision handling are one shared write path, because forking it is where a key-leak bug would hide. Exactly one whitelisted key is ever copied, and every byte written is re-checked for a PEM private-key header first: on the name-driven leg the source is key-bearing by construction. The projection is owner-referenced to the release, so deleting the application collects it with no prune logic, and a Secret the controller did not create is never overwritten — a Secret carrying key material on the canonical name is refused with a Warning Event rather than adopted. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la>
Wire the reconciler into the platform controller manager and grant its ServiceAccount the Secret verbs the projection needs: create/update/patch to publish the trust anchor, and delete to withdraw it when an application stops publishing one. Read was already covered by the catch-all rule, and no tenant RBAC is widened. Give the reconciler its own dedicated, scoped Secret cache rather than narrowing the manager's shared cache. Only labelled CA sources are held in that informer, so a cluster-wide Secret watch never caches every Secret, and so every private key, in the cluster; the operator-owned name-driven sources and the projections are read through the uncached API reader instead. A private cache also keeps this scoping independent of any other controller on the same manager that needs to scope Secrets differently. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la>
The CA-extraction controller publishes every engine's trust anchor as an Opaque "<release>-tenant-ca" Secret labelled internal.cozystack.io/tenant-ca. That label is the single, engine-agnostic selector an ApplicationDefinition puts in spec.secrets, and it is what ultimately reaches the tenant. This helper is the other producer of that same object, for charts that already hold the CA PEM at render time. It agreed with neither half of the contract: it documented "<release>-ca-cert" as the name, and it stamped no tenant-ca label at all. Both are now fixed, and both mattered. The name "<release>-ca-cert" is not merely a second choice, it is unusable: Percona Server for MongoDB creates a Secret of exactly that name itself and puts a private key in it, so a chart rendering a trust anchor there would be writing over live key material. Without the tenant-ca label, the two producers would converge on the name but not on the label. A helper-rendered anchor would be missed by the generic selector, the lineage webhook would mark it tenantresource=false, and the tenant would be locked out of the very Secret the helper exists to publish — while a controller-projected one, of the same name, worked. Also record what the tenantresource label does and does not do. The lineage webhook recomputes it from the definition's spec.secrets selectors on every admission and overwrites whatever the chart wrote, so rendering it is necessary but not sufficient for tenant read access. Signed-off-by: Aleksei Sviridkin <f@lex.la>
Declare CNPG's operator-created CA Secret as the postgres trust-anchor
source (spec.caCert names {{ .release }}-ca), and add the generic
internal.cozystack.io/tenant-ca selector to spec.secrets so the key-free
<release>-tenant-ca projection the extraction controller publishes becomes
readable by the tenant.
postgres is the first converged engine, and the hardest: CNPG creates the
CA Secret itself, key-bearing, asynchronously, and offers no way to label
it, so it exercises the name-driven leg end to end. The test drives the
shipped ApplicationDefinition rather than a fixture, so the wiring and the
controller cannot drift apart unseen: a CNPG-shaped key-bearing
<release>-ca appears, and the projection must be produced key-free AND
selected for tenant visibility by the shipped selector.
Signed-off-by: Aleksei Sviridkin <f@lex.la>
The CA-extraction controller's label-driven leg treats a core Secret labelled internal.cozystack.io/publish-ca-cert as a trust-anchor source and lifts its ca.crt into the tenant-readable projection. That label is the whole attribution on that leg and sits on an ordinary Secret, so without a platform-side control any principal that can write a Secret in the namespace could forge a source or rewrite an existing one's ca.crt and swap the trust anchor a tenant is handed as vouched. Add a ValidatingAdmissionPolicy that allows a Secret carrying that label to be created or updated only by cert-manager's controller ServiceAccount, the sole legitimate writer of these Secrets. It covers both the forge-new (CREATE) and tamper-existing (UPDATE, including a relabel-away evasion via the old object) shapes, and by keying on the publish-ca-cert label it never touches the declared-leg source, the projection, or any unlabelled Secret. The allowed identity is pinned as a literal to the shipped cert-manager package's controller ServiceAccount; a comment marks it as the fragile seam that must move if cert-manager is repackaged. Signed-off-by: Aleksei Sviridkin <f@lex.la>
Aleksei Sviridkin (lexfrei)
dismissed
myasnikovdaniil’s stale review
via
98df611
Aleksei Sviridkin (lexfrei)
force-pushed
the
feat/ca-extraction-controller
branch
from
c5d95dc to
98df611
Compare
github-actions
Bot
added
the
area/uncategorized
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@internal/controller/cacert/reconciler.go`:
- Around line 791-821: Update the projection reconciliation logic around
hasOwner to require exactly one OwnerReference matching owner, rather than
merely checking whether the desired owner exists. Treat any list with extra,
stale, or differing references as non-compliant and replace
existing.OwnerReferences with a single owner entry, while preserving the
early-return path only when labels, annotations, data, and ownership all match
exactly.
🪄 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: 4dc4b7c7-ff0d-4dbc-be35-e80204e60fe1
📒 Files selected for processing (15)
api/v1alpha1/applicationdefinitions_types.goapi/v1alpha1/zz_generated.deepcopy.gocmd/cozystack-controller/main.gointernal/controller/cacert/postgres_wiring_test.gointernal/controller/cacert/reconciler.gointernal/controller/cacert/reconciler_test.gopackages/library/cozy-lib/templates/_tls.tplpackages/system/application-definition-crd/definition/cozystack.io_applicationdefinitions.yamlpackages/system/cozystack-basics/templates/publish-ca-cert-writer-policy.yamlpackages/system/cozystack-basics/tests/publish-ca-cert-writer-policy_test.yamlpackages/system/cozystack-controller/templates/rbac.yamlpackages/system/cozystack-controller/tests/rbac_test.yamlpackages/system/postgres-rd/cozyrds/postgres.yamlpackages/tests/cozy-lib-tests/templates/tests/tls-cacert.yamlpackages/tests/cozy-lib-tests/tests/tls_cacert_test.yaml
🚧 Files skipped from review as they are similar to previous changes (9)
- packages/system/application-definition-crd/definition/cozystack.io_applicationdefinitions.yaml
- packages/tests/cozy-lib-tests/templates/tests/tls-cacert.yaml
- internal/controller/cacert/postgres_wiring_test.go
- packages/system/cozystack-basics/tests/publish-ca-cert-writer-policy_test.yaml
- packages/library/cozy-lib/templates/_tls.tpl
- api/v1alpha1/zz_generated.deepcopy.go
- packages/system/cozystack-controller/tests/rbac_test.yaml
- api/v1alpha1/applicationdefinitions_types.go
- cmd/cozystack-controller/main.go
| if maps.EqualFunc(existing.Data, desired, bytes.Equal) && | ||
| existing.Labels[TenantCALabel] == trueValue && | ||
| existing.Annotations[SourceRefAnnotation] == ref && | ||
| existing.Annotations[SelectorsDigestAnnotation] == digest && | ||
| hasOwner(existing.OwnerReferences, owner) { | ||
| return nil | ||
| } | ||
|
|
||
| // Merge, never replace: the lineage admission webhook stamps its own labels | ||
| // (managed-by-cozystack, tenantresource, application.*) on the projection at | ||
| // every admission, and replacing the map wholesale would strip the | ||
| // tenantresource label the tenant's read path depends on. | ||
| if existing.Labels == nil { | ||
| existing.Labels = map[string]string{} | ||
| } | ||
| existing.Labels[TenantCALabel] = trueValue | ||
| existing.Labels[ManagedLabel] = trueValue | ||
| if existing.Annotations == nil { | ||
| existing.Annotations = map[string]string{} | ||
| } | ||
| existing.Annotations[SourceRefAnnotation] = ref | ||
| existing.Annotations[SelectorsDigestAnnotation] = digest | ||
| if !hasOwner(existing.OwnerReferences, owner) { | ||
| // REPLACE, never append. A projection has exactly one controller owner, | ||
| // and the API server rejects a second reference with Controller=true | ||
| // outright ("Only one reference can have Controller set to true"). | ||
| // A stale reference is exactly what deleting and recreating an | ||
| // application under the same name leaves behind — same release name, new | ||
| // UID — so appending would wedge the reconciler on an Invalid error | ||
| // instead of re-homing the projection onto the live release. | ||
| existing.OwnerReferences = []metav1.OwnerReference{owner} |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Enforce exactly one owner reference on every projection.
Lines 795 and 813 only verify that the desired HelmRelease owner exists, so an added second owner survives reconciliation. After the HelmRelease disappears, Line 429 stops reconciliation and garbage collection retains the Secret while that additional owner remains, leaving the trust anchor published indefinitely.
Require the owner-reference list to contain exactly the desired owner and replace it otherwise.
Proposed fix
- hasOwner(existing.OwnerReferences, owner) {
+ len(existing.OwnerReferences) == 1 &&
+ hasOwner(existing.OwnerReferences, owner) {
return nil
}
...
- if !hasOwner(existing.OwnerReferences, owner) {
+ if len(existing.OwnerReferences) != 1 || !hasOwner(existing.OwnerReferences, owner) {
existing.OwnerReferences = []metav1.OwnerReference{owner}
}Based on PR objectives, projections must be garbage-collected after application deletion.
📝 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.
| if maps.EqualFunc(existing.Data, desired, bytes.Equal) && | |
| existing.Labels[TenantCALabel] == trueValue && | |
| existing.Annotations[SourceRefAnnotation] == ref && | |
| existing.Annotations[SelectorsDigestAnnotation] == digest && | |
| hasOwner(existing.OwnerReferences, owner) { | |
| return nil | |
| } | |
| // Merge, never replace: the lineage admission webhook stamps its own labels | |
| // (managed-by-cozystack, tenantresource, application.*) on the projection at | |
| // every admission, and replacing the map wholesale would strip the | |
| // tenantresource label the tenant's read path depends on. | |
| if existing.Labels == nil { | |
| existing.Labels = map[string]string{} | |
| } | |
| existing.Labels[TenantCALabel] = trueValue | |
| existing.Labels[ManagedLabel] = trueValue | |
| if existing.Annotations == nil { | |
| existing.Annotations = map[string]string{} | |
| } | |
| existing.Annotations[SourceRefAnnotation] = ref | |
| existing.Annotations[SelectorsDigestAnnotation] = digest | |
| if !hasOwner(existing.OwnerReferences, owner) { | |
| // REPLACE, never append. A projection has exactly one controller owner, | |
| // and the API server rejects a second reference with Controller=true | |
| // outright ("Only one reference can have Controller set to true"). | |
| // A stale reference is exactly what deleting and recreating an | |
| // application under the same name leaves behind — same release name, new | |
| // UID — so appending would wedge the reconciler on an Invalid error | |
| // instead of re-homing the projection onto the live release. | |
| existing.OwnerReferences = []metav1.OwnerReference{owner} | |
| if maps.EqualFunc(existing.Data, desired, bytes.Equal) && | |
| existing.Labels[TenantCALabel] == trueValue && | |
| existing.Annotations[SourceRefAnnotation] == ref && | |
| existing.Annotations[SelectorsDigestAnnotation] == digest && | |
| len(existing.OwnerReferences) == 1 && | |
| hasOwner(existing.OwnerReferences, owner) { | |
| return nil | |
| } | |
| // Merge, never replace: the lineage admission webhook stamps its own labels | |
| // (managed-by-cozystack, tenantresource, application.*) on the projection at | |
| // every admission, and replacing the map wholesale would strip the | |
| // tenantresource label the tenant's read path depends on. | |
| if existing.Labels == nil { | |
| existing.Labels = map[string]string{} | |
| } | |
| existing.Labels[TenantCALabel] = trueValue | |
| existing.Labels[ManagedLabel] = trueValue | |
| if existing.Annotations == nil { | |
| existing.Annotations = map[string]string{} | |
| } | |
| existing.Annotations[SourceRefAnnotation] = ref | |
| existing.Annotations[SelectorsDigestAnnotation] = digest | |
| if len(existing.OwnerReferences) != 1 || !hasOwner(existing.OwnerReferences, owner) { | |
| // REPLACE, never append. A projection has exactly one controller owner, | |
| // and the API server rejects a second reference with Controller=true | |
| // outright ("Only one reference can have Controller set to true"). | |
| // A stale reference is exactly what deleting and recreating an | |
| // application under the same name leaves behind — same release name, new | |
| // UID — so appending would wedge the reconciler on an Invalid error | |
| // instead of re-homing the projection onto the live release. | |
| existing.OwnerReferences = []metav1.OwnerReference{owner} | |
| } |
🤖 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 `@internal/controller/cacert/reconciler.go` around lines 791 - 821, Update the
projection reconciliation logic around hasOwner to require exactly one
OwnerReference matching owner, rather than merely checking whether the desired
owner exists. Treat any list with extra, stale, or differing references as
non-compliant and replace existing.OwnerReferences with a single owner entry,
while preserving the early-return path only when labels, annotations, data, and
ownership all match exactly.
IvanHunters
left a comment
IvanHunters
left a comment
There was a problem hiding this comment.
LGTM. The engine-agnostic cacert controller is fail-closed on every security-critical path: the declared source takes priority over the forgeable label leg, the write path copies exactly one whitelisted ca.crt and rejects any private-key/non-cert PEM, owner-ref GC is native, and Secret.type immutability is handled. The new VAP is a GA in-apiserver policy with no webhook pod (no cold-start trap) and rejects no existing Flux write (no chart writes the publish-ca-cert label). Additive CRD field with committed deepcopy/schema, no migration; cozyrds selects the key-free tenant-ca projection, so there is no TLS key leak.
Non-blocking follow-ups (already tracked): x509-parse the guard instead of header-match only; distinguish declaration-removed from source-temporarily-absent in pruneProjection; harden the cross-ns source lookup; the hardcoded cert-manager SA in the VAP is a documented fragile seam.
myasnikovdaniil
left a comment
myasnikovdaniil
left a comment
There was a problem hiding this comment.
LGTM — re-reviewed after the rebase onto current main. The core controller, _tls.tpl, the API types, the CRD, the postgres wiring, and the ValidatingAdmissionPolicy's rendered output are byte-identical to the previously-approved revision. The force-push added two targeted tests and two documentation clarifications, on top of the environment change from the wildcard-secret controller landing on main.
What I re-verified
- The only behavioral change to this controller's environment is that the manager's shared Secret informer is now scoped (by the wildcard-secret controller) where it was previously unscoped. That makes the invariant "every Secret read goes through the uncached
Readeror this controller's private, scopedcaSourceClustercache — never the shared cached Client" load-bearing rather than a review-only guarantee. Audited every Secret access: the five Secret reads (reconciler.go:613viaCache;:677,:719,:869,:887viaReader) all bypass the shared cache; the remaining cachedGet/Listcalls (:424,:692,:1243,:1272) are HelmRelease / ApplicationDefinition, not Secrets; writes (:736,:830,:907) bypass the cache. The scoping does not regress this controller. - The two new tests (
TestReconcile_NameDrivenSource_ReadThroughUncachedReader,TestReconcile_UpsertExistingCheck_ReadThroughUncachedReader) lock exactly that invariant; both pass.go test ./internal/controller/cacert/...andgo vetare green, andhelm unittestpasses for the cozystack-basics VAP and the cozystack-controller RBAC. - The doc-only changes are accurate: the VAP comment explaining why DELETE is intentionally ungated (integrity-only policy; a DELETE-matching VAP would also gate the garbage-collector and namespace-controller and wedge namespace teardown), and the RBAC comment now attributing the shared Secret write grant to both reconcilers (verbs unchanged).
- The two red CI checks are unrelated to this PR:
Build packages/apps/kubernetesfails on an unresolvable upstreamcentos:stream9image digest (that package is untouched here), andAnalyze (go)was canceled mid-run after its Go build step passed.
Non-blocking follow-ups
- Single-owner enforcement on the projection (
reconciler.go:795,:813) — see inline comment. - Carried over from the earlier review, still applicable (these files are unchanged):
- decode and
x509.ParseCertificatethe write-path guard instead of validating by PEM-header substring (reconciler.go:965, mirrored in_tls.tpl); - the declaration-withdrawn-while-source-absent prune corner leaves a retired anchor tenant-readable until the source reappears or the release is deleted (
:890); - the key-free canonical branch returns no
RequeueAfterand skips the single-ca.crt/Opaque assertion (:576); selectorsDigestretains only the low 64 bits of the SHA-256 (:952);applicationDefinitionreturns the first kind match without flagging duplicate kinds (:696);- the cluster-wide Secret write grant enlarges the controller ServiceAccount's blast radius (
rbac.yaml); - the VAP's cert-manager writer is pinned as a literal, and a DR restore of a labelled source is denied (
publish-ca-cert-writer-policy.yaml).
- decode and
None of these block merge.
| } | ||
| existing.Annotations[SourceRefAnnotation] = ref | ||
| existing.Annotations[SelectorsDigestAnnotation] = digest | ||
| if !hasOwner(existing.OwnerReferences, owner) { |
There was a problem hiding this comment.
Non-blocking hardening. The up-to-date short-circuit (:795) and this normalization both check only that the desired HelmRelease owner is present (hasOwner), not that it is the sole owner. A namespace actor able to update the projection could append a second, non-controller owner reference pointing at a long-lived object; it survives reconciliation, and once the HelmRelease is deleted the garbage collector keeps the Secret because that other owner still exists — the retired trust anchor lingers indefinitely.
Impact is bounded: the projection is key-free, same-namespace, and self-inflicted — the same class as the known limitation already noted in the PR description — so this is not a blocker. Cheap to close by gating both sites on len(existing.OwnerReferences) == 1 && hasOwner(...); worth a line in the known-limitations note either way.
3 participants
What this PR does
A tenant connecting to a managed TLS endpoint needs the CA certificate (
ca.crt) to verify the server, and nothing more. Today the platform delivers it for no engine except kafka, and the in-flight per-app TLS work would each hand the tenant a Secret that also carries a private key — the cert-manager CA Secret holdstls.key, CloudNativePG's<release>-caholdsca.key. Granting read on one of those to deliver the trust anchor hands over key material too.This adds a controller that publishes, for every engine, one canonical key-free object per release: an Opaque Secret named
<release>-tenant-caholding onlyca.crt. The name is deliberately not<release>-ca-cert, which Percona Server for MongoDB already creates carrying a private key.Source discovery has two legs behind one write path, and the declared leg wins. When an ApplicationDefinition declares
spec.caCert, that named source is authoritative and a labelled Secret is not consulted for the engine; the label leg serves only engines with no declaration. This ordering is a security boundary: the declaration lives on a platform object no tenant can write, while a publish label sits on a Secret a namespace writer could create — so a declared engine's trust anchor cannot be swapped for a forged one.The name-driven (declared) leg exists because CloudNativePG and Percona Server for MongoDB create their CA Secret themselves and cannot be made to label it. The label-driven leg serves the cert-manager-minting charts, which opt in through
Certificate.spec.secretTemplate.labels; on it a new required labelinternal.cozystack.io/publish-ca-cert-releasenames the release, because a cert-manager-issued Secret carries no OwnerReference (the platform shipsenableCertificateOwnerRef: false) and no Helm metadata, so nothing else can attribute it to a release.The write path is the single place bytes reach a projection, on both legs. It copies exactly one whitelisted key under the canonical
ca.crtand fails closed on any value that carries a PEM private-key header or is not a PEM certificate — so the guard matters most on the name-driven leg, where the source is key-bearing by construction. The projection is owner-referenced to the application's HelmRelease withBlockOwnerDeletion: false(theapps.cozystack.iokinds are virtual, so there is no application CR in etcd to reference), so deleting the application collects it natively. A Secret of the canonical name the controller did not create is never overwritten; the collision surfaces as a Warning Event.postgres is wired as the first consumer, validating the mechanism against the hardest input — an operator-created, asynchronous, key-bearing CA Secret on the name-driven leg. Its ApplicationDefinition declares CNPG's
<release>-caand selects the projection for tenant visibility with the genericinternal.cozystack.io/tenant-calabel. The remaining engines are tracked in #2814.The label leg is guarded at admission. A
ValidatingAdmissionPolicyrestricts writes to any Secret carryinginternal.cozystack.io/publish-ca-certto cert-manager's controller ServiceAccount, on both CREATE and UPDATE (checking the old object as well, so relabel-away is also caught). A forged or tampered label-leg source is therefore rejected before it can become a tenant's trust anchor, closing the one path where a namespace writer could otherwise spoof an anchor for a label-only engine.The complementary visibility label
internal.cozystack.io/tenant-ca— which the tenant-facing selector matches — is deliberately not admission-guarded here: this controller is its only writer today, and promoting a foreign key-bearing Secret through it (for example CNPG's<release>-ca, which holdsca.key) would require raw core-secrets write in the tenant namespace, which base tenant roles do not grant. That is one instance of the broader tenant-RBAC key-exposure surface tracked under the unified-TLS epic (#2811); this change does not widen its reachability beyond adding the label, and closing it belongs to that epic's RBAC work rather than a second per-label policy here.Known limitation: a tenant who strips the
internal.cozystack.io/ca-cert-copymarker from their own projection makes the controller treat it as foreign and stop updating that one anchor — self-inflicted, same-namespace, no key leak, a direct consequence of never adopting a Secret it cannot prove it owns.Release note
Closes #3286
Summary by CodeRabbit
New Features
ca.crt.Documentation
Tests