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

torcontrol: Query Tor for correct -onion configuration #15423

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

@luke-jr
Copy link
Member

@luke-jr luke-jr commented Feb 16, 2019

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...

@DrahtBot
Copy link
Contributor

@DrahtBot DrahtBot commented Feb 16, 2019

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #22651 (tor: respect non-onion -onlynet= for outgoing Tor connections by vasild)
  • #19358 (torcontrol : avoid to set wrong outbound proxy and network settings when creating an inbound onion service. by Saibato)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

@fanquake fanquake added the P2P label Feb 16, 2019
@kristapsk
Copy link
Contributor

@kristapsk kristapsk commented May 14, 2019

Agree with @practicalswift comments, apart from that tACK 962f168 (enabled/disabled 9050 and other ports in torrc, compared behaviour between 0.18 and this).

@laanwj
Copy link
Member

@laanwj laanwj commented Jun 5, 2019

Concept ACK, thanks, this makes sense.

src/torcontrol.cpp Show resolved Hide resolved
@kristapsk
Copy link
Contributor

@kristapsk kristapsk commented Nov 13, 2019

This came up in a twitter discussion with @luke-jr yesterday. What's blocker here? Or just forgot in a long list of unmerged PRs?

@Empact
Copy link
Member

@Empact Empact commented Jan 6, 2020

Concept ACK

@DrahtBot
Copy link
Contributor

@DrahtBot DrahtBot commented Mar 9, 2020

The last travis run for this pull request was 387 days ago and is thus outdated. To trigger a fresh travis build, this pull request should be closed and re-opened.

src/torcontrol.cpp Show resolved Hide resolved
Copy link
Contributor

@vasild vasild left a comment

Light code review ok. This is based on master from Feb 2019 and does not compile configure: error: openssl not found.. Needs rebase. I would like to compile and test it.

const std::string port_list_str = line.substr(20);
std::vector<std::string> port_list;
boost::split(port_list, port_list_str, boost::is_any_of(" "));
for (auto& portstr : port_list) {
Comment on lines +493 to +339

This comment has been minimized.

@vasild

vasild Mar 11, 2021
Contributor

Suggested change
const std::string port_list_str = line.substr(20);
std::vector<std::string> port_list;
boost::split(port_list, port_list_str, boost::is_any_of(" "));
for (auto& portstr : port_list) {
for (const auto& portstr : spanparsing::Split(port_list_str, ' ')) {

This comment has been minimized.

@luke-jr

luke-jr Jul 8, 2021
Author Member

torcontrol.cpp:340:80: error: no member named 'rbegin' in 'Span<const char>'; did you mean 'begin'?
                    if ((portstr[0] == '"' || portstr[0] == '\'') && (*portstr.rbegin() == '"' || *portstr.rbegin() == '\'')) {
                                                                               ^~~~~~

This comment has been minimized.

@vasild

vasild Jul 9, 2021
Contributor

Ah, 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.

src/torcontrol.cpp Outdated Show resolved Hide resolved
src/torcontrol.cpp Outdated Show resolved Hide resolved
@luke-jr luke-jr force-pushed the luke-jr:tor_socks_port branch from 962f168 to d37d95a Jul 8, 2021
@luke-jr
Copy link
Member Author

@luke-jr luke-jr commented Jul 8, 2021

Rebased, nits addressed

if ((portstr[0] == '"' || portstr[0] == '\'') && (*portstr.rbegin() == portstr[0])) {
portstr = portstr.substr(1, portstr.size() - 2);
if (portstr.empty()) continue;
Comment on lines +341 to +343

This comment has been minimized.

@vasild

vasild Jul 27, 2021
Contributor

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

if (socks_location.empty()) {
// Fallback to old behaviour
socks_location = "127.0.0.1";
}

CService resolved(LookupNumeric(socks_location, 9050));
Comment on lines +364 to +369

This comment has been minimized.

@vasild

vasild Jul 27, 2021
Contributor

Would be good to check the result of LookupNumeric() in case some unexpected string was returned by the Tor server.

Suggested change
if (socks_location.empty()) {
// Fallback to old behaviour
socks_location = "127.0.0.1";
}
CService resolved(LookupNumeric(socks_location, 9050));
CService resolved(LookupNumeric(socks_location, 9050));
if (!resolved.IsValid()) {
// Fallback to old behaviour
resolved = LookupNumeric("127.0.0.1", 9050);
}
socks_location = "127.0.0.1";
}

CService resolved(LookupNumeric(socks_location, 9050));

This comment has been minimized.

@prayank23

prayank23 Aug 7, 2021
Contributor

Port 9150 for Windows: #22651 (comment)

This comment has been minimized.

@kristapsk

kristapsk Aug 30, 2021
Contributor

Is it really Windows default? Not confusing with Tor Browser's default, which is 9150?

This comment has been minimized.

@prayank23

prayank23 Aug 30, 2021
Contributor

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

This comment has been minimized.

@luke-jr

luke-jr Aug 30, 2021
Author Member

9050 here serves two purposes:

  1. Fallback to old behaviour. This was always 9050.
  2. Default port if Tor's API returns just an IP. Is there any reason to think it would ever be anything other than 9050?

This comment has been minimized.

@lano1106

lano1106 Aug 31, 2021
Contributor

Here is my torcfg settings

## Tor opens a SOCKS proxy on port 9050 by default -- even if you don't
## configure one below. Set "SOCKSPort 0" if you plan to run Tor only
## as a relay, and not make any local application connections yourself.
SOCKSPort 9050 # Default: Bind to localhost:9050 for local connections.
SOCKSPort 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...

This comment has been minimized.

@vasild

vasild Sep 2, 2021
Contributor

I tested with

SOCKSPort 10000
SOCKSPort 192.168.0.1:20000
SOCKSPort 0.0.0.0:30000

The reply from the Tor control daemon is:

net/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:

  1. If the Tor daemon is on the same machine as bitcoind, then either one will work and communication will be local anyway
  2. If the Tor daemon is on another machine, only the non-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.

This comment has been minimized.

@lano1106

lano1106 Sep 2, 2021
Contributor

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.

This comment has been minimized.

@lano1106

lano1106 Sep 2, 2021
Contributor

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.

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...

This comment has been minimized.

@luke-jr

luke-jr Sep 2, 2021
Author Member

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

This comment has been minimized.

@lano1106

lano1106 Sep 3, 2021
Contributor

@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...

@vasild
Copy link
Contributor

@vasild vasild commented Aug 30, 2021

@luke-jr, are you planning to update this PR?

@@ -369,10 +416,7 @@ void TorController::auth_cb(TorControlConnection& _conn, const TorControlReply&
// Now that we know Tor is running setup the proxy for onion addresses
// if -onion isn't set to something else.
if (gArgs.GetArg("-onion", "") == "") {

This comment has been minimized.

@lano1106

lano1106 Sep 1, 2021
Contributor

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet