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.

Sign up for GitHub

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

Jump to bottom

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

Open
5 tasks
sethmlarson opened this issue Mar 5, 2022 · 6 comments · May be fixed by #2651
Open
5 tasks

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

sethmlarson opened this issue Mar 5, 2022 · 6 comments · May be fixed by #2651
Assignees
@jalopezsilva
Labels
💰 Bounty $500 If you complete this issue we'll pay you $500 on OpenCollective! TLS

Comments

@sethmlarson
Copy link
Member

sethmlarson commented Mar 5, 2022 •
edited

This was discovered in #2558 and discussed with @jalopezsilva on Discord. The gist is it appears that we're using HTTPSConnection.ssl_context to connect to HTTPS proxies when using use_forwarding_with_https = True mode. This is likely caused by us treating the proxy like it's the origin when we have proxies in "forwarding" mode and previously we didn't have a forwarding mode for HTTPS.

Minimum requirements

💵 You can get paid to complete this issue! Please read the docs for more information.

  • When ProxyConfig.use_forwarding_with_https = True and ssl_context is specified:
    • If proxy_ssl_context is specified then error out with ValueError
    • If proxy_ssl_context isn't specified:
      • On 1.26.x emit a DeprecationWarning instructing to switch to proxy_ssl_context and set the value of ssl_context to proxy_ssl_context
      • On 2.x raise a ValueError that proxy_ssl_context should be used, not ssl_context.
  • Only proxy_ssl_context should be used for creating the connection to proxies in all modes (HTTP->HTTPS, HTTPS->HTTP, HTTPS->HTTPS, in forwarding/tunnel modes).
  • Remove the skip to test_proxy_https_target_tls_error added in 9f4d05c
@sethmlarson sethmlarson added the TLS label Mar 5, 2022
sethmlarson added a commit to sethmlarson/urllib3 that referenced this issue Mar 5, 2022
@sethmlarson
Skip expected failure due to urllib3#2577
36e81bb
@sethmlarson sethmlarson added 💰 Bounty $500 If you complete this issue we'll pay you $500 on OpenCollective! and removed Paid: $500 💵 labels Jun 21, 2022
@avi364 avi364 linked a pull request Jun 22, 2022 that will close this issue
Open
avi364 added a commit to avi364/urllib3 that referenced this issue Jun 28, 2022
@avi364
Use proxy_ssl_context as SSL ctx when forwarding
c933a71 
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 an exception 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.

Fixes urllib3#2577
avi364 added a commit to avi364/urllib3 that referenced this issue Jun 28, 2022
@avi364
Use proxy_ssl_context as SSL ctx when forwarding
929fba8 
    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 added a commit to avi364/urllib3 that referenced this issue Jun 28, 2022
@avi364
Remove deprecated setting of ssl_context
7a45ff8 
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 added a commit to avi364/urllib3 that referenced this issue Jul 3, 2022
@avi364
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
@fyunusa
Copy link

fyunusa commented Aug 13, 2022 •
edited

Hi @sethmlarson have this issue been fixed ?
I propose this patch for the 2.x version

if use_forwarding_for_https == True and proxy_ssl_context != None:
            if isinstance(proxy_ssl_context, ssl.SSLContext) != True:
                self.proxy_config = ProxyConfig(proxy_ssl_context, use_forwarding_for_https)

            elif proxy_ssl_context == ssl.SSLContext:
                raise ValueError(
                    "proxy ssl context should be used, not ssl context"
                )

@pquentin
Copy link
Member

You will need a pull request with tests before we can review your code.

@fyunusa
Copy link

fyunusa commented Aug 19, 2022

OK @pquentin noted I'll work on that also

@sethmlarson
Copy link
Member Author

sethmlarson commented Aug 19, 2022 •
edited

@umarfarouk98 this issue is already being worked on by @avi364 I think in #2651?

@fyunusa
Copy link

fyunusa commented Aug 19, 2022

OK yeah, I see that

@sg3-141-592 sg3-141-592 mentioned this issue Jun 17, 2023
Closed
@sg3-141-592
Copy link
Contributor

Hey @abebeos , I only ever got as far as adding the ValueError exception and a unit test for it. I'm not going to be finishing this change so feel free to reuse anything that's useful off my draft PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@jalopezsilva jalopezsilva

Labels
💰 Bounty $500 If you complete this issue we'll pay you $500 on OpenCollective! TLS
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Use proxy_ssl_context as SSL ctx when forwarding avi364/urllib3
5 participants
@pquentin @jalopezsilva @sg3-141-592 @sethmlarson @fyunusa