feat: S3 server-side encryption #5402
Conversation
object_store/CONTRIBUTING.md
Outdated
| #### Encryption tests | ||
|
|
||
| To run integration tests with encryption, you can set the following environment variables: | ||
|
|
||
| ``` | ||
| export AWS_SERVER_SIDE_ENCRYPTION=aws:kms | ||
| export AWS_SSE_KMS_KEY_ID=<some-key-id> | ||
| export AWS_SSE_BUCKET_KEY=false | ||
| ``` | ||
|
|
||
| As well as: | ||
|
|
||
| ``` | ||
| unset AWS_SSE_BUCKET_KEY | ||
| export AWS_SERVER_SIDE_ENCRYPTION=aws:kms:dsse | ||
| export AWS_SSE_KMS_KEY_ID=<some-key-id> | ||
| ``` |
There was a problem hiding this comment.
✅ I have manually run the encryption tests against AWS S3.
wjones127
changed the title
| @@ -523,7 +535,7 @@ impl S3Client { | |||
| let part = (part_idx + 1).to_string(); | |||
|
|
|||
| let response = self | |||
| .put_request(path, data) | |||
| .put_request(path, data, false) | |||
There was a problem hiding this comment.
This doesn't appear to match the docs https://docs.aws.amazon.com/AmazonS3/latest/API/API_CreateMultipartUpload.html which would suggest PutPart must include the same headers
There was a problem hiding this comment.
You are right that CreateMultipartUpload requires the headers, but UploadPart does not accept KMS headers. I actually hit a 400 error from S3 when I was passing them earlier. If you look at the the headers for UploadPart, you'll notice the encryption headers it accepts are only the SSE-C ones, not the KMS ones.
| let mut headers = HeaderMap::new(); | ||
| headers.insert( | ||
| "x-amz-server-side-encryption", | ||
| HeaderValue::from_static(encryption_type.into()), |
There was a problem hiding this comment.
HeaderValue has an API called set_sensitive. We can adopt this to prevent accidental leakage of our header value.
There was a problem hiding this comment.
Okay. Given that, we can just derive Debug for S3EncryptionHeaders. I've made a note that for SSE-C we should use set_sensitive. The current headers are not sensitive.
| ); | ||
| } | ||
| if let Some(bucket_key_enabled) = bucket_key_enabled { | ||
| headers.insert( |
There was a problem hiding this comment.
Maybe we can only insert this key while bucket_key_enabled is true? This can save one extra long header.
There was a problem hiding this comment.
Setting it to false and not setting it at all have different meanings. If you don't set it, then it uses the default setting at the bucket level. But if you set it to false for the specific object, then it will definitely not use bucket key for that particular object. But most of the time, I don't think people will set this header, so it won't be added.
Good to know. I hope you have a good holiday. Feel free to ignore this PR then. 😄 |
wjones127
force-pushed
the
s3-encryption
branch
from
2f61ab5 to
0cc547c
Compare
* feat: s3 server-side encryption * cleanup * fix instructions * also run with encryption in CI * feedback * fix clippy
Which issue does this PR close?
Closes #5087.
Rationale for this change
What changes are included in this PR?
Adds support for
SSE-KMSandDSSE-KMSencryption with Amazon S3.SSE-C(user-provided keys) is left for a future PR.Users can configure these with the environment variables / config values
aws_server_side_encryption,aws_sse_kms_key_id,aws_sse_bucket_key_enabled. I chose these since they seem to parallel the available settings in AWS CLI.In addition there are
with_sse_kms_encryption()andwith_dsse_kms_encryption()methods on the builder.Are there any user-facing changes?
Adds new APIs.