Add support for S3 SSE with KMS #29

madddi · 2017-08-10T07:29:20Z

Issue #24

If not specified in the configuration, kmsKeyID should be an empty string, which gets passed to PutObjectInput. Because an empty string is essentially undefined and the AWS SDK checks if a key ID is given, I did not add checks on our side.

heptibot · 2017-08-10T07:29:22Z

Can one of the admins verify this patch?

ncdc · 2017-08-10T12:33:19Z

ok to test

ncdc

Thanks for the PR!

ncdc · 2017-08-10T13:19:44Z

pkg/cloudprovider/aws/object_storage_adapter.go

-		Body:   body,
+		Bucket:      &bucket,
+		Key:         &key,
+		SSEKMSKeyId: &op.KMSKeyID,


Do we also need this for the other actions?

I find the docs unclear regarding GetObjectInput with KMS. Deleting and listing objects is definitely possible without the key. I compared the code with the Docker registries implementation and usage of GetObject and it seems that s3.GetObject does "the right thing" if it tries to get an encrypted object without explicitly specifying the key ID.

Ok, we'll try it out and see how it works both with "" for the key and with a real one.

ncdc · 2017-08-10T13:20:09Z

docs/config-definition.md

@@ -70,6 +70,7 @@ The configurable parameters are as follows:
 | `disableSSL` | bool | `false` | Set this to `true` if you are using Minio (or another local, S3-compatible storage service) and your deployment is not secured. |
 | `s3ForcePathStyle` | bool | `false` | Set this to `true` if you are using a local storage service like Minio. |
 | `s3Url` | string | Required field for non-AWS-hosted storage| *Example*: http://minio:9000<br><br>You can specify the AWS S3 URL here for explicitness, but Ark can already generate it from `region`, `availabilityZone`, and `bucket`. This field is primarily for local sotrage services like Minio.|
+| `kmsKeyID` | string | Empty | *Example*: "502b409c-4da1-419f-a16e-eif453b3i49f"<br><br>Specify an AWS KMS key id to enable encryption of the backups stored in S3. Only works with AWS S3 and may require explicitly granting key usage rights. |


Remove blank space in between . and |?

jesscodez · 2017-08-10T15:38:50Z

docs/config-definition.md

@@ -70,6 +70,7 @@ The configurable parameters are as follows:
 | `disableSSL` | bool | `false` | Set this to `true` if you are using Minio (or another local, S3-compatible storage service) and your deployment is not secured. |
 | `s3ForcePathStyle` | bool | `false` | Set this to `true` if you are using a local storage service like Minio. |
 | `s3Url` | string | Required field for non-AWS-hosted storage| *Example*: http://minio:9000<br><br>You can specify the AWS S3 URL here for explicitness, but Ark can already generate it from `region`, `availabilityZone`, and `bucket`. This field is primarily for local sotrage services like Minio.|
+| `kmsKeyID` | string | Empty | *Example*: "502b409c-4da1-419f-a16e-eif453b3i49f"<br><br>Specify an AWS KMS key id to enable encryption of the backups stored in S3. Only works with AWS S3 and may require explicitly granting key usage rights.|


nit: link "AWS KMS key" to http://docs.aws.amazon.com/kms/latest/developerguide/overview.html for convenience?

other than this LGTM!

Good idea 🙂

mattmoyer

@madddi thanks for this!

It's not quite working yet, but I think I see why (commented inline above). This is the error I am getting:

I0811 15:59:22.620810       1 backup_controller.go:263] backup heptio-ark/test5 failed: InvalidArgument: Server Side Encryption with AWS KMS managed key requires HTTP header x-amz-server-side-encryption : aws:kms
	status code: 400, request id: 2D8DF113E54DD682

mattmoyer · 2017-08-11T16:03:25Z

pkg/cloudprovider/aws/object_storage_adapter.go

-		Body:   body,
+		Bucket:      &bucket,
+		Key:         &key,
+		SSEKMSKeyId: &op.KMSKeyID,


This also needs ServerSideEncryption: "aws:kms" if op.KMSKeyID != "".

Given that we need to set ServerSideEncryption based on the presence of kmsKeyID, let's only set ServerSideEncryption and SSEKMSKeyId if necessary.

mattmoyer · 2017-08-11T16:05:28Z

pkg/cloudprovider/aws/object_storage_adapter.go

@@ -27,14 +27,16 @@ import (
 var _ cloudprovider.ObjectStorageAdapter = &objectStorageAdapter{}

 type objectStorageAdapter struct {
-	s3 *s3.S3
+	s3       *s3.S3
+	KMSKeyID string


Very minor nit, but this should probably be package-local kmsKeyID rather than KMSKeyID.

mattmoyer

Works great for me now. I had a couple of comments but I'm also +1 as-is.

$ aws s3api head-object --bucket moyer-dev-ark --key test/test.tar.gz
{
    "AcceptRanges": "bytes",
    "ContentType": "binary/octet-stream",
    "LastModified": "Fri, 11 Aug 2017 20:37:49 GMT",
    "ContentLength": 54752,
    "ETag": "\"a8ec4baa50e668fc0cbe15fedc452873\"",
    "ServerSideEncryption": "aws:kms",
    "SSEKMSKeyId": "arn:aws:kms:us-east-2:835338462208:key/18a72e1f-6e67-4c2a-98d6-c81a4c0612b2",
    "Metadata": {}
}

mattmoyer · 2017-08-11T18:52:50Z

pkg/cloudprovider/aws/object_storage_adapter.go

+	// if kmsKeyID is not empty, enable "aws:kms" encryption and amend the key ID
+	if op.kmsKeyID != "" {
+		sseType := "aws:kms"
+		req.ServerSideEncryption = &sseType


Small nit if you're interested, aws-sdk-go has a helper for this req.ServerSideEncryption = aws.String("aws:kms"). I have no idea why the API is this way.

Thanks, good to know. Assigning the string a variable just for the pointer was ugly.

mattmoyer · 2017-08-11T20:44:24Z

pkg/apis/ark/v1/config.go

@@ -98,6 +98,7 @@ type AWSConfig struct {
 	DisableSSL       bool   `json:"disableSSL"`
 	S3ForcePathStyle bool   `json:"s3ForcePathStyle"`
 	S3Url            string `json:"s3Url"`
+	KMSKeyID         string `json:"kmsKeyId"`


I got tripped up on this being kmsKeyId and not kmsKeyID. It looks like we're inconsistent in other keys (s3Url rather than s3URL but disableSSL, not disableSsl). @ncdc thoughts?

It's possible some of the discrepancies are from me manually generating some tags vs using vim-go's :GoAddTags - can't say for sure. Consistency would be nice... But not something we need to change right now. Especially since it breaks backwards compat.

ncdc · 2017-08-14T13:23:10Z

docs/config-definition.md

@@ -1,9 +1,9 @@
 # Ark Config definition

-* [Overview][10]


@abiogenesis-now I thought you said we didn't have to renumber everything?

We don't >< I think the numbers might make it confusing (because people assume that it needs to be in order when it doesn't, @madddi could just append the new link to the end of the numbered list). Essentially the stuff at the bottom are K/V pairs (shorthands and their links), which are most useful when you are reusing the same link multiple times throughout a doc.

This is probably fine as is but let me know if you think we should add some sort of note about this to Contributing or something @ncdc

I'd prefer to keep the changes to a minimum, and just append the new link. Would you mind doing that @madddi? Sorry for the confusion!

I already knew I could just append the new link but thought it'd be nicer if the order of the links and the numbers would match. I'll change it, no problem.

ncdc · 2017-08-14T13:23:39Z

pkg/cloudprovider/aws/object_storage_adapter.go

 }

 func (op *objectStorageAdapter) PutObject(bucket string, key string, body io.ReadSeeker) error {
+


Remove blank line

ncdc · 2017-08-14T13:24:26Z

pkg/cloudprovider/aws/object_storage_adapter.go

 	req := &s3.PutObjectInput{
 		Bucket: &bucket,
 		Key:    &key,
 		Body:   body,
 	}

+	// if kmsKeyID is not empty, enable "aws:kms" encryption and amend the key ID


I'm not sure "amend" is the correct word. Maybe just end the comment at "encryption"?

ncdc · 2017-08-14T13:24:47Z

pkg/cloudprovider/aws/storage_adapter.go

@@ -32,7 +32,7 @@ type storageAdapter struct {

 var _ cloudprovider.StorageAdapter = &storageAdapter{}

-func NewStorageAdapter(config *aws.Config, availabilityZone string) (cloudprovider.StorageAdapter, error) {
+func NewStorageAdapter(config *aws.Config, availabilityZone string, KMSKeyID string) (cloudprovider.StorageAdapter, error) {


s/KMSKeyID/kmsKeyID/

ncdc · 2017-08-14T13:24:55Z

pkg/cloudprovider/aws/storage_adapter.go

@@ -48,7 +48,8 @@ func NewStorageAdapter(config *aws.Config, availabilityZone string) (cloudprovid
 			az:  availabilityZone,
 		},
 		objectStorage: &objectStorageAdapter{
-			s3: s3.New(sess),
+			s3:       s3.New(sess),
+			kmsKeyID: KMSKeyID,


s/KMSKeyID/kmsKeyID/

Signed-off-by: Mathias Merscher <Mathias.Merscher@dg-i.net>

ncdc · 2017-08-14T17:53:32Z

@abiogenesis-now @mattmoyer any other comments?

jesscodez · 2017-08-14T18:02:43Z

LGTM -- @ncdc do you think this encryption feature is worth mentioning in the last bullet point of cloud-provider-specifics.md? I'm ok either way, but I've noticed that people tend to read that document more.

ncdc · 2017-08-14T18:59:10Z

@abiogenesis-now we can do it in a follow-up if needed

ncdc · 2017-08-14T19:12:35Z

Thanks again @madddi!!!

use ubi8-minimal

madddi force-pushed the feature/s3-ss3-kms-support branch from f90b8ab to af0f548 Compare August 10, 2017 07:30

ncdc reviewed Aug 10, 2017

View reviewed changes

ncdc requested review from skriss and jesscodez August 10, 2017 13:26

madddi force-pushed the feature/s3-ss3-kms-support branch from af0f548 to 97a89bf Compare August 10, 2017 13:31

jesscodez reviewed Aug 10, 2017

View reviewed changes

madddi force-pushed the feature/s3-ss3-kms-support branch from 97a89bf to fdd3e88 Compare August 10, 2017 18:11

mattmoyer self-assigned this Aug 11, 2017

mattmoyer suggested changes Aug 11, 2017

View reviewed changes

mattmoyer reviewed Aug 11, 2017

View reviewed changes

madddi force-pushed the feature/s3-ss3-kms-support branch 2 times, most recently from 7b6ec6e to 9e66aa0 Compare August 11, 2017 18:40

mattmoyer approved these changes Aug 11, 2017

View reviewed changes

madddi force-pushed the feature/s3-ss3-kms-support branch from 9e66aa0 to 8958057 Compare August 11, 2017 21:18

ncdc suggested changes Aug 14, 2017

View reviewed changes

madddi force-pushed the feature/s3-ss3-kms-support branch from 8958057 to 34f4af1 Compare August 14, 2017 14:07

adds support for S3 SSE with KMS

df320d7

Signed-off-by: Mathias Merscher <Mathias.Merscher@dg-i.net>

madddi force-pushed the feature/s3-ss3-kms-support branch from 34f4af1 to df320d7 Compare August 14, 2017 16:18

ncdc approved these changes Aug 14, 2017

View reviewed changes

ncdc added this to the v0.4.0 milestone Aug 14, 2017

ncdc merged commit 41e7861 into vmware-tanzu:master Aug 14, 2017

ncdc mentioned this pull request Aug 14, 2017

Support S3 server side encryption with KMS #24

Closed

madddi deleted the feature/s3-ss3-kms-support branch July 19, 2018 16:13

jmontleon added a commit to jmontleon/velero that referenced this pull request Aug 26, 2019

Merge pull request vmware-tanzu#29 from jmontleon/ubi8-minimal

bee1543

use ubi8-minimal

timh unassigned mattmoyer Sep 30, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for S3 SSE with KMS #29

Add support for S3 SSE with KMS #29

madddi commented Aug 10, 2017

heptibot commented Aug 10, 2017

ncdc commented Aug 10, 2017

ncdc left a comment

ncdc Aug 10, 2017

madddi Aug 10, 2017 •

edited

Loading

ncdc Aug 10, 2017

ncdc Aug 10, 2017

madddi Aug 10, 2017

jesscodez Aug 10, 2017

jesscodez Aug 10, 2017

madddi Aug 10, 2017

mattmoyer left a comment

mattmoyer Aug 11, 2017

ncdc Aug 11, 2017

mattmoyer Aug 11, 2017

mattmoyer left a comment

mattmoyer Aug 11, 2017

madddi Aug 11, 2017

mattmoyer Aug 11, 2017

ncdc Aug 11, 2017

ncdc Aug 14, 2017

jesscodez Aug 14, 2017

ncdc Aug 14, 2017

madddi Aug 14, 2017

ncdc Aug 14, 2017

ncdc Aug 14, 2017

ncdc Aug 14, 2017

ncdc Aug 14, 2017

ncdc commented Aug 14, 2017

jesscodez commented Aug 14, 2017

ncdc commented Aug 14, 2017

ncdc commented Aug 14, 2017

		}

		func (op *objectStorageAdapter) PutObject(bucket string, key string, body io.ReadSeeker) error {

Add support for S3 SSE with KMS #29

Add support for S3 SSE with KMS #29

Conversation

madddi commented Aug 10, 2017

heptibot commented Aug 10, 2017

ncdc commented Aug 10, 2017

ncdc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

madddi Aug 10, 2017 • edited Loading

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

mattmoyer 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

mattmoyer 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

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

ncdc commented Aug 14, 2017

jesscodez commented Aug 14, 2017

ncdc commented Aug 14, 2017

ncdc commented Aug 14, 2017

madddi Aug 10, 2017 •

edited

Loading