diff --git a/Cargo.lock b/Cargo.lock index 53c925a..25c0365 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -3198,6 +3198,10 @@ dependencies = [ "openfut-adapter-fifa17", ] +[[package]] +name = "openfut-http" +version = "0.1.0" + [[package]] name = "openfut-launcher" version = "0.1.0" @@ -3226,6 +3230,7 @@ version = "0.1.0" dependencies = [ "openfut-adapter-fifa17", "openfut-host-config", + "openfut-http", "openfut-tls", "openssl", ] @@ -3236,6 +3241,7 @@ version = "0.1.0" dependencies = [ "openfut-adapter-fifa17", "openfut-host-config", + "openfut-http", "openfut-tls", ] diff --git a/Cargo.toml b/Cargo.toml index 0ddb683..5ace1df 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -6,6 +6,7 @@ members = [ "openfut-adapter-fifa17", "openfut-blaze-host", "openfut-host-config", + "openfut-http", "openfut-tls", "openfut-redirector-host", "openfut-roster-host", diff --git a/openfut-http/Cargo.toml b/openfut-http/Cargo.toml new file mode 100644 index 0000000..d7ebac2 --- /dev/null +++ b/openfut-http/Cargo.toml @@ -0,0 +1,12 @@ +[package] +name = "openfut-http" +version = "0.1.0" +edition = "2021" +license = "MIT" +description = "Generic HTTP/1.x request-body handling shared by OpenFUT transport hosts" +publish = false + +# No dependencies, and none wanted. This is a handful of bytes-in/bytes-out +# functions over `std::io::Read`; it knows nothing about TLS, about any game, or +# about which service is calling it. +[dependencies] diff --git a/openfut-http/src/lib.rs b/openfut-http/src/lib.rs new file mode 100644 index 0000000..4a173e7 --- /dev/null +++ b/openfut-http/src/lib.rs @@ -0,0 +1,180 @@ +//! Request-body handling shared by OpenFUT transport hosts. +//! +//! # Why this is its own crate +//! +//! Draining the request body is not a detail. Answering a client while it is +//! still sending leaves unread data in the receive queue, and Linux turns the +//! subsequent close into an **RST rather than a FIN**. That divergence is +//! invisible to any byte-comparison of the response, and it cost the redirector +//! two live gate attempts before it was found. +//! +//! Once a second host needed the same behaviour, the choice was to share the +//! code or copy it. The same reasoning that produced `openfut-tls` applies: +//! behaviour that must be identical across hosts should have one implementation. +//! +//! # Scope — deliberately narrow +//! +//! Only the part that was **byte-identical** in both hosts is here. The two +//! head-reading loops were *not* identical and are deliberately left where they +//! are: +//! +//! | | redirector | roster | +//! |---|---|---| +//! | head cap | 65536 | 16384 | +//! | read chunk | 4096 | 1024 | +//! | on read error | abort the connection | proceed if any bytes arrived | +//! +//! Those differences may well be accidental, but each host is gate-proven with +//! the values it has. Unifying them would be a behaviour change wearing a +//! refactor's clothes, and this project has already paid for that mistake — +//! the fix is evidence, not tidiness. If they should converge, that is a +//! separate change with its own gate. +//! +//! This crate has no dependencies, knows nothing about TLS, and knows nothing +//! about any game. + +use std::io::Read; + +/// What happened when the request body was drained. +/// +/// Reported per connection because "the client sent a body we never read" is +/// exactly the class of divergence a response-bytes comparison cannot see. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum BodyRead { + /// No `Content-Length` header — nothing to drain. + None, + Complete(usize), + Short { + got: usize, + want: usize, + }, +} + +impl BodyRead { + /// One-line description for a log or a test failure message. + pub fn describe(self) -> String { + match self { + BodyRead::None => "none".to_string(), + BodyRead::Complete(n) => format!("{n}B complete"), + BodyRead::Short { got, want } => format!("{got}B of {want}B SHORT"), + } + } +} + +/// Parse `Content-Length` from the head already in `buf` and read the rest of +/// the body, appending it to `buf`. +/// +/// `buf` must already contain everything read so far, including the `\r\n\r\n` +/// terminator and any body bytes that arrived with it. +/// +/// Mirrors the Python oracle: header lookup is case-insensitive, and a client +/// that stops early ends the read rather than hanging until the socket timeout. +pub fn drain_body(stream: &mut S, buf: &mut Vec) -> BodyRead { + let Some(sep) = buf.windows(4).position(|w| w == b"\r\n\r\n") else { + return BodyRead::None; + }; + let head = String::from_utf8_lossy(&buf[..sep]).to_string(); + let Some(want) = head + .lines() + .find(|l| l.to_ascii_lowercase().starts_with("content-length")) + .and_then(|l| l.split(':').nth(1)) + .and_then(|v| v.trim().parse::().ok()) + else { + return BodyRead::None; + }; + + let mut got = buf.len() - (sep + 4); + let mut chunk = [0u8; 4096]; + while got < want { + match stream.read(&mut chunk) { + Ok(0) | Err(_) => return BodyRead::Short { got, want }, + Ok(n) => { + buf.extend_from_slice(&chunk[..n]); + got += n; + } + } + } + BodyRead::Complete(got) +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn no_content_length_means_nothing_to_drain() { + let mut buf = b"GET / HTTP/1.1\r\nHost: x\r\n\r\n".to_vec(); + let mut rest: &[u8] = b""; + assert_eq!(drain_body(&mut rest, &mut buf), BodyRead::None); + } + + #[test] + fn an_unterminated_head_is_not_treated_as_a_body() { + // No \r\n\r\n yet: there is no way to know where a body would start. + let mut buf = b"POST / HTTP/1.1\r\nContent-Length: 4\r\n".to_vec(); + let mut rest: &[u8] = b"abcd"; + assert_eq!(drain_body(&mut rest, &mut buf), BodyRead::None); + } + + #[test] + fn a_body_already_fully_buffered_needs_no_further_read() { + let mut buf = b"POST / HTTP/1.1\r\nContent-Length: 4\r\n\r\nabcd".to_vec(); + // An empty stream proves nothing more was read. + let mut rest: &[u8] = b""; + assert_eq!(drain_body(&mut rest, &mut buf), BodyRead::Complete(4)); + } + + #[test] + fn a_body_split_across_reads_is_completed() { + let mut buf = b"POST / HTTP/1.1\r\nContent-Length: 8\r\n\r\nab".to_vec(); + let mut rest: &[u8] = b"cdefgh"; + assert_eq!(drain_body(&mut rest, &mut buf), BodyRead::Complete(8)); + assert!(buf.ends_with(b"abcdefgh")); + } + + /// The failure mode that matters: a client that stops early must end the + /// read rather than block. Reported as Short so the host can log it. + #[test] + fn a_client_that_stops_early_is_reported_short() { + let mut buf = b"POST / HTTP/1.1\r\nContent-Length: 10\r\n\r\nabc".to_vec(); + let mut rest: &[u8] = b""; + assert_eq!( + drain_body(&mut rest, &mut buf), + BodyRead::Short { got: 3, want: 10 } + ); + } + + #[test] + fn content_length_is_matched_case_insensitively() { + let mut buf = b"POST / HTTP/1.1\r\ncOnTeNt-LeNgTh: 4\r\n\r\n".to_vec(); + let mut rest: &[u8] = b"abcd"; + assert_eq!(drain_body(&mut rest, &mut buf), BodyRead::Complete(4)); + } + + #[test] + fn a_malformed_content_length_is_ignored_rather_than_guessed() { + let mut buf = b"POST / HTTP/1.1\r\nContent-Length: banana\r\n\r\n".to_vec(); + let mut rest: &[u8] = b"abcd"; + assert_eq!(drain_body(&mut rest, &mut buf), BodyRead::None); + } + + /// A header whose NAME merely contains "content-length" must not be read as + /// the real one. `starts_with` is what makes this safe; a `contains` would + /// match `X-Original-Content-Length` and drain the wrong number of bytes. + #[test] + fn a_lookalike_header_is_not_mistaken_for_content_length() { + let mut buf = b"POST / HTTP/1.1\r\nX-Original-Content-Length: 99\r\n\r\n".to_vec(); + let mut rest: &[u8] = b""; + assert_eq!(drain_body(&mut rest, &mut buf), BodyRead::None); + } + + #[test] + fn describe_is_stable_for_logs() { + assert_eq!(BodyRead::None.describe(), "none"); + assert_eq!(BodyRead::Complete(665).describe(), "665B complete"); + assert_eq!( + BodyRead::Short { got: 3, want: 10 }.describe(), + "3B of 10B SHORT" + ); + } +} diff --git a/openfut-redirector-host/Cargo.toml b/openfut-redirector-host/Cargo.toml index ed5308a..45e283d 100644 --- a/openfut-redirector-host/Cargo.toml +++ b/openfut-redirector-host/Cargo.toml @@ -9,6 +9,7 @@ publish = false [dependencies] openfut-adapter-fifa17 = { path = "../openfut-adapter-fifa17" } openfut-host-config = { path = "../openfut-host-config" } +openfut-http = { path = "../openfut-http" } # Shared legacy-TLS listener. See its manifest for why openssl and not rustls. openfut-tls = { path = "../openfut-tls" } diff --git a/openfut-redirector-host/src/lib.rs b/openfut-redirector-host/src/lib.rs index 6ccc467..638bfcc 100644 --- a/openfut-redirector-host/src/lib.rs +++ b/openfut-redirector-host/src/lib.rs @@ -38,6 +38,12 @@ use std::sync::Arc; use std::time::{Duration, SystemTime, UNIX_EPOCH}; use openfut_adapter_fifa17::redirector; +// Body draining is shared. Behaviour that must be identical across hosts gets +// one implementation, for the same reason TLS does: the divergence it guards +// against (an unread body turning the close into an RST) is invisible in any +// comparison of the response, and cost two live gate attempts to find. +use openfut_http::drain_body; +pub use openfut_http::BodyRead; use openssl::ssl::SslAcceptor; pub use config::RedirectorConfig; @@ -283,11 +289,7 @@ fn handle( log(&format!( "conn-{id:04} {peer} REQ {line0} req_bytes={} body={}", buf.len(), - match body_read { - BodyRead::None => "none".to_string(), - BodyRead::Complete(n) => format!("{n}B complete"), - BodyRead::Short { got, want } => format!("{got}B of {want}B SHORT"), - } + body_read.describe() )); if !line0.is_empty() && !redirector::is_get_server_instance(&line0) { log(&format!( @@ -326,52 +328,6 @@ fn handle( let _ = tls.shutdown(); } -/// What happened when the request body was drained. Reported per connection -/// because "the client sent a body we never read" is exactly the class of -/// divergence a response-bytes comparison cannot see. -#[derive(Debug, Clone, Copy, PartialEq, Eq)] -pub enum BodyRead { - /// No `Content-Length` header — nothing to drain. - None, - Complete(usize), - Short { - got: usize, - want: usize, - }, -} - -/// Parse `Content-Length` and read the remainder of the body into `buf`. -/// -/// Mirrors the oracle: header lookup is case-insensitive, and a client that -/// stops early ends the read rather than hanging until the timeout. -fn drain_body(stream: &mut S, buf: &mut Vec) -> BodyRead { - let Some(sep) = buf.windows(4).position(|w| w == b"\r\n\r\n") else { - return BodyRead::None; - }; - let head = String::from_utf8_lossy(&buf[..sep]).to_string(); - let Some(want) = head - .lines() - .find(|l| l.to_ascii_lowercase().starts_with("content-length")) - .and_then(|l| l.split(':').nth(1)) - .and_then(|v| v.trim().parse::().ok()) - else { - return BodyRead::None; - }; - - let mut got = buf.len() - (sep + 4); - let mut chunk = [0u8; 4096]; - while got < want { - match stream.read(&mut chunk) { - Ok(0) | Err(_) => return BodyRead::Short { got, want }, - Ok(n) => { - buf.extend_from_slice(&chunk[..n]); - got += n; - } - } - } - BodyRead::Complete(got) -} - #[cfg(test)] mod tests { use super::*; diff --git a/openfut-roster-host/Cargo.toml b/openfut-roster-host/Cargo.toml index 09179f4..2d03e58 100644 --- a/openfut-roster-host/Cargo.toml +++ b/openfut-roster-host/Cargo.toml @@ -9,6 +9,7 @@ publish = false [dependencies] openfut-adapter-fifa17 = { path = "../openfut-adapter-fifa17" } openfut-host-config = { path = "../openfut-host-config" } +openfut-http = { path = "../openfut-http" } # Same listener as the redirector, by construction rather than by convention. # Two hosts configuring TLS separately is how the 2026-08-11 certificate # mismatch happened, and roster is the service where that mismatch first diff --git a/openfut-roster-host/src/lib.rs b/openfut-roster-host/src/lib.rs index 1b29a12..c38560c 100644 --- a/openfut-roster-host/src/lib.rs +++ b/openfut-roster-host/src/lib.rs @@ -48,6 +48,9 @@ use std::sync::Arc; use std::time::{SystemTime, UNIX_EPOCH}; use openfut_adapter_fifa17::roster::{self, Method}; +// Shared with the redirector: see openfut-http for why this is not duplicated. +use openfut_http::drain_body; +pub use openfut_http::BodyRead; // The acceptor comes from the shared crate, so this host never names OpenSSL. use openfut_tls::SslAcceptor; @@ -92,22 +95,6 @@ pub fn banner(cfg: &RosterConfig) -> String { ) } -/// What happened when the request body was drained. -/// -/// Duplicated from the redirector host on purpose, for now. Unifying the two -/// would mean editing the redirector, and the roster A/B must change exactly -/// one thing. This is scheduled for extraction once the roster gate closes. -#[derive(Debug, Clone, Copy, PartialEq, Eq)] -pub enum BodyRead { - /// No `Content-Length` header — nothing to drain. - None, - Complete(usize), - Short { - got: usize, - want: usize, - }, -} - /// What the host did on a connection, beyond the bytes it sent. /// /// Recorded because the divergence that cost two gate attempts on the @@ -283,7 +270,7 @@ fn handle( log(&format!( "conn-{id:04} {peer} {line0} req_bytes={} body={} -> {}B", buf.len(), - describe(body), + body.describe(), response.len() )); record( @@ -308,14 +295,6 @@ fn handle( let _ = tls.shutdown(); } -fn describe(b: BodyRead) -> String { - match b { - BodyRead::None => "none".into(), - BodyRead::Complete(n) => format!("{n}B complete"), - BodyRead::Short { got, want } => format!("{got}/{want}B SHORT"), - } -} - /// `Date:` for right now, in the oracle's format. fn now_http_date() -> String { let secs = SystemTime::now() @@ -346,38 +325,6 @@ fn read_head(stream: &mut S, buf: &mut Vec) -> bool { } } -/// Parse `Content-Length` and read the remainder of the body into `buf`. -/// -/// Header lookup is case-insensitive, and a client that stops early ends the -/// read rather than hanging. -fn drain_body(stream: &mut S, buf: &mut Vec) -> BodyRead { - let Some(sep) = buf.windows(4).position(|w| w == b"\r\n\r\n") else { - return BodyRead::None; - }; - let head = String::from_utf8_lossy(&buf[..sep]).to_string(); - let Some(want) = head - .lines() - .find(|l| l.to_ascii_lowercase().starts_with("content-length")) - .and_then(|l| l.split(':').nth(1)) - .and_then(|v| v.trim().parse::().ok()) - else { - return BodyRead::None; - }; - - let mut got = buf.len() - (sep + 4); - let mut chunk = [0u8; 4096]; - while got < want { - match stream.read(&mut chunk) { - Ok(0) | Err(_) => return BodyRead::Short { got, want }, - Ok(n) => { - buf.extend_from_slice(&chunk[..n]); - got += n; - } - } - } - BodyRead::Complete(got) -} - #[cfg(test)] mod tests { use super::*; diff --git a/openfut-tls/src/lib.rs b/openfut-tls/src/lib.rs index 7bfc7dd..d656212 100644 --- a/openfut-tls/src/lib.rs +++ b/openfut-tls/src/lib.rs @@ -302,23 +302,32 @@ mod tests { /// A self-signed pair, generated here rather than borrowed from the game /// stack: this crate is game-independent and its tests must not depend on /// FIFA material. Adapter crates test their own profiles. + /// + /// Generated exactly once per process via `OnceLock`. The first version + /// checked "does the cert file exist?" and returned early if so — which is + /// a race, because the cert is written before the key. Tests run in + /// parallel, so one could observe a certificate whose key had not landed + /// yet and fail to build an acceptor. It failed in one run and passed in + /// the next, which is exactly the kind of flake that gets rerun instead of + /// fixed. fn keypair() -> (String, String) { + use std::sync::OnceLock; + static PAIR: OnceLock<(String, String)> = OnceLock::new(); + PAIR.get_or_init(|| generate_keypair("cert")).clone() + } + + fn generate_keypair(tag: &str) -> (String, String) { use openssl::asn1::Asn1Time; use openssl::hash::MessageDigest; use openssl::pkey::PKey; use openssl::rsa::Rsa; use openssl::x509::{X509Name, X509}; - let dir = std::env::temp_dir().join(format!("openfut-tls-test-{}", std::process::id())); + let dir = + std::env::temp_dir().join(format!("openfut-tls-test-{}-{tag}", std::process::id())); std::fs::create_dir_all(&dir).unwrap(); let cert_path = dir.join("cert.pem"); let key_path = dir.join("key.pem"); - if cert_path.exists() && key_path.exists() { - return ( - cert_path.to_string_lossy().into(), - key_path.to_string_lossy().into(), - ); - } let rsa = Rsa::generate(2048).unwrap(); let pkey = PKey::from_rsa(rsa).unwrap(); @@ -338,8 +347,10 @@ mod tests { b.sign(&pkey, MessageDigest::sha256()).unwrap(); let cert = b.build(); - std::fs::write(&cert_path, cert.to_pem().unwrap()).unwrap(); + // Key first, then certificate: a reader that checks for the cert can + // then never find one without its key. std::fs::write(&key_path, pkey.private_key_to_pem_pkcs8().unwrap()).unwrap(); + std::fs::write(&cert_path, cert.to_pem().unwrap()).unwrap(); ( cert_path.to_string_lossy().into(), key_path.to_string_lossy().into(), @@ -455,30 +466,9 @@ mod tests { // A different certificate must produce a different fingerprint — this // is the whole point of the check that guards cross-host parity. - let dir = std::env::temp_dir().join(format!("openfut-tls-alt-{}", std::process::id())); - std::fs::create_dir_all(&dir).unwrap(); - let other = dir.join("other.pem"); - if !other.exists() { - // Reuse the generator by pointing at a fresh temp dir. - let rsa = openssl::rsa::Rsa::generate(2048).unwrap(); - let pkey = openssl::pkey::PKey::from_rsa(rsa).unwrap(); - let mut nb = openssl::x509::X509Name::builder().unwrap(); - nb.append_entry_by_text("CN", "other").unwrap(); - let name = nb.build(); - let mut b = openssl::x509::X509::builder().unwrap(); - b.set_version(2).unwrap(); - b.set_subject_name(&name).unwrap(); - b.set_issuer_name(&name).unwrap(); - b.set_pubkey(&pkey).unwrap(); - b.set_not_before(&openssl::asn1::Asn1Time::days_from_now(0).unwrap()) - .unwrap(); - b.set_not_after(&openssl::asn1::Asn1Time::days_from_now(1).unwrap()) - .unwrap(); - b.sign(&pkey, openssl::hash::MessageDigest::sha256()) - .unwrap(); - std::fs::write(&other, b.build().to_pem().unwrap()).unwrap(); - } - let fp2 = certificate_fingerprint(other.to_str().unwrap()).unwrap(); + // Generated through the same helper, so it cannot race either. + let (other, _) = generate_keypair("alt"); + let fp2 = certificate_fingerprint(&other).unwrap(); assert_ne!(fp, fp2); }