bitcoind: add separate p2p socket for tor connections #405

erikarvstedt · 2021-10-06T15:23:02Z

Main commit msg

This re-enables onion tagging while still supporting untagged connections.

nixbitcoin

erikarvstedt · 2021-10-13T12:22:00Z

It's still not clear if we need this. In my tests bitcoind can distinguish Tor connections from regular connections that originate from the same localhost socket, so no seperate socket is required.
@jonasnick, what do you think?

erikarvstedt · 2021-10-15T13:58:15Z

Whether an incoming peer is flagged as onion to is determined by a single line of code in bitcoind. (m_onion_binds includes all addresses defined with bind=...=onion).

So this PR is still relevant. I must have accidentally used this PR in the test mentioned in my previous post.

I've found a whitelist bug related to inbound onion peers. This is addressed in btcpayserver: remove bitcoind, liquidd whitelist. Please see its commit message and the linked issue.

erikarvstedt · 2021-10-15T14:08:56Z

Here's a script which tests this PR.
Running this script on master shows that inbound onion peers are incorrectly tagged with "network": "not_publicly_routable".

nixbitcoin

modules/bitcoind.nix

jonasnick · 2021-10-20T22:22:17Z

modules/btcpayserver.nix

@@ -118,19 +118,13 @@ in {
      };
      # Enable p2p connections
      listen = true;
-      extraConfig = ''
-        whitelist=${nbLib.address cfg.nbxplorer.address}


Good catch! This is actually a low to medium security issue because obtaining the mempool via bip35 allows determining the transactions originating from a node (iirc). I think in general the download permission is still necessary, because bitcoin module users might want to use maxuploadtarget without breaking their btcpayserver. On the other hand, giving download permissions for inbound onion peers also breaks maxuploadtarget (as is the for the electrs module).

If you're confident that no whitelisting works with btcpayserver (@nixbitcoin did you test this?) we can merge this to fix the issue of fully whitelisting onion nodes. But my guess is that if we want to do this correctly we won't get around the complexity of having a whitebind.

confident that no whitelisting works with btcpayserver

We only introduced whitelisting three weeks ago and it has worked fine before.

having a whitebind

As mentioned here, whitebind is incompatible with listening on all interfaces (0.0.0.0).

We only introduced whitelisting three weeks ago and it has worked fine before.

Ugh true that. I remember having issues using btcpayserver without whitelisting but that may have been before we added the btcpayserver module to nix-bitcoin.

Whitebind localhost and add a separate onion socket (whitebind=127.0.0.1:8333, bind=127.0.0.1:8334=onion)
This is incompatible with accepting inbound connections on all interfaces bind=0.0.0.0:8333 due to the port clash.

Is there a problem with whitebinding on 8335?

Is there a problem with whitebinding on 8335?

We would need to add options listenLocal and localPort (= 8335) in bitcoind and liquidd and use them in btcpayserver and electrs.
This adds more complexity and we would move away from the default p2p port which might surprise users.

More complex, yes, but seems like the right approach.

Should I add it to this PR?

Would be good to have, but not extremely high priority (compared to not give full whitelisting permissions to onion peers).

This re-enables onion tagging while still supporting untagged connections. Onion sockets are not yet supported in the latest liquidd/elements version 0.18.1.12 available on nixpkgs.

Whitelisting localhost implicitly whitelists all inbound onion connections. This prevents banning misbehaving inbound onion peers and enables message `mempool` which can cause privacy leaks. Instead, grant `download` as the single bitcoind whitelist permission, which should be safe for onion peers. Remove liquidd whitelisting because it doesn't support fine-grained permissions. After a cursory glance at the nbxplorer code I think that nbxplorer requires none of the other default whitelist permissions (noban, mempool, relay). Details: dgarage/NBXplorer#344

jonasnick

ACK mod nit

jonasnick · 2021-10-21T11:50:45Z

modules/onion-services.nix

+
+      # When the bitcoind onion service is enabled, add an onion-tagged socket
+      # to distinguish local connections from Tor connections
+      services.bitcoind.onionPort = mkIf (cfg.bitcoind.enable or false) 8334;


what does "or false" do?

#247 (comment) 😉

it's just too much sugar for me

jonasnick

ACK ec4a4db

erikarvstedt marked this pull request as draft October 6, 2021 15:23

erikarvstedt force-pushed the bitcoind-onion-port branch 3 times, most recently from f11db15 to ec189d3 Compare October 6, 2021 15:29

nixbitcoin previously approved these changes Oct 13, 2021

View reviewed changes

erikarvstedt dismissed nixbitcoin’s stale review via 0a1fa66 October 15, 2021 13:57

erikarvstedt force-pushed the bitcoind-onion-port branch from ec189d3 to 0a1fa66 Compare October 15, 2021 13:57

erikarvstedt marked this pull request as ready for review October 15, 2021 13:58

erikarvstedt force-pushed the bitcoind-onion-port branch 5 times, most recently from 02c38ee to ffd2902 Compare October 15, 2021 16:20

nixbitcoin previously approved these changes Oct 20, 2021

View reviewed changes

jonasnick reviewed Oct 20, 2021

View reviewed changes

erikarvstedt dismissed nixbitcoin’s stale review via 1fdc5a1 October 21, 2021 09:36

erikarvstedt force-pushed the bitcoind-onion-port branch from ffd2902 to 1fdc5a1 Compare October 21, 2021 09:36

erikarvstedt added 2 commits October 21, 2021 11:40

bitcoind: add separate p2p socket for tor connections

df2070b

This re-enables onion tagging while still supporting untagged connections. Onion sockets are not yet supported in the latest liquidd/elements version 0.18.1.12 available on nixpkgs.

erikarvstedt force-pushed the bitcoind-onion-port branch from 1fdc5a1 to ec4a4db Compare October 21, 2021 09:40

jonasnick reviewed Oct 21, 2021

View reviewed changes

jonasnick approved these changes Oct 21, 2021

View reviewed changes

jonasnick merged commit bfe8ac9 into fort-nix:master Oct 21, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

bitcoind: add separate p2p socket for tor connections #405

bitcoind: add separate p2p socket for tor connections #405

erikarvstedt commented Oct 6, 2021 •

edited

Loading

nixbitcoin left a comment

erikarvstedt commented Oct 13, 2021

erikarvstedt commented Oct 15, 2021 •

edited

Loading

erikarvstedt commented Oct 15, 2021 •

edited

Loading

nixbitcoin left a comment

jonasnick Oct 20, 2021

erikarvstedt Oct 21, 2021

jonasnick Oct 21, 2021

erikarvstedt Oct 21, 2021 •

edited

Loading

jonasnick Oct 21, 2021

erikarvstedt Oct 21, 2021

jonasnick Oct 21, 2021

jonasnick left a comment

jonasnick Oct 21, 2021

erikarvstedt Oct 21, 2021

jonasnick Oct 21, 2021

jonasnick left a comment

bitcoind: add separate p2p socket for tor connections #405

bitcoind: add separate p2p socket for tor connections #405

Conversation

erikarvstedt commented Oct 6, 2021 • edited Loading

Main commit msg

nixbitcoin left a comment

Choose a reason for hiding this comment

erikarvstedt commented Oct 13, 2021

erikarvstedt commented Oct 15, 2021 • edited Loading

erikarvstedt commented Oct 15, 2021 • edited Loading

nixbitcoin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

erikarvstedt Oct 21, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonasnick left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonasnick left a comment

Choose a reason for hiding this comment

erikarvstedt commented Oct 6, 2021 •

edited

Loading

erikarvstedt commented Oct 15, 2021 •

edited

Loading

erikarvstedt commented Oct 15, 2021 •

edited

Loading

erikarvstedt Oct 21, 2021 •

edited

Loading