From 6504ca3ee298658fed9c6c53575bbf245aa490a4 Mon Sep 17 00:00:00 2001 From: Dan Hermann Date: Thu, 10 Oct 2019 17:02:17 -0500 Subject: [PATCH 1/2] validate proxy base path at parse time --- .../client/RestClientBuilder.java | 10 ++++---- .../exporter/http/HttpExporter.java | 15 +++++++++++- .../exporter/http/HttpExporterTests.java | 23 +++++++++++++++++++ 3 files changed, 43 insertions(+), 5 deletions(-) diff --git a/client/rest/src/main/java/org/elasticsearch/client/RestClientBuilder.java b/client/rest/src/main/java/org/elasticsearch/client/RestClientBuilder.java index 3deb1c8f151f1..b5775ae53bac2 100644 --- a/client/rest/src/main/java/org/elasticsearch/client/RestClientBuilder.java +++ b/client/rest/src/main/java/org/elasticsearch/client/RestClientBuilder.java @@ -135,6 +135,11 @@ public RestClientBuilder setRequestConfigCallback(RequestConfigCallback requestC * @throws IllegalArgumentException if {@code pathPrefix} is empty, or ends with more than one '/'. */ public RestClientBuilder setPathPrefix(String pathPrefix) { + this.pathPrefix = cleanPathPrefix(pathPrefix); + return this; + } + + public static String cleanPathPrefix(String pathPrefix) { Objects.requireNonNull(pathPrefix, "pathPrefix must not be null"); if (pathPrefix.isEmpty()) { @@ -154,10 +159,7 @@ public RestClientBuilder setPathPrefix(String pathPrefix) { throw new IllegalArgumentException("pathPrefix is malformed. too many trailing slashes: [" + pathPrefix + "]"); } } - - - this.pathPrefix = cleanPathPrefix; - return this; + return cleanPathPrefix; } /** 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..2ef8c2db9cffa 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 @@ -197,7 +197,20 @@ public Iterator> settings() { */ public static final Setting.AffixSetting PROXY_BASE_PATH_SETTING = Setting.affixKeySetting("xpack.monitoring.exporters.","proxy.base_path", - (key) -> Setting.simpleString(key, Property.Dynamic, Property.NodeScope)); + (key) -> Setting.simpleString( + key, + value -> { + if (Strings.isNullOrEmpty(value) == false) { + try { + RestClientBuilder.cleanPathPrefix(value); + } catch (IllegalArgumentException e) { + Setting concreteSetting = HttpExporter.PROXY_BASE_PATH_SETTING.getConcreteSetting(key); + throw new SettingsException("[" + concreteSetting.getKey() + "] is malformed [" + value + "]", e); + } + } + }, + Property.Dynamic, + Property.NodeScope)); /** * A boolean setting to enable or disable sniffing for extra connections. */ 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..79a5da518e7c1 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 @@ -279,6 +279,29 @@ public void testExporterWithHostOnly() throws Exception { new HttpExporter(config, sslService, threadContext).close(); } + public void testExporterWithInvalidProxyBasePath() throws Exception { + final String prefix = "xpack.monitoring.exporters._http"; + final String settingName = ".proxy.base_path"; + final String settingValue = "z//"; + final String expected = "[" + prefix + settingName + "] is malformed [" + settingValue + "]"; + final Settings settings = Settings.builder() + .put(prefix + ".type", HttpExporter.TYPE) + .put(prefix + ".host", "localhost:9200") + .put(prefix + settingName, settingValue) + .build(); + + final IllegalArgumentException e = expectThrows( + IllegalArgumentException.class, + () -> HttpExporter.PROXY_BASE_PATH_SETTING.getConcreteSetting(prefix + settingName).get(settings)); + assertThat( + e, + hasToString( + containsString("Failed to parse value [" + settingValue + "] for setting [" + prefix + settingName + "]"))); + + assertThat(e.getCause(), instanceOf(SettingsException.class)); + assertThat(e.getCause(), hasToString(containsString(expected))); + } + public void testCreateRestClient() throws IOException { final SSLIOSessionStrategy sslStrategy = mock(SSLIOSessionStrategy.class); From c4dd6eaca17c8a0369cc63b2964a6fb1fba7410b Mon Sep 17 00:00:00 2001 From: Dan Hermann Date: Thu, 17 Oct 2019 12:22:30 -0500 Subject: [PATCH 2/2] address code review comments --- .../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 2ef8c2db9cffa..6963cd2fab14c 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 @@ -203,7 +203,7 @@ public Iterator> settings() { if (Strings.isNullOrEmpty(value) == false) { try { RestClientBuilder.cleanPathPrefix(value); - } catch (IllegalArgumentException e) { + } catch (RuntimeException e) { Setting concreteSetting = HttpExporter.PROXY_BASE_PATH_SETTING.getConcreteSetting(key); throw new SettingsException("[" + concreteSetting.getKey() + "] is malformed [" + value + "]", e); }