feat(mariadb): add TLS support via cert-manager - #2680

Code Review

This pull request implements TLS support for MariaDB with tri-state logic, automatically enabling TLS for external access unless configured otherwise. It adds cert-manager templates, updates RBAC for TLS secrets, and includes unit tests. Feedback highlights a high-severity issue where incorrect SANs in the certificate template would cause internal TLS verification failures. Other suggestions include adding standard labels to cert-manager resources and explicitly defining default values in values.yaml to improve schema visibility.

Actionable comments posted: 2

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 `@packages/apps/mariadb/README.md`:
- Line 105: The README shows `version` under TLS because the `values.yaml` lacks
a section separator before the Version enum; open the MariaDB chart's
values.yaml and insert a new section marker (e.g., `@section` or the chart's
existing section delimiter style) immediately before the `version` parameter so
that `version` is moved out of the TLS section into its own section (or place it
into the "Application-specific parameters" section); this change to values.yaml
will let cozyvalues-gen regenerate the README with `version` shown in the
correct section.

In `@packages/system/mariadb-rd/cozyrds/mariadb.yaml`:
- Line 28: The keysOrder entry currently includes ["spec","tls"] but omits the
TLS child keys, causing unstable field ordering; update the keysOrder array (the
keysOrder symbol) to include ["spec","tls","enabled"] and
["spec","tls","required"] immediately after ["spec","tls"] so TLS child fields
render in the intended, stable order in the dashboard.

NOT LGTM — three security/correctness blockers: the chart's own legacy backup CronJob will start failing the moment TLS becomes default-on, the server certificate is missing digital signature (modern TLS clients reject it), and the dashboard use role is granted get/list/watch on the CA private key. The cert-manager chain itself is well-shaped (RSA-4096, sane lifetimes, rotation policy) and the SAN list matches the upstream operator's InternalServiceName (<name>-internal); below are the three items that need to land before merge.

Business context: First-class TLS for the managed MariaDB service via a self-contained cert-manager chain in the tenant namespace, with a tri-state tls.enabled that auto-on for externally-published instances.

Blockers

B1: tls.required: true default breaks chart's own backup CronJob

Files: packages/apps/mariadb/templates/mariadb.yaml:88-92 + packages/apps/mariadb/templates/backup-script.yaml:14,29

Issue: After this PR ships, any tenant with both external: true (or any explicit tls.enabled: true) and backup.enabled: true will lose backups silently on upgrade. The MariaDB CR's tls.required: true renders require_secure_transport=ON on mysqld, which rejects plaintext TCP connections. The chart-internal backup CronJob still runs mysql -u "$MYSQL_USER" -h "$MYSQL_HOST" and mysqldump ... -h "$MYSQL_HOST" with no --ssl-* flags or --ssl-mode=REQUIRED.

Evidence: backup-script.yaml:14,29 are unchanged plain-mysql clients; MYSQL_HOST resolves to <name>-primary or <name>-secondary (line 44 of backup-cronjob.yaml), which are TCP services. require_secure_transport rejects plain TCP regardless of credentials. Manifests helm template-rendered against external: true, backup.enabled: true produce a CronJob that will exit non-zero on every fire.

Impact: Silent backup regression on the subset (external-published + backup-enabled tenants) that is precisely the riskiest cohort. The PR description notes backup is deprecated but the chart still ships it; the new default cannot break it without an explicit upgrade note plus a fallback path.

Fix — pick one of:

• Mount <name>-ca-tls into the CronJob and pass --ssl-ca=/path/ca.crt --ssl-mode=VERIFY_CA (or VERIFY_IDENTITY) to both mysql and mysqldump.
• Gate the tls.required: true default on backup.enabled being false, with an explicit warning in the README that operators using the legacy backup must move to BackupClass before enabling TLS.
• Treat the legacy backup as fully retired and remove it in the same PR rather than half-supporting it.

B2: Server certificate missing digital signature key usage

File: packages/apps/mariadb/templates/certmanager.yaml:62-65

Issue: The leaf Certificate explicitly declares usages: [server auth, client auth, key encipherment]. Because the field is set explicitly, cert-manager will NOT apply its default key usages on top. digital signature is missing.

Evidence: For TLS 1.3 (and TLS 1.2 with any ECDHE/DHE cipher suite — which is the default since OpenSSL 1.1.0 and Go's crypto/tls), the server signs the ephemeral key-exchange parameters with the cert private key. RFC 5246 §7.4.1.4.1 and RFC 8446 §4.4 require the cert to assert digitalSignature for that operation when keyUsage is present and critical: true. cert-manager's standard server profile is digital signature, key encipherment, server auth; the omission here will be rejected by strict clients (Go's tls.Config{InsecureSkipVerify:false}, recent OpenJDK with strict KU checks, BoringSSL). key encipherment only covers the legacy RSA key-transport suites, which TLS 1.3 doesn't have at all.

Impact: TLS handshake failures from modern, RFC-conformant clients once they connect to a TLS-enabled MariaDB. The bug is silent on the chart side — cert-manager happily issues a cert with the wrong usages and mysqld serves it.

Fix: Add digital signature to the usages list in certmanager.yaml:62-65.

B3: Dashboard RBAC leaks the CA private key and the server private key to every tenant user

File: packages/apps/mariadb/templates/dashboard-resourcemap.yaml:21-23

Issue: When TLS is enabled, the {{ .Release.Name }}-dashboard-resources Role grants get/list/watch on Secrets <name>-ca-tls and <name>-tls. The RoleBinding subjects (line 39, via cozy-lib.rbac.subjectsForTenantAndAccessLevel "use") cover every group with use access or higher in the tenant — i.e. every tenant member. cert-manager Secrets of kubernetes.io/tls type carry tls.key alongside tls.crt and ca.crt. The Role lets any tenant member run kubectl -n <ns> get secret <name>-ca-tls -o yaml and recover both the CA private key and the server private key.

Evidence: cert-manager produces standard tls-type Secrets containing both tls.key and tls.crt; for the CA Certificate (certmanager.yaml:16-37), tls.key is the CA's private key. Kubernetes RBAC at the Secret level cannot scope to individual keys — any verb on the secret exposes every field. The cozy-lib.rbac.accessLevelMap defines use as level 1 (just above view), so the binding extends to tenant-<X>-use, tenant-<X>-admin, tenant-<X>-super-admin groups.

Impact: A leaked CA private key allows minting an arbitrarily-named server cert that any client trusting this CA (notably MariaDB clients pinned via --ssl-ca) will accept — instant MITM of every MariaDB connection in the tenant. A leaked server private key allows passive decryption of TLS sessions captured anywhere on the network path (for non-PFS suites) and direct impersonation of the MariaDB server.

Fix: Drop both Secret names from the dashboard Role. If the dashboard genuinely needs to display the trust bundle (ca.crt), publish that single field to a separate ConfigMap (e.g. via a small helper template that reads the Secret in a post-install Job and writes the CA cert into a ConfigMap, or via additionalOutputFormats on the Certificate object) and grant get on that ConfigMap instead. Under no circumstance should tls.key reach a use-level subject.

Non-blocking follow-ups

1. dashboard-resourcemap.yaml exposes the TLS secrets in RBAC, but packages/system/mariadb-rd/cozyrds/mariadb.yaml doesn't list them under secrets.include — the two are out of sync. Once B3 is resolved by replacing the secret with a ConfigMap, cozyrds/mariadb.yaml is the place to add it under configMaps.include (or remove the dashboard reference entirely).

2. certmanager.yaml:35-37: CA usages [signing, cert sign] cover the issuance path. Adding crl sign is conventional (even if no CRL is published today) and costs nothing.

3. The SAN list (certmanager.yaml:69-78) covers the three FQDN forms per service plus the wildcard pod-FQDN for -internal. Upstream operator docs list a few extra short forms (<name>.<ns> without .svc, *.<name>-internal without <ns>.svc). Not needed in practice unless clients resolve via ndots-shortened forms, but worth adding for full alignment with upstream's documented set.

If the same certmanager.yaml pattern (B2) and dashboard RBAC pattern (B3) repeat across the sister TLS PRs (#2681 Kafka, #2682 OpenSearch, #2683 RabbitMQ, #2684 NATS, #2685 Qdrant), the fixes should land in lockstep — those PRs will likely surface the same two blockers once reviewed.

NOT LGTM — re-review on head 7c51f41a. Two blockers remain. One of them reopens a point I previously marked resolved: the TLS backup fix does not work against the client the backup image actually ships.

Business context: first-class TLS for the managed MariaDB service via a self-contained cert-manager chain (SelfSigned → CA → leaf) in the tenant namespace, with a tri-state tls.enabled that auto-enables for externally-published instances.

Blockers

B1 (reopened): the TLS backup path uses a client flag the backup image does not support
File: packages/apps/mariadb/templates/backup-script.yaml:15
Issue: with TLS on, the script sets SSL_OPTS="--ssl-ca=/etc/mysql-ssl/ca.crt --ssl-mode=VERIFY_IDENTITY" and passes it to mysql/mysqldump. --ssl-mode is an Oracle MySQL-8 client flag; the backup image (images/mariadb-backup/Dockerfile: alpine:3.20 + apk add mariadb-client) ships MariaDB's own client, which has no such option.
Evidence: running the image base — mysqldump --ssl-mode=VERIFY_IDENTITY on mariadb-client 10.11 → mysqldump: unknown variable 'ssl-mode=VERIFY_IDENTITY'. The client fatal-errors on the unknown variable before connecting, so the DB_LIST=$(mysql ... $SSL_OPTS ...) discovery and every subsequent mysqldump fail. The helm-unittest suite (tests/tls_test.yaml:859-872) only asserts the rendered script contains the string --ssl-mode=VERIFY_IDENTITY, so it is green while the runtime is broken. My earlier acceptance of this fix was wrong — the flag as written cannot work against this client.
Impact: every TLS-enabled backup job fails before dumping a single database — silent backup loss for exactly the external-published + backup-enabled cohort (where tls.required: true auto-applies).
Fix: use the MariaDB-client equivalent: SSL_OPTS="--ssl-ca=/etc/mysql-ssl/ca.crt --ssl-verify-server-cert" (--ssl-ca implies --ssl; --ssl-verify-server-cert adds the hostname check that VERIFY_IDENTITY provides). Update the test to assert the corrected flag.

B2: dashboard RBAC grants the CA and server private keys to every tenant use member
File: packages/apps/mariadb/templates/dashboard-resourcemap.yaml:22-23
Issue: with TLS on, the -dashboard-resources Role grants get/list/watch on Secrets <release>-ca-tls and <release>-tls, bound (line 39) to cozy-lib.rbac.subjectsForTenantAndAccessLevel "use" — every tenant member at use or above. Both are cert-manager kubernetes.io/tls Secrets carrying tls.key: -ca-tls is the CA secret (isCA: true) so its tls.key is the CA private key; -tls is the leaf with the server private key.
Evidence: certmanager.yaml CA secret (isCA: true) and leaf secret; Role rule at dashboard-resourcemap.yaml:18-25; RoleBinding at :39. RBAC is per-Secret and cannot scope to individual keys, so get exposes tls.key.
Impact: any tenant member can read the CA private key and mint certs the whole --ssl-ca trust chain accepts (MITM every MariaDB client), or read the server key to impersonate the server / decrypt non-PFS sessions.
Fix: remove both -ca-tls and -tls from the Role. If tenants need the trust bundle, publish only ca.crt to a separate ConfigMap (e.g. via trust-manager) or a ca.crt-only Secret, and grant read on that. tls.key must never reach a use-level subject. This is the same defect as the sibling rabbitmq/mongodb TLS PRs — a single shared fix (ca.crt-only trust anchor) would close it across the series.

Non-blocking follow-ups

1. cozyrds/mariadb.yaml keysOrder lists ["spec","tls","required"] but not ["spec","tls","enabled"], so the dashboard can't toggle tls.enabled. This is a faithful regeneration (the field is a nullable *bool with no default) and matches the rest of the TLS series — a cross-PR follow-up if dashboard toggling is intended, not a mariadb-specific blocker.
2. Inter-node and SAN coverage is thorough and matches the operator's -internal headless service; no SAN gap once B1's flag is corrected.

Verdict

LGTM with non-blocking notes

Reviewed at b24ed36f0 against merge-base 3a70f9142. The base-to-head render diff is purely additive on every corner I rendered. The emitted CR shape is accepted by mariadb-operator 25.10.2's validating webhook, and the CA bundle the tenant anchor is lifted from does pick up the chart's CA. That leaves three MINOR craft items and one uncovered surface: no test starts an instance on the cert-manager path.

Findings

[MINOR] packages/apps/mariadb/templates/mariadb.yaml:125, multi-paragraph # prose ships into live objects

Six touched templates gain narrative # comments (mariadb.yaml 41 lines, certmanager.yaml 72, cleanup-pvc.yaml 29, dashboard-resourcemap.yaml 15, backup-cronjob.yaml 16, backup-script.yaml 16). Unlike {{/* */}}, these are copied verbatim into rendered manifests, so they land in the MariaDB CR, the dashboard Role and the cleanup Job in every customer cluster, and in the stored Helm release manifest. They also rot independently of the code beside them. Keep a one-line pointer and move the reasoning to the docs site.

$ grep -cE '^[[:space:]]*#' rendered-base/default.yaml rendered/head-default.yaml
rendered-base/default.yaml: 59
rendered/head-default.yaml: 99
$ grep -cE '^[[:space:]]*#' rendered/issuer-cm.yaml
175

[MINOR] packages/apps/mariadb/values.yaml:75, external's description does not mention its new certificate role

external now feeds the leaf certificate SAN list (certmanager.yaml:152-155) on top of switching the Service to LoadBalancer. Its @param text is unchanged, and cozyvalues-gen copies that text verbatim into values.schema.json, the README parameter table and the -rd openAPISchema the dashboard renders. An operator reading only the parameter table learns that external: true exposes the instance. The table does not tell them that under the default tls.issuer: operator the certificate cannot carry the external hostname, so --ssl-verify-server-cert from outside fails no matter which CA is trusted. The README says this; the field description, which is what the dashboard shows, does not.

[MINOR] packages/system/mariadb-rd/cozyrds/mariadb.yaml:75, the exclude backstop misses one enumerable operator credential

The comment above the list calls it the enumerable dangerous set. Against the operator's own key table at the pinned version, one credential Secret name is absent: mariadb-<name>-agent-auth, the generated basic-auth password for the Galera agent sidecar (api/v1alpha1/mariadb_keys.go:361, mounted at /var/run/secrets/mariadb-operator/agent). It is latent, not live. This chart never sets spec.galera (templates/mariadb.yaml emits replication only), so the Secret is never created today. <release>-backup-script is also outside the list; it holds the script and no credentials, so it is only a completeness point. Adding agent-auth now costs a line and removes the coupling between the backstop and a spec field someone may enable later.

$ yq eval-all 'select(.kind=="MariaDB") | .spec | keys' rendered/default.yaml | grep -c galera
0

Claim mismatches

[PARTIAL] "Merge is blocked on #3299, the CRD on main does not carry spec.caCert". Stale as of this revision. grep -rn caCert over the head tree returns only internal/controller/cacert internals; the branch emits a TenantProjection sentinel instead, and packages/system/cozystack-controller/definitions/internal.cozystack.io_tenantprojections.yaml already exists at the merge base (git cat-file -e succeeds there). Nothing in the diff needs a CRD field that main lacks.

[PARTIAL] "The tenant no longer receives <release>-ca-tls or <release>-tls". Against main those Secrets do not exist and were never granted: the base cozyrd is exclude: [] with include: [mariadb-{{ .name }}-credentials]. The sentence describes a delta against an earlier revision of this branch.

[PARTIAL] The "Fixes along the way" section (the --ssl-mode=VERIFY_IDENTITY flag, the leaf clientAuth usage) reads as fixes to shipped behaviour. Both are intra-branch: the base backup-script.yaml passes no SSL options at all, and no leaf certificate exists on main. Worth rewording so a reviewer diffing against main is not looking for code that was never there.

Caveats

• The tls.issuer: cert-manager path has no live coverage, which the PR body states. I closed as much of it statically as the source allows. The webhook accepts a CR carrying both refs (internal/webhook/v1alpha1/webhook.go:26-49, the reject fires only when caSecretRef == nil && certSecretRef != nil). The operator skips issuance entirely when both shouldIssueCA and shouldIssueCert are false and certIssuerRef is nil (pkg/controller/certificate/controller.go:67-91), so it never contends with cert-manager for those Secrets. And TLSServerCASecretKey() returns ServerCASecretRef.Name when set (api/v1alpha1/mariadb_keys.go:92-107), so reconcileTLSCABundle folds the chart CA into <release>-ca-bundle and the tenant anchor does verify the served certificate. None of that proves an instance reaches Ready and completes a handshake. That is the check to run on a disposable cluster before the path is advertised.
• The SAN list is hand-maintained and currently exact. I compared it against TLSServerDNSNames() at 25.10.2: ServiceNameVariants yields the same four depths for <name>, -primary and -secondary, HeadlessServiceNameVariants(meta, "*", "<name>-internal") the same four for the wildcard, plus localhost. The chart's cozy.local fallback matches both networking.clusterDomain (packages/core/platform/values.yaml:62) and the operator's clusterName (packages/system/mariadb-operator/values.yaml). An operator bump can silently break this, and as the template says, the unit suite compares the list against a copy of itself.
• Applying the new spec.tls block onto MariaDB objects that already exist was reasoned, not executed (no live cluster). The values are identical to what the operator's mutating webhook already stores: SetDefaults writes &TLS{Enabled: true} when spec.tls is nil (api/v1alpha1/mariadb_types.go:835-839), and IsTLSRequired derefs a nil Required to false (:923-930), so required: false is the value already in effect. No field changes value, but live apply was not replayed.
• chart_lint reports one render error for packages/apps/mariadb. It is a bootstrap stub artifact: the harness passes _cluster as a sequence, so cozy-lib.images-registry fails on index. With _cluster as a map the chart renders clean on every corner below.
• review-helper upstream-src cached the pinned operator tree outside the review prefix, at /var/folders/.../T/cozy-upstream/mariadb-operator-mariadb-operator-25.10.2. Extend the cleanup command if you want it gone.

Checked and correct

• Upgrade from N-1, rendered rather than reasoned. git worktree at the merge base against head, four corners (default, external=true, replicas=1, backup.enabled=true): the only identity delta on each is + TenantProjection/mariadb-test-ca. Nothing renamed, nothing removed, no immutable break.
• Fresh install. cert-manager is a hard dependency of the operator's own PackageSource (packages/core/platform/sources/mariadb-operator.yaml lists cozystack.cert-manager) and the system bundle emits it, so the Issuer and Certificate CRDs are served before any instance renders.
• Configuration matrix. Eleven corners rendered: issuer unset / operator / cert-manager, crossed with external, required, replicas=1, backup.enabled=true, and tls: null. Every one renders except issuer=cert-manager, external=true, _namespace=null, which is the intended required guard, and _namespace.host is always emitted (packages/apps/tenant/templates/namespace.yaml:125), so that guard is not operator-reachable.
• Test non-vacuity. Thirteen mutations, each in its own checkout, all covered: dropping serverCASecretRef, hardcoding required: false, removing the dot from the leaf secretName, re-adding client auth, removing the cert-manager names from the cleanup delete, dropping --ssl-verify-server-cert, repointing the projection source at <release>-ca, inverting the helper default, dropping the external SAN, dropping the watch label, and three on the cozyrd (dropping .ca-tls from exclude, adding an unrestricted include, adding a label-only exclude). No gaps.
• make generate is a no-op on this tree: git status --porcelain is empty after running it, so values.schema.json, the README table, the -rd openAPISchema and keysOrder are all current.
• No reconcile amplifier. No checksum/* annotation is added, and the operator's bundle builder dedupes by certificate ID (pkg/pki/pem.go:87-102), so -ca-bundle is stable across reconciles even though it reads itself.
• Certificate rotation gets less frequent, not more. The chart's leaf is duration: 8760h with renewBefore: 720h; the operator's own default is DefaultCertLifetime = 3 months at 33% renew-before (pkg/pki/x509.go:15-16, pkg/pki/renewal.go:9). Since the pod annotation hashes the server certificate (internal/controller/mariadb_controller_tls.go:207-218), an operator-issued instance already rolls roughly every two months; moving to cert-manager rolls it less often.
• tls.required: true does not strand the exporter. The operator writes tls=true plus the client cert into the exporter my.cnf whenever IsTLSEnabled() (internal/controller/mariadb_controller_metrics.go:125-150), which is already true today.
• The backup CronJob really cannot start. Its MYSQL_ROOT_PASSWORD reads secretKeyRef.name: {{ .Release.Name }}, the operator's root Secret is <name>-root (api/v1alpha1/mariadb_keys.go:13-23), and the rendered chart emits only -backup-script, -backup and -credentials. The new exit 1 on a missing CA therefore reaches no running path.
• The mechanical anti-pattern sweep is clean on the diff: no || true / || : / 2>/dev/null on a decision command in the new shell, no mutating verb granted cluster-wide (the cleanup Role stays namespaced and name-scoped), and the new bats fixtures round-trip through the real yq/jq.
• E2E selection reaches this change: hack/select-e2e.sh on the changed-file list emits the full suite, mariadb included, so the new chainsaw block runs.
• No mutable-identity hazard. tls.issuer gates whether the Certificate and Issuer objects render; it never feeds a metadata.name, a selector, or a content hash, so flipping it prunes only cert-manager objects and leaves no data-bearing resource to be re-identified.

Recommended follow-ups

• Run cozystack-pr-test on a disposable cluster with tls.issuer: cert-manager, both replicas=1 and replicas=2, and confirm the pods reach Ready and a client with the published anchor completes a verified handshake. That is the one surface no static check reaches.
• Add mariadb-<name>-agent-auth to the cozyrd exclude list so the backstop stays complete if Galera is ever enabled.
• The comment at templates/backup-script.yaml says a fully expired CA set yields "a present but empty ca.crt". BundleCertificatePEMs returns errors.New("no certificate PEMs were found") when every certificate is skipped (pkg/pki/pem.go:65-67), so the reconcile fails and the Secret keeps its previous content rather than going empty. The -s test is still worth keeping; the stated reason is not what happens.

LGTM. This lands the CA-consume contract the way #2811 settled it, and the two things I went looking for as risks both turn out to be handled deliberately.

The trust anchor follows the agreed shape. A TenantProjection of type: CACert over a key-free source, and the tenant reads it through the label-gated core.cozystack.io/tenantsecrets path rather than by name. The choice of <release>-ca-bundle as the source is the part worth keeping: it is already key-free, it exists under both tls.issuer values where a cert-manager Secret does not, and it carries the previous CA alongside the new one across a rollover, so the tenant reads one stable object regardless of which issuer is active. That is the CA-agnostic consume contract rather than a homogenised CA.

No TLS Secret is granted in the resourcemap, key-bearing or not — including -ca-bundle, on the grounds that a name grant conveys whatever occupies the name while the label path conveys only what the platform vouched for. That reasoning matches the mongodb chart almost word for word, which is what I want: one argument, applied the same way in both places.

TLS being served under both issuers is not a behaviour change, and the comment explains why in the one place it matters. mariadb.yaml sends an explicit tls.enabled: true because the operator substitutes an enabled spec only when spec.tls is absent entirely — so sending the block without it would have turned TLS off. That inverts the obvious reading of the diff, and it is the kind of thing that gets "simplified" later by someone who assumes the explicit true is redundant.

required is opt-in and typed to stay that way. Required *bool with no default keeps "unset" distinguishable from an explicit false, and the template's kindIs "invalid" guard preserves that distinction rather than collapsing it. The comment states that flipping it disconnects every client not yet on TLS and is therefore a decision rather than something an upgrade does, with the flip named as a future announced change. That sequencing is correct: serve first, enforce later, and never enforce as a side effect of an upgrade.

api/ is additive throughout — a new optional struct defaulting to {}, an enum defaulting to operator, and the pointer above. Existing MariaDB objects render unchanged.

CI is green including E2E on b24ed36f0.

One cosmetic note, not a request: the Go field is Tls where the repo's other initialisms would suggest TLS. The JSON tag is tls either way and the linter evidently does not enforce it, so this only matters if someone later turns that rule on.