net: don't bind on 0.0.0.0 if binds are restricted to Tor #20234

vasild · 2020-10-24T19:25:46Z

The semantic of -bind is to restrict the binding only to some address.
If not specified, then the user does not care and we bind to 0.0.0.0.
If specified then we should honor the restriction and bind only to the
specified address.

Before this change, if no -bind is given then we would bind to
0.0.0.0:8333 and to 127.0.0.1:8334 (incoming Tor) which is ok -
the user does not care to restrict the binding.

However, if only -bind=addr:port=onion is given (without ordinary
-bind=) then we would bind to addr:port and to 0.0.0.0:8333 in
addition.

Change the above to not do the additional bind: if only
-bind=addr:port=onion is given (without ordinary -bind=) then bind
to addr:port (only) and consider incoming connections to that as Tor
and do not advertise it. I.e. a Tor-only node.

vasild · 2020-10-24T19:26:39Z

This is a followup to #19991, cc @hebasto

luke-jr · 2020-10-24T19:47:32Z

How does Tor continue to function after this?

DrahtBot · 2020-10-25T05:25:24Z

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: fix GetListenPort() to derive the proper port #20196 by vasild

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.

jonatack · 2020-10-28T17:42:19Z

Concept ACK

jonatack

Approach ACK/almost ACK db2d6f3

jonatack · 2020-10-28T18:25:21Z

src/net.cpp

        fBound |= Bind(addrBind.m_service, (BF_EXPLICIT | BF_REPORT_ERROR), addrBind.m_flags);
    }
-    if (binds.empty() && whiteBinds.empty()) {
+    for (const auto& addr_bind : options.onion_binds) {
+        fBound |= Bind(addr_bind, BF_EXPLICIT | BF_REPORT_ERROR | BF_DONT_ADVERTISE, NetPermissionFlags::PF_NONE);


The report error flag was previously absent for the onion binds, while present for the others. This was added in bb145c9, @hebasto WDYT, just an oversight?

Yeah. #19991 (comment)

I think it is reasonable to inform the user that his request couldn't be fulfilled. E.g. if he specifies -bind=1.2.3.4:8334=onion and we can't bind on that address and port.

@hebasto WDYT, just an oversight?

Good catch! I think the BF_REPORT_ERROR flag was missed for no reason.

Stripping down this PR I had to remove the BF_REPORT_ERROR flag.

If one runs two bitcoinds with explicit -bind=... that do not conflict one may expect that they would start successfully. The functional testing framework expects this! However they both try to bind on the same default onion target 127.0.0.1:8334, one of them succeeds and the other fails. That failure is muted by the absence of BF_REPORT_ERROR. Apparently no test cares whether that bind has failed or not. To observe the problem, in master, add BF_REPORT_ERROR and run ./test/functional/test_runner.py, observe random tests failures with errors like "unable to bind to 127.0.0.1:8334".

(the full version of this PR used to fix that and thus the presence of BF_REPORT_ERROR did not cause tests to brick)

test/functional/feature_bind_extra.py

laanwj · 2020-10-29T11:14:49Z

@vasild, like any contributor, is part of "The Bitcoin Core developers"
This is not the place for copyright discussion in any case. Please keep the discussion focused on the code change done here.

gmaxwell · 2020-10-29T11:28:31Z

Unless I am misunderstanding this PR, this will break tor configuration for any user with an explicit bind configuration. Not binding to localhost is extremely weird, I can't think of any other program I've encountered doing that.

Is there an actual usecase for not binding localhost?

Might it be better to just document that localhost binds are implicit and then, if there is a need for not binding to localhost, have an argument to specifically disable that?

In other words, I think what you're actually pointing to is just a doc bug where the documentation isn't pedantically correct, but where the behaviour is what 99.999% of users would prefer. If so-- change the doc.

vasild · 2020-10-29T12:09:47Z

@gmaxwell

this will break tor configuration for any user with an explicit bind configuration. Not binding to localhost is extremely weird

You found a bug! Not in this PR, but in 0.20.1:

$ bitcoind --version
Bitcoin Core version v0.20.1.0-36355f128f8b

$ bitcoind -bind=1.1.1.1:8333 -listenonion=1
...
2020-10-29T11:51:58Z tor: Got service ID 7hgc4pgtuexasesn, advertising service 7hgc4pgtuexasesn.onion:8333
...

bitcoind tells the Tor daemon to redirect incoming connections to 127.0.0.1:8333:

https://github.com/bitcoin/bitcoin/blob/v0.20.1/src/torcontrol.cpp#L539

but then it does not listen on 127.0.0.1:8333:

$ sockstat -l |grep bitcoin
vd       bitcoind   95898 6  tcp4   127.0.0.1:8332        *:*
vd       bitcoind   95898 21 tcp4   1.1.1.1:8333          *:*
$ telnet 127.0.0.1 8333
Trying 127.0.0.1...
telnet: connect to address 127.0.0.1: Connection refused

That bug was fixed by #19991 by removing the hardcoded 127.0.0.1.

When only -bind= is provided (without -bind=...=onion), this PR would keep the same behavior as 0.20.1 as above - it would bind only on 1.1.1.1:8333 as requested. However it would tell the Tor daemon to redirect connections to 1.1.1.1:8333 instead of the bogus 127.0.0.1:8333 - as a side effect of the change from #19991 now we use the proper address instead of hardcoding 127.0.0.1.

gmaxwell · 2020-10-29T12:32:21Z

rpc bind is a separate configuration from 'bind', as is whitebind, perhaps the onion should be its own bind statement (that users normally never touch)

vasild · 2020-10-30T08:23:08Z

Fixed the test failure on Travis (confirmed on my local fork).

hebasto · 2020-10-30T14:06:16Z

If only -bind=addr is given (without -bind=...=onion) then we
would bind to addr and to 127.0.0.1:8334 in addition...

One could expect binding to the default 127.0.0.1:8334 as no -bind=...=onion is provided explicitly.

... which may
be unexpected assuming the perception of -bind=addr is
"bind only to addr".

If perception of an option is ambiguous the option docs require improvement, no?

If only -bind=addr=onion is given (without ordinary -bind=)
then we would bind to addr and to 0.0.0.0 in addition.

Again, binding to the default 0.0.0.0:8333 is reasonable as no -bind=... is provided explicitly.

I understand, that all statements above look like -bind=... and -bind=...=onion are two independent options. Are they?

vasild · 2020-10-30T14:32:46Z

I understand, that all statements above look like -bind=... and -bind=...=onion are two independent options. Are they?

I think all 3 of -bind=..., -bind=...=onion and -whitebind= should be considered "the same" because all of them control where to listen for P2P connections.

For example, if -whitebind= is present and -bind= is not, then we do not bind on 0.0.0.0 anymore:

$ bitcoind -whitebind=1.1.1.1:8888
...
$ sockstat -l |grep bitcoind
vd       bitcoind   46471 6  tcp4   127.0.0.1:8332        *:*
vd       bitcoind   46471 19 tcp4   1.1.1.1:8888          *:*
$

hebasto · 2020-10-30T14:39:09Z

For example, if -whitebind= is present and -bind= is not, then we do not bind on 0.0.0.0 anymore:

This behavior seems to need to be properly documented as well :)

vasild · 2020-11-04T11:32:05Z

This PR seems to have derailed in direction of documentation, but I think the code needs to be changed.

Take any popular server - Apache, PostgreSQL, MySQL, OpenSSH, Tor, Bitcoin Core 0.20.1 (but not the current master). They all listen for some service and if the user explicitly specifies to listen/bind only on a given address:port, they would not listen for that service on other addresses/ports in addition.

Apache, for example has Listen 80 which binds on 0.0.0.0:80, but when changed to Listen 1.1.1.1:80 it would only bind on 1.1.1.1:80.

The current master does not abide to the above and even if requested to bind on a specific address:port to listen for P2P, it may bind to 127.0.0.1 or 0.0.0.0 in addition (details are in the OP). IMO this violates the principle of least surprise.

maflcko · 2020-11-05T19:07:44Z

The 0.21 milestone has been removed for now. And this is not a regression anyway, AFAICT

ghost · 2020-12-06T08:58:16Z

I understand, that all statements above look like -bind=... and -bind=...=onion are two independent options. Are they?

It looks to me that they are.

I find the current -bind option documentation with the optional =onion suffix rather confusing.

Could it be possible to introduce a new option -onionbind (analog to names rpcbind, whitebind) to set the onion bind address explicitly and prevent confusing mixing?

Both -bind and -onionbind should be able to be unset (e.g. set to 'none') to explicitly disable them. E.g. if one would only want to "onion bind" and not to bind to clearnet at all, one could set -bind=none, while for -onionbind the default would take effect.

Here a suggestion of the documentation that could be then, which I think would be clearer:

  -bind=<addr>[:<port>]|none
       Bind to given address and always listen on it (default: 0.0.0.0). Use
       [host]:port notation for IPv6.

  -onionbind=<addr>[:<port>]|none
       Bind onion to given address, always listen on it and tag any incoming
       connections to that address and port as incoming Tor connections.
       (defaults: 127.0.0.1:8334, testnet: 127.0.0.1:18334,
       signet: 127.0.0.1:38334, regtest: 127.0.0.1:18445)
       Use [host]:port notation for IPv6.

Unfortunately, I only now came to know of to the =onion suffix introduced in #19991.

Edit: gmaxwell suggested similar (#20234 (comment))
"perhaps the onion should be its own bind statement (that users normally never touch)"

laanwj · 2021-06-21T06:33:18Z

Code review ACK a6b8fc9

src/init.cpp

hebasto · 2021-07-01T12:27:51Z

If only -bind=addr is given (without -bind=...=onion) then bind to
addr (only). If we are creating tor hidden service then use addr
as target. Same behavior as before net: Use alternative port for incoming Tor connections #19991.

-bind=addr does not mention onion network, but it does change onion-related behavior, i.e., it prevents recognizing of incoming connections that come via Tor.

This new behavior is confusing (at least to me), and I don't agree with it.

gruve-p · 2021-07-01T14:10:05Z

ACK a6b8fc9

vasild · 2021-07-01T14:37:06Z

@hebasto, that is similar to how it operates with other networks - if the user restricts the binds to only some networks, then all other networks are ditched (affected) wrt listening/binding.

For example, assume a machine has an IPv4 address 1.2.3.4 and an IPv6 address. By default if no -bind is given it would bind to both addresses. If -bind=1.2.3.4 is given it would not bind on the IPv6 address. Now this statement:

"-bind=addr does not mention IPv6, but it does change IPv6-related behavior..."

Would this change to -bind= description help?

  -bind=<addr>[:<port>][=onion]
-       Bind to given address...
+       Bind only to given address...

hebasto · 2021-07-01T14:45:30Z

For example, assume a machine has an IPv4 address 1.2.3.4 and an IPv6 address. By default if no -bind is given it would bind to both addresses. If -bind=1.2.3.4 is given it would not bind on the IPv6 address.

Right. But my concerns are not about binding itself, rather about behavior that users might expect -- the distinguishing incoming Tor connections.

jonatack · 2021-07-01T15:01:03Z

distinguishing incoming Tor connections

Right, CNode::m_inbound_onion and CNode::ConnectedThroughNetwork() need to remain pertinent, as the logic is used by the inbound eviction protection and by getpeerinfo, -netinfo, and the GUI peers window.

vasild · 2021-07-01T15:05:38Z

I see. So it is about judging which expectation to fail:

Fail the expectation that -bind=addr only binds to addr (leave this PR unmerged). This might have security implications: if a user specifies -bind=1.2.3.4:8333 and expects that this is the only listen address:port and sets up his firewall rules according to that expectation. Especially that this was the case before Oct 2020 (see below).
Fail the expectation that we distinguish incoming Tor connections if -bind=addr is given (merge this PR). We do that only after net: Use alternative port for incoming Tor connections #19991 which was merged 9 months ago (in Oct 2020). Btw, in peer eviction we semi-distinguish Tor connections even in that case - we protect localhost peers in addition to Tor ones.

vasild · 2021-07-02T08:08:36Z

@hebasto, what do you think about the second change in this PR? That is - the second item from the PR description: "* If only -bind=addr=onion is given...". If that is not contentious then maybe it would make sense that I split the PR in two.

hebasto · 2021-07-02T08:44:41Z

@hebasto, what do you think about the second change in this PR? That is - the second item from the PR description: "* If only -bind=addr=onion is given...". If that is not contentious then maybe it would make sense that I split the PR in two.

sgtm

vasild · 2021-07-06T12:12:43Z

a6b8fc9...a004833: remove the controversial part of this PR (the behavior when -bind=... is given without -bind=...=onion -- behave as in master), take review suggestions

vasild · 2021-07-06T12:18:27Z

@hebasto, what do you think about the second change in this PR? That is - the second item from the PR description: "* If only -bind=addr=onion is given...". If that is not contentious then maybe it would make sense that I split the PR in two.

sgtm

Done! Updated PR title and description.

Rspigler · 2021-07-06T23:59:26Z

Concept ACK

jonatack

ACK a004833

Verified the first test (Node0) fails on master, the second test passes (perhaps inverse their order).

test/functional/feature_bind_extra.py

The semantic of `-bind` is to restrict the binding only to some address. If not specified, then the user does not care and we bind to `0.0.0.0`. If specified then we should honor the restriction and bind only to the specified address. Before this change, if no `-bind` is given then we would bind to `0.0.0.0:8333` and to `127.0.0.1:8334` (incoming Tor) which is ok - the user does not care to restrict the binding. However, if only `-bind=addr:port=onion` is given (without ordinary `-bind=`) then we would bind to `addr:port` _and_ to `0.0.0.0:8333` in addition. Change the above to not do the additional bind: if only `-bind=addr:port=onion` is given (without ordinary `-bind=`) then bind to `addr:port` (only) and consider incoming connections to that as Tor and do not advertise it. I.e. a Tor-only node.

vasild · 2021-07-07T13:49:02Z

a0048331a1...2feec3ce31: remove commented tests - they can be resurrected later if needed and contained a typo (thanks, @jonatack for testing even commented out stuff!)

laanwj · 2021-07-08T14:38:22Z

Code review ACK 2feec3c

jonatack · 2021-07-08T14:39:53Z

utACK 2feec3c per git diff a004833 2feec3c

jonatack · 2021-07-08T14:45:41Z

Note: This PR is tagged for 0.22, but 2 Tor/I2P PRs that should probably be priority for 0.22 but are not tagged are #22179 and #22211.

hebasto

ACK 2feec3c, tested on Linux Mint 20.1 (x86_64):

$ src/bitcoind -signet
...
2021-07-08T14:50:35Z [init] Bound to 127.0.0.1:38334
2021-07-08T14:50:35Z [init] Bound to [::]:38333
2021-07-08T14:50:35Z [init] Bound to 0.0.0.0:38333
...

$ src/bitcoind -signet -bind=127.0.0.1:38334=onion
...
2021-07-08T14:52:02Z [init] Bound to 127.0.0.1:38334
...

…ed to Tor

DrahtBot added the P2P label Oct 24, 2020

DrahtBot mentioned this pull request Oct 25, 2020

net: fix GetListenPort() to derive the proper port #20196

Merged

vasild mentioned this pull request Oct 26, 2020

Add Tor icon bitcoin-core/gui#86

Closed

maflcko added this to the 0.21.0 milestone Oct 28, 2020

jonatack reviewed Oct 28, 2020

View reviewed changes

bitcoin deleted a comment Oct 29, 2020

DrahtBot mentioned this pull request Oct 30, 2020

net: Add blockfilters white{bind,list} permission flag #18972

Closed

bitcoin deleted a comment Oct 30, 2020

vasild force-pushed the bind branch from db2d6f3 to c055a50 Compare October 30, 2020 08:05

vasild closed this Oct 30, 2020

vasild reopened this Oct 30, 2020

laanwj modified the milestones: 0.21.0, 22.0 Nov 5, 2020

DrahtBot mentioned this pull request Nov 17, 2020

CConnman: move initialization to declaration #20408

Merged

DrahtBot added the Needs rebase label Nov 18, 2020

hebasto reviewed Jun 29, 2021

View reviewed changes

src/init.cpp Outdated Show resolved Hide resolved

vasild force-pushed the bind branch 2 times, most recently from c272abb to a004833 Compare July 6, 2021 12:09

vasild changed the title ~~net: don't extra bind for Tor if binds are restricted~~ net: don't bind on 0.0.0.0 if binds are restricted to Tor Jul 6, 2021

jonatack reviewed Jul 7, 2021

View reviewed changes

test/functional/feature_bind_extra.py Outdated Show resolved Hide resolved

vasild force-pushed the bind branch from a004833 to 2feec3c Compare July 7, 2021 13:47

hebasto approved these changes Jul 8, 2021

View reviewed changes

laanwj merged commit 842e2a9 into bitcoin:master Jul 12, 2021

vasild deleted the bind branch July 12, 2021 08:12

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Jul 14, 2021

Merge bitcoin#20234: net: don't bind on 0.0.0.0 if binds are restrict…

31e7f60

…ed to Tor

Rspigler mentioned this pull request Aug 18, 2021

Make it possible to disable Tor binds and abort startup on bind failure #22729

Merged

gwillen pushed a commit to ElementsProject/elements that referenced this pull request Jun 1, 2022

Merge 842e2a9 into merged_master (Bitcoin PR bitcoin/bitcoin#20234)

a4a4d73

bitcoin locked as resolved and limited conversation to collaborators Aug 16, 2022

net: don't bind on 0.0.0.0 if binds are restricted to Tor #20234

net: don't bind on 0.0.0.0 if binds are restricted to Tor #20234

Uh oh!

Conversation

vasild commented Oct 24, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vasild commented Oct 24, 2020

Uh oh!

luke-jr commented Oct 24, 2020

Uh oh!

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

Uh oh!

Conflicts

Uh oh!

jonatack commented Oct 28, 2020

Uh oh!

jonatack left a comment • 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!

jonatack Oct 28, 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 Oct 30, 2020

Choose a reason for hiding this comment

Uh oh!

hebasto Oct 30, 2020

Choose a reason for hiding this comment

Uh oh!

vasild Jul 6, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

laanwj commented Oct 29, 2020

Uh oh!

gmaxwell commented Oct 29, 2020

Uh oh!

vasild commented Oct 29, 2020

Uh oh!

gmaxwell commented Oct 29, 2020

Uh oh!

vasild commented Oct 30, 2020

Uh oh!

hebasto commented Oct 30, 2020

Uh oh!

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

Uh oh!

Uh oh!

hebasto commented Oct 30, 2020

Uh oh!

vasild commented Nov 4, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

maflcko commented Nov 5, 2020

Uh oh!

ghost commented Dec 6, 2020 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

laanwj commented Jun 21, 2021

Uh oh!

Uh oh!

hebasto commented Jul 1, 2021

Uh oh!

gruve-p commented Jul 1, 2021

Uh oh!

vasild commented Jul 1, 2021

Uh oh!

hebasto commented Jul 1, 2021

Uh oh!

jonatack commented Jul 1, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vasild commented Jul 1, 2021

Uh oh!

vasild commented Jul 2, 2021

Uh oh!

hebasto commented Jul 2, 2021

vasild commented Oct 24, 2020 •

edited

Loading

DrahtBot commented Oct 25, 2020 •

edited

Loading

jonatack left a comment •

edited

Loading

jonatack Oct 28, 2020 •

edited

Loading

vasild commented Oct 30, 2020 •

edited

Loading

vasild commented Nov 4, 2020 •

edited

Loading

ghost commented Dec 6, 2020 •

edited by ghost

Loading

jonatack commented Jul 1, 2021 •

edited

Loading

vasild commented Jul 6, 2021 •

edited

Loading

vasild commented Jul 6, 2021 •

edited

Loading

jonatack left a comment •

edited

Loading