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

Issue:
Unmarshalling issues with some JSON responses in webhook provider
**Describe the bug**
ESO webhook provider "crashes" on strings with escaped forward slahes.

**To Reproduce**
I'm using ESO v0.9.4 with the webhook provider. I use it to extract an authentication token from VENAFI TPP.
I use a `ClusterSecretStore` like this one:
```
apiVersion: external-secrets.io/v1beta1
kind: ClusterSecretStore
metadata:
  name: venafi-webhook-backend
spec:
  provider:
    webhook:
      body: '{"username":"{{ .auth.username }}","password":"{{ .auth.password }}","client_id":
        "My_Client_ID","scope":"configuration:manage,delete;certificate:manage"}'
      headers:
        Accept: application/json
        Content-Type: application/json
      method: POST
      result:
        jsonPath: $.access_token
      secrets:
      - name: auth
        secretRef:
          name: venafi-webhook-credentials
          namespace: co
      timeout: 1m
      url: https://cert-manager.adidas.com/vedauth/authorize/oauth
```

I detected VENAFI was issuing more authentication tokens than expected, so digging into the logs I saw:
```
{"level":"error","ts":1700746900.245956,"logger":"controllers.ExternalSecret","msg":"could not get secret data from provider","ExternalSecret":{"name":"venafi-webhook","namespace":"default"},"error":"failed to parse response json: yaml: found unknown escape character","stacktrace":"github.com/external-secrets/external-secrets/pkg/controllers/externalsecret.(*Reconciler).Reconcile\n\t/home/runner/work/external-secrets/external-secrets/pkg/controllers/externalsecret/externalsecret_controller.go:205\nsigs.k8s.io/controller-runtime/pkg/internal/controller.(*Controller).Reconcile\n\t/home/runner/go/pkg/mod/sigs.k8s.io/controller-runtime@v0.15.1/pkg/internal/controller/controller.go:118\nsigs.k8s.io/controller-runtime/pkg/internal/controller.(*Controller).reconcileHandler\n\t/home/runner/go/pkg/mod/sigs.k8s.io/controller-runtime@v0.15.1/pkg/internal/controller/controller.go:314\nsigs.k8s.io/controller-runtime/pkg/internal/controller.(*Controller).processNextWorkItem\n\t/home/runner/go/pkg/mod/sigs.k8s.io/controller-runtime@v0.15.1/pkg/internal/controller/controller.go:265\nsigs.k8s.io/controller-runtime/pkg/internal/controller.(*Controller).Start.func2.2\n\t/home/runner/go/pkg/mod/sigs.k8s.io/controller-runtime@v0.15.1/pkg/internal/controller/controller.go:226"}
```

The issue I identified is that access or refresh tokens can arrive with an escaped forward slash, i.e.:
```
{"access_token":"REDACTED","refresh_token":"RE\/DACTED==","expires_in":86399,"expires":1700833565,"token_type":"Bearer","scope":"certificate:manage;configuration:delete,manage","identity":"AD+mydomain.com:a748392498bcb42323427ac392ff","refresh_until":1700833565}%
```

In such cases, the Unmarshall method fails and I see the aforementioned error: `failed to parse response json: yaml: found unknown escape character`. Because of this error, ESO assumes the request has failed and it enqueues it, leading to that increase of tokens issued. The valid token is the one without the backslash.

I've tried to do a simple `strings.ReplaceAll(string(result), '\/', '/')`  on the code after `getWebhookData` and works correctly, but I'm not sure if that's the best approach (considering that probably it's just VENAFI the only one that fails this way).

What could be the best approach to sanitize the strings? unquote and replaceall? remove all single blackslash? a regexp to which replaces `\/` by `/` if `[^\\]\\/` ?

(Ideally fixing VENAFI, but that also seems difficult)

WDYT?

**Expected behavior**
I expect to fetch correctly the access_token from that JSON.


Pull request:
fix: `webhook` support more types when parsing response
## Problem Statement
This PR fixes a couple of bugs in the webhook provider:
1. webhook provider tries to coerce the type to (string) and panics
2. #2898: properly handle escaped json
3. only a small set of response payload types were supported. This PR adds support for a wider variety
4. :broom: centralize the `getTypedKey` as it was copy-pasted across providers

## Related Issue

Fixes #2898


Patch:
diff --git a/pkg/generator/vault/vault.go b/pkg/generator/vault/vault.go
index dbd27969e..8ec204010 100644
--- a/pkg/generator/vault/vault.go
+++ b/pkg/generator/vault/vault.go
@@ -29,6 +29,7 @@ import (
 
 	genv1alpha1 "github.com/external-secrets/external-secrets/apis/generators/v1alpha1"
 	provider "github.com/external-secrets/external-secrets/pkg/provider/vault"
+	"github.com/external-secrets/external-secrets/pkg/utils"
 )
 
 type Generator struct{}
@@ -114,7 +115,7 @@ func (g *Generator) generate(ctx context.Context, c *provider.Connector, jsonSpe
 	}
 
 	for k := range data {
-		response[k], err = provider.GetTypedKey(data, k)
+		response[k], err = utils.GetByteValueFromMap(data, k)
 		if err != nil {
 			return nil, err
 		}
diff --git a/pkg/provider/delinea/client.go b/pkg/provider/delinea/client.go
index d0ed6a465..492faaca7 100644
--- a/pkg/provider/delinea/client.go
+++ b/pkg/provider/delinea/client.go
@@ -17,22 +17,13 @@ import (
 	"context"
 	"encoding/json"
 	"errors"
-	"fmt"
-	"reflect"
-	"strconv"
-	"strings"
 
 	"github.com/DelineaXPM/dsv-sdk-go/v2/vault"
 	"github.com/tidwall/gjson"
 	corev1 "k8s.io/api/core/v1"
 
 	esv1beta1 "github.com/external-secrets/external-secrets/apis/externalsecrets/v1beta1"
-)
-
-const (
-	errSecretKeyFmt  = "cannot find secret data for key: %q"
-	errUnexpectedKey = "unexpected key in data: %s"
-	errSecretFormat  = "secret data for property %s not in expected format: %s"
+	"github.com/external-secrets/external-secrets/pkg/utils"
 )
 
 type client struct {
@@ -91,7 +82,7 @@ func (c *client) GetSecretMap(ctx context.Context, ref esv1beta1.ExternalSecretD
 	}
 	byteMap := make(map[string][]byte, len(secret.Data))
 	for k := range secret.Data {
-		byteMap[k], err = getTypedKey(secret.Data, k)
+		byteMap[k], err = utils.GetByteValueFromMap(secret.Data, k)
 		if err != nil {
 			return nil, err
 		}
@@ -116,34 +107,3 @@ func (c *client) getSecret(_ context.Context, ref esv1beta1.ExternalSecretDataRe
 	}
 	return c.api.Secret(ref.Key)
 }
-
-// getTypedKey is copied from pkg/provider/vault/vault.go.
-func getTypedKey(data map[string]interface{}, key string) ([]byte, error) {
-	v, ok := data[key]
-	if !ok {
-		return nil, fmt.Errorf(errUnexpectedKey, key)
-	}
-	switch t := v.(type) {
-	case string:
-		return []byte(t), nil
-	case map[string]interface{}:
-		return json.Marshal(t)
-	case []string:
-		return []byte(strings.Join(t, "\n")), nil
-	case []byte:
-		return t, nil
-	// also covers int and float32 due to json.Marshal
-	case float64:
-		return []byte(strconv.FormatFloat(t, 'f', -1, 64)), nil
-	case json.Number:
-		return []byte(t.String()), nil
-	case []interface{}:
-		return json.Marshal(t)
-	case bool:
-		return []byte(strconv.FormatBool(t)), nil
-	case nil:
-		return []byte(nil), nil
-	default:
-		return nil, fmt.Errorf(errSecretFormat, key, reflect.TypeOf(t))
-	}
-}
diff --git a/pkg/provider/ibm/provider.go b/pkg/provider/ibm/provider.go
index edfc0fb57..da3c5615e 100644
--- a/pkg/provider/ibm/provider.go
+++ b/pkg/provider/ibm/provider.go
@@ -18,7 +18,6 @@ import (
 	"encoding/json"
 	"fmt"
 	"os"
-	"strconv"
 	"strings"
 	"time"
 
@@ -565,7 +564,7 @@ func (ibm *providerIBM) GetSecretMap(_ context.Context, ref esv1beta1.ExternalSe
 func byteArrayMap(secretData map[string]interface{}, secretMap map[string][]byte) map[string][]byte {
 	var err error
 	for k, v := range secretData {
-		secretMap[k], err = getTypedKey(v)
+		secretMap[k], err = utils.GetByteValue(v)
 		if err != nil {
 			return nil
 		}
@@ -573,29 +572,6 @@ func byteArrayMap(secretData map[string]interface{}, secretMap map[string][]byte
 	return secretMap
 }
 
-// kudos Vault Provider - convert from various types.
-func getTypedKey(v interface{}) ([]byte, error) {
-	switch t := v.(type) {
-	case string:
-		return []byte(t), nil
-	case map[string]interface{}:
-		return json.Marshal(t)
-	case map[string]string:
-		return json.Marshal(t)
-	case []byte:
-		return t, nil
-		// also covers int and float32 due to json.Marshal
-	case float64:
-		return []byte(strconv.FormatFloat(t, 'f', -1, 64)), nil
-	case bool:
-		return []byte(strconv.FormatBool(t)), nil
-	case nil:
-		return []byte(nil), nil
-	default:
-		return nil, fmt.Errorf("secret not in expected format")
-	}
-}
-
 func (ibm *providerIBM) Close(_ context.Context) error {
 	return nil
 }
diff --git a/pkg/provider/vault/vault.go b/pkg/provider/vault/vault.go
index 290cdccba..572c32e4c 100644
--- a/pkg/provider/vault/vault.go
+++ b/pkg/provider/vault/vault.go
@@ -24,8 +24,6 @@ import (
 	"fmt"
 	"net/http"
 	"os"
-	"reflect"
-	"strconv"
 	"strings"
 	"time"
 
@@ -89,7 +87,6 @@ const (
 	errDataField                    = "failed to find data field"
 	errJSONUnmarshall               = "failed to unmarshall JSON"
 	errPathInvalid                  = "provided Path isn't a valid kv v2 path"
-	errSecretFormat                 = "secret data for property %s not in expected format: %s"
 	errUnexpectedKey                = "unexpected key in data: %s"
 	errVaultToken                   = "cannot parse Vault authentication token: %w"
 	errVaultRequest                 = "error from Vault request: %w"
@@ -758,7 +755,7 @@ func (v *client) GetSecret(ctx context.Context, ref esv1beta1.ExternalSecretData
 	// actual keys to take precedence over gjson syntax
 	// (2): extract key from secret with property
 	if _, ok := data[ref.Property]; ok {
-		return GetTypedKey(data, ref.Property)
+		return utils.GetByteValueFromMap(data, ref.Property)
 	}
 
 	// (3): extract key from secret using gjson
@@ -785,7 +782,7 @@ func (v *client) GetSecretMap(ctx context.Context, ref esv1beta1.ExternalSecretD
 	}
 	byteMap := make(map[string][]byte, len(secretData))
 	for k := range secretData {
-		byteMap[k], err = GetTypedKey(secretData, k)
+		byteMap[k], err = utils.GetByteValueFromMap(secretData, k)
 		if err != nil {
 			return nil, err
 		}
@@ -794,36 +791,6 @@ func (v *client) GetSecretMap(ctx context.Context, ref esv1beta1.ExternalSecretD
 	return byteMap, nil
 }
 
-func GetTypedKey(data map[string]interface{}, key string) ([]byte, error) {
-	v, ok := data[key]
-	if !ok {
-		return nil, fmt.Errorf(errUnexpectedKey, key)
-	}
-	switch t := v.(type) {
-	case string:
-		return []byte(t), nil
-	case map[string]interface{}:
-		return json.Marshal(t)
-	case []string:
-		return []byte(strings.Join(t, "\n")), nil
-	case []byte:
-		return t, nil
-	// also covers int and float32 due to json.Marshal
-	case float64:
-		return []byte(strconv.FormatFloat(t, 'f', -1, 64)), nil
-	case json.Number:
-		return []byte(t.String()), nil
-	case []interface{}:
-		return json.Marshal(t)
-	case bool:
-		return []byte(strconv.FormatBool(t)), nil
-	case nil:
-		return []byte(nil), nil
-	default:
-		return nil, fmt.Errorf(errSecretFormat, key, reflect.TypeOf(t))
-	}
-}
-
 func (v *client) Close(ctx context.Context) error {
 	// Revoke the token if we have one set, it wasn't sourced from a TokenSecretRef,
 	// and token caching isn't enabled
diff --git a/pkg/provider/webhook/webhook.go b/pkg/provider/webhook/webhook.go
index 2fa9d973a..0d3ad605f 100644
--- a/pkg/provider/webhook/webhook.go
+++ b/pkg/provider/webhook/webhook.go
@@ -19,16 +19,17 @@ import (
 	"context"
 	"crypto/tls"
 	"crypto/x509"
+	"encoding/json"
 	"fmt"
 	"io"
 	"net/http"
 	"net/url"
+	"strconv"
 	"strings"
 	tpl "text/template"
 	"time"
 
 	"github.com/PaesslerAG/jsonpath"
-	"gopkg.in/yaml.v3"
 	corev1 "k8s.io/api/core/v1"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 
@@ -152,30 +153,54 @@ func (w *WebHook) GetSecret(ctx context.Context, ref esv1beta1.ExternalSecretDat
 	}
 	if resultJSONPath != "" {
 		jsondata := interface{}(nil)
-		if err := yaml.Unmarshal(result, &jsondata); err != nil {
+		if err := json.Unmarshal(result, &jsondata); err != nil {
 			return nil, fmt.Errorf("failed to parse response json: %w", err)
 		}
 		jsondata, err = jsonpath.Get(resultJSONPath, jsondata)
 		if err != nil {
 			return nil, fmt.Errorf("failed to get response path %s: %w", resultJSONPath, err)
 		}
-		jsonvalue, ok := jsondata.(string)
-		if !ok {
-			jsonvalues, ok := jsondata.([]interface{})
-			if !ok {
-				return nil, fmt.Errorf("failed to get response (wrong type: %T)", jsondata)
-			}
-			if len(jsonvalues) == 0 {
-				return nil, fmt.Errorf("filter worked but didn't get any result")
-			}
-			jsonvalue = jsonvalues[0].(string)
-		}
-		return []byte(jsonvalue), nil
+		return extractSecretData(jsondata)
 	}
 
 	return result, nil
 }
 
+// tries to extract data from an interface{}
+// it is supposed to return a single value.
+func extractSecretData(jsondata any) ([]byte, error) {
+	switch val := jsondata.(type) {
+	case bool:
+		return []byte(strconv.FormatBool(val)), nil
+	case nil:
+		return []byte{}, nil
+	case int:
+		return []byte(strconv.Itoa(val)), nil
+	case float64:
+		return []byte(strconv.FormatFloat(val, 'f', 0, 64)), nil
+	case []byte:
+		return val, nil
+	case string:
+		return []byte(val), nil
+
+	// due to backwards compatibility we must keep this!
+	// in case we see a []something we pick the first element and return it
+	case []any:
+		if len(val) == 0 {
+			return nil, fmt.Errorf("filter worked but didn't get any result")
+		}
+		return extractSecretData(val[0])
+
+	// in case we encounter a map we serialize it instead of erroring out
+	// The user should use that data from within a template and figure
+	// out how to deal with it.
+	case map[string]any:
+		return json.Marshal(val)
+	default:
+		return nil, fmt.Errorf("failed to get response (wrong type: %T)", jsondata)
+	}
+}
+
 func (w *WebHook) GetSecretMap(ctx context.Context, ref esv1beta1.ExternalSecretDataRemoteRef) (map[string][]byte, error) {
 	provider, err := getProvider(w.store)
 	if err != nil {
@@ -188,7 +213,7 @@ func (w *WebHook) GetSecretMap(ctx context.Context, ref esv1beta1.ExternalSecret
 
 	// We always want json here, so just parse it out
 	jsondata := interface{}(nil)
-	if err := yaml.Unmarshal(result, &jsondata); err != nil {
+	if err := json.Unmarshal(result, &jsondata); err != nil {
 		return nil, fmt.Errorf("failed to parse response json: %w", err)
 	}
 	// Get subdata via jsonpath, if given
@@ -203,7 +228,7 @@ func (w *WebHook) GetSecretMap(ctx context.Context, ref esv1beta1.ExternalSecret
 	if ok {
 		// This could also happen if the response was a single json-encoded string
 		// but that is an extremely unlikely scenario
-		if err := yaml.Unmarshal([]byte(jsonstring), &jsondata); err != nil {
+		if err := json.Unmarshal([]byte(jsonstring), &jsondata); err != nil {
 			return nil, fmt.Errorf("failed to parse response json from jsonpath: %w", err)
 		}
 	}
diff --git a/pkg/utils/utils.go b/pkg/utils/utils.go
index a127e72ef..930149000 100644
--- a/pkg/utils/utils.go
+++ b/pkg/utils/utils.go
@@ -25,6 +25,7 @@ import (
 	"net/url"
 	"reflect"
 	"regexp"
+	"strconv"
 	"strings"
 	tpl "text/template"
 	"time"
@@ -237,6 +238,44 @@ func MergeStringMap(dest, src map[string]string) {
 	}
 }
 
+var (
+	ErrUnexpectedKey = errors.New("unexpected key in data")
+	ErrSecretType    = errors.New("can not handle secret value with type")
+)
+
+func GetByteValueFromMap(data map[string]interface{}, key string) ([]byte, error) {
+	v, ok := data[key]
+	if !ok {
+		return nil, fmt.Errorf("%w: %s", ErrUnexpectedKey, key)
+	}
+	return GetByteValue(v)
+}
+func GetByteValue(v interface{}) ([]byte, error) {
+	switch t := v.(type) {
+	case string:
+		return []byte(t), nil
+	case map[string]interface{}:
+		return json.Marshal(t)
+	case []string:
+		return []byte(strings.Join(t, "\n")), nil
+	case []byte:
+		return t, nil
+	// also covers int and float32 due to json.Marshal
+	case float64:
+		return []byte(strconv.FormatFloat(t, 'f', -1, 64)), nil
+	case json.Number:
+		return []byte(t.String()), nil
+	case []interface{}:
+		return json.Marshal(t)
+	case bool:
+		return []byte(strconv.FormatBool(t)), nil
+	case nil:
+		return []byte(nil), nil
+	default:
+		return nil, fmt.Errorf("%w: %T", ErrSecretType, t)
+	}
+}
+
 // IsNil checks if an Interface is nil.
 func IsNil(i interface{}) bool {
 	if i == nil {


Test patch:
diff --git a/pkg/provider/webhook/webhook_test.go b/pkg/provider/webhook/webhook_test.go
index 2edc13fba..75ede40eb 100644
--- a/pkg/provider/webhook/webhook_test.go
+++ b/pkg/provider/webhook/webhook_test.go
@@ -119,7 +119,7 @@ want:
   path: /api/getsecret?id=testkey&version=1
   err: failed to get response path
 ---
-case: error bad json data
+case: pull data out of map
 args:
   url: /api/getsecret?id={{ .remoteRef.key }}&version={{ .remoteRef.version }}
   key: testkey
@@ -128,7 +128,8 @@ args:
   response: '{"result":{"thesecret":{"one":"secret-value"}}}'
 want:
   path: /api/getsecret?id=testkey&version=1
-  err: failed to get response (wrong type
+  err: ''
+  result: '{"one":"secret-value"}'
 ---
 case: error timeout
 args:
@@ -199,10 +200,8 @@ args:
   response: 'some simple string'
 want:
   path: /api/getsecret?id=testkey&version=1
-  err: failed to get response (wrong type
-  resultmap:
-    thesecret: secret-value
-    alsosecret: another-value
+  err: "failed to parse response json: invalid character"
+  resultmap: {}
 ---
 case: error json map
 args:
@@ -213,10 +212,8 @@ args:
   response: '{"result":{"thesecret":"secret-value","alsosecret":"another-value"}}'
 want:
   path: /api/getsecret?id=testkey&version=1
-  err: failed to get response (wrong type
-  resultmap:
-    thesecret: secret-value
-    alsosecret: another-value
+  err: "failed to parse response json from jsonpath"
+  resultmap: {}
 ---
 case: good json with good templated jsonpath
 args:
@@ -265,6 +262,42 @@ args:
 want:
   path: /api/getsecret?id=testkey&version=1
   err: "filter worked but didn't get any result"
+---
+case: success with jsonpath filter and result array
+args:
+  url: /api/getsecret?id={{ .remoteRef.key }}&version={{ .remoteRef.version }}
+  key: testkey
+  version: 1
+  jsonpath: $..name
+  response: '{"secrets": [{"name": "thesecret", "value": "secret-value"}, {"name": "alsosecret", "value": "another-value"}]}'
+want:
+  path: /api/getsecret?id=testkey&version=1
+  err: ''
+  result: 'thesecret'
+---
+case: success with jsonpath filter and result array of ints
+args:
+  url: /api/getsecret?id={{ .remoteRef.key }}&version={{ .remoteRef.version }}
+  key: testkey
+  version: 1
+  jsonpath: $..name
+  response: '{"secrets": [{"name": 123, "value": "secret-value"}, {"name": 456, "value": "another-value"}]}'
+want:
+  path: /api/getsecret?id=testkey&version=1
+  err: ''
+  result: 123
+---
+case: support backslash
+args:
+  url: /api/getsecret?id={{ .remoteRef.key }}&version={{ .remoteRef.version }}
+  key: testkey
+  version: 1
+  jsonpath: $.refresh_token
+  response: '{"access_token":"REDACTED","refresh_token":"RE\/DACTED=="}'
+want:
+  path: /api/getsecret?id=testkey&version=1
+  err: ''
+  result: "RE/DACTED=="
 `
 
 func TestWebhookGetSecret(t *testing.T) {