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

proxy mode for http proxies #734

Open
wants to merge 60 commits into
base: master
Choose a base branch
from
Open

proxy mode for http proxies #734

wants to merge 60 commits into from

Conversation

T-256
Copy link
Contributor

@T-256 T-256 commented Jun 17, 2023

Closes #735
Refs: #703 #716

  • tests (forward and tunnel tests already implemented)
  • docs + warn about insecure use of ProxyMode.FORWARD
  • changelog

@T-256
Copy link
Contributor Author

T-256 commented Jun 17, 2023

current implementation is modified and upgraded of first option discussed in #703 (reply in thread).
for second option we can use only one flagged param:

class ProxyMode(enum.IntFlag):
    HTTPS_FORWARD = 1
    HTTP_TUNNEL = 2


def HTTPProxy(
    ...
    mode: Optional[ProxyMode] = None,
): ...

EDIT: Now I think, this would be better option (using one param instead of two), going for replace it.

@T-256 T-256 changed the title plain_mode and tls_mode for proxy. proxy mode for http(s) destinations. Jun 17, 2023
httpcore/_async/http_proxy.py Outdated Show resolved Hide resolved
httpcore/_async/http_proxy.py Outdated Show resolved Hide resolved
httpcore/_models.py Outdated Show resolved Hide resolved
@karpetrosyan
Copy link
Member

We should probably wait for #714, then open the issue, and only then go through this for further clarity.

@T-256
Copy link
Contributor Author

T-256 commented Jun 17, 2023

We should probably wait for #714, then open the issue, and only then go through this for further clarity.

I didn't understand. Will #714 create any blocker for this PR or vice versa?
Actually it just adds TLS for connecting to proxy server, both forwarding and tunneling will sent normally after TLS stablished.
So, IMO it is just extra layer and should be compatible with this PR.

@karpetrosyan
Copy link
Member

I've started an issue for this PR.

@karpetrosyan
Copy link
Member

Let's change the title of this PR to something like "Add force-tunneling and force-forwarding support for proxies."

@T-256 T-256 changed the title proxy mode for http(s) destinations. Add force-http-tunneling OR force-https-forwarding support for proxies. Jun 19, 2023
@T-256 T-256 changed the title Add force-http-tunneling OR force-https-forwarding support for proxies. Add force-http-tunneling and force-https-forwarding support for proxies. Jun 19, 2023
@tomchristie
Copy link
Member

Really I'd prefer that we work through design discussions first before starting on the pull request, but...

  • I'm okay with reviewing this once there's docs.
  • We also need to consider what the implications/expectations are for pushing this downstream to httpx.

@T-256
Copy link
Contributor Author

T-256 commented Aug 27, 2023

Is it possible to include in #768?

@tomchristie
Copy link
Member

tomchristie commented Aug 28, 2023

So...

  1. FORWARD/TUNNEL makes more sense to me. That's a reading that'd make sense to me from these docs. It's also consistent with urllib3, and our httpx docs.
  2. I'm not fully convinced yet that we couldn't instead have a use_forwarding_for_https flag. That's the API that urllib3 use for this manual control, and they don't seem to have open issues suggesting that "allow CONNECT tunnelling to http" is something their users are missing. I can see that we discussed this briefly at Configure FORWARD/TUNNEL mode for proxies. #703 (reply in thread). Is someone able to provide an example of a proxy that would require this configuration, so I'm able to test it out and verify that we really do want to support that as a configuration option?

@karpetrosyan
Copy link
Member

  1. Completely agree
  2. If the proxy server supports tunneling and it's not a public proxy, then using tunneling over HTTP makes sense.
    Assume that we are using a local HTTP proxy server that supports tunneling. We do not need to configure certificates and make the proxy server secure because we actually trust it.

Example

  trusted
host -> proxy

   not trusted
proxy -> remote server

| Solution |

HTTP tunneling gives
      http       https
host   ->   proxy -> remote-server

docs/proxies.md Outdated Show resolved Hide resolved
docs/proxies.md Outdated Show resolved Hide resolved
docs/proxies.md Outdated Show resolved Hide resolved
T-256 and others added 3 commits September 6, 2023 15:40
Co-authored-by: Tom Christie <tom@tomchristie.com>
Co-authored-by: Tom Christie <tom@tomchristie.com>
Co-authored-by: Tom Christie <tom@tomchristie.com>
Copy link
Member

@karpetrosyan karpetrosyan left a comment

Choose a reason for hiding this comment

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

LGTM!

@karpetrosyan karpetrosyan mentioned this pull request Sep 7, 2023
1 task
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 force-forwarding and force-tunneling
3 participants