S3 client side encryption

[xuzha]

Closes #13673

I'm picking up a unfinished PR here elastic/elasticsearch-cloud-aws#118. Most of the implementation keeps the same.

This PR add s3 client side encryption functionality. It uses the envelope encryption of the AWS ADK for Java (AES 256 in CBC mode). You case use keys of 128, 192 or 256 bits but you have to install the JCE because the envelope encryption will use 256bits keys.

I'm not sure about this: I remove the MD5 checking part in the doUpload method. I think AWS S3 SDK is doing this for us.
More here: https://github.com/aws/aws-sdk-java/blob/master/aws-java-sdk-s3/src/main/java/com/amazonaws/services/s3/AmazonS3Client.java#L1449-L1469.

[xuzha]

@dadoonet would you like to take a look, if you have time? :-)

[dadoonet]

I think we should only have one client method here. So I would replace the previous line by the new method.

[dadoonet]

I gave a first look (not complete yet) and it's a really good start.

About removing MD5 check, I do agree that it sounds already here in the AWS SDK. Any chance you can test manually that it goes through this line in debug mode? https://github.com/aws/aws-sdk-java/blob/master/aws-java-sdk-s3/src/main/java/com/amazonaws/services/s3/AmazonS3Client.java#L1455

[xuzha]

Thx @dadoonet, I just updated the PR ;-)

About the debugging, I confirmed that AWS SDK is checking the MD5 for us, the screen shot:

[xuzha]

@dadoonet can you take another look when you have time? :-)

[dadoonet]

I was wondering about this. It's all about encryption.

We already have server_side_encryption. I wonder if we should at some point rename the settings (may be within another PR) to:

PUT _snapshot/my_s3_repository
{
  "type": "s3",
  "settings": {
    "bucket": "my_bucket_name",
    "region": "us-west",
    "encryption": {
       "server": {
           "enabled":  false
       },
       "client": {
           "enabled":  true,
           "symmetric_key": "XYZ",
           "public_key": "XYZ",
           "private_key": "XYZ"
       }
    }
  }
}

Just thinking out loud here. Might be too much engineering though...

[dadoonet]

I left a note which we can discuss in another issuer later. The current PR looks good to me.
Not sure if we want to backport this to 2.x branch as well...

[xuzha]

Merged into master. Thanks @dadoonet for the review and sorry for the delay.

[xuzha]

@clintongormley I have noticed @NicolasTr did not signed the CLA at that commit. Is there any problem here?

[GlenRSmith]

@xuzha if it helps, @NicolasTr signed the CLA in:
elastic/elasticsearch-cloud-aws#118

[jasontedor]

I don't think that this pull request should have been merged. The CLA doesn't pass on one of the commits, and one of the commits has merge conflict markers in it.

[xuzha]

Thanks @jasontedor

The first two commits should be together, I picked up a really old commit from elastic/elasticsearch-cloud-aws#118, and fixed in the later commit.

[jasontedor]

The first two commits should be together, I picked up a really old commit from elastic/elasticsearch-cloud-aws#118, and fixed in the later commit.

We should never knowingly commit code that doesn't compile.

[billxinli]

Curious if we will ever see this feature implemented or merged in?

[hoffoo]

Any hope that we will see this?

[dchang-novotec]

^^^ ditto, would definitely like to see this feature in the next release

[stantona]

@xuzha / @dadoonet This feature has stalled for quite a while and it seems like it's 99% there. Is there a chance we can finalize this? This is a much-needed feature, as many organizations have a hard requirement to encrypt data due to data security, so the absence of the client-side encryption option is a show stopper for many when considering using the S3 Repository.

[sts]

Any ideas whats the status of this?

[kogelc]

Hello, do you know when this feature will be added ?

[JigarJoshi]

🏓 Any update here ?