Test adjustments for FIPS 140 #56526

jkakavas · 2020-05-11T16:17:18Z

This change aims to fix our setup in CI so that we can run 7.x in
FIPS 140 mode. The major issue that we have in 7.x and did not
have in master is that we can't use the diagnostic trust manager
in FIPS mode in Java 8 with SunJSSE in FIPS approved mode as it
explicitly disallows the wrapping of X509TrustManager.

Previous attempts like #56427 and #52211 focused on disabling the
setting in all of our tests when creating a Settings object or
on setting fips_mode.enabled accordingly (which implicitly disables
the diagnostic trust manager). The attempts weren't future proof
though as nothing would forbid someone to add new tests without
setting the necessary setting and forcing this would be very
inconvenient for any other case ( see
#56427 (comment)
for the full argumentation).

This change introduces a runtime check in SSLService that overrides
the configuration value of xpack.security.ssl.diagnose.trust and
disables the diagnostic trust manager when we are running in Java 8
and the SunJSSE provider is set in FIPS mode.

This change aims to fix our setup in CI so that we can run 7.x in FIPS 140 mode. The major issue that we have in 7.x and did not have in master is that we can't use the diagnostic trust manager in FIPS mode in Java 8 with SunJSSE in FIPS approved mode as it explicitly disallows the wrapping of X509TrustManager. Previous attempts like elastic#56427 and elastic#52211 focused on disabling the setting in all of our tests when creating a Settings object or on setting fips_mode.enabled accordingly (which implicitly disables the diagnostic trust manager). The attempts weren't future proof though as nothing would forbid someone to add new tests without setting the necessary setting and forcing this would be very inconvenient for any other case ( see elastic#56427 (comment) for the full argumentation). This change introduces a system property that effectively bypasses the configuration value of xpack.security.ssl.diagnose.trust and disables the diagnostic trust manager. We will then set this system property in our periodic CI jobs for Java 8.

elasticmachine · 2020-05-11T16:19:29Z

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

elasticmachine · 2020-05-11T16:19:31Z

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

rjernst

A few comments

plugins/discovery-ec2/build.gradle

rjernst · 2020-05-11T19:00:17Z

plugins/discovery-ec2/build.gradle

@@ -80,7 +102,11 @@ test {

  // this is needed to manipulate com.amazonaws.sdk.ec2MetadataServiceEndpointOverride system property
  // it is better rather disable security manager at all with `systemProperty 'tests.security.manager', 'false'`
-  systemProperty 'java.security.policy', "file://${buildDir}/tmp/java.policy"
+  if (BuildParams.inFipsJvm){
+    systemProperty 'java.security.policy', "=file://${buildDir}/tmp/java.policy"


What does the prefixed = do here? I wasn't able to find documentation on that special syntax. We should add a comment explaining for future readers.

I explain this here

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

Line 165 in 1fe5264

// Using the key==value format to override default JVM security settings and policy

, if you think we should copy the explanation in every use, happy to add it here too

rjernst · 2020-05-11T19:01:48Z

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

@@ -829,7 +829,12 @@ private static String sslContextAlgorithm(List<String> supportedProtocols) {
    }

    private boolean shouldEnableDiagnoseTrust() {
-        if (XPackSettings.FIPS_MODE_ENABLED.get(settings) && DIAGNOSE_TRUST_EXCEPTIONS_SETTING.exists(settings) == false ) {
+        // We disable the DiagnosticTrustManager in tests in Java 8 in FIPS 140 mode, as we're not allowed to wrap X509TrustManager
+        final boolean explicitlyDisable = Boolean.parseBoolean(System.getProperty("es.disable.diagnostic.trust.manager", "false"));


Why can't we pass in fips mode as disabled in our tests properties from BuildPlugin? I don't see the need for another property, and don't like this leaking into production code.

Because we need this to be set in all of our tests even when we ie build an SslService from an arbitrary settings object, and doing so in build plugin isn't enough, the previous two PRs contain much more detail, let me know if I should elaborate.

I didn't reuse the tests.fips.enabled setting do that this can be selectively set when running in java 8 only. Leaking into production code wasnt my first option either but the alternatives were brittle for the reasons I explain in the description of the previous PR

If java 8 is the problem, shouldn't we always disable the diagnostic trust manager for that version? Why does this only apply to tests?

Its java 8 with with SunJSSE in fips mode that is problematic, I explain this in detail in the previous two PRs, in retrospect I should have carried that discussion over to this PR.

We don't want to arbitrarily assume that there is no other way to run in fips mode in java 8, even if the problematic one is one of the most popular, so we didn't want to disable the diagnostic trust manager for all cases.

Maybe a short sync chat is worth to give you the whole context if you still have concerns about this ?

jkakavas · 2020-05-12T07:10:49Z

@elasticmachine update branch

ywangd

LGTM. A few comments.

ywangd · 2020-05-12T08:48:58Z

x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ssl/SSLServiceTests.java

-            testnodeStoreType = "jks";
-        } else {
+        // Randomise the keystore type (jks/PKCS#12) when possible
+        if (inFipsJvm()) {
            testnodeStore = getDataPath("/org/elasticsearch/xpack/security/transport/ssl/certs/simple/testnode.p12");
            testnodeStoreType = randomBoolean() ? "PKCS12" : null;


Is the null to test the store type can be auto-detected?

yes, it shouldn't make a difference as we should be able to auto detect. Arguably we don't need to test this here, but I believe it was originally there too and I didn't introduce this in this change

ywangd · 2020-05-12T08:56:43Z

...gin/security/src/test/java/org/elasticsearch/xpack/ssl/SSLReloadDuringStartupIntegTests.java

@@ -28,6 +29,11 @@
 @ClusterScope(transportClientRatio = 0)
 public class SSLReloadDuringStartupIntegTests extends SecurityIntegTestCase {

+    @BeforeClass
+    public static void skipInFips() {
+        assumeFalse("Can't use JKS keystores in FIPS JVM", inFipsJvm());


Is it possible to make this work if p12 file is used?

Maybe? But we don't support PKCS12 nor JKS stores in FIPS mode so there is not much value in testing reloading such a keystore.

You are right. Forgot about this again.

jkakavas · 2020-05-13T08:54:11Z

I revised the approach after chatting with @rjernst . We know attempt to check in runtime if the SunJSSE is in use and set in FIPS mode and if this is the case we don't enable the diagnostic trust manager

…vider in use

rjernst

LGTM

Now that elastic#56526 is merged, we do not need to explicitly disable the diagnostic trust manager for all of our test clusters - we do this dynamically in runtime if the combination of java version and JSSE provider dictates that.

Now that #56526 is merged, we do not need to explicitly disable the diagnostic trust manager for all of our test clusters - we do this dynamically in runtime if the combination of java version and JSSE provider dictates that.

This change aims to fix our setup in CI so that we can run 7.x in FIPS 140 mode. The major issue that we have in 7.x and did not have in master is that we can't use the diagnostic trust manager in FIPS mode in Java 8 with SunJSSE in FIPS approved mode as it explicitly disallows the wrapping of X509TrustManager. Previous attempts like elastic#56427 and elastic#52211 focused on disabling the setting in all of our tests when creating a Settings object or on setting fips_mode.enabled accordingly (which implicitly disables the diagnostic trust manager). The attempts weren't future proof though as nothing would forbid someone to add new tests without setting the necessary setting and forcing this would be very inconvenient for any other case ( see elastic#56427 (comment) for the full argumentation). This change introduces a runtime check in SSLService that overrides the configuration value of xpack.security.ssl.diagnose.trust and disables the diagnostic trust manager when we are running in Java 8 and the SunJSSE provider is set in FIPS mode.

This change aims to fix our setup in CI so that we can run 7.x in FIPS 140 mode. The major issue that we have in 7.x and did not have in master is that we can't use the diagnostic trust manager in FIPS mode in Java 8 with SunJSSE in FIPS approved mode as it explicitly disallows the wrapping of X509TrustManager. Previous attempts like #56427 and #52211 focused on disabling the setting in all of our tests when creating a Settings object or on setting fips_mode.enabled accordingly (which implicitly disables the diagnostic trust manager). The attempts weren't future proof though as nothing would forbid someone to add new tests without setting the necessary setting and forcing this would be very inconvenient for any other case ( see #56427 (comment) for the full argumentation). This change introduces a runtime check in SSLService that overrides the configuration value of xpack.security.ssl.diagnose.trust and disables the diagnostic trust manager when we are running in Java 8 and the SunJSSE provider is set in FIPS mode.

This change aims to fix our setup in CI so that we can run 7.x in FIPS 140 mode. The major issue that we have in 7.x and did not have in master is that we can't use the diagnostic trust manager in FIPS mode in Java 8 with SunJSSE in FIPS approved mode as it explicitly disallows the wrapping of X509TrustManager. Previous attempts like elastic#56427 and elastic#52211 focused on disabling the setting in all of our tests when creating a Settings object or on setting fips_mode.enabled accordingly (which implicitly disables the diagnostic trust manager). The attempts weren't future proof though as nothing would forbid someone to add new tests without setting the necessary setting and forcing this would be very inconvenient for any other case ( see This change introduces a runtime check in SSLService that overrides the configuration value of xpack.security.ssl.diagnose.trust and disables the diagnostic trust manager when we are running in Java 8 and the SunJSSE provider is set in FIPS mode.

This change aims to fix our setup in CI so that we can run 7.7 in FIPS 140 mode. The major issue that we have in 7.x and did not have in master is that we can't use the diagnostic trust manager in FIPS mode in Java 8 with SunJSSE in FIPS approved mode as it explicitly disallows the wrapping of X509TrustManager. Previous attempts like #56427 and #52211 focused on disabling the setting in all of our tests when creating a Settings object or on setting fips_mode.enabled accordingly (which implicitly disables the diagnostic trust manager). The attempts weren't future proof though as nothing would forbid someone to add new tests without setting the necessary setting and forcing this would be very inconvenient for any other case ( see This change introduces a runtime check in SSLService that overrides the configuration value of xpack.security.ssl.diagnose.trust and disables the diagnostic trust manager when we are running in Java 8 and the SunJSSE provider is set in FIPS mode.

jkakavas requested review from tvernum, ywangd and pugnascotia May 11, 2020 16:17

jkakavas mentioned this pull request May 11, 2020

Test adjustments for FIPS 140 #56427

Closed

jkakavas added :Delivery/Build Build or test infrastructure :Security/Security Security issues without another label >test Issues or PRs that are addressing/adding tests labels May 11, 2020

elasticmachine added the Team:Core/Infra Meta label for core/infra team label May 11, 2020

elasticmachine added the Team:Security Meta label for security team label May 11, 2020

rjernst reviewed May 11, 2020

View reviewed changes

jkakavas added 2 commits May 12, 2020 09:23

address feedback

c3c7af6

remove pasted text

58539f9

Merge branch '7.x' into fips-ci-7.x-take2

ba92aeb

ywangd approved these changes May 12, 2020

View reviewed changes

jkakavas requested a review from rjernst May 13, 2020 08:55

Remove system property and do a runtime detection of the security pro…

24a5306

…vider in use

rjernst approved these changes May 14, 2020

View reviewed changes

jkakavas mentioned this pull request May 15, 2020

Mutes failing test in FIPS 140 mode #56807

Merged

jkakavas merged commit 239ada1 into elastic:7.x May 15, 2020

jkakavas mentioned this pull request May 18, 2020

Stop disabling disgnostic trust manager in settings #56916

Merged

jkakavas mentioned this pull request Jun 5, 2020

Test adjustments for FIPS 140 (#56526) #57752

Merged

jkakavas mentioned this pull request Aug 6, 2020

Using inFipsSunJsseJvm() causes failures in ESXPackSmokeClientTestCase and MigrateToolTestCase #52391

Closed

mark-vieira added Team:Delivery Meta label for Delivery team and removed Team:Core/Infra Meta label for core/infra team labels Nov 11, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Test adjustments for FIPS 140 #56526

Test adjustments for FIPS 140 #56526

jkakavas commented May 11, 2020 •

edited

Loading

elasticmachine commented May 11, 2020

elasticmachine commented May 11, 2020

rjernst left a comment

rjernst May 11, 2020

jkakavas May 11, 2020

rjernst May 11, 2020

jkakavas May 11, 2020

rjernst May 12, 2020

jkakavas May 12, 2020

jkakavas commented May 12, 2020

ywangd left a comment

ywangd May 12, 2020

jkakavas May 12, 2020

ywangd May 12, 2020

jkakavas May 12, 2020

ywangd May 12, 2020

jkakavas commented May 13, 2020

rjernst left a comment

Test adjustments for FIPS 140 #56526

Test adjustments for FIPS 140 #56526

Conversation

jkakavas commented May 11, 2020 • edited Loading

elasticmachine commented May 11, 2020

elasticmachine commented May 11, 2020

rjernst left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jkakavas commented May 12, 2020

ywangd left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jkakavas commented May 13, 2020

rjernst left a comment

Choose a reason for hiding this comment

jkakavas commented May 11, 2020 •

edited

Loading