Support https proxy servers #83089
Support https proxy servers #83089
Conversation
|
|
||
| The fact that http.client.HTTPConnection.connect establishes the tunnel is the 1st reason | ||
| HTTPS proxies aren't supported, the 2nd is that there is no native TLS in TLS, see TLSinTLS | ||
| """ |
There was a problem hiding this comment.
Overriding _tunnel to not do anything, and then calling super()._tunnel() are probably the biggest concerns with fragility and supportability here, but urllib3 does it too, kind of:
https://github.com/urllib3/urllib3/blob/b7c2910a9f49ba0d58c8d4381500c208f9a24765/src/urllib3/connection.py#L242
https://github.com/urllib3/urllib3/blob/b7c2910a9f49ba0d58c8d4381500c208f9a24765/src/urllib3/connection.py#L631
There was a problem hiding this comment.
This doesn't look like it should be a docstring but rather a comment. Could you change?
| port = selector.port | ||
| if not port: | ||
| port = 443 if tls_in_tls else 80 | ||
| req._tunnel_host = f'{selector.hostname}:{port}' |
There was a problem hiding this comment.
All of this weirdness is just related to the fact that the existing code never attempted this, but again we're poking a private attribute.
ansibot
added
feature
| @@ -0,0 +1,41 @@ | |||
| - when: ansible_facts.distribution not in ['MacOSX'] | |||
There was a problem hiding this comment.
It's possible to also test a TLS proxy by combining https://pypi.org/p/trustme and https://pypi.org/p/proxy.py. This is what I integrated into the aiohttp test suite back when @jborean93, Satellite folks and I were poking on how to implement TLS-in-TLS transport with aiohttp a few years back. Well, the trustme was pre-integrated earlier to test true TLS w/o disabling trust verification.
I also integrated trustme into the ansible-core integration tests in December: #82334. So we do have usage examples in our codebase.
Please, consider using it here as well to make it possible to run this integration test against macOS too.
|
|
||
| - name: Generate cert for stunnel | ||
| command: >- | ||
| openssl req -new -x509 -days 365 -nodes -out stunnel.pem -keyout stunnel.pem \ |
There was a problem hiding this comment.
Based on the low-level TLS management I see here I believe that my suggestion to use trustme might simplify the configuration and positively affect the maintainability: https://github.com/ansible/ansible/pull/83089/files#r1571440157.
| self.read = functools.partial(self._process, True, sslobj.read) | ||
| self.recv = functools.partial(self._process, True, sslobj.read) | ||
| self.send = functools.partial(self._process, False, sslobj.write) | ||
| self.sendall = functools.partial(self._process, False, sslobj.write) |
There was a problem hiding this comment.
Do we need to worry about something calling sendall for this when the payload exceeds 16KiB? Is that even something we can do, i.e. a POST request with a large file?
There was a problem hiding this comment.
Ah just realized this is for TLS in TLS so it should never be sending/receiving any packets larger than this anyway.
| self.recv = functools.partial(self._process, True, sslobj.read) | ||
| self.send = functools.partial(self._process, False, sslobj.write) | ||
| self.sendall = functools.partial(self._process, False, sslobj.write) | ||
| self._decref_socketios = socket._decref_socketios |
There was a problem hiding this comment.
Is something in the http.client.HTTPSConnection calling this internally?
There was a problem hiding this comment.
socket.SocketIO used in makefile uses it in the close method, which is called by http. I could possibly get around this by not using socket.SocketIO directly, or subclass it.
webknjaz
added
the
ci_verified
|
For the record, I'm not really in favor of merging this. It's touching too much that I just don't feel is supportable. And maybe with a further investment in time and code, it could maybe be made a little better, but I'm leary of the potential burden on future me and/or other devs to support this. I'm open to being wrong about this, and just looking for feedback. |
|
While I feel the chances of urllib/http.client changing the way things are called internally I definitely wouldn't want to be the person who has to debug this if something was to break. On the flip side I do see people having to use a TLS based proxy becoming a more common scenario as time goes on so this can be a useful feature to have. Unfortunately I don't see this really being fixed properly in CPython itself so I'm not sure what the best way forward would be. |
mkrizek
removed
the
needs_triage
|
Closing this per above. |
SUMMARY
This PR adds the ability to proxy over a proxy server that listens on https.
ISSUE TYPE
ADDITIONAL INFORMATION
This will conflict with #83085