From c839a1c78291294563d153bfa5ca3e4bafd91186 Mon Sep 17 00:00:00 2001 From: Santiago Mola Date: Sun, 23 Oct 2022 00:43:18 +0200 Subject: [PATCH 1/4] Fix default for dd.appsec.enabled/DD_APPSEC_ENABLED Default is `inactive`, but on empty string it was falling back to `false`. --- .../java/datadog/trace/bootstrap/Agent.java | 4 ++ .../main/java/datadog/trace/api/Config.java | 4 ++ .../datadog/trace/api/ConfigTest.groovy | 63 +++++++++++++++++++ 3 files changed, 71 insertions(+) diff --git a/dd-java-agent/agent-bootstrap/src/main/java/datadog/trace/bootstrap/Agent.java b/dd-java-agent/agent-bootstrap/src/main/java/datadog/trace/bootstrap/Agent.java index 38d161b37f6..d2b90825281 100644 --- a/dd-java-agent/agent-bootstrap/src/main/java/datadog/trace/bootstrap/Agent.java +++ b/dd-java-agent/agent-bootstrap/src/main/java/datadog/trace/bootstrap/Agent.java @@ -897,6 +897,10 @@ private static boolean isAppSecFullyDisabled() { String settingValue = System.getProperty(featureEnabledSysprop); if (settingValue == null) { settingValue = ddGetEnv(featureEnabledSysprop); + // TODO: We may want this behavior for the system property too. + if (settingValue != null && settingValue.isEmpty()) { + settingValue = null; + } } // defaults to inactive diff --git a/internal-api/src/main/java/datadog/trace/api/Config.java b/internal-api/src/main/java/datadog/trace/api/Config.java index 61cbc31a491..905c1a30174 100644 --- a/internal-api/src/main/java/datadog/trace/api/Config.java +++ b/internal-api/src/main/java/datadog/trace/api/Config.java @@ -1099,6 +1099,10 @@ && isJavaVersionAtLeast(8) telemetryHeartbeatInterval = telemetryInterval; String appSecEnabled = configProvider.getString(APPSEC_ENABLED, DEFAULT_APPSEC_ENABLED); + if (appSecEnabled.isEmpty()) { + // ConfigProvider.getString currently doesn't fallback to default for empty strings. + appSecEnabled = DEFAULT_APPSEC_ENABLED; + } this.appSecEnabled = ProductActivationConfig.fromString(appSecEnabled); appSecReportingInband = configProvider.getBoolean(APPSEC_REPORTING_INBAND, DEFAULT_APPSEC_REPORTING_INBAND); diff --git a/internal-api/src/test/groovy/datadog/trace/api/ConfigTest.groovy b/internal-api/src/test/groovy/datadog/trace/api/ConfigTest.groovy index 0894f6be90d..1a4b70e1496 100644 --- a/internal-api/src/test/groovy/datadog/trace/api/ConfigTest.groovy +++ b/internal-api/src/test/groovy/datadog/trace/api/ConfigTest.groovy @@ -5,6 +5,7 @@ import datadog.trace.bootstrap.config.provider.ConfigConverter import datadog.trace.bootstrap.config.provider.ConfigProvider import datadog.trace.test.util.DDSpecification import org.junit.Rule +import spock.lang.Unroll import static datadog.trace.api.ConfigDefaults.DEFAULT_HTTP_CLIENT_ERROR_STATUSES import static datadog.trace.api.ConfigDefaults.DEFAULT_HTTP_SERVER_ERROR_STATUSES @@ -2035,6 +2036,68 @@ class ConfigTest extends DDSpecification { config.getMetricsIgnoredResources() == ["GET /healthcheck", "SELECT foo from bar"].toSet() } + @Unroll + def "appsec state with sys = #sys env = #env"() { + setup: + if (sys != null) { + System.setProperty("dd.appsec.enabled", sys) + } + if (env != null) { + environmentVariables.set("DD_APPSEC_ENABLED", env) + } + + when: + def config = new Config() + + then: + config.getAppSecEnabledConfig() == res + + where: + sys | env | res + null | null | ProductActivationConfig.ENABLED_INACTIVE + null | "" | ProductActivationConfig.ENABLED_INACTIVE + null | "inactive" | ProductActivationConfig.ENABLED_INACTIVE + null | "false" | ProductActivationConfig.FULLY_DISABLED + null | "0" | ProductActivationConfig.FULLY_DISABLED + null | "invalid" | ProductActivationConfig.FULLY_DISABLED + null | "true" | ProductActivationConfig.FULLY_ENABLED + null | "1" | ProductActivationConfig.FULLY_ENABLED + "" | null | ProductActivationConfig.ENABLED_INACTIVE + "" | "" | ProductActivationConfig.ENABLED_INACTIVE + "" | "inactive" | ProductActivationConfig.ENABLED_INACTIVE + "" | "false" | ProductActivationConfig.ENABLED_INACTIVE // FIXME: Should be FULLY_DISABLED? + "" | "0" | ProductActivationConfig.ENABLED_INACTIVE // FIXME: Should be FULLY_DISABLED? + "" | "invalid" | ProductActivationConfig.ENABLED_INACTIVE + "" | "true" | ProductActivationConfig.ENABLED_INACTIVE // FIXME: Should be FULLY_ENABLED? + "" | "1" | ProductActivationConfig.ENABLED_INACTIVE // FIXME: Should be FULLY_ENABLED? + "inactive" | null | ProductActivationConfig.ENABLED_INACTIVE + "inactive" | "" | ProductActivationConfig.ENABLED_INACTIVE + "inactive" | "inactive" | ProductActivationConfig.ENABLED_INACTIVE + "inactive" | "false" | ProductActivationConfig.ENABLED_INACTIVE + "inactive" | "0" | ProductActivationConfig.ENABLED_INACTIVE + "inactive" | "invalid" | ProductActivationConfig.ENABLED_INACTIVE + "inactive" | "true" | ProductActivationConfig.ENABLED_INACTIVE + "inactive" | "1" | ProductActivationConfig.ENABLED_INACTIVE + "false" | null | ProductActivationConfig.FULLY_DISABLED + "false" | "" | ProductActivationConfig.FULLY_DISABLED + "false" | "inactive" | ProductActivationConfig.FULLY_DISABLED + "false" | "false" | ProductActivationConfig.FULLY_DISABLED + "false" | "0" | ProductActivationConfig.FULLY_DISABLED + "false" | "invalid" | ProductActivationConfig.FULLY_DISABLED + "false" | "true" | ProductActivationConfig.FULLY_DISABLED + "false" | "1" | ProductActivationConfig.FULLY_DISABLED + "0" | null | ProductActivationConfig.FULLY_DISABLED + "true" | null | ProductActivationConfig.FULLY_ENABLED + "true" | "" | ProductActivationConfig.FULLY_ENABLED + "true" | "inactive" | ProductActivationConfig.FULLY_ENABLED + "true" | "false" | ProductActivationConfig.FULLY_ENABLED + "true" | "0" | ProductActivationConfig.FULLY_ENABLED + "true" | "invalid" | ProductActivationConfig.FULLY_ENABLED + "true" | "true" | ProductActivationConfig.FULLY_ENABLED + "true" | "1" | ProductActivationConfig.FULLY_ENABLED + "1" | null | ProductActivationConfig.FULLY_ENABLED + } + static class ClassThrowsExceptionForValueOfMethod { static ClassThrowsExceptionForValueOfMethod valueOf(String ignored) { throw new Throwable() From 4ade2c2883efad1e6c025f728a163960b0f6333c Mon Sep 17 00:00:00 2001 From: Santiago Mola Date: Mon, 24 Oct 2022 13:47:53 +0200 Subject: [PATCH 2/4] Apply fallback behavior to system property --- .../main/java/datadog/trace/bootstrap/Agent.java | 6 ++---- .../src/main/java/datadog/trace/api/Config.java | 14 ++++++++++---- .../groovy/datadog/trace/api/ConfigTest.groovy | 10 +++++----- 3 files changed, 17 insertions(+), 13 deletions(-) diff --git a/dd-java-agent/agent-bootstrap/src/main/java/datadog/trace/bootstrap/Agent.java b/dd-java-agent/agent-bootstrap/src/main/java/datadog/trace/bootstrap/Agent.java index d2b90825281..ab6d986c19f 100644 --- a/dd-java-agent/agent-bootstrap/src/main/java/datadog/trace/bootstrap/Agent.java +++ b/dd-java-agent/agent-bootstrap/src/main/java/datadog/trace/bootstrap/Agent.java @@ -895,12 +895,10 @@ private static boolean isAppSecFullyDisabled() { // must be kept in sync with logic from Config! final String featureEnabledSysprop = AgentFeature.APPSEC.systemProp; String settingValue = System.getProperty(featureEnabledSysprop); + settingValue = settingValue != null && settingValue.isEmpty() ? null : settingValue; if (settingValue == null) { settingValue = ddGetEnv(featureEnabledSysprop); - // TODO: We may want this behavior for the system property too. - if (settingValue != null && settingValue.isEmpty()) { - settingValue = null; - } + settingValue = settingValue != null && settingValue.isEmpty() ? null : settingValue; } // defaults to inactive diff --git a/internal-api/src/main/java/datadog/trace/api/Config.java b/internal-api/src/main/java/datadog/trace/api/Config.java index 905c1a30174..69d2c7aab52 100644 --- a/internal-api/src/main/java/datadog/trace/api/Config.java +++ b/internal-api/src/main/java/datadog/trace/api/Config.java @@ -1098,10 +1098,16 @@ && isJavaVersionAtLeast(8) } telemetryHeartbeatInterval = telemetryInterval; - String appSecEnabled = configProvider.getString(APPSEC_ENABLED, DEFAULT_APPSEC_ENABLED); - if (appSecEnabled.isEmpty()) { - // ConfigProvider.getString currently doesn't fallback to default for empty strings. - appSecEnabled = DEFAULT_APPSEC_ENABLED; + // ConfigProvider.getString currently doesn't fallback to default for empty strings. So we have + // special handling here until we have a general solution for empty string value fallback. + String appSecEnabled = configProvider.getString(APPSEC_ENABLED); + if (appSecEnabled == null || appSecEnabled.isEmpty()) { + appSecEnabled = + configProvider.getStringExcludingSource( + APPSEC_ENABLED, DEFAULT_APPSEC_ENABLED, SystemPropertiesConfigSource.class); + if (appSecEnabled.isEmpty()) { + appSecEnabled = DEFAULT_APPSEC_ENABLED; + } } this.appSecEnabled = ProductActivationConfig.fromString(appSecEnabled); appSecReportingInband = diff --git a/internal-api/src/test/groovy/datadog/trace/api/ConfigTest.groovy b/internal-api/src/test/groovy/datadog/trace/api/ConfigTest.groovy index 1a4b70e1496..45e05803ec6 100644 --- a/internal-api/src/test/groovy/datadog/trace/api/ConfigTest.groovy +++ b/internal-api/src/test/groovy/datadog/trace/api/ConfigTest.groovy @@ -2065,11 +2065,11 @@ class ConfigTest extends DDSpecification { "" | null | ProductActivationConfig.ENABLED_INACTIVE "" | "" | ProductActivationConfig.ENABLED_INACTIVE "" | "inactive" | ProductActivationConfig.ENABLED_INACTIVE - "" | "false" | ProductActivationConfig.ENABLED_INACTIVE // FIXME: Should be FULLY_DISABLED? - "" | "0" | ProductActivationConfig.ENABLED_INACTIVE // FIXME: Should be FULLY_DISABLED? - "" | "invalid" | ProductActivationConfig.ENABLED_INACTIVE - "" | "true" | ProductActivationConfig.ENABLED_INACTIVE // FIXME: Should be FULLY_ENABLED? - "" | "1" | ProductActivationConfig.ENABLED_INACTIVE // FIXME: Should be FULLY_ENABLED? + "" | "false" | ProductActivationConfig.FULLY_DISABLED + "" | "0" | ProductActivationConfig.FULLY_DISABLED + "" | "invalid" | ProductActivationConfig.FULLY_DISABLED + "" | "true" | ProductActivationConfig.FULLY_ENABLED + "" | "1" | ProductActivationConfig.FULLY_ENABLED "inactive" | null | ProductActivationConfig.ENABLED_INACTIVE "inactive" | "" | ProductActivationConfig.ENABLED_INACTIVE "inactive" | "inactive" | ProductActivationConfig.ENABLED_INACTIVE From 2e3ada1c242ac0f50dd50aa7450025af3c75cfcd Mon Sep 17 00:00:00 2001 From: Santiago Mola Date: Mon, 24 Oct 2022 14:38:08 +0200 Subject: [PATCH 3/4] add helper method for null if empty --- .../src/main/java/datadog/trace/bootstrap/Agent.java | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/dd-java-agent/agent-bootstrap/src/main/java/datadog/trace/bootstrap/Agent.java b/dd-java-agent/agent-bootstrap/src/main/java/datadog/trace/bootstrap/Agent.java index ab6d986c19f..792d1cb1c6e 100644 --- a/dd-java-agent/agent-bootstrap/src/main/java/datadog/trace/bootstrap/Agent.java +++ b/dd-java-agent/agent-bootstrap/src/main/java/datadog/trace/bootstrap/Agent.java @@ -894,10 +894,9 @@ private static boolean isFeatureEnabled(AgentFeature feature) { private static boolean isAppSecFullyDisabled() { // must be kept in sync with logic from Config! final String featureEnabledSysprop = AgentFeature.APPSEC.systemProp; - String settingValue = System.getProperty(featureEnabledSysprop); - settingValue = settingValue != null && settingValue.isEmpty() ? null : settingValue; + String settingValue = getNullIfEmpty(System.getProperty(featureEnabledSysprop)); if (settingValue == null) { - settingValue = ddGetEnv(featureEnabledSysprop); + settingValue = getNullIfEmpty(ddGetEnv(featureEnabledSysprop)); settingValue = settingValue != null && settingValue.isEmpty() ? null : settingValue; } @@ -908,6 +907,13 @@ private static boolean isAppSecFullyDisabled() { || settingValue.equalsIgnoreCase("inactive")); } + private static String getNullIfEmpty(final String value) { + if (value != null && value.isEmpty()) { + return null; + } + return value; + } + /** @return configured JMX start delay in seconds */ private static int getJmxStartDelay() { String startDelay = ddGetProperty("dd.dogstatsd.start-delay"); From bd43dbe51aca054d6092e2f093e2fa6ee0ddd11f Mon Sep 17 00:00:00 2001 From: Santiago Mola Date: Mon, 24 Oct 2022 16:41:20 +0200 Subject: [PATCH 4/4] clarify condition --- .../src/main/java/datadog/trace/bootstrap/Agent.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/dd-java-agent/agent-bootstrap/src/main/java/datadog/trace/bootstrap/Agent.java b/dd-java-agent/agent-bootstrap/src/main/java/datadog/trace/bootstrap/Agent.java index 792d1cb1c6e..dd2b6a75817 100644 --- a/dd-java-agent/agent-bootstrap/src/main/java/datadog/trace/bootstrap/Agent.java +++ b/dd-java-agent/agent-bootstrap/src/main/java/datadog/trace/bootstrap/Agent.java @@ -908,7 +908,7 @@ private static boolean isAppSecFullyDisabled() { } private static String getNullIfEmpty(final String value) { - if (value != null && value.isEmpty()) { + if (value == null || value.isEmpty()) { return null; } return value;