Skip to content

Conversation

@JPDye
Copy link
Contributor

@JPDye JPDye commented Apr 25, 2025

cc hyperium/hyper#3851, hyperium/hyper#3877 #173

Adds SocksV4 and SocksV5 connectors with simple configuration for local and remote DNS. Supports SOCKSv5 user/pass authentication and allows optimistic message sending (not in RFC but supported by most servers) to reduce number of round-trips in SOCKSv5 handshake.

TODO:

  • Properly test optimistic sending against real server implementations
  • Cleanup error handling

@JPDye JPDye marked this pull request as draft April 25, 2025 14:18
@seanmonstar
Copy link
Member

I think this looks pretty great! What's holding it back as a draft?

@JPDye JPDye force-pushed the socks-connector branch from 591d8f7 to cf3c983 Compare May 8, 2025 12:22
@JPDye
Copy link
Contributor Author

JPDye commented May 8, 2025

Needed to test optimistic sending to see if it was worth including. 5/8 of the server implementations I tested worked so I feel like it's a worthwhile addition even if it's not in the RFC.

Will fix the failing check and some of the dirty code in a little, but I'm mostly happy with everything now

@JPDye JPDye marked this pull request as ready for review May 8, 2025 12:41
@JPDye JPDye force-pushed the socks-connector branch from 56e0cff to 2370f34 Compare May 12, 2025 15:27
Copy link
Member

@seanmonstar seanmonstar left a comment

Choose a reason for hiding this comment

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

This looks phenomenal, thank you!

@seanmonstar seanmonstar merged commit 7e34375 into hyperium:master May 19, 2025
16 checks passed
@JPDye
Copy link
Contributor Author

JPDye commented May 19, 2025

Thanks for merging this. Is the goal still to refactor Reqwest to use these connectors? I'm happy to take that on to pad my stats so to speak ;)

@seanmonstar
Copy link
Member

Yes it is, just filed a new issue: seanmonstar/reqwest#2683

I've got the HTTP tunnel one nearly done, but I haven't started on the SOCKS, if you want to try that :)

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