-
Notifications
You must be signed in to change notification settings - Fork 2.3k
Add support for SSE-KMS and bucket owner verification #18312
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
Conversation
❌ Gradle check result for d04e3d3: FAILURE Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change? |
❌ Gradle check result for e2a5d28: FAILURE Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change? |
❕ Gradle check result for a3b50c3: UNSTABLE Please review all flaky tests that succeeded after retry and create an issue if one does not already exist to track the flaky failure. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #18312 +/- ##
============================================
- Coverage 72.61% 72.48% -0.13%
+ Complexity 67438 67374 -64
============================================
Files 5488 5489 +1
Lines 311067 311168 +101
Branches 45218 45222 +4
============================================
- Hits 225872 225552 -320
- Misses 66848 67265 +417
- Partials 18347 18351 +4 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
❌ Gradle check result for 5196b5d: FAILURE Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change? |
7ccc13e
to
457a26c
Compare
❌ Gradle check result for 457a26c: FAILURE Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change? |
❌ Gradle check result for 28ea988: FAILURE Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change? |
❌ Gradle check result for ca7fc4d: FAILURE Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change? |
❌ Gradle check result for 7eacbe1: FAILURE Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change? |
❌ Gradle check result for adacc1a: FAILURE Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change? |
❌ Gradle check result for adacc1a: FAILURE Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change? |
❕ Gradle check result for adacc1a: UNSTABLE Please review all flaky tests that succeeded after retry and create an issue if one does not already exist to track the flaky failure. |
@Bukhtawar @gbbafna please take a look when you get a chance, thanks! |
plugins/repository-s3/src/main/java/org/opensearch/repositories/s3/S3BlobContainer.java
Outdated
Show resolved
Hide resolved
plugins/repository-s3/src/main/java/org/opensearch/repositories/s3/S3BlobContainer.java
Outdated
Show resolved
Hide resolved
❌ Gradle check result for 65fd070: FAILURE Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change? |
…dd support for bucket owner verification Signed-off-by: Jay Deng <[email protected]>
Thanks for the first pass @gbbafna! I need to run some more manual tests on the multi-part upload scenarios as well. For the previous remote store PRs, did we have any ways to run tests directly against S3? I don't think it's possible for us to add functional coverage on any of the new settings without calling S3 so wanted to see if there was more I could do beyond manual tests. |
We created docker cluster locally which uses S3 . Yes, it is not possible to test this without actually calling S3 . |
Thanks @gbbafna. In that case I will work on adding some more manual tests that cover the async use cases and update the PR description when done |
@gbbafna I've added manual testing on the async cases to the PR overview. It's not really possible to do unhappy path testing on these either, since registering a repo does a test upload. I also manually tested the async delete path, but wasn't sure how to display the results for that. Please let me know if you think there is additional test coverage I can do, or if you think this PR is good to merge. Thanks! |
LGTM . Thanks @jed326 for the changes. |
this.serverSideEncryptionType = SERVER_SIDE_ENCRYPTION_TYPE_SETTING.get(repositoryMetadata.settings()); | ||
this.serverSideEncryptionKmsKey = SERVER_SIDE_ENCRYPTION_KMS_KEY_SETTING.get(repositoryMetadata.settings()); | ||
this.serverSideEncryptionBucketKey = SERVER_SIDE_ENCRYPTION_BUCKET_KEY_SETTING.get(repositoryMetadata.settings()); | ||
this.serverSideEncryptionEncryptionContext = SERVER_SIDE_ENCRYPTION_ENCRYPTION_CONTEXT_SETTING.get(repositoryMetadata.settings()); | ||
this.expectedBucketOwner = EXPECTED_BUCKET_OWNER_SETTING.get(repositoryMetadata.settings()); |
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.
@jed326 : For my understanding would these settings become update-able?
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.
@Bukhtawar yep, a user can change the encryption type / KMS key / encryption context at any time. On the download side these values are read from object metadata rather than user provided, so as long as the calling identity has proper permissions updating these settings won't break anything.
…dd support for bucket owner verification (opensearch-project#18312) Signed-off-by: Jay Deng <[email protected]>
…dd support for bucket owner verification (opensearch-project#18312) Signed-off-by: Jay Deng <[email protected]>
…dd support for bucket owner verification (opensearch-project#18312) Signed-off-by: Jay Deng <[email protected]>
…dd support for bucket owner verification (opensearch-project#18312) Signed-off-by: Jay Deng <[email protected]>Signed-off-by: TJ Neuenfeldt <[email protected]>
…dd support for bucket owner verification (opensearch-project#18312) Signed-off-by: Jay Deng <[email protected]>
Description
This PR makes 3 main changes
1. Remove the repository-s3 setting
server_side_encryption
As of January 5, 2023, S3 now applies SSE-S3 as the base level of encryption (the
AES256
encryption type), and there is no way to disable this. This means regardless of whether or not this setting is set or whatever value the setting is set to, it makes no difference in how objects are uploaded to S3.For more details, see: https://docs.aws.amazon.com/AmazonS3/latest/userguide/serv-side-encryption.html
Fortunately, repository settings are fully backwards compatible ever after being removed, which means that even after removing the setting, I can still register a repository like so:
2. Add settings to support SSE-KMS
There are 4 new settings introduced:
server_side_encryption_type
server_side_encryption_kms_key_id
server_side_encryption_bucket_key_enabled
server_side_encryption_encryption_context
The
server_side_encryption_type
setting supports 3 values:AES256
-- SSE-S3)aws:kms
-- SSE-KMS)bucket_default
-- this will make the request use the default encryption configuration on the S3 bucketExample repository:
3. Add bucket owner verification
The new setting
expected_bucket_owner
, when set, will be passed in all S3 bucket operation requests to verify that the bucket owner is the expected account.See: https://docs.aws.amazon.com/AmazonS3/latest/userguide/bucket-owner-condition.html#bucket-owner-condition-when-to-use
Testing
Added the new setting/fields to the existing SSE unit tests, but we are pretty limited without being able to actually call S3.
Note: Pasted actual KMS key in the test results below, but the key used for testing has been deleted for security purposes
Manual testing:
default
setting is setBucketKeyEnabled
is not present in the response above.AES256
encryption type still uses to SSE-S3I used the kNN plugin's implementation for this, as it was a little easier to set up than remote store (ref: https://github.com/opensearch-project/k-NN/blob/b7fc5dd98072ea157a9b301e7f93d79e9700984d/src/main/java/org/opensearch/knn/index/codec/nativeindex/remote/DefaultVectorRepositoryAccessor.java#L81)
Related Issues
Resolves #14606
Check List
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.