From c85cf7a6de3d59ad33dcb4873f617e18e696eb94 Mon Sep 17 00:00:00 2001 From: Dan Hermann Date: Mon, 4 Nov 2019 09:51:13 -0600 Subject: [PATCH] Validate proxy base path at parse time (#47912) (#48825) --- .../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 3deb1c8f151..b5775ae53ba 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 final class RestClientBuilder { * @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 final class RestClientBuilder { 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 3d6b315ea58..9c1f09f4534 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 @@ -236,7 +236,20 @@ public class HttpExporter extends Exporter { */ 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 (RuntimeException 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 24d2fc0d47e..3325c7190b0 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 @@ -302,6 +302,29 @@ public class HttpExporterTests extends ESTestCase { 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);