[improve][pip] PIP-489: FIPS 140-3 compliance mode for Apache Pulsar - #26155

[CORRECTNESS] (c)'s stated failure mode is not what the client does, and the rollback precondition cannot be satisfied

"they will land in retry/DLQ paths per existing crypto-failure handling" is not what the client does, and the difference matters for the operational contract in (c).

The default failure action is FAIL - ConsumerBuilderImpl.java:161-163 sets it when neither cryptoFailureAction nor decryptFailListener is configured - and the FAIL branch does this:

DecryptResult.discard() then returns out of messageReceived before any delivery, negative-ack, retry-topic or DLQ logic runs (ConsumerImpl.java:1478-1484). So a consumer that cannot unwrap a RSA_OAEP_SHA256 key does not dead-letter the message: it never sees it, never acks it, and the subscription's mark-delete position stops advancing past it. With ackTimeout set, unAckedMessageTracker.add turns that into an unbounded redelivery loop for the whole retention period. Neither outcome is visible as a DLQ an operator can drain.

Second, and I think more important: :585-587 makes rollback conditional on producer state, but the constraint is about persisted data. "provided no producer is still emitting RSA_OAEP_SHA256-wrapped keys" is not a condition anyone can establish - the wrapped keys are in the topic, so retention, backlog, replay from an earlier position, and (per :594-596) every geo-replicated copy keep SHA-256-wrapped messages unreadable to a rolled-back consumer for the whole retention window. There is no operational step that makes it true and no way to check it.

Both are fixable in prose, and the honest version is stronger than the current one:

• in the compatibility table at :562, say what actually happens (the message is not delivered and the subscription stalls at it; with an ack timeout, it redelivers indefinitely) instead of "retry/DLQ paths";
• in Downgrade/Rollback, state the precondition in terms of backlog and retention across all replicated clusters, and say plainly that opting a producer into RSA_OAEP_SHA256 is not reversible until the last SHA-256-wrapped message has aged out everywhere.

If that is too sharp an edge for an opt-in, the alternative is a consumer capability signal rather than an ordering instruction - but I would rather see the ordering instruction stated accurately than replaced with machinery, given that it is opt-in and off by default.

[DESIGN] the phase-1 validator asserts phase-2 features, and on Basic auth it green-lights a broker that cannot authenticate anyone

These two checks assert features that phase 2 delivers, while (g) itself is phase 1 (:647-652).

• saslRoleTokenSignerAlgorithm is introduced by (e), phase 2. In a phase-1 release the key does not exist, so this check is either dead or unsatisfiable - fipsMode=true with SASL enabled could never start.
• The Basic-auth check is worse than unsatisfiable, because it passes. (d) - phase 2 - is what teaches AuthenticationProviderBasic to verify $5$/$6$ entries. In a phase-1 release a $6$-only password file satisfies this bullet, the broker starts and logs the security-posture summary at :527-532 saying the FIPS check passed, and every login then fails, because authenticate still routes the entry to the DES branch:

A validator that green-lights a broker which cannot authenticate anyone is the same class of outcome as the silent non-compliance the fail-fast design exists to prevent (:543-545) - it just fails in the other direction.

Either move these two checks into phase 2 beside the features they assert, or pull (d) and (e) into phase 1. Either way, "Each phase is independently shippable" at :652 needs replacing with the actual dependency statement - the phases are shippable in order, which is a weaker and true claim.

[DESIGN] fipsMode screens the configured keystore types, but the carrier keystore the TLS stack builds for PEM material is JKS or PKCS12

This bullet screens the operator-facing tlsKeyStoreType / tlsTrustStoreType family. A PEM deployment - which the High Level Design explicitly blesses at :216-217 ("Keystores may be PEM, BCFKS, or PKCS11") - configures no store type at all, so it satisfies the check vacuously. But the JDK TLS path still materialises PEM material into a process-local carrier keystore, and that carrier's type is decided here:

With no jcaProvider pinned, inMemoryStoreType returns the caller's default - and there are two callers with two different defaults:

• KeyStoreHolder, which JdkSslContexts uses to build the key and trust managers, passes KeyStore.getDefaultType(), controlled by the JVM's keystore.type setting;
• TlsKeyStoreLoader.toInMemoryKeyStore passes the literal "PKCS12".

A caller default of JKS or PKCS12, and the explicit PKCS12 carrier, would use types this bullet rejects. As written, fipsMode=true can pass on a deployment whose TLS key material is sitting in a JKS carrier - the "FIPS-shaped rather than FIPS-compliant" outcome the switch exists to prevent.

The keystore.type=bcfks guidance in the deployment guide fixes the first, because it changes KeyStore.getDefaultType(); it does not touch the second, which is a hardcoded literal. And a java.security line an operator may or may not have written is not what a fail-fast validator is for.

The fix makes the validator stronger rather than longer: have fipsMode require jcaProvider (and brokerClientJcaProvider) to be pinned to a provider that registers BCFKS, instead of only screening configured store types. Under a pinned BC-FIPS provider inMemoryStoreType selects BCFKS at both call sites and the problem disappears. It also resolves an ambiguity in the current wording: "PKCS12 is rejected" does not say whether it reaches the carrier, and if it does, the PKCS12 entry in the carrier preference list has to be unreachable in FIPS mode rather than merely second choice.

[SIMPLIFICATION] (d) collapses to deleting one .substring(0, 2); the API it is designed around does not exist

Worth checking this section against commons-codec before writing any of it: Crypt.crypt(byte[], String) already is the prefix dispatcher this design proposes to build. From the bytecode of the 1.21.0 jar this build resolves (the catalog pins commons-codec = "1.22.1"; the dispatch has been there for many releases):

crypt(byte[] keyBytes, String salt):
    salt == null            -> Sha2Crypt.sha512Crypt(keyBytes)
    salt.startsWith("$6$")  -> Sha2Crypt.sha512Crypt(keyBytes, salt)
    salt.startsWith("$5$")  -> Sha2Crypt.sha256Crypt(keyBytes, salt)
    salt.startsWith("$1$")  -> Md5Crypt.md5Crypt(keyBytes, salt)
    otherwise               -> UnixCrypt.crypt(keyBytes, salt)   // DES

The only reason $5$/$6$ do not work in Pulsar today is that the existing call truncates the salt to two characters:

So the whole of (d)'s new code is: pass encryptedPassword where encryptedPassword.substring(0, 2) is passed now. That is a much better story for the PIP than the one currently written, and it removes the need for the design paragraph at :367-373 entirely.

Two corrections that follow:

• Sha2Crypt.crypt(password, storedHash) (:372) does not exist. Sha2Crypt's public surface is sha256Crypt(byte[]), sha256Crypt(byte[], String), sha256Crypt(byte[], String, Random) and the three sha512Crypt overloads - no crypt. The semantics the sentence describes are right (Sha2Crypt.SALT_PATTERN ends in .*, so passing the full stored hash as the salt works, rounds=N$ included); only the entry point is wrong, and the right one is Crypt.crypt.
• The same one-line change also starts accepting $1$ MD5-crypt entries, which today reach the DES branch with a 2-character salt and therefore can never verify. An entry that has always denied every password would start allowing the right one. That is a real behaviour change and belongs in the compatibility table at :563 - and it is a good argument for fipsMode's startup scan rejecting $1$ and $apr1$ explicitly rather than by omission.

What survives from (d) is the part that is genuinely Pulsar's: the fipsMode startup scan of the password file, and the choice to validate at startup rather than per request.

[ACCURACY] every remaining dependency on unmerged work has merged; five passages still describe master as it stood in August

#26326 merged on 2026-08-27 and #26154 merged on 2026-08-28 - both before this reconciliation commit was pushed. The net effect is good for the PIP: it now has no dependency on unmerged work at all. But five passages still describe master as it stood in August, and one of them is a falsifiable claim about code.

• This line - "the remaining follow-ups, including the jcaProvider configuration surface, are in #26326".
• :159-162 - "the outstanding piece it does depend on is #26326, which honors the jsseProvider keys and adds the jcaProvider ones."
• :241-243 - "on master today the axis is reachable only programmatically through the v5 builder". It is not: ServiceConfiguration.getJcaProvider() is read at DefaultBrokerTlsFactory.java:186, and conf/proxy.conf ships a commented-out jcaProvider= key.
• :257-261 - "DefaultBrokerTlsFactory resolves its JSSE provider as resolveJsseProvider(null, conf.getTlsProvider()), passing null for the explicit axis, so the broker's own jsseProvider / brokerClientJsseProvider keys are declared but ignored". master reads DefaultBrokerTlsFactory.java:148 - resolveJsseProvider(conf.getJsseProvider(), conf.getTlsProvider()) - and the broker-client leg reads conf.getBrokerClientJsseProvider() a few lines below. The whole bullet, including "FIPS mode depends on that axis, so this PIP assumes #26326", can go.
• :501-502 and :506-508 - the Configuration table's two provider rows ("Declared but ignored on master", "Added by #26326") and the paragraph beneath it.

Worth fixing for exactly the reason the July reconciliation was worth doing: the gap list is the current-state claim a voter checks, and this one is now checkable against a tree that says something else.

[DESIGN] saslRoleTokenSignerAlgorithm encodes two axes in one enum, and one of its values duplicates fipsMode

saslRoleTokenSignerAlgorithm encodes two independent axes in one enum: which algorithm to sign with, and whether to accept legacy signatures. That shape does not survive a second algorithm - HmacSHA384 would need both HmacSHA384 and HmacSHA384-strict, and the value space becomes a mix of a digest name (SHA-512, which is not a MAC algorithm name), MAC names, and a policy suffix. A 3-value enum that is really 2xN is the kind of thing worth getting right before the key is public.

-strict also duplicates something the design already expresses: :398 says it is implied by fipsMode=true, and the PIP does not say whether a non-FIPS operator who wants strict mode exists. If they do, name them and give it its own boolean; if they do not, saslRoleTokenSignerAlgorithm = SHA-512 | HmacSHA256 for the signing algorithm and fipsMode for legacy acceptance covers everything with one fewer value and no suffix grammar.

Separately, and as a positive: the &s=v2: dispatch claim at :391-393 is sound, but for a better reason than the one given. The document argues from the Base64 alphabet's "output position", which is a bit indirect. The direct arguments are that SaslRoleToken.toString() emits u=<role>&i=<session>&e=<expires> - the session attribute is i, not s - so &s= occurs exactly once; that verifyAndExtract uses lastIndexOf(SIGNATURE) regardless; and that the legacy signature is Base64.getEncoder() output over a 64-byte digest, so 88 characters from A-Za-z0-9+/=, containing neither : nor &. Stating those three makes the claim robust against a future change to SaslRoleToken's attribute letters, which the current phrasing would not survive.

[DESIGN] two alternatives are missing, one of which would change the shape of the whole proposal

Two alternatives are missing, and one of them would change the shape of the whole proposal.

A validated JVM or PKCS#11 provider instead of BC-FIPS. SunPKCS11 over a CMVP-validated NSS or OpenSSL FIPS token, or a vendor JDK that ships a validated provider, is a mainstream FedRAMP path. It would avoid the BC jar swap entirely - and with it the shaded-client conflict at :290-294, the second tarball at :283-285, and the certified-versus-patched bc-fips question. The PIP treats BC-FIPS as the only Java-crypto answer without arguing why, and it is already assuming half the mechanism: PIP-478's PulsarTlsFactory PKCS#11 hook is cited at :243-245 for the Level 3 case. Even if the answer is "BC-FIPS, because the jar swap is simpler than provisioning a token", that argument is worth making where a reader can weigh it.

Documentation and tests only - no fipsMode key, no distribution variant. Given Motivation 1's own "the gap is not missing machinery" (:91-92) and "what is missing is that nothing in the project supports, exercises, or documents the swap" (:94), this is the honest minimal competitor, and nothing in the PIP currently argues against it. It is also the baseline every new surface in this proposal should be justified against - which is a useful discipline for the review, not a criticism of the goal.

While here: Alternative 5 (:618-620) reads as a strawman - nobody is proposing a fork with different defaults, and the phase-2 distribution/server-fips plus the grpc-netty substitution at :295-300 already make this PIP a partial instance of the second-artifact cost it rejects there.

[SCOPE] make startup selection of bundled mode-specific dependencies the standard distribution path

General Notes already describes separate provider directories and selecting one at startup (pip-489.md:633-637). Could we promote that to the supported path in the standard distribution? Section (b) still specifies manual jar replacement and an optional second tarball, without committing to bundle both choices.

The review body gives the concrete launcher example: keep common dependencies in lib/, ship BC-FIPS in lib-fips/, and ordinary BC plus netty-tcnative-boringssl-static and Conscrypt in lib-non-fips/. PULSAR_FIPS_MODE=true selects lib/*:lib-fips/*; unset or false selects lib/*:lib-non-fips/*. The change appends the selected directory to PULSAR_CLASSPATH in the existing distribution branch, preserving the launcher's later additions. This supplies both modes in one distribution while keeping the other mode's jars off the selected classpath.

The directory split implements the tcnative/Conscrypt classpath exclusions; the gRPC substitution already specified in (b) needs the same mode-specific placement. Approved-only mode, provider configuration and fipsMode validation still apply. This is packaging support, not a claim that the environment variable alone establishes compliance. Please clarify whether the requirement at pip-489.md:295-308 to ship no BoringSSL object code rules out bundling inactive ordinary-mode dependencies in the same archive.

Both startup modes should be tested from the assembled archive, including provider exclusivity and a function runtime. The proposed tests (pip-489.md:463-472) omit the grpc-netty-shaded to grpc-netty substitution, which couples the function control channel to Pulsar's independently pinned Netty.

[ACCURACY] (c)'s SecureRandom bullet describes a gap #26154 closed on 2026-08-28

MessageCryptoBc no longer pins NativePRNGNonBlocking. Since #26154 merged on 2026-08-28 it prefers the BC-FIPS DRBG whenever the BCFIPS provider is registered, and NativePRNGNonBlocking is the fallback for everyone else:

So this bullet belongs in the "already closed" column rather than the gap list, and the "(open)" beside #26154 in General Notes (:642-643) wants updating too. That shortens (c) rather than lengthening it.

One completeness point while you are in this list: the org.bouncycastle.jce.* bullet above names four imports at lines 74-77; there is a fifth immediately below, MessageCryptoBc.java:78 (org.bouncycastle.jce.spec.IESParameterSpec), equally absent from bc-fips. Since the whole point of that item is that the class fails to load, the complete list is the useful one.

[NIT] six line citations have drifted; the rest are exact

Checked every file:line in the document against master. Six have drifted:

These are exact and unchanged, for what it is worth - the drift is not systematic:

distribution/server/build.gradle.kts:154 and :59
pulsar-common/build.gradle.kts:185-190
DefaultBrokerTlsFactory.java:112            (the sentence quoted from its comment is verbatim)
ServiceConfiguration:217
WebSocketProxyConfiguration:256
MessageCryptoBc:92 and :74-77
AuthenticationProviderBasic:141-149
SaslRoleTokenSigner:92-104                  (and MessageDigest.isEqual at :79, as (e) says)
KubernetesRuntime:1141 and :322, 511, 601, 730, 886
pulsar-functions/runtime/build.gradle.kts:63

A suggestion rather than a request: a PIP outlives its line numbers, and this document is on its second reconciliation pass largely because of them. Citing the symbol - DefaultBrokerTlsFactory.fromServiceConfiguration, JcaKeyStores.inMemoryStoreType, AuthenticationProviderBasic.authenticate - except where the line genuinely is the point would make the next pass much cheaper, and would have survived both #26322 and #26326 untouched.