diff --git a/src/agentex/lib/sdk/utils/webhook_signatures.py b/src/agentex/lib/sdk/utils/webhook_signatures.py new file mode 100644 index 000000000..dc03e387c --- /dev/null +++ b/src/agentex/lib/sdk/utils/webhook_signatures.py @@ -0,0 +1,157 @@ +"""Prove an inbound webhook really came from GitHub or Slack, agent-side. + +The Agentex control plane verifies a webhook's HMAC at its +``/agents/forward/name/{agent}/{path}`` ingress and then proxies the request to the +agent's own HTTP route. That verification does NOT travel with the request: the +agent's route is an ordinary FastAPI route on an app bound to ``0.0.0.0:8000``, so +any peer that can reach the pod port can POST the same shape of body directly, and +nothing about the request distinguishes it from a proxied one (VULN-3137). FastACP +adds no authentication to agent-defined routes either -- its ``x-agent-api-key`` +check lives inside the JSON-RPC handler for ``POST /api`` only. + +The control plane forwards the source's own signature headers through to the agent +(it strips only hop-by-hop headers), so the agent CAN re-verify them. That is what +this module is for: :class:`WebhookVerification` carries the raw body, the request +headers and the shared secret, and :meth:`WebhookVerification.check` fails closed +unless the signature matches. + +Two properties matter and both need the RAW body: + +* the HMAC is computed over the exact bytes the source signed -- re-serialising a + parsed ``dict`` produces different bytes and will never verify; +* Slack additionally binds a timestamp, so a captured request cannot be replayed + indefinitely. + +Stdlib only. +""" + +from __future__ import annotations + +import hmac +import time +import hashlib +from typing import Literal +from dataclasses import dataclass +from collections.abc import Mapping + +__all__ = [ + "GITHUB_SIGNATURE_HEADER", + "SLACK_MAX_AGE_SECONDS", + "SLACK_SIGNATURE_HEADER", + "SLACK_TIMESTAMP_HEADER", + "WebhookSignatureError", + "WebhookSource", + "WebhookVerification", + "verify_github_signature", + "verify_slack_signature", +] + +WebhookSource = Literal["github", "slack"] + +GITHUB_SIGNATURE_HEADER = "x-hub-signature-256" +SLACK_SIGNATURE_HEADER = "x-slack-signature" +SLACK_TIMESTAMP_HEADER = "x-slack-request-timestamp" + +# Slack's own guidance: refuse a request whose timestamp is more than five minutes +# old, so a captured body cannot be replayed forever. +SLACK_MAX_AGE_SECONDS = 300 + + +class WebhookSignatureError(RuntimeError): + """The payload could not be proved to come from the webhook source. + + Deliberately distinct from ``WebhookError``: a handler should answer this with + 401/403, not 500, and must not fall through to driving an agent turn. + """ + + +def _header(headers: Mapping[str, str], name: str) -> str: + """Case-insensitive header lookup over a plain mapping. + + Starlette's ``request.headers`` is already case-insensitive, but a handler may + hand over a plain ``dict``, and HTTP header names are not case-sensitive. + """ + value = headers.get(name) + if value is not None: + return value + lowered = name.lower() + for key, candidate in headers.items(): + if isinstance(key, str) and key.lower() == lowered: + return candidate + return "" + + +def verify_github_signature(secret: str, body: bytes, headers: Mapping[str, str]) -> None: + """Check GitHub's ``X-Hub-Signature-256``; raise :class:`WebhookSignatureError` if bad. + + The signature is ``sha256=`` of HMAC-SHA256(secret, raw body). Gitea sends + the same header for the same reason, so this covers both. + """ + if not secret: + raise WebhookSignatureError("No webhook secret configured; cannot verify the signature") + presented = _header(headers, GITHUB_SIGNATURE_HEADER).strip() + if not presented: + raise WebhookSignatureError(f"Missing {GITHUB_SIGNATURE_HEADER} header") + expected = "sha256=" + hmac.new(secret.encode("utf-8"), body, hashlib.sha256).hexdigest() + if not hmac.compare_digest(presented, expected): + raise WebhookSignatureError(f"Invalid {GITHUB_SIGNATURE_HEADER} signature") + + +def verify_slack_signature( + secret: str, + body: bytes, + headers: Mapping[str, str], + *, + now: float | None = None, + max_age_seconds: int = SLACK_MAX_AGE_SECONDS, +) -> None: + """Check Slack's ``X-Slack-Signature``; raise :class:`WebhookSignatureError` if bad. + + The signature is ``v0=`` of HMAC-SHA256(secret, ``v0::``). + A timestamp further than ``max_age_seconds`` from now (either direction, so a + future-dated one is refused too) is rejected before the HMAC is compared. + """ + if not secret: + raise WebhookSignatureError("No webhook secret configured; cannot verify the signature") + presented = _header(headers, SLACK_SIGNATURE_HEADER).strip() + if not presented: + raise WebhookSignatureError(f"Missing {SLACK_SIGNATURE_HEADER} header") + + raw_timestamp = _header(headers, SLACK_TIMESTAMP_HEADER).strip() + try: + timestamp = int(raw_timestamp) + except (TypeError, ValueError): + raise WebhookSignatureError(f"Missing or unparseable {SLACK_TIMESTAMP_HEADER} header") from None + current = time.time() if now is None else now + if abs(current - timestamp) > max_age_seconds: + raise WebhookSignatureError(f"Slack timestamp {timestamp} is more than {max_age_seconds}s from now") + + basis = b"v0:" + raw_timestamp.encode("utf-8") + b":" + body + expected = "v0=" + hmac.new(secret.encode("utf-8"), basis, hashlib.sha256).hexdigest() + if not hmac.compare_digest(presented, expected): + raise WebhookSignatureError(f"Invalid {SLACK_SIGNATURE_HEADER} signature") + + +@dataclass(frozen=True) +class WebhookVerification: + """Everything needed to prove one inbound webhook's provenance. + + ``body`` must be the RAW request bytes (``await request.body()``), not a parsed + and re-serialised dict: the HMAC covers the bytes the source signed. + """ + + source: WebhookSource + secret: str + body: bytes + headers: Mapping[str, str] + #: Override for tests; ``None`` means ``time.time()`` (Slack freshness only). + now: float | None = None + + def check(self) -> None: + """Raise :class:`WebhookSignatureError` unless the signature verifies.""" + if self.source == "github": + verify_github_signature(self.secret, self.body, self.headers) + elif self.source == "slack": + verify_slack_signature(self.secret, self.body, self.headers, now=self.now) + else: # pragma: no cover - Literal keeps this unreachable for typed callers + raise WebhookSignatureError(f"Unknown webhook source {self.source!r}") diff --git a/src/agentex/lib/sdk/utils/webhooks.py b/src/agentex/lib/sdk/utils/webhooks.py index d4b7b43e1..d5bfa8776 100644 --- a/src/agentex/lib/sdk/utils/webhooks.py +++ b/src/agentex/lib/sdk/utils/webhooks.py @@ -1,32 +1,63 @@ """Drive an agent turn from an inbound webhook, inside a forward-route handler. -The Agentex server already exposes a webhook ingress: a request to +The Agentex server exposes a webhook ingress: a request to ``/agents/forward/name/{agent}/{path}`` is signature-verified (GitHub ``sha256=`` / Slack ``v0:`` HMAC via the agent's registered keys) and proxied to the agent's own HTTP route. This helper is what that route handler calls to turn the inbound payload into an agent turn — without each agent re-implementing payload shaping, config resolution, session continuity, and reply handling. +**That ingress verification does not travel with the request.** The route below is an +ordinary FastAPI route on an app bound to ``0.0.0.0:8000``, so any peer that can reach +the pod port can POST the same body straight to it, and FastACP adds no authentication +to agent-defined routes. Reaching this route is therefore NOT evidence that the +signature was ever checked. The control plane does forward the source's own signature +headers, so the route re-verifies them here — ``handle_webhook`` refuses to drive a +turn without that proof (VULN-3137). + Typical use inside an agent:: - from fastapi import Request - from agentex.lib.sdk.utils.webhooks import handle_webhook + import json + import os + + from fastapi import HTTPException, Request + from agentex.lib.sdk.utils.webhooks import ( + WebhookVerification, + WebhookSignatureError, + handle_webhook, + ) @acp.post("/github-pr") async def github_pr(request: Request): - body = await request.json() - result = await handle_webhook( - agent_name="my-agent", - payload=body, - acp_type="sync", - shaper="github_pr", - params_source="https:///public/v5/agent_configs//resolve", - params_source_headers={"x-api-key": ..., "x-selected-account-id": ...}, - wait=True, - ) + # RAW bytes: the HMAC covers what GitHub signed, and a re-serialised dict + # produces different bytes that can never verify. + raw = await request.body() + try: + result = await handle_webhook( + agent_name="my-agent", + payload=json.loads(raw), + verify=WebhookVerification( + source="github", + secret=os.environ["GITHUB_WEBHOOK_SECRET"], + body=raw, + headers=request.headers, + ), + acp_type="sync", + shaper="github_pr", + params_source="https:///public/v5/agent_configs//resolve", + params_source_headers={"x-api-key": ..., "x-selected-account-id": ...}, + wait=True, + ) + except WebhookSignatureError: + raise HTTPException(status_code=403, detail="bad webhook signature") from None return {"task_id": result.task_id, "reply": result.reply} +``GITHUB_WEBHOOK_SECRET`` is the signing secret shown once when the trigger was +registered (``POST /agents/{name}/webhook-triggers``); give it to the agent the same +way as any other secret. Verifying also makes session continuity safe: the task name +is keyed with that secret, so only a holder can name an existing conversation. + Config-by-id: pass ``params_source`` pointing at the platform's config-resolve endpoint; the resolved params (e.g. system_prompt / harness / model / tools) are forwarded opaquely to ``task/create``. Or pass inline ``params`` for a one-off. @@ -34,6 +65,7 @@ async def github_pr(request: Request): from __future__ import annotations +import hmac import json import hashlib from typing import Any, Literal @@ -43,6 +75,11 @@ async def github_pr(request: Request): from agentex.lib import adk from agentex.lib.utils.logging import make_logger from agentex.types.task_message_content import TextContent +from agentex.lib.sdk.utils.webhook_signatures import ( + WebhookSource as WebhookSource, + WebhookVerification as WebhookVerification, + WebhookSignatureError as WebhookSignatureError, +) logger = make_logger(__name__) @@ -69,11 +106,26 @@ class WebhookResult: # --------------------------------------------------------------------------- shaping -def session_key(agent_name: str, channel: str, peer_id: str) -> str: +def session_key(agent_name: str, channel: str, peer_id: str, *, secret: str | None = None) -> str: """Stable per-conversation task name → reused for get-or-create on task/create, so - repeat events from the same source fold into one task instead of spawning new ones.""" + repeat events from the same source fold into one task instead of spawning new ones. + + ``task/create`` is get-or-create BY NAME, so this name decides which existing + conversation a turn joins. Unkeyed it is computable from public data (a repo name + and a PR number), which let an unverified caller name — and read the reply from — + somebody else's conversation (VULN-3137). ``secret`` keys the digest with the + webhook's shared secret, so the name can only be derived by someone who already + holds it, and two installations watching the same repository stay separate. + + Passing no ``secret`` keeps an unkeyed (but no longer SHA-1) name, for callers that + verified provenance themselves and have no secret to hand. + """ basis = peer_id or "main" - digest = hashlib.sha1(f"{agent_name}:{channel}:{basis}".encode()).hexdigest()[:16] + material = f"{agent_name}:{channel}:{basis}".encode() + if secret: + digest = hmac.new(secret.encode("utf-8"), material, hashlib.sha256).hexdigest()[:16] + else: + digest = hashlib.sha256(material).hexdigest()[:16] return f"wh-{channel}-{digest}" @@ -258,9 +310,13 @@ async def handle_webhook( extra_task_metadata: dict[str, str] | None = None, wait: bool = False, fetch: ParamsFetcher | None = None, + verify: WebhookVerification | None = None, + trust_caller: bool = False, ) -> WebhookResult: """Drive an agent turn from a webhook payload, agent-side, via the ADK client. + - Proves provenance FIRST: ``verify`` carries the raw body, headers and shared + secret, and nothing else runs until its signature checks out. - Shapes the payload (generic or GitHub PR) into a prompt + conversation scope. - Resolves task params: inline ``params``, or fetched from ``params_source`` (config-by-id). The platform never interprets params — they're forwarded to the @@ -268,7 +324,35 @@ async def handle_webhook( - Get-or-creates a task keyed on a stable session key, so repeat events fold in. - Sends the turn (sync → message/send returns the reply inline; async → event/send, with optional ``wait`` to poll for the reply). + + Provenance is mandatory (VULN-3137). The route this runs in is served on the + agent's own public listener, so "it reached my route" is not evidence that the + control plane's signature-verifying ingress let it through. Pass ``verify`` with + the raw request bytes, or — only if the route proved provenance by some other + means — pass ``trust_caller=True`` to say so explicitly. With neither, this + raises :class:`WebhookSignatureError` rather than driving a turn. + + Raises: + WebhookSignatureError: no provenance was established, or the signature failed. + WebhookError: the turn could not be driven (e.g. params resolution failed). """ + if verify is not None: + verify.check() + session_secret: str | None = verify.secret + elif trust_caller: + # The route says it established provenance itself. Nothing to key the session + # name with, so it falls back to an unkeyed name. + session_secret = None + else: + raise WebhookSignatureError( + "handle_webhook refuses to drive a turn without provenance: pass " + "verify=WebhookVerification(source=..., secret=..., body=, headers=request.headers), or trust_caller=True if this route " + "has already proved the request came from the webhook source. The " + "agent's HTTP route is reachable directly, so an unverified payload can " + "name and read back any existing webhook conversation (VULN-3137)." + ) + channel = channel or shaper if shaper == "github_pr": text, derived_peer, sender = shape_github_pr(payload) @@ -292,7 +376,7 @@ async def handle_webhook( for key, value in extra_task_metadata.items(): task_metadata.setdefault(key, str(value)) - name = session_key(agent_name, channel, peer_id or "") + name = session_key(agent_name, channel, peer_id or "", secret=session_secret) # task/create carries only name/params (CreateTaskParams has no task_metadata field), # so we create first, then stamp task_metadata via a follow-up update below. task = await adk.acp.create_task( diff --git a/tests/lib/test_webhooks.py b/tests/lib/test_webhooks.py index e42fac9dd..003a6ff66 100644 --- a/tests/lib/test_webhooks.py +++ b/tests/lib/test_webhooks.py @@ -1,7 +1,14 @@ -"""Unit tests for the SDK webhook helper (agentex.lib.sdk.utils.webhooks).""" +"""Unit tests for the SDK webhook helper (agentex.lib.sdk.utils.webhooks). + +The turn-driving tests pass ``trust_caller=True`` because provenance is not what they +are about; ``TestProvenance`` covers the signature gate itself. +""" from __future__ import annotations +import hmac +import json +import hashlib from types import SimpleNamespace from unittest.mock import AsyncMock @@ -16,6 +23,12 @@ shape_github_pr, resolve_remote_params, ) +from agentex.lib.sdk.utils.webhook_signatures import ( + WebhookVerification, + WebhookSignatureError, + verify_slack_signature, + verify_github_signature, +) def _pr_payload(**pr_overrides) -> dict: @@ -146,6 +159,7 @@ async def fake_resolve(_url): shaper="github_pr", params_source="https://h/v5/agent_configs/cfg-9/resolve", fetch=fake_resolve, + trust_caller=True, ) assert result.reply == "Looks good — ship it." @@ -168,6 +182,7 @@ async def test_inline_params_no_fetch(self): payload={"text": "hi"}, acp_type="sync", params={"system_prompt": "inline"}, + trust_caller=True, ) assert result.reply == "Looks good — ship it." assert self.created["params"] == {"system_prompt": "inline"} @@ -182,6 +197,7 @@ async def fake_resolve(_url): shaper="github_pr", params_source="https://h/resolve", fetch=fake_resolve, + trust_caller=True, ) assert result.task_metadata["channel"] == "github_pr" @@ -201,6 +217,7 @@ async def fake_resolve(_url): shaper="github_pr", params_source="https://h/resolve", fetch=fake_resolve, + trust_caller=True, ) stamped_metadata = self.stamped["task_metadata"] @@ -209,7 +226,9 @@ async def fake_resolve(_url): assert stamped_metadata["channel"] == "github_pr" async def test_async_without_wait_sends_event_and_returns_no_reply(self): - result = await handle_webhook(agent_name="a", payload={"text": "go"}, acp_type="async", wait=False) + result = await handle_webhook( + agent_name="a", payload={"text": "go"}, acp_type="async", wait=False, trust_caller=True + ) assert result.reply is None self.send_event.assert_awaited_once() @@ -265,3 +284,122 @@ async def no_sleep(_seconds): quiescence_s=0.0, ) assert reply == "NEW reply" + + +class TestProvenance: + """VULN-3137: the route is reachable directly, so a payload is not evidence.""" + + def test_github_signature_round_trips(self): + body = b'{"zen":"be awesome"}' + sig = "sha256=" + hmac.new(b"s3cret", body, hashlib.sha256).hexdigest() + # Does not raise. + verify_github_signature("s3cret", body, {"X-Hub-Signature-256": sig}) + + @pytest.mark.parametrize( + ("label", "headers"), + [ + ("absent", {}), + ("empty", {"X-Hub-Signature-256": ""}), + ("wrong-digest", {"X-Hub-Signature-256": "sha256=" + "a" * 64}), + ("unprefixed", {"X-Hub-Signature-256": "a" * 64}), + ], + ) + def test_github_signature_rejected(self, label, headers): + with pytest.raises(WebhookSignatureError): + verify_github_signature("s3cret", b"{}", headers) + + def test_github_signature_is_over_the_raw_bytes(self): + """A re-serialised dict is different bytes and must not verify. + + This is why handle_webhook takes the raw body rather than signing the parsed + payload: json.dumps(json.loads(x)) != x in general. + """ + raw = b'{"a": 1, "b": 2}' + sig = "sha256=" + hmac.new(b"k", raw, hashlib.sha256).hexdigest() + reserialised = json.dumps(json.loads(raw)).encode() + assert reserialised != raw + verify_github_signature("k", raw, {"X-Hub-Signature-256": sig}) + with pytest.raises(WebhookSignatureError): + verify_github_signature("k", reserialised, {"X-Hub-Signature-256": sig}) + + def test_slack_signature_round_trips_and_checks_freshness(self): + body = b"token=x&team_id=T1" + ts = "1700000000" + basis = b"v0:" + ts.encode() + b":" + body + sig = "v0=" + hmac.new(b"s3cret", basis, hashlib.sha256).hexdigest() + headers = {"X-Slack-Signature": sig, "X-Slack-Request-Timestamp": ts} + + verify_slack_signature("s3cret", body, headers, now=1700000030) + + # Same signature, replayed well outside the window. + with pytest.raises(WebhookSignatureError, match="more than"): + verify_slack_signature("s3cret", body, headers, now=1700000000 + 3600) + # ... and a future-dated timestamp is refused too. + with pytest.raises(WebhookSignatureError, match="more than"): + verify_slack_signature("s3cret", body, headers, now=1700000000 - 3600) + + def test_an_empty_secret_never_verifies(self): + body = b"{}" + sig = "sha256=" + hmac.new(b"", body, hashlib.sha256).hexdigest() + with pytest.raises(WebhookSignatureError, match="No webhook secret"): + verify_github_signature("", body, {"X-Hub-Signature-256": sig}) + + def test_headers_are_matched_case_insensitively(self): + body = b"{}" + sig = "sha256=" + hmac.new(b"k", body, hashlib.sha256).hexdigest() + verify_github_signature("k", body, {"x-hub-signature-256": sig}) + verify_github_signature("k", body, {"X-HUB-SIGNATURE-256": sig}) + + +class TestHandleWebhookProvenance: + @pytest.fixture(autouse=True) + def _never_reached(self, monkeypatch): + """Any ADK call at all means the gate let an unverified payload through.""" + boom = AsyncMock(side_effect=AssertionError("handle_webhook drove a turn")) + monkeypatch.setattr(adk.acp, "create_task", boom) + monkeypatch.setattr(adk.acp, "send_message", boom) + monkeypatch.setattr(adk.acp, "send_event", boom) + monkeypatch.setattr(adk.tasks, "update", boom) + + async def test_an_unverified_payload_drives_no_turn(self): + """The finding's direct POST: a well-formed body and nothing else.""" + with pytest.raises(WebhookSignatureError, match="without provenance"): + await handle_webhook(agent_name="golden-agent", payload=_pr_payload(), shaper="github_pr") + + async def test_a_forged_signature_drives_no_turn(self): + raw = json.dumps(_pr_payload()).encode() + with pytest.raises(WebhookSignatureError, match="Invalid"): + await handle_webhook( + agent_name="golden-agent", + payload=_pr_payload(), + shaper="github_pr", + verify=WebhookVerification( + source="github", + secret="the-real-secret", + body=raw, + headers={"X-Hub-Signature-256": "sha256=" + "a" * 64}, + ), + ) + + +class TestSessionBinding: + """VULN-3137 criterion 2: naming a conversation requires the secret.""" + + def test_an_unverified_caller_cannot_compute_a_verified_tasks_name(self): + # Public data: repo name and PR number. + peer = "acme/widgets#42" + guessed = session_key("golden-agent", "github_pr", peer) + real = session_key("golden-agent", "github_pr", peer, secret="the-real-secret") + assert guessed != real, "the task name must not be derivable from public data" + + def test_two_installations_watching_one_repo_stay_separate(self): + peer = "acme/widgets#42" + a = session_key("golden-agent", "github_pr", peer, secret="secret-a") + b = session_key("golden-agent", "github_pr", peer, secret="secret-b") + assert a != b + + def test_the_same_secret_still_folds_repeat_events(self): + peer = "acme/widgets#42" + first = session_key("golden-agent", "github_pr", peer, secret="s") + second = session_key("golden-agent", "github_pr", peer, secret="s") + assert first == second and first.startswith("wh-github_pr-")