fix: improve webhook provider PushSecret handling (#4508)

* fix: improve webhook provider PushSecret handling

Refactor Webhook provider so that the body can be specified as a
template. This allows a secret to be sent to a web provider without
requiring the web provider to accept the secret in whatever form the
secret itself is in; the secret could be provided in a well-formed,
provider-specific JSON blob.

Signed-off-by: Billie Cleek <billie.cleek@lambdal.com>

* maintain backward compatibility

Keep backward compatibility by sending the secret in the body when the
webhook provider's body field is empty.

Signed-off-by: Billie Cleek <billie.cleek@lambdal.com>

* docs: clarify Webhook PushSecret capability

Clarify that the webhook provider makes the secret available on the
remoteRef object so that it can be used in templates.

Signed-off-by: Billie Cleek <billie.cleek@lambdal.com>

* docs: clarify Webhook empty body capability

Clarify how to send an empty body with the Webhook provider pushing a
secret.

Signed-off-by: Billie Cleek <billie.cleek@lambdal.com>

---------

Signed-off-by: Billie Cleek <billie.cleek@lambdal.com>
Co-authored-by: Gergely Brautigam <182850+Skarlso@users.noreply.github.com>

docs/provider/webhook.md

@@ -81,6 +81,7 @@ spec:
   provider:
     webhook:
       url: "http://httpbin.org/push?id={{ .remoteRef.remoteKey }}&secret={{ .remoteRef.secretKey }}"
+      body: '{"secret-field": "{{ index .remoteRef .remoteRef.remoteKey }}"}'
       headers:
         Content-Type: application/json
         Authorization: Basic {{ print .auth.username ":" .auth.password | b64enc }}
@@ -113,8 +114,9 @@ spec:
         remoteRef:
           remoteKey: remotekey
 ```
+If `secretKey` is not provided, the whole secret is provided JSON encoded.
 
-If `secretKey` is not provided, the whole secret is pushed JSON encoded.
+The secret will be added to the `remoteRef` object so that it is retrievable in the templating engine. The secret will be sent in the body when the body field of the provider is empty. In the rare case that the body should be empty, the provider can be configured to use `'{{ "" }}'` for the body value.
 
 #### Limitations
 

pkg/common/webhook/webhook.go

@@ -140,6 +140,33 @@ func (w *Webhook) GetTemplateData(ctx context.Context, ref *esv1beta1.ExternalSe
 	return data, nil
 }
 
+func (w *Webhook) GetTemplatePushData(ctx context.Context, ref esv1beta1.PushSecretData, secrets []Secret, urlEncode bool) (map[string]map[string]string, error) {
+	data := map[string]map[string]string{}
+	if ref != nil {
+		if urlEncode {
+			data["remoteRef"] = map[string]string{
+				"remoteKey": url.QueryEscape(ref.GetRemoteKey()),
+			}
+			if v := ref.GetSecretKey(); v != "" {
+				data["remoteRef"]["secretKey"] = url.QueryEscape(v)
+			}
+		} else {
+			data["remoteRef"] = map[string]string{
+				"remoteKey": ref.GetRemoteKey(),
+			}
+			if v := ref.GetSecretKey(); v != "" {
+				data["remoteRef"]["secretKey"] = v
+			}
+		}
+	}
+
+	if err := w.getTemplatedSecrets(ctx, secrets, data); err != nil {
+		return nil, err
+	}
+
+	return data, nil
+}
+
 func (w *Webhook) getTemplatedSecrets(ctx context.Context, secrets []Secret, data map[string]map[string]string) error {
 	for _, secref := range secrets {
 		if _, ok := data[secref.Name]; !ok {
@@ -197,27 +224,33 @@ func (w *Webhook) PushWebhookData(ctx context.Context, provider *Spec, data []by
 		method = http.MethodPost
 	}
 
-	rawData := map[string]map[string]string{
-		"remoteRef": {
-			"remoteKey": url.QueryEscape(remoteKey.GetRemoteKey()),
-		},
+	escapedData, err := w.GetTemplatePushData(ctx, remoteKey, provider.Secrets, true)
+	if err != nil {
+		return err
 	}
-	if remoteKey.GetSecretKey() != "" {
-		rawData["remoteRef"]["secretKey"] = url.QueryEscape(remoteKey.GetSecretKey())
+	escapedData["remoteRef"][remoteKey.GetRemoteKey()] = url.QueryEscape(string(data))
+
+	rawData, err := w.GetTemplatePushData(ctx, remoteKey, provider.Secrets, false)
+	if err != nil {
+		return err
 	}
+	rawData["remoteRef"][remoteKey.GetRemoteKey()] = string(data)
 
-	turl, err := ExecuteTemplateString(provider.URL, rawData)
+	url, err := ExecuteTemplateString(provider.URL, escapedData)
 	if err != nil {
 		return fmt.Errorf("failed to parse url: %w", err)
 	}
 
-	dataMap := make(map[string]map[string]string)
-	if err := w.getTemplatedSecrets(ctx, provider.Secrets, dataMap); err != nil {
-		return err
+	bodyt := provider.Body
+	if bodyt == "" {
+		bodyt = fmt.Sprintf("{{ .remoteRef.%s }}", remoteKey.GetRemoteKey())
+	}
+	body, err := ExecuteTemplate(bodyt, rawData)
+	if err != nil {
+		return fmt.Errorf("failed to parse body: %w", err)
 	}
 
-	// read the body into the void to prevent remaining garbage to be present
-	if _, err := w.executeRequest(ctx, provider, data, turl, method, dataMap); err != nil {
+	if _, err := w.executeRequest(ctx, provider, body.Bytes(), url, method, rawData); err != nil {
 		return fmt.Errorf("failed to push webhook data: %w", err)
 	}
 

pkg/provider/webhook/webhook_test.go

@@ -61,7 +61,7 @@ type args struct {
 
 type want struct {
 	Path      string            `json:"path,omitempty"`
-	Body      string            `json:"body,omitempty"`
+	Body      *string           `json:"body,omitempty"`
 	Err       string            `json:"err,omitempty"`
 	Result    string            `json:"result,omitempty"`
 	ResultMap map[string]string `json:"resultmap,omitempty"`
@@ -364,7 +364,22 @@ func TestWebhookGetSecret(t *testing.T) {
 }
 
 var testCasesPushSecret = `
-case: good json
+---
+case: secret key not found
+args:
+  url: /api/pushsecret?id={{ .remoteRef.remoteKey }}&secret={{ .remoteRef.secretKey }}
+  key: testkey
+  secretkey: not-found
+  pushsecret: true
+  secret:
+    name: test-secret
+    data:
+      secretkey: value
+want:
+  path: /api/pushsecret?id=testkey&secret=not-found
+  err: 'failed to find secret key in secret with key: not-found'
+---
+case: default body good json
 args:
   url: /api/pushsecret?id={{ .remoteRef.remoteKey }}&secret={{ .remoteRef.secretKey }}
   key: testkey
@@ -376,26 +391,59 @@ args:
       secretkey: value
 want:
   path: /api/pushsecret?id=testkey&secret=secretkey
+  body: 'value'
   err: ''
 ---
-case: secret key not found
+case: good json
 args:
   url: /api/pushsecret?id={{ .remoteRef.remoteKey }}&secret={{ .remoteRef.secretKey }}
   key: testkey
-  secretkey: not-found
+  body: 'pre {{ .remoteRef.remoteKey }} {{ .remoteRef.testkey }} post'
+  secretkey: secretkey
   pushsecret: true
   secret:
     name: test-secret
     data:
       secretkey: value
 want:
-  path: /api/pushsecret?id=testkey&secret=not-found
-  err: 'failed to find secret key in secret with key: not-found'
+  path: /api/pushsecret?id=testkey&secret=secretkey
+  body: 'pre testkey value post'
+  err: ''
+---
+case: empty body
+args:
+  url: /api/pushsecret?id={{ .remoteRef.remoteKey }}
+  key: testkey
+  body: '{{ "" }}'
+  pushsecret: true
+  secret:
+    name: test-secret
+    data:
+      secretkey: value
+want:
+  path: /api/pushsecret?id=testkey
+  body: ''
+  err: ''
+---
+case: default body pushing without secret key
+args:
+  url: /api/pushsecret?id={{ .remoteRef.remoteKey }}
+  key: testkey
+  pushsecret: true
+  secret:
+    name: test-secret
+    data:
+      secretkey: value
+want:
+  path: /api/pushsecret?id=testkey
+  body: '{"secretkey":"value"}'
+  err: ''
 ---
 case: pushing without secret key
 args:
   url: /api/pushsecret?id={{ .remoteRef.remoteKey }}
   key: testkey
+  body: 'pre {{ .remoteRef.remoteKey }} {{ index (.remoteRef.testkey | fromJson) "secretkey" }} post'
   pushsecret: true
   secret:
     name: test-secret
@@ -403,8 +451,23 @@ args:
       secretkey: value
 want:
   path: /api/pushsecret?id=testkey
+  body: 'pre testkey value post'
   err: ''
 ---
+case: pushing without secret key with dynamic resolution
+args:
+  url: /api/pushsecret?id={{ .remoteRef.remoteKey }}
+  key: testkey
+  body: 'pre {{ .remoteRef.remoteKey }} {{ index (index .remoteRef .remoteRef.remoteKey | fromJson) "secretkey" }} post'
+  pushsecret: true
+  secret:
+    name: test-secret
+    data:
+      secretkey: value
+want:
+  path: /api/pushsecret?id=testkey
+  body: 'pre testkey value post'
+  err: ''
 `
 
 func TestWebhookPushSecret(t *testing.T) {
@@ -427,10 +490,10 @@ func testCaseServer(tc testCase, t *testing.T) *httptest.Server {
 		if tc.Want.Path != "" && req.URL.String() != tc.Want.Path {
 			t.Errorf("%s: unexpected api path: %s, expected %s", tc.Case, req.URL.String(), tc.Want.Path)
 		}
-		if tc.Want.Body != "" {
+		if tc.Want.Body != nil {
 			b, _ := io.ReadAll(req.Body)
-			if string(b) != tc.Want.Body {
-				t.Errorf("%s: unexpected body: %s, expected %s", tc.Case, string(b), tc.Want.Body)
+			if tc.Want.Body != nil && string(b) != *tc.Want.Body {
+				t.Errorf("%s: unexpected body: %s, expected %s", tc.Case, string(b), *tc.Want.Body)
 			}
 		}
 		if tc.Args.StatusCode != 0 {