Skip to content

Conversation

jstastny
Copy link

Also fixing #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.

@makuga01
Copy link

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

@FilipPyrek
Copy link

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

@ashovlin
Copy link
Member

ashovlin commented Jun 4, 2025

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.
    cls._set_sse_c_request_params(request_params, cli_params)
  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
      class TestRequestParamsMapperSSE(unittest.TestCase):
    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
      def test_cp_with_sse_c_flag(self):
  3. Can you run the /scripts/new-change script and add a changelog entry of type bugfix for category s3?

@FilipPyrek
Copy link

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 🙌

@jstastny
Copy link
Author

jstastny commented Jun 5, 2025

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.

@ashovlin
Copy link
Member

ashovlin commented Jun 6, 2025

@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
Copy link
Member

Put up #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

@FilipPyrek
Copy link

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

@FilipPyrek
Copy link

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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants