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

✨ allow to set the header to AES256 when uploading files to S3 #172

Closed
wants to merge 1 commit into from
Closed

✨ allow to set the header to AES256 when uploading files to S3 #172

wants to merge 1 commit into from

Conversation

drieshooghe
Copy link

Description

Allows to specify AES256 (the default S3 encryption) as the server-side encryption method for storing objects in S3.

Considerations

Implemented this as this has become a common security practice to prevent the upload of unencrypted objects to S3.

📙 How to prevent uploads of unencrypted objects to amazon s3

Does not add support for aws:kms encryption.

Related issues

Ability to use server side encryption #154

@timkelty
Copy link
Contributor

This looks great, @drieshooghe – just doing some testing on my end and will get it merged.

@drieshooghe
Copy link
Author

@timkelty any news on this?

@timkelty
Copy link
Contributor

timkelty commented Mar 8, 2024

@timkelty any news on this?

Glad you asked, working on it today! Should be merged shortly.

@timkelty timkelty self-requested a review March 8, 2024 17:43
@timkelty
Copy link
Contributor

timkelty commented Mar 8, 2024

@drieshooghe tested and everything seems to be working as expected.

However, I'm a bit dubious about adding this as an option without another encryption method other than AES256, as all S3 uploads are encrypted with this by default. (the article you posted is from 2016, but since January 5, 2023, all s3 objects have default AES256 encryption.

Including this as an option is likely to confuse people into thinking they're opting into something more secure, when they really aren't changing the behavior from the default.

The only thing explicitly sending the header/param gets you is the ability to restrict things with a bucket policy, which isn't really meaningful if there isn't another encryption method option (kms).

I'm curious to hear your use-case, or if I'm missing something, though.

@drieshooghe
Copy link
Author

@timkelty you're right, it purely a fix for bucket policy enforcements. Our buckets all have a requirement that the ServerSideEncryption header is present and is set to AES256 when doing uploads to S3.

The copy is indeed a bit misleading, it should explain that this doesn't enable SSE but only adds the header.
Would changing it to something more descriptive about the purpose of the setting help?

@timkelty
Copy link
Contributor

@drieshooghe well, I'm thinking maybe we should just ditch the setting all together, and just change the behavior to send the encryption header, since that aligns with the default, but allows bucket policies like you want.

I feel like that might make more sense unless we support aws:kms or something other than the default.

If that makes sense to you, let me know and I can make that change.

@drieshooghe
Copy link
Author

@timkelty that makes sense 👍🏻

@timkelty
Copy link
Contributor

@drieshooghe in that case I'm going to close this in favor of #174
I'll keep this around in case we support aws:kms in the future.

craftcms/aws-s3:2.2.0 is out with this change.

Thanks, @drieshooghe!

@timkelty timkelty closed this Mar 13, 2024
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.

2 participants