[improve][misc] PIP-478: remove the superseded PIP-337 TLS stack (not merged to master) - #26320
Merged
Merged
Conversation
…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
approved these changes
Aug 13, 2026
lhotari
added a commit
that referenced
this pull request
Aug 13, 2026
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 |
Member
|
Sorry that I didn't notice this title. |
Member
Author
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. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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
Motivation
With the PIP-478 migration complete — the
PulsarTlsFactorySPI, 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 theSecurityUtilitymonolith that PIP-478 decomposed into thecommon.util.tlshelpers, 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:
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, whichPulsarConfigurationLoaderTestandConfigurationDataUtilsTestalready pin.SecurityUtility.createAutoRefreshSslContextForClientgoes with them: it was the sole production referent of the two proxies and had no callers of its own.SecurityUtility. One live consumer remained — message crypto's BouncyCastle provider lookup — which moves toJcaProviders.requireBouncyCastleProvider(). Same FIPS-agnostic resolution (non-FIPSBCor FIPSBCFIPS, 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.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:
dNSNameSAN, 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'sHostnameCheckergoverns both.SecurityUtility's static initializer installed the CN-tolerantTlsHostnameVerifieras Conscrypt's process-global default. With it gone, Conscrypt applies its own RFC 2818 verification and rejects a server certificate with nosubjectAltName. 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 forTlsPurpose.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.mdargues 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) andquickCheckpass — 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.AuthenticationTlsHostnameVerificationTestandProxyWithAuthorizationTest— 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
The public API: thirteen public classes in
pulsar-commonare removed, all of them the PIP-337 stack or its CN-matching helpers, and all inventoried in PIP-478's removal impact section.InetAddressUtilsandNoopHostnameVerifiersurvive in the same package.Deployment: see the compatibility statement above — a Conscrypt-pinned client using CN-only server certificates must reissue them.
Documentation
doc-requireddoc-not-neededdocdoc-completeThe 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).