Skip to content

[feat][client] PIP-478: asynchronous v5 client authentication and TLS factory integration (core migration) - #26282

Open
lhotari wants to merge 20 commits into
apache:masterfrom
lhotari:lh-pip-478-core-migration-v2
Open

[feat][client] PIP-478: asynchronous v5 client authentication and TLS factory integration (core migration)#26282
lhotari wants to merge 20 commits into
apache:masterfrom
lhotari:lh-pip-478-core-migration-v2

Conversation

@lhotari

@lhotari lhotari commented Aug 7, 2026

Copy link
Copy Markdown
Member

PIP: #25890 (pip/pip-478.md)

Motivation

This is the third implementation PR for PIP-478, following #26222 (the pulsar-tls-factory-api / pulsar-http-client-api SPI modules) and #26271 (the default FileBasedTlsFactory implementation).

The two PRs so far were additive: the new SPI and its default implementation landed with no consumers, and PIP-337's PulsarSslFactory / SecurityUtility stack remained the TLS path actually in use. This PR is the migration itself — it wires the asynchronous v5 authentication interface and the PulsarTlsFactory SPI into the client, admin, broker, proxy, WebSocket proxy and functions worker, so the new SPI becomes the path that runs.

That is also what turns the engine and provider selection added in #26271 into something an operator can configure end to end, and what makes PIP-478's secure-by-default posture (hostname verification on, SAN-only matching) real.

Modifications

Asynchronous v5 client authentication.

  • pulsar-client-api-v5 gains the org.apache.pulsar.client.api.v5.auth SPI: Authentication, AuthenticationData, AuthChallenge / ChallengeResponse, the binary and HTTP challenge handlers, and the init/call contexts. Every operation that can perform I/O returns a CompletableFuture, so an authentication plugin never blocks an event-loop thread — the core motivation of the PIP.
  • pulsar-client implements it in org.apache.pulsar.client.impl.auth.v5: BinaryAuthenticationExchange (the binary-protocol challenge/response engine), HttpAuthenticationDriver + FrameworkHttpClient (the HTTP side, on the PulsarHttpClient SPI), the built-in token/basic plugins, and AuthMetrics.
  • v4 compatibility runs both ways: LegacyV4AuthenticationAdapter drives an existing v4 plugin from the v5 engine, and V5ToV4AuthenticationAdapter lets a v5 plugin serve the v4 client. Existing third-party v4 plugins keep working unchanged.

Server-side and client-side TLS factory integration.

  • Broker: PulsarService, BrokerService, PulsarChannelInitializer and WebService acquire TLS through DefaultBrokerTlsFactory / JettyTlsFactory instead of PulsarSslFactory.
  • Proxy: ProxyService, DirectProxyHandler and AdminProxyHandler (Jetty SslContextFactory.Client), plus ProxyTlsFactories for the proxy's own purposes.
  • WebSocket proxy and functions worker acquire their web-listener and broker-client material the same way.
  • Client and admin: ClientCnx, ConnectionPool, PulsarClientImpl, HttpClient and the admin HTTP connector.

Configuration surface.

  • The PIP-337 sslFactoryPlugin / sslFactoryPluginParams / brokerClientSslFactoryPlugin / brokerClientSslFactoryPluginParams config keys are removed, together with the ClientBuilder / PulsarAdminBuilder sslFactoryPlugin(...) / sslFactoryPluginParams(...) methods and the pulsar-admin clusters --tls-factory-plugin / --tls-factory-plugin-params CLI options. A stale non-default value in a config file is rejected at startup with a migration pointer rather than silently ignored. The ClusterData.brokerClientSslFactoryPlugin / ...Params accessors are the one exception: they stay on the metadata schema for wire compatibility, now @Deprecated, and a value read from the store is ignored with a WARN rather than making a cluster unloadable.
  • Their successors arrive: tlsFactoryClassName / tlsFactoryConfig (and brokerClientTlsFactoryClassName / brokerClientTlsFactoryConfig) select a PulsarTlsFactory by name, on the server configs and on the v4 client and admin builders.
  • A new jsseProvider key (client, broker, proxy, and the brokerClient* variants) names the JSSE (SSLContext) java.security.Provider. tlsProvider stays overloaded for v4 parity: a value that is not a Netty engine literal is routed to jsseProvider, which wins when both are set.
  • ClusterData gains brokerClientTlsFactoryClassName / brokerClientTlsFactoryConfig, so a deployment can drive connections to one remote cluster through its own factory. A cluster entry already carries that cluster's TLS material; these select the factory that consumes it, for both broker-client legs the entry drives — the binary replication client and the cross-cluster admin (HTTPS) client. A blank value inherits the broker-level setting rather than reverting to the default factory: the factory selects the mechanism rather than the material, so a deployment with a custom broker-client factory must not silently downgrade because a cluster entry did not repeat it. pulsar-admin clusters create|update expose them as --tls-factory-class-name / --tls-factory-config.

Default value changes (all called out in the PIP's Upgrade section):

  • Hostname verification is on by defaulttlsHostnameVerificationEnabled (broker/proxy/websocket), tlsEnableHostnameVerification (client), tlsEnableHostnameVerification (functions worker) — and CN-based matching is gone, so a server certificate must carry a matching SubjectAltName. This is the secure-by-default hardening PIP-478 exists to deliver; the remediation (reissue certificates with a SAN, or opt out per component) is documented in the PIP.
  • webServiceTlsProvider (broker and proxy) and the WebSocket proxy's tlsProvider no longer default to Conscrypt. Under PIP-337 that default only reached Jetty's SslContextFactory.setProvider(...), which is inert on a factory that overrides getSslContext() with a pre-built context — so it never actually selected a provider. This PR makes those keys authoritative on the JSSE axis, and conscrypt-openjdk-uber ships native libraries for x86_64 only, so keeping the default would fail the web listener at startup on aarch64 (Apple silicon, ARM servers) and s390x. Unset selects the JVM default, which is what deployments have effectively been running; an operator who wants Conscrypt configures it explicitly and still gets a loud failure where it cannot be loaded.

Test fixtures. tests/certificate-authority/ec/server.cert.pem and jks/broker.keystore.jks are regenerated with a localhost SubjectAltName — they previously identified the host only through the CN, which SAN-only verification rejects. generate_keystore.sh records why only the broker keystore needs a SAN (the client and proxy keystores are client identities and are never hostname-verified).

Not in this PR. The PIP-337 classes themselves (PulsarSslFactory, DefaultPulsarSslFactory, SecurityUtility, KeyStoreSSLContext) still exist, now unused by production code; deleting them, along with migrating the Athenz and SASL plugins to the v5 SPI, are the remaining PRs in the series.

Review follow-ups (since the first review)

The review on this PR found three functional regressions and a set of consistency items. All confirmed ones are fixed here, each mutation-verified:

  • Per-cluster TLS factory selection never took effect. BrokerService resolved the cluster's choice onto the configuration, but both PulsarService apply methods read ServiceConfiguration directly — so a broker-level factory silently discarded the cluster override, and a blank broker-level key left the custom factory unwrapped, making a compliant BROKER_CLIENT-only factory answer CLIENT_DEFAULT with empty and fail the connection. The class name and its config now also resolve atomically.
  • PulsarAdmin dropped an OAuth2 plugin's IdP TLS material (regression vs v4, [feat][client] oauth2 trustcerts file and timeouts #24944). The framework HTTP client factory reads conf.getTlsFactory(), which the admin path never sets, and binding it disables the StandaloneOAuth2HttpClientFactory fallback that honours idpTlsPolicy(). Such a plugin is now left unbound — which also covers a plaintext http:// admin URL.
  • The proxy bound its listeners before building its broker-client TLS, so a connection accepted in that window saw a null SslContext/factory. Master is unaffected: it held no broker-client TLS state and built per remote host lazily.
  • A 4.x-serialized AuthenticationToken stayed deserializable (the supplier move renamed classes, which Java serialization resolves by name).
  • LegacyV4TlsAdapter now starts and closes the v4 plugin it wraps.
  • The promised-but-never-emitted WARN for a stale per-cluster brokerClientSslFactoryPlugin; five pip-478.md passages that still claimed the ClusterData fields were removed; and jsseProvider documented in conf/functions_worker.yml.

One reported item is not a defect: HttpAuthenticationDriver / AsyncHttpAuthenticationProvider look unused because their implementor is the next PR in the series — AuthenticationSasl implements the provider and SaslAuthenticationV5 implements HttpAuthChallengeHandler, which the driver consumes.

Verifying this change

This change added tests and can be verified as follows:

  • New unit coverage for the v5 authentication engine (challenge/response state machines, the v4↔v5 bridges, the HTTP driver) and for the TLS wiring on each component (ProxyTlsFactoriesTest, ProxyTlsFactoryMetricsTest, DefaultBrokerTlsPolicyTest, TlsFactorySupportTest).
  • DefaultBrokerTlsPolicyTest#theDefaultConfigurationPinsNoJsseProviderOnAnyPurpose locks down the provider-default change; PerfClientUtilsTest#hostnameVerificationAloneDoesNotEnableTls locks down that hostname verification being on by default does not imply TLS intent. Both were verified to fail against the unfixed code.
  • TestCmdClusters#testTlsFactoryOptions and ReplicatorTlsFactoryTest#perClusterTlsFactorySelectsTheFactoryWhenTheBrokerLevelSettingIsUnset cover the per-cluster factory: CLI parsing through to the ClusterData sent to the admin API, and the resolution rule across cluster-only, broker-only and both-set. Both were verified to fail against the unfixed code — the both-set case is what catches inverted precedence.
  • Existing TLS/auth suites now exercise the new stack end to end, including TlsWithECCertificateFileTest, the keystore-TLS tests, TlsProducerConsumerTest, AdminApiTlsAuthTest, ReplicatorTlsTest and the proxy authentication tests.
  • Local: :pulsar-common:test :pulsar-broker-common:test :pulsar-client-original:test :pulsar-client-admin-original:test :pulsar-client-tools:test :pulsar-testclient:test :pulsar-websocket:test :pulsar-client-v5:test :pulsar-functions:pulsar-functions-worker:test — 2726 tests, 0 failures. assemble rat spotlessCheck checkstyleMain checkstyleTest checkBinaryLicense passes.
  • Full CI (Personal CI, see below) is green across all unit, integration and system suites.

Does this pull request potentially affect one of the following parts:

If the box was checked, please highlight the changes

  • Dependencies (add or upgrade a dependency)
  • The public API — adds the org.apache.pulsar.client.api.v5.auth SPI, and adds brokerClientTlsFactoryClassName / brokerClientTlsFactoryConfig to ClusterData (accessors plus builder methods); removes the v4 ClientBuilder.sslFactoryPlugin(...) / sslFactoryPluginParams(...) and the PulsarAdminBuilder equivalents (a source-compatibility break on upgrade, inventoried in the PIP). The ClusterData factory-plugin accessors are deprecated and ignored rather than removed, so no existing metadata field changes meaning.
  • The schema
  • The default values of configurations — hostname verification now defaults to on (client, broker, proxy, websocket, functions worker), and webServiceTlsProvider / the websocket tlsProvider no longer default to Conscrypt. Both are detailed under Modifications above and in the PIP's Upgrade section.
  • The threading model
  • The binary protocol
  • The REST endpoints
  • The admin CLI options — pulsar-admin clusters create|update lose the PIP-337 --tls-factory-plugin / --tls-factory-plugin-params and gain --tls-factory-class-name / --tls-factory-config, which write the new per-cluster ClusterData fields.
  • The metrics
  • Anything that affects deployment — a deployment whose server certificates carry the hostname only in the CN will fail TLS after upgrade until the certificates are reissued with a SubjectAltName, and configuration that sets a removed PIP-337 key is rejected at startup.

Documentation

  • doc-required — the configuration surface changes (new tlsFactoryClassName / tlsFactoryConfig / jsseProvider keys, removed sslFactoryPlugin* keys) and the secure-by-default hostname-verification behaviour need to be reflected in the security documentation on the website.
  • doc-not-needed
  • doc
  • doc-complete

Matching PR in forked repository

PR in forked repository: lhotari#249 (Personal CI — full matrix green)

This PR was prepared with the assistance of Claude Code (Opus 5); the change was reviewed and is submitted by a human contributor who takes responsibility for it, per the ASF Generative Tooling guidance.

lhotari added 6 commits August 7, 2026 09:25
… + server-side TLS factory integration (core migration)

This is the core of PIP-478: migrate the client and server to the asynchronous v5
authentication interface and the pluggable server-side TLS factory. The legacy PIP-337
PulsarSslFactory and SecurityUtility code paths are replaced throughout client, admin,
broker, proxy, websocket and functions-worker, with secure-by-default, SAN-only hostname
matching.

Rebased onto the reworked default TLS factory (apache#26271) and current master.

Assisted-by: Claude Code (Opus 5)
…tory API

The default TLS factory was reworked during the review of apache#26271, so two of the
APIs this migration calls have moved:

- JettyTlsFactory's two public builders now require an Executor for the rotation reload, so
  that it never runs inline on the factory's delivery thread. Each of the five call sites
  passes the executor that component already owns for TLS work: the broker's shared executor
  (WebService), the websocket / functions-worker scheduled executor, the proxy web server's
  dedicated TLS refresh executor, and the admin handler's SSL refresher.
- TlsContextAcquisition's HTTP rotation connection-TTL constant and system property were
  dropped from apache#26271 as unused; their consumers (HttpClient and the v5
  FrameworkHttpClientFactory) arrive here, so they are restored with this change.

Assisted-by: Claude Code (Opus 5)
Under PIP-337 the web-service provider keys only reached Jetty's
SslContextFactory.setProvider(...), which is inert on a factory that overrides
getSslContext() with a pre-built context — so the shipped Conscrypt default never actually
selected a provider. PIP-478 routes those keys onto TlsPolicy.jsseProvider, where a
configured name is pinned and an unresolvable one fails startup. That makes the default
real for the first time.

conscrypt-openjdk-uber ships native libraries for x86_64 only (linux, macOS, Windows), so
with the default in force the broker, proxy and websocket web listeners fail to start on
aarch64 (Apple silicon, ARM servers) and s390x:

    No java.security.Provider named 'Conscrypt' could be resolved ...
        at JcaProviders.resolveNamedProvider
        at TlsContexts.buildJdkContext
        at WebService.<init>

Reproduced locally on macOS/aarch64 by AdminProxyHandlerKeystoreTLSTest and
ProxyAuthenticatedProducerConsumerTest; linux-x86_64 CI cannot catch it.

Drop the Conscrypt default from ServiceConfiguration.webServiceTlsProvider,
ProxyConfiguration.webServiceTlsProvider and WebSocketProxyConfiguration.tlsProvider, and
from broker.conf, standalone.conf, proxy.conf, websocket.conf and functions_worker.yml.
Unset means the JVM default, which is what deployments have effectively been running all
along; an operator who wants Conscrypt still configures it explicitly and still gets the
loud failure when it cannot be resolved.

DefaultBrokerTlsPolicyTest#theDefaultConfigurationPinsNoJsseProviderOnAnyPurpose locks this
down and is mutation-verified: restoring the Conscrypt default fails it.

Assisted-by: Claude Code (Opus 5)
…ts are removed

The Configuration "Defaults" note still described webServiceTlsProvider and the WebSocket
proxy's tlsProvider as shipping Conscrypt. Making those keys authoritative on the JSSE axis
is what forced the defaults to be dropped, so state that, with the reason (x86_64-only
natives) and why it is operationally a no-op (the defaults were inert under PIP-337). Add
the matching entry to the behaviour-change list.

Assisted-by: Claude Code (Opus 5)
TLS hostname verification is on by default in 5.0 and CN-based matching is removed, so a test
server certificate that names its host only in the CN is now rejected with "No subject
alternative DNS name matching localhost found".

Two fixtures still had that shape:

- tests/certificate-authority/ec/server.cert.pem (and the matching JKS) carried
  DNS:pulsar, DNS:pulsar.default, IP:127.0.0.1, IP:192.168.1.2 — no localhost — which failed
  TlsWithECCertificateFileTest in CI (Broker Group 2).
- tests/certificate-authority/jks/broker.keystore.jks carried CN=localhost and no SAN at all.

Regenerate both with a localhost SAN, and record in generate_keystore.sh why the broker
keystore needs one while the client and proxy keystores deliberately do not: only the broker
cert is presented as a TLS server certificate, and only server certificates are
hostname-verified.

These fixtures were regenerated in the branch behind apache#26271 but left out of that
PR, since nothing there wires the factory in and the old certificates still passed. This is
the change that makes hostname verification live, so they belong here.

Assisted-by: Claude Code (Opus 5)
…ame verification is on

conf/client.conf now ships tlsEnableHostnameVerification=true (hostname verification is on by
default since 5.0), and PerformanceBaseArguments resolves that key through picocli's
descriptionKey. Every pulsar-perf invocation in a distribution therefore sees
tlsHostnameVerificationEnable=TRUE, which the V5 client-builder helper read as "the user wants
TLS" and answered by wiring a TlsPolicy — and PulsarClientBuilderV5#tlsPolicy unconditionally
flips useTls=true.

The result is a TLS handshake against a plaintext pulsar:// endpoint, which the broker closes:

    WARN  PulsarDecoder - TLS handshake failed ... SslHandshakeCompletionEvent(
          StacklessClosedChannelException)
    Suppressed: StacklessSSLHandshakeException: Connection closed while SSL/TLS handshake was
               in progress

This is what failed CI - Integration - Cli (PerfToolTest.testConsume); it reproduced on both
runs of this content.

Hostname verification can no longer signal intent now that it is the default, so drop it from
the test. The remaining signals — a pulsar+ssl:// URL, an explicit trust-cert path, or
tlsAllowInsecureConnection=TRUE — all still mean what they did. The flag continues to configure
the policy once TLS is on for one of those reasons.

The decision moves into a package-private wantsTls() so it can be asserted directly;
PerfClientUtilsTest#hostnameVerificationAloneDoesNotEnableTls is mutation-verified (restoring
the hostname-verification signal fails it).

Assisted-by: Claude Code (Opus 5)
A ClusterData entry configures the broker's outbound connections to one remote cluster, and it
already carries that cluster's own TLS material (brokerClientTls*). It had no way to name the
PulsarTlsFactory that consumes it: factory selection was broker-level only, so a deployment
could not drive connections to one remote cluster through a different factory — an HSM- or
KMS-backed one, say — while the rest of the cluster used the default.

Add brokerClientTlsFactoryClassName and brokerClientTlsFactoryConfig to ClusterData, and apply
them to both broker-client legs a cluster entry drives: the binary-protocol replication client
(configTlsSettings) and the cross-cluster admin HTTPS client (configAdminTlsSettings). They are
the successors of the deprecated brokerClientSslFactoryPlugin / ...Params, whose javadoc now
points at them.

A blank per-cluster value inherits the broker-level brokerClientTlsFactoryClassName /
brokerClientTlsFactoryConfig rather than falling back to the default factory. That differs from
the TLS material in a cluster entry, which is taken wholesale, and the asymmetry is deliberate:
the factory selects the mechanism that loads material rather than the material itself, so a
deployment with a custom broker-client factory must not silently revert to the file-based
default just because a cluster entry did not repeat the setting. It is the same reasoning that
already keeps brokerClientSslProvider / brokerClientJsseProvider broker-level in these helpers —
a silent downgrade of the TLS mechanism is a security regression.

pulsar-admin clusters create/update gain --tls-factory-class-name and --tls-factory-config,
replacing the removed PIP-337 --tls-factory-plugin flags.

Both tests are mutation-verified: TestCmdClusters#testTlsFactoryOptions fails when the builder
wiring is dropped, and ReplicatorTlsFactoryTest#perClusterTlsFactorySelects... covers
cluster-only, broker-only and both-set, the last of which fails when precedence is inverted.

Assisted-by: Claude Code (Opus 5)
@lhotari
lhotari marked this pull request as draft August 7, 2026 08:50
lhotari added 2 commits August 7, 2026 13:42
…ient

Preparation for making the v5 authentication model the client's native driver. The bridge that
adapts a legacy v4 plugin into the v5 SPI lives in pulsar-client-v5, which depends on
pulsar-client — so the v4 client cannot reach it. Move it to where the v4 client can:

  pulsar-client-v5  org.apache.pulsar.client.impl.v5.auth.LegacyV4AuthenticationAdapter
  pulsar-client-v5  org.apache.pulsar.client.impl.v5.auth.TlsAuthentication
  pulsar-client-v5  org.apache.pulsar.client.impl.v5.V5AuthenticationLoader
    ->  pulsar-client  org.apache.pulsar.client.impl.auth.v5.*

V5AuthenticationLoader and its create(...) overloads widen to public, since callers are now in
another module. No behaviour changes.

Two adjustments the move forces:

- LegacyV4AuthenticationAdapter logged through slf4j, which is not on pulsar-client's main
  compile classpath; it now uses slog (@CustomLog) like the rest of the module.
- The relocated tests used SimpleAuthInitContext / SimpleAuthCallContext, which are
  package-private in pulsar-client-v5. They now build the same contexts through the public
  V5AuthContexts factory. V5AuthenticationLoaderTest stays in pulsar-client-v5 because it
  exercises V5ToV4AuthenticationAdapter, which does not move.

Assisted-by: Claude Code (Opus 5)
…en parameters

Preparation for resolving authPluginClassName straight to a v5 plugin: the v5 token body could
only be constructed from a Supplier<String>, so a client configured with
authPluginClassName=...AuthenticationToken had no way to reach it by name.

TokenAuthenticationV5 gains fromParams(String) and the tokenSupplier(String) parser behind it,
accepting exactly the forms the v4 plugin accepted — token:<literal>, file:<uri>, a JSON object
with a token member, or the bare token. The two serializable suppliers move onto it as
LiteralTokenSupplier / FileTokenSupplier.

The v4 AuthenticationToken now delegates its configure(String) to that parser rather than
keeping a second copy, so the by-name path and the v4 shim cannot drift apart.

Assisted-by: Claude Code (Opus 5)

@david-streamlio david-streamlio left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Reviewed the full diff against bab5c77631 (215 files) in a worktree at d4ce308dc9; ./gradlew quickCheck passes. This is an impressively disciplined migration — the offload discipline, the ctor-throw cleanup on the TLS initializers, the use-after-free pinning around newHandler/newEngine, and the generation/liveness guards in the ClientCnx state machine all read as carefully thought through. The notes below are what I found; the first three look like genuine functional regressions, the rest are consistency/robustness/doc items.

Per-cluster TLS factory selection does not reach the cross-cluster admin client

The PR describes ClusterData.brokerClientTlsFactoryClassName as selecting the factory "for both broker-client legs the entry drives — the binary replication client and the cross-cluster admin (HTTPS) client." The admin leg does not honour it.

In BrokerService.getClusterPulsarAdmin, configAdminTlsSettings(...) writes the resolved (cluster-preferred) value onto the builder's conf via adminBuilder.tlsFactoryClassName(...) / .tlsFactoryConfig(...). Then BrokerService.java:1918 calls pulsar.applyBrokerClientTlsFactoryToAdmin(builder), which at PulsarService.java:1915 is gated on — and parameterised by — the broker-level getConfiguration().getBrokerClientTlsFactoryClassName():

  • broker-level set, cluster overrides itconf.setTlsFactory(ClientTlsFactorySupport.brokerClientTlsFactory(conf, /* broker-level */ factoryClassName)) and conf.setTlsFactoryParams(parseFactoryConfig(/* broker-level */ ...)). resolveClientTlsFactory short-circuits on a non-null conf.getTlsFactory(), so the per-cluster name and config that configAdminTlsSettings just wrote are never read. The cluster override is silently dropped.
  • broker-level blank, cluster names a factory → the method returns early, and AsyncHttpConnector later resolves by name through resolveClientTlsFactory(conf, ..., /* brokerClientPurpose */ false). So the custom factory is not wrapped in BrokerClientPurposeFactory. By the reasoning in ClientTlsFactorySupport#wrapBrokerClientPurpose, a compliant custom factory serving only BROKER_CLIENT then returns empty() for the transport's CLIENT_DEFAULT request — a hard connect failure.

ReplicatorTlsFactoryTest#perClusterTlsFactorySelectsTheFactoryWhenTheBrokerLevelSettingIsUnset is a good test, but it only exercises getReplicationClient; nothing covers getClusterPulsarAdmin, which is why this is invisible in CI.

Admin-only OAuth2 loses its IdP TLS material

PulsarAdminImpl.java:466 builds the FrameworkHttpClientFactory with conf::getTlsFactory as the TLS supplier. Nothing ever sets a factory on that conf on the admin path: AsyncHttpConnector.resolveNewTlsFactory keeps its factory in its own field rather than on the conf, and it runs after auth.start() anyway. So hasTlsFactory() is false, FrameworkHttpClientFactory.configureTls takes the legacy branch, and because the purpose is CLIENT_OAUTH2 rather than CLIENT_DEFAULT it applies nothing — the IdP connection falls back to the platform default trust store.

Binding a factory at all is also what disables the fallback that would have handled this: FlowBase.resolveHttpClientFactory() only reaches StandaloneOAuth2HttpClientFactory (the one place idpTlsPolicy() is consumed) when httpClientFactory == null.

Net effect: a PulsarAdmin using OAuth2 with trustCertsFilePath / tlsCertFile / tlsKeyFile in its authParams silently stops honouring them. v4 honoured them via FlowBase.defaultHttpClient, and PulsarClientImpl explicitly preserves the behaviour through hasOAuth2IdpTlsMaterial() / foldOAuth2IdpPolicy — the admin path has no equivalent. AdminOnlyOAuth2AuthTest uses a plaintext mock IdP, and none of OAuth2IdpTlsFoldTest / OAuth2IdpTlsFrameworkClientTest / OAuth2IdpTlsPlaintextBrokerTest involves PulsarAdmin.

tlsCertFile / tlsKeyFile pairing is no longer validated

The removed FlowBase.defaultHttpClient threw on a half-configured pair:

if (hasCertFile != hasKeyFile) {
    throw new IllegalArgumentException("Invalid TLS client certificate configuration: "
        + CONFIG_PARAM_CERT_FILE + " and " + CONFIG_PARAM_TLS_KEY_FILE + " must be provided together");
}

FlowBase.idpTlsPolicy() (line 154) replaces it with if (isNotBlank(certFile) && isNotBlank(keyFile)), and hasOwnTlsMaterial() returns true when either is set. A config with only one of the two now yields a policy with no client identity and no error — the mTLS handshake to the IdP silently degrades instead of failing at startup. Nothing else in the tree validates the pair (TlsClientAuthFlow.fromParameters doesn't either, and TlsClientAuthFlowTest lost its factory-mocking wrapper).

tlsCertRefreshCheckDurationSec=0 now means two different things

DefaultBrokerTlsFactory.refreshIntervalSeconds maps <= 0 to 0, with a deliberate comment: "an operator who set 0 still gets no poll." The other three components do the opposite:

  • ProxyTlsFactories.java:144DEFAULT_REFRESH_INTERVAL_SECONDS (60)
  • ProxyServer.buildDefaultWebTlsFactory (websocket) → same
  • WorkerServer.java:392 → same

Under PIP-337 all three gated the refresh task on > 0, so 0 disabled polling everywhere. This is both a behaviour change and an inconsistency with the broker's documented handling of the same key.

Proxy startup race: listeners bind before the broker-client TLS is built

ProxyService.start() binds listenChannel and listenChannelTls and only then, at line 344, builds brokerClientTlsFactory / brokerClientSslContext / lookupClientTlsFactory. A connection accepted in that window hits either TlsContextAcquisition.withPinnedContext(service::getBrokerClientSslContext, ...) in DirectProxyHandler with a null context, or a null conf.getTlsFactory() from ProxyConnection.createClientConfiguration — the javadoc on getLookupClientTlsFactory() already acknowledges it is "transiently null while it is being built at startup". Under PIP-337 DirectProxyHandler built its factory lazily per remote host, so no such window existed. Moving the isTlsEnabledWithBroker() block above the binds closes it.

Per-cluster factory class and its config fall back independently

BrokerService.java:1756 resolveBrokerClientTlsFactory is applied separately to ...ClassName and ...Config. A ClusterData that sets only the class name therefore inherits the broker-level brokerClientTlsFactoryConfig — factory A's init params handed to factory B. Given the reasoning in the method's own javadoc (don't silently change the TLS mechanism), the pair should probably resolve atomically: if the cluster supplies a class name, its config wins even when blank.

The documented WARN is never emitted

ClusterData#getBrokerClientSslFactoryPlugin, the ClusterDataImpl @Schema descriptions, the PR body, and pip-478.md §1192 all say a stale value read from the store is "ignored with a WARN". Grepping every non-test main source for sslFactoryPlugin turns up only field declarations and comments — nothing logs. Since this is explicitly called out as the one place a stale PIP-337 value cannot fail loud, the WARN is the whole remediation signal.

pip-478.md contradicts itself and this PR

§1238 and §1241 still state that the ClusterData factory fields are removed from the metadata model along with their accessors and builder methods, that the pulsar-admin clusters options are removed, and that "per-cluster factory selection no longer exists — set the factory broker-level." §1192 and this PR do the opposite: deprecate-and-retain the old fields and add per-cluster selection plus --tls-factory-class-name / --tls-factory-config. The PIP is the normative spec and this PR already touches it, so those two sections should be updated in the same change.

Smaller items

  • ProxyServer.createTlsFactoryWebServer (line 239) leaks on partial init failure. WebService.createTlsFactoryWebServer, ServiceChannelInitializer.initializeTlsFactory and the broker's PulsarChannelInitializer.initializeTlsFactory all wrap the post-createFactory steps in a try/catch that disposes the subscription and closes the factory. ProxyServer doesn't, and its caller rethrows as PulsarServerException without ever reaching close(), so the cert watchers survive a failed startup.
  • The v4 auth shims rebuild the driver per connection attempt. AuthenticationToken, AuthenticationBasic and AuthenticationOAuth2 each do new V5BinaryAuthenticationDriver(new XxxV5(...), authServices).newAuthenticationExchange(host) inside newAuthenticationExchange. That constructs a fresh AuthMetrics — two OpenTelemetry instrument builds — and re-runs ensureInitialized()'s initializeAsync().join() on every connection attempt, including reconnect storms. HttpAuthenticationDriver's own javadoc states the intended shape ("One instance is created per plugin and reused across requests"); caching the driver in a field would match it.
  • ensureInitialized() joins on the calling thread in both V5BinaryAuthenticationDriver and HttpAuthenticationDriver. The comments justify it by "the built-in bodies complete immediately", which holds for the built-ins, but nothing in the Authentication SPI javadoc makes that a contract — a third-party v5 plugin doing I/O in initializeAsync would block whatever thread first opens an exchange, which is the hazard the PIP exists to remove. Worth either stating it as a hard contract on initializeAsync or hoisting init off the call path.
  • LegacyV4AuthenticationAdapter.wrap routes by auth-method name. Only the literal "sasl" gets LegacyV4ChallengeResponseAdapter; every other name falls to LegacyV4CredentialAdapter, which does not expose BinaryAuthChallengeHandler. A third-party v4 multi-round plugin under a different method name will fail the first CommandAuthChallenge with "does not expose BinaryAuthChallengeHandler". Separately, LegacyV4TlsAdapter forwards configure(...) but never v4.start() or v4.close(), so a custom plugin reporting method name "tls" is neither started nor closed.
  • LegacyV4CredentialAdapter.getHttpHeadersAsync calls the deprecated no-arg v4.getAuthData() even though the HttpAuthCallContext carries requestUri(), dropping per-host credential selection.
  • Per-host client TLS material. The old client keyed a PulsarSslFactory per SNI host and fed it getAuthData(host); DirectProxyHandler did the same per remote host. Both now share one factory/context with only a TlsEndpoint hint. Is dropping per-host client identity intentional? If so it's probably worth a line in the PIP's upgrade notes.
  • newConnectCommand()buildConnectCommand(AuthData) silently breaks out-of-tree ClientCnx subclasses (all in-tree callers are updated). A deliberate 5.0 break, but it isn't in the PR's public-API inventory.
  • PulsarClientProvider.authenticationTls(String, String)authenticationTls() is a v5 public-API signature change not called out under Modifications.
  • jsseProvider is missing from conf/functions_worker.yml even though WorkerConfig gained the field and broker/proxy/standalone/client all document it.
  • Deleted tests for still-present classes. KeyStoreTlsTest, JettySslContextFactoryTest and JettySslContextFactoryWithKeyStoreTest are removed here, but the PIP-337 classes they cover (KeyStoreSSLContext, and JettySslContextFactory — that one is deleted, so its tests going is fine) remain in the tree until a later PR. Was there a reason not to keep KeyStoreTlsTest until KeyStoreSSLContext itself goes?

lhotari added a commit to lhotari/pulsar that referenced this pull request Aug 7, 2026
…ually take effect

Review of apache#26282 found that ClusterData.brokerClientTlsFactoryClassName never
reached the factory that gets built, in either direction:

- With a broker-level factory configured, PulsarService.maybeApplyBrokerClientTlsFactory and
  applyBrokerClientTlsFactoryToAdmin both read ServiceConfiguration directly, built the
  broker-level factory, and left conf.tlsFactory non-null — so the cluster's selection, which
  BrokerService had already resolved onto the configuration, was silently discarded.
- With the broker-level key blank, both methods returned early. The cluster's class name then
  rode the config into resolveClientTlsFactory with brokerClientPurpose=false, so the custom
  factory was never wrapped in BrokerClientPurposeFactory. A compliant BROKER_CLIENT-only
  factory answers the transport's CLIENT_DEFAULT request with empty, failing the connection.

Both apply methods now read the resolved selection off the configuration the caller populated,
falling back to the broker-level key, so the cluster value both wins and is purpose-wrapped.

The class name and its configuration also resolve atomically now: a cluster that overrides only
the class name previously inherited the broker-level brokerClientTlsFactoryConfig, handing
factory A's init parameters to factory B.

Also emit the WARN that ClusterData's deprecation javadoc, the @Schema descriptions and
pip-478.md all promise for a stale PIP-337 brokerClientSslFactoryPlugin read from the store —
nothing logged it, and it is the whole remediation signal for the one stale value that cannot
fail loud. It fires once per cluster and names it.

ReplicatorTlsFactoryTest now asserts a factory is actually built, not just that the name reaches
the config, and covers getClusterPulsarAdmin — the leg nothing exercised, which is why this was
invisible in CI. Mutation-verified: restoring either method's broker-level read fails them.

Assisted-by: Claude Code (Opus 5)
lhotari added 4 commits August 7, 2026 20:30
…ually take effect

Review of apache#26282 found that ClusterData.brokerClientTlsFactoryClassName never
reached the factory that gets built, in either direction:

- With a broker-level factory configured, PulsarService.maybeApplyBrokerClientTlsFactory and
  applyBrokerClientTlsFactoryToAdmin both read ServiceConfiguration directly, built the
  broker-level factory, and left conf.tlsFactory non-null — so the cluster's selection, which
  BrokerService had already resolved onto the configuration, was silently discarded.
- With the broker-level key blank, both methods returned early. The cluster's class name then
  rode the config into resolveClientTlsFactory with brokerClientPurpose=false, so the custom
  factory was never wrapped in BrokerClientPurposeFactory. A compliant BROKER_CLIENT-only
  factory answers the transport's CLIENT_DEFAULT request with empty, failing the connection.

Both apply methods now read the resolved selection off the configuration the caller populated,
falling back to the broker-level key, so the cluster value both wins and is purpose-wrapped.

The class name and its configuration also resolve atomically now: a cluster that overrides only
the class name previously inherited the broker-level brokerClientTlsFactoryConfig, handing
factory A's init parameters to factory B.

Also emit the WARN that ClusterData's deprecation javadoc, the @Schema descriptions and
pip-478.md all promise for a stale PIP-337 brokerClientSslFactoryPlugin read from the store —
nothing logged it, and it is the whole remediation signal for the one stale value that cannot
fail loud. It fires once per cluster and names it.

ReplicatorTlsFactoryTest now asserts a factory is actually built, not just that the name reaches
the config, and covers getClusterPulsarAdmin — the leg nothing exercised, which is why this was
invisible in CI. Mutation-verified: restoring either method's broker-level read fails them.

Two documentation fixes ride along, since they describe this same disposition:

- pip-478.md contradicted itself and this PR on whether the PIP-337 ClusterData fields are
  removed. Five passages said removed — the breaking-change bullet, the "one silently-dropped
  case" bullet, the geo-replication note (which explained compatibility through Jackson
  lenient-dropping an unknown property, impossible for a retained field), the removal list's
  "nothing PIP-337 is retained (no @deprecated fields)", and the implementation-approach
  paragraph. All now describe the retained-and-ignored disposition, the successor fields, and
  the WARN that replaces lenient-dropping as the remediation signal.
- conf/functions_worker.yml gains the jsseProvider documentation it was missing; WorkerConfig has
  the field and broker.conf, proxy.conf, standalone.conf and client.conf all document it.

Assisted-by: Claude Code (Opus 5)
…rializable

Moving the token suppliers onto TokenAuthenticationV5 renamed them from
AuthenticationToken$SerializableTokenSupplier / $SerializableURITokenSupplier to
TokenAuthenticationV5$LiteralTokenSupplier / $FileTokenSupplier. Java serialization resolves by
class name, so carrying the original serialVersionUID across the move does nothing: an
AuthenticationToken serialized by Pulsar 4.x fails to deserialize on 5.0 with
ClassNotFoundException. The v4 Authentication interface extends Serializable, so that is a
compatibility break.

Nothing in-tree Java-serializes an Authentication — Functions and connectors carry
authPluginClassName and authParams as strings — but the interface's Serializable contract is
public, so retain the two 4.x inner classes as deserialization shims with their original UIDs.
Each readResolve()s into the corresponding v5 supplier, so a restored plugin holds the new type
and nothing downstream keeps the deprecated shim alive.

AuthenticationTokenSerializationCompatTest reconstructs the 4.x wire form and round-trips it;
mutation-verified by deleting the shim, which fails it.

Assisted-by: Claude Code (Opus 5)
…P TLS material

A PulsarAdmin using OAuth2 with trustCertsFilePath / tlsCertFile / tlsKeyFile in its authParams
silently stopped honouring them: the IdP connection fell back to the platform default trust
store with no client certificate. v4 honoured them (issue apache#24944), and PulsarClientImpl
preserves the behaviour through hasOAuth2IdpTlsMaterial() / foldOAuth2IdpPolicy — the admin path
had no equivalent.

Two things combine to lose it. The framework HTTP client factory the admin binds reads its TLS
material from conf.getTlsFactory(), and nothing on the admin path ever sets one — AsyncHttpConnector
keeps the factory it resolves in its own field, and is built after auth.start() in any case — so
the factory takes its legacy branch, which applies material only for CLIENT_DEFAULT while OAuth2
requests CLIENT_OAUTH2. And binding a factory at all is what disables the path that does work:
FlowBase falls back to StandaloneOAuth2HttpClientFactory, the sole consumer of idpTlsPolicy(),
only when no factory is bound.

So for exactly this case — an OAuth2 plugin carrying IdP TLS material, with no client TLS factory
available to serve it — the admin now leaves the plugin unbound and lets the flow self-provision
its IdP-aware HTTP client. This also covers a plaintext http:// admin URL, where no client TLS
factory is ever resolved; reordering admin construction to publish a factory would not have.

The decision is a package-private static so it can be asserted directly rather than through the
admin constructor. AdminOAuth2IdpTlsBindingTest covers custom trust, mTLS material, no material,
and a non-OAuth2 plugin; mutation-verified by neutralising the guard.

Assisted-by: Claude Code (Opus 5)
…ing its listeners

ProxyService.start() bound listenChannel and listenChannelTls and only afterwards built
brokerClientTlsFactory, brokerClientSslContext and lookupClientTlsFactory. A connection accepted
in that window reached DirectProxyHandler with a null brokerClientSslContext, or took a null
factory from ProxyConnection.createClientConfiguration.

The window is new in this series: under PIP-337 the proxy held no broker-client TLS state at all
(ProxyService had no SslFactory field) and DirectProxyHandler built its factory lazily per remote
host, keyed in a Map<String, PulsarSslFactory>, so nothing could observe a half-built state.
Master is unaffected.

Move the whole isTlsEnabledWithBroker() block above the binds. serviceUrl / serviceUrlTls are
computed after the binds because they need the actual bound port, so the representative
ClientConfigurationData built here now sees them null — which is safe and already documented:
createClientConfiguration's javadoc says that config is read only for its tls* fields, and
ClientTlsFactorySupport never looks at the service URL. Noted at the call site so the coupling is
not reintroduced.

Also make LegacyV4TlsAdapter start and close the plugin it wraps. It forwarded configure(...)
only, so a custom v4 plugin reporting the "tls" method name never had start() called — any
credential loading it does there simply did not happen — and never had close() called, leaking
whatever it held. The built-in AuthenticationTls has an empty start(), which is why this went
unnoticed. Mutation-verified by dropping the forwarding.

Assisted-by: Claude Code (Opus 5)
}
if (policy.keyFilePath() != null) {
conf.setTlsKeyFilePath(policy.keyFilePath());
conf.setUseTls(true);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

It looks like an IdP-only policy also enables broker TLS here. This overload allows CLIENT_OAUTH2 to configure a separate IdP trust domain, while useTls controls the binary broker transport. With serviceUrl("pulsar://...").tlsPolicy(CLIENT_OAUTH2, policy), the connection pool will therefore try to use TLS against a plaintext broker. Keeping useTls limited to a broker-transport purpose such as CLIENT_DEFAULT, while separately using a purpose-specific policy map to trigger TLS factory creation, would preserve the plaintext-broker case. A v5 builder test covering CLIENT_OAUTH2 with a pulsar:// service URL would also help protect this behavior.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Correct, and your framing is the one I've used in the fix: useTls governs the binary broker transport, while CLIENT_OAUTH2 describes a different trust domain. Setting only an IdP policy against a pulsar:// URL was turning broker TLS on, and the connection pool then attempted TLS against a plaintext port.

Fixed in eb73429: useTls is set only for a broker-transport purpose. The policy map is what triggers TLS-factory creation, so a non-transport purpose still gets its factory without touching the transport — which is the separation you described.

Your suggested test is in as anIdpOnlyPolicyDoesNotEnableBrokerTls (pulsar:// service URL + CLIENT_OAUTH2 policy, asserting useTls stays false and the policy is still registered), with a companion asserting a CLIENT_DEFAULT policy does still enable it. Mutation-verified: restoring the unconditional setUseTls(true) fails the first test.

This also turned out to interact with the admin-side OAuth2 work in b5f23da, which now folds the IdP policy into the client's own factory rather than letting the plugin self-provision one.

public static SslProvider engineProvider(String sslProvider) {
if (sslProvider != null) {
String p = sslProvider.trim();
if ("OPENSSL".equalsIgnoreCase(p) || "OPENSSL_REFCNT".equalsIgnoreCase(p)) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

It looks like this mapping loses the distinction between OPENSSL and OPENSSL_REFCNT: an explicit OPENSSL_REFCNT setting is returned as OPENSSL, and an unset provider falls through to JDK. Previously an unset client provider was passed to Netty, which selected the native provider when available, so this also changes the historical default. The server-side TlsFactorySupport.engineProvider in this PR already preserves OPENSSL_REFCNT and selects it by default when OpenSSL is available. Aligning the client mapping with that behavior, with coverage for the explicit and unset cases, would keep the two paths consistent.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Makes sense.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Correct on both halves, and fixed in 7a85b02.

The OPENSSL_REFCNTOPENSSL collapse matters more than it looks: only the reference-counted variant is free of the finalize() that JEP 421 deprecates, so an operator explicitly asking for it was silently given the engine that leaks its native SSL_CTX under --finalization=disabled — the exact thing this series moved the server side onto OPENSSL_REFCNT to avoid.

And you're right that the unset case was a behaviour change, not just an inconsistency. The PIP-337 client passed a null provider to SslContextBuilder, which then selected the native engine wherever a netty-tcnative binary existed, so returning JDK moved every client with no explicit provider off the native engine.

The client mapping is now identical to TlsFactorySupport.engineProvider: engine literals verbatim, any other non-blank value treated as a JSSE provider name (so it selects no native engine), unset → OPENSSL_REFCNT when OpenSsl.isAvailable(). ClientEngineProviderTest covers literals, JSSE names, and unset in both availability states.

@david-streamlio david-streamlio left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Re-reviewed at d0c4577466 against my round-1 notes on d4ce308dc9 — six commits, 23 files, +634/−124. All 45 CI checks green.

Where I make a claim below I've tried to give you a way to falsify it in seconds rather than take my word for it: a grep that shows the contradiction, or an actual run. Where something is a multi-file trace rather than a direct observation, I say so.

Thanks for the turnaround. Confirming what's genuinely fixed, because it is:

  • Per-cluster factory selection works now on both legs and in both directions. I traced broker-level A + cluster B through configAdminTlsSettingsapplyBrokerClientTlsFactoryToAdminbrokerClientTlsFactorySelection; the admin client ends up with B, and with the broker level blank the cluster's factory is purpose-wrapped rather than handed to the transport raw.
  • Proxy startup ordering is a pure relocation. I diffed the moved lines with comments stripped — character-identical, so nothing was quietly edited under cover of the move. The non-null guarantee on brokerClientSslContext rests on the documented happens-before at PulsarTlsFactory.java:99, not on luck.
  • AuthenticationToken serialization was your catch, not mine, and a good one. I diffed every SUID and field descriptor against f419587445: a genuine 4.x blob will deserialize.
  • TokenAuthenticationV5's v4 parameter parsing is byte-faithful, including the narrow catch (JsonSyntaxException). Routing the shim through one implementation removes the duplicate-parser drift risk.
  • The stale-value WARN is on the metadata-read path where it needed to be, and the pip-478.md contradictions about ClusterData are resolved.

Two corrections to my own round-1 review first.

I was wrong about the deleted tests. JettySslContextFactoryTest / JettySslContextFactoryWithKeyStoreTest cover JettySslContextFactory, which this PR deletes — removing them is correct. And KeyStoreSSLContext is not left uncovered: SslContextTest drives it through DefaultPulsarSslFactory, plus AdminApiKeyStoreTlsAuthTest at integration level. Only KeyStoreTlsTest touched a still-shipping class, and its removal narrows rather than eliminates coverage. Withdrawn.

Also ProxyService.getLookupClientTlsFactory()'s "transiently null" javadoc is still accurateProxyConnection.createClientConfiguration genuinely reads it null from the self-call at ProxyService.java:331. I nearly filed it as stale. Worth a clarifying clause so it doesn't read like the race you just closed, but it isn't wrong.


Before merge

1. The admin OAuth2 fix leaves the original defect live on the broker / worker path

The mainstream case is fixed and the lifecycle is clean. But the guard is the wrong predicate:

// PulsarAdminImpl.java:490
return conf.getTlsFactory() == null && auth instanceof AuthenticationOAuth2 oauth2 && oauth2.idpTlsPolicy().isPresent();

conf.getTlsFactory() != null is standing in for "a preset factory can serve the IdP material". It can't:

$ grep -n 'new FileBasedTlsFactory(Map.of(TlsPurpose.CLIENT_DEFAULT' \
    pulsar-client/src/main/java/org/apache/pulsar/client/impl/tls/ClientTlsFactorySupport.java
384:        return new FileBasedTlsFactory(Map.of(TlsPurpose.CLIENT_DEFAULT, policy), settings(conf), authSuppliers);

$ grep -n -A2 'if (purpose.role() == TlsPurpose.Role.CLIENT)' \
    pulsar-common/src/main/java/org/apache/pulsar/common/tls/impl/FileBasedTlsFactory.java
417:        if (purpose.role() == TlsPurpose.Role.CLIENT) {
418-            return systemDefaultSource();

PulsarService.applyBrokerClientTlsFactoryToAdmin and WorkerUtils.applyBrokerClientTlsFactoryToAdmin set a factory on the admin builder's conf before build; that factory registers only CLIENT_DEFAULT; so the guard returns false, the framework factory binds, configureTls asks for CLIENT_OAUTH2, and resolve silently hands back the system default. A broker or functions worker with brokerClientTlsFactoryClassName set, an https:// admin URL and an OAuth2 plugin carrying trustCertsFilePath still falls back to the platform trust store — silently. The v5-adopted custom-factory path has the same hole, since resolveClientTlsFactory adopts conf.getTlsFactory() verbatim without folding.

The predicate probably wants to be "the bound factory does not serve CLIENT_OAUTH2".

(This one is a trace across four files rather than a single observable — the two greps above are the load-bearing links, the rest is following the call chain. Worth confirming independently.)

Smaller, same line: the guard ignores conf.getTlsPolicyMap(), so an explicitly-supplied CLIENT_OAUTH2 policy is discarded in favour of the auth-params-derived one, inverting the putIfAbsent precedence composePolicies establishes.

2. The same fix drops SOCKS5 for IdP traffic

StandaloneOAuth2HttpClientFactory.java:85 builds its delegate with a fresh, empty config:

this.delegate = new FrameworkHttpClientFactory(() -> null, () -> null, () -> null,
        () -> factoryForSupplier, new ClientConfigurationData(), instanceId);

Three lines establish the consequence:

$ grep -n 'setSocks5ProxyScope' pulsar-client-admin/src/main/java/org/apache/pulsar/client/admin/internal/PulsarAdminBuilderImpl.java
59:        this.conf.setSocks5ProxyScope(Socks5ProxyScope.HTTP_ONLY);

$ grep -n 'private Socks5ProxyScope socks5ProxyScope' pulsar-client/src/main/java/org/apache/pulsar/client/impl/conf/ClientConfigurationData.java
486:    private Socks5ProxyScope socks5ProxyScope = Socks5ProxyScope.BINARY_ONLY;

$ grep -n 'appliesToHttp' pulsar-client/src/main/java/org/apache/pulsar/client/impl/auth/v5/FrameworkHttpClientFactory.java
235:        if (scope == null || !scope.appliesToHttp()) {

The admin defaults to HTTP_ONLY, so before this commit an admin with socks5ProxyAddress(...) did route IdP metadata/token calls through the proxy. A fresh ClientConfigurationData has a null address and defaults to BINARY_ONLY, so now it doesn't — and the socks5Proxy.address system-property fallback is dead here for the same reason. PulsarAdmin + SOCKS5-only egress + OAuth2 can no longer reach the IdP. Threading the admin's real conf through fixes it.

Secondary cost of the same line: a second FileBasedTlsFactory and a dedicated oauth2-idp-tls scheduler thread per admin, while the connector's own factory keeps folding CLIENT_OAUTH2 for nobody.

Worth noting the divergence: PulsarClientImpl.hasOAuth2IdpTlsMaterial() uses identical detection but the opposite remedy — compose/fold into the shared factory, keeping the event loop, timer, DNS resolver and conf-derived transport settings. The commit message argues the client's approach "would not have covered plaintext http://", but PulsarClientImpl composes precisely when !conf.isUseTls() && hasOAuth2IdpTlsMaterial(), so that path was available.

3. The atomic config resolution contradicts the stable API and the published schema

Atomicity is the right call and this implements it. But making the config non-inheriting wasn't propagated:

$ grep -n 'isNotBlank(clusterClassName) ? clusterConfig : brokerConfig' \
    pulsar-broker/src/main/java/org/apache/pulsar/broker/service/BrokerService.java
1804:        return StringUtils.isNotBlank(clusterClassName) ? clusterConfig : brokerConfig;

$ grep -c 'blank to inherit the broker-level' \
    pulsar-client-admin-api/src/main/java/org/apache/pulsar/common/policies/data/ClusterData.java
6

Six promises of inheritance in the stable admin API alone (accessors and builder methods), plus ClusterDataImpl.java:180-184 — which is the published REST/OpenAPI schema description — plus CmdClusters.java:374-378 CLI help, plus pip-478.md:1192, :1241, :1286. Note :1241 and :1286 were rewritten by this commit, so it asserts inheritance in prose while removing it in code.

Concretely: a cluster that sets a class name and leaves config blank gets empty init params where the docs promise the broker-level config.

The inverse is a new silent drop: a cluster that sets brokerClientTlsFactoryConfig with a blank class name has its config discarded in favour of the broker-level one, with nothing logged. Same failure shape you added warnOnStalePip337ClusterFactory for — either honour it or warn.

4. There is a third per-cluster leg, and it doesn't participate

ClusterData.java:70-72, ClusterDataImpl.java:174-176, CmdClusters.java:369-371 and pip-478.md:1192 all say a cluster entry drives exactly both outbound legs. There's a third:

$ grep -c 'createClientImpl' pulsar-broker/src/main/java/org/apache/pulsar/broker/namespace/NamespaceService.java
1
$ grep -c 'TlsFactory'      pulsar-broker/src/main/java/org/apache/pulsar/broker/namespace/NamespaceService.java
0

NamespaceService.java:1745-1772 builds a per-cluster lookup PulsarClient from ClusterData — it reads getBrokerServiceUrlTls(), getServiceUrlTls() and so on — and never touches the factory fields, so it silently uses broker-level values only. Either wire it up or narrow the claim.


Test strength

I want to flag this carefully, because "mutation-verified" is doing real work in the PR description and in three places it doesn't hold. For the one that mattered most I stopped inferring and ran it.

5a. AdminOAuth2IdpTlsBindingTest does not detect removal of the fix — demonstrated

All four tests call PulsarAdminImpl.leaveOAuth2Standalone(...) directly; no PulsarAdmin is ever constructed. So I deleted the call site — the actual fix — and left the static method the tests exercise untouched:

--- a/pulsar-client-admin/src/main/java/org/apache/pulsar/client/admin/internal/PulsarAdminImpl.java
@@ private void bindAuthenticationServices(ClientConfigurationData conf) {
-        if (leaveOAuth2Standalone(auth, conf)) {
-            return;
-        }
$ ./gradlew :pulsar-client-admin-original:test --tests '*AdminOAuth2IdpTlsBindingTest*'
BUILD SUCCESSFUL
# TEST-...AdminOAuth2IdpTlsBindingTest.xml
tests="4" skipped="0" failures="0" errors="0"

The bug is fully restored and all four tests still pass. The mutation claim holds only for mutating the static's own body, which isn't the fix. It also can't distinguish "material honoured" from "material bound but ignored", which is the reported failure mode.

The client path has the real thing — OAuth2IdpTlsFrameworkClientTest against a TLS WireMock IdP with a negative twin. The admin equivalent would be a PulsarAdmin against a CA-signed HTTPS mock IdP asserting a Bearer token, plus the no-trustCertsFilePath twin asserting PKIX failure.

5b. AuthenticationTokenSerializationCompatTest can't detect a 4.x break

The code fix is right — I checked every SUID and field descriptor against f419587445. But the test builds the "4.x" object via Class.forName(...) on the current classpath and round-trips it in the current JVM, so both ends move together and a SUID drift or field-layout change is undetectable. There's no committed blob or .ser fixture anywhere in the tree (find . -name '*.ser' → nothing). One Base64.getDecoder().decode("rO0ABX...") captured under a real 4.x jar closes it permanently.

Two gaps beyond that: nothing round-trips a whole AuthenticationToken graph containing a 4.x supplier (the interesting part being outer SUID 1L surviving the added interfaces), and forward compat is now broken and unmentionedAuthenticationToken(String) stores a TokenAuthenticationV5.LiteralTokenSupplier, so a 5.0-serialized token read by a 4.x peer throws ClassNotFoundException. For a mixed-version Functions fleet that's the same break in the other direction.

5c. The per-cluster tests don't cover the wrap or failure mode (i)

Both ReplicatorTlsFactoryTest cases use the class name "default", for which isCustomFactoryClass is false — so wrapBrokerClientPurpose is never executed by any broker test, despite the comment at :114 claiming the test proves the factory is "purpose-wrapped". No test sets broker-level to one name and the cluster to a different name, which is the case that catches inverted precedence. And brokerClientTlsFactoryConfig(conf) (PulsarService.java:1929) has no coverage at all — grep getTlsFactoryParams over pulsar-broker/src/test returns nothing — so the atomicity half of the fix isn't mutation-covered either.

perClusterTlsFactoryAlsoDrivesTheCrossClusterAdminClient is a real improvement for mode (ii) though; that one does discriminate.

Also stale and now misleading: AdminOnlyOAuth2AuthTest.java:42-43 says a missing binding would throw "OAuth2 requires the authentication to be initialized by a PulsarClient/PulsarAdmin". That string exists nowhere but that comment now; FlowBase self-provisions instead, so the test passes on either path and no longer verifies binding.

5d. The proxy ordering fix has no test

SslContextFallbackSynthesisTest:201-209 only reads the fields after start() returns and would pass against the pre-fix ordering. A white-box assertion works — a ProxyService subclass overriding the bind to assert both fields non-null, or a PulsarTlsFactory recording whether a bind preceded its first createInstance.


Previously raised, no response

Flagging in case these were dropped rather than declined — happy to be told any are deliberate.

tlsCertRefreshCheckDurationSec=0 still means two different things. DefaultBrokerTlsFactory maps <= 00; ProxyTlsFactories:144, ProxyServer:289 and WorkerServer:392 map it → 60. The broker's own javadoc argues the case: "every v4 consumer guards its refresh task with > 0 … Pass it through rather than substituting the default, so an operator who set 0 still gets no poll." The other three do the opposite. Behaviour change vs PIP-337 plus an internal inconsistency.

The partial-init leak is still open, and there's a second site. Six places build a factory then run steps that can throw:

$ for f in <the six>; do awk '/TlsFactorySupport.createFactory/,/^    }/' $f \
    | grep -qc 'catch (Exception' && echo "GUARDED  $f" || echo "UNGUARDED $f"; done
GUARDED   broker/web/WebService.java
GUARDED   functions/worker/rest/WorkerServer.java
GUARDED   proxy/server/ServiceChannelInitializer.java
GUARDED   broker/service/PulsarChannelInitializer.java
UNGUARDED websocket/service/ProxyServer.java
UNGUARDED proxy/server/WebServer.java

ProxyServer (L239-257) is the one I reported; it also strands the proxy-websocket-ssl-refresh executor created at L115. WebServer.java:454-470 (pulsar-proxy) is new — it allocates sslRefreshScheduledExecutor at L455 before the unguarded sequence, and its caller is the WebServer constructor whose catch is throw new RuntimeException(e), so stop() is unreachable. Your own comment in WebService states the invariant and applies verbatim to both. Given WorkerServer was fixed here, this reads like an incomplete sweep. Both also need the executor shut down, which WebService/WorkerServer sidestep by borrowing a shared one.

LegacyV4AuthenticationAdapter.wrap still routes by auth-method name. "sasl" is still a string literal at L107; everything else falls to LegacyV4CredentialAdapter, which never exposes BinaryAuthChallengeHandler. A third-party v4 multi-round plugin reporting "kerberos" or "my-corp-sso" still hard-fails its first CommandAuthChallenge — silently until then, since onStarted probes hasDataFromCommand/hasDataForHttp and reports healthy. The probe machinery to fix it is already in onStarted().

LegacyV4CredentialAdapter.getHttpHeadersAsync still calls the deprecated no-arg getAuthData() (L323), ignoring callContext entirely, while the binary sibling at L310 threads callContext.brokerHost() through. Per-host credential selection is still dropped on the HTTP path.

The v4 shims still rebuild the driver per connection attemptAuthenticationToken:108, AuthenticationBasic:98, AuthenticationOAuth2:307. Uniformly absent rather than half-applied, so caching in a field fixes all three identically. Cost per attempt is two OTel instrument builds plus a re-run of ensureInitialized(); ClientCnx:412 and :638 mean every reconnect pays it.

ensureInitialized() still joins on the calling thread, and the SPI javadoc contradicts the justification. The comment says the built-in bodies "complete initializeAsync immediately … so join() does not block". But Authentication.initializeAsync's own javadoc says "Called once by the framework with runtime services after configuration. May do I/O". V5BinaryAuthenticationDriver's constructors are public and accept any third-party body, so the comment describes today's in-tree callers, not the contract. A body that fetches an IdP token in initializeAsync blocks a Netty event loop inside the synchronized (this) block, stalling every connection sharing that driver — the hazard PIP-478 exists to remove. Either the SPI needs a hard "must not block" statement (contradicting "May do I/O"), or the join needs to compose.

tlsCertFile/tlsKeyFile pairing is still unvalidated. FlowBase.hasOwnTlsMaterial() returns true when either is set; idpTlsPolicy() only attaches the client identity when both are. TlsClientAuthFlow.fromParameters is safe (it uses parseParameterString, which throws), but ClientCredentialsFlow.fromParameters uses params.get(...) with no pairing check. A client_credentials flow with one of the two produces a present-but-identity-less policy, and the mTLS handshake fails at the IdP with an opaque error instead of the startup exception v4 gave. New wrinkle since the OAuth2 fix: leaveOAuth2Standalone now returns true on that policy, so a whole standalone factory plus scheduler thread spins up to serve what is effectively "system default".

The two public-API breaks are still uninventoried, and one is worse: pip-478.md:980 still documents ClientCnx.newConnectCommand() — the method this PR renames to buildConnectCommand(AuthData) — and :1273 still asserts "The v4 API surface is otherwise untouched apart from the PIP-337 changes above". PulsarClientProvider.authenticationTls(String, String)authenticationTls() appears nowhere in the PIP.

Per-host client TLS material — withdrawing half of this. pip-478.md:801 does record it as design decision (b), destination as an optional hint "which factories may ignore". That answers intent. Missing is the operator-facing half: nothing in the Upgrade / runtime-behaviour-delta section mentions it, and a deployment issuing per-host client certificates loses that silently.


Smaller

  • Orphaned javadoc at BrokerService.java:1745-1760. The new method was inserted between the existing javadoc and the method it documented, leaving two consecutive blocks. resolveBrokerClientTlsFactory at :1787 — the method the whole fix pivots on, carrying the "silent downgrade of the TLS mechanism is a security regression" rationale — is now undocumented, and {@link #resolveBrokerClientTlsFactory} at :1792 links to a doc-less method.
  • Stale comment at BrokerService.java:1815-1820 still says the per-cluster field is "removed … lenient-dropped on metadata read". It's retained and read by warnOnStalePip337ClusterFactory 50 lines below. You fixed this exact wording in pip-478.md but not in the source.
  • warnOnStalePip337ClusterFactory details. Triggers on plugin || pluginParams but logs only .attr("brokerClientSslFactoryPlugin", ...), so a cluster with only ...PluginParams set emits a WARN with a null attribute and text about a plugin that isn't configured. And stalePip337ClusterFactoryWarned is never cleaned up on cluster removal (BrokerService:932-940 clears replicationClients/clusterAdmins but not this), so a delete/recreate cycle emits no fresh WARN.
  • brokerClient_ property precedence changed for the broker's own clients. brokerClientTlsFactoryConfig(conf) gates on the resolved class name being non-blank, which is also true when BrokerService wrote the broker-level name. Net effect: brokerClient_tlsFactoryClassName=com.acme.F now overrides brokerClientTlsFactoryClassName (previously ignored) and suppresses brokerClientTlsFactoryConfig, so the operator gets F with empty init params. Inconsistent across legs, since configTlsSettings/configAdminTlsSettings clobber the name after loadConf.
  • LegacyV4TlsAdapter start/close is fixed, but runs v4.start() inline (L414), as does configure (L398). The same file documents at L58-65 that all v4 configure/start/probe work runs on AuthenticationInitContext#blockingExecutor(), and the base class honours that via supplyOffloaded. The commit's own rationale — a custom tls plugin may do credential loading in start() — is exactly the blocking-I/O case.
  • TokenAuthenticationV5 implements Serializable is unmeetable. The javadoc justifies it so "the v4 AuthenticationToken shim round-trips", but the shim holds a bare Supplier, never a TokenAuthenticationV5; and the instance built at AuthenticationToken:108 is constructed with a non-serializable lambda () -> tokenSupplier.get(), so serializing one throws NotSerializableException.
  • FrameworkHttpClientFactory.hasTlsFactory() is dead code — zero callers.
  • ProxyService bind-failure leak profile. Post-fix a bind failure leaves the TLS factory, subscription and lookup factory live with refresh work on statsExecutor, and start() has no cleanup. Strictly better than before for the TLS-build failure, worse for the bind failure. Harmless in production (ProxyServiceStarter.main force-halts) but it bites embedded/test usage where a failed @BeforeMethod skips teardown.
  • lookupClientTlsFactory / brokerClientTlsFactory are non-volatile while brokerClientSslContext is. Correct today via the bind-submission happens-before chain, but the fix now leans on that chain where it previously didn't. Marking them volatile costs nothing.
  • Narrow residual unwrapped path. maybeApplyBrokerClientTlsFactory early-returns on !conf.isUseTls(), but PulsarClientImpl still resolves a factory when hasOAuth2IdpTlsMaterial() is true via the 4-arg resolveClientTlsFactory (brokerClientPurpose = false). A custom factory named via brokerClient_tlsFactoryClassName on a plaintext replication client with an OAuth2 broker-client plugin gets instantiated unwrapped — the original mode (ii). Narrow, but the guard asymmetry between the two methods is real.

The hard parts of this change — the offload discipline, the use-after-free pinning, the generation guards, the SPI happens-before contract — remain its strongest feature, and the three regressions I raised are all genuinely addressed. What I'd want resolved before merge is §1 (the reported defect is still live on the path most likely to hit it), §2 (fresh regression), §3 (a stable-API javadoc and the published OpenAPI schema now say the opposite of the code), and §5a either fixed or the "mutation-verified" wording softened. Everything under Previously raised and Smaller is yours to triage — several are one-liners, and I'm happy to be told any of them are deliberate.

Verification: worktree at d0c4577466; :pulsar-client-admin-original:test run on JDK 21 for the §5a mutation (baseline and mutant both tests="4" failures="0"). Everything else is grep-reproducible from the commands inline above, except §1, which is a four-file trace and flagged as such.

…erver (review: void-ptr974)

The client's sslProvider -> Netty engine mapping lost two properties the server-side
TlsFactorySupport.engineProvider already had:

- An explicit OPENSSL_REFCNT collapsed to OPENSSL. Only the reference-counted variant is free of
  the finalize() that JEP 421 deprecates, so an operator asking for it silently got the engine
  that leaks its native SSL_CTX under --finalization=disabled.
- An unset provider returned JDK. The PIP-337 client passed a null provider to SslContextBuilder,
  which then selected the native engine wherever a netty-tcnative binary existed, so this
  silently moved every client with no explicit provider off the native engine.

Both now match the server: engine literals are honoured verbatim, any other non-blank value is a
JSSE provider name and selects no native engine, and unset selects OPENSSL_REFCNT when
OpenSsl.isAvailable().

Assisted-by: Claude Code (Opus 5)
…TLS (review: void-ptr974)

PulsarClientBuilderV5.tlsPolicy(purpose, policy) set useTls unconditionally. useTls governs the
binary broker transport, while CLIENT_OAUTH2 describes a separate trust domain — the identity
provider — so configuring only an IdP policy against a plaintext broker turned broker TLS on and
the connection pool then attempted TLS against a plaintext port:

    PulsarClient.builder().serviceUrl("pulsar://broker:6650")
        .tlsPolicy(TlsPurpose.CLIENT_OAUTH2, idpPolicy)

useTls is now set only for a broker-transport purpose. The policy map is what triggers TLS-factory
creation, so a non-transport purpose still gets its factory without touching the transport.

Mutation-verified: restoring the unconditional set fails the new plaintext-broker test.

Assisted-by: Claude Code (Opus 5)
…the code, and warn on the dropped case

The atomic class-name/config resolution was right, but it was never propagated to what the API
promises. Six javadoc statements on the stable admin ClusterData API, the @Schema description
that becomes the published REST/OpenAPI schema, the pulsar-admin CLI help and three pip-478.md
passages all still said a blank brokerClientTlsFactoryConfig inherits the broker-level value —
including two PIP passages rewritten by the very commit that removed the inheritance.

All of them now describe what the code does: the class name inherits when blank, and the config
follows the class name rather than inheriting on its own, so factory A's init parameters can
never be handed to factory B.

That leaves one silent drop, which review flagged as the same failure shape the stale PIP-337
WARN exists for: a cluster that sets brokerClientTlsFactoryConfig but no class name has its
config discarded in favour of the broker-level one. It now logs a WARN naming the cluster and
saying what to set instead, once per cluster.

Assisted-by: Claude Code (Opus 5)
…val of the fix

Review demonstrated that the "mutation-verified" claim on AdminOAuth2IdpTlsBindingTest did not
hold: all four tests called PulsarAdminImpl.leaveOAuth2Standalone(...) directly, so deleting the
guard's invocation in bindAuthenticationServices — the actual fix — restored the bug with every
test still green. The claim only ever covered mutating the static's own body.

Add a test that builds a real PulsarAdmin and observes, through a package-private accessor,
whether the framework HTTP client factory was bound. It covers both directions: a plugin carrying
IdP TLS material must be left standalone, one without it must still be bound.

The plugin's start() is stubbed out because it would otherwise fetch OAuth2 server metadata over
the network; the binding decision runs before start() and does not depend on it.

Verified with the reviewer's exact mutation: deleting the call site now fails this test.

Note this still asserts the binding decision rather than end-to-end IdP trust. The
client-equivalent (OAuth2IdpTlsFrameworkClientTest against a TLS WireMock IdP with a negative
twin) has no admin counterpart yet; that gap is tracked in the review response plan.

Assisted-by: Claude Code (Opus 5)
… TLS factory

Replaces the leaveOAuth2Standalone guard with the remedy PulsarClientImpl already uses, fixing
two review findings at once.

The guard tested conf.getTlsFactory() == null as a proxy for "a preset factory can serve the IdP
material". It cannot: brokerClientTlsFactory built the factory with only CLIENT_DEFAULT
registered, and FileBasedTlsFactory resolves any other client purpose to the system default. So a
broker or functions worker with brokerClientTlsFactoryClassName set, an https:// admin URL and an
OAuth2 plugin carrying trustCertsFilePath still reached the IdP through the platform trust store,
silently — the original defect, on the path most likely to hit it.

Leaving the plugin standalone also cost the admin its transport settings.
StandaloneOAuth2HttpClientFactory builds its delegate from a fresh ClientConfigurationData, so the
admin's Socks5ProxyScope.HTTP_ONLY and proxy address were replaced by the BINARY_ONLY default:
PulsarAdmin + SOCKS5-only egress + OAuth2 could no longer reach the IdP at all. That was a
regression introduced by the previous fix, not a pre-existing one.

Folding fixes both. The admin now registers the plugin's idpTlsPolicy() under CLIENT_OAUTH2 on its
own configuration before binding, and brokerClientTlsFactory registers the same purpose when it
builds a preset factory, so both the admin-built and broker/worker-preset paths serve it. The fold
is putIfAbsent, so an explicitly supplied CLIENT_OAUTH2 policy still wins — restoring the
precedence composePolicies establishes. It also drops the duplicate FileBasedTlsFactory and its
per-admin refresh scheduler thread.

Mutation-verified: removing the fold fails the new tests.

Assisted-by: Claude Code (Opus 5)
…tail, dead code, volatility

Smaller items from review round 2, none behaviour-critical on their own.

- BrokerService.resolveBrokerClientTlsFactory lost its javadoc when the config-resolution helper
  was inserted above it, leaving two consecutive doc blocks and a {@link} pointing at an
  undocumented method. Restored, including the rationale that a silent downgrade of the TLS
  mechanism is a security regression, which is why the factory falls back where material does not.
- warnOnStalePip337ClusterFactory triggers on either PIP-337 field but logged only the plugin
  attribute, so a cluster setting only ...PluginParams produced a WARN with a null attribute and
  text about a plugin that was not configured. It now logs both and names the settings.
- The comment above configTlsSettings still said the per-cluster field is removed and
  lenient-dropped on metadata read. It is retained and ignored, and warnOnStalePip337ClusterFactory
  logs it — the same wording already corrected in pip-478.md.
- TokenAuthenticationV5 no longer implements Serializable. The javadoc justified it so the v4
  AuthenticationToken shim could round-trip, but the shim holds a bare Supplier and the instance it
  builds closes over a non-serializable lambda, so the claim was unmeetable. Java serialization is
  not a compatibility surface for new code; the suppliers stay serializable only so a 4.x blob
  still deserializes.
- FrameworkHttpClientFactory.hasTlsFactory() had no callers; removed.
- ProxyService.brokerClientTlsFactory / lookupClientTlsFactory are volatile, matching
  brokerClientSslContext. Correct before via the bind-submission happens-before chain, but the
  startup reordering now leans on that chain where it previously did not.

Assisted-by: Claude Code (Opus 5)
@lhotari

lhotari commented Aug 10, 2026

Copy link
Copy Markdown
Member Author

Thanks — and §5a is the most useful thing in this review, so let me start there.

§5a — you're right, and my claim was wrong

You deleted the call site, all four tests passed, and that is precisely what "mutation-verified" should have caught. It didn't, because every test called leaveOAuth2Standalone(...) directly and none of them ran bindAuthenticationServices. I mutated the predicate's body, saw red, and generalised from it — the claim covered the predicate, not the fix. I've been more careful about that distinction in this round.

bc17a22 adds a test that builds a real PulsarAdmin and observes, through a package-private accessor, whether the framework factory was bound. I reproduced your mutation verbatim: deleting the call site now fails it.

§1 and §2 — fixed together, by taking your pointer

You noted that PulsarClientImpl had the compose/fold remedy available and that my commit message's plaintext argument didn't hold up. That was right, so I stopped patching the predicate and adopted the fold instead.

b5f23da drops leaveOAuth2Standalone entirely. The admin folds the plugin's idpTlsPolicy() into its own configuration under CLIENT_OAUTH2 before binding, and — this is the part that closes §1 — brokerClientTlsFactory now registers the same purpose when it builds a preset factory. So the broker/worker path is covered where the factory is constructed, rather than by inferring capability from its presence, which was the flaw you identified.

That subsumes §2 rather than fixing it separately: with no standalone factory there is no fresh ClientConfigurationData, so the admin's HTTP_ONLY SOCKS5 scope and address survive, and the duplicate FileBasedTlsFactory plus its per-admin oauth2-idp-tls scheduler thread are gone. The fold is putIfAbsent, so the precedence inversion you noted in the same section is fixed too — there's a test for an explicitly supplied CLIENT_OAUTH2 policy winning.

§3 — fixed, and the count was worse than you reported

Six javadoc promises on the stable API, the @Schema description that becomes the published OpenAPI schema, the CLI help, and three PIP passages — two of which my own commit had rewritten while removing the inheritance. That last part is the bit I should have caught. 46f6884 makes all of them state what the code does: the class name inherits when blank, the config follows the class name. The inverse silent drop you identified now logs a WARN naming the cluster, matching warnOnStalePip337ClusterFactory.

Smaller — six fixed in 0bea320

The orphaned javadoc (you're right that resolveBrokerClientTlsFactory had lost the "silent downgrade of the TLS mechanism is a security regression" rationale the whole fix pivots on — restored); the WARN now logs both PIP-337 attributes, so a params-only cluster no longer emits a null attribute; the stale "removed … lenient-dropped" comment in BrokerService; the dead hasTlsFactory(); ProxyService's two factory fields made volatile; and TokenAuthenticationV5 implements Serializable dropped — your analysis that the claim was unmeetable is correct.

§5b — declining, and it's a scope decision rather than a disagreement

Java serialization compatibility across v4/v5 is not a requirement for this change. Functions doesn't depend on it (auth travels as authPluginClassName + params strings); the remaining consumer is assumed to be the Flink connector, which will get a separate v5 connector. New code shouldn't depend on Java serialization at all — it's a legacy feature.

So the forward-compat break you flagged is out of scope rather than unmentioned, and the .ser fixture isn't worth adding. Your point about the round-trip test proving less than it appears is still correct, and it's why TokenAuthenticationV5 no longer claims Serializable at all. The deserialization shims stay because they cost nothing.

Still open

Not declined, just not done — all recorded with fix shapes:

  • §4 — the third per-cluster leg. NamespaceService builds a per-cluster lookup client and never reads the factory fields, while four places claim a cluster entry drives exactly both legs. Either wire it or narrow the claim.
  • §5c — you're right that no broker test executes wrapBrokerClientPurpose, since both cases use "default"; and brokerClientTlsFactoryConfig has no coverage. Needs a custom factory class and a broker-A-vs-cluster-B case.
  • §5d — the proxy ordering fix still has no test that would fail against the pre-fix ordering.
  • §5a residual — the new test asserts the binding decision, not end-to-end IdP trust; the admin counterpart of OAuth2IdpTlsFrameworkClientTest doesn't exist yet.
  • Everything under Previously raised, plus the remaining Smaller items.

Nothing in that list is deliberate-as-is. Thanks for the falsifiable framing throughout — the greps made these quick to confirm, and the §5a run made it impossible to argue with.

@lhotari
lhotari requested a review from void-ptr974 August 10, 2026 21:11
…in, not the message tip

BrokerServiceTest.testTlsEnabled case 3 (TLS, insecure connections disallowed, no trust certs)
asserted that the top-level exception message contains "unable to find valid certification path
to requested target". That held only because the client's HTTP lookup went through
PulsarHttpAsyncSslEngineFactory, which built engines from a JDK SSLContext; the JDK engine copies
the ValidatorException text straight into the SSLHandshakeException message.

The core migration deletes that factory and has HttpClient build its engines from the same Netty
SslContext as the binary protocol, so with netty-tcnative on the classpath the engine is now
OpenSSL, whose handshake exception reads "General OpenSslEngine problem" and carries the PKIX
failure as a cause instead. The verification itself is unchanged — Netty's OpenSSL client context
still delegates to the JDK trust manager, so SunCertPathBuilderException stays in the chain — and
the binary protocol already behaved this way on master, where a null SslProvider resolves to
Netty's default client provider.

Assert on the whole cause chain so the check is engine-agnostic and keeps testing what it meant
to: the connection is rejected because the broker certificate does not validate.

Assisted-by: Claude Code (Opus 5)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants