Tag AppSec on Rack top-level span #2858

lloeki · 2023-05-17T11:11:43Z

What does this PR do?
Tag AppSec on Rack top-level span

Motivation
AppSec discovers service name based on top-level span. Tagging at trace level results in tags on the trace root span, which is usually Rack's top-level span.

When request queuing is enabeld, though, a synthetic span is added in front, representing a service outside the application. This tag then gets mistakenly tagged with AppSec information, resulting in much confusion.

The situation gets even trickier with partial flushing, as partially flushed spans may receive a trace tag.

Ensuring we tag the proper span should solve both issues.

Additional Notes

Starting this PR as draft since I am still figuring out how to handle Datadog::Kit tagging in a backward compatible manner.

How to test the change?

Add request_queuing: true to Rack or Rails tracing integration, then:

curl -vvv -H 'User-Agent: Arachni/v2' -H "X-Request-Start: $(ruby -e 'puts "%d" % (Time.now.to_f * 1000)')" -H "X-Queue-Start: $(ruby -e 'puts "%d" % (Time.now.to_f * 1000)')" 'http://user:pass@127.0.0.1:9292/'

GustavoCaso · 2023-05-25T14:11:29Z

lib/datadog/kit/appsec/events.rb

+          if (appsec_scope = Datadog::AppSec.active_scope)
+            trace = appsec_scope.trace
+            span = appsec_scope.span
+          end
+
+          trace ||= Datadog::Tracing.active_trace
+          span ||= trace.active_span || Datadog::Tracing.active_span
+
+          if trace.trace_id != span.trace_id
+            raise ArgumentError, "span #{span.span_id} does not belong to trace #{trace.trace_id}"
+          end


We should probably extract this to a separate helper method within appsec 😄

Yeah, I went for the quick fix route here, as I find it awkward currently to hide that code behind the wrong abstraction.

Let's address this later and get the release done.

GustavoCaso · 2023-05-25T16:19:46Z

lib/datadog/appsec/contrib/rack/request_middleware.rb

+          def active_scope(env)
+            env['datadog.appsec.scope']
+          end


I find this one line methods no very useful, since it requires the reader to check what the method is actually doing. In this case checking the env['datadog.appsec.scope'] is more readable in line 39 in my opinion.

Agreed. Let's address this later and get the release done.

AppSec discovers service name based on top-level span. Tagging at trace level results in tags on the trace root span, which is usually Rack's top-level span. When request queuing is enabeld, though, a synthetic span is added in front, representing a service outside the application. This tag then gets mistakenly tagged with AppSec information, resulting in much confusion. The situation gets even trickier with partial flushing, as partially flushed spans may receive a trace tag. Ensuring we tag the proper span should solve both issues.

- Create a AppSec::Scope concept to capture toplevel references - Extract context management out of Processor to the more general Scope class - Propagate Scope instead of Context through Rack - Refactor Processor::Context users to find context through Scope instead

GustavoCaso · 2023-05-26T14:26:00Z

lib/datadog/kit/appsec/events.rb

+          trace ||= Datadog::Tracing.active_trace
+          span ||= trace.active_span || Datadog::Tracing.active_span
+
+          raise ArgumentError, "span #{span.span_id} does not belong to trace #{trace.id}" if trace.id != span.trace_id


Should we raise or log an message letting the user that they have done something odd?

As with the active scope (prev. active context) sanity checks, this should not happen, ever. If we swallow it we'll never be aware.

Practically since we now use the AppSec scope as an override, it can only happen if a user forcefully passes both trace and span args outside an AppSec scope. In that case they made a mistake and will want to be aware immediately.

…hanged to datadog.appsec.scope

codecov-commenter · 2023-05-31T08:46:49Z

Codecov Report

Merging #2858 (3bdd5c6) into master (df8612f) will increase coverage by 0.01%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #2858      +/-   ##
==========================================
+ Coverage   98.09%   98.10%   +0.01%     
==========================================
  Files        1269     1266       -3     
  Lines       69969    69990      +21     
  Branches     3161     3172      +11     
==========================================
+ Hits        68633    68662      +29     
+ Misses       1336     1328       -8

see 42 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

lloeki · 2023-06-01T10:11:30Z

lib/datadog/appsec/contrib/rack/request_body_middleware.rb

@@ -17,7 +17,7 @@ def initialize(app, opt = {})
          end

          def call(env)
-            context = env['datadog.waf.context']
+            context = env['datadog.appsec.scope']


Thanks @GustavoCaso !

GustavoCaso

LGTM

github-actions bot added appsec Application Security monitoring product integrations Involves tracing integrations labels May 17, 2023

lloeki force-pushed the attach-appec-to-toplevel-span branch from 2c7d1e1 to fcc90d2 Compare May 17, 2023 11:13

lloeki mentioned this pull request May 17, 2023

[APPSEC-9989] Allow setting remote configuration service name #2853

Merged

lloeki force-pushed the attach-appec-to-toplevel-span branch 2 times, most recently from 277d339 to 9b73c08 Compare May 25, 2023 13:47

GustavoCaso reviewed May 25, 2023

View reviewed changes

lloeki added 3 commits May 26, 2023 13:32

[WIP] tag on service entry span instead of trace

5279f6b

lloeki force-pushed the attach-appec-to-toplevel-span branch from 07898dc to 3a16237 Compare May 26, 2023 11:39

[WIP] Fix signatures

114b7a0

lloeki force-pushed the attach-appec-to-toplevel-span branch from 3a16237 to 114b7a0 Compare May 26, 2023 12:24

lloeki and others added 4 commits May 26, 2023 14:24

[WIP] Fix specs

6e4a227

[WIP] Add scope spec

3b03e2b

Update Kit::Identity specs

d3bbf40

add specs to Datadog::Kit::AppSec::Events

65581ba

GustavoCaso reviewed May 26, 2023

View reviewed changes

[WIP] Test AppSec against service span

3080fb7

lloeki force-pushed the attach-appec-to-toplevel-span branch from 41edde7 to 3080fb7 Compare May 30, 2023 13:12

GustavoCaso added 3 commits May 31, 2023 10:15

fix broken test. Issue was that the env key for the waf context has c…

d3f44ed

…hanged to datadog.appsec.scope

move scope key to a constant

9cb953a

fix rubocop

3bdd5c6

GustavoCaso force-pushed the attach-appec-to-toplevel-span branch from d5a3357 to 3bdd5c6 Compare May 31, 2023 08:28

lloeki commented Jun 1, 2023

View reviewed changes

lloeki marked this pull request as ready for review June 1, 2023 10:11

lloeki requested a review from a team June 1, 2023 10:11

lloeki mentioned this pull request Jun 1, 2023

[APPSEC-9545] Devise integration and automatic user events #2877

Merged

GustavoCaso approved these changes Jun 1, 2023

View reviewed changes

GustavoCaso merged commit 80c3573 into master Jun 1, 2023

GustavoCaso deleted the attach-appec-to-toplevel-span branch June 1, 2023 14:10

github-actions bot added this to the 1.12.0 milestone Jun 1, 2023

GustavoCaso mentioned this pull request Jun 16, 2023

[APPSEC-10049] Appsec handle tracing disabled and appsec events #2909

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Tag AppSec on Rack top-level span #2858

Tag AppSec on Rack top-level span #2858

lloeki commented May 17, 2023 •

edited

Loading

GustavoCaso May 25, 2023 •

edited

Loading

lloeki Jun 1, 2023 •

edited

Loading

GustavoCaso May 25, 2023

lloeki Jun 1, 2023

GustavoCaso May 26, 2023

lloeki Jun 1, 2023 •

edited

Loading

codecov-commenter commented May 31, 2023

lloeki Jun 1, 2023

GustavoCaso left a comment

Tag AppSec on Rack top-level span #2858

Tag AppSec on Rack top-level span #2858

Conversation

lloeki commented May 17, 2023 • edited Loading

GustavoCaso May 25, 2023 • edited Loading

Choose a reason for hiding this comment

lloeki Jun 1, 2023 • edited Loading

Choose a reason for hiding this comment

GustavoCaso May 25, 2023

Choose a reason for hiding this comment

lloeki Jun 1, 2023

Choose a reason for hiding this comment

GustavoCaso May 26, 2023

Choose a reason for hiding this comment

lloeki Jun 1, 2023 • edited Loading

Choose a reason for hiding this comment

codecov-commenter commented May 31, 2023

Codecov Report

lloeki Jun 1, 2023

Choose a reason for hiding this comment

GustavoCaso left a comment

Choose a reason for hiding this comment

lloeki commented May 17, 2023 •

edited

Loading

GustavoCaso May 25, 2023 •

edited

Loading

lloeki Jun 1, 2023 •

edited

Loading

lloeki Jun 1, 2023 •

edited

Loading