You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@jclouds.apache.org by Diwaker Gupta <no...@github.com> on 2013/07/31 02:08:17 UTC
[jclouds] URL encode string to sign. (#78)
As recommended at
http://s3.amazonaws.com/doc/s3-developer-guide/RESTAuthentication.html:
"You can also send a signature as a URL-encoded query-string parameter
in the URL for the request."
You can merge this Pull Request by running:
git pull https://github.com/maginatics/jclouds feature/s3-temp-signed-url
Or you can view, comment on it, or merge it online at:
https://github.com/jclouds/jclouds/pull/78
-- Commit Summary --
* URL encode string to sign.
-- File Changes --
M providers/aws-s3/src/main/java/org/jclouds/aws/s3/blobstore/AWSS3BlobRequestSigner.java (4)
M providers/aws-s3/src/test/java/org/jclouds/aws/s3/blobstore/AWSS3BlobSignerExpectTest.java (8)
-- Patch Links --
https://github.com/jclouds/jclouds/pull/78.patch
https://github.com/jclouds/jclouds/pull/78.diff
Re: [jclouds] URL encode string to sign. (#78)
Posted by Diwaker Gupta <no...@github.com>.
> Could you tweak the expect test to actually verify that the URL encoding is happening?
@demobox I tried but that would basically require conjuring up the right timestamp such that the signature ends up having a space in it :-/ I gave up after a few tries.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/78#issuecomment-21834158
Re: [jclouds] URL encode string to sign. (#78)
Posted by Andrew Gaul <no...@github.com>.
Closed #78.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/78
Re: [jclouds] URL encode string to sign. (#78)
Posted by BuildHive <no...@github.com>.
[jclouds » jclouds #242](https://buildhive.cloudbees.com/job/jclouds/job/jclouds/242/) SUCCESS
This pull request looks good
[(what's this?)](https://www.cloudbees.com/what-is-buildhive)
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/78#issuecomment-21892445
Re: [jclouds] URL encode string to sign. (#78)
Posted by Diwaker Gupta <no...@github.com>.
> @@ -43,9 +45,9 @@ public AWSS3BlobSignerExpectTest() {
> protected HttpRequest getBlobWithTime() {
> return HttpRequest.builder().method("GET")
> .endpoint("https://container.s3.amazonaws.com/name" +
> - "?Signature=Y4Ac4sZfBemGZmgfG78F7IX%20IFg%3D&Expires=1212683902&AWSAccessKeyId=identity")
Note that the %20 in the Signature now changes to %2B
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/78/files#r5513239
Re: [jclouds] URL encode string to sign. (#78)
Posted by Andrew Phillips <no...@github.com>.
Could you tweak the expect test to actually _verify_ that the URL encoding is happening?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/78#issuecomment-21832978
Re: [jclouds] URL encode string to sign. (#78)
Posted by Andrew Gaul <no...@github.com>.
Please expand the commit message to explain what this addresses. Also reference JCLOUDS-200.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/78#issuecomment-21832731
Re: [jclouds] URL encode string to sign. (#78)
Posted by Andrew Phillips <no...@github.com>.
> @@ -91,10 +94,23 @@ private HttpRequest signForTemporaryAccess(HttpRequest request, long timeInSecon
> Date date = dateService.rfc1123DateParse(dateString);
> String expiration = String.valueOf(TimeUnit.MILLISECONDS.toSeconds(date.getTime()) + timeInSeconds);
> HttpRequest.Builder<?> builder = request.toBuilder().replaceHeader(HttpHeaders.DATE, expiration);
> - final String signature = authSigner.sign(authSigner.createStringToSign(builder.build()));
> - return builder.addQueryParam(TEMPORARY_SIGNATURE_PARAM, signature)
> - .addQueryParam(HttpHeaders.EXPIRES, expiration)
> + String stringToSign = authSigner.createStringToSign(builder.build());
> + // We MUST encode the signature because addQueryParam internally _always_ decodes values
> + // and if we don't encode the signature here, the decoding may change the signature. For e.g.
> + // any '+' characters in the signature will be converted to space ' ' on decoding.
> + String signature = authSigner.sign(stringToSign);
> + try {
> + signature = URLEncoder.encode(signature, Charsets.UTF_8.name());
> + } catch (UnsupportedEncodingException e) {
> + throw new IllegalStateException("Bad encoding on input: " + signature, e);
> + }
> I'm only using URLEncoder because Strings2 does surprising things.
Makes sense, thanks for the research and explanation. Now that this logic has got bigger, perhaps pull it out as a separate method?
```
HttpRequest.Builder<?> builder = ...
String stringToSign = authSigner.createStringToSign(builder.build());
HttpRequest ret = builder.addQueryParam(HttpHeaders.EXPIRES, expiration)
.addQueryParam("AWSAccessKeyId", identity)
// Signature MUST be the last parameter because if it isn't, even encoded '+' values in the
// signature will be converted to a space by a subsequent addQueryParameter.
// See HttpRequestTest.testAddBase64AndUrlEncodedQueryParams for more details.
.addQueryParam(TEMPORARY_SIGNATURE_PARAM, createEncodedSignature(stringToSign))
.build();
...
private static createEncodedSignature(String stringToSign) {
// We MUST encode the signature because addQueryParam internally _always_ decodes values
...
String signature = authSigner.sign(stringToSign);
try {
return URLEncoder.encode(signature, Charsets.UTF_8.name());
} catch (UnsupportedEncodingException e) {
throw new IllegalStateException("Bad encoding on input: " + signature, e);
}
}
```
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/78/files#r5517393
Re: [jclouds] URL encode string to sign. (#78)
Posted by BuildHive <no...@github.com>.
[jclouds » jclouds #237](https://buildhive.cloudbees.com/job/jclouds/job/jclouds/237/) SUCCESS
This pull request looks good
[(what's this?)](https://www.cloudbees.com/what-is-buildhive)
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/78#issuecomment-21833797
Re: [jclouds] URL encode string to sign. (#78)
Posted by Andrew Gaul <no...@github.com>.
+1, note that we need this for 1.6.x.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/78#issuecomment-21896285
Re: [jclouds] URL encode string to sign. (#78)
Posted by Andrew Phillips <no...@github.com>.
>conjuring up the right timestamp such that the signature ends up having a space in it
Bit of an NP-complete problem there, I guess. Understood. Happily, it looks like the signature from `getBlobWithTime` already contains a space: `Y4Ac4sZfBemGZmgfG78F7IX%20IFg`?
That seems correctly encoded...how was that test passing without this change?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/78#issuecomment-21835108
Re: [jclouds] URL encode string to sign. (#78)
Posted by Andrew Phillips <no...@github.com>.
> @@ -91,7 +92,8 @@ private HttpRequest signForTemporaryAccess(HttpRequest request, long timeInSecon
> Date date = dateService.rfc1123DateParse(dateString);
> String expiration = String.valueOf(TimeUnit.MILLISECONDS.toSeconds(date.getTime()) + timeInSeconds);
> HttpRequest.Builder<?> builder = request.toBuilder().replaceHeader(HttpHeaders.DATE, expiration);
> - final String signature = authSigner.sign(authSigner.createStringToSign(builder.build()));
> + String stringToSign = authSigner.createStringToSign(builder.build());
> + String signature = Strings2.urlEncode(authSigner.sign(stringToSign));
> return builder.addQueryParam(TEMPORARY_SIGNATURE_PARAM, signature)
Just a quick one to make the diff smaller - happy also to leave as is:
```
import static org.jclouds.util.Strings2.urlEncode;
...
String signature = authSigner.sign(authSigner.createStringToSign(builder.build()));
return builder.addQueryParam(TEMPORARY_SIGNATURE_PARAM, urlEncode(signature))
...
```
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/78/files#r5493072
Re: [jclouds] URL encode string to sign. (#78)
Posted by Andrew Phillips <no...@github.com>.
> I'm only using URLEncoder because Strings2 does surprising things.
In a future cleanup, it may be worth seeing whether we can replace some of the Strings2 black magic with Guava's [UriEscapers](http://docs.guava-libraries.googlecode.com/git-history/242cf19dc4af8779f54038b89d26e56c1b16ed9d/javadoc/com/google/common/net/UriEscapers.html)
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/78#issuecomment-21943048
Re: [jclouds] URL encode string to sign. (#78)
Posted by Diwaker Gupta <no...@github.com>.
> @@ -43,9 +45,9 @@ public AWSS3BlobSignerExpectTest() {
> protected HttpRequest getBlobWithTime() {
> return HttpRequest.builder().method("GET")
> .endpoint("https://container.s3.amazonaws.com/name" +
> - "?Signature=Y4Ac4sZfBemGZmgfG78F7IX%20IFg%3D&Expires=1212683902&AWSAccessKeyId=identity")
@demobox Not sure I follow your question. It's not about `Strings2` vs `URLEncoder` but that previously because of the parameter ordering, jclouds would encode a '+' as '%20' instead of '%2B'. I don't have context on why this was done but it seems to be the "expected" behavior -- see `HttpRequestTest.testAddBase64AndUrlEncodedQueryParams` for instance. I'm only using URLEncoder because Strings2 does surprising things.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/78/files#r5514776
Re: [jclouds] URL encode string to sign. (#78)
Posted by Diwaker Gupta <no...@github.com>.
@demobox hmm good point, maybe there's some other filter that URL encodes the entire query path post signature generation? Will look into it.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/78#issuecomment-21840949
Re: [jclouds] URL encode string to sign. (#78)
Posted by Andrew Gaul <no...@github.com>.
Ran all unit tests and integration tests against aws-s3. Pushed to master and 1.6.x.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/78#issuecomment-21905353
Re: [jclouds] URL encode string to sign. (#78)
Posted by Diwaker Gupta <no...@github.com>.
> @@ -91,10 +94,23 @@ private HttpRequest signForTemporaryAccess(HttpRequest request, long timeInSecon
> Date date = dateService.rfc1123DateParse(dateString);
> String expiration = String.valueOf(TimeUnit.MILLISECONDS.toSeconds(date.getTime()) + timeInSeconds);
> HttpRequest.Builder<?> builder = request.toBuilder().replaceHeader(HttpHeaders.DATE, expiration);
> - final String signature = authSigner.sign(authSigner.createStringToSign(builder.build()));
> - return builder.addQueryParam(TEMPORARY_SIGNATURE_PARAM, signature)
> - .addQueryParam(HttpHeaders.EXPIRES, expiration)
> + String stringToSign = authSigner.createStringToSign(builder.build());
> + // We MUST encode the signature because addQueryParam internally _always_ decodes values
> + // and if we don't encode the signature here, the decoding may change the signature. For e.g.
> + // any '+' characters in the signature will be converted to space ' ' on decoding.
> + String signature = authSigner.sign(stringToSign);
> + try {
> + signature = URLEncoder.encode(signature, Charsets.UTF_8.name());
> + } catch (UnsupportedEncodingException e) {
> + throw new IllegalStateException("Bad encoding on input: " + signature, e);
> + }
@demobox there's a single caller and the method is already specialized so I'd prefer to leave it as-is. Happy to refactor out if you feel strongly though.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/78/files#r5518501
Re: [jclouds] URL encode string to sign. (#78)
Posted by Andrew Phillips <no...@github.com>.
> Need to wait for Guava 15 for that :\
Indeed :-( Seems like the "Since: 11.0" Javadoc entry was a bit of a copy-paste ghost :-)
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/78#issuecomment-21951491
Re: [jclouds] URL encode string to sign. (#78)
Posted by Andrew Phillips <no...@github.com>.
> @@ -91,10 +94,23 @@ private HttpRequest signForTemporaryAccess(HttpRequest request, long timeInSecon
> Date date = dateService.rfc1123DateParse(dateString);
> String expiration = String.valueOf(TimeUnit.MILLISECONDS.toSeconds(date.getTime()) + timeInSeconds);
> HttpRequest.Builder<?> builder = request.toBuilder().replaceHeader(HttpHeaders.DATE, expiration);
> - final String signature = authSigner.sign(authSigner.createStringToSign(builder.build()));
> - return builder.addQueryParam(TEMPORARY_SIGNATURE_PARAM, signature)
> - .addQueryParam(HttpHeaders.EXPIRES, expiration)
> + String stringToSign = authSigner.createStringToSign(builder.build());
> + // We MUST encode the signature because addQueryParam internally _always_ decodes values
> + // and if we don't encode the signature here, the decoding may change the signature. For e.g.
> + // any '+' characters in the signature will be converted to space ' ' on decoding.
> + String signature = authSigner.sign(stringToSign);
> + try {
> + signature = URLEncoder.encode(signature, Charsets.UTF_8.name());
> + } catch (UnsupportedEncodingException e) {
> + throw new IllegalStateException("Bad encoding on input: " + signature, e);
> + }
OK, let's roll as-is then and get this in to 1.6.2-incubating ;-)
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/78/files#r5518991
Re: [jclouds] URL encode string to sign. (#78)
Posted by Andrew Gaul <no...@github.com>.
> @@ -45,7 +47,7 @@ protected HttpRequest getBlobWithTime() {
> .endpoint("https://container.s3.amazonaws.com/name" +
> "?Signature=Y4Ac4sZfBemGZmgfG78F7IX%20IFg%3D&Expires=1212683902&AWSAccessKeyId=identity")
> .addHeader("Host", "container.s3.amazonaws.com")
> - .addHeader("Date", "Thu, 05 Jun 2008 16:38:19 GMT").build();
> + .addHeader("Date", DATE).build();
Appreciated but unrelated; please submit a separate PR in the future.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/78/files#r5493022
Re: [jclouds] URL encode string to sign. (#78)
Posted by Diwaker Gupta <no...@github.com>.
Need to wait for Guava 15 for that :\
On Thu, Aug 1, 2013 at 8:10 AM, Andrew Phillips <no...@github.com>wrote:
> I'm only using URLEncoder because Strings2 does surprising things.
>
> In a future cleanup, it may be worth seeing whether we can replace some of
> the Strings2 black magic with Guava's UriEscapers<http://docs.guava-libraries.googlecode.com/git-history/242cf19dc4af8779f54038b89d26e56c1b16ed9d/javadoc/com/google/common/net/UriEscapers.html>
>
> —
> Reply to this email directly or view it on GitHub<https://github.com/jclouds/jclouds/pull/78#issuecomment-21943048>
> .
>
--
http://floatingsun.net
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/78#issuecomment-21948261
Re: [jclouds] URL encode string to sign. (#78)
Posted by Andrew Phillips <no...@github.com>.
> @@ -43,9 +45,9 @@ public AWSS3BlobSignerExpectTest() {
> protected HttpRequest getBlobWithTime() {
> return HttpRequest.builder().method("GET")
> .endpoint("https://container.s3.amazonaws.com/name" +
> - "?Signature=Y4Ac4sZfBemGZmgfG78F7IX%20IFg%3D&Expires=1212683902&AWSAccessKeyId=identity")
Ah, interesting. Any reason why `Strings2.urlEncode` wouldn't work here?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/78/files#r5514473