feat(nats): publish ca.crt-only trust anchor to tenants - #3340
Conversation
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
github-actions
Bot
added
size/M
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughNATS adds a TLS-gated ChangesNATS tenant CA projection
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant NATSChart
participant TenantProjectionController
participant TenantSecretsAPI
participant Tenant
NATSChart->>TenantProjectionController: Render TLS-gated TenantProjection
TenantProjectionController->>TenantSecretsAPI: Publish release tenant-ca with ca.crt
Tenant->>TenantSecretsAPI: Retrieve CA certificate
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 |
|
Closing — opened before the branch cleared its review gate. Will reopen once the gate is green. |
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
IvanHunters
left a comment
IvanHunters
left a comment
There was a problem hiding this comment.
Reviewed hermetically. LGTM with non-blocking notes.
Verified:
- Same key-free trust-anchor shape as the qdrant change: the key-bearing CA Secret is labeled
publish-ca-cert, the tenant cozyrds selector matchestenant-ca, the leaf cert is not published. No private key reaches tenants. helm unittest53/53; mutation test (removing the labels) turns the test red, so it is non-vacuous.- Upgrade: additive
secretTemplate.labelson the CA Secret withrotationPolicy: Never, no reissue; cozyrds change only adds an include entry, no RBAC contraction. - Inert until controller #3299 lands.
Non-blocking:
- cert-manager propagating new
secretTemplate.labelsonto an already-existing CA Secret without reissue is expected v1.5+ behaviour but was reasoned, not executed (hermetic review). Worth confirming once exercised on a cluster that the existing CA Secret gains the labels without rotation.
|
Confirmed the secretTemplate propagation at source level rather than by reasoning. In cert-manager v1.20.2 (the version vendored in this repo) the issuing controller's I re-ran the mutation in both directions while checking this: dropping the secretTemplate block from the CA certificate turns the suite red, and leaking the same labels onto the leaf certificate turns the Agreed the on-cluster confirmation is still worth having. It gets exercised naturally once the controller side lands, since this change is inert until then. |
Aleksei Sviridkin (lexfrei)
added
the
do-not-merge/hold
8eefc6f to
2a633e7
Compare
github-actions
Bot
added
size/L
Aleksei Sviridkin (lexfrei)
added
area/database
myasnikovdaniil
left a comment
myasnikovdaniil
left a comment
There was a problem hiding this comment.
LGTM — correct, and the hardening added under review makes this the reference implementation of the contract. Merge after #3299.
Business context: tenants with TLS enabled get no CA to verify the server against, and the only existing CA Secret carries the private key; this opts nats into the shared key-free trust-anchor contract so tenants receive ca.crt alone.
What this PR actually does
Worth stating precisely, because the framing in the series description undersells the security property. This PR does not project ca.crt — it creates no Secret at all. It does exactly two things: stamps the publish-ca-cert labels on the cert-manager-authored CA Secret via secretTemplate, and adds a matchLabels selector to the ResourceDefinition. The key-free guarantee lives entirely in #3299's controller, which builds the projection from parsed x509 blocks.
What this PR does own is the inverse risk: stamping a tenant-visible label onto the key-bearing Secret. That is the thing worth guarding, and this PR guards it.
Verified
- Wire contract matches #3299 exactly — all three constants line up with
SourceLabel(reconciler.go:229),SourceReleaseLabel(:262),TenantCALabel(:268). - The negative tests have teeth.
tests/certmanager_test.yaml:167-190assertsnotExistsfor bothtenant-caandtenantresourceon both key-bearing certs (CA at documentIndex 1, leaf at 3). I mutation-tested this: stampinginternal.cozystack.io/tenant-ca: "true"onto the CAsecretTemplatefails the suite with "key-bearing CA Secret is never marked as a tenant-readable trust anchor". That is the assertion that matters, since the RD selector added by this PR ismatchLabels: internal.cozystack.io/tenant-ca: "true"and the CA Secret holds the CA private key undertls.key. hack/check-nats-rd-secrets.batscovers a layer helm-unittest cannot. Neither the cozyrds nor the dashboard Role is a chart template, so without this guard, deleting the selector or adding a key-bearing name toresourceNameswould be unguarded. It runs clean under the real harness (hack/cozytest.sh hack/check-nats-rd-secrets.bats→ 3/3) and uses${BATS_TEST_FILENAME:-$0}— the idiom that avoids theset -uabort trap that has bitten other suites in this repo.- Fully gated when TLS is disabled (
templates/certmanager.yaml:4) — nothing renders. - The README command is accurate —
tenantsecretis a real singular resource (pkg/registry/core/tenantsecret/rest.go:43), a pass-through name view over Secrets labelledtenantresource=true. helm unittest .→ 55/55.
Scope drift — worth crediting rather than flagging
Advertised at +34/-0 across 3 files; now +189/-0 across 5. That is 5.6×, but the functional core is unchanged at 6 lines (4 in certmanager.yaml, 2 in the cozyrds). The added 155 lines are entirely hardening picked up under review — the 107-line bats guard, 57 lines of helm-unittest, 19 lines of README. Exactly the right direction of travel for a security-shaped change.
Follow-ups (non-blocking, both belong to #3299)
1. Disabling TLS strands a published CA. enableCertificateOwnerRef: false at packages/system/cert-manager/charts/cert-manager/values.yaml:268 is not overridden in the cozystack overlay, so flipping tls.enabled true→false removes the Certificate but leaves the <release>-ca Secret with the publish labels intact. #3299's pruneProjection withdraws only on positive opt-out, so a surviving labelled Secret reads as still-opted-in and the projection persists indefinitely. Reachable through an ordinary operation, since tls.enabled follows external. Not a key leak — the projection stays key-free — but tenants keep a trust anchor for an instance no longer serving TLS. Fix belongs in the controller or a shared chart pattern, not here.
2. Docs placement, to settle across the pair. This PR adds 19 lines of tenant-facing README; #3341 argues user-facing documentation belongs with the controller that creates it (#3299) rather than with a per-app label leg. Both positions are defensible but they cannot both be right for the same contract — worth picking one and applying it to both engines.
Merge order
After #3299. Both labels have zero producers and consumers on main, so this is inert until the controller lands, and merging ahead of it would arm an admission control whose CEL is verified by #3299's chainsaw suite. #3299 is reviewed and ready.
4d744d2
4d744d2 to
44e94cf
Compare
github-actions
Bot
added
the
area/uncategorized
44e94cf to
4556ff5
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
d53f936 to
366634a
Compare
7453dd2 to
10a13b9
Compare
A tenant enabling TLS had no way to obtain the CA certificate without also being granted a Secret that carries a private key, so the server certificate could not actually be verified. Render a TenantProjection sentinel naming the chart's CA Secret as the source of a key-free CACert projection. The CA-extraction controller publishes that projection as <release>.tenant-ca, and a label selector on the application definition surfaces it to the tenant through tenantsecrets. The key-bearing <release>-ca stays tenant-invisible. The sentinel is gated on the chart's effective TLS state, the same condition that governs the cert-manager objects, because the source Secret exists only while TLS is on. It deliberately carries no ownerReferences, namespace or chart-added labels: the controller derives the published name from the helm.toolkit.fluxcd.io/name label Flux stamps, and an ownerReference would short-circuit that lineage walk. Assisted-By: Claude <[email protected]> Signed-off-by: Aleksei Sviridkin <[email protected]>
The ResourceDefinition and the dashboard Role are the tenant-facing half of the trust-anchor delivery path, and neither is a chart template, so the helm-unittest suite covers neither. Deleting the tenant-CA selector or granting a key-bearing Secret by name both left the suite fully green. nats-<name>-ca holds the CA private key and nats-<name>-tls the server key. Granting either to a tenant would let the holder issue certificates rather than merely verify them. What a client needs is ca.crt alone, delivered as the key-free <release>.tenant-ca projection and selected by label, since a label is stamped only on an object the platform produced while a name grant conveys whatever occupies the name. The include set is asserted as a whitelist so a key-bearing Secret cannot slip in under a spelling the test did not anticipate. Signed-off-by: Aleksei Sviridkin <[email protected]> Assisted-By: Claude <[email protected]>
Enabling TLS gave a tenant no documented way to obtain the CA certificate, so the server could not actually be verified — which is the point of turning TLS on in the first place. Records where the trust anchor lives (<release>.tenant-ca, reached through tenantsecrets), and why the chart's nats-<name>-ca and nats-<name>-tls Secrets are not that path: both carry private keys, so read access to either would convey the ability to issue certificates rather than verify them. Signed-off-by: Aleksei Sviridkin <[email protected]> Assisted-By: Claude <[email protected]>
10a13b9 to
936bdf0
Compare
myasnikovdaniil
left a comment
myasnikovdaniil
left a comment
There was a problem hiding this comment.
The security property holds and I could not break it. Requesting changes because the description describes a design that is no longer in the diff and says the change is inert when it is live on merge, and because nothing anywhere witnesses the one premise that differs from the postgres case this follows.
Start with what is solid, since it is most of the PR. The projection is ca.crt and nothing else by construction, not by assertion: projectionData returns a single literal map key and rebuilds the value from re-encoded certificate DER rather than copying the source, and update replaces Data wholesale so no key survives a rotation. Ten mutations, nine red, including pointing sourceKey at tls.key, stamping either tenant label on the key-bearing CA secretTemplate, stamping it on the leaf, adding ownerReferences, removing the TLS gate, and replacing the gate with a naive read of .Values.tls.enabled. The notExists guards fail closed rather than vacuously, renaming the CA secretName so the selector matches nothing gives document not found rather than a pass, which was the shape most worth suspecting here.
The three grant surfaces agree. Base tenant RBAC grants only tenantsecrets get/list/watch with no direct core secrets grant anywhere, the RD adds the label selector, and the dashboard Role stays at <release>-credentials matching postgres. The label-only entry is name-unbounded but contained, because the webhook resolves each object's own lineage to its own definition, and the two key-bearing cert-manager Secrets have no resolvable lineage at all under enableCertificateOwnerRef: false, so they are never marked readable. Even a hand-stamped label on them would not help an attacker.
And it is the postgres pattern rather than a parallel one, byte-identical in the RD comment included. The one divergence, the TLS gate, is justified since the nats CA exists only while TLS resolves true and an ungated sentinel would sit SourceNotFound forever on every plaintext release, and both implied legs of the tri-state are pinned.
Now the two things.
The description is stale in a way that changes how this gets reviewed. It describes stamping publish-ca-cert labels via secretTemplate on packages/apps/nats/templates/certmanager.yaml across a five-file diff. certmanager.yaml is untouched, the diff is six files, and the mechanism is a TenantProjection sentinel. It also says inert until the CA-extraction controller lands and held from merge until then. That controller is on main as of #3407, so this takes effect the moment it merges, and do-not-merge/hold is guarding a condition that no longer exists. That is not tidiness. Inert is exactly the premise under which a reviewer skips checking the CA source name and key, which is the one thing here that needs checking.
Because nothing witnesses that nats-<name>-ca carries ca.crt. The source name is verifiable in-repo at templates/certmanager.yaml:19. The key is not. hack/e2e-chainsaw/ holds 24 suites and none is nats, and nats appears in no hack/e2e-*.bats and no workflow. postgres has exactly this witness at hack/e2e-chainsaw/postgres/chainsaw-test.yaml:138-207, and its own comment says why it has to exist, the unit tests encode the same assumption the shipped ApplicationDefinition encodes so the two agree by construction and cannot catch each other being wrong. Substitute cert-manager for CNPG and that is where nats stands. If cert-manager does not populate ca.crt on a selfSigned plus isCA: true Certificate's Secret, the sentinel sits Ready=False/SourceNotReady on every TLS-enabled nats forever, the tenant still cannot verify the server, and all 61 helm-unittest cases and 3 bats cases stay green because none of them touches a cluster. templates/nats.yaml:62-64 already points the upstream chart's tlsCA.secretName at that Secret and the upstream chart mounts ca.crt from it, which is decent corroboration, but that chart is not vendored here so it is reasoning rather than evidence. Three lines from a dev stand close it: the key list of nats-<name>-ca, and the projection appearing with both labels and exactly one key.
Two cheap tightenings, both mutation-demonstrated. tests/tenant_projection_test.yaml:44-50 pins spec.projections[0] and nothing pins the count, and hasDocuments: count: 1 counts documents rather than list entries, so appending a second CACert entry declaring sourceKey: tls.key renders and leaves the suite at 61 of 61. The controller does fail closed on that, it refuses the whole sentinel above one entry and projectionData would refuse tls.key regardless, so it is a silent feature death rather than a leak. lengthEqual on spec.projections closes it. And hack/check-nats-rd-secrets.bats:20-24 promises the grants are whitelists so no key-bearing Secret can slip in under an unanticipated spelling, while the extraction reads only resourceNames. Appending a matchLabels selector to spec.secrets.include leaves all three tests passing. Not exploitable today for the lineage reason above, but the label dimension is the one this PR added and it is the one the guard does not check. Whitelist the whole include list.
Both gaps exist in postgres too, so fixing them here is a pattern fix rather than a nats regression.
One accuracy note while the tests are open. tests/certmanager_test.yaml:145-148 says stamping either label there would hand that key to every tenant subject. It would not, today, for the lineage reason above. Keep the assertions, they become load-bearing the moment secretTemplate adds helm labels or enableCertificateOwnerRef flips, just soften the claim. And the labels carry area/database while nats is messaging.
hasDocuments counts rendered documents, not list entries, so a second projection naming tls.key as its source rendered inside the same document and left the suite green. The controller refuses a sentinel carrying more than one entry, which makes that a silent loss of the tenant's trust anchor. Assert the list length alongside the entry it pins. Signed-off-by: Aleksei Sviridkin <[email protected]>
The guard filtered each surface down to the entries that name a Secret and compared the subset. Every such filter admits the spelling it did not anticipate: on the ResourceDefinition a label selector names no resource, and controller.cert-manager.io/fao is stamped on both key-bearing Secrets, so a selector on it grants the CA private key; on the dashboard Role a rule with no resourceNames names no Secret, a rule with resources ["*"] names no resource, and a ClusterRole is not a Role. All of them passed green. Compare the whole ResourceDefinition include list, the rendered RBAC kinds and the Role's complete rule list instead. Aggregating with eval-all also drops the dependence on the Role being the first rendered document. The header claimed both surfaces are out of reach of helm-unittest for want of a chart template; both are rendered, and what is missing is a suite that asserts on them. Signed-off-by: Aleksei Sviridkin <[email protected]>
…t reaches The comment claimed a visibility label on either cert-manager Certificate would hand its private key to every tenant subject. It would not, today: tenantsecrets serves only what the lineage webhook marked tenantresource=true, and lineage resolves through ownerReferences or the Flux helm label, neither of which a cert-manager Secret carries while enableCertificateOwnerRef stays false. The assertions stay -- they become load-bearing as soon as either gate moves, and one of them is not this chart's to hold. Signed-off-by: Aleksei Sviridkin <[email protected]>
A selfSigned isCA Certificate's Secret carries the certificate under ca.crt and tls.crt alike, which invites reading the source key as the wrong one of the two. ca.crt is what the trust-anchor contract names and what the projection republishes under. Signed-off-by: Aleksei Sviridkin <[email protected]>
The section documents how to retrieve the anchor without stating when it is there at all. It is absent on a plaintext release, which is the default for an instance that is not exposed outside the cluster. Signed-off-by: Aleksei Sviridkin <[email protected]>
|
Both points were right, and the description was the worse of the two: it described a The Both tightenings are in, each demonstrated by the mutation it was written to catch. The |
IvanHunters
left a comment
IvanHunters
left a comment
There was a problem hiding this comment.
LGTM with non-blocking notes.
Core is identical to #3341 and correctly copies the merged postgres pattern (only ca.crt reaches the tenant, canonical <release>.tenant-ca, lineage isolation intact).
- [MINOR] hack/check-nats-rd-secrets.bats — the guard reads the source file
packages/system/nats-rd/cozyrds/nats.yamldirectly instead of rendering the chart. The qdrant sibling rendershelm templateand would catch a second or dotfile document undercozyrds/*; nats would not. Only one file exists there today, so no live miss, but the qdrant approach is stronger by this PR's own argument. - [MINOR] Role coverage lives only inside bats (one permutation) whereas #3341 ships a full helm-unittest
dashboard-resourcemap_test.yamlwith a tri-state TLS matrix. Consider converging both PRs on the stronger scheme. - [NIT] templates/tenant-projection.yaml — stray cert-manager comment not present in the qdrant/postgres versions.
## What this PR does Converges qdrant onto the key-free CA trust-anchor contract (#2814), the same way #3340 does it for nats. The chart declares a `TenantProjection` sentinel naming its cert-manager CA Secret. The CA-extraction controller reads that declaration and publishes a key-free copy, `ca.crt` and nothing else, as `<release>.tenant-ca`. The qdrant ResourceDefinition selects that projection by label so it shows up in the tenant's secret list. No key-bearing Secret gains a tenant-visible label. The sentinel is gated on the chart's TLS condition. Engines whose operator mints a CA unconditionally do not need that, but qdrant only has a CA while TLS is on, and an ungated sentinel would sit `Ready=False`/`SourceNotFound` on every plaintext release. This was held while the CA-extraction controller was still open. That controller landed in #3407, so the sentinel is honoured as soon as this merges and the hold is gone. Tests pin the invariants that otherwise fail silently. The sentinel carries no `ownerReferences`, so the lineage walk falls through to the Flux release label. Neither key-bearing Certificate stamps a tenant-visible label on its Secret. The two objects that decide which Secrets a tenant may actually read, the dashboard Role with its binding and the ResourceDefinition, are pinned whole rather than checked for known-bad names: a rule granting secrets with no `resourceNames` is unrestricted rather than narrow, and a label selector can reach a key-bearing Secret while every name in the file stays correct, so neither adds a name for a probe to notice. Part of #2814. ### Follow-ups What this PR does not do, and where each belongs instead. - nats converges the same contract in #3340 and has no `dashboard-resourcemap_test.yaml`, so the dashboard Role pin belongs there too. Tracked in #3701 along with postgres, which already carries the label selector with no guard behind it. - The qdrant e2e fixture runs plaintext (`hack/e2e-chainsaw/qdrant/qdrant.yaml` sets `external: false`), so nothing in CI walks the path from the sentinel to `tenantsecrets`. postgres has a `verify-tenant-ca-projection` step to copy. - `hack/check-qdrant-rd-secrets.bats` is engine-agnostic apart from two literals, the chart path and the expected Secret set. The third copy of it is the point to parameterise over `packages/system/*-rd/cozyrds/` instead of copying again. ### Downstream repositories Walked the trigger map in `docs/agents/contributing.md` against the diff, file by file. Nothing needs a manual follow-up, but not for the reason a values-only walk would give: the website reference page for an app is regenerated from that package's `README.md`, and this PR does change it. `qdrant` is in the app list hardcoded in the website `Makefile`, so the docs bot picks the new section up on the next stable tag and there is nothing to open by hand. The Terraform provider's schema is hand-written against `values.schema.json`, which is untouched, and the provider offers no typed access to the new projection, the same position it holds for every other Secret it does not model. No package is added, renamed or removed, and the `cozyrds` file is edited in place rather than renamed, so the ccp dependency-contract anchor still resolves. User-facing documentation for consuming the trust anchor ships with the chart README, matching #3340. - [x] No downstream repository is affected by this change - [ ] [cozystack/website](https://github.com/cozystack/website) - follow-up: - [ ] [cozystack/terraform-provider-cozystack](https://github.com/cozystack/terraform-provider-cozystack) - follow-up: - [ ] [cozystack/ansible-cozystack](https://github.com/cozystack/ansible-cozystack) - follow-up: - [ ] [cozystack/ccp](https://github.com/cozystack/ccp) - follow-up: - [ ] [cozystack/talm](https://github.com/cozystack/talm) - follow-up: - [ ] [cozystack/cozyhr](https://github.com/cozystack/cozyhr) - follow-up: - [ ] [cozystack/cozy-proxy](https://github.com/cozystack/cozy-proxy) - follow-up: - [ ] [cozystack/cozystack-telemetry-server](https://github.com/cozystack/cozystack-telemetry-server) - follow-up: - [ ] [cozystack/external-apps-example](https://github.com/cozystack/external-apps-example) - follow-up: - [ ] [cozystack/examples](https://github.com/cozystack/examples) - follow-up: ### Release note ```release-note feat(qdrant): expose the CA certificate to tenants as a key-free `<release>.tenant-ca` Secret ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Tenant environments can securely access the Qdrant TLS CA certificate without receiving the private key. * TLS CA access is automatically enabled when TLS or external access is configured. * **Documentation** * Expanded TLS guidance explains certificate validation and provides commands for retrieving the tenant CA certificate. * **Tests** * Added coverage for TLS projections, secret visibility, dashboard resources, and secure fail-closed secret exposure. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
myasnikovdaniil
left a comment
myasnikovdaniil
left a comment
There was a problem hiding this comment.
LGTM. Both blockers from last round are closed and I verified them by mutation instead of reading: lengthEqual on spec.projections and the whole-include whitelist both go red now, so does dropping the tls gate, replacing it with a naive .Values.tls.enabled, stamping either tenant label on the CA secretTemplate, and adding <release>-ca to the dashboard Role. Ten mutations, nine red. The one that stayed green is in the inline comment.
do-not-merge/hold is still on the PR and your own body says it stopped guarding anything once #3407 landed, so it needs removing or this cannot merge. Labels also say area/database and area/uncategorized while nats is messaging.
Branch is 338 commits behind main and the e2e run is from Aug 15. Those failures are chainsaw/kubernetes-previous and chainsaw/kubernetes-latest, tenant k8s suites, and nats is in no e2e suite at all, so unrelated. I merged onto current main locally to check anyway: clean merge, helm unittest packages/apps/nats 61/61, bats guard 3/3, bats-no-exit-trap and cozytest-capture-gate green. Rebase and a fresh run still owed.
Dashboard Role is pinned at one permutation inside bats while #3341 ships a tri-state matrix in dashboard-resourcemap_test.yaml. That is #3701, not this PR.
| # guard stays green. | ||
|
|
||
| REPO_ROOT="$(cd "$(dirname "${BATS_TEST_FILENAME:-$0}")/.." && pwd)" | ||
| COZYRDS="$REPO_ROOT/packages/system/nats-rd/cozyrds/nats.yaml" |
There was a problem hiding this comment.
This reads one file but templates/cozyrd.yaml ships .Files.Glob "cozyrds/*", so a second file in that directory carries its own tenant grants past all three tests. I added cozyrds/nats-extra.yaml with resourceNames: [nats-{{ .name }}-ca] and the suite stayed green. Rendering the nats-rd chart, like the qdrant guard does, closes it and also catches a dotfile, which helm ignores only under templates/.
## What this PR does Converges qdrant onto the key-free CA trust-anchor contract (#2814), the same way #3340 does it for nats. The chart declares a `TenantProjection` sentinel naming its cert-manager CA Secret. The CA-extraction controller reads that declaration and publishes a key-free copy, `ca.crt` and nothing else, as `<release>.tenant-ca`. The qdrant ResourceDefinition selects that projection by label so it shows up in the tenant's secret list. No key-bearing Secret gains a tenant-visible label. The sentinel is gated on the chart's TLS condition. Engines whose operator mints a CA unconditionally do not need that, but qdrant only has a CA while TLS is on, and an ungated sentinel would sit `Ready=False`/`SourceNotFound` on every plaintext release. This was held while the CA-extraction controller was still open. That controller landed in #3407, so the sentinel is honoured as soon as this merges and the hold is gone. Tests pin the invariants that otherwise fail silently. The sentinel carries no `ownerReferences`, so the lineage walk falls through to the Flux release label. Neither key-bearing Certificate stamps a tenant-visible label on its Secret. The two objects that decide which Secrets a tenant may actually read, the dashboard Role with its binding and the ResourceDefinition, are pinned whole rather than checked for known-bad names: a rule granting secrets with no `resourceNames` is unrestricted rather than narrow, and a label selector can reach a key-bearing Secret while every name in the file stays correct, so neither adds a name for a probe to notice. Part of #2814. ### Follow-ups What this PR does not do, and where each belongs instead. - nats converges the same contract in #3340 and has no `dashboard-resourcemap_test.yaml`, so the dashboard Role pin belongs there too. Tracked in #3701 along with postgres, which already carries the label selector with no guard behind it. - The qdrant e2e fixture runs plaintext (`hack/e2e-chainsaw/qdrant/qdrant.yaml` sets `external: false`), so nothing in CI walks the path from the sentinel to `tenantsecrets`. postgres has a `verify-tenant-ca-projection` step to copy. - `hack/check-qdrant-rd-secrets.bats` is engine-agnostic apart from two literals, the chart path and the expected Secret set. The third copy of it is the point to parameterise over `packages/system/*-rd/cozyrds/` instead of copying again. ### Downstream repositories Walked the trigger map in `docs/agents/contributing.md` against the diff, file by file. Nothing needs a manual follow-up, but not for the reason a values-only walk would give: the website reference page for an app is regenerated from that package's `README.md`, and this PR does change it. `qdrant` is in the app list hardcoded in the website `Makefile`, so the docs bot picks the new section up on the next stable tag and there is nothing to open by hand. The Terraform provider's schema is hand-written against `values.schema.json`, which is untouched, and the provider offers no typed access to the new projection, the same position it holds for every other Secret it does not model. No package is added, renamed or removed, and the `cozyrds` file is edited in place rather than renamed, so the ccp dependency-contract anchor still resolves. User-facing documentation for consuming the trust anchor ships with the chart README, matching #3340. - [x] No downstream repository is affected by this change - [ ] [cozystack/website](https://github.com/cozystack/website) - follow-up: - [ ] [cozystack/terraform-provider-cozystack](https://github.com/cozystack/terraform-provider-cozystack) - follow-up: - [ ] [cozystack/ansible-cozystack](https://github.com/cozystack/ansible-cozystack) - follow-up: - [ ] [cozystack/ccp](https://github.com/cozystack/ccp) - follow-up: - [ ] [cozystack/talm](https://github.com/cozystack/talm) - follow-up: - [ ] [cozystack/cozyhr](https://github.com/cozystack/cozyhr) - follow-up: - [ ] [cozystack/cozy-proxy](https://github.com/cozystack/cozy-proxy) - follow-up: - [ ] [cozystack/cozystack-telemetry-server](https://github.com/cozystack/cozystack-telemetry-server) - follow-up: - [ ] [cozystack/external-apps-example](https://github.com/cozystack/external-apps-example) - follow-up: - [ ] [cozystack/examples](https://github.com/cozystack/examples) - follow-up: ### Release note ```release-note feat(qdrant): expose the CA certificate to tenants as a key-free `<release>.tenant-ca` Secret ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Tenant environments can securely access the Qdrant TLS CA certificate without receiving the private key. * TLS CA access is automatically enabled when TLS or external access is configured. * **Documentation** * Expanded TLS guidance explains certificate validation and provides commands for retrieving the tenant CA certificate. * **Tests** * Added coverage for TLS projections, secret visibility, dashboard resources, and secure fail-closed secret exposure. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
## What this PR does Converges nats onto the key-free CA trust-anchor contract. A TLS-enabled nats release renders a `TenantProjection` sentinel naming the chart's cert-manager CA Secret as its source, and the nats ResourceDefinition selects the resulting `<release>.tenant-ca` object by label. The projection is built by the CA extraction controller in `internal/controller/cacert`, which parses the source certificate and re-encodes it into a single `ca.crt` key, so a tenant that enables TLS gets something to verify the server against without ever getting a private key. No key-bearing Secret is granted to a tenant, by name or by label. `packages/apps/nats/templates/certmanager.yaml` is not touched. An earlier draft of this change stamped publish labels onto the CA `Certificate`'s `secretTemplate`; the controller that shipped consumes a sentinel object instead, so the chart now declares its source rather than marking it. The functional change is one new template and one `matchLabels` entry in the ResourceDefinition. The sentinel is gated on the chart's effective TLS state, which is tri-state: left unset it follows `external`. That gate is the single divergence from the postgres implementation of the same contract, and it exists because the nats CA is only issued while TLS resolves true. An ungated sentinel would sit `Ready=False/SourceNotFound` forever on every plaintext release. The CA extraction controller is already on main, so this is live on merge rather than dormant until a follow-up lands. The `do-not-merge/hold` label predates that and no longer guards anything. ## Verified against a running cluster The premise this rests on is that cert-manager populates `ca.crt` on the Secret of a `selfSigned`, `isCA: true` Certificate. Nothing in this repository witnesses it: `hack/e2e-chainsaw/` has no nats suite, and nats appears in no e2e workflow, so every unit test here encodes the same assumption the chart encodes and the two cannot catch each other being wrong. I closed it against a live cluster instead, running cert-manager v1.20.2, the version this repo ships, on two independent nats releases: - `nats-<name>-ca`, the chart's CA Secret, holds `ca.crt`, `tls.crt` and `tls.key`. The declared source key exists. - `nats-<name>.tenant-ca`, the projection, holds exactly one key, `ca.crt`. - The projection's SHA-256 certificate fingerprint equals the source's, so it carries that CA and not some other certificate. - The projection carries `internal.cozystack.io/tenant-ca: "true"` and `internal.cozystack.io/tenantresource: "true"`. The second is stamped by the lineage webhook, computed from the ResourceDefinition selector this PR adds, so its presence means the whole chain held. - `kubectl get tenantsecret nats-<name>-ca` returns NotFound. The key-bearing Secret is not served through the tenant API. What is still missing is an automated regression witness for that premise. Adding one means a new chainsaw suite for nats, which has no e2e coverage of any kind today, and that is a larger change than this one. ## What the tests pin 61 helm-unittest cases and 3 bats cases. The sentinel's absence of `ownerReferences` (an owner would short-circuit the lineage walk before it reaches the Flux label the attribution depends on), the length of `spec.projections` as well as its single entry (the controller refuses a multi-entry sentinel, so a second entry is a silent loss of the anchor rather than a leak), both implied legs of the tri-state TLS gate, and `notExists` on either visibility label for both key-bearing Certificates. The bats guard covers the two grant surfaces nothing else asserts on. The ResourceDefinition is rendered, by the `nats-rd` chart inlining `cozyrds/*`, but that chart has neither a tests directory nor the `test` target `hack/helm-unit-tests.sh` looks for, and no `packages/system/*-rd` package does. The dashboard Role is rendered by a chart that has both, and no suite under `packages/apps/nats/tests/` asserts on it. Both are compared whole rather than filtered down to the entries that mention a Secret, because every such filter admits the spelling it did not anticipate. A name whitelist passes an added label selector, and `controller.cert-manager.io/fao` is stamped on both key-bearing Secrets, so a selector on it is a one-line grant of the CA private key. On the Role side, a rule with no `resourceNames` names no Secret, a rule with `resources: ["*"]` names no resource, and a ClusterRole is not a Role, while all three grant read access to the CA key. Comparing the rendered output whole needs no filter and leaves no spelling to find. ## Downstream repositories Walked the trigger map in `docs/agents/contributing.md` against the diff, file by file. No trigger fires, and no follow-up PR is owed. `packages/apps/nats/templates/tenant-projection.yaml` is a new template and `packages/apps/nats/tests/` holds chart tests. No package is added, renamed or removed; `values.yaml` and `values.schema.json` are untouched, so neither the hand-written Terraform schema, its version enums, nor its restated defaults have anything to track. `kind`, `plural` and `release.prefix` are unchanged, and no output Secret or Service is renamed, so the provider's concatenated name lookups still resolve. `packages/system/nats-rd/cozyrds/nats.yaml` is edited in place rather than renamed, so the ccp external-app skill's dependency-contract anchor still resolves. The provider offers no typed access to the new projection, which is the position it already holds for every Secret it does not model, and not a break. `packages/apps/nats/README.md` is covered by the one automated path that does exist: `nats` is present in the website `Makefile` `APPS` list, so the docs bot regenerates the managed-apps reference page from this `README.md` on the next stable tag. This is the silent-skip failure mode the trigger map warns about, so I checked the list rather than assuming; an app missing from it gets no page at all, which is what happened to `opensearch`. `hack/check-nats-rd-secrets.bats` is a new file, not a move or a rename, and it changes no make target's contract: `Makefile` globs `hack/*.bats` into `BATS_UNIT_FILES` automatically. The ccp skills anchor on `hack/package.mk` and `hack/common-envs.mk`, neither of which is touched. - [x] No downstream repository is affected by this change - [ ] [cozystack/website](https://github.com/cozystack/website) - follow-up: - [ ] [cozystack/terraform-provider-cozystack](https://github.com/cozystack/terraform-provider-cozystack) - follow-up: - [ ] [cozystack/ansible-cozystack](https://github.com/cozystack/ansible-cozystack) - follow-up: - [ ] [cozystack/ccp](https://github.com/cozystack/ccp) - follow-up: - [ ] [cozystack/talm](https://github.com/cozystack/talm) - follow-up: - [ ] [cozystack/cozyhr](https://github.com/cozystack/cozyhr) - follow-up: - [ ] [cozystack/cozy-proxy](https://github.com/cozystack/cozy-proxy) - follow-up: - [ ] [cozystack/cozystack-telemetry-server](https://github.com/cozystack/cozystack-telemetry-server) - follow-up: - [ ] [cozystack/external-apps-example](https://github.com/cozystack/external-apps-example) - follow-up: - [ ] [cozystack/examples](https://github.com/cozystack/examples) - follow-up: Part of #2814. ### Release note ```release-note feat(nats): expose the CA certificate to tenants as a key-free `<release>.tenant-ca` Secret ```
3 participants
What this PR does
Converges nats onto the key-free CA trust-anchor contract. A TLS-enabled nats release renders a
TenantProjectionsentinel naming the chart's cert-manager CA Secret as its source, and the nats ResourceDefinition selects the resulting<release>.tenant-caobject by label. The projection is built by the CA extraction controller ininternal/controller/cacert, which parses the source certificate and re-encodes it into a singleca.crtkey, so a tenant that enables TLS gets something to verify the server against without ever getting a private key. No key-bearing Secret is granted to a tenant, by name or by label.packages/apps/nats/templates/certmanager.yamlis not touched. An earlier draft of this change stamped publish labels onto the CACertificate'ssecretTemplate; the controller that shipped consumes a sentinel object instead, so the chart now declares its source rather than marking it. The functional change is one new template and onematchLabelsentry in the ResourceDefinition.The sentinel is gated on the chart's effective TLS state, which is tri-state: left unset it follows
external. That gate is the single divergence from the postgres implementation of the same contract, and it exists because the nats CA is only issued while TLS resolves true. An ungated sentinel would sitReady=False/SourceNotFoundforever on every plaintext release.The CA extraction controller is already on main, so this is live on merge rather than dormant until a follow-up lands. The
do-not-merge/holdlabel predates that and no longer guards anything.Verified against a running cluster
The premise this rests on is that cert-manager populates
ca.crton the Secret of aselfSigned,isCA: trueCertificate. Nothing in this repository witnesses it:hack/e2e-chainsaw/has no nats suite, and nats appears in no e2e workflow, so every unit test here encodes the same assumption the chart encodes and the two cannot catch each other being wrong. I closed it against a live cluster instead, running cert-manager v1.20.2, the version this repo ships, on two independent nats releases:nats-<name>-ca, the chart's CA Secret, holdsca.crt,tls.crtandtls.key. The declared source key exists.nats-<name>.tenant-ca, the projection, holds exactly one key,ca.crt.internal.cozystack.io/tenant-ca: "true"andinternal.cozystack.io/tenantresource: "true". The second is stamped by the lineage webhook, computed from the ResourceDefinition selector this PR adds, so its presence means the whole chain held.kubectl get tenantsecret nats-<name>-careturns NotFound. The key-bearing Secret is not served through the tenant API.What is still missing is an automated regression witness for that premise. Adding one means a new chainsaw suite for nats, which has no e2e coverage of any kind today, and that is a larger change than this one.
What the tests pin
61 helm-unittest cases and 3 bats cases. The sentinel's absence of
ownerReferences(an owner would short-circuit the lineage walk before it reaches the Flux label the attribution depends on), the length ofspec.projectionsas well as its single entry (the controller refuses a multi-entry sentinel, so a second entry is a silent loss of the anchor rather than a leak), both implied legs of the tri-state TLS gate, andnotExistson either visibility label for both key-bearing Certificates.The bats guard covers the two grant surfaces nothing else asserts on. The ResourceDefinition is rendered, by the
nats-rdchart inliningcozyrds/*, but that chart has neither a tests directory nor thetesttargethack/helm-unit-tests.shlooks for, and nopackages/system/*-rdpackage does. The dashboard Role is rendered by a chart that has both, and no suite underpackages/apps/nats/tests/asserts on it.Both are compared whole rather than filtered down to the entries that mention a Secret, because every such filter admits the spelling it did not anticipate. A name whitelist passes an added label selector, and
controller.cert-manager.io/faois stamped on both key-bearing Secrets, so a selector on it is a one-line grant of the CA private key. On the Role side, a rule with noresourceNamesnames no Secret, a rule withresources: ["*"]names no resource, and a ClusterRole is not a Role, while all three grant read access to the CA key. Comparing the rendered output whole needs no filter and leaves no spelling to find.Downstream repositories
Walked the trigger map in
docs/agents/contributing.mdagainst the diff, file by file. No trigger fires, and no follow-up PR is owed.packages/apps/nats/templates/tenant-projection.yamlis a new template andpackages/apps/nats/tests/holds chart tests. No package is added, renamed or removed;values.yamlandvalues.schema.jsonare untouched, so neither the hand-written Terraform schema, its version enums, nor its restated defaults have anything to track.kind,pluralandrelease.prefixare unchanged, and no output Secret or Service is renamed, so the provider's concatenated name lookups still resolve.packages/system/nats-rd/cozyrds/nats.yamlis edited in place rather than renamed, so the ccp external-app skill's dependency-contract anchor still resolves. The provider offers no typed access to the new projection, which is the position it already holds for every Secret it does not model, and not a break.packages/apps/nats/README.mdis covered by the one automated path that does exist:natsis present in the websiteMakefileAPPSlist, so the docs bot regenerates the managed-apps reference page from thisREADME.mdon the next stable tag. This is the silent-skip failure mode the trigger map warns about, so I checked the list rather than assuming; an app missing from it gets no page at all, which is what happened toopensearch.hack/check-nats-rd-secrets.batsis a new file, not a move or a rename, and it changes no make target's contract:Makefileglobshack/*.batsintoBATS_UNIT_FILESautomatically. The ccp skills anchor onhack/package.mkandhack/common-envs.mk, neither of which is touched.Part of #2814.
Release note