feat: detect & decode url-encoded placeholders, reencode after replacement

docs/howitworks.md

@@ -153,6 +153,40 @@ data:
   POSTGRES_URL: cG9zdGdyZXM6Ly91c2VyOnBhc3NAaG9zdDo5NDQzL215LWRiP3NzbG1vZGU9cmVxdWlyZQ==
 ```
 
+##### URL encoded placeholders
+When using plugin with a Helm chart, it is possible to use placeholders in values file. If a chart applies Helm's `urlquery` function to the value in order to safely include it in an URL, the placeholder will end up looking like this: `%3Cpath%3Asome%2Fpath%23secret-key%3E`.
+
+The plugin can handle this case by finding any url encoded placeholders (inline-path only), replacing them, and re-url encoding the result.
+
+For example, imagine that we have this value file:
+
+```yaml
+redis:
+  external:
+    addr: "redis-master.harbor.svc.cluster.local"
+    password: <path:kv/data/config/redis-pwd#password>
+```
+
+And that the Helm chart passes the password value through `urlquery`, combines it with other data into a connection string and then adds it to a ConfigMap or Secret looking like this:
+
+```yaml
+data:
+  _REDIS_URL_CORE: >-
+    redis://:%3Cpath%3Akv%2Fdata%2Fconfig%2Fredis-pwd%23password%[email protected]/0?idle_timeout_seconds=30
+```
+
+The plugin will be able to find the placeholder `%3Cpath%3Akv%2Fdata%2Fconfig%2Fredis-pwd%23password%3E`, decode it, get the password value (for example, "redis@123"), re-encode the value as "redis%40123" and put it back in the connection string.
+
+Thus, the output will look like this:
+
+```yaml
+data:
+  _REDIS_URL_CORE: >-
+    redis://:redis%[email protected]/0?idle_timeout_seconds=30
+```
+
+It will work even if the string with url-encoded placeholders was added to a Secret and base64-encoded.
+
 ##### Automatically ignoring `<placeholder>` strings
 The plugin tries to be helpful and will ignore strings in the format `<string>` if the `avp.kubernetes.io/path` annotation is missing, and only try to replace [inline-path placeholders](#inline-path-placeholders)
 

pkg/kube/util.go

@@ -13,6 +13,7 @@ import (
 	"github.com/argoproj-labs/argocd-vault-plugin/pkg/types"
 	"github.com/argoproj-labs/argocd-vault-plugin/pkg/utils"
 	k8yaml "k8s.io/apimachinery/pkg/util/yaml"
+	"net/url"
 )
 
 type missingKeyError struct {
@@ -25,6 +26,7 @@ func (e *missingKeyError) Error() string {
 
 var genericPlaceholder, _ = regexp.Compile(`(?mU)<(.*)>`)
 var specificPathPlaceholder, _ = regexp.Compile(`(?mU)<path:([^#]+)#([^#]+)(?:#([^#]+))?>`)
+var specificPathUrlEncodedPlaceholder, _ = regexp.Compile(`(?mU)%3Cpath%3A(.+)%23(.+)(?:%23(.+))?%3E`)
 var indivPlaceholderSyntax, _ = regexp.Compile(`(?mU)path:(?P<path>[^#]+?)#(?P<key>[^#]+?)(?:#(?P<version>.+?))??`)
 
 // replaceInner recurses through the given map and replaces the placeholders by calling `replacerFunc`
@@ -106,6 +108,29 @@ func genericReplacement(key, value string, resource Resource) (_ interface{}, er
 	var nonStringReplacement interface{}
 	var placeholderRegex = specificPathPlaceholder
 
+	// If some sepecific path placeholders were URL-encoded, we match them with a special regex, 
+	// decode them, get secret values and re-encode them.
+	decodedValue := specificPathUrlEncodedPlaceholder.ReplaceAllFunc([]byte(value), func(match []byte) []byte {
+		decoded, decErr:= url.QueryUnescape(string(match))
+		if decErr != nil || !placeholderRegex.Match([]byte(decoded)) {
+			err = append(err, decErr)
+			return match
+		}
+
+		repl, replErr := genericReplacement(key, decoded, resource)
+		if replErr != nil {
+			err = append(err, replErr...)
+			return match
+		}
+
+		return []byte(url.QueryEscape(stringify(repl)))
+	})
+
+	if string(decodedValue) != value {
+		utils.VerboseToStdErr("key %s had value with URL encoded placeholder", key)
+		value = string(decodedValue)
+	}
+
 	// If the Vault path annotation is present, there may be placeholders with/without an explicit path
 	// so we look for those. Only if the annotation is absent do we narrow the search to placeholders with
 	// explicit paths, to prevent catching <things> that aren't placeholders
@@ -216,7 +241,7 @@ func configReplacement(key, value string, resource Resource) (interface{}, []err
 
 func secretReplacement(key, value string, resource Resource) (interface{}, []error) {
 	decoded, err := base64.StdEncoding.DecodeString(value)
-	if err == nil && genericPlaceholder.Match(decoded) {
+	if err == nil && (genericPlaceholder.Match(decoded) || specificPathUrlEncodedPlaceholder.Match(decoded)) {
 		res, err := genericReplacement(key, string(decoded), resource)
 
 		utils.VerboseToStdErr("key %s comes from Secret manifest, base64 encoding value %s to fit", key, value)

pkg/kube/util_test.go

@@ -195,6 +195,131 @@ func TestGenericReplacement_specificPathWithValidation(t *testing.T) {
 	})
 }
 
+func TestGenericReplacement_specificPathUrlEncoded(t *testing.T) {
+	// Test that the generic replacement function can find/replace placeholders that were url-encoded
+	mv := helpers.MockVault{}
+	mv.LoadData(map[string]interface{}{
+		"namespace": "default ns",
+	})
+
+	dummyResource := Resource{
+		TemplateData: map[string]interface{}{
+			"namespace": "%3Cpath%3Ablah%2Fblah%23namespace%3E",
+			"name":      "<name>",
+		},
+		Data: map[string]interface{}{
+			"namespace": "something-else",
+			"name":      "foo",
+		},
+		Backend: &mv,
+		Annotations: map[string]string{
+			(types.AVPPathAnnotation): "",
+		},
+	}
+
+	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+ns",
+			"name":      "foo",
+		},
+		Data: map[string]interface{}{
+			"namespace": "something-else",
+			"name":      "foo",
+		},
+		replacementErrors: []error{},
+	}
+
+	assertSuccessfulReplacement(&dummyResource, &expected, t)
+}
+
+func TestGenericReplacement_specificPathUrlEncodedWithValidation(t *testing.T) {
+	// Test that the generic replacement function can find/replace placeholders that were url-encoded
+	mv := helpers.MockVault{}
+	mv.LoadData(map[string]interface{}{
+		"namespace": "default ns",
+	})
+
+	t.Run("valid path", func(t *testing.T) {
+		dummyResource := Resource{
+			TemplateData: map[string]interface{}{
+				"namespace": "%3Cpath%3Ablah%2Fblah%23namespace%3E",
+				"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+ns",
+				"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": "%3Cpath%3A..%2Fblah%2Fblah%23namespace%3E",
+			},
+			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": "%3Cpath%3A..%2Fblah%2Fblah%23namespace%3E",
+			},
+			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{}
@@ -316,6 +441,87 @@ func TestGenericReplacement_multiString(t *testing.T) {
 	assertSuccessfulReplacement(&dummyResource, &expected, t)
 }
 
+func TestGenericReplacement_multiStringSpecificPathUrlEncoded(t *testing.T) {
+	// Test that multiple url-encoded placeholders in one value string can all be found/replaced.
+	mv := helpers.MockVault{}
+	mv.LoadData(map[string]interface{}{
+		"name": "my app",
+		"tag": "v1",
+	})
+
+	dummyResource := Resource{
+		TemplateData: map[string]interface{}{
+			"namespace": "<namespace>",
+			"image":     "foo.io/%3Cpath%3Ablah%2Fblah%23name%3E:%3Cpath%3Ablah%2Fblah%23tag%3E",
+		},
+		Data: map[string]interface{}{
+			"namespace": "default",
+			"name":      "app",
+			"tag":       "latest",
+		},
+		Backend: &mv,
+		Annotations: map[string]string{
+			(types.AVPPathAnnotation): "",
+		},
+	}
+
+	replaceInner(&dummyResource, &dummyResource.TemplateData, genericReplacement)
+
+	expected := Resource{
+		TemplateData: map[string]interface{}{
+			"namespace": "default",
+			"image":     "foo.io/my+app:v1",
+		},
+		Data: map[string]interface{}{
+			"namespace": "default",
+			"name":      "app",
+			"tag":       "latest",
+		},
+		replacementErrors: []error{},
+	}
+
+	assertSuccessfulReplacement(&dummyResource, &expected, t)
+}
+
+func TestSecretReplacement_SpecificPathUrlEncoded_Base64Encoded(t *testing.T) {
+	// Test that the secret replacement function can find/replace placeholders that were url-encoded
+	// and then base64-encoded.
+	mv := helpers.MockVault{}
+	mv.LoadData(map[string]interface{}{
+		"password": "redis@123",
+		"username": "redis",
+	})
+
+	dummyResource := Resource{
+		TemplateData: map[string]interface{}{
+			"url": `cmVkaXM6Ly8lM0NwYXRoJTNBYmxhaCUyRmJsYWglMjN1c2VybmFtZSUzRTolM0NwYXRoJTNBYmxhaCUyRmJsYWglMjNwYXNzd29yZCUzRUByZWRpcy1tYXN0ZXIuaGFyYm9yLnN2Yy5jbHVzdGVyLmxvY2FsLzA/aWRsZV90aW1lb3V0X3NlY29uZHM9MzAK`,
+		},
+		Data: map[string]interface{}{
+			"password": "test",
+			"username": "test",
+		},
+		Backend: &mv,
+		Annotations: map[string]string{
+			(types.AVPPathAnnotation): "",
+		},
+	}
+
+	replaceInner(&dummyResource, &dummyResource.TemplateData, secretReplacement)
+
+	expected := Resource{
+		TemplateData: map[string]interface{}{
+			"url": "cmVkaXM6Ly9yZWRpczpyZWRpcyU0MDEyM0ByZWRpcy1tYXN0ZXIuaGFyYm9yLnN2Yy5jbHVzdGVyLmxvY2FsLzA/aWRsZV90aW1lb3V0X3NlY29uZHM9MzAK",
+		},
+		Data: map[string]interface{}{
+			"password": "test",
+			"username": "test",
+		},
+		replacementErrors: []error{},
+	}
+
+	assertSuccessfulReplacement(&dummyResource, &expected, t)
+}
+
 func TestGenericReplacement_Base64(t *testing.T) {
 	dummyResource := Resource{
 		TemplateData: map[string]interface{}{