Currently, we just assume any running Tor instance provides localhost port 9050 for SOCKS, and configure -onion accordingly when we get a Tor control connection.
This actually queries the Tor node for its SOCKS listeners, and uses the configured port instead.
For backward compatibility, it falls back to localhost:9050 if it can’t get any better port info. I’m not sure if that’s the correct action to take when the Tor daemon explicitly says there are no ports listening…
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
No conflicts as of last run.
482@@ -481,6 +483,53 @@ TorController::~TorController()
483 }
484 }
485
486+void TorController::get_socks_cb(TorControlConnection& _conn, const TorControlReply& reply)
487+{
488+ // NOTE: We can only get here if -onion is unset
482@@ -481,6 +483,53 @@ TorController::~TorController()
483 }
484 }
485
486+void TorController::get_socks_cb(TorControlConnection& _conn, const TorControlReply& reply)
torrc, compared behaviour between 0.18 and this).
511+ LogPrint(BCLog::TOR, "tor: Get SOCKS port command yielded %s\n", socks_port_str);
512+ } else {
513+ LogPrintf("tor: Get SOCKS port command returned nothing\n");
514+ }
515+ } else if (reply.code == 510) { // 510 Unrecognized command
516+ LogPrintf("tor: Get SOCKS port command failed with unrecognized command (You probably should upgrade Tor)\n");
Do you know what is the minimum version of Tor supporting this query? is it newer or older than what we already require? (not that it matters much, as I se it’ll gracefully fall back to the old behavior; but it might be useful for testing)
Tyi: This info call /net/listeners/socks is according to tor doku possible since [ 0.2.2.26-beta.] I tested with versions 0.2.9.16 and 0.4.3.5
Changes in tor version 0.2.2.32 - 2011-08-27
o Minor features (controller interface): - New “GETINFO net/listeners/(type)” controller command to return a list of addresses and ports that are bound for listeners for a given connection type. This is useful when the user has configured “SocksPort auto” and the controller needs to know which port got chosen. Resolves another part of ticket 3076. - Have the controller interface give a more useful message than “Internal Error” in response to failed GETINFO requests.
572@@ -524,10 +573,7 @@ void TorController::auth_cb(TorControlConnection& _conn, const TorControlReply&
573 // Now that we know Tor is running setup the proxy for onion addresses
574 // if -onion isn't set to something else.
575 if (gArgs.GetArg("-onion", "") == "") {
576- CService resolved(LookupNumeric("127.0.0.1", 9050));
577- proxyType addrOnion = proxyType(resolved, true);
578- SetProxy(NET_ONION, addrOnion);
579- SetReachable(NET_ONION, true);
580+ _conn.Command("GETINFO net/listeners/socks", std::bind(&TorController::get_socks_cb, this, std::placeholders::_1, std::placeholders::_2));
Minor Nit it does not account for early user -proxy= settings and would overrule them.
Please consider the litle patch ff That would also fix finally #14722 and make #19358 obsolete.
0diff --git a/src/torcontrol.cpp b/src/torcontrol.cpp
1index 2871f9030..dff873ce9 100644
2--- a/src/torcontrol.cpp
3+++ b/src/torcontrol.cpp
4@@ -573,7 +573,8 @@ void TorController::auth_cb(TorControlConnection& _conn, const TorControlReply&
5
6 // Now that we know Tor is running setup the proxy for onion addresses
7 // if -onion isn't set to something else.
8- if (gArgs.GetArg("-onion", "") == "") {
9+ proxyType addrOnion;
10+ if (gArgs.GetArg("-onion", "") == "" && !GetProxy(NET_ONION, addrOnion)) {
11 _conn.Command("GETINFO net/listeners/socks", std::bind(&TorController::get_socks_cb, this, std::placeholders::_1, std::placeholders::_2));
12 }
13
491+ for (const auto& line : reply.lines) {
492+ if (0 == line.compare(0, 20, "net/listeners/socks=")) {
493+ const std::string port_list_str = line.substr(20);
494+ std::vector<std::string> port_list;
495+ boost::split(port_list, port_list_str, boost::is_any_of(" "));
496+ for (auto& portstr : port_list) {
0 for (const auto& portstr : spanparsing::Split(port_list_str, ' ')) {
0torcontrol.cpp:340:80: error: no member named 'rbegin' in 'Span<const char>'; did you mean 'begin'?
1 if ((portstr[0] == '"' || portstr[0] == '\'') && (*portstr.rbegin() == '"' || *portstr.rbegin() == '\'')) {
2 ^~~~~~
Span has begin(), but no rbegin(). It has back() to lookup the last element, so *portstr.rbegin() can be replaced portstr.back() if you like it better.
493+ const std::string port_list_str = line.substr(20);
494+ std::vector<std::string> port_list;
495+ boost::split(port_list, port_list_str, boost::is_any_of(" "));
496+ for (auto& portstr : port_list) {
497+ if (portstr.empty()) continue;
498+ if ((portstr[0] == '"' || portstr[0] == '\'') && (*portstr.rbegin() == '"' || *portstr.rbegin() == '\'')) {
"123' and '456". I guess it is ok, but is also easy to avoid, thus I mention it.
518+ LogPrintf("tor: Get SOCKS port command failed; error code %d\n", reply.code);
519+ }
520+
521+ if (socks_port_str.empty()) {
522+ // Fallback to old behaviour
523+ socks_port_str = "127.0.0.1";
...port.... variable an address, not a port. Maybe s/socks_port_str/socks_location/.
master from Feb 2019 and does not compile configure: error: openssl not found.. Needs rebase. I would like to compile and test it.
338+ boost::split(port_list, port_list_str, boost::is_any_of(" "));
339+ for (auto& portstr : port_list) {
340+ if (portstr.empty()) continue;
341+ if ((portstr[0] == '"' || portstr[0] == '\'') && (*portstr.rbegin() == portstr[0])) {
342+ portstr = portstr.substr(1, portstr.size() - 2);
343+ if (portstr.empty()) continue;
If portstr contains just " (it is a string with length 1), then this condition will evaluate to true. On the subsequent line portstr.size() - 2 will be -1 converted to size_t (will cause a warning by the integer sanitizer). Then substr(1, -1) will return an empty string and it will work as intended - continue; will be executed.
But anyway, I think it is better to add one more condition to avoid the above: && portstr.size() >= 2
364+ if (socks_location.empty()) {
365+ // Fallback to old behaviour
366+ socks_location = "127.0.0.1";
367+ }
368+
369+ CService resolved(LookupNumeric(socks_location, 9050));
Would be good to check the result of LookupNumeric() in case some unexpected string was returned by the Tor server.
0 CService resolved(LookupNumeric(socks_location, 9050));
1
2 if (!resolved.IsValid()) {
3 // Fallback to old behaviour
4 resolved = LookupNumeric("127.0.0.1", 9050);
5 }
Not confusing with Tor Browser’s default, which is 9150?
Yes this is what I normally follow:
Windows: Running Tor browser and using it as proxy
Linux: Running Tor after installing with command: sudo apt install tor
Running Tor as service on Windows involves some workaround which I am not sure many do or maybe I am not aware of other easier ways to do it: https://superuser.com/questions/1631178/how-to-configure-tor-as-service-on-windows
9050 here serves two purposes:
Here is my torcfg settings
0## Tor opens a SOCKS proxy on port 9050 by default -- even if you don't
1## configure one below. Set "SOCKSPort 0" if you plan to run Tor only
2## as a relay, and not make any local application connections yourself.
3SOCKSPort 9050 # Default: Bind to localhost:9050 for local connections.
4SOCKSPort 192.168.1.2:9050 # Bind to this address:port too.
If only the port is provided, it is binded implicitly to 127.0.0.1. I need to test this patch to see the actual output but if what is printed by nyx is what is returned as-is by torcontrol, the config above would return:
9050, 192.168.1.2:9050
Your function wants to favor 127.0.0.1 but I think that it could miss the standalone port value scenario…
I have few more thoughts to share to possibly improve further the code of this function. Here is my setup:
I have 1 tor proxy configured on a single machine in my LAN. It is on 192.168.1.2. This allows me to share a single tor proxy instance among all the trusted machines inside my LAN.
Bitcoind is running on a different machine: 192.168.1.179.
My first attempt to make torcontrol available for bitcoind was to bind the control listening socket to 192.168.1.2:9051. It works except that tor is complaining big time that this is opening a security risk as the control protocol exchange is in clear text over the network.
To fix that, I did create a SSH tunnel between the 2 machines 127.0.0.1:9051.
What that means is that when I start bitcoind from 192.168.1.179 machine with the cmdline: -proxy 192.168.1.2:9050 -torcontrol 127.0.0.1:9051
When bitcoind connects with torcontrol, despite it is a localhost address, it does actually connect to a remote server.
What all this means for this commit?
Well, it favors the tor proxy address 127.0.0.1. With my setup, it will fail.
The first solution to address this issue that came to my mind is to test that 127.0.0.1:9050 is listening but this is clumsy, there is no simple and portable way to check that beside actually connect a socket to the address…
Then, I did ask myself the following question: What are exactly the benefits of connecting to the localhost proxy address if present?
I am not sure if there is any.
OTOH, if you connect to the first public IP address listed in the reply, it will work with the most common case where the proxy is on the same machine and it will also work on more funky setups like mine…
How do you feel toward this suggestion? Does that makes sense to you?
I tested with
0SOCKSPort 10000
1SOCKSPort 192.168.0.1:20000
2SOCKSPort 0.0.0.0:30000
The reply from the Tor control daemon is:
0net/listeners/socks="127.0.0.1:10000" "192.168.0.1:20000" "0.0.0.0:30000"
so it does not return just port. The spec is here: https://github.com/torproject/torspec/blob/845a4d7213e8e28bb039d6925437b5b1c0d607e7/control-spec.txt#L912.
Maybe it makes sense to choose the non-127.0.0.1 address if there are a few:
127.0.0.1 address will work.But anyway - we can’t expect the automatic configuration to detect and work in all cases. For this we have the manual override. @lano1106, in your case you can set -onion=192.168.1.2:9050 which will work with master and with this PR.
Vasil, you have been faster than me… I just tested too this morning:
2021-09-02T13:51:22Z tor: reply line: net/listeners/socks="127.0.0.1:9050" "192.168.1.2:9050"
So overall this pull request works well. I just think non localhost address should be prefered over 127.0.0.1. This would work with my setup and I don’t see any extra benefits of picking 127.0.0.1 in any scenario.
But anyway - we can’t expect the automatic configuration to detect and work in all cases. For this we have the manual override. @lano1106, in your case you can set
-onion=192.168.1.2:9050which will work withmasterand with this PR.
you are correct but see my other suggestion below. I did configure the onion proxy with the following options: -proxy=192.168.1.2:9050 -torcontrol=127.0.0.1:9151
I think that the condition:
gArgs.GetArg("-onion", "") == ""
should be changed with:
!GetProxy(NET_ONION, addrOnion)
because -onion switch is not the only way to configure the onion proxy. This is the root cause of my problem and why I got interested in this issue in the first place…
So overall this pull request works well. I just think non localhost address should be prefered over 127.0.0.1. This would work with my setup and I don’t see any extra benefits of picking 127.0.0.1 in any scenario.
The non-localhost address may change or cease to be available?
because -onion switch is not the only way to configure the onion proxy.
But it’s the only way that should override -torcontrol IMO
@luke-jr if the non-localhost address may change or cease to be available is a serious and valid concern for not modifying the code behavior then so be it.
I have said everything that I have to say on the topic and I’ll conclude by saying that without being aware of the initial problem that motivated this pull request, with the reasonable assumption that the vast majority of tor proxy setups will list the localhost address, this pull request, if left as-is, will just be a shinier, more complex method to essentially reach the exact same result than what the current code is doing…
415@@ -369,10 +416,7 @@ void TorController::auth_cb(TorControlConnection& _conn, const TorControlReply&
416 // Now that we know Tor is running setup the proxy for onion addresses
417 // if -onion isn't set to something else.
418 if (gArgs.GetArg("-onion", "") == "") {
This line did cause me a problem and this is why I created #22830
In a nutshell, you could have the onion proxy set with the following cmdline: -proxy=192.168.1.2:9050 -torcontrol=127.0.0.1:9050
I believe that check the presence of the -onion switch is not the right check because the check as-is overwrite the user settings.
The code should instead call GetProxy() and only enter the conditional block if function returns false because the onion proxy is not currently valid.
utACK https://github.com/bitcoin/bitcoin/pull/15423/commits/4314a216e319b70ab0b47969eae4d8ad86262f37
nit: #15423 (review)
I never used 9050 port on Windows and it was 9150 with browser. Not sure how many Bitcoin Core users run this software on Windows but Windows is still used for most of the desktop PCs and easiest process is to use 9150 which would work as default.
52@@ -53,6 +53,7 @@ static const float RECONNECT_TIMEOUT_EXP = 1.5;
53 * this is belt-and-suspenders sanity limit to prevent memory exhaustion.
54 */
55 static const int MAX_LINE_LENGTH = 100000;
56+static const uint16_t DEFAULT_TOR_SOCKS_PORT = 9050;
s/const/constexpr/ if you retouch
as a follow-up, maybe use this constant in src/init.cpp
there is also
0src/qt/optionsmodel.h:20:static constexpr uint16_t DEFAULT_GUI_PROXY_PORT = 9050;
ACK b2774fc0bed53dfaf98206d353d42c474c5bbb1a review, rebased to master, debug build, ran unit tests, tested happy path only
02022-03-16T21:31:10Z [torcontrol] tor: Authentication successful
12022-03-16T21:31:10Z [torcontrol] tor: Get SOCKS port command yielded 127.0.0.1:9050
22022-03-16T21:31:10Z [torcontrol] tor: Configuring onion proxy for 127.0.0.1:9050
32022-03-16T21:31:10Z [torcontrol] tor: ADD_ONION successful
I never used 9050 port on Windows and it was 9150 with browser.
This has nothing to do with Windows. Tor Browser’s internal Tor uses port 9150 on every OS. This is an implementation detail and it is frowned upon to use it in third-party applications (it controls circuits tightly, for example, so anything interfering with it might accidentally affect privacy). The normal Tor version for windows uses the same default port, 9050.
Tested ACK (FreeBSD) b2774fc0bed53dfaf98206d353d42c474c5bbb1a
I changed SocksPort temporary and restarted bitcoind to see if it’d pick it up. And it did.
tor:
0Mar 22 11:45:33.942 [notice] Opening Socks listener on 127.0.0.1:9066
1Mar 22 11:45:33.943 [notice] Opened Socks listener connection (ready) on 127.0.0.1:9066
bitcoind:
02022-03-22T10:46:20Z tor: Get SOCKS port command yielded 127.0.0.1:9066
12022-03-22T10:46:20Z tor: Configuring onion proxy for 127.0.0.1:9066