out_s3: Add SSE support + validation tests #11410
Conversation
Signed-off-by: Gabriel Yamin <gabrielyamin98@gmail.com>
📝 WalkthroughWalkthroughAdds server-side encryption (SSE) support to the S3 output plugin: new config options for SSE mode and optional KMS key ID, validation during init, storage in the plugin context, and emission of corresponding S3 headers; includes unit tests for valid and invalid configurations. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 84be0b4d73
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@plugins/out_s3/s3.c`:
- Around line 4215-4221: Update the documentation string for the configuration
entry sse_kms_key_id so it notes the setting applies when the sse option equals
"aws:kms" or "aws:kms:dsse"; locate the FLB_CONFIG_MAP_STR entry for
"sse_kms_key_id" in s3.c and change the descriptive text to include
"aws:kms:dsse" (DSSE-KMS) alongside "aws:kms" so users know the KMS key ID/ARN
is used for both SSE types.
- Around line 264-275: The code currently adds the ctx->sse_kms_key_id header
unconditionally; change both the header count logic and the header construction
logic to only include the x-amz-server-side-encryption-aws-kms-key-id header
when ctx->sse is a KMS-type value (e.g., "aws:kms" or "aws:kms:dsse").
Concretely, in the same function that builds s3_headers (the block that checks
ctx->sse and ctx->sse_kms_key_id) wrap the ctx->sse_kms_key_id checks with a
condition like strcmp(ctx->sse, "aws:kms") == 0 || strcmp(ctx->sse,
"aws:kms:dsse") == 0 (and mirror this logic in the earlier header-counting
section) so the KMS key-id header is only added when SSE type is KMS-based.
🧹 Nitpick comments (2)
tests/runtime/out_s3.c (1)
231-322: Test coverage is good but could be more comprehensive.The tests correctly validate invalid and valid SSE configurations. Consider adding tests for:
- The
aws:kms:dsseSSE type (currently untested valid value)- The
sse_kms_key_idconfiguration optionplugins/out_s3/s3.c (1)
896-910: Consider warning whensse_kms_key_idis set without KMS-based SSE.The validation correctly rejects invalid SSE values. However, it might be helpful to warn users if they set
sse_kms_key_idwithout a KMS-based SSE mode, as this configuration would be ignored.💡 Optional enhancement
tmp = flb_output_get_property("sse_kms_key_id", ins); if (tmp) { + if (ctx->sse == NULL || + (strcasecmp(ctx->sse, "aws:kms") != 0 && + strcasecmp(ctx->sse, "aws:kms:dsse") != 0)) { + flb_plg_warn(ctx->ins, "'sse_kms_key_id' is only applicable when 'sse' is set to 'aws:kms' or 'aws:kms:dsse'"); + } ctx->sse_kms_key_id = (char *) tmp; }
Signed-off-by: Gabriel <gabrielyamin98@gmail.com>
Signed-off-by: Gabriel <gabrielyamin98@gmail.com>
1 participant
Summary
Adds server-side encryption support for S3 uploads with SSE-KMS and DSSE-KMS.
Changes
sseconfiguration option:AES256,aws:kms,aws:kms:dssesse_kms_key_idoption to specify a custom KMS key ARNConfiguration Example
[OUTPUT] Name s3 Match * bucket my-bucket region us-east-1 sse aws:kms sse_kms_key_id arn:aws:kms:us-east-1:123456789:key/my-key-idEnter
[N/A]in the box, if an item is not applicable to your change.Testing
Before we can approve your change; please submit the following in a comment:
Click to expand practical test results
No SSE
[OUTPUT] Name s3 Match valid_no_sse bucket <BUCKET-NAME> region eu-west-1 upload_timeout 5s use_put_object true total_file_size 1M s3_key_format /flb-test/valid_no_sse.logSSE-KMS (single layer)
[OUTPUT] Name s3 Match valid_sse_kms bucket <BUCKET-NAME> region eu-west-1 upload_timeout 5s use_put_object true total_file_size 1M sse aws:kms s3_key_format /flb-test/valid_sse_kms.logTrying to upload without SSE headers (bucket requires SSE-KMS):
With SSE header:
SSE-S3 (AES256)
[OUTPUT] Name s3 Match valid_sse_aes256 bucket <BUCKET-NAME> region eu-west-1 upload_timeout 5s use_put_object true total_file_size 1M sse AES256 s3_key_format /flb-test/valid_sse_aes256.logDSSE-KMS (dual layer)
[OUTPUT] Name s3 Match valid_sse_kms_dsse bucket <BUCKET-NAME> region eu-west-1 upload_timeout 5s use_put_object true total_file_size 1M sse aws:kms:dsse s3_key_format /flb-test/valid_sse_kms_dsse.logSSE-KMS with specific key ID
[OUTPUT] Name s3 Match valid_sse_kms_with_key bucket <BUCKET-NAME> region eu-west-1 upload_timeout 5s use_put_object true total_file_size 1M sse aws:kms sse_kms_key_id arn:aws:kms:eu-west-1:<ACCOUNT-NUMBER>:key/<KEY-ID> s3_key_format /flb-test/valid_sse_kms_with_key.logDSSE-KMS with specific key ID
[OUTPUT] Name s3 Match valid_sse_kms_dsse_with_key bucket <BUCKET-NAME> region eu-west-1 upload_timeout 5s use_put_object true total_file_size 1M sse aws:kms:dsse sse_kms_key_id arn:aws:kms:eu-west-1:<ACCOUNT-NUMBER>:key/<KEY-ID> s3_key_format /flb-test/valid_sse_kms_dsse_with_key.logInvalid SSE value rejected
[OUTPUT] Name s3 Match invalid_sse_value bucket <BUCKET-NAME> region eu-west-1 upload_timeout 5s use_put_object true total_file_size 1M sse invalid_encryption s3_key_format /flb-test/invalid_sse_value.logClick to expand valgrind report
###Even though this PR introduced no memory allocations, I've ran the valgrind test just to be safe and add it here.
Memory Analysis (Valgrind)
This PR does not allocate new memory — the SSE feature only reads config values and sets header pointers to static strings.
If this is a change to packaging of containers or native binaries then please confirm it works for all targets.
ok-package-testlabel to test for all targets (requires maintainer to do).New unit tests report
Click to expand test results
Documentation
fluent/fluent-bit-docs#2365
Backporting
Fluent Bit is licensed under Apache 2.0, by submitting this pull request I understand that this code will be released under the terms of that license.
Summary by CodeRabbit
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.