fix: sign webhook HMACs over the exact transmitted bytes (#1441)

.duplication-baseline

@@ -1,5 +1,5 @@
 {
   "src": 36,
-  "tests": 89,
+  "tests": 88,
   "scripts": 0
 }

src/core/webhook_authenticator.py

@@ -1,63 +1,41 @@
-"""Webhook authentication using HMAC signatures.
-
-This module provides HMAC-SHA256 signing for webhook payloads to allow
-receivers to verify that webhooks are genuinely from this server.
-
-Similar to GitHub, Stripe, and Slack webhook authentication.
+"""Webhook signature verification (buyer/receiver reference).
+
+Verifies the AdCP legacy HMAC-SHA256 scheme: the signature covers
+``{unix_timestamp}.{raw_http_body}`` — the raw bytes exactly as received,
+never a re-serialization. Similar to GitHub, Stripe, and Slack webhook
+authentication.
+
+SENDING side note: the signer that used to live here (``sign_payload``) was
+removed — it signed a sorted-compact re-serialization while the HTTP client
+re-serialized the body differently, so its signatures never matched the wire
+(#1441). Senders now use :func:`adcp.sign_legacy_webhook`, which returns the
+signed headers together with the exact ``body_bytes`` to transmit.
 """
 
 import hashlib
 import hmac
-import json
 import time
 
 
 class WebhookAuthenticator:
-    """Handles webhook payload signing with HMAC-SHA256."""
-
-    @staticmethod
-    def sign_payload(payload: dict, secret: str) -> dict[str, str]:
-        """
-        Sign webhook payload with HMAC-SHA256.
-
-        Creates a signature that receivers can verify to ensure the webhook
-        is authentic and hasn't been tampered with.
-
-        Args:
-            payload: The webhook payload dict to sign
-            secret: The shared secret key (from webhook config)
-
-        Returns:
-            Headers dict to include in webhook HTTP request
-        """
-        # Serialize payload consistently (sorted keys, no spaces)
-        payload_str = json.dumps(payload, separators=(",", ":"), sort_keys=True)
-
-        # Include timestamp to prevent replay attacks
-        timestamp = str(int(time.time()))
-
-        # Create signed message: timestamp.payload
-        signed_payload = f"{timestamp}.{payload_str}"
-
-        # Generate HMAC-SHA256 signature
-        signature = hmac.new(secret.encode("utf-8"), signed_payload.encode("utf-8"), hashlib.sha256).hexdigest()
-
-        return {
-            "X-Webhook-Signature": f"sha256={signature}",
-            "X-Webhook-Timestamp": timestamp,
-        }
+    """Verifies webhook payload signatures (HMAC-SHA256, raw-body)."""
 
     @staticmethod
     def verify_signature(
         payload: str, signature: str, timestamp: str, secret: str, tolerance_seconds: int = 300
     ) -> bool:
         """
-        Verify webhook signature (for testing or customer reference).
+        Verify a webhook signature against the RAW request body.
+
+        ``payload`` must be the raw HTTP body exactly as received (decoded
+        text) — re-parsing and re-serializing the JSON before verification
+        breaks the byte-equality contract and rejects valid signatures.
 
         Args:
-            payload: The raw payload string
-            signature: The signature from X-Webhook-Signature header (with "sha256=" prefix)
-            timestamp: The timestamp from X-Webhook-Timestamp header
+            payload: The raw payload string (exact received bytes, decoded)
+            signature: The signature from the X-AdCP-Signature header (with
+                "sha256=" prefix)
+            timestamp: The unix-seconds timestamp from X-AdCP-Timestamp
             secret: The shared secret key
             tolerance_seconds: Max age of webhook to accept (default 5 minutes)
 

src/core/webhook_delivery.py

@@ -5,9 +5,10 @@
 - Database tracking of delivery attempts
 - Retry on 5xx errors, no retry on 4xx client errors
 - SSRF protection via WebhookURLValidator
-- HMAC signing support via WebhookAuthenticator
+- HMAC signing via adcp.sign_legacy_webhook (spec byte-equality contract)
 """
 
+import json
 import logging
 import time
 import uuid
@@ -16,9 +17,9 @@
 from typing import Any
 
 import requests
+from adcp import sign_legacy_webhook
 
 from src.core.database.database_session import get_db_session
-from src.core.webhook_authenticator import WebhookAuthenticator
 from src.core.webhook_validator import WebhookURLValidator
 
 logger = logging.getLogger(__name__)
@@ -101,11 +102,20 @@ def deliver_webhook_with_retry(delivery: WebhookDelivery) -> tuple[bool, dict[st
     # Generate delivery ID for tracking
     delivery_id = f"whd_{uuid.uuid4().hex[:12]}"
 
-    # Add HMAC signature if secret provided
+    # Serialize ONCE; when signing, sign those exact bytes. The AdCP
+    # legacy-HMAC contract is byte-equality (signature covers
+    # ``{unix_timestamp}.{raw_http_body}``) — the old path signed a
+    # sorted-compact re-serialization while ``requests(json=...)`` sent a
+    # spaced one, so signatures could never match the wire. Also moves this
+    # sender onto the spec header names (X-AdCP-Signature with sha256=
+    # prefix / X-AdCP-Timestamp) instead of the legacy X-Webhook-*.
     headers = delivery.headers.copy()
+    headers.setdefault("Content-Type", "application/json")
     if delivery.signing_secret:
-        signature_headers = WebhookAuthenticator.sign_payload(delivery.payload, delivery.signing_secret)
-        headers.update(signature_headers)
+        signed_headers, body_bytes = sign_legacy_webhook(delivery.signing_secret, delivery.payload)
+        headers.update(signed_headers)
+    else:
+        body_bytes = json.dumps(delivery.payload, separators=(",", ":")).encode("utf-8")
 
     # Track delivery attempts
     attempts = 0
@@ -133,9 +143,8 @@ def deliver_webhook_with_retry(delivery: WebhookDelivery) -> tuple[bool, dict[st
                 f"[Webhook Delivery] Attempt {attempt + 1}/{delivery.max_retries} for {delivery_id} to {delivery.webhook_url}"
             )
 
-            response = requests.post(
-                delivery.webhook_url, json=delivery.payload, headers=headers, timeout=delivery.timeout
-            )
+            # data= (not json=) so the signed bytes ARE the sent bytes
+            response = requests.post(delivery.webhook_url, data=body_bytes, headers=headers, timeout=delivery.timeout)
 
             response_code = response.status_code
             attempt_duration = time.time() - attempt_start

src/services/protocol_webhook_service.py

@@ -24,7 +24,7 @@
 
 import requests
 from a2a.types import Task, TaskStatusUpdateEvent
-from adcp import extract_webhook_result_data, get_adcp_signed_headers_for_webhook
+from adcp import extract_webhook_result_data, sign_legacy_webhook
 from adcp.types import McpWebhookPayload
 from google.protobuf.json_format import MessageToDict
 
@@ -182,24 +182,31 @@ async def send_notification(
         # lowercase enum values; Pydantic -> model_dump; Mapping -> dict.
         payload_dict: dict[str, Any] = _to_wire_dict(payload)
 
-        # Apply authentication based on schemes
+        # Serialize ONCE at the delivery boundary, before the retry loop. The
+        # AdCP legacy-HMAC rule is byte-equality: the signature covers
+        # ``{timestamp}.{raw_http_body}``, so the exact bytes that are signed
+        # MUST be the exact bytes that go on the wire — signing one
+        # serialization and letting the HTTP client re-serialize another
+        # (the old ``json=payload`` path) invalidates every signature.
         if (
             push_notification_config.authentication_type == "HMAC-SHA256"
             and push_notification_config.authentication_token
         ):
-            # Sign payload with HMAC-SHA256
-            timestamp = str(int(time.time()))
-            get_adcp_signed_headers_for_webhook(
-                headers, push_notification_config.authentication_token, timestamp, payload_dict
+            signed_headers, body_bytes = sign_legacy_webhook(
+                push_notification_config.authentication_token, payload_dict, headers=headers
             )
-
-        elif push_notification_config.authentication_type == "Bearer" and push_notification_config.authentication_token:
-            # Use Bearer token authentication
-            headers["Authorization"] = f"Bearer {push_notification_config.authentication_token}"
+            headers.update(signed_headers)
+        else:
+            body_bytes = json.dumps(payload_dict, separators=(",", ":")).encode("utf-8")
+            if (
+                push_notification_config.authentication_type == "Bearer"
+                and push_notification_config.authentication_token
+            ):
+                headers["Authorization"] = f"Bearer {push_notification_config.authentication_token}"
 
         # Send notification with retry logic and logging
         return await self._send_with_retry_and_logging(
-            url=url, payload=payload_dict, headers=headers, metadata=metadata
+            url=url, payload=payload_dict, body_bytes=body_bytes, headers=headers, metadata=metadata
         )
 
     @staticmethod
@@ -251,13 +258,18 @@ async def _send_with_retry_and_logging(
         self,
         url: str,
         payload: dict[str, Any],
+        body_bytes: bytes,
         headers: dict,
         metadata: dict[str, Any],
         max_attempts: int = 3,
     ) -> bool:
-        """Send webhook with exponential backoff retry logic, logging, and audit trail."""
-        # Calculate payload size for metrics
-        payload_size_bytes = len(json.dumps(payload).encode("utf-8"))
+        """Send webhook with exponential backoff retry logic, logging, and audit trail.
+
+        ``payload`` is used only for metadata extraction (task ids, result
+        data); ``body_bytes`` is the single serialization that was signed and
+        is what actually goes on the wire.
+        """
+        payload_size_bytes = len(body_bytes)
 
         task_type = metadata["task_type"] if "task_type" in metadata else None
         tenant_id = metadata["tenant_id"] if "tenant_id" in metadata else None
@@ -293,7 +305,8 @@ async def _send_with_retry_and_logging(
                 logger.info(f"Sending webhook for task {task_id} to {url} (attempt {attempt + 1}/{max_attempts})")
 
                 def _post() -> requests.Response:
-                    return self._session.post(url, json=payload, headers=headers, timeout=10.0)
+                    # data= (not json=) so the signed bytes ARE the sent bytes
+                    return self._session.post(url, data=body_bytes, headers=headers, timeout=10.0)
 
                 response = await asyncio.to_thread(_post)
                 response.raise_for_status()

src/services/webhook_delivery_service.py

@@ -11,8 +11,6 @@
 """
 
 import atexit
-import hashlib
-import hmac
 import json
 import logging
 import random
@@ -24,7 +22,7 @@
 from typing import Any
 
 import httpx
-from adcp import get_adcp_spec_version
+from adcp import get_adcp_spec_version, sign_legacy_webhook
 
 logger = logging.getLogger(__name__)
 
@@ -305,26 +303,6 @@ def send_delivery_webhook(
             )
             return False
 
-    def _generate_hmac_signature(self, payload: dict[str, Any], secret: str, timestamp: str) -> str:
-        """Generate HMAC-SHA256 signature for webhook payload.
-
-        Args:
-            payload: Webhook payload
-            secret: Webhook secret (min 32 characters)
-            timestamp: ISO format timestamp
-
-        Returns:
-            HMAC signature as hex string
-        """
-        # Create signature input: timestamp + json payload
-        payload_str = json.dumps(payload, sort_keys=True, separators=(",", ":"))
-        message = f"{timestamp}.{payload_str}"
-
-        # Generate HMAC-SHA256
-        signature = hmac.new(secret.encode("utf-8"), message.encode("utf-8"), hashlib.sha256).hexdigest()
-
-        return signature
-
     def _verify_secret_strength(self, secret: str) -> bool:
         """Verify webhook secret meets minimum strength requirements.
 
@@ -448,22 +426,30 @@ def _deliver_with_backoff(
 
         config = webhook_data["config"]
         payload = webhook_data["payload"]
-        timestamp = webhook_data["timestamp"].isoformat()
+        # AdCP legacy-HMAC replay prevention uses unix seconds — the signed
+        # message is ``{unix_timestamp}.{raw_http_body}`` (the previous ISO
+        # form here never matched a spec-compliant verifier).
+        timestamp = int(webhook_data["timestamp"].timestamp())
 
-        # Generate HMAC signature if webhook secret is configured
         webhook_secret = getattr(config, "webhook_secret", None)
         headers = {
             "Content-Type": "application/json",
             "User-Agent": "AdCP-Sales-Agent/2.3 (Enhanced Webhooks)",
-            "X-ADCP-Timestamp": timestamp,  # For replay prevention
         }
 
-        if webhook_secret:
-            if not self._verify_secret_strength(webhook_secret):
+        # Serialize ONCE, before the retry loop, and sign those exact bytes.
+        # Byte-equality is the whole contract: signing a sorted-compact
+        # re-serialization while httpx re-serialized the body differently (the
+        # old path) made every signature fail raw-body verification.
+        if webhook_secret and self._verify_secret_strength(webhook_secret):
+            signed_headers, body_bytes = sign_legacy_webhook(webhook_secret, payload, timestamp=timestamp)
+            # X-AdCP-Signature: sha256=<hex> + X-AdCP-Timestamp (unix seconds)
+            headers.update(signed_headers)
+        else:
+            if webhook_secret:
                 logger.warning(f"⚠️ Webhook secret for {config.url} is too weak (min 32 characters required)")
-            else:
-                signature = self._generate_hmac_signature(payload, webhook_secret, timestamp)
-                headers["X-ADCP-Signature"] = signature
+            body_bytes = json.dumps(payload, separators=(",", ":")).encode("utf-8")
+            headers["X-ADCP-Timestamp"] = str(timestamp)  # For replay prevention
 
         # Add authentication
         if config.authentication_type == "bearer" and config.authentication_token:
@@ -481,9 +467,10 @@ def _deliver_with_backoff(
 
                 # Send webhook
                 with httpx.Client(timeout=10.0) as client:
+                    # content= (not json=) so the signed bytes ARE the sent bytes
                     response = client.post(
                         config.url,
-                        json=payload,
+                        content=body_bytes,
                         headers=headers,
                     )