[improve][misc] PIP-478: remove the superseded PIP-337 TLS stack

[lhotari]

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.

−3293 lines, +109.

Modifications

Six 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.
4. Review fixes. The RFC citation below (the phrase was inherited from the SecurityUtility comment this PR deletes, which had it wrong in both directions), the three files the first sweep missed plus the two conf files, and two build-file comments that credited JcaProviders with SecurityUtility's eager static initializer when it resolves BouncyCastle lazily — the very property this PR preserves. Comment-only.
5. Second-pass corrections. The CN fallback qualified as hostname-only (an IP literal is matched against iPAddress SANs and never the CN — commit 4 said this in the keystore script and omitted it from the three normative places); two OAuth2 test comments claiming the shared broker.keystore.jks has no SAN when it carries DNS:localhost + IP:127.0.0.1 and actually fails as self-signed; and the two builder javadocs below. Comment/javadoc-only.
6. The current standard. RFC 6125 is obsoleted by RFC 9525 (2023), which forbids the CN check outright — so Conscrypt's SAN-only verification is not extra-standard, it is the current rule, and the default engines are the lenient party. Also re-attributes the fallback condition to RFC 2818 §3.1, which is what the JDK's HostnameChecker actually implements (RFC 6125 §6.4.4's condition is stricter — no DNS-ID, SRV-ID or URI-ID at all). Comment/javadoc-only.

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 2818 §3.1 — and as the JDK's HostnameChecker implements it — the CN is consulted when the client connects by hostname and the 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 fallback never applied to an IP literal, which is matched against iPAddress SANs only.
• 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 SAN-only verification and rejects a server certificate with no subjectAltName. That is stricter than RFC 2818, not an application of it — §3.1 mandates the CN fallback ("Otherwise, the (most specific) Common Name field ... MUST be used") and RFC 6125 §6.4.4 permitted it as a last resort. But it is not a Conscrypt eccentricity either: RFC 9525 (2023) obsoletes RFC 6125 and forbids the CN check outright, so Conscrypt implements the current rule and the default engines are the lenient party. 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. Test comments in fourteen suites, two certificate-generation scripts, two operator-facing conf files and two public-API javadocs repeated the phrase or a variant of it and are swept too, so the corrected record is not contradicted by the next file a reader opens. The two conf files — conf/client.conf and conf/functions_worker.yml — shipped with the merged core-migration PR (#26282) and are the most consequential of the set: an operator reading them to decide whether to reissue certificates before a 5.0 upgrade would have got the retracted answer.

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.
• The hostname-verification claims were verified against sources rather than reasoned about: the JDK 21 HostnameChecker (matchDNS falls back to the CN whenever no dNSName SAN entry exists; matchIP never consults the CN), the Conscrypt 2.6.1 OkHostnameVerifier (SAN types 2 and 7 only, no CN path at all), and RFC 2818 §3.1 / RFC 6125 §6.4.4 / RFC 9525 directly.
• The fixture claims in the corrected comments were verified against the certificates themselves (openssl x509 -noout -subject -ext subjectAltName, keytool -list -v), not inferred: no-subject-alt-cert.pem is CN=Broker with no extensions, hn-verification/broker-cert.pem is CN=broker.pulsar.apache.org with no extensions, both suites advertise localhost, and the shared broker.keystore.jks carries DNS:localhost + IP:127.0.0.1 while being self-signed.

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. Additionally, enableTlsHostnameVerification's javadoc on both ClientBuilder and PulsarAdminBuilder is corrected: it documented hostname verification as matching hostname(CN/SAN) per RFC 2818 §3.1 unconditionally, which this PR makes false for a client that pins Conscrypt via sslProvider(...). Javadoc only — no signature change.

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).}

---

_{Stack created with GitHub Stacks CLI • Give Feedback 💬}

[david-streamlio]

A −3265 removal PR lives or dies on two questions: is the removed code actually dead, and is the compatibility story true. I checked both independently rather than reading them off the description, and the substance holds up well. The retraction in the compatibility section is the right call and clearly took work to get right.

Deadness — verified. No dangling import of any of the thirteen removed types anywhere in the tree, and no fully-qualified usage except the two deliberate string literals (PulsarConfigurationLoader:68, ConfigurationDataUtils:50) that name the old default FQCN for the tolerance check — which is exactly right, since those must survive the class they name. Every remaining textual hit is a false positive: Athenz's keyRefresher.getKeyManagerProxy() is an Athenz-library type, SchemaRegistryServiceImpl's convertFromDomainType is schema code, the rest are comments and PIP prose.

The BouncyCastle move — semantics preserved. Same resolution order (registered BC → BCFIPS → classpath), Security.addProvider side effect retained on the classpath path, and MessageCryptoBc's own BcProviderHolder is what preserves first-use timing, so the claim survives the move to a lazy holder. I also checked the failure mode a removal like this invites — nothing in the tree, production or test, resolves BouncyCastle by name (Security.getProvider("BC") / getInstance(…, "BC")), so losing SecurityUtility's eager registration strands nothing. Conscrypt genuinely is an eager static final field in both classes, so that part of the claim is exact.

Deleted tests — coverage has successors. SslContextTest is PIP-337-only despite the generic name, and each of its four methods maps onto a live one: serverPemContextAcrossEngineAndCiphers / clientPemContextAcrossEngineAndCiphers for the SslProvider×cipher matrix, serverKeystoreContextBuildsWithCiphers / clientKeystoreContextBuildsWithCiphers for the keystore paths, plus defaultProtocolsAppliedWhenPolicyLeavesThemUnset for the TLSv1.3/1.2 floor.

The compatibility claim itself — verified at the source. SecurityUtility.loadConscryptProvider() really did call Conscrypt.setDefaultHostnameVerifier(wrap(new TlsHostnameVerifier())) process-globally from a static field initializer, so scoping the break to Conscrypt-pinned clients is correct. The base branch's own JcaProviders CAVEAT predicted this PR almost verbatim. ClusterData retention is backed by real code (BrokerService.warnOnStalePip337ClusterFactory, deduped per cluster), the config tolerance does the right thing (default FQCN treated as unset, custom value rejected with a migration message), and the JdkSslContexts javadoc line about the Conscrypt propagation workaround was genuinely stale — no such code remains in the method.

Three things:

1. The sweep is incomplete, and it misses in the two suites the PR cites as its evidence.

The stated goal is that "the corrected record is not contradicted by the next file a reader opens." Three files still contradict it:

• AuthenticationTlsHostnameVerificationTest.java:143 — "CN-based matching is no longer supported (PIP-478)." That is verbatim the retracted claim. Also :128 and :182, "SAN-based (RFC 2818) host-name verification". This file is not in the diff at all, and it is the first suite the PR names under Verifying this change.
• ProxyWithAuthorizationTest.java:77-79 — "SAN-based (RFC 2818) hostname verification … CN-based matching is no longer supported (PIP-478, Pulsar 5.0), so this cert always fails hostname verification." This file is in the diff — the sweep corrected lines 186 and 245 and stopped short of this one.
• JcaProviders.java:298 — "standard RFC 2818 (SAN-based) verification, matching PIP-478's removal of the deprecated CN-based matching" — three lines above the corrected text this PR adds at :300.

The ProxyWithAuthorizationTest one is worth more than staleness: it states the wrong reason the test passes. The PR's own analysis is that these suites "fail on CN mismatch, not on SAN absence" — the comment tells the next reader the opposite, next to the fixture that would mislead them.

I checked whether these are simply deferred to a later PR in the stack: they are not. All three survive unchanged at lh-pip-478-proxy-async-auth (#26328), the top of the stack, which contains this branch plus #26326 and #26327.

2. "RFC 2818" is the wrong citation, and the PR body repeats it.

RFC 2818 §3.1 mandates the CN fallback: "If a subjectAltName extension of type dNSName is present, that MUST be used as the identity. Otherwise, the (most specific) Common Name field … MUST be used." So Conscrypt refusing a SAN-less certificate is stricter than RFC 2818, not "its own RFC 2818 verification" (PR body, and pip-478.md:1247). The phrase is inherited from the SecurityUtility comment being deleted, which had it wrong too.

package-info.java gets this exactly right — "per RFC 6125 the JDK and OpenSSL engines consult the CN … Conscrypt does not fall back to the CN" — so the code doc and the PIP now characterize the same behaviour with different standards. Since this passage is the corrected record, I'd align the PIP and body on the package-info's framing: RFC 6125 for the fallback, and "does not implement the CN fallback RFC 2818 allows" for Conscrypt.

3. Nit: two build-file comments now misdescribe the timing.

pulsar-common/build.gradle.kts and pulsar-broker-common/build.gradle.kts read "tests that exercise JcaProviders (which loads org.bouncycastle.jce.provider.BouncyCastleProvider in a static initializer)". True of SecurityUtility (BC_PROVIDER was an eager static final), not of JcaProviders, which resolves BC through BouncyCastleHolder on first use — the very property the PR highlights as preserved. Mechanical rename carrying over a clause that no longer describes the class.

None of this touches the removal itself, which I think is sound. 1 and 2 are about the corrected record being self-consistent — and given that this PR exists partly to retract a false compatibility claim, a reader landing on AuthenticationTlsHostnameVerificationTest:143 would come away with the retracted version.

For disclosure: this is static analysis over the stacked branch. I did not run sanityCheck or the cited suites locally.

[Copilot]

Pull request overview

Removes the superseded PIP-337 TLS stack and documents provider-specific hostname verification behavior.

Changes:

• Deletes obsolete TLS factories, utilities, hostname verifier classes, and tests.
• Moves message-crypto provider lookup to JcaProviders.
• Corrects SAN-only compatibility claims.

Reviewed changes

Copilot reviewed 41 out of 41 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
tests/certificate-authority/generate_keystore.sh	Revises certificate guidance.
tests/certificate-authority/ec/jks/key_store_generation.txt	Revises SAN guidance.
pulsar-proxy/.../SslContextFallbackSynthesisTest.java	Corrects TLS comment.
pulsar-proxy/.../ProxyWithoutServiceDiscoveryTest.java	Corrects TLS comment.
pulsar-proxy/.../ProxyWithAuthorizationTest.java	Revises hostname-verification comments.
pulsar-proxy/.../ProxyTlsTest.java	Corrects TLS comment.
pulsar-proxy/.../ProxyTlsFactoryTest.java	Corrects TLS comment.
pulsar-proxy/.../ProxyServiceTlsStarterTest.java	Corrects TLS comment.
pulsar-proxy/.../ProxyMutualTlsTest.java	Corrects TLS comment.
pulsar-proxy/.../ProxyKeyStoreTlsWithoutAuthTest.java	Corrects TLS comment.
pulsar-proxy/.../ProxyKeyStoreTlsWithAuthTest.java	Corrects TLS comment.
pulsar-proxy/.../ProxyKeyStoreTlsTransportTest.java	Corrects TLS comment.
pulsar-common/.../TrustManagerProxyTest.java	Removes obsolete proxy test.
pulsar-common/.../tls/JdkSslContextsTest.java	Corrects provider test terminology.
pulsar-common/.../netty/SslContextTest.java	Removes legacy factory tests.
pulsar-common/.../KeyManagerProxyTest.java	Removes obsolete proxy test.
pulsar-common/.../DefaultPulsarSslFactoryTest.java	Removes legacy factory tests.
pulsar-common/.../TrustManagerProxy.java	Removes obsolete trust-manager proxy.
pulsar-common/.../tls/JdkSslContexts.java	Updates provider documentation.
pulsar-common/.../tls/JcaProviders.java	Documents native Conscrypt behavior.
pulsar-common/.../SecurityUtility.java	Removes superseded TLS utility.
pulsar-common/.../PulsarSslFactory.java	Removes PIP-337 SPI.
pulsar-common/.../PulsarSslConfiguration.java	Removes legacy configuration model.
pulsar-common/.../keystoretls/SSLContextValidatorEngine.java	Removes unused validator.
pulsar-common/.../keystoretls/package-info.java	Removes obsolete package metadata.
pulsar-common/.../keystoretls/KeyStoreSSLContext.java	Removes legacy keystore context.
pulsar-common/.../KeyManagerProxy.java	Removes obsolete key-manager proxy.
pulsar-common/.../DefaultPulsarSslFactory.java	Removes legacy factory implementation.
pulsar-common/.../tls/TlsHostnameVerifier.java	Removes custom hostname verifier.
pulsar-common/.../tls/SubjectName.java	Removes verifier helper.
pulsar-common/.../tls/PublicSuffixMatcher.java	Removes verifier helper.
pulsar-common/.../tls/PublicSuffixList.java	Removes verifier helper.
pulsar-common/.../tls/package-info.java	Documents provider-specific CN fallback.
pulsar-common/.../tls/impl/TlsContexts.java	Updates legacy references.
pulsar-common/.../tls/DomainType.java	Removes verifier helper.
pulsar-common/build.gradle.kts	Updates dependency comment.
pulsar-client-messagecrypto-bc/.../MessageCryptoBc.java	Uses JcaProviders for Bouncy Castle.
pulsar-client-messagecrypto-bc/build.gradle.kts	Updates provider-resolution comment.
pulsar-broker-common/.../TlsFactorySupport.java	Corrects hostname-verification documentation.
pulsar-broker-common/build.gradle.kts	Updates dependency comment.
pip/pip-478.md	Corrects compatibility and upgrade guidance.

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[david-streamlio]

Consolidating the sweep into one checklist, now that the Copilot review has landed — it found two sites I missed, and chasing those turned up an eighth. Same defect in all of them: the retracted "a CN-only certificate is no longer accepted" claim, in the wording each file happens to use.

File	Line(s)	Text	In this diff?
AuthenticationTlsHostnameVerificationTest.java	128	"SAN-based (RFC 2818) host-name verification, closes the connection"	no
"	143	"CN-based matching is no longer supported (PIP-478)"	no
"	182	"SAN-based (RFC 2818) host-name verification and continues"	no
ProxyWithAuthorizationTest.java	77-79	"SAN-based (RFC 2818) … CN-based matching is no longer supported"	yes
"	251	"which cannot match any host regardless of the advertised address"	yes
JcaProviders.java	298	"standard RFC 2818 (SAN-based) verification"	yes
generate_keystore.sh	30-33	"every peer that now verifies hostnames rejects it with …"	yes
ec/jks/key_store_generation.txt	10-11	"the server certificate MUST carry a SubjectAltName"	yes

Six of the eight are in files this PR already edits, and two of those (ProxyWithAuthorizationTest:251, generate_keystore.sh:33) sit in the immediate context of hunks the sweep did touch.

Why my original three missed the rest, in case it helps you check for a ninth: I grepped for the retracted phrasings — SAN-only, SAN-based, CN-based … no longer — and the cert scripts state the same claim without any of those tokens ("every peer … rejects it", "MUST carry a SubjectAltName"). A phrasing-independent sweep is closer to git grep -niE 'reject|must carry|no subject alternative|cannot match' over the tests and cert tooling.

The last one is milder than the others and I'd treat it separately: key_store_generation.txt is prescriptive guidance for generating a fixture rather than a claim about peer behaviour, and "carry a SAN" remains good advice. It is only the word MUST that overstates it. Copilot's suggested framing on generate_keystore.sh — describe the SANs as making the fixture portable across providers and endpoint forms rather than universally mandatory — reads correctly for both scripts, and has the merit of staying true whichever engine a future reader is on. Worth noting the SAN genuinely is required for the IP:127.0.0.1 case regardless of provider, since the CN fallback does not apply to IP-literal verification; so the fixtures themselves are right, it is only the stated reason that isn't.

Nothing here changes my read of the removal itself, which I still think is sound.

[lhotari]

Your three findings are fixed in e5808294649. Two further adversarial passes over that fix found three more each, in 9a1d095e700 and 1854e8b8b65 — all three commits are comment/doc-only, no behaviour change. The PR body is corrected too; it carried the same wrong citation.

---

Thank you — and the part I most needed checking is the part you checked: that a PR whose stated
purpose is retracting a false compatibility claim had not quietly shipped a second one. It had.
All three findings are real and all three are fixed in e5808294649.

2 first, because 1 and 3 fall out of it.

The RFC citation is wrong, and you have the mandate the right way round. RFC 2818 §3.1: "If a
subjectAltName extension of type dNSName is present, that MUST be used as the identity. Otherwise,
the (most specific) Common Name field ... MUST be used." So a verifier that refuses a SAN-less
certificate is stricter than RFC 2818, not an implementation of it. Calling Conscrypt's default
"its own RFC 2818 verification" gets the standard exactly backwards.

You also traced where it came from, and it is worth stating for the record, because it explains why
the phrase was so sticky. It is inherited from the comment this PR deletes, in
SecurityUtility.loadConscryptProvider:

Configure Conscrypt's default hostname verifier to use Pulsar's TlsHostnameVerifier which is more
relaxed than the Conscrypt HostnameVerifier checking for RFC 2818 conformity.
[...] RFC 2818 recommends the use of SAN

Both halves are wrong, and they are wrong in mutually inconsistent ways — RFC 2818 does not
"recommend" SAN in the sense of leaving the CN optional, it mandates the CN as the fallback. So the
citation was never right; my sweep carried it forward from the class it was deleting and gave it a
second life in the corrected record. Corrected in all three places it appeared:

• JcaProviders — now: SAN-only, "stricter than the fallback RFC 2818 §3.1 mandates when a
  certificate carries no dNSName SAN, and stricter than the last-resort CN-ID check RFC 6125 §6.4.4
  still permits."
• pip-478.md — same correction, with the mandating sentence quoted inline so the next reader does
  not have to take it on trust.
• The PR body, which repeated it. Updated.

I took your suggestion to align on package-info.java's framing, since that passage had it right
all along. I did not reword package-info.java itself.

1. The sweep was incomplete — and worse than you could see from the diff. Fixed, and extended
past the three files you named.

Your three:

• AuthenticationTlsHostnameVerificationTest — :143 (the verbatim retracted claim) and the two
  javadoc blocks at :128 / :182.
• ProxyWithAuthorizationTest:77-79 — the one the sweep skipped over between the two it corrected.
• JcaProviders:298 — folded into the citation fix above.

You were right that this matters more than staleness on the ProxyWithAuthorizationTest one, and
the same is true of AuthenticationTlsHostnameVerificationTest: both stated the wrong reason the
test passes. Both fixtures fail on two counts, and the comments named the one that is not
operative. Confirmed against the certificates rather than inferred:

$ openssl x509 -in .../ProxyWithAuthorizationTest/no-subject-alt-cert.pem -noout -subject -ext subjectAltName
No extensions in certificate
subject=C=US, ST=CA, O=Apache Pulsar, OU=Broker, CN=Broker

$ openssl x509 -in .../authentication/tls/hn-verification/broker-cert.pem -noout -subject -ext subjectAltName
No extensions in certificate
subject=CN=broker.pulsar.apache.org

Both are reached at localhost, so on a default engine HostnameChecker.matchDNS falls back to the
CN and finds Broker / broker.pulsar.apache.org. The absence of a SAN is what routes to the CN;
the CN mismatch is what fails. The comments now say so.

Two files you could not have found from the diff, and they are the ones that matter most.
Checking whether the claim survived anywhere else turned up conf/client.conf:50 and
conf/functions_worker.yml:352:

# ... (RFC 2818/6125; CN matching is not supported).

That is the retracted claim, in the wrong-citation form, in operator-facing configuration files —
shipped by the merged core-migration PR (#26282), which is why neither this PR's diff nor the stacked
branches show it. An operator reading client.conf to decide whether to reissue certificates before a
5.0 upgrade would have got precisely the answer the retraction exists to prevent. Both now state that
SANs are authoritative, that the CN is consulted only for a certificate with no dNSName SAN, and
that a Conscrypt-pinned client does not fall back at all.

3. The build-file comments — taken, and the inversion is the point. JcaProviders resolves
BouncyCastle through BouncyCastleHolder (:108, reached via :126), so the clause was true of
SecurityUtility's eager static final and false of the class that replaced it — while the PR
argues the lazy timing as a preserved property. Both comments now read "resolves ...
reflectively, on first use". Same defect class as 1 and 2: a mechanical rename outrunning the text
attached to it.

Two things I deliberately did not change, so they are visible rather than silent:

• ClientBuilder:381 and PulsarAdminBuilder:205 — "matches provided hostname(CN/SAN) ... It
  follows RFC 2818, 3.1." These are pre-existing v4 public-API javadocs, and they are the one place
  in the tree where the RFC 2818 citation is correct: SAN preferred, CN as fallback, which is
  §3.1 exactly. Left alone.
• package-info.java — endorsed by your read, and I agree it is the sharpest statement of the
  behaviour in the tree.

On scope: three of the corrected files (conf/client.conf, conf/functions_worker.yml,
AuthenticationTlsHostnameVerificationTest.java) were not previously in this PR's diff. I have
pulled them in rather than deferring, because the PR's stated goal is that the corrected record is
not contradicted by the next file a reader opens, and a conf file is the first file an operator
opens. Say the word if you would rather see them split out.

Three more, found by running an adversarial pass over the fix itself — worth listing because two
of them are the same defect class you identified, and one of them was in my own correction:

• The corrected text was itself incomplete: the CN fallback is for hostnames only. I wrote "the
  CN is consulted when a certificate carries no dNSName SAN" in pip-478.md, JcaProviders and
  package-info.java — but the JDK's HostnameChecker dispatches an IP literal to matchIP, which
  consults iPAddress SANs and never the CN. So a SAN-less certificate has never worked for an IP
  connection, on any provider. I had stated this correctly in generate_keystore.sh and not in the
  three normative places, which is an inconsistency inside a single commit. Qualified in all three.
• Two OAuth2 test comments assert a fixture has no SAN when it does.
  OAuth2IdpTlsFrameworkClientTest:72 and AdminOAuth2IdpTlsEndToEndTest:72 both say the shared
  broker.keystore.jks "carries no SAN" and therefore fails hostname verification. It carries
  DNSName: localhost and IPAddress: 127.0.0.1 (keytool -list -v); it fails because it is
  self-signed and does not chain to ca.cert.pem — a trust failure, not a hostname one. Same
  origin as the conf files ([feat][client] PIP-478: asynchronous v5 client authentication and TLS factory integration (core migration) #26282). Corrected to state the real reason.
• ClientBuilder:381 / PulsarAdminBuilder:205 — I was wrong to leave these. My first pass kept
  them on the grounds that their RFC 2818 citation is the one correct one in the tree. That is true
  and beside the point: both builders also expose sslProvider(String), and this PR is what makes
  the documented hostname(CN/SAN) promise false for a client that pins Conscrypt — before it,
  SecurityUtility installed the CN-tolerant verifier process-globally, so the promise held there
  too. The javadoc now records that the CN is a fallback, that it applies only on the default engines
  and only for hostnames, and that Conscrypt never falls back.

And one more, which finishes the thought your finding started. You said the record should cite
the standards accurately. It now goes one step further than my first attempt did:

• RFC 6125 is obsoleted. I wrote that Conscrypt is stricter than "the last-resort CN-ID check RFC
  6125 §6.4.4 still permits". RFC 9525 (2023) obsoletes RFC 6125 and forbids the check outright —
  "The Common Name RDN MUST NOT be used to identify a service". So Conscrypt is not extra-standard
  at all: it implements the current rule, and the default engines are the lenient party. My
  framing would have invited the next maintainer to cite RFC 6125 as the last word and reopen this.
• The fallback condition is RFC 2818's, not RFC 6125's. I attributed "consulted when the
  certificate carries no dNSName SAN" to RFC 6125 §6.4.4. That RFC's condition is stricter — no
  DNS-ID, SRV-ID or URI-ID at all — whereas the JDK's HostnameChecker falls back on a certificate
  carrying only a URI SAN, which RFC 6125 forbids. RFC 2818 §3.1 is the accurate citation for what
  Pulsar actually gets. Corrected, with the divergence noted.
• Also fixed the pre-existing @see link text on ClientBuilder, which read "RFC 818".

Noted on disclosure: static analysis over the stacked branch, no local suite run. On this side
quickCheck and sanityCheck both pass, and the change is comment/javadoc-only throughout. The
fixture facts, the JDK fallback behaviour and the Conscrypt behaviour were each verified against the
actual certificates, the JDK 21 HostnameChecker source and the Conscrypt 2.6.1 source rather than
reasoned about — which is how the last three came to light.

[david-streamlio]

Re-ran my inventory on the current head: all eight sites are corrected, including the two the cert scripts stated in wording my greps had missed. The sweep is complete as far as I can find.

More usefully — you corrected my correction, and you are right.

I told you to cite RFC 6125 for the CN fallback. That was wrong, and the new text says why: RFC 6125 §6.4.4 permits the CN check only when the certificate presents no DNS-ID, SRV-ID or URI-ID at all, which is a stricter condition than what the JDK's HostnameChecker actually implements — it consults the CN whenever there is no dNSName SAN. So the behaviour Pulsar observes really is RFC 2818's, and swapping the citation to 6125 would have replaced one inaccuracy with another that is harder to catch, because it reads more modern.

The RFC 9525 point is the part I did not know and should have: 9525 (2023) obsoletes 6125 and forbids the CN check outright, so Conscrypt is not being eccentric — it implements the current standard, and the default engines are the lenient ones. That reframes the whole compatibility note from "Conscrypt is stricter than the spec" to "the default engines are still doing what the IETF retired", which is a materially better thing for an operator to read. The IP-literal clarification is a good addition too — a SAN-less certificate has never worked for an IP connection, so the fixtures were always right even where the stated reason was not.

Net: my finding that "Conscrypt applies its own RFC 2818 verification" was wrong stands, but my proposed replacement was also wrong, and what landed is better than either. Worth recording here rather than leaving the thread implying I was right on both halves.

No further findings on this one.

[david-streamlio]

Approving. I re-ran my full inventory on this head and every site is corrected, including the two the cert scripts stated in wording my own greps had missed.

The RFC work went further than my finding: my "RFC 2818 is the wrong citation for Conscrypt" was right, but my proposed replacement was also wrong, and what landed — 2818 for the mandated fallback, 6125 §6.4.4 for the last-resort CN-ID, 9525 for the current position — is better than either. Details in the comment above.

The removal itself I still consider sound: deadness independently verified, the BouncyCastle move semantics-preserving, and the deleted tests have named successors in FileBasedTlsFactoryTest.

[lhotari]

/pulsarbot rerun