Followup: Adding Hashicorp Vault KMS #623

ldue · 2020-02-05T19:29:41Z

Required improvements for PR 507.

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

ldue · 2020-02-05T19:33:56Z

I‘ll try to add some more tests, but please feel free to inspire me for that 🙂

codecov-io · 2020-02-05T19:46:07Z

Codecov Report

Merging #623 into develop will decrease coverage by 0.81%.
The diff coverage is 19.85%.

@@             Coverage Diff             @@
##           develop     #623      +/-   ##
===========================================
- Coverage    37.15%   36.34%   -0.82%     
===========================================
  Files           21       22       +1     
  Lines         2893     3189     +296     
===========================================
+ Hits          1075     1159      +84     
- Misses        1724     1914     +190     
- Partials        94      116      +22

Impacted Files	Coverage Δ
stores/stores.go	`0% <0%> (ø)`	⬆️
keyservice/server.go	`5.26% <0%> (-1.03%)`	⬇️
keyservice/keyservice.go	`0% <0%> (ø)`	⬆️
keyservice/keyservice.pb.go	`4.12% <1.61%> (-0.17%)`	⬇️
hcvault/keysource.go	`48.12% <48.12%> (ø)`
config/config.go	`71.63% <62.5%> (-0.02%)`	⬇️

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 f7e880b...adf1b23. Read the comment docs.

ldue · 2020-02-10T09:37:55Z

I noticed during testing that the Readme maybe should contain some more notes what to do for the functional testing (e.g. enable the vault kv store). @gitirabassi has done this nicely for this PR, but maybe we should do this reworks in a separate PR and maybe also decide with that one, if the dependency for vault while unit testing should be kept.

ajvb · 2020-02-11T21:03:51Z

@ldue I completely agree on the lack of docs for functional tests. If you're willing to create an issue with any notes on what specifically was hard to find I'd be happy to work on it.

autrilla

Looks great, thanks a lot!

ldue · 2020-02-18T08:18:11Z

@ajvb yeah sure, I think I will get to this in the next week.
@autrilla thanks for the review! So then it's ready to merge?

autrilla · 2020-02-18T08:21:09Z

Ideally I’d want @ajvb to give it a look as well before merging. I’d also really appreciate the notes on what kind of tests and docs you find lacking!

README.rst

mmorev

@ldue why do you use URI string for single key configuration in .sops.yaml?

I think it would be more clear and definite to use separate parameters in sops.yml, just like these you offer in key_groups.hc_vault.

However, single-string URI is still very convenient for command line options like --add-hc-vault.

Also I would suggest rename hc-vault to vault-transit, because it is common name for this key management service type.

Overall, great job! I was really looking forward for this feature.

mmorev · 2020-02-18T20:38:11Z

config/config.go

@@ -110,6 +118,7 @@ type creationRule struct {
 	PGP               string
 	GCPKMS            string     `yaml:"gcp_kms"`
 	AzureKeyVault     string     `yaml:"azure_keyvault"`
+	Vault             string     `yaml:"hc_vault_uris"`


this way we can use both methods

Suggested change

Vault string `yaml:"hc_vault_uris"`

VaultUri string `yaml:"hc_vault_uris"`

Vault []vaultKey `yaml:"hc_vault"`

As mentioned elsewhere, I think we should keep the URI, but actually, please include Hashicorp and Transit in the name somehow. HcVaultTransit, for example. I'm open to alternatives, but let's keep it consistent everywhere.

I've adapted you suggestions thanks! I also changed the plural hc_vault_uris to singular hc_vault_uri

stores/stores.go

hcvault/keysource.go

hcvault/keysource_test.go

config/config_test.go

config/config.go

vault_example.yml

config/config_test.go

autrilla · 2020-02-18T23:21:57Z

I think it would be more clear and definite to use separate parameters in sops.yml, just like these you offer in key_groups.hc_vault.

However, single-string URI is still very convenient for command line options like --add-hc-vault.

I kind of disagree here, I think consistency is best and this is pretty similar to what we do for AWS: we take an ARN. We do the same for GCP, and probably Azure (but I haven't checked). The URI is a "standard" way to refer to things in Vault, no?

Also I would suggest rename hc-vault to vault-transit, because it is common name for this key management service type.

I completely agree with this. Probably even hc-vault-transit. Maybe we'll need different backends that are also provided by Hashicorp Vault in the future, and the concern about a different product also named Vault (by another company) still applies.

Thanks for giving this a look too @mmorev!

mmorev · 2020-02-19T05:46:48Z

I kind of disagree .here, I think consistency is best and this is pretty similar to what we do for AWS: we take an ARN.

ARN is much more definite because it uses colons as field separator, so if any required field is missing or incorrect, we can easily find out, which exactly. Even we can safely skip unnecessary field.
URIs are much more ambiguous. Vault can be behind reverse proxy and/or WAF, or path to secret store can contain slashes (can not as of now, but other stores can) - voila, URI is not consistent.
Also mentioned /v1/store_path/keys/key_name is not exact URI path used when calling Vault API. It can be v1/store_path/encrypt/key_name for encrypting data, for example.

So I had suggested to implement both methods - URI and separate fields.

Also @ldue please rename backend_path to secret_path or something else in code and parameters, because backend in Vault terms is actually the storage for all configured vault secrets,.

Probably even hc-vault-transit.

hc- prefix wasn't used when Vault publish was implemented, why should here?

additional to URIs

ldue · 2020-02-26T13:42:00Z

Thanks for your great feedback! Sorry it took me so long to get back to it.

@autrilla i also aggree with @mmorev it makes sense to implement both ways, uri based and "object" based config. I do not see to much disadvantage.

@mmorev I renamed BackendPath to EnginePath as I find secrets somehow ambiguous and as the vault term is "secrets engine" i think it fits.

I think the hc- prefix came exactly from some confusion around AzureKeyVault, the general concept of a vault for keys and (hashicorp) Vault, when implementing the publish feature.
I think it should be refactored for the publish feature, but not with this PR.
You can check the original PR 507 for that.

autrilla · 2020-02-27T20:32:53Z

ARN is much more definite because it uses colons as field separator, so if any required field is missing or incorrect, we can easily find out, which exactly. Even we can safely skip unnecessary field.
URIs are much more ambiguous. Vault can be behind reverse proxy and/or WAF, or path to secret store can contain slashes (can not as of now, but other stores can) - voila, URI is not consistent.
Also mentioned /v1/store_path/keys/key_name is not exact URI path used when calling Vault API. It can be v1/store_path/encrypt/key_name for encrypting data, for example.

It is in no way different than an ARN. It's a URI, but not any URI. It must match a very specific format. Just like an ARN.

The issue with reverse proxies is irrelevant here. The URI is just a way to specify the resource. It does not imply we're going to make HTTP requests to that URI directly. In fact, we're parsing it and passing it off to the Vault library.

An argument against this that I would accept is that this "URI" is not really an exclusive identifier Vault uses for the resource. It's just the way to access it through the HTTP API, but there might be other ways to access it in the future. So maybe it does make sense to take explicitly structured data instead of this URI.

So I had suggested to implement both methods - URI and separate fields.

This is absolutely what I do not want. I do not want two ways of doing the same thing. It makes the code more complicated and a pain to maintain. I don't care super strongly about what is done regarding URI vs more structured data in the config file, but let's do only one thing. The added value is tiny and the maintenance cost adds up very quickly.

hc- prefix wasn't used when Vault publish was implemented, why should here?

I think it's the right thing to do regardless of what we've done in the past. Clearly we've made some mistakes with naming. AWS KMS being named just "kms" is probably the biggest one.

ldue · 2020-02-28T12:27:38Z

An argument against this that I would accept is that this "URI" is not really an exclusive identifier Vault uses for the resource. It's just the way to access it through the HTTP API, but there might be other ways to access it in the future. So maybe it does make sense to take explicitly structured data instead of this URI.

I think in that case URI would be more consistent to the other providers.

The added value is tiny and the maintenance cost adds up very quickly.

I get that, I do not have any experience with maintaining code for that kind of config.

So I'll remove the structured config and leave VaultURI in and then we can bring this to an end? 😄

vnzongzna · 2020-03-02T14:41:37Z

cmd/sops/main.go

@@ -501,6 +523,11 @@ func main() {
 			Usage:  "comma separated list of Azure Key Vault URLs",
 			EnvVar: "SOPS_AZURE_KEYVAULT_URLS",
 		},
+		cli.StringFlag{
+			Name:   "hc-vault--transit",


Suggested change

Name: "hc-vault--transit",

Name: "hc-vault-transit",

autrilla · 2020-03-03T19:17:13Z

I think in that case URI would be more consistent to the other providers.
I get that, I do not have any experience with maintaining code for that kind of config.
So I'll remove the structured config and leave VaultURI in and then we can bring this to an end? 😄

Sounds good to me!

mmorev · 2020-03-03T19:42:45Z

@autrilla IMO what really makes code more complicated and harder to read/maintain is a function with two unobvious regular expressions which is needed to parse an URI in unobvious way and then, anyway, store result as three different values in sops metadata.
Maybe, my suggestion doesnt add value, but it does add logic and readability to source code and to users configuration files.

Anyway, it's just a suggestion, you are maintainer, you are decision maker.

autrilla · 2020-03-03T19:48:12Z

We wouldn't store the three values in SOPS metadata, we'd store just the URI.

Like I said, I'd be open to storing only the three values and not dealing with URIs at all. What I don't want is both things. Asking for three different values from the command line might be a bit annoying and clutter our flags, though.

mmorev · 2020-03-03T20:28:56Z

Oh, got it. @ldue What do you think about removing URI and regex parsing function and leaving just separate parameter set?

vnzongzna · 2020-03-10T19:55:50Z

cmd/sops/main.go

@@ -961,12 +1012,21 @@ func keyGroups(c *cli.Context, file string) ([]sops.KeyGroup, error) {
 			azkvKeys = append(azkvKeys, k)
 		}
 	}
+	if c.String("hc-vault-transit") != "" {
+		hcVaultKeys, err := hcvault.NewMasterKeysFromURIs(c.String("vault"))


Suggested change

hcVaultKeys, err := hcvault.NewMasterKeysFromURIs(c.String("vault"))

hcVaultKeys, err := hcvault.NewMasterKeysFromURIs(c.String("hc-vault-transit"))

vnzongzna · 2020-03-10T19:56:09Z

cmd/sops/main.go

 	if c.String("pgp") != "" {
 		for _, k := range pgp.MasterKeysFromFingerprintString(c.String("pgp")) {
 			pgpKeys = append(pgpKeys, k)
 		}
 	}
-	if c.String("kms") == "" && c.String("pgp") == "" && c.String("gcp-kms") == "" && c.String("azure-kv") == "" {
+	if c.String("kms") == "" && c.String("pgp") == "" && c.String("gcp-kms") == "" && c.String("azure-kv") == "" && c.String("vault") == "" {


Suggested change

if c.String("kms") == "" && c.String("pgp") == "" && c.String("gcp-kms") == "" && c.String("azure-kv") == "" && c.String("vault") == "" {

if c.String("kms") == "" && c.String("pgp") == "" && c.String("gcp-kms") == "" && c.String("azure-kv") == "" && c.String("hc-vault-transit") == "" {

binlab · 2020-03-15T00:12:00Z

Great work, thanks a lot!

jvehent · 2020-03-29T13:05:12Z

@ajvb @autrilla can this be merged?

autrilla · 2020-03-29T13:40:23Z

There's still some review comments that should be addressed, mainly only allowing one of specifying the Vault URI or specifying each value (hostname, key, etc) individually.

jvehent

per @autrilla

There's still some review comments that should be addressed, mainly only allowing one of specifying the Vault URI or specifying each value (hostname, key, etc) individually.

need to address review comments

vnzongzna · 2020-04-19T15:39:14Z

Would it be acceptable to make a new PR since we need vault feature in SOPS and this PR has been inactive for more than 2 months now.
The new PR would be third iteration for the same feature but I'll be working on it continuously.
cc @ldue @jvehent @autrilla @mmorev

autrilla · 2020-04-19T16:39:22Z

Would it be acceptable to make a new PR since we need vault feature in SOPS and this PR has been inactive for more than 2 months now.
The new PR would be third iteration for the same feature but I'll be working on it continuously.
cc @ldue @jvehent @autrilla @mmorev

Yes, that is acceptable as long as you keep authorship of the original commits. I think this PR itself started that way as well...

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

This can be closed now that #655 has been merged

gitirabassi and others added 6 commits February 5, 2020 19:15

feat: initial adding of vualt transit backend to sops

a8bc111

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

7004d5c

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

fix(vault): addressed typos and fixes from autrilla

37d68f1

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

fix(test): typo while rebasing

b5a8928

fix typos and imporve error messages for vault kms

9256361

rename package from vault to hcvault

5350855

ldue mentioned this pull request Feb 5, 2020

Adding Hashicorp Vault KMS #507

Closed

davidovich mentioned this pull request Feb 10, 2020

Vault Jsonnet Native Function grafana/tanka#101

Closed

ldue added 2 commits February 10, 2020 09:33

refactor vault keysource url validation

b68f1f2

add negative test cases for vault keysource

adf1b23

ldue marked this pull request as ready for review February 10, 2020 09:01

ldue requested a review from ajvb February 11, 2020 10:17

ajvb requested a review from autrilla February 11, 2020 20:58

autrilla previously approved these changes Feb 14, 2020

View reviewed changes

mmorev reviewed Feb 18, 2020

View reviewed changes

README.rst Outdated Show resolved Hide resolved

mmorev suggested changes Feb 18, 2020

View reviewed changes

config/config_test.go Outdated Show resolved Hide resolved

ldue added 4 commits February 26, 2020 14:12

add hc vault transit config option via objects

086b91f

additional to URIs

remove vault_example.yml

d6885b3

streamline key name to snake case

3372156

rename BackendPath to EnginePath for hc vault

201f661

ldue force-pushed the feat/hashicorp-vault branch from d2b9309 to 201f661 Compare February 26, 2020 13:33

ldue requested a review from autrilla February 27, 2020 17:41

vnzongzna reviewed Mar 2, 2020

View reviewed changes

vnzongzna reviewed Mar 10, 2020

View reviewed changes

binlab mentioned this pull request Mar 15, 2020

Support Vault for encryption #291

Open

jvehent suggested changes Mar 29, 2020

View reviewed changes

This was referenced Apr 19, 2020

adding vault transit engine to sops #654

Closed

Adding Hashicorp vault support #655

Merged

ajvb closed this Feb 24, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Followup: Adding Hashicorp Vault KMS #623

Followup: Adding Hashicorp Vault KMS #623

ldue commented Feb 5, 2020

ldue commented Feb 5, 2020

codecov-io commented Feb 5, 2020 •

edited

Loading

ldue commented Feb 10, 2020

ajvb commented Feb 11, 2020

autrilla left a comment

ldue commented Feb 18, 2020

autrilla commented Feb 18, 2020

mmorev left a comment •

edited

Loading

mmorev Feb 18, 2020 •

edited

Loading

autrilla Feb 18, 2020

ldue Feb 26, 2020

autrilla commented Feb 18, 2020

mmorev commented Feb 19, 2020 •

edited

Loading

ldue commented Feb 26, 2020

autrilla commented Feb 27, 2020

ldue commented Feb 28, 2020

vnzongzna Mar 2, 2020

autrilla commented Mar 3, 2020

mmorev commented Mar 3, 2020

autrilla commented Mar 3, 2020 •

edited

Loading

mmorev commented Mar 3, 2020

vnzongzna Mar 10, 2020

vnzongzna Mar 10, 2020

binlab commented Mar 15, 2020

jvehent commented Mar 29, 2020

autrilla commented Mar 29, 2020

jvehent left a comment

vnzongzna commented Apr 19, 2020 •

edited

Loading

autrilla commented Apr 19, 2020

mykter commented Jul 2, 2020

	Vault string `yaml:"hc_vault_uris"`
	VaultUri string `yaml:"hc_vault_uris"`
	Vault []vaultKey `yaml:"hc_vault"`

	hcVaultKeys, err := hcvault.NewMasterKeysFromURIs(c.String("vault"))
	hcVaultKeys, err := hcvault.NewMasterKeysFromURIs(c.String("hc-vault-transit"))

	if c.String("kms") == "" && c.String("pgp") == "" && c.String("gcp-kms") == "" && c.String("azure-kv") == "" && c.String("vault") == "" {
	if c.String("kms") == "" && c.String("pgp") == "" && c.String("gcp-kms") == "" && c.String("azure-kv") == "" && c.String("hc-vault-transit") == "" {

Followup: Adding Hashicorp Vault KMS #623

Followup: Adding Hashicorp Vault KMS #623

Conversation

ldue commented Feb 5, 2020

ldue commented Feb 5, 2020

codecov-io commented Feb 5, 2020 • edited Loading

Codecov Report

ldue commented Feb 10, 2020

ajvb commented Feb 11, 2020

autrilla left a comment

Choose a reason for hiding this comment

ldue commented Feb 18, 2020

autrilla commented Feb 18, 2020

mmorev left a comment • edited Loading

Choose a reason for hiding this comment

mmorev Feb 18, 2020 • edited Loading

Choose a reason for hiding this comment

autrilla Feb 18, 2020

Choose a reason for hiding this comment

ldue Feb 26, 2020

Choose a reason for hiding this comment

autrilla commented Feb 18, 2020

mmorev commented Feb 19, 2020 • edited Loading

ldue commented Feb 26, 2020

autrilla commented Feb 27, 2020

ldue commented Feb 28, 2020

vnzongzna Mar 2, 2020

Choose a reason for hiding this comment

autrilla commented Mar 3, 2020

mmorev commented Mar 3, 2020

autrilla commented Mar 3, 2020 • edited Loading

mmorev commented Mar 3, 2020

vnzongzna Mar 10, 2020

Choose a reason for hiding this comment

vnzongzna Mar 10, 2020

Choose a reason for hiding this comment

binlab commented Mar 15, 2020

jvehent commented Mar 29, 2020

autrilla commented Mar 29, 2020

jvehent left a comment

Choose a reason for hiding this comment

vnzongzna commented Apr 19, 2020 • edited Loading

autrilla commented Apr 19, 2020

mykter commented Jul 2, 2020

codecov-io commented Feb 5, 2020 •

edited

Loading

mmorev left a comment •

edited

Loading

mmorev Feb 18, 2020 •

edited

Loading

mmorev commented Feb 19, 2020 •

edited

Loading

autrilla commented Mar 3, 2020 •

edited

Loading

vnzongzna commented Apr 19, 2020 •

edited

Loading