URL-decode subresource values as well as other query parameters

[shino]

In signing request to S3, current implementation does not URL-decode subresource values in query parameters. Other query parameters are properly URL-decoded in creating canonicalResource.

For example, another client implementation, s3curl, perform URL-decode for subresource values [1].

[1] https://github.com/rtdp/s3curl/blob/master/s3curl.pl#L192-L199

[coveralls]

Coverage decreased (-0.0%) to 94.43% when pulling c2aeff5 on shino:fix-subresource-to-sign into eb83937 on aws:master.

[shino]

Pushed the commit that includes fixing wrong test.

[coveralls]

Coverage decreased (-0.0%) to 94.43% when pulling 2930831 on shino:fix-subresource-to-sign into eb83937 on aws:master.

[shino]

Access denied case with s3curl.pl was as follows, if that helps.
partNumber is 1 but it is intentionally URL-encoded to %31
(it's arbitrary to URL-encode url-encoding-safe charactoers.)
Then AWS S3 responded with correct (for them) StringToSign and partNumber value
was URL-decoded as 1.

% s3curl.pl --put tmp --id admin -- -s http://some-buckets.s3.amazonaws.com/'foo?partNumber=%31&uploadId=AA'
<?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><AWSAccessKeyId>AKIAIT2M2HIBZSYWPZHA</AWSAccessKeyId><StringToSign>PUT



x-amz-date:Thu, 12 Mar 2015 08:53:53 GMT
/some-buckets/foo?partNumber=1&amp;uploadId=AA</StringToSign><SignatureProvided>ASD2oFQMizISn6rgQ3pzkGM/rRE=</SignatureProvided><StringToSignBytes>50 55 54 0a 0a 0a 0a 78 2d 61 6d 7a 2d 64 61 74 65 3a 54 68 75 2c 20 31 32 20 4d 61 72 20 32 30 31 35 20 30 38 3a 35 33 3a 35 33 20 47 4d 54 0a 2f 73 6f 6d 65 2d 62 75 63 6b 65 74 73 2f 66 6f 6f 3f 70 61 72 74 4e 75 6d 62 65 72 3d 31 26 75 70 6c 6f 61 64 49 64 3d 41 41</StringToSignBytes><RequestId>DEB2EF9AF838DA84</RequestId><HostId>5PnAxyxIJmaTi3Mql5gLA7rxz+3muivl2pBeprxLtQAF+hatOrZ85Wu/9f6Dcwt5</HostId></Error>%                                                                                                               %

As a comparison, with normal (not URL-encoded) uploadId, S3 responds
with NoSuchBucket, I guess it's after authentication.

% s3curl.pl --put tmp --id admin -- -s http://some-buckets.s3.amazonaws.com/'foo?partNumber=1&uploadId=AA'
<?xml version="1.0" encoding="UTF-8"?>
<Error><Code>NoSuchBucket</Code><Message>The specified bucket does not exist</Message><BucketName>some-buckets</BucketName><RequestId>F4D7295E8BBEA974</RequestId><HostId>HkndoAW6MbPhongMTwhas3oYdcJFkkRRSpzHQOgISZHoe7BYjywGa5lkm3lzKht3</HostId></Error>

[AdityaManohar]

@shino Can you provide a reproduction test case with a failing request?

[shino]

@AdityaManohar I wrote simple sample code [1] using List Parts API [2].

Command line usage:

% nodejs list_parts_node.js <REGION> <BUCKET> <KEY> <UPLOAD-ID>

Now only signature is related, we can use any bucket, key and uploadId.
Successful authentication case for both original and this PR, error code
is NoSuchBucket.

% node list_parts_node.js ap-northeast-1 b k u
[snip]
<Error><Code>NoSuchBucket</Code>[snip]

If uploadId is url-unsafe, then original code returns error SignatureDoesNotMatch.
This manifests that client side signature calculation is wrong.
Full output is at [3] and it can be confirmed that query paramter value
is not URL-decoded as uploadId=% in <StringToSign> element of response XML.

% node list_parts_node.js ap-northeast-1 b k '%'
[snip]
<Error><Code>SignatureDoesNotMatch</Code>[snip]

This PR fixes this case and get error NoSuchBucket properly.

[1] https://gist.github.com/shino/602caae62725a82c559c
[2] http://docs.aws.amazon.com/AmazonS3/latest/API/mpUploadListParts.html
[3] https://gist.github.com/shino/eea05420dd8c826442bd#file-url-unsafe-txt

[shino]

I forgot to add region argument, so I added it to the above comment.

[AdityaManohar]

@shino
Thanks for the reproduction test case. I'm going to verify this behavior and merge this in if everything looks good.

[shino]

Ping. If there is any subtle points or inconvenience for merge, please ping back me 😄

[shino]

Ping. How's going?

[orinciog]

I've spent three days to hunt this bug.
Thank you a lot @shino for this commit.

@AdityaManohar It's been !!two years!! from this pull request and !!4 lines of code!! Do you ever want to merge it or do I have to publish a merged version, that is actually working with v2 signatures?

Thank you.

[shino]

@orinciog Could you describe your case?
The misbehavior I wrote at the above comment (#530 (comment)) is that expected and actual cases are both error but error code is wrong.
If you find the case where the expected behavior is success (2xx) but error happens, it's valuable as additional information to this issue.

[orinciog]

@shino Yes, my case is identical with basho/riak_cs#1327
I can't use multipart upload with v2 signatures in riak because uploadPart fails with AccessDenied.
What describe SBRK in first post is identical to me.

The problem is that when computing the signature for uploadPart, the value of uploadId is not url-encoded.
I have patched with your version (thanks again), that encodes all values and now I can use uploadPart.

[srchase]

Closing out this issue. We appreciate the contribution, but due to the age, feel it does not need to be merged at this time.

[orinciog]

The issue is still valid for the current version of aws-sdk-js (2.392.0).

I had to fork your lib in order to make v2 signatures work.

[lock]

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs and link to relevant comments in this thread.