Add google cloud KMS support #245

calind · 2017-09-11T18:55:15Z

Fixes #188.

codecov-io · 2017-09-11T18:59:38Z

Codecov Report

Merging #245 into master will decrease coverage by 1.17%.
The diff coverage is 28.91%.

@@            Coverage Diff             @@
##           master     #245      +/-   ##
==========================================
- Coverage   44.94%   43.76%   -1.18%     
==========================================
  Files           6        7       +1     
  Lines        1048     1131      +83     
==========================================
+ Hits          471      495      +24     
- Misses        515      573      +58     
- Partials       62       63       +1

Impacted Files	Coverage Δ
sops.go	`41.29% <ø> (ø)`	⬆️
config/config.go	`77.27% <100%> (+2.27%)`	⬆️
gcpkms/keysource.go	`23.37% <23.37%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5caa68e...c7c94e9. Read the comment docs.

autrilla

Thank you very much for the PR. Looks good, I've left a few minor comments that need to be address before we can merge this.

autrilla · 2017-09-11T20:10:43Z

cloudkms/keysource.go

+
+// this needs to be a global var for unit tests to work (mockKMS redefines
+// it in keysource_test.go)
+// var kmsSvc kmsiface.KMSAPI


Leftover from copying from AWS KMS. This should probably have tests like the AWS KMS keysource does, so we'll probably need something like this for the GCP version.

autrilla · 2017-09-11T20:13:17Z

cloudkms/keysource.go

+	return key.ResourceId
+}
+
+// NewMasterKeyFromResourceId takes an cloud KMS resource id string and returns a new MasterKey for that


s/an cloud KMS/a cloud KMS

autrilla · 2017-09-11T20:17:20Z

cmd/sops/main.go

@@ -91,7 +92,7 @@ func main() {
 	cli.VersionPrinter = printVersion
 	app := cli.NewApp()
 	app.Name = "sops"
-	app.Usage = "sops - encrypted file editor with AWS KMS and GPG support"
+	app.Usage = "sops - encrypted file editor with AWS KMS, google cloud KMS and GPG support"


nit: Capitalize Google Cloud

autrilla · 2017-09-11T20:17:31Z

cmd/sops/main.go

@@ -104,17 +105,23 @@ func main() {
   in the -k flag or in the SOPS_KMS_ARN environment variable.
   (you need valid credentials in ~/.aws/credentials or in your env)

+   To encrypt or decrypt a document with google cloud KMS, specify the


nit: Capitalize Google Cloud

autrilla · 2017-09-11T20:18:41Z

cmd/sops/main.go

@@ -104,17 +105,23 @@ func main() {
   in the -k flag or in the SOPS_KMS_ARN environment variable.
   (you need valid credentials in ~/.aws/credentials or in your env)

+   To encrypt or decrypt a document with google cloud KMS, specify the
+   cloud KMS resource Id in the -c flag or in the SOPS_CLOUD_KMS_IDS


nit: Capitalize Cloud

autrilla · 2017-09-11T20:19:05Z

cmd/sops/main.go

@@ -141,6 +148,11 @@ func main() {
 			EnvVar: "SOPS_KMS_ARN",
 		},
 		cli.StringFlag{
+			Name:   "cloud-kms, c",
+			Usage:  "comma separated list of google cloud KMS resource IDs",


nit: Capitalize Google Cloud

autrilla · 2017-09-11T20:19:18Z

cmd/sops/main.go

@@ -104,17 +105,23 @@ func main() {
   in the -k flag or in the SOPS_KMS_ARN environment variable.
   (you need valid credentials in ~/.aws/credentials or in your env)

+   To encrypt or decrypt a document with google cloud KMS, specify the
+   cloud KMS resource Id in the -c flag or in the SOPS_CLOUD_KMS_IDS


nit: s/Id/ID for consistency, as you write it as ID in the docs

autrilla · 2017-09-11T20:20:48Z

sops.go

@@ -385,6 +386,21 @@ func (m *Metadata) AddPGPMasterKeys(pgpFps string) {
 	}
 }

+// AddCloudKMSMasterKeys parses the input comma separated string of cloud KMS resource IDs, generates a KMS MasterKey for each resource ID, and then adds the keys to the cloud KMS KeySource


Capitalize Cloud

autrilla · 2017-09-11T20:24:07Z

cmd/sops/main.go

@@ -428,7 +454,8 @@ func getKeySources(c *cli.Context, file string) ([]sops.KeySource, error) {
 	}
 	kmsKs := sops.KeySource{Name: "kms", Keys: kmsKeys}
 	pgpKs := sops.KeySource{Name: "pgp", Keys: pgpKeys}
-	return []sops.KeySource{kmsKs, pgpKs}, nil
+	cloudKmsKs := sops.KeySource{Name: "cloud-kms", Keys: cloudKmsKeys}


I think we should use cloud_kms for the key source name, as we currently use snake_case for YAML keys (e.g. unencrypted_suffix).

jvehent · 2017-09-11T20:55:12Z

First there was only one kms, then google introduced their kms and things became confusing. I think we need to name Google's kms gcp-kms and provide aliases from the current kms to aws-kms. That way, if azure creates a kms service, we could just call it ma-kms and everyone would be happy :)

calind · 2017-09-12T11:31:14Z

I think @jvehent has a point. I'll rename everything to gcp_kms since cloud_kms is a little too broad.

Naming things... :)

autrilla · 2017-09-15T16:54:03Z

We've just merged #238 which was a very big PR and has caused conflicts with this one. I'll perform the merge myself, you shouldn't worry about it.

autrilla · 2017-09-15T21:42:16Z

@calind I've performed the merge. I'd appreciate it if you could test this on your own workstation before we merge back to master.

calind · 2017-09-15T22:06:03Z

@autrilla It's broken after rebase but I'll have a look in the following days.

jvehent

Thanks a lot for the patch. The code looks good. It needs some README additions to tell people how to make use of it.

I'm a bit terrified by the size of the vendoring commit here. It vendors everything from xmpp to bigtable, just to call the KMS api. Is there a way to just vendor what we need?

autrilla · 2017-09-16T04:36:30Z

I forgot, but because of the key service this is also going to need a protobuf message for this key type, and handling for that message type will have to be added to the key service server.

calind · 2017-09-16T05:42:37Z

Thanks for the tip. I've managed to generate the protobuf stuff. I'll try finishing the PR this weekend.

Regarding @jvehent comment, I've manually added the go dependencies using govend and specifically included only the kms API stuff. I'll try find a way to reduce it but I'm pretty new to golang and any tip would be helpful.

jvehent · 2017-09-16T12:21:29Z

We vendor using make vendor, which currently only calls govend -u, but could also be used to prune unnecessary packages and reduce the size of the vendor/ directory.

calind · 2017-09-18T10:47:45Z

@autrilla, @jvehent I've updated the PR to take the keyservice into account and I've also added some docs.

Regarding the vendoring what I did was:

govend google.golang.org/api/cloudkms/v1
govend golang.org/x/oauth2/google

I've tried manually removing some folders from /vendor but any govend -u fetches them again.

autrilla · 2017-09-18T15:44:16Z

cmd/sops/main.go

@@ -129,6 +129,10 @@ func main() {
 							Name:  "kms",
 							Usage: "the KMS ARNs the new group should contain. Can be specified more than once",
 						},
+						cli.StringSliceFlag{
+							Name:  "gcp-kms",
+							Usage: "the GCP KMS Resourece ID the new group should contain. Can be specified more than once",


calind · 2017-09-18T15:48:55Z

Fixed.

autrilla · 2017-09-18T16:51:23Z

I've tested the functionality locally with my own GCP KMS key and it seems to be working.

calind · 2017-09-18T16:52:47Z

🍾

Calin Don added 2 commits September 11, 2017 21:51

Vendor google cloud KMS

ada6396

Add google cloud KMS support

0a0a803

autrilla self-requested a review September 11, 2017 18:58

Add basic testing for google cloud KMS

f57598d

autrilla suggested changes Sep 11, 2017

View reviewed changes

Calin Don added 2 commits September 12, 2017 15:37

Rename Cloud KMS to GCP KMS

f7d7244

Add suport for gcp_kms in creation_rules

460b52e

Merge branch 'master' into gcloud-kms

bcf8ade

autrilla force-pushed the gcloud-kms branch from 38243f7 to bcf8ade Compare September 15, 2017 21:34

Fix golint issues

34c7380

autrilla approved these changes Sep 15, 2017

View reviewed changes

jvehent reviewed Sep 16, 2017

View reviewed changes

Add keyservice support

15ea81d

calind force-pushed the gcloud-kms branch 3 times, most recently from 9b35f80 to 55271c1 Compare September 18, 2017 10:12

GCP KMS docs

f3dacbb

calind force-pushed the gcloud-kms branch from 55271c1 to f3dacbb Compare September 18, 2017 10:24

autrilla reviewed Sep 18, 2017

View reviewed changes

Fix typos

8e15b25

Merge branch 'master' into gcloud-kms

c7c94e9

Merge branch 'master' into gcloud-kms

e5f5812

autrilla merged commit ec672ce into getsops:master Sep 19, 2017

autrilla mentioned this pull request Apr 6, 2018

Support Vault for encryption #291

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add google cloud KMS support #245

Add google cloud KMS support #245

calind commented Sep 11, 2017

codecov-io commented Sep 11, 2017 •

edited

autrilla left a comment

autrilla Sep 11, 2017

autrilla Sep 11, 2017

autrilla Sep 11, 2017

autrilla Sep 11, 2017

autrilla Sep 11, 2017

autrilla Sep 11, 2017

autrilla Sep 11, 2017

autrilla Sep 11, 2017

autrilla Sep 11, 2017

jvehent commented Sep 11, 2017

calind commented Sep 12, 2017

autrilla commented Sep 15, 2017

autrilla commented Sep 15, 2017

calind commented Sep 15, 2017

jvehent left a comment

autrilla commented Sep 16, 2017

calind commented Sep 16, 2017

jvehent commented Sep 16, 2017

calind commented Sep 18, 2017

autrilla Sep 18, 2017

calind commented Sep 18, 2017

autrilla commented Sep 18, 2017

calind commented Sep 18, 2017

Add google cloud KMS support #245

Add google cloud KMS support #245

Conversation

calind commented Sep 11, 2017

codecov-io commented Sep 11, 2017 • edited

Codecov Report

autrilla 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

jvehent commented Sep 11, 2017

calind commented Sep 12, 2017

autrilla commented Sep 15, 2017

autrilla commented Sep 15, 2017

calind commented Sep 15, 2017

jvehent left a comment

Choose a reason for hiding this comment

autrilla commented Sep 16, 2017

calind commented Sep 16, 2017

jvehent commented Sep 16, 2017

calind commented Sep 18, 2017

Choose a reason for hiding this comment

calind commented Sep 18, 2017

autrilla commented Sep 18, 2017

calind commented Sep 18, 2017

codecov-io commented Sep 11, 2017 •

edited