torcontrol: Launch a private Tor instance when not already running

luke-jr commented at 9:18 pm on February 15, 2019: member

luke-jr force-pushed on Feb 15, 2019

luke-jr commented at 9:34 pm on February 15, 2019: member

NOTE: This uses boost::process, but currently doesn’t have anything to check that it’s actually available.

DrahtBot commented at 10:53 pm on February 15, 2019: member

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#19561 (refactor: Pass ArgsManager into functions that register args by S3RK)
#19485 (torcontrol: Create also a V3 ed25519-V3 onion address. by Saibato)
#19358 (net: Make sure we do not override proxy settings in hidden service. by Saibato)
#19288 (tests: Add fuzzing harness for TorController by practicalswift)
#15423 (torcontrol: Query Tor for correct -onion configuration by luke-jr)
#14729 (correct -onion default to -proxy behavior by qubenix)

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 added the label P2P on Feb 15, 2019

practicalswift commented at 7:45 am on February 16, 2019: contributor

This should be opt-in and not enabled by default, right?

Starting tor without the end-user asking for it explicitly is highly surprising. Also it could cause potentially unwanted outbound TCP connections to be made.

It is bad usability (and perhaps even a breach of trust) to take actions on behalf of the end-user that the end-user cannot reasonably expect.

With that said: if done in a 100% opt-in fashion this change seems fine conceptually.

Technical feedback:

Please avoid introducing the boost::process dependency.
Needs a test: Could use a mocked tor command.

Sjors commented at 8:27 am on February 16, 2019: member

@luke-jr #15382 in its current incarnation uses boost::process as well, and includes some simple detection code (it just checks if the version is >=1.64). It also has a functional test with a mock executable Python script, so that might be useful here too. @practicalswift I agree we shouldn’t be launching arbitrary binaries named “tor” without the user opting into that. I’m not sure how this PR changes existing behavior. For modern versions of Tor we automatically create a hidden service if the user is running Tor: “if Tor is running (and proper authentication has been configured), Bitcoin Core automatically creates a hidden service to listen on”, but that’s quite different from starting Tor.

practicalswift commented at 8:50 am on February 16, 2019: contributor

@Sjors Yes, that is very different: if Tor is already running and proper authentication has been configured then the user has already opted in to using Tor.

luke-jr commented at 2:12 pm on February 16, 2019: member

@practicalswift

This should be opt-in and not enabled by default, right?

Starting tor without the end-user asking for it explicitly is highly surprising. Also it could cause potentially unwanted outbound TCP connections to be made.

We already make outbound TCP connections by default. Note that this does not operate Tor as an exit node, nor does it even use it for outbound connections - it is exclusively for an inbound hidden service only.

The end goal, is to display the hidden service address in a QR code for easy pairing with mobile wallets.

Please avoid introducing the boost::process dependency.

Not practical. We’re going to need it either way from the sound of it.

luke-jr force-pushed on Feb 16, 2019

luke-jr commented at 4:45 pm on February 16, 2019: member

configure now checks for boost::process availability, and -onion is not configured for private Tor instances (which don’t offer the SOCKS service).

practicalswift commented at 11:21 pm on February 16, 2019: contributor

Could add FascistFirewall 1 to the config to make sure only port 80 and 443 are used?

luke-jr commented at 1:57 am on February 17, 2019: member

@practicalswift I don’t see the need - we already connect out on 8333…

practicalswift commented at 8:06 am on February 17, 2019: contributor

@luke-jr Sorry for being unclear. I was referring to the Tor network traffic :-)

luke-jr commented at 10:11 pm on February 17, 2019: member

@practicalswift What does it matter?

Sjors commented at 8:36 am on February 19, 2019: member

Note that this does not operate Tor as an exit node, nor does it even use it for outbound connections - it is exclusively for an inbound hidden service only.

That’s still detectable for network observers afaik. Let’s just add a tor= setting.

It also seems unsafe to call a random executable in the users path called tor, so we could ask for a full path, but that might be too annoying UX. The tor= setting could help in that regard too.

practicalswift commented at 7:38 pm on February 20, 2019: contributor

@luke-jr I don’t hold any strong opinion but my reasoning was the following: if we’re adding a default configuration then we might as well doing it using port settings which maximise the likelihood of it working out of the box.

DrahtBot added the label Needs rebase on Aug 2, 2019

luke-jr force-pushed on Feb 15, 2020

luke-jr commented at 10:38 pm on February 15, 2020: member

Rebased

DrahtBot removed the label Needs rebase on Feb 15, 2020

luke-jr force-pushed on Feb 16, 2020

luke-jr force-pushed on Mar 5, 2020

DrahtBot added the label Needs rebase on Mar 10, 2020

configure: Clone ax_boost_chrono to ax_boost_process ef98b458eb

torcontrol: Launch a private Tor instance when not already running 3681d4fc59

net: Allow AddLocal of Tor addresses even if we cannot reach Tor outbound 69bd8314fb

build: Update Boost::Process check to latest from autoconf-archive f2add18248

luke-jr force-pushed on Apr 2, 2020

DrahtBot removed the label Needs rebase on Apr 2, 2020

DrahtBot commented at 4:27 pm on July 30, 2020: member

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

DrahtBot added the label Needs rebase on Jul 30, 2020

luke-jr commented at 5:52 pm on August 20, 2020: member

Closing due to lack of agreement/reviews…

luke-jr closed this on Aug 20, 2020

DrahtBot locked this on Feb 15, 2022

torcontrol: Launch a private Tor instance when not already running #15421

Conflicts