fixes #3036 -Setup the SSL Context correctly for https proxies #3038
Conversation
|
commit needs a "fixes #3036" , |
parthaa
changed the title
| @@ -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.
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.
Well it does not work without that setattr change. The rest of PR is of no use if proxy tunneling wont run. If you can think of a better approach let me know.
There was a problem hiding this comment.
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.
Definitely not in 3.10.6 . May be the 3.11 that just got released has it afaik.
May be we should have code along
if hasattr(asyncio.sslproto._SSLProtocolTransport, "_start_tls_compatible"): <setup https tunneling>There was a problem hiding this comment.
Yes it would be Python 3.11. https://bugs.python.org/issue37179
| @@ -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.
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 Remote object in pulpcore and having it default to off. Then if that option was set for a remote it could use the system trust store. Perhaps a global setting would be another alternative. Anyways, I think users need to be in control of opting in to this new behavior.
I'd like to hear from other pulpcore devs too.
| @@ -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(): | |||
There was a problem hiding this comment.
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.
|
This should work on Python 3.11, so I'm going to close since Pulp itself doesn't have a problem to change. If anyone has questions about how to patch versions with Python < 3.11 please let me know. |
Setup the SSL Context correctly for HTTPS proxies. Look at the issue for more information
#3036