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

default port for the socks5h scheme #972

Merged
merged 4 commits into from
Oct 28, 2024
Merged

Conversation

mateuszlitwin
Copy link
Contributor

@mateuszlitwin mateuszlitwin commented Oct 25, 2024

Summary

This change is needed to unblock encode/httpx#3178 and support socks5h scheme (server-side hostname resolution socks5 proxy). Support for the socks5h is needed to ensure global environment variables like all_proxy="socks5h://..." can be used reliably with different programs (some of which are httpx based).

Checklist

  • I understand that this PR may be closed in case there was no previous discussion. (This doesn't apply to typos!)
  • I've added a test for each change that was introduced, and I tried as much as possible to make a single atomic change.
  • I've updated the documentation accordingly.

@mateuszlitwin mateuszlitwin marked this pull request as ready for review October 25, 2024 17:09
@mateuszlitwin
Copy link
Contributor Author

mateuszlitwin commented Oct 25, 2024

Can add sync and async tests for httpcore.SOCKSProxy with socks5h scheme if needed.

@tomchristie
Copy link
Member

Thanks. Do you know implementation would need to change in establishing the connection in socks_proxy.py?

@mateuszlitwin
Copy link
Contributor Author

mateuszlitwin commented Oct 25, 2024

I don't think so:

  • I tested test_socks_proxy.py with socks5h scheme and tests passed. Happy to update these tests to test both schemes if you think it is worth it.
  • I tested this change with add socks5h proxy support httpx#3178 with real proxies.

@tomchristie
Copy link
Member

tomchristie commented Oct 25, 2024

We do already use "proxy resolves the hostname" behaviour, so our SOCKS support is "sock5h".

https://superuser.com/questions/1762341/does-chromium-not-support-socks5h-with-the-h-in-the-end

It's not obvious that we should add a synonym to the protocol name. (Staying in line with chromium's behaviour here seems reasonable, perhaps?)

@mateuszlitwin
Copy link
Contributor Author

mateuszlitwin commented Oct 25, 2024

It is true that httpx already does proxy side hostname resolution, but other tools do not. For instance, curl does not for the socks5 scheme. See also libraries like urllib3 and libcurl (https://curl.se/libcurl/c/CURLOPT_PROXY.html). This makes it impossible to set global env like all_proxy="socks5h://127.0.0.1:1080" to correctly support server-side-resolution proxy in multiple tools.

@tomchristie
Copy link
Member

Okay yep.
Looks like there's a linter failure needs resolving...

+ ruff format httpcore tests --diff
1 file would be reformatted, 37 files already formatted
--- tests/test_models.py
+++ tests/test_models.py
@@ -47,7 +47,9 @@
     assert str(url.origin) == "socks5://[12](https://github.com/encode/httpcore/actions/runs/11522347142/job/32078024585?pr=972#step:5:13)7.0.0.1:1080"
 
     url = httpcore.URL("socks5h://127.0.0.1")
-    assert url.origin == httpcore.Origin(scheme=b"socks5h", host=b"127.0.0.1", port=1080)
+    assert url.origin == httpcore.Origin(
+        scheme=b"socks5h", host=b"127.0.0.1", port=1080
+    )
     assert str(url.origin) == "socks5h://127.0.0.1:1080"

@mateuszlitwin
Copy link
Contributor Author

I think you are looking at the wrong revision?

Copy link
Member

@tomchristie tomchristie left a comment

Choose a reason for hiding this comment

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

Okay, yep.
Thanks! 🙏🏼

@tomchristie tomchristie merged commit 0bfcee4 into encode:master Oct 28, 2024
5 checks passed
@mateuszlitwin mateuszlitwin deleted the patch-1 branch October 28, 2024 19:41
@mateuszlitwin
Copy link
Contributor Author

@tomchristie Could you revisit encode/httpx#3178 after merging this PR? I think this might have been the blocker.

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.

2 participants