Remove inbound request signature verification (closes #279)
All checks were successful
check / check (push) Successful in 3m10s
All checks were successful
check / check (push) Successful in 3m10s
The receiver verified an optional per-entrypoint HMAC or shared token
before accepting a request. That is removed outright: the entrypoint
UUID in the URL is the authentication secret, and possession of it
authorises submission. This reverses the feature added in fcead5d.
Deletes the internal/signature package, the signature_scheme and
signature_secret columns from Entrypoint along with their accessors,
the per-entrypoint secret form and its POST route, and the scheme
labelling in EntrypointView. Pre-1.0 with no installed base, so the
columns simply stop being written; there is no migration and no
compatibility path.
Header sanitisation goes with it. SanitizeHeaders existed to strip a
scheme's own credential header before the header map was stored and
forwarded; with no configured credential there is nothing to strip, so
the receiver marshals the headers as received.
The receiver's other protections are untouched: the 1 MB body cap, the
per-IP rate limiter, the 410 for a deactivated entrypoint and the 404
for an unknown UUID.
This commit is contained in:
@@ -2,7 +2,6 @@ package handlers
|
||||
|
||||
import (
|
||||
"encoding/json"
|
||||
"errors"
|
||||
"fmt"
|
||||
"io"
|
||||
"net/http"
|
||||
@@ -12,7 +11,6 @@ import (
|
||||
"sneak.berlin/go/webhooker/internal/database"
|
||||
"sneak.berlin/go/webhooker/internal/delivery"
|
||||
"sneak.berlin/go/webhooker/internal/logfield"
|
||||
"sneak.berlin/go/webhooker/internal/signature"
|
||||
)
|
||||
|
||||
const (
|
||||
@@ -72,8 +70,12 @@ func (h *Handlers) HandleWebhook() http.HandlerFunc {
|
||||
}
|
||||
}
|
||||
|
||||
// processWebhookRequest reads the body, verifies the sender,
|
||||
// serializes headers, loads targets, and delivers the event.
|
||||
// processWebhookRequest reads the body, serializes headers, loads
|
||||
// targets, and delivers the event.
|
||||
//
|
||||
// Nothing about the request itself is authenticated: the entrypoint
|
||||
// UUID in the path is the credential, and reaching here means it
|
||||
// matched an active entrypoint.
|
||||
func (h *Handlers) processWebhookRequest(
|
||||
w http.ResponseWriter,
|
||||
r *http.Request,
|
||||
@@ -84,26 +86,7 @@ func (h *Handlers) processWebhookRequest(
|
||||
return
|
||||
}
|
||||
|
||||
// Before anything is written. An unverified request must leave no
|
||||
// event row, no delivery row and no delivery task behind, so this
|
||||
// sits above every write rather than inside the transaction that
|
||||
// performs them. It has to sit below the body read because the
|
||||
// signature is computed over the body; readWebhookBody is what
|
||||
// bounds that read, so an unauthenticated sender still cannot make
|
||||
// the process hold more than the 1 MB cap.
|
||||
if !h.verifyInboundSignature(w, entrypoint, r.Header, body) {
|
||||
return
|
||||
}
|
||||
|
||||
// These headers are about to be stored verbatim and handed to
|
||||
// every delivery target, so the scheme's credential comes out
|
||||
// first. Under GitLab's scheme the header is the shared secret
|
||||
// itself, and leaving it in would hand the ability to forge
|
||||
// signed requests to exactly the parties the signature is meant
|
||||
// to exclude.
|
||||
headersJSON, err := json.Marshal(
|
||||
signature.SanitizeHeaders(&entrypoint, r.Header),
|
||||
)
|
||||
headersJSON, err := json.Marshal(r.Header)
|
||||
if err != nil {
|
||||
h.serverError(w, "failed to serialize headers", err)
|
||||
|
||||
@@ -122,63 +105,6 @@ func (h *Handlers) processWebhookRequest(
|
||||
)
|
||||
}
|
||||
|
||||
// verifyInboundSignature authenticates the request against the
|
||||
// entrypoint's configured secret, reporting false once it has written
|
||||
// the response.
|
||||
//
|
||||
// An entrypoint with no secret configured is not checked and this
|
||||
// returns true, which is the unchanged behaviour every existing
|
||||
// entrypoint keeps.
|
||||
//
|
||||
// A configuration that cannot be applied — an unknown scheme, or one
|
||||
// half of the pair missing — is a 500, not a 401: the request may well
|
||||
// be authentic, and calling it unauthorized would tell a legitimate
|
||||
// sender to go fix its own signing. Either way it is refused. Failing
|
||||
// open here would mean an entrypoint the operator has protected
|
||||
// quietly accepting anything.
|
||||
func (h *Handlers) verifyInboundSignature(
|
||||
w http.ResponseWriter,
|
||||
entrypoint database.Entrypoint,
|
||||
header http.Header,
|
||||
body []byte,
|
||||
) bool {
|
||||
err := signature.Verify(&entrypoint, header, body)
|
||||
if err == nil {
|
||||
return true
|
||||
}
|
||||
|
||||
if errors.Is(err, signature.ErrConfig) {
|
||||
h.log.Error(
|
||||
"entrypoint signature configuration cannot be applied",
|
||||
"entrypoint_id", entrypoint.ID,
|
||||
"webhook_id", entrypoint.WebhookID,
|
||||
"error", err,
|
||||
)
|
||||
http.Error(
|
||||
w, "Internal server error",
|
||||
http.StatusInternalServerError,
|
||||
)
|
||||
|
||||
return false
|
||||
}
|
||||
|
||||
// Every field here is bounded and none is client-chosen: the ids
|
||||
// are ours, the scheme is one of a fixed set, and the error is a
|
||||
// static string carrying no part of the secret or of what the
|
||||
// client presented. Reaching this line also requires a real
|
||||
// entrypoint UUID, so it is not a line a stranger can drive.
|
||||
h.log.Warn(
|
||||
"inbound signature verification failed",
|
||||
"entrypoint_id", entrypoint.ID,
|
||||
"webhook_id", entrypoint.WebhookID,
|
||||
"scheme", string(entrypoint.SignatureScheme),
|
||||
"error", err,
|
||||
)
|
||||
http.Error(w, "Unauthorized", http.StatusUnauthorized)
|
||||
|
||||
return false
|
||||
}
|
||||
|
||||
// loadActiveTargets returns all active targets for a webhook.
|
||||
func (h *Handlers) loadActiveTargets(
|
||||
webhookID string,
|
||||
|
||||
Reference in New Issue
Block a user