Test adjustments for FIPS 140 in Java 8 #52211

jkakavas · 2020-02-11T14:58:07Z

This commit touches many of our tests but the changes are actually
few. Setting xpack.security.ssl.diagnose.trust to true
wraps SunJSSE TrustManager with our own DiagnosticTrustManager and
this is not allowed when SunJSSE is in FIPS mode. So when we
use SunJSSE in FIPS mode ( currently only when running our tests
with FIPS mode on for Java 8 runtime ), we need to make sure that
the Diagnostic TrustManager is not enabled. This change
ensures that whenever a new SSLService is to be created, we
explicitly pass xpack.security.ssl.diagnose.trust=false in the
settings used for the SSLService, when we run in FIPS mode in
Java 8.

This commit touches many of our tests but the changes are actually few. Setting xpack.security.ssl.diagnose.trust to true wraps SunJSSE TrustManager with our own DiagnosticTrustManager and this is not allowed when SunJSSE is in FIPS mode. So when we use SunJSSE in FIPS mode ( currently only when running our tests with FIPS mode on for Java 8 runtime ), we need to make sure that the Diagnostic TrustManager is not enabled. This change ensures that whenever a new SSLService is to be created, we explicitly pass xpack.security.ssl.diagnose.trust=false in the settings used for the SSLService, when we run in FIPS mode in Java 8.

elasticmachine · 2020-02-11T14:58:09Z

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

elasticmachine · 2020-02-11T14:58:11Z

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

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

…-7.x

jkakavas · 2020-02-12T05:13:09Z

...k/plugin/core/src/test/java/org/elasticsearch/xpack/core/LocalStateCompositeXPackPlugin.java

@@ -153,12 +154,17 @@ protected void setLicenseState(XPackLicenseState licenseState) {
                                               NamedXContentRegistry xContentRegistry, Environment environment,
                                               NodeEnvironment nodeEnvironment, NamedWriteableRegistry namedWriteableRegistry) {
        List<Object> components = new ArrayList<>();
+        // This is a hack, but the settings we add in #additionalSettings() are not added to the environment instance
+        // (in org.elasticsearch.node.Node) which is passed in `createComponents` of each of the plugins. So the environment
+        // we get here wouldn't have the additional setting. This is a known issue, and once it is resolved, the code here


I'm not sure how we track this and if we have an issue for it ( couldn't find one but I might have failed ) . There was some relevant discussion around this in #49667. Discussed it briefly with @rjernst on slack, and I didn't want to open an issue for this specific case here as it is only part of the problem.

That doesn't sound right to me. Doesn't environment contain the extra settings here? I think it's supposed to.

Yes environment should have the extra settings. My comment in slack was about the Settings passed into Plugin constructors.

jkakavas · 2020-02-12T06:43:10Z

09:34:37 Step 1/26 : FROM centos:7 AS builder
09:34:37 Get https://registry-1.docker.io/v2/: net/http: TLS handshake timeout
09:34:37 
09:34:38 
09:34:38 > Task :extractElasticsearchLinux7.7.0-SNAPSHOT
09:34:39 
09:34:39 > Task :modules:reindex:javadoc
09:34:39 Could not load entry 982b9b97231514a30536541e6a2aae42 from remote build cache
09:34:39 org.gradle.caching.BuildCacheException: Read timed out

jkakavas · 2020-02-12T06:44:40Z

@elasticmachine run elasticsearch-ci/default-distro

jkakavas · 2020-02-12T08:36:30Z

importing inFipsSunJsseJvm() and using it in classes that extend LuceneTestCase directly causes failures like https://gradle-enterprise.elastic.co/s/yphikcuckyz5e where the transport client doesn't connect to the node. Ι spent time on understanding why but need to move on for now so I replicated the check for (Boolean.parseBoolean(System.getProperty(FIPS_SYSPROP)) && JavaVersion.current().getVersion().get(0) == 8) in EnrichTransportClientIT and MigrateToolIT

...k/plugin/core/src/test/java/org/elasticsearch/xpack/core/LocalStateCompositeXPackPlugin.java

pugnascotia · 2020-02-14T12:42:09Z

...curity-migrate-tests/src/test/java/org/elasticsearch/xpack/security/MigrateToolTestCase.java

-
-        TransportClient client = new PreBuiltXPackTransportClient(clientSettings).addTransportAddresses(transportAddresses);
+                .put(SecurityField.USER_SETTING.getKey(), "transport_user:x-pack-test-password");
+        if (Boolean.parseBoolean(System.getProperty(FIPS_SYSPROP)) && JavaVersion.current().getVersion().get(0) == 8) {


Can't you just do this? I tried the import against your branch and it was OK.

Suggested change

if (Boolean.parseBoolean(System.getProperty(FIPS_SYSPROP)) && JavaVersion.current().getVersion().get(0) == 8) {

if (inFipsSunJsseJvm()) {

#52211 (comment)

pugnascotia · 2020-02-14T12:43:20Z

...transport-client-tests/src/test/java/org/elasticsearch/xpack/ESXPackSmokeClientTestCase.java

@@ -67,6 +70,9 @@ private static Client startClient(Path tempDir, TransportAddress... transportAdd
                .put("client.transport.ignore_cluster_name", true)
                .put("xpack.security.enabled", false)
                .put(Environment.PATH_HOME_SETTING.getKey(), tempDir);
+        if (Boolean.parseBoolean(System.getProperty(FIPS_SYSPROP)) && JavaVersion.current().getVersion().get(0) == 8) {


Same here?

Suggested change

if (Boolean.parseBoolean(System.getProperty(FIPS_SYSPROP)) && JavaVersion.current().getVersion().get(0) == 8) {

if (inFipsSunJsseJvm()) {

see #52211 (comment)

This needs a comment in code (and preferably a tracking issue) or someone may inadvertently try to make the same obvious change.

yes, that makes total sense - I'll make do

…-7.x

tvernum

LGTM, but I'd like to understand what's going on in createComponents.

tvernum · 2020-02-17T23:42:55Z

...k/plugin/core/src/test/java/org/elasticsearch/xpack/core/LocalStateCompositeXPackPlugin.java

@@ -153,12 +154,17 @@ protected void setLicenseState(XPackLicenseState licenseState) {
                                               NamedXContentRegistry xContentRegistry, Environment environment,
                                               NodeEnvironment nodeEnvironment, NamedWriteableRegistry namedWriteableRegistry) {
        List<Object> components = new ArrayList<>();
+        // This is a hack, but the settings we add in #additionalSettings() are not added to the environment instance
+        // (in org.elasticsearch.node.Node) which is passed in `createComponents` of each of the plugins. So the environment
+        // we get here wouldn't have the additional setting. This is a known issue, and once it is resolved, the code here


That doesn't sound right to me. Doesn't environment contain the extra settings here? I think it's supposed to.

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

jkakavas · 2020-02-18T06:02:37Z

That doesn't sound right to me. Doesn't environment contain the extra settings here? I think it's supposed to.

Happy to explain @tvernum , this tripped me also while troubleshooting the errors.

See

elasticsearch/server/src/main/java/org/elasticsearch/node/Node.java

Lines 271 to 273 in a304d9a

    
           protected Node( 
        
                   final Environment environment, Collection<Class<? extends Plugin>> classpathPlugins, boolean forbidPrivateIndexSettings) { 
        
               logger = LogManager.getLogger(Node.class);

We get the environment in the constructor of the Node

Then in

elasticsearch/server/src/main/java/org/elasticsearch/node/Node.java

Lines 315 to 330 in a304d9a

    
           this.pluginsService = new PluginsService(tmpSettings, environment.configFile(), environment.modulesFile(), 
        
               environment.pluginsFile(), classpathPlugins); 
        
           final Settings settings = pluginsService.updatedSettings(); 
        
           final Set<DiscoveryNodeRole> possibleRoles = Stream.concat( 
        
                   DiscoveryNodeRole.BUILT_IN_ROLES.stream(), 
        
                   pluginsService.filterPlugins(Plugin.class) 
        
                           .stream() 
        
                           .map(Plugin::getRoles) 
        
                           .flatMap(Set::stream)) 
        
                   .collect(Collectors.toSet()); 
        
           DiscoveryNode.setPossibleRoles(possibleRoles); 
        
           localNodeFactory = new LocalNodeFactory(settings, nodeEnvironment.nodeId()); 
        
           // create the environment based on the finalized (processed) view of the settings 
        
           // this is just to makes sure that people get the same settings, no matter where they ask them from 
        
           this.environment = new Environment(settings, environment.configFile());

we call updatedSettings() on the plugins, get their new settings and create a new Environment that we store in the class variable environment. We now have the local scope environment which is what is passed in the constructor and the this.environment which is the Environment with the potentially added settings from the plugins.

I would think that we then use this.environment for the rest of the method, and we do for the most part, except when we don't.

elasticsearch/server/src/main/java/org/elasticsearch/node/Node.java

Lines 454 to 471 in a304d9a

    
           final MetaDataCreateIndexService metaDataCreateIndexService = new MetaDataCreateIndexService( 
        
                   settings, 
        
                   clusterService, 
        
                   indicesService, 
        
                   clusterModule.getAllocationService(), 
        
                   aliasValidator, 
        
                   environment, 
        
                   settingsModule.getIndexScopedSettings(), 
        
                   threadPool, 
        
                   xContentRegistry, 
        
                   systemIndexDescriptors, 
        
                   forbidPrivateIndexSettings); 
        
           Collection<Object> pluginComponents = pluginsService.filterPlugins(Plugin.class).stream() 
        
               .flatMap(p -> p.createComponents(client, clusterService, threadPool, resourceWatcherService, 
        
                                                scriptModule.getScriptService(), xContentRegistry, environment, nodeEnvironment, 
        
                                                namedWriteableRegistry).stream()) 
        
               .collect(Collectors.toList());

We create the MetaDataCreateIndexService and we call createComponents() on each of the plugins using the local scope environment which doesn't have the additional settings. And this is why this

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

Line 142 in 26b9cf7

NamedXContentRegistry xContentRegistry, Environment environment,

doesn't have the additional settings

Now, it could be that this is a simple mistake because the method parameter shadows the class field, but @elastic/es-core-infra is aware of this and it made sense to attack this at in a different PR, this my comment in #52211 (comment).

HTH

jkakavas · 2020-02-20T16:41:02Z

@rjernst I think I addressed your comment, but pinging for an explicit LGTM just in case

rjernst

Sorry for causing confusion. This does seem like a case where we should use additionalSettings().

rjernst · 2020-02-20T21:02:44Z

...k/plugin/core/src/test/java/org/elasticsearch/xpack/core/LocalStateCompositeXPackPlugin.java

+        if (inFipsSunJsseJvm()) {
+            Settings additionalSettings = Settings.builder()
+                .put(existingEnvironment.settings())
+                .put(XPackSettings.DIAGNOSE_TRUST_EXCEPTIONS_SETTING.getKey(), false)


Can't this be returnned from additionalSettings()?

Apologies if I misunderstood @rjernst but did you read through #52211 (comment) ? I'll go and double check this again but this was supposed to show why additionalSettings doesn't solve this here

Sorry I missed that explanation. It looks to me like a bug. We should be passing the final environment everywhere once it is created.

I'll raise a PR to fix that then and change to use additionalSettings here

I've opened #52602 to cleanup the name to avoid this situation.

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.

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 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 added :Delivery/Build Build or test infrastructure >test-failure Triaged test failures from CI :Security/Security Security issues without another label v7.7.0 labels Feb 11, 2020

jkakavas requested review from tvernum and mark-vieira February 11, 2020 14:58

jkakavas commented Feb 11, 2020

View reviewed changes

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

Merge remote-tracking branch 'origin/7.x' into fips-tests-adjustments…

1b023f4

…-7.x

jkakavas commented Feb 12, 2020

View reviewed changes

Fix test

633fb3a

failing test

e8c278f

pugnascotia reviewed Feb 14, 2020

View reviewed changes

...k/plugin/core/src/test/java/org/elasticsearch/xpack/core/LocalStateCompositeXPackPlugin.java Show resolved Hide resolved

pugnascotia reviewed Feb 14, 2020

View reviewed changes

pugnascotia approved these changes Feb 14, 2020

View reviewed changes

jkakavas added 2 commits February 15, 2020 18:57

address feedback

31f796f

Merge remote-tracking branch 'origin/7.x' into fips-tests-adjustments…

dd52078

…-7.x

tvernum reviewed Feb 17, 2020

View reviewed changes

jkakavas requested review from rjernst and tvernum February 20, 2020 16:38

rjernst requested changes Feb 20, 2020

View reviewed changes

bpintea added the v7.8.0 label Mar 25, 2020

bpintea removed the v7.7.0 label Mar 25, 2020

rjernst added Team:Core/Infra Meta label for core/infra team Team:Security Meta label for security team labels May 4, 2020

pugnascotia added v7.8.1 v7.9.0 and removed v7.8.0 v7.8.1 labels May 6, 2020

jkakavas mentioned this pull request May 8, 2020

Test adjustments for FIPS 140 #56427

Closed

jkakavas mentioned this pull request May 11, 2020

Test adjustments for FIPS 140 #56526

Merged

jkakavas closed this May 11, 2020

jkakavas mentioned this pull request Jun 5, 2020

Test adjustments for FIPS 140 (#56526) #57752

Merged

jkakavas mentioned this pull request Jun 9, 2020

Test adjustments for FIPS 140 (#56526) #57879

Merged

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 in Java 8 #52211

Test adjustments for FIPS 140 in Java 8 #52211

jkakavas commented Feb 11, 2020

elasticmachine commented Feb 11, 2020

elasticmachine commented Feb 11, 2020

jkakavas Feb 12, 2020

tvernum Feb 17, 2020

rjernst Feb 20, 2020

jkakavas commented Feb 12, 2020

jkakavas commented Feb 12, 2020

jkakavas commented Feb 12, 2020 •

edited

Loading

pugnascotia Feb 14, 2020

jkakavas Feb 14, 2020

pugnascotia Feb 14, 2020

jkakavas Feb 14, 2020

rjernst Feb 14, 2020

jkakavas Feb 14, 2020

tvernum left a comment

tvernum Feb 17, 2020

jkakavas commented Feb 18, 2020 •

edited

Loading

jkakavas commented Feb 20, 2020

rjernst left a comment

rjernst Feb 20, 2020

jkakavas Feb 20, 2020 •

edited

Loading

rjernst Feb 20, 2020

jkakavas Feb 20, 2020

rjernst Feb 20, 2020

	if (Boolean.parseBoolean(System.getProperty(FIPS_SYSPROP)) && JavaVersion.current().getVersion().get(0) == 8) {
	if (inFipsSunJsseJvm()) {

Test adjustments for FIPS 140 in Java 8 #52211

Test adjustments for FIPS 140 in Java 8 #52211

Conversation

jkakavas commented Feb 11, 2020

elasticmachine commented Feb 11, 2020

elasticmachine commented Feb 11, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jkakavas commented Feb 12, 2020

jkakavas commented Feb 12, 2020

jkakavas commented Feb 12, 2020 • 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

Choose a reason for hiding this comment

tvernum left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jkakavas commented Feb 18, 2020 • edited Loading

jkakavas commented Feb 20, 2020

rjernst left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jkakavas Feb 20, 2020 • 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

jkakavas commented Feb 12, 2020 •

edited

Loading

jkakavas commented Feb 18, 2020 •

edited

Loading

jkakavas Feb 20, 2020 •

edited

Loading