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.

Sign up for GitHub

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

Jump to bottom

aws s3 server_side_encryption configuration when upload object to s3 #5400

Open
wants to merge 23 commits into
base: main
Choose a base branch
from
Open

aws s3 server_side_encryption configuration when upload object to s3 #5400

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
78ec724
add aws s3 sse
samoii Sep 5, 2024
183de31
Merge branch 'main' of github.com:samoii/quickwit into aws-s3-sse
samoii Sep 7, 2024
716d39b
Merge branch 'main' of github.com:samoii/quickwit into aws-s3-sse
samoii Sep 10, 2024
f28677f
Merge branch 'main' of github.com:samoii/quickwit into aws-s3-sse
samoii Sep 12, 2024
42045be
server_side_encryption use enum varialble
samoii Sep 12, 2024
38af88b
remove unused
samoii Sep 12, 2024
4aad3c9
Merge pull request #1 from samoii/fix-s3-sse
samoii Sep 12, 2024
b8e5b78
add kms key id variable
samoii Sep 13, 2024
4e39153
Merge branch 'main' of github.com:samoii/quickwit into fix-s3-sse
samoii Sep 14, 2024
6319147
add kms key id
samoii Sep 14, 2024
21d8add
Merge pull request #2 from samoii/fix-s3-sse
samoii Sep 14, 2024
e6f0c16
function apply sse
samoii Sep 14, 2024
5147505
add sse to multipart upload and edit shown log
samoii Sep 14, 2024
ad68a93
Merge branch 'main' of github.com:samoii/quickwit into fix-s3-sse
samoii Sep 20, 2024
2996aee
fix lint
samoii Sep 20, 2024
c5d50d2
fix lint
samoii Sep 20, 2024
202c6e9
fix lint
samoii Sep 20, 2024
5730b1e
fix clippy
samoii Sep 21, 2024
4243a5f
fix rustfmt
samoii Sep 21, 2024
d9c4b8b
Merge pull request #3 from samoii/fix-s3-sse
samoii Sep 21, 2024
6ef96ad
Merge branch 'main' of github.com:samoii/quickwit into aws-s3-sse
samoii Sep 29, 2024
6529325
Merge branch 'main' of github.com:samoii/quickwit into aws-s3-sse
samoii Oct 3, 2024
660db5a
Merge branch 'main' of github.com:samoii/quickwit into aws-s3-sse
samoii Oct 5, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions quickwit/quickwit-config/src/storage_config.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -333,6 +333,8 @@ pub struct S3StorageConfig {
pub disable_multi_object_delete: bool,
#[serde(default)]
pub disable_multipart_upload: bool,
#[serde(default)]
pub server_side_encryption: Option<String>,
samoii marked this conversation as resolved.
Show resolved Hide resolved
}

impl S3StorageConfig {
Expand Down Expand Up @@ -393,6 +395,7 @@ impl fmt::Debug for S3StorageConfig {
"disable_multi_object_delete",
&self.disable_multi_object_delete,
)
.field("server_side_encryption", &self.server_side_encryption)
.finish()
}
}
Expand Down
23 changes: 20 additions & 3 deletions quickwit/quickwit-storage/src/object_storage/s3_compatible_storage.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ use aws_sdk_s3::operation::delete_objects::DeleteObjectsOutput;
use aws_sdk_s3::operation::get_object::{GetObjectError, GetObjectOutput};
use aws_sdk_s3::primitives::ByteStream;
use aws_sdk_s3::types::builders::ObjectIdentifierBuilder;
use aws_sdk_s3::types::{CompletedMultipartUpload, CompletedPart, Delete, ObjectIdentifier};
use aws_sdk_s3::types::{CompletedMultipartUpload, CompletedPart, Delete, ObjectIdentifier, ServerSideEncryption};
use aws_sdk_s3::Client as S3Client;
use base64::prelude::{Engine, BASE64_STANDARD};
use futures::{stream, StreamExt};
Expand Down Expand Up @@ -91,6 +91,7 @@ pub struct S3CompatibleObjectStorage {
retry_params: RetryParams,
disable_multi_object_delete: bool,
disable_multipart_upload: bool,
server_side_encryption: Option<String>,
}

impl fmt::Debug for S3CompatibleObjectStorage {
Expand Down Expand Up @@ -177,6 +178,7 @@ impl S3CompatibleObjectStorage {
let retry_params = RetryParams::aggressive();
let disable_multi_object_delete = s3_storage_config.disable_multi_object_delete;
let disable_multipart_upload = s3_storage_config.disable_multipart_upload;
let server_side_encryption = s3_storage_config.server_side_encryption.clone();
Ok(Self {
s3_client,
uri: uri.clone(),
Expand All @@ -186,6 +188,7 @@ impl S3CompatibleObjectStorage {
retry_params,
disable_multi_object_delete,
disable_multipart_upload,
server_side_encryption,
})
}

Expand All @@ -203,6 +206,7 @@ impl S3CompatibleObjectStorage {
retry_params: self.retry_params,
disable_multi_object_delete: self.disable_multi_object_delete,
disable_multipart_upload: self.disable_multipart_upload,
server_side_encryption: self.server_side_encryption,
}
}

Expand Down Expand Up @@ -289,12 +293,20 @@ impl S3CompatibleObjectStorage {
.byte_stream()
.await
.map_err(|io_error| Retry::Permanent(StorageError::from(io_error)))?;
self.s3_client
let mut put_object_request = self.s3_client
.put_object()
.bucket(bucket)
.key(key)
.body(body)
.content_length(len as i64)
.content_length(len as i64);
if let Some(_encryption) = &self.server_side_encryption {
put_object_request = match _encryption.as_str() {
samoii marked this conversation as resolved.
Show resolved Hide resolved
"Aes256" => put_object_request.server_side_encryption(ServerSideEncryption::Aes256),
"AwsKms" => put_object_request.server_side_encryption(ServerSideEncryption::AwsKms),
Copy link
Contributor

Choose a reason for hiding this comment

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

how are you specifying the actual KMS key to be used? are you using the default one or S3 bucket keys?

Copy link
Author

Choose a reason for hiding this comment

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

@rdettai
Thanks for your review, I've edited server_side_encryption as an enum variable, and added it in the multipart upload, I've also added the kms key id variable.

I tested it by setting the bucket policy to deny all objects that aren't encrypted, if i didn't set server_side_encryption it will access denied. I also tested encryption with AES-256, AWS KMS, and AWS KMS DSS. When the objects were uploaded to S3, I checked the object properties and found them to be as expected.

Regarding the KMS key ID, it is used with AWS KMS and AWS KMS DSS. If a specific KMS ID is provided, that key will be used. If not, AWS-managed keys will be used.

Copy link
Contributor

Choose a reason for hiding this comment

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

As mentioned earlier, the change to encryption settings also needs to be applied to multipart uploads. I suspect that your tests didn't activate the code path where multipart upload is used, probably because the objects that were created by QW were too small:

quickwit/quickwit/quickwit-storage/src/object_storage/s3_compatible_storage.rs

Lines 744 to 749 in 565becd

if self.disable_multipart_upload || part_num_bytes >= total_len {
self.put_single_part(&key, payload, total_len).await?;
} else {
self.put_multipart(&key, payload, part_num_bytes, total_len)
.await?;
}

Copy link
Author

Choose a reason for hiding this comment

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

@rdettai I've added SSE to the multipart upload, but I'm unsure what file size would be appropriate for testing. I tested with a 5GB file—would that be sufficient?

Copy link
Contributor

Choose a reason for hiding this comment

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

It's more about checking that at least one of the files created by Quickwit is larger than the multipart limit, and that that file received the proper encryption settings.

Copy link
Author

Choose a reason for hiding this comment

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

Hi @rdettai
I have updated the code to the multipart upload and tested both single-part and multipart uploads. The results are as expected.
The data is encrypted according to my configurations, and uploads are rejected if they aren't encrypted as per my bucket's policy.
Here is some of my bucket policy that denies all objects without an encryption.

        {
            "Sid": "VisualEditor0",
            "Effect": "Deny",
            "Principal": {
                "AWS": "arn:aws:iam::123456789:root"
            },
            "Action": "s3:PutObject",
            "Resource": [
                "arn:aws:s3:::my-bucket/*"
            ],
            "Condition": {
                "StringNotEquals": {
                    "s3:x-amz-server-side-encryption": [
                        "AES256",
                        "aws:kms",
                        "aws:kms:dsse"
                    ]
                }
            }
        },
        {
            "Sid": "VisualEditor1",
            "Effect": "Deny",
            "Principal": {
                "AWS": "arn:aws:iam::123456789:root"
            },
            "Action": "s3:PutObject",
            "Resource": [
                "arn:aws:s3:::my-bucket/*"
            ],
            "Condition": {
                "Null": {
                    "s3:x-amz-server-side-encryption": "true"
                }
            }
        }

_ => put_object_request,
};
}
put_object_request
.send()
.await
.map_err(|sdk_error| {
Expand Down Expand Up @@ -956,6 +968,7 @@ mod tests {
retry_params: RetryParams::for_test(),
disable_multi_object_delete: false,
disable_multipart_upload: false,
server_side_encryption: None,
};
assert_eq!(
s3_storage.relative_path("indexes/foo"),
Expand Down Expand Up @@ -1011,6 +1024,7 @@ mod tests {
retry_params: RetryParams::for_test(),
disable_multi_object_delete: true,
disable_multipart_upload: false,
server_side_encryption: None,
};
let _ = s3_storage
.bulk_delete(&[Path::new("foo"), Path::new("bar")])
Expand Down Expand Up @@ -1052,6 +1066,7 @@ mod tests {
retry_params: RetryParams::for_test(),
disable_multi_object_delete: false,
disable_multipart_upload: false,
server_side_encryption: None,
};
let _ = s3_storage
.bulk_delete(&[Path::new("foo"), Path::new("bar")])
Expand Down Expand Up @@ -1134,6 +1149,7 @@ mod tests {
retry_params: RetryParams::for_test(),
disable_multi_object_delete: false,
disable_multipart_upload: false,
server_side_encryption: None,
};
let bulk_delete_error = s3_storage
.bulk_delete(&[
Expand Down Expand Up @@ -1227,6 +1243,7 @@ mod tests {
retry_params: RetryParams::for_test(),
disable_multi_object_delete: false,
disable_multipart_upload: false,
server_side_encryption: None,
};
s3_storage
.put(Path::new("my-path"), Box::new(vec![1, 2, 3]))
Expand Down
Loading