feat(rabbitmq): add TLS support for AMQPS and management HTTPS - #2683

Actionable comments posted: 3

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/rabbitmq/templates/certmanager.yaml`:
- Around line 54-57: The SAN list creation in the $dnsNames template hardcodes
"cluster.local" which breaks clusters with custom DNS; change the third entry to
use a configurable cluster domain (e.g. .Values.clusterDomain) instead of the
literal "cluster.local" so the SAN becomes (printf "%s.%s.svc.%s" .Release.Name
.Release.Namespace .Values.clusterDomain), and add a default clusterDomain:
"cluster.local" to values.yaml so .Values.clusterDomain is available.
- Around line 5-10: The current logic only checks hasKey and then assigns
$tlsEnabled = .Values.tls.enabled which treats a null value as false and
prevents falling back to .Values.external; change the branch to explicitly treat
null as "inherit" by checking for nil (e.g., use hasKey and ne
.Values.tls.enabled nil) or use Helm's default semantics to set $tlsEnabled to
.Values.tls.enabled when it is non-null, otherwise to .Values.external; update
the assignment that sets $tlsEnabled (referencing $tlsEnabled,
.Values.tls.enabled, .Values.external, and hasKey) so that a null
.Values.tls.enabled falls back to .Values.external.

In `@packages/apps/rabbitmq/templates/rabbitmq.yaml`:
- Around line 4-9: The template treats a present-but-null .Values.tls.enabled as
an explicit value because it uses hasKey, which causes the external fallback
(.Values.external) to be skipped; replace the tri-state check with a
default-based expression (e.g. set $tlsEnabled using default .Values.external
.Values.tls.enabled or the Helm "or" idiom) so that nil/absent tls.enabled falls
back to .Values.external and downstream logic that emits spec.tls.secretName
uses the correct value; update the block that defines $tlsEnabled (currently
using hasKey .Values.tls "enabled" and .Values.tls.enabled) to the single
default/or expression.

---

Nitpick comments:
In `@packages/apps/rabbitmq/tests/rabbitmq_tls_test.yaml`:
- Around line 136-169: Add tri-state coverage for LoadBalancer port overrides:
extend the existing test blocks (the ones using
fixtures/tls_default_values.yaml, release name myrabbitmq, documentSelector
kind: RabbitmqCluster) to also assert spec.override.service.spec.ports when
tls.enabled is unset (default) and external: true; specifically add an asserts
block that checks spec.override.service.spec.ports contains the expected LB port
entries (the same check you have for the other two tests that validate
spec.tls.secretName and management.ssl.port) and mirror the same additional test
for the other range (lines 253-293) so the unset/true/false matrix is fully
covered.
- Around line 320-330: The baseline test currently asserts the rendered
RabbitmqCluster using a documentIndex (documentIndex: 0), which is brittle;
update the assertion to select the RabbitmqCluster by kind instead of by index
so the test doesn't break if other manifests are rendered first — replace the
assertion that includes "documentIndex: 0" with a selector-style isKind check
that targets "RabbitmqCluster" (matching how other tests select by kind) so the
test locates the resource by its kind rather than its position.

Code Review

This pull request introduces TLS support for the RabbitMQ application, including the necessary API changes, Helm templates for cert-manager integration, and updated schema definitions. It also adds comprehensive unit tests to verify the TLS configuration and service port overrides. Regarding the implementation, consider parameterizing the hardcoded .cluster.local domain in the cert-manager templates to better support environments with custom cluster domains.

Actionable comments posted: 1

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/rabbitmq/templates/certmanager.yaml`:
- Around line 54-58: The DNS SAN list variable $dnsNames in certmanager.yaml
currently only includes the client service names; update it to also include the
headless nodes service SANs required by RabbitMQ Cluster Operator by adding
entries for the "-nodes" service variants (e.g., add (printf "%s-nodes"
.Release.Name), (printf "%s-nodes.%s.svc" .Release.Name .Release.Namespace) and
(printf "%s-nodes.%s.svc.cluster.local" .Release.Name .Release.Namespace) to the
list) so node-to-node TLS hostname verification succeeds.

---

Duplicate comments:
In `@packages/apps/rabbitmq/templates/certmanager.yaml`:
- Around line 5-10: The template treats a present-but-null .Values.tls.enabled
as false because hasKey alone is used; change the logic so null means "inherit"
by checking for presence AND non-null. Replace the if that sets $tlsEnabled to
use: if hasKey .Values.tls "enabled" and not (eq .Values.tls.enabled nil) then
set $tlsEnabled = .Values.tls.enabled else set $tlsEnabled = .Values.external;
reference the variables $tlsEnabled, .Values.tls.enabled and .Values.external
and the hasKey check when making the change.
- Around line 54-57: The template currently hardcodes "cluster.local" in the
SANs (inside the $dnsNames list), which breaks clusters with custom cluster
domains; update the $dnsNames construction to use a configurable cluster domain
instead (e.g., use default "cluster.local" for backward compatibility via a
chart value like .Values.clusterDomain) and build the FQDN with printf
"%s.svc.%s" .Release.Name (default .Values.clusterDomain) so the SANs reflect
the actual cluster domain; adjust any values.yaml to document
.Values.clusterDomain default.

In `@packages/apps/rabbitmq/templates/rabbitmq.yaml`:
- Around line 4-9: The template treats tls.enabled=null as an explicit value and
skips the external fallback; change the logic that computes $tlsEnabled so null
is treated like "not set" — i.e., if .Values.tls.enabled is missing or nil use
.Values.external, otherwise use .Values.tls.enabled; you can implement this by
replacing the hasKey branch with a check that .Values.tls.enabled is not nil (or
by using Helm's default helper: default .Values.external .Values.tls.enabled) to
assign $tlsEnabled.

---

Nitpick comments:
In `@packages/apps/rabbitmq/tests/rabbitmq_tls_test.yaml`:
- Around line 25-38: The test currently asserts that
spec.rabbitmq.additionalConfig does not exist when tls.enabled is false;
instead, change the assertions to only scope absence to the management SSL keys
(e.g., assert notExists for paths like
spec.rabbitmq.additionalConfig.management.ssl and/or
spec.rabbitmq.additionalConfig.management.ssl.*) so unrelated additionalConfig
entries won't break the test; update the two test cases (the one shown and the
one at lines 174-190) to replace the top-level notExists on
spec.rabbitmq.additionalConfig with notExists checks targeting
spec.rabbitmq.additionalConfig.management.ssl (or the specific keys under
management.ssl) while leaving other parts of additionalConfig allowed.

NOT LGTM — cert chain is clean (correct CA usages [cert sign, crl sign], correct leaf [digital signature, key encipherment, server auth], SAN list covers both client-facing service and inter-node headless wildcard), and the tri-state tls.enabled resolution was tightened in a3acd2d2 to handle explicit null correctly. Two blockers: when TLS lands on an externally-published instance, the LoadBalancer exposes BOTH the plaintext and TLS ports, and the chart-managed CA's ca.crt is not reachable by tenant clients who need it to verify the TLS handshake.

Business context: Add TLS for the managed RabbitMQ service — AMQPS (5671) and Management HTTPS (15671) — via a self-contained cert-manager chain, with tls.enabled falling back to external when unset. Plaintext listeners are intentionally preserved for backward compatibility per the PR description.

Blockers

B1: External LoadBalancer exposes plaintext AMQP (5672) and plaintext Management HTTP (15672) alongside TLS ports

File: packages/apps/rabbitmq/templates/rabbitmq.yaml:36-56

Issue: When external: true AND TLS is enabled (either explicitly or via the auto-on tri-state), the override.service.spec.ports block unconditionally declares all four ports — amqp, amqps, management, management-tls — on the type: LoadBalancer Service. The PR description acknowledges plaintext "coexists with the TLS ports" for backward compatibility; what's not acknowledged is that this coexistence is published over the public LoadBalancer interface, not just inside the cluster.

Evidence:

{{- if and $tlsEnabled .Values.external }}
service:
  spec:
    ports:
      - name: amqp
        port: 5672
        targetPort: 5672
      - name: amqps
        port: 5671
        targetPort: 5671
      - name: management
        port: 15672
        targetPort: 15672
      - name: management-tls
        port: 15671
        targetPort: 15671
{{- end }}

spec.tls.disableNonTLSListeners is also not set — that upstream RabbitMQ-operator flag would have closed 5672/15672 at the broker level even if the Service exposed them. With this PR, an attacker on the public network can pick the plaintext AMQP port and send PLAIN SASL credentials in cleartext, or hit the management UI over HTTP and submit cookies/sessions in cleartext.

Impact: Silent security regression for the precise cohort the TLS auto-on logic targets — externally-published instances. Operators who toggle external: true (and therefore implicitly opt in to TLS via the tri-state) reasonably expect the public-facing surface to be TLS-only; today they get TLS plus full plaintext as a bonus.

Fix — pick one (not mutually exclusive):

• Set spec.tls.disableNonTLSListeners: true on the RabbitmqCluster when TLS is on. This closes plaintext at the broker level. Add a tls.required: false escape hatch (mirroring the mariadb pattern in #2680) for tenants in mid-migration that still need plaintext.

• Drop the plaintext ports from the LoadBalancer override when TLS is on:

  {{- if and $tlsEnabled .Values.external }}
  service:
    spec:
      ports:
        - name: amqps
          port: 5671
          targetPort: 5671
        - name: management-tls
          port: 15671
          targetPort: 15671
  {{- end }}

  Leaves plaintext accessible only over ClusterIP (in-cluster clients can still use it during migration), TLS-only externally.

Either fix the PR description's "preserved for backward compatibility" claim by scoping it explicitly to in-cluster traffic, with TLS-only external. The current state silently widens the attack surface.

B2: Tenant clients cannot verify the TLS handshake — the chart-managed CA ca.crt is not surfaced

File: packages/system/rabbitmq-rd/cozyrds/rabbitmq.yaml (the dashboard resource map) + packages/apps/rabbitmq/templates/dashboard-resourcemap.yaml (the RBAC)

Issue: This chart issues a brand-new self-signed CA in the tenant namespace and uses it to sign the leaf serving cert. For any client to verify the AMQPS/HTTPS handshake against that CA — outside the tenant cluster's default cluster-trust roots — they need the CA's ca.crt. The dashboard resource map exposes <release>-default-user and <release>-<user>-credentials only; the CA secret (<release>-ca) and the leaf secret (<release>-tls) are not in the map. The dashboard role binds at the use access level, so even tenant admins (without raw kubectl get secret access on the namespace) hit the same gap.

Evidence: cozyrds/rabbitmq.yaml:31-37 only lists rabbitmq-{{ .name }}-default-user and user-credential secrets under secrets.include. dashboard-resourcemap.yaml RBAC matches. cert-manager produces <release>-ca (type kubernetes.io/tls, contains the CA private key + cert) and <release>-tls (type kubernetes.io/tls, contains the server private key + cert + ca.crt). Both have tls.key alongside tls.crt; Kubernetes RBAC at the Secret level cannot scope to individual keys.

Impact: An external client that wants to do verify-peer on the AMQPS handshake (the security default for production) has no path to fetch the CA cert via the supported tenant interface. Workarounds available today (cluster admin pulls the Secret out-of-band, or tenant disables peer verification) defeat the security gain of this PR.

The cleanest fix is to publish only ca.crt — not the private keys — as a separate, tenant-visible object. cert-manager's additionalOutputFormats: [{type: "CombinedPEM"}] on the leaf Certificate doesn't help here (it adds more material to the same Secret, not a separate one). Two viable patterns:

• Post-install Helm hook Job that reads <release>-tls's ca.crt and writes it into a <release>-ca-bundle ConfigMap. Add that ConfigMap to cozyrds/rabbitmq.yaml under configMaps.include. The ConfigMap has no private key material, so the use-tier RBAC is safe.

• Use cert-manager's trust-manager (if available cluster-side) to distribute the CA cert into a ConfigMap declaratively.

The same gap is observed (and called out) in #2680 mariadb B3 — but there it was the inverse problem (RBAC TOO permissive). Here the RBAC is correctly restrictive but the chart never publishes the public-only material. Solving B3 from #2680 and B2 here with the same ConfigMap pattern aligns the whole TLS batch.

Non-blocking follow-ups

1. PR description acknowledges Erlang inter-node distribution (port 25672) stays plaintext. Worth tracking as an explicit follow-up issue (referenced by-number) so it doesn't drift. spec.tls.caSecretName + a separate per-node cert is the canonical upstream path; the chart's current single-cert model needs to be extended for that.

2. certmanager.yaml:37-42: CA private key only sets rotationPolicy: Never, leaving algorithm/size at cert-manager defaults (RSA-2048). Non-uniform across the batch (mariadb uses RSA-4096 explicitly). Acceptable but worth a project-level convention.

3. certmanager.yaml:59-68: SAN list includes *.<release>-nodes.<ns>.svc (without cluster domain). The RabbitMQ operator's inter-node communication uses rabbit@<pod>.<release>-nodes.<ns>.svc.<cluster-domain> (FQDN form), which IS covered by the wildcard on line 67. Inter-node Erlang distribution is plaintext (per follow-up #1), so today the inter-node SANs aren't exercised over TLS — they're future-proofing for when #1 lands.

Comparison to the sibling TLS PRs reviewed: B2 from #2680 (missing digital signature in usages) does NOT recur — present here on the leaf. B3 from #2680 (dashboard RBAC over TLS private keys) does NOT recur — <release>-ca and <release>-tls are correctly excluded from the dashboard role. B1 from #2684/#2685 (short-form peer hostnames not in SAN) does NOT recur for RabbitMQ either — inter-node uses FQDN by upstream chart's default and is plaintext today regardless.

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/rabbitmq/templates/dashboard-resourcemap.yaml`:
- Around line 20-24: The RBAC entry currently grants tenants "get" on the full
TLS Secret ({{ .Release.Name }}-tls) which exposes tls.key; instead, create a
dedicated CA-only Secret (e.g., {{ .Release.Name }}-ca) containing only ca.crt
and change the RBAC grant in dashboard-resourcemap.yaml to reference that CA
secret instead of {{ .Release.Name }}-tls; implement the extraction in
certmanager.yaml (or the certificate controller) to copy/extract ca.crt from the
primary TLS Secret into the new {{ .Release.Name }}-ca Secret at
issuance/rotation time, and update any template references that expect the CA
secret name.
- Around line 20-24: The RBAC rule in dashboard-resourcemap.yaml grants read
access to the entire TLS Secret ({{ .Release.Name }}-tls) when $tlsEnabled is
true, which exposes tls.key; instead, stop granting get/list/watch on {{
.Release.Name }}-tls and grant access only to a new minimal Secret that contains
the CA cert only (e.g., {{ .Release.Name }}-ca). Update
dashboard-resourcemap.yaml to reference {{ .Release.Name }}-ca when $tlsEnabled,
and change certificates provisioning
(packages/apps/rabbitmq/templates/certmanager.yaml) or add a small post-install
controller/hook to copy only the ca.crt from spec.secretName: {{ .Release.Name
}}-tls into the new {{ .Release.Name }}-ca Secret so tenants can read ca.crt
without access to tls.key.

---

Outside diff comments:
In `@packages/apps/rabbitmq/templates/rabbitmq.yaml`:
- Around line 37-54: The override using spec.override.service.spec.ports only
adds ports and does not remove operator-generated plaintext ports (5672/15672);
update the Helm/Kustomize override for the Service to explicitly remove those
ports by using delete semantics (e.g., add a $patch: delete or use the
deleteDirective keyed by port) so that only the TLS ports (5671/15671) remain
exposed; target the Service ports list (spec.override.service.spec.ports) and
identify entries to remove by their port numbers (5672 and 15672) when applying
the patch.

NOT LGTM — re-review on head 6d5f940e. Two security blockers. The first reopens a point I previously marked resolved: the "drop plaintext ports" approach does not actually remove them.

Business context: adds TLS (AMQPS 5671 + Management HTTPS 15671) to the managed RabbitMQ app via a self-contained cert-manager chain, with a tri-state tls.enabled that falls back to external when unset.

Blockers

B1 (reopened): omitting plaintext ports from the override does not remove them — plaintext AMQP/management stay on the public LoadBalancer
File: packages/apps/rabbitmq/templates/rabbitmq.yaml:36-55
Issue: when external + TLS, the override emits only amqps:5671 and management-tls:15671, relying on omission to keep plaintext amqp:5672 / management:15672 off the LoadBalancer. But the RabbitMQ Cluster Operator applies spec.override.service.spec.ports as a strategic merge keyed by port and independently regenerates its default ports — omitting a port from the override never deletes it.
Evidence: the PR's own test comment states this verbatim (tests/rabbitmq_tls_test.yaml:235-237): "This MERGES by port number rather than replacing the whole list; operator-generated defaults (amqp:5672, amqps:5671, management:15672) are unaffected." The notContains assertions only validate the rendered CR override block, not the final Service the operator produces, so the suite is green while plaintext stays exposed.
Impact: silent public plaintext exposure for exactly the external-published cohort the TLS auto-on targets — cleartext SASL PLAIN credentials over AMQP 5672 and cleartext management sessions over HTTP 15672 on a public IP. The PR's security goal is not met.
Fix: close plaintext at the broker with spec.tls.disableNonTLSListeners: true when TLS is on (the operator then also moves the readiness probe to amqps automatically). The current template comment explicitly avoids this to protect in-cluster clients during migration — so pair it with an opt-out (tls.allowPlaintext) for the migration window rather than leaving plaintext on a public LB. The omit-from-override approach cannot work against this operator.

B2: dashboard RBAC grants the server private key to every tenant use member
File: packages/apps/rabbitmq/templates/dashboard-resourcemap.yaml:20-24
Issue: with TLS on, the Role adds <release>-tls to resourceNames for the tenant use access level (RoleBinding at :39). The comment intends ca.crt retrieval, but <release>-tls is the cert-manager kubernetes.io/tls leaf Secret carrying tls.key — the broker's serving private key. RBAC is per-Secret, so get exposes the key, not just ca.crt.
Evidence: certmanager.yaml leaf secret (<release>-tls, type kubernetes.io/tls); Role grant at dashboard-resourcemap.yaml:23 bound to "use".
Impact: a use-level tenant can extract the broker private key, stand up an impersonating TLS terminator on 5671, MITM other clients' AMQP traffic, and decrypt captured non-PFS sessions.
Fix: expose only the public ca.crt (separate ConfigMap via trust-manager, or a ca.crt-only Secret), not the leaf Secret. Same defect as the sibling mariadb/mongodb TLS PRs — a single shared fix would close it across the series.

Non-blocking follow-ups

1. The test comment at tests/rabbitmq_tls_test.yaml:242 ("we include amqp, amqps, management, management-tls") is stale after the override change and will need rewriting once B1 is fixed via disableNonTLSListeners.
2. Erlang inter-node distribution (25672) stays plaintext (acknowledged in the README); inter-node -nodes SANs are already wired as future-proofing. Fine as a tracked follow-up.
3. README "TLS scope" inherits the framing that plaintext "coexists for backward compat" without disclosing that, when external: true, that coexistence is on the public LoadBalancer (B1). Fix the framing alongside B1.

Superseded by #2683 (review), which carries the same findings as inline comments on the lines they are about. The original text is removed rather than left as a second copy of the same review.

Verdict

NOT LGTM

The guard added here to prevent a wedged teardown enumerates three of the twelve topology kinds that wedge identically, and the runbook step meant to confirm the namespace is clear checks the same three.

Findings

• [MAJOR] packages/apps/rabbitmq/templates/rabbitmq.yaml:39, the topology-object refusal covers 3 of the 12 kinds that share the wedging deletion path

Claim mismatches

• "Turning TLS back off leaves the published anchor behind" is contradicted by this PR's own README.md:110, and the README matches the code: reconciler.go:1101-1103 builds a real owner reference, so deleting the sentinel garbage-collects the projection. What survives is the raw CA material, not the anchor.
• Three paragraphs describe deltas against earlier revisions of this branch rather than against main: the spec.override.service block, the <release>-tls grant and the additionalProperties guard are not in this diff at all. Worth a line saying they answer review rounds.

Caveats

• Not affected on upgrade, and that is rendered rather than reasoned: tls defaults to {} and nothing infers it from external, so three corners come out unchanged against merge-base.
• Certificate renewal reload cannot be verified statically and rests on Erlang re-reading the PEM by mtime. The merged qdrant and nats charts carry identical durations, so it is a property of the series rather than of this PR.

Verdict

LGTM with non-blocking notes

Reviewed at 3c5d9f49e510dd6870c84a331c91ec199aaf18ac against merge-base 3a70f91423b17cafa27e59a5d40fe7c3076416bf. The default path renders byte-identically to main, every schema-valid TLS corner was rendered, and fourteen mutations of the new guards and SAN entries all turn the suite red. One MINOR below, plus the live-cluster surfaces this static review cannot reach.

Findings

• [MINOR] packages/apps/rabbitmq/README.md:110, the private-key Secrets survive an application delete and only the disable path says so

Caveats

• Upgrade path: rendering the merge-base and head over the external, replicas and storageClass corners produces no difference for an instance that does not set tls, which is every instance in the field today.
• Not executed, needs a live cluster: enabling TLS on a running instance (issuance, the operator reconciling against a Secret that does not exist yet, the pod roll), the broker picking up a renewed leaf, and <release>.tenant-ca reaching tenantsecrets for this chart. All three sit behind an explicit tls.enabled: true, so no unattended upgrade reaches them. See follow-ups.
• review-helper render-matrix came back inconclusive on every corner, so the corner set was driven by helm template over the chart's own values instead. A corner on an axis I did not name could have been missed.
• Enabling TLS does not reduce what an external: true broker exposes. Plaintext 5672 and 15672 stay on the LoadBalancer, and tls.disableNonTLSListeners, the only switch that closes them, refuses to render alongside users or vhosts — the only way this chart declares users. A published broker with chart-managed users therefore has no setting that closes its public plaintext ports. The README says so; noted because it is the shape of the feature rather than a defect in it.
• tests/tls_resolution_test.yaml:154 pins a Helm JSON-schema validator message verbatim. It matches on Helm 3.19.0 and 4.0.4, but the checks job pins no Helm version, so the assertion rides on whatever the runner carries.
• Envelope warnings, unrelated to this diff: 56 CRD files declared a definition the schema pass could not use, 26 group/kind/version pairs are defined twice with differing schemas, and the hot-hunks file holds 100 of 131 matches.

Recommended follow-ups

• A cozystack-pr-test run on a disposable dev cluster for the opt-in path: enable TLS on a running instance and watch it converge, read <release>.tenant-ca as a tenant, then exercise the two-upgrade disableNonTLSListeners order and confirm the User objects are gone rather than Terminating. The author lists this as known limitation 2; it is the right shape for a separate run.
• The trust-anchor leg has an e2e precedent in hack/e2e-chainsaw/mongodb/chainsaw-test.yaml. Mirroring it for rabbitmq would cover the projection end to end without needing the broker's TLS path.
• Pre-existing and outside this change: chart-declared user passwords are rendered into <release>-<user>-credentials as stringData, so they land in the stored Helm release manifest and stay there for the retained revisions. Same class as #4078, unrelated to the TLS work, but this chart is the one holding it.