Skip to content

Commit

Permalink
Differentiate logging information.
Browse files Browse the repository at this point in the history 
Some of the logging information in snare tells the user they've
misconfigured snare or GitHub, but some logging information is really
meant to help someone debug a change in GitHub requests. This commit
teases those two out. By making HTTP request functions take `TcpStream`
(i.e. moving it) the terminal part of this function is now a little
harder to misuse, and also allows us to restore more informative HTTP
return codes (e.g. "unauthorised").
  • Loading branch information
@ltratt
ltratt committed Dec 25, 2023
1 parent 45666ba commit 119a176
Show file tree
Hide file tree
Showing 2 changed files with 98 additions and 40 deletions.
134 changes: 96 additions & 38 deletions src/httpserver.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ use std::{
};

use hmac::{Hmac, Mac};
use log::info;
use log::{error, trace};
use percent_encoding::percent_decode;
use secstr::SecStr;
use sha2::Sha256;
Expand All @@ -38,7 +38,7 @@ pub(crate) fn serve(snare: Arc<Snare>) -> Result<(), Box<dyn Error>> {
}

let active = Arc::new(AtomicUsize::new(0));
for mut stream in listener.incoming().flatten() {
for stream in listener.incoming().flatten() {
// We want to keep a limit on how many threads are started concurrently, so that an
// attacker can't DOS the machine. `active` keeps track of how many threads are (or are
// just about to be) active. Since the common case is that we haven't hit the limit, we
Expand All @@ -57,61 +57,108 @@ pub(crate) fn serve(snare: Arc<Snare>) -> Result<(), Box<dyn Error>> {
let active = Arc::clone(&active);
let snare = Arc::clone(&snare);
thread::spawn(move || {
match request(&snare, &mut stream) {
Ok(()) => {
http_200(&mut stream);
}
Err(e) => {
info!("Couldn't process HTTP request: {e}");
http_400(&mut stream)
}
}
request(&snare, stream);
active.fetch_sub(1, Ordering::Relaxed);
});
}
Ok(())
}

fn request(snare: &Arc<Snare>, stream: &mut TcpStream) -> Result<(), Box<dyn Error>> {
stream.set_read_timeout(Some(NET_TIMEOUT))?;
stream.set_write_timeout(Some(NET_TIMEOUT))?;
/// Try processing an HTTP request.
fn request(snare: &Arc<Snare>, mut stream: TcpStream) {
match (
stream.set_read_timeout(Some(NET_TIMEOUT)),
stream.set_write_timeout(Some(NET_TIMEOUT)),
) {
(Ok(_), Ok(_)) => (),
_ => {
error!("Couldn't set timeout on sockets");
http_500(stream);
return;
}
}
let req_time = Instant::now();
let (headers, body) = parse_get(stream)?;
stream.shutdown(Shutdown::Read)?;
let (headers, body) = match parse_get(&mut stream) {
Ok(x) => x,
Err(e) => {
trace!("Processing HTTP request: {e}");
http_400(stream);
return;
}
};
if stream.shutdown(Shutdown::Read).is_err() {
http_400(stream);
return;
}

let event_type = headers
.get("x-github-event")
.ok_or_else(|| "X-Github-Event header missing".to_owned())?;
let event_type = match headers.get("x-github-event") {
Some(x) => x,
None => {
trace!("HTTP request: X-Github-Event header missing");
http_400(stream);
return;
}
};
if !valid_github_event(event_type) {
return Err("Invalid event type".into());
error!("Invalid GitHub event type '{event_type}'");
http_400(stream);
return;
}
let sig = match headers
.get("x-hub-signature-256")
.and_then(|s| s.split_once('='))
{
Some(("sha256", sig)) => Some(sig),
Some(_) => return Err("Incorrectly formatted X-Hub-Signature-256 header".into()),
Some(_) => {
trace!("Incorrectly formatted X-Hub-Signature-256 header");
http_400(stream);
return;
}
None => None,
};

if !body.starts_with("payload=".as_bytes()) {
return Err("Payload does not start with 'payload='".into());
trace!("Payload does not start with 'payload='");
http_400(stream);
return;
}
let json_str = percent_decode(&body[8..]).decode_utf8()?.to_string();
let jv = serde_json::from_str::<serde_json::Value>(&json_str)?;
let json_str = match percent_decode(&body[8..]).decode_utf8() {
Ok(x) => x.to_string(),
Err(_) => {
trace!("JSON not valid UTF-8");
http_400(stream);
return;
}
};
let jv = match serde_json::from_str::<serde_json::Value>(&json_str) {
Ok(x) => x,
Err(e) => {
trace!("Can't parse JSON: {e}");
http_400(stream);
return;
}
};
let (owner, repo) = match (
&jv["repository"]["owner"]["login"].as_str(),
&jv["repository"]["name"].as_str(),
) {
(Some(o), Some(r)) => (o.to_owned(), r.to_owned()),
_ => return Err("Invalid JSON".into()),
_ => {
trace!("Invalid JSON");
http_400(stream);
return;
}
};

if !valid_github_ownername(owner) {
return Err(format!("Invalid GitHub owner '{}'.", &owner).into());
trace!("Invalid GitHub owner syntax '{owner}'.");
http_400(stream);
return;
}
if !valid_github_reponame(repo) {
return Err(format!("Invalid GitHub repository '{}'.", &repo).into());
trace!("Invalid GitHub repository syntax '{repo}'.");
http_400(stream);
return;
}

let conf = snare.conf.lock().unwrap();
Expand All @@ -120,25 +167,27 @@ fn request(snare: &Arc<Snare>, stream: &mut TcpStream) -> Result<(), Box<dyn Err
match (secret, sig) {
(Some(secret), Some(sig)) => {
if !authenticate(secret, sig, &body) {
return Err(format!("Authentication failed for {}/{}.", owner, repo).into());
error!("Authentication failed for {owner}/{repo}.");
http_401(stream);
return;
}
}
(Some(_), None) => {
return Err("Secret specified but request unsigned".into());
error!("Secret specified but request unsigned");
http_401(stream);
return;
}
(None, Some(_)) => {
return Err(format!(
"Request was signed but no secret was specified for {}/{}.",
owner, repo
)
.into());
error!("Request was signed but no secret was specified for {owner}/{repo}.");
http_401(stream);
return;
}
(None, None) => (),
}
drop(conf);

if event_type == "ping" {
return Ok(());
return;
}

let repo_id = format!("github/{}/{}", owner, repo);
Expand All @@ -156,7 +205,8 @@ fn request(snare: &Arc<Snare>, stream: &mut TcpStream) -> Result<(), Box<dyn Err
// thread will be notified anyway. If something else happens to have gone wrong, then
// we (and the OS) are probably in deep trouble anyway...
nix::unistd::write(snare.event_write_fd, &[0]).ok();
Ok(())

http_200(stream);
}

/// A very literal, and rather unforgiving, implementation of RFC2616 (HTTP/1.1), returning the URL
Expand Down Expand Up @@ -220,14 +270,22 @@ fn parse_get(stream: &mut TcpStream) -> Result<(HashMap<String, String>, Vec<u8>
Ok((headers_map, body))
}

fn http_200(stream: &mut TcpStream) {
fn http_200(mut stream: TcpStream) {
stream.write_all(b"HTTP/1.1 200 OK\r\n\r\n").ok();
}

fn http_400(stream: &mut TcpStream) {
fn http_400(mut stream: TcpStream) {
stream.write_all(b"HTTP/1.1 400\r\n\r\n").ok();
}

fn http_401(mut stream: TcpStream) {
stream.write_all(b"HTTP/1.1 401\r\n\r\n").ok();
}

fn http_500(mut stream: TcpStream) {
stream.write_all(b"HTTP/1.1 500\r\n\r\n").ok();
}

/// Authenticate this request and if successful return `true` (where "success" also includes "the
/// user didn't specify a secret for this repository").
fn authenticate(secret: &SecStr, sig: &str, pl: &[u8]) -> bool {
Expand Down
4 changes: 2 additions & 2 deletions tests/auth.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -103,7 +103,7 @@ fn bad_sha256() -> Result<(), Box<dyn Error>> {
&[(
move |port| Ok(req(port, false)),
move |response| {
if response.starts_with("HTTP/1.1 400") {
if response.starts_with("HTTP/1.1 401") {
sleep(SNARE_PAUSE);
assert!(!tp.is_file());
Ok(())
Expand All @@ -129,7 +129,7 @@ fn wrong_secret() -> Result<(), Box<dyn Error>> {
&[(
move |port| Ok(req(port, true)),
move |response| {
if response.starts_with("HTTP/1.1 400") {
if response.starts_with("HTTP/1.1 401") {
sleep(SNARE_PAUSE);
assert!(!tp.is_file());
Ok(())
Expand Down

0 comments on commit 119a176

Please sign in to comment.