You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pulsar.apache.org by "lhotari (via GitHub)" <gi...@apache.org> on 2026/08/07 06:34:31 UTC
[PR] [feat][client] PIP-478: asynchronous v5 client authentication and TLS factory integration (core migration) [pulsar]
lhotari opened a new pull request, #26282:
URL: https://github.com/apache/pulsar/pull/26282
PIP: #25890 ([`pip/pip-478.md`](https://github.com/apache/pulsar/blob/master/pip/pip-478.md))
### Motivation
This is the **third implementation PR** for [PIP-478](https://github.com/apache/pulsar/pull/25890), 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.
**Default value changes** (all called out in the PIP's *Upgrade* section):
- Hostname verification is **on by default** — `tlsHostnameVerificationEnabled` (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.
### 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.
- 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)
- [x] The public API — adds the `org.apache.pulsar.client.api.v5.auth` SPI; **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 the metadata schema is unchanged.
- [ ] The schema
- [x] 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
- [x] The admin CLI options — `pulsar-admin clusters` loses `--tls-factory-plugin` and `--tls-factory-plugin-params`; per-cluster factory selection no longer exists (set the factory broker-level with `brokerClientTlsFactoryClassName`).
- [ ] The metrics
- [x] 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
- [x] `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: https://github.com/lhotari/pulsar/pull/249 (Personal CI — full matrix green)
<sub>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.</sub>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [feat][client] PIP-478: asynchronous v5 client authentication and TLS factory integration (core migration) [pulsar]
Posted by "lhotari (via GitHub)" <gi...@apache.org>.
lhotari commented on PR #26282:
URL: https://github.com/apache/pulsar/pull/26282#issuecomment-5266300904
Thanks for the approval, and for framing the follow-ups as tracked rather than dropped — I've cleared the list. Details are in the PR description under *Round 2*; the short version, and one thing you should look at again.
**Your "one live item before merge"** — @void-ptr974's comment on `PulsarClientBuilderV5` — was a real defect and is fixed (f40121c7f85), with the parameterised "never silently downgraded to system default" test you suggested rather than one test per path. Your instinct that it was the same shape as §1 was right, and it went further than either of us thought: the **fourth** instance was on the admin path, where the connector composed a factory only for an `https://` admin URL.
**§4 — I narrowed the claim, and the evidence is what makes that the right answer rather than the convenient one.** The `NamespaceService` leg took the *broker-level* `brokerClientSslFactoryPlugin` in 4.x, never the per-cluster one, and this PR's only change there was deleting the removed PIP-337 calls. Behaviour is unchanged; six places just overstated the scope. Corrected in both `ClusterData` javadocs, the `@Schema` description, the CLI help, the CLI test, three PIP passages, and a javadoc on `getNamespaceClient` recording why that leg is broker-level.
**The partial-init sites** now use the guard `WebService` already establishes, via one idempotent helper shared with `stop()` — which also fixes the teardown order (close the factory before the executor its refresh work runs on).
**`tlsCertRefreshCheckDurationSec`** resolved to the broker's reading: every v4 listener guarded with `> 0`, so `0` means "no poll". Unified in one shared mapping so the four can't drift again, and the key's documentation — which claimed "check on every new connection", untrue for the server listeners since they moved to a shared refreshed context — is corrected. **This is a deliberate behaviour change for the proxy, WebSocket proxy and functions worker**, and the one item here I'd most like you to sanity-check.
**`ensureInitialized()`** — the contract was right and the code was wrong. Both drivers joined on `initializeAsync` while `ClientCnx.resolveConnectAuthData()` runs on an event loop. Initialization is now composed onto rather than joined; the SPI javadoc and PIP state both halves.
**The API breaks** are inventoried in the PIP. `ClientCnx.newConnectCommand()` is *renamed* to `buildConnectCommand(AuthData)` deliberately: the hook also fetched the credential, which cannot survive a future, and renaming makes a stale out-of-tree override a compile error instead of a method that silently stops being called.
**§5c is in** — `BrokerClientPerClusterTlsFactoryTest` is the first broker test that executes `wrapBrokerClientPurpose` (every prior case used `"default"`) and the first coverage of `brokerClientTlsFactoryConfig`; its factories serve only `BROKER_CLIENT`, so a missing translation shows as an empty result. Mutation-verified twice: dropping the wrapper, and resolving the class-name/config pair independently. **§5d** is verified by reverting its own commit. **§5a's residual** is now a real end-to-end test.
**The part worth your scepticism.** I had round 2 reviewed independently before asking for your time again, and it found four defects — all introduced by round 2. Two are worth naming because they repeat a lesson you already taught me in §5a: my end-to-end admin test *stubbed* `start()`, so it passed while the plugin's own cached HTTP client sat on platform-default trust; and a "failures are not cached" retry I had just documented in the SPI didn't hold for an already-failed future, because `whenComplete` runs synchronously and my clear happened before the assignment. Both fixed, both now covered by tests that fail against the old code.
Net effect on the admin: it resolves one TLS factory per client rather than one per internal connector (it has two), initializes and closes it exactly once, and releases it when construction fails. Ownership is carried explicitly by a `TlsFactoryOwnership` value rather than inferred from a flag. That's a lifecycle change on a path the broker uses, so it's called out in the PR description rather than buried in follow-ups.
CI is green (44/44) on 245308fba2d.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [feat][client] PIP-478: asynchronous v5 client authentication and TLS factory integration (core migration) [pulsar]
Posted by "lhotari (via GitHub)" <gi...@apache.org>.
lhotari commented on code in PR #26282:
URL: https://github.com/apache/pulsar/pull/26282#discussion_r3751359800
##########
pulsar-client/src/main/java/org/apache/pulsar/client/impl/tls/ClientTlsFactorySupport.java:
##########
@@ -0,0 +1,673 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pulsar.client.impl.tls;
+
+import com.fasterxml.jackson.core.type.TypeReference;
+import com.google.common.annotations.VisibleForTesting;
+import io.netty.handler.ssl.SslContext;
+import io.netty.handler.ssl.SslProvider;
+import io.opentelemetry.api.OpenTelemetry;
+import java.time.Clock;
+import java.util.ArrayList;
+import java.util.LinkedHashMap;
+import java.util.List;
+import java.util.Locale;
+import java.util.Map;
+import java.util.Optional;
+import java.util.Set;
+import java.util.concurrent.CompletableFuture;
+import java.util.concurrent.CompletionException;
+import java.util.concurrent.ExecutionException;
+import java.util.concurrent.Executor;
+import java.util.concurrent.ScheduledExecutorService;
+import java.util.function.Consumer;
+import java.util.function.Supplier;
+import org.apache.commons.lang3.StringUtils;
+import org.apache.pulsar.client.api.Authentication;
+import org.apache.pulsar.client.api.AuthenticationDataProvider;
+import org.apache.pulsar.client.impl.auth.AuthenticationKeyStoreTls;
+import org.apache.pulsar.client.impl.auth.AuthenticationTls;
+import org.apache.pulsar.client.impl.auth.oauth2.AuthenticationOAuth2;
+import org.apache.pulsar.client.impl.conf.ClientConfigurationData;
+import org.apache.pulsar.common.tls.impl.FileBasedTlsFactory;
+import org.apache.pulsar.common.tls.impl.FileBasedTlsFactorySettings;
+import org.apache.pulsar.common.tls.impl.TlsContextAcquisition;
+import org.apache.pulsar.common.tls.impl.TlsSynthesisSpec;
+import org.apache.pulsar.common.util.ObjectMapperFactory;
+import org.apache.pulsar.tls.PulsarTlsFactory;
+import org.apache.pulsar.tls.TlsEndpoint;
+import org.apache.pulsar.tls.TlsFactoryInitContext;
+import org.apache.pulsar.tls.TlsHandle;
+import org.apache.pulsar.tls.TlsPolicy;
+import org.apache.pulsar.tls.TlsPurpose;
+
+/**
+ * Client-side counterpart of the server's {@code TlsFactorySupport} (which lives in
+ * {@code pulsar-broker-common}, off the client classpath): the
+ * {@code ClientConfigurationData}→{@link TlsPolicy} composition, the {@code sslProvider}→Netty
+ * engine mapping, and the default {@link FileBasedTlsFactory} construction the v4 client's transport uses.
+ * Since the PIP-337 {@code PulsarSslFactory} removal this is the only client TLS path: a TLS
+ * client always builds a {@link PulsarTlsFactory} (an adopted v5 {@code tlsFactory(...)} or the built-in
+ * file-based default composed from the {@code tls*} fields).
+ *
+ * <p><b>Fail-fast probing.</b> Eager probing of the statically-needed client purpose
+ * ({@link TlsPurpose#CLIENT_DEFAULT}) is a <em>v5-builder-path</em> behaviour only: it runs when the v5
+ * builder configured TLS, so a bad path fails the client build with an actionable error. A plain v4 client's
+ * config stays lazily loaded, so unmodified v4 tests that lazily tolerated a bad TLS field are not broken.
+ */
+public final class ClientTlsFactorySupport {
+
+ /** Reserved factory-class value selecting the built-in file-based factory (mirrors the server side). */
+ private static final String DEFAULT_FACTORY = "default";
+
+ private ClientTlsFactorySupport() {
+ }
+
+ /**
+ * Map the v4 {@code sslProvider} string to the Netty engine provider. Only the literal OpenSSL
+ * providers select {@code OPENSSL}; every other value (JCE provider names, {@code null}) is JDK.
+ *
+ * @param sslProvider the configured {@code sslProvider} string
+ * @return the Netty {@link SslProvider}
+ */
+ public static SslProvider engineProvider(String sslProvider) {
+ if (sslProvider != null) {
+ String p = sslProvider.trim();
+ if ("OPENSSL".equalsIgnoreCase(p) || "OPENSSL_REFCNT".equalsIgnoreCase(p)) {
Review Comment:
Makes sense.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [feat][client] PIP-478: asynchronous v5 client authentication and TLS factory integration (core migration) [pulsar]
Posted by "void-ptr974 (via GitHub)" <gi...@apache.org>.
void-ptr974 commented on code in PR #26282:
URL: https://github.com/apache/pulsar/pull/26282#discussion_r3751158697
##########
pulsar-client-v5/src/main/java/org/apache/pulsar/client/impl/v5/PulsarClientBuilderV5.java:
##########
@@ -127,21 +278,133 @@ public PulsarClientBuilder transactionPolicy(TransactionPolicy policy) {
@Override
public PulsarClientBuilder tlsPolicy(TlsPolicy policy) {
- conf.setUseTls(true);
- if (policy.trustCertsFilePath() != null) {
- conf.setTlsTrustCertsFilePath(policy.trustCertsFilePath());
+ return tlsPolicy(TlsPurpose.CLIENT_DEFAULT, policy);
+ }
+
+ @Override
+ public PulsarClientBuilder tlsPolicy(TlsPurpose purpose, TlsPolicy policy) {
+ if (purpose == null || policy == null) {
+ throw new IllegalArgumentException("tlsPolicy purpose and policy must not be null");
}
- if (policy.keyFilePath() != null) {
- conf.setTlsKeyFilePath(policy.keyFilePath());
+ conf.setUseTls(true);
Review Comment:
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.
##########
pulsar-client/src/main/java/org/apache/pulsar/client/impl/tls/ClientTlsFactorySupport.java:
##########
@@ -0,0 +1,673 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pulsar.client.impl.tls;
+
+import com.fasterxml.jackson.core.type.TypeReference;
+import com.google.common.annotations.VisibleForTesting;
+import io.netty.handler.ssl.SslContext;
+import io.netty.handler.ssl.SslProvider;
+import io.opentelemetry.api.OpenTelemetry;
+import java.time.Clock;
+import java.util.ArrayList;
+import java.util.LinkedHashMap;
+import java.util.List;
+import java.util.Locale;
+import java.util.Map;
+import java.util.Optional;
+import java.util.Set;
+import java.util.concurrent.CompletableFuture;
+import java.util.concurrent.CompletionException;
+import java.util.concurrent.ExecutionException;
+import java.util.concurrent.Executor;
+import java.util.concurrent.ScheduledExecutorService;
+import java.util.function.Consumer;
+import java.util.function.Supplier;
+import org.apache.commons.lang3.StringUtils;
+import org.apache.pulsar.client.api.Authentication;
+import org.apache.pulsar.client.api.AuthenticationDataProvider;
+import org.apache.pulsar.client.impl.auth.AuthenticationKeyStoreTls;
+import org.apache.pulsar.client.impl.auth.AuthenticationTls;
+import org.apache.pulsar.client.impl.auth.oauth2.AuthenticationOAuth2;
+import org.apache.pulsar.client.impl.conf.ClientConfigurationData;
+import org.apache.pulsar.common.tls.impl.FileBasedTlsFactory;
+import org.apache.pulsar.common.tls.impl.FileBasedTlsFactorySettings;
+import org.apache.pulsar.common.tls.impl.TlsContextAcquisition;
+import org.apache.pulsar.common.tls.impl.TlsSynthesisSpec;
+import org.apache.pulsar.common.util.ObjectMapperFactory;
+import org.apache.pulsar.tls.PulsarTlsFactory;
+import org.apache.pulsar.tls.TlsEndpoint;
+import org.apache.pulsar.tls.TlsFactoryInitContext;
+import org.apache.pulsar.tls.TlsHandle;
+import org.apache.pulsar.tls.TlsPolicy;
+import org.apache.pulsar.tls.TlsPurpose;
+
+/**
+ * Client-side counterpart of the server's {@code TlsFactorySupport} (which lives in
+ * {@code pulsar-broker-common}, off the client classpath): the
+ * {@code ClientConfigurationData}→{@link TlsPolicy} composition, the {@code sslProvider}→Netty
+ * engine mapping, and the default {@link FileBasedTlsFactory} construction the v4 client's transport uses.
+ * Since the PIP-337 {@code PulsarSslFactory} removal this is the only client TLS path: a TLS
+ * client always builds a {@link PulsarTlsFactory} (an adopted v5 {@code tlsFactory(...)} or the built-in
+ * file-based default composed from the {@code tls*} fields).
+ *
+ * <p><b>Fail-fast probing.</b> Eager probing of the statically-needed client purpose
+ * ({@link TlsPurpose#CLIENT_DEFAULT}) is a <em>v5-builder-path</em> behaviour only: it runs when the v5
+ * builder configured TLS, so a bad path fails the client build with an actionable error. A plain v4 client's
+ * config stays lazily loaded, so unmodified v4 tests that lazily tolerated a bad TLS field are not broken.
+ */
+public final class ClientTlsFactorySupport {
+
+ /** Reserved factory-class value selecting the built-in file-based factory (mirrors the server side). */
+ private static final String DEFAULT_FACTORY = "default";
+
+ private ClientTlsFactorySupport() {
+ }
+
+ /**
+ * Map the v4 {@code sslProvider} string to the Netty engine provider. Only the literal OpenSSL
+ * providers select {@code OPENSSL}; every other value (JCE provider names, {@code null}) is JDK.
+ *
+ * @param sslProvider the configured {@code sslProvider} string
+ * @return the Netty {@link SslProvider}
+ */
+ public static SslProvider engineProvider(String sslProvider) {
+ if (sslProvider != null) {
+ String p = sslProvider.trim();
+ if ("OPENSSL".equalsIgnoreCase(p) || "OPENSSL_REFCNT".equalsIgnoreCase(p)) {
Review Comment:
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [feat][client] PIP-478: asynchronous v5 client authentication and TLS factory integration (core migration) [pulsar]
Posted by "void-ptr974 (via GitHub)" <gi...@apache.org>.
void-ptr974 commented on code in PR #26282:
URL: https://github.com/apache/pulsar/pull/26282#discussion_r3757911132
##########
pulsar-client-v5/src/main/java/org/apache/pulsar/client/impl/v5/PulsarClientBuilderV5.java:
##########
@@ -127,21 +278,133 @@ public PulsarClientBuilder transactionPolicy(TransactionPolicy policy) {
@Override
public PulsarClientBuilder tlsPolicy(TlsPolicy policy) {
- conf.setUseTls(true);
- if (policy.trustCertsFilePath() != null) {
- conf.setTlsTrustCertsFilePath(policy.trustCertsFilePath());
+ return tlsPolicy(TlsPurpose.CLIENT_DEFAULT, policy);
+ }
+
+ @Override
+ public PulsarClientBuilder tlsPolicy(TlsPurpose purpose, TlsPolicy policy) {
+ if (purpose == null || policy == null) {
+ throw new IllegalArgumentException("tlsPolicy purpose and policy must not be null");
}
- if (policy.keyFilePath() != null) {
- conf.setTlsKeyFilePath(policy.keyFilePath());
+ conf.setUseTls(true);
Review Comment:
Thanks for the update. I noticed one case with an explicit IdP policy on a plaintext broker. `setupClientTlsFactory()` returns when `useTls` is false and the authentication plugin does not carry its own IdP TLS material, without considering an explicit v5 `tlsPolicyMap`. As a result, `serviceUrl("pulsar://...").tlsPolicy(CLIENT_OAUTH2, policy).build()` leaves the TLS factory unset, so the framework HTTP client falls back to platform-default trust instead of using the configured IdP policy. It may be helpful to include the explicit policy map in the setup condition and extend the test through `build()`. Thanks!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [feat][client] PIP-478: asynchronous v5 client authentication and TLS factory integration (core migration) [pulsar]
Posted by "lhotari (via GitHub)" <gi...@apache.org>.
lhotari commented on PR #26282:
URL: https://github.com/apache/pulsar/pull/26282#issuecomment-5266903688
I'd like to have https://github.com/apache/pulsar/pull/26314 merged before this PR.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [feat][client] PIP-478: asynchronous v5 client authentication and TLS factory integration (core migration) [pulsar]
Posted by "lhotari (via GitHub)" <gi...@apache.org>.
lhotari commented on code in PR #26282:
URL: https://github.com/apache/pulsar/pull/26282#discussion_r3752334053
##########
pulsar-client/src/main/java/org/apache/pulsar/client/impl/tls/ClientTlsFactorySupport.java:
##########
@@ -0,0 +1,673 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pulsar.client.impl.tls;
+
+import com.fasterxml.jackson.core.type.TypeReference;
+import com.google.common.annotations.VisibleForTesting;
+import io.netty.handler.ssl.SslContext;
+import io.netty.handler.ssl.SslProvider;
+import io.opentelemetry.api.OpenTelemetry;
+import java.time.Clock;
+import java.util.ArrayList;
+import java.util.LinkedHashMap;
+import java.util.List;
+import java.util.Locale;
+import java.util.Map;
+import java.util.Optional;
+import java.util.Set;
+import java.util.concurrent.CompletableFuture;
+import java.util.concurrent.CompletionException;
+import java.util.concurrent.ExecutionException;
+import java.util.concurrent.Executor;
+import java.util.concurrent.ScheduledExecutorService;
+import java.util.function.Consumer;
+import java.util.function.Supplier;
+import org.apache.commons.lang3.StringUtils;
+import org.apache.pulsar.client.api.Authentication;
+import org.apache.pulsar.client.api.AuthenticationDataProvider;
+import org.apache.pulsar.client.impl.auth.AuthenticationKeyStoreTls;
+import org.apache.pulsar.client.impl.auth.AuthenticationTls;
+import org.apache.pulsar.client.impl.auth.oauth2.AuthenticationOAuth2;
+import org.apache.pulsar.client.impl.conf.ClientConfigurationData;
+import org.apache.pulsar.common.tls.impl.FileBasedTlsFactory;
+import org.apache.pulsar.common.tls.impl.FileBasedTlsFactorySettings;
+import org.apache.pulsar.common.tls.impl.TlsContextAcquisition;
+import org.apache.pulsar.common.tls.impl.TlsSynthesisSpec;
+import org.apache.pulsar.common.util.ObjectMapperFactory;
+import org.apache.pulsar.tls.PulsarTlsFactory;
+import org.apache.pulsar.tls.TlsEndpoint;
+import org.apache.pulsar.tls.TlsFactoryInitContext;
+import org.apache.pulsar.tls.TlsHandle;
+import org.apache.pulsar.tls.TlsPolicy;
+import org.apache.pulsar.tls.TlsPurpose;
+
+/**
+ * Client-side counterpart of the server's {@code TlsFactorySupport} (which lives in
+ * {@code pulsar-broker-common}, off the client classpath): the
+ * {@code ClientConfigurationData}→{@link TlsPolicy} composition, the {@code sslProvider}→Netty
+ * engine mapping, and the default {@link FileBasedTlsFactory} construction the v4 client's transport uses.
+ * Since the PIP-337 {@code PulsarSslFactory} removal this is the only client TLS path: a TLS
+ * client always builds a {@link PulsarTlsFactory} (an adopted v5 {@code tlsFactory(...)} or the built-in
+ * file-based default composed from the {@code tls*} fields).
+ *
+ * <p><b>Fail-fast probing.</b> Eager probing of the statically-needed client purpose
+ * ({@link TlsPurpose#CLIENT_DEFAULT}) is a <em>v5-builder-path</em> behaviour only: it runs when the v5
+ * builder configured TLS, so a bad path fails the client build with an actionable error. A plain v4 client's
+ * config stays lazily loaded, so unmodified v4 tests that lazily tolerated a bad TLS field are not broken.
+ */
+public final class ClientTlsFactorySupport {
+
+ /** Reserved factory-class value selecting the built-in file-based factory (mirrors the server side). */
+ private static final String DEFAULT_FACTORY = "default";
+
+ private ClientTlsFactorySupport() {
+ }
+
+ /**
+ * Map the v4 {@code sslProvider} string to the Netty engine provider. Only the literal OpenSSL
+ * providers select {@code OPENSSL}; every other value (JCE provider names, {@code null}) is JDK.
+ *
+ * @param sslProvider the configured {@code sslProvider} string
+ * @return the Netty {@link SslProvider}
+ */
+ public static SslProvider engineProvider(String sslProvider) {
+ if (sslProvider != null) {
+ String p = sslProvider.trim();
+ if ("OPENSSL".equalsIgnoreCase(p) || "OPENSSL_REFCNT".equalsIgnoreCase(p)) {
Review Comment:
Correct on both halves, and fixed in 7a85b02d0b6.
The `OPENSSL_REFCNT` → `OPENSSL` collapse matters more than it looks: only the reference-counted variant is free of the `finalize()` that [JEP 421](https://openjdk.org/jeps/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.
##########
pulsar-client-v5/src/main/java/org/apache/pulsar/client/impl/v5/PulsarClientBuilderV5.java:
##########
@@ -127,21 +278,133 @@ public PulsarClientBuilder transactionPolicy(TransactionPolicy policy) {
@Override
public PulsarClientBuilder tlsPolicy(TlsPolicy policy) {
- conf.setUseTls(true);
- if (policy.trustCertsFilePath() != null) {
- conf.setTlsTrustCertsFilePath(policy.trustCertsFilePath());
+ return tlsPolicy(TlsPurpose.CLIENT_DEFAULT, policy);
+ }
+
+ @Override
+ public PulsarClientBuilder tlsPolicy(TlsPurpose purpose, TlsPolicy policy) {
+ if (purpose == null || policy == null) {
+ throw new IllegalArgumentException("tlsPolicy purpose and policy must not be null");
}
- if (policy.keyFilePath() != null) {
- conf.setTlsKeyFilePath(policy.keyFilePath());
+ conf.setUseTls(true);
Review Comment:
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 eb73429f537: `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 b5f23da599d, which now folds the IdP policy into the client's own factory rather than letting the plugin self-provision one.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [feat][client] PIP-478: asynchronous v5 client authentication and TLS factory integration (core migration) [pulsar]
Posted by "lhotari (via GitHub)" <gi...@apache.org>.
lhotari merged PR #26282:
URL: https://github.com/apache/pulsar/pull/26282
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [feat][client] PIP-478: asynchronous v5 client authentication and TLS factory integration (core migration) [pulsar]
Posted by "lhotari (via GitHub)" <gi...@apache.org>.
lhotari commented on code in PR #26282:
URL: https://github.com/apache/pulsar/pull/26282#discussion_r3766027432
##########
pulsar-client-v5/src/main/java/org/apache/pulsar/client/impl/v5/PulsarClientBuilderV5.java:
##########
@@ -127,21 +278,133 @@ public PulsarClientBuilder transactionPolicy(TransactionPolicy policy) {
@Override
public PulsarClientBuilder tlsPolicy(TlsPolicy policy) {
- conf.setUseTls(true);
- if (policy.trustCertsFilePath() != null) {
- conf.setTlsTrustCertsFilePath(policy.trustCertsFilePath());
+ return tlsPolicy(TlsPurpose.CLIENT_DEFAULT, policy);
+ }
+
+ @Override
+ public PulsarClientBuilder tlsPolicy(TlsPurpose purpose, TlsPolicy policy) {
+ if (purpose == null || policy == null) {
+ throw new IllegalArgumentException("tlsPolicy purpose and policy must not be null");
}
- if (policy.keyFilePath() != null) {
- conf.setTlsKeyFilePath(policy.keyFilePath());
+ conf.setUseTls(true);
Review Comment:
Right on both counts, and the second half of your comment — "extend the test through `build()`" — is what actually found the bug. Thank you.
**The gap was real.** `setupClientTlsFactory()` composed the factory only when broker TLS was on or an OAuth2 plugin carried its own IdP material. An explicit `tlsPolicy(purpose, policy)` satisfied neither when the purpose is not the broker transport, so `serviceUrl("pulsar://…").tlsPolicy(CLIENT_OAUTH2, policy).build()` left `conf.tlsFactory` null and the framework HTTP client fell back to platform-default trust. It only became reachable once eb73429f537 correctly stopped an IdP-only policy from enabling broker TLS — the previous bug was masking this one.
Worth noting the design already said so: `pip-478.md` specifies "the client composes a TLS factory whenever TLS material is configured for *any* purpose". The implementation covered two of that rule's arms. So this is the code catching up to the spec, not a spec change.
Fixed in f40121c7f85: one predicate, `needsClientTlsFactory()` = `useTls || hasExplicitTlsPolicies() || hasOAuth2IdpTlsMaterial()`, shared with `rebuildClientTlsFactory()`. Composition still isn't enablement — the transport stays `useTls`-gated.
On the test: you were right that stopping at the builder's config was the problem. `PulsarClientBuilderV5Test.anIdpOnlyPolicySurvivesTheClientBuild` now goes through `build()`, and `OAuth2IdpTlsPlaintextBrokerTest` asserts "a configured `CLIENT_OAUTH2` trust is never downgraded to the system default" across the entry points that can describe that trust domain, rather than one test per path. Mutation-verified: restoring the old gate fails the v5 build test and the explicit-policy row, and correctly leaves the plugin-material rows passing.
One more thing your comment is responsible for. Chasing "where else does this shape hide" turned up the same defect on the **admin** path: `AsyncHttpConnector` composed a factory only for an `https://` admin URL, so a plaintext admin folded the IdP policy into a configuration no factory ever read. Fixed in the same series, with an end-to-end test against an HTTPS identity provider.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [feat][client] PIP-478: asynchronous v5 client authentication and TLS factory integration (core migration) [pulsar]
Posted by "lhotari (via GitHub)" <gi...@apache.org>.
lhotari commented on PR #26282:
URL: https://github.com/apache/pulsar/pull/26282#issuecomment-5244422559
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.
bc17a227f66 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.
b5f23da599d 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. 46f6884ae3d 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 0bea3209f77
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org