Adding Hashicorp Vault KMS #507

gitirabassi · 2019-08-07T16:03:16Z

No description provided.

codecov-io · 2019-08-07T16:10:01Z

Codecov Report

Merging #507 into develop will decrease coverage by 0.73%.
The diff coverage is 18.97%.

@@            Coverage Diff             @@
##           develop    #507      +/-   ##
==========================================
- Coverage    35.84%   35.1%   -0.74%     
==========================================
  Files           20      21       +1     
  Lines         2832    3111     +279     
==========================================
+ Hits          1015    1092      +77     
- Misses        1722    1901     +179     
- Partials        95     118      +23

Impacted Files	Coverage Δ
keyservice/server.go	`5.26% <0%> (-1.03%)`	⬇️
stores/stores.go	`0% <0%> (ø)`	⬆️
keyservice/keyservice.go	`0% <0%> (ø)`	⬆️
keyservice/keyservice.pb.go	`4.12% <1.61%> (-0.17%)`	⬇️
vault/keysource.go	`48.95% <48.95%> (ø)`
config/config.go	`64.39% <62.5%> (+0.39%)`	⬆️

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 757884b...2359d2f. Read the comment docs.

mumoshu

I was about to send my own pull request to add this feature, and realized that you were faster than me!

And yours looks MUCH better than mine, in terms of flexibility - more configuration options like backend path.

Only thing I'd comment is to add some usage on this feature around L90 in main.go, like mumoshu@8f858c2#diff-c1be616053300bd7c4176b526f9ad9c1R90 and mumoshu@8f858c2#diff-c1be616053300bd7c4176b526f9ad9c1R102.

Other than that, this LGTM! Really looking forward to see this gets merged 🎉

cmd/sops/main.go

mumoshu · 2019-08-08T05:21:59Z

Manually verified that this works 🎉

FWIW, this is the steps I've used for testing:

Start the Vault dev-server and set VAULT_DEV_ROOT_TOKEN_ID according to https://learn.hashicorp.com/vault/getting-started/dev-server
Enable the transit secrets engine by running vault secrets enable transit
Create a master key named my-key by running vault write -f transit/keys/my-key

Run:

$ VAULT_TOKEN=$VAULT_DEV_ROOT_TOKEN_ID SOPS_VAULT_URIS=http://127.0.0.1:8200/v1/transit/keys/my-key ~/go/bin/sops --verbose -e raw.yaml > encrypted.yaml`

$ VAULT_TOKEN=$VAULT_DEV_ROOT_TOKEN_ID SOPS_VAULT_URIS=http://127.0.0.1:8200/v1/transit/keys/my-key ~/go/bin/sops --verbose -d encrypted.yaml  > decrypted.yaml

# See no diff detected! (=Successfully decrypted
diff --unified raw.yaml decrypted.yaml

gitirabassi · 2019-08-08T10:02:07Z

Thanks @mumoshu.
I've added steps to test and use in the README.rst so everyone knows how to do get started with it. and just copy paster the docs from your cmd/sops/main.go: your doc was much better than mine!

gitirabassi · 2019-08-09T12:17:01Z

hashicorp/vault#7289

jvehent · 2019-08-09T12:53:16Z

I know the devops community currently associates the word vault with Hashicorp's, but it's also a genering term used in many different context in security. I'd prefer if this used the namespace hcvault, or something along those lines, in case we want to use some other vendor's vault product in the future.

(I made that mistake with kms in the early days which should really be awskms)

mumoshu · 2019-08-09T13:11:41Z

@jvehent That makes sense! I had similar feeling when I was coding my own version of this feature.

We currently use --azure-kv for Azure KeyVault. If we exactly follow that, we can name it --hashicorp-v, but this doesn't make sense of course. --hashicorp-vault seems a bit verbose and inconsistent with others.

And to be consistent with --gcp-kms and --azure-kv, I think --kebab-case might be better. So how about --hc-vault?

gitirabassi · 2019-08-09T13:23:04Z

Makes sense, can we all agree on —hc-vault so that i can make the change today?

…

On 9 Aug 2019, at 15:11, KUOKA Yusuke ***@***.***> wrote: @jvehent That makes sense! I had similar feeling when I was coding my own version of this feature. We currently use --azure-kv for Azure KeyVault. If we exactly follow that, we can name it --hashicorp-v, but this doesn't make sense of course. --hashicorp-vault seems a bit verbose and inconsistent with others. And to be consistent with --gcp-kms and --azure-kv, I think --kebab-case might be better. So how about --hc-vault? — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub, or mute the thread.

autrilla · 2019-08-09T15:01:14Z

Makes sense, can we all agree on —hc-vault so that i can make the change today?

That seems fine to me. I'll do a full review next week, probably on Monday.

jvehent · 2019-08-09T18:21:50Z

+1 for --hc-vault

gitirabassi · 2019-08-11T09:21:02Z

This weekend I wasn’t able to make the desired changes and I’m leaving for 2 weeks on vacations. I’ll be able to make those changes right away once I come back

…

On Aug 9, 2019, at 20:21, Julien Vehent [:ulfr] ***@***.***> wrote: +1 for --hc-vault — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#507>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AFFQIVHZRFYDGN5ACUULSWDQDWYUDANCNFSM4IKB7OOQ>.

autrilla

Looks good to me overall, just some nits. It'd be good to change the naming to mention HashiCorp in some way as discussed.

autrilla · 2019-08-12T11:41:04Z

vault/keysource_test.go

+	}
+
+	// pulls an image, creates a container based on it and runs it
+	resource, err := pool.Run("vault", "1.1.3", []string{"VAULT_DEV_ROOT_TOKEN_ID=secret"})


I think I like this. We should probably do it for our other vault tests too

vault/keysource.go

autrilla · 2019-08-12T11:49:26Z

vault/keysource.go

+// This is simply copied from GCPKMS
+// TODO: handle key rotation on vault side
+func (key *MasterKey) NeedsRotation() bool {
+	//TODO: manage rewrapping https://www.vaultproject.io/api/secret/transit/index.html#rewrap-data


That's interesting! I think it'd require changes to the MasterKey interface to make it useful, but it's good to know that there's encryption backends that allow this.

keyservice/keyservice.proto

README.rst

ajvb · 2019-08-14T18:09:31Z

As to not duplicate effort, once autrilla's code review is addressed I'll dive into a review. Thanks for this awesome contribution @gitirabassi!

mumoshu · 2019-09-04T05:42:02Z

@gitirabassi I'm still looking forward to make this happen. Please let me know if you need any help 😃

initial work on integration feat(vault): added cli coomands working for vualt" fix(vault): fixed config with correct tests fix(vault): added vault to keygroup and to keyservice server fixed metadata load

fix(doc): fix rst formatting" fix(doc): fix rst formatting

feat(cli): moved vault to hc-vault naming

gitirabassi · 2019-09-04T08:48:16Z

@mumoshu @autrilla I fixed the cli and interfaces as described

mumoshu · 2019-09-05T04:50:04Z

@ajvb Now that autrilla's code review is addressed, would you mind reviewing this? 😃

ajvb

This is looking awesome @gitirabassi ! Thank you again for this PR.

One piece I mentioned is make sure there are functional tests for the cli. You can run them locally with make functional-tests and they are written in Rust here - https://github.com/mozilla/sops/blob/master/functional-tests/src/lib.rs. If you'd like to take a crack at adding one, that would be great! If not, no worries, I would be happy to add some as a follow-up.

ajvb · 2019-09-05T21:37:03Z

README.rst

+	Cluster ID      e532e461-e8f0-1352-8a41-fc7c11096908
+	HA Enabled      false
+
+	$ # We need to enable a transit engine if not already done (I prefer to create a transit engine specifically for sops, in which I can have multiple keys with various permission levels)


I prefer... - Let's change this from first person to more directional, i.e. It is suggested to..., etc

ajvb · 2019-09-05T21:42:48Z

README.rst

+	HA Enabled      false
+
+	$ # We need to enable a transit engine if not already done (I prefer to create a transit engine specifically for sops, in which I can have multiple keys with various permission levels)
+	$ vault secrets enable -path=sops transit


What does the error message look like if HC vault is not configured properly before trying to use it? Is the error message passed down to the user?

ajvb · 2019-09-05T21:48:15Z

vault/keysource_test.go

+	}
+
+	// pulls an image, creates a container based on it and runs it
+	resource, err := pool.Run("vault", "1.2.2", []string{"VAULT_DEV_ROOT_TOKEN_ID=secret"})


This is pretty awesome imo, but we might want to change this. In the develop branch, we currently are running vault within Travis CI (https://github.com/mozilla/sops/blob/develop/.travis.yml#L22-L24). This is currently only being used for functional tests (which we should also consider adding either in this PR or in a follow-up), but no reason it can't be used and required for unit tests. @autrilla do you agree?

I don't think it's great to require people to install Vault to run unit tests. Personally I feel it's better to have the tests pull down any required dependencies if possible -- if we could, we should probably make the functional tests set up a temporary Vault instance as well, so users don't have to worry about their environment.

ajvb · 2019-09-05T21:52:52Z

vault/keysource.go

+	}
+	log.Debugf("Path: %v \nValues: %v\n", fullPath, values)
+	if values[0] != "v1" {
+		return "", "", fmt.Errorf("probably forgot v1 in the URI")


I'd consider making this more generic and similar to the len() check: Vault path does not seem to be formatted correctly (eg. https://vault.example.com:8200/v1/transit/keys/keyName)

ajvb · 2019-09-05T21:53:06Z

vault/keysource.go

+	values := strings.Split(fullPath, "/")
+	// minimum length should be 4 "v1", "transit", "keys", "keyName"
+	if len(values) < 4 {
+		return "", "", fmt.Errorf("The path to the key is not long enough: (eg. https://vault.example.com:8200/v1/transit/keys/keyName")


missing a closing ) in the string.

ajvb · 2019-09-05T21:53:28Z

vault/keysource.go

+		return "", "", fmt.Errorf("probably forgot v1 in the URI")
+	}
+	if values[len(values)-2] != "keys" {
+		return "", "", fmt.Errorf("probably forgot 'keys' in the URI")


Same here in regards to making the error message more generic.

ajvb · 2019-09-05T21:53:48Z

vault/keysource.go

+		return nil, err
+	}
+	if u.Scheme == "" {
+		return nil, fmt.Errorf("missing scheme in vault URL (should be like this: https://vault.example.com:8200/v1/transit/keys/keyName, got: %v", uri)


Missing closing ) in string

ajvb · 2019-09-05T21:57:33Z

vault/keysource_test.go

+	"github.com/ory/dockertest"
+	"github.com/stretchr/testify/assert"
+)
+


These tests look great, but it would be nice to see some failure test cases. What happens if the vault address is incorrect? Of the path? etc.

ajvb · 2019-09-05T22:01:18Z

vault/keysource.go

@@ -0,0 +1,272 @@
+package vault


I believe it would be worth renaming this package to hcvault to bring it into alignment with the CLI and from our learned lesson on having kms and gcpkms packages.

ghost · 2019-11-30T19:31:25Z

Is it planned to merge it ? Because without this merge, it's impossible to use sops for internal use (no cloud)

autrilla · 2019-11-30T19:36:52Z

Is it planned to merge it ? Because without this merge, it's impossible to use sops for internal use (no cloud)

Well, if the review comments are addressed, yes. Since this seems somewhat stale, feel free to open a new PR based on this branch addressing all the comments and resolving merge conflicts.

peterloron · 2020-01-08T04:22:25Z

@gitirabassi Hi! Might you have a chance to pick this up again? Would be awesome to see this merged!

gitirabassi · 2020-01-08T19:25:52Z

I'm hoping to get this done by the end of the month

ldue · 2020-02-05T19:30:38Z

I added a PR based on this one an tried to fix the points from the review: #623

binlab · 2020-03-15T00:13:14Z

Awesome, thank you!

mykter · 2020-07-02T14:58:46Z

This can be closed now that #655 has been merged.

mumoshu approved these changes Aug 8, 2019

View reviewed changes

mumoshu reviewed Aug 8, 2019

View reviewed changes

cmd/sops/main.go Outdated Show resolved Hide resolved

jvehent requested review from ajvb and autrilla August 9, 2019 12:49

autrilla reviewed Aug 12, 2019

View reviewed changes

gakonst mentioned this pull request Aug 26, 2019

Improve key storage approach interledger/interledger-rs#108

Closed

gitirabassi added 4 commits September 4, 2019 09:53

feat: initial adding of vualt transit backend to sops

6d60b77

initial work on integration feat(vault): added cli coomands working for vualt" fix(vault): fixed config with correct tests fix(vault): added vault to keygroup and to keyservice server fixed metadata load

feat(docs): added docs in README.md and in command help

eb5cb91

fix(doc): fix rst formatting" fix(doc): fix rst formatting

fix(vault): addressed typos and fixes from autrilla

7cb3f8f

feat(cli): moved vault to hc-vault naming

fix(test): typo while rebasing

399b139

ajvb reviewed Sep 5, 2019

View reviewed changes

ldue mentioned this pull request Feb 5, 2020

Followup: Adding Hashicorp Vault KMS #623

Closed

gitirabassi closed this Jul 2, 2020

Adding Hashicorp Vault KMS #507

Adding Hashicorp Vault KMS #507

Conversation

gitirabassi commented Aug 7, 2019

codecov-io commented Aug 7, 2019 • edited Loading

Codecov Report

mumoshu left a comment

Choose a reason for hiding this comment

mumoshu commented Aug 8, 2019

gitirabassi commented Aug 8, 2019

gitirabassi commented Aug 9, 2019

jvehent commented Aug 9, 2019

mumoshu commented Aug 9, 2019

gitirabassi commented Aug 9, 2019 via email

autrilla commented Aug 9, 2019

jvehent commented Aug 9, 2019

gitirabassi commented Aug 11, 2019 via email

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

ajvb commented Aug 14, 2019

mumoshu commented Sep 4, 2019

gitirabassi commented Sep 4, 2019

mumoshu commented Sep 5, 2019 • edited Loading

ajvb 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

ghost commented Nov 30, 2019

autrilla commented Nov 30, 2019

peterloron commented Jan 8, 2020

gitirabassi commented Jan 8, 2020

ldue commented Feb 5, 2020

binlab commented Mar 15, 2020

mykter commented Jul 2, 2020

codecov-io commented Aug 7, 2019 •

edited

Loading

mumoshu commented Sep 5, 2019 •

edited

Loading