AppSec: improve behavior with empty DD_SERVICE/DD_ENV #2888

cataphract · 2024-10-10T16:11:36Z

Description

Reviewer checklist

Test coverage seems ok.
Appropriate labels assigned.

codecov-commenter · 2024-10-10T16:20:00Z

Codecov Report

Attention: Patch coverage is 22.22222% with 14 lines in your changes missing coverage. Please review.

Project coverage is 78.21%. Comparing base (a3e85fc) to head (c59e00d).

Files with missing lines	Patch %	Lines
appsec/src/extension/request_lifecycle.c	18.75%	9 Missing and 4 partials ⚠️
appsec/src/extension/commands/client_init.c	0.00%	1 Missing ⚠️

Additional details and impacted files

@@                Coverage Diff                @@
##             bob/rc-tags    #2888      +/-   ##
=================================================
- Coverage          78.24%   78.21%   -0.03%     
  Complexity          2526     2526              
=================================================
  Files                173      173              
  Lines              18742    18749       +7     
  Branches             981      988       +7     
=================================================
  Hits               14664    14664              
- Misses              3538     3544       +6     
- Partials             540      541       +1

Flag	Coverage Δ
appsec-extension	`68.37% <22.22%> (-0.12%)`	⬇️
tracer-extension	`78.10% <ø> (ø)`
tracer-php	`82.09% <ø> (ø)`

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

Files with missing lines	Coverage Δ
appsec/src/extension/commands/config_sync.c	`100.00% <100.00%> (ø)`
appsec/src/extension/ddappsec.c	`70.56% <ø> (+1.10%)`	⬆️
appsec/src/extension/commands/client_init.c	`83.14% <0.00%> (ø)`
appsec/src/extension/request_lifecycle.c	`61.50% <18.75%> (-1.66%)`	⬇️

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 a3e85fc...c59e00d. Read the comment docs.

Anilm3 · 2024-10-10T16:16:10Z

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


Not sure about this, the trace level with RC has just too much crap, which makes it difficult to analyse anything WAF-specific...

the previous behavior resulted in 90% of the messages being from libddwaf, esp during initialization, with pearls like this:

[2024-10-10 14:01:39.863][debug][181] Parsed rule crs-932-180 [2024-10-10 14:01:39.863][debug][181] Found address server.request.headers.no_cookies [2024-10-10 14:01:39.863][debug][181] Found address server.request.headers.no_cookies [2024-10-10 14:01:39.863][debug][181] Found address server.request.headers.no_cookies [2024-10-10 14:01:39.863][debug][181] Found address server.request.headers.no_cookies

makes it difficult to read the logs

I've looked at the code of libddwaf and the only trace messages are about matches, which make them lower-volume, and actually look more important than most debug messages.

We had to demote match logs a while ago because of PII concerns.

appsec/src/extension/request_lifecycle.c

pr-commenter · 2024-10-10T16:46:17Z

Benchmarks [ appsec ]

Benchmark execution time: 2024-10-11 18:19:44

Comparing candidate commit c59e00d in PR branch glopes/empty-service-env with baseline commit a3e85fc in branch bob/rc-tags.

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

bwoebi · 2024-10-10T18:16:41Z

appsec/src/extension/request_lifecycle.c

+static bool _has_empty_service_or_env()
 {
+    return zend_string_equals_literal(get_DD_ENV(), "") ||
+           zend_string_equals_literal(get_DD_SERVICE(), "");
+}


You should cache that at rinit, it's possible to change these via ini_set() too at runtime... Which will then not reflect the truth for you in rshutdown.

Can't I use get_global_DD_ENV() here for the same effect?

I've moved it to startup with get_global

No, get_global is definitely inferior. It will also bypass all perdir changes. Like vhost specific config.

Just cache the value in a thread-local variable after rinit.

pr-commenter · 2024-10-11T18:10:22Z

Benchmarks [ tracer ]

Benchmark execution time: 2024-10-11 18:13:18

Comparing candidate commit c59e00d in PR branch glopes/empty-service-env with baseline commit a3e85fc in branch bob/rc-tags.

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

scenario:MessagePackSerializationBench/benchMessagePackSerialization

🟩 execution_time [-8.926µs; -5.974µs] or [-5.081%; -3.401%]

scenario:PDOBench/benchPDOBaseline

🟩 execution_time [-13.692µs; -12.480µs] or [-7.154%; -6.520%]

scenario:PDOBench/benchPDOBaseline-opcache

🟩 execution_time [-15.180µs; -7.102µs] or [-8.031%; -3.757%]

scenario:SamplingRuleMatchingBench/benchRegexMatching2-opcache

🟥 execution_time [+157.462ns; +378.538ns] or [+2.287%; +5.497%]

scenario:TraceFlushBench/benchFlushTrace

🟥 execution_time [+1000.000ns; +1000.000ns] or [+100.000%; +100.000%]

scenario:TraceSerializationBench/benchSerializeTrace

🟩 execution_time [-10.346µs; -5.854µs] or [-5.355%; -3.030%]

bwoebi

Looks good to me now.

Anilm3

Lgtm

* Send git tags via remote config Signed-off-by: Bob Weinand <bob.weinand@datadoghq.com> * Submit root span data when service/env/version ini is changed Signed-off-by: Bob Weinand <bob.weinand@datadoghq.com> * AppSec: improve behavior with empty DD_SERVICE/DD_ENV (#2888) --------- Signed-off-by: Bob Weinand <bob.weinand@datadoghq.com> Co-authored-by: Gustavo Lopes <gustavo.lopes@datadoghq.com>

cataphract requested a review from a team as a code owner October 10, 2024 16:11

github-actions bot added tracing area:asm labels Oct 10, 2024

Anilm3 reviewed Oct 10, 2024

View reviewed changes

bwoebi reviewed Oct 10, 2024

View reviewed changes

cataphract force-pushed the glopes/empty-service-env branch 2 times, most recently from ae6398d to 8e9b214 Compare October 11, 2024 17:41

cataphract requested review from a team as code owners October 11, 2024 17:41

cataphract changed the base branch from master to bob/rc-tags October 11, 2024 17:42

AppSec: improve behavior with empty DD_SERVICE/DD_ENV

c59e00d

cataphract force-pushed the glopes/empty-service-env branch from 8e9b214 to c59e00d Compare October 11, 2024 17:44

bwoebi approved these changes Oct 11, 2024

View reviewed changes

Anilm3 approved these changes Oct 11, 2024

View reviewed changes

bwoebi merged commit ac7eb62 into bob/rc-tags Oct 14, 2024
685 of 700 checks passed

bwoebi deleted the glopes/empty-service-env branch October 14, 2024 02:01

bwoebi added this to the 1.4.1 milestone Oct 14, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

AppSec: improve behavior with empty DD_SERVICE/DD_ENV #2888

AppSec: improve behavior with empty DD_SERVICE/DD_ENV #2888

cataphract commented Oct 10, 2024

codecov-commenter commented Oct 10, 2024 •

edited

Loading

Anilm3 Oct 10, 2024

cataphract Oct 10, 2024

cataphract Oct 11, 2024

Anilm3 Oct 11, 2024

pr-commenter bot commented Oct 10, 2024 •

edited

Loading

bwoebi Oct 10, 2024 •

edited

Loading

cataphract Oct 11, 2024

cataphract Oct 11, 2024

bwoebi Oct 11, 2024

bwoebi Oct 11, 2024

pr-commenter bot commented Oct 11, 2024 •

edited

Loading

bwoebi left a comment

Anilm3 left a comment

AppSec: improve behavior with empty DD_SERVICE/DD_ENV #2888

AppSec: improve behavior with empty DD_SERVICE/DD_ENV #2888

Conversation

cataphract commented Oct 10, 2024

Description

Reviewer checklist

codecov-commenter commented Oct 10, 2024 • edited Loading

Codecov Report

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 10, 2024 • edited Loading

Benchmarks [ appsec ]

bwoebi Oct 10, 2024 • edited Loading

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 11, 2024 • edited Loading

Benchmarks [ tracer ]

scenario:MessagePackSerializationBench/benchMessagePackSerialization

scenario:PDOBench/benchPDOBaseline

scenario:PDOBench/benchPDOBaseline-opcache

scenario:SamplingRuleMatchingBench/benchRegexMatching2-opcache

scenario:TraceFlushBench/benchFlushTrace

scenario:TraceSerializationBench/benchSerializeTrace

bwoebi left a comment

Choose a reason for hiding this comment

Anilm3 left a comment

Choose a reason for hiding this comment

codecov-commenter commented Oct 10, 2024 •

edited

Loading

pr-commenter bot commented Oct 10, 2024 •

edited

Loading

bwoebi Oct 10, 2024 •

edited

Loading

pr-commenter bot commented Oct 11, 2024 •

edited

Loading