Network Namespaces #187

nixbitcoin · 2020-05-29T14:13:36Z

Network Namespaces are a significant security improvement, mitigating the bind race leak security vulnerability and unnecessary communications between nix-bitcoin services. Network namespaces round-off our service isolation to cover all levels of inter-service interaction.

Special thank you to @erikarvstedt for generalizing the makeNetnsServices 🚀

Kixunil · 2020-05-29T18:03:16Z

Regarding 4) I solved a similar problem for lncli by writing a wrapper script and named it lncli using update-alternatives. I plan to do the same for bitcoin-cli Maybe a similar approach can work here?

mmilata

Haven't tested or thought much about the actual functionality but left a few nitpicks regarding implementation.

mmilata · 2020-06-01T12:26:14Z

modules/netns-chef.nix

+  netns = builtins.mapAttrs (n: v: {
+    inherit (v) id;
+    address = "169.254.${toString cfg.addressblock}.${toString v.id}";
+    availableNetns = builtins.filter (x: config.services.${x}.enable) availableNetns.${n};


Perhaps you can call isEnabled here?

How is this better? @erikarvstedt wrote this function, mabye he can take a look. I'm not competent enough yet to fully understand it.

Implemented under d3a8abc

mmilata · 2020-06-01T12:57:45Z

modules/netns-chef.nix

+
+    nix-bitcoin.netns-isolation.services = {
+      bitcoind = {
+        id = 12;


Is it desirable for two services to have the same id, or do the ids need to be persistent? If not you should be able to assign the ids automatically, e.g. by defining serviceName -> id map like:

idMap = builtins.listToAttrs (lib.imap1 (num: srvName: lib.nameValuePair srvName num) (lib.attrNames cfg.services))

Why would you want to do that? It makes everything simpler IMO to just have hardcoded addresses in a changeable addressblock.

mmilata · 2020-06-01T13:02:17Z

modules/netns-chef.nix

+  isEnabled = x: config.services.${x}.enable;
+
+  ip = "${pkgs.iproute}/bin/ip";
+  iptables = "${pkgs.iptables}/bin/iptables";


Consider using config.networking.firewall.package instead for nftables. (Though if NixOS/nixpkgs#81172 should make this uneccessary.)

Is nftables better than iptables for what we're trying to do?

Sorry, I meant to write "... instead for compatibility with nftables". Using config.networking.firewall.package instead of a concrete package would make it work regardless of whether the system is set to use iptables or nftables.

IMO nftables is the future but both work fine for this use case.

See fixup! architecture: pkgs.iptables -> config.networking.firewall.package

mmilata · 2020-06-01T13:08:02Z

modules/netns-chef.nix

+    networking.firewall.interfaces.br0.allowedTCPPorts = [ 9050 ];
+    boot.kernel.sysctl."net.ipv4.ip_forward" = true;
+
+    nix-bitcoin.netns-isolation.services = {


For the sake of modularity it might be preferable to define the implementation in this file and then set the concrete mapping and other parameters elsewhere, e.g. in secure-node.nix?

@jonasnick and I talked about restructuring this file like so: parameters in each individual module, mapping in secure-node.nix and general implementation in netns-chef.nix. What are your thoughts on this approach?

Sounds reasonable!

nixbitcoin · 2020-06-01T16:53:16Z

Regarding 4) I solved a similar problem for lncli by writing a wrapper script and named it lncli using update-alternatives. I plan to do the same for bitcoin-cli Maybe a similar approach can work here?

We've used wrappers before, check out this. The problem is that ip netns exec requires elevated privileges, which I don't think is a good idea to give the operator user. If the operator user can run ip netns exec with root privileges, he can run any following command with root privileges.

I've been thinking about how to solve 4. and I think the best idea is to create a separate nb-operator namespace with access to all other namespaces. All the user scripts will automatically execute with the right flags, like -rpcconnect for bitcoin-cli. When the operator user ssh's into the box he's automatically placed into a shell inside the nb-operator network namespace. Thoughts?

mmilata · 2020-06-01T17:56:57Z

What about setcap wrapper? Namespaces require CAP_SYS_ADMIN which is pretty much root but I guess it's marginally better than the sudo rule and the capability won't be inherited by the program passed to ip netns exec as an argument?

erikarvstedt · 2020-06-01T19:53:39Z

Please rename netns-chef.nix -> netns-isolation.nix. chef is amusing in the context of onions, but it's inappropriate here.

jonasnick

Concept ACK. Really cool and thanks to abstractions much simpler than I had imagined.

modules/netns-chef.nix

jonasnick · 2020-06-03T15:48:45Z

modules/netns-chef.nix

+      default = {};
+      type = types.attrsOf (types.submodule {
+        options = {
+          id = mkOption { type = types.int; };


Description would be helpful. "must be unique", "must not be 10"

See fixup! id option: add description

modules/netns-chef.nix

modules/bitcoind.nix

jonasnick · 2020-06-03T15:59:59Z

It would be helpful to document how this works and in particular how this can be customized to allow access through clearnet or wireguard interfaces. As an example here's the diagram I sketched while trying to understand what is happening.

nixbitcoin · 2020-06-04T18:55:41Z

I force pushed a new and improved HEAD that includes lnd, an operator namespace, and several fixes. It also removes the clightning autolisten option, as documented in the commit.

These todos remain:

I've tried implementing these and have gotten errors that I can't solve (please help @erikarvstedt @jonasnick):

Restructure the custom netns configs so they are only set when the individual services are actually enabled.
Make bitcoind-import-banlist run in the nb-bitcoind namespace.

I need your conceptual input on these:

Think about placing custom netns configs in each individual module, mapping in secure-node.nix and general implementation in netns-isolation.nix.
Drop the operator user into a shell inside nb-operator and give him aliases that include flags like rpcconnect. Something like ip netns exec nb-operator sudo -u operator bash should do.
Mitigate or document LND's breaking changes.
Find a better way to amend the LND cert than hardcoding the ip address.
Update the testing framework to test with and without network namespaces.

I can take care of these but they depend on the above:

Document clearnet and wireguard access.

nixbitcoin · 2020-06-10T17:25:55Z

Force pushed a new and improved HEAD, that fixes most issues.

Only three issues remaining are

Mitigate or document LND's breaking changes, including finding a better way to amend the LND cert than hardcoding the lnd ip address.
Update the testing framework to test with and without network namespaces.
Give operator user aliases that include flags like rpcconnect inside his login shell.

I will document clearnet and wireguard access in a later PR. Out of scope for now.

nixbitcoin · 2020-06-18T11:41:43Z

I got rid of the operator shell and replaced it with custom cli scripts that use Jonas Nick's netns-exec to execute bitcoin-cli, lncli, and elements-cli in the appropriate network namespaces. Operator can use netns-exec because of a security.wrappers program with cap_sys_admin.

I also rewrote the test_script.py to use netns-isolation. If we want to test without netns-isolation we need to write a second test_script.py. Local tests run nicely now (something is wrong with Travis again).

Only remaining issues are the breaking changes to lnd (hardcoded ip address in cert and new hidden service).

Very close to mergeable IMO.

nixbitcoin · 2020-06-26T12:16:17Z

Fixed some nits.

test/test-script.py

modules/netns-isolation.nix

jonasnick

The netns-exec operator access works well. And nice to see that you managed to get the tests to pass.

I'm not concerned about the breaking changes to lnd. The hardcoded ip isn't a problem unless in very very rare cases. We mention the new onion adress and the requirement in the release notes and can make a minor version bump to signal that users may have to take action.

I'd suggest to disable this module by default for now. That way we can merge this PR with little risk and work on reliability and usability until we make it default. To do this effectively it would be really helpful to be able to test with and without netns isolation.

modules/bitcoind.nix

jonasnick · 2020-07-08T18:17:02Z

modules/bitcoind.nix

@@ -68,6 +70,13 @@ in {
        default = "/var/lib/bitcoind";
        description = "The data directory for bitcoind.";
      };
+      host = mkOption {


I feel like we don't really need these host options. In some modules (like bitcoind) they aren't even used. As far as I can see they are mainly used for the hidden service toHost option. For clightning for example we can directly use bind-addr. For lnd we can keep host (or rename to listen, because that's how the option is called in lnd). For bitcoind we can add a bind option (we should do that anyway) which if set will result in an additional line bind=${...} in the bitcoind config.

Fixed where possible. Nanopos, lightning-charge and spark-wallet actually have a host option, so naming is correct. It's needed in electrs for now to direct traffic to the nginx netns, can be removed when #203 is merged.

jonasnick · 2020-07-08T18:21:24Z

modules/netns-isolation.nix

+              ${ip} link set br-${vethName} master br0
+              ${ipNetns} route add default via ${bridgeIp}
+              ${netnsIptables} -w -P INPUT DROP
+              ${netnsIptables} -w -A INPUT -s 127.0.0.1,${bridgeIp} -j ACCEPT


It should be possible to allow the ipNetns to connect to ipNetns. In order to allow that I think we need to add ${ipNetns} here and in the OUTPUT rule below. That allows simplifying the tests by testing connectivity from the same container. Also we can remove additional connection from clightning to spark in the tests.

Fixed in 43a34be

nixbitcoin · 2020-07-09T15:49:19Z

Rebased and fixed @jonasnick comments.

a2dc3da notes

Disabled module by default
Inserted ${ipNetns} firewall rule
Removed unnecessary toStrings
Removed unnecessary DOLLAR_CURLY's
Defaulted bitcoin cli to cli-nonetns-exec
Got rid of host options and used service-specific options
Added rpclisten and restlisten options of type types.listOf types.str to lnd module
Fixed missing comment in test_script.py
Simplified spark-wallet test by testing from same container
Set ping deadline
Implemented must succeed pings
Implemented netns-exec tests
- That it drops capabilities
- That it can't execute in bad namespaces

Running tests with and without netns-isolation requires creating something like

nodes =
    { machine1 =
        { config, pkgs, ... }: { … };
      machine2 =
        { config, pkgs, ... }: { … };
      …
    };

where nix-bitcoin.netns-isolation.enable = mkForce true; for one of the machines. test-script.py would have to be rewritten to perform tests on both machines concurrently. I think @erikarvstedt is the best person to implement this elegantly.

jonasnick · 2020-07-15T16:11:32Z

test/test-script.py

+
+# Positive ping tests
+machine.succeed(
+    "ip netns exec nb-bitcoind ping -c 1 -w 1 169.254.1.12 && \


Can you define constants for these addresses? That would give them a name and make it easy to see what they're referring to.

Fixed in 9d7181b5aaadf326728a3a5c9b255f0cae36598c

jonasnick · 2020-07-15T16:11:57Z

test/test-script.py

+)
+
+# netns-exec tests
+machine.fail("netns-exec nb-electrs ip a")


Can you add a comment to explain what this does and why this must fail?

And how about checking that netns-exec can not be executed by users that are not operator?

Both fixed in 9d7181b5aaadf326728a3a5c9b255f0cae36598c

jonasnick · 2020-07-15T16:13:01Z

test/test-script.py

+machine.fail("netns-exec nb-electrs ip a")
+assert_matches_exactly(
+    "su operator -c 'netns-exec nb-bitcoind capsh --print | grep Current | tr -d \"\\n\"'",
+    "Current: =",


Can't you match against "Current: =\n" instead of using tr?

Fixed in a3aaff1

nixbitcoin · 2020-07-20T13:19:02Z

Moved nix-bitcoin.netns-isolation.enable to configuration.nix

c program allows executing commands in nb-bitcoind, nb-lnd, nb-liquidd (the netns's needed for operator cli scripts).

- Adds network namespace instantiation and routing architecture. - netns-isolation disabled by default. Can be enabled with configuration.nix FIXME. - Uses mkMerge to toggle certain options for non netns and netns systems. - Adds security wrapper for netns-exec which allows operator to exec with cap_sys_admin - User can select the 169.254.N.0/24 addressblock netns's are created in. - nix-bitcoin-services IpAddressAllow is amended with link-local addresses

- Adds bitcoind to netns-isolation.services - Adds rpcbind and rpcallowip options to allow using bitcoind with network namespaces - Adds bind option (defaults to localhost), used as target of hidden service - Makes bitcoind-import-banlist run in netns

nonetns script needed for bitcoind-import-banlist

From the clightning manpage: autolisten=BOOL By default, we bind (and maybe announce) on IPv4 and IPv6 interfaces if no addr, bind-addr or announce-addr options are specified. Setting this to false disables that. We already set bind-addr by default, so autolisten had no effect. Therefore, this commit replaces autolisten with the more granular announce-addr option. For now we are Tor-only, so we only need to announce our hidden service to accept incoming connections. In the future, we can add clearnet connectivity with `addr` and route connections into our netns with NAT.

Simplifies the clightning module.

- Adds clightning to netns-isolation.services - Adds bitcoin-rpcconnect option to allow using clightning with network namespaces - Uses bind-addr option (defaults to localhost) as target of hidden service - Adds different bind-addr options depending on if netns-isolation is enabled or not.

- Adds bitcoind-host, and tor-socks options to allow using with network namespaces. - Adds listen, rpclisten, and restlisten option to specify host on which to listen on for peer, rpc and rest connections respectively - Adds announce-tor option and generates Tor Hidden Service with nix instead of lnd to bring in line with clightning. WARNING: Breaking changes for Tor Hidden Service. Manual migration necessary.

- Adds lnd to netns-isolation.services - Specifies listen option (defaults to localhost) as target of hiddenService. - Amends hardcoded lnd ip to lnd-cert WARNING: Breaking changes for lnd cert. lnd-key and lnd-cert will have to be deleted and redeployed.

- Adds liquidd to netns-isolation.services - Adds rpcbind, rpcallowip, and mainchainrpchost options to allow using liquidd with network namespaces - Adds bind option (defaults to localhost) as target of hidden service

- Adds electrs to netns-isolation.services - Adds daemonrpc option and specifies address option to allow using electrs with network namespaces - Adds host option (defaults to localhost) as target of hidden service

- Adds spark-wallet to netns-isolation.services - Adds extraArgs option to allow using spark-wallet with network namespaces - Adds host option (defaults to localhost) as target of hidden service - Adds enforceTor option to bring in line with other services

- Adds lightning-charge to netns-isolation.services - Adds cfg.enforceTor to bring lightning-charge in line with other services - Adds extraArgs option to allow using lightning-charge with network namespaces - Adds host option (defaults to localhost) as target of hidden service

- Adds nanopos to netns-isolation.services - Adds cfg.enforceTor and extraArgs to bring nanopos in line with other services - Adds charged-url option to allow using nanopos with network namespaces. - Modularizes nginx so webindex can be used without nanopos. - Adds host option (defaults to localhost) as target of hidden service - Removes unnecessary after

- Adds recurring-donations to netns-isolation.services - Adds cfg.enforceTor to bring recurring-donations in line with other services - Removes torsocks dependency in favor of `curl --socks-hostname`

- Adds nginx to netns-isolation.services - Adds host option (defaults to localhost) as target of hidden service

nixbitcoin · 2020-07-21T09:44:41Z

Fixed final nits

jonasnick

ACK 6817282

epic merge is epic

mmilata reviewed Jun 1, 2020

View reviewed changes

nixbitcoin added this to the Remove "experimental" from README.md milestone Jun 3, 2020

jonasnick reviewed Jun 3, 2020

View reviewed changes

nixbitcoin force-pushed the netns branch from fe2159f to 25eb6f4 Compare June 4, 2020 18:53

nixbitcoin force-pushed the netns branch from 25eb6f4 to d3a8abc Compare June 10, 2020 17:21

nixbitcoin force-pushed the netns branch from d3a8abc to c3ad8ef Compare June 18, 2020 11:36

nixbitcoin force-pushed the netns branch from c3ad8ef to f7de5bc Compare June 26, 2020 12:12