Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use TLS settings in selecting connection pool #6655

Merged
merged 1 commit into from
Mar 11, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
58 changes: 57 additions & 1 deletion src/requests/adapters.py
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@

import os.path
import socket # noqa: F401
import typing

from urllib3.exceptions import ClosedPoolError, ConnectTimeoutError
from urllib3.exceptions import HTTPError as _HTTPError
Expand Down Expand Up @@ -61,12 +62,38 @@ def SOCKSProxyManager(*args, **kwargs):
raise InvalidSchema("Missing dependencies for SOCKS support.")


if typing.TYPE_CHECKING:
from .models import PreparedRequest
Comment on lines +65 to +66
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not against this, but I think this is the first time we're introducing typing into Requests. I'm curious if we want to start that or push it into typeshed since this will be precedent for future inline typing?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is for a private method (that I fully anticipate people abusing) but we're not advertising things are typed and so it's not something I'm concerned with.



DEFAULT_POOLBLOCK = False
DEFAULT_POOLSIZE = 10
DEFAULT_RETRIES = 0
DEFAULT_POOL_TIMEOUT = None


def _urllib3_request_context(
request: "PreparedRequest", verify: "bool | str | None"
) -> "(typing.Dict[str, typing.Any], typing.Dict[str, typing.Any])":
host_params = {}
pool_kwargs = {}
parsed_request_url = urlparse(request.url)
scheme = parsed_request_url.scheme.lower()
port = parsed_request_url.port
cert_reqs = "CERT_REQUIRED"
if verify is False:
cert_reqs = "CERT_NONE"
if isinstance(verify, str):
pool_kwargs["ca_certs"] = verify
pool_kwargs["cert_reqs"] = cert_reqs
host_params = {
"scheme": scheme,
"host": parsed_request_url.hostname,
"port": port,
}
return host_params, pool_kwargs


class BaseAdapter:
"""The Base Transport Adapter"""

Expand Down Expand Up @@ -327,6 +354,35 @@ def build_response(self, req, resp):

return response

def _get_connection(self, request, verify, proxies=None):
# Replace the existing get_connection without breaking things and
# ensure that TLS settings are considered when we interact with
# urllib3 HTTP Pools
proxy = select_proxy(request.url, proxies)
try:
host_params, pool_kwargs = _urllib3_request_context(request, verify)
except ValueError as e:
raise InvalidURL(e, request=request)
if proxy:
proxy = prepend_scheme_if_needed(proxy, "http")
proxy_url = parse_url(proxy)
if not proxy_url.host:
raise InvalidProxyURL(
"Please check proxy URL. It is malformed "
"and could be missing the host."
)
proxy_manager = self.proxy_manager_for(proxy)
conn = proxy_manager.connection_from_host(
**host_params, pool_kwargs=pool_kwargs
)
else:
# Only scheme should be lower case
conn = self.poolmanager.connection_from_host(
**host_params, pool_kwargs=pool_kwargs
)

return conn

def get_connection(self, url, proxies=None):
"""Returns a urllib3 connection for the given URL. This should not be
called from user code, and is only exposed for use when subclassing the
Expand Down Expand Up @@ -453,7 +509,7 @@ def send(
"""

try:
conn = self.get_connection(request.url, proxies)
conn = self._get_connection(request, verify, proxies)
except LocationValueError as e:
raise InvalidURL(e, request=request)

Expand Down
7 changes: 7 additions & 0 deletions tests/test_requests.py
Original file line number Diff line number Diff line change
Expand Up @@ -2828,6 +2828,13 @@ def test_status_code_425(self):
assert r5 == 425
assert r6 == 425

def test_different_connection_pool_for_tls_settings(self):
s = requests.Session()
r1 = s.get("https://invalid.badssl.com", verify=False)
assert r1.status_code == 421
with pytest.raises(requests.exceptions.SSLError):
s.get("https://invalid.badssl.com")
Comment on lines +2831 to +2836
Copy link
Member

@nateprewitt nateprewitt Mar 10, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There may not be a better way to test this but I don't know if we have other tests that require contacting a live site with TLS disabled. That may have some durability issues and means we're going to take the first response we get back. Probably minor, but figured I'd call it out.

Copy link
Contributor Author

@sigmavirus24 sigmavirus24 Mar 10, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are many alternatives here, but those are all significantly more effort and this shows the behaviour is fixed before and after handily. I'm sure Linux folks will get pissed but I'm not as bothered about finding time later to do this a different way after we have fixed this

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll try to prioritize better (offline) tests soon



def test_json_decode_errors_are_serializable_deserializable():
json_decode_error = requests.exceptions.JSONDecodeError(
Expand Down
2 changes: 1 addition & 1 deletion tox.ini
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ extras =
security
socks
commands =
pytest tests
pytest {posargs:tests}

[testenv:default]

Expand Down
Loading