fix(instrumentation-aws-sdk): remove un-sanitised db.statement span attribute from DynamoDB spans #1748

ramesius · 2023-10-20T10:40:31Z

Which problem is this PR solving?

DynamoDB spans include the db.statement span attribute which can include un-sanitised sensitive data.

Short description of the changes

Remove the db.statement attribute from DynamoDB spans due to not being sanitised of sensitive data.

According to the semantic conventions spec:

(db.statement) Should be collected by default only if there is sanitisation that excludes sensitive information.

I have updated the tests to explicitly assert that db.statement remains undefined.

Contributes to #1552

linux-foundation-easycla · 2023-10-20T10:40:34Z

The committers listed above are authorized under a signed CLA.

✅ login: ramesius / name: Matthew Turner (a5acebf, 52d09b7, c198332, a37da4a, 55ce8d5, 648c034, 449d159)
✅ login: blumamir / name: Amir Blum (a61251c)

andymac4182 · 2023-10-23T06:04:18Z

@carolabadeer @blumamir Are you able to review please? This is impacting on our services. It is pushing private database values to traces.

srprash · 2023-10-30T03:25:57Z

It is indeed not a good practice to record raw query as db.statement span attribute, and thanks for raising this PR.
I propose that we follow a similar approach as the redis instrumentation, where a custom function can be provided to serialize the db.statement value.

By default, this instrumentation should either not record db.statement at all, or record only the unsensitive part of the queries.

@ramesius Would you be able to add the feature in this PR?

ramesius · 2023-10-30T22:18:46Z

@srprash Yeah I can take care of that in this PR.
The naming of the configuration will match.

I will raise the change with a default implementation that redacts the whole db.statement for now.

ramesius · 2023-11-01T02:02:55Z

This now adds the configuration dynamoDBStatementSerializer to customise the statement serialization.

ramesius · 2023-11-01T10:56:49Z

@srprash Good for another review when you are ready.

ramesius · 2023-11-06T23:20:24Z

@srprash Do you know when you will be able to review this again?

blumamir

Overall LGTM.
Added few minor comments.

Sorry for taking long time to review 🙏

blumamir · 2023-11-07T09:31:06Z

plugins/node/opentelemetry-instrumentation-aws-sdk/src/services/dynamodb.ts

    const spanAttributes = {
      [SemanticAttributes.DB_SYSTEM]: DbSystemValues.DYNAMODB,
      [SemanticAttributes.DB_NAME]: normalizedRequest.commandInput?.TableName,
      [SemanticAttributes.DB_OPERATION]: operation,
-      [SemanticAttributes.DB_STATEMENT]: JSON.stringify(
+      [SemanticAttributes.DB_STATEMENT]: dbStatementSerializer(


Should we record this attribute if config.dynamoDBStatementSerializer is undefined or if the config function returned undefined?

blumamir · 2023-11-07T09:44:00Z

plugins/node/opentelemetry-instrumentation-aws-sdk/src/services/dynamodb.ts

    const spanAttributes = {
      [SemanticAttributes.DB_SYSTEM]: DbSystemValues.DYNAMODB,
      [SemanticAttributes.DB_NAME]: normalizedRequest.commandInput?.TableName,
      [SemanticAttributes.DB_OPERATION]: operation,
-      [SemanticAttributes.DB_STATEMENT]: JSON.stringify(
+      [SemanticAttributes.DB_STATEMENT]: dbStatementSerializer(


Since this function is supplied by the user, the instrumentation should protect. itself from any exception thrown from this function and prevent the patch code from crashing in this case.

You can see many examples in other instrumentations

Most definitely, I will add this in 👍🏻

blumamir · 2023-11-07T09:46:35Z

plugins/node/opentelemetry-instrumentation-aws-sdk/test/dynamodb.test.ts

-          expect(
-            JSON.parse(attrs[SemanticAttributes.DB_STATEMENT] as string)
-          ).toEqual(params);
+          expect(attrs).not.toHaveProperty(SemanticAttributes.DB_STATEMENT);


I suggest that we also assert it's expected value and not only it's present.

Something missed, thanks for picking it up, will sort that out.

In fact, since this is testing default behaviour and the upcoming change to not set the property if the serializer is not configured this can probably stay as is?

blumamir · 2023-11-07T09:50:42Z

plugins/node/opentelemetry-instrumentation-aws-sdk/test/dynamodb.test.ts

+    const dynamoDBStatementSerializer: AwsSdkDynamoDBStatementSerializer = (
+      _command: CommandInput
+    ): string => {
+      return SERIALIZED_DB_STATEMENT;
+    };


I suggest adding a test where this serializer throw an exception to verify the instrumentation do not crash in this case

blumamir · 2023-11-07T09:52:49Z

plugins/node/opentelemetry-instrumentation-aws-sdk/test/dynamodb.test.ts

+      );
+    });
+
+    it('should properly execute the db statement serializer for CreateTable operation', done => {


What is the gain from testing this feature with few operations? is there anything in the code that should behave differently based on the operation that determines how to serializer is executed?

Probably a little excessive in hindsight.

Thinking about https://github.com/open-telemetry/opentelemetry-js-contrib/pull/1748/files/52d09b71cc206a6a40a77cfcf7ab69f7809805d2#r1384667968 multiple tests that assert the operation would probably be the correct direction.

blumamir · 2023-11-07T10:00:13Z

plugins/node/opentelemetry-instrumentation-aws-sdk/src/types.ts

+export type AwsSdkDynamoDBStatementSerializer = (
+  commandInput: CommandInput
+) => string | undefined;


I wonder if the serializer should also get the operation to be able to properly sanitize. I guess the sanitization logic can probably work differently for different operations?

This can also be added in the future if someone brings up the need.

I like the suggestion. As pointed out elsewhere, the sanitize function is supplied as part of the public API so I think including operation now just avoids breaking changes later 👍🏻

… returned undefined Allow for passing operation to serializer

codecov · 2023-11-14T05:06:28Z

Codecov Report

Merging #1748 (a61251c) into main (86a21d7) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #1748   +/-   ##
=======================================
  Coverage   91.44%   91.45%           
=======================================
  Files         144      144           
  Lines        7400     7406    +6     
  Branches     1481     1483    +2     
=======================================
+ Hits         6767     6773    +6     
  Misses        633      633

Files	Coverage Δ
...entelemetry-instrumentation-aws-sdk/src/aws-sdk.ts	`97.48% <100.00%> (ø)`
...ntation-aws-sdk/src/services/ServicesExtensions.ts	`100.00% <100.00%> (ø)`
...y-instrumentation-aws-sdk/src/services/dynamodb.ts	`100.00% <100.00%> (ø)`
...try-instrumentation-aws-sdk/src/services/lambda.ts	`97.77% <100.00%> (ø)`
...emetry-instrumentation-aws-sdk/src/services/sns.ts	`94.11% <100.00%> (ø)`
...emetry-instrumentation-aws-sdk/src/services/sqs.ts	`100.00% <100.00%> (ø)`

ramesius · 2023-11-15T04:10:01Z

I think I have addressed all the feedback, ready for another round 👍🏻

ramesius · 2023-11-27T10:11:32Z

@srprash @blumamir Would you mind reviewing this again to hopefully get this merged soon? 🤞🏻

ramesius requested a review from a team October 20, 2023 10:40

github-actions bot assigned blumamir Oct 20, 2023

ramesius mentioned this pull request Oct 20, 2023

Change db.statement to only be collected if there is sanitization #1552

Open

3 tasks

david-luna approved these changes Oct 25, 2023

View reviewed changes

feat: add configuration to customise dynamodb statement serialization

a5acebf

ramesius force-pushed the main branch from 9b50f09 to a5acebf Compare November 1, 2023 02:00

fix: format readme

52d09b7

david-luna mentioned this pull request Nov 2, 2023

Santinize db.statement for static queries #1744

Open

blumamir approved these changes Nov 7, 2023

View reviewed changes

ramesius and others added 3 commits November 9, 2023 22:56

chore: pass DiagLogger to DynamoDB instrumentation

c198332

chore: omit db statement when serializer is not configured or when it…

a37da4a

… returned undefined Allow for passing operation to serializer

Merge branch 'main' into main

55ce8d5

chore: run lint:fix

648c034

Merge branch 'main' into main

449d159

pichlermarc added the pkg:instrumentation-aws-sdk label Nov 27, 2023

pichlermarc changed the title ~~fix: remove un-sanitised db.statement span attribute from DynamoDB spans~~ fix(instrumentation-aws-sdk): remove un-sanitised db.statement span attribute from DynamoDB spans Nov 27, 2023

blumamir approved these changes Dec 7, 2023

View reviewed changes

Merge branch 'main' into main

a61251c

blumamir merged commit cdbb29f into open-telemetry:main Dec 7, 2023

dyladan mentioned this pull request Dec 7, 2023

chore: release main #1829

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(instrumentation-aws-sdk): remove un-sanitised db.statement span attribute from DynamoDB spans #1748

fix(instrumentation-aws-sdk): remove un-sanitised db.statement span attribute from DynamoDB spans #1748

ramesius commented Oct 20, 2023 •

edited

Loading

linux-foundation-easycla bot commented Oct 20, 2023 •

edited

Loading

andymac4182 commented Oct 23, 2023 •

edited

Loading

srprash commented Oct 30, 2023 •

edited

Loading

ramesius commented Oct 30, 2023

ramesius commented Nov 1, 2023

ramesius commented Nov 1, 2023

ramesius commented Nov 6, 2023

blumamir left a comment

blumamir Nov 7, 2023

blumamir Nov 7, 2023

ramesius Nov 9, 2023

blumamir Nov 7, 2023

ramesius Nov 9, 2023

ramesius Nov 9, 2023

blumamir Nov 7, 2023

blumamir Nov 7, 2023

ramesius Nov 9, 2023

blumamir Nov 7, 2023

ramesius Nov 9, 2023

codecov bot commented Nov 14, 2023 •

edited

Loading

ramesius commented Nov 15, 2023

ramesius commented Nov 27, 2023

fix(instrumentation-aws-sdk): remove un-sanitised db.statement span attribute from DynamoDB spans #1748

fix(instrumentation-aws-sdk): remove un-sanitised db.statement span attribute from DynamoDB spans #1748

Conversation

ramesius commented Oct 20, 2023 • edited Loading

Which problem is this PR solving?

Short description of the changes

linux-foundation-easycla bot commented Oct 20, 2023 • edited Loading

andymac4182 commented Oct 23, 2023 • edited Loading

srprash commented Oct 30, 2023 • edited Loading

ramesius commented Oct 30, 2023

ramesius commented Nov 1, 2023

ramesius commented Nov 1, 2023

ramesius commented Nov 6, 2023

blumamir left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Nov 14, 2023 • edited Loading

Codecov Report

ramesius commented Nov 15, 2023

ramesius commented Nov 27, 2023

ramesius commented Oct 20, 2023 •

edited

Loading

linux-foundation-easycla bot commented Oct 20, 2023 •

edited

Loading

andymac4182 commented Oct 23, 2023 •

edited

Loading

srprash commented Oct 30, 2023 •

edited

Loading

codecov bot commented Nov 14, 2023 •

edited

Loading