proxy: improve transparency of host headers and absolute-uris (linker…

…d#535)

In some cases, we would adjust an existing Host header, or add one. And in all cases when an HTTP/1 request was received with an absolute-form target, it was not passed on.

Now, the Host header is never changed. And if the Uri was in absolute-form, it is sent in the same format.

Closes linkerd#518

proxy/Cargo.toml

@@ -22,7 +22,7 @@ futures = "0.1"
 h2 = "0.1"
 http = "0.1"
 httparse = "1.2"
-hyper = { version = "0.11.21", default-features = false, features = ["compat"] }
+hyper = { version = "0.11.22", default-features = false, features = ["compat"] }
 ipnet = "1.0"
 log = "0.4.1"
 indexmap = "0.4.1"

proxy/src/bind.rs

@@ -6,8 +6,8 @@ use std::net::SocketAddr;
 use std::sync::Arc;
 use std::sync::atomic::AtomicUsize;
 
-use futures::{future, Future, Poll};
-use futures::future::{Either, Map};
+use futures::{Future, Poll};
+use futures::future::Map;
 use http::{self, uri};
 use tokio_core::reactor::Handle;
 use tower;
@@ -69,11 +69,11 @@ pub enum Host {
 ///   the authority given in the `Host:` header, or, failing that, from the
 ///   request's original destination according to `SO_ORIGINAL_DST`.
 #[derive(Copy, Clone, Debug)]
-pub struct ReconstructUri<S> {
+pub struct NormalizeUri<S> {
     inner: S
 }
 
-pub type Service<B> = Reconnect<ReconstructUri<NewHttp<B>>>;
+pub type Service<B> = Reconnect<NormalizeUri<NewHttp<B>>>;
 
 pub type NewHttp<B> = sensor::NewHttp<Client<B>, B, HttpBody>;
 
@@ -204,7 +204,7 @@ where
 
         // Rewrite the HTTP/1 URI, if the authorities in the Host header
         // and request URI are not in agreement, or are not present.
-        let proxy = ReconstructUri::new(sensors);
+        let proxy = NormalizeUri::new(sensors);
 
         // Automatically perform reconnects if the connection fails.
         //
@@ -241,16 +241,16 @@ where
 }
 
 
-// ===== impl ReconstructUri =====
+// ===== impl NormalizeUri =====
 
 
-impl<S> ReconstructUri<S> {
+impl<S> NormalizeUri<S> {
     fn new (inner: S) -> Self {
         Self { inner }
     }
 }
 
-impl<S, B> tower::NewService for ReconstructUri<S>
+impl<S, B> tower::NewService for NormalizeUri<S>
 where
     S: tower::NewService<
         Request=http::Request<B>,
@@ -260,24 +260,24 @@ where
         Request=http::Request<B>,
         Response=HttpResponse,
     >,
-    ReconstructUri<S::Service>: tower::Service,
+    NormalizeUri<S::Service>: tower::Service,
     B: tower_h2::Body,
 {
     type Request = <Self::Service as tower::Service>::Request;
     type Response = <Self::Service as tower::Service>::Response;
     type Error = <Self::Service as tower::Service>::Error;
-    type Service = ReconstructUri<S::Service>;
+    type Service = NormalizeUri<S::Service>;
     type InitError = S::InitError;
     type Future = Map<
         S::Future,
-        fn(S::Service) -> ReconstructUri<S::Service>
+        fn(S::Service) -> NormalizeUri<S::Service>
     >;
     fn new_service(&self) -> Self::Future {
-        self.inner.new_service().map(ReconstructUri::new)
+        self.inner.new_service().map(NormalizeUri::new)
     }
 }
 
-impl<S, B> tower::Service for ReconstructUri<S>
+impl<S, B> tower::Service for NormalizeUri<S>
 where
     S: tower::Service<
         Request=http::Request<B>,
@@ -288,29 +288,17 @@ where
     type Request = S::Request;
     type Response = HttpResponse;
     type Error = S::Error;
-    type Future = Either<
-        S::Future,
-        future::FutureResult<Self::Response, Self::Error>,
-    >;
+    type Future = S::Future;
 
     fn poll_ready(&mut self) -> Poll<(), S::Error> {
         self.inner.poll_ready()
     }
 
     fn call(&mut self, mut request: S::Request) -> Self::Future {
-        if request.version() == http::Version::HTTP_2 {
-            // skip `reconstruct_uri` entirely if the request is HTTP/2.
-            return Either::A(self.inner.call(request));
-        }
-
-        if let Err(_) = h1::reconstruct_uri(&mut request) {
-            let res = http::Response::builder()
-                .status(http::StatusCode::BAD_REQUEST)
-                .body(Default::default())
-                .unwrap();
-            return Either::B(future::ok(res));
+        if request.version() != http::Version::HTTP_2 {
+            h1::normalize_our_view_of_uri(&mut request);
         }
-        Either::A(self.inner.call(request))
+        self.inner.call(request)
     }
 }
 

proxy/src/transparency/client.rs

@@ -9,6 +9,7 @@ use tower_h2;
 
 use bind;
 use super::glue::{BodyStream, HttpBody, HyperConnect};
+use super::h1::UriIsAbsoluteForm;
 
 /// A `NewService` that can speak either HTTP/1 or HTTP/2.
 pub struct Client<C, B>
@@ -83,6 +84,9 @@ where
                 let h1 = hyper::Client::configure()
                     .connector(HyperConnect::new(connect))
                     .body()
+                    // hyper should never try to automatically set the Host
+                    // header, instead always just passing whatever we received.
+                    .set_host(false)
                     .build(&executor);
                 Client {
                     inner: ClientInner::Http1(h1),
@@ -177,6 +181,12 @@ where
 
         match self.inner {
             ClientServiceInner::Http1(ref h1) => {
+                // This sentinel may be set in h1::normalize_our_view_of_uri
+                // when the original request-target was in absolute-form. In
+                // that case, for hyper 0.11.x, we need to call `req.set_proxy`.
+                let was_absolute_form = req.extensions()
+                    .get::<UriIsAbsoluteForm>()
+                    .is_some();
                 // 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
@@ -206,6 +216,7 @@ where
                 if should_take_body {
                     req.body_mut().take();
                 }
+                req.set_proxy(was_absolute_form);
                 ClientServiceFuture::Http1(h1.request(req))
             },
             ClientServiceInner::Http2(ref mut h2) => {

proxy/src/transparency/h1.rs

@@ -4,31 +4,29 @@ use std::sync::Arc;
 
 use bytes::BytesMut;
 use http;
-use http::header::{HeaderValue, HOST};
+use http::header::HOST;
 use http::uri::{Authority, Parts, Scheme, Uri};
 use ctx::transport::{Server as ServerCtx};
 
-pub fn reconstruct_uri<B>(req: &mut http::Request<B>) -> Result<(), ()> {
-    // RFC7230#section-5.4
-    // If an absolute-form uri is received, it must replace
-    // the host header
-    if let Some(auth) = req.uri().authority_part().cloned() {
-        if let Some(host) = req.headers().get(HOST) {
-            if auth.as_str().as_bytes() == host.as_bytes() {
-                // host and absolute-form agree, nothing more to do
-                return Ok(());
-            }
-        }
-        let host = HeaderValue::from_shared(auth.into_bytes())
-            .expect("a valid authority is valid header value");
-        req.headers_mut().insert(HOST, host);
-        return Ok(());
+/// Sentinel extension to signal that we should tell hyper to serialize
+/// the `Uri` in absolute-form.
+pub struct UriIsAbsoluteForm;
+
+/// Tries to make sure the `Uri` of the request is in a form needed by
+/// hyper's Client.
+///
+/// Also sets the `UriIsAbsoluteForm` extension if received `Uri` was
+/// already in absolute-form.
+pub fn normalize_our_view_of_uri<B>(req: &mut http::Request<B>) {
+    if req.uri().authority_part().is_some() {
+        req.extensions_mut().insert(UriIsAbsoluteForm);
+        return;
     }
 
     // try to parse the Host header
     if let Some(auth) = authority_from_host(&req) {
         set_authority(req.uri_mut(), auth);
-        return Ok(());
+        return;
     }
 
     // last resort is to use the so_original_dst
@@ -43,11 +41,7 @@ pub fn reconstruct_uri<B>(req: &mut http::Request<B>) -> Result<(), ()> {
         let auth = Authority::from_shared(bytes)
             .expect("socket address is valid authority");
         set_authority(req.uri_mut(), auth);
-
-        return Ok(());
     }
-
-    Err(())
 }
 
 /// Returns an Authority from a request's Host header.

proxy/tests/transparency.rs

@@ -117,53 +117,58 @@ fn http10_without_host() {
     let srv = server::http1()
         .route_fn("/", move |req| {
             assert_eq!(req.version(), http::Version::HTTP_10);
+            assert!(!req.headers().contains_key("host"));
+            assert_eq!(req.uri().to_string(), "/");
             Response::builder()
                 .version(http::Version::HTTP_10)
                 .body("".into())
                 .unwrap()
         })
         .run();
     let ctrl = controller::new()
-        .destination(&srv.addr.to_string(), srv.addr)
         .run();
     let proxy = proxy::new()
         .controller(ctrl)
-        .outbound(srv)
+        .inbound(srv)
         .run();
-    let client = client::http1(proxy.outbound, "foo.bar");
 
-    let res = client.request(client.request_builder("/")
-        .version(http::Version::HTTP_10)
-        .header("host", ""));
+    let client = client::tcp(proxy.inbound);
 
-    assert_eq!(res.status(), http::StatusCode::OK);
-    assert_eq!(res.version(), http::Version::HTTP_10);
+    let tcp_client = client.connect();
+
+    tcp_client.write("\
+        GET / HTTP/1.0\r\n\
+        \r\n\
+    ");
+
+    let expected = "HTTP/1.0 200 OK\r\n";
+    assert_eq!(s(&tcp_client.read()[..expected.len()]), expected);
 }
 
 #[test]
 fn http11_absolute_uri_differs_from_host() {
     let _ = env_logger::try_init();
 
-    let host = "transparency.test.svc.cluster.local";
+    // We shouldn't touch the URI or the Host, just pass directly as we got.
+    let auth = "transparency.test.svc.cluster.local";
+    let host = "foo.bar";
     let srv = server::http1()
         .route_fn("/", move |req| {
-            assert_eq!(req.version(), http::Version::HTTP_11);
-            assert_eq!(req.headers().get("host").unwrap(), host);
-            Response::builder()
-                .body("".into())
-                .unwrap()
+            assert_eq!(req.headers()["host"], host);
+            assert_eq!(req.uri().to_string(), format!("http://{}/", auth));
+            Response::new("".into())
         })
         .run();
     let ctrl = controller::new().run();
     let proxy = proxy::new()
         .controller(ctrl)
         .inbound(srv)
         .run();
-    let client = client::http1_absolute_uris(proxy.inbound, host);
+    let client = client::http1_absolute_uris(proxy.inbound, auth);
 
     let res = client.request(client.request_builder("/")
         .version(http::Version::HTTP_11)
-        .header("host", "foo.bar"));
+        .header("host", host));
 
     assert_eq!(res.status(), http::StatusCode::OK);
     assert_eq!(res.version(), http::Version::HTTP_11);