Skip to content

Commit

Permalink
Merge bitcoin/bitcoin#20234: net: don't bind on 0.0.0.0 if binds are …
Browse files Browse the repository at this point in the history
…restricted to Tor

2feec3c net: don't bind on 0.0.0.0 if binds are restricted to Tor (Vasil Dimov)

Pull request description:

  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.

ACKs for top commit:
  laanwj:
    Code review ACK 2feec3c
  jonatack:
    utACK 2feec3c per `git diff a004833 2feec3c`
  hebasto:
    ACK 2feec3c, tested on Linux Mint 20.1 (x86_64):

Tree-SHA512: a04483af601706da928958b92dc560f9cfcc78ab0bb9d74414636eed1c6f29ed538ce1fb5a17d41ed82c9c9a45ca94899d0966e7ef93da809c9bcdcdb1d1f040
  • Loading branch information
laanwj committed Jul 12, 2021
2 parents e0fe658 + 2feec3c commit 842e2a9
Show file tree
Hide file tree
Showing 5 changed files with 129 additions and 29 deletions.
33 changes: 21 additions & 12 deletions src/init.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1717,25 +1717,34 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
return InitError(ResolveErrMsg("bind", bind_arg));
}

if (connOptions.onion_binds.empty()) {
connOptions.onion_binds.push_back(DefaultOnionServiceTarget());
}

if (args.GetBoolArg("-listenonion", DEFAULT_LISTEN_ONION)) {
const auto bind_addr = connOptions.onion_binds.front();
if (connOptions.onion_binds.size() > 1) {
InitWarning(strprintf(_("More than one onion bind address is provided. Using %s for the automatically created Tor onion service."), bind_addr.ToStringIPPort()));
}
StartTorControl(bind_addr);
}

for (const std::string& strBind : args.GetArgs("-whitebind")) {
NetWhitebindPermissions whitebind;
bilingual_str error;
if (!NetWhitebindPermissions::TryParse(strBind, whitebind, error)) return InitError(error);
connOptions.vWhiteBinds.push_back(whitebind);
}

// If the user did not specify -bind= or -whitebind= then we bind
// on any address - 0.0.0.0 (IPv4) and :: (IPv6).
connOptions.bind_on_any = args.GetArgs("-bind").empty() && args.GetArgs("-whitebind").empty();

CService onion_service_target;
if (!connOptions.onion_binds.empty()) {
onion_service_target = connOptions.onion_binds.front();
} else {
onion_service_target = DefaultOnionServiceTarget();
connOptions.onion_binds.push_back(onion_service_target);
}

if (args.GetBoolArg("-listenonion", DEFAULT_LISTEN_ONION)) {
if (connOptions.onion_binds.size() > 1) {
InitWarning(strprintf(_("More than one onion bind address is provided. Using %s "
"for the automatically created Tor onion service."),
onion_service_target.ToStringIPPort()));
}
StartTorControl(onion_service_target);
}

for (const auto& net : args.GetArgs("-whitelist")) {
NetWhitelistPermissions subnet;
bilingual_str error;
Expand Down
21 changes: 8 additions & 13 deletions src/net.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -2465,38 +2465,33 @@ bool CConnman::Bind(const CService &addr, unsigned int flags, NetPermissionFlags
return true;
}

bool CConnman::InitBinds(
const std::vector<CService>& binds,
const std::vector<NetWhitebindPermissions>& whiteBinds,
const std::vector<CService>& onion_binds)
bool CConnman::InitBinds(const Options& options)
{
bool fBound = false;
for (const auto& addrBind : binds) {
for (const auto& addrBind : options.vBinds) {
fBound |= Bind(addrBind, (BF_EXPLICIT | BF_REPORT_ERROR), NetPermissionFlags::None);
}
for (const auto& addrBind : whiteBinds) {
for (const auto& addrBind : options.vWhiteBinds) {
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_DONT_ADVERTISE, NetPermissionFlags::None);
}
if (options.bind_on_any) {
struct in_addr inaddr_any;
inaddr_any.s_addr = htonl(INADDR_ANY);
struct in6_addr inaddr6_any = IN6ADDR_ANY_INIT;
fBound |= Bind(CService(inaddr6_any, GetListenPort()), BF_NONE, NetPermissionFlags::None);
fBound |= Bind(CService(inaddr_any, GetListenPort()), !fBound ? BF_REPORT_ERROR : BF_NONE, NetPermissionFlags::None);
}

for (const auto& addr_bind : onion_binds) {
fBound |= Bind(addr_bind, BF_EXPLICIT | BF_DONT_ADVERTISE, NetPermissionFlags::None);
}

return fBound;
}

bool CConnman::Start(CScheduler& scheduler, const Options& connOptions)
{
Init(connOptions);

if (fListen && !InitBinds(connOptions.vBinds, connOptions.vWhiteBinds, connOptions.onion_binds)) {
if (fListen && !InitBinds(connOptions)) {
if (clientInterface) {
clientInterface->ThreadSafeMessageBox(
_("Failed to listen on any port. Use -listen=0 if you want this."),
Expand Down
8 changes: 4 additions & 4 deletions src/net.h
Original file line number Diff line number Diff line change
Expand Up @@ -768,6 +768,9 @@ class CConnman
std::vector<NetWhitebindPermissions> vWhiteBinds;
std::vector<CService> vBinds;
std::vector<CService> onion_binds;
/// True if the user did not specify -bind= or -whitebind= and thus
/// we should bind on `0.0.0.0` (IPv4) and `::` (IPv6).
bool bind_on_any;
bool m_use_addrman_outgoing = true;
std::vector<std::string> m_specified_outgoing;
std::vector<std::string> m_added_nodes;
Expand Down Expand Up @@ -962,10 +965,7 @@ class CConnman

bool BindListenPort(const CService& bindAddr, bilingual_str& strError, NetPermissionFlags permissions);
bool Bind(const CService& addr, unsigned int flags, NetPermissionFlags permissions);
bool InitBinds(
const std::vector<CService>& binds,
const std::vector<NetWhitebindPermissions>& whiteBinds,
const std::vector<CService>& onion_binds);
bool InitBinds(const Options& options);

void ThreadOpenAddedConnections();
void AddAddrFetch(const std::string& strDest);
Expand Down
95 changes: 95 additions & 0 deletions test/functional/feature_bind_extra.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,95 @@
#!/usr/bin/env python3
# Copyright (c) 2014-2021 The Bitcoin Core developers
# Distributed under the MIT software license, see the accompanying
# file COPYING or http://www.opensource.org/licenses/mit-license.php.
"""
Test starting bitcoind with -bind and/or -bind=...=onion and confirm
that bind happens on the expected ports.
"""

import sys

from test_framework.netutil import (
addr_to_hex,
get_bind_addrs,
)
from test_framework.test_framework import (
BitcoinTestFramework,
SkipTest,
)
from test_framework.util import (
PORT_MIN,
PORT_RANGE,
assert_equal,
rpc_port,
)

class BindExtraTest(BitcoinTestFramework):
def set_test_params(self):
self.setup_clean_chain = True
# Avoid any -bind= on the command line. Force the framework to avoid
# adding -bind=127.0.0.1.
self.bind_to_localhost_only = False
self.num_nodes = 2

def setup_network(self):
# Override setup_network() because we want to put the result of
# p2p_port() in self.extra_args[], before the nodes are started.
# p2p_port() is not usable in set_test_params() because PortSeed.n is
# not set at that time.

# Due to OS-specific network stats queries, we only run on Linux.
self.log.info("Checking for Linux")
if not sys.platform.startswith('linux'):
raise SkipTest("This test can only be run on Linux.")

loopback_ipv4 = addr_to_hex("127.0.0.1")

# Start custom ports after p2p and rpc ports.
port = PORT_MIN + 2 * PORT_RANGE

# Array of tuples [command line arguments, expected bind addresses].
self.expected = []

# Node0, no normal -bind=... with -bind=...=onion, thus only the tor target.
self.expected.append(
[
[f"-bind=127.0.0.1:{port}=onion"],
[(loopback_ipv4, port)]
],
)
port += 1

# Node1, both -bind=... and -bind=...=onion.
self.expected.append(
[
[f"-bind=127.0.0.1:{port}", f"-bind=127.0.0.1:{port + 1}=onion"],
[(loopback_ipv4, port), (loopback_ipv4, port + 1)]
],
)
port += 2

self.extra_args = list(map(lambda e: e[0], self.expected))
self.add_nodes(self.num_nodes, self.extra_args)
# Don't start the nodes, as some of them would collide trying to bind on the same port.

def run_test(self):
for i in range(len(self.expected)):
self.log.info(f"Starting node {i} with {self.expected[i][0]}")
self.start_node(i)
pid = self.nodes[i].process.pid
binds = set(get_bind_addrs(pid))
# Remove IPv6 addresses because on some CI environments "::1" is not configured
# on the system (so our test_ipv6_local() would return False), but it is
# possible to bind on "::". This makes it unpredictable whether to expect
# that bitcoind has bound on "::1" (for RPC) and "::" (for P2P).
ipv6_addr_len_bytes = 32
binds = set(filter(lambda e: len(e[0]) != ipv6_addr_len_bytes, binds))
# Remove RPC ports. They are not relevant for this test.
binds = set(filter(lambda e: e[1] != rpc_port(i), binds))
assert_equal(binds, set(self.expected[i][1]))
self.stop_node(i)
self.log.info(f"Stopped node {i}")

if __name__ == '__main__':
BindExtraTest().main()
1 change: 1 addition & 0 deletions test/functional/test_runner.py
Original file line number Diff line number Diff line change
Expand Up @@ -139,6 +139,7 @@
'interface_zmq.py',
'rpc_invalid_address_message.py',
'interface_bitcoin_cli.py',
'feature_bind_extra.py',
'mempool_resurrect.py',
'wallet_txn_doublespend.py --mineblock',
'tool_wallet.py --legacy-wallet',
Expand Down

0 comments on commit 842e2a9

Please sign in to comment.