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

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

Conversation

samoii
Copy link

@samoii samoii commented Sep 7, 2024

Description

aws s3 server_side_encryption configuration when upload object to s3 and config in storage part.

How was this PR tested?

I builded docker image and installed to EKS with helm chart with config
storage: s3: region: ap-southeast-1 server_side_encryption: Aes256

server_side_encryption can be set to Aes256, Awskms or left empty if encryption is not required.
I tried sending logs via Kafka and used the Quickstart document to test if the logs can be written to S3.

@CLAassistant
Copy link

CLAassistant commented Sep 7, 2024

CLA assistant check
All committers have signed the CLA.

Copy link
Contributor

@rdettai rdettai left a comment

Choose a reason for hiding this comment

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

I think the encryption you provided in your test (Aes256) is actually the default encryption now (https://docs.aws.amazon.com/AmazonS3/latest/userguide/UsingServerSideEncryption.html). If you do specify a KMS encryption, you will notice that most of the objects written by Quickwit as a result of this PR will still not use that setting. That's because Quickwit uses multipart uploads by default, and your change didn't update that code path.

if let Some(_encryption) = &self.server_side_encryption {
put_object_request = match _encryption.as_str() {
"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:

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"
                }
            }
        }

quickwit/quickwit-config/src/storage_config.rs Outdated Show resolved Hide resolved
@samoii samoii requested a review from rdettai September 20, 2024 02:42
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.

3 participants