aws-cli icon indicating copy to clipboard operation
aws-cli copied to clipboard

Fix sse-c HeadObject headers for s3-s3 copy

Open jstastny opened this issue 2 years ago • 7 comments

Also fixing https://github.com/aws/aws-cli/issues/6012, which describes the unencrypted to encrypted copy, while we were running into the same thing with encrypted to encrypted (different key) scenario.

Description of changes: This fixes the S3 to S3 copy when using SSE-C keys for multipart object.

Without this, the head object, which is part of the copy flow, fails because it uses target sse-c keys when accessing the objects from the source.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

jstastny avatar Nov 21 '23 12:11 jstastny

@hssyoo Hey, this is important for us. What do we do to get this merged?

makuga01 avatar Jun 12 '24 09:06 makuga01

Hello, any progress on this one AWS? 👀 This is quite a blocker for us at @deepnote

FilipPyrek avatar May 01 '25 08:05 FilipPyrek

Hey @jstastny and all, sorry about the delay getting to this. A few folks pointed us at it internally.

Are you still available to proceed here? Understandable if not given the lapsed time, just let us know and we can take it over.

If proceeding:

  1. Is this patch working for you? Testing the S3 unencrypted -> S3 encrypted case (aws s3 cp s3://bucket/key s3://bucket/key --sse-c AES256 --sse-c-key <key>), this call still appears problematic to me, but succeeds when I replace that with your new _set_sse_c_request_params_with_copy_source_sse. https://github.com/aws/aws-cli/blob/196a9131eafd4bfc77ec547b3a45aefe0e986670/awscli/customizations/s3/utils.py#L527
  2. For testing:
    1. Can we duplicate this case, but without the copy_source pair? Then assert SSECustomerAlgorithm/SSECustomerKey aren't set in test_head_object https://github.com/aws/aws-cli/blob/196a9131eafd4bfc77ec547b3a45aefe0e986670/tests/unit/customizations/s3/test_utils.py#L610
    2. Can we duplicate this case (or one of its neighbors), but adjust to do the S3 unencrypted -> S3 encrypted flow? Again we can assert that HeadObject call doesn't have unexpected params https://github.com/aws/aws-cli/blob/196a9131eafd4bfc77ec547b3a45aefe0e986670/tests/functional/s3/test_cp_command.py#L661
  3. Can you run the /scripts/new-change script and add a changelog entry of type bugfix for category s3?

ashovlin avatar Jun 04 '25 21:06 ashovlin

Hello @ashovlin 👋 Nice to meet you!

To give you the context, I was the one who initiated internal pushing. 🙂

@jstastny is our ex-colleague at @deepnote so I assume this PR is no longer his priority, since he changed jobs.

At the moment we don't have much of free capacities, so if you guys were up for taking it over, it would be awesome! 😊

We can assist with any testing you need.

Is there anything you need from our side?

Adding my teammates to cc: @hc2p @mfranczel

Thanks 🙌

FilipPyrek avatar Jun 05 '25 09:06 FilipPyrek

Hi @ashovlin. Great to see that you started looking into this.

As @FilipPyrek said -- I am no longer with Deepnote and don't currently have capacity to finish the steps drafted by you.

jstastny avatar Jun 05 '25 10:06 jstastny

@jstastny no problem, happy to take it over given our delay reviewing. I'll cut a new PR and close this once ready, thanks for the head start!

ashovlin avatar Jun 06 '25 15:06 ashovlin

Put up https://github.com/aws/aws-cli/pull/9559 with a slightly different approach, and test coverage.

@FilipPyrek @hc2p @mfranczel - can you clarify what S3 -> S3 copy scenerio with different keys you're seeing fail? I was only able to reproduce with unencrypted -> encrypted. I think the fix would still apply logically, but hoping to double-check.

I tried both single object and directory, with a mix of file sizes, but not sure if there's a permutation I'm not covering.

# Both of these were already working for me
echo "\nS3 encrypted -> S3 encrypted (different keys)"
aws s3 cp s3://$bucket/ssec.txt s3://$bucket/ssec-copy-diff-key.txt --sse-c-copy-source AES256 --sse-c-copy-source-key $key_1 --sse-c AES256 --sse-c-key $key_2
aws s3 cp --recursive s3://$bucket/ssec-files s3://$bucket/ssec-files-diff-key --sse-c-copy-source AES256 --sse-c-copy-source-key $key_1 --sse-c AES256 --sse-c-key $key_2

ashovlin avatar Jun 18 '25 15:06 ashovlin

Thanks @ashovlin I will test it and let you know.

FilipPyrek avatar Jun 19 '25 09:06 FilipPyrek

Hi @ashovlin

For us the issue is: S3 encrypted -> S3 encrypted (different keys)

I tried now installing the latest AWS CLI from https://awscli.amazonaws.com/awscli-exe-linux-x86_64.zip and I tested it.

And got the error:

An error occurred (403) when calling the HeadObject operation: Forbidden
copy failed: s3://bucket-name/some-prefix/object-1 to s3://bucket-name/other-prefix/object-1

We are calling the CLI like this:

aws s3 sync s3://bucket-name/some-prefix/ s3://bucket-name/other-prefix/ --no-follow-symlinks --sse-c AES256 --sse-c-key some-key-here --sse-c-copy-source AES256 --sse-c-copy-source-key some-other-key

I see you are testing s3 cp but we use s3 sync, but I guess your fixes apply to both, right?

FilipPyrek avatar Jun 22 '25 12:06 FilipPyrek