feat: add secret-path validation

Signed-off-by: Valentin Süß <[email protected]>

Author: c0deltin

cmd/generate.go

@@ -2,6 +2,7 @@ package cmd
 
 import (
 	"fmt"
+	"regexp"
 	"strconv"
 	"strings"
 
@@ -76,8 +77,15 @@ func NewGenerateCommand() *cobra.Command {
 			}
 
 			for _, manifest := range manifests {
+				var pathValidation *regexp.Regexp
+				if rexp := v.GetString(types.EnvPathValidation); rexp != "" {
+					pathValidation, err = regexp.Compile(rexp)
+					if err != nil {
+						return fmt.Errorf("%s is not a valid regular expression: %s", rexp, err)
+					}
+				}
 
-				template, err := kube.NewTemplate(manifest, cmdConfig.Backend)
+				template, err := kube.NewTemplate(manifest, cmdConfig.Backend, pathValidation)
 				if err != nil {
 					return err
 				}

cmd/generate_test.go

@@ -224,10 +224,37 @@ func TestMain(t *testing.T) {
 		}
 	})
 
+	t.Run("will return that path validation env is not valid", func(t *testing.T) {
+		args := []string{"../fixtures/input/nonempty"}
+		cmd := NewGenerateCommand()
+
+		// set specific env and register cleanup func
+		os.Setenv("AVP_PATH_VALIDATION", `^\/(?!\/)(.*?)`)
+		t.Cleanup(func() {
+			os.Unsetenv("AVP_PATH_VALIDATION")
+		})
+
+		b := bytes.NewBufferString("")
+		cmd.SetArgs(args)
+		cmd.SetErr(b)
+		cmd.SetOut(bytes.NewBufferString(""))
+		cmd.Execute()
+		out, err := ioutil.ReadAll(b) // Read buffer to bytes
+		if err != nil {
+			t.Fatal(err)
+		}
+
+		expected := "^\\/(?!\\/)(.*?) is not a valid regular expression: error parsing regexp: invalid or unsupported Perl syntax: `(?!`"
+		if !strings.Contains(string(out), expected) {
+			t.Fatalf("expected to contain: %s but got %s", expected, out)
+		}
+	})
+
 	os.Unsetenv("AVP_TYPE")
 	os.Unsetenv("VAULT_ADDR")
 	os.Unsetenv("AVP_AUTH_TYPE")
 	os.Unsetenv("AVP_SECRET_ID")
 	os.Unsetenv("AVP_ROLE_ID")
 	os.Unsetenv("VAULT_SKIP_VERIFY")
+	os.Unsetenv("AVP_PATH_VALIDATION")
 }

docs/config.md

@@ -72,7 +72,7 @@ We support all the backend specific environment variables each backend's SDK wil
 We also support these AVP specific variables:
 
 | Name                       | Description                                         | Notes                                                                                                                                                                        |
-| -------------------------- | --------------------------------------------------- | ---------------------------------------------------------------------------------------------------------------------------------------------------------------------------- |
+| -------------------------- |-----------------------------------------------------|------------------------------------------------------------------------------------------------------------------------------------------------------------------------------|
 | AVP_TYPE                   | The type of Vault backend                           | Supported values: `vault`, `ibmsecretsmanager`, `awssecretsmanager`, `gcpsecretmanager`, `yandexcloudlockbox` and `1passwordconnect`                                         |
 | AVP_KV_VERSION             | The vault secret engine                             | Supported values: `1` and `2` (defaults to 2). KV_VERSION will be ignored if the `avp.kubernetes.io/kv-version` annotation is present in a YAML resource.                    |
 | AVP_AUTH_TYPE              | The type of authentication                          | Supported values: vault: `approle, github, k8s, token`. Only honored for `AVP_TYPE` of `vault`                                                                               |
@@ -89,6 +89,7 @@ We also support these AVP specific variables:
 | AVP_YCL_SERVICE_ACCOUNT_ID | Yandex Cloud Lockbox service account ID             | Required with `TYPE` of `yandexcloudlockbox`                                                                                                                                 |
 | AVP_YCL_KEY_ID             | Yandex Cloud Lockbox service account Key ID         | Required with `TYPE` of `yandexcloudlockbox`                                                                                                                                 |
 | AVP_YCL_PRIVATE_KEY        | Yandex Cloud Lockbox service account private key    | Required with `TYPE` of `yandexcloudlockbox`                                                                                                                                 |
+| AVP_PATH_VALIDATION        | Regular Expression to validate the Vault path       | Optional. Can be used for e.g. to prevent path traversals.                                                                                                                   |
 
 ### Full List of Supported Annotation
 
@@ -237,4 +238,4 @@ spec:
           value: foo-team-namespace
 ```
 
-**Note**: Exposing tokens (like `AVP_ROLE_ID` or `AVP_SECRET_ID`) in plain-text in Argo CD app manifests should be avoided. Prefer to pass those tokens through one of the means mentioned above.
+**Note**: Exposing tokens (like `AVP_ROLE_ID` or `AVP_SECRET_ID`) in plain-text in Argo CD app manifests should be avoided. Prefer to pass those tokens through one of the means mentioned above.

pkg/kube/template.go

@@ -2,6 +2,7 @@ package kube
 
 import (
 	"fmt"
+	"regexp"
 	"strings"
 
 	"github.com/argoproj-labs/argocd-vault-plugin/pkg/types"
@@ -18,6 +19,7 @@ type Resource struct {
 	replacementErrors []error                // Any errors encountered in performing replacements
 	Data              map[string]interface{} // The data to replace with, from Vault
 	Annotations       map[string]string
+	PathValidation    *regexp.Regexp
 }
 
 // Template is the template for Kubernetes
@@ -26,14 +28,17 @@ type Template struct {
 }
 
 // NewTemplate returns a *Template given the template's data, and a VaultType
-func NewTemplate(template unstructured.Unstructured, backend types.Backend) (*Template, error) {
+func NewTemplate(template unstructured.Unstructured, backend types.Backend, pathValidation *regexp.Regexp) (*Template, error) {
 	annotations := template.GetAnnotations()
 	path := annotations[types.AVPPathAnnotation]
 	version := annotations[types.AVPSecretVersionAnnotation]
 
 	var err error
 	var data map[string]interface{}
 	if path != "" {
+		if pathValidation != nil && !pathValidation.MatchString(path) {
+			return nil, fmt.Errorf("the path %s is disallowed by %s restriction", path, types.EnvPathValidation)
+		}
 		data, err = backend.GetSecrets(path, version, annotations)
 		if err != nil {
 			return nil, err
@@ -44,11 +49,12 @@ func NewTemplate(template unstructured.Unstructured, backend types.Backend) (*Te
 
 	return &Template{
 		Resource{
-			Kind:         template.GetKind(),
-			TemplateData: template.Object,
-			Backend:      backend,
-			Data:         data,
-			Annotations:  annotations,
+			Kind:           template.GetKind(),
+			TemplateData:   template.Object,
+			Backend:        backend,
+			Data:           data,
+			Annotations:    annotations,
+			PathValidation: pathValidation,
 		},
 	}, nil
 }

pkg/kube/template_test.go

@@ -3,6 +3,7 @@ package kube
 import (
 	"io/ioutil"
 	"reflect"
+	"regexp"
 	"strings"
 	"testing"
 
@@ -219,7 +220,7 @@ func TestNewTemplate(t *testing.T) {
 					},
 				},
 			},
-		}, &mv)
+		}, &mv, nil)
 		if template.Resource.Kind != "Service" {
 			t.Fatalf("template should have Kind of %s, instead it has %s", "Service", template.Resource.Kind)
 		}
@@ -253,7 +254,7 @@ func TestNewTemplate(t *testing.T) {
 					},
 				},
 			},
-		}, &mv)
+		}, &mv, nil)
 		if mv.GetSecretsCalled {
 			t.Fatalf("template contains avp.kubernetes.io/ignore:True so GetSecrets should NOT be called")
 		}
@@ -286,7 +287,7 @@ func TestNewTemplate(t *testing.T) {
 					"old-value": "<password>",
 				},
 			},
-		}, &mv)
+		}, &mv, nil)
 
 		if template.Resource.Kind != "Secret" {
 			t.Fatalf("template should have Kind of %s, instead it has %s", "Secret", template.Resource.Kind)
@@ -349,7 +350,110 @@ func TestNewTemplate(t *testing.T) {
 					"new-value": "<password>",
 				},
 			},
-		}, &mv)
+		}, &mv, nil)
+
+		if template.Resource.Kind != "Secret" {
+			t.Fatalf("template should have Kind of %s, instead it has %s", "Secret", template.Resource.Kind)
+		}
+
+		if !mv.GetSecretsCalled {
+			t.Fatalf("template does contain <placeholders> so GetSecrets should be called")
+		}
+
+		err := template.Replace()
+		if err != nil {
+			t.Fatalf(err.Error())
+		}
+
+		expected := map[string]interface{}{
+			"kind":       "Secret",
+			"apiVersion": "v1",
+			"metadata": map[string]interface{}{
+				"annotations": map[string]interface{}{
+					types.AVPPathAnnotation: "path/to/secret",
+				},
+				"namespace": "default",
+				"name":      "my-app",
+			},
+			"data": map[string]interface{}{
+				"new-value": "changed-value",
+				"old-value": "original-value",
+			},
+		}
+
+		if !reflect.DeepEqual(expected, template.TemplateData) {
+			t.Fatalf("expected %s got %s", expected, template.TemplateData)
+		}
+	})
+
+	t.Run("GetSecrets with path validation and invalid path", func(t *testing.T) {
+		mv := helpers.MockVault{}
+
+		mv.LoadData(map[string]interface{}{
+			"password": "original-value",
+		})
+		mv.LoadData(map[string]interface{}{
+			"password": "changed-value",
+		})
+
+		template, err := NewTemplate(unstructured.Unstructured{
+			Object: map[string]interface{}{
+				"kind":       "Secret",
+				"apiVersion": "v1",
+				"metadata": map[string]interface{}{
+					"annotations": map[string]interface{}{
+						types.AVPPathAnnotation: "path/to/secret",
+					},
+					"namespace": "default",
+					"name":      "my-app",
+				},
+				"data": map[string]interface{}{
+					"old-value": "<path:/path/to/secret#password#1>",
+					"new-value": "<password>",
+				},
+			},
+		}, &mv, regexp.MustCompile(`/[A-Z]/`))
+
+		if template != nil {
+			t.Fatalf("expected template to be nil got %s", template)
+		}
+		if err == nil {
+			t.Fatalf("expected error got nil")
+		}
+
+		expected := "the path path/to/secret is disallowed by AVP_PATH_VALIDATION restriction"
+		if err.Error() != expected {
+			t.Fatalf("expected %s got %s", expected, err.Error())
+		}
+	})
+
+	t.Run("will GetSecrets with latest version by default and path validation regexp", func(t *testing.T) {
+		mv := helpers.MockVault{}
+
+		mv.LoadData(map[string]interface{}{
+			"password": "original-value",
+		})
+		mv.LoadData(map[string]interface{}{
+			"password": "changed-value",
+		})
+
+		template, _ := NewTemplate(unstructured.Unstructured{
+			Object: map[string]interface{}{
+				"kind":       "Secret",
+				"apiVersion": "v1",
+				"metadata": map[string]interface{}{
+					"annotations": map[string]interface{}{
+						types.AVPPathAnnotation: "path/to/secret",
+					},
+					"namespace": "default",
+					"name":      "my-app",
+				},
+				"data": map[string]interface{}{
+					"old-value": "<path:/path/to/secret#password#1>",
+					"new-value": "<password>",
+				},
+			},
+		}, &mv, regexp.MustCompile(`^([A-Za-z/]*)$`))
 
 		if template.Resource.Kind != "Secret" {
 			t.Fatalf("template should have Kind of %s, instead it has %s", "Secret", template.Resource.Kind)

pkg/kube/util.go

@@ -133,6 +133,11 @@ func genericReplacement(key, value string, resource Resource) (_ interface{}, er
 			key := indivSecretMatches[indivPlaceholderSyntax.SubexpIndex("key")]
 			version := indivSecretMatches[indivPlaceholderSyntax.SubexpIndex("version")]
 
+			if resource.PathValidation != nil && !resource.PathValidation.MatchString(path) {
+				err = append(err, fmt.Errorf("the path %s is disallowed by %s restriction", path, types.EnvPathValidation))
+				return match
+			}
+
 			utils.VerboseToStdErr("calling GetIndividualSecret for secret %s from path %s at version %s", key, path, version)
 			secretValue, secretErr = resource.Backend.GetIndividualSecret(path, strings.TrimSpace(key), version, resource.Annotations)
 			if secretErr != nil {

pkg/kube/util_test.go

@@ -5,6 +5,7 @@ import (
 	"encoding/json"
 	"fmt"
 	"reflect"
+	"regexp"
 	"testing"
 
 	"github.com/argoproj-labs/argocd-vault-plugin/pkg/helpers"
@@ -111,6 +112,89 @@ func TestGenericReplacement_specificPath(t *testing.T) {
 	assertSuccessfulReplacement(&dummyResource, &expected, t)
 }
 
+func TestGenericReplacement_specificPathWithValidation(t *testing.T) {
+	// Test that the specific-path placeholder syntax is used to find/replace placeholders
+	// along with the generic syntax, since the generic Vault path is defined
+	mv := helpers.MockVault{}
+	mv.LoadData(map[string]interface{}{
+		"namespace": "default",
+	})
+
+	t.Run("valid path", func(t *testing.T) {
+		dummyResource := Resource{
+			TemplateData: map[string]interface{}{
+				"namespace": "<path:blah/blah#namespace>",
+				"name":      "<name>",
+			},
+			Data: map[string]interface{}{
+				"namespace": "something-else",
+				"name":      "foo",
+			},
+			Backend: &mv,
+			Annotations: map[string]string{
+				(types.AVPPathAnnotation): "",
+			},
+			PathValidation: regexp.MustCompile(`^([A-Za-z/]*)$`),
+		}
+
+		replaceInner(&dummyResource, &dummyResource.TemplateData, genericReplacement)
+
+		if !mv.GetIndividualSecretCalled {
+			t.Fatalf("expected GetSecrets to be called since placeholder contains explicit path so Vault lookup is neeed")
+		}
+
+		expected := Resource{
+			TemplateData: map[string]interface{}{
+				"namespace": "default",
+				"name":      "foo",
+			},
+			Data: map[string]interface{}{
+				"namespace": "something-else",
+				"name":      "foo",
+			},
+			replacementErrors: []error{},
+		}
+
+		assertSuccessfulReplacement(&dummyResource, &expected, t)
+	})
+
+	t.Run("invalid path", func(t *testing.T) {
+		dummyResource := Resource{
+			TemplateData: map[string]interface{}{
+				"namespace": "<path:../blah/blah#namespace>",
+			},
+			Data: map[string]interface{}{
+				"namespace": "something-else",
+			},
+			Backend: &mv,
+			Annotations: map[string]string{
+				(types.AVPPathAnnotation): "",
+			},
+			PathValidation: regexp.MustCompile(`^([A-Za-z/]*)$`),
+		}
+
+		replaceInner(&dummyResource, &dummyResource.TemplateData, genericReplacement)
+
+		if !mv.GetIndividualSecretCalled {
+			t.Fatalf("expected GetSecrets to be called since placeholder contains explicit path so Vault lookup is neeed")
+		}
+
+		expected := Resource{
+			TemplateData: map[string]interface{}{
+				"namespace": "<path:../blah/blah#namespace>",
+			},
+			Data: map[string]interface{}{
+				"namespace": "something-else",
+			},
+			replacementErrors: []error{
+				fmt.Errorf("the path ../blah/blah is disallowed by AVP_PATH_VALIDATION restriction"),
+			},
+		}
+
+		assertFailedReplacement(&dummyResource, &expected, t)
+	})
+}
+
 func TestGenericReplacement_specificPathVersioned(t *testing.T) {
 	// Test that the specific-path placeholder syntax with versioning is used to find/replace placeholders
 	mv := helpers.MockVault{}