Merge pull request #2935 from DataDog/no-helper-with-appsec-disabled

Disable helper when appsec is fully disabled.
DataDog · Nov 11, 2024 · 3abddef · 3abddef
2 parents 1c8e506 + 74be367
commit 3abddef
Show file tree

Hide file tree

Showing 14 changed files with 147 additions and 80 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
@@ -247,7 +241,11 @@ static PHP_MSHUTDOWN_FUNCTION(ddappsec)
     return SUCCESS;
 }
 
-static void _rinit_once() { dd_config_first_rinit(); }
+static void _rinit_once()
+{
+    dd_config_first_rinit();
+    _check_enabled();
+}
 
 void dd_appsec_rinit_once()
 {
@@ -363,40 +361,32 @@ __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
     };
 }
 
-__attribute__((visibility("default"))) void dd_appsec_rc_conf(
-    bool *nonnull appsec_features, bool *nonnull appsec_conf) // NOLINT
-{
-    bool prev_enabled = DDAPPSEC_G(enabled);
-    bool prev_active = DDAPPSEC_G(active);
-    bool prev_to_be_configured = DDAPPSEC_G(to_be_configured);
-    _check_enabled();
-
-    *appsec_features = DDAPPSEC_G(enabled) == APPSEC_ENABLED_VIA_REMCFG;
-    // only enable ASM / ASM_DD / ASM_DATA if no rules file is specified
-    *appsec_conf = get_global_DD_APPSEC_RULES()->len == 0;
-
-    DDAPPSEC_G(enabled) = prev_enabled;
-    DDAPPSEC_G(active) = prev_active;
-    DDAPPSEC_G(to_be_configured) = prev_to_be_configured;
-}
-
 static PHP_FUNCTION(datadog_appsec_is_enabled)
 {
     if (zend_parse_parameters_none() == FAILURE) {

diff --git a/appsec/src/extension/ddappsec.h b/appsec/src/extension/ddappsec.h
@@ -9,14 +9,14 @@
 // 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 <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;
@@ -56,9 +56,6 @@ extern __thread void *unspecnull ATTR_TLS_LOCAL_DYNAMIC TSRMLS_CACHE;
 void dd_appsec_rinit_once(void);
 int dd_appsec_rshutdown(bool ignore_verdict);
 
-__attribute__((visibility("default"))) void dd_appsec_rc_conf(
-    bool *nonnull appsec_features, bool *nonnull appsec_conf); // NOLINT
-
 // Add a NO_CACHE version.
 // Use tsrm_get_ls_cache() instead of thread-local _tsrmls_ls_cache
 #ifdef ZTS

diff --git a/appsec/src/extension/ddappsec.version b/appsec/src/extension/ddappsec.version
@@ -2,6 +2,5 @@
 	global:
 		get_module;
 		dd_appsec_maybe_enable_helper;
-		dd_appsec_rc_conf;
 	local: *;
 };
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;
@@ -172,16 +173,25 @@ static void _read_settings()
         return;
     }
 
-    dd_appsec_rinit_once();
-
     zval runtime_path;
     ZVAL_STR(&runtime_path, get_DD_APPSEC_HELPER_RUNTIME_PATH());
     dd_on_runtime_path_update(NULL, &runtime_path, NULL);
 }
 
-__attribute__((visibility("default"))) void dd_appsec_maybe_enable_helper(
-    sidecar_enable_appsec_t nonnull enable_appsec)
+__attribute__((visibility("default"))) bool dd_appsec_maybe_enable_helper(
+    sidecar_enable_appsec_t nonnull enable_appsec,
+    // NOLINTNEXTLINE(bugprone-easily-swappable-parameters)
+    bool *nonnull appsec_features, bool *nonnull appsec_conf)
 {
+    dd_appsec_rinit_once();
+
+    if (DDAPPSEC_G(enabled) == APPSEC_FULLY_DISABLED ||
+        get_global_DD_APPSEC_TESTING()) {
+        *appsec_features = false;
+        *appsec_conf = false;
+        return false;
+    }
+
     _read_settings();
 
     ddog_CharSlice helper_path = to_char_slice(get_DD_APPSEC_HELPER_PATH());
@@ -195,6 +205,12 @@ __attribute__((visibility("default"))) void dd_appsec_maybe_enable_helper(
         to_char_slice(get_global_DD_APPSEC_HELPER_LOG_LEVEL());
 
     enable_appsec(helper_path, socket_path, lock_path, log_path, log_level);
+
+    *appsec_features = DDAPPSEC_G(enabled) == APPSEC_ENABLED_VIA_REMCFG;
+    // only enable ASM / ASM_DD / ASM_DATA if no rules file is specified
+    *appsec_conf = get_global_DD_APPSEC_RULES()->len == 0;
+
+    return true;
 }
 
 void dd_helper_close_conn()

diff --git a/appsec/src/extension/helper_process.h b/appsec/src/extension/helper_process.h
@@ -14,8 +14,9 @@
 
 typedef typeof(&ddog_sidecar_enable_appsec) sidecar_enable_appsec_t;
 
-__attribute__((visibility("default")))
-void dd_appsec_maybe_enable_helper(sidecar_enable_appsec_t nonnull enable_appsec);
+__attribute__((visibility("default"))) bool dd_appsec_maybe_enable_helper(
+    sidecar_enable_appsec_t nonnull enable_appsec,
+    bool *nonnull appsec_features, bool *nonnull appsec_conf);
 
 void dd_helper_startup(void);
 void dd_helper_shutdown(void);

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')
@@ -38,6 +42,7 @@ class SidecarFeaturesDisabledTests {
                 @Override
                 void configure() {
                     super.configure()
+                    // these force sidecar to run
                     withEnv('DD_INSTRUMENTATION_TELEMETRY_ENABLED', 'false')
                     withEnv('DD_TRACE_SIDECAR_TRACE_SENDER', 'false')
                 }
@@ -48,6 +53,7 @@ class SidecarFeaturesDisabledTests {
     }
 
     @Test
+    @Order(1)
     void 'appsec is enabled and sidecar is launched'() {
         HttpRequest req = CONTAINER.buildReq('/hello.php')
                 .GET().build()
@@ -58,7 +64,72 @@ 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")
+        }
+    }
+
+    @Order(3)
+    @Test
+    void 'sidecar is enabled but not appsec'() {
+        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('/bin/bash', '-c',
+            'echo DD_INSTRUMENTATION_TELEMETRY_ENABLED=true >> /etc/apache2/envvars; 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("Could not find helper: $res.stdout\n$res.stderr")
         }

diff --git a/components-rs/ddtrace.h b/components-rs/ddtrace.h
@@ -210,11 +210,11 @@ ddog_MaybeError ddog_send_debugger_diagnostics(const struct ddog_RemoteConfigSta
                                                const struct ddog_Probe *probe,
                                                uint64_t timestamp);
 
-ddog_MaybeError ddog_sidecar_enable_appsec(ddog_CharSlice shared_lib_path,
-                                           ddog_CharSlice socket_file_path,
-                                           ddog_CharSlice lock_file_path,
-                                           ddog_CharSlice log_file_path,
-                                           ddog_CharSlice log_level);
+void ddog_sidecar_enable_appsec(ddog_CharSlice shared_lib_path,
+                                ddog_CharSlice socket_file_path,
+                                ddog_CharSlice lock_file_path,
+                                ddog_CharSlice log_file_path,
+                                ddog_CharSlice log_level);
 
 ddog_MaybeError ddog_sidecar_connect_php(struct ddog_SidecarTransport **connection,
                                          const char *error_path,

diff --git a/components-rs/sidecar.rs b/components-rs/sidecar.rs
@@ -69,7 +69,7 @@ pub extern "C" fn ddog_sidecar_enable_appsec(
     lock_file_path: CharSlice,
     log_file_path: CharSlice,
     log_level: CharSlice,
-) -> MaybeError {
+) -> () {
     let mut appsec_config_guard = APPSEC_CONFIG.lock().unwrap();
     let shared_lib_path_os: std::ffi::OsString;
     let socket_file_path_os: std::ffi::OsString;
@@ -99,8 +99,6 @@ pub extern "C" fn ddog_sidecar_enable_appsec(
         log_file_path: log_file_path_os,
         log_level: log_level.to_utf8_lossy().to_string(),
     });
-
-    MaybeError::None
 }
 
 #[no_mangle]

diff --git a/ext/configuration.h b/ext/configuration.h
@@ -220,7 +220,6 @@ 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_TRACE_GIT_METADATA_ENABLED, "true")                                                        \
     CONFIG(STRING, DD_GIT_COMMIT_SHA, "")                                                                      \
     CONFIG(STRING, DD_GIT_REPOSITORY_URL, "")                                                                  \