Add SSE-C and SSE-KMS support to s3 subcommands

[0xkag]

This pull request adds SSE-C and SSE-KMS support into awscli s3 subcommands like "aws s3 cp" and "aws s3 sync".

From my commit message:

-- add SSE-C and SSE-KMS support to the aws s3 subcommands (SSE-{C,KMS} are two newer forms of S3 Server Side Encryption)

-- the added SSE-C and SSE-KMS support coexists with existing SSE-S3 support without breaking the current meaning of the --sse command line option

-- tested with (src, dst) of locals3, s3local, and s3s3, including copy from objects written as {S3,SSE-C,SSE-KMS,SSE-S3} to {S3,SSE-C,SSE-KMS,SSE-S3} both with the same and with different keys (for SSE-C and SSE-KMS)

-- add a shell script to test different permutations of SSE (integration into the Python functional tests is a TODO) and s3local, locals3, s3s3 src and dst with aws s3 cp and aws s3 sync

There are a few open TODO items worth discussing, mostly around documentation and integrating a test script into the python test suite. I may or may not have time to do these myself. Regardless of the need for these, it is fully functional for all of the above, and I want to get this on your radar since I see open issues requesting additional SSE support (#1377, #1466). (I have a need for SSE-KMS so I added it myself rather than wait. I figured I might as well add SSE-C too while I was at it.)

Kyle George

[mtdowling]

Thanks for kicking this off! We'll take a look.

[0xkag]

Sure, happy to help. Let me know if you have any questions/comments. I'm using it in dev/qa right now and it seems to be working fine, but I'd like to make it official before I use it in production.

[jamesls]

Thanks for the pull request. Probably one of the biggest things that needs to happen before we can merge this is, as you mention in your TODO file, converting the test scripts from bash into the python integration tests (for example as done here: https://github.com/aws/aws-cli/blob/develop/tests/integration/customizations/s3/test_plugin.py#L124). It would really help to have these in python.

I also have a few additional questions about this pull request, which I'll comment on in the diff.

[0xkag]

Thanks for the pull request.

Sure, you're welcome.

Probably one of the biggest things that needs to happen before we can merge this is, as you mention in your TODO file, converting the test scripts from bash into the python integration tests ... It would really help to have these in python.

Yeah, I know that tests in Python are needed before it merges (hence the TODO). I'll see what I can do. I'm a bit short on time at the moment and would need to familiarize myself with this project's test structure first (thanks for the example).

Is there any review of the feature that can be done before the test script is converted to Python?

I also have a few additional questions about this pull request, which I'll comment on in the diff.

I don't see your questions. Did you post them already?

If I do add tests do you want me to rebase and force push to my branch or just add commits?

[kyleknap]

Thanks again for the PR. So along with the conversion of the bash script tests to python tests, there are a couple of higher level things that you can work on. Mainly there a couple edge cases that you may have missed:

1. For kms encryption if the user specifies that they want to use kms, we should not force them to set signature version to s3v4 in the config file. We should detect the parameter and automatically create clients that have signature v4 enabled because you cannot use kms encryption and s3 with out signature version 4 signing.

2. Make sure that especially for multipart uploads, all of the parameters relevant to full multipart process use all of the parameters pertaining to sse. For example you may have to specify the key id for the instantiation of the multipart upload, the part upload, and the complete upload operations. I do not know these off of the top of my head, but the best way to make sure that these would work is flush out the intergration tests in python and show multipart uploads work for different types of encryption.

As to rebasing and force pushing or adding onto the commits, lets do rebase and force for now until we have all of the higher level points covered such as conversion of the bash scripts tests to python tests.

You also mentioned that you are short on time. If you need the help, we can take the PR from here and build off of your commits, but we will need to allocate the time as well because this is quite a big feature and we need to be careful to get it correct because the server side encryption logic can get tricky with the edge cases.

Let me know if you have any questions.

[0xkag]

Hi Kyle,

1. For kms encryption if the user specifies that they want to use kms, we should not force them to set signature version to s3v4 in the config file. ...

Ok, that's reasonable.

2. Make sure that especially for multipart uploads, all of the parameters relevant to full multipart process use all of the parameters pertaining to sse. ...

I considered all this in my branch, and I'm pretty sure I got all these edge cases. I went through the entire source tree and found all the S3 API calls that take SSE parameters and made sure they had the correct parameters for the context, including multipart. For example here and here. I also specifically tested with/without multipart and PUT/PUT Copy with src/dst SSE-{C,KMS,S3}. I agree the test shell script needs to be in Python, but just take a look at what it covers.

If you need the help, we can take the PR from here and build off of your commits, but we will need to allocate the time as well because this is quite a big feature

If you had to ballpark estimate how long it would take you guys in calendar time (as opposed to actual work time) to do this, what would it be?

we need to be careful to get it correct because the server side encryption logic can get tricky with the edge cases.

Yeah, tell me about it. It took a while to find all the places to change for this.

Kyle George

[rterbush]

In case you are looking for user demand for these features, please consider this my +1

Ran across this PR trying to figure out why API features were unavailable in awscli.

[kyleknap]

Hi again,

Here is an update on this PR. We want to pull in the feature from this pull request. However, like I said before it will take some time to get it merged in.

We looked at our schedule and decided it will take a few weeks before we will be able to pickup the rest of the work required to get the code at a point where we can merge this pull request and see the pull request through till it gets merged.

If you need the pull request merged sooner, it would be awesome if you can implement some of the feedback that both James and I have provided. This will at least expedite the merging of the pull request. If you do continue working on it, we will be available to continue to review updates as you provide them. If you find that you do not have the time to make updates to the pull request, that is fine as well. It will just take a few weeks till we start accomplishing the changes need to get this pull request merged.

Let me know what you think?

Thanks,
Kyle

[0xkag]

1. For kms encryption if the user specifies that they want to use kms, we should not force them to set signature version to s3v4 in the config file. ...

Ok, that's reasonable.

I went through the code and looked at the feasibility of this. It looks non-trivial because of PUT Copy, where if you don't use SigV4 and the source object is SSE-KMS then it will fail. The only way to check is to HEAD first, but that's a lot of overhead. I think requiring SigV4 across the board should be considered.

If you need the pull request merged sooner, it would be awesome if you can implement some of the feedback that both James and I have provided. ... If you find that you do not have the time to make updates to the pull request, that is fine as well. It will just take a few weeks till we start accomplishing the changes need to get this pull request merged.

I'll see what I can do. Given the above SigV4 comment which I will defer to further discussion, and my reply to you about multipart, the only other feedback was around converting the tests to python. Right?

[kyleknap]

Hey, so I just wanted to give you an update on this. So I am going to begin this week to take your commits and build on top of them to make the necessary changes to get this feature merged.

[kyleknap]

So thanks for the PR. It was very helpful in catching all of the edge cases when using SSE KMS and SSE-C. Here is the PR that I opened: #1623 that built upon your commit. I am going to close out your PR in favor of the one that I opened, but feel free to give feedback in case I missed something or you had additional things that you would like to see added.

[0xkag]

You're welcome. Thanks for working on getting it merged. I haven't looked at #1623 yet. I will take a look soon.