diff --git a/README.md b/README.md index c66e5ca..e1289af 100644 --- a/README.md +++ b/README.md @@ -7,6 +7,13 @@ services, durably stores them, and delivers them to configured targets with retry support, logging, and observability. Category: infrastructure / web service. License: MIT. +Each entrypoint is a version 4 UUID served at `/webhook/{uuid}`, and +that UUID is the entrypoint's only credential. webhooker does not use +shared secrets, HMAC signatures or token headers on the receiver, and +will not add them — read +[The entrypoint URL is the authentication secret](#the-entrypoint-url-is-the-authentication-secret) +before deploying one. + ## Getting Started ### Prerequisites @@ -1149,14 +1156,38 @@ backups at rest and restrict who can read them. ## The entrypoint URL is the authentication secret -The receiver verifies nothing about an inbound request. The UUID in an -entrypoint's URL is its credential: anyone who holds that URL can -submit events to it, and the receiver checks nothing else about the -sender. Treat an entrypoint URL the way you would treat an API token. +**The entrypoint UUID is the credential, and it is the only one.** +webhooker mints a version 4 UUID per entrypoint and serves it at +`/webhook/{uuid}`. Possession of that URL is the authentication: +anyone who holds it can submit events to the entrypoint, and the +receiver verifies nothing else about the sender. -There is no way to rotate the UUID in place. To retire one, delete the -entrypoint (or deactivate it, which answers `410`) and create a new -one, then point the sender at the new URL. +There is no shared secret, no HMAC signature, no bearer token and no +second factor on the receiver, and none will be added. This was +considered and rejected; the implementation that existed was removed +in [PR #279](https://git.eeqj.de/sneak/webhooker/pulls/279), closing +[issue #67](https://git.eeqj.de/sneak/webhooker/issues/67) and +[issue #241](https://git.eeqj.de/sneak/webhooker/issues/241). A +proposal to reintroduce any of them — including as "defence in depth" +alongside the UUID — is answered by this section. Inbound signature +headers a sender sends anyway (`X-Hub-Signature` and its +per-provider equivalents) are stored and forwarded as ordinary +headers; nothing checks them. + +What that means for an operator: + +- **The URL is a capability, so treat it as a secret.** Keep it out of + logs, ticket bodies, chat messages and screenshots. Anyone who reads + it anywhere can post events as that sender. +- **Rotating means minting a new entrypoint, not changing a key.** + There is no way to rotate the UUID in place. To retire one, delete + the entrypoint (or deactivate it, which answers `410`) and create a + new one, then point the sender at the new URL. +- **A sender that cannot be given a secret URL is a constraint on that + integration, not a reason to change this.** If a service only + supports signed payloads to a well-known URL, raise it as its own + problem — pick a different integration path, or accept that it + cannot be used. It is not grounds to reintroduce shared secrets. ## Entrypoints @@ -1476,7 +1507,7 @@ events should be forwarded. | `type` | TargetType | One of: `http`, `slack`, `database`, `log` | | `active` | boolean | Whether deliveries are enabled (default: true) | | `config` | JSON text | Type-specific configuration | -| `max_retries` | integer | Maximum retry attempts for `http` and `slack` targets (0 = fire-and-forget, >0 = retries with backoff and a circuit breaker). Ignored by `database` and `log` targets | +| `max_retries` | integer | Total delivery attempts for `http` and `slack` targets, not retries on top of the first: 0 is a single fire-and-forget attempt with no retries and no circuit breaker, and a value of N makes N attempts in all, with exponential backoff and a per-target circuit breaker. Ignored by `database` and `log` targets | | `max_queue_size` | integer | Stored and shown on the target's detail view, but not enforced anywhere yet: nothing in the delivery engine consults it. Queue depth is set by the two fixed 10,000-entry channels | **Relations:** Belongs to Webhook. Has many Deliveries. @@ -1484,12 +1515,12 @@ events should be forwarded. **Target types:** - **`http`** — Forward the event as an HTTP POST to a configured URL. - Behavior depends on `max_retries`: when `max_retries` is 0 (the - default), the target operates in fire-and-forget mode — a single - attempt with no retries and no circuit breaker. When `max_retries` is - greater than 0, failed deliveries are retried with exponential backoff - up to `max_retries` attempts, protected by a per-target circuit - breaker. + `max_retries` is the total number of delivery attempts, not retries on + top of the first: when `max_retries` is 0 (the default), the target + operates in fire-and-forget mode, a single attempt with no retries and + no circuit breaker; a value of N makes up to N attempts in all, + retrying failed deliveries with exponential backoff and protecting them + with a per-target circuit breaker. - **`slack`** — Post the event as a formatted message to a Slack-compatible incoming webhook URL (`webhookUrl` in `config`). It is built on the same HTTP core as `http` and honours `max_retries` @@ -2867,6 +2898,10 @@ check, see [The login endpoint](#the-login-endpoint). ### Authentication +- **Webhook receiver:** the entrypoint UUID in the URL, and nothing + else. No shared secret, no HMAC signature, no token header, and none + will be added — see + [The entrypoint URL is the authentication secret](#the-entrypoint-url-is-the-authentication-secret). - **Web UI:** Cookie-based sessions using gorilla/sessions with encrypted cookies. Sessions are configured with HttpOnly, SameSite Lax, and Secure whenever the request is on TLS — the flag follows the @@ -2906,7 +2941,8 @@ check, see [The login endpoint](#the-login-endpoint). mode - **The entrypoint URL is the receiver's only credential.** Nothing about an inbound request is verified; possession of the UUID - authorises submission (see + authorises submission, and no shared secret or signature check will + be added alongside it (see [The entrypoint URL is the authentication secret](#the-entrypoint-url-is-the-authentication-secret)) - **SSRF prevention** for HTTP delivery targets: private/reserved IP ranges (RFC 1918, loopback, link-local, cloud metadata) are blocked diff --git a/internal/database/database.go b/internal/database/database.go index f2880e8..ba28bae 100644 --- a/internal/database/database.go +++ b/internal/database/database.go @@ -17,12 +17,12 @@ import ( "gorm.io/gorm" "sneak.berlin/go/webhooker/internal/banner" "sneak.berlin/go/webhooker/internal/config" + "sneak.berlin/go/webhooker/internal/datadir" "sneak.berlin/go/webhooker/internal/gormlog" "sneak.berlin/go/webhooker/internal/logger" ) const ( - dataDirPerm = 0750 randomPasswordLen = 16 sessionKeyLen = 32 ) @@ -185,7 +185,9 @@ func (d *Database) connect() error { // caller's decision. func (d *Database) connectTo(dataDir string) error { // Ensure the data directory exists before opening the database. - err := os.MkdirAll(dataDir, dataDirPerm) + // datadir.DirPerm is the single source of the directory mode; this + // package creates the directory too, since either may run first. + err := os.MkdirAll(dataDir, datadir.DirPerm) if err != nil { return fmt.Errorf( "creating data directory %s: %w", diff --git a/internal/database/webhook_db_manager.go b/internal/database/webhook_db_manager.go index 81ca427..7da8169 100644 --- a/internal/database/webhook_db_manager.go +++ b/internal/database/webhook_db_manager.go @@ -13,6 +13,7 @@ import ( "gorm.io/driver/sqlite" "gorm.io/gorm" "sneak.berlin/go/webhooker/internal/config" + "sneak.berlin/go/webhooker/internal/datadir" "sneak.berlin/go/webhooker/internal/gormlog" "sneak.berlin/go/webhooker/internal/logger" ) @@ -53,8 +54,9 @@ func NewWebhookDBManager( log: params.Logger.Get(), } - // Create data directory if it doesn't exist - err := os.MkdirAll(m.dataDir, dataDirPerm) + // Create data directory if it doesn't exist. datadir.DirPerm is the + // single source of the directory mode; either package may run first. + err := os.MkdirAll(m.dataDir, datadir.DirPerm) if err != nil { return nil, fmt.Errorf( "creating data directory %s: %w", diff --git a/internal/datadir/lock.go b/internal/datadir/lock.go index a50929c..bb605cd 100644 --- a/internal/datadir/lock.go +++ b/internal/datadir/lock.go @@ -29,9 +29,11 @@ import ( // process that was killed with SIGKILL blocks nothing. const LockFileName = "webhooker.lock" -// dirPerm is the mode Acquire creates DATA_DIR with. It matches what -// internal/database uses, since whichever runs first creates it. -const dirPerm = 0o750 +// DirPerm is the mode DATA_DIR is created with. It is the single +// source of that mode: internal/database consumes it rather than +// keeping its own copy, so the two packages that both create the +// directory cannot drift into disagreeing about its permissions. +const DirPerm = 0o750 // ErrLocked reports that another live process holds the data // directory. Callers that need to know whether a deployment is running @@ -64,7 +66,7 @@ func Acquire(dir string) (*Lock, error) { return nil, ErrNoDir } - err := os.MkdirAll(dir, dirPerm) + err := os.MkdirAll(dir, DirPerm) if err != nil { return nil, fmt.Errorf( "creating data directory %s: %w", dir, err, diff --git a/internal/delivery/redirect_test.go b/internal/delivery/redirect_test.go index 31e3d66..1534697 100644 --- a/internal/delivery/redirect_test.go +++ b/internal/delivery/redirect_test.go @@ -170,7 +170,8 @@ func TestDelivery_CrossOriginRedirectDropsOriginScopedHeaders( // Stripping must not fire within the configured origin, or every // destination that redirects its own path would lose its // credential and start answering 401 — and would lose the inbound -// signature the receiver verifies. +// signature header the target endpoint verifies. webhooker's own +// receiver verifies no signature; it only forwards the header. func TestDelivery_SameOriginRedirectKeepsOriginScopedHeaders( t *testing.T, ) { diff --git a/internal/handlers/ui_copy_test.go b/internal/handlers/ui_copy_test.go index da9b6d6..8598de9 100644 --- a/internal/handlers/ui_copy_test.go +++ b/internal/handlers/ui_copy_test.go @@ -300,3 +300,80 @@ func TestEntrypointCopyButtonIsProgressiveEnhancement(t *testing.T) { "the page must render to completion, not abort partway", ) } + +// maxRetriesHelp is the wording both target forms must carry. The +// delivery core makes max_retries attempts in total, not that many +// retries on top of a first try (a fresh delivery starts at attempt 1 +// and target_http gives up once the attempt number reaches +// max_retries), and 0 is special-cased to a single fire-and-forget +// attempt with no circuit breaker. +const maxRetriesHelp = "This is the total number of delivery attempts, " + + "not retries on top of the first: a value of 3 makes three attempts " + + "in all. 0 means a single attempt with no retries and no circuit " + + "breaker." + +// TestTargetFormMaxRetriesCopyMatchesBehaviour pins the max_retries +// help text on both the create form (the add-target form on the webhook +// detail page) and the edit form, so the copy cannot drift back to +// calling the number a retry count. +func TestTargetFormMaxRetriesCopyMatchesBehaviour(t *testing.T) { + t.Parallel() + + var h *handlers.Handlers + + var sess *session.Session + + app := newTestApp(t, &h, &sess) + app.RequireStart() + + t.Cleanup(app.RequireStop) + + webhook := &database.Webhook{Name: "wh", RetentionDays: 14} + webhook.ID = testWebhookID + + entrypoint := database.Entrypoint{Path: "abc123"} + entrypoint.ID = "ep-1" + + createBody := renderPage( + t, h, sess, "source_detail.html", map[string]any{ + dataKeyWebhook: webhook, + "Entrypoints": handlers.NewEntrypointViews( + []database.Entrypoint{entrypoint}, + ), + "Targets": delivery.NewTargetViews(nil), + "Events": []database.Event{}, + "BaseURL": "https://hooks.example.com", + }, + ) + + assert.Contains( + t, createBody, maxRetriesHelp, + "the add-target form must explain max_retries as total attempts", + ) + + // A slack target exercises the same max_retries field while needing + // only Config.URL from the edit template, so the test data stays + // minimal. The Target key mirrors the field names the template reads + // off the handler's view value. + editBody := renderPage( + t, h, sess, "target_edit.html", map[string]any{ + dataKeyWebhook: webhook, + "Target": map[string]any{ + "ID": "tg-1", + "Name": "t", + "Type": "slack", + "Active": true, + "MaxRetries": 3, + "Config": map[string]any{ + "URL": "https://hooks.slack.com/services/x", + }, + }, + dataKeyError: "", + }, + ) + + assert.Contains( + t, editBody, maxRetriesHelp, + "the target edit form must explain max_retries as total attempts", + ) +} diff --git a/templates/source_detail.html b/templates/source_detail.html index bde8cae..3b26967 100644 --- a/templates/source_detail.html +++ b/templates/source_detail.html @@ -120,9 +120,12 @@ -
- - +
+
+ + +
+

This is the total number of delivery attempts, not retries on top of the first: a value of 3 makes three attempts in all. 0 means a single attempt with no retries and no circuit breaker.

diff --git a/templates/target_edit.html b/templates/target_edit.html index 9194721..a2ff56c 100644 --- a/templates/target_edit.html +++ b/templates/target_edit.html @@ -69,7 +69,7 @@
-

0 is fire-and-forget: one attempt, no circuit breaker.

+

This is the total number of delivery attempts, not retries on top of the first: a value of 3 makes three attempts in all. 0 means a single attempt with no retries and no circuit breaker.

{{end}}