Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

inject cloned request headers for signed aws-sdk requests #5127

Merged
merged 1 commit into from
Jan 17, 2025

Conversation

wconti27
Copy link
Contributor

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

Motivation

Plugin Checklist

Additional Notes

@wconti27 wconti27 requested review from a team as code owners January 17, 2025 17:37
Copy link

Overall package size

Self size: 8.48 MB
Deduped: 94.83 MB
No deduping: 95.35 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 | | 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

Copy link

codecov bot commented Jan 17, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 59.15%. Comparing base (726cfd6) to head (eddd476).
Report is 5 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #5127      +/-   ##
==========================================
- Coverage   63.10%   59.15%   -3.96%     
==========================================
  Files         108      109       +1     
  Lines        3456     3582     +126     
==========================================
- Hits         2181     2119      -62     
- Misses       1275     1463     +188     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@tlhunter tlhunter self-requested a review January 17, 2025 18:04
@tlhunter
Copy link
Member

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);

@tlhunter
Copy link
Member

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

@wconti27 wconti27 merged commit 4ef12fc into master Jan 17, 2025
237 of 238 checks passed
@wconti27 wconti27 deleted the conti/fix-invalid-signature-exception-aws branch January 17, 2025 21:44
watson pushed a commit that referenced this pull request Jan 22, 2025
Enables tracing header injection on AWS signed requests and fixes problem causing InvalidSignatureException.
@watson watson mentioned this pull request Jan 22, 2025
watson pushed a commit that referenced this pull request Jan 22, 2025
Enables tracing header injection on AWS signed requests and fixes problem causing InvalidSignatureException.
@watson watson mentioned this pull request Jan 22, 2025
watson added a commit that referenced this pull request Jan 22, 2025
watson added a commit that referenced this pull request Jan 22, 2025
watson added a commit that referenced this pull request Jan 22, 2025
watson added a commit that referenced this pull request Jan 22, 2025
@watson
Copy link
Collaborator

watson commented Jan 22, 2025

Heads up: This PR was reverted in #5141

watson pushed a commit that referenced this pull request Jan 23, 2025
Enables tracing header injection on AWS signed requests and fixes problem causing InvalidSignatureException.
watson added a commit that referenced this pull request Jan 23, 2025
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
3 participants