From 2fef2c72eb226db452f4d8c1104ada372577742b Mon Sep 17 00:00:00 2001 From: Simon Willnauer Date: Thu, 22 Jun 2017 21:48:51 +0200 Subject: [PATCH] Simplify SSL server configuratin validation (elastic/x-pack-elasticsearch#1826) Today we have some hidden complexity related to default configurations might specify NO_KEY which is in some cases valid for server configuration. This change removes the leniencey paramenters on the validation methods and removes obsolet asserts. Original commit: elastic/x-pack-elasticsearch@17ed4b1d20c5f4ccb560ade976c40d42c9ad380c --- .../SecurityNetty4HttpServerTransport.java | 4 ++-- .../netty4/SecurityNetty4Transport.java | 5 +---- .../elasticsearch/xpack/ssl/SSLService.java | 11 +++-------- .../xpack/ssl/SSLServiceTests.java | 18 +++++++----------- 4 files changed, 13 insertions(+), 25 deletions(-) diff --git a/plugin/src/main/java/org/elasticsearch/xpack/security/transport/netty4/SecurityNetty4HttpServerTransport.java b/plugin/src/main/java/org/elasticsearch/xpack/security/transport/netty4/SecurityNetty4HttpServerTransport.java index d74b912bb08..50c0b716a66 100644 --- a/plugin/src/main/java/org/elasticsearch/xpack/security/transport/netty4/SecurityNetty4HttpServerTransport.java +++ b/plugin/src/main/java/org/elasticsearch/xpack/security/transport/netty4/SecurityNetty4HttpServerTransport.java @@ -45,11 +45,11 @@ public class SecurityNetty4HttpServerTransport extends Netty4HttpServerTransport this.sslSettings = SSLService.getHttpTransportSSLSettings(settings); this.sslService = sslService; if (ssl) { - if (sslService.isConfigurationValidForServerUsage(sslSettings, false) == false) { + this.sslConfiguration = sslService.sslConfiguration(sslSettings, Settings.EMPTY); + if (sslService.isConfigurationValidForServerUsage(sslConfiguration) == false) { throw new IllegalArgumentException("a key must be provided to run as a server. the key should be configured using the " + "[xpack.security.http.ssl.key] or [xpack.security.http.ssl.keystore.path] setting"); } - this.sslConfiguration = sslService.sslConfiguration(sslSettings, Settings.EMPTY); } else { this.sslConfiguration = null; } diff --git a/plugin/src/main/java/org/elasticsearch/xpack/security/transport/netty4/SecurityNetty4Transport.java b/plugin/src/main/java/org/elasticsearch/xpack/security/transport/netty4/SecurityNetty4Transport.java index d1bfc3007b6..747859c2f2c 100644 --- a/plugin/src/main/java/org/elasticsearch/xpack/security/transport/netty4/SecurityNetty4Transport.java +++ b/plugin/src/main/java/org/elasticsearch/xpack/security/transport/netty4/SecurityNetty4Transport.java @@ -21,7 +21,6 @@ import org.elasticsearch.indices.breaker.CircuitBreakerService; import org.elasticsearch.threadpool.ThreadPool; import org.elasticsearch.transport.TransportSettings; import org.elasticsearch.transport.netty4.Netty4Transport; -import org.elasticsearch.xpack.security.transport.ServerTransportFilter; import org.elasticsearch.xpack.ssl.SSLConfiguration; import org.elasticsearch.xpack.ssl.SSLService; import org.elasticsearch.xpack.security.transport.filter.IPFilter; @@ -64,13 +63,11 @@ public class SecurityNetty4Transport extends Netty4Transport { final Settings profileSslSettings = profileSslSettings(profileSettings); SSLConfiguration configuration = sslService.sslConfiguration(profileSslSettings, transportSSLSettings); profileConfiguration.put(entry.getKey(), configuration); - assert sslService.isConfigurationValidForServerUsage(profileSslSettings, true) : - "the ssl configuration is not valid for server use but we are running as a server. this should have been caught by" + - " the key config validation"; } if (profileConfiguration.containsKey(TransportSettings.DEFAULT_PROFILE) == false) { profileConfiguration.put(TransportSettings.DEFAULT_PROFILE, sslConfiguration); + } this.profileConfiguration = Collections.unmodifiableMap(profileConfiguration); diff --git a/plugin/src/main/java/org/elasticsearch/xpack/ssl/SSLService.java b/plugin/src/main/java/org/elasticsearch/xpack/ssl/SSLService.java index 545a0e59400..245b615f78e 100644 --- a/plugin/src/main/java/org/elasticsearch/xpack/ssl/SSLService.java +++ b/plugin/src/main/java/org/elasticsearch/xpack/ssl/SSLService.java @@ -250,15 +250,10 @@ public class SSLService extends AbstractComponent { /** * Returns whether the provided settings results in a valid configuration that can be used for server connections - * @param settings the settings used to identify the ssl configuration, typically under a *.ssl. prefix - * @param useTransportFallback if {@code true} this will use the transport configuration for fallback, otherwise the global - * configuration will be used + * @param sslConfiguration the configuration to check */ - public boolean isConfigurationValidForServerUsage(Settings settings, boolean useTransportFallback) { - SSLConfiguration fallback = useTransportFallback ? transportSSLConfiguration.get() : globalSSLConfiguration; - SSLConfiguration sslConfiguration = new SSLConfiguration(settings, fallback); - return sslConfiguration.keyConfig() != KeyConfig.NONE - || (useTransportFallback && transportSSLConfiguration.get().keyConfig() == KeyConfig.NONE); + public boolean isConfigurationValidForServerUsage(SSLConfiguration sslConfiguration) { + return sslConfiguration.keyConfig() != KeyConfig.NONE; } /** diff --git a/plugin/src/test/java/org/elasticsearch/xpack/ssl/SSLServiceTests.java b/plugin/src/test/java/org/elasticsearch/xpack/ssl/SSLServiceTests.java index 9f8de6d9e41..987e55f03c2 100644 --- a/plugin/src/test/java/org/elasticsearch/xpack/ssl/SSLServiceTests.java +++ b/plugin/src/test/java/org/elasticsearch/xpack/ssl/SSLServiceTests.java @@ -157,10 +157,6 @@ public class SSLServiceTests extends ESTestCase { assertThat(sslEngine, notNullValue()); } - public void testCreateWithoutAnySettingsValidForServer() throws Exception { - SSLService sslService = new SSLService(Settings.EMPTY, env); - assertTrue(sslService.isConfigurationValidForServerUsage(Settings.EMPTY, true)); - } public void testCreateWithKeystoreIsValidForServer() throws Exception { Settings settings = Settings.builder() @@ -168,7 +164,8 @@ public class SSLServiceTests extends ESTestCase { .put("xpack.ssl.keystore.password", "testnode") .build(); SSLService sslService = new SSLService(settings, env); - assertTrue(sslService.isConfigurationValidForServerUsage(Settings.EMPTY, false)); + + assertTrue(sslService.isConfigurationValidForServerUsage(sslService.sslConfiguration(Settings.EMPTY))); } public void testValidForServerWithFallback() throws Exception { @@ -177,8 +174,7 @@ public class SSLServiceTests extends ESTestCase { .put("xpack.ssl.truststore.password", "testnode") .build(); SSLService sslService = new SSLService(settings, env); - // transport is valid in default config - assertTrue(sslService.isConfigurationValidForServerUsage(Settings.EMPTY, true)); + assertFalse(sslService.isConfigurationValidForServerUsage(sslService.sslConfiguration(Settings.EMPTY))); settings = Settings.builder() .put("xpack.ssl.truststore.path", testnodeStore) @@ -187,10 +183,10 @@ public class SSLServiceTests extends ESTestCase { .put("xpack.security.transport.ssl.keystore.password", "testnode") .build(); sslService = new SSLService(settings, env); - assertFalse(sslService.isConfigurationValidForServerUsage(Settings.EMPTY, false)); - assertTrue(sslService.isConfigurationValidForServerUsage( - settings.getByPrefix("xpack.security.transport.ssl."), false)); - assertTrue(sslService.isConfigurationValidForServerUsage(Settings.EMPTY, true)); + assertFalse(sslService.isConfigurationValidForServerUsage(sslService.sslConfiguration(Settings.EMPTY))); + assertTrue(sslService.isConfigurationValidForServerUsage(sslService.sslConfiguration( + settings.getByPrefix("xpack.security.transport.ssl.")))); + assertFalse(sslService.isConfigurationValidForServerUsage(sslService.sslConfiguration(Settings.EMPTY))); } public void testGetVerificationMode() throws Exception {