diff --git a/Cargo.lock b/Cargo.lock index c05bbd7fb..b3f4fcf30 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -145,9 +145,9 @@ dependencies = [ "futures 0.1.18 (registry+https://github.com/rust-lang/crates.io-index)", "futures-mpsc-lossy 0.3.0", "h2 0.1.0 (registry+https://github.com/rust-lang/crates.io-index)", - "http 0.1.4 (registry+https://github.com/rust-lang/crates.io-index)", + "http 0.1.5 (registry+https://github.com/rust-lang/crates.io-index)", "httparse 1.2.4 (registry+https://github.com/rust-lang/crates.io-index)", - "hyper 0.11.19 (registry+https://github.com/rust-lang/crates.io-index)", + "hyper 0.11.20 (registry+https://github.com/rust-lang/crates.io-index)", "indexmap 0.4.1 (registry+https://github.com/rust-lang/crates.io-index)", "ipnet 1.0.0 (registry+https://github.com/rust-lang/crates.io-index)", "libc 0.2.36 (registry+https://github.com/rust-lang/crates.io-index)", @@ -179,7 +179,7 @@ dependencies = [ "convert 0.3.0", "futures 0.1.18 (registry+https://github.com/rust-lang/crates.io-index)", "h2 0.1.0 (registry+https://github.com/rust-lang/crates.io-index)", - "http 0.1.4 (registry+https://github.com/rust-lang/crates.io-index)", + "http 0.1.5 (registry+https://github.com/rust-lang/crates.io-index)", "prost 0.3.0 (registry+https://github.com/rust-lang/crates.io-index)", "prost-derive 0.3.0 (registry+https://github.com/rust-lang/crates.io-index)", "prost-types 0.3.0 (registry+https://github.com/rust-lang/crates.io-index)", @@ -357,7 +357,7 @@ dependencies = [ "bytes 0.4.6 (registry+https://github.com/rust-lang/crates.io-index)", "fnv 1.0.6 (registry+https://github.com/rust-lang/crates.io-index)", "futures 0.1.18 (registry+https://github.com/rust-lang/crates.io-index)", - "http 0.1.4 (registry+https://github.com/rust-lang/crates.io-index)", + "http 0.1.5 (registry+https://github.com/rust-lang/crates.io-index)", "log 0.3.9 (registry+https://github.com/rust-lang/crates.io-index)", "ordermap 0.2.13 (registry+https://github.com/rust-lang/crates.io-index)", "slab 0.4.0 (registry+https://github.com/rust-lang/crates.io-index)", @@ -375,7 +375,7 @@ dependencies = [ [[package]] name = "http" -version = "0.1.4" +version = "0.1.5" source = "registry+https://github.com/rust-lang/crates.io-index" dependencies = [ "bytes 0.4.6 (registry+https://github.com/rust-lang/crates.io-index)", @@ -389,14 +389,14 @@ source = "registry+https://github.com/rust-lang/crates.io-index" [[package]] name = "hyper" -version = "0.11.19" +version = "0.11.20" source = "registry+https://github.com/rust-lang/crates.io-index" dependencies = [ "base64 0.9.0 (registry+https://github.com/rust-lang/crates.io-index)", "bytes 0.4.6 (registry+https://github.com/rust-lang/crates.io-index)", "futures 0.1.18 (registry+https://github.com/rust-lang/crates.io-index)", "futures-cpupool 0.1.8 (registry+https://github.com/rust-lang/crates.io-index)", - "http 0.1.4 (registry+https://github.com/rust-lang/crates.io-index)", + "http 0.1.5 (registry+https://github.com/rust-lang/crates.io-index)", "httparse 1.2.4 (registry+https://github.com/rust-lang/crates.io-index)", "iovec 0.1.2 (registry+https://github.com/rust-lang/crates.io-index)", "language-tags 0.2.2 (registry+https://github.com/rust-lang/crates.io-index)", @@ -1049,7 +1049,7 @@ dependencies = [ "bytes 0.4.6 (registry+https://github.com/rust-lang/crates.io-index)", "futures 0.1.18 (registry+https://github.com/rust-lang/crates.io-index)", "h2 0.1.0 (registry+https://github.com/rust-lang/crates.io-index)", - "http 0.1.4 (registry+https://github.com/rust-lang/crates.io-index)", + "http 0.1.5 (registry+https://github.com/rust-lang/crates.io-index)", "log 0.3.9 (registry+https://github.com/rust-lang/crates.io-index)", "prost 0.3.0 (registry+https://github.com/rust-lang/crates.io-index)", "tower 0.1.0 (git+https://github.com/tower-rs/tower)", @@ -1075,7 +1075,7 @@ dependencies = [ "bytes 0.4.6 (registry+https://github.com/rust-lang/crates.io-index)", "futures 0.1.18 (registry+https://github.com/rust-lang/crates.io-index)", "h2 0.1.0 (registry+https://github.com/rust-lang/crates.io-index)", - "http 0.1.4 (registry+https://github.com/rust-lang/crates.io-index)", + "http 0.1.5 (registry+https://github.com/rust-lang/crates.io-index)", "log 0.3.9 (registry+https://github.com/rust-lang/crates.io-index)", "tokio-connect 0.1.0 (git+https://github.com/carllerche/tokio-connect)", "tokio-core 0.1.12 (registry+https://github.com/rust-lang/crates.io-index)", @@ -1267,9 +1267,9 @@ dependencies = [ "checksum futures-cpupool 0.1.8 (registry+https://github.com/rust-lang/crates.io-index)" = "ab90cde24b3319636588d0c35fe03b1333857621051837ed769faefb4c2162e4" "checksum h2 0.1.0 (registry+https://github.com/rust-lang/crates.io-index)" = "5617f23e03f04b44147b0dee52d1146e61b5044994659dedf71246ccd34eb48e" "checksum heck 0.3.0 (registry+https://github.com/rust-lang/crates.io-index)" = "ea04fa3ead4e05e51a7c806fc07271fdbde4e246a6c6d1efd52e72230b771b82" -"checksum http 0.1.4 (registry+https://github.com/rust-lang/crates.io-index)" = "bf8217d8829cc05dedadc08b4bc0684e5e3fbba1126c5edc680af49053fa230c" +"checksum http 0.1.5 (registry+https://github.com/rust-lang/crates.io-index)" = "75df369fd52c60635208a4d3e694777c099569b3dcf4844df8f652dc004644ab" "checksum httparse 1.2.4 (registry+https://github.com/rust-lang/crates.io-index)" = "c2f407128745b78abc95c0ffbe4e5d37427fdc0d45470710cfef8c44522a2e37" -"checksum hyper 0.11.19 (registry+https://github.com/rust-lang/crates.io-index)" = "47659bb1cb7ef3cd7b4f9bd2a11349b8d92097d34f9597a3c09e9bcefaf92b61" +"checksum hyper 0.11.20 (registry+https://github.com/rust-lang/crates.io-index)" = "1545100ac38f42f338c63bff290d7285b7117021a564b3c0f34e8d57cf81451f" "checksum indexmap 0.4.1 (registry+https://github.com/rust-lang/crates.io-index)" = "7164c96d6e18ccc3ce43f3dedac996c21a220670a106c275b96ad92110401362" "checksum iovec 0.1.2 (registry+https://github.com/rust-lang/crates.io-index)" = "dbe6e417e7d0975db6512b90796e8ce223145ac4e33c377e4a42882a0e88bb08" "checksum ipnet 1.0.0 (registry+https://github.com/rust-lang/crates.io-index)" = "51268c3a27ad46afd1cca0bbf423a5be2e9fd3e6a7534736c195f0f834b763ef" diff --git a/proxy/Cargo.toml b/proxy/Cargo.toml index ce8c070da..34479f033 100644 --- a/proxy/Cargo.toml +++ b/proxy/Cargo.toml @@ -22,7 +22,7 @@ futures = "0.1" h2 = "0.1" http = "0.1" httparse = "1.2" -hyper = { version = "0.11.19", default-features = false, features = ["compat"] } +hyper = { version = "0.11.20", default-features = false, features = ["compat"] } ipnet = "1.0" log = "0.4.1" indexmap = "0.4.1" diff --git a/proxy/src/transparency/client.rs b/proxy/src/transparency/client.rs index f01d135e3..1cda49cd7 100644 --- a/proxy/src/transparency/client.rs +++ b/proxy/src/transparency/client.rs @@ -169,12 +169,38 @@ where } fn call(&mut self, req: Self::Request) -> Self::Future { + use http::header::CONTENT_LENGTH; + match self.inner { ClientServiceInner::Http1(ref h1) => { - let is_body_empty = req.body().is_end_stream(); + // As of hyper 0.11.x, a set body implies a body must be sent. + // If there is no content-length header, hyper adds a + // transfer-encoding: chunked header, since it has no other way + // of knowing if the body is empty or not. + // + // However, if we received a `Content-Length: 0` body ourselves, + // the `body.is_end_stream()` would be true. While its true that + // semantically a client request with no body headers and a request + // with `Content-Length: 0` have the exact same body length, we + // want to preserve the exact intent of the original request, as + // we are trying to be a transparent proxy. + // + // So, if `Content-Length` is set at all, we need to send it. If + // we unset the body, hyper assumes the RFC7230 semantics that + // it can strip content body headers. Therefore, even if the + // body is empty, it should still be passed to hyper so that + // the `Content-Length: 0` header is not stripped. + // + // This does *NOT* check for `Transfer-Encoding` as future-proofing + // measure. When we add the ability to proxy HTTP2 to HTTP1, this + // body could have come from h2, where there is no `Transfer-Encoding`. + // Instead, it has been replaced by `DATA` frames. The + // `HttpBody::is_end_stream()` manages that distinction for us. + let should_take_body = req.body().is_end_stream() + && !req.headers().contains_key(CONTENT_LENGTH); let mut req = hyper::Request::from(req.map(BodyStream::new)); - if is_body_empty { - req.headers_mut().set(hyper::header::ContentLength(0)); + if should_take_body { + req.body_mut().take(); } ClientServiceFuture::Http1(h1.request(req)) }, diff --git a/proxy/src/transparency/glue.rs b/proxy/src/transparency/glue.rs index 2b9b3dc83..436c9583d 100644 --- a/proxy/src/transparency/glue.rs +++ b/proxy/src/transparency/glue.rs @@ -81,7 +81,7 @@ impl tower_h2::Body for HttpBody { fn is_end_stream(&self) -> bool { match *self { - HttpBody::Http1(_) => false, + HttpBody::Http1(ref b) => b.is_empty(), HttpBody::Http2(ref b) => b.is_end_stream(), } } diff --git a/proxy/tests/support/client.rs b/proxy/tests/support/client.rs index 6d7698aed..0c467cd84 100644 --- a/proxy/tests/support/client.rs +++ b/proxy/tests/support/client.rs @@ -102,6 +102,9 @@ fn run(addr: SocketAddr, version: Run) -> Sender { .build(&reactor); Box::new(rx.for_each(move |(req, cb)| { let mut req = hyper::Request::from(req.map(|()| hyper::Body::empty())); + if !req.headers().has::() { + assert!(req.body_mut().take().unwrap().is_empty()); + } if absolute_uris { req.set_proxy(true); } diff --git a/proxy/tests/support/mod.rs b/proxy/tests/support/mod.rs index a6b69bdb6..21f6a951e 100644 --- a/proxy/tests/support/mod.rs +++ b/proxy/tests/support/mod.rs @@ -20,7 +20,7 @@ use self::bytes::{BigEndian, Bytes, BytesMut}; pub use self::conduit_proxy::*; pub use self::futures::*; use self::futures::sync::oneshot; -pub use self::http::{HeaderMap, Request, Response}; +pub use self::http::{HeaderMap, Request, Response, StatusCode}; use self::http::header::HeaderValue; use self::tokio_connect::Connect; use self::tokio_core::net::{TcpListener, TcpStream}; diff --git a/proxy/tests/transparency.rs b/proxy/tests/transparency.rs index 373a5d335..a0fc82301 100644 --- a/proxy/tests/transparency.rs +++ b/proxy/tests/transparency.rs @@ -345,13 +345,21 @@ fn http11_upgrade_not_supported() { } #[test] -fn http1_get_doesnt_add_transfer_encoding() { +fn http1_requests_without_body_doesnt_add_transfer_encoding() { let _ = env_logger::try_init(); let srv = server::http1() .route_fn("/", |req| { - assert!(!req.headers().contains_key("transfer-encoding")); - Response::new("hello h1".into()) + let has_body_header = req.headers().contains_key("transfer-encoding") + || req.headers().contains_key("content-length"); + let status = if has_body_header { + StatusCode::BAD_REQUEST + } else { + StatusCode::OK + }; + let mut res = Response::new(Default::default()); + *res.status_mut() = status; + res }) .run(); let ctrl = controller::new().run(); @@ -360,5 +368,240 @@ fn http1_get_doesnt_add_transfer_encoding() { .inbound(srv) .run(); let client = client::http1(proxy.inbound, "transparency.test.svc.cluster.local"); - assert_eq!(client.get("/"), "hello h1"); + + let methods = &[ + "GET", + "POST", + "PUT", + "DELETE", + "HEAD", + "PATCH", + ]; + + for &method in methods { + let resp = client.request( + client + .request_builder("/") + .method(method) + ); + + assert_eq!(resp.status(), StatusCode::OK, "method={:?}", method); + } +} + +#[test] +fn http1_content_length_zero_is_preserved() { + let _ = env_logger::try_init(); + + let srv = server::http1() + .route_fn("/", |req| { + let status = if req.headers()["content-length"] == "0" { + StatusCode::OK + } else { + StatusCode::BAD_REQUEST + }; + Response::builder() + .status(status) + .header("content-length", "0") + .body("".into()) + .unwrap() + }) + .run(); + let ctrl = controller::new().run(); + let proxy = proxy::new() + .controller(ctrl) + .inbound(srv) + .run(); + let client = client::http1(proxy.inbound, "transparency.test.svc.cluster.local"); + + + let methods = &[ + "GET", + "POST", + "PUT", + "DELETE", + "HEAD", + "PATCH", + ]; + + for &method in methods { + let resp = client.request( + client + .request_builder("/") + .method(method) + .header("content-length", "0") + ); + + assert_eq!(resp.status(), StatusCode::OK, "method={:?}", method); + assert_eq!(resp.headers()["content-length"], "0", "method={:?}", method); + } +} + +#[test] +fn http1_bodyless_responses() { + let _ = env_logger::try_init(); + + let req_status_header = "x-test-status-requested"; + + let srv = server::http1() + .route_fn("/", move |req| { + let status = req.headers() + .get(req_status_header) + .map(|val| { + val.to_str() + .expect("req_status_header should be ascii") + .parse::() + .expect("req_status_header should be numbers") + }) + .unwrap_or(200); + + Response::builder() + .status(status) + .body("".into()) + .unwrap() + }) + .run(); + let ctrl = controller::new().run(); + let proxy = proxy::new() + .controller(ctrl) + .inbound(srv) + .run(); + let client = client::http1(proxy.inbound, "transparency.test.svc.cluster.local"); + + // https://tools.ietf.org/html/rfc7230#section-3.3.3 + // > response to a HEAD request, any 1xx, 204, or 304 cannot contain a body + + //TODO: the proxy doesn't support CONNECT requests yet, but when we do, + //they should be tested here as well. As RFC7230 says, a 2xx response to + //a CONNECT request is not allowed to contain a body (but 4xx, 5xx can!). + + let resp = client.request( + client + .request_builder("/") + .method("HEAD") + ); + + assert_eq!(resp.status(), StatusCode::OK); + assert!(!resp.headers().contains_key("content-length")); + assert!(!resp.headers().contains_key("transfer-encoding")); + + let statuses = &[ + //TODO: test some 1xx status codes. + //The current test server doesn't support sending 1xx responses + //easily. We could test this by making a new unit test with the + //server being a TCP server, and write the response manually. + StatusCode::NO_CONTENT, // 204 + StatusCode::NOT_MODIFIED, // 304 + ]; + + for &status in statuses { + let resp = client.request( + client + .request_builder("/") + .header(req_status_header, status.as_str()) + ); + + assert_eq!(resp.status(), status); + assert!(!resp.headers().contains_key("content-length"), "content-length with status={:?}", status); + assert!(!resp.headers().contains_key("transfer-encoding"), "transfer-encoding with status={:?}", status); + } +} + +#[test] +fn http1_head_responses() { + let _ = env_logger::try_init(); + + let srv = server::http1() + .route_fn("/", move |req| { + assert_eq!(req.method(), "HEAD"); + Response::builder() + .header("content-length", "55") + .body("".into()) + .unwrap() + }) + .run(); + let ctrl = controller::new().run(); + let proxy = proxy::new() + .controller(ctrl) + .inbound(srv) + .run(); + let client = client::http1(proxy.inbound, "transparency.test.svc.cluster.local"); + + let resp = client.request( + client + .request_builder("/") + .method("HEAD") + ); + + assert_eq!(resp.status(), StatusCode::OK); + assert_eq!(resp.headers()["content-length"], "55"); + + let body = resp.into_body() + .concat2() + .wait() + .expect("response body concat"); + + assert_eq!(body, ""); +} + +#[test] +fn http1_response_end_of_file() { + let _ = env_logger::try_init(); + + // test both http/1.0 and 1.1 + let srv = server::tcp() + .accept(move |_read| { + "\ + HTTP/1.0 200 OK\r\n\ + \r\n\ + body till eof\ + " + }) + .accept(move |_read| { + "\ + HTTP/1.1 200 OK\r\n\ + \r\n\ + body till eof\ + " + }) + .run(); + let ctrl = controller::new().run(); + let proxy = proxy::new() + .controller(ctrl) + .inbound(srv) + .run(); + + let client = client::http1(proxy.inbound, "transparency.test.svc.cluster.local"); + + let versions = &[ + "1.0", + // TODO: We may wish to enforce not translating eof bodies to chunked, + // even if the client is 1.1. However, there also benefits of translating: + // the client can reuse the connection, and delimited messages are much + // safer than those that end with the connection (as it's difficult to + // notice if a full response was received). + // + // Either way, hyper's server does not provide the ability to do this, + // so we cannot test for it at the moment. + //"1.1", + ]; + + for v in versions { + let resp = client.request( + client + .request_builder("/") + .method("GET") + ); + + assert_eq!(resp.status(), StatusCode::OK, "HTTP/{}", v); + assert!(!resp.headers().contains_key("transfer-encoding"), "HTTP/{} transfer-encoding", v); + assert!(!resp.headers().contains_key("content-length"), "HTTP/{} content-length", v); + + let body = resp.into_body() + .concat2() + .wait() + .expect("response body concat"); + + assert_eq!(body, "body till eof", "HTTP/{} body", v); + } }