From badc83d368f7c8a67c76ebcf1ea2f5659dda05c4 Mon Sep 17 00:00:00 2001 From: cbono Date: Fri, 31 Jan 2020 23:06:48 -0600 Subject: [PATCH] Add 'uris', 'address' and 'addresses' to keys to sanitize. See gh-19999 --- .../boot/actuate/endpoint/Sanitizer.java | 35 ++++++++-- ...gurationPropertiesReportEndpointTests.java | 23 ++++++- .../boot/actuate/endpoint/SanitizerTests.java | 69 ++++++++++++++++--- .../actuate/env/EnvironmentEndpointTests.java | 11 ++- 4 files changed, 117 insertions(+), 21 deletions(-) diff --git a/spring-boot-project/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/endpoint/Sanitizer.java b/spring-boot-project/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/endpoint/Sanitizer.java index 7cb865f0523..550280d0173 100644 --- a/spring-boot-project/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/endpoint/Sanitizer.java +++ b/spring-boot-project/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/endpoint/Sanitizer.java @@ -16,8 +16,10 @@ package org.springframework.boot.actuate.endpoint; +import java.util.Arrays; import java.util.regex.Matcher; import java.util.regex.Pattern; +import java.util.stream.Collectors; import org.springframework.util.Assert; import org.springframework.util.StringUtils; @@ -32,18 +34,23 @@ import org.springframework.util.StringUtils; * @author Nicolas Lejeune * @author Stephane Nicoll * @author HaiTao Zhang + * @author Chris Bono * @since 2.0.0 */ public class Sanitizer { private static final String[] REGEX_PARTS = { "*", "$", "^", "+" }; + private static final String[] DEFAULT_KEYS_TO_SANITIZE = { "password", "secret", "key", "token", ".*credentials.*", "vcap_services", "sun.java.command", "uri", "uris", "address", "addresses" }; + + private static final String[] URI_USERINFO_KEYS = { "uri", "uris", "address", "addresses" }; + private static final Pattern URI_USERINFO_PATTERN = Pattern.compile("[A-Za-z]+://.+:(.*)@.+$"); private Pattern[] keysToSanitize; public Sanitizer() { - this("password", "secret", "key", "token", ".*credentials.*", "vcap_services", "sun.java.command", "uri"); + this(DEFAULT_KEYS_TO_SANITIZE); } public Sanitizer(String... keysToSanitize) { @@ -91,8 +98,8 @@ public class Sanitizer { } for (Pattern pattern : this.keysToSanitize) { if (pattern.matcher(key).matches()) { - if (pattern.matcher("uri").matches()) { - return sanitizeUri(value); + if (keyIsUriWithUserInfo(pattern)) { + return sanitizeUris(value.toString()); } return "******"; } @@ -100,14 +107,28 @@ public class Sanitizer { return value; } - private Object sanitizeUri(Object value) { - String uriString = value.toString(); + private boolean keyIsUriWithUserInfo(Pattern pattern) { + for (String uriKey : URI_USERINFO_KEYS) { + if (pattern.matcher(uriKey).matches()) { + return true; + } + } + return false; + } + + private Object sanitizeUris(String uriString) { + // Treat each uri value as possibly containing multiple uris (comma separated) + return Arrays.stream(uriString.split(",")) + .map(this::sanitizeUri) + .collect(Collectors.joining(",")); + } + + private String sanitizeUri(String uriString) { Matcher matcher = URI_USERINFO_PATTERN.matcher(uriString); String password = matcher.matches() ? matcher.group(1) : null; if (password != null) { return StringUtils.replace(uriString, ":" + password + "@", ":******@"); } - return value; + return uriString; } - } diff --git a/spring-boot-project/spring-boot-actuator/src/test/java/org/springframework/boot/actuate/context/properties/ConfigurationPropertiesReportEndpointTests.java b/spring-boot-project/spring-boot-actuator/src/test/java/org/springframework/boot/actuate/context/properties/ConfigurationPropertiesReportEndpointTests.java index db9e661e0d7..1620f03a5b0 100644 --- a/spring-boot-project/spring-boot-actuator/src/test/java/org/springframework/boot/actuate/context/properties/ConfigurationPropertiesReportEndpointTests.java +++ b/spring-boot-project/spring-boot-actuator/src/test/java/org/springframework/boot/actuate/context/properties/ConfigurationPropertiesReportEndpointTests.java @@ -19,6 +19,7 @@ package org.springframework.boot.actuate.context.properties; import java.net.URI; import java.time.Duration; import java.util.ArrayList; +import java.util.Arrays; import java.util.Collections; import java.util.HashMap; import java.util.List; @@ -51,6 +52,7 @@ import static org.assertj.core.api.Assertions.entry; * @author Andy Wilkinson * @author Stephane Nicoll * @author HaiTao Zhang + * @author Chris Bono */ class ConfigurationPropertiesReportEndpointTests { @@ -170,19 +172,26 @@ class ConfigurationPropertiesReportEndpointTests { } @Test - void sanitizedUriWithSensitiveInfo() { + void sanitizeUriWithSensitiveInfo() { this.contextRunner.withUserConfiguration(SensiblePropertiesConfiguration.class) .run(assertProperties("sensible", (properties) -> assertThat(properties.get("sensitiveUri")) .isEqualTo("http://user:******@localhost:8080"))); } @Test - void sanitizedUriWithNoPassword() { + void sanitizeUriWithNoPassword() { this.contextRunner.withUserConfiguration(SensiblePropertiesConfiguration.class) .run(assertProperties("sensible", (properties) -> assertThat(properties.get("noPasswordUri")) .isEqualTo("http://user:******@localhost:8080"))); } + @Test + void sanitizeAddressesFieldContainingMultipleRawSensitiveUris() { + this.contextRunner.withUserConfiguration(SensiblePropertiesConfiguration.class) + .run(assertProperties("sensible", (properties) -> assertThat(properties.get("rawSensitiveAddresses")) + .isEqualTo("http://user:******@localhost:8080,http://user2:******@localhost:8082"))); + } + @Test void sanitizeLists() { this.contextRunner.withUserConfiguration(SensiblePropertiesConfiguration.class) @@ -574,6 +583,8 @@ class ConfigurationPropertiesReportEndpointTests { private URI noPasswordUri = URI.create("http://user:@localhost:8080"); + private String rawSensitiveAddresses = "http://user:password@localhost:8080,http://user2:password2@localhost:8082"; + private List listItems = new ArrayList<>(); private List> listOfListItems = new ArrayList<>(); @@ -599,6 +610,14 @@ class ConfigurationPropertiesReportEndpointTests { return this.noPasswordUri; } + public String getRawSensitiveAddresses() { + return this.rawSensitiveAddresses; + } + + public void setRawSensitiveAddresses(final String rawSensitiveAddresses) { + this.rawSensitiveAddresses = rawSensitiveAddresses; + } + public List getListItems() { return this.listItems; } diff --git a/spring-boot-project/spring-boot-actuator/src/test/java/org/springframework/boot/actuate/endpoint/SanitizerTests.java b/spring-boot-project/spring-boot-actuator/src/test/java/org/springframework/boot/actuate/endpoint/SanitizerTests.java index deeab48b575..ff9b369d7af 100644 --- a/spring-boot-project/spring-boot-actuator/src/test/java/org/springframework/boot/actuate/endpoint/SanitizerTests.java +++ b/spring-boot-project/spring-boot-actuator/src/test/java/org/springframework/boot/actuate/endpoint/SanitizerTests.java @@ -17,6 +17,10 @@ package org.springframework.boot.actuate.endpoint; import org.junit.jupiter.api.Test; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.MethodSource; + +import java.util.stream.Stream; import static org.assertj.core.api.Assertions.assertThat; @@ -25,11 +29,12 @@ import static org.assertj.core.api.Assertions.assertThat; * * @author Phillip Webb * @author Stephane Nicoll + * @author Chris Bono */ class SanitizerTests { @Test - void defaults() { + void defaultNonUriKeys() { Sanitizer sanitizer = new Sanitizer(); assertThat(sanitizer.sanitize("password", "secret")).isEqualTo("******"); assertThat(sanitizer.sanitize("my-password", "secret")).isEqualTo("******"); @@ -40,21 +45,64 @@ class SanitizerTests { assertThat(sanitizer.sanitize("sometoken", "secret")).isEqualTo("******"); assertThat(sanitizer.sanitize("find", "secret")).isEqualTo("secret"); assertThat(sanitizer.sanitize("sun.java.command", "--spring.redis.password=pa55w0rd")).isEqualTo("******"); - assertThat(sanitizer.sanitize("my.uri", "http://user:password@localhost:8080")) - .isEqualTo("http://user:******@localhost:8080"); } - @Test - void uriWithNoPasswordShouldNotBeSanitized() { + @ParameterizedTest(name = "key = {0}") + @MethodSource("matchingUriUserInfoKeys") + void uriWithSingleEntryWithPasswordShouldBeSanitized(String key) { Sanitizer sanitizer = new Sanitizer(); - assertThat(sanitizer.sanitize("my.uri", "http://localhost:8080")).isEqualTo("http://localhost:8080"); + assertThat(sanitizer.sanitize(key, "http://user:password@localhost:8080")).isEqualTo("http://user:******@localhost:8080"); } - @Test - void uriWithPasswordMatchingOtherPartsOfString() { + @ParameterizedTest(name = "key = {0}") + @MethodSource("matchingUriUserInfoKeys") + void uriWithSingleEntryWithNoPasswordShouldNotBeSanitized(String key) { + Sanitizer sanitizer = new Sanitizer(); + assertThat(sanitizer.sanitize(key, "http://localhost:8080")).isEqualTo("http://localhost:8080"); + assertThat(sanitizer.sanitize(key, "http://user@localhost:8080")).isEqualTo("http://user@localhost:8080"); + } + + @ParameterizedTest(name = "key = {0}") + @MethodSource("matchingUriUserInfoKeys") + void uriWithSingleEntryWithPasswordMatchingOtherPartsOfStringShouldBeSanitized(String key) { Sanitizer sanitizer = new Sanitizer(); - assertThat(sanitizer.sanitize("my.uri", "http://user://@localhost:8080")) - .isEqualTo("http://user:******@localhost:8080"); + assertThat(sanitizer.sanitize(key, "http://user://@localhost:8080")).isEqualTo("http://user:******@localhost:8080"); + } + + @ParameterizedTest(name = "key = {0}") + @MethodSource("matchingUriUserInfoKeys") + void uriWithMultipleEntriesEachWithPasswordShouldHaveAllSanitized(String key) { + Sanitizer sanitizer = new Sanitizer(); + assertThat(sanitizer.sanitize(key, "http://user1:password1@localhost:8080,http://user2:password2@localhost:8082")) + .isEqualTo("http://user1:******@localhost:8080,http://user2:******@localhost:8082"); + } + + @ParameterizedTest(name = "key = {0}") + @MethodSource("matchingUriUserInfoKeys") + void uriWithMultipleEntriesNoneWithPasswordShouldHaveNoneSanitized(String key) { + Sanitizer sanitizer = new Sanitizer(); + assertThat(sanitizer.sanitize(key, "http://user@localhost:8080,http://localhost:8082")) + .isEqualTo("http://user@localhost:8080,http://localhost:8082"); + } + + @ParameterizedTest(name = "key = {0}") + @MethodSource("matchingUriUserInfoKeys") + void uriWithMultipleEntriesSomeWithPasswordShouldHaveThoseSanitized(String key) { + Sanitizer sanitizer = new Sanitizer(); + assertThat(sanitizer.sanitize(key, "http://user1:password1@localhost:8080,http://user2@localhost:8082,http://localhost:8083")) + .isEqualTo("http://user1:******@localhost:8080,http://user2@localhost:8082,http://localhost:8083"); + } + + @ParameterizedTest(name = "key = {0}") + @MethodSource("matchingUriUserInfoKeys") + void uriWithMultipleEntriesWithPasswordMatchingOtherPartsOfStringShouldBeSanitized(String key) { + Sanitizer sanitizer = new Sanitizer(); + assertThat(sanitizer.sanitize(key, "http://user1://@localhost:8080,http://user2://@localhost:8082")) + .isEqualTo("http://user1:******@localhost:8080,http://user2:******@localhost:8082"); + } + + static private Stream matchingUriUserInfoKeys() { + return Stream.of("uri", "my.uri", "myuri", "uris", "my.uris", "myuris", "address", "my.address", "myaddress", "addresses", "my.addresses", "myaddresses"); } @Test @@ -63,5 +111,4 @@ class SanitizerTests { assertThat(sanitizer.sanitize("verylOCkish", "secret")).isEqualTo("******"); assertThat(sanitizer.sanitize("veryokish", "secret")).isEqualTo("secret"); } - } diff --git a/spring-boot-project/spring-boot-actuator/src/test/java/org/springframework/boot/actuate/env/EnvironmentEndpointTests.java b/spring-boot-project/spring-boot-actuator/src/test/java/org/springframework/boot/actuate/env/EnvironmentEndpointTests.java index ecaa47813cb..fe4fd3beb74 100644 --- a/spring-boot-project/spring-boot-actuator/src/test/java/org/springframework/boot/actuate/env/EnvironmentEndpointTests.java +++ b/spring-boot-project/spring-boot-actuator/src/test/java/org/springframework/boot/actuate/env/EnvironmentEndpointTests.java @@ -50,6 +50,7 @@ import static org.assertj.core.api.Assertions.assertThat; * @author Madhura Bhave * @author Andy Wilkinson * @author HaiTao Zhang + * @author Chris Bono */ class EnvironmentEndpointTests { @@ -246,13 +247,21 @@ class EnvironmentEndpointTests { } @Test - void uriPropertryWithSensitiveInfo() { + void uriPropertyWithSensitiveInfo() { ConfigurableEnvironment environment = new StandardEnvironment(); TestPropertyValues.of("sensitive.uri=http://user:password@localhost:8080").applyTo(environment); EnvironmentEntryDescriptor descriptor = new EnvironmentEndpoint(environment).environmentEntry("sensitive.uri"); assertThat(descriptor.getProperty().getValue()).isEqualTo("http://user:******@localhost:8080"); } + @Test + void addressesPropertyWithMultipleEntriesEachWithSensitiveInfo() { + ConfigurableEnvironment environment = new StandardEnvironment(); + TestPropertyValues.of("sensitive.addresses=http://user:password@localhost:8080,http://user2:password2@localhost:8082").applyTo(environment); + EnvironmentEntryDescriptor descriptor = new EnvironmentEndpoint(environment).environmentEntry("sensitive.addresses"); + assertThat(descriptor.getProperty().getValue()).isEqualTo("http://user:******@localhost:8080,http://user2:******@localhost:8082"); + } + private static ConfigurableEnvironment emptyEnvironment() { StandardEnvironment environment = new StandardEnvironment(); environment.getPropertySources().remove(StandardEnvironment.SYSTEM_ENVIRONMENT_PROPERTY_SOURCE_NAME);