-
-
Notifications
You must be signed in to change notification settings - Fork 3.2k
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 Socks v5 support to daemon and wallet #9443
base: master
Are you sure you want to change the base?
Conversation
Crap, looks like older versions of boost don't have some ASIO functionality I used. Crap, will have to update. |
What would be the minimum Boost version for this PR? Is it time to undust #9162 and bump the minimum Boost requirement? |
The minimum version for this feature would be 1.67. In this case, its not really critical, I should be able to work-around it. |
After many years, IPv6 has started to see substantial, and increasing, adoption, so I think this would be a reasonable addition. However, it's not urgent, and, based on the comments in #9162, I think you could safely assume Boost 1.67 (Ubuntu 18.04 continues to wither) and roll out this feature after a boost version upgrade to 1.67. Of course, you can do the extra work to support the current Boost version, but you may very well have better uses for your time. |
I just pushed a change to fix the Boost issues. Everything looks good for that. The functional tests failed, but it looks spurious right now. Testing locally to confirm, and may just push a dummy update to re-run the tests. |
Good for review! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adds SOCKSv5 support with tests and documentation included. Tested working. The code looks relatively minimal such that I don't see anything to remove, really. I don't see any potential privacy or security concerns, LGTM
Closes #9443 |
Closes #9390 ? |
This PR needs rebased, and hopefully moves forward. Will be nice to support IPv6. |
64a613e
to
cef3877
Compare
Force Push:
|
cef3877
to
ba2f8a4
Compare
Force pushed a size check fix. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested, no issues.
@vtnerd would it make sense to revert this change, since boost on master and release has been updated to 1.67+? |
Yes, looks like this is no longer needed, so I will try to revert that change. |
Force pushed a change to revert the workaround for older Boost versions. I'm not sure if this is a good idea, as officially the last supported version is Boost 1.66 @selsta , what do you think? |
Nevermind, I just checked again and this code should work with Boost 1.66+ so everything should be good. |
Also, i think we're raising min to 1.69 |
@nahuhh we will use 1.69 for release binaries but ideally work around the boost bug so we can keep a lower minimum |
I just verified this still compiles with Boost 1.66-1.87, so no issues with that. |
Socks v5 adds IPv6 support and basic username/password authentication. The user/pass authentication is a cheap way to prevent rogue applications from requesting proxied connections. Tor and SSH do not support authentication but I2P, Nym, and Dante do.
When I started this implementation, I didn't know Nym added Socks v4 support. This makes the patch somewhat less useful, as the original #8562 Socks v5 request was for Nym support. This patch might want to be rejected on that reason alone - the only useful features are user/pass and IPv6.
This includes a fairly extensive explanation of proxies in the daemon and cli wallets, so that might be useful for keeping regardless.