Move helper to sidecar remote config by cataphract · Pull Request #2864 · DataDog/dd-trace-php

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

❌ 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).
⚠️ Report is 561 commits behind head on master.

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 ⚠️

❌ Your patch status has failed because the patch coverage (50.81%) is below the target coverage (90.00%). You can increase the patch coverage or adjust the target coverage.

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.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

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.

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

 {
-    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

 }

-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

 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 :)

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

    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

 }

-[[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.

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

+    // 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.

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

Comment thread appsec/tests/helper/remote_config/listeners/config_aggregators/asm_aggregator_test.cpp Outdated

Anilm3 reviewed Oct 3, 2024

View reviewed changes

Comment thread appsec/src/helper/remote_config/config.cpp Outdated

estringana reviewed Oct 3, 2024

View reviewed changes

Anilm3 reviewed Oct 3, 2024

View reviewed changes

Comment thread appsec/src/helper/remote_config/client.cpp

Comment thread appsec/src/helper/remote_config/client.cpp

Comment thread appsec/src/helper/remote_config/config.cpp Outdated

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

Comment thread appsec/tests/extension/push_params_block_02.phpt Outdated

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

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

Conversation

cataphract commented Sep 30, 2024

Description

Reviewer checklist

Uh oh!

codecov-commenter commented Sep 30, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Anilm3 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

pr-commenter Bot commented Oct 4, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Benchmarks [ tracer ]

scenario:ContextPropagationBench/benchInject64Bit-opcache

scenario:SamplingRuleMatchingBench/benchRegexMatching3-opcache

scenario:SamplingRuleMatchingBench/benchRegexMatching4

scenario:TraceFlushBench/benchFlushTrace

Uh oh!

pr-commenter Bot commented Oct 4, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Benchmarks [ appsec ]

scenario:LaravelBench/benchLaravelOverhead-appsec

scenario:SymfonyBench/benchSymfonyOverhead-appsec

scenario:WordPressBench/benchWordPressOverhead-appsec

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Anilm3 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

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