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

Add support for HTTPS proxies (available to trio/asyncio) #745

Merged
merged 13 commits into from
Sep 1, 2023

Conversation

karpetrosyan
Copy link
Member

@karpetrosyan karpetrosyan commented Jul 4, 2023

Refs #722

Related #732 (Required for sync support)

TODO

  • Add proxy_ssl_context argument to AsyncForwardHTTPConnection, AsyncTunnelHTTPConnection and AsyncHTTPProxy classes
  • Add changelog

@karpetrosyan karpetrosyan changed the title Add support for HTTPS Proxy Add support for HTTPS Proxies Jul 4, 2023
@karpetrosyan karpetrosyan added the enhancement New feature or request label Jul 7, 2023
@T-256 T-256 mentioned this pull request Jul 17, 2023
@tomchristie
Copy link
Member

Looks like we could do with a docs update here...

https://www.encode.io/httpcore/proxies/

We probably want to split "Proxy SSL and HTTP Versions" into "Proxy SSL" which documents the proxy_ssl_context argument, and HTTP Versions which notes the HTTP/1.1 constraint.

@tomchristie
Copy link
Member

Okay, so attempting to test this...

I'm using trio in this example, so as not to be blocked by #732.

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 trio
import httpcore


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

async def main():
    async with httpcore.AsyncHTTPProxy(proxy_url="http://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)

Fails with...

[...]
ssl.SSLError: [SSL: HTTPS_PROXY_REQUEST] https proxy request (_ssl.c:992)

What am I getting wrong here?

@tomchristie
Copy link
Member

tomchristie commented Aug 30, 2023

Also, occurs to me that something we could do here would be decouple this from #732.

Indicate in the CHANGELOG/docs that HTTPS proxies are only supported with trio and asyncio, and ensure that we raise a runtime error if TLS-in-TLS is attempted with the sync backend.

@T-256
Copy link
Contributor

T-256 commented Aug 30, 2023

Fails with...

[...]
ssl.SSLError: [SSL: HTTPS_PROXY_REQUEST] https proxy request (_ssl.c:992)

I think problem is here:

if self._origin.scheme == b"https":

Where you set proxy_url="http://127.0.0.1:8899/", so self._origin.scheme is http, and it won't do tls handshake.

@T-256
Copy link
Contributor

T-256 commented Aug 30, 2023

I also had this problem on #734 and ended up by scheme checking.
IMO we should do ssl context checking instead of scheme checking. (e.g. here too)

@karpetrosyan
Copy link
Member Author

karpetrosyan commented Aug 31, 2023

What am I getting wrong here?

Just change the proxy url scheme to https.

@karpetrosyan
Copy link
Member Author

Also, occurs to me that something we could do here would be decouple this from #732.

Indicate in the CHANGELOG/docs that HTTPS proxies are only supported with trio and asyncio, and ensure that we raise a runtime error if TLS-in-TLS is attempted with the sync backend.

We should probably merge #732 first, then go over this one.

@tomchristie
Copy link
Member

Wonderful thanks, tested now and all working as expected.

Okay so, should we add the following error cases here?...

  • If proxy_url is set to https and proxy_ssl_context is unset, then raise an exception.
  • If proxy_url is set to http and proxy_ssl_context is set, then raise an exception.

I'd suggest that this PR is neat and uncontentious, so let's work to getting it merged, as "Add support for HTTPS proxies (available to trio/asyncio)". Then follow up with the more awkward sync support.

Remaining here is...

  • Documentation.
  • Raise an error in the sync backend if TLS-in-TLS is attempted.

@@ -51,10 +51,33 @@ proxy = httpcore.HTTPProxy(
)
```

## Proxy SSL and HTTP Versions
## Proxy SSL
Copy link
Member

Choose a reason for hiding this comment

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

These docs are really nice, thanks! 💚

@tomchristie tomchristie mentioned this pull request Sep 1, 2023
@karpetrosyan
Copy link
Member Author

If proxy_url is set to https and proxy_ssl_context is unset, then raise an exception.

Because most users do not interact with SSL contexts, we can allow this behavior to keep things as simple as possible.

When using the public HTTPS proxy, for example, the user must create the default SSL context and pass it to the HTTPProxy class without any modifications or configurations.

If proxy_url is set to http and proxy_ssl_context is set, then raise an exception.

Agree

@karpetrosyan karpetrosyan changed the title Add support for HTTPS Proxies Add support for HTTPS proxies (available to trio/asyncio) Sep 1, 2023
docs/proxies.md Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
@tomchristie
Copy link
Member

Great - No objections to this.

Welcome to merge once you're also happy with it @karosis88.

@karpetrosyan karpetrosyan merged commit f30da8c into encode:master Sep 1, 2023
4 checks passed
This was referenced Sep 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants