feat(cozy-lib): add CA-only TLS trust-anchor helper for tenants - #2989
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (6)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughAdds a new Helm named template ChangesCA-only Secret Helm helper
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
0553b3c to
31dce87
Compare
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 provides a foundational mechanism for secure TLS trust-anchor distribution across Cozystack applications. By decoupling the CA certificate from secrets containing private keys, it enables tenants to verify per-app TLS endpoints safely. This approach establishes a standardized delivery pattern that simplifies future per-app TLS implementations while maintaining strict security boundaries. 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. 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
|
There was a problem hiding this comment.
Code Review
This pull request introduces a new Helm library helper cozy-lib.tls.caCertSecret in cozy-lib to safely render CA-only trust-anchor Secrets without exposing private keys. It includes comprehensive unit tests to verify proper validation behavior, such as rejecting private keys and ensuring the mandatory tenantresource label is applied. The reviewer suggested trimming the $caCert variable at assignment time to clean up whitespace and simplify the empty check.
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.
| {{- $caCert := default "" .caCert -}} | ||
| {{- if eq (trim $caCert) "" -}} | ||
| {{- fail "cozy-lib.tls.caCertSecret: caCert is required and must be a non-empty PEM" -}} | ||
| {{- end -}} |
There was a problem hiding this comment.
Trimming the $caCert variable at assignment time is cleaner and ensures that any accidental leading or trailing whitespace/newlines in the input .caCert are stripped before being stored in the Secret. This also simplifies the subsequent empty check.
{{- $caCert := trim (default "" .caCert) -}}
{{- if eq $caCert "" -}}
{{- fail "cozy-lib.tls.caCertSecret: caCert is required and must be a non-empty PEM" -}}
{{- end -}}
There was a problem hiding this comment.
Thanks — leaving this as-is intentionally. The empty check already trims (eq (trim $caCert) ""), so whitespace-only input is still rejected. I keep the stored value verbatim so the helper faithfully preserves the input PEM, including the trailing newline that certificate blocks conventionally carry; trimming it would strip that with no functional benefit for verification. So I'd rather not mutate the cert content here.
Per-app TLS issues a per-release self-signed CA, but the only objects that carry ca.crt — the cert-manager CA and leaf Secrets — also carry private keys. Granting a tenant read on those to obtain the trust anchor also hands over a server or CA private key. Add cozy-lib.tls.caCertSecret, which renders a canonical Opaque Secret holding only ca.crt and labelled internal.cozystack.io/tenantresource= true. The tenantsecret registry surfaces label-bearing Secrets under core.cozystack.io/tenantsecrets, which the base tenant roles already grant, so tenants reach the trust anchor without any grant on a Secret that contains tls.key. The helper fails closed on empty input and refuses any PEM carrying private key material. Covered by helm-unittest in the cozy-lib test chart. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la>
31dce87 to
73f6382
Compare
myasnikovdaniil
left a comment
There was a problem hiding this comment.
LGTM. Reviewed statically (library-helper change; stayed off dev10 due to an in-progress CNI roll).
The security core is sound: cozy-lib.tls.caCertSecret provably emits only ca.crt in an Opaque Secret — there is no path by which tls.key/tls.crt or private material reaches the rendered output. I independently verified the private-key guard with regexMatch: it matches every standard key header (PKCS#8, RSA, EC, DSA, ENCRYPTED, OPENSSH) including lowercase, and correctly does NOT match the CERTIFICATE header or a free-text friendlyName=… PRIVATE KEY … line. The "cert + trailing key block" negative test covers the realistic accident.
Verification:
make -C packages/tests/cozy-lib-tests test→ 14/14 pass.- Independent
helm templateof the consumer fixture: onlyca.crtinstringData, mandatoryinternal.cozystack.io/tenantresource=truelabel present,type: Opaque. - RBAC/registry claims cross-checked against
pkg/registry/core/tenantsecret/rest.go,pkg/apis/core/v1alpha1/tenantresource_types.go, andpackages/system/cozystack-basics/templates/clusterroles.yaml— accurate. NotesecretToTenantcopiessec.Datastraight through, which is exactly why emitting onlyca.crtis load-bearing — the helper does the right thing. - Diff is purely additive (+349/-0, 6 files), no change to existing chart behavior.
API naming (cozy-lib.tls.caCertSecret) and the single-dict-arg convention are consistent with the existing cozy-lib.* helpers. The fail-closed behavior on empty/keyed input is the correct posture for a foundational object.
One non-blocking suggestion below regarding the docstring usage example and the not-yet-issued (lookup returns empty) case the #2988/#2990 series will hit. Approving as-is.
| "namespace" .Release.Namespace | ||
| "caCert" $caCertPem | ||
| "labels" (dict "app.kubernetes.io/instance" .Release.Name) | ||
| ) }} |
There was a problem hiding this comment.
nit (non-blocking): the helper fails closed when caCert is empty (correct), but this usage example passes caCert unconditionally. The #2988 ACME flow will lookup the per-release CA, which returns empty on the first reconcile before cert-manager issues it — calling the helper at that point hard-fails the render. The safe idiom is a consumer-side guard:
{{- $caCert := (lookup "v1" "Secret" .Release.Namespace "wildcard-tls").data["ca.crt"] | default "" }}
{{- if $caCert }}
{{ include "cozy-lib.tls.caCertSecret" (dict "name" (printf "%s-ca-cert" .Release.Name) "namespace" .Release.Namespace "caCert" ($caCert | b64dec)) }}
{{- end }}
which renders nothing (no error) when the CA isn't issued yet and renders the trust anchor once it is. Consider showing the if $caCert guard in the docstring example so every per-app PR copies the not-yet-issued-safe pattern. Keep the helper's own fail-closed behavior as is.
What this PR does
Part of #2814. Part of #2811.
This is the foundational ("keystone") piece of WS3: a canonical, reusable trust-anchor object that lets a tenant obtain
ca.crtto verify a per-app TLS endpoint without read access to any object that also carries a private key. It unblocks the per-app TLS series (WS4/WS5) by giving every chart one agreed, key-free delivery shape instead of each chart inventing its own.The problem (verified against current
main)Per-app TLS issues a per-release, self-signed CA. The only objects that hold
ca.crttoday also hold private keys:<release>-cacarries the CA private key (tls.key) — full trust-chain compromise if leaked (packages/apps/nats/templates/certmanager.yaml, theisCACertificate);<release>-tlscarries the server private key (tls.key) plusca.crt(same file, the leaf Certificate);ca.crtonly inside its user-credentials Secret (packages/apps/postgres/templates/db.yaml, the operator-managed TLS block).So any RBAC path that hands a tenant
ca.crtby granting read on one of those Secrets also hands over a private key. Onmaintoday the cert-manager apps grant tenants no access to those Secrets at all, so a tenant currently cannot obtainca.crtto verify the server — the gap this object closes safely.Design decision: key-free Opaque object delivered through
tenantsecrets, not trust-managerI evaluated the two candidate delivery mechanisms.
Rejected — trust-manager. A trust-manager
Bundlereads its sources only from trust-manager's single configured "trust namespace"; itsnamespaceSelectorgoverns only where the bundle is distributed to, not where it is sourced from. A per-release self-signed CA lives in the tenant namespace, so trust-manager cannot use it as a source, and projecting one shared CA to all tenants would defeat per-release isolation. I re-verified this against current upstream docs (cert-manager.io trust-manager) and the open per-namespace-trust-bundle request (cert-manager/trust-manager#131, which is target-side only). The prior analysis still holds.Chosen — a
ca.crt-only Opaque Secret surfaced through the existing tenant-secret API. This generalizes the reference pattern from the redis work (#2729), where the operator publishes a CA-only Opaque Secret and RBAC is scoped to it instead of to the CA-private-key Secret. The newcozy-lib.tls.caCertSecrethelper renders that object once, identically, for every chart:type: Opaque, a singleca.crtdata key, notls.key/tls.crt;internal.cozystack.io/tenantresource: "true";How the RBAC wiring works (no new roles needed)
The label is the entire mechanism, and it routes through RBAC that already exists:
pkg/registry/core/tenantsecret/rest.gosurfaces, under the virtual resourcecore.cozystack.io/tenantsecrets, exactly the namespace Secrets bearinginternal.cozystack.io/tenantresource=true(buildTenantSelector, lines 199-213; label constantspkg/apis/core/v1alpha1/tenantresource_types.go:3-4; fullDatacopied through bysecretToTenant, lines 84-103).packages/system/cozystack-basics/templates/clusterroles.yamlgrantsget/list/watchoncore.cozystack.io/tenantsecretsto tenant ServiceAccounts viacozy:tenant:base(lines 44-47) and touse/admin/super-adminsubjects viacozy:tenant:use:base(lines 179-180). None of this touches raw coresecrets, so attaching the label to a key-free object exposes the trust anchor and nothing else.internal/backupcontroller/credentials_projector.go:128-133relies on the same rule in reverse — it deliberately omits the label so its key-bearing projection is not promoted to aTenantSecret. This helper is the positive counterpart: a key-free object that is safe to promote.viewdeliberately does not receive the trust anchor through this path:cozy:tenant:view:basegrants onlycore.cozystack.io/options(lines 120-126), andtenantsecretsalso includes credential Secrets, so granting it to a read-only role would leak passwords. The trust anchor reachesuseand above plus tenant ServiceAccounts — the same access level at which connection credentials are already surfaced (the per-release dashboard Role binds atuse, e.g.packages/apps/postgres/templates/dashboard-resourcemap.yamlviacozy-lib.rbac.subjectsForTenantAndAccessLevel). A chart that must showca.crtatviewlevel in the dashboard can additionally name the key-free Secret in its own per-release Role; that is a per-app detail, not a base-role change.Why a library helper and not a controller here
Populating
ca.crtstays the responsibility of whatever owns the PKI — the app operator (as the redis fork does) or a cert-manager chain resolved at the chart level. The helper is intentionally pure and value-driven so it renders deterministically and is the single shape every per-app PR converges on. A shared key-stripping projection controller is a reasonable future generalization but is out of scope for this foundational object.Convergence plan for the per-app TLS series (follow-up, not this PR)
I did not modify any of the open per-app branches. Each should adopt the canonical object as follows:
tenantresourcelabel so delivery matches the rest of the catalog.<release>-ca/<release>-tlsfrom tenants.tls.key.Tests
helm-unittestin the cozy-lib test chart (make -C packages/tests/cozy-lib-tests test) covers: the rendered object isOpaquewithca.crtand notls.key/tls.crt; thetenantresourcelabel is present; caller labels/annotations merge while the security label always wins; the helper fails closed on emptycaCert; and it refuses a PEM that carries private key material.Release note
Summary by CodeRabbit
Release Notes
New Features
Tests