|
| 1 | +# Design: Webhook & Ingestion Input Hardening |
| 2 | + |
| 3 | +## Goal |
| 4 | +Harden all HTTP ingestion endpoints against untrusted input. Forge |
| 5 | +webhook and submit API endpoints accept external data that flows into |
| 6 | +git commands, database queries, and the review pipeline. This design |
| 7 | +addresses four related security gaps that share the same attack |
| 8 | +surface. |
| 9 | + |
| 10 | +## Threat Model |
| 11 | + |
| 12 | +When Sashiko accepts unauthenticated webhook requests, an attacker |
| 13 | +who can reach the endpoint can: |
| 14 | + |
| 15 | +1. Forge webhooks to trigger expensive AI reviews (cost amplification) |
| 16 | +2. Inject arbitrary repository URLs for Sashiko to clone (SSRF risk) |
| 17 | +3. Exhaust server memory via oversized payloads or gzip bombs |
| 18 | +4. Pass malformed commit SHAs to git commands |
| 19 | +5. Pollute the review database with fake patchsets |
| 20 | + |
| 21 | +## Architecture Changes |
| 22 | + |
| 23 | +### 1. Webhook Signature Verification (`src/forge.rs`) |
| 24 | + |
| 25 | +The `ForgeProvider` trait gains body and secret parameters: |
| 26 | + |
| 27 | +```rust |
| 28 | +fn validate_event( |
| 29 | + &self, |
| 30 | + headers: &HeaderMap, |
| 31 | + body: &Bytes, |
| 32 | + secret: Option<&str>, |
| 33 | +) -> Result<(), StatusCode>; |
| 34 | +``` |
| 35 | + |
| 36 | +Three verification methods, selected by header presence: |
| 37 | + |
| 38 | +- **Standard Webhooks HMAC-SHA256** (GitLab 19.0+ signing token): |
| 39 | + verifies `webhook-signature` header over |
| 40 | + `{webhook-id}.{webhook-timestamp}.{body}`. |
| 41 | +- **GitHub HMAC-SHA256**: verifies `X-Hub-Signature-256` header. |
| 42 | +- **Legacy secret token**: constant-time comparison of |
| 43 | + `X-Gitlab-Token` header. |
| 44 | + |
| 45 | +Secret type auto-detection: `whsec_` prefix indicates a Standard |
| 46 | +Webhooks signing token (base64-decoded); plain strings are used as |
| 47 | +raw key bytes. A warning is logged if base64 decoding fails for a |
| 48 | +`whsec_`-prefixed token. |
| 49 | + |
| 50 | +All comparisons use the `subtle` crate for constant-time operations. |
| 51 | + |
| 52 | +### 2. Access Control Revision (`src/api.rs`) |
| 53 | + |
| 54 | +When `webhook_secret` is configured, non-localhost requests are |
| 55 | +permitted because the signature check is the access control. The |
| 56 | +`--enable-unsafe-all-submit` flag is only needed for unauthenticated |
| 57 | +setups. |
| 58 | + |
| 59 | +```rust |
| 60 | +let is_loopback = addr.ip().to_canonical().is_loopback(); |
| 61 | +let has_secret = webhook_secret.is_some(); |
| 62 | +if !is_loopback && !has_secret && !state.allow_all_submit { |
| 63 | + return Err(StatusCode::FORBIDDEN); |
| 64 | +} |
| 65 | +``` |
| 66 | + |
| 67 | +### 3. Body Size Limits (`src/api.rs`) |
| 68 | + |
| 69 | +- Explicit `DefaultBodyLimit::max(2 MiB)` on the axum router (makes |
| 70 | + the implicit axum 0.8 default auditable). |
| 71 | +- HTTP download cap (10 MiB) on `fetch_and_inject_thread` for |
| 72 | + lore.kernel.org mbox fetches. |
| 73 | +- Decompression cap (50 MiB) via `Read::take()` on the gzip decoder |
| 74 | + to prevent decompression bombs. |
| 75 | + |
| 76 | +### 4. Input Validation (`src/forge.rs`) |
| 77 | + |
| 78 | +Validation functions applied in both forge `parse_payload` methods: |
| 79 | + |
| 80 | +- `is_valid_git_sha(s)`: accepts 40-char SHA-1 or 64-char SHA-256, |
| 81 | + hex digits only. |
| 82 | +- `is_safe_repo_url(url)`: requires `https://`, `http://`, or `git@` |
| 83 | + scheme; rejects known SSRF targets (cloud metadata endpoints, |
| 84 | + loopback addresses). |
| 85 | +- `pr_number > 0` check on both forge providers. |
| 86 | + |
| 87 | +These validations are applied only in the forge webhook path, not in |
| 88 | +the CLI submit path, because the CLI legitimately sends git refs and |
| 89 | +local filesystem paths. |
| 90 | + |
| 91 | +Message-ID path separator sanitization (`/` and `\`) is applied in |
| 92 | +the Thread submit handler to prevent URL path manipulation when |
| 93 | +constructing lore.kernel.org fetch URLs. |
| 94 | + |
| 95 | +### 5. Startup Warnings (`src/main.rs`) |
| 96 | + |
| 97 | +Two warning conditions at startup: |
| 98 | + |
| 99 | +- Forge enabled without `webhook_secret` and without |
| 100 | + `--enable-unsafe-all-submit`: warns that non-localhost requests |
| 101 | + will be rejected. |
| 102 | +- Forge enabled without `webhook_secret` but with |
| 103 | + `--enable-unsafe-all-submit`: warns about accepting unauthenticated |
| 104 | + requests. |
| 105 | + |
| 106 | +## Configuration |
| 107 | + |
| 108 | +```toml |
| 109 | +[forge] |
| 110 | +enabled = true |
| 111 | +provider = "gitlab" |
| 112 | +webhook_secret = "whsec_..." # signing token (recommended) |
| 113 | +# OR |
| 114 | +webhook_secret = "my-secret" # plain secret token |
| 115 | +``` |
| 116 | + |
| 117 | +No new required config fields. The existing `webhook_secret` field |
| 118 | +(previously dead code) is activated. |
| 119 | + |
| 120 | +## Dependencies |
| 121 | + |
| 122 | +New crate dependencies: |
| 123 | +- `hmac = "0.13"` (HMAC-SHA256 computation) |
| 124 | +- `base64 = "0.22"` (signing token key decoding) |
| 125 | +- `subtle = "2"` (constant-time comparison) |
| 126 | + |
| 127 | +## Deployment Topologies |
| 128 | + |
| 129 | +The design supports four deployment patterns, documented in |
| 130 | +`docs/WEBHOOK_SECURITY.md`: |
| 131 | + |
| 132 | +1. Public server with reverse proxy (nginx/Caddy terminates TLS) |
| 133 | +2. Tunnel-based (ngrok, Cloudflare Tunnel, SSH) |
| 134 | +3. Self-hosted forge on same LAN |
| 135 | +4. Script-based polling (cronjob + curl) |
| 136 | + |
| 137 | +When `webhook_secret` is configured, topologies 1-3 do not require |
| 138 | +`--enable-unsafe-all-submit`. |
| 139 | + |
| 140 | +## Risks |
| 141 | + |
| 142 | +- SSRF blocklist is best-effort (DNS rebinding can bypass string |
| 143 | + checks). The primary access control is signature verification. |
| 144 | +- `subtle::ConstantTimeEq` reveals length differences between |
| 145 | + compared strings (acceptable for webhook secrets with sufficient |
| 146 | + entropy). |
| 147 | +- No replay attack prevention via timestamp validation in this |
| 148 | + version (documented as future enhancement). |
0 commit comments