Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 13 commits into from
Jul 24, 2018
Merged

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

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
c9008db
Complete changes for running IT in a fips JVM
jkakavas Jul 6, 2018
32a9b89
unused import and identation
jkakavas Jul 12, 2018
740259d
remove print and migrate to ml-native-multi-node-tests
jkakavas Jul 12, 2018
86ba696
Address feedback
jkakavas Jul 13, 2018
ced0e72
Revert "Address feedback"
jkakavas Jul 23, 2018
86bdddd
Partially addresses feedback
jkakavas Jul 23, 2018
7e1550f
Merge remote-tracking branch 'origin/master' into fips-IT
jkakavas Jul 23, 2018
dd61b4b
Address feedback
jkakavas Jul 23, 2018
8f020b1
Remove unused import
jkakavas Jul 23, 2018
7660385
Merge remote-tracking branch 'origin/master' into fips-IT
jkakavas Jul 23, 2018
12b7ccb
Address feedback
jkakavas Jul 23, 2018
a44d734
add link to SSL sql cli issue
jkakavas Jul 23, 2018
b5753bb
Fix key password parameter
jkakavas Jul 24, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 14 additions & 0 deletions build.gradle
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
*/


import com.carrotsearch.gradle.junit4.RandomizedTestingTask
import org.apache.tools.ant.taskdefs.condition.Os
import org.apache.tools.ant.filters.ReplaceTokens
import org.elasticsearch.gradle.BuildPlugin
Expand Down Expand Up @@ -476,6 +477,19 @@ allprojects {
tasks.eclipse.dependsOn(cleanEclipse, copyEclipseSettings)
}

// Set the system keystore/truststore password if we're running tests in a FIPS-140 JVM
allprojects {
tasks.withType(RandomizedTestingTask) {
// So that this gets executed only right before the test runs
doFirst {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

String inFipsJvmScript = 'print(java.security.Security.getProviders()[0].name.toLowerCase().contains("fips"));'
if (Boolean.parseBoolean(BuildPlugin.runJavascript(project, project.runtimeJavaHome, inFipsJvmScript))) {
systemProperty 'javax.net.ssl.trustStorePassword', 'password'
systemProperty 'javax.net.ssl.keyStorePassword', 'password'
}
}
}
}
// we need to add the same --debug-jvm option as
// the real RunTask has, so we can pass it through
class Run extends DefaultTask {
Expand Down
2 changes: 1 addition & 1 deletion buildSrc/src/main/groovy/org/elasticsearch/gradle/BuildPlugin.groovy
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

ByteArrayOutputStream stdout = new ByteArrayOutputStream()
ByteArrayOutputStream stderr = new ByteArrayOutputStream()
if (Os.isFamily(Os.FAMILY_WINDOWS)) {
Expand Down
30 changes: 0 additions & 30 deletions plugins/discovery-gce/build.gradle
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,36 +22,6 @@ dependencies {
compile "commons-codec:commons-codec:${versions.commonscodec}"
}


// needed to be consistent with ssl host checking
String host = InetAddress.getLoopbackAddress().getHostAddress();

// location of keystore and files to generate it
File keystore = new File(project.buildDir, 'keystore/test-node.jks')

// generate the keystore
task createKey(type: LoggedExec) {
doFirst {
project.delete(keystore.parentFile)
keystore.parentFile.mkdirs()
}
executable = new File(project.runtimeJavaHome, 'bin/keytool')
standardInput = new ByteArrayInputStream('FirstName LastName\nUnit\nOrganization\nCity\nState\nNL\nyes\n\n'.getBytes('UTF-8'))
args '-genkey',
'-alias', 'test-node',
'-keystore', keystore,
'-keyalg', 'RSA',
'-keysize', '2048',
'-validity', '712',
'-dname', 'CN=' + host,
'-keypass', 'keypass',
'-storepass', 'keypass'
}

// add keystore to test classpath: it expects it there
sourceSets.test.resources.srcDir(keystore.parentFile)
processTestResources.dependsOn(createKey)

dependencyLicenses {
mapping from: /google-.*/, to: 'google'
}
Expand Down
6 changes: 5 additions & 1 deletion server/src/test/java/org/elasticsearch/action/admin/ReloadSecureSettingsIT.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@
import java.util.concurrent.CountDownLatch;
import java.util.concurrent.atomic.AtomicReference;

import static org.hamcrest.Matchers.anyOf;
import static org.hamcrest.Matchers.equalTo;
import static org.hamcrest.Matchers.notNullValue;
import static org.hamcrest.Matchers.nullValue;
Expand Down Expand Up @@ -205,7 +206,10 @@ public void onResponse(NodesReloadSecureSettingsResponse nodesReloadResponse) {
assertThat(nodesMap.size(), equalTo(cluster().size()));
for (final NodesReloadSecureSettingsResponse.NodeResponse nodeResponse : nodesReloadResponse.getNodes()) {
assertThat(nodeResponse.reloadException(), notNullValue());
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(),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

@jkakavas jkakavas Jul 16, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

anyOf(instanceOf(IOException.class), instanceOf(SecurityException.class)));
}
} catch (final AssertionError e) {
reloadSettingsError.set(e);
Expand Down
47 changes: 18 additions & 29 deletions x-pack/plugin/build.gradle
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -104,39 +104,26 @@ integTestRunner {
systemProperty 'tests.rest.blacklist', blacklist.join(',')
}

// location of generated keystores and certificates
// location for keys and certificates
File keystoreDir = new File(project.buildDir, 'keystore')

// Generate the node's keystore
File nodeKeystore = new File(keystoreDir, 'test-node.jks')
task createNodeKeyStore(type: LoggedExec) {
doFirst {
if (nodeKeystore.parentFile.exists() == false) {
nodeKeystore.parentFile.mkdirs()
}
if (nodeKeystore.exists()) {
delete nodeKeystore
File nodeKey = new File(keystoreDir, 'testnode.pem')
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

File nodeCert = new File(keystoreDir, 'testnode.crt')

// 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) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

from('./core/src/test/resources/org/elasticsearch/xpack/security/transport/ssl/certs/simple/') {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ?

Copy link
Member Author

@jkakavas jkakavas Jul 12, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

@alpar-t alpar-t Jul 13, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ).

include 'testnode.crt', 'testnode.pem'
}
}
executable = new File(project.runtimeJavaHome, 'bin/keytool')
standardInput = new ByteArrayInputStream('FirstName LastName\nUnit\nOrganization\nCity\nState\nNL\nyes\n\n'.getBytes('UTF-8'))
args '-genkey',
'-alias', 'test-node',
'-keystore', nodeKeystore,
'-keyalg', 'RSA',
'-keysize', '2048',
'-validity', '712',
'-dname', 'CN=smoke-test-plugins-ssl',
'-keypass', 'keypass',
'-storepass', 'keypass'
into keystoreDir
}

// Add keystores to test classpath: it expects it there
sourceSets.test.resources.srcDir(keystoreDir)
processTestResources.dependsOn(createNodeKeyStore)
processTestResources.dependsOn(copyKeyCerts)

integTestCluster {
dependsOn createNodeKeyStore
dependsOn copyKeyCerts
setting 'xpack.ml.enabled', 'true'
setting 'xpack.security.enabled', 'true'
setting 'logger.org.elasticsearch.xpack.ml.datafeed', 'TRACE'
Expand All @@ -145,18 +132,20 @@ integTestCluster {
setting 'xpack.monitoring.exporters._local.enabled', 'false'
setting 'xpack.security.authc.token.enabled', 'true'
setting 'xpack.security.transport.ssl.enabled', 'true'
setting 'xpack.security.transport.ssl.keystore.path', nodeKeystore.name
setting 'xpack.security.transport.ssl.key', nodeKey.name
setting 'xpack.security.transport.ssl.certificate', nodeCert.name
setting 'xpack.security.transport.ssl.verification_mode', 'certificate'
setting 'xpack.security.audit.enabled', 'true'
setting 'xpack.license.self_generated.type', 'trial'
keystoreSetting 'bootstrap.password', 'x-pack-test-password'
keystoreSetting 'xpack.security.transport.ssl.keystore.secure_password', 'keypass'
keystoreSetting 'xpack.security.transport.ssl.secure_key_passphrase', 'testnode'
keystoreSetting 'xpack.security.ingest.hash.processor.key', 'hmackey'
distribution = 'zip' // this is important since we use the reindex module in ML

setupCommand 'setupTestUser', 'bin/elasticsearch-users', 'useradd', 'x_pack_rest_user', '-p', 'x-pack-test-password', '-r', 'superuser'

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

waitCondition = { NodeInfo node, AntBuilder ant ->
File tmpFile = new File(node.cwd, 'wait.success')
Expand Down
5 changes: 2 additions & 3 deletions x-pack/plugin/src/test/resources/rest-api-spec/test/ssl/10_basic.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@
xpack.ssl.certificates: {}

- length: { $body: 1 }
- match: { $body.0.path: "test-node.jks" }
- match: { $body.0.format: "jks" }
- match: { $body.0.alias: "test-node" }
- match: { $body.0.path: "testnode.crt" }
- match: { $body.0.format: "PEM" }
- match: { $body.0.has_private_key: true }
46 changes: 16 additions & 30 deletions x-pack/qa/ml-native-multi-node-tests/build.gradle
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,59 +18,45 @@ integTestRunner {
systemProperty 'es.set.netty.runtime.available.processors', 'false'
}

// location of generated keystores and certificates
// location for keys and certificates
File keystoreDir = new File(project.buildDir, 'keystore')

// Generate the node's keystore
File nodeKeystore = new File(keystoreDir, 'test-node.jks')
task createNodeKeyStore(type: LoggedExec) {
doFirst {
if (nodeKeystore.parentFile.exists() == false) {
nodeKeystore.parentFile.mkdirs()
}
if (nodeKeystore.exists()) {
delete nodeKeystore
}
File nodeKey = new File(keystoreDir, 'testnode.pem')
File nodeCert = new File(keystoreDir, 'testnode.crt')
// Add key and certs to test classpath: it expects it there
task copyKeyCerts(type: Copy) {
from('../../plugin/core/src/test/resources/org/elasticsearch/xpack/security/transport/ssl/certs/simple/') {
include 'testnode.crt', 'testnode.pem'
}
executable = new File(project.runtimeJavaHome, 'bin/keytool')
standardInput = new ByteArrayInputStream('FirstName LastName\nUnit\nOrganization\nCity\nState\nNL\nyes\n\n'.getBytes('UTF-8'))
args '-genkey',
'-alias', 'test-node',
'-keystore', nodeKeystore,
'-keyalg', 'RSA',
'-keysize', '2048',
'-validity', '712',
'-dname', 'CN=smoke-test-plugins-ssl',
'-keypass', 'keypass',
'-storepass', 'keypass'
into keystoreDir
}

// Add keystores to test classpath: it expects it there
// Add keys and cets to test classpath: it expects it there
sourceSets.test.resources.srcDir(keystoreDir)
processTestResources.dependsOn(createNodeKeyStore)
processTestResources.dependsOn(copyKeyCerts)

integTestCluster {
dependsOn createNodeKeyStore
dependsOn copyKeyCerts
setting 'xpack.security.enabled', 'true'
setting 'xpack.ml.enabled', 'true'
setting 'logger.org.elasticsearch.xpack.ml.datafeed', 'TRACE'
setting 'xpack.monitoring.enabled', 'false'
setting 'xpack.security.authc.token.enabled', 'true'
setting 'xpack.security.transport.ssl.enabled', 'true'
setting 'xpack.security.transport.ssl.keystore.path', nodeKeystore.name
setting 'xpack.security.transport.ssl.key', nodeKey.name
setting 'xpack.security.transport.ssl.certificate', nodeCert.name
setting 'xpack.security.transport.ssl.verification_mode', 'certificate'
setting 'xpack.security.audit.enabled', 'true'
setting 'xpack.license.self_generated.type', 'trial'

keystoreSetting 'bootstrap.password', 'x-pack-test-password'
keystoreSetting 'xpack.security.transport.ssl.keystore.secure_password', 'keypass'
keystoreSetting 'xpack.security.transport.ssl.secure_key_passphrase', 'testnode'

numNodes = 3

setupCommand 'setupDummyUser',
'bin/elasticsearch-users', 'useradd', 'x_pack_rest_user', '-p', 'x-pack-test-password', '-r', 'superuser'

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

waitCondition = { node, ant ->
File tmpFile = new File(node.cwd, 'wait.success')
Expand Down
11 changes: 7 additions & 4 deletions ...src/test/java/org/elasticsearch/xpack/ml/integration/MlNativeAutodetectIntegTestCase.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -123,9 +123,11 @@ protected Collection<Class<? extends Plugin>> transportClientPlugins() {

@Override
protected Settings externalClusterClientSettings() {
Path keyStore;
Path key;
Path certificate;
try {
keyStore = PathUtils.get(getClass().getResource("/test-node.jks").toURI());
key = PathUtils.get(getClass().getResource("/testnode.pem").toURI());
certificate = PathUtils.get(getClass().getResource("/testnode.crt").toURI());
} catch (URISyntaxException e) {
throw new IllegalStateException("error trying to get keystore path", e);
}
Expand All @@ -134,8 +136,9 @@ protected Settings externalClusterClientSettings() {
builder.put(SecurityField.USER_SETTING.getKey(), "x_pack_rest_user:" + SecuritySettingsSourceField.TEST_PASSWORD_SECURE_STRING);
builder.put(XPackSettings.MACHINE_LEARNING_ENABLED.getKey(), true);
builder.put("xpack.security.transport.ssl.enabled", true);
builder.put("xpack.security.transport.ssl.keystore.path", keyStore.toAbsolutePath().toString());
builder.put("xpack.security.transport.ssl.keystore.password", "keypass");
builder.put("xpack.security.transport.ssl.key", key.toAbsolutePath().toString());
builder.put("xpack.security.transport.ssl.certificate", certificate.toAbsolutePath().toString());
builder.put("xpack.security.transport.ssl.key_passphrase", "testnode");
builder.put("xpack.security.transport.ssl.verification_mode", "certificate");
return builder.build();
}
Expand Down
Loading