[JCLOUDS-1401] Properly URL-encode the CanonicalQueryString when it contains funny characters #1226
Conversation
…ontains funny characters.
|
Thanks, @jglick! I'd say before running live tests you could add a small unit test that just verifies the You can also add a mock unit test to the S3ClientMockTest class. Mock tests just mock HTTP requests and provide a convenient way to do assertions on the generated requests. You could write one test that verifies the signature that has been sent for a request. For the live tests, we don't mention explicitly the S3 parameters in the wiki because they are the same for all providers. You can invoke the live tests as follows (note this follows the generic format explained in the testing page): mvn clean install -Plive -pl :s3 -Dtest=S3ClientLiveTest \
-Dtest.s3.identity=<access key> \
-Dtest.s3.credential=<access secret> \
-Dtest.s3.endpoint=https://s3.amazonaws.comSince S3 is a generic API (if you read this quick page you'll get the difference between the S3 api and the AWS-S3 provider) that can be implemented by many providers, we need to set the endpoint parameter indicating where to run the live tests against. Since you are already using AWS, you could run the mvn clean install -Plive -pl :aws-s3 -Dtest=AWSS3ClientLiveTest \
-Dtest.aws-s3.identity=<acces key> \
-Dtest.aws-s3.credential=<access secret>Note that since you are now using the The credentials you need to pass in the identity and credential property are the same ones you pass to the jclouds |
|
I did try running It is good to at least have confirmation that I am less confident in the value of a unit test or mock test since it is not self-evident what the actual designed signature would be. The fix is just that which produces a signature that Amazon in fact accepts. |
|
I think I figured out that the existing tests just do not support session credentials (and I cannot access my account without them), so I need to patch the tests.
This is not true—the inherited |
|
There are some tests that fail even without my patch: These seem to be related to the region, perhaps? And: which seem to be caused by an undefined |
… to distinguish URI.query from URI.rawQuery.
| PageSet<? extends StorageMetadata> list = view.getBlobStore().list(containerName, | ||
| ListContainerOptions.Builder.prefix(dirName + "/")); | ||
| assertEquals(list.size(), 1); | ||
| StorageMetadata md = list.iterator().next(); |
There was a problem hiding this comment.
This fails with the reported AWS error if you s/rawQuery/query/.
| private static final String LIST_BUCKET_RESULT = "AWS4-HMAC-SHA256 " | ||
| + "Credential=AKIAPAEBI3QI4EXAMPLE/20150203/cn-north-1/s3/aws4_request, " | ||
| + "SignedHeaders=host;x-amz-content-sha256;x-amz-date, " | ||
| + "Signature=6cc5d0758e2599be7cb172fd57cefab2828201a2b4d372972a83dc304de93958"; |
There was a problem hiding this comment.
Merely based on running the test input against rawQuery. I did not attempt to consult the AWS documentation and manually construct the signature base as per these instructions.
The key to the fix is that Queries.parseKeyValueFromStringToDecodedMap is used to URL-decode the parameters of the query string after splitting on &, and we then reëncode in getCanonicalizedQueryString, so using URI.getQuery() (which decodes the value) not only double-decodes, it actually loses information. For example,
URI u = new URI("https://x.com/?p1=a&p2=b%26c%3Dd&p3=e");
System.out.println(u.getRawQuery());
System.out.println(u.getQuery());
will print
p1=a&p2=b%26c%3Dd&p3=e
p1=a&p2=b&c=d&p3=e
| builder.credentialsSupplier(new Supplier<Credentials>() { | ||
| @Override | ||
| public Credentials get() { | ||
| return SessionCredentials.builder().identity(identity).credential(credential).sessionToken(sessionToken).build(); |
There was a problem hiding this comment.
Without this I was unable to run live tests at all. (Maybe something about IAM?) The new parameter is optional.
|
Thanks for the patch and for taking your time to run the live tests @jglick! Merged to master and 2.1.x. I'll have a look at the failing tests, as if you run just the S3 ones and pass the endpoint, all succeed. There must be some minor mess about the endpoints in the aws-s3 tests. |
Confirmed that when running The other two failures I mentioned were in |
|
Thanks Ignasi! |
JCLOUDS-1401
I managed to write integration tests for this in Jenkins (which fail without this patch and pass with it). ~~~I would have liked to add a test case for it to
AWSS3ClientLiveTest(orS3ClientLiveTest? not sure I grasp the distinction) but did not manage to get the existing tests to run; I either got errors about an unknown access key, or a 403, regardless of what parameters I tried; and there is no apparent way to pick up values from~/.aws/credentials. Testing makes no specific mention of S3 and what theidentityandcredentialwould be, concretely; Getting Started does not give format examples or give a hint about how the internal properties map would look; and developer guides in the source tree like that for Google Cloud seem to be absent for S3.~~~@carlossg @kuisathaverat