Merge pull request #1399 from findkim/fix-vault-metadata

Fix reading Vault KVv2 secrets metadata

dependency/vault_common.go

@@ -3,8 +3,6 @@ package dependency
 import (
 	"log"
 	"math/rand"
-	"path"
-	"strings"
 	"time"
 
 	"encoding/json"
@@ -329,21 +327,3 @@ func isKVv2(client *api.Client, path string) (string, bool, error) {
 
 	return mountPath, false, nil
 }
-
-func addPrefixToVKVPath(p, mountPath, apiPrefix string) string {
-	switch {
-	case p == mountPath, p == strings.TrimSuffix(mountPath, "/"):
-		return path.Join(mountPath, apiPrefix)
-	default:
-		p = strings.TrimPrefix(p, mountPath)
-		// Don't add /data/ to the path if it's been added manually.
-		apiPathPrefix := apiPrefix
-		if !strings.HasSuffix(apiPrefix, "/") {
-			apiPathPrefix += "/"
-		}
-		if strings.HasPrefix(p, apiPathPrefix) {
-			return path.Join(mountPath, p)
-		}
-		return path.Join(mountPath, apiPrefix, p)
-	}
-}

dependency/vault_read.go

@@ -4,6 +4,7 @@ import (
 	"fmt"
 	"log"
 	"net/url"
+	"path"
 	"strings"
 	"time"
 
@@ -145,7 +146,7 @@ func (d *VaultReadQuery) readSecret(clients *ClientSet, opts *QueryOptions) (*ap
 			isKVv2 = false
 			d.secretPath = d.rawPath
 		} else if isKVv2 {
-			d.secretPath = addPrefixToVKVPath(d.rawPath, mountPath, "data")
+			d.secretPath = shimKVv2Path(d.rawPath, mountPath)
 		} else {
 			d.secretPath = d.rawPath
 		}
@@ -176,3 +177,21 @@ func deletedKVv2(s *api.Secret) bool {
 	}
 	return false
 }
+
+// shimKVv2Path aligns the supported legacy path to KV v2 specs by inserting
+// /data/ into the path for reading secrets. Paths for metadata are not modified.
+func shimKVv2Path(rawPath, mountPath string) string {
+	switch {
+	case rawPath == mountPath, rawPath == strings.TrimSuffix(mountPath, "/"):
+		return path.Join(mountPath, "data")
+	default:
+		p := strings.TrimPrefix(rawPath, mountPath)
+
+		// Only add /data/ prefix to the path if neither /data/ or /metadata/ are
+		// present.
+		if strings.HasPrefix(p, "data/") || strings.HasPrefix(p, "metadata/") {
+			return rawPath
+		}
+		return path.Join(mountPath, "data", p)
+	}
+}

dependency/vault_read_test.go

@@ -11,6 +11,7 @@ import (
 	"github.com/hashicorp/vault/api"
 	"github.com/pkg/errors"
 	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
 )
 
 func TestNewVaultReadQuery(t *testing.T) {
@@ -398,6 +399,18 @@ func TestVaultReadQuery_Fetch_KVv2(t *testing.T) {
 		})
 	}
 
+	t.Run("read_metadata", func(t *testing.T) {
+		d, err := NewVaultReadQuery(secretsPath + "/metadata/foo/bar")
+		require.NoError(t, err)
+
+		act, _, err := d.Fetch(clients, nil)
+		require.NoError(t, err)
+		require.NotNil(t, act)
+
+		versions := act.(*Secret).Data["versions"]
+		assert.Len(t, versions, 2)
+	})
+
 	t.Run("read_deleted", func(t *testing.T) {
 		// only needed for KVv2 as KVv1 doesn't have metadata
 		path := "data/foo/zed"
@@ -651,3 +664,70 @@ func TestVaultReadQuery_String(t *testing.T) {
 		})
 	}
 }
+
+func TestShimKVv2Path(t *testing.T) {
+	cases := []struct {
+		name      string
+		path      string
+		mountPath string
+		expected  string
+	}{
+		{
+			"full path",
+			"secret/data/foo/bar",
+			"secret/",
+			"secret/data/foo/bar",
+		}, {
+			"data prefix added",
+			"secret/foo/bar",
+			"secret/",
+			"secret/data/foo/bar",
+		}, {
+			"full path with data* in subpath",
+			"secret/data/datafoo/bar",
+			"secret/",
+			"secret/data/datafoo/bar",
+		}, {
+			"prefix added with data* in subpath",
+			"secret/datafoo/bar",
+			"secret/",
+			"secret/data/datafoo/bar",
+		}, {
+			"prefix added with *data in subpath",
+			"secret/foodata/foo/bar",
+			"secret/",
+			"secret/data/foodata/foo/bar",
+		}, {
+			"prefix not added to metadata",
+			"secret/metadata/foo/bar",
+			"secret/",
+			"secret/metadata/foo/bar",
+		}, {
+			"prefix added with metadata* in subpath",
+			"secret/metadatafoo/foo/bar",
+			"secret/",
+			"secret/data/metadatafoo/foo/bar",
+		}, {
+			"prefix added with *metadata in subpath",
+			"secret/foometadata/foo/bar",
+			"secret/",
+			"secret/data/foometadata/foo/bar",
+		}, {
+			"prefix added to mount path",
+			"secret/",
+			"secret/",
+			"secret/data",
+		}, {
+			"prefix added to mount path not exact match",
+			"secret",
+			"secret/",
+			"secret/data",
+		},
+	}
+	for _, tc := range cases {
+		t.Run(tc.name, func(t *testing.T) {
+			actual := shimKVv2Path(tc.path, tc.mountPath)
+			assert.Equal(t, tc.expected, actual)
+		})
+	}
+}