Fix SecretGroup common policy path

cyberark · Nov 18, 2021 · 78c0a21 · 78c0a21
1 parent 972a813
commit 78c0a21
Show file tree

Hide file tree

Showing 4 changed files with 141 additions and 34 deletions.
diff --git a/pkg/secrets/pushtofile/retrieve_secrets.go b/pkg/secrets/pushtofile/retrieve_secrets.go
@@ -20,16 +20,25 @@ func FetchSecretsForGroups(
 	secretGroups []*SecretGroup,
 ) (map[string][]*Secret, error) {
 	var err error
+	secretPaths := []string{}
+	resolvedSpecsByGroup := map[string][]SecretSpec{}
 	secretsByGroup := map[string][]*Secret{}
 
-	secretPaths := getAllPaths(secretGroups)
+	for _, group := range secretGroups {
+		resolvedSpecs := group.ResolvedSecretSpecs()
+		resolvedSpecsByGroup[group.Name] = resolvedSpecs
+		for _, spec := range resolvedSpecs {
+			secretPaths = append(secretPaths, spec.Path)
+		}
+	}
+
 	secretValueById, err := depRetrieveSecrets(secretPaths)
 	if err != nil {
 		return nil, err
 	}
 
-	for _, group := range secretGroups {
-		for _, spec := range group.SecretSpecs {
+	for groupName, specs := range resolvedSpecsByGroup {
+		for _, spec := range specs {
 			sValue, ok := secretValueById[spec.Path]
 			if !ok {
 				err = fmt.Errorf(
@@ -39,8 +48,8 @@ func FetchSecretsForGroups(
 				return nil, err
 			}
 
-			secretsByGroup[group.Name] = append(
-				secretsByGroup[group.Name],
+			secretsByGroup[groupName] = append(
+				secretsByGroup[groupName],
 				&Secret{
 					Alias: spec.Alias,
 					Value: string(sValue),
@@ -51,13 +60,3 @@ func FetchSecretsForGroups(
 
 	return secretsByGroup, err
 }
-
-func getAllPaths(secretGroups []*SecretGroup) []string {
-	var ids []string
-	for _, group := range secretGroups {
-		for _, spec := range group.SecretSpecs {
-			ids = append(ids, spec.Path)
-		}
-	}
-	return ids
-}
diff --git a/pkg/secrets/pushtofile/retrieve_secrets_test.go b/pkg/secrets/pushtofile/retrieve_secrets_test.go
@@ -9,29 +9,31 @@ import (
 )
 
 type retrieveSecretsTestCase struct {
-	description string
-	secretSpecs map[string][]SecretSpec
-	assert      func(t *testing.T, result map[string][]*Secret, err error)
+	description        string
+	policyPathPrefixes map[string]string
+	secretSpecs        map[string][]SecretSpec
+	assert             func(t *testing.T, result map[string][]*Secret, err error)
 }
 
 func (tc retrieveSecretsTestCase) Run(
 	t *testing.T,
 	depFetchSecrets conjur.RetrieveSecretsFunc,
 ) {
 	t.Run(tc.description, func(t *testing.T) {
-		s := createSecretGroups(tc.secretSpecs)
+		s := createSecretGroups(tc.secretSpecs, tc.policyPathPrefixes)
 		ret, err := FetchSecretsForGroups(depFetchSecrets, s)
 
 		tc.assert(t, ret, err)
 	})
 }
 
-func createSecretGroups(groupSpecs map[string][]SecretSpec) []*SecretGroup {
+func createSecretGroups(groupSpecs map[string][]SecretSpec, policyPathPrefixes map[string]string) []*SecretGroup {
 	var secretGroups []*SecretGroup
 	for name, secretSpecs := range groupSpecs {
 		secretGroup := &SecretGroup{
-			Name:        name,
-			SecretSpecs: secretSpecs,
+			Name:             name,
+			PolicyPathPrefix: policyPathPrefixes[name],
+			SecretSpecs:      secretSpecs,
 		}
 		secretGroups = append(secretGroups, secretGroup)
 	}
@@ -91,6 +93,37 @@ var retrieveSecretsTestCases = []retrieveSecretsTestCase{
 			},
 		}),
 	},
+	{
+		description: "Happy Case given Policy Path Prefix",
+		policyPathPrefixes: map[string]string{
+			"cache": "dev/openshift",
+			"db":    "ci/openshift",
+		},
+		secretSpecs: map[string][]SecretSpec{
+			"cache": []SecretSpec{
+				{Alias: "api-url", Path: "api-url"},
+				{Alias: "username", Path: "username"},
+				{Alias: "password", Path: "password"},
+			},
+			"db": []SecretSpec{
+				{Alias: "api-url", Path: "api-url"},
+				{Alias: "username", Path: "username"},
+				{Alias: "password", Path: "password"},
+			},
+		},
+		assert: assertGoodResults(map[string][]*Secret{
+			"cache": []*Secret{
+				{Alias: "api-url", Value: "https://postgres.example.com"},
+				{Alias: "username", Value: "admin"},
+				{Alias: "password", Value: "open-$e$ame"},
+			},
+			"db": []*Secret{
+				{Alias: "api-url", Value: "https://ci.postgres.example.com"},
+				{Alias: "username", Value: "administrator"},
+				{Alias: "password", Value: "open-$e$ame"},
+			},
+		}),
+	},
 	{
 		description: "Bad ID",
 		secretSpecs: map[string][]SecretSpec{
@@ -109,6 +142,28 @@ var retrieveSecretsTestCases = []retrieveSecretsTestCase{
 			assert.Contains(t, err.Error(), "no_conjur_secret_error")
 		},
 	},
+	{
+		description: "Bad ID given Policy Path Prefix",
+		policyPathPrefixes: map[string]string{
+			"cache": "foo/opensift",
+			"db":    "ci/openshift",
+		},
+		secretSpecs: map[string][]SecretSpec{
+			"cache": []SecretSpec{
+				{Alias: "api-url", Path: "bar"},
+				{Alias: "username", Path: "username"},
+				{Alias: "password", Path: "password"},
+			},
+			"db": []SecretSpec{
+				{Alias: "api-url", Path: "api-url"},
+				{Alias: "username", Path: "username"},
+				{Alias: "password", Path: "password"},
+			},
+		},
+		assert: func(t *testing.T, result map[string][]*Secret, err error) {
+			assert.Contains(t, err.Error(), "no_conjur_secret_error")
+		},
+	},
 }
 
 type mockSecretFetcher struct {

diff --git a/pkg/secrets/pushtofile/secret_group.go b/pkg/secrets/pushtofile/secret_group.go
@@ -42,9 +42,7 @@ func (sg *SecretGroup) ResolvedSecretSpecs() []SecretSpec {
 
 	// Update specs with policy path prefix
 	for i := range specs {
-		specs[i].Path = strings.TrimSuffix(sg.PolicyPathPrefix, "/") +
-			"/" +
-			strings.TrimPrefix(specs[i].Path, "/")
+		specs[i].Path = path.Join(sg.PolicyPathPrefix, specs[i].Path)
 	}
 
 	return specs
@@ -309,6 +307,7 @@ func newSecretGroup(groupName string, secretsBasePath string, annotations map[st
 	filePath := annotations[secretGroupFilePathPrefix+groupName]
 	fileFormat := annotations[secretGroupFileFormatPrefix+groupName]
 	policyPathPrefix := annotations[secretGroupPolicyPathPrefix+groupName]
+	policyPathPrefix = strings.TrimPrefix(policyPathPrefix, "/")
 
 	// Default to "yaml" file format
 	if len(fileTemplate)+len(fileFormat) == 0 {

diff --git a/pkg/secrets/pushtofile/secret_group_test.go b/pkg/secrets/pushtofile/secret_group_test.go
@@ -100,8 +100,9 @@ func goodSecretSpecs() []SecretSpec {
 func TestNewSecretGroups(t *testing.T) {
 	t.Run("valid annotations", func(t *testing.T) {
 		secretGroups, errs := NewSecretGroups("/basepath", map[string]string{
-			"conjur.org/conjur-secrets.first": `- path/to/secret/first1
-- aliasfirst2: path/to/secret/first2`,
+			"conjur.org/conjur-secrets-policy-path.first": "/path/to/secret/",
+			"conjur.org/conjur-secrets.first": `- first1
+- aliasfirst2: first2`,
 			"conjur.org/secret-file-path.first":      "firstfilepath",
 			"conjur.org/secret-file-template.first":  `firstfiletemplate`,
 			"conjur.org/conjur-secrets.second":       "- path/to/secret/second",
@@ -114,19 +115,20 @@ func TestNewSecretGroups(t *testing.T) {
 		}
 		assert.Len(t, secretGroups, 2)
 		assert.Equal(t, *secretGroups[0], SecretGroup{
-			Name:            "first",
-			FilePath:        "/basepath/firstfilepath",
-			FileTemplate:    "firstfiletemplate",
-			FileFormat:      "",
-			FilePermissions: defaultFilePermissions,
+			Name:             "first",
+			FilePath:         "/basepath/firstfilepath",
+			FileTemplate:     "firstfiletemplate",
+			FileFormat:       "",
+			FilePermissions:  defaultFilePermissions,
+			PolicyPathPrefix: "path/to/secret/",
 			SecretSpecs: []SecretSpec{
 				{
 					Alias: "first1",
-					Path:  "path/to/secret/first1",
+					Path:  "first1",
 				},
 				{
 					Alias: "aliasfirst2",
-					Path:  "path/to/secret/first2",
+					Path:  "first2",
 				},
 			},
 		})
@@ -580,6 +582,58 @@ func TestSecretGroup_pushToFileWithDeps(t *testing.T) {
 	}
 }
 
+func assertGoodSecretSpec(expectedSpecs []SecretSpec) func(*testing.T, []SecretSpec) {
+	return func(t *testing.T, resultSpecs []SecretSpec) {
+		assert.Equal(t, expectedSpecs, resultSpecs)
+	}
+}
+
+func TestSecretGroup_ResolvedSecretSpecs(t *testing.T) {
+	for _, tc := range []struct {
+		description      string
+		policyPathPrefix string
+		secretSpecs      []SecretSpec
+		assert           func(t *testing.T, result []SecretSpec)
+	}{
+		{
+			description:      "Happy Case with common policy path",
+			policyPathPrefix: "path/prefix",
+			secretSpecs: []SecretSpec{
+				{Alias: "a", Path: "relative/path/to/a"},
+				{Alias: "b", Path: "another/path/to/b"},
+			},
+			assert: assertGoodSecretSpec([]SecretSpec{
+				{Alias: "a", Path: "path/prefix/relative/path/to/a"},
+				{Alias: "b", Path: "path/prefix/another/path/to/b"},
+			}),
+		},
+		{
+			description:      "Happy Case without common policy path",
+			policyPathPrefix: "",
+			secretSpecs: []SecretSpec{
+				{Alias: "a", Path: "path/to/a"},
+				{Alias: "b", Path: "path/to/b"},
+			},
+			assert: assertGoodSecretSpec([]SecretSpec{
+				{Alias: "a", Path: "path/to/a"},
+				{Alias: "b", Path: "path/to/b"},
+			}),
+		},
+	} {
+		t.Run(tc.description, func(t *testing.T) {
+			sg := SecretGroup{
+				Name:             "test-group",
+				PolicyPathPrefix: tc.policyPathPrefix,
+				SecretSpecs:      tc.secretSpecs,
+			}
+
+			sSpec := sg.ResolvedSecretSpecs()
+			assert.Equal(t, len(tc.secretSpecs), len(sSpec))
+			tc.assert(t, sSpec)
+		})
+	}
+}
+
 func TestSecretGroup_PushToFile(t *testing.T) {
 	// Create temp directory
 	dir, err := ioutil.TempDir("", "")