Merge pull request #2331 from DataDog/backport/2321

Backport #2321

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

@@ -6,6 +6,9 @@
 require_relative '../../processor'
 require_relative '../../assets'
 
+require_relative '../../../tracing/client_ip'
+require_relative '../../../tracing/contrib/rack/header_collection'
+
 module Datadog
   module AppSec
     module Contrib
@@ -30,7 +33,7 @@ def call(env)
             env['datadog.waf.context'] = context
             request = ::Rack::Request.new(env)
 
-            add_appsec_tags
+            add_appsec_tags(active_trace, active_span, env)
 
             request_return, request_response = Instrumentation.gateway.push('rack.request', request) do
               @app.call(env)
@@ -56,7 +59,7 @@ def call(env)
 
             request_return
           ensure
-            add_waf_runtime_tags(context) if context
+            add_waf_runtime_tags(active_trace, context) if context
             context.finalize if context
           end
 
@@ -70,41 +73,60 @@ def active_trace
             Datadog::Tracing.active_trace
           end
 
-          def add_appsec_tags
-            return unless active_trace
+          def active_span
+            # TODO: factor out tracing availability detection
+
+            return unless defined?(Datadog::Tracing)
+
+            Datadog::Tracing.active_span
+          end
+
+          def add_appsec_tags(trace, span, env)
+            return unless trace
+
+            trace.set_tag('_dd.appsec.enabled', 1)
+            trace.set_tag('_dd.runtime_family', 'ruby')
+            trace.set_tag('_dd.appsec.waf.version', Datadog::AppSec::WAF::VERSION::BASE_STRING)
 
-            active_trace.set_tag('_dd.appsec.enabled', 1)
-            active_trace.set_tag('_dd.runtime_family', 'ruby')
-            active_trace.set_tag('_dd.appsec.waf.version', Datadog::AppSec::WAF::VERSION::BASE_STRING)
+            if span && span.get_tag(Tracing::Metadata::Ext::HTTP::TAG_CLIENT_IP).nil?
+              request_header_collection = Datadog::Tracing::Contrib::Rack::Header::RequestHeaderCollection.new(env)
+
+              # always collect client ip, as this is part of AppSec provided functionality
+              Datadog::Tracing::ClientIp.set_client_ip_tag!(
+                span,
+                headers: request_header_collection,
+                remote_ip: env['REMOTE_ADDR']
+              )
+            end
 
             if @processor.ruleset_info
-              active_trace.set_tag('_dd.appsec.event_rules.version', @processor.ruleset_info[:version])
+              trace.set_tag('_dd.appsec.event_rules.version', @processor.ruleset_info[:version])
 
               unless @oneshot_tags_sent
                 # Small race condition, but it's inoccuous: worst case the tags
                 # are sent a couple of times more than expected
                 @oneshot_tags_sent = true
 
-                active_trace.set_tag('_dd.appsec.event_rules.loaded', @processor.ruleset_info[:loaded].to_f)
-                active_trace.set_tag('_dd.appsec.event_rules.error_count', @processor.ruleset_info[:failed].to_f)
-                active_trace.set_tag('_dd.appsec.event_rules.errors', JSON.dump(@processor.ruleset_info[:errors]))
-                active_trace.set_tag('_dd.appsec.event_rules.addresses', JSON.dump(@processor.addresses))
+                trace.set_tag('_dd.appsec.event_rules.loaded', @processor.ruleset_info[:loaded].to_f)
+                trace.set_tag('_dd.appsec.event_rules.error_count', @processor.ruleset_info[:failed].to_f)
+                trace.set_tag('_dd.appsec.event_rules.errors', JSON.dump(@processor.ruleset_info[:errors]))
+                trace.set_tag('_dd.appsec.event_rules.addresses', JSON.dump(@processor.addresses))
 
                 # Ensure these tags reach the backend
-                active_trace.keep!
+                trace.keep!
               end
             end
           end
 
-          def add_waf_runtime_tags(context)
-            return unless active_trace
+          def add_waf_runtime_tags(trace, context)
+            return unless trace
             return unless context
 
-            active_trace.set_tag('_dd.appsec.waf.timeouts', context.timeouts)
+            trace.set_tag('_dd.appsec.waf.timeouts', context.timeouts)
 
             # these tags expect time in us
-            active_trace.set_tag('_dd.appsec.waf.duration', context.time_ns / 1000.0)
-            active_trace.set_tag('_dd.appsec.waf.duration_ext', context.time_ext_ns / 1000.0)
+            trace.set_tag('_dd.appsec.waf.duration', context.time_ns / 1000.0)
+            trace.set_tag('_dd.appsec.waf.duration_ext', context.time_ext_ns / 1000.0)
           end
         end
       end

lib/datadog/appsec/event.rb

@@ -51,7 +51,7 @@ def self.record(*events)
         end
       end
 
-      def self.record_via_span(*events)
+      def self.record_via_span(*events) # rubocop:disable Metrics/AbcSize
         events.group_by { |e| e[:trace] }.each do |trace, event_group|
           unless trace
             Datadog.logger.debug { "{ error: 'no trace: cannot record', event_group: #{event_group.inspect}}" }
@@ -75,9 +75,7 @@ def self.record_via_span(*events)
 
               tags['http.host'] = request.host
               tags['http.useragent'] = request.user_agent
-              tags['network.client.ip'] = request.ip
-
-              # tags['actor.ip'] = request.ip # TODO: uses client IP resolution algorithm
+              tags['network.client.ip'] = request.env['REMOTE_ADDR'] if request.env['REMOTE_ADDR']
             end
 
             if (response = event[:response])

lib/datadog/core/configuration/settings.rb

@@ -667,13 +667,27 @@ def initialize(*_)
             # Whether client IP collection is enabled. When enabled client IPs from HTTP requests will
             #   be reported in traces.
             #
+            # Usage of the DD_TRACE_CLIENT_IP_HEADER_DISABLED environment variable is deprecated.
+            #
             # @see https://docs.datadoghq.com/tracing/configure_data_security#configuring-a-client-ip-header
             #
-            # @default The negated value of the `DD_TRACE_CLIENT_IP_HEADER_DISABLED` environment
-            #   variable or `true` if it doesn't exist.
+            # @default `DD_TRACE_CLIENT_IP_ENABLED` environment variable, otherwise `false`.
             # @return [Boolean]
             option :enabled do |o|
-              o.default { !env_to_bool(Tracing::Configuration::Ext::ClientIp::ENV_DISABLED, false) }
+              o.default do
+                disabled = env_to_bool(Tracing::Configuration::Ext::ClientIp::ENV_DISABLED)
+
+                enabled = if disabled.nil?
+                            false
+                          else
+                            Datadog.logger.warn { "#{Tracing::Configuration::Ext::ClientIp::ENV_DISABLED} environment variable is deprecated, found set to #{disabled}, use #{Tracing::Configuration::Ext::ClientIp::ENV_ENABLED}=#{!disabled}" }
+
+                            !disabled
+                          end
+
+                # ENABLED env var takes precedence over deprecated DISABLED
+                env_to_bool(Tracing::Configuration::Ext::ClientIp::ENV_ENABLED, enabled)
+              end
               o.lazy
             end
 

lib/datadog/tracing/client_ip.rb

@@ -39,6 +39,17 @@ module ClientIp
       def self.set_client_ip_tag(span, headers: nil, remote_ip: nil)
         return unless configuration.enabled
 
+        set_client_ip_tag!(span, headers: headers, remote_ip: remote_ip)
+      end
+
+      # Forcefully sets the `http.client_ip` tag on the given span.
+      #
+      # This function ignores the user's `enabled` setting.
+      #
+      # @param [Span] span The span that's associated with the request.
+      # @param [HeaderCollection, #get, nil] headers A collection with the request headers.
+      # @param [String, nil] remote_ip The remote IP the request associated with the span is sent to.
+      def self.set_client_ip_tag!(span, headers: nil, remote_ip: nil)
         result = raw_ip_from_request(headers, remote_ip)
 
         if result.raw_ip

lib/datadog/tracing/configuration/ext.rb

@@ -54,7 +54,8 @@ module Transport
 
         # @public_api
         module ClientIp
-          ENV_DISABLED = 'DD_TRACE_CLIENT_IP_HEADER_DISABLED'.freeze
+          ENV_ENABLED = 'DD_TRACE_CLIENT_IP_ENABLED'.freeze
+          ENV_DISABLED = 'DD_TRACE_CLIENT_IP_HEADER_DISABLED'.freeze # TODO: deprecated, remove later
           ENV_HEADER_NAME = 'DD_TRACE_CLIENT_IP_HEADER'.freeze
         end
       end

spec/datadog/appsec/contrib/rack/integration_test_spec.rb

@@ -60,6 +60,9 @@
       JSON.parse(json).fetch('triggers', []) if json
     end
 
+    let(:remote_addr) { '127.0.0.1' }
+    let(:client_ip) { remote_addr }
+
     shared_examples 'a GET 200 span' do
       it { expect(span.get_tag('http.method')).to eq('GET') }
       it { expect(span.get_tag('http.status_code')).to eq('200') }
@@ -120,12 +123,14 @@
       it { expect(trace.send(:metrics)['_dd.appsec.enabled']).to be_nil }
       it { expect(trace.send(:meta)['_dd.runtime_family']).to be_nil }
       it { expect(trace.send(:meta)['_dd.appsec.waf.version']).to be_nil }
+      it { expect(span.send(:meta)['http.client_ip']).to eq nil }
     end
 
     shared_examples 'a trace with AppSec tags' do
       it { expect(trace.send(:metrics)['_dd.appsec.enabled']).to eq(1.0) }
       it { expect(trace.send(:meta)['_dd.runtime_family']).to eq('ruby') }
       it { expect(trace.send(:meta)['_dd.appsec.waf.version']).to match(/^\d+\.\d+\.\d+/) }
+      it { expect(span.send(:meta)['http.client_ip']).to eq client_ip }
 
       context 'with appsec disabled' do
         let(:appsec_enabled) { false }
@@ -165,11 +170,12 @@
       end
 
       describe 'GET request' do
-        subject(:response) { get url, params, headers }
+        subject(:response) { get url, params, env }
 
         let(:url) { '/success' }
         let(:params) { {} }
         let(:headers) { {} }
+        let(:env) { { 'REMOTE_ADDR' => remote_addr }.merge!(headers) }
 
         context 'with a non-event-triggering request' do
           it { is_expected.to be_ok }
@@ -203,8 +209,9 @@
         context 'with an event-triggering request in IP' do
           skip 'TODO: config not implemented'
 
-          # TODO: let(:config) { { ip_denylist: ['1.2.3.4'] } }
-          let(:headers) { { 'HTTP_X_FORWARDED_FOR' => '1.2.3.4' } }
+          let(:client_ip) { '1.2.3.4' }
+          # TODO: let(:config) { { ip_denylist: [client_ip] } }
+          let(:headers) { { 'HTTP_X_FORWARDED_FOR' => client_ip } }
 
           it { is_expected.to be_ok }
 
@@ -226,11 +233,12 @@
       end
 
       describe 'POST request' do
-        subject(:response) { post url, params, headers }
+        subject(:response) { post url, params, env }
 
         let(:url) { '/success' }
         let(:params) { {} }
         let(:headers) { {} }
+        let(:env) { { 'REMOTE_ADDR' => remote_addr }.merge!(headers) }
 
         context 'with a non-event-triggering request' do
           it { is_expected.to be_ok }

spec/datadog/appsec/contrib/rails/integration_test_spec.rb

@@ -77,6 +77,9 @@ def success
       JSON.parse(json).fetch('triggers', []) if json
     end
 
+    let(:remote_addr) { '127.0.0.1' }
+    let(:client_ip) { remote_addr }
+
     let(:span) { rack_span }
 
     shared_examples 'a GET 200 span' do
@@ -125,12 +128,14 @@ def success
       it { expect(trace.send(:metrics)['_dd.appsec.enabled']).to be_nil }
       it { expect(trace.send(:meta)['_dd.runtime_family']).to be_nil }
       it { expect(trace.send(:meta)['_dd.appsec.waf.version']).to be_nil }
+      it { expect(span.send(:meta)['http.client_ip']).to eq nil }
     end
 
     shared_examples 'a trace with AppSec tags' do
       it { expect(trace.send(:metrics)['_dd.appsec.enabled']).to eq(1.0) }
       it { expect(trace.send(:meta)['_dd.runtime_family']).to eq('ruby') }
       it { expect(trace.send(:meta)['_dd.appsec.waf.version']).to match(/^\d+\.\d+\.\d+/) }
+      it { expect(span.send(:meta)['http.client_ip']).to eq client_ip }
 
       context 'with appsec disabled' do
         let(:appsec_enabled) { false }
@@ -169,11 +174,12 @@ def success
       end
 
       describe 'GET request' do
-        subject(:response) { get url, params, headers }
+        subject(:response) { get url, params, env }
 
         let(:url) { '/success' }
         let(:params) { {} }
         let(:headers) { {} }
+        let(:env) { { 'REMOTE_ADDR' => remote_addr }.merge!(headers) }
 
         context 'with a non-event-triggering request' do
           it { is_expected.to be_ok }
@@ -220,6 +226,20 @@ def success
           it_behaves_like 'a trace with AppSec events'
         end
 
+        context 'with an event-triggering request in IP' do
+          skip 'TODO: config not implemented'
+
+          let(:client_ip) { '1.2.3.4' }
+          # TODO: let(:config) { { ip_denylist: [client_ip] } }
+          let(:headers) { { 'HTTP_X_FORWARDED_FOR' => client_ip } }
+
+          it { is_expected.to be_ok }
+
+          # TODO: it_behaves_like 'a GET 403 span'
+          it_behaves_like 'a trace with AppSec tags'
+          # TODO: it_behaves_like 'a trace with AppSec events'
+        end
+
         context 'with an event-triggering response' do
           let(:url) { '/admin.php' } # well-known scanned path
 
@@ -233,11 +253,12 @@ def success
       end
 
       describe 'POST request' do
-        subject(:response) { post url, params, headers }
+        subject(:response) { post url, params, env }
 
         let(:url) { '/success' }
         let(:params) { {} }
         let(:headers) { {} }
+        let(:env) { { 'REMOTE_ADDR' => remote_addr }.merge!(headers) }
 
         context 'with a non-event-triggering request' do
           it { is_expected.to be_ok }

spec/datadog/appsec/contrib/sinatra/integration_test_spec.rb

@@ -84,6 +84,9 @@
       JSON.parse(json).fetch('triggers', []) if json
     end
 
+    let(:remote_addr) { '127.0.0.1' }
+    let(:client_ip) { remote_addr }
+
     let(:span) { rack_span }
 
     shared_examples 'a GET 200 span' do
@@ -132,12 +135,14 @@
       it { expect(trace.send(:metrics)['_dd.appsec.enabled']).to be_nil }
       it { expect(trace.send(:meta)['_dd.runtime_family']).to be_nil }
       it { expect(trace.send(:meta)['_dd.appsec.waf.version']).to be_nil }
+      it { expect(span.send(:meta)['http.client_ip']).to eq nil }
     end
 
     shared_examples 'a trace with AppSec tags' do
       it { expect(trace.send(:metrics)['_dd.appsec.enabled']).to eq(1.0) }
       it { expect(trace.send(:meta)['_dd.runtime_family']).to eq('ruby') }
       it { expect(trace.send(:meta)['_dd.appsec.waf.version']).to match(/^\d+\.\d+\.\d+/) }
+      it { expect(span.send(:meta)['http.client_ip']).to eq client_ip }
 
       context 'with appsec disabled' do
         let(:appsec_enabled) { false }
@@ -181,11 +186,12 @@
       end
 
       describe 'GET request' do
-        subject(:response) { get url, params, headers }
+        subject(:response) { get url, params, env }
 
         let(:url) { '/success' }
         let(:params) { {} }
         let(:headers) { {} }
+        let(:env) { { 'REMOTE_ADDR' => remote_addr }.merge!(headers) }
 
         context 'with a non-event-triggering request' do
           it { is_expected.to be_ok }
@@ -234,6 +240,20 @@
           it_behaves_like 'a trace with AppSec events'
         end
 
+        context 'with an event-triggering request in IP' do
+          skip 'TODO: config not implemented'
+
+          let(:client_ip) { '1.2.3.4' }
+          # TODO: let(:config) { { ip_denylist: [client_ip] } }
+          let(:headers) { { 'HTTP_X_FORWARDED_FOR' => client_ip } }
+
+          it { is_expected.to be_ok }
+
+          # TODO: it_behaves_like 'a GET 403 span'
+          it_behaves_like 'a trace with AppSec tags'
+          # TODO: it_behaves_like 'a trace with AppSec events'
+        end
+
         context 'with an event-triggering response' do
           let(:url) { '/admin.php' } # well-known scanned path
 
@@ -247,11 +267,12 @@
       end
 
       describe 'POST request' do
-        subject(:response) { post url, params, headers }
+        subject(:response) { post url, params, env }
 
         let(:url) { '/success' }
         let(:params) { {} }
         let(:headers) { {} }
+        let(:env) { { 'REMOTE_ADDR' => remote_addr }.merge!(headers) }
 
         context 'with a non-event-triggering request' do
           it { is_expected.to be_ok }