Add validation for output file path / name

pkg/secrets/pushtofile/secret_group.go

@@ -15,6 +15,7 @@ const secretGroupFilePathPrefix = "conjur.org/secret-file-path."
 const secretGroupFileFormatPrefix = "conjur.org/secret-file-format."
 
 const defaultFilePermissions os.FileMode = 0664
+const maxFilenameLen = 255
 
 // SecretGroup incorporates all of the information about a secret group
 // that has been parsed from that secret group's Annotations.
@@ -108,17 +109,18 @@ func (sg *SecretGroup) absoluteFilePath(secretsBasePath string) (string, error)
 		)
 	}
 
-	filePathIsDir := strings.HasSuffix(filePath, "/")
+	pathContainsFilename := !strings.HasSuffix(filePath, "/") && len(filePath) > 0
 
-	// fileTemplate requires filePath to point to a file (not a directory)
-	if filePathIsDir && len(fileTemplate) > 0 {
-		return "", fmt.Errorf(
-			"provided filepath %q for secret group %q must specify a path to a file, without a trailing %q",
-			filePath, groupName, "/",
-		)
-	}
-	// Without the restrictions of fileTemplate, the filename defaults to "{groupName}.{fileFormat}"
-	if filePathIsDir && len(fileTemplate) == 0 {
+	if !pathContainsFilename {
+		if len(fileTemplate) > 0 {
+			// fileTemplate requires filePath to point to a file (not a directory)
+			return "", fmt.Errorf(
+				"provided filepath %q for secret group %q must specify a path to a file, without a trailing %q",
+				filePath, groupName, "/",
+			)
+		}
+
+		// Without the restrictions of fileTemplate, the filename defaults to "{groupName}.{fileFormat}"
 		filePath = path.Join(
 			filePath,
 			fmt.Sprintf("%s.%s", groupName, fileFormat),
@@ -137,6 +139,17 @@ func (sg *SecretGroup) absoluteFilePath(secretsBasePath string) (string, error)
 		)
 	}
 
+	// Filename cannot be longer than allowed by the filesystem
+	_, filename := path.Split(absoluteFilePath)
+	if len(filename) > maxFilenameLen {
+		return "", fmt.Errorf(
+			"filename %q for provided filepath for secret group %q must not be longer than %d characters",
+			filename,
+			groupName,
+			maxFilenameLen,
+		)
+	}
+
 	return absoluteFilePath, nil
 }
 
@@ -244,6 +257,8 @@ func NewSecretGroups(secretsBasePath string, annotations map[string]string) ([]*
 		}
 	}
 
+	errors = append(errors, validateGroupFilePaths(sgs)...)
+
 	if len(errors) > 0 {
 		return nil, errors
 	}
@@ -301,3 +316,27 @@ func newSecretGroup(groupName string, secretsBasePath string, annotations map[st
 
 	return sg, nil
 }
+
+func validateGroupFilePaths(secretGroups []*SecretGroup) []error {
+	// Iterate over the secret groups and group any that have the same file path
+	groupFilePaths := make(map[string][]string)
+	for _, sg := range secretGroups {
+		if len(groupFilePaths[sg.FilePath]) == 0 {
+			groupFilePaths[sg.FilePath] = []string{sg.Name}
+			continue
+		}
+
+		groupFilePaths[sg.FilePath] = append(groupFilePaths[sg.FilePath], sg.Name)
+	}
+
+	// If any file paths are used in more than one group, log all the groups that share the path
+	var errors []error
+	for path, groupNames := range groupFilePaths {
+		if len(groupNames) > 1 {
+			errors = append(errors, fmt.Errorf(
+				"duplicate filepath %q for groups: %q", path, strings.Join(groupNames, `, `),
+			))
+		}
+	}
+	return errors
+}

pkg/secrets/pushtofile/secret_group_test.go

@@ -5,6 +5,7 @@ import (
 	"io/ioutil"
 	"os"
 	"path"
+	"strings"
 	"testing"
 
 	"github.com/stretchr/testify/assert"
@@ -169,6 +170,62 @@ func TestNewSecretGroups(t *testing.T) {
 		)
 	})
 
+	t.Run("file path longer than 255 characters", func(t *testing.T) {
+		_, errs := NewSecretGroups("/basepath", map[string]string{
+			"conjur.org/conjur-secrets.first": `- path/to/secret/first1
+- aliasfirst2: path/to/secret/first2`,
+			"conjur.org/secret-file-path.first":      "firstfilepath",
+			"conjur.org/secret-file-template.first":  `firstfiletemplate`,
+			"conjur.org/conjur-secrets.second":       "- path/to/secret/second",
+			"conjur.org/secret-file-path.second":     strings.Repeat("secondfile", 26),
+			"conjur.org/secret-file-template.second": `secondfiletemplate`,
+		})
+		assert.Len(t, errs, 1)
+		assert.Contains(
+			t,
+			errs[0].Error(),
+			`filepath for secret group "second" must not be longer than 255 characters`,
+		)
+	})
+
+	t.Run("duplicate file paths", func(t *testing.T) {
+		_, errs := NewSecretGroups("/basepath", map[string]string{
+			"conjur.org/conjur-secrets.first": `- path/to/secret/first1
+- aliasfirst2: path/to/secret/first2`,
+			"conjur.org/secret-file-path.first":      "firstfilepath",
+			"conjur.org/secret-file-template.first":  `firstfiletemplate`,
+			"conjur.org/conjur-secrets.second":       "- path/to/secret/second",
+			"conjur.org/secret-file-path.second":     "firstfilepath",
+			"conjur.org/secret-file-template.second": `secondfiletemplate`,
+			"conjur.org/conjur-secrets.third":        "- path/to/secret/third",
+			"conjur.org/secret-file-path.third":      "firstfilepath",
+			"conjur.org/secret-file-template.third":  `thirdfiletemplate`,
+		})
+
+		assert.Len(t, errs, 1)
+		assert.Contains(t, errs[0].Error(), `duplicate filepath "/basepath/firstfilepath" for groups`)
+		// The order of the groups in the error message is not deterministic, so don't check the order.
+		assert.Contains(t, errs[0].Error(), "first")
+		assert.Contains(t, errs[0].Error(), "second")
+		assert.Contains(t, errs[0].Error(), "third")
+	})
+
+	t.Run("duplicate file path using default", func(t *testing.T) {
+		_, errs := NewSecretGroups("/basepath", map[string]string{
+			"conjur.org/conjur-secrets.first": `- path/to/secret/first1
+- aliasfirst2: path/to/secret/first2`,
+			"conjur.org/secret-file-path.first":  "./relative/path/to/folder/",
+			"conjur.org/conjur-secrets.second":   "- path/to/secret/second",
+			"conjur.org/secret-file-path.second": "./relative/path/to/folder/first.yaml",
+		})
+
+		assert.Len(t, errs, 1)
+		assert.Contains(t, errs[0].Error(), `duplicate filepath "/basepath/relative/path/to/folder/first.yaml" for groups`)
+		// The order of the groups in the error message is not deterministic, so don't check the order.
+		assert.Contains(t, errs[0].Error(), "first")
+		assert.Contains(t, errs[0].Error(), "second")
+	})
+
 	t.Run("secret file template requires file path annotation as file", func(t *testing.T) {
 		_, errs := NewSecretGroups("/basepath", map[string]string{
 			"conjur.org/conjur-secrets.first": `
@@ -187,6 +244,40 @@ func TestNewSecretGroups(t *testing.T) {
 		)
 	})
 
+	t.Run("secret file template requires explicit file path", func(t *testing.T) {
+		_, errs := NewSecretGroups("/basepath", map[string]string{
+			"conjur.org/conjur-secrets.first": `
+- path/to/secret/first1
+- aliasfirst2: path/to/secret/first2
+`,
+			"conjur.org/secret-file-template.first": "some template",
+		})
+
+		assert.Len(t, errs, 1)
+		assert.Contains(
+			t,
+			errs[0].Error(),
+			`path to a file`,
+		)
+	})
+
+	t.Run("secret file path default", func(t *testing.T) {
+		groups, errs := NewSecretGroups("/basepath", map[string]string{
+			"conjur.org/conjur-secrets.first": `
+- path/to/secret/first1
+- aliasfirst2: path/to/secret/first2
+`,
+		})
+
+		assert.Len(t, errs, 0)
+		assert.Len(t, groups, 1)
+		assert.Equal(
+			t,
+			groups[0].FilePath,
+			`/basepath/first.yaml`,
+		)
+	})
+
 	t.Run("secret file path as directory default filename", func(t *testing.T) {
 		groups, errs := NewSecretGroups("/basepath", map[string]string{
 			"conjur.org/conjur-secrets.first": `
@@ -240,6 +331,30 @@ func TestNewSecretGroups(t *testing.T) {
 			"yaml",
 		)
 	})
+
+	t.Run("secret file path overrides default extension", func(t *testing.T) {
+		groups, errs := NewSecretGroups("/basepath", map[string]string{
+			"conjur.org/conjur-secrets.first": `- path/to/secret/first1
+- aliasfirst2: path/to/secret/first2`,
+			"conjur.org/secret-file-path.first":   "./relative/path/to/folder/firstfilepath.json",
+			"conjur.org/secret-file-format.first": "yaml",
+		})
+
+		assert.Len(t, errs, 0)
+		assert.Len(t, groups, 1)
+		assert.Equal(
+			t,
+			groups[0].FilePath,
+			`/basepath/relative/path/to/folder/firstfilepath.json`,
+		)
+		assert.Contains(
+			t,
+			groups[0].FileFormat,
+			"yaml",
+		)
+
+	})
+
 }
 
 var pushToFileWithDepsTestCases = []pushToFileWithDepsTestCase{