wireguard: add module #17933

ericsagnes · 2016-08-23T04:40:52Z

Motivation for this change

WIP PR for adding a wireguard module.

Things done

Tested using sandboxing
(nix.useChroot on NixOS,
or option build-use-chroot in nix.conf
on non-NixOS)
Built on platform(s)
- NixOS
- OS X
- Linux
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/)
Fits CONTRIBUTING.md.

mention-bot · 2016-08-23T04:40:56Z

@ericsagnes, thanks for your PR! By analyzing the annotation information on this pull request, we identified @edolstra, @bjornfor and @offlinehacker to be potential reviewers

zx2c4 · 2016-08-23T11:18:24Z

nixos/modules/services/networking/wireguard.nix

+        default = null;
+        example = "rVXs/Ni9tu3oDBLS4hOyAUAa1qTWVA3loR8eL20os3I=";
+        type = with types; nullOr str;
+        description = "Preshared key key used by the interface.";


You might want to update the description to say, "Optional preshared key used by the interface; most users will not need this."

zx2c4 · 2016-08-23T11:34:00Z

Hi -- WireGuard author here. This is nowhere near ready to be merged. I've made several comments inline with the source to review. There's also one big thing missing here:

If the IP of the interface is 192.168.2.1/24, and the allowed IPs are 192.168.2.0/24 or a list of anything that falls within 192.168.2.0/24, then you don't need to do anything. However, if the allowed IPs contain something laying outside of 192.168.2.0/24, such as 10.0.0.0/16, then you need to add those routes to the routing table: ip route add 10.0.0.0/16 dev wg0 in the unit. If you don't understand why this is necessary, send me an email and we'll talk about it on the phone.

ericsagnes · 2016-08-23T11:50:12Z

@zx2c4 Thanks for all the inputs, that is very appreciated!

It is indeed a rough sketch made in very short time to have some base to talk and work on.
It is still a WIP so there is no risk it gets merged :)

ericsagnes · 2016-08-23T13:09:50Z

Added a commit to clean things up a little, not perfect but still an improvement.

However, if the allowed IPs contain something laying outside of 192.168.2.0/24, such as 10.0.0.0/16, then you need to add those routes to the routing table: ip route add 10.0.0.0/16 dev wg0 in the unit.

Nix expression language is not really a general purpose language and doing a such thing programmatically could end in a little complicated an bloated code.
For the moment I chose the easy alternative to add an extraRoutes option so the user can setup the extra routes they need.

zx2c4 · 2016-08-23T13:16:53Z

Nix expression language is not really a general purpose language and doing a such thing programmatically could end in a little complicated an bloated code. For the moment I chose the easy alternative to add an extraRoutes option so the user can setup the extra routes they need.

This is completely unacceptable. Remove this option immediately. It's not okay, complicates things, and makes no sense.

If you can't do any real computation in the language, just add all the allowed-ips as routes. This is equally as valid and the linux routing table will sort things out for you. I don't know this language, but something like this should do:

foreach peer in values.peers:
${concatStrings (map (ip: ''
ip route add ${ip} dev ${name}
'') peer.allowed-ips)}

ericsagnes · 2016-08-23T14:26:38Z

One last quick commit to remove unacceptable and insanity :)

I have no much more time to work on this this week, so if you are in a hurry please make a PR to this branch, sorry.

zx2c4 · 2016-08-23T14:29:46Z

Changes look good. Will wait til next week to see what's next. Thanks for your hard work on this!

ericsagnes · 2016-08-23T14:51:38Z

Regarding the unit ExecStart, it can run only one command unless the unit type is oneshot. (upstream documentation)

preStart and postStart are a pretty common pattern in NixOS when running multiple commands is needed. (see for example dhcpd)

zx2c4 · 2016-08-23T14:58:28Z

Yes. In this case you do want Type=oneshot. It's not a daemon like dhcpd. It's a oneshot set of commands. It's similar to any of these units, which are also oneshot:

$ grep -rl oneshot
nixos/modules/virtualisation/rkt.nix
nixos/modules/virtualisation/azure-agent.nix
nixos/modules/virtualisation/libvirtd.nix
nixos/modules/virtualisation/brightbox-image.nix
nixos/modules/virtualisation/google-compute-image.nix
nixos/modules/virtualisation/azure-image.nix
nixos/modules/virtualisation/ec2-data.nix
nixos/modules/virtualisation/virtualbox-host.nix
nixos/modules/virtualisation/containers.nix
nixos/modules/services/databases/redis.nix
nixos/modules/services/computing/torque/mom.nix
nixos/modules/services/computing/torque/server.nix
nixos/modules/services/hardware/amd-hybrid-graphics.nix
nixos/modules/services/hardware/nvidia-optimus.nix
nixos/modules/services/hardware/tlp.nix
nixos/modules/services/amqp/activemq/default.nix
nixos/modules/services/networking/softether.nix
nixos/modules/services/networking/networkmanager.nix
nixos/modules/services/networking/firewall.nix
nixos/modules/services/networking/fan.nix
nixos/modules/services/networking/nat.nix
nixos/modules/services/web-servers/tomcat.nix
nixos/modules/services/network-filesystems/nfsd.nix
nixos/modules/services/system/cloud-init.nix
nixos/modules/services/security/fprot.nix
nixos/modules/services/mail/dspam.nix
nixos/modules/services/audio/mopidy.nix
nixos/modules/services/audio/alsa.nix
nixos/modules/services/cluster/kubernetes.nix
nixos/modules/services/monitoring/ups.nix
nixos/modules/services/monitoring/apcupsd.nix
nixos/modules/services/monitoring/das_watchdog.nix
nixos/modules/services/desktops/profile-sync-daemon.nix
nixos/modules/services/misc/gitolite.nix
nixos/modules/services/misc/sundtek.nix
nixos/modules/tasks/cpu-freq.nix
nixos/modules/tasks/swraid.nix
nixos/modules/tasks/filesystems.nix
nixos/modules/tasks/network-interfaces.nix
nixos/modules/tasks/filesystems/zfs.nix
nixos/modules/tasks/network-interfaces-scripted.nix
nixos/modules/system/boot/systemd-unit-options.nix
nixos/modules/system/boot/kexec.nix
nixos/modules/system/boot/networkd.nix
nixos/modules/system/boot/shutdown.nix
nixos/modules/system/boot/systemd.nix
nixos/modules/security/acme.nix
nixos/modules/security/audit.nix
nixos/modules/security/grsecurity.nix
nixos/modules/security/apparmor.nix
nixos/modules/programs/xfs_quota.nix
nixos/modules/installer/tools/auto-upgrade.nix
nixos/modules/config/swap.nix
nixos/modules/config/zram.nix
nixos/modules/config/power-management.nix

fpletz · 2016-08-24T20:27:14Z

Wow, an upstream author reviewing a NixOS module… that's a first. Thanks a lot for your feedback @zx2c4! 🍻

zx2c4 · 2016-08-24T22:15:58Z

nixos/modules/services/networking/wireguard.nix

+  generateUnit = name: values: 
+    nameValuePair "wireguard-${name}" 
+      {
+        description = "Wireguard unit for interface ${name}";


Not a very nice description. Try instead:

WireGuard Tunnel - ${name}

zx2c4 · 2016-08-24T22:18:38Z

@fpletz My pleasure. 😜

The latest changes look mostly good to me, but still a few things are funky:

I don't think that a user setting preSetup or postSetup should result in those sections overriding what's currently there. Instead, what's currently in preSetup and postSetup should just be moved to the main body section. Then, when a user uses preSetup and postSetup, it will be respectively prepended and appended.
The echo bringing the interface! lines look ugly to me, as journal should already be logging the actual command executions. If this is the way NixOS modules generally work, and you're just following convention, then okay. But if not, it seems unnecessary.

Putting these two points together yields this rearranged section:

        script = ''
              ${values.preSetup}
              ip link del dev ${name} 2>/dev/null || true
              ip link add dev ${name} type wireguard
              wg setconf "${name}" ${generateConf name values}
              ${concatStrings (map (ip: ''
              ip address add ${ip} dev "${name}"
              '') values.ips)}
              ip link set up dev "${name}"
              ${concatStrings (flatten (map (peer: (map (ip: ''
              ip route add ${ip} dev "${name}"
              '') peer.allowedIPs)) values.peers))}
              ${values.postSetup}
            '';

The descriptions of postSetup and preSetup should then be updated to reflect this change.

This is my personal opinion on the matter, but if NixOS developers have a good reason for the override behavior instead, I guess that's fine. My only suggestion, then, would be to call it overridePostSetup and overridePreSetup or the like.

ericsagnes · 2016-08-28T16:05:34Z

@zx2c4 Thanks for the feedback, updated the commit according to your last comment.

I added a basic example to the interfaces option, let me know if you want to change it to with more appropriate values.

Do you have any suggestion of relevant examples for preSetup and postSetup?

zx2c4 · 2016-08-29T13:31:26Z

Hey @ericsagnes -- excellent work! Something I missed before, that disappeared in the reworkings is that there's no longer an ExecStop. ExecStop should definitely run ip link del dev ${name}.

Unrelated question: How does script work in NixOS? Does this actually get wired up to ExecStart? What's the advantage of doing it how you have it rather than having a series of ExecStart lines?

As for your questions:

It might be less confusing to have for the example these IPs instead:

...
ips = [192.168.20.4/24]
...
peers = {
   allowed-ips = [192.168.20.1/32]
...
}

One use of postSetup that might be useful is adding a custom DNS server, depending on how the user likes to manage DNS. For example, on my system I use resolvconf a silly little tool. After bringing up a WireGuard interface I usually run something like printf 'nameserver 10.200.100.1' | resolvconf -a wg0 -m 0, and when shutting the interface down (postShutdown?) I usually run something like resolvconf -d wg0. But that's just me.

ericsagnes · 2016-08-29T20:04:55Z

How does script work in NixOS? Does this actually get wired up to ExecStart? What's the advantage of doing it how you have it rather than having a series of ExecStart lines?

In a nutshell, script contents are inserted in a script that is called by the unit ExecStart.

There are two advantages in using script:

NixOS modules attributes names have to be unique, so to generate a list of ExecStart commands it is needed to pass the commands as a list, for example ExecStart = [ "${pkgs.iproute}/bin/ip ..." "${pkgs.wireguard}/bin/wg ..." ... ];.
Note that it is possible to turn the multiline string into a string list programmatically with something like builtins.filter (x: x != "") (lib.splitString "\n" multilinestring), but this is rather ugly.
Commands in ExecStart have to be absolute.
script on the other side can take advantage of the unit path, so users can use ip ... instead of ${pkgs.iproute}/bin/ip ... in pre/postSetup. Note that the unit path contains only iproute and wireguard, so other commands such as resolvconf must be prepended with ${pkgs.foo}/bin/. (a nice thing with ${pkgs.foo}/bin/foo notation is that the package doesn't need to be installed on the system, nix automatically manage it)

So using script make the module a little more user friendly while improving its readability/maintainability.

The added preStop is script for ExecStop, its contents are inserted in a script that is called by the unit ExecStop.

Changes are in a new commit to facilitate review, most important ones are:

added ExecStop (via preStop).
added a silly example for preSetup that you might want to change.
added postShutdown to fit more use cases like the resolconf example.
added meta.maintainers.

Mic92 · 2016-08-30T16:34:00Z

One advantage I see, when ExecStart is used over script, that in case of an error,
one can see exactly which command has failed with systemctl status.

ericsagnes · 2016-08-30T18:14:03Z

@Mic92 Thanks, this is indeed a valid point in favor of ExecStart/Stop.

@zx2c4 Please let me know if you think ExecStart is preferable over script for this module.

Update:

ExecStart needs simple commands and will fail with anything that use shell notations like 2>/dev/null || true or printf 'nameserver 10.200.100.1' |.

So as an unfortunate result, the usage of ExecStart will limit the possibilities of pre/postSetup.

I tend to think that the flexibility provided by script surpass the possible advantages of ExecStart in this module case.

fpletz · 2016-08-30T22:26:31Z

Looks fine and mergeable to me in the current state. Will test later and merge. Thanks a lot for your work and the review! 👍

Mic92 · 2016-08-31T05:59:19Z

Well, || true could have been replaced by:

ExecStart=-${pkgs.iproute2.bin}/ip link del dev ${name}

so this command is allowed to fail.
But the fact that nixos does not accept a list for ExecStart also looks problematic to me.

ericsagnes · 2016-10-03T08:09:55Z

@zx2c4 Could you review the latest changes?

zx2c4 · 2016-10-03T12:36:05Z

Looks mostly good to me. One question is -- with what umask does generateConf write the file? It should be written with 077. If it's readable by the world, this is bad, since it contains private key material.

ericsagnes · 2016-10-03T13:02:39Z

Thank you for the review, I squashed the commits and removed the WIP flag so the PR is now ready to merge.

It should be written with 077. If it's readable by the world, this is bad, since it contains private key material.

Unfortunately, actually any file in the Nix store is world readable, handling private files in the nix store is an open and complex problem.

This is well known problem and other modules also suffer this limitation, but I am afraid there is nothing that can be done in this PR scope to fix that.

Mic92 · 2016-10-05T14:20:03Z

When I tried the example, I noticed that empty ExecStart= entries were generated.
To fix this I just made the user defined commands list instead, which also saves some code:
Mic92@826baaa

ericsagnes · 2016-10-05T16:50:45Z

@Mic92 Good catch, thanks!

I updated the scriptToList so it catches newlines too, that should fix the problem you faced.

And thanks for the diff, this way indeed save some code, but might make the code harder to understand and to maintain on the long term.

Mic92 · 2016-10-05T18:05:28Z

nixos/modules/services/networking/wireguard.nix

+
+      postSetup = mkOption {
+        example = literalExample ''
+          printf 'nameserver 10.200.100.1' | ${pkgs.openresolv}/bin/resolvconf -a wg0 -m 0


this example will not work, because it is not executed within in a shell - take a look at my version.

Mic92 · 2016-10-05T18:07:27Z

this version preserves maintainability while still allowing multiple commands and not splitting user defined variables at newline.

ericsagnes · 2016-10-05T23:28:52Z

Thanks, applied!

Mic92 · 2016-10-06T07:43:24Z

Thanks. there was a typo, I made in the postSetup example.

It was deprecated and removed from all modules in the tree by NixOS#18319. The wireguard module PR (NixOS#17933) was still in the review at the time and the deprecated usage managed to slip inside.

basvandijk · 2017-01-03T23:27:11Z

One important comment from @zx2c4 that wasn't addressed is the fact that wireguard.conf is stored in the world readable /nix/store. Although I'm guilty myself of storing secrets in the /nix/store (because it's so damn convenient as a deployment mechanism) we shouldn't force users to do something dangerous. It should always be there choice.

@zx2c4 one simple feature that could really help us is if you can store the private key in a file and reference it with PrivateKeyFile. This is just like SSH referencing the file storing the private key with IdentityFile. For convenience and consistency it would be good to also have PresharedKeyFile and PublicKeyFile.

Ideally PrivateKey is deprecated in favour of PrivateKeyFile and a check is added that makes sure the private key file isn't world readable.

Finally I would like to say that I'm excited that WireGuard is being integrated into NixOS. My colleague @FPtje and I are currently setting up a StrongSwan VPN and it's difficult and error prone. I'm looking forward switching over to WireGuard once it's in the next NixOS release. Thanks to anybody involved in this PR!

Mic92 · 2017-01-03T23:55:36Z

@basvandijk note that wireguard is still experimental, which means the protocol can change in a backwards incompatible way.

zx2c4 reviewed Aug 23, 2016
View reviewed changes

ericsagnes force-pushed the wireguard-module branch 2 times, most recently from e35a699 to 97e45cf Compare August 23, 2016 14:22

ericsagnes force-pushed the wireguard-module branch 2 times, most recently from 7eb877d to 2d2982e Compare August 23, 2016 16:14

fpletz added 6.topic: nixos 8.has: module (new) 2.status: work-in-progress labels Aug 24, 2016

zx2c4 reviewed Aug 24, 2016
View reviewed changes

ericsagnes force-pushed the wireguard-module branch from 2d2982e to 9953373 Compare August 28, 2016 15:57

fpletz removed the 2.status: work-in-progress label Aug 30, 2016

Mic92 mentioned this pull request Sep 22, 2016

systemd.services.<service>.serviceConfig should support multiple entries per option #18838

Closed

ericsagnes force-pushed the wireguard-module branch from 04cab36 to 50ce418 Compare October 3, 2016 12:52

ericsagnes changed the title ~~[WIP] wireguard: add module~~ wireguard: add module Oct 3, 2016

ericsagnes force-pushed the wireguard-module branch from 50ce418 to 16ba139 Compare October 5, 2016 16:40

Mic92 reviewed Oct 5, 2016

View reviewed changes

wireguard: add module

e234f73

ericsagnes force-pushed the wireguard-module branch from 16ba139 to e234f73 Compare October 5, 2016 23:28

Mic92 closed this in 0bd263e Oct 6, 2016

lbonn mentioned this pull request Nov 30, 2016

wireguard: remove dependency on ip-up.target #20825

Merged

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

wireguard: add module #17933

wireguard: add module #17933

ericsagnes commented Aug 23, 2016

mention-bot commented Aug 23, 2016

zx2c4 Aug 23, 2016

zx2c4 commented Aug 23, 2016

ericsagnes commented Aug 23, 2016

ericsagnes commented Aug 23, 2016

zx2c4 commented Aug 23, 2016

ericsagnes commented Aug 23, 2016

zx2c4 commented Aug 23, 2016

ericsagnes commented Aug 23, 2016

zx2c4 commented Aug 23, 2016 •

edited

Loading

fpletz commented Aug 24, 2016

zx2c4 Aug 24, 2016

zx2c4 commented Aug 24, 2016

ericsagnes commented Aug 28, 2016

zx2c4 commented Aug 29, 2016 •

edited

Loading

ericsagnes commented Aug 29, 2016

Mic92 commented Aug 30, 2016 •

edited

Loading

ericsagnes commented Aug 30, 2016 •

edited

Loading

fpletz commented Aug 30, 2016

Mic92 commented Aug 31, 2016 •

edited

Loading

ericsagnes commented Oct 3, 2016

zx2c4 commented Oct 3, 2016

ericsagnes commented Oct 3, 2016

Mic92 commented Oct 5, 2016 •

edited

Loading

ericsagnes commented Oct 5, 2016

Mic92 Oct 5, 2016

Mic92 commented Oct 5, 2016 •

edited

Loading

ericsagnes commented Oct 5, 2016

Mic92 commented Oct 6, 2016

basvandijk commented Jan 3, 2017 •

edited

Loading

Mic92 commented Jan 3, 2017

wireguard: add module #17933

wireguard: add module #17933

Conversation

ericsagnes commented Aug 23, 2016

Motivation for this change

Things done

mention-bot commented Aug 23, 2016

zx2c4 Aug 23, 2016

Choose a reason for hiding this comment

zx2c4 commented Aug 23, 2016

ericsagnes commented Aug 23, 2016

ericsagnes commented Aug 23, 2016

zx2c4 commented Aug 23, 2016

ericsagnes commented Aug 23, 2016

zx2c4 commented Aug 23, 2016

ericsagnes commented Aug 23, 2016

zx2c4 commented Aug 23, 2016 • edited Loading

fpletz commented Aug 24, 2016

zx2c4 Aug 24, 2016

Choose a reason for hiding this comment

zx2c4 commented Aug 24, 2016

ericsagnes commented Aug 28, 2016

zx2c4 commented Aug 29, 2016 • edited Loading

ericsagnes commented Aug 29, 2016

Mic92 commented Aug 30, 2016 • edited Loading

ericsagnes commented Aug 30, 2016 • edited Loading

fpletz commented Aug 30, 2016

Mic92 commented Aug 31, 2016 • edited Loading

ericsagnes commented Oct 3, 2016

zx2c4 commented Oct 3, 2016

ericsagnes commented Oct 3, 2016

Mic92 commented Oct 5, 2016 • edited Loading

ericsagnes commented Oct 5, 2016

Mic92 Oct 5, 2016

Choose a reason for hiding this comment

Mic92 commented Oct 5, 2016 • edited Loading

ericsagnes commented Oct 5, 2016

Mic92 commented Oct 6, 2016

basvandijk commented Jan 3, 2017 • edited Loading

Mic92 commented Jan 3, 2017

zx2c4 commented Aug 23, 2016 •

edited

Loading

zx2c4 commented Aug 29, 2016 •

edited

Loading

Mic92 commented Aug 30, 2016 •

edited

Loading

ericsagnes commented Aug 30, 2016 •

edited

Loading

Mic92 commented Aug 31, 2016 •

edited

Loading

Mic92 commented Oct 5, 2016 •

edited

Loading

Mic92 commented Oct 5, 2016 •

edited

Loading

basvandijk commented Jan 3, 2017 •

edited

Loading