You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@jclouds.apache.org by Andrew Gaul <no...@github.com> on 2014/04/11 08:20:05 UTC
[jclouds] JCLOUDS-533: Amazon S3 server-side encryption (#344)
This commit mimics the implementation of storage classes, hooking into
AWSS3PutObjectOptions.
You can merge this Pull Request by running:
git pull https://github.com/andrewgaul/jclouds s3-server-side-encryption
Or you can view, comment on it, or merge it online at:
https://github.com/jclouds/jclouds/pull/344
-- Commit Summary --
* JCLOUDS-533: Amazon S3 server-side encryption
-- File Changes --
M apis/s3/src/main/java/org/jclouds/s3/domain/MutableObjectMetadata.java (3)
M apis/s3/src/main/java/org/jclouds/s3/domain/ObjectMetadata.java (6)
M apis/s3/src/main/java/org/jclouds/s3/domain/internal/BucketListObjectMetadata.java (13)
M apis/s3/src/main/java/org/jclouds/s3/domain/internal/CopyObjectResult.java (5)
M apis/s3/src/main/java/org/jclouds/s3/domain/internal/MutableObjectMetadataImpl.java (25)
M apis/s3/src/main/java/org/jclouds/s3/functions/ParseObjectMetadataFromHeaders.java (8)
M apis/s3/src/main/java/org/jclouds/s3/reference/S3Headers.java (3)
M providers/aws-s3/src/main/java/org/jclouds/aws/s3/blobstore/options/AWSS3PutObjectOptions.java (18)
M providers/aws-s3/src/test/java/org/jclouds/aws/s3/AWSS3ClientExpectTest.java (36)
-- Patch Links --
https://github.com/jclouds/jclouds/pull/344.patch
https://github.com/jclouds/jclouds/pull/344.diff
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/344
Re: [jclouds] JCLOUDS-533: Amazon S3 server-side encryption (#344)
Posted by Andrew Phillips <no...@github.com>.
> @@ -35,6 +35,10 @@
> */
> public interface ObjectMetadata extends Comparable<ObjectMetadata> {
>
> + public enum ServerSideEncryption {
> + NONE, AES256
> if we know it is NONE we should say so and if it is unknown we should say so as well.
Fair point. How about having an enum constant `UNKNOWN` rather than saying `null` = "unknown" and "NONE" = "known, none"?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/344/files#r13033625
Re: [jclouds] JCLOUDS-533: Amazon S3 server-side encryption (#344)
Posted by Andrew Phillips <no...@github.com>.
> this.contentMetadata = new BaseMutableContentMetadata();
> }
>
> public MutableObjectMetadataImpl(ObjectMetadata from) {
> - this.storageClass = StorageClass.STANDARD;
> + this.storageClass = from.getStorageClass();
Was this a different bug? I'm guessing this change is unrelated to this PR?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/344/files#r11540693
Re: [jclouds/jclouds] JCLOUDS-533: Amazon S3 server-side encryption
(#344)
Posted by Andrew Gaul <no...@github.com>.
This patch did not merge since we did not look at the interactions with SSE-C.
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/344#issuecomment-316160130
Re: [jclouds/jclouds] JCLOUDS-533: Amazon S3 server-side encryption
(#344)
Posted by Andrew Gaul <no...@github.com>.
jclouds lacks support for server-side encryption. We will gladly accept pull requests to complete this work, including building on top of this one, but someone needs to look at the interactions with SSE-C so that we do not preclude implementing it in the future.
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/344#issuecomment-340539829
Re: [jclouds] JCLOUDS-533: Amazon S3 server-side encryption (#344)
Posted by Andrew Phillips <no...@github.com>.
> + HttpRequest.builder()
> + .method("PUT")
> + .endpoint("https://test.s3-eu-west-1.amazonaws.com/test")
> + .addHeader("Expect", "100-continue")
> + .addHeader(SERVER_SIDE_ENCRYPTION, "AES256")
> + .addHeader("Host", "test.s3-eu-west-1.amazonaws.com")
> + .addHeader("Date", CONSTANT_DATE)
> + .addHeader("Authorization", "AWS identity:wTWjt+DikN+hL0DBLHuWWVUloJk=")
> + .payload("content").build(),
> + HttpResponse.builder()
> + .statusCode(200)
> + .addHeader("x-amz-id-2", "w0rL+9fALQiCOToesVQefs8WalIgn+ZhMD7hHMKYud/xv7MyKkAWQOtFNEfK97Ri")
> + .addHeader("x-amz-request-id", "7A84C3CD4437A4C0")
> + .addHeader("Date", CONSTANT_DATE)
> + .addHeader("ETag", "437b930db84b8079c2dd804a71936b5f")
> + .addHeader("Server", "AmazonS3").build()
> I will add the expected response header
Thanks!
> but this test cannot test its presence since S3Client.putObject returns a boolean instead of the object
> metadata.
Ah, OK. Hm...so we have _no_ tests currently that cover that unmarshalling code? What API could would a user make to get the metadata back...a GET, I guess? Do we need to add some assertions to _that/those_ tests to figure out whether our new unmarshalling code works?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/344/files#r13033662
Re: [jclouds/jclouds] JCLOUDS-533: Amazon S3 server-side encryption
(#344)
Posted by deyanTG <no...@github.com>.
So, as I understand it is not possible to use aws s3 server side encryption with jcloud api? If that is not the case how one is supposed to trigger server side encryption?
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/344#issuecomment-340473032
Re: [jclouds] JCLOUDS-533: Amazon S3 server-side encryption (#344)
Posted by Andrew Phillips <no...@github.com>.
> @@ -84,6 +86,12 @@ public MutableObjectMetadata apply(HttpResponse from) {
> // amz has an etag, but matches syntax for usermetadata
> to.getUserMetadata().remove("object-etag");
> to.setCacheControl(from.getFirstHeaderOrNull(HttpHeaders.CACHE_CONTROL));
> + String serverSideEncryption = from.getFirstHeaderOrNull(S3Headers.SERVER_SIDE_ENCRYPTION);
I'm guessing we're sure this returns NONE or AES256 as the only two possibilities?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/344/files#r11540729
Re: [jclouds] JCLOUDS-533: Amazon S3 server-side encryption (#344)
Posted by Andrew Phillips <no...@github.com>.
> this.contentMetadata = new BaseMutableContentMetadata();
> }
>
> public MutableObjectMetadataImpl(ObjectMetadata from) {
> - this.storageClass = StorageClass.STANDARD;
> + this.storageClass = from.getStorageClass();
Ah, OK...thanks for explaining.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/344/files#r13033633
Re: [jclouds] JCLOUDS-533: Amazon S3 server-side encryption (#344)
Posted by Andrew Phillips <no...@github.com>.
> jclouds-pull-requests #749 UNSTABLE
Spurious [test failure](https://jclouds.ci.cloudbees.com/job/jclouds-pull-requests/749/testReport/)
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/344#issuecomment-40226195
Re: [jclouds] JCLOUDS-533: Amazon S3 server-side encryption (#344)
Posted by "Christian S." <no...@github.com>.
@andrewgaul server side encryption interacts not at all with user-provided encryption keys. The user provided encryption keys feature is only implemented in the AWS client SDKs. It could even be combined with each other.
User provided encryption keys could even be a blobstore-generic feature which works for all blobstores (which treat data as opaque).
In short it works by encrypting it locally and adding one piece of user metadata (the checksum of the key) to identify the key later.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/344#issuecomment-77545099
Re: [jclouds] JCLOUDS-533: Amazon S3 server-side encryption (#344)
Posted by Andrew Phillips <no...@github.com>.
> @@ -32,6 +32,14 @@
> public static class Builder {
>
> /**
> + * @see AWSS3PutObjectOptions#serverSideEncryption
> + */
> + public static AWSS3PutObjectOptions serverSideEncryption(ObjectMetadata.ServerSideEncryption serverSideEncryption) {
> but let's codify the preferred style via Checkstyle instead of inconsistently applying it in reviews.
Agreed. Fine with leaving this as-is for now.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/344/files#r13033642
Re: [jclouds] JCLOUDS-533: Amazon S3 server-side encryption (#344)
Posted by Andrew Gaul <no...@github.com>.
> @@ -84,6 +86,12 @@ public MutableObjectMetadata apply(HttpResponse from) {
> // amz has an etag, but matches syntax for usermetadata
> to.getUserMetadata().remove("object-etag");
> to.setCacheControl(from.getFirstHeaderOrNull(HttpHeaders.CACHE_CONTROL));
> + String serverSideEncryption = from.getFirstHeaderOrNull(S3Headers.SERVER_SIDE_ENCRYPTION);
This is the only possibility I know of today, although S3 clones might implement their own mechanisms.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/344/files#r13023551
Re: [jclouds] JCLOUDS-533: Amazon S3 server-side encryption (#344)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-java-7-pull-requests #1219](https://jclouds.ci.cloudbees.com/job/jclouds-java-7-pull-requests/1219/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/344#issuecomment-40177376
Re: [jclouds] JCLOUDS-533: Amazon S3 server-side encryption (#344)
Posted by Andrew Gaul <no...@github.com>.
We should not commit this until we investigate how it interacts with user-provided encryption keys: https://aws.amazon.com/blogs/aws/s3-encryption-with-your-keys/
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/344#issuecomment-49676357
Re: [jclouds] JCLOUDS-533: Amazon S3 server-side encryption (#344)
Posted by Andrew Phillips <no...@github.com>.
> @@ -76,6 +76,11 @@ public CanonicalUser getOwner() {
> return null;
> }
>
> + @Override
> + public ServerSideEncryption getServerSideEncryption() {
> + return null;
Should this be `null` or `NONE`?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/344/files#r11540678
Re: [jclouds] JCLOUDS-533: Amazon S3 server-side encryption (#344)
Posted by Andrew Phillips <no...@github.com>.
> @@ -84,6 +86,12 @@ public MutableObjectMetadata apply(HttpResponse from) {
> // amz has an etag, but matches syntax for usermetadata
> to.getUserMetadata().remove("object-etag");
> to.setCacheControl(from.getFirstHeaderOrNull(HttpHeaders.CACHE_CONTROL));
> + String serverSideEncryption = from.getFirstHeaderOrNull(S3Headers.SERVER_SIDE_ENCRYPTION);
> + if (serverSideEncryption != null) {
> + to.setServerSideEncryption(ObjectMetadata.ServerSideEncryption.valueOf(serverSideEncryption));
> + }
> + // TODO: headObject does not populate STORAGE_CLASS. Reference:
> + // http://docs.aws.amazon.com/AmazonS3/latest/API/RESTObjectHEAD.html
Should we have a JIRA issue for this?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/344/files#r13033637
Re: [jclouds] JCLOUDS-533: Amazon S3 server-side encryption (#344)
Posted by Andrew Gaul <no...@github.com>.
I do not see many actionable comments but I will clean up the storage class code I modeled this pull request on, rebase, and resubmit for review.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/344#issuecomment-44072011
Re: [jclouds] JCLOUDS-533: Amazon S3 server-side encryption (#344)
Posted by Andrew Phillips <no...@github.com>.
> @@ -76,6 +76,11 @@ public CanonicalUser getOwner() {
> return null;
> }
>
> + @Override
> + public ServerSideEncryption getServerSideEncryption() {
> + return null;
> We do not fully consume the response from copy object, which includes the server-side encryption setting and
> the storage class:
I guess the obvious question would be "is there some way we can do that, then"? But, in any case, that would probably be a different PR.
As regards the return value: see the above comment about `UNKNOWN` vs. `null`?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/344/files#r13033630
Re: [jclouds] JCLOUDS-533: Amazon S3 server-side encryption (#344)
Posted by Andrew Gaul <no...@github.com>.
> @@ -35,6 +35,10 @@
> */
> public interface ObjectMetadata extends Comparable<ObjectMetadata> {
>
> + public enum ServerSideEncryption {
> + NONE, AES256
```Optional``` and ```null``` provide less clarity to the caller about the encryption type; if we know it is ```NONE``` we should say so and if it is unknown we should say so as well. We can use ```Optional``` for situations where we do not know and probably should for ```StorageClass``` in some situations (see TODO).
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/344/files#r13023281
Re: [jclouds] JCLOUDS-533: Amazon S3 server-side encryption (#344)
Posted by BuildHive <no...@github.com>.
[jclouds ยป jclouds #1017](https://buildhive.cloudbees.com/job/jclouds/job/jclouds/1017/) 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/344#issuecomment-40176746
Re: [jclouds] JCLOUDS-533: Amazon S3 server-side encryption (#344)
Posted by Andrew Gaul <no...@github.com>.
> @@ -32,6 +32,14 @@
> public static class Builder {
>
> /**
> + * @see AWSS3PutObjectOptions#serverSideEncryption
> + */
> + public static AWSS3PutObjectOptions serverSideEncryption(ObjectMetadata.ServerSideEncryption serverSideEncryption) {
I have no opinion on static vs. non-static imports, but let's codify the preferred style via Checkstyle instead of inconsistently applying it in reviews.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/344/files#r13023543
Re: [jclouds/jclouds] JCLOUDS-533: Amazon S3 server-side encryption
(#344)
Posted by Udaya Kumar <no...@github.com>.
Has this fix made into any 2.0.x? or is it planned to go into 2.1?
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/344#issuecomment-316158047
Re: [jclouds] JCLOUDS-533: Amazon S3 server-side encryption (#344)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-pull-requests #749](https://jclouds.ci.cloudbees.com/job/jclouds-pull-requests/749/) UNSTABLE
Looks like there's a problem with this pull request
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/344#issuecomment-40177063
Re: [jclouds] JCLOUDS-533: Amazon S3 server-side encryption (#344)
Posted by Andrew Phillips <no...@github.com>.
> @@ -35,6 +35,10 @@
> */
> public interface ObjectMetadata extends Comparable<ObjectMetadata> {
>
> + public enum ServerSideEncryption {
> + NONE, AES256
What do we need `NONE` for, actually? Could we use `null` for that instead? After all, `NONE` isn't what comes **back** from S3 (if I'm understanding this correctly) if there is no encryption - the header simply isn't present, so getFirstHeaderOrNull returns `null`.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/344/files#r11541085
Re: [jclouds] JCLOUDS-533: Amazon S3 server-side encryption (#344)
Posted by Andrew Gaul <no...@github.com>.
> + HttpRequest.builder()
> + .method("PUT")
> + .endpoint("https://test.s3-eu-west-1.amazonaws.com/test")
> + .addHeader("Expect", "100-continue")
> + .addHeader(SERVER_SIDE_ENCRYPTION, "AES256")
> + .addHeader("Host", "test.s3-eu-west-1.amazonaws.com")
> + .addHeader("Date", CONSTANT_DATE)
> + .addHeader("Authorization", "AWS identity:wTWjt+DikN+hL0DBLHuWWVUloJk=")
> + .payload("content").build(),
> + HttpResponse.builder()
> + .statusCode(200)
> + .addHeader("x-amz-id-2", "w0rL+9fALQiCOToesVQefs8WalIgn+ZhMD7hHMKYud/xv7MyKkAWQOtFNEfK97Ri")
> + .addHeader("x-amz-request-id", "7A84C3CD4437A4C0")
> + .addHeader("Date", CONSTANT_DATE)
> + .addHeader("ETag", "437b930db84b8079c2dd804a71936b5f")
> + .addHeader("Server", "AmazonS3").build()
Unfortunately this test only captures building and correctly marshalling the request. I will add the expected response header but this test cannot test its presence since ```S3Client.putObject``` returns a boolean instead of the object metadata.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/344/files#r13023512
Re: [jclouds] JCLOUDS-533: Amazon S3 server-side encryption (#344)
Posted by Andrew Phillips <no...@github.com>.
> @@ -84,6 +86,12 @@ public MutableObjectMetadata apply(HttpResponse from) {
> // amz has an etag, but matches syntax for usermetadata
> to.getUserMetadata().remove("object-etag");
> to.setCacheControl(from.getFirstHeaderOrNull(HttpHeaders.CACHE_CONTROL));
> + String serverSideEncryption = from.getFirstHeaderOrNull(S3Headers.SERVER_SIDE_ENCRYPTION);
> although S3 clones might implement their own mechanisms.
In that case, would we need a try...catch around the `valueOf` below to handle different return values?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/344/files#r13033640
Re: [jclouds] JCLOUDS-533: Amazon S3 server-side encryption (#344)
Posted by Andrew Gaul <no...@github.com>.
> @@ -76,6 +76,11 @@ public CanonicalUser getOwner() {
> return null;
> }
>
> + @Override
> + public ServerSideEncryption getServerSideEncryption() {
> + return null;
We do not fully consume the response from copy object, which includes the server-side encryption setting and the storage class:
http://docs.aws.amazon.com/AmazonS3/latest/API/RESTObjectCOPY.html
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/344/files#r13023349
Re: [jclouds] JCLOUDS-533: Amazon S3 server-side encryption (#344)
Posted by Andrew Phillips <no...@github.com>.
> @@ -35,6 +35,10 @@
> */
> public interface ObjectMetadata extends Comparable<ObjectMetadata> {
>
> + public enum ServerSideEncryption {
> + NONE, AES256
Or use an `Optional`?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/344/files#r11559898
Re: [jclouds] JCLOUDS-533: Amazon S3 server-side encryption (#344)
Posted by Andrew Phillips <no...@github.com>.
> @@ -32,6 +32,14 @@
> public static class Builder {
>
> /**
> + * @see AWSS3PutObjectOptions#serverSideEncryption
> + */
> + public static AWSS3PutObjectOptions serverSideEncryption(ObjectMetadata.ServerSideEncryption serverSideEncryption) {
I'd say "static import ObjectMetadata.ServerSideEncryption?", but then I guess we should do that for OM.StorageClass, too.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/344/files#r11540782
Re: [jclouds] JCLOUDS-533: Amazon S3 server-side encryption (#344)
Posted by Everett Toews <no...@github.com>.
Closed #344.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/344#event-191653829
Re: [jclouds] JCLOUDS-533: Amazon S3 server-side encryption (#344)
Posted by Andrew Phillips <no...@github.com>.
> + HttpRequest.builder()
> + .method("PUT")
> + .endpoint("https://test.s3-eu-west-1.amazonaws.com/test")
> + .addHeader("Expect", "100-continue")
> + .addHeader(SERVER_SIDE_ENCRYPTION, "AES256")
> + .addHeader("Host", "test.s3-eu-west-1.amazonaws.com")
> + .addHeader("Date", CONSTANT_DATE)
> + .addHeader("Authorization", "AWS identity:wTWjt+DikN+hL0DBLHuWWVUloJk=")
> + .payload("content").build(),
> + HttpResponse.builder()
> + .statusCode(200)
> + .addHeader("x-amz-id-2", "w0rL+9fALQiCOToesVQefs8WalIgn+ZhMD7hHMKYud/xv7MyKkAWQOtFNEfK97Ri")
> + .addHeader("x-amz-request-id", "7A84C3CD4437A4C0")
> + .addHeader("Date", CONSTANT_DATE)
> + .addHeader("ETag", "437b930db84b8079c2dd804a71936b5f")
> + .addHeader("Server", "AmazonS3").build()
Doesn't the response acknowledge the request by [including the header in the response](http://docs.aws.amazon.com/AmazonS3/latest/dev/SSEUsingRESTAPI.html)? We have some `getFirstHeaderOrNull` parsing logic to **extract** that header from the response, if I understand this PR correctly. Do we need a test to validate that?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/344/files#r11541192
Re: [jclouds] JCLOUDS-533: Amazon S3 server-side encryption (#344)
Posted by Everett Toews <no...@github.com>.
It's release time in jclouds and that means we like to do a little house cleaning by sweeping out the pull request queue. This PR hasn't been touched in over 6 months. Please give us a status update.
If we don't hear anything by the end of the week, we'll close this pull request.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/344#issuecomment-59872320
Re: [jclouds] JCLOUDS-533: Amazon S3 server-side encryption (#344)
Posted by Andrew Gaul <no...@github.com>.
> this.contentMetadata = new BaseMutableContentMetadata();
> }
>
> public MutableObjectMetadataImpl(ObjectMetadata from) {
> - this.storageClass = StorageClass.STANDARD;
> + this.storageClass = from.getStorageClass();
The storage class logic looks like it needs some love; let me see if I can clean this up.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/344/files#r13023359