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

Emit insecure proxy event when using unverified proxies #3070

Open
wants to merge 11 commits into
base: main
Choose a base branch
from
1 change: 1 addition & 0 deletions changelog/2791.feature.rst
@@ -0,0 +1 @@
Emit ``InsecureProxyWarning`` when the proxy connection is not verified but the proxy.scheme is https
6 changes: 6 additions & 0 deletions docs/advanced-usage.rst
Expand Up @@ -509,6 +509,12 @@ be resolved in different ways.
verification enabled. Follow the :ref:`certificate verification <ssl>`
guide to resolve this warning.

* :class:`~exceptions.InsecureProxyWarning`
This happens when a request is made to an HTTPS Proxy without certificate
verification enabled or the proxy connection is not verified.
Follow the :ref:`certificate verification <ssl>` guide to
resolve this warning.

.. _disable_ssl_warnings:

Making unverified HTTPS requests is **strongly** discouraged, however, if you
Expand Down
15 changes: 13 additions & 2 deletions src/urllib3/connectionpool.py
Expand Up @@ -29,6 +29,7 @@
EmptyPoolError,
FullPoolError,
HostChangedError,
InsecureProxyWarning,
InsecureRequestWarning,
LocationValueError,
MaxRetryError,
Expand Down Expand Up @@ -1098,11 +1099,21 @@ def _validate_conn(self, conn: BaseHTTPConnection) -> None:
if conn.is_closed:
conn.connect()

# TODO revise this, see https://github.com/urllib3/urllib3/issues/2791
if conn.proxy and conn.proxy.scheme == "https" and not conn.proxy_is_verified:
warnings.warn(
(
f"Unverified HTTPS request is being made using proxy host '{conn.proxy.host}'. "
"Adding certificate verification is strongly advised. See: "
"https://urllib3.readthedocs.io/en/latest/advanced-usage.html"
"#tls-warnings"
),
InsecureProxyWarning,
)

if not conn.is_verified and not conn.proxy_is_verified:
warnings.warn(
(
f"Unverified HTTPS request is being made to host '{conn.host}'. "
f"Unverified HTTPS request is being made to host '{conn._tunnel_host if conn._tunnel_host else conn.host}'. " # type: ignore[attr-defined]
"Adding certificate verification is strongly advised. See: "
"https://urllib3.readthedocs.io/en/latest/advanced-usage.html"
"#tls-warnings"
Expand Down
4 changes: 4 additions & 0 deletions src/urllib3/exceptions.py
Expand Up @@ -214,6 +214,10 @@ class InsecureRequestWarning(SecurityWarning):
"""Warned when making an unverified HTTPS request."""


class InsecureProxyWarning(SecurityWarning):
"""Warned when using an unverified proxy."""


class NotOpenSSLWarning(SecurityWarning):
"""Warned when using unsupported SSL library"""

Expand Down
27 changes: 23 additions & 4 deletions test/with_dummyserver/test_no_ssl.py
Expand Up @@ -9,10 +9,10 @@

import urllib3
from dummyserver.testcase import (
HTTPSHypercornDummyServerTestCase,
HypercornDummyProxyTestCase,
HypercornDummyServerTestCase,
)
from urllib3.exceptions import InsecureRequestWarning
from urllib3.exceptions import InsecureProxyWarning, InsecureRequestWarning

from ..test_no_ssl import TestWithoutSSL

Expand All @@ -24,13 +24,32 @@ def test_simple(self) -> None:
assert r.status == 200, r.data


class TestHTTPSWithoutSSL(HTTPSHypercornDummyServerTestCase, TestWithoutSSL):
class TestHTTPSWithoutSSL(HypercornDummyProxyTestCase, TestWithoutSSL):
def test_simple(self) -> None:
with urllib3.HTTPSConnectionPool(
self.host, self.port, cert_reqs="NONE"
self.https_host, self.https_port, cert_reqs="NONE"
) as pool:
with pytest.warns(InsecureRequestWarning):
try:
pool.request("GET", "/")
except urllib3.exceptions.SSLError as e:
assert "SSL module is not available" in str(e)

def test_simple_proxy(self) -> None:
Copy link
Member

Choose a reason for hiding this comment

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

It would be good to test both HTTPS->HTTP and HTTPS->HTTPS proxy scenarios.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What is the expected HTTPS -> HTTP proxy scenario? Currently, it throws an InsecureRequestWarning as conn.is_verified is set to False while using HTTPs->HTTP connection. Should it only throw InsecureProxyWarning ?

Copy link
Member

Choose a reason for hiding this comment

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

If I understand correctly, this Seth's suggestion is related to #3070 (comment).
You change the message for InsecureRequestWarning, it would be good to test what host is included in the message when a proxy is used as well as changing test_simple to check what host is used there.

https_proxy_url = f"https://{self.proxy_host}:{int(self.https_proxy_port)}"
with urllib3.ProxyManager(https_proxy_url, cert_reqs="NONE") as pool:
with pytest.warns((InsecureProxyWarning, InsecureRequestWarning)) as record:
Copy link
Member

Choose a reason for hiding this comment

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

This makes pytest check that at least one of the two warnings is raised, not that both of them are raised sequentially

try:
pool.request("GET", "https://urllib3.readthedocs.io")
except urllib3.exceptions.SSLError as e:
assert "SSL module is not available" in str(e)
assert "localhost" in str(record[0].message)
assert "urllib3.readthedocs.io" in str(record[1].message)

with pytest.warns((InsecureProxyWarning, InsecureRequestWarning)) as record:
try:
pool.request("GET", f"http://{self.http_host}:{self.http_port}")
except urllib3.exceptions.SSLError as e:
assert "SSL module is not available" in str(e)
assert "localhost" in str(record[0].message)
assert self.http_host in str(record[1].message)