Emit insecure proxy event when using unverified proxies #3070

tushar5526 · 2023-06-13T21:51:33Z

illia-v · 2023-06-14T22:10:38Z

@tushar5526 thanks for the change! Could you please add a test similar to test.with_dummyserver.test_no_ssl.TestHTTPSWithoutSSL and a changelog entry?

src/urllib3/connectionpool.py

sethmlarson

This is an excellent start, thank you! 🚀 Here's a few suggestions I can think of:

sethmlarson · 2023-06-17T04:01:09Z

src/urllib3/connectionpool.py

+                InsecureProxyWarning,
+            )
+
+        elif not conn.is_verified:


Since both can be true, can we change this elif to an if and add a testcase for both warnings being emitted in one connection?

There seems to be something off when I am using a proxy. The following snippet will raise both InsecureRequestWarning and InsecureProxyWarning after I change the elif to if.

import urllib3 proxy = urrlib3.ProxyManager('https://localhost:8443', cert_reqs='NONE') res = proxy.request('GET', 'https://www.google.com')

Outputs the following.

/home/tushar55/projects/urllib3/src/urllib3/connectionpool.py:1100: InsecureProxyWarning: Unverified HTTPS request is being made using proxy host 'localhost'. Adding certificate verification is strongly advised. See: https://urllib3.readthedocs.io/en/latest/advanced-usage.html#tls-warnings warnings.warn( /home/tushar55/projects/urllib3/src/urllib3/connectionpool.py:1111: InsecureRequestWarning: Unverified HTTPS request is being made to host 'localhost'. Adding certificate verification is strongly advised. See: https://urllib3.readthedocs.io/en/latest/advanced-usage.html#tls-warnings warnings.warn(

Both the conn.host and conn.proxy.host is set to the same value which is localhost. conn._tunnel_host is set to www.google.com

I suspect this line is resetting the https://github.com/urllib3/urllib3/blob/main/src/urllib3/connectionpool.py#L1062 updates the host and port when proxy is being used. Can you clarify, what is the intended behaviour here ?

A few more examples that I tested out locally after setting up a simple python http server on port 8000 using python -m http.server

import urllib3 proxy = urllib3.ProxyManager('https://localhost:8443', ca_certs='/home/tushar55/projects/urllib3/dummyserver/certs/cacert.pem', cert_reqs='NONE') res = proxy.request('GET', 'http://127.0.0.1:8000') print(res.status, res.data)

This raises the following error -

/home/tushar55/projects/urllib3/src/urllib3/connectionpool.py:1146: InsecureRequestWarning: Unverified HTTPS request is being made to host 'localhos t'. Adding certificate verification is strongly advised. See: https://urllib3.readthedocs.io/en/latest/advanced-usage.html#tls-warnings warnings.warn(

This is definitely confusing, but in the case of a proxied HTTPS origin we use CONNECT tunneling so the actual destination host we want to connect to is HTTPConnection._tunnel_host. There might need to be some modifications to a warning message in order to pick the right host to use. Hopefully this helps?

Yeah I think it's clear now. So, if certificate verification is disabled for a proxy manager then it should throw both the InsecureProxy and InsecureRequest warnings, right?

proxy = urllib3.ProxyManager('https://localhost:8443', cert_reqs='NONE') res = proxy.request('GET', 'https://localhost:8000')

and for normal PoolManger, it should just throw InsecureRequest warning.

pool = urllib3.PoolManager(cert_reqs='NONE') res = pool.request('GET', 'https://localhost:8000')

I will update the InsecureRequest warning's message to host or _tunnel_host depending upon whether we are using an HTTPS proxy or not.

test/with_dummyserver/test_no_ssl.py

sethmlarson · 2023-06-17T04:14:24Z

test/with_dummyserver/test_no_ssl.py

+    def test_simple_proxy(self) -> None:
+        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):


Can we make an assertion about the message of InsecureProxyWarning (including it containing the right host) as well?

Added checks in the recent commits.

sethmlarson · 2023-06-17T04:14:38Z

test/with_dummyserver/test_no_ssl.py

        ) 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:


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

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 ?

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.

sethmlarson · 2023-06-17T04:15:34Z

test/with_dummyserver/test_no_ssl.py

@@ -21,13 +21,22 @@ def test_simple(self) -> None:
            assert r.status == 200, r.data


-class TestHTTPSWithoutSSL(HTTPSDummyServerTestCase, TestWithoutSSL):
+class TestHTTPSWithoutSSL(HTTPDummyProxyTestCase, TestWithoutSSL):


You might want to add some assertions to existing HTTPS proxy tests as well, specifically it'd be good to assert that the InsecureProxyWarning isn't raised when using the proper configuration for secure proxies.

Thanks, I have added them.

…TTPs proxy is being used. fix: Raise warnings for both Unverified Proxy and Request

tushar5526 · 2023-09-01T10:40:28Z

Hi @sethmlarson , it seems that proxy_is_verified is never set for HTTPs to HTTP proxies. proxy_is_verified is set only at https://github.com/urllib3/urllib3/blob/main/src/urllib3/connection.py#L694 and the function _connect_tls_proxy containing this line is called only when we are creating a HTTP tunnel https://github.com/urllib3/urllib3/blob/main/src/urllib3/connection.py#L672. We would have to set the value for proxy_is_verified for scenarios when we are not using a TLS-in-TLS tunnel and instead setting up a TLS connection for HTTPs to HTTP case.

illia-v

@tushar5526 interesting, it does look like a bug that HTTPS proxy with a verified certificate + HTTP destination sets is_verified=True and proxy_is_verified=None. I'd expect is_verified=False and proxy_is_verified=True according to https://github.com/urllib3/urllib3/blob/main/notes/connection-lifecycle.md

illia-v · 2023-09-01T14:49:38Z

test/with_dummyserver/test_no_ssl.py

+    def test_simple_proxy(self) -> None:
+        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:


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

tushar5526 · 2023-09-14T15:42:42Z

@tushar5526 interesting, it does look like a bug that HTTPS proxy with a verified certificate + HTTP destination sets is_verified=True and proxy_is_verified=None. I'd expect is_verified=False and proxy_is_verified=True according to https://github.com/urllib3/urllib3/blob/main/notes/connection-lifecycle.md

Yes, thanks for confirming. This has been bugging me for a long time. Should I open a new issue?

The tests for this PR is dependent on the correct behaviour of proxy_is_verifed and is_verified

illia-v · 2023-09-19T16:39:41Z

@tushar5526 yes, an issue for this would be really appreciated

tushar5526 marked this pull request as draft June 14, 2023 19:10

Emit insecure proxy event when using unverified proxies

96b5363

tushar5526 force-pushed the 2791 branch from 16eb313 to 96b5363 Compare June 14, 2023 19:20

tushar5526 marked this pull request as ready for review June 14, 2023 19:21

illia-v reviewed Jun 14, 2023

View reviewed changes

src/urllib3/connectionpool.py Outdated Show resolved Hide resolved

Add tests and update docs for changes.

747e233

sethmlarson requested changes Jun 17, 2023

View reviewed changes

fix: Update InsecureRequestWarning message to use _tunnel_host when H…

5054829

…TTPs proxy is being used. fix: Raise warnings for both Unverified Proxy and Request

tushar5526 marked this pull request as draft June 19, 2023 21:17

tushar5526 marked this pull request as ready for review July 14, 2023 11:35

tushar5526 added 5 commits August 28, 2023 22:06

Merge branch 'urllib3:main' into 2791

015db1b

Use absolute URL in proxy test case

32c6a73

Assert host in warning strings

2a024b4

Add testcases for HTTPs to HTTP proxies

0a07cfc

Fix lint errors

150172d

illia-v reviewed Sep 13, 2023

View reviewed changes

Merge branch 'main' into 2791

8ece270

tushar5526 mentioned this pull request Sep 20, 2023

Incorrect behaviour of proxy_is_verified #3130

Closed

Merge branch 'main' into 2791

a0186f5

illia-v force-pushed the 2791 branch from bc9d1ad to a0186f5 Compare January 9, 2024 21:22

Merge branch 'main' into 2791

93279fc

ecerulm mentioned this pull request Jan 23, 2024

Pin to pypy-3.9-v7.3.13 in CI #3308

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Emit insecure proxy event when using unverified proxies #3070

Emit insecure proxy event when using unverified proxies #3070

tushar5526 commented Jun 13, 2023

illia-v commented Jun 14, 2023

sethmlarson left a comment

sethmlarson Jun 17, 2023

tushar5526 Jun 17, 2023 •

edited

tushar5526 Jun 17, 2023

sethmlarson Jun 18, 2023 •

edited

tushar5526 Jun 19, 2023 •

edited

sethmlarson Jun 17, 2023

tushar5526 Aug 28, 2023

sethmlarson Jun 17, 2023

tushar5526 Jun 19, 2023 •

edited

illia-v Jul 18, 2023

sethmlarson Jun 17, 2023

tushar5526 Aug 28, 2023

tushar5526 commented Sep 1, 2023

illia-v left a comment

illia-v Sep 1, 2023

tushar5526 commented Sep 14, 2023

illia-v commented Sep 19, 2023

Emit insecure proxy event when using unverified proxies #3070

Are you sure you want to change the base?

Emit insecure proxy event when using unverified proxies #3070

Conversation

tushar5526 commented Jun 13, 2023

illia-v commented Jun 14, 2023

sethmlarson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tushar5526 Jun 17, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sethmlarson Jun 18, 2023 • edited

Choose a reason for hiding this comment

tushar5526 Jun 19, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tushar5526 Jun 19, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tushar5526 commented Sep 1, 2023

illia-v left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tushar5526 commented Sep 14, 2023

illia-v commented Sep 19, 2023

tushar5526 Jun 17, 2023 •

edited

sethmlarson Jun 18, 2023 •

edited

tushar5526 Jun 19, 2023 •

edited

tushar5526 Jun 19, 2023 •

edited