This PR allows torcontrol to auto configure the HiddenService even when Tor is running on a different host or within a containerized environment.
In such an environment, the previously assumed static target of
127.0.0.1 was insufficient.
fixes #16693
Concept ACK.
You might want to add some validationfor the option value: at the least whether it’s <addr>:port, to prevent passing weird values to Tor’s control port with unclear outcomes.
Edit: this is also the first step towards making #8973 possible.
Edit.2: apparently that uses the unix:/tmp/path/to/tor/socket syntax. We’d also want to support that at some point, though right now is pointless as bitcoind can’t bind on a UNIX socket.
Concept ACK.
Some validation and testing would be nice here. @MDrollette, are you still active on this?
You might want to add some validationfor the option value: at the least whether it’s
<addr>:port, to prevent passing weird values to Tor’s control port with unclear outcomes.
I’ve added what I believe to be some sanitation/validation. I am happy to do something differently if anyone can point to a better example. My apologies, I’m not a C developer, so this is very much an introduction for me. @luke-jr
I’m confused how torcontrol is talking to Tor on another host?
-torcontrol can be a remote host or a different interface on the same host (ie. when running in a container) - particularly when used in combination with -torpassword instead of the cookie file method.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
Reviewers, this pull request conflicts with the following ones:
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.
530@@ -531,12 +531,20 @@ void TorController::auth_cb(TorControlConnection& _conn, const TorControlReply&
531 SetReachable(NET_ONION, true);
532 }
533
534+ // Validate the target of the hidden service
535+ std::string targetArg = gArgs.GetArg("-tortarget", "127.0.0.1");
GetListenPort() is used below on L537
This allows torcontrol to auto configure the HiddenService even
when Tor is running on a different host or within a containerized
environment.
530@@ -531,12 +531,20 @@ void TorController::auth_cb(TorControlConnection& _conn, const TorControlReply&
531 SetReachable(NET_ONION, true);
532 }
533
534+ // Validate the target of the hidden service
Shouldn’t this also be changed, a few lines above:
0 // Now that we know Tor is running setup the proxy for onion addresses
1 // if -onion isn't set to something else.
2 if (gArgs.GetArg("-onion", "") == "") {
3 CService resolved(LookupNumeric("127.0.0.1", 9050));
4 proxyType addrOnion = proxyType(resolved, true);
5 SetProxy(NET_ONION, addrOnion);
6 SetReachable(NET_ONION, true);
7 }
If the tor daemon is running on another machine, then it will not be reachable on 127.0.0.1:9050.
460@@ -461,6 +461,7 @@ void SetupServerArgs(NodeContext& node)
461 argsman.AddArg("-timeout=<n>", strprintf("Specify connection timeout in milliseconds (minimum: 1, default: %d)", DEFAULT_CONNECT_TIMEOUT), ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);
462 argsman.AddArg("-peertimeout=<n>", strprintf("Specify p2p connection timeout in seconds. This option determines the amount of time a peer may be inactive before the connection to it is dropped. (minimum: 1, default: %d)", DEFAULT_PEER_CONNECT_TIMEOUT), ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::CONNECTION);
463 argsman.AddArg("-torcontrol=<ip>:<port>", strprintf("Tor control port to use if onion listening enabled (default: %s)", DEFAULT_TOR_CONTROL), ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);
464+ argsman.AddArg("-tortarget=<ip:port>", strprintf("Target for Tor HiddenService (default: 127.0.0.1:%i)", defaultChainParams->GetDefaultPort()), ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);
port is optional, maybe change to -tortarget=<ip[:port]>? Or -tortarget=<ip>[:<port>] to be in line with the existent -torcontrol=<ip>:<port>.
ADD_ONION command avoiding the need for the -tortarget option suggested in this PR.
@MDrollette, do you think that would be sufficient? I can imagine some exceptional cases where we bind to 1.2.3.4:8334 to listen for incoming tor connections, BUT from the point of view of the tor daemon which runs in another machine, that is reachable as another address, e.g. 5.6.7.8:8334. Maybe that is an overkill and we need not support this. I.e. if we bind to 1.2.3.4:8334 to listen for tor connections, then we simply pass the same 1.2.3.4:8334 to the ADD_ONION command. Agreed?
@MDrollette, do you think that would be sufficient?
That seems like an appropriate default, if nothing else. I think it’s still common in the docker case to simply specify listening on 0.0.0.0 since the IP is randomly assigned when the container is created. In this case I would run the bitcoin container with -tortarget=bitcoind:1234 so that the container IP is resolved at runtime.
It’s possible to make it work with only your PR, so it’s more flexible than the current state. But it’s not as trivial as docker run.
-tortarget=bitcoind:1234? Does it resolve to a different IP address from inside the container and from outside?
🐙 This pull request conflicts with the target branch and needs rebase.
Want to unsubscribe from rebase notifications on this pull request? Just convert this pull request to a “draft”.