Run the Security plugin under FIPS approved-only mode with BC providers

[beanuwave]

Description

Category: Enhancement, New feature, Bug fix

This branch makes the Security plugin run correctly under FIPS approved-only mode (BCFIPS as the sole crypto provider). Every change traces to one of: an algorithm/format/usage that FIPS disallows (whether or not BCFIPS rejects it at runtime), a weakness surfaced during the audit, or build/packaging plumbing to make FIPS the always-available baseline.

At a glance:

• Single source of truth: FipsMode.isEnabled() (env OPENSEARCH_FIPS_MODE=true) replaces CryptoServicesRegistrar.isInApprovedOnlyMode() everywhere. Intent is decoupled from provider state and cross-checked at startup.
• Build & FIPS activation: BCFIPS is always built (compileOnly, shipped by core); the compile-time FipsBuildParams fork is gone. FIPS engages purely through the java.security the JVM loads (no provider is registered in code) - the runtime launcher merges in fips_java.security on OPENSEARCH_FIPS_MODE=true, while tests swap it wholesale (-D...properties==<file>).
• OBO/JWT tokens: encryption rewritten from default-mode AES (ECB) -> AES-GCM + HKDF (fixes a FIPS/NIST violation and a deterministic-crypto weakness), plus an entropy floor, lazy fail-closed init, an optional BCFKS keystore for the keys, and secret redaction.
• Keystores / TLS: JKS/PKCS12 out, BCFKS in (PKCS11 opt-in, signed via SunJSSE); PemKeyReader rebuilt on BouncyCastle; TLSv1.1 dropped in FIPS mode.
• LDAP: SNI re-implemented for BCTLS, and ldaptive's non-FIPS internal PKCS12 key-copy bypassed.
• Auth hardening: Kerberos cleanup, timing-safe dummy hash, a 14-char password floor in FIPS, approved-DRBG randomness, key-material zeroing.
• CLI: securityadmin accepts BCFKS/PKCS11 and now launches via core's opensearch-cli.

Key changes

OBO / JWT tokens

The OBO key path was audited end-to-end; the encryption rewrite is the FIPS trigger, the rest are weaknesses found alongside it.

• Encryption rewrite. EncryptionDecryptionUtil: built its cipher with the bare Cipher.getInstance("AES") and a key forced to 16 bytes via Arrays.copyOf (silently truncating or zero-padding) -> now AES-256-GCM (random 12-byte IV, 128-bit tag), key derived via HKDF-SHA256. A bare "AES" transformation resolves to the JCE provider's default mode - ECB - which for data confidentiality violates NIST SP 800-38A² (ECB is approved only for key wrapping, SP 800-38F³). The mode is never spelled out or configurable, and BC FIPS permits the ECB primitive at runtime - so this had to be caught by review, not the provider.
• Entropy floor (SP 800-133r2⁵). HKDF can't create entropy, so a short encryption_key would produce a nominal AES-256 key with sub-112-bit strength. A FIPS guard rejects input keying material < 32 bytes and zeroes the IKM after derivation.
• Signing-key length check. Validates the decoded key bytes; measuring the Base64 string length would over-count by ~4/3 and let a 384-bit key pass a 512-bit gate.
• Lazy, fail-closed init. OnBehalfOfAuthenticator initializes lazily and atomically: the constructor is inert, the first token request triggers init, a failure logs once and declines - a bad key can't throw out of the constructor, propagate through config reload, and retry forever. Mirrors ApiTokenAuthenticator.
• Keystore support. The OBO signing and encryption keys can now be loaded from a BCFKS keystore (KeyUtils.loadKeyFromKeystore) instead of inline config, keeping the secrets out of cluster state. Relative *_keystore_path values resolve against the node config dir (consistent with other security file settings). This is an SP 800-57⁶ key-at-rest hardening (protecting keying material / limiting exposure), not a FIPS 140-3¹ requirement.
• Secret redaction (CWE-532). OnBehalfOfSettings.toString() redacts the signing_key/encryption_key values so they no longer leak into logs.

Keystores / TLS

• PemKeyReader rewritten onto BouncyCastle (PEMParser / JcaPEMKeyConverter / PKCS8 decryptor) instead of raw JCE; adds BCFKS + PKCS11 and store-type auto-detection.
• SSLConfigConstants: both defaults are now FIPS-conditional - default store type is forced to BCFKS in FIPS, and ALLOWED_SSL_PROTOCOLS drops TLSv1.1 in FIPS. Both LDAP backends now reuse ALLOWED_SSL_PROTOCOLS for their default enabled_ssl_protocols.
• PKCS#11 keys are signed via SunJSSE, not BCJSSE. A PKCS#11/HSM private key is non-exportable and BCJSSE can't sign with it (no encoding for key); SunJSSE delegates the handshake signature to the key's own provider (SunPKCS11). So when the keystore is PKCS#11, SslConfiguration builds the SSLContext against SunJSSE (SslContextBuilder.sslContextProvider(SunJSSE)).
• The BC FIPS provider is declared, not instantiated. main self-registered it at plugin load (OpenSearchSecuritySSLPlugin.tryAddSecurityProvider() -> Security.addProvider(new BouncyCastleFipsProvider())); that method is removed. Providers now come solely from the active java.security file (JCA lazy-loads them), so FIPS vs non-FIPS is a launch-time provider swap (BCJSSE vs SunJSSE) with no code branch - the security files are a core/distribution concern.

LDAP

• SNI for BCTLS - two sequential (not duplicate) concerns: SNISettingTLSSocketFactory sets the ClientHello SNI before the handshake so a multi-cert server serves the right cert; HostnameVerifyingTrustManager checks the returned cert after. For IP targets the SNI factory early-returns (no SNI) and the trust manager is the only hostname check; for DNS it sets SNI + endpointIdentification (the trust-manager hostname check is then redundant). HostnameAwareConnectionFactory threads the target hostname through so SNI works.
• Avoiding ldaptive's internal PKCS12 copy (both authentication and authorization paths) - ldaptive's create*CredentialConfig with key aliases routes through KeyStoreSSLContextInitializer.getKeyManagers(), which copies the private key into a fresh in-memory PKCS12 store; SunJCE protects it with PBEWithHmacSHA256AndAES_256, not available in FIPS. Fix: build BCFKS keystores from PEM via PemKeyReader.toTruststore/toKeystore and pass null key aliases, so kmf.init(keystore, password) is called directly and the PKCS12-copy branch is bypassed. (LDAPAuthorizationBackend previously used createX509CredentialConfig, which hit the same path unconditionally.)
• Extracted the default backend's Java9CL into a shared SocketFactoryClassLoader (resolves the JNDI socket-factory classes by name) and set it on ldap2's JNDI provider config, so PrivilegedProvider's thread-context swap resolves the socket factory on reconnect. Fixes an ldap2 LDAPS reconnect ClassNotFoundException.

Auth hardening (found mid-audit)

• HTTPSpnegoAuthenticator: no longer mutates global System.setProperty debug flags; stops logging the acceptor principal; proper LoginContext.logout() + decoded-header zeroing in finally.
• InternalAuthenticationBackend + PasswordHasher.getDummyHash(): not-found timing path now uses the configured hasher (closes user-enumeration side-channel under PBKDF2).
• Password-length floor. PBKDF2 keys are derived from the password itself, and BC FIPS rejects key material under 112 bits (< 14 ASCII chars) at hashing time. PasswordValidator.FIPS_MIN_PASSWORD_LENGTH (14) now anchors both ends: FIPS raises an unset restapi.password_min_length to 14, and startup rejects a lower explicit value - otherwise the REST API accepts passwords the hasher then refuses.
• Randomness from core. Randomness.createSecure() replaces new SecureRandom() for OBO encryption, api-tokens, and user passwords - it resolves to the approved SP 800-90A⁴ DRBG in FIPS. UserService generates 20-27 chars in FIPS (>=119 bits over the 62-char alphabet), 8-15 otherwise; char[] zeroed in finally.

CLI

• SecurityAdmin: accepts BCFKS/PKCS11; PKCS#11 PIN prompt. Launcher scripts now delegate to core's shared opensearch-cli. Standalone bundle ships BCFIPS jars under deps/.
• SecurityAdmin.buildPkcs11SslContext routes PKCS#11 keys through SunJSSE client-side (same as the server TLS layer - see Keystores/TLS).

Note

Reviewer call-outs

1. OBO token wire-format break - GCM-encrypted tokens are not interchangeable with old AES-ECB tokens across the upgrade boundary. Benign in practice (OBO TTL < 10 min, self-heals shortly after rollout completes), but worth a release note for hot/rolling deployments.
2. securityadmin now launches via core's opensearch-cli - this targets the in-distribution path; the standalone bundle is no longer self-launching and is effectively deprecated.
3. Test strategy diverges from core's convention - this branch runs one dual-mode suite (skip incompatible cases at runtime), whereas the core project forks the suite: a FIPS test class extends the non-FIPS one.
4. Dropped the auto System.setProperty(disableEndpointIdentification, true) from the default ldap backend - now warn-only. Two reasons: (a) it aligns both backends on the same logic (ldap2 never set it); (b) mutating a global JVM property from application code is bad practice - it's process-wide and load-order-dependent, so one auth-domain's verify_hostnames: false silently reconfigured hostname checking for the whole JVM. Operators who need it must now set the -D flag deliberately.
5. Why are verify_hostnames and trust_all coupled to the same verifier? - verifyHostnames = !trustAll && <setting>, so trust_all: true forces AllowAnyHostnameVerifier on top of AllowAnyTrustManager. Chain validation and hostname matching are orthogonal concerns; collapsing them onto one code path means you can't relax one without the other and hides which layer a config change actually touches. Worth untangling so each parameter maps to exactly one verification layer.

Core / distribution follow-ups

Not plugin changes - each fix belongs in core or the distribution, tracked here so it can be routed:

• bctls-fips has no socket connect grant. Core's base security.policy grants bc-fips/bcpkix-fips but not bctls-fips, so under the agent all outbound BCJSSE TLS (LDAPS, audit sinks, remote reindex/snapshot over https) is denied - and plugin policies can't grant a core lib/ jar. Verified against a live LDAPS-over-FIPS cluster.
• JUL->log4j bridge not active for BC FIPS at rollout. LogConfigurator sets java.util.logging.manager at runtime, too late for the BouncyCastle FIPS JSSE provider (which initialises JUL during bootstrap); its per-handshake INFO traces then leak to System.err as [WARN][stderr] spam. Fix in the distribution: set -Djava.util.logging.manager=org.apache.logging.log4j.jul.LogManager as a launch-time -D in jvm.options, and default org.bouncycastle.jsse to warn in the shipped log4j2.properties.
• Core's org.opensearch.fips.FipsMode still detects FIPS via CryptoServicesRegistrar.isInApprovedOnlyMode() - the provider-state probe this branch replaced with the env var; it should follow.
• BCFipsEntropyDaemonFilter should move into core's test framework. The local thread-filter accommodation for the "BC FIPS Entropy Daemon" thread belongs in core's BouncyCastleThreadFilter, not this plugin's test sources.

Additional notes

• JKS can't hold SecretKey entries (engineSetKeyEntry requires PrivateKey) - relevant to the JWT-signing-key path; BCFKS is the only FIPS-approved store that holds secret keys.
• Provider list is a complete override; SunJGSS is deliberately retained for Kerberos/SPNEGO.
• Operational: keystore passwords must be >=14 chars (BCFIPS); user passwords likewise (the 112-bit PBKDF2 floor, now enforced - see Auth hardening); the "BC FIPS Entropy Daemon" thread required a test thread-filter accommodation.
• SAML is scoped out - both SAML stacks it uses (OneLogin java-saml and OpenSAML/Shibboleth) are not FIPS-compliant (tests skip in FIPS mode).
• HTTP/3 exclusion is a build-level constraint, not a categorical ban - a FIPS-certified BoringSSL substituted at the OS level re-enables it.
• The SNI plumbing (SNISettingTLSSocketFactory + the SniAwareConnection decorator + hostname ThreadLocal + HostnameAwareConnectionFactory) works around the JNDI LDAP provider resolving hostnames to IPs before socket creation (bcgit/bc-java#460); ldaptive 2.x's native (Netty) transport opens sockets with the real hostname, which would let this entire stack be deleted (out of scope here).

Testing

The suite runs in non-FIPS mode by default. To exercise the FIPS code paths, set the environment variable before invoking Gradle:

OPENSEARCH_FIPS_MODE=true ./gradlew test integrationTest

When set, the build swaps in the FIPS java.security policy (BCFIPS-only providers), enables -Dorg.bouncycastle.fips.approved_only=true, and points the JVM at the BCFKS truststore. FIPS-incompatible tests (BCrypt, Argon2, SAML, SSLv3, JKS/PKCS12, weak/short passwords) are auto-skipped via JUnit assumptions. Static bcrypt fixtures and their short demo passwords are rewritten to PBKDF2 and padded past the 14-char floor by FipsHashAdapter (a no-op outside FIPS), and a few timing-sensitive integ tests scale down under FIPS, where PBKDF2 logins and BCTLS handshakes are markedly slower.

For a running cluster, select the FIPS-approved password hasher in opensearch.yml (BCrypt/Argon2 are not available in approved-only mode):

plugins.security.password.hashing.algorithm: pbkdf2

The demo hashes in config/opensearch-security/internal_users.yml are BCrypt, which won't verify under PBKDF2 - regenerate the hash for each test account (e.g. with tools/hash.sh) and replace it before applying the security config.

Test securityadmin.sh with BCFKS + PKCS#11 keystores (SoftHSM)

Exercises a token-resident node TLS key (signed via SunJSSE) and securityadmin authenticating with a PKCS#11 client key. This example is FIPS-specific, but adjusts easily to non-FIPS by registering a SunPKCS11 provider via OPENSEARCH_JAVA_OPTS="-Djava.security.properties=$OPENSEARCH_HOME/config/java.security" instead of OPENSEARCH_FIPS_MODE=true. All paths below are relative to $OPENSEARCH_HOME.

# 1. Set up a test cluster.
...
cd $OPENSEARCH_HOME
sh bin/opensearch-keystore create --password
sh plugins/opensearch-security/tools/install_demo_configuration.sh -y -i -s

# 2. Init token - the --pin becomes the keystore/truststore password.
softhsm2-util --init-token \
  --free \
  --label opensearch \
  --so-pin 4321 \
  --pin 1234

# 3. Register the provider in config/fips_java.security:
      security.provider.<n>=SunPKCS11 /path/to/config/softhsm-pkcs11.cfg
#    with softhsm-pkcs11.cfg in the same dir:
      name = SoftHSM
      library = /usr/lib/softhsm/libsofthsm2.so
      slotListIndex = 0

# 4. Import node + admin keys WITH chains (PKCS#12 -> token).
#    Repeat the keytool step for kirk (swap -name / -srcalias / -destalias).
openssl pkcs12 -export \
  -inkey config/esnode-key.pem \
  -in config/esnode.pem \
  -certfile config/root-ca.pem \
  -name esnode-cert \
  -out /tmp/esnode.p12 \
  -passout pass:1234

jdk/bin/keytool \
  -importkeystore \
  -srckeystore /tmp/esnode.p12 \
  -srcstoretype PKCS12 \
  -srcstorepass 1234 \
  -srcalias esnode-cert \
  -destkeystore NONE \
  -deststoretype PKCS11 \
  -deststorepass 1234 \
  -destalias esnode-cert \
  -addprovider SunPKCS11 \
  -providerarg config/softhsm-pkcs11.cfg

# 5. Trust anchor -> BCFKS file.
jdk/bin/keytool -importcert -noprompt \
  -alias root-ca \
  -file config/root-ca.pem \
  -keystore config/root-ca.bcfks \
  -storetype BCFKS \
  -storepass changeit \
  -providerClass org.bouncycastle.jcajce.provider.BouncyCastleFipsProvider \
  -providerPath lib/bc-fips-2.1.2.jar

# 6. In opensearch.yml: comment the demo *.pem*_filepath lines, keep admin_dn: CN=kirk,...,
#    and add for both transport and http:

# --- Node identity from the PKCS#11 token ---
plugins.security.ssl.transport.keystore_type: PKCS11
plugins.security.ssl.transport.keystore_alias: esnode-cert
plugins.security.ssl.transport.keystore_password: "1234"      # SoftHSM PIN
plugins.security.ssl.http.keystore_type: PKCS11
plugins.security.ssl.http.keystore_alias: esnode-cert
plugins.security.ssl.http.keystore_password: "1234"

# --- Trust anchor from a BCFKS file ---
plugins.security.ssl.transport.truststore_type: BCFKS
plugins.security.ssl.transport.truststore_filepath: root-ca.bcfks
plugins.security.ssl.transport.truststore_password: "changeit"
plugins.security.ssl.http.truststore_type: BCFKS
plugins.security.ssl.http.truststore_filepath: root-ca.bcfks
plugins.security.ssl.http.truststore_password: "changeit"

# 7. Run the cluster and apply security config with the PKCS#11 admin key.
#    Pass = 'Connected as "CN=kirk,..."' followed by 'Done with success'.
OPENSEARCH_FIPS_MODE=true sh plugins/opensearch-security/tools/securityadmin.sh \
  -cd config/opensearch-security/ \
  -icl \
  -nhnv \
  -cacert config/root-ca.pem \
  -kst PKCS11 \
  -kspass 1234 \
  -ksalias kirk

Test LDAP authentication over LDAPS (SNI, hostname verification, mTLS)

Self-contained manual tests for the LDAP TLS changes: SNI / hostname verification, mutual TLS, and the TLS protocol floor. Authentication only - these changes don't touch authz code, so role resolution is out of scope (verify that against a real directory).

Run them against any LDAPS directory that supports mTLS (a client cert is required). The walkthrough uses a local UnboundID in-memory stand-in only because it's repeatable and trivial to set up - a convenience, not a requirement; substitute your own server anywhere it appears. Its setup lives in LDAP_UNBOUNDID_STANDIN_GUIDE.md.

TLS material is the OpenSearch install's own demo certs in $OPENSEARCH_HOME/config/ - esnode (server, SAN includes localhost), root-ca.pem (trust anchor), kirk (client). Run the node with OPENSEARCH_FIPS_MODE=true (omit for non-FIPS; the only observable difference is the TLS protocol floor).

FIPS agent-build gotchas (both are Core / distribution follow-ups): (1) core's base security.policy grants bc-fips/bcpkix-fips but not bctls-fips, so every LDAPS bind is denied under the agent; (2) the JUL->log4j bridge isn't active for BC FIPS JSSE at handshake time, so each bind's INFO traces leak to stderr as [WARN][stderr] spam (noise, not a failure).

cd $OPENSEARCH_HOME
export LDAP_USER="testuser"; export LDAP_PASS="testpassword"   # cn=Test User,ou=people,o=TEST

# Optional trace logging for the "what to look for" lines (config/log4j2.properties):
#   logger.ldap.name=org.opensearch.security.auth.ldap
#   logger.ldap.level=trace
#   logger.ldap2.name=org.opensearch.security.auth.ldap2
#   logger.ldap2.level=trace
#   logger.ldaptive.name=org.ldaptive
#   logger.ldaptive.level=debug

# === Baseline config (authc.ldap.authentication_backend.config; o=TEST, no authz block) ======
# Edit this first, then apply below. Bare *_filepath resolve against config/; kirk + root-ca are the
# shipped demo certs, so as shipped this IS scenario 1a / 2a -> 200. Run every scenario once per
# backend (flip the type: line).
#   type: ldap                  # DEFAULT | LDAP2: org.opensearch.security.auth.ldap2.LDAPAuthenticationBackend2
#   config:
#     enable_ssl: true
#     enable_ssl_client_auth: true          # mTLS - client cert required
#     verify_hostnames: true
#     hosts: [localhost:8636]
#     pemtrustedcas_filepath: root-ca.pem
#     pemcert_filepath: kirk.pem
#     pemkey_filepath: kirk-key.pem
#     bind_dn: "cn=opensearch-bind,ou=people,o=TEST"
#     password: "bindpassword"
#     userbase: "ou=people,o=TEST"
#     usersearch: '(uid={0})'
#     username_attribute: uid

# === Apply / authenticate (verify loop - re-run after each config.yml edit) ==
# apply: push the authc.ldap block (-t config, live; TLS-material/host edits need a node restart).
sh ./plugins/opensearch-security/tools/securityadmin.sh \
  -f ./config/opensearch-security/config.yml \
  -t config \
  -icl \
  -nhnv \
  -cacert config/root-ca.pem \
  -cert config/kirk.pem \
  -key config/kirk-key.pem \
  -h localhost \
  -p 9200

# authenticate: 200 + user_name=testuser (backend_roles empty - no authz) = LDAPS + mTLS bind OK.
curl -sk -u "$LDAP_USER:$LDAP_PASS" https://localhost:9200/_plugins/_security/authinfo?pretty

# What to look for (baseline 200):
#   Configuring SNI for hostname: localhost ...            # SNI server_name set (the fix)
#   checkServerTrusted ... succeeded                       # esnode chains to root-ca
#   verifyDNS found hostname match: localhost              # hostname layer 1 pass
#   Opened a connection, total count is now 1              # DEFAULT | LDAP2: Authenticated username testuser

Test matrix. Run every scenario in all four cells - flip the backend on type:; for FIPS set OPENSEARCH_FIPS_MODE=true (launcher loads fips_java.security -> BCJSSE), for non-FIPS set OPENSEARCH_JAVA_OPTS="-Djava.security.properties=$OPENSEARCH_HOME/config/java.security" (BCFIPS stays declared - the FIPS installer converts the node stores to BCFKS - but TLS runs on SunJSSE). Launch-time provider swap, no code path (see Keystores / TLS). Outcomes are identical; only the protocol floor ([TLSv1.3, TLSv1.2] FIPS vs + TLSv1.1 non-FIPS) and the provider differ, so the Prov*/TlsFatalAlert class names in the excerpts are BCJSSE-only.

	DEFAULT ldap	LDAP2 (...ldap2.LDAPAuthenticationBackend2)
FIPS	yes	yes
non-FIPS	yes	yes

Scenario 1 - hostname verification. Same trusted esnode cert throughout; 1b-1d dial a name not in its SAN (echo "127.0.0.1 ldap-wrong.example.com" | sudo tee -a /etc/hosts, then set hosts: [ldap-wrong.example.com:8636]), so the only thing that can object is one of the two hostname guards: (1) ldaptive's verifier (verify_hostnames), (2) JNDI endpoint-id (-Dcom.sun.jndi.ldap.object.disableEndpointIdentification=true in config/jvm.options; the plugin no longer sets it, only warns). Chain trust is valid throughout, so this isolates hostname checking - the untrusted-cert case is 2c. Apply + restart per row.

	verify_hostnames	JNDI endpoint-id	Result / what it proves
1a	true	on	correct name -> 200 (baseline; SNI fix works)
1b	true	on	rejected, layer 1 - ldaptive DefaultHostnameVerifier
1c	false	on	rejected, layer 2 - JNDI/BC endpoint-id (verify_hostnames: false alone isn't enough)
1d	false	off	accepted (200) - hostname unenforced only when both guards are off

1b  HostnameVerifyingTrustManager ... hostnames=[ldap-wrong.example.com] failed         # layer 1 (ldaptive)
    CertificateException: Hostname '[ldap-wrong.example.com]' does not match 'CN=node-0.example.com...'
    (for ldap2 the reject fires inside SniAwareConnection.open() = no fail-open)
1c  AllowAnyHostnameVerifier ... succeeded                                              # layer 1 off
    CertificateException: No subject alternative name found matching domain name ldap-wrong.example.com  # layer 2
        at org.bouncycastle.jsse.provider.ProvX509TrustManager.checkEndpointID(...)
1d  (no hostname reject) -> Opened a connection / Authenticated username testuser

Cleanup: remove the /etc/hosts line + the jvm.options flag, restore hosts: + verify_hostnames: true. Never set disableEndpointIdentification=true in production (it's process-wide).

Scenario 2 - mTLS client authentication. Vary only the client cert / trust anchor; apply + restart per row, and restore pemtrustedcas_filepath: root-ca.pem after 2c. First generate the two "bad" credentials once - both self-signed, so neither chains to the demo root-ca - into config/:

cd $OPENSEARCH_HOME/config
# 2b: untrusted client cert + key (self-signed; does NOT chain to root-ca)
openssl req -x509 -newkey rsa:2048 -nodes -days 30 -subj "/CN=untrusted-client" \
  -keyout untrusted-client.key -out untrusted-client.pem
# 2c: untrusted CA - a trust anchor that did NOT sign esnode
openssl req -x509 -newkey rsa:2048 -nodes -days 30 -subj "/CN=untrusted-ca" \
  -keyout untrusted-ca.key -out untrusted-ca.pem

	change (in config.yml)	Result / what it proves
2a	pemcert/pemkey_filepath: kirk.* (baseline)	accepted - PEM client key loads + signs the handshake under BC
2b	pemcert/pemkey_filepath: untrusted-client.*	rejected - BC withholds the client cert -> server aborts mandatory mTLS
2c	pemtrustedcas_filepath: untrusted-ca.pem	fails - server cert no longer chains to the trust anchor

2b  checkServerTrusted ... succeeded                        # server side fine
    received fatal(2) certificate_required(116) alert       # client cert withheld
    must NOT log: ClassNotFoundException ...SNISettingTLSSocketFactory  (ldap2 reconnect bug, fixed here)
2c  checkServerTrusted ... failed
    CertPathBuilderException: No issuer certificate for certificate in certification path found.
    TlsFatalAlert: certificate_unknown(46)  ->  Authentication finally failed

Test OnBehalfOf (OBO) token

Exercises OBO token issuance and verification against an already-running cluster, in three modes: keys inline in the dynamic config (Scenario A), held in a BCFKS keystore out of cluster state (Scenario B), or in a PKCS#12 keystore for non-FIPS builds (Scenario C). No restart needed - -t config pushes only the dynamic on_behalf_of block, picked up live. Pick ONE scenario, edit config.yml, then run the apply / issue / use steps. All paths are relative to $OPENSEARCH_HOME.

cd $OPENSEARCH_HOME
export ADMIN_AUTH="admin:<admin-password>"

# === Apply / issue / use (the verify loop - run after editing config.yml) ====
# apply: push the dynamic config (-t config = on_behalf_of block only, live reload).
sh ./plugins/opensearch-security/tools/securityadmin.sh \
  -f ./config/opensearch-security/config.yml \
  -t config \
  -icl \
  -nhnv \
  -cacert config/root-ca.pem \
  -cert config/kirk.pem \
  -key config/kirk-key.pem \
  -h localhost \
  -p 9200

# issue: generate a token.
export OBO_TOKEN=$(curl -sk \
  -u "$ADMIN_AUTH" \
  -X POST \
  -H 'Content-Type: application/json' \
  https://localhost:9200/_plugins/_security/api/generateonbehalfoftoken \
  -d '{
        "description": "obo test",
        "service": "test-service",
        "durationSeconds": "300"
      }' | jq -r '.authenticationToken')
echo "$OBO_TOKEN"

# use: a populated user_name / roles proves the verify side loaded the key,
#      checked the signature, and decrypted the roles.
curl -sk \
  -H "Authorization: Bearer $OBO_TOKEN" \
  https://localhost:9200/_plugins/_security/authinfo?pretty

# === Scenario A - inline keys ================================================
# signing_key >= 512 bits (64 bytes) for HS512; encryption_key >= 256 bits
# (32 bytes) for the FIPS IKM floor.
export OBO_SIGNING_KEY=$(openssl rand 64 | base64 -w0)
export OBO_ENCRYPTION_KEY=$(openssl rand 32 | base64 -w0)

# Put under config.dynamic.on_behalf_of in config/opensearch-security/config.yml:
#   on_behalf_of:
#     enabled: true
#     signing_key: "<value of $OBO_SIGNING_KEY>"
#     encryption_key: "<value of $OBO_ENCRYPTION_KEY>"
#
# Negative checks: a too-short encryption_key (e.g. ZW5jcnlwdGlvbktleQ==, 13 bytes)
# is declined in FIPS mode ("encryption_key is not strong enough for FIPS mode");
# tampering with the token's last segment makes the `use` step return no credentials.

# === Scenario B - BCFKS keystore =========
keytool \
  -genseckey \
  -alias obo-signing \
  -keyalg HmacSHA512 \
  -keysize 512 \
  -storetype BCFKS \
  -providername BCFIPS \
  -keystore config/obo.bcfks \
  -storepass kspass \
  -keypass keypass \
  -providerClass org.bouncycastle.jcajce.provider.BouncyCastleFipsProvider \
  -providerPath lib/bc-fips-2.1.2.jar

keytool \
  -genseckey \
  -alias obo-enc \
  -keyalg AES \
  -keysize 256 \
  -storetype BCFKS \
  -providername BCFIPS \
  -keystore config/obo.bcfks \
  -storepass kspass \
  -keypass keypass \
  -providerClass org.bouncycastle.jcajce.provider.BouncyCastleFipsProvider \
  -providerPath lib/bc-fips-2.1.2.jar

# Reference the keystore in config.yml (no inline keys). <key>_keystore_path is
# config-dir-relative.
#   on_behalf_of:
#     enabled: true
#     signing_key_keystore_path: "obo.bcfks"
#     signing_key_keystore_type: "BCFKS"
#     signing_key_keystore_alias: "obo-signing"
#     signing_key_keystore_password: "kspass"
#     signing_key_keystore_key_password: "keypass"
#     encryption_key_keystore_path: "obo.bcfks"
#     encryption_key_keystore_type: "BCFKS"
#     encryption_key_keystore_alias: "obo-enc"
#     encryption_key_keystore_password: "kspass"
#     encryption_key_keystore_key_password: "keypass"
#
# Same result as Scenario A, but no key material in the config index. GET
# /_plugins/_security/api/securityconfig shows only keystore references.

# === Scenario C - PKCS#12 keystore (NON-FIPS builds only) ====================
# A FIPS build rejects PKCS#12 (no FIPS-approved PKCS#12 in BC FIPS). PKCS#12 has
# no per-entry passwords, so use one value for -storepass and -keypass.
keytool \
  -genseckey \
  -alias obo-signing \
  -keyalg HmacSHA512 \
  -keysize 512 \
  -storetype PKCS12 \
  -keystore config/obo.p12 \
  -storepass kspass \
  -keypass kspass

keytool \
  -genseckey \
  -alias obo-enc \
  -keyalg AES \
  -keysize 256 \
  -storetype PKCS12 \
  -keystore config/obo.p12 \
  -storepass kspass \
  -keypass kspass

# Reference it in config.yml as in Scenario B with the PKCS#12 path/type (the
# loader falls back to the keystore password when _keystore_key_password is absent).
#   on_behalf_of:
#     enabled: true
#     signing_key_keystore_path: "obo.p12"
#     signing_key_keystore_type: "PKCS12"
#     signing_key_keystore_alias: "obo-signing"
#     signing_key_keystore_password: "kspass"
#     encryption_key_keystore_path: "obo.p12"
#     encryption_key_keystore_type: "PKCS12"
#     encryption_key_keystore_alias: "obo-enc"
#     encryption_key_keystore_password: "kspass"

Issues Resolved

Resolves RFC

Related to the series of FIPS PRs in the security plugin:

• Validate password hashing algorithm for FIPS #6126
• Facilitate FIPS-compliant store resolution #6059
• Add BCFKS keystore generation utilities #6087
• Refactor certificate revocation validation #6042
• Make security plugin aware of FIPS build param (-Pcrypto.standard=FIPS-140-3) #5952
• Add gradle.properties file to build security with -Pcrypto.standard=FIPS=140-3 by default #6004
• Replace the standard distribution of BouncyCastle with BC-FIPS #5439
• Moved OpenSAML jars to a Shadow Jar configuration to facilitate its use in FIPS enabled environments #5404
• Replace bouncy castle blake2b #4275
• Add support for PBKDF2 for password hashing & add support for configuring BCrypt and PBKDF2 #4524

Check List

• New functionality includes testing
• New functionality has been documented
• New Roles/Permissions have a corresponding security dashboards plugin PR
• API changes companion pull request created
• Commits are signed per the DCO using --signoff

References

1. FIPS 140-3 - Security Requirements for Cryptographic Modules
2. NIST SP 800-38A - Block Cipher Modes of Operation (ECB/CBC/CFB/OFB/CTR)
3. NIST SP 800-38F - Methods for Key Wrapping
4. NIST SP 800-90A Rev. 1 - Random Number Generation Using DRBGs
5. NIST SP 800-133 Rev. 2 - Recommendation for Cryptographic Key Generation
6. NIST SP 800-57 Part 1 Rev. 5 - Recommendation for Key Management

[github-actions]

PR Reviewer Guide 🔍

(Review updated until commit fd9ed30)

Here are some key observations to aid the review process:

🧪 PR contains tests

🔒 No security concerns identified

✅ No TODO sections

🔀 Multiple PR themes Sub-PR theme: LDAP SNI and BCTLS integration Relevant files: src/main/java/org/opensearch/security/auth/ldap/backend/LDAPAuthorizationBackend.java src/main/java/org/opensearch/security/auth/ldap2/SNISettingTLSSocketFactory.java src/main/java/org/opensearch/security/auth/ldap2/SocketFactoryClassLoader.java src/main/java/org/opensearch/security/auth/ldap2/SniAwareConnection.java src/main/java/org/opensearch/security/auth/ldap2/HostnameAwareConnectionFactory.java src/test/java/org/opensearch/security/auth/ldap/LdapBackendTest.java Sub-PR theme: OBO/JWT encryption rewrite + keystore support Relevant files: src/main/java/org/opensearch/security/authtoken/jwt/EncryptionDecryptionUtil.java src/main/java/org/opensearch/security/support/PemKeyReader.java src/test/java/org/opensearch/security/authtoken/jwt/JwtVendorTest.java src/test/java/org/opensearch/security/http/OnBehalfOfAuthenticatorTest.java Sub-PR theme: FIPS password floor + test-side hash adaptation Relevant files: src/test/java/org/opensearch/security/test/helper/file/FipsHashAdapter.java src/test/java/org/opensearch/security/tools/HasherTests.java src/integrationTest/java/org/opensearch/security/api/InternalUsersRestApiIntegrationTest.java

⚡ Recommended focus areas for review Possible NPE configureSNISocketFactory invokes connFactory.getProvider().getProviderConfig(). If providerConfig returns null (or if getProvider() returns a provider without a properties bag configured), setting properties will throw NPE. Consider defensive null-checking before calling setProperties. @SuppressWarnings({ "rawtypes", "unchecked" }) private static void configureSNISocketFactory(DefaultConnectionFactory connFactory) { Map<String, Object> props = new HashMap<>(); props.put("java.naming.ldap.factory.socket", "org.opensearch.security.auth.ldap2.SNISettingTLSSocketFactory"); final ProviderConfig providerConfig = connFactory.getProvider().getProviderConfig(); providerConfig.setProperties(props); } SNI Extraction Bypass In getConnection0, SNI hostname is taken from split[0] directly, whereas checkConnection0 extracts host from a ldaps: URI via new URI(...).getHost(). If split[0] is an IP address or malformed (e.g. bracketed IPv6), SNI configuration will use an invalid hostname, potentially breaking TLS handshake or hostname verification. Consider unifying URI-based parsing and handling blank/invalid hosts consistently. // Register custom socket factory for SNI hostname verification with BouncyCastle FIPS // This addresses a known issue where JNDI LDAP doesn't pass hostname to SSLSocketFactory // See: https://github.com/bcgit/bc-java/issues/460 if (enableSSL) { configureSNISocketFactory(connFactory); try (var ignored = SNISettingTLSSocketFactory.configure(split[0])) { connection = connFactory.getConnection(); connection.open(); } } else { connection = connFactory.getConnection(); connection.open(); } Resource Leak In loadKeyStore, the non-PKCS11 branch opens new FileInputStream(storePath) inside a try-with-resources, which is correct, but the outer method previously loaded the store without closing the stream. Verify all failure paths (e.g. KeyStore.getInstance failure before opening the stream) do not leak resources; current code appears fine, but ensure store.load exceptions still close the stream (they do via try-with-resources). } } else { store = KeyStore.getInstance(storeType); try (final var in = new FileInputStream(storePath)) { store.load(in, password); } } Fragile Fixture Mapping The hard-coded BCRYPT_HASH_TO_PLAINTEXT map ties FIPS test rewriting to a fixed set of bcrypt fixtures. Any new fixture or hash change will silently fall through as an unmapped hash, causing FIPS test failures that may be hard to diagnose. Consider logging a warning when a bcrypt hash is encountered in configs that is not present in the mapping, to surface drift early. public static String adaptConfig(final String content) { if (content == null || content.isEmpty() || !FipsMode.isEnabled()) { return content; } String adapted = content; for (final Map.Entry<String, String> replacement : pbkdf2Replacements().entrySet()) { if (adapted.contains(replacement.getKey())) { adapted = adapted.replace(replacement.getKey(), replacement.getValue()); } } return adapted; } Weak Key Test Regression testCreateJwkFromSettingsWithWeakKey now asserts on "The secret length must be at least 256 bits" but this depends on the underlying library's error message. If Nimbus changes the wording, the test will silently fail; consider matching on exception type plus a stable substring like "256". public void testCreateJwkFromSettingsWithWeakKey() { Settings settings = Settings.builder().put(SIGNING_KEY_PROPERTY_KEY, "abcd1234").build(); Throwable exception = assertThrows( OpenSearchException.class, () -> JwtVendor.createJwkFromSettings(settings, tempDir.getRoot().toPath()) ); assertThat(exception.getMessage(), containsString("The secret length must be at least 256 bits")); }

[github-actions]

PR Code Suggestions ✨

Latest suggestions up to fd9ed30

Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	Avoid wiping caller-owned key material Zeroing secretBytes in the finally block wipes the caller's byte array, which is problematic when the same bytes came from SecretKey.getEncoded() (via fromSettings) or a caller-owned buffer. This can invalidate the keystore-derived key for subsequent uses and cause hard-to-diagnose failures. Copy the input first and only wipe the local copy. src/main/java/org/opensearch/security/authtoken/jwt/EncryptionDecryptionUtil.java [111-134] - byte[] derivedKey = new byte[AES_KEY_LENGTH_BYTES]; // AES-256 - kdf.generateBytes(derivedKey); - return new SecretKeySpec(derivedKey, "AES"); - } catch (final Exception e) { - throw new RuntimeException("Error deriving key from secret", e); - } finally { - Arrays.fill(secretBytes, (byte) 0); - } + byte[] ikm = secretBytes.clone(); + try { + FipsKDF.AgreementKDFParameters hkdfParams = FipsKDF.HKDF.withPRF(FipsKDF.AgreementKDFPRF.SHA256_HMAC) + .using(ikm) + .withIV(HKDF_INFO); + KDFCalculator<FipsKDF.AgreementKDFParameters> kdf = new FipsKDF.AgreementOperatorFactory().createKDFCalculator(hkdfParams); + byte[] derivedKey = new byte[AES_KEY_LENGTH_BYTES]; + kdf.generateBytes(derivedKey); + return new SecretKeySpec(derivedKey, "AES"); + } catch (final Exception e) { + throw new RuntimeException("Error deriving key from secret", e); + } finally { + Arrays.fill(ikm, (byte) 0); + } Suggestion importance[1-10]: 7 __ Why: Valid concern: the finally block zeroes secretBytes which may be caller-owned (e.g., from SecretKey.getEncoded()), potentially corrupting subsequent uses. Cloning first is a defensive improvement, though the current constructor is only called with freshly-decoded bytes.	Medium
	Guard against NPE on missing filepath When isPkcs11 is false and KEYSTORE_FILEPATH is not set, resolvePath(...) will be called with null, then path.toString() will NPE. Guard against a missing filepath explicitly and produce a descriptive error (same for the truststore variant below). src/main/java/org/opensearch/security/ssl/config/SslCertificatesLoader.java [133-136] final String explicitType = environment.settings().get(sslConfigSuffix + KEYSTORE_TYPE); final boolean isPkcs11 = PemKeyReader.PKCS11.equalsIgnoreCase(explicitType); -final Path path = isPkcs11 ? null : resolvePath(environment.settings().get(sslConfigSuffix + KEYSTORE_FILEPATH), environment); +final String filepath = environment.settings().get(sslConfigSuffix + KEYSTORE_FILEPATH); +if (!isPkcs11 && filepath == null) { + throw new OpenSearchException("Missing keystore filepath setting: " + sslConfigSuffix + KEYSTORE_FILEPATH); +} +final Path path = isPkcs11 ? null : resolvePath(filepath, environment); final String resolvedType = isPkcs11 ? PemKeyReader.PKCS11 : PemKeyReader.extractStoreType(path.toString(), explicitType); Suggestion importance[1-10]: 7 __ Why: Valid concern: if isPkcs11 is false and KEYSTORE_FILEPATH is unset, path.toString() would throw NPE with an unclear error. An explicit check with a descriptive message improves robustness and diagnostics.	Medium
	Handle multi-URL LDAP configuration safely config.getLdapUrl() can return a space-separated list of URLs (ldaptive supports multi-URL configs). Passing the raw list to new URI(...) will throw URISyntaxException, and even when it doesn't, the extracted host may be wrong. Split on whitespace and use the first entry, matching the pattern used in getConnection0. src/main/java/org/opensearch/security/auth/ldap/backend/LDAPAuthorizationBackend.java [211-222] String sniHostname = null; if (config.getLdapUrl() != null && config.getLdapUrl().startsWith("ldaps:")) { configureSNISocketFactory(connFactory); String ldapUrl = config.getLdapUrl(); + String firstUrl = ldapUrl.split("\\s+")[0]; try { - sniHostname = new URI(ldapUrl).getHost(); + sniHostname = new URI(firstUrl).getHost(); Suggestion importance[1-10]: 6 __ Why: Reasonable point that ldaptive supports space-separated LDAP URL lists which would fail URI parsing. Since getConnection0 uses a similar split pattern, applying the same here improves robustness, though this path is checkConnection0 and behavior in multi-URL configs is not verified.	Low
	Avoid catching Throwable in password check Catching Throwable here swallows critical errors like OutOfMemoryError, StackOverflowError, and ThreadDeath, which should never be silently converted to a false authentication result. Restrict the catch to Exception (or a more targeted FipsUnapprovedOperationError/RuntimeException), and consider logging at debug so failures are diagnosable. src/main/java/org/opensearch/security/hasher/PBKDF2PasswordHasher.java [47-53] try { return Password.check(passwordBuffer, hash).with(getPBKDF2FunctionFromHash(hash)); -} catch (Throwable e) { +} catch (Exception e) { return false; } finally { cleanup(passwordBuffer); } Suggestion importance[1-10]: 6 __ Why: Catching Throwable can swallow serious JVM errors like OutOfMemoryError. However, the FIPS FipsUnapprovedOperationError is an Error subclass, which is why Throwable was likely chosen; narrowing may need care. Moderate impact.	Low
	Fix initialization race in lazy init initialized and jwtParser/encryptionUtil are read outside the synchronized block via ensureInitialized()'s return, but the fields are not volatile here (only declared volatile — good), yet setting initialized = true before attempting initialization means a concurrent caller could observe initialized == true while jwtParser is still null due to a race with the try block. Set initialized = true only after initialization completes (or in a finally) to avoid mis-reporting state, and ensure repeated failed attempts don't silently swallow errors on every request. src/main/java/org/opensearch/security/http/OnBehalfOfAuthenticator.java [79-90] private synchronized boolean ensureInitialized() { if (!initialized) { - initialized = true; try { jwtParser = AccessController.doPrivileged(this::buildJwtParser); encryptionUtil = EncryptionDecryptionUtil.fromSettings(settings, ENCRYPTION_KEY, configPath); } catch (final RuntimeException e) { log.error("On-behalf-of authentication is misconfigured; OBO tokens will be rejected: {}", e.toString(), e); + } finally { + initialized = true; } } return jwtParser != null; } Suggestion importance[1-10]: 5 __ Why: The method is synchronized, so there is no actual race between concurrent callers. However, moving initialized = true to a finally block is a minor stylistic improvement; the current code intentionally avoids repeated failed initialization attempts on every request.	Low
General	Preserve prior SNI hostname on nested configure configure() unconditionally overwrites any existing thread-local hostname and its returned close clears the value entirely. If nested/reentrant configuration ever occurs (e.g., pooled connection triggers another open on the same thread), the inner close will erase the outer hostname before the outer socket is created. Save and restore the prior value instead of clearing. src/main/java/org/opensearch/security/auth/ldap2/SNISettingTLSSocketFactory.java [81-85] public static SniContext configure(String hostname) { + final String previous = hostnameThreadLocal.get(); hostnameThreadLocal.set(hostname); log.debug("Configured SNI context: hostname={}", hostname); - return SNISettingTLSSocketFactory::clearContext; + return () -> { + if (previous == null) { + hostnameThreadLocal.remove(); + } else { + hostnameThreadLocal.set(previous); + } + }; } Suggestion importance[1-10]: 6 __ Why: Reasonable defensive improvement for potential nested/reentrant configuration scenarios. While the current usage pattern (via HostnameAwareConnectionFactory) may not trigger nested calls, save/restore semantics is more robust for ThreadLocal-based context management.	Low
	Use case-sensitive class name comparison Class name comparison should be case-sensitive. Using equalsIgnoreCase here is inconsistent with the exact match used for SNISettingTLSSocketFactory above and could cause incorrect class resolution if a similarly-named class exists. Use equals to match Java class name semantics. src/main/java/org/opensearch/security/auth/ldap2/SocketFactoryClassLoader.java [40-42] -if (ThreadLocalTLSSocketFactory.class.getName().equalsIgnoreCase(name)) { +if (ThreadLocalTLSSocketFactory.class.getName().equals(name)) { return ThreadLocalTLSSocketFactory.class; } Suggestion importance[1-10]: 6 __ Why: Java class name resolution is case-sensitive, so using equalsIgnoreCase here is inconsistent with the exact match used for SNISettingTLSSocketFactory and could theoretically cause incorrect resolution. Reasonable correctness improvement.	Low
	Use locale-independent uppercase conversion type.toUpperCase() uses the default locale and can misbehave under Turkish locale (e.g., "pkcs11" → "PKCS11" fine, but general defensive coding requires Locale.ROOT). Also, KeyStore.getInstance is case-sensitive for some providers/types; passing the caller-supplied type as-is (or normalized to ROOT) is safer. src/main/java/org/opensearch/security/tools/SecurityAdmin.java [1717-1728] private static KeyStore loadStore(String path, String type, char[] password) throws Exception { - KeyStore ks = KeyStore.getInstance(type.toUpperCase()); + KeyStore ks = KeyStore.getInstance(type.toUpperCase(java.util.Locale.ROOT)); if ("PKCS11".equalsIgnoreCase(type)) { ks.load(null, password); } else { try (final var fin = new FileInputStream(Paths.get(path).toFile())) { ks.load(fin, password); } } return ks; } Suggestion importance[1-10]: 5 __ Why: Valid best-practice suggestion to use Locale.ROOT for locale-independent uppercase conversion, avoiding potential Turkish-locale surprises. Low-impact but a legitimate code-quality improvement.	Low
	Zero full encoded password buffer safely passwordByteBuffer.array() is only accessible if the buffer is backed by an accessible array; more importantly, after get(passwordBytes) the backing array positions before arrayOffset()+position are past-encoded bytes — you should zero the full backing array via its offset/limit, and preferably check hasArray(). Also, the temporary CharBuffer.wrap(plainTextPassword) and any intermediate encoder state may retain characters; ensure zeroing covers the full backing array length. src/main/java/org/opensearch/security/user/UserService.java [328-332] byte[] accountBytes = accountName.getBytes(StandardCharsets.UTF_8); ByteBuffer passwordByteBuffer = StandardCharsets.UTF_8.encode(CharBuffer.wrap(plainTextPassword)); byte[] passwordBytes = new byte[passwordByteBuffer.remaining()]; passwordByteBuffer.get(passwordBytes); -Arrays.fill(passwordByteBuffer.array(), (byte) 0); +if (passwordByteBuffer.hasArray()) { + Arrays.fill(passwordByteBuffer.array(), 0, passwordByteBuffer.array().length, (byte) 0); +} Suggestion importance[1-10]: 5 __ Why: The suggestion addresses a valid concern about safely zeroing the backing array using hasArray() check, which is defensive but the current code works for standard encoder outputs. Minor security hardening improvement for credential handling.	Low
	Restrict visibility of mutable static supplier A mutable public static supplier is a global that any caller (or test) can silently overwrite, and it also breaks the "single source of truth" documented in the class Javadoc. Make the field private and non-final only via a package-private setter for tests, or use a @VisibleForTesting guarded mechanism. src/main/java/org/opensearch/security/support/FipsMode.java [17] -public static java.util.function.Supplier<String> envSupplier = () -> System.getenv("OPENSEARCH_FIPS_MODE"); +static java.util.function.Supplier<String> envSupplier = () -> System.getenv("OPENSEARCH_FIPS_MODE"); public static boolean isEnabled() { return "true".equalsIgnoreCase(envSupplier.get()); } Suggestion importance[1-10]: 5 __ Why: Making the mutable supplier public allows any caller to override FIPS detection globally, which contradicts the "single source of truth" documentation. Reducing visibility is a reasonable defensive improvement.	Low
	Avoid null path detection for PKCS11 When storePath == null and type is PKCS11, extractStoreType(null, "PKCS11") will invoke detectStoreType(null) if type is somehow null-cased differently, and even in the current path it depends on type being non-null. Additionally detectStoreType(null) would NPE if the branch were ever changed. Guard explicitly by passing PKCS11 as the resolved type when storePath is null to avoid future regressions. src/main/java/org/opensearch/security/support/PemKeyReader.java [155-159] public static KeyStore loadKeyStore(final String storePath, final String keyStorePassword, final String type) throws Exception { if (storePath == null && !PKCS11.equalsIgnoreCase(type)) { return null; } - String storeType = extractStoreType(storePath, type); + String storeType = storePath == null ? PKCS11 : extractStoreType(storePath, type); Suggestion importance[1-10]: 3 __ Why: Minor defensive improvement. In the current code flow, extractStoreType is only called with non-null path or with type=PKCS11 (which short-circuits via Optional.ofNullable(storeType)), so this is a hypothetical hardening rather than a real bug.	Low
	Guard against null keystore path When type is "PKCS11" the code intentionally calls load(null, password) and path may be null. The error message branch is correct, but the outer catch will also fire before path is dereferenced elsewhere; ensure the earlier Files.newInputStream(path) block is only reached when path is non-null (it is, since it's inside the else branch), which is fine — however, verify callers do not pass a null path with a non-PKCS11 type, which would NPE inside Files.newInputStream. Add an explicit guard. src/main/java/org/opensearch/security/ssl/config/KeyStoreUtils.java [217-227] -} catch (Exception e) { - throw new OpenSearchException("Failed to load keystore from " + (path != null ? path : "PKCS#11 token"), e); +} else { + if (path == null) { + throw new OpenSearchException("Keystore path is required for type " + type); + } + try (final var in = Files.newInputStream(path)) { + keyStore.load(in, password); + } catch (IOException e) { + throw new RuntimeException(e); + } } Suggestion importance[1-10]: 3 __ Why: The suggestion adds a defensive null check, but callers presumably pass a valid path for non-PKCS11 types, and a resulting NPE would be caught by the outer catch anyway. Low impact.	Low

---

Previous suggestions

Suggestions up to commit 3ab9228

Category	Suggestion	Impact
Possible issue	Avoid zeroing caller-owned key bytes deriveKey zeroes out the passed-in secretBytes array in its finally block. When callers pass a SecretKey.getEncoded() result (as in fromSettings), this wipes the shared key material array, which can corrupt the keystore's cached key and any subsequent access. Clone the array before mutating it, or don't zero it here. src/main/java/org/opensearch/security/authtoken/jwt/EncryptionDecryptionUtil.java [69-71] public EncryptionDecryptionUtil(final byte[] secretBytes) { - this.aesKey = deriveKey(secretBytes); + this.aesKey = deriveKey(secretBytes.clone()); } Suggestion importance[1-10]: 8 __ Why: Valid concern: deriveKey zeroes secretBytes in its finally block, and fromSettings passes keystoreKey.getEncoded() which for some KeyStore implementations may return a shared reference, potentially corrupting the cached key. Cloning is a defensive improvement.	Medium
	Avoid catching Throwable in check method Catching Throwable here silently swallows critical errors such as OutOfMemoryError, ThreadDeath, and JVM-level Errors, which should never be caught and returned as false. This can also mask FIPS configuration/provider issues that should surface. Catch Exception (or narrow specific exceptions like IllegalArgumentException) instead. src/main/java/org/opensearch/security/hasher/PBKDF2PasswordHasher.java [49-53] CharBuffer passwordBuffer = CharBuffer.wrap(password); try { return Password.check(passwordBuffer, hash).with(getPBKDF2FunctionFromHash(hash)); -} catch (Throwable e) { +} catch (Exception e) { return false; } finally { cleanup(passwordBuffer); } Suggestion importance[1-10]: 6 __ Why: Catching Throwable can indeed swallow serious JVM errors and mask configuration issues; narrowing to Exception is a reasonable correctness/robustness improvement.	Low
	Validate path for non-PKCS11 keystores When loading a non-PKCS11 keystore, path is dereferenced without a null check; if a caller invokes this with type != "PKCS11" but path == null (now possible since files() may be empty for PKCS#11 configurations), Files.newInputStream(null) will throw NullPointerException instead of a meaningful error. Validate path explicitly and throw a clear OpenSearchException. src/main/java/org/opensearch/security/ssl/config/KeyStoreUtils.java [215-223] if ("PKCS11".equalsIgnoreCase(type)) { keyStore.load(null, password); } else { + if (path == null) { + throw new OpenSearchException("Keystore path is required for type " + type); + } try (final var in = Files.newInputStream(path)) { keyStore.load(in, password); } catch (IOException e) { throw new RuntimeException(e); } } return keyStore; Suggestion importance[1-10]: 5 __ Why: Given the PR now allows null path for PKCS#11 keystores, adding an explicit null-check for non-PKCS11 types produces clearer error messages rather than an NPE; a modest robustness improvement.	Low
	Guard against null path for non-PKCS11 stores When isPkcs11 is false but the keystore filepath setting is missing, resolvePath may return null (or path may be null), causing a NullPointerException on path.toString(). Validate that the filepath is present for non-PKCS#11 keystores before dereferencing, or guard the path.toString() call. The same applies to the truststore configuration below. src/main/java/org/opensearch/security/ssl/config/SslCertificatesLoader.java [133-136] final String explicitType = environment.settings().get(sslConfigSuffix + KEYSTORE_TYPE); final boolean isPkcs11 = PemKeyReader.PKCS11.equalsIgnoreCase(explicitType); final Path path = isPkcs11 ? null : resolvePath(environment.settings().get(sslConfigSuffix + KEYSTORE_FILEPATH), environment); -final String resolvedType = isPkcs11 ? PemKeyReader.PKCS11 : PemKeyReader.extractStoreType(path.toString(), explicitType); +final String resolvedType = isPkcs11 + ? PemKeyReader.PKCS11 + : PemKeyReader.extractStoreType(Objects.requireNonNull(path, "keystore filepath must be set for non-PKCS#11 keystores").toString(), explicitType); Suggestion importance[1-10]: 4 __ Why: The concern about NPE is valid in theory, but the caller loadConfiguration already checks hasValue(KEYSTORE_FILEPATH) || isPkcs11Keystore before invoking, so a null path when non-PKCS11 is unlikely. The suggestion is defensive but marginal.	Low
	Handle PKCS11 keystore without file path When -kst PKCS11 is used without -ks, the non-PKCS11 branch is skipped and no key material is loaded, meaning client-cert authentication will fail. Add a branch that loads PKCS11 key material via loadStore(null, "PKCS11", ...) when keyStoreType is PKCS11, so PKCS11-only keystore configurations still initialize the SSL context correctly. Alternatively, ensure this method is only reached through buildPkcs11SslContext for PKCS11. src/main/java/org/opensearch/security/tools/SecurityAdmin.java [1576-1588] -if (ks != null) { +if (ks != null || "PKCS11".equalsIgnoreCase(keyStoreType)) { final char[] kspassChars = kspass == null ? null : kspass.toCharArray(); final KeyStore keyStore = loadStore(ks, keyStoreType, kspassChars); sslContextBuilder.loadKeyMaterial(keyStore, kspassChars, (aliases, socket) -> { Suggestion importance[1-10]: 2 __ Why: The suggestion appears to misread the code flow: buildPkcs11SslContext is invoked earlier when keyStoreType is PKCS11 and returns before reaching this block, so the PKCS11 case is already handled separately.	Low
General	Validate SNI hostname before configuring split[0] is used as the SNI hostname without validation. If split[0] is an IP address, empty, or malformed (e.g. an LDAP URL rather than host), SNISettingTLSSocketFactory.configure may throw IllegalArgumentException and abort the entire connection attempt. Guard with blank/host-shape checks as done in checkConnection0, and log-and-continue on failure. src/main/java/org/opensearch/security/auth/ldap/backend/LDAPAuthorizationBackend.java [335-344] if (enableSSL) { configureSNISocketFactory(connFactory); - try (var ignored = SNISettingTLSSocketFactory.configure(split[0])) { + final String sniHost = StringUtils.isBlank(split[0]) ? null : split[0]; + try (var ignored = sniHost == null ? null : SNISettingTLSSocketFactory.configure(sniHost)) { connection = connFactory.getConnection(); connection.open(); } } else { Suggestion importance[1-10]: 6 __ Why: Reasonable robustness improvement to guard against blank/invalid split[0] values, matching the more careful handling in checkConnection0. Impact is moderate as split[0] is typically a hostname.	Low
	Make SNI thread-local context reentrant-safe configure() overwrites any previously-set thread-local without preserving it, and clearContext() unconditionally removes the value. If configure() calls are ever nested (e.g. reopen invoked from within an open path on the same thread), the inner try-with-resources will clear the outer's hostname prematurely. Save and restore the prior value to make the context reentrant-safe. src/main/java/org/opensearch/security/auth/ldap2/SNISettingTLSSocketFactory.java [81-85] public static SniContext configure(String hostname) { + final String previous = hostnameThreadLocal.get(); hostnameThreadLocal.set(hostname); log.debug("Configured SNI context: hostname={}", hostname); - return SNISettingTLSSocketFactory::clearContext; + return () -> { + if (previous == null) { + hostnameThreadLocal.remove(); + } else { + hostnameThreadLocal.set(previous); + } + }; } Suggestion importance[1-10]: 6 __ Why: Reasonable defensive improvement for nested configure() calls (e.g., reopen from open on same thread). Makes the SNI thread-local context reentrant-safe, which could prevent subtle bugs, though the current usage patterns may not exercise nesting.	Low
	Reconsider one-shot initialization on failure Setting initialized = true before attempting initialization means transient failures (e.g. a keystore file temporarily unavailable) permanently disable OBO authentication for the lifetime of the process. Consider setting initialized = true only on successful initialization, or explicitly document that misconfiguration is a permanent, non-recoverable state so operators know a restart is required. src/main/java/org/opensearch/security/http/OnBehalfOfAuthenticator.java [79-90] private synchronized boolean ensureInitialized() { if (!initialized) { - initialized = true; try { jwtParser = AccessController.doPrivileged(this::buildJwtParser); encryptionUtil = EncryptionDecryptionUtil.fromSettings(settings, ENCRYPTION_KEY, configPath); + initialized = true; } catch (final RuntimeException e) { + initialized = true; log.error("On-behalf-of authentication is misconfigured; OBO tokens will be rejected: {}", e.toString(), e); } } return jwtParser != null; } Suggestion importance[1-10]: 5 __ Why: Valid observation about transient failures making OBO permanently disabled. However, the improved code still sets initialized = true in the catch block, so it doesn't actually change the behavior — only reorders the assignment on success. The suggestion's value is limited.	Low
	Use case-sensitive class name comparison Class name comparison should be case-sensitive; Java class names are case-sensitive and equalsIgnoreCase could theoretically match unintended names. Use equals for consistency with the SNISettingTLSSocketFactory check just above. src/main/java/org/opensearch/security/auth/ldap2/SocketFactoryClassLoader.java [40-42] if (SNISettingTLSSocketFactory.class.getName().equals(name)) { return SNISettingTLSSocketFactory.class; } -if (ThreadLocalTLSSocketFactory.class.getName().equalsIgnoreCase(name)) { +if (ThreadLocalTLSSocketFactory.class.getName().equals(name)) { return ThreadLocalTLSSocketFactory.class; } return super.loadClass(name); Suggestion importance[1-10]: 5 __ Why: Java class name lookups are case-sensitive, so using equalsIgnoreCase is inconsistent and could theoretically match unintended names; a minor correctness/consistency fix.	Low
	Avoid path lookup for PKCS11 stores When storePath is null and type is PKCS11, extractStoreType(null, "PKCS11") is called, which then invokes detectStoreType(null) if type were null — but even with a set type it may still short-circuit correctly. However, detectStoreType opens a FileInputStream on the path, so passing null would NPE. Since type is provided here, it's safe, but consider explicitly bypassing extractStoreType's path-dependent branch for PKCS11 to make intent explicit and avoid future breakage. src/main/java/org/opensearch/security/support/PemKeyReader.java [155-159] public static KeyStore loadKeyStore(final String storePath, final String keyStorePassword, final String type) throws Exception { if (storePath == null && !PKCS11.equalsIgnoreCase(type)) { return null; } - String storeType = extractStoreType(storePath, type); + String storeType = PKCS11.equalsIgnoreCase(type) ? type : extractStoreType(storePath, type); Suggestion importance[1-10]: 3 __ Why: Minor defensive/clarity improvement. The current code works correctly since type is provided, but making the PKCS11 bypass explicit could improve robustness against future changes.	Low
Security	Safely wipe encoded password buffer passwordByteBuffer.array() returns the buffer's backing array only if the buffer is backed by an accessible array; for buffers with an offset, only the used portion may hold the sensitive bytes and other regions may share the array. More importantly, the caller should also zero out accountBytes (contains account name — less sensitive but still leaked) and account for the possibility that the encoder-produced buffer might not be array-backed in future JDKs. Consider using passwordByteBuffer.hasArray() check or explicitly wiping via Arrays.fill(passwordByteBuffer.array(), passwordByteBuffer.arrayOffset(), passwordByteBuffer.arrayOffset() + passwordByteBuffer.capacity(), (byte)0). src/main/java/org/opensearch/security/user/UserService.java [329-332] byte[] accountBytes = accountName.getBytes(StandardCharsets.UTF_8); ByteBuffer passwordByteBuffer = StandardCharsets.UTF_8.encode(CharBuffer.wrap(plainTextPassword)); byte[] passwordBytes = new byte[passwordByteBuffer.remaining()]; passwordByteBuffer.get(passwordBytes); -Arrays.fill(passwordByteBuffer.array(), (byte) 0); +if (passwordByteBuffer.hasArray()) { + final byte[] backing = passwordByteBuffer.array(); + final int offset = passwordByteBuffer.arrayOffset(); + Arrays.fill(backing, offset, offset + passwordByteBuffer.capacity(), (byte) 0); +} Suggestion importance[1-10]: 6 __ Why: Valid security concern about correctly wiping the encoded password buffer using hasArray() check and proper offset/capacity range, since the current Arrays.fill(passwordByteBuffer.array(), (byte)0) may not fully cover all sensitive regions. Improves defense-in-depth for password material handling.	Low
	Restrict visibility of mutable FIPS supplier The mutable public static envSupplier field allows any code to change global FIPS-mode detection at runtime, which is a serious correctness/security concern for a "single source of truth". Make it private (or package-private) and non-mutable in production; provide a dedicated test hook (e.g., a package-private setter guarded for tests) rather than a public writable field. src/main/java/org/opensearch/security/support/FipsMode.java [17] -public static java.util.function.Supplier<String> envSupplier = () -> System.getenv("OPENSEARCH_FIPS_MODE"); +static java.util.function.Supplier<String> envSupplier = () -> System.getenv("OPENSEARCH_FIPS_MODE"); public static boolean isEnabled() { return "true".equalsIgnoreCase(envSupplier.get()); } Suggestion importance[1-10]: 6 __ Why: A public mutable static field for FIPS-mode detection is a legitimate concern; reducing visibility improves encapsulation and prevents accidental override in production code.	Low

Suggestions up to commit b12e4f7

Category	Suggestion	Impact
Possible issue	Fix PKCS#11 provider availability check Security.getProviders(filter) returns an empty array (not null) when no matching provider is found, so this check never triggers and the misleading error message is unreachable. Use a length check instead. src/main/java/org/opensearch/security/tools/SecurityAdmin.java [1256-1258] - if (pkcs11Keystore && Security.getProviders("KeyStore.PKCS11") == null) { - throw new ParseException("No PKCS#11 provider is registered in the JVM; cannot use -kst PKCS11"); + if (pkcs11Keystore) { + Provider[] p11Providers = Security.getProviders("KeyStore.PKCS11"); + if (p11Providers == null || p11Providers.length == 0) { + throw new ParseException("No PKCS#11 provider is registered in the JVM; cannot use -kst PKCS11"); + } } Suggestion importance[1-10]: 8 __ Why: Correct catch: Security.getProviders(filter) returns null when no providers match (not an empty array), but many developers assume the opposite. Actually per Java docs it returns null if none match, so the original check may work; however the suggested defensive check with length is safer and clearer.	Medium
	Avoid zeroing caller-owned key bytes Zeroing secretBytes in the finally block is dangerous because the caller-supplied array may still be referenced elsewhere (e.g. by KeyUtils.loadKeyFromKeystore returning a SecretKey whose getEncoded() was passed in). Wiping it silently corrupts the caller's key material. Either document/rename this contract clearly, or copy the bytes and wipe only the local copy. src/main/java/org/opensearch/security/authtoken/jwt/EncryptionDecryptionUtil.java [127-129] - return new SecretKeySpec(derivedKey, "AES"); + SecretKey result = new SecretKeySpec(derivedKey, "AES"); + Arrays.fill(derivedKey, (byte) 0); + return result; } catch (final Exception e) { throw new RuntimeException("Error deriving key from secret", e); - } finally { - Arrays.fill(secretBytes, (byte) 0); } Suggestion importance[1-10]: 7 __ Why: Valid concern: secretBytes may be caller-owned (e.g., from SecretKey.getEncoded()), and wiping it in finally could corrupt the caller's key material. The suggested fix of wiping only the local derivedKey is reasonable, though the impact depends on caller usage patterns.	Medium
	Prevent NullPointerException when keystore path is missing When explicitType is null and KEYSTORE_FILEPATH is unset, resolvePath may return null and calling path.toString() will NPE. The same pattern appears in buildJdkTrustStoreConfiguration. Guard against a null path before calling toString() so a clear error is surfaced instead of a NullPointerException. src/main/java/org/opensearch/security/ssl/config/SslCertificatesLoader.java [133-136] final String explicitType = environment.settings().get(sslConfigSuffix + KEYSTORE_TYPE); final boolean isPkcs11 = PemKeyReader.PKCS11.equalsIgnoreCase(explicitType); final Path path = isPkcs11 ? null : resolvePath(environment.settings().get(sslConfigSuffix + KEYSTORE_FILEPATH), environment); +if (!isPkcs11 && path == null) { + throw new OpenSearchException("Missing keystore filepath setting: " + sslConfigSuffix + KEYSTORE_FILEPATH); +} final String resolvedType = isPkcs11 ? PemKeyReader.PKCS11 : PemKeyReader.extractStoreType(path.toString(), explicitType); Suggestion importance[1-10]: 6 __ Why: Valid defensive check: if KEYSTORE_TYPE is not PKCS#11 and KEYSTORE_FILEPATH is unset, resolvePath could return null and path.toString() would NPE. However, upstream validation in validateKeyStoreSettings may already prevent this, limiting impact.	Low
Security	Precompute dummy hash to preserve timing safety The dummy hash is intended for constant-time comparison to prevent user-enumeration timing attacks, but computing it lazily on first call means the first "user not found" authentication will incur an extra hashing cost, giving attackers a timing signal for the very case this method is meant to mask. Pre-compute the dummy hash eagerly (e.g., in the constructor or via an initializer) so the first call has the same cost as subsequent ones. src/main/java/org/opensearch/security/hasher/AbstractPasswordHasher.java [33-45] -private volatile String dummyHash; +private final java.util.concurrent.atomic.AtomicReference<String> dummyHashRef = new java.util.concurrent.atomic.AtomicReference<>(); @Override public String getDummyHash() { - if (dummyHash == null) { - synchronized (this) { - if (dummyHash == null) { - dummyHash = hash(new char[] { 'd', 'u', 'm', 'm', 'y', 'p', 'a', 's', 's', 'w', 'o', 'r', 'd', 'h', 'a', 's', 'h' }); - } + String h = dummyHashRef.get(); + if (h == null) { + h = hash(new char[] { 'd', 'u', 'm', 'm', 'y', 'p', 'a', 's', 's', 'w', 'o', 'r', 'd', 'h', 'a', 's', 'h' }); + if (!dummyHashRef.compareAndSet(null, h)) { + h = dummyHashRef.get(); } } - return dummyHash; + return h; } Suggestion importance[1-10]: 7 __ Why: Valid security concern: lazy initialization of the dummy hash means the first "user not found" request has different timing, which partially undermines the purpose of the dummy hash for preventing user enumeration.	Medium
	Restrict mutability of FIPS mode supplier The public mutable static envSupplier allows any code to alter FIPS-mode detection at runtime, which is a serious security concern for a "single source of truth". Make it private (or at least final and package-private) and expose a controlled setter for tests only, or read the env variable directly inside isEnabled(). src/main/java/org/opensearch/security/support/FipsMode.java [17-21] -public static java.util.function.Supplier<String> envSupplier = () -> System.getenv("OPENSEARCH_FIPS_MODE"); +private static volatile java.util.function.Supplier<String> envSupplier = () -> System.getenv("OPENSEARCH_FIPS_MODE"); public static boolean isEnabled() { return "true".equalsIgnoreCase(envSupplier.get()); } Suggestion importance[1-10]: 6 __ Why: Valid point that a public mutable static field is a poor design for a "single source of truth", though the risk in a test-only-mutable pattern is limited. The suggested improvement (making it private/volatile) improves encapsulation.	Low
General	Fail fast on malformed LDAPS URLs Assigning null to a try-with-resources variable is legal but the close() on the SNI context will be skipped when the hostname was blank — which is fine — however, if sniHostname extraction failed (URISyntaxException logged as a warning), the LDAPS connection then proceeds without SNI, which will break hostname verification against many servers. Consider failing fast on a malformed LDAPS URL rather than silently proceeding. src/main/java/org/opensearch/security/auth/ldap/backend/LDAPAuthorizationBackend.java [225-232] - try ( - SNISettingTLSSocketFactory.SniContext ignored = StringUtils.isBlank(sniHostname) - ? null - : SNISettingTLSSocketFactory.configure(sniHostname) - ) { + if (config.getLdapUrl() != null && config.getLdapUrl().startsWith("ldaps:")) { + configureSNISocketFactory(connFactory); + try { + sniHostname = new URI(config.getLdapUrl()).getHost(); + } catch (URISyntaxException e) { + throw new LdapException("Malformed LDAPS URL: " + config.getLdapUrl(), e); + } + if (StringUtils.isBlank(sniHostname)) { + throw new LdapException("Could not extract hostname from LDAPS URL: " + config.getLdapUrl()); + } + } + try (SNISettingTLSSocketFactory.SniContext ignored = sniHostname == null ? null : SNISettingTLSSocketFactory.configure(sniHostname)) { connection = connFactory.getConnection(); connection.open(); } Suggestion importance[1-10]: 6 __ Why: Reasonable concern: silently proceeding without SNI on a malformed LDAPS URL could cause hostname verification failures. Fail-fast behavior would be clearer, though the current fallback with warning is also a defensible design choice.	Low
	Avoid catching Throwable in password check Catching Throwable and returning false silently swallows Errors (e.g., OutOfMemoryError, StackOverflowError, JVM linkage errors) and hides configuration bugs as authentication failures. Narrow the catch to Exception (or specific runtime exceptions from the PBKDF2 library) and log the failure so operational issues remain diagnosable. src/main/java/org/opensearch/security/hasher/PBKDF2PasswordHasher.java [47-53] try { return Password.check(passwordBuffer, hash).with(getPBKDF2FunctionFromHash(hash)); -} catch (Throwable e) { +} catch (Exception e) { + log.debug("PBKDF2 password check failed", e); return false; } finally { cleanup(passwordBuffer); } Suggestion importance[1-10]: 6 __ Why: Catching Throwable is generally discouraged as it swallows serious JVM errors. Narrowing to Exception and logging improves diagnosability, though the impact is moderate.	Low
	Guard ByteBuffer array access before wiping passwordByteBuffer.array() is only safe when the buffer is backed by an accessible array; more importantly, StandardCharsets.UTF_8.encode may return a buffer whose backing array is larger than the encoded data, and after get the position/limit metadata is not enough to guarantee zeroing covers the actual bytes previously read. Also, hasArray() should be checked before calling array(). Guard the wipe to avoid UnsupportedOperationException and ensure the full backing array is cleared. src/main/java/org/opensearch/security/user/UserService.java [335-339] byte[] accountBytes = accountName.getBytes(StandardCharsets.UTF_8); ByteBuffer passwordByteBuffer = StandardCharsets.UTF_8.encode(CharBuffer.wrap(plainTextPassword)); byte[] passwordBytes = new byte[passwordByteBuffer.remaining()]; passwordByteBuffer.get(passwordBytes); -Arrays.fill(passwordByteBuffer.array(), (byte) 0); +if (passwordByteBuffer.hasArray()) { + Arrays.fill(passwordByteBuffer.array(), (byte) 0); +} Suggestion importance[1-10]: 5 __ Why: Reasonable defensive check; StandardCharsets.UTF_8.encode returns a HeapByteBuffer which does have an accessible array, so this is unlikely to fail in practice, but adding hasArray() guard is a minor safety improvement.	Low
	Avoid passing null path to detection When storePath is null and type is PKCS11, extractStoreType(storePath, type) is called with a null path. Inside extractStoreType, detectStoreType(storePath) would NPE if type were also null; here type is non-null so Optional.ofNullable(storeType) short-circuits, but this is fragile. Consider explicitly branching on PKCS11 before calling extractStoreType, so the intent is clear and a null path never flows into detection. src/main/java/org/opensearch/security/support/PemKeyReader.java [155-159] public static KeyStore loadKeyStore(final String storePath, final String keyStorePassword, final String type) throws Exception { if (storePath == null && !PKCS11.equalsIgnoreCase(type)) { return null; } - String storeType = extractStoreType(storePath, type); + final String storeType = PKCS11.equalsIgnoreCase(type) ? PKCS11 : extractStoreType(storePath, type); Suggestion importance[1-10]: 4 __ Why: Minor defensive improvement for code clarity. The current code works correctly for the PKCS11 case since type is non-null, so this is a fragility/readability concern rather than a bug.	Low
	Handle null configPath when resolving keystore path If configPath is null (which can occur in some plugin contexts) and pathStr is a relative path, configPath.resolve(pathStr) will throw NullPointerException. Fall back to using the raw path when configPath is null so absolute paths still work and a clearer error is produced for relative paths. src/main/java/org/opensearch/security/util/KeyUtils.java [77-82] private static String resolveKeystorePath(final String pathStr, final Path configPath) { if (pathStr == null || pathStr.isEmpty()) { return pathStr; } + if (configPath == null) { + return java.nio.file.Paths.get(pathStr).toAbsolutePath().toString(); + } return configPath.resolve(pathStr).toAbsolutePath().toString(); } Suggestion importance[1-10]: 4 __ Why: Minor robustness improvement; configPath is unlikely to be null in plugin usage as it is the node config directory, but the guard adds defensive safety.	Low
	Validate non-null path for file keystores For non-PKCS11 keystores, path is dereferenced without a null-check; a null path will cause a NullPointerException that gets wrapped by the outer catch and misreports as "Failed to load keystore from null". Validate that path is non-null for non-PKCS11 types (or fail fast with a clear message) to make configuration errors obvious. src/main/java/org/opensearch/security/ssl/config/KeyStoreUtils.java [215-224] if ("PKCS11".equalsIgnoreCase(type)) { keyStore.load(null, password); } else { + if (path == null) { + throw new IllegalArgumentException("Keystore path must be provided for type " + type); + } try (final var in = Files.newInputStream(path)) { keyStore.load(in, password); } catch (IOException e) { throw new RuntimeException(e); } } return keyStore; Suggestion importance[1-10]: 4 __ Why: Adding a null-check gives clearer error messages for misconfigurations, but the existing exception wrapping would already surface the issue, so the improvement is minor.	Low
	Clarify initialization-failure semantics for OBO initialized is declared volatile but is being set to true at the start of the try block; if buildJwtParser throws, initialized remains true and subsequent calls will silently return false without retrying or re-logging. Consider setting initialized = true only after successful initialization, or set it only in a finally block if a permanent failure is intended — otherwise a transient error at startup permanently disables OBO auth with no clear signal. src/main/java/org/opensearch/security/http/OnBehalfOfAuthenticator.java [79-90] private synchronized boolean ensureInitialized() { if (!initialized) { - initialized = true; try { jwtParser = AccessController.doPrivileged(this::buildJwtParser); encryptionUtil = EncryptionDecryptionUtil.fromSettings(settings, ENCRYPTION_KEY, configPath); + initialized = true; } catch (final RuntimeException e) { log.error("On-behalf-of authentication is misconfigured; OBO tokens will be rejected: {}", e.toString(), e); + initialized = true; // keep failure permanent by design; document intent } } return jwtParser != null; } Suggestion importance[1-10]: 3 __ Why: The current code intentionally makes initialization one-shot (setting initialized = true at the start), and the suggestion doesn't materially change behavior. The observation about permanent failure is valid but the improved code retains the same behavior.	Low

Suggestions up to commit 905dbb1

Category	Suggestion	Impact
Possible issue	Restore static state modified by test This test mutates the static FipsMode.envSupplier without restoring it in an @After/finally block, which will leak state into subsequent tests in the same JVM and cause flaky/incorrect FIPS-mode detection elsewhere. Save the previous supplier before overriding and restore it after the test (as FipsModeTest does). src/test/java/org/opensearch/security/authtoken/jwt/EncryptionDecryptionUtilsTest.java [106-115] @Test public void testFipsModeAcceptsStrongEncryptionKey() { + java.util.function.Supplier<String> previous = FipsMode.envSupplier; FipsMode.envSupplier = () -> "true"; - // 32-byte (256-bit) key material satisfies the FIPS minimum - String strongSecret = Base64.getEncoder().encodeToString("mySecretKey12345mySecretKey12345".getBytes()); - String data = "Hello, OpenSearch!"; + try { + // 32-byte (256-bit) key material satisfies the FIPS minimum + String strongSecret = Base64.getEncoder().encodeToString("mySecretKey12345mySecretKey12345".getBytes()); + String data = "Hello, OpenSearch!"; - EncryptionDecryptionUtil util = new EncryptionDecryptionUtil(strongSecret); + EncryptionDecryptionUtil util = new EncryptionDecryptionUtil(strongSecret); - assertThat(util.decrypt(util.encrypt(data)), is(data)); + assertThat(util.decrypt(util.encrypt(data)), is(data)); + } finally { + FipsMode.envSupplier = previous; + } } Suggestion importance[1-10]: 8 __ Why: Correctly identifies that mutating the static FipsMode.envSupplier without restoration leaks state across tests, causing potential flakiness. This is a legitimate test hygiene issue.	Medium
	Avoid zeroing caller-owned key bytes The secretBytes array is zeroed in the finally block, but for the string-based constructor this array is decoded from the caller and unique to the util. However, callers that pass a byte[] directly (e.g., via fromSettings from keystoreKey.getEncoded()) will have their key material zeroed unexpectedly, potentially corrupting the original SecretKey. Clone the input at construction or only zero when the util owns the bytes. src/main/java/org/opensearch/security/authtoken/jwt/EncryptionDecryptionUtil.java [111-129] try { FipsKDF.AgreementKDFParameters hkdfParams = FipsKDF.HKDF.withPRF(FipsKDF.AgreementKDFPRF.SHA256_HMAC) .using(secretBytes) - .withIV(HKDF_INFO); // BC FIPS maps withIV → HKDF info (expand-phase context binding per RFC 5869) + .withIV(HKDF_INFO); KDFCalculator<FipsKDF.AgreementKDFParameters> kdf = new FipsKDF.AgreementOperatorFactory().createKDFCalculator(hkdfParams); - byte[] derivedKey = new byte[AES_KEY_LENGTH_BYTES]; // AES-256 + byte[] derivedKey = new byte[AES_KEY_LENGTH_BYTES]; kdf.generateBytes(derivedKey); return new SecretKeySpec(derivedKey, "AES"); } catch (final Exception e) { throw new RuntimeException("Error deriving key from secret", e); -} finally { - Arrays.fill(secretBytes, (byte) 0); } Suggestion importance[1-10]: 6 __ Why: Reasonable concern that Arrays.fill(secretBytes, (byte) 0) in the finally block may zero out caller-owned byte arrays (e...

[github-actions]

Persistent review updated to latest commit f908586

[github-actions]

Persistent review updated to latest commit de757a3

[github-actions]

Persistent review updated to latest commit b9f7617

[github-actions]

Persistent review updated to latest commit b4fce9b

[github-actions]

Persistent review updated to latest commit 8e3f614

[github-actions]

Persistent review updated to latest commit 5c719f9

[github-actions]

Persistent review updated to latest commit b194efc

[github-actions]

Persistent review updated to latest commit 07f75d1

[github-actions]

Persistent review updated to latest commit ab97aab

[github-actions]

Persistent review updated to latest commit 905dbb1

[github-actions]

Persistent review updated to latest commit 8f6e1e1

[github-actions]

Persistent review updated to latest commit fd9ed30

[reta]

We don't care about FIPS compatibility here (as in https://github.com/opensearch-project/security/pull/5866/changes?w=1#diff-f34c84b94aa1e1326e55ce390191ae380ce278f997f4ce1c2849310a662331c4R78)?

[beanuwave]

"encryptionKey!!" at 120 bits satisfies cryptographic minimum the provider cares about >= 112bit. But we could reuse TestSecurityConfig.DEFAULT_TEST_PASSWORD here - so it future-proof.

[reta]

Would be good to reuse, thanks

[reta]

This looks odd, why do we have to change these settings? No relation to FIPS I believe

[beanuwave]

The generic client fires all requests at once ParallelFlux.from(monos) - for the test with 15k unbounded requests in FIPS-mode (executed in local env) ~80% of exceptions are connect/handshake timeouts at the node: the server can't accept() + handshake fast enough, so SYNs/handshakes expire. FIPS trips it first because the BCTLS handshake is ~x4 costlier.

IMO it comes down to two solutions:

• Explicitly limit parallel requests - this code (introduces in fd9ed30)
• Proper saturation via backpressure (parallelism = demand). Let Reactor apply backpressure so parallelism is the concurrency limit:

Flux.fromArray(monos).flatMap(mono -> mono, parallelism).collectList().block(...);

[reta]

What about third option to increase connect timeouts?

[beanuwave]

generic-client storm (15k requests, parallelism 100, FIPS)

Metric	30s / 30s	120s / 120s
ok	7,920 (52.8%)	13,525 (90.2%)
failed	7,080	1,475
TCP_CONNECT_TIMEOUT	5,144	0
TLS_HANDSHAKE_TIMEOUT	839	0
TCP_CONNECTION_RESET	738	1,281
PREMATURE_CLOSE	359	194
elapsed time	50.1s	81.9s

Takeaway: raising the connect/handshake timeout (30s -> 120s) eliminates the*_TIMEOUT failures but cannot make the run green - TCP_CONNECTION_RESET (accept-queue overflow) grows and wall-clock rises.
2nd option is a better solution, because it keeps the accept queue shallow so nothing overflows and nothing waits near a timeout.

[reta]

I see, thank you, let's do 2nd option

[reta]

I believe we used to have openssl tests (and dependencies) but not anymore, how come non-JDK provider could be selected? We shouldn't have any on classpath

[beanuwave]

true, it's dead code on arrival. We can revert this change or replace it with assert !OpenSsl.isAvailable()

[reta]

Suggested change

	new SecureRandom().nextBytes(keyMaterial);
	Randomness.createSecure().nextBytes(keyMaterial)
	```?

[reta]

Suggested change

	new SecureRandom().nextBytes(bytes);
	Randomness.createSecure().nextBytes(bytes);
	```?

[reta]

The LDAP related changes (SNI, etc) seems to be unrelated to FIPS. could we extract them into separate feature + pull request?

[beanuwave]

I wouldn't quite call the LDAP changes FIPS-unrelated. The SNI plumbing and the PKCS12 workaround only become necessary because of running LDAPS through the BCTLS/BCFIPS provider stack.

That said, I do agree they're a fairly substantial, self-contained part of this PR. Splitting them out would make this diff smaller and keep LDAP-related chunk in one place.

Btw. as mentioned in the PR description, updating ldaptive for version 2.x would likely make the SNI plumbing unnecessary anyway.

[reta]

Thanks @beanuwave

That said, I do agree they're a fairly substantial, self-contained part of this PR. Splitting them out would make this diff smaller and keep LDAP-related chunk in one place.

👍 , this is exactly the reasons

Btw. as mentioned in the PR description, updating ldaptive for version 2.x would likely make the SNI plumbing unnecessary anyway.

If we could do that (as separate change) so we could get rid of LDAP workarounds - even better

[beanuwave]

sound like a plan - I'll prepare a new LDAP PR!

[reta]

I think with this change, the upgrade path (rolling upgrade primarily) for existing clusters will not be possible, right?

[reta]

Probably AtomicBoolean + CAS will be safer: if (initialized.compareAndSet(false, true)) ..., than we don't really need synchronized

[reta]

Could we instead of checking PKCS11 in many places as true/false, represent that as data classes? For Pkcs11KeyStoreConfiguration and BcfksKeyStoreConfiguration fe?

[reta]

@beanuwave this is tremendous effort, thank you, a few high level comments:

• the change is very large and difficult to review (it took me at least full day, split over the week), I strongly believe it will help to split it into smaller chunks (fe LDAP we already identified, there are probably other subareas)
• the logic is primarily driven by ifs (fips mode, store types, etc..), if we could project that to data classes / abstractions, it will be much simpler to read and reason about
• the upgrade path (for existing clusters) in unclear (to me): could non FIPS node be updated in place? could FIPS node join non-FIPS cluster? (or vice versa)

Hope it make sense, thank you!

[beanuwave]

@reta Thank you for pushing this through the review process - that's great!

If you plan to do a second review, I'd really appreciate it if you could also take the "Reviewer call-outs" into account.

the logic is primarily driven by ifs (fips mode, store types, etc..), if we could project that to data classes / abstractions, it will be much simpler to read and reason about

The FIPS test class abstraction is a no-brainer, since it would then follow the same style as the core project. However, I'm not sure the same general rules can be applied to production code - it's more of a case-by-case assessment.

the upgrade path (for existing clusters) in unclear (to me): could non FIPS node be updated in place? could FIPS node join non-FIPS cluster? (or vice versa)

Do you have a specific scenario in mind where a heterogeneous cluster would be advantageous and provide real-world value? Assuming it would be possible for a non-FIPS node to join a FIPS cluster, IMO that would introduce a weak link in the chain and undermine the FIPS security guarantees of the entire cluster.

[reta]

If you plan to do a second review, I'd really appreciate it if you could also take the "Reviewer call-outs" into account.

👍

Do you have a specific scenario in mind where a heterogeneous cluster would be advantageous and provide real-world value? Assuming it would be possible for a non-FIPS node to join a FIPS cluster, IMO that would introduce a weak link in the chain and undermine the FIPS security guarantees of the entire cluster.

It is probably could be summarized like that: how would we recommend to start with FIPS support. Only brand new clusters or gradual migrations.

The FIPS test class abstraction is a no-brainer, since it would then follow the same style as the core project. However, I'm not sure the same general rules can be applied to production code - it's more of a case-by-case assessment

Correct, tests are indeed on-brainer, the one which really clicked for me for PCKS11, I think we could have cleaner implementation, but certainly - case by case.