From 70cb25630d1168cdd8f39c182a544b227b7247e1 Mon Sep 17 00:00:00 2001 From: Sean Klein Date: Wed, 30 Oct 2024 14:08:27 -0700 Subject: [PATCH 1/8] Add support library for returning Dropshot range requests --- Cargo.toml | 4 + range-requests/Cargo.toml | 23 +++ range-requests/src/lib.rs | 397 ++++++++++++++++++++++++++++++++++++++ 3 files changed, 424 insertions(+) create mode 100644 range-requests/Cargo.toml create mode 100644 range-requests/src/lib.rs diff --git a/Cargo.toml b/Cargo.toml index 4fe0f4deac..39757467d3 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -96,6 +96,7 @@ members = [ "oximeter/types", "package", "passwords", + "range-requests", "rpaths", "sled-agent", "sled-agent/api", @@ -221,6 +222,7 @@ default-members = [ "oximeter/types", "package", "passwords", + "range-requests", "rpaths", "sled-agent", "sled-agent/api", @@ -390,6 +392,7 @@ highway = "1.2.0" hkdf = "0.12.4" http = "1.1.0" http-body-util = "0.1.2" +http-range = "0.1.5" httpmock = "0.8.0-alpha.1" httptest = "0.16.1" hubtools = { git = "https://github.com/oxidecomputer/hubtools.git", branch = "main" } @@ -526,6 +529,7 @@ rand = "0.8.5" rand_core = "0.6.4" rand_distr = "0.4.3" rand_seeder = "0.3.0" +range-requests = { path = "range-requests" } ratatui = "0.28.1" rayon = "1.10" rcgen = "0.12.1" diff --git a/range-requests/Cargo.toml b/range-requests/Cargo.toml new file mode 100644 index 0000000000..0d8b033258 --- /dev/null +++ b/range-requests/Cargo.toml @@ -0,0 +1,23 @@ +[package] +name = "range-requests" +description = "Helpers for making and receiving range requests" +version = "0.1.0" +edition = "2021" +license = "MPL-2.0" + +[lints] +workspace = true + +[dependencies] +bytes.workspace = true +dropshot.workspace = true +futures.workspace = true +http.workspace = true +http-range.workspace = true +http-body-util.workspace = true +hyper.workspace = true +thiserror.workspace = true + +[dev-dependencies] +tokio.workspace = true +tokio-util.workspace = true diff --git a/range-requests/src/lib.rs b/range-requests/src/lib.rs new file mode 100644 index 0000000000..432fd134f1 --- /dev/null +++ b/range-requests/src/lib.rs @@ -0,0 +1,397 @@ +// This Source Code Form is subject to the terms of the Mozilla Public +// License, v. 2.0. If a copy of the MPL was not distributed with this +// file, You can obtain one at https://mozilla.org/MPL/2.0/. + +use dropshot::Body; +use dropshot::HttpError; +use futures::TryStreamExt; +use hyper::{ + header::{ACCEPT_RANGES, CONTENT_LENGTH, CONTENT_RANGE, CONTENT_TYPE}, + Response, StatusCode, +}; + +/// Errors which may be returned when processing range requests +#[derive(thiserror::Error, Debug)] +pub enum Error { + #[error("Using multiple ranges is not supported")] + MultipleRangesUnsupported, + + #[error("Failed to parse range")] + Parse(http_range::HttpRangeParseError), + + #[error(transparent)] + Http(#[from] http::Error), +} + +impl From for HttpError { + fn from(err: Error) -> Self { + match err { + Error::MultipleRangesUnsupported | Error::Parse(_) => { + HttpError::for_bad_request(None, err.to_string()) + } + Error::Http(err) => err.into(), + } + } +} + +fn bad_range_response(file_size: u64) -> Response { + hyper::Response::builder() + .status(StatusCode::RANGE_NOT_SATISFIABLE) + .header(ACCEPT_RANGES, "bytes") + .header(CONTENT_RANGE, format!("bytes */{file_size}")) + .body(Body::empty()) + .unwrap() +} + +/// Generate a GET response, optionally for a HTTP range request. The total +/// file length should be provided, whether or not the expected Content-Length +/// for a range request is shorter. +pub fn make_get_response( + range: Option, + file_length: u64, + content_type: Option<&str>, + rx: S, +) -> Result, Error> +where + E: Send + Sync + std::error::Error + 'static, + D: Into, + S: Send + Sync + futures::stream::Stream> + 'static, +{ + Ok(make_response_common(range, file_length, content_type).body( + Body::wrap(http_body_util::StreamBody::new( + rx.map_ok(|b| hyper::body::Frame::data(b.into())), + )), + )?) +} + +/// Generate a HEAD response, optionally for a HTTP range request. The total +/// file length should be provided, whether or not the expected Content-Length +/// for a range request is shorter. +pub fn make_head_response( + range: Option, + file_length: u64, + content_type: Option<&str>, +) -> Result, Error> { + Ok(make_response_common(range, file_length, content_type) + .body(Body::empty())?) +} + +fn make_response_common( + range: Option, + file_length: u64, + content_type: Option<&str>, +) -> hyper::http::response::Builder { + let mut res = Response::builder(); + res = res.header(ACCEPT_RANGES, "bytes"); + res = res.header( + CONTENT_TYPE, + content_type.unwrap_or("application/octet-stream"), + ); + + if let Some(range) = range { + res = res.header(CONTENT_LENGTH, range.content_length().to_string()); + res = res.header(CONTENT_RANGE, range.to_content_range()); + res = res.status(StatusCode::PARTIAL_CONTENT); + } else { + res = res.header(CONTENT_LENGTH, file_length.to_string()); + res = res.status(StatusCode::OK); + } + + res +} + +/// Represents the raw, unparsed values of "range" from a request header. +pub struct PotentialRange(Vec); + +impl PotentialRange { + /// Parses a single range request out of the range request. + /// + /// `len` is the total length of the document, for the range request being made. + /// + /// On failure, returns a range response with the appropriate headers + /// to inform the caller how to make a correct range request. + pub fn parse(&self, len: u64) -> Result> { + self.single_range(len).map_err(|_err| bad_range_response(len)) + } + + fn single_range(&self, len: u64) -> Result { + match http_range::HttpRange::parse_bytes(&self.0, len) { + Ok(ranges) => { + if ranges.len() != 1 || ranges[0].length < 1 { + // Right now, we don't want to deal with encoding a + // response that has multiple ranges. + Err(Error::MultipleRangesUnsupported) + } else { + Ok(SingleRange::new(ranges[0], len)?) + } + } + Err(err) => Err(Error::Parse(err)), + } + } +} + +/// A parsed range request, and associated "total document length". +#[derive(Clone, Debug)] +pub struct SingleRange { + range: http_range::HttpRange, + total: u64, +} + +#[cfg(test)] +impl PartialEq for SingleRange { + fn eq(&self, other: &Self) -> bool { + self.range.start == other.range.start + && self.range.length == other.range.length + && self.total == other.total + } +} + +impl SingleRange { + pub fn new( + range: http_range::HttpRange, + total: u64, + ) -> Result { + let http_range::HttpRange { start, mut length } = range; + + const INVALID_RANGE: Error = + Error::Parse(http_range::HttpRangeParseError::InvalidRange); + + // Clip the length to avoid going beyond the end of the total range + if start.checked_add(length).ok_or(INVALID_RANGE)? >= total { + length = total.checked_sub(start).ok_or(INVALID_RANGE)?; + } + // If the length is zero, we cannot satisfy the range request + if length == 0 { + return Err(INVALID_RANGE); + } + + Ok(Self { range: http_range::HttpRange { start, length }, total }) + } + + /// Return the first byte in this range for use in inclusive ranges. + pub fn start(&self) -> u64 { + self.range.start + } + + /// Return the last byte in this range for use in inclusive ranges. + pub fn end_inclusive(&self) -> u64 { + assert!(self.range.length > 0); + + self.range + .start + .checked_add(self.range.length) + .unwrap() + .checked_sub(1) + .unwrap() + } + + /// Generate the Content-Range header for inclusion in a HTTP 206 partial + /// content response using this range. + pub fn to_content_range(&self) -> String { + format!( + "bytes {}-{}/{}", + self.range.start, + self.end_inclusive(), + self.total + ) + } + + /// Generate a Range header for inclusion in another HTTP request; e.g., + /// to a backend object store. + pub fn to_range(&self) -> String { + format!("bytes={}-{}", self.range.start, self.end_inclusive()) + } + + pub fn content_length(&self) -> u64 { + assert!(self.range.length > 0); + + self.range.length + } +} + +/// A trait, implemented for [dropshot::RequestContext], to pull a range header +/// out of the request headers. +pub trait RequestContextEx { + fn range(&self) -> Option; +} + +impl RequestContextEx for dropshot::RequestContext +where + T: Send + Sync + 'static, +{ + /// If there is a Range header, return it for processing during response + /// generation. + fn range(&self) -> Option { + self.request + .headers() + .get(hyper::header::RANGE) + .map(|hv| PotentialRange(hv.as_bytes().to_vec())) + } +} + +#[cfg(test)] +mod test { + use super::*; + use futures::stream::once; + use std::convert::Infallible; + use tokio_util::io::ReaderStream; + + #[test] + fn parse_range_valid() { + // Whole range + let pr = PotentialRange(b"bytes=0-100".to_vec()); + assert_eq!( + pr.single_range(100).unwrap(), + SingleRange { + range: http_range::HttpRange { start: 0, length: 100 }, + total: 100 + } + ); + + // Clipped + let pr = PotentialRange(b"bytes=0-100".to_vec()); + assert_eq!( + pr.single_range(50).unwrap(), + SingleRange { + range: http_range::HttpRange { start: 0, length: 50 }, + total: 50 + } + ); + + // Single byte + let pr = PotentialRange(b"bytes=49-49".to_vec()); + assert_eq!( + pr.single_range(50).unwrap(), + SingleRange { + range: http_range::HttpRange { start: 49, length: 1 }, + total: 50 + } + ); + } + + #[test] + fn parse_range_invalid() { + let pr = PotentialRange(b"bytes=50-50".to_vec()); + assert!(matches!( + pr.single_range(50).expect_err("Range should be invalid"), + Error::Parse(http_range::HttpRangeParseError::NoOverlap), + )); + + let pr = PotentialRange(b"bytes=20-1".to_vec()); + assert!(matches!( + pr.single_range(50).expect_err("Range should be invalid"), + Error::Parse(http_range::HttpRangeParseError::InvalidRange), + )); + } + + #[test] + fn get_response_no_range() { + let bytes = b"Hello world"; + + let response = make_get_response( + None, + bytes.len() as u64, + None, + ReaderStream::new(bytes.as_slice()), + ) + .expect("Should have made response"); + + assert_eq!(response.status(), StatusCode::OK); + + let headers = response.headers(); + println!("Headers: {headers:#?}"); + assert_eq!(headers.len(), 3); + assert_eq!(headers.get(ACCEPT_RANGES).unwrap(), "bytes"); + assert_eq!( + headers.get(CONTENT_TYPE).unwrap(), + "application/octet-stream" + ); + assert_eq!( + headers.get(CONTENT_LENGTH).unwrap(), + &bytes.len().to_string() + ); + } + + #[test] + fn get_response_with_range() { + let ranged_get_request = |start, length, total_length| { + let range = SingleRange::new( + http_range::HttpRange { start, length }, + total_length, + ) + .unwrap(); + + let b = vec![0; length as usize]; + let response = make_get_response( + Some(range.clone()), + total_length, + None, + once(async move { Ok::<_, Infallible>(b) }), + ) + .expect("Should have made response"); + + response + }; + + // First half + let response = ranged_get_request(0, 512, 1024); + assert_eq!(response.status(), StatusCode::PARTIAL_CONTENT); + let headers = response.headers(); + println!("Headers: {headers:#?}"); + assert_eq!(headers.len(), 4); + assert_eq!(headers.get(ACCEPT_RANGES).unwrap(), "bytes"); + assert_eq!( + headers.get(CONTENT_TYPE).unwrap(), + "application/octet-stream" + ); + assert_eq!(headers.get(CONTENT_LENGTH).unwrap(), "512"); + assert_eq!( + headers.get(CONTENT_RANGE).unwrap(), + &format!("bytes 0-511/1024") + ); + + // Second half + let response = ranged_get_request(512, 512, 1024); + assert_eq!(response.status(), StatusCode::PARTIAL_CONTENT); + let headers = response.headers(); + println!("Headers: {headers:#?}"); + assert_eq!(headers.len(), 4); + assert_eq!(headers.get(ACCEPT_RANGES).unwrap(), "bytes"); + assert_eq!( + headers.get(CONTENT_TYPE).unwrap(), + "application/octet-stream" + ); + assert_eq!(headers.get(CONTENT_LENGTH).unwrap(), "512"); + assert_eq!( + headers.get(CONTENT_RANGE).unwrap(), + &format!("bytes 512-1023/1024") + ); + + // Partially out of bounds + let response = ranged_get_request(1000, 512, 1024); + assert_eq!(response.status(), StatusCode::PARTIAL_CONTENT); + let headers = response.headers(); + println!("Headers: {headers:#?}"); + assert_eq!(headers.len(), 4); + assert_eq!(headers.get(ACCEPT_RANGES).unwrap(), "bytes"); + assert_eq!( + headers.get(CONTENT_TYPE).unwrap(), + "application/octet-stream" + ); + assert_eq!(headers.get(CONTENT_LENGTH).unwrap(), "24"); + assert_eq!( + headers.get(CONTENT_RANGE).unwrap(), + &format!("bytes 1000-1023/1024") + ); + + // Fully out of bounds + assert!(matches!( + SingleRange::new( + http_range::HttpRange { start: 1024, length: 512 }, + 1024 + ) + .expect_err("Should have thrown an error"), + Error::Parse(http_range::HttpRangeParseError::InvalidRange) + )); + } +} From 6b1b55604af7d01432b869b57c20c5e99d2c5477 Mon Sep 17 00:00:00 2001 From: Sean Klein Date: Wed, 30 Oct 2024 14:16:56 -0700 Subject: [PATCH 2/8] lockfile and clippy, classic combo --- Cargo.lock | 16 ++++++++++++++++ range-requests/src/lib.rs | 6 +++--- 2 files changed, 19 insertions(+), 3 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 87899e699c..b739f2912f 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -8901,6 +8901,22 @@ dependencies = [ "rand_core", ] +[[package]] +name = "range-requests" +version = "0.1.0" +dependencies = [ + "bytes", + "dropshot", + "futures", + "http 1.1.0", + "http-body-util", + "http-range", + "hyper 1.4.1", + "thiserror", + "tokio", + "tokio-util", +] + [[package]] name = "ratatui" version = "0.28.1" diff --git a/range-requests/src/lib.rs b/range-requests/src/lib.rs index 432fd134f1..c1a08634b5 100644 --- a/range-requests/src/lib.rs +++ b/range-requests/src/lib.rs @@ -347,7 +347,7 @@ mod test { assert_eq!(headers.get(CONTENT_LENGTH).unwrap(), "512"); assert_eq!( headers.get(CONTENT_RANGE).unwrap(), - &format!("bytes 0-511/1024") + "bytes 0-511/1024", ); // Second half @@ -364,7 +364,7 @@ mod test { assert_eq!(headers.get(CONTENT_LENGTH).unwrap(), "512"); assert_eq!( headers.get(CONTENT_RANGE).unwrap(), - &format!("bytes 512-1023/1024") + "bytes 512-1023/1024", ); // Partially out of bounds @@ -381,7 +381,7 @@ mod test { assert_eq!(headers.get(CONTENT_LENGTH).unwrap(), "24"); assert_eq!( headers.get(CONTENT_RANGE).unwrap(), - &format!("bytes 1000-1023/1024") + "bytes 1000-1023/1024", ); // Fully out of bounds From 536117ca4233f65e95b1eb95b65bddcaa9b7fe9d Mon Sep 17 00:00:00 2001 From: Sean Klein Date: Wed, 30 Oct 2024 14:18:19 -0700 Subject: [PATCH 3/8] And fmt, how could i forget --- range-requests/src/lib.rs | 15 +++------------ 1 file changed, 3 insertions(+), 12 deletions(-) diff --git a/range-requests/src/lib.rs b/range-requests/src/lib.rs index c1a08634b5..4af1bbf941 100644 --- a/range-requests/src/lib.rs +++ b/range-requests/src/lib.rs @@ -345,10 +345,7 @@ mod test { "application/octet-stream" ); assert_eq!(headers.get(CONTENT_LENGTH).unwrap(), "512"); - assert_eq!( - headers.get(CONTENT_RANGE).unwrap(), - "bytes 0-511/1024", - ); + assert_eq!(headers.get(CONTENT_RANGE).unwrap(), "bytes 0-511/1024",); // Second half let response = ranged_get_request(512, 512, 1024); @@ -362,10 +359,7 @@ mod test { "application/octet-stream" ); assert_eq!(headers.get(CONTENT_LENGTH).unwrap(), "512"); - assert_eq!( - headers.get(CONTENT_RANGE).unwrap(), - "bytes 512-1023/1024", - ); + assert_eq!(headers.get(CONTENT_RANGE).unwrap(), "bytes 512-1023/1024",); // Partially out of bounds let response = ranged_get_request(1000, 512, 1024); @@ -379,10 +373,7 @@ mod test { "application/octet-stream" ); assert_eq!(headers.get(CONTENT_LENGTH).unwrap(), "24"); - assert_eq!( - headers.get(CONTENT_RANGE).unwrap(), - "bytes 1000-1023/1024", - ); + assert_eq!(headers.get(CONTENT_RANGE).unwrap(), "bytes 1000-1023/1024",); // Fully out of bounds assert!(matches!( From d175e8c39f264464309ec4542abf6137d21b0c8b Mon Sep 17 00:00:00 2001 From: Sean Klein Date: Wed, 30 Oct 2024 14:20:24 -0700 Subject: [PATCH 4/8] one more hakari swing --- Cargo.lock | 1 + range-requests/Cargo.toml | 1 + 2 files changed, 2 insertions(+) diff --git a/Cargo.lock b/Cargo.lock index b739f2912f..8eb34603db 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -8912,6 +8912,7 @@ dependencies = [ "http-body-util", "http-range", "hyper 1.4.1", + "omicron-workspace-hack", "thiserror", "tokio", "tokio-util", diff --git a/range-requests/Cargo.toml b/range-requests/Cargo.toml index 0d8b033258..5a9a526258 100644 --- a/range-requests/Cargo.toml +++ b/range-requests/Cargo.toml @@ -17,6 +17,7 @@ http-range.workspace = true http-body-util.workspace = true hyper.workspace = true thiserror.workspace = true +omicron-workspace-hack.workspace = true [dev-dependencies] tokio.workspace = true From c8421fb422569da9aad0c22b6d092178411d2411 Mon Sep 17 00:00:00 2001 From: Sean Klein Date: Thu, 31 Oct 2024 11:17:36 -0700 Subject: [PATCH 5/8] Body checking too --- Cargo.lock | 1 + Cargo.toml | 1 + range-requests/Cargo.toml | 1 + range-requests/src/lib.rs | 177 +++++++++++++++++++++++++------------- 4 files changed, 118 insertions(+), 62 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index a11915efc1..cc9f87776b 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -8909,6 +8909,7 @@ dependencies = [ "dropshot", "futures", "http 1.1.0", + "http-body 1.0.1", "http-body-util", "http-range", "hyper 1.4.1", diff --git a/Cargo.toml b/Cargo.toml index e1c3acbb45..43750561f7 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -391,6 +391,7 @@ hickory-server = "0.24.1" highway = "1.2.0" hkdf = "0.12.4" http = "1.1.0" +http-body = "1.0.1" http-body-util = "0.1.2" http-range = "0.1.5" httpmock = "0.8.0-alpha.1" diff --git a/range-requests/Cargo.toml b/range-requests/Cargo.toml index 5a9a526258..aab9692905 100644 --- a/range-requests/Cargo.toml +++ b/range-requests/Cargo.toml @@ -20,5 +20,6 @@ thiserror.workspace = true omicron-workspace-hack.workspace = true [dev-dependencies] +http-body.workspace = true tokio.workspace = true tokio-util.workspace = true diff --git a/range-requests/src/lib.rs b/range-requests/src/lib.rs index 4af1bbf941..888d7015c1 100644 --- a/range-requests/src/lib.rs +++ b/range-requests/src/lib.rs @@ -46,6 +46,10 @@ fn bad_range_response(file_size: u64) -> Response { /// Generate a GET response, optionally for a HTTP range request. The total /// file length should be provided, whether or not the expected Content-Length /// for a range request is shorter. +/// +/// It is the responsibility of the caller to ensure that `rx` is a stream of +/// data matching the requested range in the `range` argument, if it is +/// supplied. pub fn make_get_response( range: Option, file_length: u64, @@ -232,7 +236,10 @@ where #[cfg(test)] mod test { use super::*; + + use bytes::Bytes; use futures::stream::once; + use http_body_util::BodyExt; use std::convert::Infallible; use tokio_util::io::ReaderStream; @@ -298,88 +305,134 @@ mod test { assert_eq!(response.status(), StatusCode::OK); - let headers = response.headers(); - println!("Headers: {headers:#?}"); - assert_eq!(headers.len(), 3); - assert_eq!(headers.get(ACCEPT_RANGES).unwrap(), "bytes"); - assert_eq!( - headers.get(CONTENT_TYPE).unwrap(), - "application/octet-stream" - ); - assert_eq!( - headers.get(CONTENT_LENGTH).unwrap(), - &bytes.len().to_string() + expect_headers( + response.headers(), + &[ + (ACCEPT_RANGES, "bytes"), + (CONTENT_TYPE, "application/octet-stream"), + (CONTENT_LENGTH, &bytes.len().to_string()), + ], ); } - #[test] - fn get_response_with_range() { - let ranged_get_request = |start, length, total_length| { - let range = SingleRange::new( - http_range::HttpRange { start, length }, - total_length, - ) - .unwrap(); - - let b = vec![0; length as usize]; - let response = make_get_response( - Some(range.clone()), - total_length, - None, - once(async move { Ok::<_, Infallible>(b) }), - ) - .expect("Should have made response"); + // Makes a get response with a Vec of bytes that counts from zero. + // + // The u8s aren't normal bounds on the length, but they make the mapping + // of "the data is the index" easy. + fn ranged_get_request( + start: u8, + length: u8, + total_length: u8, + ) -> Response { + let range = SingleRange::new( + http_range::HttpRange { + start: start.into(), + length: length.into(), + }, + total_length.into(), + ) + .unwrap(); - response - }; + let b: Vec<_> = (u8::try_from(range.start()).unwrap() + ..=u8::try_from(range.end_inclusive()).unwrap()) + .collect(); + let response = make_get_response( + Some(range.clone()), + total_length.into(), + None, + once(async move { Ok::<_, Infallible>(b) }), + ) + .expect("Should have made response"); + + response + } + + // Validates the headers exactly match the map + fn expect_headers( + headers: &http::HeaderMap, + expected: &[(http::HeaderName, &str)], + ) { + println!("Headers: {headers:#?}"); + assert_eq!(headers.len(), expected.len()); + for (k, v) in expected { + assert_eq!(headers.get(k).unwrap(), v); + } + } + + // Validates the data matches an incrementing Vec of u8 values + async fn expect_data( + body: &mut (dyn http_body::Body< + Data = Bytes, + Error = Box, + > + Unpin), + start: u8, + length: u8, + ) { + println!("Checking data from {start}, with length {length}"); + let frame = body + .frame() + .await + .expect("Error reading frame") + .expect("Should have one frame") + .into_data() + .expect("Should be a DATA frame"); + assert_eq!(frame.len(), usize::from(length),); + + for i in 0..length { + assert_eq!(frame[i as usize], i + start); + } + } + + #[tokio::test] + async fn get_response_with_range() { // First half - let response = ranged_get_request(0, 512, 1024); + let mut response = ranged_get_request(0, 32, 64); assert_eq!(response.status(), StatusCode::PARTIAL_CONTENT); - let headers = response.headers(); - println!("Headers: {headers:#?}"); - assert_eq!(headers.len(), 4); - assert_eq!(headers.get(ACCEPT_RANGES).unwrap(), "bytes"); - assert_eq!( - headers.get(CONTENT_TYPE).unwrap(), - "application/octet-stream" + expect_data(response.body_mut(), 0, 32).await; + expect_headers( + response.headers(), + &[ + (ACCEPT_RANGES, "bytes"), + (CONTENT_TYPE, "application/octet-stream"), + (CONTENT_LENGTH, "32"), + (CONTENT_RANGE, "bytes 0-31/64"), + ], ); - assert_eq!(headers.get(CONTENT_LENGTH).unwrap(), "512"); - assert_eq!(headers.get(CONTENT_RANGE).unwrap(), "bytes 0-511/1024",); // Second half - let response = ranged_get_request(512, 512, 1024); + let mut response = ranged_get_request(32, 32, 64); assert_eq!(response.status(), StatusCode::PARTIAL_CONTENT); - let headers = response.headers(); - println!("Headers: {headers:#?}"); - assert_eq!(headers.len(), 4); - assert_eq!(headers.get(ACCEPT_RANGES).unwrap(), "bytes"); - assert_eq!( - headers.get(CONTENT_TYPE).unwrap(), - "application/octet-stream" + expect_data(response.body_mut(), 32, 32).await; + expect_headers( + response.headers(), + &[ + (ACCEPT_RANGES, "bytes"), + (CONTENT_TYPE, "application/octet-stream"), + (CONTENT_LENGTH, "32"), + (CONTENT_RANGE, "bytes 32-63/64"), + ], ); - assert_eq!(headers.get(CONTENT_LENGTH).unwrap(), "512"); - assert_eq!(headers.get(CONTENT_RANGE).unwrap(), "bytes 512-1023/1024",); // Partially out of bounds - let response = ranged_get_request(1000, 512, 1024); + let mut response = ranged_get_request(60, 32, 64); assert_eq!(response.status(), StatusCode::PARTIAL_CONTENT); - let headers = response.headers(); - println!("Headers: {headers:#?}"); - assert_eq!(headers.len(), 4); - assert_eq!(headers.get(ACCEPT_RANGES).unwrap(), "bytes"); - assert_eq!( - headers.get(CONTENT_TYPE).unwrap(), - "application/octet-stream" + expect_data(response.body_mut(), 60, 4).await; + expect_headers( + response.headers(), + &[ + (ACCEPT_RANGES, "bytes"), + (CONTENT_TYPE, "application/octet-stream"), + (CONTENT_LENGTH, "4"), + (CONTENT_RANGE, "bytes 60-63/64"), + ], ); - assert_eq!(headers.get(CONTENT_LENGTH).unwrap(), "24"); - assert_eq!(headers.get(CONTENT_RANGE).unwrap(), "bytes 1000-1023/1024",); // Fully out of bounds assert!(matches!( SingleRange::new( - http_range::HttpRange { start: 1024, length: 512 }, - 1024 + http_range::HttpRange { start: 64, length: 32 }, + 64 ) .expect_err("Should have thrown an error"), Error::Parse(http_range::HttpRangeParseError::InvalidRange) From 9bf3f5ce3e27b36eb0d38589cd12c977ab931b17 Mon Sep 17 00:00:00 2001 From: Sean Klein Date: Fri, 1 Nov 2024 16:44:15 -0700 Subject: [PATCH 6/8] Feedback --- Cargo.lock | 1 + range-requests/Cargo.toml | 1 + range-requests/src/lib.rs | 138 +++++++++++++++++++++++++++++++------- 3 files changed, 115 insertions(+), 25 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index a463218043..b409b28c8f 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -8920,6 +8920,7 @@ dependencies = [ "http-range", "hyper 1.4.1", "omicron-workspace-hack", + "proptest", "thiserror", "tokio", "tokio-util", diff --git a/range-requests/Cargo.toml b/range-requests/Cargo.toml index aab9692905..ab9972180d 100644 --- a/range-requests/Cargo.toml +++ b/range-requests/Cargo.toml @@ -21,5 +21,6 @@ omicron-workspace-hack.workspace = true [dev-dependencies] http-body.workspace = true +proptest.workspace = true tokio.workspace = true tokio-util.workspace = true diff --git a/range-requests/src/lib.rs b/range-requests/src/lib.rs index 888d7015c1..20b871dff8 100644 --- a/range-requests/src/lib.rs +++ b/range-requests/src/lib.rs @@ -5,18 +5,33 @@ use dropshot::Body; use dropshot::HttpError; use futures::TryStreamExt; +use http::HeaderValue; use hyper::{ header::{ACCEPT_RANGES, CONTENT_LENGTH, CONTENT_RANGE, CONTENT_TYPE}, Response, StatusCode, }; +const ACCEPT_RANGES_BYTES: http::HeaderValue = + http::HeaderValue::from_static("bytes"); +const CONTENT_TYPE_OCTET_STREAM: http::HeaderValue = + http::HeaderValue::from_static("application/octet-stream"); + /// Errors which may be returned when processing range requests #[derive(thiserror::Error, Debug)] pub enum Error { #[error("Using multiple ranges is not supported")] MultipleRangesUnsupported, - #[error("Failed to parse range")] + #[error("Range would overflow (start + length is too large)")] + RangeOverflow, + + #[error("Range would underflow (total content length < start)")] + RangeUnderflow, + + #[error("Empty Range")] + EmptyRange, + + #[error("Failed to parse range: {0:?}")] Parse(http_range::HttpRangeParseError), #[error(transparent)] @@ -26,9 +41,21 @@ pub enum Error { impl From for HttpError { fn from(err: Error) -> Self { match err { - Error::MultipleRangesUnsupported | Error::Parse(_) => { + Error::MultipleRangesUnsupported => { HttpError::for_bad_request(None, err.to_string()) } + Error::RangeUnderflow + | Error::RangeOverflow + | Error::EmptyRange => HttpError::for_client_error( + None, + http::StatusCode::RANGE_NOT_SATISFIABLE, + "Range was parseable, but invalid".to_string(), + ), + Error::Parse(_) => HttpError::for_client_error( + None, + http::StatusCode::RANGE_NOT_SATISFIABLE, + "Range cannot be parsed".to_string(), + ), Error::Http(err) => err.into(), } } @@ -37,10 +64,10 @@ impl From for HttpError { fn bad_range_response(file_size: u64) -> Response { hyper::Response::builder() .status(StatusCode::RANGE_NOT_SATISFIABLE) - .header(ACCEPT_RANGES, "bytes") + .header(ACCEPT_RANGES, ACCEPT_RANGES_BYTES) .header(CONTENT_RANGE, format!("bytes */{file_size}")) .body(Body::empty()) - .unwrap() + .expect("'bad range response' creation should be infallible") } /// Generate a GET response, optionally for a HTTP range request. The total @@ -53,7 +80,7 @@ fn bad_range_response(file_size: u64) -> Response { pub fn make_get_response( range: Option, file_length: u64, - content_type: Option<&str>, + content_type: Option>, rx: S, ) -> Result, Error> where @@ -74,7 +101,7 @@ where pub fn make_head_response( range: Option, file_length: u64, - content_type: Option<&str>, + content_type: Option>, ) -> Result, Error> { Ok(make_response_common(range, file_length, content_type) .body(Body::empty())?) @@ -83,13 +110,13 @@ pub fn make_head_response( fn make_response_common( range: Option, file_length: u64, - content_type: Option<&str>, + content_type: Option>, ) -> hyper::http::response::Builder { let mut res = Response::builder(); - res = res.header(ACCEPT_RANGES, "bytes"); + res = res.header(ACCEPT_RANGES, ACCEPT_RANGES_BYTES); res = res.header( CONTENT_TYPE, - content_type.unwrap_or("application/octet-stream"), + content_type.map(|t| t.into()).unwrap_or(CONTENT_TYPE_OCTET_STREAM), ); if let Some(range) = range { @@ -157,16 +184,13 @@ impl SingleRange { ) -> Result { let http_range::HttpRange { start, mut length } = range; - const INVALID_RANGE: Error = - Error::Parse(http_range::HttpRangeParseError::InvalidRange); - // Clip the length to avoid going beyond the end of the total range - if start.checked_add(length).ok_or(INVALID_RANGE)? >= total { - length = total.checked_sub(start).ok_or(INVALID_RANGE)?; + if start.checked_add(length).ok_or(Error::RangeOverflow)? >= total { + length = total.checked_sub(start).ok_or(Error::RangeUnderflow)?; } // If the length is zero, we cannot satisfy the range request if length == 0 { - return Err(INVALID_RANGE); + return Err(Error::EmptyRange); } Ok(Self { range: http_range::HttpRange { start, length }, total }) @@ -184,26 +208,32 @@ impl SingleRange { self.range .start .checked_add(self.range.length) - .unwrap() + .expect("start + length overflowed, but should have been checked in 'SingleRange::new'") .checked_sub(1) - .unwrap() + .expect("start + length underflowed, but should have been checked in 'SingleRange::new'") } /// Generate the Content-Range header for inclusion in a HTTP 206 partial /// content response using this range. - pub fn to_content_range(&self) -> String { - format!( + pub fn to_content_range(&self) -> HeaderValue { + HeaderValue::from_str(&format!( "bytes {}-{}/{}", self.range.start, self.end_inclusive(), self.total - ) + )) + .expect("Content-Range value should have been ASCII string") } /// Generate a Range header for inclusion in another HTTP request; e.g., /// to a backend object store. - pub fn to_range(&self) -> String { - format!("bytes={}-{}", self.range.start, self.end_inclusive()) + pub fn to_range(&self) -> HeaderValue { + HeaderValue::from_str(&format!( + "bytes={}-{}", + self.range.start, + self.end_inclusive() + )) + .expect("Range bounds should have been ASCII string") } pub fn content_length(&self) -> u64 { @@ -240,9 +270,67 @@ mod test { use bytes::Bytes; use futures::stream::once; use http_body_util::BodyExt; + use proptest::prelude::*; use std::convert::Infallible; use tokio_util::io::ReaderStream; + proptest! { + #[test] + fn potential_range_parsing_does_not_crash( + bytes: Vec, + len in 0_u64..=u64::MAX, + ) { + let result = PotentialRange(bytes).parse(len); + let Ok(range) = result else { return Ok(()); }; + let _ = range.start(); + let _ = range.end_inclusive(); + let _ = range.to_content_range(); + let _ = range.to_range(); + } + + #[test] + fn single_range_parsing_does_not_crash( + start in 0_u64..=u64::MAX, + length in 0_u64..=u64::MAX, + total in 0_u64..=u64::MAX + ) { + let result = SingleRange::new(http_range::HttpRange { + start, length + }, total); + + let Ok(range) = result else { return Ok(()); }; + + assert_eq!(range.start(), start); + let _ = range.end_inclusive(); + let _ = range.to_content_range(); + let _ = range.to_range(); + } + } + + #[test] + fn invalid_ranges() { + assert!(matches!( + SingleRange::new( + http_range::HttpRange { start: u64::MAX, length: 1 }, + 1 + ), + Err(Error::RangeOverflow) + )); + + assert!(matches!( + SingleRange::new( + http_range::HttpRange { start: 100, length: 0 }, + 10 + ), + Err(Error::RangeUnderflow) + )); + + assert!(matches!( + SingleRange::new(http_range::HttpRange { start: 0, length: 0 }, 1), + Err(Error::EmptyRange) + )); + } + #[test] fn parse_range_valid() { // Whole range @@ -298,7 +386,7 @@ mod test { let response = make_get_response( None, bytes.len() as u64, - None, + None::, ReaderStream::new(bytes.as_slice()), ) .expect("Should have made response"); @@ -340,7 +428,7 @@ mod test { let response = make_get_response( Some(range.clone()), total_length.into(), - None, + None::, once(async move { Ok::<_, Infallible>(b) }), ) .expect("Should have made response"); @@ -435,7 +523,7 @@ mod test { 64 ) .expect_err("Should have thrown an error"), - Error::Parse(http_range::HttpRangeParseError::InvalidRange) + Error::EmptyRange, )); } } From c195b0300cd770d904c1397f492eff7e51543e96 Mon Sep 17 00:00:00 2001 From: Sean Klein Date: Fri, 1 Nov 2024 16:49:04 -0700 Subject: [PATCH 7/8] nonzero --- range-requests/src/lib.rs | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/range-requests/src/lib.rs b/range-requests/src/lib.rs index 20b871dff8..bef482a6d3 100644 --- a/range-requests/src/lib.rs +++ b/range-requests/src/lib.rs @@ -236,10 +236,11 @@ impl SingleRange { .expect("Range bounds should have been ASCII string") } - pub fn content_length(&self) -> u64 { - assert!(self.range.length > 0); - - self.range.length + /// Returns the content length for this range + pub fn content_length(&self) -> std::num::NonZeroU64 { + self.range.length.try_into().expect( + "Length should be more than zero, validated in SingleRange::new", + ) } } From 6c2562d95f1a78eb9842c2448a9630fba7c03900 Mon Sep 17 00:00:00 2001 From: Sean Klein Date: Mon, 4 Nov 2024 15:02:52 -0800 Subject: [PATCH 8/8] Make it harder to see 'not satisfiable' without corresponding headers --- range-requests/src/lib.rs | 60 ++++++++++++++++++++------------------- 1 file changed, 31 insertions(+), 29 deletions(-) diff --git a/range-requests/src/lib.rs b/range-requests/src/lib.rs index bef482a6d3..ccd250d949 100644 --- a/range-requests/src/lib.rs +++ b/range-requests/src/lib.rs @@ -3,7 +3,6 @@ // file, You can obtain one at https://mozilla.org/MPL/2.0/. use dropshot::Body; -use dropshot::HttpError; use futures::TryStreamExt; use http::HeaderValue; use hyper::{ @@ -38,36 +37,34 @@ pub enum Error { Http(#[from] http::Error), } -impl From for HttpError { - fn from(err: Error) -> Self { - match err { - Error::MultipleRangesUnsupported => { - HttpError::for_bad_request(None, err.to_string()) - } - Error::RangeUnderflow - | Error::RangeOverflow - | Error::EmptyRange => HttpError::for_client_error( - None, - http::StatusCode::RANGE_NOT_SATISFIABLE, - "Range was parseable, but invalid".to_string(), - ), - Error::Parse(_) => HttpError::for_client_error( - None, - http::StatusCode::RANGE_NOT_SATISFIABLE, - "Range cannot be parsed".to_string(), - ), - Error::Http(err) => err.into(), - } - } +// TODO(https://github.com/oxidecomputer/dropshot/issues/39): Return a dropshot +// type here (HttpError?) to e.g. include the RequestID in the response. +// +// Same for the other functions returning "Response" below - we're doing +// this so the "RANGE_NOT_SATISFIABLE" response can attach extra info, but it's +// currently happening at the expense of headers that Dropshot wants to supply. + +fn bad_request_response() -> Response { + hyper::Response::builder() + .status(StatusCode::BAD_REQUEST) + .body(Body::empty()) + .expect("'bad request response' creation should be infallible") +} + +fn internal_error_response() -> Response { + hyper::Response::builder() + .status(StatusCode::INTERNAL_SERVER_ERROR) + .body(Body::empty()) + .expect("'internal error response' creation should be infallible") } -fn bad_range_response(file_size: u64) -> Response { +fn not_satisfiable_response(file_size: u64) -> Response { hyper::Response::builder() .status(StatusCode::RANGE_NOT_SATISFIABLE) .header(ACCEPT_RANGES, ACCEPT_RANGES_BYTES) .header(CONTENT_RANGE, format!("bytes */{file_size}")) .body(Body::empty()) - .expect("'bad range response' creation should be infallible") + .expect("'not satisfiable response' creation should be infallible") } /// Generate a GET response, optionally for a HTTP range request. The total @@ -142,7 +139,15 @@ impl PotentialRange { /// On failure, returns a range response with the appropriate headers /// to inform the caller how to make a correct range request. pub fn parse(&self, len: u64) -> Result> { - self.single_range(len).map_err(|_err| bad_range_response(len)) + self.single_range(len).map_err(|err| match err { + Error::MultipleRangesUnsupported | Error::Parse(_) => { + bad_request_response() + } + Error::RangeOverflow + | Error::RangeUnderflow + | Error::EmptyRange => not_satisfiable_response(len), + Error::Http(_err) => internal_error_response(), + }) } fn single_range(&self, len: u64) -> Result { @@ -178,10 +183,7 @@ impl PartialEq for SingleRange { } impl SingleRange { - pub fn new( - range: http_range::HttpRange, - total: u64, - ) -> Result { + fn new(range: http_range::HttpRange, total: u64) -> Result { let http_range::HttpRange { start, mut length } = range; // Clip the length to avoid going beyond the end of the total range