Repository: external-secrets/external-secrets (go)

Issue:
Reverse unicode conversion strategy for the PushSecret object
**Is your feature request related to a problem? Please describe.**
Certain use cases require defining keys which would have invalid characters for Kubernetes Secret naming conventions standpoint, for example using square brackets to access indexed arrays like `myArray[0].someProperty`, the `ExternalSecret` object handles that nicely via the support for `conversionStrategy: Unicode`, which will essentially encode invalid characters into their unicode representation, like `_U005b_`. However if you later want to push a secret like that to a different provider using the `PushSecret` object, you probably what to get the secret synced on it's original form, meaning a reverse unicode conversion strategy should be in place to revert the unicode representation back to the original characters.

**Describe the solution you'd like**
It would be nice if the `PushSecret` object also offers a `conversionStrategy` feature, allowing the user to opt-in for a reverse encoding strategy.



Pull request:
fix: introducing support for conversion strategy for PushSecret.
## Problem Statement

Certain use cases require defining keys which would have invalid characters for Kubernetes Secret naming conventions standpoint, for example using square brackets to access indexed arrays like `myArray[0].someProperty`, the `ExternalSecret` object handles that nicely via the support for `conversionStrategy: Unicode`, which will essentially encode invalid characters into their unicode representation, like `_U005b_`. However if you later want to push a secret like that to a different provider using the `PushSecret` object, you probably what to get the secret synced on it's original form, meaning a reverse unicode conversion strategy should be in place to revert the unicode representation back to the original characters.

## Related Issue

Fixes #3291

## Proposed Changes

Introduces support for `conversionStrategy` in the PushSecret object.

## Checklist

- [x] I have read the [contribution guidelines](https://external-secrets.io/latest/contributing/process/#submitting-a-pull-request)
- [x] All commits are signed with `git commit --signoff`
- [x] My changes have reasonable test coverage
- [x] All tests pass with `make test`
- [x] I ensured my PR is ready for review with `make reviewable`


Patch:
diff --git a/apis/externalsecrets/v1alpha1/pushsecret_types.go b/apis/externalsecrets/v1alpha1/pushsecret_types.go
index b645bce85..3fc221620 100644
--- a/apis/externalsecrets/v1alpha1/pushsecret_types.go
+++ b/apis/externalsecrets/v1alpha1/pushsecret_types.go
@@ -57,6 +57,14 @@ const (
 	PushSecretDeletionPolicyNone   PushSecretDeletionPolicy = "None"
 )
 
+// +kubebuilder:validation:Enum=None;ReverseUnicode
+type PushSecretConversionStrategy string
+
+const (
+	PushSecretConversionNone           PushSecretConversionStrategy = "None"
+	PushSecretConversionReverseUnicode PushSecretConversionStrategy = "ReverseUnicode"
+)
+
 // PushSecretSpec configures the behavior of the PushSecret.
 type PushSecretSpec struct {
 	// The Interval to which External Secrets will try to push a secret definition
@@ -121,6 +129,10 @@ type PushSecretData struct {
 	// The structure of metadata is provider specific, please look it up in the provider documentation.
 	// +optional
 	Metadata *apiextensionsv1.JSON `json:"metadata,omitempty"`
+	// +optional
+	// Used to define a conversion Strategy for the secret keys
+	// +kubebuilder:default="None"
+	ConversionStrategy PushSecretConversionStrategy `json:"conversionStrategy,omitempty"`
 }
 
 func (d PushSecretData) GetMetadata() *apiextensionsv1.JSON {
diff --git a/config/crds/bases/external-secrets.io_pushsecrets.yaml b/config/crds/bases/external-secrets.io_pushsecrets.yaml
index 7562ca994..24016035a 100644
--- a/config/crds/bases/external-secrets.io_pushsecrets.yaml
+++ b/config/crds/bases/external-secrets.io_pushsecrets.yaml
@@ -50,6 +50,14 @@ spec:
                 description: Secret Data that should be pushed to providers
                 items:
                   properties:
+                    conversionStrategy:
+                      default: None
+                      description: Used to define a conversion Strategy for the secret
+                        keys
+                      enum:
+                      - None
+                      - ReverseUnicode
+                      type: string
                     match:
                       description: Match a given Secret Key to be pushed to the provider.
                       properties:
@@ -315,6 +323,14 @@ spec:
                 additionalProperties:
                   additionalProperties:
                     properties:
+                      conversionStrategy:
+                        default: None
+                        description: Used to define a conversion Strategy for the
+                          secret keys
+                        enum:
+                        - None
+                        - ReverseUnicode
+                        type: string
                       match:
                         description: Match a given Secret Key to be pushed to the
                           provider.
diff --git a/deploy/crds/bundle.yaml b/deploy/crds/bundle.yaml
index d7b4945f0..dccc76e8e 100644
--- a/deploy/crds/bundle.yaml
+++ b/deploy/crds/bundle.yaml
@@ -5547,6 +5547,13 @@ spec:
                   description: Secret Data that should be pushed to providers
                   items:
                     properties:
+                      conversionStrategy:
+                        default: None
+                        description: Used to define a conversion Strategy for the secret keys
+                        enum:
+                          - None
+                          - ReverseUnicode
+                        type: string
                       match:
                         description: Match a given Secret Key to be pushed to the provider.
                         properties:
@@ -5802,6 +5809,13 @@ spec:
                   additionalProperties:
                     additionalProperties:
                       properties:
+                        conversionStrategy:
+                          default: None
+                          description: Used to define a conversion Strategy for the secret keys
+                          enum:
+                            - None
+                            - ReverseUnicode
+                          type: string
                         match:
                           description: Match a given Secret Key to be pushed to the provider.
                           properties:
diff --git a/docs/guides/all-keys-one-secret.md b/docs/guides/all-keys-one-secret.md
index 048a580c3..7453a0831 100644
--- a/docs/guides/all-keys-one-secret.md
+++ b/docs/guides/all-keys-one-secret.md
@@ -9,7 +9,7 @@ Then create a secret in Google Cloud Secret Manager that contains a JSON string
 ![secret-value](../pictures/screenshot_json_string_gcp_secret_value.png)
 
 Let's call this secret all-keys-example-secret on Google Cloud.
-
+ 
 
 ### Creating dataFrom external secret
 
@@ -18,10 +18,53 @@ Now, when creating our ExternalSecret resource, instead of using the data field,
 ```yaml
 {% include 'gcpsm-data-from-external-secret.yaml' %}
 ```
+Here, "example" is the name of the external secret that will be created in our cluster.    
+Whereas, "secret-to-be-created" is the name of Kubernetes secrets that will be created.    
+Note: Since these secrets are namespace-based resources, you can also explicitly specify the "namespace" under the "metadata" block of the above external secret file.    
+when we use, 
 
-To check both values we can run:
+```
+  dataFrom:
+  - extract:
+      key: all-keys-example-secret
+```
+We get all the key-value pairs present over the remote secret store (GCP or AWS or Azure) and can pass either all or a few key-values as environment variables.    
+Please note that, "all-keys-example-secret" is the name of your secret present on GCP/AWS secrets manager/Azure    
+    
+We can pass a few secrets as env variables as below:
+```
+        env:
+          - name: key1
+            valueFrom:
+              secretKeyRef:
+                name: secret-to-be-created
+                key: username
 
+          - name: key2
+            valueFrom:
+              secretKeyRef:
+                name: secret-to-be-created
+                key: surname
+```
+ 
+Here,    
+\<key1\> and \<key2> are the names of keys that will be created and passed as env variables.    
+\<secret-to-be-created\>: is the name of your Kubernetes secret created by you.    
+\<username\> and \<surname>: is the particular key in the secrets manager whose value you want to pass.    
+To check both values we can run:
 ```
 kubectl get secret secret-to-be-created -n <namespace> -o jsonpath='{.data.username}' | base64 -d
 kubectl get secret secret-to-be-created -n <namespace> -o jsonpath='{.data.surname}' | base64 -d
 ```
+
+Also, if you have a large number of secrets and you want to pass all of them as enviromnent variables, then either you can replicate the above steps in your deployment file for all the keys or you can use the envFrom block as below:    
+
+```
+    spec:
+      containers:
+      - command:
+        - mkdir abc.sh
+        envFrom:
+        - secretRef:
+            name: secret-to-be-created
+```
diff --git a/docs/guides/pushsecrets.md b/docs/guides/pushsecrets.md
index 757d126ce..2e0fc959b 100644
--- a/docs/guides/pushsecrets.md
+++ b/docs/guides/pushsecrets.md
@@ -40,3 +40,6 @@ This will _marshal_ the entire secret data and push it into this single property
 
 !!! warning inline
     This should _ONLY_ be done if the secret data is marshal-able. Values like, binary data cannot be marshaled and will result in error or invalid secret data.
+
+### Key conversion strategy
+You can also set `data[*].conversionStrategy: ReverseUnicode` to reverse the invalid character replaced by the `conversionStrategy: Unicode` configuration in the `ExternalSecret` object as [documented here](../guides/getallsecrets/#avoiding-name-conflicts).
diff --git a/docs/snippets/full-pushsecret.yaml b/docs/snippets/full-pushsecret.yaml
index 0976c8f3a..7b5a3c87c 100644
--- a/docs/snippets/full-pushsecret.yaml
+++ b/docs/snippets/full-pushsecret.yaml
@@ -29,7 +29,8 @@ spec:
           items:
             - key: config.yml
   data:
-    - match:
+    - conversionStrategy: None # Also supports the ReverseUnicode strategy
+      match:
         secretKey: best-pokemon # Source Kubernetes secret key to be pushed
         remoteRef:
           remoteKey: my-first-parameter # Remote reference (where the secret is going to be pushed)
diff --git a/pkg/controllers/pushsecret/pushsecret_controller.go b/pkg/controllers/pushsecret/pushsecret_controller.go
index 700f2fc38..227d13704 100644
--- a/pkg/controllers/pushsecret/pushsecret_controller.go
+++ b/pkg/controllers/pushsecret/pushsecret_controller.go
@@ -38,6 +38,7 @@ import (
 	"github.com/external-secrets/external-secrets/pkg/controllers/pushsecret/psmetrics"
 	"github.com/external-secrets/external-secrets/pkg/controllers/secretstore"
 	"github.com/external-secrets/external-secrets/pkg/provider/util/locks"
+	"github.com/external-secrets/external-secrets/pkg/utils"
 )
 
 const (
@@ -47,6 +48,7 @@ const (
 	errGetClusterSecretStore = "could not get ClusterSecretStore %q, %w"
 	errSetSecretFailed       = "could not write remote ref %v to target secretstore %v: %v"
 	errFailedSetSecret       = "set secret failed: %v"
+	errConvert               = "could not apply conversion strategy to keys: %v"
 	pushSecretFinalizer      = "pushsecret.externalsecrets.io/finalizer"
 )
 
@@ -289,11 +291,17 @@ func (r *Reconciler) handlePushSecretDataForStore(ctx context.Context, ps esapi.
 		Name: storeName,
 		Kind: refKind,
 	}
+	originalSecretData := secret.Data
 	secretClient, err := mgr.Get(ctx, storeRef, ps.GetNamespace(), nil)
 	if err != nil {
 		return out, fmt.Errorf("could not get secrets client for store %v: %w", storeName, err)
 	}
 	for _, data := range ps.Spec.Data {
+		secretData, err := utils.ReverseKeys(data.ConversionStrategy, originalSecretData)
+		if err != nil {
+			return nil, fmt.Errorf(errConvert, err)
+		}
+		secret.Data = secretData
 		key := data.GetSecretKey()
 		if !secretKeyExists(key, secret) {
 			return out, fmt.Errorf("secret key %v does not exist", key)
diff --git a/pkg/utils/utils.go b/pkg/utils/utils.go
index 43dabdeff..cc2805bb8 100644
--- a/pkg/utils/utils.go
+++ b/pkg/utils/utils.go
@@ -33,6 +33,7 @@ import (
 
 	apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1"
 
+	esv1alpha1 "github.com/external-secrets/external-secrets/apis/externalsecrets/v1alpha1"
 	esv1beta1 "github.com/external-secrets/external-secrets/apis/externalsecrets/v1beta1"
 	esmeta "github.com/external-secrets/external-secrets/apis/meta/v1"
 	"github.com/external-secrets/external-secrets/pkg/template/v2"
@@ -45,6 +46,7 @@ const (
 
 var (
 	errKeyNotFound = errors.New("key not found")
+	unicodeRegex   = regexp.MustCompile(`_U([0-9a-fA-F]{4,5})_`)
 )
 
 // JSONMarshal takes an interface and returns a new escaped and encoded byte slice.
@@ -237,6 +239,48 @@ func convert(strategy esv1beta1.ExternalSecretConversionStrategy, str string) st
 	return strings.Join(newName, "")
 }
 
+// ReverseKeys reverses a secret map into a valid key map as expected by push secrets.
+// Replaces the unicode encoded representation characters back to the actual unicode character depending on convert strategy.
+func ReverseKeys(strategy esv1alpha1.PushSecretConversionStrategy, in map[string][]byte) (map[string][]byte, error) {
+	out := make(map[string][]byte, len(in))
+	for k, v := range in {
+		key := reverse(strategy, k)
+		if _, exists := out[key]; exists {
+			return nil, fmt.Errorf("secret name collision during conversion: %s", key)
+		}
+		out[key] = v
+	}
+	return out, nil
+}
+
+func reverse(strategy esv1alpha1.PushSecretConversionStrategy, str string) string {
+	switch strategy {
+	case esv1alpha1.PushSecretConversionReverseUnicode:
+		matches := unicodeRegex.FindAllStringSubmatchIndex(str, -1)
+
+		for i := len(matches) - 1; i >= 0; i-- {
+			match := matches[i]
+			start := match[0]
+			end := match[1]
+			unicodeHex := str[match[2]:match[3]]
+
+			unicodeInt, err := strconv.ParseInt(unicodeHex, 16, 32)
+			if err != nil {
+				continue // Skip invalid unicode representations
+			}
+
+			unicodeChar := fmt.Sprintf("%c", unicodeInt)
+			str = str[:start] + unicodeChar + str[end:]
+		}
+
+		return str
+	case esv1alpha1.PushSecretConversionNone:
+		return str
+	default:
+		return str
+	}
+}
+
 // MergeStringMap performs a deep clone from src to dest.
 func MergeStringMap(dest, src map[string]string) {
 	for k, v := range src {


Test patch:
diff --git a/pkg/controllers/pushsecret/pushsecret_controller_test.go b/pkg/controllers/pushsecret/pushsecret_controller_test.go
index ef434ec32..3e77c12b6 100644
--- a/pkg/controllers/pushsecret/pushsecret_controller_test.go
+++ b/pkg/controllers/pushsecret/pushsecret_controller_test.go
@@ -637,6 +637,64 @@ var _ = Describe("ExternalSecret controller", func() {
 			return true
 		}
 	}
+	// if conversion strategy is defined, revert the keys based on the strategy.
+	syncSuccessfullyWithConversionStrategy := func(tc *testCase) {
+		fakeProvider.SetSecretFn = func() error {
+			return nil
+		}
+		tc.pushsecret = &v1alpha1.PushSecret{
+			ObjectMeta: metav1.ObjectMeta{
+				Name:      PushSecretName,
+				Namespace: PushSecretNamespace,
+			},
+			Spec: v1alpha1.PushSecretSpec{
+				SecretStoreRefs: []v1alpha1.PushSecretStoreRef{
+					{
+						Name: PushSecretStore,
+						Kind: "SecretStore",
+					},
+				},
+				Selector: v1alpha1.PushSecretSelector{
+					Secret: v1alpha1.PushSecretSecret{
+						Name: SecretName,
+					},
+				},
+				Data: []v1alpha1.PushSecretData{
+					{
+						ConversionStrategy: v1alpha1.PushSecretConversionReverseUnicode,
+						Match: v1alpha1.PushSecretMatch{
+							SecretKey: "some-array[0].entity",
+							RemoteRef: v1alpha1.PushSecretRemoteRef{
+								RemoteKey: "path/to/key",
+							},
+						},
+					},
+				},
+			},
+		}
+		tc.secret = &v1.Secret{
+			ObjectMeta: metav1.ObjectMeta{
+				Name:      SecretName,
+				Namespace: PushSecretNamespace,
+			},
+			Data: map[string][]byte{
+				"some-array_U005b_0_U005d_.entity": []byte("value"),
+			},
+		}
+		tc.assert = func(ps *v1alpha1.PushSecret, secret *v1.Secret) bool {
+			Eventually(func() bool {
+				By("checking if Provider value got updated")
+				secretValue := secret.Data["some-array_U005b_0_U005d_.entity"]
+				providerValue, ok := fakeProvider.SetSecretArgs[ps.Spec.Data[0].Match.RemoteRef.RemoteKey]
+				if !ok {
+					return false
+				}
+				got := providerValue.Value
+				return bytes.Equal(got, secretValue)
+			}, time.Second*10, time.Second).Should(BeTrue())
+			return true
+		}
+	}
 	// if target Secret name is not specified it should use the ExternalSecret name.
 	syncMatchingLabels := func(tc *testCase) {
 		fakeProvider.SetSecretFn = func() error {
@@ -937,6 +995,7 @@ var _ = Describe("ExternalSecret controller", func() {
 		Entry("should update the PushSecret status correctly if UpdatePolicy=IfNotExists", updateIfNotExistsSyncStatus),
 		Entry("should fail if secret existence cannot be verified if UpdatePolicy=IfNotExists", updateIfNotExistsSyncFailed),
 		Entry("should sync with template", syncSuccessfullyWithTemplate),
+		Entry("should sync with conversion strategy", syncSuccessfullyWithConversionStrategy),
 		Entry("should delete if DeletionPolicy=Delete", syncAndDeleteSuccessfully),
 		Entry("should track deletion tasks if Delete fails", failDelete),
 		Entry("should track deleted stores if Delete fails", failDeleteStore),
diff --git a/pkg/utils/utils_test.go b/pkg/utils/utils_test.go
index f62c089fd..97103bc57 100644
--- a/pkg/utils/utils_test.go
+++ b/pkg/utils/utils_test.go
@@ -24,13 +24,17 @@ import (
 	v1 "k8s.io/api/core/v1"
 	apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1"
 
+	esv1alpha1 "github.com/external-secrets/external-secrets/apis/externalsecrets/v1alpha1"
 	esv1beta1 "github.com/external-secrets/external-secrets/apis/externalsecrets/v1beta1"
 )
 
 const (
-	base64DecodedValue    string = "foo%_?bar"
-	base64EncodedValue    string = "Zm9vJV8/YmFy"
-	base64URLEncodedValue string = "Zm9vJV8_YmFy"
+	base64DecodedValue         string = "foo%_?bar"
+	base64EncodedValue         string = "Zm9vJV8/YmFy"
+	base64URLEncodedValue      string = "Zm9vJV8_YmFy"
+	keyWithEmojis              string = "😀foo😁bar😂baz😈bing"
+	keyWithInvalidChars        string = "some-array[0].entity"
+	keyWithEncodedInvalidChars string = "some-array_U005b_0_U005d_.entity"
 )
 
 func TestObjectHash(t *testing.T) {
@@ -212,7 +216,7 @@ func TestConvertKeys(t *testing.T) {
 			args: args{
 				strategy: esv1beta1.ExternalSecretConversionUnicode,
 				in: map[string][]byte{
-					"😀foo😁bar😂baz😈bing": []byte(`noop`),
+					keyWithEmojis: []byte(`noop`),
 				},
 			},
 			want: map[string][]byte{
@@ -234,6 +238,77 @@ func TestConvertKeys(t *testing.T) {
 	}
 }
 
+func TestReverseKeys(t *testing.T) {
+	type args struct {
+		encodingStrategy esv1beta1.ExternalSecretConversionStrategy
+		decodingStrategy esv1alpha1.PushSecretConversionStrategy
+		in               map[string][]byte
+	}
+	tests := []struct {
+		name    string
+		args    args
+		want    map[string][]byte
+		wantErr bool
+	}{
+		{
+			name: "encoding and decoding strategy are selecting Unicode conversion and reverse unicode, so the in and want should match, this test covers Unicode characters beyond the Basic Multilingual Plane (BMP)",
+			args: args{
+				encodingStrategy: esv1beta1.ExternalSecretConversionUnicode,
+				decodingStrategy: esv1alpha1.PushSecretConversionReverseUnicode,
+				in: map[string][]byte{
+					keyWithEmojis: []byte(`noop`),
+				},
+			},
+			want: map[string][]byte{
+				keyWithEmojis: []byte(`noop`),
+			},
+		},
+		{
+			name: "encoding and decoding strategy are selecting Unicode conversion and reverse unicode, so the in and want should match, this test covers Unicode characters in the Basic Multilingual Plane (BMP)",
+			args: args{
+				encodingStrategy: esv1beta1.ExternalSecretConversionUnicode,
+				decodingStrategy: esv1alpha1.PushSecretConversionReverseUnicode,
+				in: map[string][]byte{
+					keyWithInvalidChars: []byte(`noop`),
+				},
+			},
+			want: map[string][]byte{
+				keyWithInvalidChars: []byte(`noop`),
+			},
+		},
+		{
+			name: "the encoding strategy is selecting Unicode conversion, but the decoding strategy is none, so we want an encoded representation of the content",
+			args: args{
+				encodingStrategy: esv1beta1.ExternalSecretConversionUnicode,
+				decodingStrategy: esv1alpha1.PushSecretConversionNone,
+				in: map[string][]byte{
+					keyWithInvalidChars: []byte(`noop`),
+				},
+			},
+			want: map[string][]byte{
+				keyWithEncodedInvalidChars: []byte(`noop`),
+			},
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			got, err := ConvertKeys(tt.args.encodingStrategy, tt.args.in)
+			if (err != nil) != tt.wantErr {
+				t.Errorf("ConvertKeys() error = %v, wantErr %v", err, tt.wantErr)
+				return
+			}
+			got, err = ReverseKeys(tt.args.decodingStrategy, got)
+			if (err != nil) != tt.wantErr {
+				t.Errorf("ReverseKeys() error = %v, wantErr %v", err, tt.wantErr)
+				return
+			}
+			if !reflect.DeepEqual(got, tt.want) {
+				t.Errorf("ReverseKeys() = %v, want %v", got, tt.want)
+			}
+		})
+	}
+}
+
 func TestDecode(t *testing.T) {
 	type args struct {
 		strategy esv1beta1.ExternalSecretDecodingStrategy
@@ -542,6 +617,50 @@ func TestRewrite(t *testing.T) {
 	}
 }
 
+func TestReverse(t *testing.T) {
+	type args struct {
+		strategy esv1alpha1.PushSecretConversionStrategy
+		in       string
+	}
+	tests := []struct {
+		name string
+		args args
+		want string
+	}{
+		{
+			name: "do not change the key when using the None strategy",
+			args: args{
+				strategy: esv1alpha1.PushSecretConversionNone,
+				in:       keyWithEncodedInvalidChars,
+			},
+			want: keyWithEncodedInvalidChars,
+		},
+		{
+			name: "reverse an unicode encoded key",
+			args: args{
+				strategy: esv1alpha1.PushSecretConversionReverseUnicode,
+				in:       keyWithEncodedInvalidChars,
+			},
+			want: keyWithInvalidChars,
+		},
+		{
+			name: "do not attempt to decode an invalid unicode representation",
+			args: args{
+				strategy: esv1alpha1.PushSecretConversionReverseUnicode,
+				in:       "_U0xxx_x_U005b_",
+			},
+			want: "_U0xxx_x[",
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			if got := reverse(tt.args.strategy, tt.args.in); got != tt.want {
+				t.Errorf("reverse() = %v, want %v", got, tt.want)
+			}
+		})
+	}
+}
+
 func TestFetchValueFromMetadata(t *testing.T) {
 	type args struct {
 		key  string