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

fixes #3036 -Setup the SSL Context correctly for https proxies #3038

Closed
wants to merge 1 commit into from
Closed

fixes #3036 -Setup the SSL Context correctly for https proxies #3038

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGES/3036.bugfix
View file
Open in desktop
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
9 changes: 9 additions & 0 deletions pulpcore/download/factory.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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():
Copy link
Member

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.

sslcontext = ssl.create_default_context()

if self._remote.client_key and self._remote.client_cert:
if not sslcontext:
sslcontext = ssl.create_default_context()
Expand All @@ -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()
Copy link
Member

Choose a reason for hiding this comment

The 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 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.

tcp_conn_opts["ssl_context"] = sslcontext

headers = MultiDict({"User-Agent": DownloaderFactory.user_agent()})
Expand Down Expand Up @@ -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)
Copy link
Member

Choose a reason for hiding this comment

The 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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.
May be we should have code along

if hasattr(asyncio.sslproto._SSLProtocolTransport, "_start_tls_compatible"):  <setup https tunneling>

Copy link
Member

Choose a reason for hiding this comment

The 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(
Expand Down Expand Up @@ -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"