Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

bitcoind: add separate p2p socket for tor connections #405

Merged
merged 2 commits into from
Oct 21, 2021
Merged

bitcoind: add separate p2p socket for tor connections #405

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
df2070b
bitcoind: add separate p2p socket for tor connections
erikarvstedt Oct 15, 2021
ec4a4db
btcpayserver: fix whitelist security issue
erikarvstedt Oct 15, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 12 additions & 2 deletions modules/bitcoind.nix
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,14 @@ let
default = 8333;
description = "Port to listen for peer connections.";
};
onionPort = mkOption {
type = types.nullOr types.port;
default = null;
description = ''
Port to listen for Tor peer connections.
If set, inbound connections to this port are tagged as onion peers.
'';
};
getPublicAddressCmd = mkOption {
type = types.str;
default = "";
Expand Down Expand Up @@ -253,8 +261,10 @@ let
${optionalString (cfg.assumevalid != null) "assumevalid=${cfg.assumevalid}"}

# Connection options
${optionalString cfg.listen "bind=${cfg.address}"}
port=${toString cfg.port}
${optionalString cfg.listen
"bind=${cfg.address}:${toString cfg.port}"}
${optionalString (cfg.listen && cfg.onionPort != null)
"bind=${cfg.address}:${toString cfg.onionPort}=onion"}
${optionalString (cfg.proxy != null) "proxy=${cfg.proxy}"}
listen=${if cfg.listen then "1" else "0"}
${optionalString (cfg.discover != null) "discover=${if cfg.discover then "1" else "0"}"}
Expand Down
5 changes: 1 addition & 4 deletions modules/btcpayserver.nix
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -119,7 +119,7 @@ in {
# Enable p2p connections
listen = true;
extraConfig = ''
whitelist=${nbLib.address cfg.nbxplorer.address}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Collaborator Author

@erikarvstedt erikarvstedt Oct 21, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should I add it to this PR?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

whitelist=download@${nbLib.address cfg.nbxplorer.address}
'';
};
services.clightning.enable = mkIf (cfg.btcpayserver.lightningBackend == "clightning") true;
Expand All @@ -128,9 +128,6 @@ in {
enable = true;
# Enable p2p connections
listen = true;
extraConfig = ''
whitelist=${nbLib.address cfg.nbxplorer.address}
'';
};

services.lnd.macaroons.btcpayserver = mkIf (cfg.btcpayserver.lightningBackend == "lnd") {
Expand Down
8 changes: 6 additions & 2 deletions modules/onion-services.nix
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ let
default = config.public;
description = ''
Create an onion service for the given service.
The service must define options 'address' and 'port'.
The service must define options 'address' and 'onionPort' (or `port`).
'';
};
public = mkOption {
Expand Down Expand Up @@ -64,7 +64,7 @@ in {
inherit (cfg.${name}) externalPort;
in nbLib.mkOnionService {
port = if externalPort != null then externalPort else service.port;
target.port = service.port;
target.port = service.onionPort or service.port;
target.addr = nbLib.address service.address;
}
);
Expand Down Expand Up @@ -118,6 +118,10 @@ in {
externalPort = 80;
};
};

# 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;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what does "or false" do?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#247 (comment) 😉

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's just too much sugar for me

}
];
}