Enable tests in FIPS 140 in JDK 11 #48378

jkakavas · 2019-10-23T10:06:10Z

This change enables us to run our test suites in JVMs configured in
FIPS 140 approved mode. It does so by:

Using BouncyCastle FIPS Cryptographic provider and BSJSSE in
FIPS mode. These are used as testRuntime dependencies for unit
tests and internal clusters, and copied (relevant jars)
explicitly to the lib directory for testclusters used in REST tests
Configuring any given runtime Java in FIPS mode using the system
properties java.security.properties and java.security.policy
with the == operator that overrides the default JVM properties
and policy.

Running the tests in FIPS 140 approved mode doesn't require an
additional configuration either in CI workers or locally and is
controlled by specifying -Dtests.fips.enabled=true

Closes: #37250
Supersedes: #41024

This change enables us to run our test suites in JVMs configured in FIPS 140 approved mode. It does so by: - Using BouncyCastle FIPS Cryptographic provider and BSJSSE in FIPS mode. These are used as testRuntime dependencies for unit tests and internal clusters, and copied (relevant jars) explicitly to the lib directory for testclusters used in REST tests - Configuring any given runtime Java in FIPS mode using the system properties `java.security.properties` and `java.security.policy` with the `==` operator that overrides the default JVM properties and policy. Running the tests in FIPS 140 approved mode doesn't require an additional configuration either in CI workers or locally and is controlled by specifying `-Dtests.fips.enabled=true`

elasticmachine · 2019-10-23T10:06:12Z

Pinging @elastic/es-core-infra (:Core/Infra/Build)

elasticmachine · 2019-10-23T10:06:14Z

Pinging @elastic/es-security (:Security/Security)

jkakavas · 2019-10-23T10:18:27Z

x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ssl/SSLService.java

-                X509ExtendedTrustManager loadedTrustManager = Optional.ofNullable(trustConfig.createTrustManager(env)).
-                    orElse(getEmptyTrustManager());
+                X509ExtendedKeyManager loadedKeyManager = keyConfig.createKeyManager(env);
+                X509ExtendedTrustManager loadedTrustManager = trustConfig.createTrustManager(env);


We'd never call getEmptyTrustManager as none of the implementations of createTrustManager returns null. As for an emtpyKeyManager, this could only happen for KeyConfig NONE - so I moved this there

jkakavas · 2019-10-23T10:19:15Z

...lugin/core/src/test/java/org/elasticsearch/xpack/core/ssl/SSLConfigurationReloaderTests.java

@@ -153,15 +153,16 @@ public void testReloadingKeyStore() throws Exception {
     * Tests the reloading of SSLContext when a PEM key and certificate are used.
     */
    public void testPEMKeyConfigReloading() throws Exception {
+        assumeFalse("This fails with BCJSSE with 'certificate does not verify with supplied key'", inFipsJvm());


I'll follow these up with appropriate issues once the troubleshooting is concluded

Can we link them to an issue or something. I don't love having temporary assumeFalse in tests without a way to trace it back to an action.

jkakavas · 2019-10-23T10:19:53Z

...in/security/src/test/java/org/elasticsearch/xpack/security/transport/nio/SSLDriverTests.java

@@ -38,6 +38,7 @@
    private final AtomicInteger openPages = new AtomicInteger(0);

    public void testPingPongAndClose() throws Exception {
+        assumeFalse("Fails in normalClose as receiveDriver.getOutboundBuffer().hasEncryptedBytesToFlush() is false", inFipsJvm());


I'll follow these up with appropriate issues once the troubleshooting is concluded

jkakavas · 2019-10-23T11:48:10Z

Fun fact: BCJSSE doesn't disallow the use of JKS keystores as SunJSSE did. SunJSSE also does not explicitly disallow JKS, it just mandates that the keystore instances used for calling init() on a KeyManagerFactory should come from the Provider. Since the BCFIPS provider does not offer a JKS keystore implementation, JKS keystores could not be used.
This "limitation" is now lifted because BCJSSE doesn't control this but the implementation of JKS (https://github.com/AdoptOpenJDK/openjdk-jdk11/blob/999dbd4192d0f819cb5224f26e9e7fa75ca6f289/src/java.base/share/classes/sun/security/provider/KeyProtector.java#L46) is not FIPS 140 approved so we should continue to not support JKS keystores in FIPS mode and thus shouldn't attempt to run tests that depend on using a JKS keystore in FIPS.

alpar-t · 2019-10-23T14:20:20Z

buildSrc/src/main/java/org/elasticsearch/gradle/testclusters/ElasticsearchNode.java

+        }
+    }
+
+    private void configureNodeForFips() {


We should do this externally, you can use testclusters.all {} to configure all clusters in a project, so combining that with allprojects {} would allow you to configure it across the board

I can't figure out how I can reference the file locations from within the BuildPlugin if I do this in testclusters.all {}. Any ideas ?

Locations of what files exactly?

The fips_java.security, fips_java.policy and cacerts.bcfks that I copy as extra config files

That's problematic indeed. getConfigDir is public, but we don't have a way to iterate over the nodes, or configure nodes individually.

Does this needs to be an absolute path ? Would a path relative to cwd work ? e.x. conf/fips_java.policy i think that's how this problem is solved elsewhere.

These paths are passed as values to system properties, not used in ES configuration so we either need an absolute path. Maybe we have a global reference to the ES_CONF_DIR that can be resolved in a system property? I'll look into it

Solution required adding ES_PATH_CONF to the substitutions we do when parsing jvm.properties . Can't think of a side-effect but I wanted to raise it for visibility when this is reviewed

buildSrc/src/main/groovy/org/elasticsearch/gradle/BuildPlugin.groovy

alpar-t · 2019-10-23T14:26:50Z

buildSrc/src/main/java/org/elasticsearch/gradle/testclusters/ElasticsearchNode.java

+     * Copies extra jars to the `/lib` directory.
+     * //TODO: Remove this when system modules are available
+     */
+    private void copyExtraJars() {


We prefer not to couple testclusters with random project configuration like an extraJars configuration.
Would be better to have extra jars work exactly as extra config files so these jars could be passed in externally.

A more generic way would be to add the possibility to add hooks right before the task starts.

I moved this outside as suggested, similar to how extraConfigurationFiles work

jkakavas · 2019-10-25T09:09:52Z

@atorok I have addressed your comments, please take another look

jkakavas · 2019-10-25T09:44:51Z

ci/1 failed due to #48440

@elasticmachine run elasticsearch-ci/1

alpar-t

LGTM. Thanks !

jkakavas · 2019-10-25T15:16:45Z

@elasticmachine run elasticsearch-ci/packaging-sample-matrix

rjernst · 2019-10-29T17:31:22Z

buildSrc/src/main/java/org/elasticsearch/gradle/testclusters/ElasticsearchNode.java

+            logToProcessStdout("Setting up " + extraJarFiles.size() + " additional jar dependencies");
+        }
+        extraJarFiles.forEach(from -> {
+            Path destination = getDistroDir().resolve("lib").resolve(from.getName());


I think this will leave trash behind. @atorok don't we setup a link for the lib dir? This would copy into the lib dir and be left behind after the test is run?

We don't set up links for directories, so this will be fine.
We create a matching directory structure, and create hard links for the files.

That does bring up a slightly different problem, if we were to replace one of the jars with this mechanism, that will be reflected for all invocations across all projects because the initial file could have changed. That would be really hard to spot and debug. We originally had code to set the links to read only so this would error out, maybe we should get that back ? It would work across the board, as opposed to just checking that the libs don't replace other libs.

is this something that should be taken into consideration in the context of this PR @atorok or are we ok to merge this and address it in a follow up ?

I think it's ok as a follow-up

jkakavas · 2019-10-30T11:03:32Z

@elasticmachine run elasticsearch-ci/1

jkakavas · 2019-11-12T13:03:55Z

ping @tvernum for a look at this one. Specifically the changes around SSLService and the empty{Trust,Key}Manager

tvernum · 2019-11-13T03:03:11Z

...lugin/core/src/test/java/org/elasticsearch/xpack/core/ssl/SSLConfigurationReloaderTests.java

@@ -153,15 +153,16 @@ public void testReloadingKeyStore() throws Exception {
     * Tests the reloading of SSLContext when a PEM key and certificate are used.
     */
    public void testPEMKeyConfigReloading() throws Exception {
+        assumeFalse("This fails with BCJSSE with 'certificate does not verify with supplied key'", inFipsJvm());


Can we link them to an issue or something. I don't love having temporary assumeFalse in tests without a way to trace it back to an action.

tvernum · 2019-11-13T03:04:33Z

...urity/src/test/java/org/elasticsearch/xpack/security/authc/ldap/LdapSessionFactoryTests.java

@@ -239,6 +239,8 @@ public void testGroupLookupBase() throws Exception {
     * (one failure, one success) depending on which file content is in place.
     */
    public void testSslTrustIsReloaded() throws Exception {
+        assumeFalse("NPE thrown in BCFIPS JSSE - addressed in https://github" +
+            ".com/bcgit/bc-java/commit/5aed687e17a3cd63f34373cafe92699b90076fb6#diff-8e5d8089bc0d504d93194a1e484d3950R179", inFipsJvm());


Can we re-wrap this string so the URL is on a single line?

This change enables us to run our test suites in JVMs configured in FIPS 140 approved mode. It does so by: - Using BouncyCastle FIPS Cryptographic provider and BSJSSE in FIPS mode. These are used as testRuntime dependencies for unit tests and internal clusters, and copied (relevant jars) explicitly to the lib directory for testclusters used in REST tests - Configuring any given runtime Java in FIPS mode with the bundled policy and security properties files, setting the system properties java.security.properties and java.security.policy with the == operator that overrides the default JVM properties and policy. Running the tests in FIPS 140 approved mode doesn't require an additional configuration either in CI workers or locally and is controlled by specifying -Dtests.fips.enabled=true Closes: elastic#37250 Supersedes: elastic#41024

The relevant handling part has been removed in infra, FIPS 140 testing needs to be enabled in the same manner as it happened for master and 7.x in elastic#48378 and elastic#49485 resolves: elastic#51924

The relevant handling part has been removed in infra, FIPS 140 testing needs to be enabled in the same manner as it happened for master and 7.x in #48378 and #49485 resolves: #51924

The relevant handling part has been removed in infra, FIPS 140 testing needs to be enabled in the same manner as it happened for master and 7.x in elastic#48378 and elastic#49485 resolves: elastic#51980

The relevant handling part has been removed in infra, FIPS 140 testing needs to be enabled in the same manner as it happened for master and 7.x in #48378 and #49485 resolves: #51980

jkakavas added 3 commits September 23, 2019 09:23

WIP

0b98221

Merge remote-tracking branch 'origin/master' into reenable-fips-tests

f5651a2

jkakavas added >test Issues or PRs that are addressing/adding tests :Delivery/Build Build or test infrastructure :Security/Security Security issues without another label v8.0.0 labels Oct 23, 2019

jkakavas requested review from tvernum and alpar-t October 23, 2019 10:06

jkakavas changed the title ~~Reenable fips tests~~ Enable tests in FIPS 140 in JDK 11 Oct 23, 2019

rename property

cc1c5fe

jkakavas commented Oct 23, 2019

View reviewed changes

jkakavas added 2 commits October 23, 2019 13:43

String.format

1a864a7

Merge remote-tracking branch 'origin/master' into reenable-fips-tests

74b0548

alpar-t reviewed Oct 23, 2019

View reviewed changes

buildSrc/src/main/groovy/org/elasticsearch/gradle/BuildPlugin.groovy Outdated Show resolved Hide resolved

alpar-t reviewed Oct 23, 2019

View reviewed changes

jkakavas added 2 commits October 24, 2019 11:26

Merge remote-tracking branch 'origin/master' into reenable-fips-tests

6b30693

decouple extraJars configuration from testclusters

4b17e9a

jkakavas requested review from mark-vieira and alpar-t October 24, 2019 09:13

jkakavas added 3 commits October 24, 2019 12:17

Unused import

85e3cf3

Move fips configuration outside of ElasticsearchNode

80a28ee

Merge remote-tracking branch 'origin/master' into reenable-fips-tests

3543a4b

unused import

4b48f7f

jkakavas added 2 commits October 25, 2019 13:59

Environment variable might not be set

403abd3

someone needs a break

a3db46d

alpar-t approved these changes Oct 25, 2019

View reviewed changes

rjernst reviewed Oct 29, 2019

View reviewed changes

jkakavas added 3 commits October 30, 2019 08:52

Merge remote-tracking branch 'origin/master' into reenable-fips-tests

b44534f

fix merge woes

66d1a99

complete list of missing classes from optional dependencies

e4f59f0

Merge remote-tracking branch 'origin/master' into reenable-fips-tests

d6ea75a

tvernum approved these changes Nov 13, 2019

View reviewed changes

jkakavas added 2 commits November 14, 2019 16:55

point to specific GH issue for muted tests

99aeb03

Merge remote-tracking branch 'origin/master' into reenable-fips-tests

f1cf5b9

jkakavas merged commit c7689ce into elastic:master Nov 15, 2019

jkakavas mentioned this pull request Nov 19, 2019

Adjustments for FIPS 140 testing #49319

Merged

jkakavas mentioned this pull request Nov 22, 2019

Enable tests in FIPS 140 in JDK 11 #49485

Merged

jkakavas mentioned this pull request Feb 5, 2020

Remove 8fips from our runtime matrix #51934

Merged

jkakavas mentioned this pull request Feb 6, 2020

Remove 8fips from our runtime matrix #51983

Merged

mark-vieira added the Team:Delivery Meta label for Delivery team label Nov 11, 2020

jakelandis added v8.0.0-alpha1 and removed v8.0.0 labels Jul 26, 2021

Enable tests in FIPS 140 in JDK 11 #48378

Enable tests in FIPS 140 in JDK 11 #48378

Uh oh!

Conversation

jkakavas commented Oct 23, 2019

Uh oh!

elasticmachine commented Oct 23, 2019

Uh oh!

elasticmachine commented Oct 23, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jkakavas commented Oct 23, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jkakavas commented Oct 25, 2019

Uh oh!

jkakavas commented Oct 25, 2019

Uh oh!

alpar-t left a comment

Choose a reason for hiding this comment

Uh oh!

jkakavas commented Oct 25, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alpar-t Oct 30, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jkakavas commented Oct 30, 2019

Uh oh!

jkakavas commented Nov 12, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

alpar-t Oct 30, 2019 •

edited

Loading