From 8e48cfc8fdcdf4d2291ee986922ec1829d3bda55 Mon Sep 17 00:00:00 2001 From: Aleksei Sviridkin Date: Sun, 19 Jul 2026 16:10:02 +0300 Subject: [PATCH] fix(rabbitmq): refuse contradictory TLS settings and correct the upgrade note MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two things this change got wrong about its own behaviour. tls.disableNonTLSListeners was silently discarded whenever TLS resolved to off, because the value was folded into the TLS condition before anything read it. Asking to close the plaintext listeners with TLS off leaves nothing listening, and the operator rejects the combination itself, returning "TLS must be enabled if disableNonTLSListeners is set to true" and setting ReconcileSuccess=False. Swallowing it in the chart was worse than what the operator does: the operator at least records a TLSError event, while a dropped flag rendered a healthy-looking cluster with every plaintext listener open and no trace of the request. The likeliest way in is an operator hardening an internal broker who sets only the flag, since tls.enabled follows external and so resolves to false there. Fail with a message naming tls.enabled instead. The README also claimed messaging clients are unaffected by enabling TLS. That holds only for a client with a hardcoded port. -default-user is the Service Binding object the chart grants to the tenant, and the operator rewrites its port key from 5672 to 5671 as soon as spec.tls.secretName is set — gated on TLS alone, not on whether the plaintext listener stayed open — along with the per-plugin port keys. A binding-driven client, which is the documented way to consume it, follows that change and must speak TLS from then on, without being able to verify the certificate until the trust anchor is obtainable. Say so. Also record that the connection-uri annotation fans out: the operator's annotation filter strips only kubernetes.io and k8s.io keys, so it lands on every child object including the Secret the tenant reads. Assisted-by: LLM Signed-off-by: Aleksei Sviridkin --- packages/apps/rabbitmq/README.md | 8 +++--- .../apps/rabbitmq/templates/rabbitmq.yaml | 25 +++++++++++++++++-- .../rabbitmq/tests/rabbitmq_tls_test.yaml | 25 ++++++++++++++----- 3 files changed, 47 insertions(+), 11 deletions(-) diff --git a/packages/apps/rabbitmq/README.md b/packages/apps/rabbitmq/README.md index ff854d823..436db020b 100644 --- a/packages/apps/rabbitmq/README.md +++ b/packages/apps/rabbitmq/README.md @@ -45,7 +45,7 @@ The service utilizes official RabbitMQ operator. This ensures the reliability an ## TLS scope -This chart adds TLS listeners for AMQP (port 5671) and the management HTTPS interface (port 15671) using cert-manager. It adds encrypted endpoints; it does not close the unencrypted ones. +This chart adds TLS listeners for AMQP (port 5671) and the management HTTPS interface (port 15671) using cert-manager. By default it adds encrypted endpoints without closing the unencrypted ones — but it does change which endpoint clients are pointed at, and `tls.disableNonTLSListeners` can close the plaintext ones outright. Both are covered below. **By default this chart does not close plaintext, inside or outside the cluster.** AMQP on 5672 and management on 15672 keep listening whenever TLS is on, and when `external` is `true` they are published on the public LoadBalancer alongside the TLS ports. Enabling TLS adds encrypted endpoints; using them is the client's choice rather than something the endpoint enforces. Treat a default `external: true` RabbitMQ as reachable in plaintext from outside the cluster, and restrict it at the network layer if that is not acceptable. @@ -57,9 +57,11 @@ The broker pods roll once, because the operator mounts the new TLS Secret into t And **the published metrics port changes**. The operator exposes *either* `prometheus` (15692) *or* `prometheus-tls` (15691) on its Service, never both — deliberately, so that a scrape config selecting every port does not count each node twice — and that choice keys off TLS alone. Enabling TLS therefore withdraws 15692 from the Service and publishes 15691 in its place. The broker itself keeps listening on 15692, but a `ServiceMonitor` or `VMServiceScrape` selecting the port by the name `prometheus` stops matching. Retarget it to `prometheus-tls`, which is HTTPS under the release CA and so needs a `tlsConfig` carrying the trust anchor described below. The chart does not paper over this by re-adding 15692 through `spec.override.service`: the operator drops any port name it does not own on each reconcile and the override would reinstate it without a NodePort, which on a LoadBalancer means a fresh allocation and another reconcile every round. -Messaging clients are unaffected — the plaintext AMQP and management listeners stay open, as above. Set `tls.enabled: false` explicitly to keep the previous behaviour entirely. +And **the connection details handed to clients change**. `-default-user` is the Service Binding object this chart grants to the tenant, and the operator rewrites its `port` key from `5672` to `5671` as soon as `spec.tls.secretName` is set — gated on TLS alone, not on whether the plaintext listener is still open. A client that hardcodes `:5672` keeps working, because that listener does stay open. A client that resolves its endpoint from the binding — the documented way to consume it — follows the change to 5671 and must speak TLS from then on, and it cannot verify the certificate until the trust anchor below is obtainable. Per-plugin port keys (`mqtt-port`, `stomp-port`, `stream-port` and the web variants) move to their TLS values the same way. -**The plaintext management listener is load-bearing.** `messaging-topology-operator` is what reconciles the `User`, `Vhost` and `Permission` objects this chart creates from `users` and `vhosts`, and it talks to the broker over the management API. Once TLS is on it would switch to HTTPS on 15671 and verify the broker against the operator process's system certificate pool, which cannot contain a per-release self-signed CA — so every user and permission would fail to reconcile with `certificate signed by unknown authority`. The chart therefore annotates the cluster with `rabbitmq.com/operator-connection-uri` pointing at plaintext `http://…:15672`, keeping that operator on the in-cluster plaintext port. This affects only the operator's own management connection, never broker traffic. This affects only the operator's own management connection, never broker traffic, and it is why `tls.disableNonTLSListeners` cannot be combined with `users` or `vhosts`. +Set `tls.enabled: false` explicitly to keep the previous behaviour entirely. + +**The plaintext management listener is load-bearing.** `messaging-topology-operator` is what reconciles the `User`, `Vhost` and `Permission` objects this chart creates from `users` and `vhosts`, and it talks to the broker over the management API. Once TLS is on it would switch to HTTPS on 15671 and verify the broker against the operator process's system certificate pool, which cannot contain a per-release self-signed CA — so every user and permission would fail to reconcile with `certificate signed by unknown authority`. The chart therefore annotates the cluster with `rabbitmq.com/operator-connection-uri` pointing at plaintext `http://…:15672`, keeping that operator on the in-cluster plaintext port. This affects only the operator's own management connection, never broker traffic, and it is why `tls.disableNonTLSListeners` cannot be combined with `users` or `vhosts`. **Erlang distribution and epmd stay plaintext no matter what you set.** Inter-node distribution (25672) and epmd (4369) are carried on the headless Service, which `disableNonTLSListeners` never touches — the flag does not apply to them. So `tls.disableNonTLSListeners: true` does not mean "TLS everywhere": it means every *client-facing* listener is TLS-only while inter-node traffic stays in the clear. Inter-node mTLS via `spec.tls.caSecretName` is not wired by this chart. diff --git a/packages/apps/rabbitmq/templates/rabbitmq.yaml b/packages/apps/rabbitmq/templates/rabbitmq.yaml index a5cfb11ce..32a510689 100644 --- a/packages/apps/rabbitmq/templates/rabbitmq.yaml +++ b/packages/apps/rabbitmq/templates/rabbitmq.yaml @@ -1,5 +1,20 @@ {{- $tlsEnabled := (include "rabbitmq.tls.enabled" .) | eq "true" }} -{{- $disableNonTLS := and $tlsEnabled (dig "disableNonTLSListeners" false (.Values.tls | default dict)) }} +{{- $wantDisableNonTLS := dig "disableNonTLSListeners" false (.Values.tls | default dict) }} + +{{- /* Refuse the contradiction rather than drop it. Asking to close the plaintext + listeners while TLS is off leaves nothing listening at all, and the operator + rejects the combination itself — reconcile_tls.go returns + "TLS must be enabled if disableNonTLSListeners is set to true" and sets + ReconcileSuccess=False. Silently ignoring the flag here would be worse than + what the operator does: it at least records a TLSError event, whereas a + dropped flag renders a healthy-looking cluster with every plaintext listener + open and no trace of the request. The likely way to reach this is an operator + hardening an internal broker who sets only disableNonTLSListeners, since TLS + is off by default when external is false. */}} +{{- if and $wantDisableNonTLS (not $tlsEnabled) }} +{{- fail "tls.disableNonTLSListeners is set but TLS is off, which would leave no listener at all: set tls.enabled: true (it defaults to the value of external). The RabbitMQ operator rejects this combination too." }} +{{- end }} +{{- $disableNonTLS := and $tlsEnabled $wantDisableNonTLS }} {{- /* disableNonTLSListeners closes the plaintext management port, and messaging-topology-operator has no working route to the broker without it. @@ -42,7 +57,13 @@ metadata: tlsEnabled=false for an http:// scheme, so the operator uses the plaintext management listener — which the chart deliberately keeps open. That listener is load-bearing, not merely tolerated. Broker traffic is unaffected; this - governs only the operator's own management connection. */}} + governs only the operator's own management connection. + + Note it does not stay on this object: ReconcileAndFilterAnnotations strips + only keys containing kubernetes.io or k8s.io, so this annotation is copied + onto every child the operator builds — Service, StatefulSet, ServiceAccount, + Role and the default-user Secret the tenant reads. Harmless, but it is why + an internal plumbing annotation shows up on tenant-visible objects. */}} annotations: rabbitmq.com/operator-connection-uri: "http://{{ .Release.Name }}.{{ .Release.Namespace }}.svc.{{ include "rabbitmq.clusterDomain" . }}:15672" {{- end }} diff --git a/packages/apps/rabbitmq/tests/rabbitmq_tls_test.yaml b/packages/apps/rabbitmq/tests/rabbitmq_tls_test.yaml index 59a3c440c..1a31c77f0 100644 --- a/packages/apps/rabbitmq/tests/rabbitmq_tls_test.yaml +++ b/packages/apps/rabbitmq/tests/rabbitmq_tls_test.yaml @@ -383,7 +383,7 @@ tests: path: spec.tls.disableNonTLSListeners value: true - - it: ignores the flag when TLS is off, since the operator rejects that combination + - it: refuses the flag when TLS is explicitly off release: name: myrabbitmq namespace: tenant-test @@ -391,12 +391,25 @@ tests: tls.enabled: false external: false tls.disableNonTLSListeners: true - documentSelector: - path: kind - value: RabbitmqCluster asserts: - - notExists: - path: spec.tls + # Dropping the request silently would render a healthy-looking cluster with + # every plaintext listener open. The operator rejects this combination too, + # and at least leaves a TLSError event; a swallowed flag leaves nothing. + - failedTemplate: + errorPattern: "would leave no listener at all" + + - it: refuses the flag when TLS is off by default because external is false + release: + name: myrabbitmq + namespace: tenant-test + set: + external: false + tls.disableNonTLSListeners: true + asserts: + # The likeliest way in: hardening an internal broker by setting only the + # flag, where tls.enabled defaults to external and so resolves to false. + - failedTemplate: + errorPattern: "would leave no listener at all" - it: drops the operator connection annotation when the plaintext port is closed release: