Move helper to sidecar remote config #2864

cataphract · 2024-09-30T10:12:33Z

Description

Reviewer checklist

Test coverage seems ok.
Appropriate labels assigned.

codecov-commenter · 2024-09-30T14:53:41Z

Codecov Report

Attention: Patch coverage is 50.81967% with 30 lines in your changes missing coverage. Please review.

Project coverage is 78.23%. Comparing base (c1fd94c) to head (1856dd0).

Files with missing lines	Patch %	Lines
appsec/src/extension/ddappsec.c	15.78%	14 Missing and 2 partials ⚠️
appsec/src/extension/request_lifecycle.c	45.00%	6 Missing and 5 partials ⚠️
appsec/src/extension/ddtrace.c	70.00%	1 Missing and 2 partials ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##             master    #2864      +/-   ##
============================================
- Coverage     78.36%   78.23%   -0.14%     
  Complexity     2526     2526              
============================================
  Files           173      173              
  Lines         18742    18742              
  Branches        976      981       +5     
============================================
- Hits          14688    14662      -26     
- Misses         3517     3540      +23     
- Partials        537      540       +3

Flag	Coverage Δ
appsec-extension	`68.49% <50.81%> (-0.60%)`	⬇️
tracer-extension	`78.10% <ø> (ø)`
tracer-php	`82.07% <ø> (-0.02%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines	Coverage Δ
appsec/src/extension/backtrace.c	`70.96% <100.00%> (ø)`
appsec/src/extension/commands/client_init.c	`83.14% <100.00%> (-1.07%)`	⬇️
appsec/src/extension/commands/config_sync.c	`100.00% <100.00%> (ø)`
appsec/src/extension/helper_process.c	`62.23% <100.00%> (ø)`
appsec/src/extension/msgpack_helpers.c	`55.05% <100.00%> (ø)`
appsec/src/extension/ddtrace.c	`60.00% <70.00%> (ø)`
appsec/src/extension/request_lifecycle.c	`63.15% <45.00%> (-1.17%)`	⬇️
appsec/src/extension/ddappsec.c	`69.45% <15.78%> (-4.64%)`	⬇️

... and 1 file with indirect coverage changes

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c1fd94c...1856dd0. Read the comment docs.

Anilm3

Still reviewing and testing, so far:

Some configurations are being ignored but I haven't checked why:

Ignoring config with key employee/ASM_DD/29.recommended.json/config; unsupported product

The capabilities are not correct, the list should be:

ASM_ACTIVATION
ASM_EXCLUSIONS
ASM_CUSTOM_BLOCKING_RESPONSE
ASM_REQUEST_BLOCKING
ASM_RESPONSE_BLOCKING
ASM_CUSTOM_RULES
ASM_TRUSTED_IPS
ASM_DD_RULES
ASM_IP_BLOCKING
ASM_USER_BLOCKING

And updates seem to performed on every poll even if no changes have been made to the actual configurations.

appsec/src/helper/main.cpp

appsec/src/extension/ddappsec.c

Anilm3 · 2024-10-03T12:23:23Z

appsec/src/helper/engine.cpp

 {
-    auto common = std::atomic_load(&common_);
-    common->subscribers.emplace_back(sub);
+    common_->subscribers.emplace_back(std::move(sub));


Are you sure removing the std::atomic_load doesn't create a race condition?

If multiple threads of execution access the same std::shared_ptr object without synchronization and any of those accesses uses a non-const member function of shared_ptr then a data race will occur unless all such access is performed through these functions, which are overloads of the corresponding atomic access functions (std::atomic_load, std::atomic_store, etc.).

Subscription is effectively part of the engine creation (in engine::from_settings), so this is not a problem. common_->subscribers is effectively final. What is changed on engine update is actually common_, at that one is updated atomically in engine::update.

Anilm3 · 2024-10-03T13:31:41Z

appsec/src/helper/engine.cpp

@@ -116,21 +118,22 @@ void engine::context::get_meta_and_metrics(
    }
 }

-engine::ptr engine::from_settings(const dds::engine_settings &eng_settings,
+std::unique_ptr<engine> engine::from_settings(


This is confusing, albeit not incorrect as far as I can tell, the engine is used as a shared_ptr but you return a unique_ptr.

The function of engine::from_settings is to create an engine and give ownership to the caller. If the instance will have shared ownership afterwards doesn't really concern this factory method.

Anilm3 · 2024-10-03T13:44:09Z

appsec/src/helper/remote_config/product.hpp


 namespace dds::remote_config {

 class product {
 public:
-    explicit product(std::string_view name, listener_base::shared_ptr listener)
-        : name_(name), listener_(std::move(listener))
+    explicit constexpr product(std::string_view name) : name_{name} {}


At this point...

using product = std::string_view;

AFAIC the alias vs newtype debate is conclusively settled :)

appsec/src/helper/remote_config/client.cpp

appsec/tests/helper/remote_config/listeners/config_aggregators/asm_aggregator_test.cpp

appsec/src/helper/remote_config/config.cpp

estringana · 2024-10-03T14:45:15Z

appsec/tests/integration/src/test/bin/enable_extensions.sh

@@ -24,6 +24,7 @@ if [[ -f /appsec/ddappsec.so && -d /project ]]; then
    echo datadog.appsec.helper_path=/appsec/libddappsec-helper.so
    echo datadog.appsec.helper_log_file=/tmp/logs/helper.log
    echo datadog.appsec.helper_log_level=info
+#    echo datadog.appsec.helper_log_level=debug


should this be not commented out?

Unfortunately debug is too verbose because of the WAF. It's good for debugging, but not more normal tests runs I think.

estringana · 2024-10-03T14:50:02Z

appsec/src/helper/remote_config/client.cpp

 }

-[[nodiscard]] protocol::get_configs_request client::generate_request() const
+bool client::poll()


Maybe I missed it but is there any tests on helper tests checking this? I saw the client_test got removed

You're right, I removed it. These are tested only on the integration tests. Reading of remote config is done by calling ddog_remote_config_read, which is a function implemented in Rust that we import.

Any test using the real Rust read function would writing in shared memory in the format expected by that rust code. By that time, we're having the test depend on implementation details of the rust code.

And by mocking ddog_remote_config_read, we're testing much useful stuff anymore.

appsec/src/helper/remote_config/client.cpp

appsec/src/helper/remote_config/config.cpp

pr-commenter · 2024-10-04T10:03:44Z

Benchmarks [ tracer ]

Benchmark execution time: 2024-10-07 18:06:29

Comparing candidate commit 1856dd0 in PR branch glopes/helper-rc with baseline commit c1fd94c in branch master.

Found 4 performance improvements and 0 performance regressions! Performance is the same for 174 metrics, 0 unstable metrics.

scenario:ContextPropagationBench/benchInject64Bit-opcache

🟩 execution_time [-459.588ns; -216.412ns] or [-6.009%; -2.830%]

scenario:SamplingRuleMatchingBench/benchRegexMatching3-opcache

🟩 execution_time [-504.894ns; -194.706ns] or [-7.184%; -2.771%]

scenario:SamplingRuleMatchingBench/benchRegexMatching4

🟩 execution_time [-542.518ns; -138.682ns] or [-7.832%; -2.002%]

scenario:TraceFlushBench/benchFlushTrace

🟩 execution_time [-1000.000ns; -1000.000ns] or [-50.000%; -50.000%]

pr-commenter · 2024-10-04T10:09:27Z

Benchmarks [ appsec ]

Benchmark execution time: 2024-10-07 18:13:46

Comparing candidate commit 1856dd0 in PR branch glopes/helper-rc with baseline commit c1fd94c in branch master.

Found 0 performance improvements and 3 performance regressions! Performance is the same for 9 metrics, 0 unstable metrics.

scenario:LaravelBench/benchLaravelOverhead-appsec

🟥 execution_time [+1.893ms; +2.237ms] or [+16.666%; +19.694%]

scenario:SymfonyBench/benchSymfonyOverhead-appsec

🟥 execution_time [+2.012ms; +2.256ms] or [+29.188%; +32.728%]

scenario:WordPressBench/benchWordPressOverhead-appsec

🟥 execution_time [+2.369ms; +2.835ms] or [+8.824%; +10.558%]

bwoebi · 2024-10-04T12:30:24Z

appsec/src/helper/remote_config/config.cpp

+    // check that the uid of the shared memory segment is the same as ours
+    struct ::stat shm_stat {};
+    if (::fstat(fd, &shm_stat) == -1) {
+        throw std::runtime_error{
+            "Call to fstat on memory segment failed: " + strerror_ts(errno)};
+    }
+    if (shm_stat.st_uid != ::getuid()) {
+        throw std::runtime_error{"Shared memory segment does not have the "
+                                 "expected owner. Expect our uid " +
+                                 std::to_string(::getuid()) + " but found " +
+                                 std::to_string(shm_stat.st_uid)};
+    }


Why are we checking that in the first place? Maybe do it within MAP_FAILED condition to give a better message, but no need to check that ahead of time.

if you're referring to the owner, to check whether the file was not actually put there by another, possibly unprivileged, user.

I see, that makes sense. Then the tracer should have the same check actually. However you should use geteuid, not getuid then. As the sidecar is bound to the effective user id too.

appsec/tests/extension/push_params_block_02.phpt

Anilm3

After an extensive review and testing, the only thing that remains a mystery is the missing ASM_TRUSTED_IPS capability.

Please fix that before merging.

cataphract changed the base branch from master to bob/live-debugger September 30, 2024 10:13

cataphract force-pushed the glopes/helper-rc branch 2 times, most recently from 2b26cf5 to 90639bf Compare September 30, 2024 14:46

cataphract mentioned this pull request Sep 30, 2024

Support for remote config in the appsec helper DataDog/libdatadog#657

Merged

bwoebi force-pushed the bob/live-debugger branch from 058b09e to 8cb68ab Compare September 30, 2024 17:31

cataphract force-pushed the glopes/helper-rc branch 2 times, most recently from f7f901d to a231e58 Compare September 30, 2024 19:17

bwoebi force-pushed the bob/live-debugger branch from 257c19e to e4b4417 Compare September 30, 2024 20:04

cataphract force-pushed the glopes/helper-rc branch from a231e58 to 9572bea Compare September 30, 2024 22:21

bwoebi force-pushed the bob/live-debugger branch from e4b4417 to def357c Compare October 1, 2024 00:03

cataphract force-pushed the glopes/helper-rc branch 2 times, most recently from 7d954a5 to 01b184e Compare October 1, 2024 10:52

bwoebi force-pushed the bob/live-debugger branch 2 times, most recently from fcf2f95 to 4e538d5 Compare October 1, 2024 13:28

cataphract force-pushed the glopes/helper-rc branch 4 times, most recently from b83fda5 to e86dbb8 Compare October 1, 2024 17:16

bwoebi force-pushed the bob/live-debugger branch 3 times, most recently from 6805e43 to 30a3015 Compare October 1, 2024 22:43

cataphract force-pushed the glopes/helper-rc branch 3 times, most recently from f68263b to e909749 Compare October 2, 2024 14:20

cataphract marked this pull request as ready for review October 2, 2024 14:32

cataphract requested review from a team as code owners October 2, 2024 14:32

bwoebi force-pushed the bob/live-debugger branch from ef4fdab to 4c61ac8 Compare October 2, 2024 14:42

Anilm3 requested changes Oct 3, 2024

View reviewed changes

estringana reviewed Oct 3, 2024

View reviewed changes

appsec/tests/helper/remote_config/listeners/config_aggregators/asm_aggregator_test.cpp Outdated Show resolved Hide resolved

Anilm3 reviewed Oct 3, 2024

View reviewed changes

appsec/src/helper/remote_config/config.cpp Outdated Show resolved Hide resolved

estringana reviewed Oct 3, 2024

View reviewed changes

Anilm3 reviewed Oct 3, 2024

View reviewed changes

appsec/src/helper/remote_config/client.cpp Show resolved Hide resolved

appsec/src/helper/remote_config/client.cpp Show resolved Hide resolved

appsec/src/helper/remote_config/config.cpp Outdated Show resolved Hide resolved

cataphract added 2 commits October 4, 2024 00:13

Use sidecar Remote Config impl in AppSec helper

6ea002c

wip

708dbe4

cataphract force-pushed the glopes/helper-rc branch from 8d142c2 to 708dbe4 Compare October 4, 2024 09:34

bwoebi reviewed Oct 4, 2024

View reviewed changes

Fixes after review

b424aab

cataphract force-pushed the glopes/helper-rc branch from 2c3f99b to b424aab Compare October 4, 2024 12:42

More review changes

bee3eef

cataphract force-pushed the glopes/helper-rc branch from f1b0cb1 to bee3eef Compare October 4, 2024 15:34

cataphract and others added 2 commits October 4, 2024 17:06

Helper: check shmem against process euid, rather than uid

5866db2

Merge branch 'master' into glopes/helper-rc

1e777f2

Anilm3 reviewed Oct 7, 2024

View reviewed changes

appsec/tests/extension/push_params_block_02.phpt Outdated Show resolved Hide resolved

Anilm3 added 2 commits October 7, 2024 11:13

Remove stray test

55faa29

Suppress appsec on certain tracer tests

2cbe028

Anilm3 approved these changes Oct 7, 2024

View reviewed changes

cataphract force-pushed the glopes/helper-rc branch 2 times, most recently from 93fa0f0 to 4b1cb4d Compare October 7, 2024 16:06

Fix Appsec integration tests

1856dd0

cataphract force-pushed the glopes/helper-rc branch from 4b1cb4d to 1856dd0 Compare October 7, 2024 17:38

Anilm3 merged commit 1c81b25 into master Oct 7, 2024
676 of 691 checks passed

Anilm3 deleted the glopes/helper-rc branch October 7, 2024 19:19

github-actions bot added this to the 1.4.0 milestone Oct 7, 2024

nclavaud mentioned this pull request Oct 14, 2024

[Bug]: Appsec warning: Remote config path too long #2892

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move helper to sidecar remote config #2864

Move helper to sidecar remote config #2864

cataphract commented Sep 30, 2024

codecov-commenter commented Sep 30, 2024 •

edited

Loading

Anilm3 left a comment

Anilm3 Oct 3, 2024

cataphract Oct 4, 2024

Anilm3 Oct 3, 2024

cataphract Oct 4, 2024

Anilm3 Oct 3, 2024

cataphract Oct 4, 2024

estringana Oct 3, 2024

cataphract Oct 4, 2024

estringana Oct 3, 2024

cataphract Oct 4, 2024

pr-commenter bot commented Oct 4, 2024 •

edited

Loading

pr-commenter bot commented Oct 4, 2024 •

edited

Loading

bwoebi Oct 4, 2024

cataphract Oct 4, 2024

bwoebi Oct 4, 2024

Anilm3 left a comment

Move helper to sidecar remote config #2864

Move helper to sidecar remote config #2864

Conversation

cataphract commented Sep 30, 2024

Description

Reviewer checklist

codecov-commenter commented Sep 30, 2024 • edited Loading

Codecov Report

Anilm3 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pr-commenter bot commented Oct 4, 2024 • edited Loading

Benchmarks [ tracer ]

scenario:ContextPropagationBench/benchInject64Bit-opcache

scenario:SamplingRuleMatchingBench/benchRegexMatching3-opcache

scenario:SamplingRuleMatchingBench/benchRegexMatching4

scenario:TraceFlushBench/benchFlushTrace

pr-commenter bot commented Oct 4, 2024 • edited Loading

Benchmarks [ appsec ]

scenario:LaravelBench/benchLaravelOverhead-appsec

scenario:SymfonyBench/benchSymfonyOverhead-appsec

scenario:WordPressBench/benchWordPressOverhead-appsec

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Anilm3 left a comment

Choose a reason for hiding this comment

codecov-commenter commented Sep 30, 2024 •

edited

Loading

pr-commenter bot commented Oct 4, 2024 •

edited

Loading

pr-commenter bot commented Oct 4, 2024 •

edited

Loading