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.

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

[PR #5992/c29e5fb5 backport][3.8] Add secure proxy support in the client #6049

Conversation

webknjaz
Copy link
Member

@webknjaz webknjaz commented Oct 5, 2021

This is a backport of PR #5992 as merged into master (c29e5fb). It also includes #6072.

Note: --threadless in proxy.py is disabled under Windows and macOS because of abhinavsingh/proxy.py#492.

What do these changes do?

This patch opens up the code path and adds the implementation that allows end-users to start sending HTTPS requests through HTTPS proxies.

The support for TLS-in-TLS (needed for this to work) in the stdlib is kinda available since Python 3.7 but is disabled for asyncio with an attribute/flag/toggle. When the upstream CPython enables it finally, aiohttp v3.8+ will be able to work with it out of the box.

Currently the tests monkey-patch asyncio in order to verify that this works. The users who are willing to do the same, will be able to take advantage of it right now. Eventually (hopefully starting Python 3.11), the need for monkey-patching should be eliminated.

Refs:

Original details

I test with this script:

import aiohttp
import asyncio


url = "http://example.com"
url = "https://example.com"

proxy_url = "http://localhost:3128/"
proxy_url = "https://localhost:3130/"

async def main():
    import pydevd_pycharm
    # pydevd_pycharm.settrace('localhost', port=29437, stdoutToServer=True, stderrToServer=True, suspend=False)
    async with aiohttp.ClientSession(headers={"Cache-Control": "no-cache"}) as session:
        async with session.get(url, proxy=proxy_url, verify_ssl=False) as resp:
            print(resp.status)
            print(resp)
            print(await resp.text())


loop = asyncio.get_event_loop()
loop.run_until_complete(main())

And I get

Traceback (most recent call last):
  File "/home/vagrant/devel/aiohttp_proxy_test.py", line 22, in <module>
    loop.run_until_complete(main())
  File "/usr/lib64/python3.9/asyncio/base_events.py", line 642, in run_until_complete
    return future.result()
  File "/home/vagrant/devel/aiohttp_proxy_test.py", line 15, in main
    async with session.get(url, proxy=proxy_url, verify_ssl=False) as resp:
  File "/home/vagrant/devel/aiohttp/aiohttp/client.py", line 1117, in __aenter__
    self._resp = await self._coro
  File "/home/vagrant/devel/aiohttp/aiohttp/client.py", line 520, in _request
    conn = await self._connector.connect(
  File "/home/vagrant/devel/aiohttp/aiohttp/connector.py", line 535, in connect
    proto = await self._create_connection(req, traces, timeout)
  File "/home/vagrant/devel/aiohttp/aiohttp/connector.py", line 890, in _create_connection
    _, proto = await self._create_proxy_connection(req, traces, timeout)
  File "/home/vagrant/devel/aiohttp/aiohttp/connector.py", line 1139, in _create_proxy_connection
    transport, proto = await self._wrap_create_connection(
  File "/home/vagrant/devel/aiohttp/aiohttp/connector.py", line 969, in _wrap_create_connection
    return await self._loop.create_connection(*args, **kwargs)  # type: ignore  # noqa
  File "/usr/lib64/python3.9/asyncio/base_events.py", line 1081, in create_connection
    transport, protocol = await self._create_connection_transport(
  File "/usr/lib64/python3.9/asyncio/base_events.py", line 1111, in _create_connection_transport
    await waiter
ConnectionResetError

The squid proxy does show 1631129861.057 45 ::1 TCP_TUNNEL/200 39 CONNECT example.com:443 - HIER_DIRECT/93.184.216.34 - and I can see the 200 OK from the direct connection.

What fails is when taking the TCP socket from the direct connection and TLS wrapping it I get a ConnectionResetError.

Are there changes in behavior for the user?

They get the ability to send out HTTPS queries through HTTPS proxies if they monkey-patch the stdlib asyncio.

Related issue number

Resolves #3816
Resolves #4268

Checklist

  • I think the code is well written
  • Unit tests for the changes exist
  • Documentation reflects the changes
  • If you provide code modification, please add yourself to CONTRIBUTORS.txt
    • The format is <Name> <Surname>.
    • Please keep alphabetical order, the file is sorted by names.
  • Add a new news fragment into the CHANGES folder
    • name it <issue_id>.<type> for example (588.bugfix)
    • if you don't have an issue_id change it to the pr id after creating the pr
    • ensure type is one of the following:
      • .feature: Signifying a new feature.
      • .bugfix: Signifying a bug fix.
      • .doc: Signifying a documentation improvement.
      • .removal: Signifying a deprecation or removal of public API.
      • .misc: A ticket has been closed, but it is not of interest to users.
    • Make sure to use full sentences with correct case and punctuation, for example: "Fix issue with non-ascii contents in doctest text files."

@psf-chronographer psf-chronographer bot added the bot:chronographer:provided There is a change note present in this PR label Oct 5, 2021
@webknjaz webknjaz added this to the 4.0 milestone Oct 5, 2021
@webknjaz webknjaz force-pushed the patchback/backports/3.8/c29e5fb58efb65c6198ea75b95805ab972e98adc/pr-5992 branch from fe151e4 to d3f881a Compare October 5, 2021 23:54
@webknjaz
Copy link
Member Author

webknjaz commented Oct 6, 2021

@asvetlov when do you think we would be able to drop support for Python 3.6? Can we drop it in aiohttp v3.8 or do you want to wait?

This backport breaks one code path under Python 3.6 which is not a problem for master but we need to decide what to do for 3.8. It should be possible to improve the backport to preserve parts of the original behavior and make the respective tests pass but it would be much easier if we didn't have to support Python 3.6, especially as it's going EOL in 2.5 months.

@webknjaz webknjaz requested a review from asvetlov October 6, 2021 22:34
@webknjaz webknjaz self-assigned this Oct 6, 2021
@webknjaz webknjaz modified the milestones: 4.0, 3.8 Oct 7, 2021
@Dreamsorcerer
Copy link
Member

@asvetlov when do you think we would be able to drop support for Python 3.6? Can we drop it in aiohttp v3.8 or do you want to wait?

Given the large number of bug fixes in the 3.8 release, I'd vote for trying to push a release that still supports Python 3.6 before we drop it.

@webknjaz
Copy link
Member Author

webknjaz commented Oct 7, 2021

Given the large number of bug fixes in the 3.8 release, I'd vote for trying to push a release that still supports Python 3.6 before we drop it.

I hear you. It's just that I need this backport in and supporting 3.6 means more work for me :) I've already convinced the project needing this not to use Python 3.6 FWIW.

@webknjaz webknjaz force-pushed the patchback/backports/3.8/c29e5fb58efb65c6198ea75b95805ab972e98adc/pr-5992 branch from d3f881a to 991c35e Compare October 11, 2021 00:47
@webknjaz webknjaz force-pushed the patchback/backports/3.8/c29e5fb58efb65c6198ea75b95805ab972e98adc/pr-5992 branch from 991c35e to f91ca06 Compare October 11, 2021 00:57
@webknjaz webknjaz marked this pull request as ready for review October 11, 2021 01:01
@webknjaz webknjaz enabled auto-merge (squash) October 11, 2021 01:03
@webknjaz webknjaz force-pushed the patchback/backports/3.8/c29e5fb58efb65c6198ea75b95805ab972e98adc/pr-5992 branch 2 times, most recently from 900cfa2 to ff0fedc Compare October 11, 2021 23:30
bmbouter and others added 2 commits October 12, 2021 02:01
This patch opens up the code path and adds the implementation that
allows end-users to start sending HTTPS requests through
HTTPS proxies.

The support for TLS-in-TLS (needed for this to work) in the stdlib is
kinda available since Python 3.7 but is disabled for `asyncio` with an
attribute/flag/toggle. When the upstream CPython enables it finally,
aiohttp v3.8+ will be able to work with it out of the box.

Currently the tests monkey-patch `asyncio` in order to verify that
this works. The users who are willing to do the same, will be able to
take advantage of it right now. Eventually (hopefully starting Python
3.11), the need for monkey-patching should be eliminated.

Refs:
* https://bugs.python.org/issue37179
* python/cpython#28073
* https://docs.aiohttp.org/en/stable/client_advanced.html#proxy-support
* aio-libs#6044

PR aio-libs#5992
Resolves aio-libs#3816
Resolves aio-libs#4268

Co-authored-by: Brian Bouterse <bmbouter@gmail.com>
Co-authored-by: Jordan Borean <jborean93@gmail.com>
Co-authored-by: Sviatoslav Sydorenko <webknjaz@redhat.com>
(cherry picked from commit c29e5fb)
Apparently, if an exception gets raised before the warning gets a
chance to be emitted, it doesn't emit an assertion error. Pytest bug?
@webknjaz webknjaz force-pushed the patchback/backports/3.8/c29e5fb58efb65c6198ea75b95805ab972e98adc/pr-5992 branch from 817ab73 to 320f545 Compare October 12, 2021 00:01
@webknjaz webknjaz force-pushed the patchback/backports/3.8/c29e5fb58efb65c6198ea75b95805ab972e98adc/pr-5992 branch from 320f545 to 2715040 Compare October 12, 2021 00:03
@webknjaz webknjaz merged commit cf641aa into aio-libs:3.8 Oct 12, 2021
@webknjaz webknjaz deleted the patchback/backports/3.8/c29e5fb58efb65c6198ea75b95805ab972e98adc/pr-5992 branch April 19, 2024 21:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bot:chronographer:provided There is a change note present in this PR client enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants