diff --git a/appsec/src/extension/commands/request_shutdown.c b/appsec/src/extension/commands/request_shutdown.c index 9b4d4e4eec7..88f53731d10 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 57bb633253a..58a0663cb92 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 d5857f990de..4913a1c489f 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 b8af96c2091..b582215fdfe 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 7c5f33d19bc..98a5a400214 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 acd6916355d..2abc4cdacb7 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 ee234fd0e54..372f1b10736 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 0ec0fbae7af..64cfa4531b3 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);