Skip to content

Update logic in FIPS bootstrap check - #21415

Merged
cwperks merged 4 commits into
opensearch-project:mainfrom
terryquigleysas:main
Apr 28, 2026
Merged

Update logic in FIPS bootstrap check#21415
cwperks merged 4 commits into
opensearch-project:mainfrom
terryquigleysas:main

Conversation

@terryquigleysas

Copy link
Copy Markdown
Contributor

Description

Currently, Bootstrap.java looks for the system prop (org.bouncycastle.fips.approved_only) to determine if FIPS is enforced at runtime. This system prop is specific to the bouncycastle library and implicitly set here in opensearch-env. This PR makes configuring FIPS more intentional by the cluster operator by looking for an OpenSearch env var instead of the bouncycastle system prop to determine if FIPS should be enforced at runtime.

Accompanies #21366

Related Issues

Resolves #19702

Related to:
#21366
#20738

Check List

  • Functionality includes testing.
  • API changes companion pull request created, if applicable.
  • Public documentation issue/PR created, if applicable.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

@terryquigleysas
terryquigleysas requested a review from a team as a code owner April 28, 2026 14:15
dependabot Bot and others added 2 commits April 28, 2026 15:16
…project#21291)

Bumps com.google.protobuf from 0.9.6 to 0.10.0.

---
updated-dependencies:
- dependency-name: com.google.protobuf
  dependency-version: 0.10.0
  dependency-type: direct:production
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: Sandesh Kumar <sandeshkr419@gmail.com>
Co-authored-by: Craig Perkins <cwperx@amazon.com>
Signed-off-by: Terry Quigley <terry.quigley@sas.com>
Signed-off-by: Terry Quigley <terry.quigley@sas.com>
@github-actions

github-actions Bot commented Apr 28, 2026

Copy link
Copy Markdown
Contributor

PR Code Analyzer ❗

AI-powered 'Code-Diff-Analyzer' found issues on commit 733f46d.

PathLineSeverityDescription
server/src/main/java/org/opensearch/bootstrap/Bootstrap.java200mediumFIPS mode activation via the standard BouncyCastle JVM system property 'org.bouncycastle.fips.approved_only' has been silently removed and replaced with a new OpenSearch-specific environment variable 'OPENSEARCH_FIPS_MODE'. Deployments that rely on the BouncyCastle property to trigger FIPS compliance enforcement (removal of non-compliant providers, FIPS trust store validation) will no longer have those safeguards applied, even while BouncyCastle itself operates in approved-only mode — creating a compliance gap. The change is not clearly documented as an intentional API migration and could weaken FIPS posture for existing deployments without operator awareness.

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.

terryquigleysas and others added 2 commits April 28, 2026 15:20
Signed-off-by: Terry Quigley <terry.quigley@sas.com>
@cwperks

cwperks commented Apr 28, 2026

Copy link
Copy Markdown
Member

@terryquigleysas thank you for this! I created #21366 yesterday, but missed this area.

Comment thread server/src/main/java/org/opensearch/bootstrap/Bootstrap.java
@terryquigleysas

Copy link
Copy Markdown
Contributor Author

@terryquigleysas thank you for this! I created #21366 yesterday, but missed this area.

Hey @cwperks , yes, I've only just seen your change from yesterday and was about to contact you with this accompanying, proposed PR but you beat me to it :-)

@cwperks cwperks added the skip-diff-analyzer Maintainer to skip code-diff-analyzer check, after reviewing issues in AI analysis. label Apr 28, 2026
@github-actions

Copy link
Copy Markdown
Contributor

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

🧪 No relevant tests
🔒 Security concerns

Sensitive information exposure:
Removing the BouncyCastle org.bouncycastle.fips.approved_only system property check without a migration path means that clusters previously relying on this property for FIPS enforcement will silently run in non-FIPS mode after upgrading, potentially exposing them to non-FIPS-compliant cryptographic operations without any warning or error.

✅ No TODO sections
🔀 No multiple PR themes
⚡ Recommended focus areas for review

Backward Compatibility

The old code checked the BouncyCastle system property org.bouncycastle.fips.approved_only as a fallback for FIPS mode detection. This PR removes that check entirely and replaces it with a new OPENSEARCH_FIPS_MODE env var. Existing deployments that relied on the BouncyCastle system property to trigger FIPS mode (without setting OPENSEARCH_CRYPTO_STANDARD) will silently lose FIPS enforcement after this change, which could be a security regression.

if ("FIPS-140-3".equals(cryptoStandard) || "true".equalsIgnoreCase(fipsMode)) {
    LogManager.getLogger(Bootstrap.class).info("running in FIPS-140-3 mode");
    SecurityProviderManager.removeNonCompliantFipsProviders();
    FipsTrustStoreValidator.validate();
}
Inconsistent Conditions

The condition now accepts both OPENSEARCH_CRYPTO_STANDARD=FIPS-140-3 and OPENSEARCH_FIPS_MODE=true as independent triggers for FIPS mode. There is no validation or warning when both are set with conflicting values (e.g., OPENSEARCH_CRYPTO_STANDARD set to something other than FIPS-140-3 while OPENSEARCH_FIPS_MODE=true). This could lead to confusing behavior.

if ("FIPS-140-3".equals(cryptoStandard) || "true".equalsIgnoreCase(fipsMode)) {
    LogManager.getLogger(Bootstrap.class).info("running in FIPS-140-3 mode");
    SecurityProviderManager.removeNonCompliantFipsProviders();
    FipsTrustStoreValidator.validate();
}

@github-actions

Copy link
Copy Markdown
Contributor

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
Possible issue
Preserve removed FIPS detection condition

The original code checked the system property org.bouncycastle.fips.approved_only to
determine FIPS mode, but the new code checks the environment variable
OPENSEARCH_FIPS_MODE instead. If the intent is to also support the BouncyCastle FIPS
approved-only mode detection, the old system property check should be preserved
alongside the new environment variable check to avoid a regression.

server/src/main/java/org/opensearch/bootstrap/Bootstrap.java [200-202]

 var fipsMode = System.getenv("OPENSEARCH_FIPS_MODE");
 
-if ("FIPS-140-3".equals(cryptoStandard) || "true".equalsIgnoreCase(fipsMode)) {
+if ("FIPS-140-3".equals(cryptoStandard) || "true".equalsIgnoreCase(fipsMode) || "true".equalsIgnoreCase(System.getProperty("org.bouncycastle.fips.approved_only"))) {
Suggestion importance[1-10]: 6

__

Why: The PR intentionally replaces the org.bouncycastle.fips.approved_only system property check with a new OPENSEARCH_FIPS_MODE environment variable. While the suggestion raises a valid concern about potential regression if existing deployments relied on the BouncyCastle property, this appears to be a deliberate design change rather than an oversight. The suggestion has moderate value as it could prevent a regression for users who previously relied on the system property.

Low

@github-actions

Copy link
Copy Markdown
Contributor

❌ Gradle check result for 733f46d: null

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

Copy link
Copy Markdown
Contributor

✅ Gradle check result for 733f46d: SUCCESS

@codecov

codecov Bot commented Apr 28, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 0% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 73.34%. Comparing base (0b0eae9) to head (733f46d).
⚠️ Report is 4 commits behind head on main.

Files with missing lines Patch % Lines
.../main/java/org/opensearch/bootstrap/Bootstrap.java 0.00% 2 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##               main   #21415      +/-   ##
============================================
- Coverage     73.40%   73.34%   -0.07%     
+ Complexity    74262    74243      -19     
============================================
  Files          5961     5961              
  Lines        337610   337611       +1     
  Branches      48704    48704              
============================================
- Hits         247833   247619     -214     
- Misses        69954    70173     +219     
+ Partials      19823    19819       -4     

☔ 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.

@cwperks
cwperks merged commit d16f189 into opensearch-project:main Apr 28, 2026
24 of 29 checks passed
imRishN pushed a commit to imRishN/OpenSearch that referenced this pull request May 8, 2026
* Update logic in FIPS bootstrap check

Signed-off-by: Terry Quigley <terry.quigley@sas.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

skip-diff-analyzer Maintainer to skip code-diff-analyzer check, after reviewing issues in AI analysis.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[BUG] Security provider should not be forcibly removed due to a Bouncy Castle property

2 participants