diff --git a/lib/datadog/appsec/contrib/rack/request_middleware.rb b/lib/datadog/appsec/contrib/rack/request_middleware.rb index 9cde36f6c3f..1b7177d6276 100644 --- a/lib/datadog/appsec/contrib/rack/request_middleware.rb +++ b/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 diff --git a/lib/datadog/appsec/event.rb b/lib/datadog/appsec/event.rb index 82b08f2c757..f218efc21ac 100644 --- a/lib/datadog/appsec/event.rb +++ b/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]) diff --git a/lib/datadog/core/configuration/settings.rb b/lib/datadog/core/configuration/settings.rb index fe0c6cb058d..5695b57b0f1 100644 --- a/lib/datadog/core/configuration/settings.rb +++ b/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 diff --git a/lib/datadog/tracing/client_ip.rb b/lib/datadog/tracing/client_ip.rb index 3b3ce366e29..76fedf82ee0 100644 --- a/lib/datadog/tracing/client_ip.rb +++ b/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 diff --git a/lib/datadog/tracing/configuration/ext.rb b/lib/datadog/tracing/configuration/ext.rb index fb8b86fe6de..14ae2deae7a 100644 --- a/lib/datadog/tracing/configuration/ext.rb +++ b/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 diff --git a/spec/datadog/appsec/contrib/rack/integration_test_spec.rb b/spec/datadog/appsec/contrib/rack/integration_test_spec.rb index 51f7d5dba8c..31ca03280ec 100644 --- a/spec/datadog/appsec/contrib/rack/integration_test_spec.rb +++ b/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 } diff --git a/spec/datadog/appsec/contrib/rails/integration_test_spec.rb b/spec/datadog/appsec/contrib/rails/integration_test_spec.rb index 9a8ccf682a5..f95947c3b06 100644 --- a/spec/datadog/appsec/contrib/rails/integration_test_spec.rb +++ b/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 } diff --git a/spec/datadog/appsec/contrib/sinatra/integration_test_spec.rb b/spec/datadog/appsec/contrib/sinatra/integration_test_spec.rb index abf76737845..1efc472a75a 100644 --- a/spec/datadog/appsec/contrib/sinatra/integration_test_spec.rb +++ b/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 }