Skip to content

Commit 39193ca

Browse files
pichlermarcpichlermarc
authored
fix(redis-common): expand redaction to include ACL, CONFIG, PSETEX, GETSET (#3472)
1 parent 6d0205c commit 39193ca

5 files changed

Lines changed: 292 additions & 2 deletions

File tree

‎packages/instrumentation-ioredis/test/ioredis.test.ts‎

Lines changed: 96 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -663,6 +663,102 @@ describe('ioredis', () => {
663663
});
664664
});
665665
});
666+
667+
describe('sensitive command sanitization', function () {
668+
after(async () => {
669+
// cleanup added user
670+
client.acl('DELUSER', 'testuser');
671+
});
672+
673+
it('should redact CONFIG SET arguments in db.statement', async function () {
674+
const span = provider
675+
.getTracer('ioredis-test')
676+
.startSpan('test span');
677+
await context.with(
678+
trace.setSpan(context.active(), span),
679+
async function () {
680+
await client.config('SET', 'hz', '15');
681+
span.end();
682+
const endedSpans = memoryExporter.getFinishedSpans();
683+
assert.strictEqual(
684+
endedSpans[0].attributes[ATTR_DB_STATEMENT],
685+
'config SET [2 other arguments]'
686+
);
687+
assert.strictEqual(
688+
endedSpans[0].attributes[ATTR_DB_QUERY_TEXT],
689+
'config SET [2 other arguments]'
690+
);
691+
}
692+
);
693+
});
694+
695+
it('should redact ACL SETUSER arguments in db.statement', async function () {
696+
const span = provider
697+
.getTracer('ioredis-test')
698+
.startSpan('test span');
699+
await context.with(
700+
trace.setSpan(context.active(), span),
701+
async function () {
702+
await (client as any).acl('setuser', 'testuser');
703+
span.end();
704+
const endedSpans = memoryExporter.getFinishedSpans();
705+
assert.strictEqual(
706+
endedSpans[0].attributes[ATTR_DB_STATEMENT],
707+
'acl setuser [1 other arguments]'
708+
);
709+
assert.strictEqual(
710+
endedSpans[0].attributes[ATTR_DB_QUERY_TEXT],
711+
'acl setuser [1 other arguments]'
712+
);
713+
}
714+
);
715+
await (client as any).acl('deluser', 'testuser'); // cleanup
716+
});
717+
718+
it('should redact GETSET value in db.statement', async function () {
719+
const span = provider
720+
.getTracer('ioredis-test')
721+
.startSpan('test span');
722+
await context.with(
723+
trace.setSpan(context.active(), span),
724+
async function () {
725+
await client.getset(testKeyName, 'secret-value');
726+
span.end();
727+
const endedSpans = memoryExporter.getFinishedSpans();
728+
assert.strictEqual(
729+
endedSpans[0].attributes[ATTR_DB_STATEMENT],
730+
`getset ${testKeyName} [1 other arguments]`
731+
);
732+
assert.strictEqual(
733+
endedSpans[0].attributes[ATTR_DB_QUERY_TEXT],
734+
`getset ${testKeyName} [1 other arguments]`
735+
);
736+
}
737+
);
738+
});
739+
740+
it('should redact PSETEX value in db.statement', async function () {
741+
const span = provider
742+
.getTracer('ioredis-test')
743+
.startSpan('test span');
744+
await context.with(
745+
trace.setSpan(context.active(), span),
746+
async function () {
747+
await client.psetex(testKeyName, 60000, 'secret-value');
748+
span.end();
749+
const endedSpans = memoryExporter.getFinishedSpans();
750+
assert.strictEqual(
751+
endedSpans[0].attributes[ATTR_DB_STATEMENT],
752+
`psetex ${testKeyName} [2 other arguments]`
753+
);
754+
assert.strictEqual(
755+
endedSpans[0].attributes[ATTR_DB_QUERY_TEXT],
756+
`psetex ${testKeyName} [2 other arguments]`
757+
);
758+
}
759+
);
760+
});
761+
});
666762
});
667763

668764
describe('Instrumenting without parent span', () => {

‎packages/instrumentation-redis/test/v2-v3/redis.test.ts‎

Lines changed: 74 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -335,6 +335,80 @@ describe('redis v2-v3', () => {
335335
});
336336
});
337337

338+
describe('sensitive command sanitization', () => {
339+
it('should redact CONFIG SET arguments in db.statement', function (done) {
340+
const span = tracer.startSpan('test span');
341+
context.with(trace.setSpan(context.active(), span), () => {
342+
client.config('set', 'hz', '15', (err: unknown) => {
343+
try {
344+
assert.ifError(err);
345+
span.end();
346+
const endedSpans = testUtils.getTestSpans();
347+
assert.strictEqual(
348+
endedSpans[0].attributes[ATTR_DB_STATEMENT],
349+
'config set [2 other arguments]'
350+
);
351+
assert.strictEqual(
352+
endedSpans[0].attributes[ATTR_DB_QUERY_TEXT],
353+
'config set [2 other arguments]'
354+
);
355+
done();
356+
} catch (error) {
357+
done(error);
358+
}
359+
});
360+
});
361+
});
362+
363+
it('should redact GETSET value in db.statement', function (done) {
364+
const span = tracer.startSpan('test span');
365+
context.with(trace.setSpan(context.active(), span), () => {
366+
client.getset('test', 'secret-value', (err: unknown) => {
367+
try {
368+
assert.ifError(err);
369+
span.end();
370+
const endedSpans = testUtils.getTestSpans();
371+
assert.strictEqual(
372+
endedSpans[0].attributes[ATTR_DB_STATEMENT],
373+
'getset test [1 other arguments]'
374+
);
375+
assert.strictEqual(
376+
endedSpans[0].attributes[ATTR_DB_QUERY_TEXT],
377+
'getset test [1 other arguments]'
378+
);
379+
done();
380+
} catch (err) {
381+
done(err);
382+
}
383+
});
384+
});
385+
});
386+
387+
it('should redact PSETEX value in db.statement', function (done) {
388+
const span = tracer.startSpan('test span');
389+
context.with(trace.setSpan(context.active(), span), () => {
390+
client.psetex('test', 60000, 'secret-value', (err: unknown) => {
391+
try {
392+
assert.ifError(err);
393+
span.end();
394+
const endedSpans = testUtils.getTestSpans();
395+
assert.strictEqual(
396+
endedSpans[0].attributes[ATTR_DB_STATEMENT],
397+
'psetex test [2 other arguments]'
398+
);
399+
assert.strictEqual(
400+
endedSpans[0].attributes[ATTR_DB_QUERY_TEXT],
401+
'psetex test [2 other arguments]'
402+
);
403+
done();
404+
} catch (err) {
405+
done(err);
406+
}
407+
});
408+
});
409+
});
410+
});
411+
338412
describe('dbStatementSerializer config', () => {
339413
const dbStatementSerializer = (
340414
cmdName: string,

‎packages/instrumentation-redis/test/v4-v5/redis.test.ts‎

Lines changed: 66 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -215,6 +215,72 @@ describe('redis v4-v5', () => {
215215
'ERR value is not an integer or out of range'
216216
);
217217
});
218+
219+
describe('sensitive command sanitization', function () {
220+
it('redacts CONFIG SET arguments in db.statement', async function () {
221+
await client.sendCommand(['CONFIG', 'SET', 'hz', '15']);
222+
const [span] = getTestSpans();
223+
assert.strictEqual(
224+
span?.attributes[ATTR_DB_STATEMENT],
225+
'CONFIG SET [2 other arguments]'
226+
);
227+
assert.strictEqual(
228+
span?.attributes[ATTR_DB_QUERY_TEXT],
229+
'CONFIG SET [2 other arguments]'
230+
);
231+
});
232+
233+
it('redacts ACL SETUSER arguments in db.statement', async function () {
234+
await client.sendCommand(['ACL', 'SETUSER', 'testuser']);
235+
const [span] = getTestSpans();
236+
assert.strictEqual(
237+
span?.attributes[ATTR_DB_STATEMENT],
238+
'ACL SETUSER [1 other arguments]'
239+
);
240+
assert.strictEqual(
241+
span?.attributes[ATTR_DB_QUERY_TEXT],
242+
'ACL SETUSER [1 other arguments]'
243+
);
244+
await context.with(
245+
suppressTracing(context.active()),
246+
async function () {
247+
await client.sendCommand(['ACL', 'DELUSER', 'testuser']);
248+
}
249+
);
250+
});
251+
252+
it('redacts GETSET value in db.statement', async function () {
253+
await context.with(
254+
suppressTracing(context.active()),
255+
async function () {
256+
await client.set('key', 'initial');
257+
}
258+
);
259+
await client.sendCommand(['GETSET', 'key', 'secret-value']);
260+
const [span] = getTestSpans();
261+
assert.strictEqual(
262+
span?.attributes[ATTR_DB_STATEMENT],
263+
'GETSET key [1 other arguments]'
264+
);
265+
assert.strictEqual(
266+
span?.attributes[ATTR_DB_QUERY_TEXT],
267+
'GETSET key [1 other arguments]'
268+
);
269+
});
270+
271+
it('redacts PSETEX value in db.statement', async function () {
272+
await client.sendCommand(['PSETEX', 'key', '60000', 'secret-value']);
273+
const [span] = getTestSpans();
274+
assert.strictEqual(
275+
span?.attributes[ATTR_DB_STATEMENT],
276+
'PSETEX key [2 other arguments]'
277+
);
278+
assert.strictEqual(
279+
span?.attributes[ATTR_DB_QUERY_TEXT],
280+
'PSETEX key [2 other arguments]'
281+
);
282+
});
283+
});
218284
});
219285

220286
describe('client connect', () => {

‎packages/redis-common/src/index.ts‎

Lines changed: 9 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -28,16 +28,23 @@ const serializationSubsets = [
2828
args: 0,
2929
},
3030
{
31-
regex: /^(LPUSH|MSET|PFA|PUBLISH|RPUSH|SADD|SET|SPUBLISH|XADD|ZADD)/i,
31+
regex:
32+
/^(GETSET|LPUSH|MSET|PFA|PSETEX|PUBLISH|RPUSH|SADD|SET|SPUBLISH|XADD|ZADD)/i,
3233
args: 1,
3334
},
3435
{
3536
regex: /^(HSET|HMSET|LSET|LINSERT)/i,
3637
args: 2,
3738
},
39+
// ACL and CONFIG subcommands may contain sensitive data (e.g. passwords),
40+
// so only serialize the subcommand name (first argument).
41+
{
42+
regex: /^(ACL|CONFIG)/i,
43+
args: 1,
44+
},
3845
{
3946
regex:
40-
/^(ACL|BIT|B[LRZ]|CLIENT|CLUSTER|CONFIG|COMMAND|DECR|DEL|EVAL|EX|FUNCTION|GEO|GET|HINCR|HMGET|HSCAN|INCR|L[TRLM]|MEMORY|P[EFISTU]|RPOP|S[CDIMORSU]|XACK|X[CDGILPRT]|Z[CDILMPRS])/i,
47+
/^(BIT|B[LRZ]|CLIENT|CLUSTER|COMMAND|DECR|DEL|EVAL|EX|FUNCTION|GEO|GET|HINCR|HMGET|HSCAN|INCR|L[TRLM]|MEMORY|P[EFISTU]|RPOP|S[CDIMORSU]|XACK|X[CDGILPRT]|Z[CDILMPRS])/i,
4148
args: -1,
4249
},
4350
];

‎packages/redis-common/test/redis-common.test.ts‎

Lines changed: 47 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -43,6 +43,53 @@ describe('#defaultDbStatementSerializer()', () => {
4343
cmdArgs: ['key', 5],
4444
expected: 'INCRBY key 5',
4545
},
46+
// ACL subcommands with sensitive data should be redacted
47+
{
48+
cmdName: 'ACL',
49+
cmdArgs: [
50+
'SETUSER',
51+
'alice',
52+
'on',
53+
'>MySecretPass',
54+
'~user:alice:*',
55+
'+@read',
56+
'+@write',
57+
],
58+
expected: 'ACL SETUSER [6 other arguments]',
59+
},
60+
{
61+
cmdName: 'ACL',
62+
cmdArgs: ['WHOAMI'],
63+
expected: 'ACL WHOAMI',
64+
},
65+
{
66+
cmdName: 'ACL',
67+
cmdArgs: ['LIST'],
68+
expected: 'ACL LIST',
69+
},
70+
// CONFIG subcommands with sensitive data should be redacted
71+
{
72+
cmdName: 'CONFIG',
73+
cmdArgs: ['SET', 'requirepass', 'MyNewPassword123'],
74+
expected: 'CONFIG SET [2 other arguments]',
75+
},
76+
{
77+
cmdName: 'CONFIG',
78+
cmdArgs: ['GET', 'maxmemory'],
79+
expected: 'CONFIG GET [1 other arguments]',
80+
},
81+
// GETSET (deprecated) args should be redacted since it can contain sensitive data
82+
{
83+
cmdName: 'GETSET',
84+
cmdArgs: ['key', 'secret_value'],
85+
expected: 'GETSET key [1 other arguments]',
86+
},
87+
// PSETEX (deprecated) can also contain sensitive data
88+
{
89+
cmdName: 'PSETEX',
90+
cmdArgs: ['key', '100000', 'secret_value'],
91+
expected: 'PSETEX key [2 other arguments]',
92+
},
4693
].forEach(({ cmdName, cmdArgs, expected }) => {
4794
it(`should serialize the correct number of arguments for ${cmdName}`, () => {
4895
assert.strictEqual(

0 commit comments

Comments
 (0)