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: Launch a private Tor instance when not already running #15421

Open
wants to merge 4 commits into
base: master
from

Conversation

@luke-jr
Copy link
Member

luke-jr commented Feb 15, 2019

No description provided.

@luke-jr luke-jr force-pushed the luke-jr:tor_subprocess branch from 9ee79a4 to 61e1a11 Feb 15, 2019
@luke-jr

This comment has been minimized.

Copy link
Member Author

luke-jr commented Feb 15, 2019

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

@DrahtBot

This comment has been minimized.

Copy link
Contributor

DrahtBot commented Feb 15, 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:

  • #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 fanquake added the P2P label Feb 15, 2019
@practicalswift

This comment has been minimized.

Copy link
Member

practicalswift commented Feb 16, 2019

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

This comment has been minimized.

Copy link
Member

Sjors commented Feb 16, 2019

@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

This comment has been minimized.

Copy link
Member

practicalswift commented Feb 16, 2019

@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

This comment has been minimized.

Copy link
Member Author

luke-jr commented Feb 16, 2019

@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 luke-jr force-pushed the luke-jr:tor_subprocess branch 2 times, most recently from 429377d to 1977874 Feb 16, 2019
@luke-jr

This comment has been minimized.

Copy link
Member Author

luke-jr commented Feb 16, 2019

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

@practicalswift

This comment has been minimized.

Copy link
Member

practicalswift commented Feb 16, 2019

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

@luke-jr

This comment has been minimized.

Copy link
Member Author

luke-jr commented Feb 17, 2019

@practicalswift I don't see the need - we already connect out on 8333...

@practicalswift

This comment has been minimized.

Copy link
Member

practicalswift commented Feb 17, 2019

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

@luke-jr

This comment has been minimized.

Copy link
Member Author

luke-jr commented Feb 17, 2019

@practicalswift What does it matter?

@Sjors

This comment has been minimized.

Copy link
Member

Sjors commented Feb 19, 2019

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

This comment has been minimized.

Copy link
Member

practicalswift commented Feb 20, 2019

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

@luke-jr luke-jr force-pushed the luke-jr:tor_subprocess branch from 48f4c54 to 5aef051 Feb 15, 2020
@luke-jr

This comment has been minimized.

Copy link
Member Author

luke-jr commented Feb 15, 2020

Rebased

@luke-jr luke-jr force-pushed the luke-jr:tor_subprocess branch from 6b8d283 to 85faf3e Mar 5, 2020
@luke-jr luke-jr force-pushed the luke-jr:tor_subprocess branch from 85faf3e to f2add18 Apr 2, 2020
@DrahtBot DrahtBot removed the Needs rebase label Apr 2, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants
You can’t perform that action at this time.