breaking: Use sha256 to hash lambda traceId that are triggered by Step Functions and set _dd.p.tid by kimi-p · Pull Request #534 · DataDog/datadog-lambda-js

kimi-p · 2024-05-02T18:57:02Z

What does this PR do?

using the most significant 128 bits of sha256 (instead of current md5) to hash trace_id and the first 64 for span_id.
the 1st and 65th bit should be 0.
manually take:
- the first 64 bits from the sha256 hash for _dd.p.tid tag
- the 2nd 64 bits from the sha256 hash for traceId
- if it were for spanId, use sha256 hash for parentId (which is step function task's spanId)
ts-md5 package is remove.
@node/type in DevDependencies needs to be upgraded by running yarn add @types/node -D as suggested in this the issue Issue with TS4.9 - 'AbortSignal' was also declared here. microsoft/TypeScript#51567
note that for the root span it has span_id in number but trace_id in hex
relevant PR for 64 bits using md5

{
    "trace": {
        "root_id": "3124668925291824462",
        "spans": {
            "3124668925291824462": {
                "trace_id": "41919d29ba4149cd40fcae25a0e4e323",
                "span_id": "3124668925291824462",
                "parent_id": "0",
                "start": 1716393574.422,
                "end": 1716393576.138,
                "duration": 1.716,
                "type": "stepfunctions",
                "service": "kimi-test-nodejs-linking",
                "name": "aws.stepfunctions",
                "resource": "kimi-test-nodejs-linking",
                "resource_hash": "c76d3199bc4b077",
                "host_id": 3884291,
                "hostname": "none",
                "env": "dev",
                "host_groups": [
                    "env:dev"
                ],
...

Motivation

To support 128 bits trace IDs and to avoid showing up in security vulnerability scans, we are upgrading the hashing method (md5) to sha256.

Testing Guidelines

trace link
example traceId 41919d29ba4149cd40fcae25a0e4e323 is a hash, not a number anymore.
verified that the parent_id of the aws.lambda span matches the aws.stepfunctions.lambda task span.

Additional Notes

This is a breaking change that will affect all current Step Functions public beta users. And what will break is the Step Functions trace and Lambda traces linking.

Since this has to be done at some point, we want to put this change out before GA. Serverless-Integrations team will work with Sumedha and have some sort of announcement to let customers know the exact cut off time so that they can deploy their Lambda around the same time.

Referencing PR for implementing MD5

Types of Changes

Bug fix
New feature
Breaking change
Misc (docs, refactoring, dependency upgrade, etc.)

Check all that apply

This PR's description is comprehensive
This PR contains breaking changes that are documented in the description
This PR introduces new APIs or parameters that are documented and unlikely to change in the foreseeable future
This PR impacts documentation, and it has been updated (or a ticket has been logged)
This PR's changes are covered by the automated tests
This PR collects user input/sensitive content into Datadog
This PR passes the integration tests (ask a Datadog member to run the tests)

nine5two7 · 2024-05-10T19:02:12Z

-    const res = "0" + binary.substring(1, 64);
-    if (res === "0".repeat(64)) {
+    const res = "0" + binary.substring(1, 128);
+    if (res === "0".repeat(128)) {


Here, we need to ensure that the 0th and 64th bits of the 128-bit data are both set to 0.

nine5two7

LGTM

kimi-p · 2024-05-16T19:43:54Z

+        "arn:aws:states:sa-east-1:425362996713:stateMachine:MyStateMachine-b276uka1j#lambda#2",
+        _64_BITS,
+      );
+      expect(actual).toEqual("5759173372325510050");


this matches logs-backend's hash result
https://github.com/DataDog/logs-backend/pull/71616/files#diff-820e0a1dd4f0f97815ac54f993c08b3c44c67dd26a312968c214f16ab73494c0R20-R21

kimi-p · 2024-05-16T19:44:30Z

+        "arn:aws:states:sa-east-1:425362996713:stateMachine:MyStateMachine-b276uka1j#lambda#1",
+        _64_BITS,
+      );
+      expect(actual).toEqual("3711631873188331089");


this matches logs-backend PR's hashing result
https://github.com/DataDog/logs-backend/pull/71616/files#diff-820e0a1dd4f0f97815ac54f993c08b3c44c67dd26a312968c214f16ab73494c0R15

…/update-sha256

kimi-p · 2024-05-22T20:34:59Z

+    if (type === TRACE_ID) {
+      intArray = uint8Array.subarray(8, 16);
+    } else {
+      // type === SPAN_ID || type === DD_P_TID
+      intArray = uint8Array.subarray(0, 8);
+    }


This section matches the logic that we put on logs-backend implementation.

nine5two7 · 2024-05-31T13:44:30Z

-    "@smithy/util-middleware" "^2.2.0"
-    "@smithy/util-retry" "^2.2.0"
-    "@smithy/util-utf8" "^2.3.0"
+    "@aws-sdk/client-sso-oidc" "3.577.0"


Is it normal to have so many changes on the yarn.lock file?

I think it's just that these are the result of not pinned packages. When I remove the lock file and regenerated, some packages have been upgraded all at once.

nine5two7 · 2024-05-31T15:12:53Z

    });

+    const ptid = this.deterministicSha256HashToBigIntString(this.context["step_function.execution_id"], DD_P_TID);
+    if (ptid === "0".repeat(16)) {


Can you add some explanations for the condition check in Line 15?

Good catch. I think this was coming from similar logic implemented somewhere the dd-trace-js. But since deterministicSha256HashToBigIntString will never return all zeros, we can remove the check here.

nine5two7

LGTM

duncanista · 2024-06-04T20:10:34Z

+    const _DatadogSpanContext = require("dd-trace/packages/dd-trace/src/opentracing/span_context");
+    const id = require("dd-trace/packages/dd-trace/src/id");

-    const spanContext = SpanContextWrapper.fromTraceContext({
-      traceId,
-      parentId,
-      sampleMode,
-      source: TraceSource.Event,
+    const ddTraceContext = new _DatadogSpanContext({
+      traceId: id(traceId, 10),
+      spanId: id(parentId, 10),
+      sampling: { priority: sampleMode.toString(2) },
    });

+    const ptid = this.deterministicSha256HashToBigIntString(this.context["step_function.execution_id"], DD_P_TID);
+    ddTraceContext._trace.tags["_dd.p.tid"] = id(ptid, 10).toString(16);
+    const spanContext = new SpanContextWrapper(ddTraceContext, TraceSource.Event);
+


Why are we not using the current SpanContextWrapper code? Is it because we need to add the ptid?

If so, is there another way to not require dd-trace code here? Or at least, we need to lazy load it, since the tracer could be not available

Yes.

Lemme try to lazy load it.

duncanista

Left a comment

Co-authored-by: jordan gonzález <30836115+duncanista@users.noreply.github.com>

…text

kimi-p · 2024-06-05T02:38:41Z

I have verified after these change based on the comments that the spans linking still works in staging.

duncanista

LGTM

kimi-p added 11 commits May 2, 2024 14:55

v1

131f778

fix test

b9032cd

remove md5 package and regenerate yarn lock

aa1a0fa

lint

60e0445

upgrade node/type

1617e1a

fix 3 tests

858cfd5

fix span ids for the same tests (previous commit was for trace id

3338526

add @aws-crypto/sha256-js

3c7b456

remove 8h

89a3ce3

Empty-Commit

43fe7b8

update yarn.lock

4ed002f

kimi-p marked this pull request as ready for review May 7, 2024 18:50

kimi-p requested a review from a team as a code owner May 7, 2024 18:50

nine5two7 reviewed May 10, 2024

View reviewed changes

kimi-p added 2 commits May 16, 2024 10:42

update 65th bit to be 0 and tests all accidentally pass

1a22e0a

rename

ee7d6bd

nine5two7 approved these changes May 16, 2024

View reviewed changes

kimi-p added 2 commits May 16, 2024 11:20

rename

eb49ebf

add option to hashing to 64 bits or 128 bits

f3c87db

kimi-p changed the title ~~Update Step Function hashing to use sha256 and return the most significant 128 bit~~ Update Step Function hashing to use sha256 and return the most significant 128 bit for trace_id May 16, 2024

merge main

e8470cd

kimi-p commented May 16, 2024

View reviewed changes

kimi-p added 6 commits May 16, 2024 15:44

update test name

d9c4072

try log-backend log

b3d76cd

remote print

36f3942

update 64 zeros case

a3b7bd7

Empty-Commit

e3be794

Empty-Commit

848a3b2

kimi-p commented May 22, 2024

View reviewed changes

Comment thread src/trace/step-function-service.ts Outdated

kimi-p added 2 commits May 22, 2024 15:22

lint

0de000f

Merge branch 'main' of github.com:DataDog/datadog-lambda-js into kimi…

0c3dbbd

…/update-sha256

kimi-p commented May 22, 2024

View reviewed changes

kimi-p changed the title ~~Update Step Function hashing to use sha256 and return the most significant 128 bit for trace_id~~ Use sha256 to hash lambda traceId that are triggered by Step Functions and set _dd.p.tid May 23, 2024

nine5two7 reviewed May 31, 2024

View reviewed changes

remove unnecessary edge case check

cf96c98

nine5two7 approved these changes May 31, 2024

View reviewed changes

Merge branch 'main' into kimi/update-sha256

8e28404

duncanista reviewed Jun 4, 2024

View reviewed changes

Comment thread src/trace/step-function-service.ts Outdated

duncanista reviewed Jun 4, 2024

View reviewed changes

Comment thread src/trace/step-function-service.ts Outdated

duncanista reviewed Jun 4, 2024

View reviewed changes

kimi-p and others added 3 commits June 4, 2024 16:17

small logic update from comment

0debb2a

Co-authored-by: jordan gonzález <30836115+duncanista@users.noreply.github.com>

use .reduce()

1837ffe

Co-authored-by: jordan gonzález <30836115+duncanista@users.noreply.github.com>

lazy load require dd-trace/packages/dd-trace/src/opentracing/span_con…

b385614

…text

duncanista reviewed Jun 4, 2024

View reviewed changes

Comment thread src/trace/step-function-service.ts Outdated

lazy load tracer classes

d3e6fbf

duncanista approved these changes Jun 5, 2024

View reviewed changes

duncanista mentioned this pull request Jun 5, 2024

breaking: upgrade version with breaking change #554

Merged

11 tasks

duncanista changed the title ~~Use sha256 to hash lambda traceId that are triggered by Step Functions and set _dd.p.tid~~ breaking: Use sha256 to hash lambda traceId that are triggered by Step Functions and set _dd.p.tid Jun 5, 2024

kimi-p merged commit 7753de1 into main Jun 5, 2024

kimi-p deleted the kimi/update-sha256 branch June 5, 2024 19:18

Conversation

kimi-p commented May 2, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What does this PR do?

Motivation

Testing Guidelines

Additional Notes

Types of Changes

Check all that apply

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nine5two7 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nine5two7 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

duncanista left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

kimi-p commented Jun 5, 2024

Uh oh!

duncanista left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

kimi-p commented May 2, 2024 •

edited

Loading