mirror of https://github.com/linkerd/linkerd2.git
proxy: preserve body headers in http1 (#457)
As a goal of being a transparent proxy, we want to proxy requests and responses with as little modification as possible. Basically, servers and clients should see messages that look the same whether the proxy was injected or not. With that goal in mind, we want to make sure that body headers (things like `Content-Length`, `Transfer-Encoding`, etc) are left alone. Prior to this commit, we at times were changing behavior. Sometimes `Transfer-Encoding` was added to requests, or `Content-Length: 0` may have been removed. While RC 7230 defines that differences are semantically the same, implementations may not handle them correctly. Now, we've added some fixes to prevent any of these header changes from occurring, along with tests to make sure library updates don't regress. For requests: - With no message body, `Transfer-Encoding: chunked` should no longer be added. - With `Content-Length: 0`, the header is forwarded untouched. For responses: - Tests were added that responses not allowed to have bodies (to HEAD requests, 204, 304) did not have `Transfer-Encoding` added. - Tests that `Content-Length: 0` is preserved. - Tests that HTTP/1.0 responses with no body headers do not have `Transfer-Encoding` added. - Tests that `HEAD` responses forward `Content-Length` headers (but not an actual body). Closes #447 Signed-off-by: Sean McArthur <sean@seanmonstar.com>
This commit is contained in:
Sean McArthur
GitHub
parent
5a4c5aa683
commit
c278228c1b
|
|
@ -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"
|
||||
|
|
|
|||
|
|
@ -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"
|
||||
|
|
|
|||
|
|
@ -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))
|
||||
},
|
||||
|
|
|
|||
|
|
@ -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(),
|
||||
}
|
||||
}
|
||||
|
|
|
|||
|
|
@ -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::<hyper::header::ContentLength>() {
|
||||
assert!(req.body_mut().take().unwrap().is_empty());
|
||||
}
|
||||
if absolute_uris {
|
||||
req.set_proxy(true);
|
||||
}
|
||||
|
|
|
|||
|
|
@ -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};
|
||||
|
|
|
|||
|
|
@ -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::<u16>()
|
||||
.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);
|
||||
}
|
||||
}
|
||||
|
|
|
|||
Loading…
Reference in New Issue