AwsV4Signer::sign invalid for S3 GET with signQuery

[jed]

When I use aws s3 presign like this,

AWS_REGION=ap-northeast-1 \
AWS_ACCESS_KEY_ID=AKIA................ \
AWS_SECRET_ACCESS_KEY=........................................ \
aws s3 presign 's3://BUCKET_NAME/BUCKET_KEY'

I get a URL like this, which works.

https://BUCKET_NAME.s3.ap-northeast-1.amazonaws.com/BUCKET_KEY
  ?X-Amz-Algorithm=AWS4-HMAC-SHA256
  &X-Amz-Credential=AKIA................%2F20210608%2Fap-northeast-1%2Fs3%2Faws4_request
  &X-Amz-Date=20210608T051344Z
  &X-Amz-Expires=3600
  &X-Amz-SignedHeaders=host
  &X-Amz-Signature=ccf8e91fc184c3805342093f0b68f111cfa04790f5e8b2ae7abd3511f05ff127

But when I use AwsV4Signer::sign like this,

let url = 'https://BUCKET_NAME.s3.ap-northeast-1.amazonaws.com/BUCKET_KEY'
let params = {url, accessKeyId, secretAccessKey, signQuery: true}
let signer = new AwsV4Signer(params)
let {url: {href}} = await signer.sign()
console.log(href)

I get a URL like this, which does not work.

https://BUCKET_NAME.s3.ap-northeast-1.amazonaws.com/BUCKET_KEY
  ?X-Amz-Date=20210608T051604Z
  &X-Amz-Expires=86400
  &X-Amz-Algorithm=AWS4-HMAC-SHA256
  &X-Amz-Credential=AKIATTIKK53SN5ZYA7UX%2F20210608%2Fap-northeast-1%2Fs3%2Faws4_request
  &X-Amz-SignedHeaders=host%3Bx-amz-content-sha256
  &X-Amz-Signature=bffa444c66d3f959a691b9d4886387c0a1b74809dd0658939128e5741cd16056

It seems that aws4fetch is adding the x-amz-content-sha256 to the signed headers (and omitting it from the querystring), which apparently is not correct for querystring signatures.

I fixed this for my limited use case by adding 'x-amz-content-sha256' to UNSIGNABLE_HEADERS, but I imagine this would break signing for non-querystring, non-GET cases.

[mhart]

Huh! I might need to add a test specifically for S3 GET – I have one for POST.

It might just be a matter of messing with this logic:

aws4fetch/src/main.js

Lines 170 to 172 in 447d747

	if (this.service === 's3' && !this.headers.has('X-Amz-Content-Sha256')) {
	this.headers.set('X-Amz-Content-Sha256', 'UNSIGNED-PAYLOAD')
	}

Either use params instead of this.headers, or add a check to the if for signQuery

Will need some testing in any case 🤔

[mikeRead]

Looked into that if statement but ran into some issues for the AwsClient.fetch function throwing an arrayBuffer error, which I haven't had time to look into.

However checking for this when filtering signableHeaders does seem to work well for all

I might have gone overboard with checking for this exact use case but as long as signQuery is true in the confg and request is a s3 GET request, the x-amz-content-sha25 header will be ignored which should help with the non-querystring, non-GET cases.

this.signableHeaders = ['host', ...this.headers.keys()]
      .filter(header => {
        if (header === 'x-amz-content-sha256' && !allHeaders && signQuery && this.service.toLowerCase() === 's3' && this.method.toUpperCase() === 'GET' && this.headers.get(header) === 'UNSIGNED-PAYLOAD') {
          return false
        }
        return allHeaders || !UNSIGNABLE_HEADERS.includes(header)
      })
      .sort()

[bradworsfold-wd]

If someone were to put up a PR for this, would @mhart be happy to review, approve, merge, publish etc?

[mhart]

Finally got around to addressing this 😅 Published as v1.0.14

Hope that doesn't break anyone!