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

HTTPS proxy support #786

Merged
merged 19 commits into from
Sep 1, 2023
Merged

HTTPS proxy support #786

merged 19 commits into from
Sep 1, 2023

Conversation

tomchristie
Copy link
Member

@tomchristie tomchristie commented Sep 1, 2023

This PR is @karosis88 work from #732 but simplified down to the minimal possible change.

Closes #722

Why would you take that work and then drop the timeout behaviour and test cases, @tomchristie?

Deep breath...

Okay, to me there are still some contentions remaining around expectations for timeout behaviour in #732. I don't want to get too deep into this because I'm not sure the conversation effort is worth us blocking HTTPS proxy support. But briefly...

It's okay for the read and write timeouts to semantically be "socket read" and "socket write" timeouts, rather than "stream read" and "stream write" timeouts. If that isn't quite what we want, then we're already failing in our constraints - see the write timeout behaviour when the buffer requires multiple send operations. In any case it'd be sensible for us to unblock any conversations on this from the higher priority "let's have HTTPS proxy support".

Testing this also adds a bit additional chunk of complexity. I'm not wild about conftest - it's decoupled in a way that makes the flow of code non-obvious. I'm going to suggest that we go very slightly off-piste here, and add this functionality with a bigger-tha-usual chunk of "pragma: nocover", and then deal with any follow up in a way that isn't blocking us.

So...

Here's how to test this functionality locally...

Installation...

$ pip install httpcore trio trustme proxy.py

Use trustme to generate certs...

$ python -m trustme
Generated a certificate for 'localhost', '127.0.0.1', '::1'
Configure your server to use the following files:
  cert=/Users/tomchristie/GitHub/encode/httpcore/server.pem
  key=/Users/tomchristie/GitHub/encode/httpcore/server.key
Configure your client to use the following files:
  cert=/Users/tomchristie/GitHub/encode/httpcore/client.pem

Running the proxy...

$ proxy --cert-file server.pem --key-file server.key 

Make the requests...

import ssl
import httpcore


ctx = ssl.create_default_context()
ctx.load_verify_locations('client.pem')

with httpcore.HTTPProxy(proxy_url="https://127.0.0.1:8899/", proxy_ssl_context=ctx) as pool:
    response = pool.request("GET", "https://www.example.com/")
    print(response)
    response = pool.request("GET", "http://www.example.com/")
    print(response)

Or with async...

import ssl
import trio
import httpcore


ctx = ssl.create_default_context()
ctx.load_verify_locations('client.pem')

async def main():
    async with httpcore.AsyncHTTPProxy(proxy_url="https://127.0.0.1:8899/", proxy_ssl_context=ctx) as http:
        response = await http.request("GET", "https://www.example.com/")
        print(response)
        response = await http.request("GET", "http://www.example.com/")
        print(response)


trio.run(main)

Do we have enough in agreement here that we could merge this PR and deal with (or punt on) the above points of contention in a way that doesn't block our HTTPS proxy support?

CHANGELOG.md Outdated Show resolved Hide resolved
tomchristie and others added 2 commits September 1, 2023 12:52
Co-authored-by: Kar Petrosyan <92274156+karosis88@users.noreply.github.com>
@karpetrosyan
Copy link
Member

I assumed we'd merge this once #721 was resolved. But I'm fine with it.

@tomchristie tomchristie merged commit a42df6c into master Sep 1, 2023
4 checks passed
@tomchristie tomchristie deleted the https-proxy-support branch September 1, 2023 13:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support HTTPS proxies.
2 participants