Skip to content

inject cloned request headers for http requests#5144

Merged
wconti27 merged 2 commits intomasterfrom
conti/fix-invalid-signature-exception-aws
Jan 22, 2025
Merged

inject cloned request headers for http requests#5144
wconti27 merged 2 commits intomasterfrom
conti/fix-invalid-signature-exception-aws

Conversation

@wconti27
Copy link
Copy Markdown
Contributor

@wconti27 wconti27 commented Jan 22, 2025

What does this PR do?

Fixes this issue opened in the Datadog Lambda JS Serverless Layer
Header injection for distributed context was breaking AWS signed requests, specifically when requests were retried, and ended up being turned off for these requests. This PR fixes the problem and re-enables injection for signed requests. Instead of mutating request headers, we now clone them. AWS-SDK was regenerating request signatures based off the mutated headers (now with DD context data) during retries, causing signed requests to fail.

Solution based off similar fix in Opentelemetry-js SDK
Deeper Explanation of issue

See #5127 #5141

Motivation

Plugin Checklist

Additional Notes

@wconti27 wconti27 requested review from a team as code owners January 22, 2025 16:39
@wconti27
Copy link
Copy Markdown
Contributor Author

Note that this work is related to: #4867, #4717

@github-actions
Copy link
Copy Markdown
Contributor

Overall package size

Self size: 8.52 MB
Deduped: 94.88 MB
No deduping: 95.39 MB

Dependency sizes | name | version | self size | total size | |------|---------|-----------|------------| | @datadog/libdatadog | 0.4.0 | 29.44 MB | 29.44 MB | | @datadog/native-appsec | 8.4.0 | 19.25 MB | 19.26 MB | | @datadog/native-iast-taint-tracking | 3.2.0 | 13.9 MB | 13.91 MB | | @datadog/pprof | 5.4.1 | 9.76 MB | 10.13 MB | | protobufjs | 7.2.5 | 2.77 MB | 5.16 MB | | @datadog/native-iast-rewriter | 2.6.1 | 2.59 MB | 2.73 MB | | @opentelemetry/core | 1.14.0 | 872.87 kB | 1.47 MB | | @datadog/native-metrics | 3.1.0 | 1.06 MB | 1.46 MB | | @opentelemetry/api | 1.8.0 | 1.21 MB | 1.21 MB | | import-in-the-middle | 1.11.2 | 112.74 kB | 826.22 kB | | source-map | 0.7.4 | 226 kB | 226 kB | | opentracing | 0.14.7 | 194.81 kB | 194.81 kB | | lru-cache | 7.18.3 | 133.92 kB | 133.92 kB | | pprof-format | 2.1.0 | 111.69 kB | 111.69 kB | | @datadog/sketches-js | 2.1.0 | 109.9 kB | 109.9 kB | | semver | 7.6.3 | 95.82 kB | 95.82 kB | | lodash.sortby | 4.7.0 | 75.76 kB | 75.76 kB | | ignore | 5.3.1 | 51.46 kB | 51.46 kB | | shell-quote | 1.8.1 | 44.96 kB | 44.96 kB | | istanbul-lib-coverage | 3.2.0 | 29.34 kB | 29.34 kB | | rfdc | 1.3.1 | 25.21 kB | 25.21 kB | | @isaacs/ttlcache | 1.4.1 | 25.2 kB | 25.2 kB | | tlhunter-sorted-set | 0.1.0 | 24.94 kB | 24.94 kB | | limiter | 1.1.5 | 23.17 kB | 23.17 kB | | dc-polyfill | 0.1.4 | 23.1 kB | 23.1 kB | | retry | 0.13.1 | 18.85 kB | 18.85 kB | | jest-docblock | 29.7.0 | 8.99 kB | 12.76 kB | | crypto-randomuuid | 1.0.0 | 11.18 kB | 11.18 kB | | ttl-set | 1.0.0 | 4.61 kB | 9.69 kB | | path-to-regexp | 0.1.12 | 6.6 kB | 6.6 kB | | koalas | 1.0.2 | 6.47 kB | 6.47 kB | | module-details-from-path | 1.0.3 | 4.47 kB | 4.47 kB |

🤖 This report was automatically generated by heaviest-objects-in-the-universe

@wconti27
Copy link
Copy Markdown
Contributor Author

Comment from Reverted PR:
@tlhunter

William tested this locally while communicating with real AWS. He was able to reproduce the issue using 100 parallel network requests. He was able to ensure that his change fixed the issue.

However, when he tried to add the test to CI, he wasn't able to reproduce the issue. It seems that this was due to making Local Stack requests to localhost.

For these reasons it seems that the code is safe to merge despite having acceptance tests. The unit test does ensure that the modified header object is not being reused for subsequent request retries.

Here's the code that William tested with:

const tracer = require('dd-trace').init();
const AWS = require("aws-sdk");

const main = async () => {
    const cwl = new AWS.CloudWatchLogs({ region: "us-east-1" });

    const promises = new Array(100).fill(true).map(() => new Promise((resolve, reject) => {
        cwl.describeLogGroups(function (err, data) {
            if (err) {
                console.log(err.name);
                console.log("Got error:", err.message);
                console.log("ERROR Request Authorization:");
                console.log(this.request.httpRequest.headers.Authorization);
                console.log("ERROR Request traceparent:");
                console.log(this.request.httpRequest.headers.traceparent);
                console.log("Retry count:", this.retryCount);
                console.log(this.request.httpRequest.headers);

                reject(err);
                return;
            }

            resolve(data);
        });
    }));

    const result = await Promise.all(promises);

    console.log(result.length);
};
main().catch(console.error);

@wconti27 wconti27 self-assigned this Jan 22, 2025
@wconti27 wconti27 merged commit 37546ab into master Jan 22, 2025
@wconti27 wconti27 deleted the conti/fix-invalid-signature-exception-aws branch January 22, 2025 17:44
szegedi pushed a commit that referenced this pull request Jan 28, 2025
* fix aws-sdk invalid signature exception

* fix fetch and undici tests
@szegedi szegedi mentioned this pull request Jan 28, 2025
szegedi pushed a commit that referenced this pull request Jan 29, 2025
* fix aws-sdk invalid signature exception

* fix fetch and undici tests
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Intermittent AWS InvalidSignatureException with v9.116.0

2 participants