mhart / aws4

Signs and prepares Node.js requests using AWS Signature Version 4
MIT License
699 stars 175 forks source link

Canonical headers include entries from `extraHeadersToIgnore` #167

Closed mxxk closed 1 month ago

mxxk commented 2 months ago

Thanks for maintaining this repo, @mhart!

With regards to extraHeadersToIgnore, I noticed that the current logic only omits them from signedHeaders():

https://github.com/mhart/aws4/blob/7e2d1cb64f9f604f5a303cd85f19c32f4f5f131a/aws4.js#L310-L321

They are, however, still present in the canonical headers:

https://github.com/mhart/aws4/blob/7e2d1cb64f9f604f5a303cd85f19c32f4f5f131a/aws4.js#L298-L308

Is this intentional? From what I can tell, including headers in the canonicalHeaders but not in the signedHeaders messes up the signature calculation.

For sake of example, suppose X-Amz-Date is to be excluded from the signature (and instead an unsigned Date header is provided in its place):

See code
```js const bucket = "BUCKET"; const request = { service: "s3", hostname: `${bucket}.s3.${process.env.AWS_REGION}.amazonaws.com`, region: process.env.AWS_REGION, method: "GET", path: "/", extraHeadersToIgnore: { "x-amz-date": true, }, }; const signer = new aws4.RequestSigner(request); signer.sign(); const canonicalRequest = signer.canonicalString(); const url = new URL(`https://${request.hostname}${request.path}`); delete request.headers["X-Amz-Date"]; console.log({ canonicalRequest, headers: request.headers, url: url.href, }); const response = await fetch(url, { method: request.method, headers: { ...request.headers, Date: signer.getDateTime(), }, }); console.log(`${response.status} ${response.statusText}`); console.log(await response.text()); ```

The resulting request to fails AWS signature validation because the canonical headers generated by aws4 include x-amz-date, but the canonical headers reconstructed by S3 do not. (S3 echoes its canonical request in the <CanonicalRequest> element of the error response):

Source Canonical request
aws4 ``` GET / host:BUCKET.us-west-2.amazonaws.com x-amz-content-sha256:e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855 x-amz-date:20240713T221300Z x-amz-security-token:REDACTED host;x-amz-content-sha256;x-amz-security-token e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855 ```
S3 ``` GET / host:BUCKET.s3.us-west-2.amazonaws.com x-amz-content-sha256:e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855 x-amz-security-token:REDACTED host;x-amz-content-sha256;x-amz-security-token e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855 ```

[!NOTE]

S3's canonical request computation makes sense, because I believe S3 computes canonical headers only from those in SignedHeaders (of the Authorization header).

To workaround this issue, I can trick aws4 by making it parse the request (prepareRequest()), deleting the X-Amz-Date header, and then finally making it sign the request without calling prepareRequest() again:

See diff
```diff @@ -9,10 +9,11 @@ const request = { }, }; const signer = new aws4.RequestSigner(request); +signer.prepareRequest(); +delete request.headers["X-Amz-Date"]; signer.sign(); const canonicalRequest = signer.canonicalString(); const url = new URL(`https://${request.hostname}${request.path}`); -delete request.headers["X-Amz-Date"]; console.log({ canonicalRequest, headers: request.headers, ```

However, this seems brittle, since it relies on what seems to be an internal implementation detail of aws4:

https://github.com/mhart/aws4/blob/7e2d1cb64f9f604f5a303cd85f19c32f4f5f131a/aws4.js#L172

@mhart given this, do you think it makes sense to exclude headers listed in extraHeadersToIgnore from the canonical header string? If so, I'd be happy to contribute a PR for this.

mxxk commented 1 month ago

Looks like I overlooked #157 as an earlier issue about the same thing, so closing this issue as a duplicate.