From 96757f0c58432f982976340c0622b17c3e10d124 Mon Sep 17 00:00:00 2001 From: jean-philippe bempel Date: Thu, 2 Apr 2020 17:54:02 +0200 Subject: [PATCH 1/4] Remove sensitive information from debug log Config.toString() method is dumped when logging in debug the conf. It includes in some case the profile api key when used with env vars. Also proxy password is also dumped. toString method generated by Lombok now excludes both fields --- .../src/main/java/datadog/trace/api/Config.java | 2 +- .../test/groovy/datadog/trace/api/ConfigTest.groovy | 13 +++++++++++++ 2 files changed, 14 insertions(+), 1 deletion(-) diff --git a/dd-trace-api/src/main/java/datadog/trace/api/Config.java b/dd-trace-api/src/main/java/datadog/trace/api/Config.java index e9b6e9c816..72ed152750 100644 --- a/dd-trace-api/src/main/java/datadog/trace/api/Config.java +++ b/dd-trace-api/src/main/java/datadog/trace/api/Config.java @@ -38,7 +38,7 @@ import lombok.extern.slf4j.Slf4j; * system property, but uppercased with '.' -> '_'. */ @Slf4j -@ToString(includeFieldNames = true) +@ToString(includeFieldNames = true, exclude = {"profilingApiKey", "profilingProxyPassword"}) public class Config { /** Config keys below */ private static final String PREFIX = "dd."; diff --git a/dd-trace-api/src/test/groovy/datadog/trace/api/ConfigTest.groovy b/dd-trace-api/src/test/groovy/datadog/trace/api/ConfigTest.groovy index db37f07a6a..82e29caab6 100644 --- a/dd-trace-api/src/test/groovy/datadog/trace/api/ConfigTest.groovy +++ b/dd-trace-api/src/test/groovy/datadog/trace/api/ConfigTest.groovy @@ -406,6 +406,19 @@ class ConfigTest extends DDSpecification { config.profilingApiKey == "test-api-key" } + def "sensitive information removed for toString/debug log"() { + setup: + environmentVariables.set(DD_PROFILING_API_KEY_ENV, "test-secret-api-key") + environmentVariables.set(PROFILING_PROXY_PASSWORD, "test-secret-proxy-password") + + when: + def config = new Config() + + then: + !config.toString().contains("test-secret-api-key") + !config.toString().contains("test-secret-proxy-password") + } + def "sys props override env vars"() { setup: environmentVariables.set(DD_SERVICE_NAME_ENV, "still something else") From 4d15d5ae968f0bf00df95acd7dc7b1f69bd97cf9 Mon Sep 17 00:00:00 2001 From: jean-philippe bempel Date: Thu, 2 Apr 2020 18:26:14 +0200 Subject: [PATCH 2/4] Sensor information instead of excluding the fields --- .../src/main/java/datadog/trace/api/Config.java | 10 +++++++++- .../test/groovy/datadog/trace/api/ConfigTest.groovy | 2 ++ 2 files changed, 11 insertions(+), 1 deletion(-) diff --git a/dd-trace-api/src/main/java/datadog/trace/api/Config.java b/dd-trace-api/src/main/java/datadog/trace/api/Config.java index 72ed152750..38fdc20eff 100644 --- a/dd-trace-api/src/main/java/datadog/trace/api/Config.java +++ b/dd-trace-api/src/main/java/datadog/trace/api/Config.java @@ -38,7 +38,7 @@ import lombok.extern.slf4j.Slf4j; * system property, but uppercased with '.' -> '_'. */ @Slf4j -@ToString(includeFieldNames = true, exclude = {"profilingApiKey", "profilingProxyPassword"}) +@ToString(includeFieldNames = true) public class Config { /** Config keys below */ private static final String PREFIX = "dd."; @@ -202,6 +202,14 @@ public class Config { /** A tag intended for internal use only, hence not added to the public api DDTags class. */ private static final String INTERNAL_HOST_NAME = "_dd.hostname"; + /** Used for masking sensitive information when doing toString */ + @ToString.Include(name = "profilingApiKey") + private String profilingApiKeyMasker() { return "****"; } + + /** Used for masking sensitive information when doing toString */ + @ToString.Include(name = "profilingProxyPassword") + private String profilingProxyPasswordMasker() { return "****"; } + /** * this is a random UUID that gets generated on JVM start up and is attached to every root span * and every JMX metric that is sent out. diff --git a/dd-trace-api/src/test/groovy/datadog/trace/api/ConfigTest.groovy b/dd-trace-api/src/test/groovy/datadog/trace/api/ConfigTest.groovy index 82e29caab6..5959849288 100644 --- a/dd-trace-api/src/test/groovy/datadog/trace/api/ConfigTest.groovy +++ b/dd-trace-api/src/test/groovy/datadog/trace/api/ConfigTest.groovy @@ -415,7 +415,9 @@ class ConfigTest extends DDSpecification { def config = new Config() then: + config.toString().contains("profilingApiKey=****"); !config.toString().contains("test-secret-api-key") + config.toString().contains("profilingProxyPassword=****"); !config.toString().contains("test-secret-proxy-password") } From 58ea15e590baca4aa5894cd3033b0c4894dcf759 Mon Sep 17 00:00:00 2001 From: jean-philippe bempel Date: Thu, 2 Apr 2020 19:17:26 +0200 Subject: [PATCH 3/4] if not populated returns null for toString --- dd-trace-api/src/main/java/datadog/trace/api/Config.java | 8 ++++++-- .../src/test/groovy/datadog/trace/api/ConfigTest.groovy | 5 ++++- 2 files changed, 10 insertions(+), 3 deletions(-) diff --git a/dd-trace-api/src/main/java/datadog/trace/api/Config.java b/dd-trace-api/src/main/java/datadog/trace/api/Config.java index 38fdc20eff..430b01adf0 100644 --- a/dd-trace-api/src/main/java/datadog/trace/api/Config.java +++ b/dd-trace-api/src/main/java/datadog/trace/api/Config.java @@ -204,11 +204,15 @@ public class Config { /** Used for masking sensitive information when doing toString */ @ToString.Include(name = "profilingApiKey") - private String profilingApiKeyMasker() { return "****"; } + private String profilingApiKeyMasker() { + return profilingApiKey != null ? "****" : null; + } /** Used for masking sensitive information when doing toString */ @ToString.Include(name = "profilingProxyPassword") - private String profilingProxyPasswordMasker() { return "****"; } + private String profilingProxyPasswordMasker() { + return profilingProxyPassword != null ? "****" : null; + } /** * this is a random UUID that gets generated on JVM start up and is attached to every root span diff --git a/dd-trace-api/src/test/groovy/datadog/trace/api/ConfigTest.groovy b/dd-trace-api/src/test/groovy/datadog/trace/api/ConfigTest.groovy index 5959849288..4eb3ef7ad3 100644 --- a/dd-trace-api/src/test/groovy/datadog/trace/api/ConfigTest.groovy +++ b/dd-trace-api/src/test/groovy/datadog/trace/api/ConfigTest.groovy @@ -93,6 +93,7 @@ class ConfigTest extends DDSpecification { private static final DD_PROFILING_API_KEY_ENV = "DD_PROFILING_API_KEY" private static final DD_PROFILING_API_KEY_OLD_ENV = "DD_PROFILING_APIKEY" private static final DD_PROFILING_TAGS_ENV = "DD_PROFILING_TAGS" + private static final DD_PROFILING_PROXY_PASSWORD_ENV = "DD_PROFILING_PROXY_PASSWORD" def "verify defaults"() { when: @@ -409,7 +410,7 @@ class ConfigTest extends DDSpecification { def "sensitive information removed for toString/debug log"() { setup: environmentVariables.set(DD_PROFILING_API_KEY_ENV, "test-secret-api-key") - environmentVariables.set(PROFILING_PROXY_PASSWORD, "test-secret-proxy-password") + environmentVariables.set(DD_PROFILING_PROXY_PASSWORD_ENV, "test-secret-proxy-password") when: def config = new Config() @@ -419,6 +420,8 @@ class ConfigTest extends DDSpecification { !config.toString().contains("test-secret-api-key") config.toString().contains("profilingProxyPassword=****"); !config.toString().contains("test-secret-proxy-password") + config.getProfilingApiKey() == "test-secret-api-key" + config.getProfilingProxyPassword() == "test-secret-proxy-password" } def "sys props override env vars"() { From e1f2052e25574d9007207bd0c1e407d6419087b6 Mon Sep 17 00:00:00 2001 From: Nikolay Martynov Date: Thu, 2 Apr 2020 16:03:03 -0400 Subject: [PATCH 4/4] Minor clean up and add one more test --- .../datadog/trace/api/ConfigTest.groovy | 43 +++++++++++-------- 1 file changed, 26 insertions(+), 17 deletions(-) diff --git a/dd-trace-api/src/test/groovy/datadog/trace/api/ConfigTest.groovy b/dd-trace-api/src/test/groovy/datadog/trace/api/ConfigTest.groovy index 4eb3ef7ad3..853552b7a9 100644 --- a/dd-trace-api/src/test/groovy/datadog/trace/api/ConfigTest.groovy +++ b/dd-trace-api/src/test/groovy/datadog/trace/api/ConfigTest.groovy @@ -407,23 +407,6 @@ class ConfigTest extends DDSpecification { config.profilingApiKey == "test-api-key" } - def "sensitive information removed for toString/debug log"() { - setup: - environmentVariables.set(DD_PROFILING_API_KEY_ENV, "test-secret-api-key") - environmentVariables.set(DD_PROFILING_PROXY_PASSWORD_ENV, "test-secret-proxy-password") - - when: - def config = new Config() - - then: - config.toString().contains("profilingApiKey=****"); - !config.toString().contains("test-secret-api-key") - config.toString().contains("profilingProxyPassword=****"); - !config.toString().contains("test-secret-proxy-password") - config.getProfilingApiKey() == "test-secret-api-key" - config.getProfilingProxyPassword() == "test-secret-proxy-password" - } - def "sys props override env vars"() { setup: environmentVariables.set(DD_SERVICE_NAME_ENV, "still something else") @@ -1123,4 +1106,30 @@ class ConfigTest extends DDSpecification { config.mergedProfilingTags == [a: "1", f: "6", (HOST_TAG): config.getHostName(), (RUNTIME_ID_TAG): config.getRuntimeId(), (SERVICE_TAG): config.serviceName, (LANGUAGE_TAG_KEY): LANGUAGE_TAG_VALUE] } + + def "sensitive information removed for toString/debug log"() { + setup: + environmentVariables.set(DD_PROFILING_API_KEY_ENV, "test-secret-api-key") + environmentVariables.set(DD_PROFILING_PROXY_PASSWORD_ENV, "test-secret-proxy-password") + + when: + def config = new Config() + + then: + config.toString().contains("profilingApiKey=****") + !config.toString().contains("test-secret-api-key") + config.toString().contains("profilingProxyPassword=****") + !config.toString().contains("test-secret-proxy-password") + config.profilingApiKey == "test-secret-api-key" + config.profilingProxyPassword == "test-secret-proxy-password" + } + + def "toString works when passwords are empty"() { + when: + def config = new Config() + + then: + config.toString().contains("profilingApiKey=null") + config.toString().contains("profilingProxyPassword=null") + } }