-
Notifications
You must be signed in to change notification settings - Fork 811
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
blob/s3blob: Support S3 server side encryption headers for Write and Copy #3340
blob/s3blob: Support S3 server side encryption headers for Write and Copy #3340
Conversation
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
Adds support to s3blob for setting AWS S3 server side encryption headers when making requests that require such headers. The additional settings can be specified with the `ssetype` and `kmskeyid` URL params, similar to the other configuration settings
c8d1a71
to
2017a81
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #3340 +/- ##
==========================================
- Coverage 77.47% 73.13% -4.34%
==========================================
Files 104 113 +9
Lines 13933 14825 +892
==========================================
+ Hits 10794 10843 +49
- Misses 2378 3210 +832
- Partials 761 772 +11 ☔ View full report in Codecov by Sentry. |
@vangent Thanks for the feedback! I implemented the requested changes. I left a couple unresolved since I wasn't sure if I did it exactly how you had in mind. This is ready for re-review Let me know if I can force push to fix the CLA check, one of the commits has a slightly incorrect version of my email due to a local misconfiguration |
Please go ahead and force-push and I'll re-review. |
Updates the EncryptionType field of s3blob.Options to use the AWS type. An error is thrown at initialisation time if an invalid value is provided for the ssetype parameter
84f7555
to
a7a2899
Compare
Ready for re-review |
…yption. Add tests
@vangent Sorry for the delay on this one, priorities forced my to put this aside for a moment, but I am back now. This is ready for re-review, and I re-did the smoke testing as well. Unit tests passRedoing the smoke testing after the adjustments:SDKv2Bucket policy enforces KMS headers - none provided (effectively the issue described in #3337)Bucket policy enforces KMS headers - values providedExpect: No errors. Object is uploaded Bucket policy doesn't enforce KMS - no values providedExpect: No errors. Object is uploaded SDKv1Bucket policy enforces KMS headers - none provided (effectively the issue described in #3337)Bucket policy enforces KMS headers - values providedExpect: No errors. Object is uploaded Bucket policy doesn't enforce KMS - no values provided |
Adds support to s3blob for setting AWS S3 server side encryption headers when making requests that require such headers. The additional settings can be specified with the
ssetype
andkmskeyid
URL params, similar to the other configuration settingsFixes #3337
Local testing
SDKv2
Bucket policy enforces KMS headers - none provided (effectively the issue described in #3337)
Expect:
AccessDenied
failureBucket policy enforces KMS headers - values provided
Expect: No errors. Object is uploaded
Bucket policy doesn't enforce KMS - no values provided
Expect: No errors. Object is uploaded
SDKv1
Bucket policy enforces KMS headers - none provided (effectively the issue described in #3337)
Expect:
AccessDenied
failureBucket policy enforces KMS headers - values provided
Expect: No errors. Object is uploaded
Bucket policy doesn't enforce KMS - no values provided
Expect: No errors. Object is uploaded