-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
Comments
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? |
…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.
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. |
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
Or with
This deliberately uses a bogus Signature and result in something like:
The relevant portion of which is:
Note that the expected canonical string has sorted the query params by value for the same key (after sorting the keys) – |
Of course, S3 being S3, it has completely different expectations and eliminates duplicate query param values altogether:
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&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") |
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. |
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. |
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:
In addition to this @mhart also points out addition issues with how the canonical string is calculated which also need bottoming out too.
The text was updated successfully, but these errors were encountered: