Skip to content

Facilitate FIPS-compliant store resolution#6059

Merged
cwperks merged 9 commits intoopensearch-project:mainfrom
sternadsoftware:create_bcfks_test_stores
Apr 13, 2026
Merged

Facilitate FIPS-compliant store resolution#6059
cwperks merged 9 commits intoopensearch-project:mainfrom
sternadsoftware:create_bcfks_test_stores

Conversation

@beanuwave
Copy link
Copy Markdown
Contributor

@beanuwave beanuwave commented Apr 1, 2026

Description

Adds BCFKS keystore files alongside all existing JKS/PKCS12 test keystores in src/test/resources/ (excluding saml/ and on-behalf-token/), and updates test infrastructure to resolve the correct format at runtime.

RestHelper.java and AbstractSecurityUnitTest.java use resolveStorePath for all keystore/truststore loading. inferStoreType(resolvedPath) derives the KeyStore type from the resolved path extension, keeping type and format always consistent.

inferStoreType(resolvedPath) is a clone from core-projects test-framework https://github.com/opensearch-project/OpenSearch/blob/5b95e609b5ee1bcf5a5452a152a861bbe8971f31/test/framework/src/main/java/org/opensearch/test/KeyStoreUtils.java#L37-L48

Tests that explicitly exercise JDK store formats as part of their logic (SslSettingsManagerReloadListenerTest.java, JdkSslCertificatesLoaderTest.java) are out of scope and will be handled separately in #5866

Other changes:

  • DEFAULT_STORE_TYPE resolves to BCFKS in FIPS-mode, replacing hardcoded JKS and KeyStore.getDefaultType() across keystore creation
    sites
  • New keystores created during alias extraction (KeyStoreUtils) now inherit the source store's type, preventing BCFKS key material from being re-encrypted into PKCS12
  • PemKeyReader.detectStoreType sniffs file bytes to distinguish JKS/PKCS12/BCFKS
  • Fixed parseOtherName in Certificate and DefaultSecurityKeyStore to handle both JDK X.509 & BC X.509 encoding classes.
  • Extract SAN parsing logic from Certificate.subjectAlternativeNames() and
    DefaultSecurityKeyStore.getSubjectAlternativeNames() into a shared SanParser utility class.

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

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.

@beanuwave beanuwave force-pushed the create_bcfks_test_stores branch 2 times, most recently from 47a78ed to fb4339e Compare April 7, 2026 09:51
@codecov
Copy link
Copy Markdown

codecov bot commented Apr 7, 2026

Codecov Report

❌ Patch coverage is 82.43243% with 13 lines in your changes missing coverage. Please review.
✅ Project coverage is 74.65%. Comparing base (75b1483) to head (efac29d).
⚠️ Report is 15 commits behind head on main.

Files with missing lines Patch % Lines
.../org/opensearch/security/ssl/config/SanParser.java 80.76% 2 Missing and 3 partials ⚠️
.../org/opensearch/security/support/PemKeyReader.java 83.87% 2 Missing and 3 partials ⚠️
...ensearch/security/ssl/util/SSLConfigConstants.java 33.33% 1 Missing and 1 partial ⚠️
...ensearch/security/ssl/DefaultSecurityKeyStore.java 0.00% 1 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #6059      +/-   ##
==========================================
+ Coverage   74.02%   74.65%   +0.62%     
==========================================
  Files         441      447       +6     
  Lines       27461    28419     +958     
  Branches     4116     4331     +215     
==========================================
+ Hits        20329    21216     +887     
+ Misses       5196     5195       -1     
- Partials     1936     2008      +72     
Files with missing lines Coverage Δ
...rg/opensearch/security/ssl/config/Certificate.java 57.14% <100.00%> (-6.10%) ⬇️
.../opensearch/security/ssl/config/KeyStoreUtils.java 54.95% <100.00%> (+0.40%) ⬆️
...rch/security/ssl/config/SslCertificatesLoader.java 87.50% <100.00%> (+0.46%) ⬆️
...a/org/opensearch/security/tools/SecurityAdmin.java 48.99% <100.00%> (+0.23%) ⬆️
...ensearch/security/ssl/DefaultSecurityKeyStore.java 34.08% <0.00%> (+2.96%) ⬆️
...ensearch/security/ssl/util/SSLConfigConstants.java 88.57% <33.33%> (-5.18%) ⬇️
.../org/opensearch/security/ssl/config/SanParser.java 80.76% <80.76%> (ø)
.../org/opensearch/security/support/PemKeyReader.java 72.91% <83.87%> (+2.08%) ⬆️

... and 63 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

iigonin and others added 3 commits April 7, 2026 16:18
Signed-off-by: Iwan Igonin <iigonin@sternad.de>
Co-authored-by: Benny Goerzig <benny.goerzig@sap.com>
Co-authored-by: Karsten Schnitter <k.schnitter@sap.com>
Co-authored-by: Kai Sternad <k.sternad@sternad.de>
Signed-off-by: Iwan Igonin <iigonin@sternad.de>
Co-authored-by: Benny Goerzig <benny.goerzig@sap.com>
Co-authored-by: Karsten Schnitter <k.schnitter@sap.com>
Co-authored-by: Kai Sternad <k.sternad@sternad.de>
…er.getResource` for resource resolution and updated keystore file extensions to avoid double endings.

Signed-off-by: Iwan Igonin <iigonin@sternad.de>
Co-authored-by: Benny Goerzig <benny.goerzig@sap.com>
Co-authored-by: Karsten Schnitter <k.schnitter@sap.com>
Co-authored-by: Kai Sternad <k.sternad@sternad.de>
@beanuwave beanuwave force-pushed the create_bcfks_test_stores branch from fb4339e to d2c929f Compare April 7, 2026 14:19
@beanuwave beanuwave marked this pull request as ready for review April 7, 2026 14:19
@beanuwave beanuwave marked this pull request as draft April 8, 2026 08:08
…liant BCFKS stores.

Signed-off-by: Iwan Igonin <iigonin@sternad.de>
Co-authored-by: Benny Goerzig <benny.goerzig@sap.com>
Co-authored-by: Karsten Schnitter <k.schnitter@sap.com>
Co-authored-by: Kai Sternad <k.sternad@sternad.de>
@beanuwave beanuwave force-pushed the create_bcfks_test_stores branch from d2c929f to b625e85 Compare April 8, 2026 12:24
Signed-off-by: Iwan Igonin <iigonin@sternad.de>
Co-authored-by: Benny Goerzig <benny.goerzig@sap.com>
Co-authored-by: Karsten Schnitter <k.schnitter@sap.com>
Co-authored-by: Kai Sternad <k.sternad@sternad.de>
@beanuwave beanuwave force-pushed the create_bcfks_test_stores branch from 582a8a7 to 1cf3309 Compare April 8, 2026 16:03
Signed-off-by: Iwan Igonin <iigonin@sternad.de>
Co-authored-by: Benny Goerzig <benny.goerzig@sap.com>
Co-authored-by: Karsten Schnitter <k.schnitter@sap.com>
Co-authored-by: Kai Sternad <k.sternad@sternad.de>
@beanuwave beanuwave marked this pull request as ready for review April 9, 2026 11:47
Comment thread src/main/java/org/opensearch/security/support/PemKeyReader.java Outdated
Comment thread src/test/java/org/opensearch/security/test/helper/file/FileHelper.java Outdated
Comment thread src/test/java/org/opensearch/security/test/DynamicSecurityConfig.java Outdated
@reta
Copy link
Copy Markdown
Collaborator

reta commented Apr 10, 2026

Thanks @beanuwave , the change makes sense to me, a few minor comments

iigonin and others added 2 commits April 13, 2026 12:12
…e + path

Signed-off-by: Iwan Igonin <iigonin@sternad.de>
Co-authored-by: Benny Goerzig <benny.goerzig@sap.com>
Co-authored-by: Karsten Schnitter <k.schnitter@sap.com>
Co-authored-by: Kai Sternad <k.sternad@sternad.de>
…e `ClassLoader.getResource` logic + remove unused code

Signed-off-by: Iwan Igonin <iigonin@sternad.de>
Co-authored-by: Benny Goerzig <benny.goerzig@sap.com>
Co-authored-by: Karsten Schnitter <k.schnitter@sap.com>
Co-authored-by: Kai Sternad <k.sternad@sternad.de>
Signed-off-by: Iwan Igonin <iigonin@sternad.de>
Co-authored-by: Benny Goerzig <benny.goerzig@sap.com>
Co-authored-by: Karsten Schnitter <k.schnitter@sap.com>
Co-authored-by: Kai Sternad <k.sternad@sternad.de>
Comment thread src/main/java/org/opensearch/security/ssl/config/SanParser.java
Copy link
Copy Markdown
Member

@cwperks cwperks left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@beanuwave the changes LGTM, but can we add a README somewhere with instructions on how some of the trust stores were created? I believe we have a README outlining how some of the various test certs were generated.

Copy link
Copy Markdown
Member

@cwperks cwperks left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@beanuwave The change looks good to me and thanks for the thoroughness.

A couple comments and lmk what you think:

  1. I'd love to have it documented somewhere in the repo how the key stores are generated through a README or other markdown file. Since we're checking expiring certs into the repo, someone in the future will need to replace them and know how they were generated.
  2. I think we should use this opportunity to add a new CI check in the security repo to run the security plugin's integrationTest against a cluster running in FIPS enforced mode since the security repo has a lot of coverage on different actions used on a cluster.

@cwperks cwperks merged commit 1a51f0c into opensearch-project:main Apr 13, 2026
68 of 69 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants