search

craftcms / aws-s3

Amazon S3 volume type for Craft CMS.
https://plugins.craftcms.com/aws-s3
MIT License
61 stars 28 forks source link

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

Closed drieshooghe closed 6 months ago

drieshooghe commented 8 months ago

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 commented 7 months ago

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

drieshooghe commented 6 months ago

@timkelty any news on this?

timkelty commented 6 months ago

@timkelty any news on this?

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

timkelty commented 6 months ago

@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 commented 6 months ago

@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 commented 6 months ago

@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 commented 6 months ago

@timkelty that makes sense 👍🏻

timkelty commented 6 months ago

@drieshooghe in that case I'm going to close this in favor of https://github.com/craftcms/aws-s3/pull/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!