#41196 | Add aws KMS encryption in the elastic search repository-s3 plugin #47486

Prasannasaraf · 2019-10-03T05:59:24Z

No description provided.

# Conflicts: # plugins/repository-s3/src/main/java/org/elasticsearch/repositories/s3/S3Repository.java

elasticmachine · 2019-10-03T06:41:09Z

Pinging @elastic/es-distributed (:Distributed/Snapshot/Restore)

satishdash · 2019-10-03T09:36:38Z

plugins/repository-s3/src/main/java/org/elasticsearch/repositories/s3/S3BlobStore.java

+
+    public boolean sseAwsKeyIsEmpty() {
+        if (sseAwsKeyId == null || sseAwsKeyId.trim().isEmpty()) {
+            return true;


Can be simplified into:

return sseAwsKeyId == null || sseAwsKeyId.trim().isEmpty()

original-brownbear

Thanks for this PR @Prasannasaraf !

I left a few comments/questions on the exact mechanics of how the new setting should work.

Also, we definitely have to add a 3rd party test for this that runs if the appropriate environment variable is set. Something along the lines of what we did for Azure SAS tokens is probably what we want here (see https://github.com/elastic/elasticsearch/pull/42982/files#diff-972bd167c62a9bf3a9bf9a80d6ea2298R79).

I also wonder if we sohuld make setting an endpoint on a repo or client mutually exclusive with using KMS on the settings layer already since this won't work with non-AWS S3 endpoints anyway?

original-brownbear · 2019-10-03T11:07:30Z

plugins/repository-s3/src/main/java/org/elasticsearch/repositories/s3/S3BlobContainer.java

-            md.setSSEAlgorithm(ObjectMetadata.AES_256_SERVER_SIDE_ENCRYPTION);
+            if (blobStore.sseAwsKeyIsEmpty()) {
+                md.setSSEAlgorithm(ObjectMetadata.AES_256_SERVER_SIDE_ENCRYPTION);
+            }else {


NIT: formatting is off here

original-brownbear · 2019-10-03T11:09:47Z

plugins/repository-s3/src/main/java/org/elasticsearch/repositories/s3/S3BlobContainer.java

@@ -313,14 +314,22 @@ void executeSingleUpload(final S3BlobStore blobStore,
        }

        final ObjectMetadata md = new ObjectMetadata();
+        final PutObjectRequest putRequest = new PutObjectRequest(blobStore.bucket(), blobName, input, md);
        md.setContentLength(blobSize);
        if (blobStore.serverSideEncryption()) {


This is a little strange now IMO. I think we should always use encryption if a kms key is set. Otherwise, we force the user to set the true flag on server_side_encryption for a value of the kms key to take effect. That's counterintuitive and error prone isn't it? :)

Should we put a warning if sse is disabled but KMS key is set and continue?

original-brownbear · 2019-10-03T11:10:39Z

plugins/repository-s3/src/main/java/org/elasticsearch/repositories/s3/S3Repository.java

+     * When aws_kms_key is set, files are encrypted on server side using kms key provided or AE256 algorithm is used.
+     * Defaults to AES256 algorithm.
+     */
+    static final Setting<String> AWS_KMS_KEY = Setting.simpleString("aws_kms_key");


We will need a documentation change for this and document this setting in repository-s3.asciidoc.
Before we do we should come to a conclusion on how this setting will work relative to server_side_encryption though :)

I'd also like a second opinion here (@ywelsch, @tlrx maybe?) on whether or not this should be a secure setting. It seems technically safe since it's just a key id but I wonder if not having it in the key store might create some potential (compliance?) issues?

If we want to add support for KMS then I think we need smarter settings that allow to distinguish between server side encryption using AES256, SSE with KMS and default key and SSE with KMS and specific key id.

I'd also like a second opinion here (@ywelsch, @tlrx maybe?) on whether or not this should be a secure setting.

I tend to think that it should be a secured setting. I also think that it should be correctly documented as we don't want users to remove or update keys and not being able to decrypt data afterwards? Also, @albertzaharovits might have an opinion on this.

I don't think the key identifier should be a secure setting. It's just an identifier, and I think that putting it in the keystore creates unnecessary complications for the user.

Also, from https://docs.aws.amazon.com/AmazonS3/latest/API/API_PutObject.html I see that the API has complexish rules:

x-amz-server-side-encryption x-amz-server-side-encryption-aws-kms-key-id x-amz-server-side-encryption-context Note: If you specify x-amz-server-side-encryption:aws:kms, but don't provide x-amz-server-side- encryption-aws-kms-key-id, Amazon S3 uses the default AWS KMS key to protect the data.

that don't really make much sense to me.

I would think to whitelist these request headers, together with x-amz-server-side-encryption-customer-algorithm, x-amz-server-side-encryption-customer-key, x-amz-server-side-encryption-customer-key-MD5 from below, and delegate the responsibility to the user to validate them. What do you think?

original-brownbear · 2019-11-25T08:53:43Z

Hi @Prasannasaraf,

sorry for the delay here. As I said in my last comment, this will require adding some actual third party tests against real AWS S3 to validate that it works. Are you interested in working on those and continuing here? (I realize that this is a non-trivial amount of effort on top of the actual change so no worries if not.)

Prasannasaraf · 2019-12-02T14:36:32Z

Hi @original-brownbear

I can work on adding third-party tests along with the changes suggested by @albertzaharovits. Do let me know the amount of work involved here.

jstolle · 2020-11-20T20:20:40Z

Is there an ETA on the completion of this merge?

wywywywy · 2024-05-30T10:35:41Z

Is this ever going to get merged?

DaveCTurner · 2024-05-30T10:48:32Z

Is this ever going to get merged?

Not in its current state, no, we can't accept this kind of change without supporting tests. I'm going to close this because it has been waiting for the missing tests to be added for a very long time - see the contributing guidelines for further information.

Add aws KMS encryption in the elastic search repository-s3 plugin

0eaa467

# Conflicts: # plugins/repository-s3/src/main/java/org/elasticsearch/repositories/s3/S3Repository.java

Prasannasaraf mentioned this pull request Oct 3, 2019

#41196 | Add aws KMS encryption in the elastic search repository-s3 plugin #47466

Closed

Prasannasaraf changed the title ~~Add aws KMS encryption in the elastic search repository-s3 plugin~~ #41196 | Add aws KMS encryption in the elastic search repository-s3 plugin Oct 3, 2019

original-brownbear self-requested a review October 3, 2019 06:41

original-brownbear added the :Distributed/Snapshot/Restore Anything directly related to the `_snapshot/*` APIs label Oct 3, 2019

Prasannasaraf mentioned this pull request Oct 3, 2019

Repository-S3: support for KMS server-side encryption #41196

Open

satishdash suggested changes Oct 3, 2019

View reviewed changes

Simplify logic

f4d11c4

satishdash approved these changes Oct 3, 2019

View reviewed changes

original-brownbear requested changes Oct 3, 2019

View reviewed changes

Use kms when sse aws key exists

e37bc99

Merge branch 'master' into kms

1ccc519

rjernst added the Team:Distributed Meta label for distributed team label May 4, 2020

elasticsearchmachine changed the base branch from master to main July 22, 2022 23:14

DaveCTurner closed this May 30, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

#41196 | Add aws KMS encryption in the elastic search repository-s3 plugin #47486

#41196 | Add aws KMS encryption in the elastic search repository-s3 plugin #47486

Prasannasaraf commented Oct 3, 2019

elasticmachine commented Oct 3, 2019

satishdash Oct 3, 2019

original-brownbear left a comment

original-brownbear Oct 3, 2019

original-brownbear Oct 3, 2019

Prasannasaraf Oct 9, 2019

original-brownbear Oct 3, 2019

original-brownbear Oct 3, 2019

tlrx Oct 4, 2019

albertzaharovits Oct 9, 2019

albertzaharovits Oct 9, 2019

original-brownbear commented Nov 25, 2019

Prasannasaraf commented Dec 2, 2019

jstolle commented Nov 20, 2020

wywywywy commented May 30, 2024

DaveCTurner commented May 30, 2024

#41196 | Add aws KMS encryption in the elastic search repository-s3 plugin #47486

#41196 | Add aws KMS encryption in the elastic search repository-s3 plugin #47486

Conversation

Prasannasaraf commented Oct 3, 2019

elasticmachine commented Oct 3, 2019

Choose a reason for hiding this comment

original-brownbear left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

original-brownbear commented Nov 25, 2019

Prasannasaraf commented Dec 2, 2019

jstolle commented Nov 20, 2020

wywywywy commented May 30, 2024

DaveCTurner commented May 30, 2024