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.

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

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

Merged
merged 2 commits into from Aug 21, 2019

Conversation

@dguibert
Copy link
Member

commented Aug 20, 2018

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
Mic92 approved these changes Aug 20, 2018
Copy link
Contributor

left a comment

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

nixos/modules/system/boot/networkd.nix Outdated
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

This comment has been minimized.

Copy link
@Mic92

Mic92 Aug 20, 2018

Contributor

[WireGuard]

This comment has been minimized.

Copy link
@dguibert

dguibert Aug 21, 2018

Author Member

done.

nixos/modules/system/boot/networkd.nix Outdated
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

This comment has been minimized.

Copy link
@Mic92

Mic92 Aug 20, 2018

Contributor

[WireGuardPeer]

nixos/modules/system/boot/networkd.nix Outdated
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

This comment has been minimized.

Copy link
@Mic92

Mic92 Aug 20, 2018

Contributor

[WireguardPeer]

@Mic92

This comment has been minimized.

Copy link
Contributor

commented Aug 20, 2018

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

@Mic92 Mic92 changed the title wireguard via sytemd netlink wireguard via systemd netlink Aug 20, 2018
@Mic92 Mic92 changed the title wireguard via systemd netlink nixos/wireguard: setup interface with systemd-networkd Aug 20, 2018
nixos/modules/services/networking/wireguard.nix Outdated
nameValuePair "wireguard-${name}"
{
description = "WireGuard Tunnel - ${name}";
after = [ "network.target" ];
after = [ "sys-subsystem-net-devices-${name}.device" ];

This comment has been minimized.

Copy link
@andir

andir Aug 21, 2018

Member

shouldn't we wait for DNS/network configuration?

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

This comment has been minimized.

Copy link
@dguibert

dguibert Aug 21, 2018

Author Member

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

This comment has been minimized.

Copy link
@Mic92

Mic92 Aug 21, 2018

Contributor

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.

@dguibert dguibert force-pushed the dguibert:dg/wireguard branch Aug 21, 2018
@andir

This comment has been minimized.

Copy link
Member

commented Aug 21, 2018

Copy link
Member

left a comment

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
nixos/modules/system/boot/networkd.nix Outdated
@@ -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"

This comment has been minimized.

Copy link
@globin

globin Aug 22, 2018

Member

wireguard is in the line above already

This comment has been minimized.

Copy link
@dguibert

dguibert Aug 22, 2018

Author Member

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

nixos/modules/services/networking/wireguard.nix Outdated
wg set ${name} private-key ${privKey} ${
optionalString (values.listenPort != null) " listen-port ${toString values.listenPort}"}

wg set ${name} private-key ${values.privateKeyFile}

This comment has been minimized.

Copy link
@globin

globin Aug 22, 2018

Member

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

This comment has been minimized.

Copy link
@dguibert

dguibert Aug 22, 2018

Author Member

sure.

@dguibert

This comment has been minimized.

Copy link
Member Author

commented Aug 22, 2018

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

@globin

This comment has been minimized.

Copy link
Member

commented Aug 22, 2018

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

This comment has been minimized.

Copy link
Member

commented Aug 23, 2018

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

@globin globin dismissed their stale review Aug 23, 2018

useNetworkd concerns

@Mic92

This comment has been minimized.

Copy link
Contributor

commented Aug 23, 2018

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

@globin

This comment has been minimized.

Copy link
Member

commented Aug 23, 2018

Then this should mkDefault enable it :)

@dguibert dguibert force-pushed the dguibert:dg/wireguard branch Aug 23, 2018
@dguibert

This comment has been minimized.

Copy link
Member Author

commented Aug 23, 2018

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

This comment has been minimized.

Copy link
Contributor

commented Aug 23, 2018

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

This comment has been minimized.

Copy link
Member Author

commented Aug 24, 2018

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.

nixos/modules/services/networking/wireguard.nix Outdated
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

This comment has been minimized.

Copy link
@Mic92

Mic92 Aug 24, 2018

Contributor

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

This comment has been minimized.

Copy link
@globin

globin Aug 24, 2018

Member

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

Also the indentation is off in a few places.

This comment has been minimized.

Copy link
@Mic92

Mic92 Aug 24, 2018

Contributor

Maybe?

This comment has been minimized.

Copy link
@Mic92

Mic92 Aug 24, 2018

Contributor

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 comment has been minimized.

Copy link
@dguibert

dguibert Aug 24, 2018

Author Member

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.

nixos/modules/services/networking/wireguard.nix Outdated
nameValuePair "wireguard-${name}"
{
description = "WireGuard Tunnel - ${name}";
after = [ "network.target" ];
after = [ "sys-subsystem-net-devices-${name}.device" "systemd-networkd-online" ];

This comment has been minimized.

Copy link
@Mic92

Mic92 Aug 24, 2018

Contributor

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

This comment has been minimized.

Copy link
@globin

globin Aug 24, 2018

Member

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

nixos/modules/services/networking/wireguard.nix Outdated
while ! networkctl status $DEVICE | grep configured 1>/dev/null; do
sleep 1
done
sleep 1

This comment has been minimized.

Copy link
@Mic92

Mic92 Aug 24, 2018

Contributor

Why this sleep?

nixos/modules/services/networking/wireguard.nix Outdated
@@ -201,43 +230,30 @@ let
};

script = ''
modprobe wireguard

This comment has been minimized.

Copy link
@yorickvP

yorickvP Aug 27, 2018

Contributor

Do you load the module anywhere?

This comment has been minimized.

Copy link
@Mic92

Mic92 Aug 27, 2018

Contributor

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
@dguibert dguibert force-pushed the dguibert:dg/wireguard branch Aug 28, 2018
@xeji

This comment has been minimized.

Copy link
Contributor

commented Sep 2, 2018

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 dguibert force-pushed the dguibert:dg/wireguard branch to 6da208e Sep 4, 2018
@dguibert

This comment has been minimized.

Copy link
Member Author

commented Sep 4, 2018

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

This comment has been minimized.

Copy link

commented Jan 1, 2019

@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 dguibert force-pushed the dguibert:dg/wireguard branch from 6da208e to fe9ecb2 Jan 1, 2019
@dguibert dguibert requested a review from Infinisil as a code owner Jan 1, 2019
@dguibert

This comment has been minimized.

Copy link
Member Author

commented Jan 1, 2019

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

@arianvp

This comment has been minimized.

Copy link
Contributor

commented May 5, 2019

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

This comment has been minimized.

Copy link
Contributor

commented May 5, 2019

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

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

This comment has been minimized.

Copy link
Member Author

commented May 7, 2019

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

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

This comment has been minimized.

Copy link
Member

commented Aug 7, 2019

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

@dguibert

This comment has been minimized.

Copy link
Member Author

commented Aug 7, 2019

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

This comment has been minimized.

Copy link
Contributor

commented Aug 9, 2019

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 .

dguibert and others added 2 commits Aug 16, 2018
@dguibert

This comment has been minimized.

Copy link
Member Author

commented Aug 21, 2019

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.

@dguibert dguibert force-pushed the dguibert:dg/wireguard branch from 36affe1 to 0528816 Aug 21, 2019
@NinjaTrappeur NinjaTrappeur referenced this pull request Aug 21, 2019
8 of 10 tasks complete
@NinjaTrappeur

This comment has been minimized.

Copy link
Contributor

commented Aug 21, 2019

Thanks @dguibert. Closed the other one.

My bad for the multi-peer faulty behavior.

Looking forward to see this PR merged.

@flokli
flokli approved these changes Aug 21, 2019
Copy link
Contributor

left a comment

LGTM, thanks!

@flokli flokli merged commit 9f237fe into NixOS:master Aug 21, 2019
12 checks passed
12 checks passed
Evaluation Performance Report Evaluator Performance Report
Details
grahamcofborg-eval ^.^!
Details
grahamcofborg-eval-check-meta config.nix: checkMeta = true
Details
grahamcofborg-eval-darwin nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A darwin-tested
Details
grahamcofborg-eval-nixos nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release-combined.nix -A tested
Details
grahamcofborg-eval-nixos-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release.nix -A manual
Details
grahamcofborg-eval-nixos-options nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release.nix -A options
Details
grahamcofborg-eval-nixpkgs-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A manual
Details
grahamcofborg-eval-nixpkgs-tarball nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A tarball
Details
grahamcofborg-eval-nixpkgs-unstable-jobset nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A unstable
Details
grahamcofborg-eval-package-list nix-env -qa --json --file .
Details
grahamcofborg-eval-package-list-no-aliases nix-env -qa --json --file . --arg config { allowAliases = false; }
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.