Repository: seanmonstar/reqwest (language: rust)

Issue:
User-Agent should be sent to proxies
Hi

On windows, reqwest reads the registry and sets proxy from there.
Implemented in #547 .

First, I want to say it is great !

However, I have an issue that I can't mitigate from this change.
At work, we have 2 proxies : one auth, one basic.
By default from GPO, settings point to the auth proxy, which is not supported by reqwest.
If I set the basic proxy in the environment, it is ignored. Which is what is expected from the code.

I need environment to take precedence over registry, if it is set. And I think this is expected behaviour.

Do you think it can be changed ?

NB: My issue arised when trying to update rust with rustup, which use reqwest.


Discussion / hints:
It *looks* like the code will check the environment first, and only if it is empty, will it check the registry. It's not doing that? How are you setting the environment?
I tried cmd and powershell, with their respective env commands, set or $env:

I tried lowercase and upcase.

I tested get_proxies() by copying everything, and it gets the proxy from environment.

I tried to debug a ClientBuilder, but it only outputs "ClientBuilder".

- How can I check the proxies set with "let c = Client::builder().use_sys_proxy();" ?

I then tried with manually setting the proxy, and I still got "auth req". 😮

- How can I get the raw answer from the proxy ?

And I double checked the proxy I'm using, it is definately the one without auth.

PS : It also happen in the WSL. I tried curl there, and it does not fail. Gonna try with an Ubuntu tomorrow.
> I tested get_proxies() by copying everything, and it gets the proxy from environment.

To be clear, you copied the `fn get_proxies` code into a new rust file and ran it, and it correctly sees the environment before the register, but then reqwest doesn't? Most odd! What version of reqwest is being used?
No.

I did copy get_proxies (and a few others). It sees the environment first. I think reqwest also get the environment, but I don't know how to check.

Then I tried :
```
    let c2 = Client::builder()
        .proxy(reqwest::Proxy::all("http://my.nonauth.proxy:8082")?)
        .build()?;
    let res = c2.get("https://monip.org").send();
```
And I still get 

> Error(Hyper(Error(Connect, Custom { kind: Other, error: "proxy authentication required" })), "https://monip.org/")

I think reqwest get the proxy right, but the proxy answer something wich makes reqwest (or hyper) 

- think it is an auth proxy, while it isn't.

- or reroute the request to the auth proxy.

Without the raw exchange, I can't tell. If there is no way to print it from rust, I will sniff the conversation, but I would like to try it first in rust, to know how to do it.

I opened this issue on reqwest because I have a few others softwares going through the non auth proxy, and they work just fine. So far, only rustup and my little program to simply check an url using reqwest have this behaviour.
I captured network with wireshark, and the only difference I see is that reqwest does not supply a user-agent. Even if I force one.

If I use wget --user-agent="", it also fails with proxy authentication required.

I'm not sure it's required per spec, but it seems to be on my proxy.

I didn't see how to set it for the proxy in reqwest.
My IT is gonna change the setting, hopefully.

Not sure anything should be done on reqwest side.
[HTTP 1.1 RFC](https://tools.ietf.org/html/rfc7231#section-5.5.3) says :

A user agent SHOULD send a User-Agent field in each request unless specifically configured not to do so.
Ya, I think the user agent should be sent to the proxy.

Fix patch:
diff --git a/src/async_impl/client.rs b/src/async_impl/client.rs
index c0d68bef3..029cb6eeb 100644
--- a/src/async_impl/client.rs
+++ b/src/async_impl/client.rs
@@ -137,6 +137,11 @@ impl ClientBuilder {
         let proxies = Arc::new(config.proxies);
 
         let mut connector = {
+            #[cfg(feature = "tls")]
+            fn user_agent(headers: &HeaderMap) -> HeaderValue {
+                headers[USER_AGENT].clone()
+            }
+
             #[cfg(feature = "tls")]
             match config.tls {
                 #[cfg(feature = "default-tls")]
@@ -153,7 +158,7 @@ impl ClientBuilder {
                         id.add_to_native_tls(&mut tls)?;
                     }
 
-                    Connector::new_default_tls(tls, proxies.clone(), config.local_address, config.nodelay)?
+                    Connector::new_default_tls(tls, proxies.clone(), user_agent(&config.headers), config.local_address, config.nodelay)?
                 },
                 #[cfg(feature = "rustls-tls")]
                 TlsBackend::Rustls => {
@@ -182,7 +187,7 @@ impl ClientBuilder {
                         id.add_to_rustls(&mut tls)?;
                     }
 
-                    Connector::new_rustls_tls(tls, proxies.clone(), config.local_address, config.nodelay)?
+                    Connector::new_rustls_tls(tls, proxies.clone(), user_agent(&config.headers), config.local_address, config.nodelay)?
                 }
             }
 
diff --git a/src/connect.rs b/src/connect.rs
index e9d3c26d7..0247524e1 100644
--- a/src/connect.rs
+++ b/src/connect.rs
@@ -11,6 +11,8 @@ use native_tls::{TlsConnector, TlsConnectorBuilder};
 use futures::Poll;
 #[cfg(feature = "tls")]
 use bytes::BufMut;
+#[cfg(feature = "tls")]
+use http::header::HeaderValue;
 
 use std::io;
 use std::sync::Arc;
@@ -32,7 +34,9 @@ pub(crate) struct Connector {
     proxies: Arc<Vec<Proxy>>,
     timeout: Option<Duration>,
     #[cfg(feature = "tls")]
-    nodelay: bool
+    nodelay: bool,
+    #[cfg(feature = "tls")]
+    user_agent: HeaderValue,
 }
 
 enum Inner {
@@ -69,6 +73,7 @@ impl Connector {
     pub(crate) fn new_default_tls<T>(
         tls: TlsConnectorBuilder,
         proxies: Arc<Vec<Proxy>>,
+        user_agent: HeaderValue,
         local_addr: T,
         nodelay: bool) -> ::Result<Connector>
         where
@@ -84,7 +89,8 @@ impl Connector {
             inner: Inner::DefaultTls(http, tls),
             proxies,
             timeout: None,
-            nodelay
+            nodelay,
+            user_agent,
         })
     }
 
@@ -92,6 +98,7 @@ impl Connector {
     pub(crate) fn new_rustls_tls<T>(
         tls: rustls::ClientConfig,
         proxies: Arc<Vec<Proxy>>,
+        user_agent: HeaderValue,
         local_addr: T,
         nodelay: bool) -> ::Result<Connector>
         where
@@ -114,7 +121,8 @@ impl Connector {
             inner: Inner::RustlsTls { http, tls, tls_proxy },
             proxies,
             timeout: None,
-            nodelay
+            nodelay,
+            user_agent,
         })
     }
 
@@ -326,9 +334,10 @@ impl Connect for Connector {
                         http.set_nodelay(nodelay);
                         let http = ::hyper_tls::HttpsConnector::from((http, tls.clone()));
                         let tls = tls.clone();
+                        let user_agent = self.user_agent.clone();
                         return timeout!(http.connect(ndst).and_then(move |(conn, connected)| {
                             trace!("tunneling HTTPS over proxy");
-                            tunnel(conn, host.clone(), port, auth)
+                            tunnel(conn, host.clone(), port, user_agent, auth)
                                 .and_then(move |tunneled| {
                                     tls.connect_async(&host, tunneled)
                                         .map_err(|e| io::Error::new(io::ErrorKind::Other, e))
@@ -348,12 +357,13 @@ impl Connect for Connector {
                         http.set_nodelay(nodelay);
                         let http = ::hyper_rustls::HttpsConnector::from((http, tls_proxy.clone()));
                         let tls = tls.clone();
+                        let user_agent = self.user_agent.clone();
                         return timeout!(http.connect(ndst).and_then(move |(conn, connected)| {
                             trace!("tunneling HTTPS over proxy");
                             let maybe_dnsname = DNSNameRef::try_from_ascii_str(&host)
                                 .map(|dnsname| dnsname.to_owned())
                                 .map_err(|_| io::Error::new(io::ErrorKind::Other, "Invalid DNS Name"));
-                            tunnel(conn, host, port, auth)
+                            tunnel(conn, host, port, user_agent, auth)
                                 .and_then(move |tunneled| Ok((maybe_dnsname?, tunneled)))
                                 .and_then(move |(dnsname, tunneled)| {
                                     RustlsConnector::from(tls).connect(dnsname.as_ref(), tunneled)
@@ -388,12 +398,18 @@ pub(crate) type Conn = Box<dyn AsyncConn + Send + Sync + 'static>;
 pub(crate) type Connecting = Box<dyn Future<Item=(Conn, Connected), Error=io::Error> + Send>;
 
 #[cfg(feature = "tls")]
-fn tunnel<T>(conn: T, host: String, port: u16, auth: Option<::http::header::HeaderValue>) -> Tunnel<T> {
+fn tunnel<T>(conn: T, host: String, port: u16, user_agent: HeaderValue, auth: Option<HeaderValue>) -> Tunnel<T> {
     let mut buf = format!("\
         CONNECT {0}:{1} HTTP/1.1\r\n\
         Host: {0}:{1}\r\n\
     ", host, port).into_bytes();
 
+    // user-agent
+    buf.extend_from_slice(b"User-Agent: ");
+    buf.extend_from_slice(user_agent.as_bytes());
+    buf.extend_from_slice(b"\r\n");
+
+    // proxy-authorization
     match auth {
         Some(value) => {
             debug!("tunnel to {}:{} using basic auth", host, port);
@@ -699,6 +715,7 @@ mod tests {
     use super::tunnel;
     use proxy;
 
+    static TUNNEL_UA: &'static str = "tunnel-test/x.y";
     static TUNNEL_OK: &'static [u8] = b"\
         HTTP/1.1 200 OK\r\n\
         \r\n\
@@ -717,9 +734,10 @@ mod tests {
             let connect_expected = format!("\
                 CONNECT {0}:{1} HTTP/1.1\r\n\
                 Host: {0}:{1}\r\n\
-                {2}\
+                User-Agent: {2}\r\n\
+                {3}\
                 \r\n\
-            ", addr.ip(), addr.port(), $auth).into_bytes();
+            ", addr.ip(), addr.port(), TUNNEL_UA, $auth).into_bytes();
 
             thread::spawn(move || {
                 let (mut sock, _) = listener.accept().unwrap();
@@ -742,7 +760,7 @@ mod tests {
         let host = addr.ip().to_string();
         let port = addr.port();
         let work = work.and_then(|tcp| {
-            tunnel(tcp, host, port, None)
+            tunnel(tcp, host, port, TUNNEL_UA.parse().unwrap(), None)
         });
 
         rt.block_on(work).unwrap();
@@ -757,7 +775,7 @@ mod tests {
         let host = addr.ip().to_string();
         let port = addr.port();
         let work = work.and_then(|tcp| {
-            tunnel(tcp, host, port, None)
+            tunnel(tcp, host, port, TUNNEL_UA.parse().unwrap(), None)
         });
 
         rt.block_on(work).unwrap_err();
@@ -772,7 +790,7 @@ mod tests {
         let host = addr.ip().to_string();
         let port = addr.port();
         let work = work.and_then(|tcp| {
-            tunnel(tcp, host, port, None)
+            tunnel(tcp, host, port, TUNNEL_UA.parse().unwrap(), None)
         });
 
         rt.block_on(work).unwrap_err();
@@ -791,7 +809,7 @@ mod tests {
         let host = addr.ip().to_string();
         let port = addr.port();
         let work = work.and_then(|tcp| {
-            tunnel(tcp, host, port, None)
+            tunnel(tcp, host, port, TUNNEL_UA.parse().unwrap(), None)
         });
 
         let error = rt.block_on(work).unwrap_err();
@@ -810,7 +828,7 @@ mod tests {
         let host = addr.ip().to_string();
         let port = addr.port();
         let work = work.and_then(|tcp| {
-            tunnel(tcp, host, port, Some(proxy::encode_basic_auth("Aladdin", "open sesame")))
+            tunnel(tcp, host, port, TUNNEL_UA.parse().unwrap(), Some(proxy::encode_basic_auth("Aladdin", "open sesame")))
         });
 
         rt.block_on(work).unwrap();
diff --git a/src/dns.rs b/src/dns.rs
index 40d224b20..06b1aae90 100644
--- a/src/dns.rs
+++ b/src/dns.rs
@@ -49,7 +49,7 @@ impl TrustDnsResolver {
 
 impl hyper_dns::Resolve for TrustDnsResolver {
     type Addrs = vec::IntoIter<IpAddr>;
-    type Future = Box<Future<Item=Self::Addrs, Error=io::Error> + Send>;
+    type Future = Box<dyn Future<Item=Self::Addrs, Error=io::Error> + Send>;
 
     fn resolve(&self, name: hyper_dns::Name) -> Self::Future {
         let inner = self.inner.clone();


Test patch: