Skip to content

[improve][misc] PIP-478: remove the superseded PIP-337 TLS stack (not merged to master) - #26320

Merged
nodece merged 4 commits into
lh-pip-478-auth-plugins-v3from
lh-pip-478-remove-pip337-v3
Aug 13, 2026
Merged

[improve][misc] PIP-478: remove the superseded PIP-337 TLS stack (not merged to master)#26320
nodece merged 4 commits into
lh-pip-478-auth-plugins-v3from
lh-pip-478-remove-pip337-v3

Conversation

@lhotari

@lhotari lhotari commented Aug 13, 2026

Copy link
Copy Markdown
Member

UPDATE: This PR was merged to a base branch of another PR in a GitHub PR stack, https://docs.github.com/en/pull-requests/how-tos/stacked-pull-requests. The replacement is #26322.

Main Issue: #25890

PIP: #25890

Stacked on #26319 — this PR's base is lh-pip-478-auth-plugins-v3, the Athenz/SASL branch, which is itself based on #26317. Review those first; the diff here shows only this part.

Motivation

With the PIP-478 migration complete — the PulsarTlsFactory SPI, the default file-based factory, the core migration and the v5-native auth inversion all landed — the PIP-337 TLS infrastructure it replaced has no callers left. So does the SecurityUtility monolith that PIP-478 decomposed into the common.util.tls helpers, and the CN-based hostname-matching machinery whose only referent was that monolith.

Removing them is the point of the exercise: PIP-478 justified its breaking changes partly on the maintenance cost of carrying two TLS stacks, and that cost is only actually paid down when the old one goes.

−3239 lines, +23.

Modifications

Three commits, each independently compiling, ordered so the reference graph unwinds cleanly:

  1. The PIP-337 factory stack. PulsarSslFactory, DefaultPulsarSslFactory, PulsarSslConfiguration, KeyManagerProxy, TrustManagerProxy, KeyStoreSSLContext, SSLContextValidatorEngine, and the four tests that exist only to exercise them. Each verified unreachable, including by class-name string — the configuration keys that once named the default factory are gone and now sit on the removed-key tolerance lists, which PulsarConfigurationLoaderTest and ConfigurationDataUtilsTest already pin. SecurityUtility.createAutoRefreshSslContextForClient goes with them: it was the sole production referent of the two proxies and had no callers of its own.
  2. SecurityUtility. One live consumer remained — message crypto's BouncyCastle provider lookup — which moves to JcaProviders.requireBouncyCastleProvider(). Same FIPS-agnostic resolution (non-FIPS BC or FIPS BCFIPS, whichever is present), same initialization-on-demand timing, so a resolution failure still surfaces at first asymmetric-crypto use rather than at class loading. Conscrypt registration is unaffected: both classes register it in their static initializers.
  3. The CN-matching machinery. TlsHostnameVerifier, PublicSuffixMatcher, PublicSuffixList, SubjectName, DomainType.

The compatibility statement, stated carefully

An earlier draft of this change announced that hostname verification becomes SAN-only and a CN-only certificate is no longer accepted. That is false, and shipping it would have been worse than saying nothing: operators would have reissued certificates for a break that does not exist, and the deployments that genuinely do break would have had no warning.

Verified with real JDK handshakes against this repository's own fixtures:

  • The default engines still fall back to the CN. Per RFC 6125 §6.4.4 the CN is consulted when a certificate carries no dNSName SAN, and ignored once any is present. This holds on the JDK engine and on Netty's native OpenSSL/BoringSSL engine — the native engine does not verify hostnames itself, it delegates to the Java trust manager, so the JDK's HostnameChecker governs both.
  • The real change is Conscrypt-pinned clients. SecurityUtility's static initializer installed the CN-tolerant TlsHostnameVerifier as Conscrypt's process-global default. With it gone, Conscrypt applies its own RFC 2818 verification and rejects a server certificate with no subjectAltName. A deployment is affected only if it names Conscrypt as its client-side JSSE provider and still uses CN-only server certificates; the remedy is to reissue those with the hostname in the SAN. Conscrypt is never the client-side default — it is defaulted only for TlsPurpose.WEB, i.e. server listeners, which do not verify hostnames.

Four documents asserted the wrong claim and are corrected, including the breaking-change bullet under Backward & Forward Compatibility that the other corrected passages route readers to. Eleven test comments and two certificate-generation scripts repeated the phrase and are swept too, so the corrected record is not contradicted by the next file a reader opens.

Deliberately no new "a CN-only certificate must be rejected" test: it would fail today on the default engines, which is the whole point.

What this PR does not remove

An earlier draft also removed ClusterData.brokerClientSslFactoryPlugin / ...Params. That is dropped, because its stated rationale — "factory selection is broker-level" — was refuted by the core migration, which made per-cluster selection real. pip-478.md argues their retention as a deliberate compatibility decision in nine places, one of them promising that a 5.0 broker "preserves it rather than erasing it on rewrite". Removing them would also delete the only operator-facing signal for a stale value, and erase the fields from stored metadata on any peer-cluster or migration update — silently downgrading a 4.x broker in a mixed fleet to the default factory.

Verifying this change

This change is a removal covered by existing tests:

  • sanityCheck (all modules, main and test sources) and quickCheck pass — the deletion's correctness is a reference-graph property, and a full compile is what checks it.
  • :pulsar-common:test --tests '*tls*' --tests '*Crypto*' — the decomposed helpers and the migrated BouncyCastle lookup.
  • AuthenticationTlsHostnameVerificationTest and ProxyWithAuthorizationTest — the two suites carrying SAN-less negative fixtures, which would notice if hostname behaviour moved. Both still pass, and for the same reason as before: they fail on CN mismatch, not on SAN absence.

Deadness was verified independently by review, including reflective and class-name-string loading, with the false positives triaged.

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
  • The schema
  • The default values of configurations
  • The threading model
  • The binary protocol
  • The REST endpoints
  • The admin CLI options
  • The metrics
  • Anything that affects deployment

The public API: thirteen public classes in pulsar-common are removed, all of them the PIP-337 stack or its CN-matching helpers, and all inventoried in PIP-478's removal impact section. InetAddressUtils and NoopHostnameVerifier survive in the same package.

Deployment: see the compatibility statement above — a Conscrypt-pinned client using CN-only server certificates must reissue them.

Documentation

  • doc-required
  • doc-not-needed
  • doc
  • doc-complete

The operator-facing consequence is documented in pip-478.md's compatibility section, which this PR corrects.

Matching PR in forked repository

PR in forked repository: lhotari#254

Prepared with the assistance of Claude Code (Opus 5).

…ry stack

The PIP-337 TLS infrastructure has no callers left. The core migration replaced it with
the PulsarTlsFactory SPI and the common.util.tls helpers, and the configuration keys that
once named DefaultPulsarSslFactory by class name (sslFactoryPlugin,
brokerClientSslFactoryPlugin) are gone — they now sit on the removed-key tolerance lists,
which PulsarConfigurationLoaderTest and ConfigurationDataUtilsTest already pin.

Deleted, each verified unreachable from any production path: PulsarSslFactory,
DefaultPulsarSslFactory, PulsarSslConfiguration, KeyManagerProxy, TrustManagerProxy,
KeyStoreSSLContext, SSLContextValidatorEngine, and the four tests that exist only to
exercise them. SSLContextValidatorEngine had no references at all, not even from its
sibling.

SecurityUtility.createAutoRefreshSslContextForClient goes with them: it was the sole
production referent of KeyManagerProxy and TrustManagerProxy and had no callers of its
own. The rest of SecurityUtility is removed in the next commit, once its last live
consumer is migrated.

Despite its name and package, SslContextTest is not a general Netty test — every case
constructs a DefaultPulsarSslFactory with a PulsarSslConfiguration.

Assisted-by: Claude Code (Opus 5)
…move SecurityUtility

SecurityUtility had exactly one live consumer left after the PIP-337 stack went: message
crypto's BouncyCastle provider lookup. Point it at the decomposed equivalent and delete the
monolith.

MessageCryptoBc now resolves the provider through
JcaProviders.requireBouncyCastleProvider(), which performs the same FIPS-agnostic lookup —
non-FIPS "BC" or FIPS "BCFIPS", whichever is on the classpath. The lookup stays inside the
initialization-on-demand holder, so a resolution failure still surfaces at first
asymmetric-crypto use rather than at class loading. One deliberate difference: an absent
BouncyCastle now fails with an IllegalStateException naming the caller instead of a
RuntimeException wrapping the classpath-load failure. Same failure point, better message.

Conscrypt registration is unaffected. Both classes register it eagerly in their static
initializers, so it still lands in the JCA provider list; what SecurityUtility additionally
installed — a process-global CN-tolerant hostname verifier — is dealt with in the next
commit, where the classes it depended on are removed.

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

Pulsar carried its own hostname verifier — TlsHostnameVerifier plus SubjectName,
PublicSuffixMatcher, PublicSuffixList and DomainType — whose only referent was
SecurityUtility, now gone. Delete them; verification is whatever the configured provider
implements under the standard endpoint-identification algorithm "HTTPS".

Be precise about the compatibility story, because the obvious statement of it is wrong.
The standard algorithm is NOT SAN-only. Per RFC 6125 the JDK and OpenSSL/BoringSSL engines
consult a certificate's CN when it carries no dNSName SAN, and ignore the CN once any is
present — verified here with real JDK handshakes against this repo's own fixtures: the
CN-only hn-verification/broker-cert.pem is accepted for its CN host and rejected for
another, and the SAN-bearing broker.cert.pem is rejected for its CN host. So on the default
engines a CN-only certificate is accepted exactly as before.

What does change is Conscrypt-pinned clients. SecurityUtility's static initializer
installed the CN-tolerant verifier as Conscrypt's process-global default; without it,
Conscrypt applies its own RFC 2818 verification and rejects a server certificate with no
subjectAltName. That reaches a deployment only if it names Conscrypt as its client-side
JSSE provider and still uses CN-only server certificates. Conscrypt is never the
client-side default — it is defaulted only for TlsPurpose.WEB, i.e. server listeners, which
do not verify hostnames.

Four documents asserted the SAN-only guarantee and are corrected rather than kept:
pip-478.md's summary line, its hostname-matching bullet, its breaking-change note, and the
common.tls package javadoc. Deliberately no new "CN-only must be rejected" test: it would
fail today on the default engines, which is the point.

Both in-repo SAN-less server certificates are negative fixtures on the default engine and
keep passing for the same reason as before — AuthenticationTlsHostnameVerificationTest
fails on CN mismatch, not on SAN absence.

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

Review found that three of the four CN/SAN passages were corrected and the fourth was not —
and the one missed is the breaking-change bullet under Backward & Forward Compatibility,
which is exactly where the other two send compatibility readers. It still announced "SAN
required" and "a CN-only certificate no longer suffices", contradicting the Security
Considerations text two sections above. An operator following the document's own routing
would have reissued fleet certificates for a break that does not exist on the default
engines.

The bullet now states what actually changes: hostname verification is on by default, a
matching SAN always suffices, the CN is still consulted on the default engines for a
certificate carrying no dNSName SAN, and Conscrypt-pinned clients are what tighten.

The mechanism is stated precisely too. Netty's native OpenSSL/BoringSSL engine does not
verify the hostname itself — it delegates to the Java trust manager — so the JDK's
HostnameChecker and its CN fallback govern both default engines, not BoringSSL's own
matching.

Also swept the framing out of the code: the broker TLS support javadoc claimed PIP-478
turns on "SAN-only verification", and eleven proxy tests plus two certificate-generation
scripts carried the same phrase, so the corrected record would have been contradicted by
the next file a reader opened.

Two mislabels found in the same review, pre-existing but adjacent: the trust-manager
fallback is documented as being for "a provider that offers none (e.g. Conscrypt)", but
real Conscrypt registers both PKIX factories — the fallback is for a third-party provider,
and the test stub named "Conscrypt-shaped" models a provider that does not exist. Renamed
and re-described. The same javadoc still advertised a Conscrypt hostname-verifier
propagation workaround whose body was removed when Conscrypt 2.6.1 landed.

Assisted-by: Claude Code (Opus 5)
@nodece
nodece merged commit 7d13956 into lh-pip-478-auth-plugins-v3 Aug 13, 2026
2 checks passed
@lhotari lhotari changed the title [improve][misc] PIP-478: remove the superseded PIP-337 TLS stack [improve][misc] PIP-478: remove the superseded PIP-337 TLS stack (not merged to master) Aug 13, 2026
@lhotari

lhotari commented Aug 13, 2026

Copy link
Copy Markdown
Member Author

@nodece I created this PR as part of a "stacked PR" (https://docs.github.com/en/pull-requests/how-tos/stacked-pull-requests), however I hadn't yet added it to the stack so it got merged to the base branch of another PR in the stack. The stacked PR for the same content is now in review in #26322

@nodece

nodece commented Aug 13, 2026

Copy link
Copy Markdown
Member

Sorry that I didn't notice this title.

@lhotari

lhotari commented Aug 13, 2026

Copy link
Copy Markdown
Member Author

Sorry that I didn't notice this title.

No worries. It's the first time I tried out this feature and didn't take care that it got linked as a stack on GitHub side and didn't mark the PR as draft.

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.

2 participants