From 0a525d47075eade67373018d5da46564f49c0d45 Mon Sep 17 00:00:00 2001 From: Alexander Reelsen Date: Thu, 22 Mar 2018 11:00:27 +0100 Subject: [PATCH] Watcher: Hide credentials/secret data of integrations in toXContent (elastic/x-pack-elasticsearch#4162) If a user specifies an 'Authorization' header in an HTTPRequest we, which might be needed instead of using HTTP Basic Auth due to using Bearer Authentication, then in case of an failure, the request gets logged including that Authorization header. In addition, each implementation of a sent message for jira/hipchat/slack filters out special fields when a HTTP request is written in case of a failed response in order to not leak secret data. Relates elastic/x-pack-elasticsearch#3800 Original commit: elastic/x-pack-elasticsearch@66efdd9b36a73bf7e5841480e8990d6ac9f1850c --- .../watcher/common/http/HttpRequest.java | 50 ++++++++++++++--- .../notification/hipchat/SentMessages.java | 14 ++++- .../notification/pagerduty/SentEvent.java | 18 +++++- .../notification/slack/SentMessages.java | 15 ++++- .../watcher/common/http/HttpRequestTests.java | 20 ++++++- .../hipchat/HipChatMessageTests.java | 42 ++++++++++++++ .../hipchat/IntegrationAccountTests.java | 18 ++++-- .../pagerduty/SentEventTests.java | 56 +++++++++++++++++++ .../slack/message/SlackMessageTests.java | 43 ++++++++++++++ 9 files changed, 256 insertions(+), 20 deletions(-) create mode 100644 plugin/watcher/src/test/java/org/elasticsearch/xpack/watcher/notification/pagerduty/SentEventTests.java diff --git a/plugin/watcher/src/main/java/org/elasticsearch/xpack/watcher/common/http/HttpRequest.java b/plugin/watcher/src/main/java/org/elasticsearch/xpack/watcher/common/http/HttpRequest.java index b2ef351181c..7d9e91384e5 100644 --- a/plugin/watcher/src/main/java/org/elasticsearch/xpack/watcher/common/http/HttpRequest.java +++ b/plugin/watcher/src/main/java/org/elasticsearch/xpack/watcher/common/http/HttpRequest.java @@ -12,21 +12,28 @@ import org.elasticsearch.common.Strings; import org.elasticsearch.common.unit.TimeValue; import org.elasticsearch.common.xcontent.ToXContent; import org.elasticsearch.common.xcontent.ToXContentObject; +import org.elasticsearch.common.xcontent.XContent; import org.elasticsearch.common.xcontent.XContentBuilder; import org.elasticsearch.common.xcontent.XContentParser; import org.elasticsearch.common.xcontent.XContentType; import org.elasticsearch.rest.RestUtils; import org.elasticsearch.xpack.core.watcher.support.WatcherDateTimeUtils; import org.elasticsearch.xpack.core.watcher.support.WatcherUtils; +import org.elasticsearch.xpack.core.watcher.support.xcontent.WatcherParams; +import org.elasticsearch.xpack.core.watcher.support.xcontent.WatcherXContentParser; import org.elasticsearch.xpack.watcher.common.http.auth.HttpAuth; import org.elasticsearch.xpack.watcher.common.http.auth.HttpAuthRegistry; +import java.io.ByteArrayInputStream; +import java.io.ByteArrayOutputStream; import java.io.IOException; +import java.io.InputStream; import java.io.UnsupportedEncodingException; import java.net.URI; import java.net.URISyntaxException; import java.net.URLDecoder; import java.net.URLEncoder; +import java.util.Collections; import java.util.HashMap; import java.util.Map; import java.util.Objects; @@ -136,7 +143,7 @@ public class HttpRequest implements ToXContentObject { } @Override - public XContentBuilder toXContent(XContentBuilder builder, ToXContent.Params params) throws IOException { + public XContentBuilder toXContent(XContentBuilder builder, ToXContent.Params toXContentParams) throws IOException { builder.startObject(); builder.field(Field.HOST.getPreferredName(), host); builder.field(Field.PORT.getPreferredName(), port); @@ -145,15 +152,21 @@ public class HttpRequest implements ToXContentObject { if (path != null) { builder.field(Field.PATH.getPreferredName(), path); } - if (!this.params.isEmpty()) { + if (this.params.isEmpty() == false) { builder.field(Field.PARAMS.getPreferredName(), this.params); } - if (!headers.isEmpty()) { - builder.field(Field.HEADERS.getPreferredName(), headers); + if (headers.isEmpty() == false) { + if (WatcherParams.hideSecrets(toXContentParams) && headers.containsKey("Authorization")) { + Map sanitizedHeaders = new HashMap<>(headers); + sanitizedHeaders.put("Authorization", WatcherXContentParser.REDACTED_PASSWORD); + builder.field(Field.HEADERS.getPreferredName(), sanitizedHeaders); + } else { + builder.field(Field.HEADERS.getPreferredName(), headers); + } } if (auth != null) { builder.startObject(Field.AUTH.getPreferredName()) - .field(auth.type(), auth, params) + .field(auth.type(), auth, toXContentParams) .endObject(); } if (body != null) { @@ -168,7 +181,7 @@ public class HttpRequest implements ToXContentObject { HttpRequest.Field.READ_TIMEOUT_HUMAN.getPreferredName(), readTimeout); } if (proxy != null) { - proxy.toXContent(builder, params); + proxy.toXContent(builder, toXContentParams); } return builder.endObject(); } @@ -438,8 +451,8 @@ public class HttpRequest implements ToXContentObject { } public HttpRequest build() { - HttpRequest request = new HttpRequest(host, port, scheme, method, path, unmodifiableMap(params), unmodifiableMap(headers), - auth, body, connectionTimeout, readTimeout, proxy); + HttpRequest request = new HttpRequest(host, port, scheme, method, path, unmodifiableMap(params), + unmodifiableMap(headers), auth, body, connectionTimeout, readTimeout, proxy); params = null; headers = null; return request; @@ -489,4 +502,25 @@ public class HttpRequest implements ToXContentObject { ParseField PROXY = new ParseField("proxy"); ParseField URL = new ParseField("url"); } + + /** + * Write a request via toXContent, but filter certain parts of it - this is needed to not expose secrets + * + * @param request The HttpRequest object to serialize + * @param xContent The xContent from the parent outputstream builder + * @param params The ToXContentParams from the parent write + * @param excludeField The field to exclude + * @return A bytearrayinputstream that contains the serialized request + * @throws IOException + */ + public static InputStream filterToXContent(HttpRequest request, XContent xContent, ToXContent.Params params, + String excludeField) throws IOException { + try (ByteArrayOutputStream bos = new ByteArrayOutputStream(); + XContentBuilder filteredBuilder = new XContentBuilder(xContent, bos, + Collections.emptySet(), Collections.singleton(excludeField))) { + request.toXContent(filteredBuilder, params); + filteredBuilder.flush(); + return new ByteArrayInputStream(bos.toByteArray()); + } + } } diff --git a/plugin/watcher/src/main/java/org/elasticsearch/xpack/watcher/notification/hipchat/SentMessages.java b/plugin/watcher/src/main/java/org/elasticsearch/xpack/watcher/notification/hipchat/SentMessages.java index 9db59cee019..ed05c4fe5ad 100644 --- a/plugin/watcher/src/main/java/org/elasticsearch/xpack/watcher/notification/hipchat/SentMessages.java +++ b/plugin/watcher/src/main/java/org/elasticsearch/xpack/watcher/notification/hipchat/SentMessages.java @@ -10,10 +10,12 @@ import org.elasticsearch.common.Nullable; import org.elasticsearch.common.ParseField; import org.elasticsearch.common.xcontent.ToXContentObject; import org.elasticsearch.common.xcontent.XContentBuilder; +import org.elasticsearch.xpack.core.watcher.support.xcontent.WatcherParams; import org.elasticsearch.xpack.watcher.common.http.HttpRequest; import org.elasticsearch.xpack.watcher.common.http.HttpResponse; import java.io.IOException; +import java.io.InputStream; import java.util.Collections; import java.util.Iterator; import java.util.List; @@ -123,8 +125,16 @@ public class SentMessages implements ToXContentObject, Iterable E randomFromWithExcludes(E[] values, E... exclude) { diff --git a/plugin/watcher/src/test/java/org/elasticsearch/xpack/watcher/notification/hipchat/IntegrationAccountTests.java b/plugin/watcher/src/test/java/org/elasticsearch/xpack/watcher/notification/hipchat/IntegrationAccountTests.java index e3d590dc8dd..b85348d7810 100644 --- a/plugin/watcher/src/test/java/org/elasticsearch/xpack/watcher/notification/hipchat/IntegrationAccountTests.java +++ b/plugin/watcher/src/test/java/org/elasticsearch/xpack/watcher/notification/hipchat/IntegrationAccountTests.java @@ -9,6 +9,8 @@ import org.apache.logging.log4j.Logger; import org.elasticsearch.common.Strings; import org.elasticsearch.common.settings.Settings; import org.elasticsearch.common.settings.SettingsException; +import org.elasticsearch.common.xcontent.ToXContent; +import org.elasticsearch.common.xcontent.XContentBuilder; import org.elasticsearch.test.ESTestCase; import org.elasticsearch.xpack.watcher.common.http.HttpClient; import org.elasticsearch.xpack.watcher.common.http.HttpMethod; @@ -16,9 +18,12 @@ import org.elasticsearch.xpack.watcher.common.http.HttpRequest; import org.elasticsearch.xpack.watcher.common.http.HttpResponse; import org.elasticsearch.xpack.watcher.common.http.Scheme; +import static org.elasticsearch.common.xcontent.XContentFactory.jsonBuilder; import static org.hamcrest.Matchers.containsString; import static org.hamcrest.Matchers.equalTo; +import static org.hamcrest.Matchers.hasSize; import static org.hamcrest.Matchers.is; +import static org.hamcrest.Matchers.not; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; @@ -113,12 +118,13 @@ public class IntegrationAccountTests extends ESTestCase { } public void testSend() throws Exception { + String token = randomAlphaOfLength(10); HttpClient httpClient = mock(HttpClient.class); String room = "Room with Spaces"; IntegrationAccount account = new IntegrationAccount("_name", Settings.builder() .put("host", "_host") .put("port", "443") - .put("auth_token", "_token") + .put("auth_token", token) .put("room", room) .build(), HipChatServer.DEFAULT, httpClient, mock(Logger.class)); @@ -133,7 +139,7 @@ public class IntegrationAccountTests extends ESTestCase { // url encoded already .path("/v2/room/Room+with+Spaces/notification") .setHeader("Content-Type", "application/json") - .setHeader("Authorization", "Bearer _token") + .setHeader("Authorization", "Bearer " + token) .body(Strings.toString((builder, params) -> { builder.field("message", message.body); if (message.format != null) { @@ -153,8 +159,12 @@ public class IntegrationAccountTests extends ESTestCase { when(res.status()).thenReturn(200); when(httpClient.execute(req)).thenReturn(res); - account.send(message, null); - + SentMessages sentMessages = account.send(message, null); verify(httpClient).execute(req); + assertThat(sentMessages.asList(), hasSize(1)); + try (XContentBuilder builder = jsonBuilder()) { + sentMessages.asList().get(0).toXContent(builder, ToXContent.EMPTY_PARAMS); + assertThat(Strings.toString(builder), not(containsString(token))); + } } } diff --git a/plugin/watcher/src/test/java/org/elasticsearch/xpack/watcher/notification/pagerduty/SentEventTests.java b/plugin/watcher/src/test/java/org/elasticsearch/xpack/watcher/notification/pagerduty/SentEventTests.java new file mode 100644 index 00000000000..bd1072ca7ac --- /dev/null +++ b/plugin/watcher/src/test/java/org/elasticsearch/xpack/watcher/notification/pagerduty/SentEventTests.java @@ -0,0 +1,56 @@ +/* + * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one + * or more contributor license agreements. Licensed under the Elastic License; + * you may not use this file except in compliance with the Elastic License. + */ +package org.elasticsearch.xpack.watcher.notification.pagerduty; + +import org.elasticsearch.common.Strings; +import org.elasticsearch.common.xcontent.DeprecationHandler; +import org.elasticsearch.common.xcontent.NamedXContentRegistry; +import org.elasticsearch.common.xcontent.ToXContent; +import org.elasticsearch.common.xcontent.XContentBuilder; +import org.elasticsearch.common.xcontent.XContentParser; +import org.elasticsearch.test.ESTestCase; +import org.elasticsearch.xpack.core.watcher.support.xcontent.WatcherParams; +import org.elasticsearch.xpack.watcher.common.http.HttpRequest; +import org.elasticsearch.xpack.watcher.common.http.HttpResponse; + +import static org.elasticsearch.common.xcontent.XContentFactory.jsonBuilder; +import static org.hamcrest.Matchers.containsString; +import static org.hamcrest.Matchers.not; + +public class SentEventTests extends ESTestCase { + + public void testToXContentBodyFiltering() throws Exception { + HttpResponse response = new HttpResponse(500); + String body = randomAlphaOfLength(20); + HttpRequest request = HttpRequest.builder("localhost", 1234).body(body).build(); + IncidentEvent incidentEvent = new IncidentEvent("description", "eventtype", null, null, null, null, false, null, null); + SentEvent sentEvent = SentEvent.responded(incidentEvent, request, response); + + try (XContentBuilder builder = jsonBuilder()) { + WatcherParams params = WatcherParams.builder().hideSecrets(false).build(); + sentEvent.toXContent(builder, params); + assertThat(Strings.toString(builder), containsString(body)); + + try (XContentParser parser = builder.contentType().xContent() + .createParser(NamedXContentRegistry.EMPTY, DeprecationHandler.THROW_UNSUPPORTED_OPERATION, + Strings.toString(builder))) { + parser.map(); + } + } + try (XContentBuilder builder = jsonBuilder()) { + sentEvent.toXContent(builder, ToXContent.EMPTY_PARAMS); + assertThat(Strings.toString(builder), not(containsString(body))); + + try (XContentParser parser = builder.contentType().xContent() + .createParser(NamedXContentRegistry.EMPTY, DeprecationHandler.THROW_UNSUPPORTED_OPERATION, + Strings.toString(builder))) { + parser.map(); + } + } + + } + +} \ No newline at end of file diff --git a/plugin/watcher/src/test/java/org/elasticsearch/xpack/watcher/notification/slack/message/SlackMessageTests.java b/plugin/watcher/src/test/java/org/elasticsearch/xpack/watcher/notification/slack/message/SlackMessageTests.java index 5fda793b404..4075bd3dadd 100644 --- a/plugin/watcher/src/test/java/org/elasticsearch/xpack/watcher/notification/slack/message/SlackMessageTests.java +++ b/plugin/watcher/src/test/java/org/elasticsearch/xpack/watcher/notification/slack/message/SlackMessageTests.java @@ -5,13 +5,22 @@ */ package org.elasticsearch.xpack.watcher.notification.slack.message; +import org.elasticsearch.common.Strings; import org.elasticsearch.common.settings.Settings; +import org.elasticsearch.common.xcontent.DeprecationHandler; +import org.elasticsearch.common.xcontent.NamedXContentRegistry; import org.elasticsearch.common.xcontent.ToXContent; +import org.elasticsearch.common.xcontent.XContent; import org.elasticsearch.common.xcontent.XContentBuilder; import org.elasticsearch.common.xcontent.XContentParser; +import org.elasticsearch.common.xcontent.XContentType; import org.elasticsearch.test.ESTestCase; +import org.elasticsearch.xpack.core.watcher.support.xcontent.WatcherParams; +import org.elasticsearch.xpack.watcher.common.http.HttpRequest; +import org.elasticsearch.xpack.watcher.common.http.HttpResponse; import org.elasticsearch.xpack.watcher.common.text.TextTemplate; import org.elasticsearch.xpack.watcher.common.text.TextTemplateEngine; +import org.elasticsearch.xpack.watcher.notification.slack.SentMessages; import org.elasticsearch.xpack.watcher.test.MockTextTemplateEngine; import java.io.IOException; @@ -25,8 +34,10 @@ import static org.hamcrest.CoreMatchers.notNullValue; import static org.hamcrest.CoreMatchers.nullValue; import static org.hamcrest.Matchers.arrayContaining; import static org.hamcrest.Matchers.arrayContainingInAnyOrder; +import static org.hamcrest.Matchers.containsString; import static org.hamcrest.Matchers.equalTo; import static org.hamcrest.Matchers.is; +import static org.hamcrest.Matchers.not; public class SlackMessageTests extends ESTestCase { @@ -559,6 +570,38 @@ public class SlackMessageTests extends ESTestCase { } } + // the url path contains sensitive information, which should not be exposed + public void testUrlPathIsFiltered() throws Exception { + HttpResponse response = new HttpResponse(500); + String path = randomAlphaOfLength(20); + HttpRequest request = HttpRequest.builder("localhost", 1234).path(path).build(); + SlackMessage slackMessage = new SlackMessage("from", new String[] {"to"}, "icon", "text", null); + SentMessages sentMessages = new SentMessages("foo", + Arrays.asList(SentMessages.SentMessage.responded("recipient", slackMessage, request, response))); + + try (XContentBuilder builder = jsonBuilder()) { + WatcherParams params = WatcherParams.builder().hideSecrets(false).build(); + sentMessages.toXContent(builder, params); + assertThat(Strings.toString(builder), containsString(path)); + + try (XContentParser parser = builder.contentType().xContent() + .createParser(NamedXContentRegistry.EMPTY, DeprecationHandler.THROW_UNSUPPORTED_OPERATION, + Strings.toString(builder))) { + parser.map(); + } + } + try (XContentBuilder builder = jsonBuilder()) { + sentMessages.toXContent(builder, ToXContent.EMPTY_PARAMS); + assertThat(Strings.toString(builder), not(containsString(path))); + + try (XContentParser parser = builder.contentType().xContent() + .createParser(NamedXContentRegistry.EMPTY, DeprecationHandler.THROW_UNSUPPORTED_OPERATION, + Strings.toString(builder))) { + parser.map(); + } + } + } + private static void writeFieldIfNotNull(XContentBuilder builder, String field, Object value) throws IOException { if (value != null) { builder.field(field, value);