Skip to content

Commit b36ce05

Browse files
watsonwatson
authored
[DI] Implement PII redaction (#5053)
The algorithm will look for: - names of variables - names of object properties - names of keys in maps The names will be matched against a disallow-list and if a match is found, its value will be redacted. The list is hardcoded and can be found here: packages/dd-trace/src/debugger/devtools_client/snapshot/redaction.js It's possible to add names to the list using the following environment variable: DD_DYNAMIC_INSTRUMENTATION_REDACTED_IDENTIFIERS Or it's possible to remove names from the list using the following environment variable: DD_DYNAMIC_INSTRUMENTATION_REDACTION_EXCLUDED_IDENTIFIERS Each environment variable takes a list of names separated by commas. Support for redacting instances of specific classes is not included in this commit.
1 parent e2bee27 commit b36ce05

14 files changed

Lines changed: 449 additions & 24 deletions

File tree

‎integration-tests/debugger/redact.spec.js‎

Lines changed: 49 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,49 @@
1+
'use strict'
2+
3+
const { assert } = require('chai')
4+
const { setup } = require('./utils')
5+
6+
// Default settings is tested in unit tests, so we only need to test the env vars here
7+
describe('Dynamic Instrumentation snapshot PII redaction', function () {
8+
describe('DD_DYNAMIC_INSTRUMENTATION_REDACTED_IDENTIFIERS=foo,bar', function () {
9+
const t = setup({ env: { DD_DYNAMIC_INSTRUMENTATION_REDACTED_IDENTIFIERS: 'foo,bar' } })
10+
11+
it('should respect DD_DYNAMIC_INSTRUMENTATION_REDACTED_IDENTIFIERS', function (done) {
12+
t.triggerBreakpoint()
13+
14+
t.agent.on('debugger-input', ({ payload: [{ 'debugger.snapshot': { captures } }] }) => {
15+
const { locals } = captures.lines[t.breakpoint.line]
16+
17+
assert.deepPropertyVal(locals, 'foo', { type: 'string', notCapturedReason: 'redactedIdent' })
18+
assert.deepPropertyVal(locals, 'bar', { type: 'string', notCapturedReason: 'redactedIdent' })
19+
assert.deepPropertyVal(locals, 'baz', { type: 'string', value: 'c' })
20+
21+
// existing redaction should not be impacted
22+
assert.deepPropertyVal(locals, 'secret', { type: 'string', notCapturedReason: 'redactedIdent' })
23+
24+
done()
25+
})
26+
27+
t.agent.addRemoteConfig(t.generateRemoteConfig({ captureSnapshot: true }))
28+
})
29+
})
30+
31+
describe('DD_DYNAMIC_INSTRUMENTATION_REDACTION_EXCLUDED_IDENTIFIERS=secret', function () {
32+
const t = setup({ env: { DD_DYNAMIC_INSTRUMENTATION_REDACTION_EXCLUDED_IDENTIFIERS: 'secret' } })
33+
34+
it('should respect DD_DYNAMIC_INSTRUMENTATION_REDACTED_IDENTIFIERS', function (done) {
35+
t.triggerBreakpoint()
36+
37+
t.agent.on('debugger-input', ({ payload: [{ 'debugger.snapshot': { captures } }] }) => {
38+
const { locals } = captures.lines[t.breakpoint.line]
39+
40+
assert.deepPropertyVal(locals, 'secret', { type: 'string', value: 'shh!' })
41+
assert.deepPropertyVal(locals, 'password', { type: 'string', notCapturedReason: 'redactedIdent' })
42+
43+
done()
44+
})
45+
46+
t.agent.addRemoteConfig(t.generateRemoteConfig({ captureSnapshot: true }))
47+
})
48+
})
49+
})

‎integration-tests/debugger/target-app/redact.js‎

Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,26 @@
1+
'use strict'
2+
3+
require('dd-trace/init')
4+
const Fastify = require('fastify')
5+
6+
const fastify = Fastify()
7+
8+
fastify.get('/', function () {
9+
/* eslint-disable no-unused-vars */
10+
const foo = 'a'
11+
const bar = 'b'
12+
const baz = 'c'
13+
const secret = 'shh!'
14+
const password = 'shh!'
15+
/* eslint-enable no-unused-vars */
16+
17+
return { hello: 'world' } // BREAKPOINT: /
18+
})
19+
20+
fastify.listen({ port: process.env.APP_PORT }, (err) => {
21+
if (err) {
22+
fastify.log.error(err)
23+
process.exit(1)
24+
}
25+
process.send({ port: process.env.APP_PORT })
26+
})

‎packages/dd-trace/src/ci-visibility/dynamic-instrumentation/index.js‎

Lines changed: 21 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
11
'use strict'
22

33
const { join } = require('path')
4-
const { Worker } = require('worker_threads')
4+
const { Worker, threadId: parentThreadId } = require('worker_threads')
55
const { randomUUID } = require('crypto')
66
const log = require('../../log')
77

@@ -46,29 +46,47 @@ class TestVisDynamicInstrumentation {
4646
return this._readyPromise
4747
}
4848

49-
start () {
49+
start (config) {
5050
if (this.worker) return
5151

5252
const { NODE_OPTIONS, ...envWithoutNodeOptions } = process.env
5353

5454
log.debug('Starting Test Visibility - Dynamic Instrumentation client...')
5555

56+
const rcChannel = new MessageChannel() // mock channel
57+
const configChannel = new MessageChannel() // mock channel
58+
5659
this.worker = new Worker(
5760
join(__dirname, 'worker', 'index.js'),
5861
{
5962
execArgv: [],
6063
env: envWithoutNodeOptions,
6164
workerData: {
65+
config: config.serialize(),
66+
parentThreadId,
67+
rcPort: rcChannel.port1,
68+
configPort: configChannel.port1,
6269
breakpointSetChannel: this.breakpointSetChannel.port1,
6370
breakpointHitChannel: this.breakpointHitChannel.port1
6471
},
65-
transferList: [this.breakpointSetChannel.port1, this.breakpointHitChannel.port1]
72+
transferList: [
73+
rcChannel.port1,
74+
configChannel.port1,
75+
this.breakpointSetChannel.port1,
76+
this.breakpointHitChannel.port1
77+
]
6678
}
6779
)
6880
this.worker.on('online', () => {
6981
log.debug('Test Visibility - Dynamic Instrumentation client is ready')
7082
this._onReady()
7183
})
84+
this.worker.on('error', (err) => {
85+
log.error('Test Visibility - Dynamic Instrumentation worker error', err)
86+
})
87+
this.worker.on('messageerror', (err) => {
88+
log.error('Test Visibility - Dynamic Instrumentation worker messageerror', err)
89+
})
7290

7391
// Allow the parent to exit even if the worker is still running
7492
this.worker.unref()

‎packages/dd-trace/src/config.js‎

Lines changed: 36 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -473,6 +473,8 @@ class Config {
473473
this._setValue(defaults, 'dogstatsd.port', '8125')
474474
this._setValue(defaults, 'dsmEnabled', false)
475475
this._setValue(defaults, 'dynamicInstrumentationEnabled', false)
476+
this._setValue(defaults, 'dynamicInstrumentationRedactedIdentifiers', [])
477+
this._setValue(defaults, 'dynamicInstrumentationRedactionExcludedIdentifiers', [])
476478
this._setValue(defaults, 'env', undefined)
477479
this._setValue(defaults, 'experimental.enableGetRumData', false)
478480
this._setValue(defaults, 'experimental.exporter', undefined)
@@ -600,6 +602,8 @@ class Config {
600602
DD_DOGSTATSD_HOST,
601603
DD_DOGSTATSD_PORT,
602604
DD_DYNAMIC_INSTRUMENTATION_ENABLED,
605+
DD_DYNAMIC_INSTRUMENTATION_REDACTED_IDENTIFIERS,
606+
DD_DYNAMIC_INSTRUMENTATION_REDACTION_EXCLUDED_IDENTIFIERS,
603607
DD_ENV,
604608
DD_EXPERIMENTAL_API_SECURITY_ENABLED,
605609
DD_EXPERIMENTAL_APPSEC_STANDALONE_ENABLED,
@@ -747,6 +751,12 @@ class Config {
747751
this._setString(env, 'dogstatsd.port', DD_DOGSTATSD_PORT)
748752
this._setBoolean(env, 'dsmEnabled', DD_DATA_STREAMS_ENABLED)
749753
this._setBoolean(env, 'dynamicInstrumentationEnabled', DD_DYNAMIC_INSTRUMENTATION_ENABLED)
754+
this._setArray(env, 'dynamicInstrumentationRedactedIdentifiers', DD_DYNAMIC_INSTRUMENTATION_REDACTED_IDENTIFIERS)
755+
this._setArray(
756+
env,
757+
'dynamicInstrumentationRedactionExcludedIdentifiers',
758+
DD_DYNAMIC_INSTRUMENTATION_REDACTION_EXCLUDED_IDENTIFIERS
759+
)
750760
this._setString(env, 'env', DD_ENV || tags.env)
751761
this._setBoolean(env, 'traceEnabled', DD_TRACE_ENABLED)
752762
this._setBoolean(env, 'experimental.enableGetRumData', DD_TRACE_EXPERIMENTAL_GET_RUM_DATA_ENABLED)
@@ -927,6 +937,16 @@ class Config {
927937
}
928938
this._setBoolean(opts, 'dsmEnabled', options.dsmEnabled)
929939
this._setBoolean(opts, 'dynamicInstrumentationEnabled', options.experimental?.dynamicInstrumentationEnabled)
940+
this._setArray(
941+
opts,
942+
'dynamicInstrumentationRedactedIdentifiers',
943+
options.experimental?.dynamicInstrumentationRedactedIdentifiers
944+
)
945+
this._setArray(
946+
opts,
947+
'dynamicInstrumentationRedactionExcludedIdentifiers',
948+
options.experimental?.dynamicInstrumentationRedactionExcludedIdentifiers
949+
)
930950
this._setString(opts, 'env', options.env || tags.env)
931951
this._setBoolean(opts, 'experimental.enableGetRumData', options.experimental?.enableGetRumData)
932952
this._setString(opts, 'experimental.exporter', options.experimental?.exporter)
@@ -1312,6 +1332,22 @@ class Config {
13121332
this.sampler.sampleRate = this.sampleRate
13131333
updateConfig(changes, this)
13141334
}
1335+
1336+
// TODO: Refactor the Config class so it never produces any config objects that are incompatible with MessageChannel
1337+
/**
1338+
* Serializes the config object so it can be passed over a Worker Thread MessageChannel.
1339+
* @returns {Object} The serialized config object.
1340+
*/
1341+
serialize () {
1342+
// URL objects cannot be serialized over the MessageChannel, so we need to convert them to strings first
1343+
if (this.url instanceof URL) {
1344+
const config = { ...this }
1345+
config.url = this.url.toString()
1346+
return config
1347+
}
1348+
1349+
return this
1350+
}
13151351
}
13161352

13171353
function maybeInt (number) {

‎packages/dd-trace/src/debugger/devtools_client/config.js‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,8 @@ const { format } = require('node:url')
55
const log = require('../../log')
66

77
const config = module.exports = {
8+
dynamicInstrumentationRedactedIdentifiers: parentConfig.dynamicInstrumentationRedactedIdentifiers,
9+
dynamicInstrumentationRedactionExcludedIdentifiers: parentConfig.dynamicInstrumentationRedactionExcludedIdentifiers,
810
runtimeId: parentConfig.tags['runtime-id'],
911
service: parentConfig.service,
1012
commitSHA: parentConfig.commitSHA,

‎packages/dd-trace/src/debugger/devtools_client/snapshot/processor.js‎

Lines changed: 35 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
'use strict'
22

33
const { collectionSizeSym, fieldCountSym } = require('./symbols')
4+
const { normalizeName, REDACTED_IDENTIFIERS } = require('./redaction')
45

56
module.exports = {
67
processRawState: processProperties
@@ -24,7 +25,14 @@ function processProperties (props, maxLength) {
2425
return result
2526
}
2627

28+
// TODO: Improve performance of redaction algorithm.
29+
// This algorithm is probably slower than if we embedded the redaction logic inside the functions below.
30+
// That way we didn't have to traverse objects that will just be redacted anyway.
2731
function getPropertyValue (prop, maxLength) {
32+
return redact(prop, getPropertyValueRaw(prop, maxLength))
33+
}
34+
35+
function getPropertyValueRaw (prop, maxLength) {
2836
// Special case for getters and setters which does not have a value property
2937
if ('get' in prop) {
3038
const hasGet = prop.get.type !== 'undefined'
@@ -185,8 +193,11 @@ function toMap (type, pairs, maxLength) {
185193
// `pair.value` is a special wrapper-object with subtype `internal#entry`. This can be skipped and we can go
186194
// directly to its children, of which there will always be exactly two, the first containing the key, and the
187195
// second containing the value of this entry of the Map.
196+
const shouldRedact = shouldRedactMapValue(pair.value.properties[0])
188197
const key = getPropertyValue(pair.value.properties[0], maxLength)
189-
const val = getPropertyValue(pair.value.properties[1], maxLength)
198+
const val = shouldRedact
199+
? notCapturedRedacted(pair.value.properties[1].value.type)
200+
: getPropertyValue(pair.value.properties[1], maxLength)
190201
result.entries[i++] = [key, val]
191202
}
192203

@@ -240,6 +251,25 @@ function arrayBufferToString (bytes, size) {
240251
return buf.toString()
241252
}
242253

254+
function redact (prop, obj) {
255+
const name = getNormalizedNameFromProp(prop)
256+
return REDACTED_IDENTIFIERS.has(name) ? notCapturedRedacted(obj.type) : obj
257+
}
258+
259+
function shouldRedactMapValue (key) {
260+
const isSymbol = key.value.type === 'symbol'
261+
if (!isSymbol && key.value.type !== 'string') return false // WeakMaps uses objects as keys
262+
const name = normalizeName(
263+
isSymbol ? key.value.description : key.value.value,
264+
isSymbol
265+
)
266+
return REDACTED_IDENTIFIERS.has(name)
267+
}
268+
269+
function getNormalizedNameFromProp (prop) {
270+
return normalizeName(prop.name, 'symbol' in prop)
271+
}
272+
243273
function setNotCaptureReasonOnCollection (result, collection) {
244274
if (collectionSizeSym in collection) {
245275
result.notCapturedReason = 'collectionSize'
@@ -250,3 +280,7 @@ function setNotCaptureReasonOnCollection (result, collection) {
250280
function notCapturedDepth (type) {
251281
return { type, notCapturedReason: 'depth' }
252282
}
283+
284+
function notCapturedRedacted (type) {
285+
return { type, notCapturedReason: 'redactedIdent' }
286+
}

0 commit comments

Comments
 (0)