-
Notifications
You must be signed in to change notification settings - Fork 113
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
fixes #3036 -Setup the SSL Context correctly for https proxies #3038
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
Fixed setting up of default ssl context if the proxy is https |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -107,6 +107,9 @@ def _make_aiohttp_session_from_remote(self): | |
sslcontext = None | ||
if self._remote.ca_cert: | ||
sslcontext = ssl.create_default_context(cadata=self._remote.ca_cert) | ||
elif self._is_remote_proxy_secure(): | ||
sslcontext = ssl.create_default_context() | ||
|
||
if self._remote.client_key and self._remote.client_cert: | ||
if not sslcontext: | ||
sslcontext = ssl.create_default_context() | ||
|
@@ -123,6 +126,7 @@ def _make_aiohttp_session_from_remote(self): | |
sslcontext.check_hostname = False | ||
sslcontext.verify_mode = ssl.CERT_NONE | ||
if sslcontext: | ||
sslcontext.load_default_certs() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think we can opt all of our users into this behavior given that it's been an application level trust config for all the pulpcore 3.y so far. One option would be to make a configurable option on the Master I'd like to hear from other pulpcore devs too. |
||
tcp_conn_opts["ssl_context"] = sslcontext | ||
|
||
headers = MultiDict({"User-Agent": DownloaderFactory.user_agent()}) | ||
|
@@ -194,6 +198,8 @@ class to be instantiated. | |
""" | ||
options = {"session": self._session} | ||
if self._remote.proxy_url: | ||
if self._is_remote_proxy_secure(): | ||
setattr(asyncio.sslproto._SSLProtocolTransport, "_start_tls_compatible", True) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't feel comfortable with us monkey patching a Python standard library. I believe this could be done in a downstream patch or a patch to a specific system though just fine. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Well it does not work without that There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The suggestion I read from the aiohttp community was to use a newer version of Python. I forget which version, but the newer versions of Python have this attr set. If downstreams want to set this attr on older versions of Python, that would be a good patch for the downstream builds. In terms of where in the downstream builds, that's kind of up to whoever is patching that. My opinion is that it shouldn't be in Pulp since Pulp even in its downstream shouldn't be patching Python itself. Really I think the best path is to upgrade the version of Python. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Definitely not in 3.10.6 . May be the 3.11 that just got released has it afaik. if hasattr(asyncio.sslproto._SSLProtocolTransport, "_start_tls_compatible"): <setup https tunneling> There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes it would be Python 3.11. https://bugs.python.org/issue37179 |
||
options["proxy"] = self._remote.proxy_url | ||
if self._remote.proxy_username and self._remote.proxy_password: | ||
options["proxy_auth"] = aiohttp.BasicAuth( | ||
|
@@ -225,3 +231,6 @@ class to be instantiated. | |
is configured with the remote settings. | ||
""" | ||
return download_class(url, **kwargs) | ||
|
||
def _is_remote_proxy_secure(self): | ||
return self._remote.proxy_url and urlparse(self._remote.proxy_url).scheme == "https" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this would be tied to the setting also. I don't expect the proxy to be the defining aspect of if the
remote.ca_cert
is ignored or not. Additionally, even if the default trust store was loaded, I suspect the remote's ca_cert would be layered on top.