apache / apache/pulsar

[improve][misc] PIP-478: the published TLS factory SPI omits the normative custom-factory obligations

Open
#26,389 0 comments 0 reactions 1 assignee Claimed by @lhotari View on GitHub
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

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.