AppSec: improve behavior with empty DD_SERVICE/DD_ENV

appsec/src/extension/commands/client_init.c

@@ -58,9 +58,9 @@ static dd_result _pack_command(
         bool has_rules_file = rules_file && *rules_file;
 
         if (!has_rules_file) {
-            mlog(dd_log_info,
+            mlog(dd_log_debug,
                 "datadog.appsec.rules was not provided. The helper "
-                "will atttempt to use the default file");
+                "will atttempt to use the default file/remote config");
         }
         dd_mpack_write_nullable_cstr(w, rules_file);
     }

appsec/src/extension/commands/config_sync.c

@@ -9,6 +9,7 @@
 
 #include "../commands_helpers.h"
 #include "../ddtrace.h"
+#include "../logging.h"
 #include "../msgpack_helpers.h"
 #include "config_sync.h"
 #include <mpack.h>
@@ -29,6 +30,10 @@ static const dd_command_spec _spec = {
 dd_result dd_config_sync(
     dd_conn *nonnull conn, const struct config_sync_data *nonnull data)
 {
+    mlog_g(dd_log_debug,
+        "Sending config sync request to the helper with path %s",
+        data->rem_cfg_path);
+
     return dd_command_exec(conn, &_spec, (void *)data);
 }
 

appsec/src/extension/ddappsec.c

@@ -254,21 +254,6 @@ void dd_appsec_rinit_once()
     pthread_once(&_rinit_once_control, _rinit_once);
 }
 
-static void _warn_on_empty_service_or_env()
-{
-    if (!get_global_DD_APPSEC_TESTING() && get_DD_REMOTE_CONFIG_ENABLED() &&
-        DDAPPSEC_G(enabled) != APPSEC_FULLY_DISABLED &&
-        (zend_string_equals_literal(get_DD_ENV(), "") ||
-            zend_string_equals_literal(get_DD_SERVICE(), ""))) {
-        mlog(dd_log_warning,
-            "AppSec is not disabled and Datadog service or env is empty. "
-            "Please set DD_SERVICE and DD_ENV rather than setting the "
-            "corresponding properties on the root span. Otherwise, remote "
-            "configuration for AppSec will use service=unnamed-php-service and "
-            "env=none");
-    }
-}
-
 // NOLINTNEXTLINE
 static PHP_RINIT_FUNCTION(ddappsec)
 {
@@ -281,7 +266,6 @@ static PHP_RINIT_FUNCTION(ddappsec)
     dd_appsec_rinit_once();
     zai_config_rinit();
     _check_enabled();
-    _warn_on_empty_service_or_env();
 
     if (DDAPPSEC_G(enabled) == APPSEC_FULLY_DISABLED) {
         return SUCCESS;

appsec/src/extension/request_lifecycle.c

@@ -40,6 +40,7 @@ const zend_array *nonnull _get_server_equiv(
 static void _register_testing_objects(void);
 
 static bool _enabled_user_req;
+static bool _empty_service_or_env;
 static zend_string *_server_zstr;
 
 static THREAD_LOCAL_ON_ZTS zend_object *nullable _cur_req_span;
@@ -75,6 +76,9 @@ void dd_req_lifecycle_startup()
     }
 
     _server_zstr = zend_string_init_interned(LSTRARG("_SERVER"), 1);
+    _empty_service_or_env =
+        zend_string_equals_literal(get_global_DD_ENV(), "") ||
+        zend_string_equals_literal(get_global_DD_SERVICE(), "");
 
     _register_testing_objects();
 }
@@ -128,8 +132,13 @@ static zend_array *nullable _do_request_begin_user_req(zval *nullable rbe_zv)
     return _do_request_begin(rbe_zv, true);
 }
 
-static bool _rem_cfg_path_changed()
+static bool _rem_cfg_path_changed(bool ignore_empty /* called from rinit */)
 {
+    if (ignore_empty && _empty_service_or_env &&
+        _last_rem_cfg_path[0] != '\0') {
+        return false;
+    }
+
     const char *cur_path = dd_trace_remote_config_get_path();
     if (!cur_path) {
         cur_path = "";
@@ -184,7 +193,7 @@ static zend_array *nullable _do_request_begin(
     }
 
     int res = dd_success;
-    if (_rem_cfg_path_changed() ||
+    if (_rem_cfg_path_changed(true) ||
         (!DDAPPSEC_G(active) &&
             DDAPPSEC_G(enabled) == APPSEC_ENABLED_VIA_REMCFG)) {
         res = dd_config_sync(conn,
@@ -253,6 +262,27 @@ void dd_req_lifecycle_rshutdown(bool ignore_verdict, bool force)
         _do_request_finish_php(ignore_verdict);
         // _rest_globals already called
     }
+
+    // if we don't have service/env, our only chance to update the remote config
+    // path is rshutdown because ddtrace's rinit is called before ours and it
+    // resets the path
+    if (_empty_service_or_env && _rem_cfg_path_changed(false)) {
+        mlog_g(dd_log_debug, "No DD_SERVICE/DD_ENV; trying to sync remote "
+                             "config path on rshutdown");
+        dd_conn *conn = dd_helper_mgr_cur_conn();
+        if (conn == NULL) {
+            mlog_g(dd_log_debug,
+                "No connection to the helper for rshutdown config sync");
+        } else {
+            dd_result res = dd_config_sync(conn,
+                &(struct config_sync_data){.rem_cfg_path = _last_rem_cfg_path});
+            if (res) {
+                mlog_g(dd_log_info,
+                    "Failed to sync remote config path on rshutdown: %s",
+                    dd_result_to_string(res));
+            }
+        }
+    }
 }
 
 static void _do_request_finish_php(bool ignore_verdict)

appsec/src/helper/client.cpp

@@ -448,10 +448,17 @@ void client::update_remote_config_path(std::string_view path,
         return;
     }
 
-    SPDLOG_INFO("Remote config path changed to {}, recreating service", path);
     remote_config::settings rc_settings;
-    rc_settings.enabled = true;
-    rc_settings.shmem_path = path;
+    if (path.empty()) {
+        SPDLOG_INFO("Remote config path is empty, recreating service with "
+                    "disabled remote config");
+        rc_settings.enabled = false;
+    } else {
+        SPDLOG_INFO(
+            "Remote config path changed to {}, recreating service", path);
+        rc_settings.enabled = true;
+        rc_settings.shmem_path = path;
+    }
 
     service_ = service_manager_->create_service(
         *engine_settings_, rc_settings, meta, metrics, true);

appsec/src/helper/remote_config/listeners/asm_features_listener.hpp

@@ -20,6 +20,7 @@ class asm_features_listener : public listener_base {
     void on_update(const config &config) override;
     void on_unapply(const config & /*config*/) override
     {
+        SPDLOG_DEBUG("reset ASM activation status");
         service_config_->unset_asm();
     }
 

appsec/src/helper/service_config.hpp

@@ -8,16 +8,46 @@
 #include <atomic>
 #include <map>
 #include <optional>
+#include <spdlog/spdlog.h>
 #include <unordered_map>
 
 namespace dds {
 
 enum class enable_asm_status : unsigned { NOT_SET = 0, ENABLED, DISABLED };
 
+inline std::string_view to_string_view(enable_asm_status status)
+{
+    if (status == enable_asm_status::NOT_SET) {
+        return "NOT_SET";
+    }
+    if (status == enable_asm_status::ENABLED) {
+        return "ENABLED";
+    }
+    if (status == enable_asm_status::DISABLED) {
+        return "DISABLED";
+    }
+    return "UNKNOWN";
+}
+
 struct service_config {
-    void enable_asm() { asm_enabled = enable_asm_status::ENABLED; }
-    void disable_asm() { asm_enabled = enable_asm_status::DISABLED; }
-    void unset_asm() { asm_enabled = enable_asm_status::NOT_SET; }
+    void enable_asm()
+    {
+        SPDLOG_DEBUG("Enabling ASM, previous state: {}", asm_enabled);
+        asm_enabled = enable_asm_status::ENABLED;
+    }
+
+    void disable_asm()
+    {
+        SPDLOG_DEBUG("Disabling ASM, previous state: {}", asm_enabled);
+        asm_enabled = enable_asm_status::DISABLED;
+    }
+
+    void unset_asm()
+    {
+        SPDLOG_DEBUG("Unsetting ASM status, previous state: {}", asm_enabled);
+        asm_enabled = enable_asm_status::NOT_SET;
+    }
+
     enable_asm_status get_asm_enabled_status() { return asm_enabled; }
 
 protected:

appsec/src/helper/subscriber/waf.cpp

@@ -78,7 +78,7 @@ DDWAF_LOG_LEVEL spdlog_level_to_ddwaf(spdlog::level::level_enum level)
     case spdlog::level::trace:
         return DDWAF_LOG_TRACE;
     case spdlog::level::debug:
-        return DDWAF_LOG_DEBUG;
+    // libddwaf is too verbose at debug level
     case spdlog::level::info:
         return DDWAF_LOG_INFO;
     case spdlog::level::warn:
@@ -101,10 +101,8 @@ void log_cb(DDWAF_LOG_LEVEL level, const char *function, const char *file,
     auto new_level = spdlog::level::off;
     switch (level) {
     case DDWAF_LOG_TRACE:
-        new_level = spdlog::level::trace;
-        break;
     case DDWAF_LOG_DEBUG:
-        new_level = spdlog::level::debug;
+        new_level = spdlog::level::trace;
         break;
     case DDWAF_LOG_INFO:
         new_level = spdlog::level::info;

appsec/tests/integration/src/main/groovy/com/datadog/appsec/php/mock_agent/TracesV04Handler.groovy

@@ -69,14 +69,15 @@ class TracesV04Handler implements Handler {
             }
             if (capturedTraces.size() == 0) {
                 log.info("Waiting up to $timeoutInMs ms for a trace")
+                long before = System.currentTimeMillis()
                 capturedTraces.wait(timeoutInMs)
                 if (savedError) {
                     throw new AssertionError('Error in mock agent http thread', savedError)
                 }
                 if (capturedTraces.size() == 0) {
                     throw new AssertionError("No trace gotten within $timeoutInMs ms" as Object)
                 } else {
-                    log.info('Wait finished. Last gotten: {}', capturedTraces.last())
+                    log.info('Wait finished after {} ms', System.currentTimeMillis() - before)
                     if (capturedTraces.size() > 1) {
                         log.info("There are a total of ${capturedTraces.size()} traces stored")
                     }

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

@@ -327,25 +327,26 @@ class RemoteConfigTests {
 
         doReq.call(200, '/hello.php', ['User-agent': 'dd-test-scanner-log-block'])
 
-        // changes env at the end of the request. The new rem cfg path is not transmitted
-        // to helper because appsec transmit rc path on req init
-        doReq.call(200, '/change_env.php?env=another-env')
-
-        // new rem cfg path is transmitted to the helper on config_sync
+        // changes env during the the request. The new rem cfg path is transmitted on a
+        // config_sync on rshutdown
         doReq.call(200, '/change_env.php?env=another-env')
 
+        // enable appsec for another-env
         applyRemoteConfig(newTarget, [
-                'datadog/2/ASM_FEATURES/asm_features_activation/config': [asm: [enabled: true]]])
+                'datadog/2/ASM_FEATURES/asm_features_activation2/config': [asm: [enabled: true]]])
 
         def status = doReq.call(null, '/hello.php', ['User-agent': 'dd-test-scanner-log-block'])
-        if (status == 200) {
-            assumeTrue(false, "Test fails because env of rc client is reset on ddtrace_sidecar_rinit(), " +
-                    "which runs before appsec rinit")
-        }
+        assert status == 403
+
+        // last request reset the target to INITIAL_TARGET, where appsec is disabled
+        // after this request, the target is reset to newTarget
+        status = doReq.call(null, '/change_env.php?env=another-env&ini', ['User-agent': 'dd-test-scanner-log-block'])
+        assert status == 200
+
+        status = doReq.call(null, '/hello.php', ['User-agent': 'dd-test-scanner-log-block'])
         assert status == 403
 
         dropRemoteConfig(INITIAL_TARGET)
-        dropRemoteConfig(newTarget)
     }
 
     private RemoteConfigRequest applyRemoteConfig(Target target, Map<String, Map> files) {

appsec/tests/integration/src/test/www/base/public/change_env.php

@@ -1,7 +1,10 @@
 <?php
 
 $root_span = \DDTrace\root_span();
-sleep(1);
-$root_span->env = $_GET['env'];
+if (key_exists('ini', $_GET)) {
+    ini_set('datadog.env', $_GET['env']);
+} else {
+    $root_span->env = $_GET['env'];
+}
 
 var_dump($root_span);