Repository: fluxcd/notification-controller (go)

Issue:
Allow signing of generic webhook provider requests
The HTTP endpoint should be able to ensure the provenance of incoming requests from the `generic` provider.

GitHub and GitLab use a shared-secret mechanism, where the outgoing requests have a header with an HMAC of the request body, the receiver can then sign the body it receives, and confirm the origin of the request.

_This should be optional_.

One proposal, is to add an additional signature field to the spec:

```yaml
spec:
 signature:
   secretRef:
       name: hmac
```

Another option would be to use the presence of an `hmac` key in the Secret referenced by `spec.secretRef`.

Some other considerations, sha1 or sha256? What should the header name be?

Pull request description:
Add "generic-hmac" Provider
This commit adds the "generic-hmac" Provider type for authenticating
webhook requests coming from notification-controller. I extended the
`Forwarder` notifier to accept an optional key used for generating the
HMAC. If the key is nil or empty no HMAC header is generated and the
forwarder behaves as before. If it is provided an `X-Signature` HTTP
header is added to the request carrying the HMAC.

I transformed the `TestForwarder_Post` test into a table-driven test
so that we can use the same setup and testing code for testing HMAC
and non-HMAC forwarder instances.

The Flux CLI will support this new type automatically as it doesn't validate the given type by itself (this is done by the API server using the CRD spec.

closes #99


Patch:
diff --git a/api/v1beta1/provider_types.go b/api/v1beta1/provider_types.go
index a104320..e3622fe 100644
--- a/api/v1beta1/provider_types.go
+++ b/api/v1beta1/provider_types.go
@@ -30,7 +30,7 @@ const (
 // ProviderSpec defines the desired state of Provider
 type ProviderSpec struct {
 	// Type of provider
-	// +kubebuilder:validation:Enum=slack;discord;msteams;rocket;generic;github;gitlab;bitbucket;azuredevops;googlechat;webex;sentry;azureeventhub;telegram;lark;matrix;opsgenie;alertmanager;grafana;githubdispatch;
+	// +kubebuilder:validation:Enum=slack;discord;msteams;rocket;generic;generic-hmac;github;gitlab;bitbucket;azuredevops;googlechat;webex;sentry;azureeventhub;telegram;lark;matrix;opsgenie;alertmanager;grafana;githubdispatch;
 	// +required
 	Type string `json:"type"`
 
@@ -78,6 +78,7 @@ type ProviderSpec struct {
 
 const (
 	GenericProvider        string = "generic"
+	GenericHMACProvider    string = "generic-hmac"
 	SlackProvider          string = "slack"
 	GrafanaProvider        string = "grafana"
 	DiscordProvider        string = "discord"
diff --git a/config/crd/bases/notification.toolkit.fluxcd.io_providers.yaml b/config/crd/bases/notification.toolkit.fluxcd.io_providers.yaml
index 425fb36..f801135 100644
--- a/config/crd/bases/notification.toolkit.fluxcd.io_providers.yaml
+++ b/config/crd/bases/notification.toolkit.fluxcd.io_providers.yaml
@@ -92,6 +92,7 @@ spec:
                 - msteams
                 - rocket
                 - generic
+                - generic-hmac
                 - github
                 - gitlab
                 - bitbucket
diff --git a/docs/spec/v1beta1/provider.md b/docs/spec/v1beta1/provider.md
index fb4d839..b36f92c 100644
--- a/docs/spec/v1beta1/provider.md
+++ b/docs/spec/v1beta1/provider.md
@@ -9,7 +9,7 @@ Spec:
 ```go
 type ProviderSpec struct {
 	// Type of provider
-	// +kubebuilder:validation:Enum=slack;discord;msteams;rocket;generic;github;gitlab;bitbucket;azuredevops;googlechat;webex;sentry;azureeventhub;telegram;lark;matrix;opsgenie;githubdispatch
+	// +kubebuilder:validation:Enum=slack;discord;msteams;rocket;generic;generic-hmac;github;gitlab;bitbucket;azuredevops;googlechat;webex;sentry;azureeventhub;telegram;lark;matrix;opsgenie;githubdispatch
 	// +required
 	Type string `json:"type"`
 
@@ -51,22 +51,23 @@ Notification providers:
 
 | Provider        | Type           |
 | --------------- | -------------- |
-| Alertmanager    | alertmanager   |
-| Azure Event Hub | azureeventhub  |
-| Discord         | discord        |
-| Generic webhook | generic        |
-| GitHub dispatch | githubdispatch |
-| Google Chat     | googlechat     |
-| Grafana         | grafana        |
-| Lark            | lark           |
-| Matrix          | matrix         |
-| Microsoft Teams | msteams        |
-| Opsgenie        | opsgenie       |
-| Rocket          | rocket         |
-| Sentry          | sentry         |
-| Slack           | slack          |
-| Telegram        | telegram       |
-| WebEx           | webex          |
+| Alertmanager              | alertmanager   |
+| Azure Event Hub           | azureeventhub  |
+| Discord                   | discord        |
+| Generic webhook           | generic        |
+| Generic webhook with HMAC | generic-hmac   |
+| GitHub dispatch           | githubdispatch |
+| Google Chat               | googlechat     |
+| Grafana                   | grafana        |
+| Lark                      | lark           |
+| Matrix                    | matrix         |
+| Microsoft Teams           | msteams        |
+| Opsgenie                  | opsgenie       |
+| Rocket                    | rocket         |
+| Sentry                    | sentry         |
+| Slack                     | slack          |
+| Telegram                  | telegram       |
+| WebEx                     | webex          |
 
 Git commit status providers:
 
@@ -211,6 +212,54 @@ stringData:
      X-Forwarded-Proto: https
 ```
 
+### Generic webhook with HMAC
+
+If you set the `.spec.type` of a `Provider` resource to `generic-hmac` then the HTTP request sent to the webhook will include the `X-Signature` HTTP header carrying the HMAC of the request body. This allows the webhook server to authenticate the request. The key used for the HMAC must be supplied in the `token` field of the Secret resource referenced in `.spec.secretRef`. The HTTP header value has the following format:
+
+```
+X-Signature: HASH_FUNC=HASH
+```
+
+`HASH_FUNC` denotes the Hash function used to generate the HMAC and currently defaults to `sha256` but may change in the future. You must make sure to take this value into account when verifying the HMAC. `HASH` is the hex-encoded HMAC value. The following Go code illustrates how the header is parsed and verified:
+
+```go
+func verifySignature(sig string, payload, key []byte) error {
+	sigHdr := strings.Split(sig, "=")
+	if len(shgHdr) != 2 {
+		return fmt.Errorf("invalid signature value")
+	}
+	var newF func() hash.Hash
+	switch sigHdr[0] {
+	case "sha224":
+		newF = sha256.New224
+	case "sha256":
+		newF = sha256.New
+	case "sha384":
+		newF = sha512.New384
+	case "sha512":
+		newF = sha512.New
+	default:
+		return fmt.Errorf("unsupported signature algorithm %q", sigHdr[0])
+	}
+	mac := hmac.New(newF, key)
+	if _, err := mac.Write(payload); err != nil {
+		return fmt.Errorf("error MAC'ing payload: %w", err)
+	}
+	sum := fmt.Sprintf("%x", mac.Sum(nil))
+	if sum != sigHdr[1] {
+		return fmt.Errorf("HMACs don't match: %#v != %#v", sum, sigHdr[1])
+	}
+	return nil
+}
+[...]
+key := []byte("b1fad212fb1b87a56c79e5da48018650b85ab7cf")
+if len(r.Header["X-Signature"]) > 0 {
+	if err := verifySignature(r.Header["X-Signature"][0], body, key); err != nil {
+		// handle signature verification failure here
+	}
+}
+```
+
 ### Self-signed certificates
 
 The `certSecretRef` field names a secret with TLS certificate data. This is for the purpose
diff --git a/internal/notifier/factory.go b/internal/notifier/factory.go
index 6d6b23e..91a52d9 100644
--- a/internal/notifier/factory.go
+++ b/internal/notifier/factory.go
@@ -56,7 +56,9 @@ func (f Factory) Notifier(provider string) (Interface, error) {
 	var err error
 	switch provider {
 	case v1beta1.GenericProvider:
-		n, err = NewForwarder(f.URL, f.ProxyURL, f.Headers, f.CertPool)
+		n, err = NewForwarder(f.URL, f.ProxyURL, f.Headers, f.CertPool, nil)
+	case v1beta1.GenericHMACProvider:
+		n, err = NewForwarder(f.URL, f.ProxyURL, f.Headers, f.CertPool, []byte(f.Token))
 	case v1beta1.SlackProvider:
 		n, err = NewSlack(f.URL, f.ProxyURL, f.Token, f.CertPool, f.Username, f.Channel)
 	case v1beta1.DiscordProvider:
diff --git a/internal/notifier/forwarder.go b/internal/notifier/forwarder.go
index 203cca0..afed766 100644
--- a/internal/notifier/forwarder.go
+++ b/internal/notifier/forwarder.go
@@ -18,7 +18,10 @@ package notifier
 
 import (
 	"context"
+	"crypto/hmac"
+	"crypto/sha256"
 	"crypto/x509"
+	"encoding/json"
 	"fmt"
 	"net/url"
 
@@ -38,27 +41,50 @@ type Forwarder struct {
 	ProxyURL string
 	Headers  map[string]string
 	CertPool *x509.CertPool
+	HMACKey  []byte
 }
 
-func NewForwarder(hookURL string, proxyURL string, headers map[string]string, certPool *x509.CertPool) (*Forwarder, error) {
+func NewForwarder(hookURL string, proxyURL string, headers map[string]string, certPool *x509.CertPool, hmacKey []byte) (*Forwarder, error) {
 	if _, err := url.ParseRequestURI(hookURL); err != nil {
 		return nil, fmt.Errorf("invalid hook URL %s: %w", hookURL, err)
 	}
 
+	if hmacKey != nil && len(hmacKey) == 0 {
+		return nil, fmt.Errorf("HMAC key is empty")
+	}
+
 	return &Forwarder{
 		URL:      hookURL,
 		ProxyURL: proxyURL,
 		Headers:  headers,
 		CertPool: certPool,
+		HMACKey:  hmacKey,
 	}, nil
 }
 
+func sign(payload, key []byte) string {
+	h := hmac.New(sha256.New, key)
+	h.Write(payload)
+	return fmt.Sprintf("%x", h.Sum(nil))
+}
+
 func (f *Forwarder) Post(ctx context.Context, event events.Event) error {
+	var sig string
+	if len(f.HMACKey) != 0 {
+		eventJSON, err := json.Marshal(event)
+		if err != nil {
+			return fmt.Errorf("failed marshalling event: %w", err)
+		}
+		sig = fmt.Sprintf("sha256=%s", sign(eventJSON, f.HMACKey))
+	}
 	err := postMessage(ctx, f.URL, f.ProxyURL, f.CertPool, event, func(req *retryablehttp.Request) {
 		req.Header.Set(NotificationHeader, event.ReportingController)
 		for key, val := range f.Headers {
 			req.Header.Set(key, val)
 		}
+		if sig != "" {
+			req.Header.Set("X-Signature", sig)
+		}
 	})
 
 	if err != nil {


Test patch:
diff --git a/internal/notifier/forwarder_test.go b/internal/notifier/forwarder_test.go
index eb4dd66..85e0be1 100644
--- a/internal/notifier/forwarder_test.go
+++ b/internal/notifier/forwarder_test.go
@@ -25,42 +25,124 @@ import (
 	"net/http"
 	"net/http/httptest"
 	"testing"
+	"time"
 
 	fuzz "github.com/AdaLogics/go-fuzz-headers"
 	"github.com/fluxcd/pkg/runtime/events"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 
 	"github.com/stretchr/testify/require"
 )
 
+func TestForwarder_New(t *testing.T) {
+	tests := []struct {
+		name    string
+		hmacKey []byte
+		err     bool
+	}{
+		{
+			name:    "nil HMAC key passes",
+			hmacKey: nil,
+			err:     false,
+		},
+		{
+			name:    "empty HMAC key fails",
+			hmacKey: []byte{},
+			err:     true,
+		},
+		{
+			name:    "happy path with HMAC key from empty string",
+			hmacKey: []byte(""),
+			err:     true,
+		},
+		{
+			name:    "non-empty HMAC key adds signature header",
+			hmacKey: []byte("7152fed34dd6149a7c75a276c510da27cb6f82b0"),
+			err:     false,
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			_, err := NewForwarder("http://example.org", "", nil, nil, tt.hmacKey)
+			if tt.err {
+				require.Error(t, err)
+			} else {
+				require.NoError(t, err)
+			}
+		})
+	}
+}
+
 func TestForwarder_Post(t *testing.T) {
-	ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
-		b, err := io.ReadAll(r.Body)
-		require.NoError(t, err)
-
-		require.Equal(t, "source-controller", r.Header.Get("gotk-component"))
-		require.Equal(t, "token", r.Header.Get("Authorization"))
-		var payload = events.Event{}
-		err = json.Unmarshal(b, &payload)
-		require.NoError(t, err)
-		require.Equal(t, "webapp", payload.InvolvedObject.Name)
-		require.Equal(t, "metadata", payload.Metadata["test"])
-	}))
-	defer ts.Close()
-
-	headers := make(map[string]string)
-	headers["Authorization"] = "token"
-	forwarder, err := NewForwarder(ts.URL, "", headers, nil)
-	require.NoError(t, err)
-
-	err = forwarder.Post(context.TODO(), testEvent())
-	require.NoError(t, err)
+	tests := []struct {
+		name       string
+		hmacKey    []byte
+		hmacHeader string
+		xSigHeader string
+	}{
+		{
+			name: "happy path with nil HMAC key",
+		},
+		{
+			name:       "preset X-Signature header should persist",
+			xSigHeader: "should be preserved",
+		},
+		{
+			name:       "non-empty HMAC key adds signature header",
+			hmacKey:    []byte("7152fed34dd6149a7c75a276c510da27cb6f82b0"),
+			hmacHeader: "sha256=65b018549b1254e7226d1c08f9567ee45bc9de0fc4e7b1a40253f9a018b08be7",
+			xSigHeader: "should be overwritten with actual signature",
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+				b, err := io.ReadAll(r.Body)
+				require.NoError(t, err)
+
+				require.Equal(t, "source-controller", r.Header.Get("gotk-component"))
+				require.Equal(t, "token", r.Header.Get("Authorization"))
+				if tt.hmacHeader == "" {
+					sigHdrVal, ok := r.Header["X-Signature"]
+					if tt.xSigHeader == "" {
+						require.Equal(t, false, ok, "expected signature header to be absent but it was present")
+					} else {
+						require.Equal(t, []string{tt.xSigHeader}, sigHdrVal)
+					}
+				} else {
+					require.Equal(t, tt.hmacHeader, r.Header.Get("X-Signature"))
+				}
+				var payload = events.Event{}
+				err = json.Unmarshal(b, &payload)
+				require.NoError(t, err)
+				require.Equal(t, "webapp", payload.InvolvedObject.Name)
+				require.Equal(t, "metadata", payload.Metadata["test"])
+			}))
+			defer ts.Close()
+
+			headers := make(map[string]string)
+			headers["Authorization"] = "token"
+			if tt.xSigHeader != "" {
+				headers["X-Signature"] = tt.xSigHeader
+			}
+			forwarder, err := NewForwarder(ts.URL, "", headers, nil, tt.hmacKey)
+			require.NoError(t, err)
+
+			ev := testEvent()
+			ev.Timestamp = metav1.NewTime(time.Unix(1664520029, 0))
+			err = forwarder.Post(context.TODO(), ev)
+			require.NoError(t, err)
+		})
+	}
 }
 
 func Fuzz_Forwarder(f *testing.F) {
-	f.Add("", []byte{}, []byte{})
+	f.Add("", []byte{}, []byte{}, []byte{})
 
 	f.Fuzz(func(t *testing.T,
-		urlSuffix string, seed, response []byte) {
+		urlSuffix string, seed, response, hmacKey []byte) {
 		ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
 			w.Write(response)
 			io.Copy(io.Discard, r.Body)
@@ -74,7 +156,7 @@ func Fuzz_Forwarder(f *testing.F) {
 		header := make(map[string]string)
 		_ = fuzz.NewConsumer(seed).FuzzMap(&header)
 
-		forwarder, err := NewForwarder(fmt.Sprintf("%s/%s", ts.URL, urlSuffix), "", header, &cert)
+		forwarder, err := NewForwarder(fmt.Sprintf("%s/%s", ts.URL, urlSuffix), "", header, &cert, hmacKey)
 		if err != nil {
 			return
 		}