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

Do not include sse configuration of source object in that of the destination object #3016

Merged
merged 5 commits into from
May 5, 2024

Conversation

matchbookmac
Copy link
Contributor

@matchbookmac matchbookmac commented Apr 30, 2024

When copying objects using Aws::S3::Object.copy_to(..., multipart_copy: true), the ObjectMultipartCopier was inferring the metadata of the destination object from that of the source object. This is fine for content_type, content_length, etc.; however, the ssekms_key_id and server_side_encryption attributes were also copied. This causes the destination object to use the kms key of the source, which is not usually appropriate if the destination bucket has a different default encryption than the source. This is especially true when copying from one account to the other. This change is in line with the behavior of Aws::S3::Client.copy_object which does not copy the source object encryption.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

When copying objects using `Aws::S3::Object.copy_to(..., multipart_copy: true)`,
the `ObjectMultipartCopier` was inferring the metadata of the destination object
from that of the source object. This is fine for `content_type`, `content_length`, etc.;
however, the `ssekms_key_id` and `server_side_encryption` attributes were also copied.
This causes the destination object to use the kms key of the source, which is not usually
appropriate if the destination bucket has a different default encryption than the source.
This is especially true when copying from one account to the other.
Copy link
Contributor

@alextwoods alextwoods left a comment

Choose a reason for hiding this comment

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

Thanks for submitting this! The logic looks good - can you add a test case for this?

@matchbookmac
Copy link
Contributor Author

Sure thing!

@matchbookmac
Copy link
Contributor Author

@alextwoods added

@@ -257,8 +257,7 @@ module S3
allow(client).to receive(:head_object).with({
bucket: 'source-bucket',
key: 'source key'
}).and_return(Types::HeadObjectOutput.new(
content_length: size, content_type: 'ContentType'))
}).and_return(Types::HeadObjectOutput.new(content_length: size, content_type: 'ContentType'))
Copy link
Contributor

Choose a reason for hiding this comment

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

Yuck.. I'm not sure why this is here. Do you mind changing all of these to stub_data?

@matchbookmac
Copy link
Contributor Author

@mullermp gladly

Copy link
Contributor

@mullermp mullermp left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution (and clean up :D)

@mullermp mullermp merged commit c59f722 into aws:version-3 May 5, 2024
22 of 27 checks passed
@matchbookmac
Copy link
Contributor Author

Sure thing! Thanks for the merge.

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.

None yet

3 participants