Skip to content

Certificates SANs FQDN - #713

Open
gulecroc wants to merge 1 commit into
apache:masterfrom
gulecroc:feat/certs-fqdn-san
Open

Certificates SANs FQDN#713
gulecroc wants to merge 1 commit into
apache:masterfrom
gulecroc:feat/certs-fqdn-san

Conversation

@gulecroc

@gulecroc gulecroc commented Jul 30, 2026

Copy link
Copy Markdown
Contributor

Fixes #712

Motivation

Currently certificates are generated with wildcard SANs.

This PR allow to generate SANs with either :

  • wildcard (default)
  • fqdn for stafesulsets
  • none (use tls.<component>.dnsNames)

Modifications

Add tls.common.sanMode :

  • wildcard mode :
  dnsNames:
    - "*.<fullname>-<component>-headless.<namespace>.svc.<cluster>" # only for broker and zookeeper
    - "<fullname>-<component>-headless.<namespace>.svc.<cluster>" # only for broker and zookeeper
    - "<fullname>-<component>.<namespace>.svc.<cluster>"
    - "<fullname>-<component>"
  • fqdn mode :
  dnsNames:
    - "<fullname>-<component>-<replicas>.<fullname>-<component>-headless.<namespace>.svc.<cluster>" # headless statefulsets
    - "<fullname>-<component>-<replicas>.<fullname>-<component>.<namespace>.svc.<cluster>" # non headless statefulsets
    - "<fullname>-<component>-headless.<namespace>.svc.<cluster>" # only for broker and zookeeper
    - "<fullname>-<component>.<namespace>.svc.<cluster>"
    - "<fullname>-<component>"
  • none mode :
  dnsNames:
    - "<fullname>-<component>.<namespace>.svc.<cluster>"
    - "<fullname>-<component>"

Verifying this change

  • Make sure that the change passes the CI checks.

@lhotari lhotari mentioned this pull request Aug 14, 2026
1 task

@lhotari lhotari left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this. To set the bar I'm reviewing against, from my reply on #712:

wildcard certs are required for broker, bookie & zk TLS when hostname verification is enabled on the client side (it should be enabled for security reason). The "client" can be a pulsar proxy or broker (towards bk, zk).
an alternative for wildcard certs would be to list all possible hostnames for SAN.

fqdn mode is exactly that alternative, and it's the right shape for the issuer-forbids-wildcards case in #712 — where wildcard isn't a fallback the user can keep. So the question this review has to answer is narrow: does fqdn mode actually list all possible hostnames?

Today it doesn't, in three cases, and two of them hit broker and bookie — two of the three components that need it.

Component Headless svc? Per-pod FQDN resolves? SAN list complete?
zookeeper yes yes yes — replicaCount is authoritative, no HPA
bookie yes yes no — breaks on kubectl scale
broker yes yes no — breaks under its own HPA
proxy no (ClusterIP) no n/a — client, not a verified server

What's solid

wildcard mode renders byte-identical to master across proxy/broker/bookie/recovery/toolset/zookeeper — I diffed it, so existing users are genuinely unaffected. An invalid sanMode fails the render with a clear message rather than silently emitting a certificate with no SANs, which is the right call. clusterDomain is respected throughout rather than hardcoding cluster.local.

Render recipe I used:

helm dependency build charts/pulsar
helm template test charts/pulsar --set "certs.internal_issuer.enabled=true,\
components.proxy=true,components.toolset=true,tls.enabled=true,tls.broker.enabled=true,\
tls.proxy.enabled=true,tls.zookeeper.enabled=true,tls.bookie.enabled=true,\
tls.autorecovery.enabled=true,tls.toolset.enabled=true" \
--set tls.common.sanMode=fqdn -s templates/tls-certs-internal.yaml

One more thing, not a blocker

Switching wildcardfqdn changes Certificate.spec.dnsNames (so cert-manager reissues) but doesn't change any pod-template checksum — I rendered broker/ZK StatefulSets under both modes and got identical hashes. So Helm performs no restart and the tightened SANs may not take effect until processes reload. Not an outage on transition, since the old wildcard still covers existing ordinals while issuance completes, but worth a docs note that a rolling restart is needed — consistent with the existing TLS upgrade guidance.

Unrelated pre-existing bug I noticed while checking this

standalone-deployment.yaml sets subdomain: <fullname>-standalone-headless and advertises $(hostname -f), so its real name is <pod>.<fullname>-standalone-headless.<ns>.svc.<domain>. No SAN mode on master includes that — not even wildcard, which emits *.<fullname>-standalone…, a different subdomain. With hostname verification on, lookup-directed standalone connections would fail today. I'll file that separately; nothing for you to do here.

Reviewed with Codex gpt-5.6-sol and Claude Opus 5; every finding reproduced locally by rendering the chart.

{{/*
Return the StatefulSet replica count for a certificate component.
*/}}
{{- define "pulsar.certs.statefulset.replicaCount" -}}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the one that matters most, because completeness is the whole point of the mode.

This helper reads .Values.<component>.replicaCount — but for broker and proxy that value isn't the live replica count. Both StatefulSets omit spec.replicas entirely when autoscaling is on:

# broker-statefulset.yaml:34-36
  {{- if not .Values.broker.autoscaling.enabled }}
  replicas: {{ .Values.broker.replicaCount }}
  {{- end }}

So with broker.autoscaling.enabled: true, maxReplicas: 6, the certificate covers ordinals 0-2 only and replicaCount isn't even consulted by the workload. When the HPA creates broker-3, a proxy verifying the broker's hostname fails — which is precisely the failure fqdn mode exists to prevent, on exactly the client→server hop from #712.

Bookies have no HPA, but kubectl scale breaks them the same way, and scaling bookies is routine.

Even the Helm path isn't fully safe: changing replicaCount does re-render the Certificate, but cert-manager reissues asynchronously, so the StatefulSet can create the new ordinal before the new Secret lands. For ZooKeeper the stale certificate can persist until restart, since we enable certReload for ZK clients but not the ZK server.

Options, roughly in order of preference:

  1. Enumerate up to autoscaling.maxReplicas when autoscaling is enabled. Cheap, covers the HPA case completely, and SANs for pods that don't exist yet are harmless.
  2. Add a tls.<component>.extraDnsNames escape hatch and document that fqdn covers only the rendered replica count.
  3. At minimum, fail when sanMode: fqdn is combined with autoscaling.enabled: true, rather than silently issuing an incomplete certificate.

{{- if not (has $sanMode (list "wildcard" "fqdn" "none")) -}}
{{- fail (printf "tls.common.sanMode must be one of: wildcard, fqdn, none (got %q)" $sanMode) -}}
{{- end -}}
{{- $statefulSetComponents := list "zookeeper" "bookie" "broker" "proxy" "toolset" "recovery" "function-worker" -}}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

$component comes from the user-configurable .componentConfig.component (e.g. bookkeeper.component, broker.component), but this list — and both new helpers above — compare it against hard-coded literals.

Set bookkeeper.component: pulsar-bookie with sanMode: fqdn and pulsar-bookie isn't in this list, so no ordinal SANs are emitted at all. And since fqdn also drops the wildcard, the resulting certificate covers none of the per-pod names. Silent — no warning, no render failure.

For a user who moved to fqdn precisely because their issuer forbids wildcards (#712), that's a hostname-verification outage with no fallback.

Either derive the set from the component values rather than literals, or fail when a renamed component is used with sanMode: fqdn.

{{- $replicaCount := (include "pulsar.certs.statefulset.replicaCount" (dict "root" $root "component" $component) | int) -}}
{{- if gt $replicaCount 0 }}
{{- range $i := until $replicaCount }}
- {{ printf "%s-%d.%s.%s.svc.%s" (printf "%s-%s" (include "pulsar.fullname" $root) $component) $i $serviceName (include "pulsar.namespace" $root) $root.Values.clusterDomain | quote }}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For proxy, the service name resolved above falls through to <fullname>-proxy, which does match the StatefulSet's serviceName — but proxy-service.yaml renders type: {{ .Values.proxy.service.type }} (default ClusterIP) with no clusterIP: None. Per-pod <pod>.<svc>.<ns>.svc.<domain> records only exist for headless services, so <release>-pulsar-proxy-0.<release>-pulsar-proxy… never resolves.

(bookkeeper, autorecovery and toolset services all set clusterIP: None, so theirs do resolve — this is proxy-specific.)

Lower severity than the two above: per #712 the proxy is a client toward the broker, not one of the servers whose hostname gets verified, and its service FQDN and short name remain valid. But fqdn mode does swap proxy's wildcard for three names that never resolve, which is misleading in a certificate.

Suggest either dropping proxy from $statefulSetComponents — it isn't a component this mode is for — or emitting per-pod names only when the proxy service is actually headless.

Comment thread charts/pulsar/values.yaml
# - wildcard (default): use wildcard SANs (existing behavior)
# - fqdn: for StatefulSet components, generate explicit per-pod FQDN SANs
# from replica counts
# - none: do not add automatic SAN entries (use tls.<component>.dnsNames and/or ipAddresses)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Small accuracy fix: none doesn't remove all automatic SAN entries. The service FQDN and short name are unconditional, and broker/zookeeper additionally keep their headless name, because that block sits outside the sanMode conditional. Rendered none output for broker is three entries, not the two the PR description lists.

Suggest wording it as what it actually does — skip the wildcard and per-pod names, keep the service names:

Suggested change
# - none: do not add automatic SAN entries (use tls.<component>.dnsNames and/or ipAddresses)
# - none: no wildcard or per-pod SANs; only the service FQDN and short name are added
# (plus the headless name for broker/zookeeper). Use tls.<component>.dnsNames
# and/or ipAddresses to add your own.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants