net: support unix domain sockets for -proxy and -onion

[pinheadmz]

trafficstars

Closes https://github.com/bitcoin/bitcoin/issues/27252

UNIX domain sockets are a mechanism for inter-process communication that are faster than local TCP ports (because there is no need for TCP overhead) and potentially more secure because access is managed by the filesystem instead of serving an open port on the system.

There has been work on unix domain sockets before but for now I just wanted to start on this single use-case which is enabling unix sockets from the client side, specifically connecting to a local Tor proxy (Tor can listen on unix sockets and even enforces strict curent-user-only access permission before binding) configured by -onion= or -proxy=

I copied the prefix unix: usage from Tor. With this patch built locally you can test with your own filesystem path (example):

tor --SocksPort unix:/Users/matthewzipkin/torsocket/x

bitcoind -proxy=unix:/Users/matthewzipkin/torsocket/x

Prep work for this feature includes:

• Moving where and how we create sockaddr and Sock to accommodate AF_UNIX without disturbing CService
• Expanding Proxy class to represent either a CService or a UNIX socket (by its file path)

Future work:

• Enable UNIX sockets for ZMQ (https://github.com/bitcoin/bitcoin/pull/27679)
• Enable UNIX sockets for I2P SAM proxy (some code is included in this PR but not tested or exposed to user options yet)
• Enable UNIX sockets on windows where supported
• Update Network Proxies dialog in GUI to support UNIX sockets

[DrahtBot]

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

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	Sjors, vasild, tdb3, achow101
Stale ACK	willcl-ark

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

• #29641 (scripted-diff: Use LogInfo/LogDebug over LogPrintf/LogPrint by maflcko)
• #29015 (kernel: Streamline util library by ryanofsky)
• #28834 (net: attempts to connect to all resolved addresses when connecting to a node by sr-gi)

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.

[pinheadmz]

Thanks so much for the great review @vasild I think I addressed all your feedback up to the adding UNIX to CNetAddr. I understand your concerns there and we can chat about it, I'll look at alternatives as well. What I did do in this last push was mostly separate unix sockets from regular sockets, and adding Proxy.GetSocket() to decide which socket factory to call.

[pinheadmz]

@vasild did an overhaul of this PR and implemented more how we discussed on IRC. Edited the description to explain. I ended up not using std::variant because I felt it added uneeded complexity. Current approach is Proxy just has a is_unix flag and abstracts away the CService methods like IsValid() etc to support TCP or UNIX sockets

[pinheadmz]

🐙 This pull request conflicts with the target branch and needs rebase.

🕺

[pinheadmz]

@vasild thanks for the excellent detailed review! All comments addressed except where noted.

[pinheadmz]

Rebased on master to pass CI (✅ ). @vasild and @willcl-ark would really love re-review from you handsome chaps when convenient!

[pinheadmz]

64d0f23 looks good, except there is some messup in the contents of these two commits:

Thanks @vasild sorry about the messy rebase there. I cleaned up the commits and fixed the const and m_... nits. Rebased on master for merge conflicts.

[willcl-ark]

It's a tNACK from me currently @ c3b0f137a9 as I can no longer get this functionality to work in practice on Ubuntu 23.04 with Tor 0.4.7.13.

The send of the data in vSocks5Init seems to be failing, meaning the proxy cannot be used: https://github.com/pinheadmz/bitcoin/blob/c3b0f137a9ccd894f2f1d287ccdab26310b3aaf2/src/netbase.cpp#L372-L375

I haven't yet totally ruled out issues with Tor, but I do get responses from the unix socket about it not being an HTTP proxy if I try to send an HTTP request over it, so I conclude that the Tor daemon is listening on the socket correctly waiting for SOCKS requests, and the issue may lie with Bitcoin Core.

Will continue investigating and report back.

bitcoin.conf options set

2023-07-11T11:12:35Z Config file: /home/will/.bitcoin/bitcoin.conf
2023-07-11T11:12:35Z Config file arg: [main] addnode="xx.xx.xx.xx:8333"
2023-07-11T11:12:35Z Config file arg: [main] blockfilterindex="1"
2023-07-11T11:12:35Z Config file arg: [main] blocksdir="/media/will/crucial1/bitcoin"
2023-07-11T11:12:35Z Config file arg: [main] daemon="1"
2023-07-11T11:12:35Z Config file arg: [main] debug="net"
2023-07-11T11:12:35Z Config file arg: [main] debug="proxy"
2023-07-11T11:12:35Z Config file arg: [main] debug="tor"
2023-07-11T11:12:35Z Config file arg: [main] discover="1"
2023-07-11T11:12:35Z Config file arg: [main] externalip="xx.xx.xx.xx"
2023-07-11T11:12:35Z Config file arg: [main] i2pacceptincoming="1"
2023-07-11T11:12:35Z Config file arg: [main] i2psam="127.0.0.1:7656"
2023-07-11T11:12:35Z Config file arg: [main] listen="1"
2023-07-11T11:12:35Z Config file arg: [main] listenonion="1"
2023-07-11T11:12:35Z Config file arg: [main] maxconnections="125"
2023-07-11T11:12:35Z Config file arg: [main] mempoolfullrbf="1"
2023-07-11T11:12:35Z Config file arg: [main] onion="unix:/run/tor/socks"
2023-07-11T11:12:35Z Config file arg: [main] onlynet="onion"
2023-07-11T11:12:35Z Config file arg: [main] port="8833"
2023-07-11T11:12:35Z Config file arg: [main] rpcauth=****
2023-07-11T11:12:35Z Config file arg: [main] rpcthreads="16"
2023-07-11T11:12:35Z Config file arg: [main] rpcworkqueue="64"
2023-07-11T11:12:35Z Config file arg: [main] server="1"
2023-07-11T11:12:35Z Config file arg: [main] txindex="1"

torrc

SocksPort unix:/run/tor/socks WorldWritable
ControlPort 9051
CookieAuthentication 1
CookieAuthFileGroupReadable 1
DataDirectoryGroupReadable 1

[pinheadmz]

@willcl-ark wow thank you so much for catching that. After all the refactoring, we had forgotten to actually connect to the unix socket! I added a functional test to ensure that the proxy works now and I have also tested in production with Tor locally.

This required a fairly intense refactor that included inserting 2 new commits and rebasing several others:

git range-diff 32a5a20225 c3b0f137a9 b1199bce0f

[pinheadmz]

@vasild Thanks for your work on this :-) I reorganized the extra commit you sent me. For now, still going without variant for Proxy just to keep the refator more simple, so leaving the i2p stuff mostly alone as well.

[pinheadmz]

@vasild OK I got this branch passing all CI. ✅ I ran tests locally on Ubuntu and MacOS and also tested the feature in production on both platforms with tor --SocksPort unix:/... Everything is working! yay.

The real fix was here actually (diff below) Embarrassingly simple. This was a bug on Linux but NOT on macOS.

I did not need the SendComplete changes or Socks5 changes which is great because that keeps review simple and the branch focused. I will be happy to review those changes if you want to open a new PR for them (or I can open it if you like)

The only other new change to the branch besides this is adding a release note and explanation in help text for -onion and -proxy

diff --git a/src/netbase.cpp b/src/netbase.cpp
index a49f882130..044b0ba6c3 100644
--- a/src/netbase.cpp
+++ b/src/netbase.cpp
@@ -638,7 +638,7 @@ std::unique_ptr<Sock> Proxy::Connect() const
     addrun.sun_family = AF_UNIX;
     // leave the last char in addrun.sun_path[] to be always '\0'
     memcpy(addrun.sun_path, path.c_str(), std::min(sizeof(addrun.sun_path) - 1, path.length()));
-    socklen_t len = strlen(addrun.sun_path) + sizeof(addrun);
+    socklen_t len = sizeof(addrun);
 
     if(!ConnectToSocket(*sock, (struct sockaddr*)&addrun, len, path, /*manual_connection=*/true)) {
         LogPrintf("Cannot connect to socket for %s\n", path);

[pinheadmz]

@vasild thanks again for your review! Nits addressed and rebased on master. I will help you review https://github.com/bitcoin/bitcoin/pull/28649

[pinheadmz]

Thanks again @vasild rebased on master - I see some new I2P tests in there now! Had to update those with the changes to CreateSock()

[maflcko]

test/i2p_tests.cpp:162:79: error: no matching function for call to ‘i2p::sam::Session::Session(const fs::path&, CService, CThreadInterrupt*)’

[pinheadmz]

Rebased on master to fix silent conflict with new I2P tests.

@maflcko thanks. Can you help me understand why this only broke the previous builds task?

[maflcko]

To preserve CI resources, only one task is re-run to detect silent merge conflicts.

[Sjors]

Would be nice to have this for RPC too, but can wait for a followup.

Ditto for direct peer connections.

[pinheadmz]

@Sjors https://github.com/bitcoin/bitcoin/pull/27375#discussion_r1506062696

Both call sites of ConnectThroughProxy have access to conn_type so might as well pass in manual_connection.

Do you mean to pass it ConnectThroughProxy() -> proxy.Connect() -> ConnectDirectly()?

That would also mean we need manual_connection in i2p.cpp where we call proxy.Connect() ?

[Sjors]

That would also mean we need manual_connection in i2p.cpp where we call proxy.Connect() ?

@vasild does the i2p code know if a connection is manual? We can always set manual_connection to true if we don't know (or it adds too much complexity), but we should set it to the correct value if we do know.

[pinheadmz]

@Sjors If I'm following this correctly, the only benefit of passing manual_connection to ConnectDirectly() is that it is then passed to ConnectToSocket() which only uses the boolean when logging with LogConnectFailure().

On current master, ConnectThroughProxy() calls ConnectSocketDirectly() with a hard-coded true for manual_connection. I believe the motivation is in https://github.com/bitcoin/bitcoin/pull/12569

So I'm inclined to leave this hard-coded as true because IIUC the connection being described by the boolean in this context isn't "manual" in the "p2p addrman vs addnode" sense, but is used to refer to the connection between bitcoind and the input to the proxy.

[pinheadmz]

git range-diff master 6dddc1c2eda514d35219cce03c229bd6f822be55 567cec9a05e1261e955535f734826b12341684b6

Rebased on master to fix conflicts and ( 🤞 ) pass CI. @Sjors thank you so much for the review, I addressed all your nits except the one or two where indicated in discussion.

[Sjors]

but is used to refer to the connection between bitcoind and the input to the proxy

I agree that shouldn't be called "manual".

re-ACK 567cec9a05e1261e955535f734826b12341684b6

[tdb3]

re ACK for 567cec9a05e1261e955535f734826b12341684b6.

Pulled, built, ran all unit and functional tests (all passed).

Re-ran the test actions (quote below) from https://github.com/bitcoin/bitcoin/pull/27375#pullrequestreview-1876248880

Overall, similar results were observed. Each test action was performed with a fresh signet directory (with the exception of blocks and chainstate, which were not cleared, to prevent IBD). For the first action (i.e. running with proxy=unix:/run/tor/socks set, but onion and onlynet not set), peer connection was observed to grow over the span of a half our (with ipv4, ipv6, and onion peers connected). debug log showed general socks connection failures (although these also occur in v26.0 when using localhost:9050 for socks):

<date> Socks5() connect to <IPv4 ADDR>:38333 failed: general failure

For the second action (i.e. running with proxy and onlynet=onion with manual seed added), onion peer connections were observed.

For both actions, UNIX socket traffic was sniffed and contained bitcoin protocol traffic.

ACK for 6dddc1c. Great work on a great feature. Seems to work well. Reviewed the code changes, but didn't see anything noteworthy over what vasild, luke-jr, and willcl-ark already saw.

Ran the following test actions:

* Cloned and checked out PR branch, compiled, and ran all functional tests (all passed)

* Configured Tor to listen on only the unix domain socket /run/tor/socks (SocksPort 9050 and ControlPort 9051 disabled)

* Started bitcoind (signet), `proxy=/run/tor/socks` set, `onion` and `onlynet` not set.  Connections were established with multiple peers and successful block download was observed.  Sniffed the unix socket (`/run/tor/socks`) with https://github.com/mechpen/sockdump, and observed bitcoin traffic in Wireshark with a pcap dump from sockdump.  `getpeerinfo` command to the node showed ipv4, ipv6, and onion addr peers.

* Started bitcoind (signet), `onion=/run/tor/socks` set, `proxy` not set, but `onlynet=onion` set.  Manually added seed `v7ajjeirttkbnt32wpy3c6w3emwnfr3fkla7hpxcfokr3ysd3kqtzmqd.onion:38333` with `addnode` command.  Connections were established with multiple peers and successful block download was observed.  Sniffed the unix socket and again observed bitcoin traffic over the socket.  `getpeerinfo` showed multiple onion addr peers.

[achow101]

ACK 567cec9a05e1261e955535f734826b12341684b6

[achow101]

Seems like there's a silent merge conflict somewhere, getting a functional test failure when merged with master:

2024-03-12T18:12:19.695000Z TestFramework (ERROR): Assertion failed
Traceback (most recent call last):
  File "/home/ava/bitcoin/bitcoin/master/test/functional/test_framework/test_framework.py", line 565, in start_nodes
    node.wait_for_rpc_connection()
  File "/home/ava/bitcoin/bitcoin/master/test/functional/test_framework/test_node.py", line 255, in wait_for_rpc_connection
    raise FailedToStartError(self._node_msg(
test_framework.test_node.FailedToStartError: [node 5] bitcoind exited with status 1 during initialization. Error: Invalid -proxy address or hostname: 'unix:/tmp/tmp9eh_pwt0'

[maflcko]

Seems like there's a silent merge conflict somewhere, getting a functional test failure when merged with master:

I re-ran the CI and it didn't fail. Are you sure this is a silent conflict, because if it was, the CI should be detecting it on a re-run?

[achow101]

Oh! The test command I use doesn't rerun autogen.sh every time, I think that's what caused the test failure. @pinheadmz You can reset the branch back to 567cec9a05e and this can be merged. Sorry about the confusion.

[pinheadmz]

Oh! The test command I use doesn't rerun autogen.sh every time, I think that's what caused the test failure. @pinheadmz You can reset the branch back to 567cec9 and this can be merged. Sorry about the confusion.

Ugh and when I was trying to reproduce I ALSO didn't rerun autogen or config :facepalm: wow THANKS @maflcko for the sanity check

[vasild]

autogen

wen cmake?

[fanquake]

Is the expectation here that we should see a lot more Cannot connect to socket for xxxxx in debug.logs? (when not using any of these new features).