Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for --sse aws:kms and --sse-c #1623

Merged
merged 12 commits into from
Nov 18, 2015
Merged

Conversation

kyleknap
Copy link
Contributor

@kyleknap kyleknap commented Nov 5, 2015

So this builds on top of: #1487.

The pull request adds support for SSE using KMS and SSE-C. To support this functionality, here are the changes required:

  1. --sse can take now both AES256 and aws:kms as values. If kms is specified, the clients default to sigv4. Note that if --sse is specified it still results in AES256.
  2. --sse-kms-key-id was added to allow users to specify a key id.
  3. --sse-c and --sse-c-copy-source (for copies) was added to support SSE-C. This required the sibling parameters --sse-c-key and --sse-c-copy-source. If you are missing one part of either pair, an error gets thrown. Also --sse-c-copy-source can only be used for copies.

As to tests, the integration are pretty exhaustive. I was trying to make sure I caught all of the different edge cases in terms of interacting with S3. The unit and functional tests are focused on more on covering some of the areas that I could not or was not efficient to cover with integration tests. Let me know if there are some tests that you would like to see added.

Fixes #1377
Fixes #1466

cc @jamesls @mtdowling @rayluo @JordonPhillips

@@ -320,7 +323,11 @@ def _list_single_object(self, s3_path):
# instead use a HeadObject request.
bucket, key = find_bucket_key(s3_path)
try:
response = self._client.head_object(Bucket=bucket, Key=key)
params = {'Bucket': bucket, 'Key': key}
if self.sse_c:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well that's unfortunate. Seems like we're missing a level of abstraction here. Maybe it's worth abstracting these away as a parameter object so we don't have to change this class in the future if new params are required for head_object with new features.

Either that or I think it might be worth pulling this whole "metadata for a single object" concept out into its own object. This would isolate future changes even more.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah its really unfortunate. I am not sure about the second option. These reason we do HeadObject for a single object is that it is much cheaper than ListObjects. We are not necessarily doing it to get metadata. I think I am going to do something along the lines of the first option.

@jamesls
Copy link
Member

jamesls commented Nov 6, 2015

Overall seems ok. I would prefer that we take the time to split out all the param handling logic in FileInfo into it's own class so we can test it more easily. Seems like this is only going to get worse as we need to support more parameters.

@kyleknap
Copy link
Contributor Author

Alright, I think I addressed your feedback @jamesls. I split up the parameter mapping into its own utility class where the public methods map cli parameters to request parameters depending on what operation is being ran.

@0xkag
Copy link
Contributor

0xkag commented Nov 10, 2015

Kyle,

Just took a brief look at your changes.

In #1487 I required that someone globally set signature s3v4 for KMS to work. In this request I see where you set signature s3v4 if someone has explicitly specified SSE w/ KMS. That's certainly better, but it won't handle the case where someone is copying an object where the source is encrypted with SSE-KMS and the destination is not SSE-KMS. As I said on #1487:

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.

I'm not saying that HEAD'ing every object before copy should be done, but I think that it at least needs to be documented or the failure mode handled with an appropriate error message. I only saw one test that explicitly references "s3v4", and I don't think it covers this case.

@kyleknap
Copy link
Contributor Author

That is true. I do not think that we will be able to HEAD the object to determine if it is kms encrypted before doing a copy because that would adding an extra call that costs users time and money. I think the best option is to improve the error message on how to set sigv4 when you get kms/sigv4 errors from s3. This option will be nice as well because we would be able to apply it to both s3 and s3api commands.

@kyleknap kyleknap added the pr:needs-review This PR needs a review from a Member. label Nov 13, 2015
@@ -116,7 +116,7 @@ class FileGenerator(object):
``FileInfo`` objects to send to a ``Comparator`` or ``S3Handler``.
"""
def __init__(self, client, operation_name, follow_symlinks=True,
page_size=None, result_queue=None):
page_size=None, result_queue=None, request_parameters=None):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does it make sense to move page_size in the request_parameters dict with an entry for ListObjects?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe? If I add it, I will probably will want to break the BucketLister interface to use request_parameters in lieu of the current arguments it takes. How about I add page_size to request_parameters whenever we decide that we need to use request_parameters because of a needed extra request parameter for ListObjects?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok sounds good.

@jamesls
Copy link
Member

jamesls commented Nov 18, 2015

:shipit: Had some small feedback, otherwise looks good. The RequestMapper stuff makes the code much easier to follow.

0xkag and others added 12 commits November 18, 2015 10:52
-- 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
Integration tests pass as well
We cannot test it with integration tests because we cannot delete the kms
key so just do it functionally.
Also update code and tests such that unit tests pass
Moved logic out of FileInfo into a utility class to better keep track
of all of the different edge cases when mapping CLI params to request params
for a specific operation
@kyleknap
Copy link
Contributor Author

Update PR with docstring. Merging.

kyleknap added a commit that referenced this pull request Nov 18, 2015
Add support for --sse aws:kms and --sse-c
@kyleknap kyleknap merged commit f1ed24b into aws:develop Nov 18, 2015
@kyleknap kyleknap deleted the kms-ssec branch November 18, 2015 18:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr:needs-review This PR needs a review from a Member.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants