From 8e9b214c5772c6b916dc944c56ea2480e433753a Mon Sep 17 00:00:00 2001 From: Gustavo Lopes Date: Thu, 10 Oct 2024 17:11:05 +0100 Subject: [PATCH] AppSec: improve behavior with empty DD_SERVICE/DD_ENV --- appsec/src/extension/commands/client_init.c | 4 +-- appsec/src/extension/commands/config_sync.c | 5 +++ appsec/src/extension/ddappsec.c | 16 --------- appsec/src/extension/request_lifecycle.c | 34 ++++++++++++++++-- appsec/src/helper/client.cpp | 13 +++++-- .../listeners/asm_features_listener.hpp | 1 + appsec/src/helper/service_config.hpp | 36 +++++++++++++++++-- appsec/src/helper/subscriber/waf.cpp | 6 ++-- .../php/mock_agent/TracesV04Handler.groovy | 3 +- .../php/integration/RemoteConfigTests.groovy | 23 ++++++------ .../src/test/www/base/public/change_env.php | 7 ++-- 11 files changed, 104 insertions(+), 44 deletions(-) diff --git a/appsec/src/extension/commands/client_init.c b/appsec/src/extension/commands/client_init.c index a20513129d2..20cbef65023 100644 --- a/appsec/src/extension/commands/client_init.c +++ b/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); } diff --git a/appsec/src/extension/commands/config_sync.c b/appsec/src/extension/commands/config_sync.c index c7fcbddd583..40e1a9d2352 100644 --- a/appsec/src/extension/commands/config_sync.c +++ b/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 @@ -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); } diff --git a/appsec/src/extension/ddappsec.c b/appsec/src/extension/ddappsec.c index eb840d940aa..f886165ff8e 100644 --- a/appsec/src/extension/ddappsec.c +++ b/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; diff --git a/appsec/src/extension/request_lifecycle.c b/appsec/src/extension/request_lifecycle.c index 3aa826d903d..36ea5653dfc 100644 --- a/appsec/src/extension/request_lifecycle.c +++ b/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) diff --git a/appsec/src/helper/client.cpp b/appsec/src/helper/client.cpp index 17b9470176d..545c6005bbd 100644 --- a/appsec/src/helper/client.cpp +++ b/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); diff --git a/appsec/src/helper/remote_config/listeners/asm_features_listener.hpp b/appsec/src/helper/remote_config/listeners/asm_features_listener.hpp index 9ca500549c5..d18aed760ae 100644 --- a/appsec/src/helper/remote_config/listeners/asm_features_listener.hpp +++ b/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(); } diff --git a/appsec/src/helper/service_config.hpp b/appsec/src/helper/service_config.hpp index 475ddf8a376..50f277fb45a 100644 --- a/appsec/src/helper/service_config.hpp +++ b/appsec/src/helper/service_config.hpp @@ -8,16 +8,46 @@ #include #include #include +#include #include 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: diff --git a/appsec/src/helper/subscriber/waf.cpp b/appsec/src/helper/subscriber/waf.cpp index 4a5062c0061..0b843f63a7a 100644 --- a/appsec/src/helper/subscriber/waf.cpp +++ b/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; diff --git a/appsec/tests/integration/src/main/groovy/com/datadog/appsec/php/mock_agent/TracesV04Handler.groovy b/appsec/tests/integration/src/main/groovy/com/datadog/appsec/php/mock_agent/TracesV04Handler.groovy index b6882c8dc34..8168387842e 100644 --- a/appsec/tests/integration/src/main/groovy/com/datadog/appsec/php/mock_agent/TracesV04Handler.groovy +++ b/appsec/tests/integration/src/main/groovy/com/datadog/appsec/php/mock_agent/TracesV04Handler.groovy @@ -69,6 +69,7 @@ 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) @@ -76,7 +77,7 @@ class TracesV04Handler implements Handler { 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") } diff --git a/appsec/tests/integration/src/test/groovy/com/datadog/appsec/php/integration/RemoteConfigTests.groovy b/appsec/tests/integration/src/test/groovy/com/datadog/appsec/php/integration/RemoteConfigTests.groovy index b06027dca1c..0ca7be22f3d 100644 --- a/appsec/tests/integration/src/test/groovy/com/datadog/appsec/php/integration/RemoteConfigTests.groovy +++ b/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 files) { diff --git a/appsec/tests/integration/src/test/www/base/public/change_env.php b/appsec/tests/integration/src/test/www/base/public/change_env.php index 129a83b2fd9..d40f4b18185 100644 --- a/appsec/tests/integration/src/test/www/base/public/change_env.php +++ b/appsec/tests/integration/src/test/www/base/public/change_env.php @@ -1,7 +1,10 @@ env = $_GET['env']; +if (key_exists('ini', $_GET)) { + ini_set('datadog.env', $_GET['env']); +} else { + $root_span->env = $_GET['env']; +} var_dump($root_span);