docs(unified-tls-pki): fix the canonical trust-anchor name and its owner#36
Open
Aleksei Sviridkin (lexfrei) wants to merge 3 commits into
Open
docs(unified-tls-pki): fix the canonical trust-anchor name and its owner#36Aleksei Sviridkin (lexfrei) wants to merge 3 commits into
Aleksei Sviridkin (lexfrei) wants to merge 3 commits into
Conversation
📝 WalkthroughWalkthroughThe proposals standardize the tenant trust-anchor Secret as ChangesTenant trust-anchor contract
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![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 updates the design proposals for external database exposure and unified TLS PKI. It renames the canonical trust-anchor Secret from <release>-ca-cert to <release>-tenant-ca to prevent naming collisions with existing operator-created Secrets (such as Percona PSMDB). Additionally, it updates the extraction controller's design to owner-reference the HelmRelease instead of the virtual application instance CR to ensure proper garbage collection. The reviewer feedback points out that the external database exposure proposal still contains outdated text claiming redis self-publishes its CA directly, whereas under the updated design, all engines (including redis and kafka) utilize the extraction controller to project their CAs to the canonical <release>-tenant-ca Secret.
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.
| ## Scope and related proposals | ||
|
|
||
| - **Depends on:** `design-proposals/unified-tls-pki` — provides the `<release>-ca-cert` key-free trust anchor that external clients use to verify the endpoint. This proposal does not re-specify it. It is a companion submission under the same epic, on its own branch; the path resolves once both proposals merge. The dependency is **per-engine, not blanket**: an engine is exposable here only once its `ca.crt` is actually delivered under that contract, and the path differs by engine — redis self-publishes a key-free `<release>-ca-cert` through its forked operator, while postgres and mongodb obtain theirs through the extraction controller in `unified-tls-pki`. So SNI exposure for a given engine is gated on that engine's `unified-tls-pki` convergence, not merely on the contract existing. | ||
| - **Depends on:** `design-proposals/unified-tls-pki` — provides the `<release>-tenant-ca` key-free trust anchor that external clients use to verify the endpoint. This proposal does not re-specify it. It is a companion submission under the same epic, on its own branch; the path resolves once both proposals merge. The dependency is **per-engine, not blanket**: an engine is exposable here only once its `ca.crt` is actually delivered under that contract, and the path differs by engine — redis self-publishes a key-free `<release>-ca-cert` through its forked operator, while postgres and mongodb obtain theirs through the extraction controller in `unified-tls-pki`. So SNI exposure for a given engine is gated on that engine's `unified-tls-pki` convergence, not merely on the contract existing. |
There was a problem hiding this comment.
The text still states that redis self-publishes a key-free <release>-ca-cert through its forked operator, while postgres and mongodb obtain theirs through the extraction controller.
However, under the updated design, redis and kafka also require the extraction controller to project their engine-specific CA Secrets to the canonical <release>-tenant-ca Secret. This sentence should be updated to reflect that all engines now utilize the extraction controller/projection mechanism to deliver the canonical trust anchor.
For example:
- **Depends on:** `design-proposals/unified-tls-pki` — provides the `<release>-tenant-ca` key-free trust anchor that external clients use to verify the endpoint. This proposal does not re-specify it. It is a companion submission under the same epic, on its own branch; the path resolves once both proposals merge. The dependency is **per-engine, not blanket**: an engine is exposable here only once its `ca.crt` is actually delivered under that contract, and the path differs by engine — while redis self-publishes a key-free `<release>-ca-cert` and postgres/mongodb have key-bearing CAs, all of them now obtain their canonical `<release>-tenant-ca` through the extraction controller in `unified-tls-pki`. So SNI exposure for a given engine is gated on that engine's `unified-tls-pki` convergence, not merely on the contract existing.
Aleksei Sviridkin (lexfrei)
marked this pull request as ready for review
July 14, 2026 16:24
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@design-proposals/external-database-exposure/README.md`:
- Line 139: Update the diagram’s trust-anchor Secret reference near the external
database exposure flow to use <release>.tenant-ca instead of <release>-ca-cert,
matching the trust-anchor name defined in the surrounding proposal and keeping
the ca.crt-only contract unchanged.
In `@design-proposals/unified-tls-pki/README.md`:
- Around line 156-167: Define one canonical delivery path: require every
engine-specific CA Secret, including self-publishing Redis and similar sources,
to be projected into <release>.tenant-ca by the extraction controller rather
than writing that canonical object directly. Update
design-proposals/unified-tls-pki/README.md at lines 156-167 to make this
mandatory and consistent, and update
design-proposals/external-database-exposure/README.md at line 19 to describe
Redis’s <release>-ca-cert strictly as an internal source.
- Around line 156-167: Clarify the design so every engine has one mandatory path
for producing <release>.tenant-ca. Either require the extraction controller to
project the canonical Secret for all engines, or explicitly document each
self-publishing engine's equivalent canonical-object creation and remove any
conflicting opt-out language. Update the descriptions in the
extraction-controller and reuse sections consistently.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ff0f6c24-ea7c-4bf2-a5a6-98138745a2e1
📒 Files selected for processing (2)
design-proposals/external-database-exposure/README.mddesign-proposals/unified-tls-pki/README.md
IvanHunters
reviewed
Jul 18, 2026
IvanHunters
left a comment
IvanHunters
left a comment
Contributor
There was a problem hiding this comment.
Reviewed as a docs pass. The rename and the ownership correction are applied only in the definitional sections, so the document currently contradicts itself on both points it sets out to fix. Suggest holding until the references are reconciled.
Stale/contradictory references (design-proposals/unified-tls-pki/README.md unless noted):
- L175 still says the one canonical trust-anchor object a tenant sees is
<release>-ca-cert, contradicting the new definition at L137 (<release>.tenant-ca). - L197 re-asserts both errors this PR fixes: the stale name, and garbage-collection via an owner reference to the application instance, contradicting rewritten section (b) which shows the owner-ref must be the HelmRelease.
- L99 (interior-tier diagram node, ca.crt-only) and external-database-exposure/README.md L110 (diagram node projected via tenantsecrets) still use the stale canonical name.
- L171, L179, L203, L228 still reference the projected/canonical object by the old name.
Note: occurrences at L41, L43, L121-124, L156, L160, L167, L214, L226 correctly keep -ca-cert because they name engine-specific source secrets or describe the old-name collision; those should not change.
Minor: external-database-exposure/README.md L19 says redis self-publishes while unified-tls-pki/README.md L167 says redis still needs projection; the two docs disagree.
Implementation surfaced two errors in the accepted design. The canonical projection cannot be named <release>-ca-cert. Percona PSMDB creates a Secret of exactly that name and puts a private key in it, so source and target collide on the very engine that most needs the projection; Strimzi publishes its key-free CA under a different name again. The document itself warned three times that this name means opposite things on different engines, used that to reject name-based source selection, and then picked it for the target anyway. The canonical object is now <release>-tenant-ca, which no shipped operator claims. The projection also cannot be owner-referenced to the application CR. The apps.cozystack.io kinds are virtual: an application is stored as a HelmRelease, so there is no object in etcd to reference. The HelmRelease is the only real owner, and deleting the application deletes it, which garbage-collects the projection. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la>
Implementation showed the source-selection label is not enough on its own. The platform runs cert-manager with enableCertificateOwnerRef set to false, so a cert-manager-issued Secret carries no ownerReference back to its application and the lineage graph cannot tell which release it belongs to. The consume contract therefore requires a second label, internal.cozystack.io/publish-ca-cert-release, carrying the release name, stamped alongside the opt-in label in the same secretTemplate block. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la>
…truction The trust anchor was named <release>-tenant-ca on the grounds that no operator the platform ships claims it. That is false. It collides across releases: for an application foo the projection is postgres-foo-tenant-ca, and for a sibling application foo-tenant CloudNativePG's own CA secret is postgres-foo-tenant plus -ca, which is the same string. Both names are legal and several Postgres instances in one namespace is ordinary. The collision has a direction no guard can refuse. When the projection is written first, CloudNativePG rejects the key-free secret with 'missing ca.key secret data' and the sibling's PKI never completes, so that application never starts, blaming a secret its owner never created. The controller that wrote first was within its rights, so there is nothing to refuse; and before this contract existed both applications worked. The name is now <release>.tenant-ca, which is disjoint by character class rather than by survey: application names are DNS-1035 labels and cannot contain a dot, release prefixes are dot-free, and secret names are DNS-1123 subdomains where a dot is legal. No prefix, application name and operator suffix can therefore produce a dotted name, for any operator, now or later. This is the second time the survey argument failed — the previous name was rejected because Percona claims it. Recording the reasoning so the third name is not chosen the same way. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la>
Aleksei Sviridkin (lexfrei)
force-pushed
the
fix/tls-pki-canonical-name-and-ownerref
branch
from
July 20, 2026 02:49
bbbbfce to
1366c24
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@design-proposals/unified-tls-pki/README.md`:
- Line 160: Update the source-value validation contract to strictly parse every
PEM value with x509.ParseCertificate, rejecting non-certificate PEM blocks,
malformed input, and any trailing data instead of relying on PRIVATE KEY header
detection. Extend the controller tests covering the source selection and
certificate validation flow with malformed and trailing-data cases.
- Line 215: The proposal inconsistently treats publish-ca-cert and
publish-ca-cert-release as both required contract fields and unresolved names.
Update the documentation around the label/annotation convention and the
definitions at the corresponding contract section to either finalize these exact
names consistently or mark them as placeholders throughout, ensuring source
producers and the controller use the same contract.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 680ecd39-8923-4eb8-8ab7-9bba409a51ad
📒 Files selected for processing (2)
design-proposals/external-database-exposure/README.mddesign-proposals/unified-tls-pki/README.md
🚧 Files skipped from review as they are similar to previous changes (1)
- design-proposals/external-database-exposure/README.md
| **(b) A small extraction controller.** Its watch/upsert skeleton can follow the wildcard-secret reconciler from PR `cozystack/cozystack#2990` (`internal/controller/wildcardsecret/reconciler.go`), but it carries none of that reconciler's copy-marking or prune logic — lineage provides those (part (c)) — and, being **intra-namespace**, it can do something that reconciler cannot. For each label-selected source Secret it upserts a `type: Opaque` Secret named `<release>-ca-cert` containing **only** `ca.crt`, re-copying on every source change so a CA rotation propagates without a chart re-render. It does four security-load-bearing things and nothing more: | ||
| **(b) A small extraction controller.** Its watch/upsert skeleton can follow the wildcard-secret reconciler from PR `cozystack/cozystack#2990` (`internal/controller/wildcardsecret/reconciler.go`), but it carries none of that reconciler's copy-marking or prune logic — lineage provides those (part (c)) — and, being **intra-namespace**, it can do something that reconciler cannot. For each selected source Secret it upserts a `type: Opaque` Secret named `<release>.tenant-ca` containing **only** `ca.crt`, re-copying on every source change so a CA rotation propagates without a chart re-render. It does four security-load-bearing things and nothing more: | ||
|
|
||
| - **Key off the label and the content, never the name.** Select sources by the `publish-ca-cert` label; before writing, verify the lifted value is a CA certificate and carries no `-----BEGIN … PRIVATE KEY-----` header. This is what lets one controller serve a key-free redis source it should ignore and a key-bearing PSMDB source it must strip, both named `<release>-ca-cert`-adjacent, without confusion. |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Require strict PEM parsing for every source value.
The contract still relies on detecting PRIVATE KEY headers. Instead, decode every PEM block with x509.ParseCertificate, reject non-certificate blocks and any trailing data, and add malformed/trailing-data cases to the controller tests.
Also applies to: 192-193, 202-202
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@design-proposals/unified-tls-pki/README.md` at line 160, Update the
source-value validation contract to strictly parse every PEM value with
x509.ParseCertificate, rejecting non-certificate PEM blocks, malformed input,
and any trailing data instead of relying on PRIVATE KEY header detection. Extend
the controller tests covering the source selection and certificate validation
flow with malformed and trailing-data cases.
|
|
||
| - **Labelling operator-created sources.** The cert-manager charts stamp the source label via `Certificate.spec.secretTemplate.labels`. Operators that create the CA Secret themselves (CNPG `<release>-ca`, PSMDB `<release>-ca-cert`) may not let the chart label their output. For those, the controller needs either an operator that supports output labels or a small per-engine source-name configuration as a fallback — this is the one place the engine-agnostic label is not yet sufficient, and it should be resolved before postgres/mongodb convergence. | ||
| - The exact namespace convention for the `<release>-ca-cert` object (per-release in the app namespace is assumed here) and the final label/annotation names. This proposal uses `internal.cozystack.io/publish-ca-cert` on the source and `internal.cozystack.io/tenant-ca` on the projected copy, matching the `internal.cozystack.io/` convention of the existing `tenantresource` and `managed-by-cozystack` markers. | ||
| - The exact namespace convention for the `<release>.tenant-ca` object (per-release in the app namespace is assumed here) and the final label/annotation names. This proposal uses `internal.cozystack.io/publish-ca-cert` and `internal.cozystack.io/publish-ca-cert-release` on the source and `internal.cozystack.io/tenant-ca` on the projected copy, matching the `internal.cozystack.io/` convention of the existing `tenantresource` and `managed-by-cozystack` markers. |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Close the label-name contract before implementation.
Line 156 defines publish-ca-cert and publish-ca-cert-release as required contract fields, but Line 215 leaves their final names unresolved. Either finalize these exact names or mark them as placeholders consistently; otherwise source producers and the controller can ship incompatible labels.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@design-proposals/unified-tls-pki/README.md` at line 215, The proposal
inconsistently treats publish-ca-cert and publish-ca-cert-release as both
required contract fields and unresolved names. Update the documentation around
the label/annotation convention and the definitions at the corresponding
contract section to either finalize these exact names consistently or mark them
as placeholders throughout, ensuring source producers and the controller use the
same contract.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Implementing the accepted design surfaced four errors in it. All are corrected here, across both proposals. Every one was found by building the thing, not by reading it.
The canonical trust anchor is now
<release>.tenant-ca, and the reason it took three names is the point.<release>-ca-certwas rejected because Percona PSMDB creates a Secret of exactly that name and puts a private key in it — a collision across engines. Its replacement<release>-tenant-cawas justified as "claimed by no operator this platform ships". That was also false, one level up: it collides across releases. For an applicationfoothe projection ispostgres-foo-tenant-ca; for a sibling applicationfoo-tenant, CloudNativePG's own CA ispostgres-foo-tenant+-ca— the same string. Both names are legal and several Postgres instances per namespace is ordinary. One direction of that collision cannot be guarded at all: if the projection is written first, CloudNativePG rejects the key-free Secret (missing ca.key secret data), the sibling's PKI never completes, and that application never starts — silently, blaming a Secret its owner never created. It is a regression, and there is nothing to refuse, because the controller that wrote first was legitimate.<release>.tenant-cais disjoint by character class instead: application names are DNS-1035 labels and structurally cannot contain a dot, release prefixes are dot-free, and Secret names are DNS-1123 subdomains where a dot is legal — so no prefix + application + operator suffix can ever produce a dotted name, for any operator, now or later. That is a proof rather than a survey, which is what the two previous names lacked.The projection cannot be owner-referenced to the application CR. The
apps.cozystack.iokinds are virtual: they are served by the aggregated apiserver, and an application is stored as a HelmRelease whosespec.valuesis the application spec verbatim. There is no application object in etcd and therefore no UID to reference. The HelmRelease is the only real object in the chain — and it is sufficient, because deleting the application through the aggregated API deletes the HelmRelease, which garbage-collects the projection with it.The source-selection label is not enough on its own. The platform runs cert-manager with
enableCertificateOwnerRef: false, so a cert-manager-issued Secret carries no ownerReference back to its application and the lineage graph cannot resolve which release it belongs to. The consume contract therefore requires a second label,internal.cozystack.io/publish-ca-cert-release, carrying the release name, stamped alongside the opt-in label in the samesecretTemplateblock.The trust-anchor guard must parse, not pattern-match. The proposal's fail-closed boundary was specified as a header check — reject anything carrying a private-key header, accept anything carrying a certificate header. That is not sufficient: PEM armour around arbitrary bytes satisfies it, and
pem.Decodedoes not close the gap either, since it validates the armour and the base64 rather than the contents. Onlyx509.ParseCertificateon every decoded block, with no trailing remainder, actually asserts "this is a certificate".All four were found by the implementation in cozystack/cozystack#3299.
Summary by CodeRabbit
<release>.tenant-caSecret name (replacing the prior<release>-ca-cert).ca.crtand uses the updated tenant-visible labeling.