From ed22258b832eaf0286be0b67b827b9a551393f2e Mon Sep 17 00:00:00 2001 From: Gustavo Lopes Date: Thu, 7 Nov 2024 14:37:05 +0000 Subject: [PATCH 1/5] Disable helper when appsec is fully disabled. Make datadog.appsec.enabled unmodifiable --- .../src/extension/commands/request_shutdown.c | 1 + appsec/src/extension/configuration.h | 2 +- appsec/src/extension/ddappsec.c | 25 ++++++------ appsec/src/extension/ddappsec.h | 7 ++-- appsec/src/extension/helper_process.c | 8 +++- .../SidecarFeaturesDisabledTests.groovy | 39 ++++++++++++++++++- ext/configuration.h | 3 +- ext/ddtrace.c | 2 +- 8 files changed, 67 insertions(+), 20 deletions(-) diff --git a/appsec/src/extension/commands/request_shutdown.c b/appsec/src/extension/commands/request_shutdown.c index 9b4d4e4eec..88f53731d1 100644 --- 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 index 57bb633253..58a0663cb9 100644 --- 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 index d5857f990d..4913a1c489 100644 --- 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 #include @@ -206,8 +202,6 @@ static PHP_MINIT_FUNCTION(ddappsec) return FAILURE; } - DDAPPSEC_G(enabled) = APPSEC_ENABLED_VIA_REMCFG; - dd_log_startup(); #ifdef TESTING @@ -363,6 +357,10 @@ __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)) { @@ -370,10 +368,15 @@ static void _check_enabled() 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 index b8af96c209..b582215fdf 100644 --- 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 #include -#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 index 7c5f33d19b..98a5a40021 100644 --- 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/appsec/tests/integration/src/test/groovy/com/datadog/appsec/php/integration/SidecarFeaturesDisabledTests.groovy b/appsec/tests/integration/src/test/groovy/com/datadog/appsec/php/integration/SidecarFeaturesDisabledTests.groovy index acd6916355..2abc4cdacb 100644 --- a/appsec/tests/integration/src/test/groovy/com/datadog/appsec/php/integration/SidecarFeaturesDisabledTests.groovy +++ b/appsec/tests/integration/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 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 index ee234fd0e5..372f1b1073 100644 --- 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 index 0ec0fbae7a..64cfa4531b 100644 --- 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); From 9d047b6be8055bd02ed38d7e099d916a2aee653e Mon Sep 17 00:00:00 2001 From: Gustavo Lopes Date: Thu, 7 Nov 2024 15:57:41 +0000 Subject: [PATCH 2/5] Fix and refactor logic for helper activation --- appsec/src/extension/ddappsec.c | 24 +++---------- appsec/src/extension/ddappsec.h | 3 -- appsec/src/extension/ddappsec.version | 1 - appsec/src/extension/helper_process.c | 29 ++++++++++------ appsec/src/extension/helper_process.h | 5 +-- .../SidecarFeaturesDisabledTests.groovy | 34 +++++++++++++++++++ components-rs/ddtrace.h | 10 +++--- components-rs/sidecar.rs | 4 +-- ext/configuration.h | 2 -- ext/ddtrace.c | 17 +++++++--- ext/sidecar.c | 29 +++++----------- ext/sidecar.h | 3 +- 12 files changed, 89 insertions(+), 72 deletions(-) diff --git a/appsec/src/extension/ddappsec.c b/appsec/src/extension/ddappsec.c index 4913a1c489..ec3c41ce46 100644 --- a/appsec/src/extension/ddappsec.c +++ b/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) { diff --git a/appsec/src/extension/ddappsec.h b/appsec/src/extension/ddappsec.h index b582215fdf..58422c8ce3 100644 --- a/appsec/src/extension/ddappsec.h +++ b/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 diff --git a/appsec/src/extension/ddappsec.version b/appsec/src/extension/ddappsec.version index 7ab9abb2c3..eb4dab81b2 100644 --- 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 index 98a5a40021..89366ecf43 100644 --- a/appsec/src/extension/helper_process.c +++ b/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() diff --git a/appsec/src/extension/helper_process.h b/appsec/src/extension/helper_process.h index 9534bfdecf..18878cf9a3 100644 --- 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/appsec/tests/integration/src/test/groovy/com/datadog/appsec/php/integration/SidecarFeaturesDisabledTests.groovy b/appsec/tests/integration/src/test/groovy/com/datadog/appsec/php/integration/SidecarFeaturesDisabledTests.groovy index 2abc4cdacb..7deb59ce61 100644 --- a/appsec/tests/integration/src/test/groovy/com/datadog/appsec/php/integration/SidecarFeaturesDisabledTests.groovy +++ b/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 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 index d1cb950c81..f18f7a6dbe 100644 --- 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 index 3801858fa1..3b0d0b4409 100644 --- 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 index 372f1b1073..d13c7af3fb 100644 --- a/ext/configuration.h +++ b/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, "") \ diff --git a/ext/ddtrace.c b/ext/ddtrace.c index 64cfa4531b..bd97d6115d 100644 --- a/ext/ddtrace.c +++ b/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 diff --git a/ext/sidecar.c b/ext/sidecar.c index 0157f6088b..e616565481 100644 --- a/ext/sidecar.c +++ b/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(); diff --git a/ext/sidecar.h b/ext/sidecar.h index d860945f9f..25e5d0d4e1 100644 --- a/ext/sidecar.h +++ b/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); From 5367caa98413a122adf0629d6f016e45c3c525f3 Mon Sep 17 00:00:00 2001 From: Gustavo Lopes Date: Thu, 7 Nov 2024 16:57:27 +0000 Subject: [PATCH 3/5] Fix build on PHP 8.4 --- appsec/src/extension/ddappsec.h | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/appsec/src/extension/ddappsec.h b/appsec/src/extension/ddappsec.h index 58422c8ce3..e15ac3a7e3 100644 --- a/appsec/src/extension/ddappsec.h +++ b/appsec/src/extension/ddappsec.h @@ -9,10 +9,11 @@ // This header MUST be included in files that use EG/PG/OG/... // See https://bugs.php.net/bug.php?id=81634 -#include "attributes.h" #include #include +#include "attributes.h" + typedef enum _enabled_configuration { APPSEC_UNSET_STATE = 0, APPSEC_ENABLED_VIA_REMCFG, From 21d4dd1e278f492c2d2da4d3ac97ce36d59dc7a3 Mon Sep 17 00:00:00 2001 From: Gustavo Lopes Date: Fri, 8 Nov 2024 10:15:50 +0000 Subject: [PATCH 4/5] Fix clang-tidy warning --- appsec/src/extension/helper_process.c | 1 + 1 file changed, 1 insertion(+) diff --git a/appsec/src/extension/helper_process.c b/appsec/src/extension/helper_process.c index 89366ecf43..9b331f924b 100644 --- a/appsec/src/extension/helper_process.c +++ b/appsec/src/extension/helper_process.c @@ -180,6 +180,7 @@ static void _read_settings() __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(); From 74be367f87d16c4a0fe4fa6da17dff9bc9db6a7b Mon Sep 17 00:00:00 2001 From: Gustavo Lopes Date: Fri, 8 Nov 2024 10:44:52 +0000 Subject: [PATCH 5/5] appsec: fix zts --- appsec/src/extension/ddappsec.c | 1 + 1 file changed, 1 insertion(+) diff --git a/appsec/src/extension/ddappsec.c b/appsec/src/extension/ddappsec.c index ec3c41ce46..01ebf8f62b 100644 --- a/appsec/src/extension/ddappsec.c +++ b/appsec/src/extension/ddappsec.c @@ -264,6 +264,7 @@ static PHP_RINIT_FUNCTION(ddappsec) dd_appsec_rinit_once(); zai_config_rinit(); + _check_enabled(); if (DDAPPSEC_G(enabled) == APPSEC_FULLY_DISABLED) { return SUCCESS;