Add fips.mode configuration to control truststore check in FIPS mode for GCS repository

[Richard-coco]

Description

[GCS] Fix FIPS mode truststore handling without opt-out

This commit removes the previously proposed FIPS bypass switch and implements a compliant solution for default Google certificates in FIPS mode.

When running in FIPS mode (BCFIPS provider active) and no custom truststore is configured:

• The code now dynamically creates a FIPS-compliant BCFKS KeyStore
• Copies all trusted Google root certificates from the built-in Google trust store into the BCFKS container
• Maintains full FIPS compliance without any opt-out or security relaxation

The existing FIPS conversion scripts provided by OpenSearch are intended for user-managed certificates and JVM trust stores, not for the bundled Google certificates inside the GCS client library. Runtime conversion to BCFKS is the cleanest, most user-friendly approach to maintain out-of-the-box functionality.

This change ensures the GCS plugin works correctly in FIPS mode while adhering to all security requirements.

Related Issues

Fixes #20405

[github-actions]

PR Code Analyzer ❗

AI-powered 'Code-Diff-Analyzer' found issues on commit 5170a9d.

Path	Line	Severity	Description
plugins/repository-gcs/src/main/java/org/opensearch/repositories/gcs/GoogleCloudStorageService.java	235	medium	Removes a deliberate security enforcement: previously, FIPS mode without an explicitly configured custom truststore threw an IllegalStateException to force operators to supply audited certificates. The new code silently falls back to Google's built-in default certificate bundle, bypassing that requirement. While likely a usability improvement, it weakens the FIPS posture by allowing trust decisions to be made implicitly rather than via explicit operator configuration. Maintainers should confirm this relaxation is intentional and that GoogleUtils.getCertificateTrustStore() returns a sufficiently audited certificate set for FIPS environments.

The table above displays the top 10 most important findings.

Total: 1 | Critical: 0 | High: 0 | Medium: 1 | Low: 0

---

Pull Requests Author(s): Please update your Pull Request according to the report above.

Repository Maintainer(s): You can bypass diff analyzer by adding label skip-diff-analyzer after reviewing the changes carefully, then re-run failed actions. To re-enable the analyzer, remove the label, then re-run all actions.

---

⚠️ Note: The Code-Diff-Analyzer helps protect against potentially harmful code patterns. Please ensure you have thoroughly reviewed the changes beforehand.

Thanks.

[github-actions]

PR Reviewer Guide 🔍

(Review updated until commit 1012319)

Here are some key observations to aid the review process:

🧪 PR contains tests

🔒 No security concerns identified

✅ No TODO sections

🔀 No multiple PR themes

⚡ Recommended focus areas for review Incorrect KeyStore Type In createFipsCompliantGoogleTrustStore(), the code calls KeyStore.getInstance("BCFIPS", "BCFIPS"). The FIPS-compliant BouncyCastle keystore type is BCFKS, not BCFIPS. Using BCFIPS as the keystore type will likely throw a KeyStoreException at runtime. The provider argument may also be incorrect — it should typically be BCFIPS or BC depending on the setup, but the type should be BCFKS. KeyStore trustStore = KeyStore.getInstance("BCFIPS", "BCFIPS"); Null Password Risk trustStore.load(null, null) initializes the BCFKS keystore with a null password. Some BCFIPS-compliant KeyStore implementations require a non-null password for initialization and may throw an exception or behave unexpectedly when null is passed. This should be validated against the actual BCFIPS provider behavior. trustStore.load(null, null); Certificate Validity The method copies certificates from GoogleUtils.getCertificateTrustStore() into a BCFKS store. However, there is no validation that the certificates being copied are FIPS-approved (e.g., using approved algorithms). If any Google root certificate uses a non-FIPS-approved algorithm, inserting it into a BCFKS store under a strict BCFIPS provider may fail or silently skip entries. The behavior when setCertificateEntry encounters a non-compliant cert should be tested. while (aliases.hasMoreElements()) { String alias = aliases.nextElement(); if (googleDefaultStore.isCertificateEntry(alias)) { trustStore.setCertificateEntry(alias, googleDefaultStore.getCertificate(alias)); } } Missing BCFIPS Provider The test testFipsModeTruststoreConfiguration relies on the BCFIPS provider being registered (since the production code checks Security.getProvider("BCFIPS") != null). If the test environment does not have the BCFIPS provider registered, the test will exercise the non-FIPS code path and not actually test the new createFipsCompliantGoogleTrustStore() logic. There should be a check or setup that ensures the BCFIPS provider is active during this test. public void testFipsModeTruststoreConfiguration() throws Exception { final var service = new GoogleCloudStorageService(); final var statsCollector = new GoogleCloudStorageOperationsStats("bucket"); { // Scenario 1: No truststore configuration -> auto-create FIPS truststore final var secureSettings = new MockSecureSettings(); final var settings = Settings.builder().setSecureSettings(secureSettings).build(); service.refreshAndClearCache(GoogleCloudStorageClientSettings.load(settings)); // Should create client successfully without exception final var storage = service.client("default", "repo", statsCollector); assertNotNull(storage); }

[github-actions]

PR Code Suggestions ✨

Latest suggestions up to 1012319

Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	Fix incorrect FIPS keystore type name The KeyStore.getInstance call uses "BCFIPS" as both the keystore type and provider name, but the FIPS-compliant keystore type in Bouncy Castle FIPS is "BCFKS", not "BCFIPS". Using the wrong type will cause a KeyStoreException at runtime. The provider name "BCFIPS" is correct, but the type should be "BCFKS". plugins/repository-gcs/src/main/java/org/opensearch/repositories/gcs/GoogleCloudStorageService.java [307] -KeyStore trustStore = KeyStore.getInstance("BCFIPS", "BCFIPS"); +KeyStore trustStore = KeyStore.getInstance("BCFKS", "BCFIPS"); Suggestion importance[1-10]: 9 __ Why: This is a critical bug fix. The code uses "BCFIPS" as the keystore type, but the correct FIPS-compliant keystore type in Bouncy Castle FIPS is "BCFKS". Using the wrong type would cause a KeyStoreException at runtime, completely breaking FIPS mode functionality.	High
General	Validate trust store is not empty after copying If none of the entries in googleDefaultStore are certificate entries (e.g., all are key entries), the resulting trustStore will be empty, causing all TLS connections to fail silently without any warning. A check should be added after the loop to verify that at least one certificate was copied, and log a warning or throw an exception if the trust store is empty. plugins/repository-gcs/src/main/java/org/opensearch/repositories/gcs/GoogleCloudStorageService.java [315-321] // Copy all certificate entries into the FIPS-compliant BCFKS trust store +int certCount = 0; while (aliases.hasMoreElements()) { String alias = aliases.nextElement(); if (googleDefaultStore.isCertificateEntry(alias)) { trustStore.setCertificateEntry(alias, googleDefaultStore.getCertificate(alias)); + certCount++; } } +if (certCount == 0) { + throw new IllegalStateException( + "No certificate entries found in Google default trust store. FIPS-compliant trust store would be empty." + ); +} Suggestion importance[1-10]: 5 __ Why: This is a reasonable defensive check that would catch an edge case where googleDefaultStore contains no certificate entries, which would result in silent TLS failures. However, since GoogleUtils.getCertificateTrustStore() is a well-known Google library method that should always return valid certificates, this scenario is unlikely in practice.	Low

---

Previous suggestions

Suggestions up to commit 20cc8d4

Category	Suggestion	Impact
Possible issue	Fix incorrect FIPS KeyStore type name The KeyStore.getInstance("BCFIPS", "BCFIPS") call uses an incorrect KeyStore type for FIPS mode. The BCFIPS provider uses "BCFKS" as the KeyStore type, not "BCFIPS". Using the wrong type will cause a KeyStoreException at runtime. plugins/repository-gcs/src/main/java/org/opensearch/repositories/gcs/GoogleCloudStorageService.java [307] -KeyStore trustStore = KeyStore.getInstance("BCFIPS", "BCFIPS"); +KeyStore trustStore = KeyStore.getInstance("BCFKS", "BCFIPS"); Suggestion importance[1-10]: 8 __ Why: The KeyStore.getInstance("BCFIPS", "BCFIPS") call uses the wrong KeyStore type. The BCFIPS provider's KeyStore type is "BCFKS", not "BCFIPS". This would cause a KeyStoreException at runtime, making the FIPS mode non-functional.	Medium
General	Validate trust store is not empty after copying If none of the entries in googleDefaultStore are certificate entries (e.g., all are key entries), the returned trustStore will be empty, causing all TLS connections to fail silently. A check should be added to verify that at least one certificate was copied, and an exception should be thrown if the resulting trust store is empty. plugins/repository-gcs/src/main/java/org/opensearch/repositories/gcs/GoogleCloudStorageService.java [315-323] // Copy all certificate entries into the FIPS-compliant BCFKS trust store +int copiedCount = 0; while (aliases.hasMoreElements()) { String alias = aliases.nextElement(); if (googleDefaultStore.isCertificateEntry(alias)) { trustStore.setCertificateEntry(alias, googleDefaultStore.getCertificate(alias)); + copiedCount++; } +} + +if (copiedCount == 0) { + throw new IllegalStateException("No certificate entries found in Google default trust store; FIPS-compliant trust store would be empty."); } return trustStore; Suggestion importance[1-10]: 5 __ Why: Adding a check for an empty trustStore after copying is a reasonable defensive measure, but in practice GoogleUtils.getCertificateTrustStore() is expected to always contain certificate entries, making this an edge case with low probability of occurring.	Low

Suggestions up to commit f9f92c5

Category	Suggestion	Impact
Possible issue	Fix incorrect FIPS keystore type string The KeyStore.getInstance call uses "BCFIPS" as both the keystore type and provider name, but the FIPS-compliant keystore type from Bouncy Castle is "BCFKS", not "BCFIPS". Using the wrong type will likely cause a KeyStoreException at runtime. The provider name "BCFIPS" may also be incorrect; it is typically "BCFIPS" for the provider but the store type should be "BCFKS". plugins/repository-gcs/src/main/java/org/opensearch/repositories/gcs/GoogleCloudStorageService.java [307] -KeyStore trustStore = KeyStore.getInstance("BCFIPS", "BCFIPS"); +KeyStore trustStore = KeyStore.getInstance("BCFKS", "BCFIPS"); Suggestion importance[1-10]: 9 __ Why: The keystore type "BCFIPS" is incorrect; the FIPS-compliant Bouncy Castle keystore type is "BCFKS". Using "BCFIPS" as the type would cause a KeyStoreException at runtime, making this a critical bug fix.	High
General	Avoid null password for FIPS keystore initialization Initializing a BCFKS keystore with a null password may not be supported in strict FIPS mode, as BCFKS typically requires a non-null password for integrity protection. Passing null could cause a KeyStoreException or result in an unprotected store. Consider using an empty char array or a configurable password instead. plugins/repository-gcs/src/main/java/org/opensearch/repositories/gcs/GoogleCloudStorageService.java [309] -trustStore.load(null, null); +trustStore.load(null, new char[0]); Suggestion importance[1-10]: 5 __ Why: Passing null as the password to trustStore.load() may not be supported in strict FIPS mode with BCFKS, and using an empty char[] is a safer alternative, though this may not always cause a failure depending on the BCFIPS provider version.	Low

Suggestions up to commit 804ecb4

Category	Suggestion	Impact
Possible issue	Fix incorrect FIPS KeyStore type name The KeyStore type and provider name for BouncyCastle FIPS are incorrect. The correct KeyStore type is "BCFKS" (BouncyCastle FIPS KeyStore), not "BCFIPS". Using the wrong type will cause a KeyStoreException at runtime. plugins/repository-gcs/src/main/java/org/opensearch/repositories/gcs/GoogleCloudStorageService.java [307] -KeyStore trustStore = KeyStore.getInstance("BCFIPS", "BCFIPS"); +KeyStore trustStore = KeyStore.getInstance("BCFKS", "BCFIPS"); Suggestion importance[1-10]: 9 __ Why: The KeyStore type "BCFIPS" is incorrect; the proper BouncyCastle FIPS KeyStore type is "BCFKS". This is a real bug that would cause a KeyStoreException at runtime, making the FIPS code path completely non-functional.	High
General	Validate trust store is not empty after population If none of the entries in the Google default trust store are certificate entries (e.g., all are key entries), the resulting BCFKS trust store will be empty, silently causing all TLS connections to fail. Add a check after the loop to verify that at least one certificate was copied, and throw an informative exception if none were found. plugins/repository-gcs/src/main/java/org/opensearch/repositories/gcs/GoogleCloudStorageService.java [316-321] +int copiedCount = 0; while (aliases.hasMoreElements()) { String alias = aliases.nextElement(); if (googleDefaultStore.isCertificateEntry(alias)) { trustStore.setCertificateEntry(alias, googleDefaultStore.getCertificate(alias)); + copiedCount++; } } +if (copiedCount == 0) { + throw new IllegalStateException( + "No certificate entries found in Google default trust store. " + + "FIPS-compliant trust store could not be populated." + ); +} Suggestion importance[1-10]: 5 __ Why: Adding a check for an empty trust store after copying certificates is a reasonable defensive measure, but in practice GoogleUtils.getCertificateTrustStore() is expected to always contain certificate entries, making this an edge case with low probability of occurring.	Low

Suggestions up to commit 5170a9d

Category	Suggestion	Impact
Possible issue	Fix incorrect FIPS keystore type string The KeyStore.getInstance call uses "BCFIPS" as both the keystore type and provider, but the FIPS-compliant keystore type for BouncyCastle is "BCFKS", not "BCFIPS". Using the wrong type will likely cause a KeyStoreException at runtime. The provider name "BCFIPS" is correct, but the type should be "BCFKS". plugins/repository-gcs/src/main/java/org/opensearch/repositories/gcs/GoogleCloudStorageService.java [307] -KeyStore trustStore = KeyStore.getInstance("BCFIPS", "BCFIPS"); +KeyStore trustStore = KeyStore.getInstance("BCFKS", "BCFIPS"); Suggestion importance[1-10]: 9 __ Why: This is a critical bug fix - using "BCFIPS" as the keystore type instead of "BCFKS" would cause a KeyStoreException at runtime since "BCFKS" is the correct BouncyCastle FIPS keystore type. The provider name "BCFIPS" is correct, but the type must be "BCFKS".	High
General	Guard against empty FIPS trust store If none of the entries in googleDefaultStore are certificate entries (e.g., all are key entries), the resulting trustStore will be empty and no TLS connections will be trusted, causing silent failures. Consider logging a warning or throwing an exception if no certificates were copied into the FIPS trust store. plugins/repository-gcs/src/main/java/org/opensearch/repositories/gcs/GoogleCloudStorageService.java [316-321] +int certCount = 0; while (aliases.hasMoreElements()) { String alias = aliases.nextElement(); if (googleDefaultStore.isCertificateEntry(alias)) { trustStore.setCertificateEntry(alias, googleDefaultStore.getCertificate(alias)); + certCount++; } } +if (certCount == 0) { + throw new IllegalStateException( + "No certificate entries found in Google default trust store. FIPS-compliant trust store would be empty." + ); +} Suggestion importance[1-10]: 6 __ Why: Adding a check for an empty trust store is a reasonable defensive measure that would prevent silent TLS failures. However, in practice GoogleUtils.getCertificateTrustStore() is expected to always contain certificate entries, making this an edge case guard rather than a critical fix.	Low

Suggestions up to commit b4909fe

Category	Suggestion	Impact
Possible issue	Handle missing else branch to avoid NullPointerException When neither a custom truststore is configured nor FIPS mode enforcement triggers an error, the code falls through without setting certTrustStore to any value, which would result in a NullPointerException when certTrustStore is subsequently used. Ensure there is an else branch that loads the default JVM truststore (e.g., using KeyStore.getInstance(KeyStore.getDefaultType()) or the system default) so that non-FIPS, non-custom-truststore scenarios are handled correctly. plugins/repository-gcs/src/main/java/org/opensearch/repositories/gcs/GoogleCloudStorageService.java [233-237] } else if (clientSettings.isFipsMode() && Security.getProvider("BCFIPS") != null) { throw new IllegalStateException( "FIPS mode is active but no custom truststore is configured. " + "Please configure gcs.client.<client-name>.truststore.path and " + "gcs.client.<client-name>.truststore.secure_password settings." + ); +} else { + certTrustStore = null; // use JVM default trust store +} Suggestion importance[1-10]: 7 __ Why: When neither a custom truststore is configured nor FIPS mode is active, certTrustStore would be uninitialized, potentially causing a NullPointerException. Adding an else branch to handle the default case is important for correctness, though the suggestion sets it to null which may or may not be valid depending on how builder.trustCertificates() handles null values.	Medium
	Reconsider default value for FIPS mode setting The default value for fips.mode is true, meaning FIPS validation is enforced by default. However, the setting's purpose is described as a way to "bypass" the truststore check, implying users need to explicitly opt out. If a user upgrades without configuring this setting, they may unexpectedly hit the FIPS validation error even if they previously worked fine without a truststore. Consider defaulting to false so that FIPS enforcement is an explicit opt-in, or at minimum ensure the documentation and behavior are consistent. plugins/repository-gcs/src/main/java/org/opensearch/repositories/gcs/GoogleCloudStorageClientSettings.java [199] -key -> Setting.boolSetting(key, true, Setting.Property.NodeScope) +key -> Setting.boolSetting(key, false, Setting.Property.NodeScope) Suggestion importance[1-10]: 6 __ Why: The default value of true for fips.mode could cause unexpected failures for existing users who upgrade without configuring a truststore, since the FIPS validation would be enforced by default. Defaulting to false would make FIPS enforcement an explicit opt-in, which is safer for backward compatibility.	Low

[github-actions]

❌ Gradle check result for b4909fe: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

[Richard-coco]

/retest

[Richard-coco]

This change adds a fips.mode setting (default: true) to control strict FIPS truststore validation behavior.
The default value preserves existing security behavior.
The setting is only for operational flexibility in regulated environments.
This is a safe, backward-compatible change and requires a security review approval.

[reta]

This change adds a fips.mode setting (default: true) to control strict FIPS truststore validation behavior. The default value preserves existing security behavior. The setting is only for operational flexibility in regulated environments. This is a safe, backward-compatible change and requires a security review approval.

Thanks @Richard-coco , I am not entirely onboard with this change. The whole idea behind enabling FIPS is to make sure we could harden the distribution and all its components. Adding such ad-hoc opt-outs calls for issues, if the distribution or some of its components could not be run in the FIPS mode, well, the distribution has to turn it off. For operational flexibility, migrating to FIPS compatible trust store would be preferred (I believe we do provide some scripts already).

[Richard-coco]

@reta Thanks for your guidance and feedback.

I've removed the FIPS bypass switch and updated the code to use a proper FIPS-compliant BCFKS trust store for Google's default certificates.

The scripts you mentioned work great for user-managed certificates, but they don't apply to the built-in Google certificates that come bundled with the GCS library.
To keep things simple for users, this change creates the BCFKS trust store dynamically at runtime, which meets FIPS requirements while maintaining the out-of-the-box experience.

[github-actions]

Persistent review updated to latest commit 5170a9d

[github-actions]

❌ Gradle check result for 5170a9d: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

[github-actions]

❌ Gradle check result for 5170a9d: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

[github-actions]

❌ Gradle check result for 5170a9d: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

[github-actions]

❌ Gradle check result for 5170a9d: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

[github-actions]

✅ Gradle check result for 5170a9d: SUCCESS

[codecov]

Codecov Report

❌ Patch coverage is 0% with 12 lines in your changes missing coverage. Please review.
✅ Project coverage is 73.23%. Comparing base (d26ad17) to head (1012319).
⚠️ Report is 6 commits behind head on main.

Files with missing lines	Patch %	Lines
...ch/repositories/gcs/GoogleCloudStorageService.java	0.00%	12 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##               main   #21122      +/-   ##
============================================
- Coverage     73.35%   73.23%   -0.13%     
+ Complexity    73209    73156      -53     
============================================
  Files          5921     5932      +11     
  Lines        333798   333935     +137     
  Branches      48124    48127       +3     
============================================
- Hits         244862   244541     -321     
- Misses        69387    69877     +490     
+ Partials      19549    19517      -32     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[github-actions]

Persistent review updated to latest commit 804ecb4

[github-actions]

✅ Gradle check result for 804ecb4: SUCCESS

[reta]

Thanks @Richard-coco , would you be up adding test case to GoogleCloudStorageServiceFipsTests for this particular configuration? (no trust store, FIPS mode) Thank you.

[github-actions]

Persistent review updated to latest commit f9f92c5

[Richard-coco]

@reta Sure, the corresponding test case for no trust store in FIPS mode has been added and pushed.
Thank you for the review!

[github-actions]

❌ Gradle check result for f9f92c5: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

[github-actions]

Persistent review updated to latest commit 20cc8d4

[github-actions]

❌ Gradle check result for 20cc8d4: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

[Richard-coco]

@reta The gradle-check failed on the unrelated arrow-flight-rpc plugin test, which is a flaky test unrelated to my GCS plugin changes. Could you please re-run the CI for me? Thanks a lot!

[github-actions]

Persistent review updated to latest commit 1012319

[github-actions]

❕ Gradle check result for 1012319: UNSTABLE

Please review all flaky tests that succeeded after retry and create an issue if one does not already exist to track the flaky failure.