fix(security): migrate webhook auth to HMAC-SHA256 signature header (#970)

src/channels/http.rs

@@ -6,12 +6,15 @@ use async_trait::async_trait;
 use axum::{
     Json, Router,
     extract::{DefaultBodyLimit, State},
-    http::StatusCode,
+    http::{HeaderMap, StatusCode},
     response::IntoResponse,
     routing::{get, post},
 };
+use bytes::Bytes;
+use hmac::{Hmac, Mac};
 use secrecy::{ExposeSecret, SecretString};
 use serde::{Deserialize, Serialize};
+use sha2::Sha256;
 use subtle::ConstantTimeEq;
 use tokio::sync::{RwLock, mpsc, oneshot};
 use tokio_stream::wrappers::ReceiverStream;
@@ -24,6 +27,8 @@ use crate::channels::{
 use crate::config::HttpConfig;
 use crate::error::ChannelError;
 
+type HmacSha256 = Hmac<Sha256>;
+
 /// HTTP webhook channel.
 pub struct HttpChannel {
     config: HttpConfig,
@@ -135,7 +140,8 @@ struct WebhookRequest {
     content: String,
     /// Optional thread ID for conversation tracking.
     thread_id: Option<String>,
-    /// Optional webhook secret for authentication.
+    /// Deprecated: webhook secret in request body. Use X-IronClaw-Signature header instead.
+    /// This field is accepted for backward compatibility but will be removed in a future release.
     secret: Option<String>,
     /// Whether to wait for a synchronous response.
     #[serde(default)]
@@ -191,10 +197,36 @@ async fn health_handler() -> impl IntoResponse {
     })
 }
 
+/// Verify an HMAC-SHA256 signature against the raw request body.
+///
+/// The expected header format is: `sha256=<hex_digest>`
+/// where the digest is HMAC-SHA256(secret_key, body_bytes) encoded as lowercase hex.
+fn verify_hmac_signature(secret: &str, body: &[u8], signature_header: &str) -> bool {
+    let hex_digest = match signature_header.strip_prefix("sha256=") {
+        Some(h) => h,
+        None => return false,
+    };
+
+    let provided_mac = match hex::decode(hex_digest) {
+        Ok(bytes) => bytes,
+        Err(_) => return false,
+    };
+
+    let mut mac = match HmacSha256::new_from_slice(secret.as_bytes()) {
+        Ok(mac) => mac,
+        Err(_) => return false,
+    };
+    mac.update(body);
+    let expected_mac = mac.finalize().into_bytes();
+
+    bool::from(expected_mac.as_slice().ct_eq(&provided_mac))
+}
+
 async fn webhook_handler(
     State(state): State<Arc<HttpChannelState>>,
-    Json(req): Json<WebhookRequest>,
-) -> (StatusCode, Json<WebhookResponse>) {
+    headers: HeaderMap,
+    body: Bytes,
+) -> impl IntoResponse {
     // Rate limiting
     {
         let mut limiter = state.rate_limit.lock().await;
@@ -211,10 +243,153 @@ async fn webhook_handler(
                     status: "error".to_string(),
                     response: Some("Rate limit exceeded".to_string()),
                 }),
-            );
+            )
+                .into_response();
         }
     }
 
+    let content_type_ok = headers
+        .get("content-type")
+        .and_then(|value| value.to_str().ok())
+        .map(|value| value.starts_with("application/json"))
+        .unwrap_or(false);
+
+    if !content_type_ok {
+        return (
+            StatusCode::UNSUPPORTED_MEDIA_TYPE,
+            Json(WebhookResponse {
+                message_id: Uuid::nil(),
+                status: "error".to_string(),
+                response: Some("Content-Type must be application/json".to_string()),
+            }),
+        )
+            .into_response();
+    }
+
+    let mut fallback_req = None;
+    {
+        let webhook_secret = state.webhook_secret.read().await;
+        if let Some(expected_secret) = webhook_secret.as_ref() {
+            let expected_secret = expected_secret.expose_secret();
+
+            match headers.get("x-ironclaw-signature") {
+                Some(raw_signature) => match raw_signature.to_str() {
+                    Ok(signature) => {
+                        if !verify_hmac_signature(expected_secret, &body, signature) {
+                            return (
+                                StatusCode::UNAUTHORIZED,
+                                Json(WebhookResponse {
+                                    message_id: Uuid::nil(),
+                                    status: "error".to_string(),
+                                    response: Some("Invalid webhook signature".to_string()),
+                                }),
+                            )
+                                .into_response();
+                        }
+                    }
+                    Err(_) => {
+                        return (
+                            StatusCode::UNAUTHORIZED,
+                            Json(WebhookResponse {
+                                message_id: Uuid::nil(),
+                                status: "error".to_string(),
+                                response: Some("Invalid signature header encoding".to_string()),
+                            }),
+                        )
+                            .into_response();
+                    }
+                },
+                None => {
+                    let req: WebhookRequest = match serde_json::from_slice(&body) {
+                        Ok(req) => req,
+                        Err(_) => {
+                            return (
+                                StatusCode::UNAUTHORIZED,
+                                Json(WebhookResponse {
+                                    message_id: Uuid::nil(),
+                                    status: "error".to_string(),
+                                    response: Some(
+                                        "Webhook authentication required. Provide X-IronClaw-Signature header \
+                                         (preferred) or 'secret' field in body (deprecated)."
+                                            .to_string(),
+                                    ),
+                                }),
+                            )
+                                .into_response();
+                        }
+                    };
+
+                    match &req.secret {
+                        Some(provided)
+                            if bool::from(
+                                provided.as_bytes().ct_eq(expected_secret.as_bytes()),
+                            ) =>
+                        {
+                            tracing::warn!(
+                                "Webhook authenticated via deprecated 'secret' field in request body. \
+                                 Migrate to X-IronClaw-Signature header (HMAC-SHA256). \
+                                 Body secret support will be removed in a future release."
+                            );
+                            fallback_req = Some(req);
+                        }
+                        Some(_) => {
+                            return (
+                                StatusCode::UNAUTHORIZED,
+                                Json(WebhookResponse {
+                                    message_id: Uuid::nil(),
+                                    status: "error".to_string(),
+                                    response: Some("Invalid webhook secret".to_string()),
+                                }),
+                            )
+                                .into_response();
+                        }
+                        None => {
+                            return (
+                                StatusCode::UNAUTHORIZED,
+                                Json(WebhookResponse {
+                                    message_id: Uuid::nil(),
+                                    status: "error".to_string(),
+                                    response: Some(
+                                        "Webhook authentication required. Provide X-IronClaw-Signature header \
+                                         (preferred) or 'secret' field in body (deprecated)."
+                                            .to_string(),
+                                    ),
+                                }),
+                            )
+                                .into_response();
+                        }
+                    }
+                }
+            }
+        }
+    }
+
+    if let Some(req) = fallback_req {
+        return process_authenticated_request(state, req).await;
+    }
+
+    let req: WebhookRequest = match serde_json::from_slice(&body) {
+        Ok(req) => req,
+        Err(e) => {
+            return (
+                StatusCode::BAD_REQUEST,
+                Json(WebhookResponse {
+                    message_id: Uuid::nil(),
+                    status: "error".to_string(),
+                    response: Some(format!("Invalid JSON: {e}")),
+                }),
+            )
+                .into_response();
+        }
+    };
+
+    process_authenticated_request(state, req).await
+}
+
+async fn process_authenticated_request(
+    state: Arc<HttpChannelState>,
+    req: WebhookRequest,
+) -> axum::response::Response {
     let _ = req.user_id.as_ref().map(|user_id| {
         tracing::debug!(
             provided_user_id = %user_id,
@@ -222,36 +397,6 @@ async fn webhook_handler(
         );
     });
 
-    // Validate secret if configured
-    if let Some(ref expected_secret) = *state.webhook_secret.read().await {
-        let expected_bytes = expected_secret.expose_secret().as_bytes();
-        match &req.secret {
-            Some(provided) if bool::from(provided.as_bytes().ct_eq(expected_bytes)) => {
-                // Secret matches, continue
-            }
-            Some(_) => {
-                return (
-                    StatusCode::UNAUTHORIZED,
-                    Json(WebhookResponse {
-                        message_id: Uuid::nil(),
-                        status: "error".to_string(),
-                        response: Some("Invalid webhook secret".to_string()),
-                    }),
-                );
-            }
-            None => {
-                return (
-                    StatusCode::UNAUTHORIZED,
-                    Json(WebhookResponse {
-                        message_id: Uuid::nil(),
-                        status: "error".to_string(),
-                        response: Some("Webhook secret required".to_string()),
-                    }),
-                );
-            }
-        }
-    }
-
     if req.content.len() > MAX_CONTENT_BYTES {
         return (
             StatusCode::PAYLOAD_TOO_LARGE,
@@ -260,10 +405,12 @@ async fn webhook_handler(
                 status: "error".to_string(),
                 response: Some("Content too large".to_string()),
             }),
-        );
+        )
+            .into_response();
     }
 
-    // Validate and decode attachments
+    let wait_for_response = req.wait_for_response;
+
     let attachments = if !req.attachments.is_empty() {
         if req.attachments.len() > MAX_ATTACHMENTS {
             return (
@@ -273,7 +420,8 @@ async fn webhook_handler(
                     status: "error".to_string(),
                     response: Some(format!("Too many attachments (max {})", MAX_ATTACHMENTS)),
                 }),
-            );
+            )
+                .into_response();
         }
 
         let mut decoded_attachments = Vec::new();
@@ -291,7 +439,8 @@ async fn webhook_handler(
                                 status: "error".to_string(),
                                 response: Some("Invalid base64 in attachment".to_string()),
                             }),
-                        );
+                        )
+                            .into_response();
                     }
                 };
                 if data.len() > MAX_ATTACHMENT_BYTES {
@@ -305,7 +454,8 @@ async fn webhook_handler(
                                 MAX_ATTACHMENT_BYTES
                             )),
                         }),
-                    );
+                    )
+                        .into_response();
                 }
                 total_bytes += data.len();
                 if total_bytes > MAX_TOTAL_ATTACHMENT_BYTES {
@@ -316,7 +466,8 @@ async fn webhook_handler(
                             status: "error".to_string(),
                             response: Some("Total attachment size exceeds limit".to_string()),
                         }),
-                    );
+                    )
+                        .into_response();
                 }
                 decoded_attachments.push(IncomingAttachment {
                     id: Uuid::new_v4().to_string(),
@@ -331,7 +482,6 @@ async fn webhook_handler(
                     duration_secs: None,
                 });
             } else if let Some(ref url) = att.url {
-                // URL-only attachment: set source_url but don't download (SSRF prevention)
                 decoded_attachments.push(IncomingAttachment {
                     id: Uuid::new_v4().to_string(),
                     kind: AttachmentKind::from_mime_type(&att.mime_type),
@@ -353,7 +503,7 @@ async fn webhook_handler(
 
     let mut msg = IncomingMessage::new("http", &state.user_id, &req.content).with_metadata(
         serde_json::json!({
-            "wait_for_response": req.wait_for_response,
+            "wait_for_response": wait_for_response,
         }),
     );
 
@@ -365,7 +515,9 @@ async fn webhook_handler(
         msg = msg.with_thread(thread_id);
     }
 
-    process_message(state, msg, req.wait_for_response).await
+    process_message(state, msg, wait_for_response)
+        .await
+        .into_response()
 }
 
 async fn process_message(
@@ -515,7 +667,7 @@ impl ChannelSecretUpdater for HttpChannelState {
 #[cfg(test)]
 mod tests {
     use axum::body::Body;
-    use axum::http::Request;
+    use axum::http::{HeaderValue, Request};
     use secrecy::SecretString;
     use tower::ServiceExt;
 
@@ -530,6 +682,14 @@ mod tests {
         })
     }
 
+    fn compute_signature(secret: &str, body: &[u8]) -> String {
+        let mut mac =
+            HmacSha256::new_from_slice(secret.as_bytes()).expect("HMAC key creation failed");
+        mac.update(body);
+        let result = mac.finalize().into_bytes();
+        format!("sha256={}", hex::encode(result))
+    }
+
     #[tokio::test]
     async fn test_http_channel_requires_secret() {
         let channel = test_channel(None);
@@ -538,9 +698,76 @@ mod tests {
     }
 
     #[tokio::test]
-    async fn webhook_correct_secret_returns_ok() {
+    async fn webhook_hmac_signature_returns_ok() {
+        let secret = "test-secret-123";
+        let channel = test_channel(Some(secret));
+        let _stream = channel.start().await.unwrap();
+        let app = channel.routes();
+
+        let body = serde_json::json!({
+            "content": "hello"
+        });
+        let body_bytes = serde_json::to_vec(&body).unwrap();
+        let signature = compute_signature(secret, &body_bytes);
+        let req = Request::builder()
+            .method("POST")
+            .uri("/webhook")
+            .header("content-type", "application/json")
+            .header("x-ironclaw-signature", signature)
+            .body(Body::from(body_bytes))
+            .unwrap();
+
+        let resp = app.oneshot(req).await.unwrap();
+        assert_eq!(resp.status(), StatusCode::OK);
+    }
+
+    #[tokio::test]
+    async fn webhook_wrong_hmac_signature_returns_unauthorized() {
+        let channel = test_channel(Some("correct-secret"));
+        let _stream = channel.start().await.unwrap();
+        let app = channel.routes();
+
+        let body = serde_json::json!({
+            "content": "hello"
+        });
+        let body_bytes = serde_json::to_vec(&body).unwrap();
+        let signature = compute_signature("wrong-secret", &body_bytes);
+        let req = Request::builder()
+            .method("POST")
+            .uri("/webhook")
+            .header("content-type", "application/json")
+            .header("x-ironclaw-signature", signature)
+            .body(Body::from(body_bytes))
+            .unwrap();
+
+        let resp = app.oneshot(req).await.unwrap();
+        assert_eq!(resp.status(), StatusCode::UNAUTHORIZED);
+    }
+
+    #[tokio::test]
+    async fn webhook_malformed_signature_returns_unauthorized() {
+        let channel = test_channel(Some("correct-secret"));
+        let _stream = channel.start().await.unwrap();
+        let app = channel.routes();
+
+        let body = serde_json::json!({
+            "content": "hello"
+        });
+        let req = Request::builder()
+            .method("POST")
+            .uri("/webhook")
+            .header("content-type", "application/json")
+            .header("x-ironclaw-signature", "not-a-valid-signature")
+            .body(Body::from(serde_json::to_vec(&body).unwrap()))
+            .unwrap();
+
+        let resp = app.oneshot(req).await.unwrap();
+        assert_eq!(resp.status(), StatusCode::UNAUTHORIZED);
+    }
+
+    #[tokio::test]
+    async fn webhook_deprecated_body_secret_still_works() {
         let channel = test_channel(Some("test-secret-123"));
-        // Start the channel so the tx sender is populated (otherwise 503).
         let _stream = channel.start().await.unwrap();
         let app = channel.routes();
 
@@ -560,7 +787,7 @@ mod tests {
     }
 
     #[tokio::test]
-    async fn webhook_wrong_secret_returns_unauthorized() {
+    async fn webhook_wrong_body_secret_returns_unauthorized() {
         let channel = test_channel(Some("correct-secret"));
         let _stream = channel.start().await.unwrap();
         let app = channel.routes();
@@ -581,7 +808,7 @@ mod tests {
     }
 
     #[tokio::test]
-    async fn webhook_missing_secret_returns_unauthorized() {
+    async fn webhook_missing_all_auth_returns_unauthorized() {
         let channel = test_channel(Some("correct-secret"));
         let _stream = channel.start().await.unwrap();
         let app = channel.routes();
@@ -600,6 +827,104 @@ mod tests {
         assert_eq!(resp.status(), StatusCode::UNAUTHORIZED);
     }
 
+    #[tokio::test]
+    async fn webhook_hmac_takes_precedence_over_body_secret() {
+        let secret = "test-secret-123";
+        let channel = test_channel(Some(secret));
+        let _stream = channel.start().await.unwrap();
+        let app = channel.routes();
+
+        let body = serde_json::json!({
+            "content": "hello",
+            "secret": "wrong-secret-in-body"
+        });
+        let body_bytes = serde_json::to_vec(&body).unwrap();
+        let signature = compute_signature(secret, &body_bytes);
+
+        let req = Request::builder()
+            .method("POST")
+            .uri("/webhook")
+            .header("content-type", "application/json")
+            .header("x-ironclaw-signature", signature)
+            .body(Body::from(body_bytes))
+            .unwrap();
+
+        let resp = app.oneshot(req).await.unwrap();
+        assert_eq!(resp.status(), StatusCode::OK);
+    }
+
+    #[tokio::test]
+    async fn webhook_invalid_json_returns_bad_request() {
+        let secret = "test-secret";
+        let channel = test_channel(Some(secret));
+        let _stream = channel.start().await.unwrap();
+        let app = channel.routes();
+
+        let body = b"not json".to_vec();
+        let signature = compute_signature(secret, &body);
+
+        let req = Request::builder()
+            .method("POST")
+            .uri("/webhook")
+            .header("content-type", "application/json")
+            .header("x-ironclaw-signature", signature)
+            .body(Body::from(body))
+            .unwrap();
+
+        let resp = app.oneshot(req).await.unwrap();
+        assert_eq!(resp.status(), StatusCode::BAD_REQUEST);
+    }
+
+    #[tokio::test]
+    async fn webhook_rejects_non_json_content_type() {
+        let secret = "test-secret";
+        let channel = test_channel(Some(secret));
+        let _stream = channel.start().await.unwrap();
+        let app = channel.routes();
+
+        let body = serde_json::json!({
+            "content": "hello"
+        });
+        let body_bytes = serde_json::to_vec(&body).unwrap();
+        let signature = compute_signature(secret, &body_bytes);
+
+        let req = Request::builder()
+            .method("POST")
+            .uri("/webhook")
+            .header("content-type", "text/plain")
+            .header("x-ironclaw-signature", signature)
+            .body(Body::from(body_bytes))
+            .unwrap();
+
+        let resp = app.oneshot(req).await.unwrap();
+        assert_eq!(resp.status(), StatusCode::UNSUPPORTED_MEDIA_TYPE);
+    }
+
+    #[tokio::test]
+    async fn webhook_invalid_signature_header_encoding_returns_unauthorized() {
+        let channel = test_channel(Some("test-secret"));
+        let _stream = channel.start().await.unwrap();
+        let app = channel.routes();
+
+        let body = serde_json::json!({
+            "content": "hello"
+        });
+
+        let mut req = Request::builder()
+            .method("POST")
+            .uri("/webhook")
+            .header("content-type", "application/json")
+            .body(Body::from(serde_json::to_vec(&body).unwrap()))
+            .unwrap();
+        req.headers_mut().insert(
+            "x-ironclaw-signature",
+            HeaderValue::from_bytes(b"\xFF").unwrap(),
+        );
+
+        let resp = app.oneshot(req).await.unwrap();
+        assert_eq!(resp.status(), StatusCode::UNAUTHORIZED);
+    }
+
     #[tokio::test]
     async fn test_update_secret_hot_swap() {
         let channel = test_channel(Some("old-secret"));
@@ -751,4 +1076,37 @@ mod tests {
             "All concurrent requests should succeed with correct secrets after update"
         );
     }
+
+    #[test]
+    fn verify_hmac_signature_valid() {
+        let secret = "my-secret";
+        let body = b"test body content";
+        let sig = compute_signature(secret, body);
+        assert!(verify_hmac_signature(secret, body, &sig));
+    }
+
+    #[test]
+    fn verify_hmac_signature_invalid_digest() {
+        let secret = "my-secret";
+        let body = b"test body content";
+        assert!(!verify_hmac_signature(
+            secret,
+            body,
+            "sha256=0000000000000000000000000000000000000000000000000000000000000000"
+        ));
+    }
+
+    #[test]
+    fn verify_hmac_signature_missing_prefix() {
+        let secret = "my-secret";
+        let body = b"test body content";
+        assert!(!verify_hmac_signature(secret, body, "deadbeef"));
+    }
+
+    #[test]
+    fn verify_hmac_signature_invalid_hex() {
+        let secret = "my-secret";
+        let body = b"test body content";
+        assert!(!verify_hmac_signature(secret, body, "sha256=not-hex!"));
+    }
 }