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

Test adjustments for FIPS 140 in Java 8 #52211

Closed
wants to merge 6 commits into from
Closed

Test adjustments for FIPS 140 in Java 8 #52211

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
1d5f2aa
Test adjustments for FIPS 140 in Java 8
jkakavas Feb 11, 2020
1b023f4
Merge remote-tracking branch 'origin/7.x' into fips-tests-adjustments…
jkakavas Feb 11, 2020
633fb3a
Fix test
jkakavas Feb 12, 2020
e8c278f
failing test
jkakavas Feb 12, 2020
31f796f
address feedback
jkakavas Feb 15, 2020
dd52078
Merge remote-tracking branch 'origin/7.x' into fips-tests-adjustments…
jkakavas Feb 15, 2020
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
5 changes: 5 additions & 0 deletions test/framework/src/main/java/org/elasticsearch/test/ESIntegTestCase.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,7 @@
import org.elasticsearch.action.search.SearchResponse;
import org.elasticsearch.action.support.DefaultShardOperationFailedException;
import org.elasticsearch.action.support.IndicesOptions;
import org.elasticsearch.bootstrap.JavaVersion;
import org.elasticsearch.client.AdminClient;
import org.elasticsearch.client.Client;
import org.elasticsearch.client.Requests;
Expand Down Expand Up @@ -2256,4 +2257,8 @@ public static Index resolveIndex(String index) {
public static boolean inFipsJvm() {
return Boolean.parseBoolean(System.getProperty(FIPS_SYSPROP));
}

public static boolean inFipsSunJsseJvm() {
return inFipsJvm() && JavaVersion.current().getVersion().get(0) == 8;
}
}
4 changes: 4 additions & 0 deletions test/framework/src/main/java/org/elasticsearch/test/ESTestCase.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1391,6 +1391,10 @@ public static boolean inFipsJvm() {
return Boolean.parseBoolean(System.getProperty(FIPS_SYSPROP));
}

public static boolean inFipsSunJsseJvm() {
return inFipsJvm() && JavaVersion.current().getVersion().get(0) == 8;
}

/**
* Returns a unique port range for this JVM starting from the computed base port
*/
Expand Down
21 changes: 19 additions & 2 deletions ...lugin/core/src/test/java/org/elasticsearch/xpack/core/LocalStateCompositeXPackPlugin.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,6 +93,7 @@
import java.util.stream.Collectors;

import static java.util.stream.Collectors.toList;
import static org.elasticsearch.test.ESTestCase.inFipsSunJsseJvm;

public class LocalStateCompositeXPackPlugin extends XPackPlugin implements ScriptPlugin, ActionPlugin, IngestPlugin, NetworkPlugin,
ClusterPlugin, DiscoveryPlugin, MapperPlugin, AnalysisPlugin, PersistentTaskPlugin, EnginePlugin {
Expand Down Expand Up @@ -153,12 +154,17 @@ public Collection<Object> createComponents(Client client, ClusterService cluster
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
Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

// can be adjusted accordingly
final Environment updatedEnvironment = getUpdatedEnvironment(environment);
components.addAll(super.createComponents(client, clusterService, threadPool, resourceWatcherService, scriptService,
xContentRegistry, environment, nodeEnvironment, namedWriteableRegistry));
xContentRegistry, updatedEnvironment, nodeEnvironment, namedWriteableRegistry));

filterPlugins(Plugin.class).stream().forEach(p ->
components.addAll(p.createComponents(client, clusterService, threadPool, resourceWatcherService, scriptService,
xContentRegistry, environment, nodeEnvironment, namedWriteableRegistry))
xContentRegistry, updatedEnvironment, nodeEnvironment, namedWriteableRegistry))
);
return components;
}
Expand Down Expand Up @@ -476,4 +482,15 @@ private <T> List<T> filterPlugins(Class<T> type) {
.collect(Collectors.toList());
}

private Environment getUpdatedEnvironment(Environment existingEnvironment){
jkakavas marked this conversation as resolved.
Show resolved Hide resolved
if (inFipsSunJsseJvm()) {
Settings additionalSettings = Settings.builder()
.put(existingEnvironment.settings())
.put(XPackSettings.DIAGNOSE_TRUST_EXCEPTIONS_SETTING.getKey(), false)
Copy link
Member

Choose a reason for hiding this comment

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

Can't this be returnned from additionalSettings()?

Copy link
Member Author

@jkakavas jkakavas Feb 20, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

.build();
return new Environment(additionalSettings, existingEnvironment.configFile());
}
return existingEnvironment;
}

}
4 changes: 2 additions & 2 deletions x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/TestXPackTransportClient.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@
import java.util.concurrent.TimeUnit;

import static org.elasticsearch.test.ESTestCase.getTestTransportPlugin;
import static org.elasticsearch.test.ESTestCase.inFipsJvm;
import static org.elasticsearch.test.ESTestCase.inFipsSunJsseJvm;

/**
* TransportClient.Builder that installs the XPackPlugin by default.
Expand Down Expand Up @@ -55,7 +55,7 @@ public void close() {

private static Settings possiblyDisableTlsDiagnostic(Settings settings) {
Settings.Builder builder = Settings.builder().put(settings);
if (inFipsJvm()) {
if (inFipsSunJsseJvm()) {
builder.put(XPackSettings.DIAGNOSE_TRUST_EXCEPTIONS_SETTING.getKey(), false);
}
return builder.build();
Expand Down
7 changes: 6 additions & 1 deletion ...test/java/org/elasticsearch/xpack/core/security/transport/ProfileConfigurationsTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
import org.elasticsearch.env.Environment;
import org.elasticsearch.env.TestEnvironment;
import org.elasticsearch.test.ESTestCase;
import org.elasticsearch.xpack.core.XPackSettings;
import org.elasticsearch.xpack.core.ssl.SSLConfiguration;
import org.elasticsearch.xpack.core.ssl.SSLService;
import org.elasticsearch.xpack.core.ssl.VerificationMode;
Expand Down Expand Up @@ -65,10 +66,14 @@ private Settings.Builder getBaseSettings() {
MockSecureSettings secureSettings = new MockSecureSettings();
secureSettings.setString("xpack.security.transport.ssl.keystore.secure_password", "testnode");

return Settings.builder()
Settings.Builder builder = Settings.builder()
.setSecureSettings(secureSettings)
.put("xpack.security.transport.ssl.enabled", true)
.put("xpack.security.transport.ssl.keystore.path", keystore.toString());
if (inFipsSunJsseJvm()) {
builder.put(XPackSettings.DIAGNOSE_TRUST_EXCEPTIONS_SETTING.getKey(), false);
}
return builder;
}

}
21 changes: 15 additions & 6 deletions ...in/core/src/test/java/org/elasticsearch/xpack/core/ssl/SSLConfigurationReloaderTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@
import org.elasticsearch.threadpool.TestThreadPool;
import org.elasticsearch.threadpool.ThreadPool;
import org.elasticsearch.watcher.ResourceWatcherService;
import org.elasticsearch.xpack.core.XPackSettings;
import org.junit.After;
import org.junit.Before;

Expand Down Expand Up @@ -108,7 +109,7 @@ public void testReloadingKeyStore() throws Exception {
Files.copy(getDataPath("/org/elasticsearch/xpack/security/transport/ssl/certs/simple/testnode_updated.jks"), updatedKeystorePath);
MockSecureSettings secureSettings = new MockSecureSettings();
secureSettings.setString("xpack.security.transport.ssl.keystore.secure_password", "testnode");
final Settings settings = Settings.builder()
final Settings settings = getSettingsBuilder()
.put("path.home", createTempDir())
.put("xpack.security.transport.ssl.enabled", true)
.put("xpack.security.transport.ssl.keystore.path", keystorePath)
Expand Down Expand Up @@ -166,7 +167,7 @@ public void testPEMKeyConfigReloading() throws Exception {
Files.copy(getDataPath("/org/elasticsearch/xpack/security/transport/ssl/certs/simple/testnode_updated.crt"), updatedCertPath);
MockSecureSettings secureSettings = new MockSecureSettings();
secureSettings.setString("xpack.security.transport.ssl.secure_key_passphrase", "testnode");
final Settings settings = Settings.builder()
final Settings settings = getSettingsBuilder()
.put("path.home", createTempDir())
.put("xpack.security.transport.ssl.enabled", true)
.put("xpack.security.transport.ssl.key", keyPath)
Expand All @@ -175,7 +176,7 @@ public void testPEMKeyConfigReloading() throws Exception {
.setSecureSettings(secureSettings)
.build();
final Environment env = randomBoolean() ? null :
TestEnvironment.newEnvironment(Settings.builder().put("path.home", createTempDir()).build());
TestEnvironment.newEnvironment(getSettingsBuilder().put("path.home", createTempDir()).build());
// Load HTTPClient once. Client uses a keystore containing testnode key/cert as a truststore
try (CloseableHttpClient client = getSSLClient(Collections.singletonList(certPath))) {
final Consumer<SSLContext> keyMaterialPreChecks = (context) -> {
Expand Down Expand Up @@ -325,7 +326,7 @@ public void testReloadingKeyStoreException() throws Exception {
Files.copy(getDataPath("/org/elasticsearch/xpack/security/transport/ssl/certs/simple/testnode.jks"), keystorePath);
MockSecureSettings secureSettings = new MockSecureSettings();
secureSettings.setString("xpack.security.transport.ssl.keystore.secure_password", "testnode");
Settings settings = Settings.builder()
Settings settings = getSettingsBuilder()
.put("xpack.security.transport.ssl.enabled", true)
.put("xpack.security.transport.ssl.keystore.path", keystorePath)
.setSecureSettings(secureSettings)
Expand Down Expand Up @@ -376,7 +377,7 @@ public void testReloadingPEMKeyConfigException() throws Exception {
Files.copy(getDataPath("/org/elasticsearch/xpack/security/transport/ssl/certs/simple/testclient.crt"), clientCertPath);
MockSecureSettings secureSettings = new MockSecureSettings();
secureSettings.setString("xpack.security.transport.ssl.secure_key_passphrase", "testnode");
Settings settings = Settings.builder()
Settings settings = getSettingsBuilder()
.put("xpack.security.transport.ssl.enabled", true)
.put("xpack.security.transport.ssl.key", keyPath)
.put("xpack.security.transport.ssl.certificate", certPath)
Expand Down Expand Up @@ -519,7 +520,7 @@ private Settings.Builder baseKeystoreSettings(Path tempDir, MockSecureSettings s
}
secureSettings.setString("xpack.security.transport.ssl.secure_key_passphrase", "testnode");

return Settings.builder()
return getSettingsBuilder()
.put("xpack.security.transport.ssl.key", keyPath.toString())
.put("xpack.security.transport.ssl.certificate", certPath.toString())
.setSecureSettings(secureSettings);
Expand Down Expand Up @@ -632,6 +633,14 @@ private static CloseableHttpClient createHttpClient(SSLContext sslContext) {
.build();
}

private Settings.Builder getSettingsBuilder() {
pugnascotia marked this conversation as resolved.
Show resolved Hide resolved
Settings.Builder builder = Settings.builder();
if (inFipsSunJsseJvm()) {
builder.put(XPackSettings.DIAGNOSE_TRUST_EXCEPTIONS_SETTING.getKey(), false);
}
return builder;
}

/**
* Creates our own HttpConnectionFactory that changes how the connection is closed to prevent issues with
* the MockWebServer going into an endless loop based on the way that HttpClient closes its connection.
Expand Down
Loading