-
Notifications
You must be signed in to change notification settings - Fork 333
Fix #2685 [BUG] S3 block spilling no longer passes request headers #3101
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
base: master
Are you sure you want to change the base?
Fix #2685 [BUG] S3 block spilling no longer passes request headers #3101
Conversation
|
⏳ I'm reviewing this pull request for security vulnerabilities and code quality issues. I'll provide an update when I'm done |
1 similar comment
|
⏳ I'm reviewing this pull request for security vulnerabilities and code quality issues. I'll provide an update when I'm done |
|
✅ I finished the code review, and didn't find any security or code quality issues. |
1 similar comment
|
✅ I finished the code review, and didn't find any security or code quality issues. |
4572d26 to
3ca2f46
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #3101 +/- ##
============================================
+ Coverage 63.67% 68.35% +4.68%
- Complexity 4344 5009 +665
============================================
Files 621 636 +15
Lines 23286 24170 +884
Branches 2859 3000 +141
============================================
+ Hits 14827 16522 +1695
+ Misses 7070 6210 -860
- Partials 1389 1438 +49 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| /** | ||
| * Creates an AwsRequestOverrideConfiguration with custom headers from the environment | ||
| */ | ||
| private AwsRequestOverrideConfiguration createRequestOverrideConfig() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
return an optional here is better than nulls.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the suggestion. Updated the code to return an Optional instead of null.
81e8dcb to
195bbcb
Compare
195bbcb to
bac649c
Compare
Issue #, if available:
#2685
Description of changes:
This PR fixes issue #2685 where
spill_put_request_headersconfiguration was incorrectly being set as object metadata instead of request headers on S3 PutObject requests. This caused SSE-KMS encryption headers to not appear in CloudTrailrequestParameters, breaking encryption requirements and audit logging.Problem
In versions >= v2024.42.1, the
spill_put_request_headersenvironment variable values were being set as object metadata using.metadata()instead of request headers. This meant:x-amz-server-side-encryption,x-amz-server-side-encryption-aws-kms-key-id) were not sent as request parametersrequestParameters(they only appeared inresponseElements)Solution
Changed
S3BlockSpiller.write()to useAwsRequestOverrideConfigurationto set headers as request headers via.overrideConfiguration()instead of.metadata().Test Document:
Bug_Fix_2685.docx
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.