Remove inbound request signature verification (closes #279) #281
Reference in New Issue
Block a user
Delete Branch "issue-279-remove-inbound-signing"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Closes #279. Reverses #67 (landed in
fcead5d) and closes #241 with it.The receiver now verifies nothing about an inbound request. The entrypoint UUID in the URL is the authentication secret: possession of it authorises submission. Straight deletion — no flag, no migration, no compatibility path.
What went
internal/signature/— package and tests.Entrypoint.SignatureSchemeandEntrypoint.SignatureSecret, theSignatureSchemetype and its constants, andSignatureConfigured/SignatureHalfConfigured.verifyInboundSignaturein the receiver, and thesignature.SanitizeHeaderscall above the first write.SanitizeHeadersexisted only to strip a scheme's own credential header (GitLab'sX-Gitlab-Token) before the header map was stored and forwarded; with no configured credential there is nothing to strip, so the receiver marshalsr.Headeras received.HandleEntrypointSecret,applyEntrypointSecret,storeEntrypointSecretand thePOST /source/{id}/entrypoints/{entrypointID}/secretroute.EntrypointView(Configured,Scheme,SchemeLabel,SchemeHeader) and its three label constants. The view itself stays — it is still the display projection that keeps a raw row out of a template.Signature:line, the Configure/Rotate button, the secret form and itsshowSecretAlpine state intemplates/source_detail.html.Inbound Signature Verificationsection, the twoentrypointscolumns and theirAutoMigratenote, the/secretroute row, the tree entries, the security bullet, the resubmit paragraph and pipeline step 3.No config keys were removed because the feature never added any.
The columns are simply no longer written. Pre-1.0, no installed base, no stored data — a vestigial column in a database nobody has is not worth a migration.
What stayed, deliberately
The receiver's other protections are untouched, and each was exercised on a running instance (below): the 1 MB body cap, the per-IP rate limiter, the
410for a deactivated entrypoint and the404for an unknown UUID.No UUID masking or redaction was added anywhere. #254 and #179 are untouched.
Three delivery-side comments naming
X-Hub-Signature/X-Gitlab-Tokenare left alone —internal/delivery/redirect.go(origin-scoped header stripping),internal/delivery/target_redact.go(outbound redaction) andinternal/server/sentry.go(the Sentry header allowlist). Senders still send those headers; those files are about forwarding and redaction, not about verification, and each statement is still true.README
Replaced with a short section,
The entrypoint URL is the authentication secret: the receiver verifies nothing, the UUID is the credential, treat the URL like an API token, and there is no in-place rotation — delete or deactivate the entrypoint and create a new one.Verification
make checkgreen on the rebased branch withGOFLAGS=-count=1: 575 tests, 0 cached, lint in Docker reporting0 issues.Live instance on port 19310 with an HTTP sink target:
200, delivered to the sinkX-Hub-Signature-256and a wrongX-Gitlab-Token200, delivered to the sink200413 Request body too large429— the default per-IP receiver limit, exactly410 Gone404200Both accepted payloads were confirmed in the sink's received log, so acceptance and delivery were both proven rather than just the status code.
Grep across the tree finds no surviving
signature_scheme,signature_secret,SignatureScheme,SignatureSecret,SanitizeHeaders,HandleEntrypointSecret,verifyInboundSignatureorinternal/signaturereference in code, templates, config or docs.Note for the reviewer, not addressed here
golangci-lintemits a deprecation warning on every run:gomodguardis deprecated since v2.12.0 in favour ofgomodguard_v2. It predates this branch and is out of scope for this issue; it wants its own issue.PASS. Independent review: DoD of #279 met;
make checkgreen from a clean clone withGOFLAGS=-count=1(1203 tests, 0(cached), lint in Docker ran 53s, 0 issues); CI green oncf48b46; merges clean intonext; no dead route, dead export or stale template/fixture; no attribution trailers.Anomalies and disclosures, none blocking:
Stale comment,
internal/delivery/redirect_test.go:173— "and would lose the inbound signature the receiver verifies". The receiver verifies nothing now, so that clause is false. The PR body claims the three delivery-side comments naming these headers were audited and "each statement is still true"; this is a fourth, and it is not. Nit-level (the rest of that rationale holds and the test itself is correct), but it should read as something like "and would lose the sender's forwarded headers".SanitizeHeadersruled on — read on032f265: it cloned the header map and did exactly one thing,clone.Del(info.Header)for a scheme whose header is the credential itself, returning the clone unchanged for GitHub or an unknown scheme. No normalisation, no size bounding, no hop-by-hop removal. Removing it wholesale took nothing else.Forwarded sender headers are consistency, not a new hole — verified live: with a wrong
X-Gitlab-TokenandX-Hub-Signature-256on the wire, both reach the operator-configured target exactly asX-Gitlab-Eventand every other sender header always did (the strip only ever fired for an entrypoint with the GitLab scheme configured, a state that no longer exists). Redirect-hop protection from #233 / #243 is intact and was proven, not assumed: a target redirecting127.0.0.1:19442->127.0.0.2:19443deliveredX-Gitlab-Token,X-Hub-Signature-256and the operator's ownX-Op-Credto hop 1 and none of the three to hop 2.TestDelivery_CrossOriginRedirectDropsOriginScopedHeadersalso ran uncached. Exposure is bounded to the configured target.internal/handlers/entrypoint_view.goexists only because offcead5dand now projects every non-relational field of the model. Keeping it was declared and is consistent withdelivery.TargetView; noted, not a defect.README conflict resolution verified — #269's
internal/reqtls.IsTLSwording survives byte-identical in all four places it appears; only the adjacent signature bullet was replaced."No config keys were removed because the feature never added any" is true —
fcead5dtouched no file underinternal/config.Receiver guards re-proven on a live instance, not taken from the PR body: 1048575 bytes ->
200, 1048577 ->413 Request body too large; 200 sustained POSTs from one IP -> 114200then 86429(120/min per IP per entrypoint, six already spent in that window); deactivated entrypoint ->410, reactivated ->200; unknown UUID ->404; unsigned and wrongly-signed POSTs ->200and both confirmed in the sink's log.Follow-up for the manager, out of scope here:
TODO.md:32andTODO.md:302still describe inbound signature verification as landed, and line 302 keeps "Stripe HMAC signature verification" as a Future Step premised on the GitHub and GitLab schemes.TODO.mdsays issue branches must not touch it, so this PR was right to leave it; it needs fixing onnextby whoever maintains that file.