torcontrol: add -tortarget config #19043

MDrollette · 2020-05-21T22:13:05Z

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

laanwj · 2020-05-26T10:58:54Z

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.

luke-jr · 2020-06-03T03:56:53Z

I'm confused how torcontrol is talking to Tor on another host?

jonatack · 2020-07-17T13:26:06Z

Concept ACK.

Some validation and testing would be nice here.

@MDrollette, are you still active on this?

MDrollette · 2020-07-17T18:52:22Z

@laanwj

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.

DrahtBot · 2020-07-17T22:25:27Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

net: Add CNode::ConnectedThroughNetwork member function #19998 (net: Add CNode::ConnectedThroughNetwork member function by hebasto)
net: Use alternative port for incoming Tor connections #19991 (net: Use alternative port for incoming Tor connections by hebasto)

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.

darosior

Concept ACK

darosior · 2020-07-30T12:43:03Z

src/torcontrol.cpp

@@ -531,12 +531,20 @@ void TorController::auth_cb(TorControlConnection& _conn, const TorControlReply&
            SetReachable(NET_ONION, true);
        }

+        // Validate the target of the hidden service
+        std::string targetArg = gArgs.GetArg("-tortarget", "127.0.0.1");


Looks like the port is missing in the default value ?

If no port is specified, either on the passed in value or in this case as the default, then 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.

vasild · 2020-09-23T11:13:48Z

src/torcontrol.cpp

@@ -531,12 +531,20 @@ void TorController::auth_cb(TorControlConnection& _conn, const TorControlReply&
            SetReachable(NET_ONION, true);
        }

+        // Validate the target of the hidden service


Shouldn't this also be changed, a few lines above:

// 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", "") == "") { CService resolved(LookupNumeric("127.0.0.1", 9050)); proxyType addrOnion = proxyType(resolved, true); SetProxy(NET_ONION, addrOnion); SetReachable(NET_ONION, true); }

If the tor daemon is running on another machine, then it will not be reachable on 127.0.0.1:9050.

vasild · 2020-09-23T11:16:36Z

src/init.cpp

@@ -461,6 +461,7 @@ void SetupServerArgs(NodeContext& node)
    argsman.AddArg("-timeout=<n>", strprintf("Specify connection timeout in milliseconds (minimum: 1, default: %d)", DEFAULT_CONNECT_TIMEOUT), ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);
    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);
    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);
+    argsman.AddArg("-tortarget=<ip:port>", strprintf("Target for Tor HiddenService (default: 127.0.0.1:%i)", defaultChainParams->GetDefaultPort()), ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);


Because port is optional, maybe change to -tortarget=<ip[:port]>? Or -tortarget=<ip>[:<port>] to be in line with the existent -torcontrol=<ip>:<port>.

vasild · 2020-09-23T11:50:18Z

In #19991 we are considering adding the possibility to bind to a dedicated addr:port for incoming tor connections, mainly to be able to distinguish them from incoming clearnet ones. If we do that, then we can automatically pass that addr:port to the 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 · 2020-09-23T20:10:23Z

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

vasild · 2020-09-29T14:14:55Z

What is "bitcoind" in -tortarget=bitcoind:1234? Does it resolve to a different IP address from inside the container and from outside?

96571b3 doc: Update onion service target port numbers in tor.md (Hennadii Stepanov) bb145c9 net: Extend -bind config option with optional network type (Hennadii Stepanov) 92bd3c1 net, refactor: Move AddLocal call one level up (Hennadii Stepanov) 57f17e5 net: Pass onion service target to Tor controller (Hennadii Stepanov) e3f0785 refactor: Rename TorController::target to m_tor_control_center (Hennadii Stepanov) fdd3ae4 net, refactor: Refactor CBaseChainParams::RPCPort function (Hennadii Stepanov) a5266d4 net: Add alternative port for onion service (Hennadii Stepanov) b3273cf net: Use network byte order for in_addr.s_addr (Hennadii Stepanov) Pull request description: This PR adds ability to label incoming Tor connections as different from normal localhost connections. Closes #8973. Closes #16693. Default onion service target ports are: - 8334 on mainnnet - 18334 on testnet - 38334 on signet - 18445 on regtest To set the onion service target socket manually the extended `-bind` config option could be used: ``` $ src/bitcoind -help | grep -A 6 -e '-bind' -bind=<addr>[:<port>][=onion] Bind to given address and always listen on it (default: 0.0.0.0). Use [host]:port notation for IPv6. Append =onion to tag any incoming connections to that address and port as incoming Tor connections (default: 127.0.0.1:8334=onion, testnet: 127.0.0.1:18334=onion, signet: 127.0.0.1:38334=onion, regtest: 127.0.0.1:18445=onion) ``` Since [pr19991.02 update](#19991 (comment)) this PR is an alternative to #19043. ACKs for top commit: Sjors: re-utACK 96571b3 vasild: ACK 96571b3 laanwj: Re-ACK 96571b3 Tree-SHA512: cb0eade80f4b3395f405f775e1b89c086a1f09d5a4464df6cb4faf808d9c2245474e1720b2b538f203f6c1996507f69b09f5a6e35ea42633c10e22bd733d4438

DrahtBot · 2020-10-02T13:41:17Z

🐙 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".}

vasild · 2020-10-02T14:35:56Z

I think this is superseded now by #19991 which got merged.

MDrollette · 2020-10-02T15:27:30Z

Closing this PR since #19991 addresses this issue and also has further discussion

…ctions 96571b3 doc: Update onion service target port numbers in tor.md (Hennadii Stepanov) bb145c9 net: Extend -bind config option with optional network type (Hennadii Stepanov) 92bd3c1 net, refactor: Move AddLocal call one level up (Hennadii Stepanov) 57f17e5 net: Pass onion service target to Tor controller (Hennadii Stepanov) e3f0785 refactor: Rename TorController::target to m_tor_control_center (Hennadii Stepanov) fdd3ae4 net, refactor: Refactor CBaseChainParams::RPCPort function (Hennadii Stepanov) a5266d4 net: Add alternative port for onion service (Hennadii Stepanov) b3273cf net: Use network byte order for in_addr.s_addr (Hennadii Stepanov) Pull request description: This PR adds ability to label incoming Tor connections as different from normal localhost connections. Closes bitcoin#8973. Closes bitcoin#16693. Default onion service target ports are: - 8334 on mainnnet - 18334 on testnet - 38334 on signet - 18445 on regtest To set the onion service target socket manually the extended `-bind` config option could be used: ``` $ src/bitcoind -help | grep -A 6 -e '-bind' -bind=<addr>[:<port>][=onion] Bind to given address and always listen on it (default: 0.0.0.0). Use [host]:port notation for IPv6. Append =onion to tag any incoming connections to that address and port as incoming Tor connections (default: 127.0.0.1:8334=onion, testnet: 127.0.0.1:18334=onion, signet: 127.0.0.1:38334=onion, regtest: 127.0.0.1:18445=onion) ``` Since [pr19991.02 update](bitcoin#19991 (comment)) this PR is an alternative to bitcoin#19043. ACKs for top commit: Sjors: re-utACK 96571b3 vasild: ACK 96571b3 laanwj: Re-ACK 96571b3 Tree-SHA512: cb0eade80f4b3395f405f775e1b89c086a1f09d5a4464df6cb4faf808d9c2245474e1720b2b538f203f6c1996507f69b09f5a6e35ea42633c10e22bd733d4438

DrahtBot added the P2P label May 21, 2020

fanquake requested a review from laanwj May 21, 2020 23:25

DrahtBot mentioned this pull request Jul 17, 2020

torcontrol: Create also a V3 ed25519-V3 onion address. #19485

Closed

DrahtBot mentioned this pull request Jul 21, 2020

refactor: Pass ArgsManager into functions that register args #19561

Merged

Saibato mentioned this pull request Jul 24, 2020

torcontrol : avoid to set wrong outbound proxy and network settings when creating an inbound onion service. #19358

Closed

kristapsk mentioned this pull request Jul 29, 2020

.onion address doesn't work when bitcoin is trying to call TOR from another host #19618

Closed

nolim1t mentioned this pull request Jul 29, 2020

Improvements to tor config getumbrel/umbrel#58

Closed

3 tasks

darosior reviewed Jul 30, 2020

View reviewed changes

DrahtBot added the Needs rebase label Jul 30, 2020

MDrollette force-pushed the tor-hidden-target branch from 7e57666 to 3fde32c Compare July 30, 2020 16:43

torcontrol: add -tortarget config

b5d5d23

This allows torcontrol to auto configure the HiddenService even when Tor is running on a different host or within a containerized environment.

MDrollette force-pushed the tor-hidden-target branch from 3fde32c to b5d5d23 Compare July 30, 2020 16:45

DrahtBot removed the Needs rebase label Jul 30, 2020

DrahtBot mentioned this pull request Aug 2, 2020

Add -netinfo peer connections dashboard #19643

Merged

This was referenced Sep 22, 2020

net: Use alternative port for incoming Tor connections #19991

Merged

net: Add CNode::ConnectedThroughNetwork member function #19998

Merged

vasild reviewed Sep 23, 2020

View reviewed changes

DrahtBot mentioned this pull request Sep 24, 2020

net, rpc, cli: expose peer network in getpeerinfo; simplify/improve -netinfo #20002

Merged

DrahtBot added the Needs rebase label Oct 2, 2020

MDrollette closed this Oct 2, 2020

bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022

torcontrol: add -tortarget config #19043

torcontrol: add -tortarget config #19043

Uh oh!

Conversation

MDrollette commented May 21, 2020

Uh oh!

laanwj commented May 26, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

luke-jr commented Jun 3, 2020

Uh oh!

jonatack commented Jul 17, 2020

Uh oh!

MDrollette commented Jul 17, 2020

Uh oh!

DrahtBot commented Jul 17, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

darosior left a comment

Choose a reason for hiding this comment

Uh oh!

darosior Jul 30, 2020

Choose a reason for hiding this comment

Uh oh!

MDrollette Jul 30, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vasild Sep 23, 2020

Choose a reason for hiding this comment

Uh oh!

vasild Sep 23, 2020

Choose a reason for hiding this comment

Uh oh!

vasild commented Sep 23, 2020

Uh oh!

MDrollette commented Sep 23, 2020

Uh oh!

vasild commented Sep 29, 2020

Uh oh!

DrahtBot commented Oct 2, 2020

Uh oh!

vasild commented Oct 2, 2020

Uh oh!

MDrollette commented Oct 2, 2020

Uh oh!

Uh oh!

laanwj commented May 26, 2020 •

edited

Loading

DrahtBot commented Jul 17, 2020 •

edited

Loading

MDrollette Jul 30, 2020 •

edited

Loading