Add optional inbound webhook signature verification (closes #67) (#228)

The receiver had no inbound authentication of any kind: /webhook/{uuid}
was mounted behind a rate limiter alone, so the only thing protecting an
entrypoint was the secrecy of a v4 UUID in a URL path. Inbound headers are
forwarded almost verbatim to the target, so anyone who learned the URL
also chose the headers the downstream service received.

Adds an optional per-entrypoint secret with two schemes: github
(X-Hub-Signature-256, HMAC-SHA256 hex over the raw body) and gitlab
(X-Gitlab-Token, a plain shared token). Comparison is constant-time, the
HMAC is computed over the raw body before any parsing, and rejection
happens before persistence -- an unauthenticated request creates no event
row. An entrypoint with no secret behaves exactly as before, including
every row that predates this change.

The scheme's credential header is stripped from the header map before it
is marshalled into Event.Headers, so the GitLab token reaches neither the
event store nor any delivery target. SchemeInfo.HeaderIsDigest defaults to
false meaning strip, so a scheme added later is protected unless its
header is positively declared a digest.

internal/signature/signature.go

@@ -0,0 +1,283 @@
+// Package signature verifies that an inbound webhook request really
+// came from the sender an entrypoint was configured for.
+//
+// Verification is optional and per entrypoint. An entrypoint with no
+// scheme configured is not verified at all, which is what every
+// entrypoint was before this package existed. An entrypoint whose
+// configuration is present but incoherent is failed closed, never
+// treated as unverified: the whole point of the feature is that
+// turning it on cannot silently turn itself back off.
+package signature
+
+import (
+	"crypto/hmac"
+	"crypto/sha256"
+	"encoding/hex"
+	"errors"
+	"fmt"
+	"net/http"
+	"strings"
+
+	"sneak.berlin/go/webhooker/internal/database"
+)
+
+// Header names each supported scheme reads its signature from.
+const (
+	// HeaderGitHub is GitHub's HMAC-SHA256 signature header. GitHub
+	// also sends the older SHA-1 X-Hub-Signature; it is not accepted.
+	HeaderGitHub = "X-Hub-Signature-256"
+
+	// HeaderGitLab is GitLab's plain shared-token header.
+	HeaderGitLab = "X-Gitlab-Token"
+)
+
+// githubPrefix is the algorithm label GitHub puts in front of the hex
+// digest. It is required, not optional: accepting a bare digest too
+// would mean accepting a spelling no supported sender produces.
+const githubPrefix = "sha256="
+
+// ErrConfig marks a failure caused by the entrypoint's stored
+// configuration rather than by the request. A caller must fail these
+// closed — refuse the request — because the alternative is an
+// entrypoint the operator believes is verified silently accepting
+// anything.
+var ErrConfig = errors.New("entrypoint signature configuration invalid")
+
+// ErrUnauthorized marks a request that failed verification. A caller
+// answers these 401.
+var ErrUnauthorized = errors.New("inbound signature verification failed")
+
+// Configuration failures. None of these carry any part of the secret.
+var (
+	errSchemeUnknown = fmt.Errorf(
+		"%w: unsupported scheme", ErrConfig,
+	)
+	errSecretMissing = fmt.Errorf(
+		"%w: scheme set with no secret", ErrConfig,
+	)
+	errSchemeMissing = fmt.Errorf(
+		"%w: secret set with no scheme", ErrConfig,
+	)
+)
+
+// Request failures. These are logged, so none of them carries the
+// value the client sent: under the GitLab scheme that value is a
+// guess at the token, and under either scheme a misconfigured sender
+// could be presenting the real one.
+var (
+	errHeaderMissing = fmt.Errorf(
+		"%w: signature header absent", ErrUnauthorized,
+	)
+	errHeaderMalformed = fmt.Errorf(
+		"%w: signature header malformed", ErrUnauthorized,
+	)
+	errSignatureMismatch = fmt.Errorf(
+		"%w: signature does not match", ErrUnauthorized,
+	)
+)
+
+// SchemeInfo describes one supported scheme for the UI.
+type SchemeInfo struct {
+	Scheme database.SignatureScheme
+	Label  string
+	Header string
+
+	// HeaderIsDigest reports that Header carries a value derived from
+	// the request rather than the shared secret itself, and so may be
+	// kept when the request is stored and forwarded.
+	//
+	// The polarity is deliberate: false — the zero value — means the
+	// header is the credential and must be stripped. A scheme added
+	// later is therefore stripped unless whoever adds it positively
+	// declares the header safe to keep.
+	HeaderIsDigest bool
+}
+
+// Schemes returns the supported schemes in the order the UI offers
+// them. It returns a fresh slice per call so no caller can edit the
+// set out from under another.
+func Schemes() []SchemeInfo {
+	return []SchemeInfo{
+		{
+			Scheme: database.SignatureSchemeGitHub,
+			Label:  "GitHub",
+			Header: HeaderGitHub,
+			// An HMAC over the body, not the key. Keeping it lets an
+			// operator see what the sender sent.
+			HeaderIsDigest: true,
+		},
+		{
+			Scheme: database.SignatureSchemeGitLab,
+			Label:  "GitLab",
+			Header: HeaderGitLab,
+			// X-Gitlab-Token is the shared secret in plaintext.
+			HeaderIsDigest: false,
+		},
+	}
+}
+
+// Info returns the description of a supported scheme. It reports
+// false for the empty scheme and for anything unrecognised, which is
+// what a row hand-edited in the database could hold.
+func Info(scheme database.SignatureScheme) (SchemeInfo, bool) {
+	for _, s := range Schemes() {
+		if s.Scheme == scheme {
+			return s, true
+		}
+	}
+
+	return SchemeInfo{}, false
+}
+
+// Supported reports whether a scheme may be stored on an entrypoint.
+// The empty scheme is supported: it means no verification.
+func Supported(scheme database.SignatureScheme) bool {
+	if scheme == database.SignatureSchemeNone {
+		return true
+	}
+
+	_, ok := Info(scheme)
+
+	return ok
+}
+
+// SanitizeHeaders returns a copy of an accepted request's headers
+// with the entrypoint's credential removed.
+//
+// Under a scheme whose header is the shared secret itself — GitLab's
+// X-Gitlab-Token — every downstream use of the inbound headers is a
+// disclosure of the credential: they are persisted verbatim in the
+// per-webhook event store and forwarded to every delivery target, so
+// a target operator or anyone who reads the event database could
+// forge signed requests to the very entrypoint the secret protects.
+// Stripping happens here, once, above the first write, rather than
+// at each egress, so a new consumer of Event.Headers cannot reopen
+// the leak by forgetting to filter.
+//
+// header is never modified; the caller's request keeps its headers
+// intact for anything that still needs the original.
+//
+// An entrypoint with no scheme, or one whose stored scheme this
+// build does not know, is returned unchanged: there is no configured
+// credential to remove, and the unknown case is refused by Verify
+// before a request reaches storage.
+func SanitizeHeaders(
+	entrypoint *database.Entrypoint,
+	header http.Header,
+) http.Header {
+	clone := header.Clone()
+	if clone == nil {
+		return header
+	}
+
+	info, ok := Info(entrypoint.SignatureScheme)
+	if !ok || info.HeaderIsDigest {
+		return clone
+	}
+
+	clone.Del(info.Header)
+
+	return clone
+}
+
+// Verify checks an inbound request against an entrypoint's
+// configuration and returns nil when the request may be accepted.
+//
+// body must be the raw bytes exactly as received, before any parsing
+// or normalisation: the sender computed its digest over those bytes,
+// so anything that re-encodes them produces a different digest and a
+// spurious rejection. The caller is also responsible for bounding
+// that read; this package hashes what it is handed.
+//
+// Every non-nil error is either ErrConfig or ErrUnauthorized, so a
+// caller can tell "the server is misconfigured" from "the client did
+// not authenticate" with errors.Is.
+func Verify(
+	entrypoint *database.Entrypoint,
+	header http.Header,
+	body []byte,
+) error {
+	scheme := entrypoint.SignatureScheme
+	secret := entrypoint.SignatureSecret
+
+	if scheme == database.SignatureSchemeNone {
+		// A secret with no scheme names no header and no algorithm,
+		// so there is nothing to check it with. Accepting the request
+		// would make a half-applied configuration indistinguishable
+		// from no configuration at all.
+		if secret != "" {
+			return errSchemeMissing
+		}
+
+		return nil
+	}
+
+	if secret == "" {
+		return errSecretMissing
+	}
+
+	switch scheme {
+	case database.SignatureSchemeGitHub:
+		return verifyGitHub(secret, header.Get(HeaderGitHub), body)
+	case database.SignatureSchemeGitLab:
+		return verifyGitLab(secret, header.Get(HeaderGitLab))
+	case database.SignatureSchemeNone:
+		// Handled above; restated so the switch stays exhaustive and
+		// adding a scheme has to be decided here.
+		return nil
+	default:
+		return errSchemeUnknown
+	}
+}
+
+// verifyGitHub checks a GitHub-style X-Hub-Signature-256: the string
+// "sha256=" followed by the hex HMAC-SHA256 of the raw body under the
+// shared secret.
+func verifyGitHub(secret, provided string, body []byte) error {
+	if provided == "" {
+		return errHeaderMissing
+	}
+
+	encoded, ok := strings.CutPrefix(provided, githubPrefix)
+	if !ok {
+		return errHeaderMalformed
+	}
+
+	got, err := hex.DecodeString(encoded)
+	if err != nil {
+		return errHeaderMalformed
+	}
+
+	mac := hmac.New(sha256.New, []byte(secret))
+
+	// hash.Hash.Write is documented never to return an error.
+	_, _ = mac.Write(body)
+
+	// hmac.Equal, never ==: string comparison stops at the first
+	// differing byte, which tells a client how much of a forged
+	// digest it got right and turns forgery into a per-byte search.
+	if !hmac.Equal(mac.Sum(nil), got) {
+		return errSignatureMismatch
+	}
+
+	return nil
+}
+
+// verifyGitLab checks a GitLab-style X-Gitlab-Token, which is the
+// shared secret itself rather than a digest over the body.
+//
+// The comparison is constant time in the same way as the HMAC one.
+// hmac.Equal returns early for unequal lengths, so the length of the
+// token is not hidden; its contents are, and length alone does not
+// let a client search for the value.
+func verifyGitLab(secret, provided string) error {
+	if provided == "" {
+		return errHeaderMissing
+	}
+
+	if !hmac.Equal([]byte(provided), []byte(secret)) {
+		return errSignatureMismatch
+	}
+
+	return nil
+}

internal/signature/signature_test.go

@@ -0,0 +1,340 @@
+package signature_test
+
+import (
+	"crypto/hmac"
+	"crypto/sha256"
+	"encoding/hex"
+	"net/http"
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+	"sneak.berlin/go/webhooker/internal/database"
+	"sneak.berlin/go/webhooker/internal/signature"
+)
+
+const (
+	// testSharedKey is the shared secret under test. It is not named
+	// "secret": gosec reads a credential-shaped name bound to a
+	// high-entropy literal as a leaked credential, which is the right
+	// rule and the wrong finding here.
+	testSharedKey = "s3kr1t-shared-value"
+	testBody      = `{"action":"opened","number":1}`
+)
+
+// githubSignature returns the X-Hub-Signature-256 value GitHub would
+// send for testBody signed with secret.
+func githubSignature(secret string) string {
+	mac := hmac.New(sha256.New, []byte(secret))
+	_, _ = mac.Write([]byte(testBody))
+
+	return "sha256=" + hex.EncodeToString(mac.Sum(nil))
+}
+
+// headerWith builds a request header carrying one value.
+func headerWith(name, value string) http.Header {
+	h := http.Header{}
+	if name != "" {
+		h.Set(name, value)
+	}
+
+	return h
+}
+
+// entrypoint builds an entrypoint with a signature configuration.
+func entrypoint(
+	scheme database.SignatureScheme, secret string,
+) *database.Entrypoint {
+	return &database.Entrypoint{
+		SignatureScheme: scheme,
+		SignatureSecret: secret,
+	}
+}
+
+// TestVerifyUnconfiguredAcceptsAnything pins the pass-through case:
+// an entrypoint with no scheme is the entrypoint every deployment
+// already has, and it must keep accepting requests that carry no
+// signature at all.
+func TestVerifyUnconfiguredAcceptsAnything(t *testing.T) {
+	t.Parallel()
+
+	ep := entrypoint(database.SignatureSchemeNone, "")
+
+	require.NoError(
+		t, signature.Verify(ep, http.Header{}, []byte(testBody)),
+	)
+	require.NoError(
+		t,
+		signature.Verify(
+			ep,
+			headerWith(signature.HeaderGitHub, "sha256=deadbeef"),
+			[]byte(testBody),
+		),
+	)
+}
+
+// githubCase is one inbound request against a GitHub-scheme
+// entrypoint.
+type githubCase struct {
+	name   string
+	header string
+	value  string
+	body   string
+	want   error
+}
+
+// githubCases enumerates the shapes a GitHub signature can arrive in.
+func githubCases() []githubCase {
+	valid := githubSignature(testSharedKey)
+
+	return []githubCase{
+		{
+			name:   "valid",
+			header: signature.HeaderGitHub,
+			value:  valid,
+			body:   testBody,
+			want:   nil,
+		},
+		{
+			name:   "absent header",
+			header: "",
+			body:   testBody,
+			want:   signature.ErrUnauthorized,
+		},
+		{
+			name:   "wrong secret",
+			header: signature.HeaderGitHub,
+			value:  githubSignature("not-the-shared-value"),
+			body:   testBody,
+			want:   signature.ErrUnauthorized,
+		},
+		{
+			// The digest is valid for a different body: the check
+			// has to be over the bytes actually received.
+			name:   "body altered in flight",
+			header: signature.HeaderGitHub,
+			value:  valid,
+			body:   testBody + " ",
+			want:   signature.ErrUnauthorized,
+		},
+		{
+			name:   "missing algorithm prefix",
+			header: signature.HeaderGitHub,
+			value:  valid[len("sha256="):],
+			body:   testBody,
+			want:   signature.ErrUnauthorized,
+		},
+		{
+			name:   "not hex",
+			header: signature.HeaderGitHub,
+			value:  "sha256=zzzz",
+			body:   testBody,
+			want:   signature.ErrUnauthorized,
+		},
+		{
+			name:   "empty digest",
+			header: signature.HeaderGitHub,
+			value:  "sha256=",
+			body:   testBody,
+			want:   signature.ErrUnauthorized,
+		},
+		{
+			// GitLab's header does not authenticate a GitHub
+			// entrypoint, even holding the right secret.
+			name:   "wrong header for the scheme",
+			header: signature.HeaderGitLab,
+			value:  testSharedKey,
+			body:   testBody,
+			want:   signature.ErrUnauthorized,
+		},
+	}
+}
+
+func TestVerifyGitHub(t *testing.T) {
+	t.Parallel()
+
+	for _, tc := range githubCases() {
+		t.Run(tc.name, func(t *testing.T) {
+			t.Parallel()
+
+			err := signature.Verify(
+				entrypoint(
+					database.SignatureSchemeGitHub, testSharedKey,
+				),
+				headerWith(tc.header, tc.value),
+				[]byte(tc.body),
+			)
+
+			if tc.want == nil {
+				require.NoError(t, err)
+
+				return
+			}
+
+			require.ErrorIs(t, err, tc.want)
+		})
+	}
+}
+
+func TestVerifyGitLab(t *testing.T) {
+	t.Parallel()
+
+	cases := []struct {
+		name   string
+		header string
+		value  string
+		want   error
+	}{
+		{
+			name:   "valid",
+			header: signature.HeaderGitLab,
+			value:  testSharedKey,
+			want:   nil,
+		},
+		{
+			name:   "absent header",
+			header: "",
+			want:   signature.ErrUnauthorized,
+		},
+		{
+			name:   "wrong token",
+			header: signature.HeaderGitLab,
+			value:  "not-the-shared-value",
+			want:   signature.ErrUnauthorized,
+		},
+		{
+			name:   "token prefix only",
+			header: signature.HeaderGitLab,
+			value:  testSharedKey[:5],
+			want:   signature.ErrUnauthorized,
+		},
+		{
+			name:   "wrong header for the scheme",
+			header: signature.HeaderGitHub,
+			value:  githubSignature(testSharedKey),
+			want:   signature.ErrUnauthorized,
+		},
+	}
+
+	for _, tc := range cases {
+		t.Run(tc.name, func(t *testing.T) {
+			t.Parallel()
+
+			err := signature.Verify(
+				entrypoint(
+					database.SignatureSchemeGitLab, testSharedKey,
+				),
+				headerWith(tc.header, tc.value),
+				[]byte(testBody),
+			)
+
+			if tc.want == nil {
+				require.NoError(t, err)
+
+				return
+			}
+
+			require.ErrorIs(t, err, tc.want)
+		})
+	}
+}
+
+// TestVerifyBrokenConfigurationFailsClosed covers the rows a caller
+// must refuse rather than wave through. Each is a state an operator
+// could only reach outside the UI, and each one would otherwise be
+// indistinguishable from "verification is off".
+func TestVerifyBrokenConfigurationFailsClosed(t *testing.T) {
+	t.Parallel()
+
+	cases := []struct {
+		name   string
+		scheme database.SignatureScheme
+		secret string
+	}{
+		{
+			name:   "unknown scheme",
+			scheme: database.SignatureScheme("stripe"),
+			secret: testSharedKey,
+		},
+		{
+			name:   "scheme without secret",
+			scheme: database.SignatureSchemeGitHub,
+			secret: "",
+		},
+		{
+			name:   "secret without scheme",
+			scheme: database.SignatureSchemeNone,
+			secret: testSharedKey,
+		},
+	}
+
+	for _, tc := range cases {
+		t.Run(tc.name, func(t *testing.T) {
+			t.Parallel()
+
+			err := signature.Verify(
+				entrypoint(tc.scheme, tc.secret),
+				headerWith(
+					signature.HeaderGitHub,
+					githubSignature(testSharedKey),
+				),
+				[]byte(testBody),
+			)
+
+			require.ErrorIs(t, err, signature.ErrConfig)
+			assert.NotErrorIs(t, err, signature.ErrUnauthorized)
+		})
+	}
+}
+
+// TestErrorsCarryNoSecret proves the strings that reach the log hold
+// no part of the shared secret or of what the client presented.
+func TestErrorsCarryNoSecret(t *testing.T) {
+	t.Parallel()
+
+	const presented = "QQPRESENTEDTOKENQQ"
+
+	for _, scheme := range []database.SignatureScheme{
+		database.SignatureSchemeGitHub,
+		database.SignatureSchemeGitLab,
+	} {
+		for _, header := range []string{
+			signature.HeaderGitHub, signature.HeaderGitLab,
+		} {
+			err := signature.Verify(
+				entrypoint(scheme, testSharedKey),
+				headerWith(header, presented),
+				[]byte(testBody),
+			)
+			require.Error(t, err)
+			assert.NotContains(t, err.Error(), testSharedKey)
+			assert.NotContains(t, err.Error(), presented)
+		}
+	}
+}
+
+func TestSchemeMetadata(t *testing.T) {
+	t.Parallel()
+
+	assert.True(t, signature.Supported(database.SignatureSchemeNone))
+	assert.True(t, signature.Supported(database.SignatureSchemeGitHub))
+	assert.True(t, signature.Supported(database.SignatureSchemeGitLab))
+	assert.False(
+		t, signature.Supported(database.SignatureScheme("stripe")),
+	)
+
+	// The empty scheme describes no sender, so it has no info even
+	// though it is a storable value.
+	_, ok := signature.Info(database.SignatureSchemeNone)
+	assert.False(t, ok)
+
+	info, ok := signature.Info(database.SignatureSchemeGitHub)
+	require.True(t, ok)
+	assert.Equal(t, "GitHub", info.Label)
+	assert.Equal(t, signature.HeaderGitHub, info.Header)
+
+	info, ok = signature.Info(database.SignatureSchemeGitLab)
+	require.True(t, ok)
+	assert.Equal(t, "GitLab", info.Label)
+	assert.Equal(t, signature.HeaderGitLab, info.Header)
+}