From a76b9a0c636bb775f63659738cdd696fd313223b Mon Sep 17 00:00:00 2001 From: Max Jonas Werner Date: Fri, 30 Sep 2022 14:43:33 +0200 Subject: [PATCH] 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. Any existing `X-Signature` header value set through a `Provider.spec.secretRef` Secret's `header` field will be overwritten. closes #99 Signed-off-by: Max Jonas Werner --- api/v1beta1/provider_types.go | 3 +- ...ification.toolkit.fluxcd.io_providers.yaml | 1 + docs/spec/v1beta1/provider.md | 83 ++++++++--- internal/notifier/factory.go | 4 +- internal/notifier/forwarder.go | 28 +++- internal/notifier/forwarder_test.go | 130 ++++++++++++++---- 6 files changed, 205 insertions(+), 44 deletions(-) diff --git a/api/v1beta1/provider_types.go b/api/v1beta1/provider_types.go index a1043202a..e3622fed8 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 425fb3609..f80113523 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 fb4d83978..b36f92cf5 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 6d6b23eaa..91a52d90c 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 203cca070..afed766bc 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 { diff --git a/internal/notifier/forwarder_test.go b/internal/notifier/forwarder_test.go index eb4dd6621..85e0be18f 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 }