Reviewing files that changed from the base of the PR and between 0fc9331 and 11e3c7a.

• distribution/tools/keystore-cli/src/test/java/org/opensearch/bootstrap/BootstrapFipsTests.java (1 hunks)
• libs/ssl-config/src/test/java/org/opensearch/common/ssl/StoreTrustConfigTests.java (3 hunks)
• libs/ssl-config/src/test/resources/certs/pem-utils/README.md (1 hunks)
• modules/reindex/src/test/java/org/opensearch/index/reindex/ReindexRestClientSslFipsTests.java (1 hunks)
• modules/reindex/src/test/java/org/opensearch/index/reindex/ReindexRestClientSslTests.java (2 hunks)
• modules/transport-grpc/src/test/java/org/opensearch/transport/grpc/ssl/SecureSettingsHelpers.java (5 hunks)
• modules/transport-grpc/src/test/resources/README.md (1 hunks)
• plugins/discovery-gce/src/main/java/org/opensearch/cloud/gce/GceInstancesServiceImpl.java (3 hunks)

• modules/transport-grpc/src/test/resources/README.md

• GitHub Check: gradle-check
• GitHub Check: precommit (25, ubuntu-latest)
• GitHub Check: assemble (25, ubuntu-latest)
• GitHub Check: assemble (25, windows-latest)
• GitHub Check: assemble (21, ubuntu-latest)
• GitHub Check: assemble (25, ubuntu-24.04-arm)
• GitHub Check: assemble (21, windows-latest)
• GitHub Check: precommit (21, windows-2025, true)
• GitHub Check: assemble (21, ubuntu-24.04-arm)
• GitHub Check: precommit (25, ubuntu-24.04-arm)
• GitHub Check: precommit (25, windows-latest)
• GitHub Check: precommit (21, ubuntu-24.04-arm)
• GitHub Check: precommit (25, macos-15)
• GitHub Check: precommit (25, macos-15-intel)
• GitHub Check: precommit (21, windows-latest)
• GitHub Check: precommit (21, macos-15)
• GitHub Check: precommit (21, ubuntu-latest)
• GitHub Check: precommit (21, macos-15-intel)
• GitHub Check: detect-breaking-change
• GitHub Check: Analyze (java)

libs/ssl-config/src/test/java/org/opensearch/common/ssl/StoreTrustConfigTests.java (11)

56-60: Good extraction of shared keystore/password constants

Centralizing keystore types, passwords, and the default algorithm as protected constants is clean and makes subclassing (e.g., FIPS tests) straightforward. No issues seen here.

---

62-74: Constructors now consistently use shared keystore constants

Using P12_PASS/PKCS12 and JKS_PASS/JKS directly in these two happy‑path tests improves readability and removes string duplication. The behavior is unchanged and looks correct.

---

76-88: Randomizing keystore type for failure cases is reasonable

Switching to randomFrom(PKCS12, JKS) for the bad‑format and missing‑file tests is a nice way to exercise both keystore types over repeated runs, and the assertions (invalid format / file not found) are independent of the chosen type. This looks good.

---

91-103: Error‑path coverage for incorrect password and missing trust entries looks solid

These tests clearly differentiate:

• incorrect password for PKCS12, and
• missing trust entries in a JKS keystore,

and validate precise error messages. The use of shared constants keeps them easy to extend for FIPS subclasses. No changes suggested.

---

105-111: P12 missing‑entries case and shared assertion helper

Adding the P12 counterpart and delegating to assertMissingCertificateEntries ensures both keystore types see the same “missing entries” behavior. This is a good symmetry improvement.

---

133-142: Making assertCertificateChain protected aids reuse

Exposing this helper as protected is a clean way to let subclasses (e.g., FIPS‑specific tests) reuse the chain verification logic without duplication. Implementation remains straightforward and correct.

---

144-149: assertInvalidFileFormat matches expected failure contract

The helper asserts “cannot read”, “keystore”, and the absolute file path, which is a clear and stable contract for bad‑format errors. Elevating it to protected is appropriate for shared negative‑path tests.

---

151-157: assertFileNotFound behavior and messaging are well‑defined

Validating both the human‑readable message (“file does not exist”, “keystore”, path) and the absence of a cause keeps the error semantics precise. No issues here.

---

159-169: Password‑related helpers look consistent and precise

Both assertPasswordIsIncorrect and assertCannotCreatePasswordManager check for the key path plus specific message fragments. That gives good coverage for different password‑related failure modes without overfitting to full messages. Looks good as‑is.

---

171-176: assertNoCertificateEntries clearly targets empty truststores

The assertions on “does not contain any trusted certificate entries”, “truststore”, and the file path accurately describe the empty‑entries scenario and should be robust across keystore types.

---

178-182: assertMissingCertificateEntries complements the truststore helper well

This helper mirrors the “no certificate entries” message while being keyed on the provided path, which is useful for scenarios where the trust entries are missing from a given keystore. Implementation is consistent with the other helpers.

modules/reindex/src/test/java/org/opensearch/index/reindex/ReindexRestClientSslTests.java (2)

91-91: Secret length/comment looks appropriate for FIPS test coverage

The updated STRONG_PRIVATE_SECRET value and comment explicitly satisfying a ≥112‑bit strength requirement align with typical FIPS password/KEK constraints and should keep the PemKeyConfig / key‑passphrase paths usable in FIPS mode. No changes needed here.

---

170-183: Renamed verification‑disabled test is clear and FIPS‑agnostic

The rename to testClientWithVerificationDisabled plus running it unconditionally (no FIPS guard) keeps the behavior straightforward: when reindex.ssl.verification_mode=NONE, the client should still be able to establish a connection and receive 200. The assertions are simple and provider‑independent; this looks good.

distribution/tools/keystore-cli/src/test/java/org/opensearch/bootstrap/BootstrapFipsTests.java (1)

40-42: I encountered a repository cloning issue that prevents me from directly accessing the codebase. However, based on the review comment provided, I can still analyze the concern raised.

The review comment flags a legitimate security testing concern: using toString() for equality checks on SecureString objects may not validate the actual secret values if toString() is overridden to return a placeholder or length representation (a common security practice to avoid printing secrets).

Since I cannot access the actual SecureString implementation to definitively verify:

1. How toString() is implemented
2. Whether equals() is value-based
3. The actual test context and intent

I'll provide a rewritten review comment that reflects the inability to fully verify the concern while preserving its substance:

---

Verify the implementation of SecureString to determine if the equality check is testing the correct values.

The assertion compares toString() representations of SecureString objects:

assertEquals(seedAfterLoad.toString(), seed.toString());

This raises a concern: if SecureString.toString() is overridden to return a placeholder or length instead of the actual secret (a common security practice), this assertion may pass even when underlying values differ. Before proceeding:

• Check if SecureString provides a value-based equals() method; if so, use assertEquals(seedAfterLoad, seed) directly
• If SecureString exposes char/byte accessors (e.g., getChars()), use assertArrayEquals on those arrays instead
• Verify that the current assertion actually validates the seed value, not just the string representation

modules/transport-grpc/src/test/java/org/opensearch/transport/grpc/ssl/SecureSettingsHelpers.java (2)

121-123: Provider-specific SSLContext + Randomness.createSecure() is a solid FIPS-aware setup.

Using SSLContext.getInstance("TLS", provider) with provider switched between "BCJSSE" and "SunJSSE", plus Randomness.createSecure(), aligns well with the broader FIPS story (FIPS-aware RNG and JSSE provider). It also side-steps the earlier issue of explicitly listing non-FIPS protocols, since the provider enforces its own approved protocol set.

One operational caveat: this will now throw NoSuchProviderException if the expected provider isn’t registered when FipsMode.CHECK.isFipsEnabled() is true. That’s appropriate for a FIPS test, but it does mean CI/Jenkins FIPS jobs must ensure the matching JSSE provider is installed and enabled.

Please confirm that all FIPS JVM configurations used for the new -Pcrypto.standard=FIPS-140-3 runs have the expected JSSE provider registered under the name used here (provider), so these tests don’t fail with NoSuchProviderException.

---

82-108: I'll verify the concerns raised in this review comment by examining the call sites of these test helpers.
<function_calls>

#!/bin/bash

Check usages of the helpers to ensure keystorePath is passed without an extension.

rg -n "getTestKeyManagerFactory(" modules/transport-grpc/src/test/java
rg -n "getTestTrustManagerFactory(" modules/transport-grpc/src/test/java


</function_calls>

Now let me examine the actual call sites to see what arguments are being passed:
<function_calls>

#!/bin/bash

Find all test files that use these helpers

rg -l "getTestKeyManagerFactory|getTestTrustManagerFactory" modules/transport-grpc/src/test/java


</function_calls>

Let me get more context around these function calls:
<function_calls>

#!/bin/bash

Check usages with more context

rg -n -B2 -A2 "getTestKeyManagerFactory(" modules/transport-grpc/src/test/java
rg -n -B2 -A2 "getTestTrustManagerFactory(" modules/transport-grpc/src/test/java


</function_calls>

Let me also check what fileExtension is set to in the helper file:
<function_calls>

#!/bin/bash

Look at the SecureSettingsHelpers file to understand the fileExtension variable

cat -n modules/transport-grpc/src/test/java/org/opensearch/transport/grpc/ssl/SecureSettingsHelpers.java | head -30


</function_calls>

<function_calls>

#!/bin/bash

Get the full content of SecureSettingsHelpers.java

wc -l modules/transport-grpc/src/test/java/org/opensearch/transport/grpc/ssl/SecureSettingsHelpers.java


</function_calls>

<function_calls>

#!/bin/bash

Read the entire file

cat -n modules/transport-grpc/src/test/java/org/opensearch/transport/grpc/ssl/SecureSettingsHelpers.java


</function_calls>