correct -onion default to -proxy behavior

qubenix commented at 4:06 am on November 15, 2018: contributor

fixes #14722
only use default tor ip:port (127.0.0.1:9050) if -onion and -proxy not set

I’m not sure if this is done correctly, but it does fix the behavior in my tests. I will not be offended if this gets rejected for a more appropriate method.

fanquake added the label P2P on Nov 15, 2018

qubenix force-pushed on Nov 15, 2018

qubenix commented at 4:59 pm on November 15, 2018: contributor

Pushed a fix to the comment to reflect the new behavior and rebased.

correct -onion default to -proxy behavior

- fixes #14722
- only use default tor ip:port (127.0.0.1:9050) if -onion and -proxy not set
- fix comment to reflect new behavior

38640a021a

in src/torcontrol.cpp:527 in 3474736b8c outdated

521@@ -522,9 +522,9 @@ void TorController::auth_cb(TorControlConnection& _conn, const TorControlReply&
522     if (reply.code == 250) {
523         LogPrint(BCLog::TOR, "tor: Authentication successful\n");
524 
525-        // Now that we know Tor is running setup the proxy for onion addresses
526-        // if -onion isn't set to something else.
527-        if (gArgs.GetArg("-onion", "") == "") {
528+        // Now that we know Tor is running, setup the proxy for onion addresses
529+        // if -onion and -proxy isn't set to something else.
530+        if (gArgs.GetArg("-onion", "-proxy") == "") {

MarcoFalke commented at 5:03 pm on November 15, 2018:

The default must be set to a value, not the name of the arg it should default to.

qubenix commented at 8:35 pm on November 15, 2018:

I think I might have fixed it now.

qubenix commented at 9:42 pm on November 16, 2018:

@MarcoFalke I noticed in another spots this was handled differently. Would it be more preferable like this:

0        // Now that we know Tor is running setup the proxy for onion addresses
1        // if -onion isn't set to something else.
2        std::string onion = gArgs.GetArg("-onion", "");
3        std::string proxy = gArgs.GetArg("-proxy", "");
4        if (onion.empty() && proxy.empty()) {

qubenix force-pushed on Nov 15, 2018

qubenix commented at 8:30 pm on November 15, 2018: contributor

Thank you for reviewing.

I think I may have done this the correct way now. If not, I will stop wasting your time and close this so someone more suited can take it up.

https://github.com/bitcoin/bitcoin/blob/38640a021a67e376c32414d457f5f850ebbee083/src/torcontrol.cpp#L525-L527

in src/torcontrol.cpp:527 in 38640a021a

521@@ -522,9 +522,9 @@ void TorController::auth_cb(TorControlConnection& _conn, const TorControlReply&
522     if (reply.code == 250) {
523         LogPrint(BCLog::TOR, "tor: Authentication successful\n");
524 
525-        // Now that we know Tor is running setup the proxy for onion addresses
526-        // if -onion isn't set to something else.
527-        if (gArgs.GetArg("-onion", "") == "") {
528+        // Now that we know Tor is running, setup the proxy for onion addresses
529+        // if -onion and -proxy isn't set to something else.
530+        if (gArgs.GetArg("-onion", "") == "" && gArgs.GetArg("-proxy", "") == "") {

laanwj commented at 7:56 am on November 23, 2018:

I guess this will work in most cases.

Though ideally we’d query Net’s state directly here for “is there a proxy for onion already” instead of indirectly divising it from the arguments. This would really solve the problem of the current code here.

Saibato commented at 2:16 pm on June 26, 2020:

I guess this will work in most cases.

Though ideally we’d query Net’s state directly here for “is there a proxy for onion already” instead of indirectly divising it from the arguments. This would really solve the problem of the current code here.

Hi, @qubenix

 0
 1diff --git a/src/torcontrol.cpp b/src/torcontrol.cpp
 2index bec64fbf2..b632513c3 100644
 3--- a/src/torcontrol.cpp
 4+++ b/src/torcontrol.cpp
 5@@ -524,7 +524,8 @@ void TorController::auth_cb(TorControlConnection& _conn, const TorControlReply&
 6 
 7         // Now that we know Tor is running, setup the proxy for onion addresses
 8         // if -onion and -proxy isn't set to something else.
 9-        if (gArgs.GetArg("-onion", "") == "" && gArgs.GetArg("-proxy", "") == "") {
10+        proxyType addrOnion;
11+        if (gArgs.GetArg("-onion", "") == "" && !GetProxy(NET_ONION, addrOnion)) {
12             CService resolved(LookupNumeric("127.0.0.1", 9050));
13             proxyType addrOnion = proxyType(resolved, true);
14             SetProxy(NET_ONION, addrOnion);

This litle patch should do the trick and fix the problem and that should be in line what @laanwj suggested, please feel free to copy paste. ;-)

DrahtBot commented at 11:31 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:

#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)
#15423 (torcontrol: Query Tor for correct -onion configuration by luke-jr)

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.

luke-jr commented at 5:24 pm on February 16, 2019: member

I think when using the Tor controller, we probably want to use the Tor’s own listening config (see #15423) over a -proxy setting…

qubenix commented at 11:41 pm on February 18, 2019: contributor

@luke-jr Could be better to listen to the control port, just don’t forget about situations where onion-grater is used and the response from the Tor controller intentionally lies to the application. Onion-grater use is considered a Tor best practice and is being used in Whonix and TAILS.

In Whonix there is a dedicated port for Bitcoin traffic, <gateway-ip>:9111, but if you rely on the control port you get a response like this:

0user@host:~$ nc -nv 127.0.0.1 9051
1(UNKNOWN) [127.0.0.1] 9051 (?) open
2AUTHENTICATE
3250 OK
4GETINFO net/listeners/socks
5250-net/listeners/socks="127.0.0.1:9150"
6250 OK

Probably best to rely on the controller and systems using onion-grater can use a profile. Maybe an example one can be included in this repo for such systems.

This PR was only meant to correct the defined behavior of defaulting to -proxy:

0-onion=ip:port
1Use separate SOCKS5 proxy to reach peers via Tor hidden services, set -noonion to disable (default: -proxy)

@laanwj

Though ideally we’d query Net’s state directly here for “is there a proxy for onion already” instead of indirectly divising it from the arguments. This would really solve the problem of the current code here.

Sorry, I’m not sure how to do that. I only wanted to help, but I’m afraid the existence of this PR may be keeping others from doing it the ideal way which you suggested. Just let me know if closing this is more help.

DrahtBot commented at 1:48 pm on August 16, 2019: member

DrahtBot closed this on Aug 16, 2019

DrahtBot reopened this on Aug 16, 2019

adamjonas commented at 7:48 pm on May 18, 2020: member

@qubenix bumping here to see if you are planning on continuing to push this forward.

qubenix commented at 8:22 pm on May 18, 2020: contributor

@adamjonas I don’t what I can/should be doing to push this forward. This PR fixes the bug, but if it’s not done in an acceptable way then there’s nothing more I can do. I’m not a coder by trade, I just found a bug and offered a solution that I was able to figure out. That being said, I’m happy to do whatever is expected of me here as long as it’s within my ability. I’m just not sure what that is.

Saibato commented at 3:53 pm on June 23, 2020: contributor

@adamjonas I don’t what I can/should be doing to push this forward. This PR fixes the bug, but if it’s not done in an acceptable way then there’s nothing more I can do. I’m not a coder by trade, I just found a bug and offered a solution that I was able to figure out. That being said, I’m happy to do whatever is expected of me here as long as it’s within my ability. I’m just not sure what that is.

Hi @qubenix might take a look at #19358? I run in the same problem and found that other settings are also ignored by this footgun proxy fallback, so i took the freedom and pushed this alternative fix.

fanquake commented at 6:39 am on September 29, 2020: member

Anyone interested in this PR should instead review #19358.

fanquake closed this on Sep 29, 2020

DrahtBot locked this on Feb 15, 2022

correct -onion default to -proxy behavior #14729

Conflicts