Skip to content

Model key and trust store configurations as records and support PKCS#11 - #6395

Draft
beanuwave wants to merge 2 commits into
opensearch-project:mainfrom
sternadsoftware:fips-split/2-ssl-store-config
Draft

Model key and trust store configurations as records and support PKCS#11#6395
beanuwave wants to merge 2 commits into
opensearch-project:mainfrom
sternadsoftware:fips-split/2-ssl-store-config

Conversation

@beanuwave

Copy link
Copy Markdown
Contributor

Description

Category: Enhancement, Refactoring

Independent of FIPS: this adds PKCS#11 (HSM) support for node TLS and replaces
ad-hoc isPkcs11()-style branching with a type-driven model. Reviewable without
any FIPS context.

Key changes

  • Sealed records replace branching. Key stores and trust stores are modelled
    as pem / jdk / pkcs11 records, and PKCS#11 dispatch moves into the records
    themselves rather than being re-decided at each call site.
  • StorePassword wrapper. Store passwords are wrapped in a type whose
    toString() redacts the value, so a password cannot reach a log through an
    accidental interpolation.
  • A PKCS#11 store has no file. The key lives on the token, so the path becomes
    optional throughout: KeyStoreUtils loads such stores with a null stream, and
    error messages name the token instead of a non-existent file. PemKeyReader
    learns the PKCS11 store type and validates that a PKCS#11 provider is actually
    registered, rather than failing obscurely later.
  • PKCS#11 keys are signed via SunJSSE, not BCJSSE. A PKCS#11/HSM private key is
    non-exportable and BCJSSE cannot 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)).
  • Ignored settings now warn. Trust store settings that a PKCS#11 configuration
    disregards produce a warning naming the exact setting, instead of being silently
    dropped.

Reviewer notes

  • Reviewable as two commits: the records refactor (no capability change), then the
    PKCS#11 capability on top.
  • One test placeholder: PemSslCertificatesLoaderTest uses
    nextAlphanumeric(10) where the final branch uses
    PasswordValidator.FIPS_MIN_PASSWORD_LENGTH. That constant does not exist until
    PR 5 — this is deliberate, please don't "fix" it here.

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.

iigonin and others added 2 commits August 13, 2026 17:03
Introduces gradle/fips.gradle as the single place where FIPS mode is decided
and applied to the build's test surface: which test classes exist in each mode
and the JVM arguments test workers need to actually be in FIPS mode. Mode is
driven by the OPENSEARCH_FIPS_MODE environment variable, surfaced to production
code through the new FipsMode enum.

BC FIPS artifacts move to compileOnly in both modes (they are provided by
OpenSearch core), bctls-fips is added, and the securityadmin standalone bundles
now ship the BC FIPS jars in deps/.

Because java_test.security registers BouncyCastleFipsProvider in every test JVM
-- including non-FIPS runs -- any suite that touches JCA now leaves a
"BC FIPS Entropy Daemon" thread behind, which RandomizedRunner reports as a
leak. BCFipsEntropyDaemonFilter covers it; the framework's BouncyCastleThreadFilter
does not. It is applied to the suites that trip over it here, and reused by
later FIPS work.

No FIPS variant test classes exist yet, so this otherwise lands inert: the
default build is unchanged and fips.gradle currently selects nothing.

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>
Replaces the isPkcs11()-style branching in the SSL configuration layer with
sealed pem/jdk/pkcs11 records for both key stores and trust stores, and moves
PKCS#11 dispatch into those records. Store passwords are wrapped in a
StorePassword type so they are redacted in toString() rather than leaking into
logs.

A PKCS#11 store lives on the token rather than on disk, so the path becomes
optional throughout: KeyStoreUtils loads such stores with a null stream, and
error messages name the token instead of a file. PemKeyReader learns the
PKCS11 store type and validates that a PKCS#11 provider is actually registered.
Trust store settings that a PKCS#11 configuration ignores now produce a warning
instead of being silently dropped.

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>
@github-actions

Copy link
Copy Markdown
Contributor

PR Code Analyzer ❗

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

Hard block: Issues at High severity or above will block this PR from merging.

PathLineSeverityDescription
build.gradle600highNew dependency added: org.bouncycastle:bctls-fips (also appears at lines ~706 and ~812). Per mandatory supply-chain rule, any new dependency must be flagged regardless of apparent legitimacy — maintainers must verify the artifact hash and provenance.
src/main/java/org/opensearch/security/support/FipsMode.java17mediumFIPS mode detection uses a mutable public static field (envSupplier). Any code loaded in the same JVM can reassign it to () -> "false", silently disabling all FIPS enforcement. This is a security-critical control plane that should use a final, package-private, or at minimum private field with no public setter.
gradle/fips.gradle79mediumHardcoded truststore password 'changeit' is injected as a JVM system property on every test worker (-Djavax.net.ssl.trustStorePassword=changeit). Even though this is test infrastructure, embedding a plaintext password in a build script that is committed to source control and potentially shared across environments warrants review.
src/test/resources/fips-jvm-truststore.bcfks1lowA binary BCFKS truststore is added as a committed test resource. Binary blobs cannot be reviewed in a diff; maintainers should verify the file's provenance, confirm it contains only the expected demo/test CA certificates, and ensure it was not substituted with a truststore anchoring an attacker-controlled CA.
src/test/resources/fips_java_test.security51lowOCSP stapling and CRL distribution point checks are enabled (ocsp.enable=true, org.bouncycastle.x509.enableCRLDP=true, com.sun.net.ssl.checkRevocation=true). Applied to all test workers, this causes outbound network calls to external OCSP/CRL endpoints during certificate validation, which may expose CI infrastructure egress paths or cause test failures in air-gapped environments.

The table above displays the top 10 most important findings.

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


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.

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.

2 participants