HTTPS requests through proxies in proposed/3.0.0 aren't configured correctly

In current master:

```
>>> import requests
>>> requests.__version__
'2.11.1'
>>> session = requests.Session()
>>> r = session.get('https://www.jcline.org/', verify=True, proxies={'http': 'http://vagrant:vagrant@localhost:3128', 'https': 'http://vagrant:vagrant@localhost:3128'})
>>> 
```

In current proposed/3.0.0:

```
>>> import requests
>>> requests.__version__
'3.0.0'
>>> session = requests.Session()
>>> r = session.get('https://www.jcline.org/', verify=True, proxies={'http': 'http://vagrant:vagrant@localhost:3128', 'https': 'http://vagrant:vagrant@localhost:3128'})
requests/packages/urllib3/connectionpool.py:838: InsecureRequestWarning: Unverified HTTPS request is being made. Adding certificate verification is strongly advised. See: https://urllib3.readthedocs.io/en/latest/security.html
  InsecureRequestWarning)
>>> 
```

This is a problem I introduced in https://github.com/kennethreitz/requests/pull/3109 :disappointed:. What happens right now is if a request is _not_ through a proxy and it's HTTPS, the urllib3 pool manager's `connection_pool_kw` are updated before requesting a new connection using [requests.adapters.HTTPAdapter._update_poolmanager_ssl_kw](https://github.com/kennethreitz/requests/blob/proposed/3.0.0/requests/adapters.py#L204). If it _is_ through a proxy, the keywords aren't updated and the request is made with the default settings for urllib3.

To me, the most appealing way to fix this is to add a keyword argument, `connection_kwargs` or something, to all the `urllib3.poolmanager.PoolManager.connection_from_*` methods that is either merged into `connection_pool_kw` or overrides them. That way `urllib3` can handle getting the connection pool with the new kwargs in a thread-safe manner. Currently, `requests` has to manage updating the keys and getting the new connection pool with a lock. It seems like that would be better in `urllib3`.

The other option is to patch up what's currently in `HTTPAdapter` so it handles updating the proxy manager or plain pool manager based on whether proxies are in use.

What do people think?


I am open to having a urllib3 patch for this, at least in principle. What would it look like?


This current patch I've got should give you the general idea, although I'm still playing with it a bit:

```
From 0b95fd47688d5dcbc7ab0798f5d110e7c17eef86 Mon Sep 17 00:00:00 2001
From: Jeremy Cline <jeremy@jcline.org>
Date: Sun, 23 Oct 2016 12:11:10 -0400
Subject: [PATCH] The ``connection_from_*`` methods now accept pool_kw

Work in progress

Signed-off-by: Jeremy Cline <jeremy@jcline.org>
---
 urllib3/poolmanager.py | 47 ++++++++++++++++++++++++++++++-----------------
 1 file changed, 30 insertions(+), 17 deletions(-)

diff --git a/urllib3/poolmanager.py b/urllib3/poolmanager.py
index 276b54d..ab9a108 100644
--- a/urllib3/poolmanager.py
+++ b/urllib3/poolmanager.py
@@ -2,6 +2,10 @@ from __future__ import absolute_import
 import collections
 import functools
 import logging
+try:
+    from threading import RLock
+except ImportError:  # threading is an optional module and may not be present.
+    from dummy_threading import RLock

 from ._collections import RecentlyUsedContainer
 from .connectionpool import HTTPConnectionPool, HTTPSConnectionPool
@@ -120,6 +124,7 @@ class PoolManager(RequestMethods):
         # override them.
         self.pool_classes_by_scheme = pool_classes_by_scheme
         self.key_fn_by_scheme = key_fn_by_scheme.copy()
+        self.new_pool_lock = RLock()

     def __enter__(self):
         return self
@@ -155,7 +160,7 @@ class PoolManager(RequestMethods):
         """
         self.pools.clear()

-    def connection_from_host(self, host, port=None, scheme='http'):
+    def connection_from_host(self, host, port=None, scheme='http', pool_kwargs=None):
         """
         Get a :class:`ConnectionPool` based on the host, port, and scheme.

@@ -166,14 +171,19 @@ class PoolManager(RequestMethods):
         if not host:
             raise LocationValueError("No host specified.")

-        request_context = self.connection_pool_kw.copy()
-        request_context['scheme'] = scheme or 'http'
-        if not port:
-            port = port_by_scheme.get(request_context['scheme'].lower(), 80)
-        request_context['port'] = port
-        request_context['host'] = host
+        with self.new_pool_lock:
+            # Update the connection_pool_kw and get a new pool atomically.
+            for k, v in pool_kwargs.items():
+                self.connection_pool_kw[k] = v

-        return self.connection_from_context(request_context)
+            request_context = self.connection_pool_kw.copy()
+            request_context['scheme'] = scheme or 'http'
+            if not port:
+                port = port_by_scheme.get(request_context['scheme'].lower(), 80)
+            request_context['port'] = port
+            request_context['host'] = host
+
+            return self.connection_from_context(request_context)

     def connection_from_context(self, request_context):
         """
@@ -209,7 +219,7 @@ class PoolManager(RequestMethods):

         return pool

-    def connection_from_url(self, url):
+    def connection_from_url(self, url, pool_kwargs=None):
         """
         Similar to :func:`urllib3.connectionpool.connection_from_url` but
         doesn't pass any additional parameters to the
@@ -219,9 +229,10 @@ class PoolManager(RequestMethods):
         constructor.
         """
         u = parse_url(url)
-        return self.connection_from_host(u.host, port=u.port, scheme=u.scheme)
+        return self.connection_from_host(u.host, port=u.port, scheme=u.scheme,
+                                         pool_kwargs=pool_kwargs)

-    def urlopen(self, method, url, redirect=True, **kw):
+    def urlopen(self, method, url, redirect=True, pool_kwargs=None, **kw):
         """
         Same as :meth:`urllib3.connectionpool.HTTPConnectionPool.urlopen`
         with custom cross-host redirect logic and only sends the request-uri
@@ -231,7 +242,8 @@ class PoolManager(RequestMethods):
         :class:`urllib3.connectionpool.ConnectionPool` can be chosen for it.
         """
         u = parse_url(url)
-        conn = self.connection_from_host(u.host, port=u.port, scheme=u.scheme)
+        conn = self.connection_from_host(u.host, port=u.port, scheme=u.scheme,
+                                         pool_kwargs=pool_kwargs)

         kw['assert_same_host'] = False
         kw['redirect'] = False
@@ -322,13 +334,13 @@ class ProxyManager(PoolManager):
         super(ProxyManager, self).__init__(
             num_pools, headers, **connection_pool_kw)

-    def connection_from_host(self, host, port=None, scheme='http'):
+    def connection_from_host(self, host, port=None, scheme='http', pool_kwargs=None):
         if scheme == "https":
             return super(ProxyManager, self).connection_from_host(
-                host, port, scheme)
+                host, port, scheme, pool_kwargs=pool_kwargs)

         return super(ProxyManager, self).connection_from_host(
-            self.proxy.host, self.proxy.port, self.proxy.scheme)
+            self.proxy.host, self.proxy.port, self.proxy.scheme, pool_kwargs=pool_kwargs)

     def _set_proxy_headers(self, url, headers=None):
         """
@@ -345,7 +357,7 @@ class ProxyManager(PoolManager):
             headers_.update(headers)
         return headers_

-    def urlopen(self, method, url, redirect=True, **kw):
+    def urlopen(self, method, url, redirect=True, pool_kwargs=None, **kw):
         "Same as HTTP(S)ConnectionPool.urlopen, ``url`` must be absolute."
         u = parse_url(url)

@@ -356,7 +368,8 @@ class ProxyManager(PoolManager):
             headers = kw.get('headers', self.headers)
             kw['headers'] = self._set_proxy_headers(url, headers)

-        return super(ProxyManager, self).urlopen(method, url, redirect=redirect, **kw)
+        return super(ProxyManager, self).urlopen(method, url, redirect=redirect,
+                                                 pool_kwargs=None, **kw)
-- 
2.9.3
```

And then requests would get a patch something like:

```
From 4fd2d962b6883860beea03ca599fa38959a0e7ad Mon Sep 17 00:00:00 2001
From: Jeremy Cline <jeremy@jcline.org>
Date: Sun, 23 Oct 2016 12:16:02 -0400
Subject: [PATCH] Use ``pool_kwargs`` when getting a new connection

Work in progress

Signed-off-by: Jeremy Cline <jeremy@jcline.org>
---
 requests/adapters.py | 54 +++++++++++++++++++++++++++-------------------------
 1 file changed, 28 insertions(+), 26 deletions(-)

diff --git a/requests/adapters.py b/requests/adapters.py
index 1032d2e..41f0d8c 100644
--- a/requests/adapters.py
+++ b/requests/adapters.py
@@ -201,7 +201,8 @@ class HTTPAdapter(BaseAdapter):

         return manager

-    def _update_poolmanager_ssl_kw(self, verify, cert):
+    @staticmethod
+    def _pool_kwargs(verify, cert):
         """Update the :class:`PoolManager <urllib3.poolmanager.PoolManager>`
         connection_pool_kw with the necessary SSL configuration. This method
         should not be called from user code, and is only exposed for use when
@@ -215,6 +216,8 @@ class HTTPAdapter(BaseAdapter):
                      key concatenated in a single file, or as a tuple of
                      (cert_file, key_file).
         """
+        pool_kwargs = {}
+
         if verify:

             cert_loc = None
@@ -229,25 +232,27 @@ class HTTPAdapter(BaseAdapter):
             if not cert_loc:
                 raise Exception("Could not find a suitable SSL CA certificate bundle.")

-            self.poolmanager.connection_pool_kw['cert_reqs'] = 'CERT_REQUIRED'
+            pool_kwargs['cert_reqs'] = 'CERT_REQUIRED'

             if not os.path.isdir(cert_loc):
-                self.poolmanager.connection_pool_kw['ca_certs'] = cert_loc
-                self.poolmanager.connection_pool_kw['ca_cert_dir'] = None
+                pool_kwargs['ca_certs'] = cert_loc
+                pool_kwargs['ca_cert_dir'] = None
             else:
-                self.poolmanager.connection_pool_kw['ca_cert_dir'] = cert_loc
-                self.poolmanager.connection_pool_kw['ca_certs'] = None
+                pool_kwargs['ca_cert_dir'] = cert_loc
+                pool_kwargs['ca_certs'] = None
         else:
-            self.poolmanager.connection_pool_kw['cert_reqs'] = 'CERT_NONE'
-            self.poolmanager.connection_pool_kw['ca_certs'] = None
-            self.poolmanager.connection_pool_kw['ca_cert_dir'] = None
+            pool_kwargs['cert_reqs'] = 'CERT_NONE'
+            pool_kwargs['ca_certs'] = None
+            pool_kwargs['ca_cert_dir'] = None

         if cert:
             if not isinstance(cert, basestring):
-                self.poolmanager.connection_pool_kw['cert_file'] = cert[0]
-                self.poolmanager.connection_pool_kw['key_file'] = cert[1]
+                pool_kwargs['cert_file'] = cert[0]
+                pool_kwargs['key_file'] = cert[1]
             else:
-                self.poolmanager.connection_pool_kw['cert_file'] = cert
+                pool_kwargs['cert_file'] = cert
+
+        return pool_kwargs

     def build_response(self, req, resp):
         """Builds a :class:`Response <requests.Response>` object from a urllib3
@@ -295,21 +300,18 @@ class HTTPAdapter(BaseAdapter):
         :param proxies: (optional) A Requests-style dictionary of proxies used on this request.
         :rtype: requests.packages.urllib3.ConnectionPool
         """
-        with self._pool_kw_lock:
-            if url.lower().startswith('https'):
-                self._update_poolmanager_ssl_kw(verify, cert)
-
-            proxy = select_proxy(url, proxies)
+        pool_kwargs = self._pool_kwargs(verify, cert)
+        proxy = select_proxy(url, proxies)

-            if proxy:
-                proxy = prepend_scheme_if_needed(proxy, 'http')
-                proxy_manager = self.proxy_manager_for(proxy)
-                conn = proxy_manager.connection_from_url(url)
-            else:
-                # Only scheme should be lower case
-                parsed = urlparse(url)
-                url = parsed.geturl()
-                conn = self.poolmanager.connection_from_url(url)
+        if proxy:
+            proxy = prepend_scheme_if_needed(proxy, 'http')
+            proxy_manager = self.proxy_manager_for(proxy)
+            conn = proxy_manager.connection_from_url(url, pool_kwargs=pool_kwargs)
+        else:
+            # Only scheme should be lower case
+            parsed = urlparse(url)
+            url = parsed.geturl()
+            conn = self.poolmanager.connection_from_url(url, pool_kwargs=pool_kwargs)

         return conn

-- 
2.9.3
```

If those look generally reasonable I'll write some tests and open a urllib3 PR where we can hammer out the details.


Why does the patch change the connection pool keyword arguments on the PoolManager?

Anyway, this patch does seem like it's a reasonable enough direction to go, so please head on over to urllib3 and open a PR.


I like the direction this is headed.  In `PoolManager.connection_from_host()` consider:

```
request_context = self.connection_pool_kw.copy()
request_context.update(pool_kwargs or {})
```

instead of updating the `PoolManager.connection_pool_kw`. 

This has two advantages:
- Don't need the lock.
- Updating the PoolManager.connection_pool_kw when getting a connection seems unintuitive and an undesirable side effect.

The patch in _requests_ looks good.

Looking forward to seeing the PR.


@jeremycline, was there any work left to do here, or can we close this out? I was under the impression we solved this with your last patch.

@nateprewitt Yep, it should all be done, I just forgot to close this issue.