From d678df9e0800cdbad50bde31f36744c41b12c975 Mon Sep 17 00:00:00 2001 From: Clint Shryock Date: Tue, 6 Nov 2018 14:14:30 -0600 Subject: [PATCH 1/6] regression test for vault kv2 --- runner_test.go | 88 ++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 88 insertions(+) create mode 100644 runner_test.go diff --git a/runner_test.go b/runner_test.go new file mode 100644 index 00000000..ae091030 --- /dev/null +++ b/runner_test.go @@ -0,0 +1,88 @@ +package main + +import ( + "fmt" + "testing" + + "github.com/hashicorp/consul-template/config" + "github.com/hashicorp/consul-template/dependency" + "github.com/y0ssar1an/q" +) + +func TestRunner_appendSecrets(t *testing.T) { + t.Parallel() + + cases := []struct { + name string + f []string + e *Config + path string + data *dependency.Secret + err bool + }{ + { + "kv1_secret", + []string{"-secret", "kv/bar"}, + &Config{ + Secrets: &PrefixConfigs{ + &PrefixConfig{ + Path: config.String("kv/bar"), + }, + }, + }, + "kv/bar", + &dependency.Secret{ + Data: map[string]interface{}{ + "key_field": "some_value", + }, + }, + false, + }, + { + "kv2_secret", + []string{"-secret", "secret/data/foo"}, + &Config{ + Secrets: &PrefixConfigs{ + &PrefixConfig{ + Path: config.String("Secret/data/foo"), + }, + }, + }, + "secret/data/foo", + &dependency.Secret{ + Data: map[string]interface{}{ + "metadata": map[string]interface{}{ + "deletion_time": "", + "destroyed": bool(false), + "version": "1", + "created_time": "2018-11-06T16:43:59.705051Z", + }, + "data": map[string]interface{}{ + "kvVersion": "kv2", + }, + }, + }, + false, + }, + } + + for i, tc := range cases { + t.Run(fmt.Sprintf("%d_%s", i, tc.name), func(t *testing.T) { + c := DefaultConfig().Merge(tc.e) + r, err := NewRunner(c, true) + if err != nil { + t.Fatal(err) + } + vrq, err := dependency.NewVaultReadQuery(tc.path) + if err != nil { + t.Fatal(err) + } + env := make(map[string]string) + appendError := r.appendSecrets(env, vrq, tc.data) + if appendError != nil { + t.Fatalf("got err: %s", appendError) + } + q.Q("what is end env:", env) + }) + } +} From 64dcc7e861494b9bf8d8c0c8c059320b84c3e195 Mon Sep 17 00:00:00 2001 From: Clint Shryock Date: Tue, 6 Nov 2018 14:24:06 -0600 Subject: [PATCH 2/6] fix typo --- runner_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/runner_test.go b/runner_test.go index ae091030..c756858a 100644 --- a/runner_test.go +++ b/runner_test.go @@ -44,7 +44,7 @@ func TestRunner_appendSecrets(t *testing.T) { &Config{ Secrets: &PrefixConfigs{ &PrefixConfig{ - Path: config.String("Secret/data/foo"), + Path: config.String("secret/data/foo"), }, }, }, From 60bce0e46351d1c55c66e4f03facd8384582b2de Mon Sep 17 00:00:00 2001 From: Clint Shryock Date: Tue, 6 Nov 2018 14:24:36 -0600 Subject: [PATCH 3/6] fix printf format error --- runner.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/runner.go b/runner.go index d21c2b5a..e4f32b65 100644 --- a/runner.go +++ b/runner.go @@ -195,7 +195,7 @@ func (r *Runner) Stop() { r.stopChild() if err := r.deletePid(); err != nil { - log.Printf("[WARN] (runner) could not remove pid at %q: %s", + log.Printf("[WARN] (runner) could not remove pid at %#v: %s", r.config.PidFile, err) } From 6f09eb399da571472eae4c799ec3348bea7dfdd6 Mon Sep 17 00:00:00 2001 From: Clint Shryock Date: Tue, 6 Nov 2018 14:49:58 -0600 Subject: [PATCH 4/6] refactor test --- runner_test.go | 71 +++++++++++++++++++++++++------------------------- 1 file changed, 36 insertions(+), 35 deletions(-) diff --git a/runner_test.go b/runner_test.go index c756858a..4760cecf 100644 --- a/runner_test.go +++ b/runner_test.go @@ -2,63 +2,42 @@ package main import ( "fmt" + "strings" "testing" "github.com/hashicorp/consul-template/config" "github.com/hashicorp/consul-template/dependency" - "github.com/y0ssar1an/q" ) func TestRunner_appendSecrets(t *testing.T) { t.Parallel() - cases := []struct { - name string - f []string - e *Config + secretValue := "somevalue" + + cases := map[string]struct { path string data *dependency.Secret err bool }{ - { - "kv1_secret", - []string{"-secret", "kv/bar"}, - &Config{ - Secrets: &PrefixConfigs{ - &PrefixConfig{ - Path: config.String("kv/bar"), - }, - }, - }, + "kv1_secret": { "kv/bar", &dependency.Secret{ Data: map[string]interface{}{ - "key_field": "some_value", + "key_field": secretValue, }, }, false, }, - { - "kv2_secret", - []string{"-secret", "secret/data/foo"}, - &Config{ - Secrets: &PrefixConfigs{ - &PrefixConfig{ - Path: config.String("secret/data/foo"), - }, - }, - }, + "kv2_secret": { "secret/data/foo", &dependency.Secret{ Data: map[string]interface{}{ "metadata": map[string]interface{}{ - "deletion_time": "", - "destroyed": bool(false), - "version": "1", - "created_time": "2018-11-06T16:43:59.705051Z", + "destroyed": bool(false), + "version": "1", }, "data": map[string]interface{}{ - "kvVersion": "kv2", + "key_field": secretValue, }, }, }, @@ -66,9 +45,16 @@ func TestRunner_appendSecrets(t *testing.T) { }, } - for i, tc := range cases { - t.Run(fmt.Sprintf("%d_%s", i, tc.name), func(t *testing.T) { - c := DefaultConfig().Merge(tc.e) + for name, tc := range cases { + t.Run(fmt.Sprintf("%s", name), func(t *testing.T) { + cfg := Config{ + Secrets: &PrefixConfigs{ + &PrefixConfig{ + Path: config.String(tc.path), + }, + }, + } + c := DefaultConfig().Merge(&cfg) r, err := NewRunner(c, true) if err != nil { t.Fatal(err) @@ -82,7 +68,22 @@ func TestRunner_appendSecrets(t *testing.T) { if appendError != nil { t.Fatalf("got err: %s", appendError) } - q.Q("what is end env:", env) + + if len(env) > 1 { + t.Fatalf("Expected only 1 value in this test") + } + + keyName := tc.path + "_key_field" + keyName = strings.Replace(keyName, "/", "_", -1) + + var value string + value, ok := env[keyName] + if !ok { + t.Fatalf("expected (%s) key, but was not found", keyName) + } + if value != secretValue { + t.Fatalf("values didn't match, expected (%s), got (%s)", secretValue, value) + } }) } } From 56106bb0c3b8d933be35bc73c9404e952c3eb600 Mon Sep 17 00:00:00 2001 From: Clint Shryock Date: Tue, 6 Nov 2018 14:50:13 -0600 Subject: [PATCH 5/6] add support for vault KV 2 format --- runner.go | 36 +++++++++++++++++++++++++++++++++++- 1 file changed, 35 insertions(+), 1 deletion(-) diff --git a/runner.go b/runner.go index e4f32b65..44180938 100644 --- a/runner.go +++ b/runner.go @@ -22,9 +22,10 @@ import ( "github.com/pkg/errors" ) -// Regexp for invalid characters in keys +// InvalidRegexp is a regexp for invalid characters in keys var InvalidRegexp = regexp.MustCompile(`[^a-zA-Z0-9_]`) +// Runner executes a given child process with configuration type Runner struct { // ErrCh and DoneCh are channels where errors and finish notifications occur. ErrCh chan error @@ -418,6 +419,39 @@ func (r *Runner) appendSecrets( cp := r.configPrefixMap[d.String()] for key, value := range typed.Data { + // check for presence of "metadata", indicating this value came from Vault + // kv version 2, and we should then use the sub map instead + if key == "metadata" { + continue + } + + // Vault Secrets KV1 and KV2 return different formats. Here we check the key + // value, and if we've found another key called "data" that is of type + // map[string]interface, we assume it's KV2 and use the key/value pair from + // it, otherwise we assume it's KV1 + // + // value here in KV1 format is a simple string. + // value in KV2 format is a map: + // map[string]interface {}{ + // "key": "value", + // } + if key == "data" { + switch value.(type) { + case map[string]interface{}: + log.Printf("[DEBUG] Found KV2 secret") + mapV := value.(map[string]interface{}) + + // assumes this map is only 1 element long, we change the key and value + // to match the sub element + for k, v := range mapV { + key = k + value = v + } + default: + // only handle the sub data map, do nothing otherwise + } + } + // Ignore any keys that are empty (not sure if this is even possible in // Vault, but I play defense). if strings.TrimSpace(key) == "" { From 3884fc8c99ce927acc138812824c7ff0ecae4550 Mon Sep 17 00:00:00 2001 From: Clint Shryock Date: Thu, 15 Nov 2018 10:22:09 -0600 Subject: [PATCH 6/6] Update Vault kv2 handling for nil/deleted secrets --- runner.go | 6 +++++- runner_test.go | 26 +++++++++++++++++++++----- 2 files changed, 26 insertions(+), 6 deletions(-) diff --git a/runner.go b/runner.go index 44180938..89d388cf 100644 --- a/runner.go +++ b/runner.go @@ -446,9 +446,13 @@ func (r *Runner) appendSecrets( for k, v := range mapV { key = k value = v + break } default: - // only handle the sub data map, do nothing otherwise + // Only handle the sub data map, do nothing otherwise. + // If the secret has been deleted but not destroyed, Vault will return a + // response with a nil sub data map, and will not be handled by the + // above block } } diff --git a/runner_test.go b/runner_test.go index 4760cecf..31bf79f1 100644 --- a/runner_test.go +++ b/runner_test.go @@ -15,9 +15,9 @@ func TestRunner_appendSecrets(t *testing.T) { secretValue := "somevalue" cases := map[string]struct { - path string - data *dependency.Secret - err bool + path string + data *dependency.Secret + notFound bool }{ "kv1_secret": { "kv/bar", @@ -43,6 +43,19 @@ func TestRunner_appendSecrets(t *testing.T) { }, false, }, + "kv2_secret_destroyed": { + "secret/data/foo", + &dependency.Secret{ + Data: map[string]interface{}{ + "metadata": map[string]interface{}{ + "destroyed": bool(true), + "version": "2", + }, + "data": nil, + }, + }, + true, + }, } for name, tc := range cases { @@ -78,10 +91,13 @@ func TestRunner_appendSecrets(t *testing.T) { var value string value, ok := env[keyName] - if !ok { + if !ok && !tc.notFound { t.Fatalf("expected (%s) key, but was not found", keyName) } - if value != secretValue { + if ok && tc.notFound { + t.Fatalf("expected to not find key, but (%s) was found", keyName) + } + if ok && value != secretValue { t.Fatalf("values didn't match, expected (%s), got (%s)", secretValue, value) } })