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 03cdcc4)

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/BCTLS support and hostname-aware connections 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/SniAwareConnection.java src/main/java/org/opensearch/security/auth/ldap2/SocketFactoryClassLoader.java src/main/java/org/opensearch/security/auth/ldap2/HostnameAwareConnectionFactory.java src/test/java/org/opensearch/security/auth/ldap2/SniAwareConnectionTest.java src/test/java/org/opensearch/security/auth/ldap2/SNISettingTLSSocketFactoryTest.java src/test/java/org/opensearch/security/auth/ldap2/HostnameAwareConnectionFactoryTest.java src/test/java/org/opensearch/security/auth/ldap2/LdapMtlsSniAuthenticationTest.java Sub-PR theme: OBO/JWT tests updated for AES-GCM/HKDF encryption + BCFKS keystore support Relevant files: src/test/java/org/opensearch/security/http/OnBehalfOfAuthenticatorTest.java src/test/java/org/opensearch/security/authtoken/jwt/JwtVendorTest.java Sub-PR theme: PemKeyReader rebuilt on BouncyCastle with BCFKS/PKCS11 support Relevant files: src/main/java/org/opensearch/security/support/PemKeyReader.java src/test/java/org/opensearch/security/support/PemKeyReaderLoadKeyStoreTest.java src/test/java/org/opensearch/security/support/PemKeyReaderLoadSecretKeyTest.java

⚡ Recommended focus areas for review Possible NPE configureSNISocketFactory calls connFactory.getProvider().getProviderConfig() and then providerConfig.setProperties(props). If getProviderConfig() returns null (or the provider has not been initialized for this factory), this will throw NPE and prevent the LDAPS connection from being established. Consider a null check or defensive initialization before setting the SNI socket factory property. @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 hostname source In getConnection0, configureSNISocketFactory is called and SNI is configured using split[0], which appears to be the host component parsed earlier from the LDAP URL. If split[0] includes a scheme or is empty for some URL forms, SNI will be wrong or invalid. Consider parsing via URI for consistency with checkConnection0 and validating the hostname before configuring SNI. if (enableSSL) { configureSNISocketFactory(connFactory); try (var ignored = SNISettingTLSSocketFactory.configure(split[0])) { connection = connFactory.getConnection(); connection.open(); } } else { connection = connFactory.getConnection(); connection.open(); } NPE on unspecified type extractStoreType calls finalStoreType.toUpperCase(...) in the error message. If storeType is null and detectStoreType(storePath) returns null (e.g., unrecognized magic bytes or read error), finalStoreType may be null, causing an NPE in the error path instead of the intended IllegalArgumentException. Add a null check before invoking toUpperCase. public static String extractStoreType(String storePath, String storeType) { var finalStoreType = Optional.ofNullable(storeType).orElseGet(() -> detectStoreType(storePath)); var isFipsSupportedStoreType = Stream.of(PKCS11, BCFKS).anyMatch(it -> it.equalsIgnoreCase(finalStoreType)); if (FipsMode.isEnabled() && !isFipsSupportedStoreType) { throw new IllegalArgumentException( finalStoreType.toUpperCase(Locale.ROOT) + " keystores / truststores are not supported in FIPS mode - use BCFKS or PKCS#11" ); } return finalStoreType; } Fragile hash mapping BCRYPT_HASH_TO_PLAINTEXT hardcodes fixture bcrypt hashes to plaintext. If a fixture hash is added or modified without updating this map, tests will silently use unadapted (bcrypt) hashes under FIPS and fail obscurely. Consider generating hashes at runtime from a known plaintext set, or asserting coverage in a setup step to fail fast when a fixture is not registered here. private static final Map<String, String> BCRYPT_HASH_TO_PLAINTEXT = Map.ofEntries( // top-level internal_users.yml and the various <resource-folder>/internal_users.yml fixtures Map.entry("$2a$04$idGSEpNOhFbyiRL6toGPT.orh7ENOEU8kAqwkRFaXWRdA6wVgyqUu", "user_b"), Map.entry("$2a$04$jQcEXpODnTFoGDuA7DPdSevA84CuH/7MOYkb80M3XZIrH76YMWS9G", "user_c"), Map.entry("$2a$04$NDy7mGbRNrmPMh9nSnIB.OTMFkcioEd69A04ReSGkJDd7QHxnCcVC", "user_a"), Map.entry("$2a$12$4AcgAt3xwOWadA5s5blL6ev39OXDNhmOesEoo33eZtrq2N0YrU3H.", "kibanaserver"), Map.entry("$2a$12$61vXe3cXy32p0cjsW0Y/SeZa7kEVSWuQK0jg98D9d5zOGXfo5NgyC", "crusherw"), Map.entry("$2a$12$6.4Y6L//xeKQ7t8YEG0s6OH4F4q9gMw0J8E0GjmUMNZeyIWu1IRWS", "user_role01_role02_role03"), Map.entry("$2a$12$A41IxPXV1/Dx46C6i1ufGubv.p3qYX7xVcY46q33sylYbIqQVwTMu", "worf"), Map.entry("$2a$12$bP0CO5d5nhmaTOj7mGteHugXQQ8jlSV0dxcl5//moZ1xnI.pVPXfe", "abc:abc"), Map.entry("$2a$12$GI9JXffO3WUjTsU7Yy3E4.LBxC2ILo66Zg/rr79BpikSL2IIRezQa", "spock"), Map.entry("$2a$12$Ioo1uXmH.Nq/lS5dUVBEsePSmZ5pSIpVO/xKHaquU/Jvq97I7nAgG", "sarek"), Map.entry("$2a$12$JU2QjYVTlI24Q/enEOpf2uTLCPGchN.eXWCsrBiieUcRoeh53NB0y", "restoreuser"), Map.entry("$2a$12$LZvbDVnegkTbEFTu9hHnWO4HIrdB9rGaKcEOID5n0VV4j58cnvyZ.", "writer"), Map.entry("$2a$12$n5nubfWATfQjSYHiWtUyeOxMIxFInUHOAx8VMmGmxFNPGpaBmeB.m", "nagilum"), Map.entry("$2a$12$P.QbiwOsnxgz7kLBT10F7u6GhY7//Keyz7Xwf7lNzskRxpo9.zxFS", "theindexadmin"), Map.entry("$2a$12$wkY2BsRneCU5za1OPYlzsehQit6gu2vprVv/4jHiSEEBv2ThunaTS", "picard"), Map.entry("$2a$12$XrBfLQh2T8wIzpxE5vzhUOPjjGfONcD8UEjd5IT5KveG8ULZaj04.", "user_role01"), Map.entry("$2a$12$xZOcnwYPYQ3zIadnlQIJ0eNhX1ngwMkTN.oMwkKxoGvDVPn4/6XtO", "kirk"), Map.entry("$2a$12$9Zr4IgoJRqK6xJq4xjoa6OZAnY4QOQ6xIhcCxeYoQtB/HriMkeJSC", "dlsnoinvest"), Map.entry("$2a$12$VcCDgh2NDk07JGN0rjGbM.Ad41qVR/YFJcgHp0UGns5JDymv..TOG", "admin"), Map.entry("$2a$12$1HqHxm3QTfzwkse7vwzhFOV4gDv787cZ8BwmCwNEyJhn0CZoo8VVu", "test"), // FLS/DLS caching + indexing fixtures (dlsfls/, cache/) Map.entry("$2a$12$7QIoVBGdO41qSCNoecU3L.yyXb9vGrCvEtVlpnC4oWLt/q0AsAN52", "hr_employee"), Map.entry("$2a$12$JJSXNfTowz7Uu5ttXfeYpeYE0arACvcwlPBStB1F.MI7f0U9Z4DGC", "kibanaro"), Map.entry("$2a$12$YCBrpxYyFusK609FurY5Ee3BlmuzWw0qHwpwqEyNhM2.XnQY3Bxpe", "password"), Map.entry("$2y$12$SP9z.rBgEHTlueKkiqSK/OxqB2PLJN/eRoNJ8WOPoHWIpirvbFAAy", "password"), Map.entry("$2a$12$30rb6oabnodiSdysdWJnhO.4sVRkyNudPC1woYCJFhXja3rkyXbam", "dlsflsuser"), Map.entry("$2a$12$Kv.4sU5r1zy2ZqnSDm99Ae6ImCMKtjJq4enT.9d3c55cA0O2LGNH6", "finance_employee"), Map.entry("$2a$12$c26Pnq6yiZcgi8PxNEyp5O3wIn1G1eJfCvJFifEKosQJeojUZf/D6", "finance_trainee"), Map.entry("$2a$12$LRNG7ETwMcO68VNh14B3AuKPkvOaC0k26.QnSrv9AvbmT1JRNMJum", "hr.employee"), Map.entry("$2a$12$s6rC7o345lvXp.JpTrA91O6xYAGVCCxKdVclsNkWJTaquW4GK9E9u", "hr_trainee"), Map.entry("$2a$12$gxsE8oEicXy3mNBkGEO5K.P3J/CDq3GHXDYeQTmVI/v3AA84vqIXm", "no_roles"), Map.entry("$2a$12$6sre4JH7O4Rgh7ubWmeyWus6UIIA13MqW8eR8KD5Qbxn06CDbJG/G", "snapshotrestore"), // rest-api admin fixtures (restapi/) Map.entry("$2y$12$ft8tXtxb.dyO/5MrDXHLc.e1o3dktEQJMvR2e.sgVDyD/gR7G9dLS", "admin_all_access"), Map.entry("$2y$12$W5AdCO/j08KiDu7EF/1Zf.nkcQM/7s.TtAdN2pRpbDM31xXcIIJUq", "rest_api_admin_allowlist"), Map.entry("$2y$12$xFUIepz0vILRMzMkZMGY1Ow1P1eJo8TJ2oGiaFXaenGrOMsmDnKZS", "rest_api_admin_nodesdn"), Map.entry("$2y$12$X5ZamIheHYc2bihGTbK66Oe1.1vJ19akH0OFGF7TvI2BhbbED.KcO", "rest_api_admin_user"), Map.entry("$2y$12$aHkyhk95XbrMCByYYVAlrek1thXpTDuVKJW01vdLYPh6kyR36j7x6", "user_rest_api_access"), Map.entry("$2y$12$xgJfGiHpYOkRpF9W9dXYZOpJJ4bHz3VTwdv7ZZYTwlvx7NbH62qUi", "user_tenant_parameters_substitution"), Map.entry("$2y$12$capXg1HNP49Vxeb6ijzRnu5BLMUE0ZePq1l3MhF8tjnuxg614uaY6", "rest_api_admin_config_update"), Map.entry("$2y$12$pUn1a6jdIeR.stkvEqNe5uK3rOY7Dj3uQfE8Cvd2bjNjTQ2HbsBMK", "rest_api_admin_internalusers"), Map.entry("$2y$12$BR.CBsElNLj8v2dzpHJ7bOKVLwWKWjKDhlEvBIvAe9b6/m0xWy2Bq", "rest_api_admin_roles"), Map.entry("$2y$12$irI4k0eKE8z9OXEd1jO4eeQfPV8WRMfttzutAhEeRBWy5XNXOlpr.", "rest_api_admin_ssl_info"), Map.entry("$2y$12$DxNdaBBMvTq5wO5XlnwlTeGSaC7yNoFoJt2N5TVtraopxPnGjMol2", "rest_api_admin_ssl_reloadcerts"), Map.entry("$2y$12$q05T7m7DFtkLLj.MVJ6jjuZkAywG4ZwaNi9fiYn6XCJelN2TUXCy2", "rest_api_admin_tenants"), // index-pattern / protected-indices / system-indices fixtures Map.entry("$2y$12$93KcWlQxeify28LSx8EjYOnHv1AQ6vJZXSRUVTnfSN7AxTfvCBfu.", "indexAccessNoRoleUser"), Map.entry("$2y$12$LavzCpUFiFwXD22rc0n.SOVExdnzDcn6lHY48XKPl6KKdvAHm0awm", "protectedIndexUser"), Map.entry("$2y$12$15ZXoaH/sB.0nESo6VABt.V02HkpA2lQ5QvIFcVqNelUoAdXv1g3O", "negated_regex_user"), Map.entry("$2y$12$eaSv29maDe1Y0FQXMHi1legXq8Ec/YbWujMq5Mg2RhYZEc9Pzw19y", "negative_lookahead_user"), Map.entry("$2y$12$d1ONiqarfTF9xOuqKeNukeze1bVBXC1FyXJSpuC3B6/Ekbfu3ULHm", "normal_user"), Map.entry("$2y$12$V3.ACgUpHP9TlSbV3CNekOGB1NVov1C6Rq3QtXWCvACKVeQnkBCgG", "normal_user_without_system_index") );

[github-actions]

PR Code Suggestions ✨

Latest suggestions up to 03cdcc4

Explore these optional code suggestions:

Category	Suggestion	Impact
General	Handle space-separated LDAP URLs for SNI parsing config.getLdapUrl() may contain multiple space-separated URLs (ldaptive supports this). Passing the whole string to new URI(...) will throw URISyntaxException and silently disable SNI. Parse only the first URL, or iterate and pick the first ldaps host, to reliably extract the hostname. src/main/java/org/opensearch/security/auth/ldap/backend/LDAPAuthorizationBackend.java [214-217] String sniHostname = null; if (config.getLdapUrl() != null && config.getLdapUrl().startsWith("ldaps:")) { configureSNISocketFactory(connFactory); - String ldapUrl = config.getLdapUrl(); + String ldapUrl = config.getLdapUrl().split("\\s+")[0]; try { sniHostname = new URI(ldapUrl).getHost(); Suggestion importance[1-10]: 7 __ Why: Valid concern: ldaptive supports space-separated URLs, and passing the whole string to new URI(...) would throw URISyntaxException and silently disable SNI (only logging a warning). Splitting to use the first URL is a reasonable improvement for reliability.	Medium
	Add double-checked locking fast path initialized is only written under the synchronized block but read from the non-synchronized extractCredentials0 path via ensureInitialized(). Since initialized, jwtParser, and encryptionUtil are declared volatile, the fast path (skipping the synchronized block) is missing — every call takes the lock. Add a volatile read fast-path check to preserve the intended one-time initialization performance. 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); +private boolean ensureInitialized() { + if (initialized) { + return jwtParser != null; + } + synchronized (this) { + if (!initialized) { + 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); + } + initialized = true; } } return jwtParser != null; } Suggestion importance[1-10]: 6 __ Why: Valid performance improvement: the current synchronized method forces every authentication call to acquire the lock even after initialization. Double-checked locking with the volatile flag preserves correctness while eliminating contention on the hot path.	Low
	Parse LDAP URL once in constructor Parsing ldapUrl on every getConnection() call is wasteful and, more importantly, will throw on every invocation if the URL is malformed instead of failing fast at construction. Parse the URL once in the constructor and store the hostname as a final field. src/main/java/org/opensearch/security/auth/ldap2/HostnameAwareConnectionFactory.java [43-46] +private final String hostname; + +public HostnameAwareConnectionFactory(ConnectionConfig config, String ldapUrl) { + super(config); + this.ldapUrl = ldapUrl; + this.hostname = new LdapURL(ldapUrl).getEntry().getHostname(); +} + @Override public Connection getConnection() { - String hostname = new LdapURL(ldapUrl).getEntry().getHostname(); return new SniAwareConnection(super.getConnection(), hostname); } Suggestion importance[1-10]: 5 __ Why: Valid minor optimization and fail-fast improvement. Impact is modest since connection creation is not on the hot path, but it improves error diagnosis for malformed URLs.	Low
	Validate keystore password before loading key material loadKeyMaterial requires a non-null password when the keystore contains encrypted key entries. Passing null (when kspass is null) will trigger NPE or fail key extraction. Consider validating that kspass is non-null before this call, or handle the null case explicitly (e.g., prompt or fail with a clear error). src/main/java/org/opensearch/security/tools/SecurityAdmin.java [1576-1579] if (ks != null) { - final char[] kspassChars = kspass == null ? null : kspass.toCharArray(); + if (kspass == null) { + throw new IllegalArgumentException("Keystore password is required to load key material from '" + ks + "'"); + } + final char[] kspassChars = kspass.toCharArray(); final KeyStore keyStore = loadStore(ks, keyStoreType, kspassChars); sslContextBuilder.loadKeyMaterial(keyStore, kspassChars, (aliases, socket) -> { Suggestion importance[1-10]: 4 __ Why: While a null password can cause issues with encrypted keys, loadKeyMaterial accepts null passwords for keystores without encrypted entries. The suggestion adds a strict validation that may break valid use cases, and this is a CLI tool where errors would surface clearly anyway.	Low
	Guard buffer.array() usage against unsupported buffers ByteBuffer.array() throws UnsupportedOperationException for read-only or non-array-backed buffers. Although StandardCharsets.UTF_8.encode(...) typically returns a heap buffer today, this is not contractually guaranteed; guard with hasArray() (and consider arrayOffset()) to avoid a potential runtime failure. 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()) { + Arrays.fill(passwordByteBuffer.array(), passwordByteBuffer.arrayOffset(), passwordByteBuffer.arrayOffset() + passwordByteBuffer.capacity(), (byte) 0); +} Suggestion importance[1-10]: 4 __ Why: Technically correct that ByteBuffer.array() can throw for non-array-backed buffers, but StandardCharsets.UTF_8.encode() reliably returns a heap buffer, making this largely defensive.	Low
	Avoid catching Throwable when verifying hashes Catching Throwable is dangerous because it swallows Errors such as OutOfMemoryError, StackOverflowError, and VirtualMachineError, which can hide serious JVM problems and make debugging very difficult. Narrow the catch to Exception (or a more specific exception type) so fatal errors continue to propagate. src/main/java/org/opensearch/security/hasher/PBKDF2PasswordHasher.java [49-50] public boolean check(char[] password, String hash) { checkPasswordNotNullOrEmpty(password); checkHashNotNullOrEmpty(hash); 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]: 4 __ Why: Narrowing to Exception is a reasonable practice, but catching Throwable here is likely intentional to handle BC FIPS FipsUnapprovedOperationError (an Error subclass) during hash checks. The suggestion could regress that behavior.	Low
	Document idempotency behavior of config adapter Since PBKDF2 hashes contain non-deterministic salt, pbkdf2Replacements() is memoized once at first call. If the same bcrypt hash appears multiple times in a config file, all occurrences are replaced with the same PBKDF2 hash — which is fine — but re-invoking adaptConfig on already-adapted content will not match. Consider documenting this or making the replacement idempotent by checking whether the content already contains PBKDF2 markers. src/test/java/org/opensearch/security/test/helper/file/FipsHashAdapter.java [111-122] public static String adaptConfig(final String content) { if (content == null || content.isEmpty() || !FipsMode.isEnabled()) { return content; } + // Idempotency guard: skip if already adapted (PBKDF2 hashes start with "$3$"). 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; } Suggestion importance[1-10]: 2 __ Why: Minor documentation suggestion for a test-only helper. The proposed "improved_code" only adds a comment without changing behavior, providing marginal value.	Low
	Escape hash before embedding in YAML The generated hash is interpolated directly into a YAML string with quotes, so a hash containing a " or </code> character will produce invalid YAML or an incorrect hash value. Escape backslashes and double quotes in the hash before embedding it into the YAML template to guarantee the file remains parseable regardless of the hasher's output. src/integrationTest/java/org/opensearch/security/ConfigurationFiles.java [47-66] private static void writeInternalUsersFile(Path destination) throws IOException { String hash = TestSecurityConfig.hashPassword(TestSecurityConfig.DEFAULT_TEST_PASSWORD); + String escapedHash = hash.replace("\\", "\\\\").replace("\"", "\\\""); String content = """ --- _meta: type: "internalusers" config_version: 2 new-user: hash: "%s" limited-user: hash: "%s" opendistro_security_roles: - "user_limited-user__limited-role" admin: hash: "%s" opendistro_security_roles: - "user_admin__all_access" - """.formatted(hash, hash, hash); + """.formatted(escapedHash, escapedHash, escapedHash); Files.writeString(destination, content, StandardCharsets.UTF_8); } Suggestion importance[1-10]: 2 __ Why: BCrypt/PBKDF2 hashes produced by the password hasher do not contain " or \ characters, so escaping is unnecessary in practice. This is defensive but low-impact for a test utility.	Low
Possible issue	Avoid path-based type detection for PKCS11 When type is PKCS11 and storePath is null, extractStoreType(null, type) will be called. Inspect extractStoreType/detectStoreType — if either dereferences storePath (e.g., calls toLowerCase() on it) this will NPE. Guard by passing the type directly for PKCS11 without invoking path-based 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); + String storeType = PKCS11.equalsIgnoreCase(type) ? PKCS11 : extractStoreType(storePath, type); Suggestion importance[1-10]: 7 __ Why: Valid concern: when type is PKCS11 and storePath is null, extractStoreType(null, type) is called. If detection paths dereference storePath, this could NPE. The suggested guard is a reasonable defensive fix, though extractStoreType may already handle this since type is provided.	Medium
	Guard against empty TrustManagers array TrustManagerFactory.getTrustManagers() may return multiple trust managers (e.g., when the truststore has entries of different types or when the provider returns more than one). Blindly picking index 0 can drop trust anchors or, if the array is empty, throw ArrayIndexOutOfBoundsException. Prefer passing all returned trust managers, or at minimum guard against an empty array. src/main/java/org/opensearch/security/auth/ldap2/LDAPConnectionFactoryFactory.java [355-365] if (this.sslConfig.isTrustAllEnabled()) { ldaptiveSslConfig.setTrustManagers(new AllowAnyTrustManager()); } else { try { TrustManagerFactory tmf = TrustManagerFactory.getInstance("PKIX"); tmf.init(this.sslConfig.getEffectiveTruststore()); - ldaptiveSslConfig.setTrustManagers(tmf.getTrustManagers()[0]); + final var tms = tmf.getTrustManagers(); + if (tms == null || tms.length == 0) { + throw new IllegalStateException("No PKIX TrustManagers returned for LDAPS"); + } + ldaptiveSslConfig.setTrustManagers(tms); } catch (GeneralSecurityException e) { throw new IllegalStateException("Failed to initialize PKIX TrustManager for LDAPS", e); } } Suggestion importance[1-10]: 6 __ Why: Valid defensive improvement: passing all returned trust managers (rather than only index 0) is more correct and avoids potential ArrayIndexOutOfBoundsException. Impact is moderate since PKIX typically returns a single trust manager.	Low
	Validate decoded payload length before decryption If encryptedString decodes to fewer than GCM_NONCE_LENGTH bytes (e.g., short/corrupted input), Arrays.copyOfRange will pad the nonce with zeros and produce an empty ciphertext, potentially masking corruption as a generic AEAD failure or worse. Validate the minimum length up front to fail fast with a clear error. src/main/java/org/opensearch/security/authtoken/jwt/EncryptionDecryptionUtil.java [98-102] public String decrypt(final String encryptedString) { byte[] decodedBytes = Base64.getDecoder().decode(encryptedString); + if (decodedBytes.length < GCM_NONCE_LENGTH) { + throw new RuntimeException("Encrypted payload too short: missing GCM nonce"); + } try { byte[] nonce = Arrays.copyOfRange(decodedBytes, 0, GCM_NONCE_LENGTH); byte[] ciphertext = Arrays.copyOfRange(decodedBytes, GCM_NONCE_LENGTH, decodedBytes.length); Suggestion importance[1-10]: 5 __ Why: Reasonable input validation to fail fast with a clearer error on short/corrupted ciphertexts. The current code would still fail via AEAD verification, so the impact is mainly error clarity.	Low
Security	Restrict mutable FIPS detection field visibility The envSupplier field is public static and mutable, which allows any code (not just tests) to alter global FIPS-mode detection at runtime — a security-sensitive setting that should be immutable in production. Restrict visibility to package-private (or use a dedicated test hook) so production callers cannot flip FIPS detection. src/main/java/org/opensearch/security/support/FipsMode.java [17-21] public final class FipsMode { - 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: Valid security concern — a public mutable static that controls FIPS-mode detection is a risk. Reducing visibility to package-private is a good defensive change, though the test in a different package would then need adjustment.	Low

---

Previous suggestions

Suggestions up to commit 5579028

Category	Suggestion	Impact
Possible issue	Avoid catching Throwable in password check Catching Throwable here silently swallows serious errors such as OutOfMemoryError, StackOverflowError, and JVM-level errors like FipsUnapprovedOperationError that indicate configuration problems administrators must see. Narrow the catch to Exception (or a more specific exception type) so fatal errors propagate, while still returning false for expected failures like malformed hashes. src/main/java/org/opensearch/security/hasher/PBKDF2PasswordHasher.java [49-50] public boolean check(char[] password, String hash) { checkPasswordNotNullOrEmpty(password); checkHashNotNullOrEmpty(hash); 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 is indeed too broad and could hide serious errors like OutOfMemoryError. However, the PR author may have intentionally used Throwable to catch FipsUnapprovedOperationError (which extends Error, not Exception). The suggestion is reasonable but context-dependent.	Low
	Avoid file detection for PKCS11 stores When storePath is null and type is PKCS11, extractStoreType(null, type) is invoked which then flows into detectStoreType(null) if type were somehow null, and more importantly path.toString() style callers may fail. Ensure extractStoreType handles the PKCS11 case where storePath is null without attempting file detection. src/main/java/org/opensearch/security/support/PemKeyReader.java [156-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) ? PKCS11 : extractStoreType(storePath, type); Suggestion importance[1-10]: 5 __ Why: Reasonable defensive suggestion: when storePath is null and type is PKCS11, extractStoreType may attempt detectStoreType(null). Guarding the PKCS11 case improves clarity and avoids potential NPE, though the actual code path may already handle it.	Low
	Avoid NPE on null keystore type loadStore calls type.toUpperCase() unconditionally, which will NPE if keyStoreType is null. Guard against a null keyStoreType (or fall back to a sensible default) before calling loadStore, since the earlier auto-detection may not run for every code path. src/main/java/org/opensearch/security/tools/SecurityAdmin.java [1577-1579] if (ks != null) { final char[] kspassChars = kspass == null ? null : kspass.toCharArray(); + if (keyStoreType == null) { + keyStoreType = PemKeyReader.extractStoreType(ks, null); + } final KeyStore keyStore = loadStore(ks, keyStoreType, kspassChars); Suggestion importance[1-10]: 4 __ Why: The concern is partially valid — loadStore calls type.toUpperCase() which would NPE on null, but in the execute flow the type is auto-detected earlier via PemKeyReader.extractStoreType, so this is a defensive improvement rather than a real bug fix.	Low
General	Validate SNI hostname before configuring split[0] is used as the SNI hostname without validation; if the configured LDAP URL is malformed or empty, this can pass a blank or invalid value to SNI configuration (unlike the checkConnection0 path, which validates via URI and logs a warning). Parse the URL and skip SNI configuration when the hostname is blank, consistent with the check-connection code path. src/main/java/org/opensearch/security/auth/ldap/backend/LDAPAuthorizationBackend.java [335-344] if (enableSSL) { configureSNISocketFactory(connFactory); - try (var ignored = SNISettingTLSSocketFactory.configure(split[0])) { + String sniHost = null; + try { + sniHost = new URI(split[0]).getHost(); + } catch (URISyntaxException e) { + log.warn("Malformed LDAP URL '{}'; proceeding without SNI configuration: {}", split[0], e.getMessage()); + } + try (var ignored = StringUtils.isBlank(sniHost) ? null : SNISettingTLSSocketFactory.configure(sniHost)) { connection = connFactory.getConnection(); connection.open(); } } else { connection = connFactory.getConnection(); connection.open(); } Suggestion importance[1-10]: 6 __ Why: Valid consistency concern: the checkConnection0 path validates via URI and handles malformed URLs, but getConnection0 uses split[0] directly. Adding validation improves robustness against malformed LDAP URLs.	Low
	Avoid mutating caller-owned key bytes deriveKey zeros out the input secretBytes in its finally block. When callers pass their own byte array to the byte[] constructor, this silently mutates their input, which can break subsequent use of the array by the caller. Either document this destructive contract clearly or defensively copy the array before passing it to deriveKey. src/main/java/org/opensearch/security/authtoken/jwt/EncryptionDecryptionUtil.java [69-71] public EncryptionDecryptionUtil(final String encodedSecret) { this(decodeBase64(encodedSecret)); } public EncryptionDecryptionUtil(final byte[] secretBytes) { - this.aesKey = deriveKey(secretBytes); + // deriveKey zeroes the array it receives; work on a copy so we don't mutate caller-owned input. + this.aesKey = deriveKey(secretBytes.clone()); } Suggestion importance[1-10]: 6 __ Why: Valid observation that deriveKey zeroes the input array in its finally block, which silently mutates caller-owned byte arrays passed to the public byte[] constructor. Cloning defensively is a reasonable improvement, though the current behavior may be intentional for security (wiping key material ASAP).	Low
	Select X509TrustManager explicitly, not by index Blindly picking getTrustManagers()[0] assumes an X509 trust manager is at index 0; some providers can return a different order or type, causing a ClassCastException or ineffective trust checks downstream. Filter for the first X509TrustManager (or X509ExtendedTrustManager) and fail explicitly if none is found. src/main/java/org/opensearch/security/auth/ldap2/LDAPConnectionFactoryFactory.java [357-364] } else { try { TrustManagerFactory tmf = TrustManagerFactory.getInstance("PKIX"); tmf.init(this.sslConfig.getEffectiveTruststore()); - ldaptiveSslConfig.setTrustManagers(tmf.getTrustManagers()[0]); + javax.net.ssl.TrustManager x509Tm = null; + for (javax.net.ssl.TrustManager tm : tmf.getTrustManagers()) { + if (tm instanceof javax.net.ssl.X509TrustManager) { + x509Tm = tm; + break; + } + } + if (x509Tm == null) { + throw new IllegalStateException("No X509TrustManager returned by PKIX TrustManagerFactory"); + } + ldaptiveSslConfig.setTrustManagers(x509Tm); } catch (GeneralSecurityException e) { throw new IllegalStateException("Failed to initialize PKIX TrustManager for LDAPS", e); } } Suggestion importance[1-10]: 6 __ Why: Valid robustness improvement: while PKIX TrustManagerFactory typically returns an X509TrustManager first, filtering explicitly avoids potential ClassCastException in edge cases and makes the intent clearer.	Low
	Make SNI ThreadLocal context reentrant-safe If configure is called while an SNI context is already set on the thread (e.g. nested calls, or a prior call that failed to close), the previous value is silently overwritten and the returned SniContext will remove the ThreadLocal entirely on close, leaking the "outer" context. Save and restore the prior value to make configure 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]: 5 __ Why: Reasonable defensive suggestion; nested configure calls would overwrite the previous hostname and the returned context would remove rather than restore it. Impact is moderate since current usage patterns via SniAwareConnection do not appear to nest.	Low
	Preserve backward-compatible public API method The previous loginUsingKeytab method has been renamed/replaced by loginUsingKeytabWithContext, changing its return type from Subject to LoginContext. This is a breaking API change; if any external callers still use loginUsingKeytab, they will fail to compile. Consider keeping the original method as a thin wrapper (delegating to the new method and returning loginContext.getSubject()) to preserve backward compatibility. src/main/java/org/opensearch/security/auth/http/kerberos/util/JaasKrbUtil.java [48-52] +public static Subject loginUsingKeytab(final Set<String> principalAsStrings, final Path keytabPath, final boolean initiator) + throws LoginException { + return loginUsingKeytabWithContext(principalAsStrings, keytabPath, initiator).getSubject(); +} + public static LoginContext loginUsingKeytabWithContext( final Set<String> principalAsStrings, final Path keytabPath, final boolean initiator ) throws LoginException { final Set<Principal> principals = new HashSet<Principal>(); for (String p : principalAsStrings) { Suggestion importance[1-10]: 4 __ Why: The suggestion to preserve backward compatibility is reasonable, but this is an internal utility class within the security plugin, so external API breakage is unlikely. Score reflects moderate value.	Low
Security	Close SecureString after use securePassword is a SecureString acquired from SecureSetting.secureString(...).get(settings) but is never closed. Since SecureString implements Closeable and holds sensitive material, use a try-with-resources or explicitly close it after copying the chars into StorePassword to avoid leaving the underlying char array retained by the secure string. src/main/java/org/opensearch/security/ssl/config/SslCertificatesLoader.java [132-139] -if (!securePassword.isEmpty()) { - return StorePassword.of(securePassword.getChars()); +try (var secure = securePassword) { + if (!secure.isEmpty()) { + return StorePassword.of(secure.getChars()); + } } Suggestion importance[1-10]: 5 __ Why: Valid security hygiene point — SecureString implements Closeable and should be closed to clear sensitive material. However, the impact is limited since the chars are already copied to StorePassword.	Low
	Make password equality fully constant-time The early-return on length mismatch (password.length != other.length) makes equals non-constant-time by leaking length via timing. Since this class is documented as holding critical security parameters and uses XOR accumulation elsewhere for constant-time comparison, the length check should not short-circuit; returning false immediately on length difference undermines the intent. src/main/java/org/opensearch/security/ssl/config/StorePassword.java [41-56] @Override public boolean equals(Object o) { if (this == o) return true; if (o == null || getClass() != o.getClass()) return false; final var other = ((StorePassword) o).password; if (password == null || other == null) { return password == other; } - if (password.length != other.length) { - return false; - } - var difference = 0; - for (int i = 0; i < password.length; i++) { + var difference = password.length ^ other.length; + for (int i = 0; i < Math.min(password.length, other.length); i++) { difference |= password[i] ^ other[i]; } return difference == 0; Suggestion importance[1-10]: 5 __ Why: Valid observation about timing leak via length short-circuit for constant-time comparison. The impact is minor since password lengths leaking through timing is generally low-risk, but the suggestion aligns with the class's documented security intent.	Low
	Zero out password char array after use The truststorePassword char[] is never cleared after use; storing passwords as char[] is only beneficial if they are zeroed out promptly. Add a finally block to overwrite the array with Arrays.fill(truststorePassword, '\0') after ts.load() so the password does not linger in memory. src/main/java/org/opensearch/security/ssl/util/SSLRequestHelper.java [256-262] -if ("PKCS11".equalsIgnoreCase(truststoreType)) { - ts.load(null, truststorePassword); -} else { - try (final var fin = new FileInputStream(env.configDir().resolve(truststore).toAbsolutePath().toString())) { - ts.load(fin, truststorePassword); +try { + if ("PKCS11".equalsIgnoreCase(truststoreType)) { + ts.load(null, truststorePassword); + } else { + try (final var fin = new FileInputStream(env.configDir().resolve(truststore).toAbsolutePath().toString())) { + ts.load(fin, truststorePassword); + } + } +} finally { + if (truststorePassword != null) { + java.util.Arrays.fill(truststorePassword, '\0'); } } Suggestion importance[1-10]: 5 __ Why: Valid security hygiene suggestion to zero out the password char array. The benefit is modest since the password came from settings and likely exists elsewhere in memory, but it aligns with best practices for handling sensitive data.	Low

Suggestions up to commit 3ac26d1

Category	Suggestion	Impact
General	Handle multi-URL ldaptive URLs for SNI The LDAP URL may contain multiple space-separated URLs (ldaptive supports failover URLs of the form "ldaps://a ldaps://b"). Parsing the whole string as a URI will fail or take only the first URL's host; the SNI hostname will thus be wrong or missing when connecting to secondary URLs. Split the URL on whitespace and extract host only for the first, or accept that SNI may be incorrect across failover and document it. src/main/java/org/opensearch/security/auth/ldap/backend/LDAPAuthorizationBackend.java [211-223] 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(); if (StringUtils.isBlank(sniHostname)) { log.warn("Could not extract hostname from LDAP URL '{}'; proceeding without SNI configuration", ldapUrl); } } catch (URISyntaxException e) { log.warn("Malformed LDAP URL '{}'; proceeding without SNI configuration: {}", ldapUrl, e.getMessage()); } } Suggestion importance[1-10]: 6 __ Why: Valid point about ldaptive's failover URL support; parsing a space-separated URL string with new URI() would fail. The suggested split addresses the first URL correctly, though SNI for failover URLs remains a limitation.	Low
	Avoid swallowing Errors in password check Catching Throwable (including Error subclasses like FipsUnapprovedOperationError and OutOfMemoryError) and silently returning false hides serious FIPS-mode violations and JVM errors from operators/logs. At minimum, Error should be rethrown, or the caught type narrowed to Exception, and the failure logged so misconfigurations are diagnosable. 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) { + log.debug("PBKDF2 password check failed", e); return false; } finally { cleanup(passwordBuffer); } Suggestion importance[1-10]: 6 __ Why: Valid concern - catching Throwable including Error subclasses can hide FIPS violations and JVM errors. However, the improved code introduces a log reference that may not exist in the class, and the change might have been intentional to gracefully handle FIPS errors during password check.	Low
	Defensively check for empty provider array Security.getProviders(filter) returns null only when no providers match; but the API returns null for an empty result. Verify semantics: for a filter like "KeyStore.PKCS11", getProviders returns null when no provider supports it and a non-null array otherwise. The check is correct, but consider also handling the case where a length-0 array is returned (defensive), and use Security.getProviders("KeyStore.PKCS11") == null || Security.getProviders("KeyStore.PKCS11").length == 0. src/main/java/org/opensearch/security/tools/SecurityAdmin.java [1256-1258] boolean pkcs11Keystore = "PKCS11".equalsIgnoreCase(line.getOptionValue("kst")); if (!line.hasOption("vc") && !line.hasOption("ks") && !line.hasOption("cert") && !pkcs11Keystore) { throw new ParseException("Specify at least -ks, -cert, or -kst PKCS11"); } if (pkcs11Keystore && line.hasOption("ks")) { throw new ParseException( "Do not specify -ks together with -kst PKCS11; PKCS11 keystores are token-based and have no file path" ); } -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[] pkcs11Providers = Security.getProviders("KeyStore.PKCS11"); + if (pkcs11Providers == null || pkcs11Providers.length == 0) { + throw new ParseException("No PKCS#11 provider is registered in the JVM; cannot use -kst PKCS11"); + } } Suggestion importance[1-10]: 5 __ Why: Adding a length check is a reasonable defensive measure, though Security.getProviders(filter) typically returns null (not empty array) when no providers match, making this a minor robustness improvement.	Low
	Document destructive mutation of input array deriveKey zeroes the input secretBytes in its finally block, so callers who pass a byte array they still need will find it wiped. More critically, the String-based constructor allocates the array internally via decodeBase64, which is fine, but the public byte[] constructor silently mutates caller-owned data. Either document this destructive contract prominently or defensively copy the array before derivation to avoid surprising callers (including your own test testByteArrayConstructorWipesInput which relies on this behavior). src/main/java/org/opensearch/security/authtoken/jwt/EncryptionDecryptionUtil.java [69-71] public EncryptionDecryptionUtil(final String encodedSecret) { this(decodeBase64(encodedSecret)); } +/** + * NOTE: the provided {@code secretBytes} array will be zeroed out after key derivation. + * Callers must not reuse the array afterwards. + */ public EncryptionDecryptionUtil(final byte[] secretBytes) { this.aesKey = deriveKey(secretBytes); } Suggestion importance[1-10]: 5 __ Why: Adding a javadoc note about the destructive Arrays.fill on the caller-provided secretBytes is a reasonable documentation improvement, though it's a minor concern as the behavior is already tested and intentional for FIPS security.	Low
	Cache parsed LDAP hostname in constructor Parsing the LDAP URL on every getConnection() call is wasteful and re-throws IllegalArgumentException for malformed URLs on each connection attempt from a pool. Parse the URL once in the constructor and cache the hostname so misconfigurations fail fast at factory creation. src/main/java/org/opensearch/security/auth/ldap2/HostnameAwareConnectionFactory.java [43-46] +private final String hostname; + +public HostnameAwareConnectionFactory(ConnectionConfig config, String ldapUrl) { + super(config); + this.ldapUrl = ldapUrl; + this.hostname = new LdapURL(ldapUrl).getEntry().getHostname(); +} + @Override public Connection getConnection() { - String hostname = new LdapURL(ldapUrl).getEntry().getHostname(); return new SniAwareConnection(super.getConnection(), hostname); } Suggestion importance[1-10]: 5 __ Why: Reasonable optimization that also improves fail-fast behavior for malformed URLs. Impact is minor since LDAP connection creation is not a hot path.	Low
	Warn on ignored PKCS11 truststore path For PKCS11 truststores the truststore path setting is silently ignored, but no validation confirms the operator's intent. If a user configures a PKCS11 type with a path value expecting it to be honored, they get no feedback. Consider logging a warning when truststore is non-empty while type is PKCS11 so the misconfiguration is visible. src/main/java/org/opensearch/security/ssl/util/SSLRequestHelper.java [256-262] if ("PKCS11".equalsIgnoreCase(truststoreType)) { + if (truststore != null && !truststore.isEmpty()) { + log.warn("Ignoring truststore path '{}' because truststore type is PKCS11", truststore); + } ts.load(null, truststorePassword); } else { try (final var fin = new FileInputStream(env.configDir().resolve(truststore).toAbsolutePath().toString())) { ts.load(fin, truststorePassword); } } Suggestion importance[1-10]: 4 __ Why: Minor usability improvement to help operators diagnose misconfigurations, but not critical since PKCS11 configurations are relatively rare and this is a diagnostic enhancement rather than a functional fix.	Low
	Set initialization flag after attempt initialized is set to true before buildJwtParser runs, so any exception permanently disables OBO with no way to retry (e.g. after fixing the misconfiguration or resolving a transient keystore-read issue) without a node restart. Set initialized = true only after the initialization successfully completes, or in a finally block after logging, but only if you also intentionally want to prevent retries. If retries are undesirable, keep as-is but consider setting it after the try to at least allow success on the first run to be atomic. 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]: 3 __ Why: The suggested change is functionally equivalent to the original (both set initialized = true unconditionally). The suggestion doesn't actually enable retries as it claims - a finally block still sets it to true on failure.	Low
	Avoid sharing password char arrays between stores resolvePassword returns defaultPassword when no password is configured, and the callers for keystore and truststore share the same default instance through defaultStorePassword() invocations. However, since resolvePassword returns the same reference and downstream code may zero out the char array via password.chars() cleanup, the second use could see a wiped array. Ensure each call site receives a fresh StorePassword instance (already done by calling the method twice — verify that defaultStorePassword() is invoked separately per call, not cached). src/main/java/org/opensearch/security/ssl/config/SslCertificatesLoader.java [106-108] +private static StorePassword defaultStorePassword() { + return StorePassword.of(DEFAULT_STORE_PASSWORD.toCharArray()); +} - Suggestion importance[1-10]: 2 __ Why: The suggestion's improved_code is identical to the existing_code, providing no actual change. It only asks to verify existing behavior, which is low value.	Low
Possible issue	Avoid NPE for PKCS11 keystore load When storePath is null and type is PKCS11, extractStoreType(null, "PKCS11") will be called and its detectStoreType fallback attempts to open the (null) file, causing a NullPointerException. Skip the extraction/type-detection path for PKCS#11 since there is no file to detect from, and just use the given type directly. 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) ? PKCS11 : extractStoreType(storePath, type); Suggestion importance[1-10]: 6 __ Why: Valid concern: when storePath is null for PKCS11, calling extractStoreType(null, "PKCS11") could trigger detectStoreType(null) if type were null, but since type is PKCS11 here, extractStoreType returns early. Still, the defensive fix improves clarity and avoids potential NPE risk.	Low
	Guard against empty trust manager array Blindly picking tmf.getTrustManagers()[0] assumes at least one trust manager is returned and that the first one is an X509TrustManager. If the factory returns an empty array (possible with a misconfigured/empty truststore) this will throw ArrayIndexOutOfBoundsException with no useful context. Validate the array is non-empty and log/throw a clearer error, and consider passing all returned managers rather than just the first. src/main/java/org/opensearch/security/auth/ldap2/LDAPConnectionFactoryFactory.java [357-364] } else { try { TrustManagerFactory tmf = TrustManagerFactory.getInstance("PKIX"); tmf.init(this.sslConfig.getEffectiveTruststore()); - ldaptiveSslConfig.setTrustManagers(tmf.getTrustManagers()[0]); + final javax.net.ssl.TrustManager[] tms = tmf.getTrustManagers(); + if (tms == null || tms.length == 0) { + throw new IllegalStateException("PKIX TrustManagerFactory returned no trust managers for LDAPS"); + } + ldaptiveSslConfig.setTrustManagers(tms[0]); } catch (GeneralSecurityException e) { throw new IllegalStateException("Failed to initialize PKIX TrustManager for LDAPS", e); } } Suggestion importance[1-10]: 4 __ Why: In practice TrustManagerFactory.getTrustManagers() returns at least one manager after init() succeeds, so this is a defensive check with limited real-world impact, but it does improve error clarity.	Low
Security	Restrict visibility of mutable env supplier The mutable public static envSupplier field is not thread-safe and is intended only for tests, but exposes global mutable state to production code where a concurrent write could flip FIPS detection at runtime. Consider making it volatile and package-private (or using a dedicated test hook via reflection) to prevent accidental external mutation. src/main/java/org/opensearch/security/support/FipsMode.java [17-21] public final class FipsMode { - public static java.util.function.Supplier<String> envSupplier = () -> System.getenv("OPENSEARCH_FIPS_MODE"); + 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 publicly mutable static field for FIPS detection is a security concern. Making it package-private and volatile would better encapsulate the test hook while preventing unintended external mutation.	Low
	Zeroing is undermined by String returns Base64.getUrlEncoder().encodeToString(credentialBytes) produces a String that internally holds a copy of the password bytes (post-encoding), and new BasicAuthToken("Basic " + authToken) produces yet another. The careful zeroing of credentialBytes, passwordBytes, and passwordByteBuffer.array() is undermined because the plaintext password is still recoverable from the returned Base64 String in the heap. Either document this as inherent to returning the token as a String, or return the encoded bytes and let callers manage the lifetime. src/main/java/org/opensearch/security/user/UserService.java [328-340] -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); -credentialBytes = new byte[accountBytes.length + 1 + passwordBytes.length]; -System.arraycopy(accountBytes, 0, credentialBytes, 0, accountBytes.length); -credentialBytes[accountBytes.length] = ':'; -System.arraycopy(passwordBytes, 0, credentialBytes, accountBytes.length + 1, passwordBytes.length); -Arrays.fill(passwordBytes, (byte) 0); - +// NOTE: the returned BasicAuthToken contains a Base64-encoded copy of the password; +// the plaintext remains recoverable from the returned String until garbage-collected. authToken = Base64.getUrlEncoder().encodeToString(credentialBytes); return new BasicAuthToken("Basic " + authToken); Suggestion importance[1-10]: 4 __ Why: Valid observation that the Base64 String still holds the credential in memory, undermining some of the zeroing effort. However, the suggestion only proposes adding a comment rather than a functional fix, limiting its impact.	Low

Suggestions up to commit b5760ff

Category	Suggestion	Impact
General	Handle multi-URL LDAP configurations for SNI An LDAP URL may contain multiple space-separated URLs (ldaptive supports this). new URI(ldapUrl) will fail or return the wrong host for multi-URL configurations, silently disabling SNI. Parse only the first URL token to reliably extract the hostname. src/main/java/org/opensearch/security/auth/ldap/backend/LDAPAuthorizationBackend.java [212-223] 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(); if (StringUtils.isBlank(sniHostname)) { log.warn("Could not extract hostname from LDAP URL '{}'; proceeding without SNI configuration", ldapUrl); } } catch (URISyntaxException e) { log.warn("Malformed LDAP URL '{}'; proceeding without SNI configuration: {}", ldapUrl, e.getMessage()); } } Suggestion importance[1-10]: 7 __ Why: Ldaptive genuinely supports space-separated multi-URL configurations, and new URI() would fail on such strings, silently disabling SNI for a legitimate configuration. Meaningful correctness improvement.	Medium
	Allow retry after transient init failures Initialization is attempted exactly once, so any transient failure (e.g. a keystore file not yet mounted at plugin construction) permanently disables OBO authentication until the node restarts — even after the underlying issue is fixed. Consider allowing retry on subsequent invocations while still avoiding repeated log spam (e.g. log the error once but keep initialized=false on failure so a later call can retry, or expose a management API to reinitialize). 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); + // leave initialized=false so a subsequent request can retry after the config is fixed } } return jwtParser != null; } Suggestion importance[1-10]: 6 __ Why: Allowing retry on transient initialization failures (e.g. delayed keystore availability) improves resilience, avoiding a permanent disablement of OBO authentication until node restart. This is a useful robustness improvement, though log spam on repeated failures should be considered.	Low
	Harden PKCS#11 provider availability check **Security.getProviders(filter)</code...

[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

[reta]

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

[beanuwave]

Yes, all existing OBO connections will be interrupted for up to 10 minutes after the rollout. If this maintenance window is inconvenient, I can work out a migration plan to avoid the disruption.

[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 no-brainer, the one which really clicked for me for PCKS11, I think we could have cleaner implementation, but certainly - case by case.

[github-actions]

Persistent review updated to latest commit b5760ff

[github-actions]

Persistent review updated to latest commit 3ac26d1

[github-actions]

Persistent review updated to latest commit 5579028

[github-actions]

Persistent review updated to latest commit 03cdcc4

[DarshitChanpura]

@beanuwave Are you still working on this PR? If so could you please resolve conflicts?

[beanuwave]

@DarshitChanpura I think this PR is now mostly useful as a historical archive, so we can probably close it.

@reta To make the changes easier to review, I've split it into a stack of x5 PRs, each focused on a major topic. PRs #2, #3, and #4 are interchangeable and can be reviewed in any order.

The only unresolved comment is related to OBO token wire-format break, which I'll try to fix soon. Everything else in the stack can already be reviewed.

main
 └─ 1  fips-split/1-build-tooling
     └─ 2  fips-split/2-ssl-store-config
         └─ 3  fips-split/3-ldap-sni
             └─ 4  fips-split/4-obo-keystore
                 └─ 5  fips-split/5-fips-enforcement

• Add FIPS build mode and the *FipsTests/*FipsIT test-selection convention #6394
• Model key and trust store configurations as records and support PKCS#11 #6395
• ldap2: SNI-aware connections and a shared JNDI socket-factory classloader #6396
• OBO tokens: load the encryption key from a keystore and harden the crypto #6397
• Enforce FIPS-approved cryptography across hashing, TLS, tokens and the CLI #6398