Disable helper when appsec is fully disabled. Make datadog.appsec.ena…

…bled unmodifiable
DataDog · Nov 7, 2024 · 751af31 · 751af31
1 parent a987853
commit 751af31
Show file tree

Hide file tree

Showing 8 changed files with 67 additions and 20 deletions.
diff --git a/appsec/src/extension/commands/request_shutdown.c b/appsec/src/extension/commands/request_shutdown.c
@@ -8,6 +8,7 @@
 #include "../commands_helpers.h"
 #include "../ddappsec.h"
 #include "../entity_body.h"
+#include "../logging.h"
 #include "../msgpack_helpers.h"
 #include "../php_compat.h"
 #include "../php_objects.h"

diff --git a/appsec/src/extension/configuration.h b/appsec/src/extension/configuration.h
@@ -30,7 +30,7 @@ extern bool runtime_config_first_init;
 
 // clang-format off
 #define DD_CONFIGURATION \
-    CONFIG(BOOL, DD_APPSEC_ENABLED, "false")                                                                                          \
+    SYSCFG(BOOL, DD_APPSEC_ENABLED, "false")                                                                                          \
     SYSCFG(BOOL, DD_APPSEC_CLI_START_ON_RINIT, "false")                                                                               \
     SYSCFG(STRING, DD_APPSEC_RULES, "")                                                                                               \
     SYSCFG(CUSTOM(uint64_t), DD_APPSEC_WAF_TIMEOUT, "10000", .parser = _parse_uint64)                                                 \

diff --git a/appsec/src/extension/ddappsec.c b/appsec/src/extension/ddappsec.c
@@ -20,10 +20,7 @@
 
 #include "backtrace.h"
 #include "commands/client_init.h"
-#include "commands/config_sync.h"
 #include "commands/request_exec.h"
-#include "commands/request_init.h"
-#include "commands/request_shutdown.h"
 #include "commands_ctx.h"
 #include "compatibility.h"
 #include "configuration.h"
@@ -37,13 +34,12 @@
 #include "msgpack_helpers.h"
 #include "network.h"
 #include "php_compat.h"
-#include "php_helpers.h"
 #include "php_objects.h"
 #include "request_abort.h"
 #include "request_lifecycle.h"
-#include "string_helpers.h"
 #include "tags.h"
 #include "user_tracking.h"
+#include "version.h"
 
 #include <json/json.h>
 #include <zend_string.h>
@@ -206,8 +202,6 @@ static PHP_MINIT_FUNCTION(ddappsec)
         return FAILURE;
     }
 
-    DDAPPSEC_G(enabled) = APPSEC_ENABLED_VIA_REMCFG;
-
     dd_log_startup();
 
 #ifdef TESTING
@@ -363,17 +357,26 @@ __thread void *unspecnull TSRMLS_CACHE = NULL;
 
 static void _check_enabled()
 {
+    if (DDAPPSEC_G(enabled) != APPSEC_UNSET_STATE) {
+        return;
+    }
+
     if ((!get_global_DD_APPSEC_TESTING() && !dd_trace_enabled()) ||
         (strcmp(sapi_module.name, "cli") != 0 && sapi_module.phpinfo_as_text) ||
         (strcmp(sapi_module.name, "frankenphp") == 0)) {
         DDAPPSEC_G(enabled) = APPSEC_FULLY_DISABLED;
         DDAPPSEC_G(active) = false;
         DDAPPSEC_G(to_be_configured) = false;
     } else if (!dd_cfg_enable_via_remcfg()) {
-        DDAPPSEC_G(enabled) = get_DD_APPSEC_ENABLED() ? APPSEC_FULLY_ENABLED
-                                                      : APPSEC_FULLY_DISABLED;
-        DDAPPSEC_G(active) = get_DD_APPSEC_ENABLED() ? true : false;
-        DDAPPSEC_G(to_be_configured) = false;
+        if (get_global_DD_APPSEC_ENABLED()) {
+            DDAPPSEC_G(enabled) = APPSEC_FULLY_ENABLED;
+            DDAPPSEC_G(active) = true;
+            DDAPPSEC_G(to_be_configured) = false;
+        } else {
+            DDAPPSEC_G(enabled) = APPSEC_FULLY_DISABLED;
+            DDAPPSEC_G(active) = false;
+            DDAPPSEC_G(to_be_configured) = false;
+        }
     } else {
         DDAPPSEC_G(enabled) = APPSEC_ENABLED_VIA_REMCFG;
         // leave DDAPPSEC_G(active) as is

diff --git a/appsec/src/extension/ddappsec.h b/appsec/src/extension/ddappsec.h
@@ -9,14 +9,13 @@
 // This header MUST be included in files that use EG/PG/OG/...
 // See https://bugs.php.net/bug.php?id=81634
 
-#include "logging.h"
-#include "version.h"
+#include "attributes.h"
 #include <php.h>
 #include <stdbool.h>
-#include "attributes.h"
 
 typedef enum _enabled_configuration {
-    APPSEC_ENABLED_VIA_REMCFG = 0,
+    APPSEC_UNSET_STATE = 0,
+    APPSEC_ENABLED_VIA_REMCFG,
     APPSEC_FULLY_ENABLED,
     APPSEC_FULLY_DISABLED
 } enabled_configuration;

diff --git a/appsec/src/extension/helper_process.c b/appsec/src/extension/helper_process.c
@@ -19,6 +19,7 @@
 #include "network.h"
 #include "php_compat.h"
 #include "php_objects.h"
+#include "version.h"
 
 typedef struct _dd_helper_mgr {
     dd_conn conn;
@@ -194,7 +195,12 @@ __attribute__((visibility("default"))) void dd_appsec_maybe_enable_helper(
     ddog_CharSlice log_level =
         to_char_slice(get_global_DD_APPSEC_HELPER_LOG_LEVEL());
 
-    enable_appsec(helper_path, socket_path, lock_path, log_path, log_level);
+    ddog_MaybeError res =
+        enable_appsec(helper_path, socket_path, lock_path, log_path, log_level);
+    if (res.tag == DDOG_OPTION_ERROR_SOME_ERROR) {
+        mlog_err(dd_log_error, "Failed to enable helper in sidecar: %.*s",
+            (int)res.some.message.len, res.some.message.ptr);
+    }
 }
 
 void dd_helper_close_conn()

diff --git a/...on/src/test/groovy/com/datadog/appsec/php/integration/SidecarFeaturesDisabledTests.groovy b/...on/src/test/groovy/com/datadog/appsec/php/integration/SidecarFeaturesDisabledTests.groovy
@@ -4,7 +4,10 @@ import com.datadog.appsec.php.docker.AppSecContainer
 import com.datadog.appsec.php.docker.FailOnUnmatchedTraces
 import com.datadog.appsec.php.docker.InspectContainerHelper
 import groovy.util.logging.Slf4j
+import org.junit.jupiter.api.MethodOrderer
+import org.junit.jupiter.api.Order
 import org.junit.jupiter.api.Test
+import org.junit.jupiter.api.TestMethodOrder
 import org.junit.jupiter.api.condition.DisabledIf
 import org.testcontainers.junit.jupiter.Container
 import org.testcontainers.junit.jupiter.Testcontainers
@@ -20,6 +23,7 @@ import static org.testcontainers.containers.Container.ExecResult
 @Testcontainers
 @Slf4j
 @DisabledIf('isNotPhp83')
+@TestMethodOrder(MethodOrderer.OrderAnnotation)
 class SidecarFeaturesDisabledTests {
     static boolean isNotPhp83()  {
         !getPhpVersion().startsWith('8.3')
@@ -48,6 +52,7 @@ class SidecarFeaturesDisabledTests {
     }
 
     @Test
+    @Order(1)
     void 'appsec is enabled and sidecar is launched'() {
         HttpRequest req = CONTAINER.buildReq('/hello.php')
                 .GET().build()
@@ -58,9 +63,41 @@ class SidecarFeaturesDisabledTests {
         assert trace.first().metrics."_dd.appsec.enabled" == 1.0d
 
         ExecResult res = CONTAINER.execInContainer(
-                '/bin/bash', '-ce', 'ps auxww; pgrep -f datadog-ipc-helper')
+                '/bin/bash', '-ce', 'ps auxww; pgrep -f [d]atadog-ipc-helper')
         if (res.exitCode != 0) {
             throw new AssertionError("Could not find helper: $res.stdout\n$res.stderr")
         }
     }
+
+    @Order(2)
+    @Test
+    void 'appsec is disabled and sidecar is not launched'() {
+        ExecResult res = CONTAINER.execInContainer(
+                'sed', '-i', 's/^datadog.appsec.enabled.*$/datadog.appsec.enabled=false/', '/etc/php/php.ini')
+        assert res.exitCode == 0
+
+        res = CONTAINER.execInContainer(
+                '/bin/bash', '-c', '''
+                    pid=`pgrep -f [d]atadog-ipc-helper`;
+                    if [ -n "$pid" ]; then echo "Helper is running: $pid";
+                    kill -9 $pid; fi''')
+        assert res.exitCode == 0
+
+        res = CONTAINER.execInContainer('service', 'apache2', 'restart')
+        assert res.exitCode == 0
+
+        HttpRequest req = CONTAINER.buildReq('/hello.php')
+                .GET().build()
+        def trace = CONTAINER.traceFromRequest(req, ofString()) { HttpResponse<String> resp ->
+            resp.body() == 'Hello world!'
+        }
+
+        assert !trace.first().metrics.containsKey('_dd.appsec.enabled')
+
+        res = CONTAINER.execInContainer(
+                '/bin/bash', '-ce', 'ps auxww; ! pgrep -f [d]atadog-ipc-helper')
+        if (res.exitCode != 0) {
+            throw new AssertionError("Found helper when not expected: $res.stdout\n$res.stderr")
+        }
+    }
 }
diff --git a/ext/configuration.h b/ext/configuration.h
@@ -220,7 +220,8 @@ enum ddtrace_sampling_rules_format {
     CONFIG(STRING, DD_TRACE_LOG_LEVEL, "error", .ini_change = ddtrace_alter_dd_trace_log_level,                \
            .env_config_fallback = ddtrace_conf_otel_log_level)                                                 \
     CONFIG(BOOL, DD_APPSEC_SCA_ENABLED, "false", .ini_change = zai_config_system_ini_change)                   \
-    CONFIG(BOOL, DD_APPSEC_TESTING, "false")                                                                   \
+    CONFIG(BOOL, DD_APPSEC_ENABLED, "true", .ini_change = zai_config_system_ini_change)                        \
+    CONFIG(BOOL, DD_APPSEC_TESTING, "false", .ini_change = zai_config_system_ini_change)                       \
     CONFIG(BOOL, DD_TRACE_GIT_METADATA_ENABLED, "true")                                                        \
     CONFIG(STRING, DD_GIT_COMMIT_SHA, "")                                                                      \
     CONFIG(STRING, DD_GIT_REPOSITORY_URL, "")                                                                  \

diff --git a/ext/ddtrace.c b/ext/ddtrace.c
@@ -429,7 +429,7 @@ static void dd_activate_once(void) {
             }
         }
         if (get_global_DD_INSTRUMENTATION_TELEMETRY_ENABLED() || get_global_DD_TRACE_SIDECAR_TRACE_SENDER() ||
-            (dd_appsec_module() != NULL && !get_global_DD_APPSEC_TESTING()))
+            (dd_appsec_module() != NULL && !get_global_DD_APPSEC_TESTING() && get_global_DD_APPSEC_ENABLED()))
 #endif
         {
             bool request_startup = PG(during_request_startup);