Allow Integ Tests to run in a FIPS-140 JVM #31989

jkakavas · 2018-07-12T08:30:16Z

Necessary changes for running Integration tests in a FIPS-140 JVM.
The main drive for change is that JKS keystores cannot be used for
KeyManagerFactory#init() so we should use the relevant key and
certificate instead for creating all relevant SSLContexts.

Mute :x-pack:qa:sql:security:ssl:integTest entirely as it
cannot run in FIPS 140 JVM until the SQL CLI supports key/cert.
(It currently supports only Keystores for the key configuration
and it's not trivial to support reading private keys without
depending either on BouncyCastle (Non FIPS version) or X-Pack
(PEMUtils)
Set default JVM keystore/truststore password in top level build
script for all integTest tasks in a FIPS 140 JVM as that JVM will
be using a BCFKS keystore which will be password protected.
Changed top level x-pack build script to use keys and certificates
for trust/key material when spinning up clusters for IT

Depends on #31666

- Mute :x-pack:qa:sql:security:ssl:integTest as it cannot run in FIPS 140 JVM until the SQL CLI supports key/cert. - Set default JVM keystore/truststore password in top level build script for all integTest tasks in a FIPS 140 JVM - Changed top level x-pack build script to use keys and certificates for trust/key material when spinning up clusters for IT

elasticmachine · 2018-07-12T08:30:18Z

Pinging @elastic/es-security

jkakavas · 2018-07-12T08:31:46Z

Pinging @elastic/es-core-infra for a review of the build related changes.

alpar-t · 2018-07-12T10:45:55Z

x-pack/plugin/build.gradle

+// User cert and key PEM files instead of a JKS Keystore for the cluster's trust material so that
+// it can run in a FIPS 140 JVM
+task copyKeyCerts(type: Copy) {
+  from('./core/src/test/resources/org/elasticsearch/xpack/security/transport/ssl/certs/simple/') {


Creating coupling between projects like this is not a good idea in Gradle.
We should at least reference this as somethign like project(':x-pack:plugin:core').file('src/test/resources/org/elasticsearch/xpack/security/transport/ssl/certs/simple/') to make this more explicit.
The clean solution would be to have a small Gradle project with these resources and then

dependencies { testRuntime project(':test:fips:certs') }

Where :test:fips:certs would just have these files that need to be on the CP.
Gradle will take care of dependencies and adding it to the cp automatically and it will play nicer with --parallel as well. Would also mean that the copy task doesn't have to be repeated.

In this case, would it be feasible to have the build generate these files ?
Maybe even with random passwords and such ? Rest integration tests don't run in the IDE anyhow.
Might prevent some automated checks for committed private keys from tripping on our repo - even if it's only for testing.

In this case, would it be feasible to have the build generate these files ?

Unfortunately not. keytool only generates Private Keys and stores them in a Keystore, it can't output them as a PEM file. So we can only generate a PKCS12 or a JKS keystore containing the private key and associated certificate. keytool also doesn't support exporting private keys from a keystore. The only solution would be to use an external tool like openssl to either export the key and certificate form a PKCS12 store or generate the key , certificate in the first place. We can't be sure that openssl will be available on all platforms though ( and I don't think Windows have a utility with corresponding functionality )

The clean solution would be to have a small Gradle project with these resources and then

Not all projects need all files ( test{node,client}.{jks,pem,crt} ) but it wouldn't hurt to add them. No issue going with this suggestion

How about doing in Java what openssl would do ? Maybe using BouncyCastle or some other lib ? Would it be more effort than what it's worth ?

Nope, can't do that. We explicitly removed BC dependency recently on purpose ( see #30358 on why ).
I guess I can look into the possibility of exporting the keys in Java (without external deps) but in my mind this is much more complicated and I can't really see the benefits.
Granted that the files are referenced correctly or even replicated in this Gradle project so that we don't need to reference them from x-pack:plugin:core, or even better I go down the path of the new gradle project with those files as resources, do you have any specific concerns?

In this case, BouncyCastle would be a dependency for the build, which is a bit different, I don't think we'll be able to run Gradle itself on a FIPS JVM, or if we even want to. That being said I suggested it because it seemed the cleanest and not too much work - but admittedly I'm missing a lot of context on this so I could be totally wrong. I don't have a strong preference for this, separate project or referencing trough project is also fine for me ( in this order of preference ).

alpar-t · 2018-07-12T10:48:16Z

x-pack/plugin/build.gradle

-    }
-    if (nodeKeystore.exists()) {
-      delete nodeKeystore
+File nodeKey = new File(keystoreDir, 'testnode.pem')


This is usually written file("$keystoreDir/testnode.pem") in Gradle. It doesn't make a difference in this case, but sometimes does because file() resolves to projectDir but new File() resolves to working dir which can be the directory where the daemon was started. The / does work on Windows.

Thanks, I was merely following existing conventions here. I'll change this

alpar-t · 2018-07-12T10:52:39Z

x-pack/qa/sql/security/ssl/build.gradle

+// Do not attempt to form a cluster in a FIPS JVM, as doing so with a JKS keystore will fail.
+// TODO Revisit this when SQL CLI client can handle key/certificate instead of only Keystores.
+tasks.matching({ it.name == "integTestCluster#init" }).all {
+  onlyIf {


Might be worth reducing the duplication here

Closure notRunningFips = { { Boolean.parseBoolean(BuildPlugin.runJavascript(project, project.runtimeJavaHome, 'print(java.security.Security.getProviders()[0].name.toLowerCase().contains("fips"));')) == false }

and then

onlyIf notRunningFips

jkakavas · 2018-07-12T11:51:47Z

CI run failed due to the intermittent issue that #31973 addresses, hopefully it will be merged soon.

- Attempt to limit arbitrary project coupling by bringing necessary certificate/key/keystore files for IT in a small separate project and - Introduce Closre checking if we run in a FIPS JVM

jaymode · 2018-07-13T15:54:16Z

x-pack/qa/smoke-test-plugins-ssl/build.gradle

@@ -29,132 +30,13 @@ project.sourceSets.test.output.dir(outputDir, builtBy: copyXPackPluginProps)
 // needed to be consistent with ssl host checking
 Object san = new SanEvaluator()


The certs that are pre-created, what do they have for a SAN value? We used to dynamically generate this on the fly so that we could still use hostname verification

We used to dynamically generate this on the fly so that we could still use hostname verification

Yes, I know. It's a little more complicated now - we touched upon generation and potential issues with Alpar also yesterday: #31989 (comment)

The certs that are pre-created, what do they have for a SAN value?

X509v3 Subject Alternative Name: DNS:localhost, DNS:localhost.localdomain, DNS:localhost4, DNS:localhost4.localdomain4, DNS:localhost6, DNS:localhost6.localdomain6, IP Address:127.0.0.1, IP Address:0:0:0:0:0:0:0:1

I think it will suffice, once CI reaches that we'll get a verification.
I 'd need to remove the SanEvaluator , as it is not used anymore.

that looks close to what we used to use, lets go with it and see if we have issues

jaymode · 2018-07-13T15:54:51Z

x-pack/qa/sql/security/ssl/build.gradle

+          'print(java.security.Security.getProviders()[0].name.toLowerCase().contains("fips"));')) == false
+}
+// Do not attempt to form a cluster in a FIPS JVM, as doing so with a JKS keystore will fail.
+// TODO Revisit this when SQL CLI client can handle key/certificate instead of only Keystores.


is there an issue open for this?

No, we discussed it with Costin but I didn't yet come around to opening the issue. Will do ASAP

thanks and please add it to the code comment

rjernst · 2018-07-14T00:06:20Z

build.gradle

+allprojects {
+  tasks.withType(RandomizedTestingTask) {
+    // So that this gets executed only right before the test runs
+    doFirst {


This does not actually guarantee that it will be run right before the test is executed. Other doFirst blocks could (and are) added throughout the build, and which order they run in is dependent on when they are added to the task.

I think this would be better as setting a project property in BuildPlugin.globalBuildInfo alongside how we set runtimeJavaHome. Then in BuildPlugin.commonTestConfig have a condition on whether it is a fips jvm to add these sysprops.

rjernst · 2018-07-14T00:06:45Z

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

@@ -287,7 +287,7 @@ class BuildPlugin implements Plugin<Project> {
    }

    /** Runs the given javascript using jjs from the jdk, and returns the output */
-    private static String runJavascript(Project project, String javaHome, String script) {
+    static String runJavascript(Project project, String javaHome, String script) {


see my comment about this use. This can go back to private.

rjernst · 2018-07-14T00:08:34Z

server/src/test/java/org/elasticsearch/action/admin/ReloadSecureSettingsIT.java

-                                assertThat(nodeResponse.reloadException(), instanceOf(IOException.class));
+                                // Running in a JVM with a BouncyCastle FIPS Security Provider, decrypting the Keystore with the wrong
+                                // password can return a SecurityException if the DataInputStream can't be fully consumed
+                                assertThat(nodeResponse.reloadException(),


I think we should set an additional system property when running in a fips jvm so tests can conditionalize checks like this.

Something similar is already added in ESTestCase https://github.com/elastic/elasticsearch/pull/31666/files#diff-ea1d57cff30e53747f2ce541df4dff2eR1368 (ready to be merged). However this is the case of a specific (granted, FIPS ) provider's behavior difference in how streams are consumed and might not be the case with others, so this could not be generalized enough with an if inFipsJvm() check - See our discussion in #28515

Then we should add whatever extra properties are necessary for the test to distinguish the two cases. What would happen if one jvm started throwing the other exception? The test would be out of date but we would have no idea there was a behavior change.

rjernst · 2018-07-14T00:14:12Z

x-pack/qa/smoke-test-plugins-ssl/build.gradle

  extraConfigFile nodeKeystore.name, nodeKeystore
+  extraConfigFile nodeKey.name, nodeKey


If we are going to have static files, then I think these should be resource files in buildSrc. You can add a helper method (ie property that is a closure) in BuildPlugin that is attached to all projects called resourceFile, so this can look like:

extraConfigFile nodeKey.name, project.resourceFile('testnode.pem')

Thanks for the feedback @rjernst ! I addressed the rest of your points but can you give me a pointer on how such a Closure property could look like ?

In BuildPlugin, look at configureTest. This adds an additionalTest closure that acts like a method to add additional test tasks.

jkakavas · 2018-07-19T12:51:57Z

After discussions with @atorok and @rjernst ( thanks both for the feedback ! ) the issue of how static files can be resolved from the buildSrc project resources will be tackled in a generic manner and @atorok is working on a PR that addresses that. I will pause working on this until the newly added helper methods are merged

This reverts commit 86ba696.

jkakavas · 2018-07-23T21:34:07Z

@rjernst, @jaymode this is ready for another round of review.

I adjusted my approach of referencing static files to the alternative that (continues to) use project coupling but with better referencing as Alpar suggested in #31989 (comment).

Once #32201 is merged, I will take up the task to rewrite the all the cases in our IT ( the ones touched in this PR is only a subset of those using this way of referencing static files) using the new approach.

rjernst

This looks ok, but if we are going to have these cross project deps, there needs to be a followup issue to remove them, dependent on the in progress gradle work. Also, there are a ton of whitespace related changes, can those be omitted? It makes the PR much larger than it would be otherwise.

rjernst · 2018-07-23T22:05:14Z

x-pack/plugin/build.gradle

+// Add key and certs to test classpath: it expects them there
+// User cert and key PEM files instead of a JKS Keystore for the cluster's trust material so that
+// it can run in a FIPS 140 JVM
+task copyKeyCerts(type: Copy) {


Can you please add a comment with a TODO and link to the PR that will make this not use cross project references?

jaymode

LGTM

* Complete changes for running IT in a fips JVM - Mute :x-pack:qa:sql:security:ssl:integTest as it cannot run in FIPS 140 JVM until the SQL CLI supports key/cert. - Set default JVM keystore/truststore password in top level build script for all integTest tasks in a FIPS 140 JVM - Changed top level x-pack build script to use keys and certificates for trust/key material when spinning up clusters for IT

* Complete changes for running IT in a fips JVM - Mute :x-pack:qa:sql:security:ssl:integTest as it cannot run in FIPS 140 JVM until the SQL CLI supports key/cert. - Set default JVM keystore/truststore password in top level build script for all integTest tasks in a FIPS 140 JVM - Changed top level x-pack build script to use keys and certificates for trust/key material when spinning up clusters for IT This is a backport of #31989

Now that elastic#31666 and elastic#31989 are merged we can run our tests in fips JVM. This commits enables us to run tests on a Java 8 JVM using BouncyCastleFIPS as a security Provider.

* master: Security: revert to old way of merging automata (#32254) Networking: Fix test leaking buffer (#32296) Undo a debugging change that snuck in during the field aliases merge. Painless: Update More Methods to New Naming Scheme (#32305) [TEST] Fix assumeFalse -> assumeTrue in SSLReloadIntegTests Ingest: Support integer and long hex values in convert (#32213) Introduce fips_mode setting and associated checks (#32326) Add V_6_3_3 version constant [DOCS] Removed extraneous callout number. Rest HL client: Add put license action (#32214) Add ERR to ranking evaluation documentation (#32314) Introduce Application Privileges with support for Kibana RBAC (#32309) Build: Shadow x-pack:protocol into x-pack:plugin:core (#32240) [Kerberos] Add Kerberos authentication support (#32263) [ML] Extract persistent task methods from MlMetadata (#32319) Add Restore Snapshot High Level REST API Register ERR metric with NamedXContentRegistry (#32320) fixes broken build for third-party-tests (#32315) Allow Integ Tests to run in a FIPS-140 JVM (#31989) [DOCS] Rollup Caps API incorrectly mentions GET Jobs API (#32280) awaitsfix testRandomClusterStateUpdates [TEST] add version skip to weighted_avg tests Consistent encoder names (#29492) Add WeightedAvg metric aggregation (#31037) Switch monitoring to new style Requests (#32255) Rename ranking evaluation `quality_level` to `metric_score` (#32168) Fix a test bug around nested aggregations and field aliases. (#32287) Add new permission for JDK11 to load JAAS libraries (#32132) Silence SSL reload test that fails on JDK 11 [test] package pre-install java check (#32259) specify subdirs of lib, bin, modules in package (#32253) Switch x-pack:core to new style Requests (#32252) awaitsfix SSLConfigurationReloaderTests Painless: Clean up add methods in PainlessLookup (#32258) Fail shard if IndexShard#storeStats runs into an IOException (#32241) AwaitsFix RecoveryIT#testHistoryUUIDIsGenerated Remove unnecessary warning supressions (#32250) CCE when re-throwing "shard not available" exception in TransportShardMultiGetAction (#32185) Add new fields to monitoring template for Beats state (#32085)

Now that #31666 and #31989 are merged we can run our tests in fips JVM. This commits enables us to run tests on a Java 8 JVM using BouncyCastleFIPS as a security Provider.

jkakavas added 3 commits July 11, 2018 18:15

unused import and identation

32a9b89

remove print and migrate to ml-native-multi-node-tests

740259d

jkakavas added >enhancement >test Issues or PRs that are addressing/adding tests v7.0.0 :Security/TLS SSL/TLS, Certificates v6.4.0 labels Jul 12, 2018

jkakavas requested a review from jaymode July 12, 2018 08:30

alpar-t reviewed Jul 12, 2018

View reviewed changes

Address feedback

86ba696

- Attempt to limit arbitrary project coupling by bringing necessary certificate/key/keystore files for IT in a small separate project and - Introduce Closre checking if we run in a FIPS JVM

jaymode reviewed Jul 13, 2018

View reviewed changes

rjernst requested changes Jul 14, 2018

View reviewed changes

alpar-t mentioned this pull request Jul 19, 2018

Access build tools resources #32201

Merged

elasticmachine mentioned this pull request Jul 20, 2018

Security: FIPS 140-2 Compliance roadmap #29851

Closed

12 tasks

jkakavas added 4 commits July 23, 2018 17:29

Revert "Address feedback"

ced0e72

This reverts commit 86ba696.

Partially addresses feedback

86bdddd

Merge remote-tracking branch 'origin/master' into fips-IT

7e1550f

Address feedback

dd61b4b

Remove unused import

8f020b1

rjernst approved these changes Jul 23, 2018

View reviewed changes

jkakavas added 3 commits July 24, 2018 01:43

Merge remote-tracking branch 'origin/master' into fips-IT

7660385

Address feedback

12b7ccb

add link to SSL sql cli issue

a44d734

jaymode approved these changes Jul 24, 2018

View reviewed changes

Fix key password parameter

b5753bb

jkakavas merged commit a2dbd83 into elastic:master Jul 24, 2018

jkakavas added the backport pending label Jul 24, 2018

jkakavas removed the backport pending label Jul 24, 2018

jkakavas mentioned this pull request Jul 24, 2018

Enable FIPS JVM in CI #32330

Merged

jkakavas added a commit that referenced this pull request Jul 25, 2018

Enable FIPS JVM in CI (#32330)

d0fd236

Now that #31666 and #31989 are merged we can run our tests in fips JVM. This commits enables us to run tests on a Java 8 JVM using BouncyCastleFIPS as a security Provider.

jkakavas added a commit that referenced this pull request Jul 25, 2018

Enable FIPS JVM in CI (#32330)

6f5b691

Now that #31666 and #31989 are merged we can run our tests in fips JVM. This commits enables us to run tests on a Java 8 JVM using BouncyCastleFIPS as a security Provider.

jkakavas mentioned this pull request Jul 26, 2018

[CI] Test Failure ReloadSecureSettingsIT testWrongKeystorePassword #32411

Closed

jkakavas deleted the fips-IT branch September 14, 2018 06:55

jimczi added v7.0.0-beta1 and removed v7.0.0 labels Feb 7, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow Integ Tests to run in a FIPS-140 JVM #31989

Allow Integ Tests to run in a FIPS-140 JVM #31989

jkakavas commented Jul 12, 2018

elasticmachine commented Jul 12, 2018

jkakavas commented Jul 12, 2018

alpar-t Jul 12, 2018

jkakavas Jul 12, 2018

alpar-t Jul 12, 2018

jkakavas Jul 12, 2018 •

edited

Loading

alpar-t Jul 13, 2018 •

edited

Loading

alpar-t Jul 12, 2018

jkakavas Jul 12, 2018

alpar-t Jul 12, 2018

jkakavas commented Jul 12, 2018

jaymode Jul 13, 2018

jkakavas Jul 13, 2018

jaymode Jul 13, 2018

jaymode Jul 13, 2018

jkakavas Jul 13, 2018

jaymode Jul 13, 2018

rjernst Jul 14, 2018

rjernst Jul 14, 2018

rjernst Jul 14, 2018

jkakavas Jul 16, 2018 •

edited

Loading

rjernst Jul 16, 2018

rjernst Jul 14, 2018

jkakavas Jul 16, 2018

rjernst Jul 16, 2018

jkakavas commented Jul 19, 2018

jkakavas commented Jul 23, 2018

rjernst left a comment

rjernst Jul 23, 2018

jaymode left a comment

		@@ -29,132 +30,13 @@ project.sourceSets.test.output.dir(outputDir, builtBy: copyXPackPluginProps)
		// needed to be consistent with ssl host checking
		Object san = new SanEvaluator()

		extraConfigFile nodeKeystore.name, nodeKeystore
		extraConfigFile nodeKey.name, nodeKey

Allow Integ Tests to run in a FIPS-140 JVM #31989

Allow Integ Tests to run in a FIPS-140 JVM #31989

Conversation

jkakavas commented Jul 12, 2018

elasticmachine commented Jul 12, 2018

jkakavas commented Jul 12, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jkakavas Jul 12, 2018 • edited Loading

Choose a reason for hiding this comment

alpar-t Jul 13, 2018 • 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 Jul 12, 2018

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jkakavas Jul 16, 2018 • 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

jkakavas commented Jul 19, 2018

jkakavas commented Jul 23, 2018

rjernst left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jaymode left a comment

Choose a reason for hiding this comment

jkakavas Jul 12, 2018 •

edited

Loading

alpar-t Jul 13, 2018 •

edited

Loading

jkakavas Jul 16, 2018 •

edited

Loading