From 3247f010a8f32a01c91f04f54d3a96d5271d5cff Mon Sep 17 00:00:00 2001 From: ishabi Date: Mon, 18 Nov 2024 09:30:54 +0100 Subject: [PATCH] add support to api security sampling (#4755) * add support to api security sampling * fix express plugin schema extraction * use priority simpler to get span priority * use lru cache package * use route path instead of url * use route.path or url to generate the key * use ttlcache * Fix standalone integration test * Increase test timeout * simplify force sample * avoid checking is sampled twice * use route.path or url to generate the key * remove unnecessary tests of sample delay * fix non experimental options test * remove unused isSampled * always sample request if delay is 0 --------- Co-authored-by: Igor Unanua Co-authored-by: simon-id --- LICENSE-3rdparty.csv | 1 + docs/test.ts | 1 - index.d.ts | 10 +- integration-tests/standalone-asm.spec.js | 51 +++-- package.json | 1 + .../datadog-instrumentations/src/express.js | 2 +- .../src/appsec/api_security_sampler.js | 77 ++++--- packages/dd-trace/src/appsec/index.js | 12 +- .../src/appsec/remote_config/capabilities.js | 2 +- .../src/appsec/remote_config/index.js | 7 - packages/dd-trace/src/config.js | 7 +- .../test/appsec/api_security_sampler.spec.js | 199 +++++++++++++++--- .../test/appsec/index.express.plugin.spec.js | 9 +- .../appsec/index.sequelize.plugin.spec.js | 2 +- packages/dd-trace/test/appsec/index.spec.js | 136 ++++++------ .../test/appsec/remote_config/index.spec.js | 129 ------------ .../test/appsec/response_blocking.spec.js | 3 + packages/dd-trace/test/config.spec.js | 48 +---- yarn.lock | 5 + 19 files changed, 352 insertions(+), 350 deletions(-) diff --git a/LICENSE-3rdparty.csv b/LICENSE-3rdparty.csv index 772cd9b2553..f078d0aa4ae 100644 --- a/LICENSE-3rdparty.csv +++ b/LICENSE-3rdparty.csv @@ -8,6 +8,7 @@ require,@datadog/pprof,Apache license 2.0,Copyright 2019 Google Inc. require,@datadog/sketches-js,Apache license 2.0,Copyright 2020 Datadog Inc. require,@opentelemetry/api,Apache license 2.0,Copyright OpenTelemetry Authors require,@opentelemetry/core,Apache license 2.0,Copyright OpenTelemetry Authors +require,@isaacs/ttlcache,ISC,Copyright (c) 2022-2023 - Isaac Z. Schlueter and Contributors require,crypto-randomuuid,MIT,Copyright 2021 Node.js Foundation and contributors require,dc-polyfill,MIT,Copyright 2023 Datadog Inc. require,ignore,MIT,Copyright 2013 Kael Zhang and contributors diff --git a/docs/test.ts b/docs/test.ts index 37342718c2a..8991c8680a5 100644 --- a/docs/test.ts +++ b/docs/test.ts @@ -115,7 +115,6 @@ tracer.init({ }, apiSecurity: { enabled: true, - requestSampling: 1.0 }, rasp: { enabled: true diff --git a/index.d.ts b/index.d.ts index 940ca6a06db..f8d4679c570 100644 --- a/index.d.ts +++ b/index.d.ts @@ -662,19 +662,13 @@ declare namespace tracer { mode?: 'safe' | 'extended' | 'disabled' }, /** - * Configuration for Api Security sampling + * Configuration for Api Security */ apiSecurity?: { /** Whether to enable Api Security. - * @default false + * @default true */ enabled?: boolean, - - /** Controls the request sampling rate (between 0 and 1) in which Api Security is triggered. - * The value will be coerced back if it's outside of the 0-1 range. - * @default 0.1 - */ - requestSampling?: number }, /** * Configuration for RASP diff --git a/integration-tests/standalone-asm.spec.js b/integration-tests/standalone-asm.spec.js index 4e57b25bad6..fec30ad012b 100644 --- a/integration-tests/standalone-asm.spec.js +++ b/integration-tests/standalone-asm.spec.js @@ -81,33 +81,42 @@ describe('Standalone ASM', () => { }) }) - it('should keep second req because RateLimiter allows 1 req/min and discard the next', async () => { - // 1st req kept because waf init - // 2nd req kept because it's the first one hitting RateLimiter - // next in the first minute are dropped - await doWarmupRequests(proc) - - return curlAndAssertMessage(agent, proc, ({ headers, payload }) => { + it('should keep fifth req because RateLimiter allows 1 req/min', async () => { + const promise = curlAndAssertMessage(agent, proc, ({ headers, payload }) => { assert.propertyVal(headers, 'datadog-client-computed-stats', 'yes') assert.isArray(payload) - assert.strictEqual(payload.length, 4) - - const secondReq = payload[1] - assert.isArray(secondReq) - assert.strictEqual(secondReq.length, 5) + if (payload.length === 4) { + assertKeep(payload[0][0]) + assertDrop(payload[1][0]) + assertDrop(payload[2][0]) + assertDrop(payload[3][0]) + + // req after a minute + } else { + const fifthReq = payload[0] + assert.isArray(fifthReq) + assert.strictEqual(fifthReq.length, 5) + + const { meta, metrics } = fifthReq[0] + assert.notProperty(meta, 'manual.keep') + assert.notProperty(meta, '_dd.p.appsec') + + assert.propertyVal(metrics, '_sampling_priority_v1', AUTO_KEEP) + assert.propertyVal(metrics, '_dd.apm.enabled', 0) + } + }, 70000, 2) - const { meta, metrics } = secondReq[0] - assert.notProperty(meta, 'manual.keep') - assert.notProperty(meta, '_dd.p.appsec') + // 1st req kept because waf init + // next in the first minute are dropped + // 5nd req kept because RateLimiter allows 1 req/min + await doWarmupRequests(proc) - assert.propertyVal(metrics, '_sampling_priority_v1', AUTO_KEEP) - assert.propertyVal(metrics, '_dd.apm.enabled', 0) + await new Promise(resolve => setTimeout(resolve, 60000)) - assertDrop(payload[2][0]) + await curl(proc) - assertDrop(payload[3][0]) - }) - }) + return promise + }).timeout(70000) it('should keep attack requests', async () => { await doWarmupRequests(proc) diff --git a/package.json b/package.json index a66ca1197d2..70b8fca7b2c 100644 --- a/package.json +++ b/package.json @@ -89,6 +89,7 @@ "@datadog/native-metrics": "^3.0.1", "@datadog/pprof": "5.4.1", "@datadog/sketches-js": "^2.1.0", + "@isaacs/ttlcache": "^1.4.1", "@opentelemetry/api": ">=1.0.0 <1.9.0", "@opentelemetry/core": "^1.14.0", "crypto-randomuuid": "^1.0.0", diff --git a/packages/datadog-instrumentations/src/express.js b/packages/datadog-instrumentations/src/express.js index b093eab7830..74e159fb042 100644 --- a/packages/datadog-instrumentations/src/express.js +++ b/packages/datadog-instrumentations/src/express.js @@ -29,7 +29,7 @@ function wrapResponseJson (json) { obj = arguments[1] } - responseJsonChannel.publish({ req: this.req, body: obj }) + responseJsonChannel.publish({ req: this.req, res: this, body: obj }) } return json.apply(this, arguments) diff --git a/packages/dd-trace/src/appsec/api_security_sampler.js b/packages/dd-trace/src/appsec/api_security_sampler.js index 68bd896af7e..c95ec820f1c 100644 --- a/packages/dd-trace/src/appsec/api_security_sampler.js +++ b/packages/dd-trace/src/appsec/api_security_sampler.js @@ -1,61 +1,84 @@ 'use strict' +const TTLCache = require('@isaacs/ttlcache') +const web = require('../plugins/util/web') const log = require('../log') +const { AUTO_REJECT, USER_REJECT } = require('../../../../ext/priority') + +const MAX_SIZE = 4096 let enabled -let requestSampling +let sampledRequests -const sampledRequests = new WeakSet() +class NoopTTLCache { + clear () { } + set (key) { return undefined } + has (key) { return false } +} function configure ({ apiSecurity }) { enabled = apiSecurity.enabled - setRequestSampling(apiSecurity.requestSampling) + sampledRequests = apiSecurity.sampleDelay === 0 + ? new NoopTTLCache() + : new TTLCache({ max: MAX_SIZE, ttl: apiSecurity.sampleDelay * 1000 }) } function disable () { enabled = false + sampledRequests?.clear() } -function setRequestSampling (sampling) { - requestSampling = parseRequestSampling(sampling) -} +function sampleRequest (req, res, force = false) { + if (!enabled) return false -function parseRequestSampling (requestSampling) { - let parsed = parseFloat(requestSampling) + const key = computeKey(req, res) + if (!key || isSampled(key)) return false - if (isNaN(parsed)) { - log.warn(`Incorrect API Security request sampling value: ${requestSampling}`) + const rootSpan = web.root(req) + if (!rootSpan) return false - parsed = 0 - } else { - parsed = Math.min(1, Math.max(0, parsed)) + let priority = getSpanPriority(rootSpan) + if (!priority) { + rootSpan._prioritySampler?.sample(rootSpan) + priority = getSpanPriority(rootSpan) } - return parsed -} - -function sampleRequest (req) { - if (!enabled || !requestSampling) { + if (priority === AUTO_REJECT || priority === USER_REJECT) { return false } - const shouldSample = Math.random() <= requestSampling - - if (shouldSample) { - sampledRequests.add(req) + if (force) { + sampledRequests.set(key) } - return shouldSample + return true +} + +function isSampled (key) { + return sampledRequests.has(key) +} + +function computeKey (req, res) { + const route = web.getContext(req)?.paths?.join('') || '' + const method = req.method + const status = res.statusCode + + if (!method || !status) { + log.warn('Unsupported groupkey for API security') + return null + } + return method + route + status } -function isSampled (req) { - return sampledRequests.has(req) +function getSpanPriority (span) { + const spanContext = span.context?.() + return spanContext._sampling?.priority } module.exports = { configure, disable, - setRequestSampling, sampleRequest, - isSampled + isSampled, + computeKey } diff --git a/packages/dd-trace/src/appsec/index.js b/packages/dd-trace/src/appsec/index.js index f4f9a4db036..0a4f6fbb992 100644 --- a/packages/dd-trace/src/appsec/index.js +++ b/packages/dd-trace/src/appsec/index.js @@ -145,10 +145,6 @@ function incomingHttpStartTranslator ({ req, res, abortController }) { persistent[addresses.HTTP_CLIENT_IP] = clientIp } - if (apiSecuritySampler.sampleRequest(req)) { - persistent[addresses.WAF_CONTEXT_PROCESSOR] = { 'extract-schema': true } - } - const actions = waf.run({ persistent }, req) handleResults(actions, req, res, rootSpan, abortController) @@ -172,6 +168,10 @@ function incomingHttpEndTranslator ({ req, res }) { persistent[addresses.HTTP_INCOMING_QUERY] = req.query } + if (apiSecuritySampler.sampleRequest(req, res, true)) { + persistent[addresses.WAF_CONTEXT_PROCESSOR] = { 'extract-schema': true } + } + if (Object.keys(persistent).length) { waf.run({ persistent }, req) } @@ -228,9 +228,9 @@ function onRequestProcessParams ({ req, res, abortController, params }) { handleResults(results, req, res, rootSpan, abortController) } -function onResponseBody ({ req, body }) { +function onResponseBody ({ req, res, body }) { if (!body || typeof body !== 'object') return - if (!apiSecuritySampler.isSampled(req)) return + if (!apiSecuritySampler.sampleRequest(req, res)) return // we don't support blocking at this point, so no results needed waf.run({ diff --git a/packages/dd-trace/src/appsec/remote_config/capabilities.js b/packages/dd-trace/src/appsec/remote_config/capabilities.js index 18c11a92104..bd729cc39cc 100644 --- a/packages/dd-trace/src/appsec/remote_config/capabilities.js +++ b/packages/dd-trace/src/appsec/remote_config/capabilities.js @@ -11,7 +11,7 @@ module.exports = { ASM_CUSTOM_RULES: 1n << 8n, ASM_CUSTOM_BLOCKING_RESPONSE: 1n << 9n, ASM_TRUSTED_IPS: 1n << 10n, - ASM_API_SECURITY_SAMPLE_RATE: 1n << 11n, + ASM_API_SECURITY_SAMPLE_RATE: 1n << 11n, // deprecated APM_TRACING_SAMPLE_RATE: 1n << 12n, APM_TRACING_LOGS_INJECTION: 1n << 13n, APM_TRACING_HTTP_HEADER_TAGS: 1n << 14n, diff --git a/packages/dd-trace/src/appsec/remote_config/index.js b/packages/dd-trace/src/appsec/remote_config/index.js index 9f0869351af..90cda5c6f61 100644 --- a/packages/dd-trace/src/appsec/remote_config/index.js +++ b/packages/dd-trace/src/appsec/remote_config/index.js @@ -4,7 +4,6 @@ const Activation = require('../activation') const RemoteConfigManager = require('./manager') const RemoteConfigCapabilities = require('./capabilities') -const apiSecuritySampler = require('../api_security_sampler') let rc @@ -24,18 +23,12 @@ function enable (config, appsec) { rc.updateCapabilities(RemoteConfigCapabilities.ASM_ACTIVATION, true) } - if (config.appsec.apiSecurity?.enabled) { - rc.updateCapabilities(RemoteConfigCapabilities.ASM_API_SECURITY_SAMPLE_RATE, true) - } - rc.setProductHandler('ASM_FEATURES', (action, rcConfig) => { if (!rcConfig) return if (activation === Activation.ONECLICK) { enableOrDisableAppsec(action, rcConfig, config, appsec) } - - apiSecuritySampler.setRequestSampling(rcConfig.api_security?.request_sample_rate) }) } diff --git a/packages/dd-trace/src/config.js b/packages/dd-trace/src/config.js index c50c05f794a..05de1cdf600 100644 --- a/packages/dd-trace/src/config.js +++ b/packages/dd-trace/src/config.js @@ -444,7 +444,7 @@ class Config { const defaults = setHiddenProperty(this, '_defaults', {}) this._setValue(defaults, 'appsec.apiSecurity.enabled', true) - this._setValue(defaults, 'appsec.apiSecurity.requestSampling', 0.1) + this._setValue(defaults, 'appsec.apiSecurity.sampleDelay', 30) this._setValue(defaults, 'appsec.blockedTemplateGraphql', undefined) this._setValue(defaults, 'appsec.blockedTemplateHtml', undefined) this._setValue(defaults, 'appsec.blockedTemplateJson', undefined) @@ -571,7 +571,7 @@ class Config { AWS_LAMBDA_FUNCTION_NAME, DD_AGENT_HOST, DD_API_SECURITY_ENABLED, - DD_API_SECURITY_REQUEST_SAMPLE_RATE, + DD_API_SECURITY_SAMPLE_DELAY, DD_APPSEC_AUTOMATED_USER_EVENTS_TRACKING, DD_APPSEC_ENABLED, DD_APPSEC_GRAPHQL_BLOCKED_TEMPLATE_JSON, @@ -700,7 +700,7 @@ class Config { DD_API_SECURITY_ENABLED && isTrue(DD_API_SECURITY_ENABLED), DD_EXPERIMENTAL_API_SECURITY_ENABLED && isTrue(DD_EXPERIMENTAL_API_SECURITY_ENABLED) )) - this._setUnit(env, 'appsec.apiSecurity.requestSampling', DD_API_SECURITY_REQUEST_SAMPLE_RATE) + this._setValue(env, 'appsec.apiSecurity.sampleDelay', maybeFloat(DD_API_SECURITY_SAMPLE_DELAY)) this._setValue(env, 'appsec.blockedTemplateGraphql', maybeFile(DD_APPSEC_GRAPHQL_BLOCKED_TEMPLATE_JSON)) this._setValue(env, 'appsec.blockedTemplateHtml', maybeFile(DD_APPSEC_HTTP_BLOCKED_TEMPLATE_HTML)) this._envUnprocessed['appsec.blockedTemplateHtml'] = DD_APPSEC_HTTP_BLOCKED_TEMPLATE_HTML @@ -880,7 +880,6 @@ class Config { tagger.add(tags, options.tags) this._setBoolean(opts, 'appsec.apiSecurity.enabled', options.appsec.apiSecurity?.enabled) - this._setUnit(opts, 'appsec.apiSecurity.requestSampling', options.appsec.apiSecurity?.requestSampling) this._setValue(opts, 'appsec.blockedTemplateGraphql', maybeFile(options.appsec.blockedTemplateGraphql)) this._setValue(opts, 'appsec.blockedTemplateHtml', maybeFile(options.appsec.blockedTemplateHtml)) this._optsUnprocessed['appsec.blockedTemplateHtml'] = options.appsec.blockedTemplateHtml diff --git a/packages/dd-trace/test/appsec/api_security_sampler.spec.js b/packages/dd-trace/test/appsec/api_security_sampler.spec.js index 5a69af05a5c..9944f9d0871 100644 --- a/packages/dd-trace/test/appsec/api_security_sampler.spec.js +++ b/packages/dd-trace/test/appsec/api_security_sampler.spec.js @@ -1,71 +1,200 @@ 'use strict' -const apiSecuritySampler = require('../../src/appsec/api_security_sampler') +const proxyquire = require('proxyquire') +const { assert } = require('chai') +const { performance } = require('node:perf_hooks') +const { USER_KEEP, AUTO_KEEP, AUTO_REJECT, USER_REJECT } = require('../../../../ext/priority') -describe('Api Security Sampler', () => { - let config +describe('API Security Sampler', () => { + const req = { route: { path: '/test' }, method: 'GET' } + const res = { statusCode: 200 } + let apiSecuritySampler, webStub, span, clock, performanceNowStub beforeEach(() => { - config = { - apiSecurity: { - enabled: true, - requestSampling: 1 + clock = sinon.useFakeTimers({ now: 10 }) + performanceNowStub = sinon.stub(performance, 'now').callsFake(() => clock.now) + + webStub = { + root: sinon.stub(), + getContext: sinon.stub(), + _prioritySampler: { + isSampled: sinon.stub() } } - sinon.stub(Math, 'random').returns(0.3) + apiSecuritySampler = proxyquire('../../src/appsec/api_security_sampler', { + '../plugins/util/web': webStub + }) + + span = { + context: sinon.stub().returns({ + _sampling: { priority: AUTO_KEEP } + }) + } + + webStub.root.returns(span) + webStub.getContext.returns({ paths: ['path'] }) + }) + + afterEach(() => { + apiSecuritySampler.disable() + performanceNowStub.restore() + clock.restore() }) - afterEach(sinon.restore) + it('should return false if not enabled', () => { + apiSecuritySampler.disable() + assert.isFalse(apiSecuritySampler.sampleRequest({}, {})) + }) - describe('sampleRequest', () => { - it('should sample request if enabled and sampling 1', () => { - apiSecuritySampler.configure(config) + it('should return false if no root span', () => { + webStub.root.returns(null) + assert.isFalse(apiSecuritySampler.sampleRequest({}, {})) + }) + + it('should return false for AUTO_REJECT priority', () => { + span.context.returns({ _sampling: { priority: AUTO_REJECT } }) + assert.isFalse(apiSecuritySampler.sampleRequest(req, res)) + }) - expect(apiSecuritySampler.sampleRequest({})).to.true + it('should return false for USER_REJECT priority', () => { + span.context.returns({ _sampling: { priority: USER_REJECT } }) + assert.isFalse(apiSecuritySampler.sampleRequest(req, res)) + }) + + it('should not sample when method or statusCode is not available', () => { + assert.isFalse(apiSecuritySampler.sampleRequest(req, {}, true)) + assert.isFalse(apiSecuritySampler.sampleRequest({}, res, true)) + }) + + describe('with TTLCache', () => { + beforeEach(() => { + apiSecuritySampler.configure({ apiSecurity: { enabled: true, sampleDelay: 30 } }) }) - it('should not sample request if enabled and sampling 0', () => { - config.apiSecurity.requestSampling = 0 - apiSecuritySampler.configure(config) + it('should not sample before 30 seconds', () => { + assert.isTrue(apiSecuritySampler.sampleRequest(req, res, true)) + + clock.tick(25000) - expect(apiSecuritySampler.sampleRequest({})).to.false + assert.isFalse(apiSecuritySampler.sampleRequest(req, res, true)) + const key = apiSecuritySampler.computeKey(req, res) + assert.isTrue(apiSecuritySampler.isSampled(key)) }) - it('should sample request if enabled and sampling greater than random', () => { - config.apiSecurity.requestSampling = 0.5 + it('should sample after 30 seconds', () => { + assert.isTrue(apiSecuritySampler.sampleRequest(req, res, true)) - apiSecuritySampler.configure(config) + clock.tick(35000) - expect(apiSecuritySampler.sampleRequest({})).to.true + assert.isTrue(apiSecuritySampler.sampleRequest(req, res, true)) }) - it('should not sample request if enabled and sampling less than random', () => { - config.apiSecurity.requestSampling = 0.1 + it('should remove oldest entry when max size is exceeded', () => { + for (let i = 0; i < 4097; i++) { + const path = `/test${i}` + webStub.getContext.returns({ paths: [path] }) + assert.isTrue(apiSecuritySampler.sampleRequest(req, res, true)) + } - apiSecuritySampler.configure(config) + webStub.getContext.returns({ paths: ['/test0'] }) + const key1 = apiSecuritySampler.computeKey(req, res) + assert.isFalse(apiSecuritySampler.isSampled(key1)) - expect(apiSecuritySampler.sampleRequest()).to.false + webStub.getContext.returns({ paths: ['/test4096'] }) + const key2 = apiSecuritySampler.computeKey(req, res) + assert.isTrue(apiSecuritySampler.isSampled(key2)) }) - it('should not sample request if incorrect config value', () => { - config.apiSecurity.requestSampling = NaN + it('should set enabled to false and clear the cache', () => { + assert.isTrue(apiSecuritySampler.sampleRequest(req, res, true)) + + apiSecuritySampler.disable() + + assert.isFalse(apiSecuritySampler.sampleRequest(req, res, true)) + }) - apiSecuritySampler.configure(config) + it('should create different keys for different methods', () => { + const getReq = { method: 'GET' } + const postReq = { method: 'POST' } + assert.isTrue(apiSecuritySampler.sampleRequest(getReq, res, true)) + assert.isTrue(apiSecuritySampler.sampleRequest(postReq, res, true)) - expect(apiSecuritySampler.sampleRequest()).to.false + const key1 = apiSecuritySampler.computeKey(getReq, res) + assert.isTrue(apiSecuritySampler.isSampled(key1)) + const key2 = apiSecuritySampler.computeKey(postReq, res) + assert.isTrue(apiSecuritySampler.isSampled(key2)) }) - it('should sample request according to the config', () => { - config.apiSecurity.requestSampling = 1 + it('should create different keys for different status codes', () => { + const res200 = { statusCode: 200 } + const res404 = { statusCode: 404 } - apiSecuritySampler.configure(config) + assert.isTrue(apiSecuritySampler.sampleRequest(req, res200, true)) + assert.isTrue(apiSecuritySampler.sampleRequest(req, res404, true)) - expect(apiSecuritySampler.sampleRequest({})).to.true + const key1 = apiSecuritySampler.computeKey(req, res200) + assert.isTrue(apiSecuritySampler.isSampled(key1)) + const key2 = apiSecuritySampler.computeKey(req, res404) + assert.isTrue(apiSecuritySampler.isSampled(key2)) + }) + + it('should sample for AUTO_KEEP priority without checking prioritySampler', () => { + span.context.returns({ _sampling: { priority: AUTO_KEEP } }) + assert.isTrue(apiSecuritySampler.sampleRequest(req, res)) + }) + + it('should sample for USER_KEEP priority without checking prioritySampler', () => { + span.context.returns({ _sampling: { priority: USER_KEEP } }) + assert.isTrue(apiSecuritySampler.sampleRequest(req, res)) + }) + }) + + describe('with NoopTTLCache', () => { + beforeEach(() => { + apiSecuritySampler.configure({ apiSecurity: { enabled: true, sampleDelay: 0 } }) + }) + + it('should always return true for sampleRequest', () => { + assert.isTrue(apiSecuritySampler.sampleRequest(req, res, true)) + + assert.isTrue(apiSecuritySampler.sampleRequest(req, res, true)) + + clock.tick(50000) + assert.isTrue(apiSecuritySampler.sampleRequest(req, res, true)) + }) + + it('should never mark requests as sampled', () => { + apiSecuritySampler.sampleRequest(req, res, true) + const key = apiSecuritySampler.computeKey(req, res) + assert.isFalse(apiSecuritySampler.isSampled(key)) + }) + + it('should handle multiple different requests', () => { + const requests = [ + { req: { method: 'GET', route: { path: '/test1' } }, res: { statusCode: 200 } }, + { req: { method: 'POST', route: { path: '/test2' } }, res: { statusCode: 201 } }, + { req: { method: 'PUT', route: { path: '/test3' } }, res: { statusCode: 204 } } + ] + + requests.forEach(({ req, res }) => { + assert.isTrue(apiSecuritySampler.sampleRequest(req, res, true)) + const key = apiSecuritySampler.computeKey(req, res) + assert.isFalse(apiSecuritySampler.isSampled(key)) + }) + }) + + it('should not be affected by max size', () => { + for (let i = 0; i < 5000; i++) { + webStub.getContext.returns({ paths: [`/test${i}`] }) + assert.isTrue(apiSecuritySampler.sampleRequest(req, res, true)) + } - apiSecuritySampler.setRequestSampling(0) + webStub.getContext.returns({ paths: ['/test0'] }) + assert.isTrue(apiSecuritySampler.sampleRequest(req, res, true)) - expect(apiSecuritySampler.sampleRequest()).to.false + webStub.getContext.returns({ paths: ['/test4999'] }) + assert.isTrue(apiSecuritySampler.sampleRequest(req, res, true)) }) }) }) diff --git a/packages/dd-trace/test/appsec/index.express.plugin.spec.js b/packages/dd-trace/test/appsec/index.express.plugin.spec.js index c38d496623b..bb674015f78 100644 --- a/packages/dd-trace/test/appsec/index.express.plugin.spec.js +++ b/packages/dd-trace/test/appsec/index.express.plugin.spec.js @@ -275,7 +275,7 @@ withVersions('express', 'express', version => { }) app.post('/json', (req, res) => { - res.jsonp({ jsonResKey: 'jsonResValue' }) + res.json({ jsonResKey: 'jsonResValue' }) }) getPort().then((port) => { @@ -307,9 +307,9 @@ withVersions('express', 'express', version => { appsec.disable() }) - describe('with requestSampling 1.0', () => { + describe('with sample delay 10', () => { beforeEach(() => { - config.appsec.apiSecurity.requestSampling = 1.0 + config.appsec.apiSecurity.sampleDelay = 10 appsec.enable(config) }) @@ -374,7 +374,8 @@ withVersions('express', 'express', version => { }) it('should not get the schema', async () => { - config.appsec.apiSecurity.requestSampling = 0 + config.appsec.apiSecurity.enabled = false + config.appsec.apiSecurity.sampleDelay = 10 appsec.enable(config) const res = await axios.post('/', { key: 'value' }) diff --git a/packages/dd-trace/test/appsec/index.sequelize.plugin.spec.js b/packages/dd-trace/test/appsec/index.sequelize.plugin.spec.js index d444b82ec5e..49442e361b2 100644 --- a/packages/dd-trace/test/appsec/index.sequelize.plugin.spec.js +++ b/packages/dd-trace/test/appsec/index.sequelize.plugin.spec.js @@ -21,7 +21,7 @@ describe('sequelize', () => { rules: path.join(__dirname, 'express-rules.json'), apiSecurity: { enabled: true, - requestSampling: 1 + sampleDelay: 10 } } })) diff --git a/packages/dd-trace/test/appsec/index.spec.js b/packages/dd-trace/test/appsec/index.spec.js index 4b8c6c0438c..5d79c5ae569 100644 --- a/packages/dd-trace/test/appsec/index.spec.js +++ b/packages/dd-trace/test/appsec/index.spec.js @@ -69,7 +69,7 @@ describe('AppSec Index', function () { }, apiSecurity: { enabled: false, - requestSampling: 0 + sampleDelay: 10 }, rasp: { enabled: true @@ -78,7 +78,11 @@ describe('AppSec Index', function () { } web = { - root: sinon.stub() + root: sinon.stub(), + getContext: sinon.stub(), + _prioritySampler: { + isSampled: sinon.stub() + } } blocking = { @@ -105,9 +109,10 @@ describe('AppSec Index', function () { disable: sinon.stub() } - apiSecuritySampler = require('../../src/appsec/api_security_sampler') + apiSecuritySampler = proxyquire('../../src/appsec/api_security_sampler', { + '../plugins/util/web': web + }) sinon.spy(apiSecuritySampler, 'sampleRequest') - sinon.spy(apiSecuritySampler, 'isSampled') rasp = { enable: sinon.stub(), @@ -472,47 +477,13 @@ describe('AppSec Index', function () { } web.root.returns(rootSpan) - }) - - it('should not trigger schema extraction with sampling disabled', () => { - config.appsec.apiSecurity = { - enabled: true, - requestSampling: 0 - } - - AppSec.enable(config) - - const req = { - url: '/path', - headers: { - 'user-agent': 'Arachni', - host: 'localhost', - cookie: 'a=1;b=2' - }, - method: 'POST', - socket: { - remoteAddress: '127.0.0.1', - remotePort: 8080 - } - } - const res = {} - - AppSec.incomingHttpStartTranslator({ req, res }) - - expect(waf.run).to.have.been.calledOnceWithExactly({ - persistent: { - 'server.request.uri.raw': '/path', - 'server.request.headers.no_cookies': { 'user-agent': 'Arachni', host: 'localhost' }, - 'server.request.method': 'POST', - 'http.client_ip': '127.0.0.1' - } - }, req) + web.getContext.returns({ paths: ['path'] }) }) it('should not trigger schema extraction with feature disabled', () => { config.appsec.apiSecurity = { enabled: false, - requestSampling: 1 + sampleDelay: 1 } AppSec.enable(config) @@ -528,18 +499,34 @@ describe('AppSec Index', function () { socket: { remoteAddress: '127.0.0.1', remotePort: 8080 + }, + body: { + a: '1' + }, + query: { + b: '2' + }, + route: { + path: '/path/:c' } } - const res = {} + const res = { + getHeaders: () => ({ + 'content-type': 'application/json', + 'content-lenght': 42 + }), + statusCode: 201 + } - AppSec.incomingHttpStartTranslator({ req, res }) + web.patch(req) + + sinon.stub(Reporter, 'finishRequest') + AppSec.incomingHttpEndTranslator({ req, res }) expect(waf.run).to.have.been.calledOnceWithExactly({ persistent: { - 'server.request.uri.raw': '/path', - 'server.request.headers.no_cookies': { 'user-agent': 'Arachni', host: 'localhost' }, - 'server.request.method': 'POST', - 'http.client_ip': '127.0.0.1' + 'server.request.body': { a: '1' }, + 'server.request.query': { b: '2' } } }, req) }) @@ -547,34 +534,52 @@ describe('AppSec Index', function () { it('should trigger schema extraction with sampling enabled', () => { config.appsec.apiSecurity = { enabled: true, - requestSampling: 1 + sampleDelay: 1 } AppSec.enable(config) const req = { - url: '/path', + route: { + path: '/path' + }, headers: { 'user-agent': 'Arachni', - host: 'localhost', - cookie: 'a=1;b=2' + host: 'localhost' }, method: 'POST', socket: { remoteAddress: '127.0.0.1', remotePort: 8080 + }, + body: { + a: '1' } } - const res = {} + const res = { + getHeaders: () => ({ + 'content-type': 'application/json', + 'content-lenght': 42 + }), + statusCode: 201 + } - AppSec.incomingHttpStartTranslator({ req, res }) + const span = { + context: sinon.stub().returns({ + _sampling: { + priority: 1 + } + }) + } + + web.root.returns(span) + web._prioritySampler.isSampled.returns(true) + + AppSec.incomingHttpEndTranslator({ req, res }) expect(waf.run).to.have.been.calledOnceWithExactly({ persistent: { - 'server.request.uri.raw': '/path', - 'server.request.headers.no_cookies': { 'user-agent': 'Arachni', host: 'localhost' }, - 'server.request.method': 'POST', - 'http.client_ip': '127.0.0.1', + 'server.request.body': { a: '1' }, 'waf.context.processor': { 'extract-schema': true } } }, req) @@ -584,8 +589,9 @@ describe('AppSec Index', function () { beforeEach(() => { config.appsec.apiSecurity = { enabled: true, - requestSampling: 1 + sampleDelay: 1 } + AppSec.enable(config) }) @@ -597,28 +603,30 @@ describe('AppSec Index', function () { responseBody.publish({ req: {}, body: 'string' }) responseBody.publish({ req: {}, body: null }) - expect(apiSecuritySampler.isSampled).to.not.been.called + expect(apiSecuritySampler.sampleRequest).to.not.been.called expect(waf.run).to.not.been.called }) it('should not call to the waf if it is not a sampled request', () => { - apiSecuritySampler.isSampled = apiSecuritySampler.isSampled.instantiateFake(() => false) + apiSecuritySampler.sampleRequest = apiSecuritySampler.sampleRequest.instantiateFake(() => false) const req = {} + const res = {} - responseBody.publish({ req, body: {} }) + responseBody.publish({ req, res, body: {} }) - expect(apiSecuritySampler.isSampled).to.have.been.calledOnceWith(req) + expect(apiSecuritySampler.sampleRequest).to.have.been.calledOnceWith(req, res) expect(waf.run).to.not.been.called }) it('should call to the waf if it is a sampled request', () => { - apiSecuritySampler.isSampled = apiSecuritySampler.isSampled.instantiateFake(() => true) + apiSecuritySampler.sampleRequest = apiSecuritySampler.sampleRequest.instantiateFake(() => true) const req = {} + const res = {} const body = {} - responseBody.publish({ req, body }) + responseBody.publish({ req, res, body }) - expect(apiSecuritySampler.isSampled).to.have.been.calledOnceWith(req) + expect(apiSecuritySampler.sampleRequest).to.have.been.calledOnceWith(req, res) expect(waf.run).to.been.calledOnceWith({ persistent: { [addresses.HTTP_INCOMING_RESPONSE_BODY]: body diff --git a/packages/dd-trace/test/appsec/remote_config/index.spec.js b/packages/dd-trace/test/appsec/remote_config/index.spec.js index b1804e0b646..67447cf7a69 100644 --- a/packages/dd-trace/test/appsec/remote_config/index.spec.js +++ b/packages/dd-trace/test/appsec/remote_config/index.spec.js @@ -9,7 +9,6 @@ let RemoteConfigManager let RuleManager let appsec let remoteConfig -let apiSecuritySampler describe('Remote Config index', () => { beforeEach(() => { @@ -33,11 +32,6 @@ describe('Remote Config index', () => { updateWafFromRC: sinon.stub() } - apiSecuritySampler = { - configure: sinon.stub(), - setRequestSampling: sinon.stub() - } - appsec = { enable: sinon.spy(), disable: sinon.spy() @@ -46,7 +40,6 @@ describe('Remote Config index', () => { remoteConfig = proxyquire('../src/appsec/remote_config', { './manager': RemoteConfigManager, '../rule_manager': RuleManager, - '../api_security_sampler': apiSecuritySampler, '..': appsec }) }) @@ -84,28 +77,6 @@ describe('Remote Config index', () => { expect(rc.setProductHandler).to.not.have.been.called }) - it('should listen ASM_API_SECURITY_SAMPLE_RATE when appsec.enabled=undefined and appSecurity.enabled=true', () => { - config.appsec = { enabled: undefined, apiSecurity: { enabled: true } } - - remoteConfig.enable(config) - - expect(RemoteConfigManager).to.have.been.calledOnceWithExactly(config) - expect(rc.updateCapabilities) - .to.have.been.calledWithExactly(RemoteConfigCapabilities.ASM_ACTIVATION, true) - expect(rc.updateCapabilities) - .to.have.been.calledWithExactly(RemoteConfigCapabilities.ASM_API_SECURITY_SAMPLE_RATE, true) - }) - - it('should listen ASM_API_SECURITY_SAMPLE_RATE when appsec.enabled=true and appSecurity.enabled=true', () => { - config.appsec = { enabled: true, apiSecurity: { enabled: true } } - - remoteConfig.enable(config) - - expect(RemoteConfigManager).to.have.been.calledOnceWithExactly(config) - expect(rc.updateCapabilities) - .to.have.been.calledWithExactly(RemoteConfigCapabilities.ASM_API_SECURITY_SAMPLE_RATE, true) - }) - describe('ASM_FEATURES remote config listener', () => { let listener @@ -142,106 +113,6 @@ describe('Remote Config index', () => { expect(appsec.disable).to.not.have.been.called }) }) - - describe('API Security Request Sampling', () => { - describe('OneClick', () => { - let listener - - beforeEach(() => { - config = { - appsec: { - enabled: undefined, - apiSecurity: { - requestSampling: 0.1 - } - } - } - - remoteConfig.enable(config) - - listener = rc.setProductHandler.firstCall.args[1] - }) - - it('should update apiSecuritySampler config', () => { - listener('apply', { - api_security: { - request_sample_rate: 0.5 - } - }) - - expect(apiSecuritySampler.setRequestSampling).to.be.calledOnceWithExactly(0.5) - }) - - it('should update apiSecuritySampler config and disable it', () => { - listener('apply', { - api_security: { - request_sample_rate: 0 - } - }) - - expect(apiSecuritySampler.setRequestSampling).to.be.calledOnceWithExactly(0) - }) - - it('should not update apiSecuritySampler config with values greater than 1', () => { - listener('apply', { - api_security: { - request_sample_rate: 5 - } - }) - - expect(apiSecuritySampler.configure).to.not.be.called - }) - - it('should not update apiSecuritySampler config with values less than 0', () => { - listener('apply', { - api_security: { - request_sample_rate: -0.4 - } - }) - - expect(apiSecuritySampler.configure).to.not.be.called - }) - - it('should not update apiSecuritySampler config with incorrect values', () => { - listener('apply', { - api_security: { - request_sample_rate: 'not_a_number' - } - }) - - expect(apiSecuritySampler.configure).to.not.be.called - }) - }) - - describe('Enabled', () => { - let listener - - beforeEach(() => { - config = { - appsec: { - enabled: true, - apiSecurity: { - requestSampling: 0.1 - } - } - } - - remoteConfig.enable(config) - - listener = rc.setProductHandler.firstCall.args[1] - }) - - it('should update config apiSecurity.requestSampling property value', () => { - listener('apply', { - api_security: { - request_sample_rate: 0.5 - } - }) - - expect(apiSecuritySampler.setRequestSampling).to.be.calledOnceWithExactly(0.5) - }) - }) - }) }) describe('enableWafUpdate', () => { diff --git a/packages/dd-trace/test/appsec/response_blocking.spec.js b/packages/dd-trace/test/appsec/response_blocking.spec.js index 03541858955..5ccd250eea2 100644 --- a/packages/dd-trace/test/appsec/response_blocking.spec.js +++ b/packages/dd-trace/test/appsec/response_blocking.spec.js @@ -55,6 +55,9 @@ describe('HTTP Response Blocking', () => { rules: path.join(__dirname, 'response_blocking_rules.json'), rasp: { enabled: false // disable rasp to not trigger waf.run executions due to lfi + }, + apiSecurity: { + enabled: false } } })) diff --git a/packages/dd-trace/test/config.spec.js b/packages/dd-trace/test/config.spec.js index 001ff8acf27..f840dcd4a13 100644 --- a/packages/dd-trace/test/config.spec.js +++ b/packages/dd-trace/test/config.spec.js @@ -254,7 +254,7 @@ describe('Config', () => { expect(config).to.have.nested.property('appsec.eventTracking.enabled', true) expect(config).to.have.nested.property('appsec.eventTracking.mode', 'safe') expect(config).to.have.nested.property('appsec.apiSecurity.enabled', true) - expect(config).to.have.nested.property('appsec.apiSecurity.requestSampling', 0.1) + expect(config).to.have.nested.property('appsec.apiSecurity.sampleDelay', 30) expect(config).to.have.nested.property('appsec.sca.enabled', null) expect(config).to.have.nested.property('appsec.standalone.enabled', undefined) expect(config).to.have.nested.property('remoteConfig.enabled', true) @@ -504,7 +504,7 @@ describe('Config', () => { process.env.DD_PROFILING_ENABLED = 'true' process.env.DD_INJECTION_ENABLED = 'profiler' process.env.DD_API_SECURITY_ENABLED = 'true' - process.env.DD_API_SECURITY_REQUEST_SAMPLE_RATE = 1 + process.env.DD_API_SECURITY_SAMPLE_DELAY = '25' process.env.DD_INSTRUMENTATION_INSTALL_ID = '68e75c48-57ca-4a12-adfc-575c4b05fcbe' process.env.DD_INSTRUMENTATION_INSTALL_TYPE = 'k8s_single_step' process.env.DD_INSTRUMENTATION_INSTALL_TIME = '1703188212' @@ -594,7 +594,7 @@ describe('Config', () => { expect(config).to.have.nested.property('appsec.eventTracking.enabled', true) expect(config).to.have.nested.property('appsec.eventTracking.mode', 'extended') expect(config).to.have.nested.property('appsec.apiSecurity.enabled', true) - expect(config).to.have.nested.property('appsec.apiSecurity.requestSampling', 1) + expect(config).to.have.nested.property('appsec.apiSecurity.sampleDelay', 25) expect(config).to.have.nested.property('appsec.sca.enabled', true) expect(config).to.have.nested.property('appsec.standalone.enabled', true) expect(config).to.have.nested.property('remoteConfig.enabled', false) @@ -1175,7 +1175,6 @@ describe('Config', () => { process.env.DD_APPSEC_GRAPHQL_BLOCKED_TEMPLATE_JSON = BLOCKED_TEMPLATE_JSON_PATH // json and html here process.env.DD_APPSEC_AUTOMATED_USER_EVENTS_TRACKING = 'disabled' process.env.DD_API_SECURITY_ENABLED = 'false' - process.env.DD_API_SECURITY_REQUEST_SAMPLE_RATE = 0.5 process.env.DD_REMOTE_CONFIG_POLL_INTERVAL_SECONDS = 11 process.env.DD_IAST_ENABLED = 'false' process.env.DD_IAST_COOKIE_FILTER_PATTERN = '.*' @@ -1241,8 +1240,7 @@ describe('Config', () => { mode: 'safe' }, apiSecurity: { - enabled: true, - requestSampling: 1.0 + enabled: true }, rasp: { enabled: false @@ -1320,7 +1318,6 @@ describe('Config', () => { expect(config).to.have.nested.property('appsec.eventTracking.enabled', true) expect(config).to.have.nested.property('appsec.eventTracking.mode', 'safe') expect(config).to.have.nested.property('appsec.apiSecurity.enabled', true) - expect(config).to.have.nested.property('appsec.apiSecurity.requestSampling', 1.0) expect(config).to.have.nested.property('remoteConfig.pollInterval', 42) expect(config).to.have.nested.property('iast.enabled', true) expect(config).to.have.nested.property('iast.requestSampling', 30) @@ -1351,8 +1348,7 @@ describe('Config', () => { mode: 'disabled' }, apiSecurity: { - enabled: true, - requestSampling: 1.0 + enabled: true }, rasp: { enabled: false @@ -1385,8 +1381,7 @@ describe('Config', () => { mode: 'safe' }, apiSecurity: { - enabled: false, - requestSampling: 0.5 + enabled: false }, rasp: { enabled: true @@ -1423,7 +1418,7 @@ describe('Config', () => { }, apiSecurity: { enabled: true, - requestSampling: 1.0 + sampleDelay: 30 }, sca: { enabled: null @@ -2180,35 +2175,6 @@ describe('Config', () => { }) }) - it('should sanitize values for API Security sampling between 0 and 1', () => { - expect(new Config({ - appsec: { - apiSecurity: { - enabled: true, - requestSampling: 5 - } - } - })).to.have.nested.property('appsec.apiSecurity.requestSampling', 1) - - expect(new Config({ - appsec: { - apiSecurity: { - enabled: true, - requestSampling: -5 - } - } - })).to.have.nested.property('appsec.apiSecurity.requestSampling', 0) - - expect(new Config({ - appsec: { - apiSecurity: { - enabled: true, - requestSampling: 0.1 - } - } - })).to.have.nested.property('appsec.apiSecurity.requestSampling', 0.1) - }) - context('payload tagging', () => { let env diff --git a/yarn.lock b/yarn.lock index f868a0cf0b3..01a1d7181ee 100644 --- a/yarn.lock +++ b/yarn.lock @@ -659,6 +659,11 @@ resolve-from "^3.0.0" rimraf "^3.0.0" +"@isaacs/ttlcache@^1.4.1": + version "1.4.1" + resolved "https://registry.yarnpkg.com/@isaacs/ttlcache/-/ttlcache-1.4.1.tgz#21fb23db34e9b6220c6ba023a0118a2dd3461ea2" + integrity sha512-RQgQ4uQ+pLbqXfOmieB91ejmLwvSgv9nLx6sT6sD83s7umBypgg+OIBOBbEUiJXrfpnp9j0mRhYYdzp9uqq3lA== + "@istanbuljs/load-nyc-config@^1.0.0": version "1.1.0" resolved "https://registry.npmjs.org/@istanbuljs/load-nyc-config/-/load-nyc-config-1.1.0.tgz"