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

Signing spec for unordered multi value query parameters uncertainty #1495

Closed
stevenh opened this issue Aug 30, 2017 · 7 comments
Closed

Signing spec for unordered multi value query parameters uncertainty #1495

stevenh opened this issue Aug 30, 2017 · 7 comments

Comments

@stevenh
Copy link
Contributor

stevenh commented Aug 30, 2017

Hi guys you merged my PR to fix the signing to match the public spec with regards unordered multi value query parameters.

Since then I've been having a conversation with @mhart about the same issue on his javascript library and he has raised that the published spec could well be the actual source of the issue and the library might have been doing the correct thing.

The main reason for this theory is existence of an official AWS v4 test suite, which I was previous unaware of.

The suite includes a specific test to confirm that key values are ordered, hence it raises questions over the completeness of the official specification.

So the questions are:

  1. Should we revert the changes from PR v1.10.34 #1494 (included in v1.10.34) until a conclusion is found?
  2. What is the canonical source of truth for AWS v4 signing, the test suite or the spec?
  3. If the answer to the above is the test suite then how do we get the spec updated so that it's complete and hence avoids issues like this?

In addition to this @mhart also points out addition issues with how the canonical string is calculated which also need bottoming out too.

@jasdel
Copy link
Contributor

jasdel commented Aug 30, 2017

Thanks @stevenh. I would of expected the spec and test suite to align, but in cases where they do not I can work with our internal teams to investigate and clarify the discrepancies.

I think a big area the SDK is lacking is that it does not run tests against the official test suite. We need to correct this and add the test suite validation to the SDK.

For #1494 I'll take a look that again today to see if it needs to be reverted. Did you discover services/APIs that break if the query values are not sorted as well?

@jasdel
Copy link
Contributor

jasdel commented Aug 30, 2017

I'm going to revert #1491 for now @stevenh until a clearer picture of the signing requirements are available. This work should also include integrating the test cases into the SDK.

jasdel added a commit to jasdel/aws-sdk-go that referenced this issue Aug 30, 2017
jasdel added a commit that referenced this issue Aug 30, 2017
…ses (#1499)

Reverts #1491 change since it conflicts with the AWS v4 signer test cases found in #1495. This case is not documented in the V4 spec. Additional research is needed before #1491 can be accepted. As either a implementation detail of the test cases, or an undocumented requirement of the spec.
@stevenh
Copy link
Contributor Author

stevenh commented Aug 30, 2017

Yep I agree with that, thanks for your time.

To answer your question no we haven't come across any services that break with unsorted values but I definitely think there is a risk of that.

@mhart
Copy link

mhart commented Aug 30, 2017

I can certainly confirm that a number of AWS services expect, in the canonical string, query values for the same key to be sorted. Or at the very least, their error messages strongly imply this.

Here's an example (assumes AWS_ACCESS_KEY_ID declared and uses node for the date formatting)

curl 'https://lambda.us-east-1.amazonaws.com/2015-03-31/functions/?b=1&a=2&a=1' \
  -H "X-Amz-Date: $(node -p 'new Date().toISOString().slice(0, 19).replace(/[-:]/g, "")')Z" \
  -H "Authorization: AWS4-HMAC-SHA256 Credential=${AWS_ACCESS_KEY_ID}/$(node -p 'new Date().toISOString().slice(0, 10).replace(/-/g, "")')/us-east-1/lambda/aws4_request, SignedHeaders=host;x-amz-date, Signature=1283cc55db9b511d289cf9518fd97c71a958adc6a7151d50fd4cd42d2a0dcfc6"

Or with python:

curl 'https://lambda.us-east-1.amazonaws.com/2015-03-31/functions/?b=1&a=2&a=1' \
  -H "X-Amz-Date: $(python -c'import time;print(time.strftime("%Y%m%dT%H%M%SZ", time.gmtime()))')" \
  -H "Authorization: AWS4-HMAC-SHA256 Credential=${AWS_ACCESS_KEY_ID}/$(python -c'import time;print(time.strftime("%Y%m%d", time.gmtime()))')/us-east-1/lambda/aws4_request, SignedHeaders=host;x-amz-date, Signature=1283cc55db9b511d289cf9518fd97c71a958adc6a7151d50fd4cd42d2a0dcfc6"

This deliberately uses a bogus Signature and result in something like:

{"message":"The request signature we calculated does not match the signature you provided. Check your AWS Secret Access Key and signing method. Consult the service documentation for details.\n\nThe Canonical String for this request should have been\n'GET\n/2015-03-31/functions/\na=1&a=2&b=1\nhost:lambda.us-east-1.amazonaws.com\nx-amz-date:20170830T205130Z\n\nhost;x-amz-date\ne3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855'\n\nThe String-to-Sign should have been\n'AWS4-HMAC-SHA256\n20170830T205130Z\n20170830/us-east-1/lambda/aws4_request\n9e3851521dca49b1ba0bad9261e650e5eba22fa1c39f684b62e752a6c166e2da'\n"}

The relevant portion of which is:

The Canonical String for this request should have been
'GET
/2015-03-31/functions/
a=1&a=2&b=1
host:lambda.us-east-1.amazonaws.com
x-amz-date:20170830T205130Z

host;x-amz-date
e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855'

Note that the expected canonical string has sorted the query params by value for the same key (after sorting the keys) – b=1&a=2&a=1 has become a=1&a=2&b=1

@mhart
Copy link

mhart commented Aug 30, 2017

Of course, S3 being S3, it has completely different expectations and eliminates duplicate query param values altogether:

curl 'https://s3.us-east-1.amazonaws.com/?b=1&a=2&a=1' \
  -H "X-Amz-Date: $(node -p 'new Date().toISOString().slice(0, 19).replace(/[-:]/g, "")')Z" \
  -H "X-Amz-Content-Sha256: UNSIGNED-PAYLOAD" \
  -H "Authorization: AWS4-HMAC-SHA256 Credential=${AWS_ACCESS_KEY_ID}/$(node -p 'new Date().toISOString().slice(0, 10).replace(/-/g, "")')/us-east-1/s3/aws4_request, SignedHeaders=host;x-amz-date, Signature=1283cc55db9b511d289cf9518fd97c71a958adc6a7151d50fd4cd42d2a0dcfc6"

Yields:

<?xml version="1.0" encoding="UTF-8"?>
<Error>
<Code>SignatureDoesNotMatch</Code>
<Message>The request signature we calculated does not match the signature you provided. Check your key and signing method.</Message>
...
<CanonicalRequest>GET
/
a=2&amp;b=1
host:s3.us-east-1.amazonaws.com
x-amz-date:20170830T212017Z

host;x-amz-date
UNSIGNED-PAYLOAD</CanonicalRequest>
...
</Error>

There are a bunch more issues with S3 which you can read about here (I notice some have since been addressed in the documentation, eg "you do not normalize URI paths for requests to Amazon S3")

@awstools awstools mentioned this issue Aug 31, 2017
@jasdel
Copy link
Contributor

jasdel commented Sep 15, 2017

Thanks for the additional information @stevenh. I'll raise the duplicate query parameters issue with S3 as well. Even if none of the APIs are lists, I wouldn't expect the service to invalidate the signature because of it.

@jasdel
Copy link
Contributor

jasdel commented Nov 28, 2017

After reviewing this the SDK is going to continue to sort the query values instead to the query fields when calculating the AWS Sig v4 Signature. While the Sigv4 signing spec does not explicitly state that the query values need to be sorted in addition to the keys, the test suite does exercise this expectation.

For the duplicate query parameters in S3, there doesn't seem to be any API operation which could trigger this from the SDKs. I suggest raising this issue further on the AWS S3 Forums.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants