add aws s3 signature v4 #678

zhaojin0 · 2015-02-11T03:01:08Z

AWS S3 signature v4 impl

zhaojin0 · 2015-02-11T05:12:55Z

add aws s3 signature v4, plz review.

gaul · 2015-02-11T12:33:09Z

apis/s3/src/main/java/org/jclouds/s3/config/S3HttpApiModule.java

-               return Optional.absent();
-            }
-
+    @SuppressWarnings("unchecked")


Seems like a lot of gratuitous reindentation.

gaul · 2015-02-11T12:51:10Z

@zhaojin0 Please address Checkstyle violations and remove gratuitous reindentation. I will look at this some more afterwards.

gaul · 2015-02-13T02:16:02Z

Could you tag this commit with JCLOUDS-480 so that JIRA will note it?

…va API

gaul · 2015-02-19T02:59:05Z

@zhaojin0 Any updates on this pull request? I would like to include it in the upcoming 1.9.0 release.

zhaojin0 · 2015-02-20T14:38:03Z

I'm sorry for my lazy...
i add temporary access signature code, but i dont known how to test it. It could work in aws region cn-north-1.

public class AWSS3BlobStoreContextModule extends S3BlobStoreContextModule {
   //...
   @Override
   protected void bindRequestSigner() {
      bind(BlobRequestSigner.class).to(AWSS3BlobRequestSignerV4.class);
   }
}

gaul · 2015-02-26T00:23:01Z

@zhaojin0 I do not understand how this is supposed to work -- you should bind the V4 signer somewhere like AWSS3BlobStoreContextModule:

bind(BlobRequestSigner.class).to(AWSS3BlobRequestSignerV4.class);

I also do not understand the overall strategy here, but I believe that jclouds should use the new V4 signer for all AWS calls and regions. The generic S3 can continue to use the V2 code.

I played around with this a bit and do not see any new live tests. I added one to AWSBucketsLiveTest which requires a region with a V4 signer:

public void testV4Region() throws Exception {
    String bucketName = getScratchContainerName() + "-v4-only";
    getApi().putBucketInRegion(Region.EU_CENTRAL_1, bucketName);
    try {
        assertEquals(Region.EU_CENTRAL_1, getApi().getBucketLocation(bucketName));
    } finally {
        destroyContainer(bucketName);
    } 
}

However a see a variety of signing errors of the form:

org.jclouds.aws.AWSResponseException: request GET https://gaul-blobstore-2489004073675868056-v4-only.s3-eu-central-1.amazonaws.com/ HTTP/1.1 failed with code 400, error: AWSError{requestId='7EF96215BA50542A', requestToken='t2sOzdsOU5tJqer0pLcfbuAQnuASYf4X3xf1qfWQkjSESCp5hBcgKhMuoO3zRJ6u14OLnNwHokE=', code='InvalidRequest', message='The authorization mechanism you have provided is not supported. Please use AWS4-HMAC-SHA256.', context='{HostId=t2sOzdsOU5tJqer0pLcfbuAQnuASYf4X3xf1qfWQkjSESCp5hBcgKhMuoO3zRJ6u14OLnNwHokE=}'}

Finally there are many spurious changes and Checkstyle violations in this pull request which make it hard to read. You must correct these before I can continue to review.

As for the signer tests, the general ones in BaseBlobSignerLiveTest should exercise your code if you bind the signer correctly as per my first comment.

…mporary access

zhaojin0 · 2015-02-27T07:48:16Z

it's use for sign a temporary access...
I provided testcase AWSS3BlobSignerV4ExpectTest.
sorry, this's my first pull request, I format some code....

gaul · 2015-03-04T23:08:32Z

apis/s3/src/main/java/org/jclouds/s3/filters/RequestAuthorizeSignatureV4.java

+        }
+        String contentSha256 = base16().lowerCase().encode(hash(payloadStream));
+        try {
+            payloadStream.reset();


What happens when the payload is not repeatable?

payload stream use calculate content hash.
if can not be repeatable, the payload cannot append to http request body.

gaul · 2015-03-31T20:14:56Z

Any plans to continue with this pull request?

zhaojin0 · 2015-04-01T06:37:50Z

AWS Sign V4 use sha256 content hash. If payload can not be reset, aws supported chunked uploads.
http://docs.aws.amazon.com/AmazonS3/latest/API/sigv4-streaming.html

gaul · 2015-04-17T18:46:21Z

@zhaojin0 I am testing this and see many errors of the form:

org.jclouds.aws.AWSResponseException: request GET https://gaul-blobstore3760643340725640912-v4-only.s3-eu-central-1.amazonaws.com/ HTTP/1.1 failed with code 400, error: AWSError{requestId='0F84681CE6013127', requestToken='THXHcOkpHKTdN7DedbIb8qCj7/MuvvvGyu31O42h6BjkTFSvHdKNggDL/aQ6Mm1IGROwsAO58fw=', code='InvalidRequest', message='The authorization mechanism you have provided is not supported. Please use AWS4-HMAC-SHA256.', context='{HostId=THXHcOkpHKTdN7DedbIb8qCj7/MuvvvGyu31O42h6BjkTFSvHdKNggDL/aQ6Mm1IGROwsAO58fw=}'}

when deleting items in the container between runs. Any suggestions on this? Also do all the integration tests pass for you? I see a few errors:

  AWSS3ContainerIntegrationLiveTest>BaseContainerIntegrationTest.deleteContainerIfEmptyWithoutContents:315 expected [true] but found [false]
  AWSS3ContainerLiveTest>BaseContainerLiveTest.testPublicAccessInNonDefaultLocationWithBigBlob:112->BaseContainerLiveTest.runCreateContainerInLocation:124->BaseBlobStoreIntegrationTest.assertConsistencyAwareContainerExists:361->BaseBlobStoreIntegrationTest.assertConsistencyAware:248->BaseBlobStoreIntegrationTest.assertConsistencyAware:235 » HttpResponse
  AWSS3ServiceIntegrationLiveTest>BaseServiceIntegrationTest.testAllLocations:52->BaseBlobStoreIntegrationTest.assertConsistencyAware:248->BaseBlobStoreIntegrationTest.assertConsistencyAware:235 » HttpResponse
  AWSS3ServiceIntegrationLiveTest>BaseServiceIntegrationTest.testGetAssignableLocations:93 {scope=REGION, id=eu-central-1, description=eu-central-1, parent=aws-s3, iso3166Codes=[DE-HE]} ||{scope=PROVIDER, id=aws-s3, description=https://s3.amazonaws.com, iso3166Codes=[US, US-CA, US-OR, BR-SP, IE, SG, AU-NSW, JP-13]}

Tests run: 99, Failures: 4, Errors: 0, Skipped: 1

gaul · 2015-04-17T18:47:28Z

apis/s3/src/test/java/org/jclouds/s3/internal/BaseS3ClientTest.java

+import java.io.IOException;
+
+import static org.testng.Assert.assertEquals;
+


Spurious code movement?

zhaojin0 · 2015-04-19T07:38:43Z

It's seem as region eu-central-1 doesn't supported AWS sign V2.

在 2015/4/18 2:46, Andrew Gaul 写道:

@zhaojin0 https://github.com/zhaojin0 I am testing this and see many
errors of the form:

|org.jclouds.aws.AWSResponseException: request GET https://gaul-blobstore3760643340725640912-v4-only.s3-eu-central-1.amazonaws.com/ HTTP/1.1 failed with code 400, error: AWSError{requestId='0F84681CE6013127', requestToken='THXHcOkpHKTdN7DedbIb8qCj7/MuvvvGyu31O42h6BjkTFSvHdKNggDL/aQ6Mm1IGROwsAO58fw=', code='InvalidRequest', message='The authorization mechanism you have provided is not supported. Please use AWS4-HMAC-SHA256.', context='{HostId=THXHcOkpHKTdN7DedbIb8qCj7/MuvvvGyu31O42h6BjkTFSvHdKNggDL/aQ6Mm1IGROwsAO58fw=}'}
|

when deleting items in the container between runs. Any suggestions on
this? Also do all the integration tests pass for you? I see a few errors:

| AWSS3ContainerIntegrationLiveTest>BaseContainerIntegrationTest.deleteContainerIfEmptyWithoutContents:315 expected [true] but found [false]
AWSS3ContainerLiveTest>BaseContainerLiveTest.testPublicAccessInNonDefaultLocationWithBigBlob:112->BaseContainerLiveTest.runCreateContainerInLocation:124->BaseBlobStoreIntegrationTest.assertConsistencyAwareContainerExists:361->BaseBlobStoreIntegrationTest.assertConsistencyAware:248->BaseBlobStoreIntegrationTest.assertConsistencyAware:235 » HttpResponse
AWSS3ServiceIntegrationLiveTest>BaseServiceIntegrationTest.testAllLocations:52->BaseBlobStoreIntegrationTest.assertConsistencyAware:248->BaseBlobStoreIntegrationTest.assertConsistencyAware:235 » HttpResponse
AWSS3ServiceIntegrationLiveTest>BaseServiceIntegrationTest.testGetAssignableLocations:93 {scope=REGION, id=eu-central-1, description=eu-central-1, parent=aws-s3, iso3166Codes=[DE-HE]} ||{scope=PROVIDER, id=aws-s3, description=https://s3.amazonaws.com, iso3166Codes=[US, US-CA, US-OR, BR-SP, IE, SG, AU-NSW, JP-13]}

Tests run: 99, Failures: 4, Errors: 0, Skipped: 1
|

—
Reply to this email directly or view it on GitHub
#678 (comment).

赵金
Zhao Jin
18610722868

北京优创联动科技有限公司
北京市海淀区学清路38号金码大厦16层
100083

zhaojin0 · 2015-05-04T06:15:29Z

Hi, I impl aws s3 signer v4 chunked upload, use when put object, payload cannot repeatable.

pswvg · 2015-05-15T07:18:23Z

For our project, we took the PR, applied some minor fixes (zhaojin0#1) and backported it to stable 1.8.1. With our Integration tests, it works very well. Tested it against Frankfurt - which supports v4 only.

pswvg · 2015-05-19T09:18:29Z

apis/s3/src/main/java/org/jclouds/s3/filters/ChunkedUploadPayload.java

+        public InputStream nextElement() {
+            int bytesRead;
+            try {
+                bytesRead = inputStream.read(buffer, 0, buffer.length);


bytesRead can be less than buffer.length!

This caused problems during our tests, when the underlying inputStream only allows 8k blocks. The result was, that the pre-calculated content-length (using 64k chunks) was less than the actual send content length (using 8k chunks got from the InputStream).

That leads to HttpUrlConnection's "too many bytes written" Exception later on. ByteArrayInputStream and FileInputStream normally return the length as requested. So our integration tests didn't fail in the first place. When we live tested it, it failed because the inputStream we got from tomcat (from an upload) is using 8k blocks :(.

We fixed that on our side with a wrapping InputStream that always returns the requested length when read(buf,off,len) is invoked. Performing n additional reads from the underlying inputStream if necessary.

This way, the expected 64k blocks were send and the pre-calculated content-length matched the actual content length.

Maybe a loop must be added here to read exactly buffer.length bytes from the inputStream matching the chunkedBlockSize, like we did in our inputStream wrapper.

Can also call ByteStreams.readFully(InputStream, byte[]).

pswvg · 2015-05-20T07:25:26Z

I thought about the content-length again. The old V2 implementation wasn't able to handle unknown content-lengths. So does the V4. But I think it wouldn't be so difficult to add this. Because chunking is already implemented nicely and instead of sending the content-length of the complete payload, one could switch to transfer-encoding instead - like "Note"d here:

http://docs.aws.amazon.com/AmazonS3/latest/API/sigv4-streaming.html

That would mean:

Transfer-Encoding: chunked

should do the trick and so omitting Content-Length completely.

That would be really nice, since in our project we aren't always be able to get the complete
content length in advance (live streaming from browser clients etc.). So in the worst-case, we have
temporarily store the whole stream to disk (to get the content length) before we can send it to s3.

What do you guys think?

zhaojin0 · 2015-05-21T04:55:25Z

try Multipart Upload.
initiate Multipart Upload..temporarily store specific length part, upload all stream part......complete multipart upload...
part requires Content-Length. not all a file. but I have not tried :)

pswvg · 2015-06-22T13:25:31Z

The last time I tried Multipart Upload with jclouds aws-s3, it also forced me to specify the content length in advance. See my post here:

https://www.mail-archive.com/user@jclouds.apache.org/msg01562.html.

But that's another story.

What is the state about this PR? What is missing before it could be merged? I think the build failure was because of a checkstyle error.

gaul · 2015-06-22T23:55:26Z

@pswvg This PR still needs a thorough review -- S3 is our most popular provider and I want to ensure we do not introduce any regressions for our users.

@zhaojin0 Sorry about the delay; I will try to get to this soon.

freddy33 · 2015-09-11T00:37:03Z

Any reason for this not to have been merged?
Is it reported to 2.0 version only?
Could it be merge on 1.9 branch?

pswvg · 2015-09-11T07:06:14Z

AFAIK this is reported against 1.8.x. I've applied this PR to a 1.8.1 fork without a problem.

manzke · 2015-12-16T16:07:55Z

hey guys any progress on it? I hate that we have to use a patch version in production, but we already use it for some time and have load tested it as well. 👍

demobox · 2015-12-16T16:13:11Z

@andrewgaul Ping? Is this something you'd be able to take a look at?

gaul · 2015-12-19T00:14:13Z

@demobox I will try to make a run at this early next week when I work on some related V4 issues. Sorry for the delay!

gaul · 2016-01-13T23:33:14Z

@zhaojin0 I made some stylistic modifications, squashed the commits, and pushed to master as 8bddbb4. Thank you for your contribution and for your patience with this extraordinarily long review!

add aws s3 signature v4

8c26f78

gaul reviewed Feb 11, 2015
View reviewed changes

remote unused javadoc,code. replace URLEncoder/MessageDigest with Gua…

cced1ac

…va API

Zhao Jin added 2 commits February 20, 2015 22:22

add aws s3 temporary access signature

dec09e2

move code

43b4ad0

add testcase AWSS3BlobRequestSignerV4 for AWSS3BlobRequestSignerV4 te…

9a57455

…mporary access

gaul reviewed Mar 4, 2015
View reviewed changes

improvement, clean code and checkstyle pass

abcb355

gaul reviewed Apr 17, 2015
View reviewed changes

Zhao Jin added 3 commits April 29, 2015 16:15

fixed get HTTP header HOST NPE, get host from request endpoint.

64d66ee

move RequestAuthorizeSignatureV2 class to java file.

6c50945

aws s3 signer v4 get host from endpoint.

e345b8c

impl aws s3 signer v4 chunked upload

5984a9b

pswvg reviewed May 19, 2015
View reviewed changes

Zhao Jin added 3 commits May 19, 2015 18:03

remove Http Header Content-MD5, aws signer v4 not allowed.

2236fd4

fixed Payloads with 0 byte streams failed.

e1846e8

fixed bytesRead can be less than buffer.length!

d0ac109

fixed testcase AWSS3BlobRequestSignerV4.signGetBlob NPE

f590287

gaul closed this Jan 13, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add aws s3 signature v4 #678

add aws s3 signature v4 #678

zhaojin0 commented Feb 11, 2015

zhaojin0 commented Feb 11, 2015

gaul Feb 11, 2015

gaul commented Feb 11, 2015

gaul commented Feb 13, 2015

gaul commented Feb 19, 2015

zhaojin0 commented Feb 20, 2015

gaul commented Feb 26, 2015

zhaojin0 commented Feb 27, 2015

gaul Mar 4, 2015

zhaojin0 Mar 5, 2015

gaul commented Mar 31, 2015

zhaojin0 commented Apr 1, 2015

gaul commented Apr 17, 2015

gaul Apr 17, 2015

zhaojin0 commented Apr 19, 2015

zhaojin0 commented May 4, 2015

pswvg commented May 15, 2015

pswvg May 19, 2015

gaul May 19, 2015

pswvg commented May 20, 2015

zhaojin0 commented May 21, 2015

pswvg commented Jun 22, 2015

gaul commented Jun 22, 2015

freddy33 commented Sep 11, 2015

pswvg commented Sep 11, 2015

manzke commented Dec 16, 2015

demobox commented Dec 16, 2015

gaul commented Dec 19, 2015

gaul commented Jan 13, 2016

		import java.io.IOException;

		import static org.testng.Assert.assertEquals;

add aws s3 signature v4 #678

add aws s3 signature v4 #678

Conversation

zhaojin0 commented Feb 11, 2015

zhaojin0 commented Feb 11, 2015

gaul Feb 11, 2015

Choose a reason for hiding this comment

gaul commented Feb 11, 2015

gaul commented Feb 13, 2015

gaul commented Feb 19, 2015

zhaojin0 commented Feb 20, 2015

gaul commented Feb 26, 2015

zhaojin0 commented Feb 27, 2015

gaul Mar 4, 2015

Choose a reason for hiding this comment

zhaojin0 Mar 5, 2015

Choose a reason for hiding this comment

gaul commented Mar 31, 2015

zhaojin0 commented Apr 1, 2015

gaul commented Apr 17, 2015

gaul Apr 17, 2015

Choose a reason for hiding this comment

zhaojin0 commented Apr 19, 2015

zhaojin0 commented May 4, 2015

pswvg commented May 15, 2015

pswvg May 19, 2015

Choose a reason for hiding this comment

gaul May 19, 2015

Choose a reason for hiding this comment

pswvg commented May 20, 2015

zhaojin0 commented May 21, 2015

pswvg commented Jun 22, 2015

gaul commented Jun 22, 2015

freddy33 commented Sep 11, 2015

pswvg commented Sep 11, 2015

manzke commented Dec 16, 2015

demobox commented Dec 16, 2015

gaul commented Dec 19, 2015

gaul commented Jan 13, 2016