Skip to content

Commit 7970d87

Browse files
authored
fix(asm/refactor): avoid store context data if appsec is disabled (#4163)
## Description Avoid store context data if appsec is disabled ``` +-----------------------------------------------------+----------+-------------------------+ | Benchmark | 1.x | branch | +=====================================================+==========+=========================+ | sethttpmeta-all-enabled | 33.2 us | 23.1 us: 1.44x faster | +-----------------------------------------------------+----------+-------------------------+ | sethttpmeta-no-useragentvariant | 28.5 us | 21.5 us: 1.32x faster | +-----------------------------------------------------+----------+-------------------------+ | sethttpmeta-all-disabled | 10.7 us | 8.15 us: 1.31x faster | +-----------------------------------------------------+----------+-------------------------+ | sethttpmeta-useragentvariant_exists_1 | 26.9 us | 22.6 us: 1.19x faster | +-----------------------------------------------------+----------+-------------------------+ | sethttpmeta-useragentvariant_exists_2 | 26.4 us | 25.3 us: 1.04x faster | +-----------------------------------------------------+----------+-------------------------+ | sethttpmeta-useragentvariant_exists_3 | 25.9 us | 25.1 us: 1.03x faster | +-----------------------------------------------------+----------+-------------------------+ | sethttpmeta-no-collectipvariant | 25.2 us | 23.3 us: 1.08x faster | +-----------------------------------------------------+----------+-------------------------+ | sethttpmeta-collectipvariant_exists | 35.6 us | 32.5 us: 1.10x faster | +-----------------------------------------------------+----------+-------------------------+ | sethttpmeta-obfuscation-worst-case-implicit-query | 25.1 us | 23.5 us: 1.07x faster | +-----------------------------------------------------+----------+-------------------------+ | sethttpmeta-obfuscation-worst-case-explicit-query | 25.3 us | 23.3 us: 1.09x faster | +-----------------------------------------------------+----------+-------------------------+ | sethttpmeta-obfuscation-regular-case-implicit-query | 26.4 us | 23.1 us: 1.14x faster | +-----------------------------------------------------+----------+-------------------------+ | sethttpmeta-obfuscation-regular-case-explicit-query | 26.1 us | 23.4 us: 1.11x faster | +-----------------------------------------------------+----------+-------------------------+ | sethttpmeta-obfuscation-no-query | 25.8 us | 24.2 us: 1.07x faster | +-----------------------------------------------------+----------+-------------------------+ | sethttpmeta-obfuscation-send-querystring-disabled | 26.1 us | 23.3 us: 1.12x faster | +-----------------------------------------------------+----------+-------------------------+ | sethttpmeta-obfuscation-disabled | 25.6 us | 23.3 us: 1.10x faster | +-----------------------------------------------------+----------+-------------------------+ | Geometric mean | (ref) | 1.12x faster | +-----------------------------------------------------+----------+-------------------------+ ``` ## Reviewer Checklist - [ ] Title is accurate. - [ ] Description motivates each change. - [ ] No unnecessary changes were introduced in this PR. - [ ] PR cannot be broken up into smaller PRs. - [ ] Avoid breaking [API](https://ddtrace.readthedocs.io/en/stable/versioning.html#interfaces) changes unless absolutely necessary. - [ ] Tests provided or description of manual testing performed is included in the code or PR. - [ ] Release note has been added for fixes and features, or else `changelog/no-changelog` label added. - [ ] All relevant GitHub issues are correctly linked. - [ ] Backports are identified and tagged with Mergifyio. - [ ] Add to milestone.
1 parent 55cdccf commit 7970d87

File tree

4 files changed

+107
-92
lines changed

4 files changed

+107
-92
lines changed

ddtrace/contrib/trace_utils.py

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -461,7 +461,7 @@ def set_http_meta(
461461
if retries_remain is not None:
462462
span._set_str_tag(http.RETRIES_REMAIN, str(retries_remain))
463463

464-
if config._appsec:
464+
if config._appsec_enabled:
465465
status_code = str(status_code) if status_code is not None else None
466466

467467
_context.set_items(

tests/appsec/test_processor.py

Lines changed: 57 additions & 45 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,5 @@
11
import json
22
import os.path
3-
import sys
43

54
import pytest
65
from six import ensure_binary
@@ -27,9 +26,10 @@
2726
RULES_MISSING_PATH = os.path.join(ROOT_DIR, "nonexistent")
2827

2928

30-
class Config(object):
31-
def __init__(self):
32-
self.is_header_tracing_configured = False
29+
@pytest.fixture
30+
def tracer_appsec(tracer):
31+
with override_global_config(dict(_appsec_enabled=True)):
32+
yield _enable_appsec(tracer)
3333

3434

3535
def _enable_appsec(tracer):
@@ -39,6 +39,11 @@ def _enable_appsec(tracer):
3939
return tracer
4040

4141

42+
class Config(object):
43+
def __init__(self):
44+
self.is_header_tracing_configured = False
45+
46+
4247
def test_transform_headers():
4348
transformed = _transform_headers(
4449
{
@@ -56,8 +61,8 @@ def test_transform_headers():
5661
assert set(transformed["foo"]) == {"bar1", "bar2", "bar3"}
5762

5863

59-
def test_enable(tracer):
60-
_enable_appsec(tracer)
64+
def test_enable(tracer_appsec):
65+
tracer = tracer_appsec
6166

6267
with tracer.trace("test", span_type=SpanTypes.WEB) as span:
6368
set_http_meta(span, {}, raw_uri="http://example.com/.git", status_code="404")
@@ -85,35 +90,35 @@ def test_enable_bad_rules(rule, exc, tracer):
8590
_enable_appsec(tracer)
8691

8792

88-
def test_retain_traces(tracer):
89-
_enable_appsec(tracer)
93+
def test_retain_traces(tracer_appsec):
94+
tracer = tracer_appsec
9095

9196
with tracer.trace("test", span_type=SpanTypes.WEB) as span:
9297
set_http_meta(span, {}, raw_uri="http://example.com/.git", status_code="404")
9398

9499
assert span.context.sampling_priority == USER_KEEP
95100

96101

97-
def test_valid_json(tracer):
98-
_enable_appsec(tracer)
102+
def test_valid_json(tracer_appsec):
103+
tracer = tracer_appsec
99104

100105
with tracer.trace("test", span_type=SpanTypes.WEB) as span:
101106
set_http_meta(span, {}, raw_uri="http://example.com/.git", status_code="404")
102107

103108
assert "triggers" in json.loads(span.get_tag(APPSEC_JSON))
104109

105110

106-
def test_header_attack(tracer):
107-
_enable_appsec(tracer)
111+
def test_header_attack(tracer_appsec):
112+
tracer = tracer_appsec
108113

109114
with tracer.trace("test", span_type=SpanTypes.WEB) as span:
110115
set_http_meta(span, Config(), request_headers={"User-Agent": "Arachni/v1", "user-agent": "aa"})
111116

112117
assert "triggers" in json.loads(span.get_tag(APPSEC_JSON))
113118

114119

115-
def test_headers_collection(tracer):
116-
_enable_appsec(tracer)
120+
def test_headers_collection(tracer_appsec):
121+
tracer = tracer_appsec
117122

118123
with tracer.trace("test", span_type=SpanTypes.WEB) as span:
119124

@@ -148,17 +153,20 @@ def test_headers_collection(tracer):
148153
],
149154
)
150155
def test_appsec_cookies_no_collection_snapshot(tracer):
151-
_enable_appsec(tracer)
152-
with tracer.trace("test", span_type=SpanTypes.WEB) as span:
153-
set_http_meta(
154-
span,
155-
{},
156-
raw_uri="http://example.com/.git",
157-
status_code="404",
158-
request_cookies={"cookie1": "im the cookie1"},
159-
)
156+
# We use tracer instead of tracer_appsec because snapshot is looking for tracer fixture and not understands
157+
# other fixtures
158+
with override_global_config(dict(_appsec_enabled=True)):
159+
_enable_appsec(tracer)
160+
with tracer.trace("test", span_type=SpanTypes.WEB) as span:
161+
set_http_meta(
162+
span,
163+
{},
164+
raw_uri="http://example.com/.git",
165+
status_code="404",
166+
request_cookies={"cookie1": "im the cookie1"},
167+
)
160168

161-
assert "triggers" in json.loads(span.get_tag(APPSEC_JSON))
169+
assert "triggers" in json.loads(span.get_tag(APPSEC_JSON))
162170

163171

164172
@snapshot(
@@ -191,16 +199,15 @@ def test_appsec_body_no_collection_snapshot(tracer):
191199
],
192200
)
193201
def test_appsec_span_tags_snapshot(tracer):
194-
_enable_appsec(tracer)
195-
196-
with tracer.trace("test", span_type=SpanTypes.WEB) as span:
197-
span.set_tag("http.url", "http://example.com/.git")
198-
set_http_meta(span, {}, raw_uri="http://example.com/.git", status_code="404")
202+
with override_global_config(dict(_appsec_enabled=True)):
203+
_enable_appsec(tracer)
204+
with tracer.trace("test", span_type=SpanTypes.WEB) as span:
205+
span.set_tag("http.url", "http://example.com/.git")
206+
set_http_meta(span, {}, raw_uri="http://example.com/.git", status_code="404")
199207

200-
assert "triggers" in json.loads(span.get_tag(APPSEC_JSON))
208+
assert "triggers" in json.loads(span.get_tag(APPSEC_JSON))
201209

202210

203-
@pytest.mark.skipif(sys.version_info > (3, 5, 0), reason="Python 2.7 and Python 3.5 test")
204211
@snapshot(
205212
include_tracer=True,
206213
ignores=[
@@ -210,19 +217,20 @@ def test_appsec_span_tags_snapshot(tracer):
210217
],
211218
)
212219
def test_appsec_span_tags_snapshot_with_errors(tracer):
213-
with override_env(dict(DD_APPSEC_RULES=os.path.join(ROOT_DIR, "rules-with-2-errors.json"))):
214-
_enable_appsec(tracer)
215-
with tracer.trace("test", span_type=SpanTypes.WEB) as span:
216-
span.set_tag("http.url", "http://example.com/.git")
217-
set_http_meta(span, {}, raw_uri="http://example.com/.git", status_code="404")
220+
with override_global_config(dict(_appsec_enabled=True)):
221+
with override_env(dict(DD_APPSEC_RULES=os.path.join(ROOT_DIR, "rules-with-2-errors.json"))):
222+
_enable_appsec(tracer)
223+
with tracer.trace("test", span_type=SpanTypes.WEB) as span:
224+
span.set_tag("http.url", "http://example.com/.git")
225+
set_http_meta(span, {}, raw_uri="http://example.com/.git", status_code="404")
218226

219-
assert span.get_tag(APPSEC_JSON) is None
227+
assert span.get_tag(APPSEC_JSON) is None
220228

221229

222230
def test_appsec_span_rate_limit(tracer):
223-
with override_env(dict(DD_APPSEC_TRACE_RATE_LIMIT="1")):
224-
_enable_appsec(tracer)
225231

232+
with override_global_config(dict(_appsec_enabled=True)), override_env(dict(DD_APPSEC_TRACE_RATE_LIMIT="1")):
233+
_enable_appsec(tracer)
226234
# we have 2 spans going through with a rate limit of 1: this is because the first span will update the rate
227235
# limiter last update timestamp. In other words, we need a first call to reset the rate limiter's clock
228236
# DEV: aligning rate limiter clock with this span (this
@@ -299,8 +307,8 @@ def test_obfuscation_parameter_key_and_value_invalid_regex():
299307
assert processor.enabled
300308

301309

302-
def test_obfuscation_parameter_value_unconfigured_not_matching(tracer):
303-
_enable_appsec(tracer)
310+
def test_obfuscation_parameter_value_unconfigured_not_matching(tracer_appsec):
311+
tracer = tracer_appsec
304312

305313
with tracer.trace("test", span_type=SpanTypes.WEB) as span:
306314
set_http_meta(span, Config(), raw_uri="http://example.com/.git?hello=goodbye", status_code="404")
@@ -312,8 +320,8 @@ def test_obfuscation_parameter_value_unconfigured_not_matching(tracer):
312320
assert "<Redacted>" not in span.get_tag("_dd.appsec.json")
313321

314322

315-
def test_obfuscation_parameter_value_unconfigured_matching(tracer):
316-
_enable_appsec(tracer)
323+
def test_obfuscation_parameter_value_unconfigured_matching(tracer_appsec):
324+
tracer = tracer_appsec
317325

318326
with tracer.trace("test", span_type=SpanTypes.WEB) as span:
319327
set_http_meta(span, Config(), raw_uri="http://example.com/.git?password=goodbye", status_code="404")
@@ -326,7 +334,9 @@ def test_obfuscation_parameter_value_unconfigured_matching(tracer):
326334

327335

328336
def test_obfuscation_parameter_value_configured_not_matching(tracer):
329-
with override_env(dict(DD_APPSEC_OBFUSCATION_PARAMETER_VALUE_REGEXP="token")):
337+
with override_global_config(dict(_appsec_enabled=True)), override_env(
338+
dict(DD_APPSEC_OBFUSCATION_PARAMETER_VALUE_REGEXP="token")
339+
):
330340

331341
_enable_appsec(tracer)
332342

@@ -341,7 +351,9 @@ def test_obfuscation_parameter_value_configured_not_matching(tracer):
341351

342352

343353
def test_obfuscation_parameter_value_configured_matching(tracer):
344-
with override_env(dict(DD_APPSEC_OBFUSCATION_PARAMETER_VALUE_REGEXP="token")):
354+
with override_global_config(dict(_appsec_enabled=True)), override_env(
355+
dict(DD_APPSEC_OBFUSCATION_PARAMETER_VALUE_REGEXP="token")
356+
):
345357

346358
_enable_appsec(tracer)
347359

tests/contrib/django/test_django_appsec.py

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -61,7 +61,7 @@ def test_django_request_cookies(client, test_spans, tracer):
6161

6262

6363
def test_django_request_cookies_attack(client, test_spans, tracer):
64-
with override_global_config(dict(_appsec_enabled=False)):
64+
with override_global_config(dict(_appsec_enabled=True)):
6565
with override_env(dict(DD_APPSEC_RULES=RULES_GOOD_PATH)):
6666
client.cookies.load({"attack": "1' or '1' = '1'"})
6767
root_span, _ = _aux_appsec_get_root_span(client, test_spans, tracer)

tests/contrib/pylons/test_pylons.py

Lines changed: 48 additions & 45 deletions
Original file line numberDiff line numberDiff line change
@@ -189,31 +189,32 @@ def test_exc_client_failure(self):
189189
assert span.get_tag(ERROR_STACK) is None
190190

191191
def test_success_200(self, query_string=""):
192-
if query_string:
193-
fqs = "?" + query_string
194-
else:
195-
fqs = ""
196-
res = self.app.get(url_for(controller="root", action="index") + fqs)
197-
assert res.status == 200
198-
199-
spans = self.pop_spans()
200-
assert spans, spans
201-
assert len(spans) == 1
202-
span = spans[0]
192+
with override_global_config(dict(_appsec_enabled=True)):
193+
if query_string:
194+
fqs = "?" + query_string
195+
else:
196+
fqs = ""
197+
res = self.app.get(url_for(controller="root", action="index") + fqs)
198+
assert res.status == 200
203199

204-
assert_is_measured(span)
205-
assert span.service == "web"
206-
assert span.resource == "root.index"
207-
assert_span_http_status_code(span, 200)
208-
if config.pylons.trace_query_string:
209-
assert span.get_tag(http.QUERY_STRING) == query_string
210-
if config._appsec:
211-
assert _context.get_item("http.request.uri", span=span) == "http://localhost:80/?" + query_string
212-
else:
213-
assert http.QUERY_STRING not in span.get_tags()
214-
if config._appsec:
215-
assert _context.get_item("http.request.uri", span=span) == "http://localhost:80/"
216-
assert span.error == 0
200+
spans = self.pop_spans()
201+
assert spans, spans
202+
assert len(spans) == 1
203+
span = spans[0]
204+
205+
assert_is_measured(span)
206+
assert span.service == "web"
207+
assert span.resource == "root.index"
208+
assert_span_http_status_code(span, 200)
209+
if config.pylons.trace_query_string:
210+
assert span.get_tag(http.QUERY_STRING) == query_string
211+
if config._appsec:
212+
assert _context.get_item("http.request.uri", span=span) == "http://localhost:80/?" + query_string
213+
else:
214+
assert http.QUERY_STRING not in span.get_tags()
215+
if config._appsec:
216+
assert _context.get_item("http.request.uri", span=span) == "http://localhost:80/"
217+
assert span.error == 0
217218

218219
def test_query_string(self):
219220
return self.test_success_200("foo=bar")
@@ -504,7 +505,7 @@ def test_response_headers(self):
504505
assert spans[0].get_tag("http.response.headers.custom-header") == "value"
505506

506507
def test_pylons_cookie_sql_injection(self):
507-
with override_env(dict(DD_APPSEC_RULES=RULES_GOOD_PATH)):
508+
with override_global_config(dict(_appsec_enabled=True)), override_env(dict(DD_APPSEC_RULES=RULES_GOOD_PATH)):
508509
self.tracer._appsec_enabled = True
509510
# Hack: need to pass an argument to configure so that the processors are recreated
510511
self.tracer.configure(api_version="v0.4")
@@ -521,18 +522,19 @@ def test_pylons_cookie_sql_injection(self):
521522
assert span["attack"] == "w00tw00t.at.isc.sans.dfind"
522523

523524
def test_pylons_cookie(self):
524-
self.tracer._appsec_enabled = True
525-
# Hack: need to pass an argument to configure so that the processors are recreated
526-
self.tracer.configure(api_version="v0.4")
527-
self.app.cookies = {"testingcookie_key": "testingcookie_value"}
528-
self.app.get(url_for(controller="root", action="index"))
525+
with override_global_config(dict(_appsec_enabled=True)):
526+
self.tracer._appsec_enabled = True
527+
# Hack: need to pass an argument to configure so that the processors are recreated
528+
self.tracer.configure(api_version="v0.4")
529+
self.app.cookies = {"testingcookie_key": "testingcookie_value"}
530+
self.app.get(url_for(controller="root", action="index"))
529531

530-
spans = self.pop_spans()
531-
root_span = spans[0]
532+
spans = self.pop_spans()
533+
root_span = spans[0]
532534

533-
assert root_span.get_tag("_dd.appsec.json") is None
534-
span = _context.get_item("http.request.cookies", span=root_span)
535-
assert span["testingcookie_key"] == "testingcookie_value"
535+
assert root_span.get_tag("_dd.appsec.json") is None
536+
span = _context.get_item("http.request.cookies", span=root_span)
537+
assert span["testingcookie_key"] == "testingcookie_value"
536538

537539
def test_pylons_body_urlencoded(self):
538540
with self.override_global_config(dict(_appsec_enabled=True)):
@@ -752,20 +754,21 @@ def test_request_method_post_200(self):
752754
assert spans[0].get_tag("http.method") == "POST"
753755

754756
def test_pylon_path_params(self):
755-
self.tracer._appsec_enabled = False
756-
self.tracer.configure(api_version="v0.4")
757-
self.app.get("/path-params/2022/july/")
757+
with override_global_config(dict(_appsec_enabled=True)):
758+
self.tracer._appsec_enabled = False
759+
self.tracer.configure(api_version="v0.4")
760+
self.app.get("/path-params/2022/july/")
758761

759-
spans = self.pop_spans()
760-
root_span = spans[0]
761-
assert root_span.get_tag("_dd.appsec.json") is None
762-
path_params = _context.get_item("http.request.path_params", span=root_span)
762+
spans = self.pop_spans()
763+
root_span = spans[0]
764+
assert root_span.get_tag("_dd.appsec.json") is None
765+
path_params = _context.get_item("http.request.path_params", span=root_span)
763766

764-
assert path_params["month"] == "july"
765-
assert path_params["year"] == "2022"
767+
assert path_params["month"] == "july"
768+
assert path_params["year"] == "2022"
766769

767770
def test_pylon_path_params_attack(self):
768-
with override_env(dict(DD_APPSEC_RULES=RULES_GOOD_PATH)):
771+
with override_global_config(dict(_appsec_enabled=True)), override_env(dict(DD_APPSEC_RULES=RULES_GOOD_PATH)):
769772
self.tracer._appsec_enabled = True
770773

771774
self.tracer.configure(api_version="v0.4")

0 commit comments

Comments
 (0)