Use proxy_ssl_context as SSL ctx when forwarding #2651

avi364 · 2022-06-22T03:37:22Z

Fixes #2577 by setting ssl_context to be the provided proxy_ssl_context when use_forwarding_for_https is set to True.

I enabled the failing test, but because the test doesn't really make sense (I think) for when a forwarding proxy is used, I removed that case from the test. However, I added additional tests to cover the different cases of valid proxy_ssl_context and ssl_context being either provided or not provided (with the existing test_https_proxy_tls_error test covering the case when a valid proxy_ssl_context is not provided).

The issue described that providing ssl_context should only be a DeprecationWarning for version 1.26.x. Should that change be done as a separate PR against that branch?

matttpt · 2022-06-22T06:12:01Z

(Chiming in here since I've also been thinking about this issue today. Please forgive me if I'm stepping on your toes at all!)

I think there's another case when proxy_ssl_context must be used that this PR should address: requesting an HTTP destination forwarded through an HTTPS proxy. It looks like ssl_context is currently being used in this case also. (The existing test suite does not catch it: in particular, test_https_proxy_with_proxy_ssl_context sets ca_certs=DEFAULT_CA when it creates the proxy, which hides the fact that proxy_ssl_context is ignored.) To see this, you can try adding a test like this to test/with_dummyserver/test_proxy_poolmanager.py (will fail):

proxy_ssl_context = create_urllib3_context()
proxy_ssl_context.load_verify_locations(DEFAULT_CA)
with proxy_from_url(
    self.https_proxy_url,
    proxy_ssl_context=proxy_ssl_context,
) as proxy:
    proxy.request("GET", f"{self.http_url}/")

Hope this is useful.

avi364 · 2022-06-28T02:07:25Z

@matttpt I was completely off-base with my understanding and my previous comments were completely irrelevant. You were right that proxy_ssl_context was being ignored for HTTP destinations. I have corrected that as well and add your test case.

pquentin · 2022-06-28T13:14:12Z

Hey @avi364 and @matttpt thanks for the great work here! We're not forgetting you but answering your questions requires some digging and we've had lot of activity recently

matttpt · 2022-06-28T14:34:25Z

@avi364 sorry I haven't been available the last few days, but thanks for looking into this! Confirming that the latest iteration does what I expected.

Use proxy_ssl_context when fowarding to both a HTTPS destination (which occurs when use_forwarding_for_https=True) and to a HTTP destination. Raise a warning if both a proxy_ssl_context and a ssl_context are provided when use_forwarding_for_https=True. Add tests to verify these cases and modify test_proxy_https_target_tls_error in test_proxy_poolmanager to account for this new behavior. Addresses urllib3#2577

avi364 · 2022-06-28T22:28:58Z

@matttpt thanks for confirming.

@pquentin no problem and sorry for the noise, I really confused myself for a while, so most of my questions are no longer necessary to answer.

My only remaining question is about emitting a DeprecationWarning in 1.26.X and a ValueError in 2.x. I assume main will eventually be the branch used to cut 2.x. To make it easier to cherry pick this change onto the 1.26 branch, I broke the fix into 2 commits, where the first commit emits the warning. Is this the right approach?

jalopezsilva

I'm very excited to see progress to see this issue! This is great work and I'm looking forward to merging it.

I have you a quick review so far, but I'll take another pass.

jalopezsilva · 2022-06-29T15:08:45Z

src/urllib3/connection.py

+                )
+            elif proxy_config.ssl_context is None and ssl_context is not None:
+                raise ValueError(
+                    "The 'ssl_context' parameter cannot be set when use_forwarding_for_https is True. Use the 'proxy_ssl_context' parameter to customizet the connection to the proxy"


nit: You have a typo, cutomizet

test/with_dummyserver/test_proxy_poolmanager.py

jalopezsilva · 2022-06-29T15:19:33Z

@sethmlarson how do we want to handle the following:

My only remaining question is about emitting a DeprecationWarning in 1.26.X and a ValueError in 2.x. I assume main will eventually be the branch used to cut 2.x. To make it easier to cherry pick this change onto the 1.26 branch, I broke the fix into 2 commits, where the first commit emits the warning. Is this the right approach?

Were we planning on doing another release with 1.26.X? It would be ideal to push the DeprecationWarning first.

Raise an exception if both a proxy_ssl_context and a ssl_context are provided when use_forwarding_for_https=True, instead of only raising a warning. Modify tests to account for change. Resolves urllib3#2577

avi364 · 2022-07-20T21:54:12Z

@jalopezsilva is there anything else you would like me to do?

jalopezsilva · 2022-07-23T18:46:32Z

@jalopezsilva is there anything else you would like me to do?

Not at the moment. I want to hear @sethmlarson's opinion on the following though.

My only remaining question is about emitting a DeprecationWarning in 1.26.X and a ValueError in 2.x. I assume main will eventually be the branch used to cut 2.x. To make it easier to cherry pick this change onto the 1.26 branch, I broke the fix into 2 commits, where the first commit emits the warning. Is this the right approach?

Should we release the warning with 1.26.X and deprecate it fully with 2.x?

sethmlarson · 2022-07-28T02:07:06Z

Sorry for taking so long on this @avi364 and @jalopezsilva. Let's emit a DeprecatingWarning on 1.26.x and a ValueError on main. Thanks for your work here!

avi364 · 2022-07-30T21:37:45Z

Thanks! From my understanding of how Github pull requests work, the 2 commits can be merged as-is into main, and then a committer can manually cherry-pick commit 929fba8 to 1.26.x to obtained the desired end-state on both branches.

jalopezsilva

Looks good to me! We probably need a review from @sethmlarson as well :)

sethmlarson

@jalopezsilva can you confirm my thinking on these comments?

sethmlarson · 2022-08-04T17:59:45Z

src/urllib3/connection.py

@@ -500,6 +515,11 @@ def connect(self) -> None:
                SystemTimeWarning,
            )

+        if self._connecting_to_proxy and self.proxy_config:


We should use if self.proxy and self.proxy_config here.

@sethmlarson this might not work.

self._connecting_to_proxy will remain True when we're using the absolute URI forwarding mode after the block in L486. I suspect that's why @avi364 is relying on it. I don't like relying on it though, seems brittle.

Could we check not self._is_using_tunnel() and self.proxy_config instead? That will default to the absolute UI forwarding mode. It might be better to check if proxy_ssl_context is set as well.

Makes complete sense to me! @avi364 could you implement these changes and then we can merge?

sethmlarson · 2022-08-04T18:00:32Z

src/urllib3/connection.py

+                )
+            else:
+
+                self.ssl_context = proxy_config.ssl_context


Do we need to change the logic of self.ssl_context here? If we're deciding which ssl_context object to use within connect() anyways I think we can unconditionally set self.ssl_context as before.

I agree with @sethmlarson here. You're already modifying the selection logic within connect(). Keep this simple?

jalopezsilva · 2022-08-29T23:45:07Z

src/urllib3/connection.py

+                )
+            else:
+
+                self.ssl_context = proxy_config.ssl_context


I agree with @sethmlarson here. You're already modifying the selection logic within connect(). Keep this simple?

jalopezsilva · 2022-08-29T23:55:16Z

src/urllib3/connection.py

@@ -500,6 +515,11 @@ def connect(self) -> None:
                SystemTimeWarning,
            )

+        if self._connecting_to_proxy and self.proxy_config:


@sethmlarson this might not work.

self._connecting_to_proxy will remain True when we're using the absolute URI forwarding mode after the block in L486. I suspect that's why @avi364 is relying on it. I don't like relying on it though, seems brittle.

Could we check not self._is_using_tunnel() and self.proxy_config instead? That will default to the absolute UI forwarding mode. It might be better to check if proxy_ssl_context is set as well.

avi364 · 2024-01-16T12:44:35Z

I do not, feel free to take it

ecerulm · 2024-01-30T20:27:41Z

I do not, feel free to take it

I think , I there is some missing comment in the issue.

@avi364, do you mean that you are not going to pursue this PR further?

avi364 force-pushed the 2577 branch from 3c22f4a to c933a71 Compare June 28, 2022 02:03

avi364 force-pushed the 2577 branch from 8a21dc9 to 7a45ff8 Compare June 28, 2022 22:24

jalopezsilva reviewed Jun 29, 2022

View reviewed changes

Remove deprecated setting of ssl_context

6b3430f

Raise an exception if both a proxy_ssl_context and a ssl_context are provided when use_forwarding_for_https=True, instead of only raising a warning. Modify tests to account for change. Resolves urllib3#2577

avi364 force-pushed the 2577 branch from 7a45ff8 to 6b3430f Compare July 3, 2022 21:44

jalopezsilva approved these changes Aug 6, 2022

View reviewed changes

jalopezsilva requested a review from sethmlarson August 6, 2022 18:31

sethmlarson reviewed Aug 6, 2022

View reviewed changes

sethmlarson mentioned this pull request Aug 19, 2022

Proxies should use proxy_ssl_context when connecting via ProxyConfig.use_forwarding_with_https = True #2577

Open

5 tasks

jalopezsilva requested changes Aug 29, 2022

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use proxy_ssl_context as SSL ctx when forwarding #2651

Use proxy_ssl_context as SSL ctx when forwarding #2651

avi364 commented Jun 22, 2022

matttpt commented Jun 22, 2022

avi364 commented Jun 28, 2022

pquentin commented Jun 28, 2022

matttpt commented Jun 28, 2022

avi364 commented Jun 28, 2022

jalopezsilva left a comment

jalopezsilva Jun 29, 2022

jalopezsilva commented Jun 29, 2022 •

edited

avi364 commented Jul 20, 2022

jalopezsilva commented Jul 23, 2022

sethmlarson commented Jul 28, 2022

avi364 commented Jul 30, 2022

jalopezsilva left a comment

sethmlarson left a comment

sethmlarson Aug 4, 2022

jalopezsilva Aug 29, 2022

sethmlarson Aug 31, 2022

sethmlarson Aug 4, 2022

jalopezsilva Aug 29, 2022

jalopezsilva Aug 29, 2022

jalopezsilva Aug 29, 2022

avi364 commented Jan 16, 2024 via email

ecerulm commented Jan 30, 2024

Use proxy_ssl_context as SSL ctx when forwarding #2651

Are you sure you want to change the base?

Use proxy_ssl_context as SSL ctx when forwarding #2651

Conversation

avi364 commented Jun 22, 2022

matttpt commented Jun 22, 2022

avi364 commented Jun 28, 2022

pquentin commented Jun 28, 2022

matttpt commented Jun 28, 2022

avi364 commented Jun 28, 2022

jalopezsilva left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jalopezsilva commented Jun 29, 2022 • edited

avi364 commented Jul 20, 2022

jalopezsilva commented Jul 23, 2022

sethmlarson commented Jul 28, 2022

avi364 commented Jul 30, 2022

jalopezsilva left a comment

Choose a reason for hiding this comment

sethmlarson left a comment

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

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

avi364 commented Jan 16, 2024 via email

ecerulm commented Jan 30, 2024

jalopezsilva commented Jun 29, 2022 •

edited