From ce4e92fdda5ef2b124accbe4d85f9733c57d2635 Mon Sep 17 00:00:00 2001 From: Paul Campbell Date: Mon, 20 May 2024 20:41:01 +0100 Subject: [PATCH] WIP: fix invalid webhook authorisations --- crates/server/src/actors/repo/webhook.rs | 27 +++--- crates/server/src/actors/webhook/message.rs | 34 +++++--- crates/server/src/actors/webhook/router.rs | 4 +- crates/server/src/actors/webhook/server.rs | 92 +++++++++++++-------- 4 files changed, 95 insertions(+), 62 deletions(-) diff --git a/crates/server/src/actors/repo/webhook.rs b/crates/server/src/actors/repo/webhook.rs index 3dc1e4c..9e079fe 100644 --- a/crates/server/src/actors/repo/webhook.rs +++ b/crates/server/src/actors/repo/webhook.rs @@ -24,8 +24,9 @@ pub struct WebhookId(String); pub struct WebhookAuth(ulid::Ulid); impl WebhookAuth { pub fn from_str(authorisation: &str) -> Result { - let id = ulid::Ulid::from_str(authorisation); - Ok(Self(id?)) + let id = ulid::Ulid::from_str(authorisation)?; + info!("Parse auth token: {}", id); + Ok(Self(id)) } fn generate() -> Self { @@ -186,11 +187,7 @@ impl Handler for RepoActor { warn!("Don't know what authorization to expect"); return; }; - let Some(received_authorization) = &msg.authorisation() else { - warn!("Missing authorization token"); - return; - }; - if received_authorization != expected_authorization { + if msg.authorisation() != expected_authorization { warn!( "Invalid authorization - expected {}", expected_authorization @@ -198,11 +195,11 @@ impl Handler for RepoActor { return; } let id = msg.id(); - let span = tracing::info_span!("handle", %id); + let span = tracing::info_span!("handle", ?id); let _guard = span.enter(); let body = msg.body(); - match serde_json::from_str::(body) { - Err(err) => warn!(?err, %body, "Not a 'push'"), + match serde_json::from_str::(body.as_str()) { + Err(err) => warn!(?err, ?body, "Not a 'push'"), Ok(push) => { if let Some(config) = &self.details.repo_config { match push.branch(config.branches()) { @@ -262,6 +259,10 @@ impl Handler for RepoActor { } } +pub fn split_ref(reference: &str) -> (&str, &str) { + reference.split_at(11) +} + #[derive(Debug, serde::Deserialize)] struct Push { #[serde(rename = "ref")] @@ -298,12 +299,8 @@ impl Push { } } -pub fn split_ref(reference: &str) -> (&str, &str) { - reference.split_at(11) -} - #[derive(Debug)] -enum Branch { +pub enum Branch { Main, Next, Dev, diff --git a/crates/server/src/actors/webhook/message.rs b/crates/server/src/actors/webhook/message.rs index 92551a9..472fa3f 100644 --- a/crates/server/src/actors/webhook/message.rs +++ b/crates/server/src/actors/webhook/message.rs @@ -1,27 +1,41 @@ // use actix::prelude::*; +use git_next_config::RepoAlias; +use ulid::Ulid; use crate::actors::repo::webhook::WebhookAuth; #[derive(Message, Debug, Clone, derive_more::Constructor)] #[rtype(result = "()")] pub struct WebhookMessage { - id: String, - path: String, - authorisation: String, - body: String, + id: Id, + // forge // TODO: differentiate between multiple forges + repo_alias: RepoAlias, + authorisation: WebhookAuth, + body: Body, } impl WebhookMessage { - pub const fn id(&self) -> &String { + pub const fn id(&self) -> &Id { &self.id } - pub const fn path(&self) -> &String { - &self.path + pub const fn repo_alias(&self) -> &RepoAlias { + &self.repo_alias } - pub const fn body(&self) -> &String { + pub const fn body(&self) -> &Body { &self.body } - pub fn authorisation(&self) -> Option { - WebhookAuth::from_str(&self.authorisation).ok() + pub const fn authorisation(&self) -> &WebhookAuth { + &self.authorisation + } +} + +#[derive(Clone, Copy, Debug, derive_more::Constructor)] +pub struct Id(Ulid); + +#[derive(Clone, Debug, derive_more::Constructor)] +pub struct Body(String); +impl Body { + pub fn as_str(&self) -> &str { + self.0.as_str() } } diff --git a/crates/server/src/actors/webhook/router.rs b/crates/server/src/actors/webhook/router.rs index 381d23f..e5a9210 100644 --- a/crates/server/src/actors/webhook/router.rs +++ b/crates/server/src/actors/webhook/router.rs @@ -28,9 +28,9 @@ impl Handler for WebhookRouter { fn handle(&mut self, msg: WebhookMessage, _ctx: &mut Self::Context) -> Self::Result { let _gaurd = self.span.enter(); - let repo_alias = RepoAlias::new(msg.path()); + let repo_alias = msg.repo_alias(); debug!(repo = %repo_alias, "Router..."); - if let Some(recipient) = self.repos.get(&repo_alias) { + if let Some(recipient) = self.repos.get(repo_alias) { info!(repo = %repo_alias, "Sending to Recipient"); recipient.do_send(msg); } diff --git a/crates/server/src/actors/webhook/server.rs b/crates/server/src/actors/webhook/server.rs index 78dad89..ea7ecac 100644 --- a/crates/server/src/actors/webhook/server.rs +++ b/crates/server/src/actors/webhook/server.rs @@ -2,13 +2,18 @@ use std::net::SocketAddr; use actix::prelude::*; +use git_next_config::RepoAlias; use tracing::{info, warn}; +use ulid::Ulid; +use warp::reject::Rejection; -use crate::actors::webhook::message::WebhookMessage; +use crate::actors::{repo::webhook::WebhookAuth, webhook::message::WebhookMessage}; + +use super::message; pub async fn start( socket_addr: SocketAddr, - address: actix::prelude::Recipient, + address: actix::prelude::Recipient, ) { // start webhook server use warp::Filter; @@ -21,45 +26,38 @@ pub async fn start( .and(warp::body::bytes()) .and_then( |recipient: Recipient, - path, + path: String, // query: String, headers: warp::http::HeaderMap, body: bytes::Bytes| async move { info!("POST received"); + let repo_alias = RepoAlias::new(path); let bytes = body.to_vec(); - let request_data = String::from_utf8_lossy(&bytes).to_string(); - let id = ulid::Ulid::new().to_string(); + let body = message::Body::new(String::from_utf8_lossy(&bytes).to_string()); + let id = message::Id::new(Ulid::new()); match headers.get("Authorization") { - Some(auhorisation) => { - info!(id, path, "Received webhook"); - let authorisation = auhorisation - .to_str() - .map_err(|e| { - warn!("Invalid value in authorization: {:?}", e); - warp::reject() - })? // valid characters - .strip_prefix("Basic ") - .ok_or_else(|| { - warn!("Authorization must be 'Basic'"); - warp::reject() - })? // must start with "Basic " - .to_string(); - let message = WebhookMessage::new( - id, - path, - /* query, headers, */ request_data, - authorisation, - ); - recipient - .try_send(message) - .map(|_| { - info!("Message sent ok"); - warp::reply::with_status("OK", warp::http::StatusCode::OK) - }) - .map_err(|e| { - warn!("Unknown error: {:?}", e); - warp::reject() - }) + Some(authorisation_header) => { + info!(?id, ?repo_alias, ?authorisation_header, "Received webhook",); + match parse_auth(authorisation_header) { + Ok(authorisation) => { + let message = + WebhookMessage::new(id, repo_alias, authorisation, body); + recipient + .try_send(message) + .map(|_| { + info!("Message sent ok"); + warp::reply::with_status("OK", warp::http::StatusCode::OK) + }) + .map_err(|e| { + warn!("Unknown error: {:?}", e); + warp::reject() + }) + } + Err(e) => { + warn!(?e, "Failed to decode authorization header"); + Err(warp::reject()) + } + } } _ => { warn!("No Authorization header"); @@ -73,3 +71,27 @@ pub async fn start( info!("Starting webhook server: {}", socket_addr); warp::serve(route).run(socket_addr).await; } + +fn parse_auth(authorization_header: &warp::http::HeaderValue) -> Result { + WebhookAuth::from_str( + authorization_header + .to_str() + .map_err(|e| { + warn!("Invalid non-ascii value in authorization: {:?}", e); + warp::reject() + }) // valid characters + .map(|v| { + info!("raw auth header: {}", v); + v + })? + .strip_prefix("Basic ") + .ok_or_else(|| { + warn!("Authorization must be 'Basic'"); + warp::reject() + })?, // must start with "Basic " + ) + .map_err(|e| { + warn!(?e, "decode error"); + warp::reject() + }) +}