[improve][misc] PIP-478: the published TLS factory SPI omits the normative custom-factory obligations
- Dominant language
- Java
- Stars
- 15.3k
- Forks
- 3.8k
- Avg merge
- 1d 14h
- Merged PRs (30d)
- 160
Description
### Motivation
`pip-478.md` places three normative obligations on a custom `PulsarTlsFactory`. None of them appears in
the published `pulsar-tls-factory-api` module — `rg -in 'hostname|verif|endpointIdent|shad|relocat|
pulsar.tls.reload' pulsar-tls-factory-api/src` finds nothing. The PIP is not shipped with the artifact;
the javadoc is what a third-party factory author reads.
**1. A natively-supplied object is used verbatim.** pip-478.md:
> A custom factory that natively supplies a richer object … has that object used verbatim — the framework
> overlays no policy on it … In particular it MUST enable client-side hostname verification unless the
> operator has explicitly opted out, MUST pin an acceptable protocol floor, and MUST NOT trust all peer
> certificates unless explicitly configured.
The framework behaviour matches: `TlsContextAcquisition` short-circuits on `direct.isPresent()` with no
overlay, and only the synthesis branch applies hostname verification, client-auth mode and the
`SSLParameters` merge. `PulsarChannelInitializer` states outright that "hostname verification is baked
into the factory-built context … so no per-connection endpoint-identification override is re-applied
here" — so there is no backstop. A KMS/HSM factory author who implements the Netty `SslContext` directly
(the natural choice, since it avoids synthesis) and omits
`SslContextBuilder.endpointIdentificationAlgorithm("HTTPS")` produces a client that completes handshakes
against any chain-valid certificate. It looks like working TLS.
**2. The metrics contract.** pip-478.md says a custom factory MUST emit `pulsar.tls.reload` and
`pulsar.tls.last_reload_success` through the `OpenTelemetry` handle on every load/reload attempt. Those
instrument names appear nowhere in the SPI module. `TlsFactoryInitContext.openTelemetry()`'s entire
javadoc is "@return the telemetry root; the framework defaults to OpenTelemetry#noop() if unset" — it
names neither instrument, nor the attributes, nor the fact that emitting them is required. Since the
reload semantics deliberately keep serving last-good material after a failed rotation, those two
instruments are the only signal that rotation has stalled; a deployment on a third-party factory loses it
and looks healthy until the certificates expire.
**3. The native-Jetty lifecycle obligations.** pip-478.md says a factory natively supplying
`SslContextFactory.Server`/`.Client` must hand it over **unstarted**, return the **same instance** per
purpose, and drive `reload(...)` on it itself. The framework depends on all three unchecked — that is
stated only in `JettyTlsFactory`'s javadoc, in `pulsar-broker-common`, an internal server module a plugin
author never compiles against. A factory returning a fresh instance per request, or never calling
`reload(...)`, pins the web listener to boot-time material forever with no error, no log and (per 2) no
metric.
**Also missing: the packaging requirement.** The well-known-class list names
`io.netty.handler.ssl.SslContext` and Jetty's `SslContextFactory` by their plain names, and the shaded v4
client artifacts relocate both (`io.netty`, `org.eclipse.jetty` → `org.apache.pulsar.shade.*`).
pip-478.md says "PIP-337 has exactly the same constraint … this PIP makes the requirement explicit" — but
the only place it is explicit is the PIP.
### Suggested fix
Add a "Custom factory obligations" block to the `PulsarTlsFactory` class javadoc (and a pointer from
`package-info`) covering: the verbatim/no-overlay rule and its three MUSTs; the two instruments, their
attributes, and that emitting them is required — cross-referenced from `openTelemetry()`, which today
gives no hint it carries a contract; the three native-Jetty obligations; and a packaging note that a
factory targeting the shaded artifacts must publish a relocated variant, while one implementing only
`javax.net.ssl.SSLContext` is shading-neutral.
Worth doing before 5.0 freezes the module, since these are the contract a factory author is held to.
---
Found in a final-state review of the PIP-478 series (#26321); each obligation was checked against the
framework code that relies on it.
Contributor guide
Assessment
This issue has not been assessed yet.