Sorting of query string variables breaks signing

[stevenh]

Sorting of the query string variables breaks signing as its not part of the spec:
http://docs.aws.amazon.com/general/latest/gr/sigv4-create-canonical-request.html

The keys should be sorted but not the values, so this like breaks thinks:
https://github.com/mhart/aws4/blob/master/aws4.js#L226

Simply removing the sort will fix the issue.

[mhart]

The AWS4 test suite from Amazon mandated this ordering – the fixtures in the test suite came directly from there.

Just looking through the latest zipfile now, it appears they may have changed a whole bunch of fixtures since I originally downloaded it, which is rather annoying.

I'll update all the fixtures and fix any issues that arise and see if this is one of the things that has changed.

Thanks for the heads up 👍

[stevenh]

Yer they have bugs in their own libraries as they just merged my PR for the official golang SDK for the exact same issue.

[mhart]

Yeah, I called them out on it years ago and never heard back – they must've silently updated the suite and never told me:

aws/aws-sdk-js#853
and
https://forums.aws.amazon.com/thread.jspa?messageID=693401

Seems they've changed a bunch of things in their test suite, including the file format of the requests with things like multiline headers. Will take a little bit of fiddling which I should be able to get to later today. Thanks for the PR too btw – I'll fix everything that it takes to get this new suite passing which should probably fix the issue, but if not I'll be happy to merge it.

[stevenh]

I suspect it won't fix it as looks like their official JS SDK still has the sort in queryParamsToString although I've not tested.

One additional thing to make you aware of, that I found when fixing this was, signing currently changes the order of params in request, which was unexpected. This shouldn't be an issue but it could impact the target service if it was param order sensitive.

I tracked this down to the use of the querystring library which doesn't maintain ordering, so there wasn't any easy fix. The main problem I see this causing is when tracing calls; if the user looks for their request string e.g. /?foob=B&fooa=C&foob=A in their logs they won't find it as the request sent will have the parameters reordered to /?foob=B&foob=A&fooa=C which isn't ideal. Obviously when using query string signing the string would change but these would ideally just be appended.

Finally, if you do think there's a conflict in the spec which needs to be resolved, you may want to raise it via the official golang sdk issue as I've found the guys there are very responsive.

[mhart]

So I've had a look through it – the current test suite (as the old one) expects the query param values to be sorted. There's specifically a test for it called get-vanilla-query-order-value – you can find it in the zipfile I linked to above.

What makes you think this was breaking signing? Was there an AWS service that you used that broke with this?

[mhart]

Ah – or was your point more that it's fine to sort the values when calculating the canonical string (indeed, you have to), but that the original query string shouldn't be modified?

[stevenh]

Given the spec the current implementation incorrectly sorts the values as well as the keys.

I don't have an example of an Amazon provided service that breaks, as typically they don't support multi value query string parameters, however services provided by other companies which also use AWS v4 signing are effected. We have one such service, which is why this came to light.

Obviously the spec should be the canonical source of truth, with variations from that being bugs, but there are also additional items to consider.

1. With regards the explicit test, I believe it's designed to ensure that the key ordering is correct and that the author created said test to verify that and in doing so also incorrectly verified that values should be sorted, as that's what the library did.
2. Order of multi value query parameters is important to web services, where as key order typically isn't.
3. The Amazon team accepted my PR to correct the behaviour and test for query sorting in the official golang SDK .

All these facts lead me to draw the conclusion that values within query string keys should not be sorted when performing AWS v4 signing.

Hope that helps explain my reasoning clearly?

[mhart]

The spec unfortunately isn't complete – as I've been noting to AWS for a couple of years now. You can read about the various obscurities here: aws/aws-sdk-js#853 (comment) – many of those encoding issues are not even mentioned in the spec.

The test that I spoke of, get-vanilla-query-order-value, doesn't have multiple keys – it's clearly specifically designed to test the order of the query key values. The request is:

GET /?Param1=value2&Param1=Value1

And the query portion of the canonical request is:

Param1=Value1&Param1=value2

So you can see that there's only one key there (Param1) – and they're specifically testing the ordering of the values.

I wouldn't be surprised if AWS' services also supported this ordering in the errors they throw for incorrect signatures where they specify what the canonical string should have been – this is usually the best place to see how the services are actually implemented. When I've got some time I can throw together some tests for this unless you'd like to.

In terms of the ordering of the values as they should be sent in the actual query string (as opposed to the canonical string) – I agree that that's a bug that needs to be fixed in this library, and I'll keep this open until I've cleaned up the new tests.

[stevenh]

Thanks @mhart, given this new info I agree with you, it does indeed seem like their spec is incomplete :(

I will reopen an issue with golang guys to see if they can shed any official light on this.

Thanks for taking the time to talk about this most appreciated 👍

[maheshsundaram]

I'm getting this error:

{
  "Error": {
    "Code": "MissingParameter",
    "Message": "The parameter MeasureName is required.",
    "Type": "Sender"
  },
  "RequestId": "6f53fa13-95d0-11e7-afdb-17c59174c8ba"
}

which seems related to this issue, however using #60 doesn't fix it... any ideas what could be happening?

[mhart]

Pretty confident it's not related to this – you'd be getting a signature exception otherwise.

This is something to do with the way you're calling whatever API you're calling

[maheshsundaram]

@mhart Wow, amazingly fast reply! Thanks for your perspective. I'm using cloudwatch. I'll keep looking.

[maheshsundaram]

Hey again, I figured out a few things that might be helpful for someone else in the future, or perhaps in narrowing down this issue's resolution.

I'm sending a request to Cloudwatch ({ service: 'monitoring' }).

When &Version=2010-08-01 is added to the path and the authorization is sent in the headers, the API returns The request signature we calculated does not match the signature you provided.

However when the authorization is sent as query params, it works fine.

Finally, when the Version query param is not added to the request, it works fine sending the authorization as query params or headers, however it returns that MetricName must be MeasureName.

[mhart]

@jasonmendes can you post some code of how you're querying CloudWatch? I suspect your issues may be to do with the way you're constructing and sending the query (and I don't think it's related to this ordering issue, so you might want to open a separate issue)

[maheshsundaram]

Thanks @mhart ! I can create a separate issue for this if you still think it's a different issue after seeing what I'm doing.

✅ Working example showing sending authorization as query params:

const inputURL = 'https://monitoring.amazonaws.com/?Namespace=AWS/ELB&Action=GetMetricStatistics&MetricName=Latency&StartTime=2017-08-29T00:26:17&EndTime=2017-09-12T00:26:17&Statistics.member.1=Average&Period=86400&Dimensions.member.1.Name=LoadBalancerName&Dimensions.member.1.Value=production-server&cloudwatch_host=monitoring.amazonaws.com';

const { host, path, protocol } = URL.parse(inputURL);

const newPath = path.concat('&Version=2010-08-01');

const signerOptions = {
  host,
  path: newPath,
  signQuery: true,
};

const signerSecrets = { accessKeyId, secretAccessKey }; // defined from the env (not shown)

const signedRequest = aws4.sign(signerOptions, signerSecrets);

const signedURL = `${protocol}//${host}${signedRequest.path}`;

// then later => request({ uri: signedURL })

🚧 Example that doesn't work, sending the authorization in the headers:

const inputURL = 'https://monitoring.amazonaws.com/?Namespace=AWS/ELB&Action=GetMetricStatistics&MetricName=Latency&StartTime=2017-08-29T00:26:17&EndTime=2017-09-12T00:26:17&Statistics.member.1=Average&Period=86400&Dimensions.member.1.Name=LoadBalancerName&Dimensions.member.1.Value=production-server&cloudwatch_host=monitoring.amazonaws.com';

const { host, path } = URL.parse(inputURL);

const newPath = path.concat('&Version=2010-08-01');

const signerOptions = {
  host,
  path: newPath,
};

const signerSecrets = { accessKeyId, secretAccessKey }; // defined from the env (not shown)

const signedRequest = aws4.sign(signerOptions, signerSecrets);

// then later => request({ uri: inputURL, headers: signedRequest.headers })

🔍 For some background as to why:

The project I'm contributing to already uses the request-promise library and is a service for handling data fetching to other services. Using e.g. the official AWS SDK could be done, but the full URLs are already being fed into this service, so destructing them to use something like CloudWatch.getMetricStatisticsResult({ ...params }) would require deconstructing the URL in a clever way (i.e. translating Dimensions.member.1.Name=LoadBalancerName into { Dimensions: [ { Name: 'LoadBalancerName' } ] } -- that is, just signing the pre-built URL seemed easier.

[mhart]

But in the example that doesn't work you're requesting a different URL to the one you're signing... The URL you sign has to match the one you request (aside from the authorization parameters which aws4 will add)

In any case, certainly not related to this issue – this issue is about the sorting order of query values for the same key (ie, https://some.host.com/?key=a&key=A&key=1)

[mhart]

@jasonmendes change that second example to this and it should work:

const origURL = 'https://monitoring.amazonaws.com/?Namespace=AWS/ELB&Action=GetMetricStatistics&MetricName=Latency&StartTime=2017-08-29T00:26:17&EndTime=2017-09-12T00:26:17&Statistics.member.1=Average&Period=86400&Dimensions.member.1.Name=LoadBalancerName&Dimensions.member.1.Value=production-server&cloudwatch_host=monitoring.amazonaws.com';

// obvs could just add this to the string above, but for illustration I'll make it explicit
const inputURL = origURL.concat('&Version=2010-08-01');

const { host, path } = URL.parse(inputURL);

// etc...

request({ uri: inputURL, headers: signedRequest.headers })

[maheshsundaram]

🤦 Sorry, that's painfully obvious now! Thanks for your time again @mhart

Edit: Given that my comments here don't contribute to fixing this issue, or rather just add noise, please feel free to delete them.