From 23cadcdd14dc8de62113ef22507817a54dbb587e Mon Sep 17 00:00:00 2001 From: Dziyana Tsetserava <67862638+sea-gull-diana@users.noreply.github.com> Date: Tue, 10 Jun 2025 16:09:23 +0200 Subject: [PATCH 1/7] feat: detect and unescape url-encoded placeholder Signed-off-by: DziyanaT --- pkg/kube/util.go | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/pkg/kube/util.go b/pkg/kube/util.go index aeaccc20..d6e8c3dc 100644 --- a/pkg/kube/util.go +++ b/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 { @@ -106,6 +107,14 @@ func genericReplacement(key, value string, resource Resource) (_ interface{}, er var nonStringReplacement interface{} var placeholderRegex = specificPathPlaceholder + decoded, decodeError := url.QueryUnescape(value) + if decodeError == nil && decoded != value && placeholderRegex.Match([]byte(decoded)) { + res, err := genericReplacement(key, string(decoded), resource) + + utils.VerboseToStdErr("key %s had URL encoded placeholder value, URL encoding value %s to fit", key, value) + return url.QueryEscape(stringify(res)), err + } + // 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 that aren't placeholders From 69a5e84c33c88c89ef3def9beef9ccca1f53b48c Mon Sep 17 00:00:00 2001 From: DziyanaT Date: Thu, 12 Jun 2025 00:47:14 +0200 Subject: [PATCH 2/7] feat: decode and reencode only placeholders instead of entire value Signed-off-by: DziyanaT --- pkg/kube/util.go | 24 +++++++++++++++++++----- 1 file changed, 19 insertions(+), 5 deletions(-) diff --git a/pkg/kube/util.go b/pkg/kube/util.go index d6e8c3dc..ddbf928c 100644 --- a/pkg/kube/util.go +++ b/pkg/kube/util.go @@ -26,6 +26,7 @@ func (e *missingKeyError) Error() string { var genericPlaceholder, _ = regexp.Compile(`(?mU)<(.*)>`) var specificPathPlaceholder, _ = regexp.Compile(`(?mU)`) +var specificPathUrlEncodedPlaceholder, _ = regexp.Compile(`(?mU)%3Cpath%3A(.+)%23(.+)(?:%23(.+))?%3E`) var indivPlaceholderSyntax, _ = regexp.Compile(`(?mU)path:(?P[^#]+?)#(?P[^#]+?)(?:#(?P.+?))??`) // replaceInner recurses through the given map and replaces the placeholders by calling `replacerFunc` @@ -107,12 +108,25 @@ func genericReplacement(key, value string, resource Resource) (_ interface{}, er var nonStringReplacement interface{} var placeholderRegex = specificPathPlaceholder - decoded, decodeError := url.QueryUnescape(value) - if decodeError == nil && decoded != value && placeholderRegex.Match([]byte(decoded)) { - res, err := genericReplacement(key, string(decoded), resource) + 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))) + }) - utils.VerboseToStdErr("key %s had URL encoded placeholder value, URL encoding value %s to fit", key, value) - return url.QueryEscape(stringify(res)), err + 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 From 05a26d017e9a68f9d031dc0d410a8721861895a6 Mon Sep 17 00:00:00 2001 From: DziyanaT Date: Thu, 12 Jun 2025 00:48:45 +0200 Subject: [PATCH 3/7] test: add tests for url-encoded placeholders Signed-off-by: DziyanaT --- pkg/kube/util_test.go | 168 ++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 168 insertions(+) diff --git a/pkg/kube/util_test.go b/pkg/kube/util_test.go index b09fef90..7cf82542 100644 --- a/pkg/kube/util_test.go +++ b/pkg/kube/util_test.go @@ -195,6 +195,133 @@ func TestGenericReplacement_specificPathWithValidation(t *testing.T) { }) } +func TestGenericReplacement_specificPathUrlEncoded(t *testing.T) { + // Test that the specific-path placeholder syntax is used to find/replace placeholders that were url-encoded + // along with the generic syntax, since the generic Vault path is defined + mv := helpers.MockVault{} + mv.LoadData(map[string]interface{}{ + "namespace": "default ns", + }) + + dummyResource := Resource{ + TemplateData: map[string]interface{}{ + "namespace": "%3Cpath%3Ablah%2Fblah%23namespace%3E", + "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 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 ns", + }) + + t.Run("valid path", func(t *testing.T) { + dummyResource := Resource{ + TemplateData: map[string]interface{}{ + "namespace": "%3Cpath%3Ablah%2Fblah%23namespace%3E", + "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 +443,47 @@ func TestGenericReplacement_multiString(t *testing.T) { assertSuccessfulReplacement(&dummyResource, &expected, t) } +func TestGenericReplacement_multiStringSpecificPathUrlEncoded(t *testing.T) { + mv := helpers.MockVault{} + mv.LoadData(map[string]interface{}{ + "name": "my app", + "tag": "v1", + }) + + dummyResource := Resource{ + TemplateData: map[string]interface{}{ + "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 TestGenericReplacement_Base64(t *testing.T) { dummyResource := Resource{ TemplateData: map[string]interface{}{ From 702fedcf3a44390325a8f424b53b7e6cb6d9a03e Mon Sep 17 00:00:00 2001 From: DziyanaT Date: Thu, 12 Jun 2025 12:41:26 +0200 Subject: [PATCH 4/7] feat: support url-encoded placeholders that were also base64-encoded Signed-off-by: DziyanaT --- pkg/kube/util.go | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/pkg/kube/util.go b/pkg/kube/util.go index ddbf928c..91d7c1af 100644 --- a/pkg/kube/util.go +++ b/pkg/kube/util.go @@ -108,6 +108,8 @@ 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)) { @@ -239,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) From 3781d7043e826bd0c8c4da90b08f1813d5fc995c Mon Sep 17 00:00:00 2001 From: DziyanaT Date: Thu, 12 Jun 2025 12:42:13 +0200 Subject: [PATCH 5/7] test: add a test for placeholders that are both url and base64-encoded Signed-off-by: DziyanaT --- pkg/kube/util_test.go | 39 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 39 insertions(+) diff --git a/pkg/kube/util_test.go b/pkg/kube/util_test.go index 7cf82542..67490fac 100644 --- a/pkg/kube/util_test.go +++ b/pkg/kube/util_test.go @@ -484,6 +484,45 @@ func TestGenericReplacement_multiStringSpecificPathUrlEncoded(t *testing.T) { 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": `JTNDcGF0aCUzQWJsYWglMkZibGFoJTIzdXNlcm5hbWUlM0U6Ly86JTNDcGF0aCUzQWJsYWglMkZibGFoJTIzcGFzc3dvcmQlM0VAcmVkaXMtbWFzdGVyLmhhcmJvci5zdmMuY2x1c3Rlci5sb2NhbC8wP2lkbGVfdGltZW91dF9zZWNvbmRzPTMwCg==`, + }, + 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": "cmVkaXM6Ly86cmVkaXMlNDAxMjNAcmVkaXMtbWFzdGVyLmhhcmJvci5zdmMuY2x1c3Rlci5sb2NhbC8wP2lkbGVfdGltZW91dF9zZWNvbmRzPTMwCg==", + }, + 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{}{ From 108a114871a146f72113767c0780c4f5432d9f07 Mon Sep 17 00:00:00 2001 From: DziyanaT Date: Thu, 12 Jun 2025 12:43:24 +0200 Subject: [PATCH 6/7] docs: add explanation about url-encoded placeholders detection Signed-off-by: DziyanaT --- docs/howitworks.md | 34 ++++++++++++++++++++++++++++++++++ 1 file changed, 34 insertions(+) diff --git a/docs/howitworks.md b/docs/howitworks.md index 6731a0f5..e82bc494 100644 --- a/docs/howitworks.md +++ b/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: +``` + +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%3E@redis-master.harbor.svc.cluster.local/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%40123@redis-master.harbor.svc.cluster.local/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 `` strings The plugin tries to be helpful and will ignore strings in the format `` if the `avp.kubernetes.io/path` annotation is missing, and only try to replace [inline-path placeholders](#inline-path-placeholders) From 4673a1a316e402967b7abdbb4b29d4bb8c1a0726 Mon Sep 17 00:00:00 2001 From: DziyanaT Date: Thu, 12 Jun 2025 13:10:18 +0200 Subject: [PATCH 7/7] test: change value used for testing url&base64-encoded placeholders, add comments Signed-off-by: DziyanaT --- pkg/kube/util_test.go | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/pkg/kube/util_test.go b/pkg/kube/util_test.go index 67490fac..b1ba5041 100644 --- a/pkg/kube/util_test.go +++ b/pkg/kube/util_test.go @@ -196,8 +196,7 @@ func TestGenericReplacement_specificPathWithValidation(t *testing.T) { } func TestGenericReplacement_specificPathUrlEncoded(t *testing.T) { - // Test that the specific-path placeholder syntax is used to find/replace placeholders that were url-encoded - // along with the generic syntax, since the generic Vault path is defined + // 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", @@ -240,8 +239,7 @@ func TestGenericReplacement_specificPathUrlEncoded(t *testing.T) { } func TestGenericReplacement_specificPathUrlEncodedWithValidation(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 + // 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", @@ -444,6 +442,7 @@ func TestGenericReplacement_multiString(t *testing.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", @@ -495,7 +494,7 @@ func TestSecretReplacement_SpecificPathUrlEncoded_Base64Encoded(t *testing.T) { dummyResource := Resource{ TemplateData: map[string]interface{}{ - "url": `JTNDcGF0aCUzQWJsYWglMkZibGFoJTIzdXNlcm5hbWUlM0U6Ly86JTNDcGF0aCUzQWJsYWglMkZibGFoJTIzcGFzc3dvcmQlM0VAcmVkaXMtbWFzdGVyLmhhcmJvci5zdmMuY2x1c3Rlci5sb2NhbC8wP2lkbGVfdGltZW91dF9zZWNvbmRzPTMwCg==`, + "url": `cmVkaXM6Ly8lM0NwYXRoJTNBYmxhaCUyRmJsYWglMjN1c2VybmFtZSUzRTolM0NwYXRoJTNBYmxhaCUyRmJsYWglMjNwYXNzd29yZCUzRUByZWRpcy1tYXN0ZXIuaGFyYm9yLnN2Yy5jbHVzdGVyLmxvY2FsLzA/aWRsZV90aW1lb3V0X3NlY29uZHM9MzAK`, }, Data: map[string]interface{}{ "password": "test", @@ -511,7 +510,7 @@ func TestSecretReplacement_SpecificPathUrlEncoded_Base64Encoded(t *testing.T) { expected := Resource{ TemplateData: map[string]interface{}{ - "url": "cmVkaXM6Ly86cmVkaXMlNDAxMjNAcmVkaXMtbWFzdGVyLmhhcmJvci5zdmMuY2x1c3Rlci5sb2NhbC8wP2lkbGVfdGltZW91dF9zZWNvbmRzPTMwCg==", + "url": "cmVkaXM6Ly9yZWRpczpyZWRpcyU0MDEyM0ByZWRpcy1tYXN0ZXIuaGFyYm9yLnN2Yy5jbHVzdGVyLmxvY2FsLzA/aWRsZV90aW1lb3V0X3NlY29uZHM9MzAK", }, Data: map[string]interface{}{ "password": "test",