From 9f40bd53c1b8dba9460c8f473cd5c712f45f6964 Mon Sep 17 00:00:00 2001 From: Gustavo Caso Date: Tue, 17 Oct 2023 14:12:49 +0200 Subject: [PATCH 1/2] append content-type and content-length header information on ASM span tags --- .../appsec/contrib/rack/gateway/request.rb | 8 ++++-- .../contrib/rack/gateway/request_spec.rb | 11 +++++--- .../contrib/rack/reactive/request_spec.rb | 27 ++++++++++++++----- 3 files changed, 35 insertions(+), 11 deletions(-) diff --git a/lib/datadog/appsec/contrib/rack/gateway/request.rb b/lib/datadog/appsec/contrib/rack/gateway/request.rb index 86513a697a2..b0b57a99087 100644 --- a/lib/datadog/appsec/contrib/rack/gateway/request.rb +++ b/lib/datadog/appsec/contrib/rack/gateway/request.rb @@ -40,9 +40,13 @@ def method end def headers - request.env.each_with_object({}) do |(k, v), h| - h[k.gsub(/^HTTP_/, '').downcase.tr('_', '-')] = v if k =~ /^HTTP_/ + result = request.env.each_with_object({}) do |(k, v), h| + h[k.gsub(/^HTTP_/, '').downcase!.tr('_', '-')] = v if k =~ /^HTTP_/ end + + result['content-type'] = request.content_type if request.content_type + result['content-length'] = request.content_length if request.content_length + result end def body diff --git a/spec/datadog/appsec/contrib/rack/gateway/request_spec.rb b/spec/datadog/appsec/contrib/rack/gateway/request_spec.rb index 495ef2fbb0b..ddc1918d06c 100644 --- a/spec/datadog/appsec/contrib/rack/gateway/request_spec.rb +++ b/spec/datadog/appsec/contrib/rack/gateway/request_spec.rb @@ -10,7 +10,7 @@ Rack::MockRequest.env_for( 'http://example.com:8080/?a=foo&a=bar&b=baz', { - 'REQUEST_METHOD' => 'GET', 'REMOTE_ADDR' => '10.10.10.10', 'HTTP_CONTENT_TYPE' => 'text/html', + 'REQUEST_METHOD' => 'GET', 'REMOTE_ADDR' => '10.10.10.10', 'CONTENT_TYPE' => 'text/html', 'HTTP_COOKIE' => 'foo=bar', 'HTTP_USER_AGENT' => 'WebKit' } ) @@ -24,8 +24,13 @@ end describe '#headers' do - it 'returns the header information and strip the HTTP_ prefix' do - expected_headers = { 'content-type' => 'text/html', 'cookie' => 'foo=bar', 'user-agent' => 'WebKit' } + it 'returns the header information. Strip the HTTP_ prefix and append content-type and content-length information' do + expected_headers = { + 'content-type' => 'text/html', + 'cookie' => 'foo=bar', + 'user-agent' => 'WebKit', + 'content-length' => '0' + } expect(request.headers).to eq(expected_headers) end end diff --git a/spec/datadog/appsec/contrib/rack/reactive/request_spec.rb b/spec/datadog/appsec/contrib/rack/reactive/request_spec.rb index 257a242b35d..efd8da9d81e 100644 --- a/spec/datadog/appsec/contrib/rack/reactive/request_spec.rb +++ b/spec/datadog/appsec/contrib/rack/reactive/request_spec.rb @@ -12,19 +12,34 @@ Datadog::AppSec::Contrib::Rack::Gateway::Request.new( Rack::MockRequest.env_for( 'http://example.com:8080/?a=foo', - { 'REQUEST_METHOD' => 'GET', 'REMOTE_ADDR' => '10.10.10.10', 'HTTP_CONTENT_TYPE' => 'text/html' } + { + 'REQUEST_METHOD' => 'GET', + 'REMOTE_ADDR' => '10.10.10.10', + 'CONTENT_TYPE' => 'text/html', + 'HTTP_USER_AGENT' => 'foo', + 'HTTP_COOKIE' => 'foo=bar' + } ) ) end + let(:expected_headers_with_cookies) do + { 'content-length' => '0', 'content-type' => 'text/html', 'user-agent' => 'foo', 'cookie' => 'foo=bar' } + end + + let(:expected_headers_without_cookies) do + { 'content-length' => '0', 'content-type' => 'text/html', 'user-agent' => 'foo' } + end + describe '.publish' do it 'propagates request attributes to the operation' do expect(operation).to receive(:publish).with('server.request.method', 'GET') expect(operation).to receive(:publish).with('request.query', { 'a' => ['foo'] }) - expect(operation).to receive(:publish).with('request.headers', { 'content-type' => 'text/html' }) + expect(operation).to receive(:publish).with('request.headers', expected_headers_with_cookies) expect(operation).to receive(:publish).with('request.uri.raw', '/?a=foo') - expect(operation).to receive(:publish).with('request.cookies', {}) + expect(operation).to receive(:publish).with('request.cookies', { 'foo' => 'bar' }) expect(operation).to receive(:publish).with('request.client_ip', '10.10.10.10') + described_class.publish(operation, request) end end @@ -52,11 +67,11 @@ expect(operation).to receive(:subscribe).and_call_original expected_waf_arguments = { - 'server.request.cookies' => {}, + 'server.request.cookies' => { 'foo' => 'bar' }, 'server.request.query' => { 'a' => ['foo'] }, 'server.request.uri.raw' => '/?a=foo', - 'server.request.headers' => { 'content-type' => 'text/html' }, - 'server.request.headers.no_cookies' => { 'content-type' => 'text/html' }, + 'server.request.headers' => expected_headers_with_cookies, + 'server.request.headers.no_cookies' => expected_headers_without_cookies, 'http.client_ip' => '10.10.10.10', 'server.request.method' => 'GET', } From 8b68fcb9d6e77c9458a43f52a4aaae52b73abbfc Mon Sep 17 00:00:00 2001 From: Gustavo Caso Date: Tue, 17 Oct 2023 16:35:01 +0200 Subject: [PATCH 2/2] improve Datadog::AppSec::Event specs --- spec/datadog/appsec/event_spec.rb | 378 +++++++++++++++++++----------- 1 file changed, 236 insertions(+), 142 deletions(-) diff --git a/spec/datadog/appsec/event_spec.rb b/spec/datadog/appsec/event_spec.rb index e79ecfbde1e..9db09d90c49 100644 --- a/spec/datadog/appsec/event_spec.rb +++ b/spec/datadog/appsec/event_spec.rb @@ -2,217 +2,311 @@ require 'datadog/appsec/event' RSpec.describe Datadog::AppSec::Event do - context 'self' do - describe '#record' do - before do - # prevent rate limiter to bias tests - Datadog::AppSec::RateLimiter.reset!(:traces) - end + context 'ALLOWED_REQUEST_HEADERS' do + it 'store the correct values' do + expect(described_class::ALLOWED_REQUEST_HEADERS).to eq( + [ + 'x-forwarded-for', + 'x-client-ip', + 'x-real-ip', + 'x-forwarded', + 'x-cluster-client-ip', + 'forwarded-for', + 'forwarded', + 'via', + 'true-client-ip', + 'content-length', + 'content-type', + 'content-encoding', + 'content-language', + 'host', + 'user-agent', + 'accept', + 'accept-encoding', + 'accept-language' + ] + ) + end + end - let(:options) { {} } - let(:trace_op) { Datadog::Tracing::TraceOperation.new(**options) } - let(:trace) { trace_op.flush! } + context 'ALLOWED_RESPONSE_HEADERS' do + it 'store the correct values' do + expect(described_class::ALLOWED_RESPONSE_HEADERS).to eq( + [ + 'content-length', + 'content-type', + 'content-encoding', + 'content-language' + ] + ) + end + end - let(:rack_request) do - dbl = double + describe '.record' do + before do + # prevent rate limiter to bias tests + Datadog::AppSec::RateLimiter.reset!(:traces) + end - allow(dbl).to receive(:host).and_return('example.com') - allow(dbl).to receive(:user_agent).and_return('Ruby/0.0') - allow(dbl).to receive(:remote_addr).and_return('127.0.0.1') + let(:options) { {} } + let(:trace_op) { Datadog::Tracing::TraceOperation.new(**options) } + let(:trace) { trace_op.flush! } - allow(dbl).to receive(:headers).and_return [ - ['user-agent', 'Ruby/0.0'], - ['SERVER_NAME', 'example.com'], - ['REMOTE_ADDR', '127.0.0.1'] - ] + let(:rack_request_headers) do + { + 'user-agent' => 'Ruby/0.0', + 'SERVER_NAME' => 'example.com', + 'REMOTE_ADDR' => '127.0.0.1', + } + end - dbl - end + let(:rack_response_headers) do + { + 'content-type' => 'text/html' + } + end - let(:rack_response) do - dbl = double + let(:rack_request) do + dbl = double - allow(dbl).to receive(:headers).and_return([]) + allow(dbl).to receive(:host).and_return('example.com') + allow(dbl).to receive(:user_agent).and_return('Ruby/0.0') + allow(dbl).to receive(:remote_addr).and_return('127.0.0.1') - dbl - end + allow(dbl).to receive(:headers).and_return(rack_request_headers) - let(:waf_result) do - dbl = double + dbl + end - allow(dbl).to receive(:events).and_return([]) - allow(dbl).to receive(:derivatives).and_return(derivatives) + let(:rack_response) do + dbl = double - dbl - end + allow(dbl).to receive(:headers).and_return(rack_response_headers) - let(:event_count) { 1 } - let(:derivatives) { {} } + dbl + end - let(:events) do - Array.new(event_count) do - { - trace: trace_op, - span: nil, # backfilled later - request: rack_request, - response: rack_response, - waf_result: waf_result, - } - end - end + let(:waf_events) do + [1, { a: :b }] + end - context 'with one event' do - let(:trace) do - trace_op.measure('request') do |span| - events.each { |e| e[:span] = span } + let(:waf_result) do + dbl = double - 10.times do |i| - trace_op.measure("span #{i}") {} - end + allow(dbl).to receive(:events).and_return(waf_events) + allow(dbl).to receive(:derivatives).and_return(derivatives) - described_class.record(span, *events) + dbl + end + + let(:event_count) { 1 } + let(:derivatives) { {} } + + let(:events) do + Array.new(event_count) do + { + trace: trace_op, + span: nil, # backfilled later + request: rack_request, + response: rack_response, + waf_result: waf_result, + } + end + end + + context 'with one event' do + let(:trace) do + trace_op.measure('request') do |span| + events.each { |e| e[:span] = span } + + 10.times do |i| + trace_op.measure("span #{i}") {} end - trace_op.flush! + described_class.record(span, *events) end - let(:top_level_span) do - trace.spans.find { |s| s.metrics['_dd.top_level'] && s.metrics['_dd.top_level'] > 0.0 } - end + trace_op.flush! + end - let(:other_spans) do - trace.spans - [top_level_span] - end + let(:top_level_span) do + trace.spans.find { |s| s.metrics['_dd.top_level'] && s.metrics['_dd.top_level'] > 0.0 } + end + + let(:other_spans) do + trace.spans - [top_level_span] + end - it 'records an event on the top level span' do - expect(top_level_span.meta).to eq( - '_dd.appsec.json' => '{"triggers":[]}', - 'http.host' => 'example.com', - 'http.useragent' => 'Ruby/0.0', - 'http.request.headers.user-agent' => 'Ruby/0.0', - 'network.client.ip' => '127.0.0.1', - '_dd.origin' => 'appsec', - ) + context 'request headers' do + context 'allowed headers' do + it 'records allowed headers' do + expect(top_level_span.meta).to include('http.request.headers.user-agent' => 'Ruby/0.0') + end end - it 'records nothing on other spans' do - other_spans.each do |other_span| - expect(other_span.meta).to be_empty + context 'discard not allowed headers' do + let(:rack_request_headers) do + { + 'not-supported-header' => 'foo', + } + end + + it 'does not records allowed headers' do + expect(top_level_span.meta).to_not include('http.request.headers.not-supported-header') end end + end - it 'marks the trace to be kept' do - expect(trace.sampling_priority).to eq Datadog::Tracing::Sampling::Ext::Priority::USER_KEEP + context 'response headers' do + context 'allowed headers' do + it 'records allowed headers' do + expect(top_level_span.meta).to include('http.response.headers.content-type' => 'text/html') + end end - context 'waf_result derivatives' do - let(:derivatives) do + context 'discard not allowed headers' do + let(:rack_response_headers) do { - '_dd.appsec.s.req.headers' => [{ 'host' => [8], 'version' => [8] }] + 'not-supported-header' => 'foo', } end - context 'JSON payload' do - it 'uses JSON string when do not exceeds MIN_SCHEMA_SIZE_FOR_COMPRESSION' do - stub_const('Datadog::AppSec::Event::MIN_SCHEMA_SIZE_FOR_COMPRESSION', 3000) - meta = top_level_span.meta - - expect(meta['_dd.appsec.s.req.headers']).to eq('[{"host":[8],"version":[8]}]') - end + it 'does not records allowed headers' do + expect(top_level_span.meta).to_not include('http.response.headers.not-supported-header') end + end + end - context 'Compressed payload' do - it 'uses compressed value when JSON string is bigger than MIN_SCHEMA_SIZE_FOR_COMPRESSION' do - result = "H4sIAOYoHGUAA4aphwAAAA=\n" - stub_const('Datadog::AppSec::Event::MIN_SCHEMA_SIZE_FOR_COMPRESSION', 1) - expect(described_class).to receive(:compressed_and_base64_encoded).and_return(result) + context 'http information' do + it 'records http information' do + expect(top_level_span.meta).to include('http.host' => 'example.com') + expect(top_level_span.meta).to include('http.useragent' => 'Ruby/0.0') + expect(top_level_span.meta).to include('network.client.ip' => '127.0.0.1') + end + end - meta = top_level_span.meta + it 'records an event on the top level span' do + expect(top_level_span.meta).to include('_dd.appsec.json' => '{"triggers":[1,{"a":"b"}]}') + expect(top_level_span.meta).to include('_dd.origin' => 'appsec') + end - expect(meta['_dd.appsec.s.req.headers']).to eq(result) - end - end + it 'records nothing on other spans' do + other_spans.each do |other_span| + expect(other_span.meta).to be_empty + end + end + + it 'marks the trace to be kept' do + expect(trace.sampling_priority).to eq Datadog::Tracing::Sampling::Ext::Priority::USER_KEEP + end - context 'derivative values exceed Event::MAX_ENCODED_SCHEMA_SIZE value' do - it 'do not add derivative key to meta' do - stub_const('Datadog::AppSec::Event::MAX_ENCODED_SCHEMA_SIZE', 1) - meta = top_level_span.meta + context 'waf_result derivatives' do + let(:derivatives) do + { + '_dd.appsec.s.req.headers' => [{ 'host' => [8], 'version' => [8] }] + } + end - expect(meta['_dd.appsec.s.req.headers']).to be_nil - end + context 'JSON payload' do + it 'uses JSON string when do not exceeds MIN_SCHEMA_SIZE_FOR_COMPRESSION' do + stub_const('Datadog::AppSec::Event::MIN_SCHEMA_SIZE_FOR_COMPRESSION', 3000) + meta = top_level_span.meta + + expect(meta['_dd.appsec.s.req.headers']).to eq('[{"host":[8],"version":[8]}]') end end - end - context 'with no event' do - let(:event_count) { 0 } + context 'Compressed payload' do + it 'uses compressed value when JSON string is bigger than MIN_SCHEMA_SIZE_FOR_COMPRESSION' do + result = "H4sIAOYoHGUAA4aphwAAAA=\n" + stub_const('Datadog::AppSec::Event::MIN_SCHEMA_SIZE_FOR_COMPRESSION', 1) + expect(described_class).to receive(:compressed_and_base64_encoded).and_return(result) - let(:trace) do - trace_op.measure('request') do |span| - events.each { |e| e[:span] = span } + meta = top_level_span.meta - described_class.record(span, *events) + expect(meta['_dd.appsec.s.req.headers']).to eq(result) end - - trace_op.flush! end - it 'does not mark the trace to be kept' do - expect(trace.sampling_priority).to eq nil + context 'derivative values exceed Event::MAX_ENCODED_SCHEMA_SIZE value' do + it 'do not add derivative key to meta' do + stub_const('Datadog::AppSec::Event::MAX_ENCODED_SCHEMA_SIZE', 1) + meta = top_level_span.meta + + expect(meta['_dd.appsec.s.req.headers']).to be_nil + end end + end + end + + context 'with no event' do + let(:event_count) { 0 } - it 'does not attempt to record in the trace' do - expect(described_class).to_not receive(:record_via_span) + let(:trace) do + trace_op.measure('request') do |span| + events.each { |e| e[:span] = span } - expect(trace).to_not be nil + described_class.record(span, *events) end - it 'does not call the rate limiter' do - expect(Datadog::AppSec::RateLimiter).to_not receive(:limit) + trace_op.flush! + end - expect(trace).to_not be nil - end + it 'does not mark the trace to be kept' do + expect(trace.sampling_priority).to eq nil end - context 'with no span' do - let(:event_count) { 1 } + it 'does not attempt to record in the trace' do + expect(described_class).to_not receive(:record_via_span) - it 'does not attempt to record in the trace' do - expect(described_class).to_not receive(:record_via_span) + expect(trace).to_not be nil + end - described_class.record(nil, events) - end + it 'does not call the rate limiter' do + expect(Datadog::AppSec::RateLimiter).to_not receive(:limit) - it 'does not call the rate limiter' do - expect(Datadog::AppSec::RateLimiter).to_not receive(:limit) + expect(trace).to_not be nil + end + end - described_class.record(nil, events) - end + context 'with no span' do + let(:event_count) { 1 } + + it 'does not attempt to record in the trace' do + expect(described_class).to_not receive(:record_via_span) + + described_class.record(nil, events) end - context 'with many traces' do - let(:rate_limit) { 100 } - let(:trace_count) { rate_limit * 2 } + it 'does not call the rate limiter' do + expect(Datadog::AppSec::RateLimiter).to_not receive(:limit) - let(:traces) do - Array.new(trace_count) do - trace_op = Datadog::Tracing::TraceOperation.new(**options) + described_class.record(nil, events) + end + end - trace_op.measure('request') do |span| - events.each { |e| e[:span] = span } + context 'with many traces' do + let(:rate_limit) { 100 } + let(:trace_count) { rate_limit * 2 } - described_class.record(span, *events) - end + let(:traces) do + Array.new(trace_count) do + trace_op = Datadog::Tracing::TraceOperation.new(**options) - trace_op.keep! + trace_op.measure('request') do |span| + events.each { |e| e[:span] = span } + + described_class.record(span, *events) end + + trace_op.keep! end + end - it 'rate limits event recording' do - expect(described_class).to receive(:record_via_span).exactly(rate_limit).times.and_call_original + it 'rate limits event recording' do + expect(described_class).to receive(:record_via_span).exactly(rate_limit).times.and_call_original - expect(traces).to have_attributes(count: trace_count) - end + expect(traces).to have_attributes(count: trace_count) end end end