diff --git a/cmd/generate.go b/cmd/generate.go index 60939667..e6f63c10 100644 --- a/cmd/generate.go +++ b/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 } diff --git a/cmd/generate_test.go b/cmd/generate_test.go index 27570e96..a5af334b 100644 --- a/cmd/generate_test.go +++ b/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") } diff --git a/docs/config.md b/docs/config.md index 78b5f083..64b64494 100644 --- a/docs/config.md +++ b/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. \ No newline at end of file diff --git a/pkg/kube/template.go b/pkg/kube/template.go index ddd6a1b7..4e4adb8a 100644 --- a/pkg/kube/template.go +++ b/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,7 +28,7 @@ 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] @@ -34,6 +36,9 @@ func NewTemplate(template unstructured.Unstructured, backend types.Backend) (*Te 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 } diff --git a/pkg/kube/template_test.go b/pkg/kube/template_test.go index da690ff2..37b4c08e 100644 --- a/pkg/kube/template_test.go +++ b/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": "", }, }, - }, &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": "", }, }, - }, &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 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": "", + "new-value": "", + }, + }, + }, &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": "", + "new-value": "", + }, + }, + }, &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) diff --git a/pkg/kube/util.go b/pkg/kube/util.go index 6905b6f2..aeaccc20 100644 --- a/pkg/kube/util.go +++ b/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 { diff --git a/pkg/kube/util_test.go b/pkg/kube/util_test.go index 7dcb3b60..b09fef90 100644 --- a/pkg/kube/util_test.go +++ b/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": "", + "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": "", + }, + 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": "", + }, + 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{} diff --git a/pkg/types/constants.go b/pkg/types/constants.go index 877a32b8..2cf16e57 100644 --- a/pkg/types/constants.go +++ b/pkg/types/constants.go @@ -25,6 +25,7 @@ const ( EnvYCLPrivateKey = "AVP_YCL_PRIVATE_KEY" EnvAvpUsername = "AVP_USERNAME" EnvAvpPassword = "AVP_PASSWORD" + EnvPathValidation = "AVP_PATH_VALIDATION" // Backend and Auth Constants VaultBackend = "vault"