-
Notifications
You must be signed in to change notification settings - Fork 178
feat(appsec): collect security-testing headers on HTTP entry spans #3925
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Changes from all commits
809a17b
7e736fa
abd3a53
434a266
e18e908
a99f53e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -56,6 +56,9 @@ | |
|
|
||
| ZEND_EXTERN_MODULE_GLOBALS(ddtrace); | ||
|
|
||
| #define DD_TAG_HTTP_REQH_ENDPOINT_SCAN "http.request.headers.x-datadog-endpoint-scan" | ||
| #define DD_TAG_HTTP_REQH_SECURITY_TEST "http.request.headers.x-datadog-security-test" | ||
|
|
||
| extern void (*profiling_notify_trace_finished)(uint64_t local_root_span_id, | ||
| zai_str span_type, | ||
| zai_str resource); | ||
|
|
@@ -693,6 +696,30 @@ static void dd_set_entrypoint_root_span_props(struct superglob_equiv *data, ddtr | |
| } | ||
|
|
||
| if (data->server) { | ||
| // Security-testing headers (APPSEC-62412): collected unconditionally | ||
| // regardless of DD_TRACE_HEADER_TAGS or AppSec being enabled. | ||
| #define DD_UNCONDITIONAL_SERVER_HEADER(server_key, tag) \ | ||
| { server_key, sizeof(server_key) - 1, tag, sizeof(tag) - 1 } | ||
| static const struct { | ||
| const char *server_key; size_t server_len; | ||
| const char *tag; size_t tag_len; | ||
| } sec_headers[] = { | ||
| DD_UNCONDITIONAL_SERVER_HEADER("HTTP_X_DATADOG_ENDPOINT_SCAN", DD_TAG_HTTP_REQH_ENDPOINT_SCAN), | ||
| DD_UNCONDITIONAL_SERVER_HEADER("HTTP_X_DATADOG_SECURITY_TEST", DD_TAG_HTTP_REQH_SECURITY_TEST), | ||
| }; | ||
| #undef DD_UNCONDITIONAL_SERVER_HEADER | ||
| for (size_t i = 0; i < sizeof(sec_headers) / sizeof(*sec_headers); i++) { | ||
| zval *hval = zend_hash_str_find(data->server, sec_headers[i].server_key, sec_headers[i].server_len); | ||
| if (hval) { | ||
| ZVAL_DEREF(hval); | ||
| if (Z_TYPE_P(hval) == IS_STRING) { | ||
| zval zv; | ||
| ZVAL_STR_COPY(&zv, Z_STR_P(hval)); | ||
| zend_hash_str_add_new(meta, sec_headers[i].tag, sec_headers[i].tag_len, &zv); | ||
| } | ||
| } | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I imagine this will be sent only in a very small amount of requests. Yet, you're adding overhead in every request looking for them. Ideally you'd add it to the hastable returned by get_DD_TRACE_HEADER_TAGS() or equivalent, though I think this would require a startup change after zai_config_minit() modyfying Anyway, give it a thought.
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good point. Two The I'll leave the current approach as-is given the tradeoff, but happy to revisit if you feel strongly about it. |
||
| } | ||
|
|
||
| zend_string *headername; | ||
| zval *headerval; | ||
| ZEND_HASH_FOREACH_STR_KEY_VAL_IND(data->server, headername, headerval) { | ||
|
|
@@ -1848,6 +1875,8 @@ ddog_SpanBytes *ddtrace_serialize_span_to_rust_span(ddtrace_span_data *span, ddo | |
| transfer_meta_data(rust_span, serialized_inferred_span, "_dd.p.dm", true); | ||
| transfer_meta_data(rust_span, serialized_inferred_span, "_dd.p.ksr", false); | ||
| transfer_meta_data(rust_span, serialized_inferred_span, "_dd.p.tid", true); | ||
| transfer_meta_data(rust_span, serialized_inferred_span, DD_TAG_HTTP_REQH_ENDPOINT_SCAN, false); | ||
| transfer_meta_data(rust_span, serialized_inferred_span, DD_TAG_HTTP_REQH_SECURITY_TEST, false); | ||
|
christophe-papazian marked this conversation as resolved.
|
||
|
|
||
| ddog_set_span_error(serialized_inferred_span, ddog_get_span_error(rust_span)); | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,71 @@ | ||
| <?php | ||
|
|
||
| namespace DDTrace\Tests\Integrations\Swoole; | ||
|
|
||
| use DDTrace\Tests\Common\WebFrameworkTestCase; | ||
| use DDTrace\Tests\Frameworks\Util\Request\GetSpec; | ||
|
|
||
| class SecurityTestingHeadersTest extends WebFrameworkTestCase | ||
| { | ||
| public static function getAppIndexScript() | ||
| { | ||
| return __DIR__ . '/../../Frameworks/Swoole/index.php'; | ||
| } | ||
|
|
||
| protected static function isSwoole() | ||
| { | ||
| return true; | ||
| } | ||
|
|
||
| protected static function getEnvs() | ||
| { | ||
| return array_merge(parent::getEnvs(), [ | ||
| 'DD_TRACE_CLI_ENABLED' => 'true', | ||
| ]); | ||
| } | ||
|
|
||
| protected static function getInis() | ||
| { | ||
| return array_merge(parent::getInis(), [ | ||
| 'extension' => 'swoole.so', | ||
| ]); | ||
| } | ||
|
|
||
| public function testSecurityTestingHeadersCollectedUnconditionally() | ||
| { | ||
| $traces = $this->tracesFromWebRequest(function () { | ||
| $spec = GetSpec::create('request', '/', [ | ||
| 'X-Datadog-Endpoint-Scan: endpoint-scan-uuid', | ||
| 'X-Datadog-Security-Test: security-test-uuid', | ||
| ]); | ||
| $this->call($spec); | ||
| }); | ||
|
|
||
| $span = $traces[0][0]; | ||
| $this->assertSame( | ||
| 'endpoint-scan-uuid', | ||
| $span['meta']['http.request.headers.x-datadog-endpoint-scan'] | ||
| ); | ||
| $this->assertSame( | ||
| 'security-test-uuid', | ||
| $span['meta']['http.request.headers.x-datadog-security-test'] | ||
| ); | ||
| } | ||
|
|
||
| public function testSecurityTestingHeadersAbsentWhenNotSent() | ||
| { | ||
| $traces = $this->tracesFromWebRequest(function () { | ||
| $this->call(GetSpec::create('request', '/')); | ||
| }); | ||
|
|
||
| $span = $traces[0][0]; | ||
| $this->assertArrayNotHasKey( | ||
| 'http.request.headers.x-datadog-endpoint-scan', | ||
| $span['meta'] | ||
| ); | ||
| $this->assertArrayNotHasKey( | ||
| 'http.request.headers.x-datadog-security-test', | ||
| $span['meta'] | ||
| ); | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,50 @@ | ||
| --TEST-- | ||
| Security-testing headers are forwarded to the inferred proxy span | ||
| --ENV-- | ||
| DD_TRACE_AUTO_FLUSH_ENABLED=0 | ||
| DD_TRACE_GENERATE_ROOT_SPAN=0 | ||
| DD_CODE_ORIGIN_FOR_SPANS_ENABLED=0 | ||
| DD_TRACE_INFERRED_PROXY_SERVICES_ENABLED=1 | ||
| HTTP_X_DD_PROXY=aws-apigateway | ||
| HTTP_X_DD_PROXY_REQUEST_TIME_MS=100 | ||
| HTTP_X_DD_PROXY_PATH=/test | ||
| HTTP_X_DD_PROXY_HTTPMETHOD=GET | ||
| HTTP_X_DD_PROXY_DOMAIN_NAME=example.com | ||
| HTTP_X_DD_PROXY_STAGE=aws-prod | ||
| HTTP_X_DATADOG_ENDPOINT_SCAN=endpoint-scan-uuid | ||
| HTTP_X_DATADOG_SECURITY_TEST=security-test-uuid | ||
| METHOD=GET | ||
| SERVER_NAME=localhost:8888 | ||
| SCRIPT_NAME=/foo.php | ||
| REQUEST_URI=/foo | ||
| DD_TRACE_DEBUG_PRNG_SEED=42 | ||
| --FILE-- | ||
| <?php | ||
| DDTrace\start_span(); | ||
| DDTrace\close_span(); | ||
| $spans = dd_trace_serialize_closed_spans(); | ||
|
|
||
| // The PHP service-entry span has a parent_id pointing to the inferred span; | ||
| // the inferred span itself has no parent_id (it is the trace root). | ||
| $rootSpan = null; | ||
| $inferredSpan = null; | ||
| foreach ($spans as $span) { | ||
| if (!isset($span['parent_id'])) { | ||
| $inferredSpan = $span; | ||
| } else { | ||
| $rootSpan = $span; | ||
| } | ||
| } | ||
|
|
||
| // Tags must be present on the PHP service-entry span | ||
| var_dump($rootSpan['meta']['http.request.headers.x-datadog-endpoint-scan'] ?? 'NOT SET'); | ||
| var_dump($rootSpan['meta']['http.request.headers.x-datadog-security-test'] ?? 'NOT SET'); | ||
| // And forwarded to the inferred proxy span | ||
| var_dump($inferredSpan['meta']['http.request.headers.x-datadog-endpoint-scan'] ?? 'NOT SET'); | ||
| var_dump($inferredSpan['meta']['http.request.headers.x-datadog-security-test'] ?? 'NOT SET'); | ||
| ?> | ||
| --EXPECT-- | ||
| string(18) "endpoint-scan-uuid" | ||
| string(18) "security-test-uuid" | ||
| string(18) "endpoint-scan-uuid" | ||
| string(18) "security-test-uuid" |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,19 @@ | ||
| --TEST-- | ||
| Security-testing headers are collected unconditionally on the root span | ||
| --ENV-- | ||
| DD_TRACE_AUTO_FLUSH_ENABLED=0 | ||
| DD_TRACE_GENERATE_ROOT_SPAN=0 | ||
| DD_TRACE_HEADER_TAGS= | ||
| HTTP_X_DATADOG_ENDPOINT_SCAN=endpoint-scan-uuid | ||
| HTTP_X_DATADOG_SECURITY_TEST=security-test-uuid | ||
| --FILE-- | ||
| <?php | ||
| DDTrace\start_span(); | ||
| DDTrace\close_span(0); | ||
| $spans = dd_trace_serialize_closed_spans(); | ||
| var_dump($spans[0]['meta']['http.request.headers.x-datadog-endpoint-scan']); | ||
| var_dump($spans[0]['meta']['http.request.headers.x-datadog-security-test']); | ||
| ?> | ||
| --EXPECT-- | ||
| string(18) "endpoint-scan-uuid" | ||
| string(18) "security-test-uuid" |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,16 @@ | ||
| --TEST-- | ||
| Security-testing header tags are absent when headers are not sent | ||
| --ENV-- | ||
| DD_TRACE_AUTO_FLUSH_ENABLED=0 | ||
| DD_TRACE_GENERATE_ROOT_SPAN=0 | ||
| --FILE-- | ||
| <?php | ||
| DDTrace\start_span(); | ||
| DDTrace\close_span(0); | ||
| $spans = dd_trace_serialize_closed_spans(); | ||
| var_dump(array_key_exists('http.request.headers.x-datadog-endpoint-scan', $spans[0]['meta'])); | ||
| var_dump(array_key_exists('http.request.headers.x-datadog-security-test', $spans[0]['meta'])); | ||
| ?> | ||
| --EXPECT-- | ||
| bool(false) | ||
| bool(false) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would try to reuse as much as possible the code inside dd_add_header_to_meta. Right now it's checking DD_TRACE_HEADER_TAGS which is not needed for this but all other aldd header related code you can reuse it. Also you can add a macro like this to avoid the duplication
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done — added the
DD_UNCONDITIONAL_SERVER_HEADERmacro exactly as suggested, and also moved the tag-name strings toDD_TAG_HTTP_REQH_ENDPOINT_SCAN/DD_TAG_HTTP_REQH_SECURITY_TESTconstants (see the follow-up comment). Both are used in the struct initializer and thetransfer_meta_datacalls.One note on the tags.c placement: the RFC requires collection to be unconditional regardless of whether AppSec is enabled, so we kept the
serializer.cblock (tracer, always loaded) and also added the headers to_relevant_basic_headersintags.cas you suggested — the two paths compose cleanly sincezend_hash_str_add_newin the tracer runs at serialization time, and AppSec's_try_add_tag(which useszend_hash_add) is a no-op if the key already exists.