Fix and refactor logic for helper activation

appsec/src/extension/ddappsec.c

@@ -241,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()
 {
@@ -260,7 +264,6 @@ static PHP_RINIT_FUNCTION(ddappsec)
 
     dd_appsec_rinit_once();
     zai_config_rinit();
-    _check_enabled();
 
     if (DDAPPSEC_G(enabled) == APPSEC_FULLY_DISABLED) {
         return SUCCESS;
@@ -383,23 +386,6 @@ static void _check_enabled()
     };
 }
 
-__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) {

appsec/src/extension/ddappsec.h

@@ -55,9 +55,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

appsec/src/extension/ddappsec.version

@@ -2,6 +2,5 @@
 	global:
 		get_module;
 		dd_appsec_maybe_enable_helper;
-		dd_appsec_rc_conf;
 	local: *;
 };

appsec/src/extension/helper_process.c

@@ -173,16 +173,24 @@ 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,
+    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,12 +203,13 @@ __attribute__((visibility("default"))) void dd_appsec_maybe_enable_helper(
     ddog_CharSlice log_level =
         to_char_slice(get_global_DD_APPSEC_HELPER_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);
-    }
+    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()

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);

appsec/tests/integration/src/test/groovy/com/datadog/appsec/php/integration/SidecarFeaturesDisabledTests.groovy

@@ -42,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')
                 }
@@ -100,4 +101,37 @@ class SidecarFeaturesDisabledTests {
             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")
+        }
+    }
 }

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,

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]

ext/configuration.h

@@ -220,8 +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_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, "")                                                                  \

ext/ddtrace.c

@@ -405,14 +405,15 @@ bool ddtrace_alter_dd_version(zval *old_value, zval *new_value, zend_string *new
     return true;
 }
 
-static zend_module_entry *dd_appsec_module() { return zend_hash_str_find_ptr(&module_registry, "ddappsec", sizeof("ddappsec") - 1); }
-
 static void dd_activate_once(void) {
     ddtrace_config_first_rinit();
     ddtrace_generate_runtime_id();
 
     // must run before the first zai_hook_activate as ddtrace_telemetry_setup installs a global hook
     if (!ddtrace_disable) {
+        bool appsec_features = false;
+        bool appsec_config = false;
+
 #ifndef _WIN32
         // Only disable sidecar sender when explicitly disabled
         bool bgs_fallback = DD_SIDECAR_TRACE_SENDER_DEFAULT && get_global_DD_TRACE_SIDECAR_TRACE_SENDER() && zai_config_memoized_entries[DDTRACE_CONFIG_DD_TRACE_SIDECAR_TRACE_SENDER].name_index < 0 && !get_global_DD_INSTRUMENTATION_TELEMETRY_ENABLED();
@@ -428,13 +429,19 @@ static void dd_activate_once(void) {
                 bgs_service = ddtrace_default_service_name();
             }
         }
-        if (get_global_DD_INSTRUMENTATION_TELEMETRY_ENABLED() || get_global_DD_TRACE_SIDECAR_TRACE_SENDER() ||
-            (dd_appsec_module() != NULL && !get_global_DD_APPSEC_TESTING() && get_global_DD_APPSEC_ENABLED()))
+
+        // if we're to enable appsec, we need to enable sidecar
+        bool enable_sidecar = ddtrace_sidecar_maybe_enable_appsec(&appsec_features, &appsec_config);
+        if (!enable_sidecar) {
+            enable_sidecar = get_global_DD_INSTRUMENTATION_TELEMETRY_ENABLED() || get_global_DD_TRACE_SIDECAR_TRACE_SENDER();
+        }
+
+        if (enable_sidecar)
 #endif
         {
             bool request_startup = PG(during_request_startup);
             PG(during_request_startup) = false;
-            ddtrace_sidecar_setup();
+            ddtrace_sidecar_setup(appsec_features, appsec_config);
             PG(during_request_startup) = request_startup;
         }
 #ifndef _WIN32

ext/sidecar.c

@@ -98,7 +98,7 @@ ddog_SidecarTransport *dd_sidecar_connection_factory(void) {
     return sidecar_transport;
 }
 
-static void maybe_enable_appsec(bool *appsec_features, bool *appsec_config) {
+bool ddtrace_sidecar_maybe_enable_appsec(bool *appsec_features, bool *appsec_config) {
     *appsec_features = false;
     *appsec_config = false;
 
@@ -107,38 +107,25 @@ static void maybe_enable_appsec(bool *appsec_features, bool *appsec_config) {
     // to run its first rinit
 
 #if defined(__linux__) || defined(__APPLE__)
-    if (get_global_DD_APPSEC_TESTING()) {
-        return;
-    }
     zend_module_entry *appsec_module = zend_hash_str_find_ptr(&module_registry, "ddappsec", sizeof("ddappsec") - 1);
     if (!appsec_module) {
-        return;
+        return false;
     }
     void *handle = dlsym(appsec_module->handle, "dd_appsec_maybe_enable_helper");
     if (!handle) {
-        return;
-    }
-    void (*dd_appsec_maybe_enable_helper)(typeof(&ddog_sidecar_enable_appsec) enable_appsec) = handle;
-    dd_appsec_maybe_enable_helper(ddog_sidecar_enable_appsec);
-
-    typedef void (*dd_appsec_rc_conf_t)(bool *, bool *);
-    dd_appsec_rc_conf_t dd_appsec_rc_conf = dlsym(RTLD_DEFAULT, "dd_appsec_rc_conf");
-    if (dd_appsec_rc_conf) {
-        dd_appsec_rc_conf(appsec_features, appsec_config);
-    } else {
-        LOG(WARN, "Could not resolve dd_appsec_rc_conf");
+        return false;
     }
+    bool (*dd_appsec_maybe_enable_helper)(typeof(&ddog_sidecar_enable_appsec), bool *, bool *) = handle;
+    return dd_appsec_maybe_enable_helper(ddog_sidecar_enable_appsec, appsec_features, appsec_config);
+#else
+    return false;
 #endif
 }
 
-void ddtrace_sidecar_setup(void) {
+void ddtrace_sidecar_setup(bool appsec_features, bool appsec_config) {
     ddtrace_set_non_resettable_sidecar_globals();
     ddtrace_set_resettable_sidecar_globals();
 
-    bool appsec_features;
-    bool appsec_config;
-    maybe_enable_appsec(&appsec_features, &appsec_config);
-
     ddog_init_remote_config(get_global_DD_INSTRUMENTATION_TELEMETRY_ENABLED(), appsec_features, appsec_config);
 
     ddtrace_sidecar = dd_sidecar_connection_factory();

ext/sidecar.h

@@ -8,7 +8,8 @@ extern ddog_SidecarTransport *ddtrace_sidecar;
 extern ddog_Endpoint *ddtrace_endpoint;
 extern struct ddog_InstanceId *ddtrace_sidecar_instance_id;
 
-void ddtrace_sidecar_setup(void);
+void ddtrace_sidecar_setup(bool appsec_features, bool appsec_config);
+bool ddtrace_sidecar_maybe_enable_appsec(bool *appsec_features, bool *appsec_config);
 void ddtrace_sidecar_ensure_active(void);
 void ddtrace_sidecar_shutdown(void);
 void ddtrace_reset_sidecar_globals(void);