nixos/wireguard: setup interface with systemd-networkd #45392

dguibert · 2018-08-20T16:28:13Z

Motivation for this change

This patch make the wireguard interface more resilient and should fix #41874.

Things done

Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS)
Built on platform(s)
- NixOS
- macOS
- other Linux distributions
Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
Tested compilation of all pkgs that depend on this change using nix-shell -p nox --run "nox-review wip"
Tested execution of all binary files (usually in ./result/bin/)
Determined the impact on package closure size (by running nix path-info -S before and after)
Fits CONTRIBUTING.md.

Mic92

I have not tested the actual module, but the transition makes sense.

Mic92 · 2018-08-20T16:51:15Z

nixos/modules/system/boot/networkd.nix

+      type = types.addCheck (types.attrsOf unitOption) checkWireGuard;
+      description = ''
+        Each attribute in this set specifies an option in the
+        <literal>[WIREGUARD]</literal> section of the unit.  See


[WireGuard]

Mic92 · 2018-08-20T16:51:23Z

nixos/modules/system/boot/networkd.nix

+      type = with types; listOf (submodule wireguardPeerOptions);
+      description = ''
+        Each attribute in this set specifies an option in the
+        <literal>[WIREGUARDPEER]</literal> section of the unit.  See


[WireGuardPeer]

Mic92 · 2018-08-20T16:52:11Z

nixos/modules/system/boot/networkd.nix

+        type = types.addCheck (types.attrsOf unitOption) checkWireGuardPeer;
+        description = ''
+          Each attribute in this set specifies an option in the
+          <literal>[Route]</literal> section of the unit.  See


[WireguardPeer]

Mic92 · 2018-08-20T16:58:43Z

Now we would only need suppress_prefixlength 0 in [RoutingPolicyRule] to support routes for 0.0.0.0.

andir · 2018-08-21T06:14:28Z

nixos/modules/services/networking/wireguard.nix

    nameValuePair "wireguard-${name}"
      {
        description = "WireGuard Tunnel - ${name}";
-        after = [ "network.target" ];
+        after = [ "sys-subsystem-net-devices-${name}.device" ];


shouldn't we wait for DNS/network configuration?

I am not sure what the behavior ended up being (@Mic92) within systemd.

Maybe we should.
(btw systemd handles the resolution of endpoints)

I added exponential backoff for dns resolution, so it will retry failed endpoints just like openvpn: https://github.com/systemd/systemd/blob/master/src/network/netdev/wireguard.c#L268

It make sense to not wait for dns to avoid leaking traffic.

andir · 2018-08-21T18:01:17Z

OK 👍

…

On Tue, 21 Aug 2018, 11:42 Jörg Thalheim, ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In nixos/modules/services/networking/wireguard.nix <#45392 (comment)>: > nameValuePair "wireguard-${name}" { description = "WireGuard Tunnel - ${name}"; - after = [ "network.target" ]; + after = [ "sys-subsystem-net-devices-${name}.device" ]; I added exponential backoff for dns resolution, so it will retry failed endpoints just like openvpn: https://github.com/systemd/systemd/blob/master/src/network/netdev/wireguard.c#L268 It make sense to not wait for dns to avoid leaking traffic. — You are receiving this because your review was requested. Reply to this email directly, view it on GitHub <#45392 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAm_dFj8W4i0BSZpFk5Hc6-qBHoqnrfbks5uS9X3gaJpZM4WEUxh> .

globin

This misses creating routes, which was done before.

Patch for the inline comment on privateKey below.
Also added back postShutdown using ip set link down. We might not want to do this in case we handle routes with networkd, too.

From 854ce74e6eb0e7362ec9a749bdda3a83b7343795 Mon Sep 17 00:00:00 2001
From: Robin Gloster <mail@glob.in>
Date: Wed, 22 Aug 2018 04:01:51 +0200
Subject: [PATCH] wireguard: fix privateKey handling, add back postShutdown

---
 nixos/modules/services/networking/wireguard.nix | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/nixos/modules/services/networking/wireguard.nix b/nixos/modules/services/networking/wireguard.nix
index 43eecca1b6c..75301179706 100644
--- a/nixos/modules/services/networking/wireguard.nix
+++ b/nixos/modules/services/networking/wireguard.nix
@@ -225,14 +225,19 @@ let
         };
 
         script = ''
-          wg set ${name} private-key ${values.privateKeyFile}
+          ${optionalString (values.privateKeyFile != null) "wg set ${name} private-key ${values.privateKeyFile}"}
           ${concatMapStringsSep "\n" (peer:
-              optionalString (peer.presharedKeyFile != null) "wg set ${name} peer  preshared-key ${peer.presharedKeyFile}"
+              optionalString (peer.presharedKeyFile != null) "wg set ${name} peer preshared-key ${peer.presharedKeyFile}"
             ) values.peers}
 
           ip link set up dev ${name}
           ${values.postSetup}
         '';
+
+        preStop = ''
+          ip link set down dev ${name}
+          ${values.postShutdown}
+        '';
       };
 
 in
-- 
2.16.4

globin · 2018-08-22T01:27:54Z

nixos/modules/system/boot/networkd.nix

@@ -46,11 +46,26 @@ let
      "bond" "bridge" "dummy" "gre" "gretap" "ip6gre" "ip6tnl" "ip6gretap" "ipip"
      "ipvlan" "macvlan" "macvtap" "sit" "tap" "tun" "veth" "vlan" "vti" "vti6"
      "vxlan" "geneve" "vrf" "vcan" "vxcan" "wireguard" "netdevsim"
+      "wireguard"


wireguard is in the line above already

I've missed that. Thanks for pointing this out.

globin · 2018-08-22T02:50:50Z

nixos/modules/services/networking/wireguard.nix

-          wg set ${name} private-key ${privKey} ${
-            optionalString (values.listenPort != null) " listen-port ${toString values.listenPort}"}
-
+          wg set ${name} private-key ${values.privateKeyFile}


This should be wrapped in optionalString (values.privateKeyFile != null) to handle the use of privateKey

dguibert · 2018-08-22T14:06:55Z

@globin, I've applied your patch and restored the option allowedIPsAsRoutes.

globin · 2018-08-22T17:17:20Z

Ah I have one more minor nitpick to the route handling that already had existed before this PR. When adding routes without specifying proto, this makes some dynamic routing daemons ignore them (e.g. babel, probably more). Citing from ip(8):

If the routing protocol ID is not given, ip assumes protocol boot (i.e. it assumes the route was added by someone who doesn't understand what they are doing).

Patch to add proto static:

From 9e699b21199d2d05025c069efe60afd03546742a Mon Sep 17 00:00:00 2001
From: Robin Gloster <mail@glob.in>
Date: Wed, 22 Aug 2018 19:10:32 +0200
Subject: [PATCH] wireguard: make wg routes proto static

This fixes distributing routes with e.g. babel that ignores `proto boot`
---
 nixos/modules/services/networking/wireguard.nix | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/nixos/modules/services/networking/wireguard.nix b/nixos/modules/services/networking/wireguard.nix
index f6d45c9d357..46fcb03d32f 100644
--- a/nixos/modules/services/networking/wireguard.nix
+++ b/nixos/modules/services/networking/wireguard.nix
@@ -234,7 +234,7 @@ let
 
           ${optionalString (values.allowedIPsAsRoutes != false) (concatStringsSep "\n" (concatMap (peer:
               (map (allowedIP:
-                "ip route replace ${allowedIP} dev ${name} table ${values.table}"
+                "ip route replace ${allowedIP} dev ${name} table ${values.table} proto static"
               ) peer.allowedIPs)
             ) values.peers))}
 
-- 
2.16.4

globin · 2018-08-23T12:39:33Z

Also this will not work without having networking.useNetworkd enabled right? I don't think we can require people to use networkd with wireguard right now, there are still to many issues that need to be fixed..

useNetworkd concerns

Mic92 · 2018-08-23T12:53:20Z

It is possible to use systemd.network.enable without using networking.useNetworkd. Networkd only cares about interfaces it was explicitly configured for.

globin · 2018-08-23T12:58:15Z

Then this should mkDefault enable it :)

dguibert · 2018-08-23T16:49:42Z

Commits have been fix up to use systemd.network.enable and keep the DEVICE env. variable in the unit...

I've noticed that the unit is not reloaded when the device is reconfigured with networkd. The privateKey is not set anymore on the wireguard link after "systemctl restart system-networkd".
Ideas how to fix it?

Mic92 · 2018-08-23T17:02:00Z

Indeed, networkd will overwrite the key whenever it configures an interface https://github.com/systemd/systemd/blob/master/src/network/netdev/wireguard.c#L69

The only way this could be prevented would be to create an .netdev unit that has the private key included.
I think we should at least merge nixos/modules/system/boot/networkd.nix since they are independent from the module, I have no idea how to cleanly combine privateKeyFile and networkd module.

dguibert · 2018-08-24T09:45:12Z

For sure it could be prevented when the privateKey (and presharedKey) is defined instead of privateKeyFile.
The last patch aims to solve the reloading issue.

Mic92 · 2018-08-24T12:38:16Z

nixos/modules/services/networking/wireguard.nix

-          wg set ${name} private-key ${privKey} ${
-            optionalString (values.listenPort != null) " listen-port ${toString values.listenPort}"}
+	  # wait for ${name} to be configured
+	  while ! networkctl status $DEVICE | grep configured 1>/dev/null; do


Ugh, this is ugly. What if the users restart systemd-networkd manually?

Could this be fixed with PartOf=systemd-networkd.service?

Also the indentation is off in a few places.

Also then module should set the device up with ip after networkctl has configured the device. Does not networkd also set the link up correctly?

This could be fixed by waiting systemd-network-wait-online@${name}.service. How could we make this unit from systemd-network-wait-online@.service?

The while loop could be replaced with ${config.systemd.package}/lib/systemd/systemd-networkd-wait-online -i ${name}. If systemd-networkd is restarted, the keys are restored as expected.

Mic92 · 2018-08-24T12:39:38Z

nixos/modules/services/networking/wireguard.nix

    nameValuePair "wireguard-${name}"
      {
        description = "WireGuard Tunnel - ${name}";
-        after = [ "network.target" ];
+        after = [ "sys-subsystem-net-devices-${name}.device" "systemd-networkd-online" ];


The target is spelled wrong and I don't see why systemd-networkd-online.target is required.

Also use utils.escapeSystemdPath on name in the unit definition as systemd will escape e.g. hyphens with \x2d

Mic92 · 2018-08-24T14:05:01Z

nixos/modules/services/networking/wireguard.nix

+	  while ! networkctl status $DEVICE | grep configured 1>/dev/null; do
+	    sleep 1
+	  done
+          sleep 1


Why this sleep?

yorickvP · 2018-08-27T15:33:54Z

nixos/modules/services/networking/wireguard.nix

@@ -201,43 +230,30 @@ let
        };

        script = ''
-          modprobe wireguard


Do you load the module anywhere?

When an unknown netlink type is requested the kernel tries to load the kernel module:

╭─[~/git/nixpkgs]─[go-1_11] ╰─ % sudo rmmod wireguard ╭─[~/git/nixpkgs]─[go-1_11] ╰─ % lsmod | grep wireguard ╭─[~/git/nixpkgs]─[go-1_11] ╰─ % sudo ip l add dev foobar type wireguard ╭─[~/git/nixpkgs]─[go-1_11] ╰─ % lsmod | grep wireguard wireguard 229376 0 ip6_udp_tunnel 16384 1 wireguard udp_tunnel 16384 1 wireguard ipv6 503808 248 nf_conntrack_ipv6,bridge,ip6t_rpfilter,nf_defrag_ipv6,nf_nat_ipv6,wireguard

xeji · 2018-09-02T15:12:45Z

I merged #45569 because I am not sure this is ready yet, and we should get at least one of these improvements into 18.09. The merge does not imply a vote against this change.
It just needs a trivial rebase. Sorry for the inconvenience.

dguibert · 2018-09-04T09:43:04Z

I've rebased this branch.

I'm using it on my machines:

with networkd enabled.
with each privateKey coming from key file deployed with nixops (and not stored world-readable in nix store).

d6e · 2019-01-01T00:12:43Z

@dguibert This PR has been stuck for several months now. Is it just the merge conflict that's blocking this? Is there any way I can help?

dguibert · 2019-01-01T18:59:37Z

Just rebased. Works for me since I've pushed the first patch series.
Is there still anything blocking this to be merged?

arianvp · 2019-05-05T09:52:01Z

Heya. I recently got a patch merged in systemd that adds a PrivateKeyFile option to the netdev spec systemd/systemd#11861 . However we do not ship this yet but would be nice to add that to the future instead of the scripts.

Anyhow. I'd like to see this merged. So I'll happily take another pass to see if we didn't miss anything . I'll put it on my list.
Also ping @grahamc as he recently changed some things on master concerning keyfiles

asymmetric · 2019-05-05T10:43:51Z

This is the specific commit in the PR that makes the change, btw.

dguibert · 2019-05-07T12:11:00Z

Heya. I recently got a patch merged in systemd that adds a PrivateKeyFile option to the netdev spec systemd/systemd#11861 . However we do not ship this yet but would be nice to add that to the future instead of the scripts.

Great! I've just updated to whole branch with the last commit supporting this new option.

PS: Maybe a presharedKeyFile option could be also added to systemd ;-)

zarelit · 2019-08-07T07:45:15Z

How is this PR related to #64040 ? Should we keep both / merge them / close one?

dguibert · 2019-08-07T08:12:34Z

PR #64040 introduces wireguard related options as well as this PR, but there are some differences.

This PR converts the options config.networking.wireguard to networkd options, this part could be removed as it conflicts with some committed changes.
It supports multiple peers per interface (as far as I can tell not the new PR).

picnoir · 2019-08-09T19:07:45Z

It supports multiple peers per interface (as far as I can tell not the new PR).

It does support it.

If you prefer merging this one, please do add the nixos test I wrote in #64040 .

(cherry picked from commit ec073e4)

dguibert · 2019-08-21T09:43:08Z

It supports multiple peers per interface (as far as I can tell not the new PR).

It does support it.

I don't figure it out. I've tried to add this to the test

wireguardPeerConfig = { Endpoint = "192.168.1.3:51820"; PublicKey = pubk; PresharedKeyFile = pkgs.writeText "psk.key" "yTL3sCOL33Wzi6yCnf9uZQl/Z8laSE+zwpqOHC4HhFU="; AllowedIPs = [ "10.0.0.3/32" ]; PersistentKeepalive = 15; };

but error: attribute 'wireguardPeerConfig' at /nixpkgs/nixos/tests/systemd-networkd-wireguard.nix:26:13 already defined at /nixpkgs/nixos/tests/systemd-networkd-wireguard.nix:19:13

If you prefer merging this one, please do add the nixos test I wrote in #64040 .

done.

I've also removed the controversal part generating the systemd-networkd units from the options of networking.wireguard module.

picnoir · 2019-08-21T09:57:04Z

Thanks @dguibert. Closed the other one.

My bad for the multi-peer faulty behavior.

Looking forward to see this PR merged.

flokli

LGTM, thanks!

GrahamcOfBorg added 6.topic: nixos 8.has: module (update) 10.rebuild-darwin: 0 10.rebuild-linux: 0 labels Aug 20, 2018

Mic92 requested a review from andir August 20, 2018 16:47

Mic92 approved these changes Aug 20, 2018

View reviewed changes

Mic92 changed the title ~~wireguard via sytemd netlink~~ wireguard via systemd netlink Aug 20, 2018

Mic92 changed the title ~~wireguard via systemd netlink~~ nixos/wireguard: setup interface with systemd-networkd Aug 20, 2018

andir reviewed Aug 21, 2018

View reviewed changes

dguibert force-pushed the dg/wireguard branch from 3dfb0ac to 9bcdb7f Compare August 21, 2018 08:16

globin requested changes Aug 22, 2018

View reviewed changes

globin reviewed Aug 22, 2018

View reviewed changes

globin previously approved these changes Aug 22, 2018

View reviewed changes

dguibert force-pushed the dg/wireguard branch from 973e9a9 to 993282a Compare August 23, 2018 16:41

globin mentioned this pull request Aug 24, 2018

wireguard: change preStop to postStop, require network.target #45569

Merged

9 tasks

Mic92 reviewed Aug 24, 2018

View reviewed changes

yorickvP reviewed Aug 27, 2018

View reviewed changes

dguibert force-pushed the dg/wireguard branch from 7311162 to 868b377 Compare August 28, 2018 15:55

dguibert force-pushed the dg/wireguard branch from 868b377 to 6da208e Compare September 4, 2018 09:38

dguibert force-pushed the dg/wireguard branch from 6da208e to fe9ecb2 Compare January 1, 2019 18:54

dguibert requested a review from infinisil as a code owner January 1, 2019 18:54

GrahamcOfBorg added 10.rebuild-linux: 1-10 and removed 10.rebuild-linux: 0 labels Jan 1, 2019

dguibert force-pushed the dg/wireguard branch from fe9ecb2 to 59a5320 Compare May 7, 2019 12:06

ofborg bot added 10.rebuild-linux: 0 and removed 10.rebuild-linux: 1-10 labels May 7, 2019

dguibert force-pushed the dg/wireguard branch from 59a5320 to 36affe1 Compare May 21, 2019 09:54

dguibert and others added 2 commits August 21, 2019 11:11

systemd-networkd: add support for wireguard netdev.

7fd91a8

systemd-networkd: add tests

0528816

(cherry picked from commit ec073e4)

dguibert force-pushed the dg/wireguard branch from 36affe1 to 0528816 Compare August 21, 2019 09:46

picnoir mentioned this pull request Aug 21, 2019

systemd-networkd: Add wireguard-related options. #64040

Closed

10 tasks

flokli approved these changes Aug 21, 2019

View reviewed changes

flokli merged commit 9f237fe into NixOS:master Aug 21, 2019

nixos/wireguard: setup interface with systemd-networkd #45392

nixos/wireguard: setup interface with systemd-networkd #45392

Conversation

dguibert commented Aug 20, 2018

Motivation for this change

Things done

Mic92 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

Mic92 Aug 20, 2018 • edited

Choose a reason for hiding this comment

Mic92 commented Aug 20, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andir commented Aug 21, 2018 via email

globin left a comment • edited

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

Choose a reason for hiding this comment

dguibert commented Aug 22, 2018

globin commented Aug 22, 2018

globin commented Aug 23, 2018

Mic92 commented Aug 23, 2018

globin commented Aug 23, 2018

dguibert commented Aug 23, 2018

Mic92 commented Aug 23, 2018

dguibert commented Aug 24, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Mic92 Aug 24, 2018 • edited

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Mic92 Aug 27, 2018 • edited

Choose a reason for hiding this comment

xeji commented Sep 2, 2018

dguibert commented Sep 4, 2018

d6e commented Jan 1, 2019 • edited

dguibert commented Jan 1, 2019

arianvp commented May 5, 2019 • edited

asymmetric commented May 5, 2019

dguibert commented May 7, 2019

zarelit commented Aug 7, 2019

dguibert commented Aug 7, 2019

picnoir commented Aug 9, 2019

dguibert commented Aug 21, 2019 • edited

picnoir commented Aug 21, 2019

flokli left a comment

Choose a reason for hiding this comment

Mic92 Aug 20, 2018 •

edited

Mic92 commented Aug 20, 2018 •

edited

globin left a comment •

edited

Mic92 Aug 24, 2018 •

edited

Mic92 Aug 27, 2018 •

edited

d6e commented Jan 1, 2019 •

edited

arianvp commented May 5, 2019 •

edited

dguibert commented Aug 21, 2019 •

edited