From 18c075dbeb81aa1fced875cdc1568fefb5697fd5 Mon Sep 17 00:00:00 2001 From: Dan Hermann Date: Tue, 8 Oct 2019 13:23:23 -0500 Subject: [PATCH 1/5] Validate AUTH_PASSWORD_SETTING at parse time --- .../exporter/http/HttpExporter.java | 40 +++++++++++++------ .../exporter/http/HttpExporterTests.java | 17 ++++++++ 2 files changed, 45 insertions(+), 12 deletions(-) diff --git a/x-pack/plugin/monitoring/src/main/java/org/elasticsearch/xpack/monitoring/exporter/http/HttpExporter.java b/x-pack/plugin/monitoring/src/main/java/org/elasticsearch/xpack/monitoring/exporter/http/HttpExporter.java index be87d4c4e2fae..89847bf2f48e8 100644 --- a/x-pack/plugin/monitoring/src/main/java/org/elasticsearch/xpack/monitoring/exporter/http/HttpExporter.java +++ b/x-pack/plugin/monitoring/src/main/java/org/elasticsearch/xpack/monitoring/exporter/http/HttpExporter.java @@ -183,7 +183,34 @@ public Iterator> settings() { */ public static final Setting.AffixSetting AUTH_PASSWORD_SETTING = Setting.affixKeySetting("xpack.monitoring.exporters.","auth.password", - (key) -> Setting.simpleString(key, Property.Dynamic, Property.NodeScope, Property.Filtered)); + (key) -> Setting.simpleString(key, + new Setting.Validator() { + @Override + public void validate(String password) { + + } + + @Override + public void validate(String password, Map, Object> settings) { + final String namespace = + HttpExporter.AUTH_PASSWORD_SETTING.getNamespace( + HttpExporter.AUTH_PASSWORD_SETTING.getConcreteSetting(key)); + final String username = + (String) settings.get(AUTH_USERNAME_SETTING.getConcreteSettingForNamespace(namespace)); + + // username is required for any auth + if (Strings.isNullOrEmpty(username)) { + if (Strings.isNullOrEmpty(password) == false) { + throw new SettingsException( + "[" + AUTH_PASSWORD_SETTING.getConcreteSettingForNamespace(namespace).getKey() + "] without [" + + AUTH_USERNAME_SETTING.getConcreteSettingForNamespace(namespace).getKey() + "]"); + } + } + } + }, + Property.Dynamic, + Property.NodeScope, + Property.Filtered)); /** * The SSL settings. * @@ -574,17 +601,6 @@ private static CredentialsProvider createCredentialsProvider(final Config config final String username = AUTH_USERNAME_SETTING.getConcreteSettingForNamespace(config.name()).get(config.settings()); final String password = AUTH_PASSWORD_SETTING.getConcreteSettingForNamespace(config.name()).get(config.settings()); - // username is required for any auth - if (Strings.isNullOrEmpty(username)) { - if (Strings.isNullOrEmpty(password) == false) { - throw new SettingsException( - "[" + AUTH_PASSWORD_SETTING.getConcreteSettingForNamespace(config.name()).getKey() + "] without [" + - AUTH_USERNAME_SETTING.getConcreteSettingForNamespace(config.name()).getKey() + "]"); - } - // nothing to configure; default situation for most users - return null; - } - final CredentialsProvider credentialsProvider = new BasicCredentialsProvider(); credentialsProvider.setCredentials(AuthScope.ANY, new UsernamePasswordCredentials(username, password)); diff --git a/x-pack/plugin/monitoring/src/test/java/org/elasticsearch/xpack/monitoring/exporter/http/HttpExporterTests.java b/x-pack/plugin/monitoring/src/test/java/org/elasticsearch/xpack/monitoring/exporter/http/HttpExporterTests.java index dab7c4a1cfc84..22d1d52f4e220 100644 --- a/x-pack/plugin/monitoring/src/test/java/org/elasticsearch/xpack/monitoring/exporter/http/HttpExporterTests.java +++ b/x-pack/plugin/monitoring/src/test/java/org/elasticsearch/xpack/monitoring/exporter/http/HttpExporterTests.java @@ -592,4 +592,21 @@ private static String exporterName() { return "xpack.monitoring.exporters._http"; } + public void testAuthPasswordRequiresAuthUsername() { + final String prefix = "xpack.monitoring.exporters.example"; + final Settings settings = Settings.builder().put(prefix + ".auth.password", "foo").build(); + final IllegalArgumentException e = expectThrows( + IllegalArgumentException.class, + () -> HttpExporter.AUTH_PASSWORD_SETTING.getConcreteSetting(prefix + ".auth.password").get(settings)); + assertThat( + e, + hasToString( + containsString("Failed to parse value [foo] for setting [xpack.monitoring.exporters.example.auth.password]"))); + + assertThat(e.getCause(), instanceOf(SettingsException.class)); + assertThat(e.getCause(), hasToString(containsString("[xpack.monitoring.exporters.example.auth.password] without " + + "[xpack.monitoring.exporters.example.auth.username]"))); + } + + } From 659f754bdc56efbd7f2db5ae1c7832c99ec0157b Mon Sep 17 00:00:00 2001 From: Dan Hermann Date: Tue, 8 Oct 2019 14:14:54 -0500 Subject: [PATCH 2/5] add settings iterator, fix unit tests --- .../exporter/http/HttpExporter.java | 11 +++++ .../exporter/http/HttpExporterTests.java | 41 +++++++------------ 2 files changed, 26 insertions(+), 26 deletions(-) diff --git a/x-pack/plugin/monitoring/src/main/java/org/elasticsearch/xpack/monitoring/exporter/http/HttpExporter.java b/x-pack/plugin/monitoring/src/main/java/org/elasticsearch/xpack/monitoring/exporter/http/HttpExporter.java index 89847bf2f48e8..a75dd975b5511 100644 --- a/x-pack/plugin/monitoring/src/main/java/org/elasticsearch/xpack/monitoring/exporter/http/HttpExporter.java +++ b/x-pack/plugin/monitoring/src/main/java/org/elasticsearch/xpack/monitoring/exporter/http/HttpExporter.java @@ -207,6 +207,17 @@ public void validate(String password, Map, Object> settings) { } } } + + @Override + public Iterator> settings() { + final String namespace = + HttpExporter.AUTH_PASSWORD_SETTING.getNamespace( + HttpExporter.AUTH_PASSWORD_SETTING.getConcreteSetting(key)); + final List> settings = List.of( + HttpExporter.AUTH_USERNAME_SETTING.getConcreteSettingForNamespace(namespace)); + return settings.iterator(); + } + }, Property.Dynamic, Property.NodeScope, diff --git a/x-pack/plugin/monitoring/src/test/java/org/elasticsearch/xpack/monitoring/exporter/http/HttpExporterTests.java b/x-pack/plugin/monitoring/src/test/java/org/elasticsearch/xpack/monitoring/exporter/http/HttpExporterTests.java index 22d1d52f4e220..e71e9a86f3673 100644 --- a/x-pack/plugin/monitoring/src/test/java/org/elasticsearch/xpack/monitoring/exporter/http/HttpExporterTests.java +++ b/x-pack/plugin/monitoring/src/test/java/org/elasticsearch/xpack/monitoring/exporter/http/HttpExporterTests.java @@ -225,17 +225,23 @@ public void testExporterWithEmptyHeaders() { public void testExporterWithPasswordButNoUsername() { final String expected = "[xpack.monitoring.exporters._http.auth.password] without [xpack.monitoring.exporters._http.auth.username]"; - final Settings.Builder builder = Settings.builder() - .put("xpack.monitoring.exporters._http.type", HttpExporter.TYPE) - .put("xpack.monitoring.exporters._http.host", "localhost:9200") - .put("xpack.monitoring.exporters._http.auth.password", "_pass"); - - final Config config = createConfig(builder.build()); + final String prefix = "xpack.monitoring.exporters._http"; + final Settings settings = Settings.builder() + .put(prefix + ".type", HttpExporter.TYPE) + .put(prefix + ".host", "localhost:9200") + .put(prefix + ".auth.password", "_pass") + .build(); - final SettingsException exception = expectThrows(SettingsException.class, - () -> new HttpExporter(config, sslService, threadContext)); + final IllegalArgumentException e = expectThrows( + IllegalArgumentException.class, + () -> HttpExporter.AUTH_PASSWORD_SETTING.getConcreteSetting(prefix + ".auth.password").get(settings)); + assertThat( + e, + hasToString( + containsString("Failed to parse value [_pass] for setting [xpack.monitoring.exporters._http.auth.password]"))); - assertThat(exception.getMessage(), equalTo(expected)); + assertThat(e.getCause(), instanceOf(SettingsException.class)); + assertThat(e.getCause(), hasToString(containsString(expected))); } public void testExporterWithUnknownBlacklistedClusterAlerts() { @@ -592,21 +598,4 @@ private static String exporterName() { return "xpack.monitoring.exporters._http"; } - public void testAuthPasswordRequiresAuthUsername() { - final String prefix = "xpack.monitoring.exporters.example"; - final Settings settings = Settings.builder().put(prefix + ".auth.password", "foo").build(); - final IllegalArgumentException e = expectThrows( - IllegalArgumentException.class, - () -> HttpExporter.AUTH_PASSWORD_SETTING.getConcreteSetting(prefix + ".auth.password").get(settings)); - assertThat( - e, - hasToString( - containsString("Failed to parse value [foo] for setting [xpack.monitoring.exporters.example.auth.password]"))); - - assertThat(e.getCause(), instanceOf(SettingsException.class)); - assertThat(e.getCause(), hasToString(containsString("[xpack.monitoring.exporters.example.auth.password] without " + - "[xpack.monitoring.exporters.example.auth.username]"))); - } - - } From 8307e2b7b7d6337ae28290d3a8610f6381d34553 Mon Sep 17 00:00:00 2001 From: Dan Hermann Date: Mon, 14 Oct 2019 09:28:57 -0500 Subject: [PATCH 3/5] add comment for empty validation method --- .../xpack/monitoring/exporter/http/HttpExporter.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/x-pack/plugin/monitoring/src/main/java/org/elasticsearch/xpack/monitoring/exporter/http/HttpExporter.java b/x-pack/plugin/monitoring/src/main/java/org/elasticsearch/xpack/monitoring/exporter/http/HttpExporter.java index a75dd975b5511..87d92033eb012 100644 --- a/x-pack/plugin/monitoring/src/main/java/org/elasticsearch/xpack/monitoring/exporter/http/HttpExporter.java +++ b/x-pack/plugin/monitoring/src/main/java/org/elasticsearch/xpack/monitoring/exporter/http/HttpExporter.java @@ -187,7 +187,7 @@ public Iterator> settings() { new Setting.Validator() { @Override public void validate(String password) { - + // no password validation that is independent of other settings } @Override From a65a82399b7f40fc73bbbc9030413ab6e3b0cbc5 Mon Sep 17 00:00:00 2001 From: Dan Hermann Date: Mon, 14 Oct 2019 10:21:34 -0500 Subject: [PATCH 4/5] change exception class --- .../xpack/monitoring/exporter/http/HttpExporter.java | 3 ++- .../xpack/monitoring/exporter/http/HttpExporterTests.java | 8 +------- 2 files changed, 3 insertions(+), 8 deletions(-) diff --git a/x-pack/plugin/monitoring/src/main/java/org/elasticsearch/xpack/monitoring/exporter/http/HttpExporter.java b/x-pack/plugin/monitoring/src/main/java/org/elasticsearch/xpack/monitoring/exporter/http/HttpExporter.java index 87d92033eb012..28071cee3ba04 100644 --- a/x-pack/plugin/monitoring/src/main/java/org/elasticsearch/xpack/monitoring/exporter/http/HttpExporter.java +++ b/x-pack/plugin/monitoring/src/main/java/org/elasticsearch/xpack/monitoring/exporter/http/HttpExporter.java @@ -15,6 +15,7 @@ import org.apache.http.nio.conn.ssl.SSLIOSessionStrategy; import org.apache.logging.log4j.LogManager; import org.apache.logging.log4j.Logger; +import org.elasticsearch.ElasticsearchParseException; import org.elasticsearch.Version; import org.elasticsearch.action.ActionListener; import org.elasticsearch.client.RestClient; @@ -201,7 +202,7 @@ public void validate(String password, Map, Object> settings) { // username is required for any auth if (Strings.isNullOrEmpty(username)) { if (Strings.isNullOrEmpty(password) == false) { - throw new SettingsException( + throw new IllegalArgumentException( "[" + AUTH_PASSWORD_SETTING.getConcreteSettingForNamespace(namespace).getKey() + "] without [" + AUTH_USERNAME_SETTING.getConcreteSettingForNamespace(namespace).getKey() + "]"); } diff --git a/x-pack/plugin/monitoring/src/test/java/org/elasticsearch/xpack/monitoring/exporter/http/HttpExporterTests.java b/x-pack/plugin/monitoring/src/test/java/org/elasticsearch/xpack/monitoring/exporter/http/HttpExporterTests.java index e71e9a86f3673..68061a25cb3b1 100644 --- a/x-pack/plugin/monitoring/src/test/java/org/elasticsearch/xpack/monitoring/exporter/http/HttpExporterTests.java +++ b/x-pack/plugin/monitoring/src/test/java/org/elasticsearch/xpack/monitoring/exporter/http/HttpExporterTests.java @@ -235,13 +235,7 @@ public void testExporterWithPasswordButNoUsername() { final IllegalArgumentException e = expectThrows( IllegalArgumentException.class, () -> HttpExporter.AUTH_PASSWORD_SETTING.getConcreteSetting(prefix + ".auth.password").get(settings)); - assertThat( - e, - hasToString( - containsString("Failed to parse value [_pass] for setting [xpack.monitoring.exporters._http.auth.password]"))); - - assertThat(e.getCause(), instanceOf(SettingsException.class)); - assertThat(e.getCause(), hasToString(containsString(expected))); + assertThat(e, hasToString(containsString(expected))); } public void testExporterWithUnknownBlacklistedClusterAlerts() { From 4577ab7930b7dc9f3b4616f68d1f43e00d5ff05b Mon Sep 17 00:00:00 2001 From: Dan Hermann Date: Fri, 25 Oct 2019 05:20:19 -0500 Subject: [PATCH 5/5] remove unused import --- .../xpack/monitoring/exporter/http/HttpExporter.java | 1 - 1 file changed, 1 deletion(-) diff --git a/x-pack/plugin/monitoring/src/main/java/org/elasticsearch/xpack/monitoring/exporter/http/HttpExporter.java b/x-pack/plugin/monitoring/src/main/java/org/elasticsearch/xpack/monitoring/exporter/http/HttpExporter.java index 28071cee3ba04..c5437c61bf55e 100644 --- a/x-pack/plugin/monitoring/src/main/java/org/elasticsearch/xpack/monitoring/exporter/http/HttpExporter.java +++ b/x-pack/plugin/monitoring/src/main/java/org/elasticsearch/xpack/monitoring/exporter/http/HttpExporter.java @@ -15,7 +15,6 @@ import org.apache.http.nio.conn.ssl.SSLIOSessionStrategy; import org.apache.logging.log4j.LogManager; import org.apache.logging.log4j.Logger; -import org.elasticsearch.ElasticsearchParseException; import org.elasticsearch.Version; import org.elasticsearch.action.ActionListener; import org.elasticsearch.client.RestClient;