Some checks failed
check / check (push) Superseded by a newer commit; never tested
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.
469 lines
13 KiB
Go
469 lines
13 KiB
Go
package handlers_test
|
|
|
|
import (
|
|
"bytes"
|
|
"context"
|
|
"crypto/hmac"
|
|
"crypto/sha256"
|
|
"encoding/hex"
|
|
"log/slog"
|
|
"net/http"
|
|
"net/http/httptest"
|
|
"strings"
|
|
"testing"
|
|
|
|
"github.com/go-chi/chi"
|
|
"github.com/stretchr/testify/assert"
|
|
"github.com/stretchr/testify/require"
|
|
"gorm.io/gorm/clause"
|
|
"sneak.berlin/go/webhooker/internal/database"
|
|
"sneak.berlin/go/webhooker/internal/handlers"
|
|
"sneak.berlin/go/webhooker/internal/signature"
|
|
)
|
|
|
|
const (
|
|
// inboundSecret is the shared secret the signed-receiver tests
|
|
// configure on their entrypoint. It doubles as a marker: no log
|
|
// line and no rendered page may contain it.
|
|
inboundSecret = "QQINBOUNDSECRETQQ"
|
|
|
|
// inboundBody is the payload the sender signs.
|
|
inboundBody = `{"zen":"Non-blocking is better than blocking."}`
|
|
|
|
// entrypointIDParam is the chi URL parameter naming an entrypoint.
|
|
entrypointIDParam = "entrypointID"
|
|
)
|
|
|
|
// hubSignature returns the X-Hub-Signature-256 value a GitHub sender
|
|
// holding secret would send for inboundBody.
|
|
func hubSignature(secret string) string {
|
|
mac := hmac.New(sha256.New, []byte(secret))
|
|
_, _ = mac.Write([]byte(inboundBody))
|
|
|
|
return "sha256=" + hex.EncodeToString(mac.Sum(nil))
|
|
}
|
|
|
|
// seedSignedEntrypoint inserts an active entrypoint for a webhook
|
|
// with the given signature configuration and returns it.
|
|
func seedSignedEntrypoint(
|
|
t *testing.T,
|
|
db *database.Database,
|
|
webhookID string,
|
|
scheme database.SignatureScheme,
|
|
secret string,
|
|
) *database.Entrypoint {
|
|
t.Helper()
|
|
|
|
ep := &database.Entrypoint{
|
|
WebhookID: webhookID,
|
|
Path: "path-" + webhookID,
|
|
Description: "signed",
|
|
Active: true,
|
|
SignatureScheme: scheme,
|
|
SignatureSecret: secret,
|
|
}
|
|
|
|
require.NoError(
|
|
t,
|
|
db.DB().Omit(clause.Associations).Create(ep).Error,
|
|
)
|
|
|
|
return ep
|
|
}
|
|
|
|
// postToEntrypoint drives the real receiver handler at an
|
|
// entrypoint's path with one optional header set.
|
|
func postToEntrypoint(
|
|
t *testing.T,
|
|
h *handlers.Handlers,
|
|
path, body, headerName, headerValue string,
|
|
) *httptest.ResponseRecorder {
|
|
t.Helper()
|
|
|
|
req := httptest.NewRequestWithContext(
|
|
context.Background(),
|
|
http.MethodPost,
|
|
"/webhook/"+path,
|
|
strings.NewReader(body),
|
|
)
|
|
req.Header.Set("Content-Type", "application/json")
|
|
|
|
if headerName != "" {
|
|
req.Header.Set(headerName, headerValue)
|
|
}
|
|
|
|
rctx := chi.NewRouteContext()
|
|
rctx.URLParams.Add("uuid", path)
|
|
|
|
req = req.WithContext(
|
|
context.WithValue(req.Context(), chi.RouteCtxKey, rctx),
|
|
)
|
|
|
|
w := httptest.NewRecorder()
|
|
h.HandleWebhook().ServeHTTP(w, req)
|
|
|
|
return w
|
|
}
|
|
|
|
// storedEvents counts the event rows a webhook's per-webhook database
|
|
// holds. A database that was never opened holds none, which is the
|
|
// state a rejected request has to leave behind.
|
|
func storedEvents(
|
|
t *testing.T,
|
|
mgr *database.WebhookDBManager,
|
|
webhookID string,
|
|
) int64 {
|
|
t.Helper()
|
|
|
|
if !mgr.DBExists(webhookID) {
|
|
return 0
|
|
}
|
|
|
|
db, err := mgr.GetDB(webhookID)
|
|
require.NoError(t, err)
|
|
|
|
var count int64
|
|
|
|
require.NoError(
|
|
t,
|
|
db.Model(&database.Event{}).
|
|
Where("webhook_id = ?", webhookID).
|
|
Count(&count).Error,
|
|
)
|
|
|
|
return count
|
|
}
|
|
|
|
// storedEventHeaders reads back the Headers column of the single
|
|
// event row a webhook's per-webhook database holds.
|
|
//
|
|
// It reads the database rather than an in-memory struct on purpose:
|
|
// what matters is what an operator, a backup or the reaper's archive
|
|
// would find on disk, not what the handler passed around.
|
|
func storedEventHeaders(
|
|
t *testing.T,
|
|
mgr *database.WebhookDBManager,
|
|
webhookID string,
|
|
) string {
|
|
t.Helper()
|
|
|
|
require.True(t, mgr.DBExists(webhookID))
|
|
|
|
db, err := mgr.GetDB(webhookID)
|
|
require.NoError(t, err)
|
|
|
|
var events []database.Event
|
|
|
|
require.NoError(
|
|
t,
|
|
db.Where("webhook_id = ?", webhookID).
|
|
Find(&events).Error,
|
|
)
|
|
require.Len(t, events, 1)
|
|
|
|
return events[0].Headers
|
|
}
|
|
|
|
// signedReceiverCase is one inbound request against an entrypoint
|
|
// with a given stored signature configuration.
|
|
type signedReceiverCase struct {
|
|
name string
|
|
scheme database.SignatureScheme
|
|
secret string
|
|
headerName string
|
|
headerValue string
|
|
body string
|
|
wantStatus int
|
|
}
|
|
|
|
// signedReceiverCases covers each supported scheme with a valid
|
|
// signature, an invalid one and none at all, plus the two states that
|
|
// are not "a client got it wrong": an entrypoint with nothing
|
|
// configured, and one whose stored configuration cannot be applied.
|
|
func signedReceiverCases() []signedReceiverCase {
|
|
return append(
|
|
schemeReceiverCases(), unverifiedReceiverCases()...,
|
|
)
|
|
}
|
|
|
|
// schemeReceiverCases covers the two supported schemes.
|
|
func schemeReceiverCases() []signedReceiverCase {
|
|
return []signedReceiverCase{
|
|
{
|
|
name: "github valid",
|
|
scheme: database.SignatureSchemeGitHub,
|
|
secret: inboundSecret,
|
|
headerName: signature.HeaderGitHub,
|
|
headerValue: hubSignature(inboundSecret),
|
|
body: inboundBody,
|
|
wantStatus: http.StatusOK,
|
|
},
|
|
{
|
|
name: "github wrong secret",
|
|
scheme: database.SignatureSchemeGitHub,
|
|
secret: inboundSecret,
|
|
headerName: signature.HeaderGitHub,
|
|
headerValue: hubSignature("wrong"),
|
|
body: inboundBody,
|
|
wantStatus: http.StatusUnauthorized,
|
|
},
|
|
{
|
|
// A digest that was valid for a different body: the
|
|
// check is over the bytes as received.
|
|
name: "github body tampered",
|
|
scheme: database.SignatureSchemeGitHub,
|
|
secret: inboundSecret,
|
|
headerName: signature.HeaderGitHub,
|
|
headerValue: hubSignature(inboundSecret),
|
|
body: inboundBody + " ",
|
|
wantStatus: http.StatusUnauthorized,
|
|
},
|
|
{
|
|
name: "github unsigned",
|
|
scheme: database.SignatureSchemeGitHub,
|
|
secret: inboundSecret,
|
|
body: inboundBody,
|
|
wantStatus: http.StatusUnauthorized,
|
|
},
|
|
{
|
|
name: "gitlab valid",
|
|
scheme: database.SignatureSchemeGitLab,
|
|
secret: inboundSecret,
|
|
headerName: signature.HeaderGitLab,
|
|
headerValue: inboundSecret,
|
|
body: inboundBody,
|
|
wantStatus: http.StatusOK,
|
|
},
|
|
{
|
|
name: "gitlab wrong token",
|
|
scheme: database.SignatureSchemeGitLab,
|
|
secret: inboundSecret,
|
|
headerName: signature.HeaderGitLab,
|
|
headerValue: "wrong",
|
|
body: inboundBody,
|
|
wantStatus: http.StatusUnauthorized,
|
|
},
|
|
{
|
|
name: "gitlab unsigned",
|
|
scheme: database.SignatureSchemeGitLab,
|
|
secret: inboundSecret,
|
|
body: inboundBody,
|
|
wantStatus: http.StatusUnauthorized,
|
|
},
|
|
}
|
|
}
|
|
|
|
// unverifiedReceiverCases covers the two entrypoint states that are
|
|
// not about a client getting its signature wrong: nothing configured
|
|
// at all, and a configuration the receiver cannot apply.
|
|
func unverifiedReceiverCases() []signedReceiverCase {
|
|
return []signedReceiverCase{
|
|
{
|
|
// The pass-through case. An entrypoint with nothing
|
|
// configured is what every deployment already has, and
|
|
// it must keep accepting unsigned requests so that an
|
|
// upgrade does not lock an operator out of their own
|
|
// receivers.
|
|
name: "unconfigured accepts unsigned",
|
|
scheme: database.SignatureSchemeNone,
|
|
body: inboundBody,
|
|
wantStatus: http.StatusOK,
|
|
},
|
|
{
|
|
// A stray signature header changes nothing when nothing
|
|
// is configured to check it.
|
|
name: "unconfigured ignores a stray header",
|
|
scheme: database.SignatureSchemeNone,
|
|
headerName: signature.HeaderGitHub,
|
|
headerValue: "sha256=deadbeef",
|
|
body: inboundBody,
|
|
wantStatus: http.StatusOK,
|
|
},
|
|
{
|
|
// A scheme this build cannot apply, reachable only by
|
|
// editing the database: refused, not waved through as
|
|
// unverified.
|
|
name: "unknown scheme fails closed",
|
|
scheme: database.SignatureScheme("stripe"),
|
|
secret: inboundSecret,
|
|
headerName: signature.HeaderGitHub,
|
|
headerValue: hubSignature(inboundSecret),
|
|
body: inboundBody,
|
|
wantStatus: http.StatusInternalServerError,
|
|
},
|
|
}
|
|
}
|
|
|
|
// TestReceiverVerifiesConfiguredEntrypoints is the load-bearing test
|
|
// for the feature: for each supported scheme a correctly signed
|
|
// request is accepted and stored, and an incorrectly signed or
|
|
// unsigned one is answered 401 having stored nothing.
|
|
//
|
|
// The event count is the half that matters most. A rejection that
|
|
// still wrote a row would leave the receiver a place for a stranger
|
|
// who knows a URL to deposit content, which is exactly what the
|
|
// signature is there to prevent.
|
|
//
|
|
// The cases share one application and take a webhook each, rather
|
|
// than each standing up its own: every newTestApp seeds an admin user
|
|
// and so pays an Argon2id hash at 64 MB, and this package's test
|
|
// budget is not large enough to spend one per table row.
|
|
func TestReceiverVerifiesConfiguredEntrypoints(t *testing.T) {
|
|
t.Parallel()
|
|
|
|
var (
|
|
h *handlers.Handlers
|
|
db *database.Database
|
|
mgr *database.WebhookDBManager
|
|
)
|
|
|
|
app := newTestApp(t, &h, &db, &mgr)
|
|
app.RequireStart()
|
|
|
|
t.Cleanup(app.RequireStop)
|
|
|
|
for _, tc := range signedReceiverCases() {
|
|
wh := seedWebhook(t, db)
|
|
ep := seedSignedEntrypoint(
|
|
t, db, wh.ID, tc.scheme, tc.secret,
|
|
)
|
|
|
|
w := postToEntrypoint(
|
|
t, h, ep.Path, tc.body,
|
|
tc.headerName, tc.headerValue,
|
|
)
|
|
|
|
assert.Equal(t, tc.wantStatus, w.Code, "case %s", tc.name)
|
|
|
|
want := int64(0)
|
|
if tc.wantStatus == http.StatusOK {
|
|
want = 1
|
|
}
|
|
|
|
assert.Equal(
|
|
t, want, storedEvents(t, mgr, wh.ID),
|
|
"case %s: stored event rows after a %d response",
|
|
tc.name, w.Code,
|
|
)
|
|
}
|
|
}
|
|
|
|
// TestReceiverLogsNoSecret proves the rejection path does not write
|
|
// the shared secret, or what the client presented, into the log. A
|
|
// GitLab token arrives as the credential itself, so echoing the
|
|
// header value would put a live secret in the log of every deployment
|
|
// whose sender is briefly misconfigured.
|
|
func TestReceiverLogsNoSecret(t *testing.T) {
|
|
t.Parallel()
|
|
|
|
const presented = "QQPRESENTEDVALUEQQ"
|
|
|
|
var (
|
|
h *handlers.Handlers
|
|
db *database.Database
|
|
)
|
|
|
|
app := newTestApp(t, &h, &db)
|
|
app.RequireStart()
|
|
|
|
t.Cleanup(app.RequireStop)
|
|
|
|
var buf bytes.Buffer
|
|
|
|
h.SetLogForTest(slog.New(slog.NewJSONHandler(&buf, nil)))
|
|
|
|
wh := seedWebhook(t, db)
|
|
ep := seedSignedEntrypoint(
|
|
t, db, wh.ID,
|
|
database.SignatureSchemeGitLab, inboundSecret,
|
|
)
|
|
|
|
w := postToEntrypoint(
|
|
t, h, ep.Path, inboundBody,
|
|
signature.HeaderGitLab, presented,
|
|
)
|
|
require.Equal(t, http.StatusUnauthorized, w.Code)
|
|
|
|
// The rejection is recorded at all — a silent 401 leaves an
|
|
// operator no way to see a sender failing to authenticate.
|
|
assert.Contains(t, buf.String(), "verification failed")
|
|
assert.NotContains(t, buf.String(), inboundSecret)
|
|
assert.NotContains(t, buf.String(), presented)
|
|
}
|
|
|
|
// TestReceiverDoesNotStoreInboundCredential proves an accepted
|
|
// request leaves no copy of the shared secret in the event store.
|
|
//
|
|
// GitLab's X-Gitlab-Token is the credential itself, not a digest
|
|
// over the request. Stored headers are read back by the UI, copied
|
|
// into every backup and archive, and handed verbatim to every
|
|
// delivery target, so a stored token is the entrypoint's only
|
|
// authentication control disclosed to precisely the parties it
|
|
// exists to exclude.
|
|
//
|
|
// The two cases share one application: every newTestApp seeds an
|
|
// admin user and pays an Argon2id hash at 64 MB, and this package's
|
|
// test budget does not stretch to one per case.
|
|
func TestReceiverDoesNotStoreInboundCredential(t *testing.T) {
|
|
t.Parallel()
|
|
|
|
var (
|
|
h *handlers.Handlers
|
|
db *database.Database
|
|
mgr *database.WebhookDBManager
|
|
)
|
|
|
|
app := newTestApp(t, &h, &db, &mgr)
|
|
app.RequireStart()
|
|
|
|
t.Cleanup(app.RequireStop)
|
|
|
|
gitlab := seedWebhook(t, db)
|
|
gitlabEP := seedSignedEntrypoint(
|
|
t, db, gitlab.ID,
|
|
database.SignatureSchemeGitLab, inboundSecret,
|
|
)
|
|
|
|
w := postToEntrypoint(
|
|
t, h, gitlabEP.Path, inboundBody,
|
|
signature.HeaderGitLab, inboundSecret,
|
|
)
|
|
require.Equal(t, http.StatusOK, w.Code)
|
|
|
|
stored := storedEventHeaders(t, mgr, gitlab.ID)
|
|
|
|
assert.NotContains(
|
|
t, stored, inboundSecret,
|
|
"the shared secret must not be persisted",
|
|
)
|
|
assert.NotContains(
|
|
t, stored, signature.HeaderGitLab,
|
|
"the credential header must not be persisted at all",
|
|
)
|
|
|
|
// Everything else the sender set is still there. A fix that
|
|
// stored no headers would satisfy the assertions above while
|
|
// discarding the record the receiver exists to keep.
|
|
assert.Contains(t, stored, "Content-Type")
|
|
|
|
// A GitHub digest is an HMAC over the body, so the key cannot be
|
|
// recovered from it and it stays: the stripping is scoped to
|
|
// what actually carries the secret.
|
|
github := seedWebhook(t, db)
|
|
githubEP := seedSignedEntrypoint(
|
|
t, db, github.ID,
|
|
database.SignatureSchemeGitHub, inboundSecret,
|
|
)
|
|
|
|
w = postToEntrypoint(
|
|
t, h, githubEP.Path, inboundBody,
|
|
signature.HeaderGitHub, hubSignature(inboundSecret),
|
|
)
|
|
require.Equal(t, http.StatusOK, w.Code)
|
|
|
|
stored = storedEventHeaders(t, mgr, github.ID)
|
|
|
|
assert.Contains(t, stored, signature.HeaderGitHub)
|
|
assert.NotContains(t, stored, inboundSecret)
|
|
}
|