Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: add vault-path validation #454

Merged
merged 1 commit into from
Feb 24, 2023
Merged

feat: add vault-path validation #454

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 9 additions & 1 deletion cmd/generate.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ package cmd

import (
"fmt"
"regexp"
"strconv"
"strings"

Expand Down Expand Up @@ -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
}
Expand Down
27 changes: 27 additions & 0 deletions cmd/generate_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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")
}
5 changes: 3 additions & 2 deletions docs/config.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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` |
Expand All @@ -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

Expand Down Expand Up @@ -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.
18 changes: 12 additions & 6 deletions pkg/kube/template.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ package kube

import (
"fmt"
"regexp"
"strings"

"github.com/argoproj-labs/argocd-vault-plugin/pkg/types"
Expand All @@ -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
Expand All @@ -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
Expand All @@ -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
}
Expand Down
112 changes: 108 additions & 4 deletions pkg/kube/template_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ package kube
import (
"io/ioutil"
"reflect"
"regexp"
"strings"
"testing"

Expand Down Expand Up @@ -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)
}
Expand Down Expand Up @@ -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")
}
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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)
Expand Down
5 changes: 5 additions & 0 deletions pkg/kube/util.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
84 changes: 84 additions & 0 deletions pkg/kube/util_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import (
"encoding/json"
"fmt"
"reflect"
"regexp"
"testing"

"github.com/argoproj-labs/argocd-vault-plugin/pkg/helpers"
Expand Down Expand Up @@ -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{}
Expand Down
Loading