Support for FIPS compliance mode #14912

beanuwave · 2024-07-23T15:34:16Z

Description

This PR makes FIPS mode available through the OPENSEARCH_CRYPTO_STANDARD=FIPS-140-3 environmental parameter instead of the tests.fips.enabled setting. It provides FIPS 140-3 support by replacing all BC dependencies with BCFIPS dependencies and making FIPS approved-only mode configurable at launch. Running this mode restricts the BCFIPS provider to rely solely on FIPS-certified ciphers.

The fips.gradle build script is removed in order to support a single-build solution.
All BC dependencies are replaced by BCFIPS.
Fixed creation of password protected internal keystore for valuable settings at node start. The add-string command was made with additional --stdin option, which interferes with password input.
The Password Matcher inside Identity-Shiro that relies on BC to check if hashed passwords match with OpenBSDBCrypt is replaced by the password4j implementation.
Adds full support for the BCFKS format (*.bcfks) for key and trust stores, also making it the default.
KeyStore instantiation was added to forbidden-apis in favour of KeyStoreFactory.
Google's truststore is converted to the BCFKS format.
Makes the best guess of which store type is provided based on the filename extension.
Store types are strictly limited to JKS, PKCS12, PKCS11, and BCFKS.
Refactors PemUtils to parse private keys in formats EC, PKCS8, PKCS1, and DSA, with or without encryption, and with or without parameters.
dependency ':libs:opensearch-common' was added to rest-client build, to support strict keystore types. It's also the reason for JVM bump JAVA8 to JAVA11.
allow ingest-attachment plugin to run in FIPS mode, since BC dependencies find no use.
The java.security file is added to the build to distinguish between FIPS and non-FIPS environments.
The fips_java.security file is altered due to evolving security standards.
The security.policy file is altered to grant necessary security permissions.
Increased security standards in KeyTabs and algorithms for Kerberos.
SecureRandom gets instantiated in to different way, depending on if it runs with FIPS or not.
Uninstalls SunJCE provider from security providers list at runtime when FIPS mode is enabled.

Runtime limitations (known so far) that come with enabling FIPS mode:

Admins can continue to manage their systems without being impacted by this change. However, for those keen on FIPS compliance, the most common obstacle will likely be the requirement to set a stronger password for the internal keystore and also convert key and truststores to *.bcfks format.

Does not allow empty passwords for keystores or private keys (they need to be at least 112 bits in strength).
The ssl.verification_mode=NONE setting is not permitted.
JKS and PKCS12 key and trust store types are not supported at all.
The internal keystore cannot be auto-migrated from versions 1 or 2.
Azure Classic Discovery plugin -> deprecated.
SQL-CLI client.
HDFS plugin won't connect since it's using RC4 cipher for token authentication.

Reasons for refactoring `PemUtils`, which is used by the Reindex API in cases of migrating data from a remote cluster that is TLS protected:

Lack of support for evolving standards like PKCS#8.
Password-Based Key Derivation Functions such as PBKDF-OPENSSL are not supported in FIPS mode in favor of the PBKDF2 standard.
Java type safety.
It is generally a good idea to let ASN1 annotation parsing be done by external security libraries.

Related Issues

opensearch-project/security#3420

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.

github-actions · 2024-07-23T15:41:17Z

❌ Gradle check result for 6016d5d: 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 · 2024-07-24T08:51:58Z

❌ Gradle check result for 8e8ed47: 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 · 2024-07-24T08:54:08Z

❌ Gradle check result for 6016d5d: 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?

.../identity-shiro/src/main/java/org/opensearch/identity/shiro/realm/BCryptPasswordMatcher.java

dblock · 2024-07-24T15:11:32Z

Could use some help maybe from @cwperks or @peternied reviewing this, please.

buildSrc/src/main/java/org/opensearch/gradle/testclusters/OpenSearchNode.java

…ional tests. Signed-off-by: Iwan Igonin <[email protected]>

Signed-off-by: Iwan Igonin <[email protected]>

…Pattern Signed-off-by: Iwan Igonin <[email protected]>

Summery: - replace unsecure kerberos crypto algorithms - add 'java.security.KeyStore' to forbidden-apis - instantiate and use SecureRandom from BCFIPS library - exclude SunJCE from security providers list at runtime, when running in FIPS JVM - exclude Azure tests when running in FIPS JVM Signed-off-by: Iwan Igonin <[email protected]>

github-actions · 2025-01-10T12:05:35Z

❌ Gradle check result for 5698702: 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 · 2025-01-10T14:35:34Z

❌ Gradle check result for e98bfce: 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 · 2025-01-10T16:07:39Z

❕ Gradle check result for f775677: 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.

beanuwave · 2025-01-10T17:22:35Z

Thanks everyone for taking the time to review this PR. The last commit includes following changes:

Move KeyStoreFactory and KeyStoreType from the :libs:opensearch-common package to :libs:opensearch-ssl-config.
Always run keystore tests with a password if FIPS mode is on.
Shorten cryptoStandard != null && cryptoStandard.equals("FIPS-140-3") statements.
Revert changes to SecureRandomHolder.

Next I will focus on the two unresolved comments from above.

reta · 2025-01-13T13:54:56Z

build.gradle

+  // test with FIPS-140-3 enabled
+  plugins.withType(JavaPlugin).configureEach {
+    tasks.withType(Test).configureEach { testTask ->
+      if (System.getenv('OPENSEARCH_CRYPTO_STANDARD') == 'FIPS-140-3') {


Let's use BuildParams consistently please:

Suggested change

if (System.getenv('OPENSEARCH_CRYPTO_STANDARD') == 'FIPS-140-3') {

if (BuildParams.inFipsJvm == true) {

reta · 2025-01-13T14:19:07Z

buildSrc/src/main/java/org/opensearch/gradle/http/WaitForHttpResource.java

@@ -239,12 +240,17 @@ private KeyStore buildTrustStoreFromCA() throws GeneralSecurityException, IOExce
        return store;
    }

+    @SuppressForbiddenApi("runs exclusively in test-context without KeyStoreFactory on classpath.")


Suggested change

@SuppressForbiddenApi("runs exclusively in test-context without KeyStoreFactory on classpath.")

@SuppressForbidden

reta · 2025-01-13T14:20:44Z

buildSrc/src/main/java/org/opensearch/gradle/http/WaitForHttpResource.java

 import org.gradle.api.logging.Logger;
 import org.gradle.api.logging.Logging;
+import org.gradle.internal.impldep.com.jcraft.jsch.annotations.SuppressForbiddenApi;


Suggested change

import org.gradle.internal.impldep.com.jcraft.jsch.annotations.SuppressForbiddenApi;

import de.thetaphi.forbiddenapis.SuppressForbidden;

reta · 2025-01-13T14:30:51Z

buildSrc/build.gradle

@@ -123,6 +123,8 @@ dependencies {
  api 'org.jruby.joni:joni:2.2.1'
  api "com.fasterxml.jackson.core:jackson-databind:${props.getProperty('jackson_databind')}"
  api "org.ajoberstar.grgit:grgit-core:5.2.1"
+  api "org.bouncycastle:bc-fips:${props.getProperty('bouncycastle_jce')}"


I believe this is optional dependency and should not be included by default (this is build tooling which mostly every single plugin will depend upon:

Suggested change

api "org.bouncycastle:bc-fips:${props.getProperty('bouncycastle_jce')}"

if (System.getenv('OPENSEARCH_CRYPTO_STANDARD') == 'FIPS-140-3') {

api "org.bouncycastle:bc-fips:${props.getProperty('bouncycastle_jce')}"

}

(we sadly cannot use BuildParams here)

reta · 2025-01-13T15:05:23Z

buildSrc/src/main/java/org/opensearch/gradle/info/GlobalBuildInfoPlugin.java

@@ -112,6 +113,8 @@ public void apply(Project project) {
        BuildParams.init(params -> {
            // Initialize global build parameters
            boolean isInternal = GlobalBuildInfoPlugin.class.getResource("/buildSrc.marker") != null;
+            var cryptoStandard = System.getenv(OPENSEARCH_CRYPTO_STANDARD);


I think it would be useful to extract this into FipsBuild helper class (or similar), so we could consolidate OPENSEARCH_CRYPTO_STANDARD usage in 1 place:

public final class FipsBuildParams { private static final String fipsMode = System.getenv("OPENSEARCH_CRYPTO_STANDARD"); private FipsBuildParams() { } public static boolean isInFipsMode() { return "FIPS-140-3".equals(fipsMode); } public static String getFipsMode() { return fipsMode; } }

(we could use it here and https://github.com/opensearch-project/OpenSearch/pull/14912/files#r1913283753)

reta · 2025-01-13T15:06:53Z

buildSrc/src/main/java/org/opensearch/gradle/http/WaitForHttpResource.java

    private SSLContext createSslContext(KeyStore trustStore) throws GeneralSecurityException {
        checkForTrustEntry(trustStore);
        TrustManagerFactory tmf = TrustManagerFactory.getInstance(TrustManagerFactory.getDefaultAlgorithm());
        tmf.init(trustStore);
        SSLContext sslContext = SSLContext.getInstance("TLSv1.2");
-        sslContext.init(new KeyManager[0], tmf.getTrustManagers(), new SecureRandom());
+        sslContext.init(new KeyManager[0], tmf.getTrustManagers(), CryptoServicesRegistrar.getSecureRandom());


Since FIPS dependency has to be optional, we need to make its usage so as well:

Suggested change

sslContext.init(new KeyManager[0], tmf.getTrustManagers(), CryptoServicesRegistrar.getSecureRandom());

private static final class SecureRandomProvider {

private final static MethodHandle MH_BC_SECURE_RANDOM;

static {

MethodHandle mh = null;

if (FipsBuildParams.isInFipsMode()) {

try {

final Class<?> cryptoServicesRegistrarClass = Class.forName("org.bouncycastle.crypto.CryptoServicesRegistrar");

mh = MethodHandles.publicLookup()

.findStatic(cryptoServicesRegistrarClass, "getSecureRandom", MethodType.methodType(SecureRandom.class));

} catch (final Throwable ex) {

throw new SecurityException("Unable to find org.bouncycastle.crypto.CryptoServicesRegistrar class", ex);

}

}

MH_BC_SECURE_RANDOM = mh;

}

public static SecureRandom getSecureRandom() throws GeneralSecurityException {

if (MH_BC_SECURE_RANDOM == null) {

return new SecureRandom();

} else try {

return (SecureRandom) MH_BC_SECURE_RANDOM.invoke();

} catch (final Throwable ex) {

throw new GeneralSecurityException(ex);

}

}

}

...

sslContext.init(new KeyManager[0], tmf.getTrustManagers(), SecureRandomProvider.getSecureRandom());

reta · 2025-01-13T15:29:15Z

client/test/build.gradle

@@ -43,6 +43,12 @@ dependencies {
  api "com.carrotsearch.randomizedtesting:randomizedtesting-runner:${versions.randomizedrunner}"
  api "junit:junit:${versions.junit}"
  api "org.hamcrest:hamcrest:${versions.hamcrest}"
+  api "org.bouncycastle:bc-fips:${versions.bouncycastle_jce}"


These dependencies should be conditional on FIPS mode:

import org.opensearch.gradle.info.BuildParams if (BuildParams.inFipsJvm == true) { api "org.bouncycastle:bc-fips:${versions.bouncycastle_jce}" api "org.bouncycastle:bcutil-fips:${versions.bouncycastle_util}" }

reta · 2025-01-13T15:29:30Z

client/test/build.gradle

+  api "org.bouncycastle:bcutil-fips:${versions.bouncycastle_util}"
+}
+
+tasks.named("dependencyLicenses").configure {


if (BuildParams.inFipsJvm == true) { tasks.named("dependencyLicenses").configure { mapping from: /bc.*/, to: 'bouncycastle' } }

reta · 2025-01-13T15:32:56Z

client/rest/build.gradle

@@ -51,12 +51,15 @@ dependencies {
  api "commons-codec:commons-codec:${versions.commonscodec}"
  api "commons-logging:commons-logging:${versions.commonslogging}"
  api "org.slf4j:slf4j-api:${versions.slf4j}"
+  api "org.bouncycastle:bctls-fips:${versions.bouncycastle_tls}"


Suggested change

api "org.bouncycastle:bctls-fips:${versions.bouncycastle_tls}"

import org.opensearch.gradle.info.BuildParams

...

if (BuildParams.inFipsJvm == true) {

api "org.bouncycastle:bctls-fips:${versions.bouncycastle_tls}"

}

reta · 2025-01-13T15:33:48Z

client/rest/build.gradle

@@ -34,8 +34,8 @@ apply plugin: 'opensearch.build'
 apply plugin: 'opensearch.publish'

 java {
-  targetCompatibility = JavaVersion.VERSION_1_8
-  sourceCompatibility = JavaVersion.VERSION_1_8
+  targetCompatibility = JavaVersion.VERSION_11


Please revert this, Rest Client must use JDK 8 baseline

reta · 2025-01-13T15:39:31Z

@beanuwave thanks for working on this, I went through the large chunk of changes and there is one major issue that bothers me (or I do not understand something). The FIPS mode compliance is optional (at least at the moment) but its leaks everywhere as a mandatory prerequisite now:

through BC fips dependencies
through BC fips providers
through java security policy

We have to make it truly optional, otherwise it is becoming a major breaking change (in my opinion).

beanuwave changed the title ~~Draft to allow run in FIPS compliace mode~~ Draft to allow run in FIPS compliance mode Jul 24, 2024

beanuwave force-pushed the fips_compliance2 branch from 8e8ed47 to 6016d5d Compare July 24, 2024 08:46

beanuwave marked this pull request as ready for review July 24, 2024 10:02

beanuwave requested review from peternied, anasalkouz, andrross, ashking94, Bukhtawar, CEHENKLE, dblock, dbwiddis, gbbafna, kotwanikunal, mch2, msfroh, nknize, owaiskazi19, reta, Rishikesh1159, sachinpkale, saratvemulapalli, shwetathareja, sohami and VachaShah as code owners July 24, 2024 10:02

dancristiancecoi reviewed Jul 24, 2024

View reviewed changes

.../identity-shiro/src/main/java/org/opensearch/identity/shiro/realm/BCryptPasswordMatcher.java Outdated Show resolved Hide resolved

terryquigleysas reviewed Jul 24, 2024

View reviewed changes

buildSrc/src/main/java/org/opensearch/gradle/testclusters/OpenSearchNode.java Outdated Show resolved Hide resolved

iigonin added 8 commits January 10, 2025 12:57

ensure FIPS-mode can be enabled by fixing existent and creating addit…

f0ee7c4

…ional tests. Signed-off-by: Iwan Igonin <[email protected]>

fixing additional tests.

71da5d5

Signed-off-by: Iwan Igonin <[email protected]>

Configure FIPS mode through global ENV VAR instead of thread-based.

bca541b

Signed-off-by: Iwan Igonin <[email protected]>

write some additional unit tests.

f78254c

Signed-off-by: Iwan Igonin <[email protected]>

test SystemJvmOptions.java & replace FIPS-140-2 by FIPS-140-3

eea51be

Signed-off-by: Iwan Igonin <[email protected]>

categorize JKS keystore as untrusted

38a9e60

Signed-off-by: Iwan Igonin <[email protected]>

general exclusion of keystore binaries and certificates from fobidden…

8cda2ce

…Pattern Signed-off-by: Iwan Igonin <[email protected]>

beanuwave force-pushed the fips_compliance2 branch from f4856e4 to 5698702 Compare January 10, 2025 11:58

beanuwave force-pushed the fips_compliance2 branch from 5698702 to e98bfce Compare January 10, 2025 13:26

remove BC libs from 'libs:common'; revert SecureRandomHolder

f775677

beanuwave force-pushed the fips_compliance2 branch from e98bfce to f775677 Compare January 10, 2025 15:07

This was referenced Jan 11, 2025

[AUTOCUT] Gradle Check Flaky Test Report for VerifyVersionConstantsIT #14585

Open

[AUTOCUT] Gradle Check Flaky Test Report for RelocationIT #14309

Open

[AUTOCUT] Gradle Check Flaky Test Report for SimpleSearchIT #16851

Open

reta reviewed Jan 13, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support for FIPS compliance mode #14912

Support for FIPS compliance mode #14912

beanuwave commented Jul 23, 2024 •

edited

Loading

github-actions bot commented Jul 23, 2024

github-actions bot commented Jul 24, 2024

github-actions bot commented Jul 24, 2024

dblock commented Jul 24, 2024

github-actions bot commented Jan 10, 2025

github-actions bot commented Jan 10, 2025

github-actions bot commented Jan 10, 2025

beanuwave commented Jan 10, 2025

reta Jan 13, 2025

reta Jan 13, 2025 •

edited

Loading

reta Jan 13, 2025

reta Jan 13, 2025 •

edited

Loading

reta Jan 13, 2025

reta Jan 13, 2025

reta Jan 13, 2025

reta Jan 13, 2025 •

edited

Loading

reta Jan 13, 2025

reta Jan 13, 2025

reta Jan 13, 2025

reta Jan 13, 2025

reta commented Jan 13, 2025

	if (System.getenv('OPENSEARCH_CRYPTO_STANDARD') == 'FIPS-140-3') {
	if (BuildParams.inFipsJvm == true) {

	@SuppressForbiddenApi("runs exclusively in test-context without KeyStoreFactory on classpath.")
	@SuppressForbidden

	import org.gradle.internal.impldep.com.jcraft.jsch.annotations.SuppressForbiddenApi;
	import de.thetaphi.forbiddenapis.SuppressForbidden;

-        sslContext.init(new KeyManager[0], tmf.getTrustManagers(), CryptoServicesRegistrar.getSecureRandom());
+    private static final class SecureRandomProvider {
+        private final static MethodHandle MH_BC_SECURE_RANDOM;
+        static {
+            MethodHandle mh = null;
+            if (FipsBuildParams.isInFipsMode()) {
+                try {
+                    final Class<?> cryptoServicesRegistrarClass = Class.forName("org.bouncycastle.crypto.CryptoServicesRegistrar");
+                    mh = MethodHandles.publicLookup()
+                        .findStatic(cryptoServicesRegistrarClass, "getSecureRandom", MethodType.methodType(SecureRandom.class));
+                } catch (final Throwable ex) {
+                    throw new SecurityException("Unable to find org.bouncycastle.crypto.CryptoServicesRegistrar class", ex);
+                }
+            }
+            MH_BC_SECURE_RANDOM = mh;
+        }
+        public static SecureRandom getSecureRandom() throws GeneralSecurityException {
+            if (MH_BC_SECURE_RANDOM == null) {
+                return new SecureRandom();
+            } else try {
+                return (SecureRandom) MH_BC_SECURE_RANDOM.invoke();
+            } catch (final Throwable ex) {
+                throw new GeneralSecurityException(ex);
+            }
+        }
+    }
+    ...
+    sslContext.init(new KeyManager[0], tmf.getTrustManagers(), SecureRandomProvider.getSecureRandom());

Support for FIPS compliance mode #14912

Are you sure you want to change the base?

Support for FIPS compliance mode #14912

Conversation

beanuwave commented Jul 23, 2024 • edited Loading

Description

Runtime limitations (known so far) that come with enabling FIPS mode:

Reasons for refactoring PemUtils, which is used by the Reindex API in cases of migrating data from a remote cluster that is TLS protected:

Related Issues

Check List

github-actions bot commented Jul 23, 2024

github-actions bot commented Jul 24, 2024

github-actions bot commented Jul 24, 2024

dblock commented Jul 24, 2024

github-actions bot commented Jan 10, 2025

github-actions bot commented Jan 10, 2025

github-actions bot commented Jan 10, 2025

beanuwave commented Jan 10, 2025

Choose a reason for hiding this comment

reta Jan 13, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

reta Jan 13, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

reta Jan 13, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

reta commented Jan 13, 2025

beanuwave commented Jul 23, 2024 •

edited

Loading

Reasons for refactoring `PemUtils`, which is used by the Reindex API in cases of migrating data from a remote cluster that is TLS protected:

reta Jan 13, 2025 •

edited

Loading

reta Jan 13, 2025 •

edited

Loading

reta Jan 13, 2025 •

edited

Loading