networkd: add support for wireguard vpn #4191

Open
wants to merge 2 commits into
from

Projects

None yet

8 participants

@Mic92
Contributor
Mic92 commented Sep 20, 2016 edited

This pull request adds support for wireguard in systemd-networkd. This is work
in progress and I want to open a discussion about inclusion of this feature
before putting too much effort in it.

What is WireGuard:

  • WireGuard is a new kernel-builtin vpn with modern fast cryptography
  • it competes with IPsec and Openvpn and beats both in terms of throughput, latency and usability

Why I want to include support for WireGuard into systemd-networkd:

WireGuard implements a new interface type called "wireguard".
WireGuard itself ships a command line util called wg for key management,
but lacks of support to configure ip addresses and routes on the interface.
My pull request intend to close this gap by implementing key management as
well as the new interface type in systemd-networkd.

Status of the WireGuard project:

  • it is an out-of-tree kernel module (>= linux v4.1) and packaged already for
    the following distribution: Debian, Fedora, Archlinux, OpenSUSE, Gentoo,
    NixOS, Openwrt, (Ubuntu as PPA)
  • the code is production ready
  • the protocol is still under development and review. Once it reaches 1.0, developers will attempt to upstream the kernel implementation into mainline

Questions to systemd maintainers:

Under what prerequisits wireguard support would be applicable for beeing merged?

Questions to wireguard team (@zx2c4):

WireGuard keys are managed via ioctl, is this API public?
If not, is header file uapi.h
public or will there be a netlink interface in future, which can be extented
backward-compatible?

cc: @steigr

@Mic92 Mic92 changed the title from add basic support for wireguard vpn to [networkd] add basic support for wireguard vpn Sep 20, 2016
@zx2c4
Contributor
zx2c4 commented Sep 20, 2016

Hey guys,

This is awesome. I'm very pleased to see this. I'll review the code and give a thorough answer to your questions shortly.

Jason

@zx2c4
Contributor
zx2c4 commented Sep 21, 2016 edited

Hey again,

So, looking at the commit, it's pretty bare bones. In order for WireGuard to be properly supported in systemd-networkd, you'll need to actually configure the WireGuard-particular settings; otherwise it's not very interesting.

Fortunately, when I designed the userspace interface for WireGuard, I had in mind various nice interfaces for wg(8). One of those interfaces is a configuration file syntax. And, that configuration file syntax resembles the INI format, a choice already inspired by systemd-networkd. So, in an amazing full circle, systemd-networkd and WireGuard have a certain semantic closeness, which should make this part extremely easy. A WireGuard configuration file looks like this:

[Interface]
PrivateKey = yAnz5TF+lXXJte14tji3zlMNq+hd2rYUIgJBgB3fBmk=
ListenPort = 41414

[Peer]
PublicKey = xTIBA5rboUvnH4htodjb6e697QjLERt1NAB4mZqp8Dg=
AllowedIPs = 10.192.122.3/32, 10.192.124.1/24

[Peer]
PublicKey = TrMvSoP4jYQlY6RIzBgbssQqY3vxI2Pi+y71lOWWXX0=
AllowedIPs = 10.192.122.4/32, 192.168.0.0/16
PersistentKeepalive = 28

[Peer]
PublicKey = gN65BkIKy1eCE9pP1wdc8ROUtkHLF2PfAqYdyYBz6EA=
AllowedIPs = 10.10.10.230/32

That's pretty much it. It's straight forward to understand and parse. So I don't think you'll have any trouble with this part -- mapping it to systemd-networkd should be simple.

Onto your questions:

WireGuard keys are managed via ioctl, is this API public?

Yes, absolutely! I tried to make it a very pleasant interface for other applications to use.

is header file uapi.h public

Yes, this header file is public. Feel free to copy it into systemd-networkd if you'd like to do it that way. I'll then make a [physical] note to submit a pull request here in case that API is ever augmented.

will there be a netlink interface in future, which can be extented backward-compatible

Depending on various ongoing discussions with the other kernel netdev developers, we may wind up adding a netlink interface. But, for now, consider that uapi.h ioctl quite stable.

@Mic92 Mic92 changed the title from [networkd] add basic support for wireguard vpn to [networkd][wip] add basic support for wireguard vpn Sep 21, 2016
@keszybz
Member
keszybz commented Sep 23, 2016

What's the upstreaming status of wireguard? I guess the systemd-networkd part for this would be rather self-contained, so we could discuss merging the support on provisional basis, but normally we'd wait for the kernel to merge support first.

As for the patch set: it would need to be extended to actually configure the interface. I guess reading the wireguard configuration file for the Peer list and Interface config would be OK, if the format is stable and public, and doesn't mix other stuff. Otherwise, a [Wireguard] section could be added to our config files. Do you have a proposal for how this would look, and how the configuration and support will be split?

@keszybz keszybz added the network label Sep 23, 2016
@Mic92
Contributor
Mic92 commented Sep 23, 2016 edited

@keszybz thanks for you feedback. I am currently working on adding a [wireguard] section to networkd's config, so the implementation will be self-contained. Waiting for a merge into kernel would be ok for me. my proposal would be the following:

# wg0.netdev
[Match]
Name=wg0
Kind=wireguard

[Wireguard]
# this is mandatory, if systemd is supposed to configure wireguard completely
PrivateKey=<private_key> 
# optional, makes wireguard ready for the post-quantum world
PresharedKey=<key>
# mandatory
ListenPort=6666

[Peer]
# mandatory
PublicKey=<public_key>
# I am not sure what the would the best solution here
# with growing number of subnets, this is easier to read
AllowedIP=10.0.0.1/32
AllowedIP=[fd42::1]/64
# ALTERNATIVE: this is what wireguard's config look like
# however sometimes people use the wrong separator, 
# so the version above is harder to mess up.
AllowedIPs=10.0.0.1/32, [fd42::1]/64
# optional
Endpoint=some.host.tld:443

# multiple peers allowed.
[Peer]
PublicKey=<public_key2>
@zx2c4
Contributor
zx2c4 commented Sep 23, 2016 edited

Hi @Mic92,

You wrote:

AllowedIP=10.0.0.1/32
AllowedIP=[fd42::1]/64

You SHOULD HAVE written:

AllowedIPs=10.0.0.1/32
AllowedIPs=[fd42::1]/64

EVERYWHERE else, WireGuard consistently uses "AllowedIPs", because this represents a list or a mask (or just a single IP). Please fix this PR to use the same label.

Update: fixed in the latest commit, thanks!

@Mic92
Contributor
Mic92 commented Sep 23, 2016

I was aware, that wireguard is using plural and my first proposed option was singular.
The idea was to split up the list into single item options, because this for example easier to
generate in deployment scripts and I once had the case, where somebody used the list variant wrong.
But it is also a fair point, that this syntax would not match with the output of wg(8),
so I will stick with this version.

@zx2c4
Contributor
zx2c4 commented Sep 23, 2016 edited

Even with one item per line, /24 still expands to 255 distinct IPs, so the name AllowedIPs still makes sense, even if its one per line.

@zx2c4

This is a great start! Thanks so much.

src/network/networkd-netdev-gperf.gperf
@@ -53,6 +54,9 @@ Tunnel.CopyDSCP, config_parse_bool, 0,
Tunnel.EncapsulationLimit, config_parse_encap_limit, 0, offsetof(Tunnel, encap_limit)
Peer.Name, config_parse_ifname, 0, offsetof(Veth, ifname_peer)
Peer.MACAddress, config_parse_hwaddr, 0, offsetof(Veth, mac_peer)
+Peer.PublicKey, config_parse_wireguard_public_key, 0, 0
+Peer.AllowedIPs, config_parse_wireguard_ips, 0, 0
@zx2c4
zx2c4 Sep 23, 2016 Contributor

---> config_parse_wireguard_allowed_ips

@poettering
poettering Oct 11, 2016 Member

I'd probably name the "PublicKey" option "WiregardPublicKey" or so if it is in the generic [Peer] section. It's fine to leave fully generic options with generic names (i.e. AllowedIPs= is pretty generic I'd say), but I'd guess that the wireguard public keys would be in a different format, than let's say ipsec would have them, hence I'd prefix the setting here to indicate that this option is supposed to be in wireguard's format, and thing else.

src/network/networkd-netdev-gperf.gperf
@@ -53,6 +54,9 @@ Tunnel.CopyDSCP, config_parse_bool, 0,
Tunnel.EncapsulationLimit, config_parse_encap_limit, 0, offsetof(Tunnel, encap_limit)
Peer.Name, config_parse_ifname, 0, offsetof(Veth, ifname_peer)
Peer.MACAddress, config_parse_hwaddr, 0, offsetof(Veth, mac_peer)
+Peer.PublicKey, config_parse_wireguard_public_key, 0, 0
+Peer.AllowedIPs, config_parse_wireguard_ips, 0, 0
+Peer.Endpoint, config_parse_wireguard_endpoint, 0, 0
@zx2c4
zx2c4 Sep 23, 2016 Contributor

TODO, I assume?

@Mic92
Mic92 Sep 23, 2016 Contributor

yes, hence the name WIP in the commit message.

src/network/networkd-netdev-wireguard.c
+ const char *rvalue,
+ void *data,
+ void *userdata) {
+ return 0;
@zx2c4
zx2c4 Sep 23, 2016 Contributor

Fortunately this is just a simple unbase64 too.

src/network/networkd-netdev-wireguard.c
+ const char *rvalue,
+ void *data,
+ void *userdata) {
+ return 0;
@keszybz
keszybz Sep 24, 2016 Member

There's very similar parsing in config_parse_address in src/network/networkd-address.c. You might need to factor out the middle part of that function into a new function.

@zx2c4
zx2c4 Sep 27, 2016 Contributor

Using those parsing functions sounds okay to me.

src/network/networkd-netdev-wireguard.c
+ void *data,
+ void *userdata) {
+ return 0;
+}
@zx2c4
zx2c4 Sep 23, 2016 Contributor

Likewise, you can copy the code used here:
https://git.zx2c4.com/WireGuard/tree/src/tools/config.c#n121

src/network/networkd-netdev-wireguard.c
+ const char *rvalue,
+ void *data,
+ void *userdata) {
+ return 0;
@zx2c4
zx2c4 Sep 23, 2016 Contributor

To make this a bit nicer, I use getaddrinfo to lookup the port. You can copy the code here:
https://git.zx2c4.com/WireGuard/tree/src/tools/config.c#n59

src/network/networkd-netdev-wireguard.h
+#define WG_KEY_LEN 32
+
+struct Wireguard {
+ NetDev meta;
@zx2c4
zx2c4 Sep 23, 2016 Contributor

You could just embed the uapi struct inside this one, so you don't have to replicate each member.

@keszybz
Member
keszybz commented Sep 24, 2016

The multiline version of AllowedIPs is better (more readable). We can always allow multiple items on the same line later if wanted.

@keszybz keszybz changed the title from [networkd][wip] add basic support for wireguard vpn to [wip] networkd: add basic support for wireguard vpn Sep 24, 2016
@zx2c4
Contributor
zx2c4 commented Sep 27, 2016 edited

@keszybz

The multiline version of AllowedIPs is better (more readable). We can always allow multiple items on the same line later if wanted.

The mainline wireguard-tools (wg(8)) actually support multiple AllowedIPs entries lines:

[Peer]
PublicKey=mF3bG9/gY17AKLJnOfICWtQzCRbPE31Yr9Jw0COwk2U=
AllowedIPs=10.0.0.0/16
AllowedIPs=[fd42::1]/64
AllowedIPs=192.168.1.0/24,[abcd::]/96
Endpoint=some.host.tld:443

Notice that they're all titled AllowedIPs, since a mask like this doesn't imply a single IP.

The above config snippet is valid in wg(8). So, I think your and @MIc92's assessment is correct about this, and now both utilities use the same syntax, which is great.

@keszybz
Member
keszybz commented Sep 27, 2016

On Tue, Sep 27, 2016 at 12:03:21PM -0700, Jason A. Donenfeld wrote:

The mainline wireguard-tools (wg(8)) actually support multiple AllowedIPs entries lines:

Then yes, it's probably best to implement compatible syntax in
systemd-networkd too.

@johannbg
Contributor

Let's see how this progresses with the upstream kernel community before implementing this and take into consideration how best this could be implemented and managed within and out containers .

@zx2c4
Contributor
zx2c4 commented Sep 27, 2016 edited

Hi @johannbg -- I see you're not marked as a systemd project member, so I'll take what you suggested with a grain of salt, but you do bring up two good points for which I think I have two good answers, below.

Let's see how this progresses with the upstream kernel community

One approach is to wait for upstream, sure, and this sort of policy will be decided by the systemd devs for when they want to pull the patch. However, I firmly believe that implementing this early on -- whether or not its merged immediately -- is very important. The reason is that the experience of seeing how integrations work out (even if they're living in a pull request) is often very helpful for determining the roadmap and direction of both projects, the integrator and the integratee. For example, I can imagine WireGuard upstream learns a lot from systemd-networkd developers' experiences with implementing the integration. And I can imagine systemd-networkd developers learn a lot about the future of networking by working on the integrations. As an investigative exercise, it can only benefit everybody. Then, when things are looking perfect, we'll look into where and when we want to merge, and what the best plan for that is. Since we'll already have done astute research work, this will positively benefit the upstream kernel conversation. In the end, everybody will be happier. The more experience, the better.

take into consideration how best this could be implemented and managed within and out containers .

This particular technical matter is something to which upstream has given a lot of thought, and there is an extended document describing this here: https://www.wireguard.io/netns/
I'd highly suggest interested systemd-networkd developers read this, as there's a lot of food for thought. Probably discussion about this that isn't directly related to this pull request is best directed toward the WireGuard upstream mailing list: https://lists.zx2c4.com/mailman/listinfo/wireguard

@johannbg
Contributor

Throw some pepper on that grain of salt and get your stuff accepted upstream first.
Have you even submitted it any form upstream yet or is that still on hold or do you really expect that everybody wants to sign them self up for carrying out of tree patches/module to the kernel indefinitely to be able to use this?.

Let's wait and see Tom approves this without this actually being merged upstream first. Lennart did so with cgroupv2 so that might be a trend now what once was frown upon.

@zx2c4
Contributor
zx2c4 commented Sep 28, 2016 edited

Throw some pepper on that grain of salt and get your stuff accepted upstream first.

That's not really a very intellectually constructive way to move the discussion forward. I'm interested in other peoples' opinions at this point; I think you've made your point now twice. I'll make mine one more time too: regardless of what's decided on the "when" question for merging a pull request like this, there's a lot to be learned and gained from both upstream wireguard and from systemd-networkd in seeing what an implementation would look like. It is research I am extremely interested in as a means of understanding more precisely how the ecosystem puts parts together.

Have you even submitted it any form upstream yet

do you really expect that everybody wants to sign them self up for carrying out of tree patches/module to the kernel indefinitely to be able to use this

I have never, not once, suggested anything of that nature.

Let's wait and see

Let's not wait and see. Instead, let's forge ahead with doing valuable research on future networking technologies, and learning how ecosystem integrations work. I know I don't intend to stop tinkering around with the systemd codebase in relation to wireguard, and I certainly wouldn't encourage anybody else to stop either.


@johannbg - please do not reply to this message or to this thread. You've made your point; I've made mine. Now I'd like to resume the useful technical discussion on here.

@teg
Contributor
teg commented Oct 1, 2016

@Mic92 thanks for the patch! I'm happy to merge support for this. However, we try not to merge support for things before they are upstream, so let's keep the discussion going and once the module is has been merged upstream we can apply your patch.

One request: it would be very nice if the kernel api was done as a netlink api rather than ioctls (though I admit I did not look into if there was a good reason not to do that), so that we could manage this like everything else.

@zx2c4
Contributor
zx2c4 commented Oct 1, 2016

Hey @teg -- good to hear from you.

let's keep the discussion going and once the module is has been merged upstream we can apply your patch

Sounds good to me.

it would be very nice if the kernel api was done as a netlink api rather than ioctls

So in developing WireGuard, for each component, I did several different implementations for each one. For example, there's a very particular lookup data structure for some odd part of wireguard, and I implemented this as a linked list, as a radix trie, as an lc-trie, as an art table, and so forth. Each implementation I did with a fresh mind. At the end, I chose the one that I thought came out the best, with an emphasis on being clean, minimal, maintainable, and auditable.

So too, I did the same thing with implementing the configuration interface. I wrote four different netlink implementations, each one written about a month apart, with a fresh mind and fresh ideas on how I could make it the prettiest. This might seem like an excessive exercise by somebody who likes to code a little too much, but it did pay off; each one was better than the previous. However, one day I wondered, "I wonder what an old school simple ioctl-based implementation would look like." Not more than an hour later did I have something half the length and double the clarity. It was just so much ridiculously better and nicer to work with that I dropped the netlink work all together and moved to the simpler ioctl interface. The resultant API is here, in uapi.h. I think you'll agree that this is a pretty nice and simple interface.

One of the reasons I liked the ioctl-based approach a lot more than the netlink one is that it didn't require me to buffer anything in the kernel. With netlink, the entire message gets composed and buffered in kernel space, and then it's transferred to userspace. When returning a peer list to userspace, with 65536 possible total peers, it's obviously not feasible to stuff them inside one netlink message, so an ugly paging multi-packet interface is required, and this gets ugly really fast. However, with the ioctl, userspace can first ask, "how much memory do I need to allocate", then userspace allocates this memory, passes a pointer to kernelspace, and then the kernel can safely and iteratively copy into this userspace region, with no huge kernelspace allocations required. This is quite nice, and I think the implementation turned out very simply.

So, with all this in mind, what precisely is my motivation for moving to netlink? (The strongest arguments in favor of netlink I've heard are related to syscall filtering being difficult with private ioctls and opaque fds, but I don't find this entirely compelling.) And after reading the above, if you believe I still should, do you have in mind any constructive suggestions for the nicest way to design such an netlink interface that won't wind up being ugly or cumbersome?

(The other [slightly less compelling but still important nonetheless] benefit to ioctl is that the same serialization format can very easily be used to talk to userspace wireguard implementations on linux, mac, and windows alike, and in fact a client app speaking this interface over a unix socket already lives in mac's homebrew.)

@poettering
Member

So, yeah, I think this looks great, and we should merge it, but I figure we should wait until it's clear that this will go in upstream in the kernel. (we don't have to wait until it's actually merged, but at least until the time where it is clear that everything is good, and the kernel/userspace interface for it is considered stable.)

@poettering

I commented on a couple of superficial review issues

src/network/networkd-netdev-wireguard.c
+ fd = socket(AF_INET, SOCK_DGRAM, 0);
+ if (fd < 0) {
+ return log_netdev_error_errno(netdev, -errno, "Failed to open AF_INET to configure wireguard: %m");
+ }
@poettering
poettering Oct 11, 2016 Member

we prefer not to use {} on single-line if blocks, see CODING_STYLE.

Also, no need to negate the errno parameter to log_netdev_error_errno(), it will negate it as necessary on its own.

(And there are spurious double whitespaces after the errno argument)

src/network/networkd-netdev-wireguard.c
+#include "networkd-netdev-wireguard.h"
+
+
+const NetDevVTable wireguard_vtable = {
@poettering
poettering Oct 11, 2016 Member

spurious double newline...

src/network/networkd-netdev-gperf.gperf
@@ -53,6 +54,9 @@ Tunnel.CopyDSCP, config_parse_bool, 0,
Tunnel.EncapsulationLimit, config_parse_encap_limit, 0, offsetof(Tunnel, encap_limit)
Peer.Name, config_parse_ifname, 0, offsetof(Veth, ifname_peer)
Peer.MACAddress, config_parse_hwaddr, 0, offsetof(Veth, mac_peer)
+Peer.PublicKey, config_parse_wireguard_public_key, 0, 0
+Peer.AllowedIPs, config_parse_wireguard_ips, 0, 0
@poettering
poettering Oct 11, 2016 Member

I'd probably name the "PublicKey" option "WiregardPublicKey" or so if it is in the generic [Peer] section. It's fine to leave fully generic options with generic names (i.e. AllowedIPs= is pretty generic I'd say), but I'd guess that the wireguard public keys would be in a different format, than let's say ipsec would have them, hence I'd prefix the setting here to indicate that this option is supposed to be in wireguard's format, and thing else.

@Mic92 Mic92 changed the title from [wip] networkd: add basic support for wireguard vpn to networkd: add basic support for wireguard vpn Nov 4, 2016
src/network/networkd-netdev-wireguard.c
+ w->dev.replace_peer_list = true;
+}
+
+int config_parse_wireguard_address_family(const char *unit,
@zx2c4
zx2c4 Nov 9, 2016 Contributor

This function and configuration option is absolutely incorrect and should be removed.

It is up to getaddrinfo to return the best match for DNS resolution. WireGuard should not handle determining the address family.

Please remove this option.

@Mic92
Mic92 Nov 9, 2016 edited Contributor

The default behavior would be as you described it and return the best match. My reasoning was that in some network environments (dual-stack, 6to4 aka he.net, ipv6 only via vpn) both protocol version are available, but only one is desired.
In those environments I found it useful to override default behavior only for the vpn. Protocol specific tunnel may even break wireguard because of lower the MTU.
The workaround would be to setup domains which only contains A or AAAA records or globally modify gai.conf for all applications. I am open for other solutions for this particular problem. Or I remove this option, if you guys think this is not a problem in common networks.

@andir
andir Nov 9, 2016 Contributor

Since @Mic92 asked me about my opinion:

While that one can be handy switch I agree with @zx2c4. If you require an IPv4 or IPv6 only DNS record you can always add one (ip4.foo.bar / ip6.foo.bar).

If you want to provide that option (besides DNS record) maybe default the address familiy to "auto" and then use the DNS result?

@Mic92
Mic92 Nov 9, 2016 edited Contributor

"auto" is the default, it is called "any" in my case - which means AddressFamily is not a required option. Providing a dedicated subdomain is what I end up doing - so let's hope users can always control their hostnames?

@zx2c4
zx2c4 Nov 11, 2016 Contributor

@poettering I'll defer to your judgement here. However, as upstream of WireGuard, my opinion is that specifying "AddressFamily" here is the wrong layer at which to do this, and this is an issue for GAI instead.

@Mic92
Contributor
Mic92 commented Nov 9, 2016 edited

This is the configuration I used for testing. If wireguard gets upstream, I will add a small integration test as well.

# wg0.netdev
[NetDev]
Name=wg0
Kind=wireguard

[Wireguard]
PrivateKey=EEGlnEPYJV//kbvvIqxKkQwOiS+UENyPncC4bF46ong=
PresharedKey=EEGlnEPYJV//kbvvIqxKkQwOiS+UENyPncC4bF46ong=
ListenPort=8633

[WireguardPeer]
AllowedIPs=192.168.0.0/24,192.168.1.0/24,::/0
Endpoint=192.168.1.2:8080
PublicKey=GkyAu7MxoG68fvLVqKYkDgu2MlpCivAulZfFTsTmGyw=
PersistentKeepalive=1

[WireguardPeer]
AllowedIPs=192.168.1.1
Endpoint=google-public-dns-a.google.com:60000
PublicKey=niwbJ/Kb2GPReslzT8LcOcRqgGPVu21Q6YxOWk/biyc=

[WireguardPeer]
AllowedIPs=192.168.1.2
Endpoint=heise.de:443
AddressFamily=ipv4
PublicKey=ZojW2Xhp2F7/BPkDYfS8R08WmgZ7NJ/em1PUGk4oHTU=

[WireguardPeer]
AllowedIPs=192.168.1.3
Endpoint=heise.de:http
AddressFamily=ipv6
PublicKey=RDf+LSpeEre7YEIKaxg+wbpsNV7du+ktR99uBEtIiCA=

[WireguardPeer]
AllowedIPs=192.168.4.0/24
Endpoint=host.invalid:8080
PublicKey=ie0u/yx5yqWlmdJbZ2WWy0953De+znQpfx299IiWqwY=
PersistentKeepalive=1
# wg0.network
[Match]
Name=wg0

[Network]
Address=fe80::1/64
@poettering

a couple of notes

src/network/networkd-netdev-wireguard.c
+
+static void resolve_endpoints(NetDev *netdev);
+
+static WireguardPeer *new_peer(Wireguard *w, unsigned section) {
@poettering
poettering Nov 11, 2016 Member

hmm, to follow our usualy coding style, please name this "wireguard_peer_new()"...

src/network/networkd-netdev-wireguard.c
+}
+
+static int update_wireguard_config(NetDev *netdev) {
+ _cleanup_close_ int fd = -2;
@poettering
poettering Nov 11, 2016 Member

why -2? we usually initialize fds to -1...

src/network/networkd-netdev-wireguard.c
+ _cleanup_close_ int fd = -2;
+ _cleanup_free_ void *data = NULL;
+ void *pos = NULL;
+ struct ifreq ifreq = {0};
@poettering
poettering Nov 11, 2016 Member

please write this as simply = {}...

src/network/networkd-netdev-wireguard.c
+
+ assert(w);
+
+ fd = socket(AF_INET, SOCK_DGRAM, 0);
@poettering
poettering Nov 11, 2016 Member

please always open fds with cloexec right away. i.e. add SOCK_CLOEXEC here to the second apram...

src/network/networkd-netdev-wireguard.c
+
+ fd = socket(AF_INET, SOCK_DGRAM, 0);
+ if (fd < 0)
+ return log_netdev_error_errno(netdev, -errno, "Failed to open AF_INET to configure wireguard: %m");
@poettering
poettering Nov 11, 2016 Member

no need to negate "errno" here, the logging functions do that on their own.

src/network/networkd-netdev-wireguard.c
+ len = strlen(netdev->ifname);
+ assert(len <= IFNAMSIZ);
+
+ memcpy(&ifreq.ifr_name, netdev->ifname, len + 1);
@poettering
poettering Nov 11, 2016 Member

strncpy() looks more appropiate here... It's kinda the only place strncpy() actually makes sense...

src/network/networkd-netdev-wireguard.c
+ assert(len <= IFNAMSIZ);
+
+ memcpy(&ifreq.ifr_name, netdev->ifname, len + 1);
+ data = calloc(sizeof(char), w->allocation_size);
@poettering
poettering Nov 11, 2016 Member

use new0(char, w->allocation_size) for this?

src/network/networkd-netdev-wireguard.c
+ memcpy(&ifreq.ifr_name, netdev->ifname, len + 1);
+ data = calloc(sizeof(char), w->allocation_size);
+ if (!data)
+ return log_netdev_error_errno(netdev, -errno, "Cannot allocate memory to configure wireguard: %m");
@poettering
poettering Nov 11, 2016 Member

for OOM errors we usually just call return log_oom()...

src/network/networkd-netdev-wireguard.c
+ }
+ }
+ assert((size_t)((char*)pos - (char*)data) == w->allocation_size);
+ ifreq.ifr_data = data;
@poettering
poettering Nov 11, 2016 Member

hmm, uh oh... the whole loop raises all eyebrows i have due to alignment concerns... I think it would be nicer to use memcpy() when putting this together...

(and I'd probably increase the ptr, which "pos = (uint8_t*) pos + siezof(struct wgdevice)" and suchlike, I think that'd be more readable...

src/network/networkd-netdev-wireguard.c
+ ifreq.ifr_data = data;
+
+ if (ioctl(fd, WG_SET_DEVICE, &ifreq) != 0)
+ return log_netdev_error_errno(netdev, -errno, "Unable configure wireguard device: %m");
@poettering
poettering Nov 11, 2016 Member

no need to negate errno

src/network/networkd-netdev-wireguard.c
+ free(e);
+}
+
+static inline void endpoint_freep(WireguardEndpoint **e) {
@poettering
poettering Nov 11, 2016 Member

looks like a job for DEFINE_TRIVIAL_CLEANUP_FUNC()

src/network/networkd-netdev-wireguard.c
+ endpoint);
+
+ if (r < 0)
+ log_netdev_error_errno(netdev, -errno, "Failed create resolver: %m");
@poettering
poettering Nov 11, 2016 Member

the error is in "r" here, not in errno...

src/network/networkd-netdev-wireguard.c
+ assert(rvalue);
+ assert(data);
+
+ // TODO replace with sd_resolve_getaddrinfo to support service names
@poettering
poettering Nov 11, 2016 Member

service names are stupid. I really wouldn't bother. service names never worked, nobody uses them, and they should just die..

@Mic92
Mic92 Nov 11, 2016 Contributor

ok, this would be then different compared to what wireguard does.

src/network/networkd-netdev-wireguard.c
+ // TODO replace with sd_resolve_getaddrinfo to support service names
+ r = safe_atou16(rvalue, &port_num);
+ if (r < 0)
+ log_syntax(unit, LOG_ERR, filename, line, r, "Invalid port specification, ignoring assignment: %s", rvalue);
@poettering
poettering Nov 11, 2016 Member

should you guard for port_num != 0 here?

(maybe we should even add parse_ip_port() or so, that is like safe_autou16, but guards this)

@Mic92
Mic92 Nov 11, 2016 edited Contributor

port_num == 0 would lead to random assignment by wireguard, which is probably not what users want.

@zx2c4
zx2c4 Nov 11, 2016 edited Contributor

No, it's certainly not random. It's quite deterministic in fact.

Most users who are "clients" rather than servers will want port=0.

@poettering
poettering Nov 11, 2016 Member

I am pretty sure in networkd we shouldn't accept Port=0 if that has a special meaning. Instead Port= should accept an additional value "auto" or so (which needs proper documentation of course) which would then internally be passed as 0 to wireguard... Using "0" with special semantics is fine for low-level code, but I doubt we should expose this to the user.

@zx2c4
zx2c4 Nov 11, 2016 Contributor

Sounds good to me.

src/network/networkd-netdev-wireguard.c
+
+ peer = new_peer(w, section_line);
+ if (!peer)
+ return -ENOMEM;
@poettering
poettering Nov 11, 2016 Member

probably should use log_oom() here...

src/network/networkd-netdev-wireguard.c
+ if (!peer)
+ return -ENOMEM;
+
+ r = strv_split_extract(&words, rvalue, ",", 0);
@poettering
poettering Nov 11, 2016 Member

so, you are not actually interested in the "words" array, right? you just want to split this up, parse the individual words and use the parsed values, but throw the words away, right?

First of all, there's a memory leak here, you never free "words"... That said, I#d suggest you do away entirely with this, and instead change this to be a loop around extract_first_word(). i.e. instead of first placing this in an strv, and then iterating through that, please just iterate each word directly, without placing it in any strv...

@Mic92
Mic92 Nov 11, 2016 Contributor

I think it should be freed, because of the cleanup function:

_cleanup_strv_free_ char** words;

but extract_first_word() is also a solution.

src/network/networkd-netdev-wireguard.c
+ STRV_FOREACH(word, words) {
+ end = strchr(*word, '/');
+ if (end)
+ address = strndupa(*word, end - *word);
@poettering
poettering Nov 11, 2016 Member

it's not OK to use strndupa() within a loop, as it allocates memory on the stack. See CODING_STYLE.

src/network/networkd-netdev-wireguard.c
+ r = safe_atou8(end + 1, &prefixlen);
+ if (r < 0) {
+ log_syntax(unit, LOG_ERR, filename, line, r, "Route destination prefix length is invalid, ignoring assignment: %s", end + 1);
+ return 0;
@poettering
poettering Nov 11, 2016 Member

prefixlen should probably be <= 32 and <=128 depending on the family, no?

Maybe we should add a new parse_prefixlen() function that does the right thing...

@Mic92
Mic92 Nov 11, 2016 Contributor

how about parse_cidr instead?

@poettering
poettering Nov 14, 2016 Member

@Mic92 hmm, a prefix length is a value. "CIDR" otoh is a concept, not a value. Hence I think parse_prefixlen() appears more appropriate to me...

@Mic92
Mic92 Nov 15, 2016 edited Contributor

@poettering I thought about parsing both, the address and subnet size in one function from CIDR notation (see cidr_from_string in my latest pr). This should be more common than a standalone prefix, no? There is one other instance in networkd, where this can be reused: networkd-route.c:722

src/network/networkd-netdev-wireguard.c
+ }
+ ipmask = new0(WireguardIpmask, 1);
+ if (!ipmask)
+ return -ENOMEM;
@poettering
poettering Nov 11, 2016 Member

probably "return log_oom()"...

src/network/networkd-netdev-wireguard.c
+
+ peer = new_peer(w, section_line);
+ if (!peer)
+ return -ENOMEM;
@poettering
poettering Nov 11, 2016 Member

log_oom()...

src/network/networkd-netdev-wireguard.c
+
+ endpoint = new0(WireguardEndpoint, 1);
+ if (!endpoint)
+ return -ENOMEM;
src/network/networkd-netdev-wireguard.c
+
+ host = strndup(begin, len);
+ if (!host)
+ return -ENOMEM;
src/network/networkd-netdev-wireguard.c
+
+ port = strdup(end);
+ if (!port)
+ return -ENOMEM;
src/network/networkd-netdev-wireguard.c
+
+ peer = new_peer(w, section_line);
+ if (!peer)
+ return -ENOMEM;
@poettering
poettering Nov 11, 2016 Member

log_oom()

src/network/networkd-netdev-wireguard.c
+ LIST_FOREACH(peers, peer, w->peers) {
+ LIST_FOREACH(ipmasks, mask, peer->ipmasks) {
+ free(mask);
+ }
@poettering
poettering Nov 11, 2016 Member

no {} here...

@Mic92
Contributor
Mic92 commented Nov 15, 2016 edited

I see three problems remaining, when resolving endpoints:

  1. When the endpoint is resolved it could fail due temporary network problems.
    As this is a long running daemon there is compared to wg(8) no easy way to recover from those errors as a user. I thought about adding exponential backoff, if network failures are detected while resolving.
  2. People might want to set wireguard as a their internet gateway - for end-users this probably the most common use case for a vpn. Endpoints are currently resolved asynchronous. Defaults routes to wireguard are likely applied before resolving is done. Resolved would gets this one right as it will use the interface where the dns server was set. However the adoption of it in upstream distributions is not high yet.
  3. When a dns cache is used such as dnsmasq or pdns (NetworkManager has a plugin for first one),
    endpoints can be resolved even if no default gateway is present. Usually a resolver will return addresses based on what routes are available - therefor it should pick the correct address family. This will not work if network is down when resolving which could happen if systemd-networkd is restarted.
    Resolved would also get this right - it flushes the cache, but again the preferred option is still the glibc resolver in most setups.
@eliasp
Contributor
eliasp commented Nov 15, 2016

Resolved would gets this one right as it will the use the interface where the dns server was set. However the adoption of it in upstream distributions is not high yet.

I wouldn't care about this too much! A distribution which is modern enough to adopt wireguard/systemd-wireguard will also be modern enough to have systemd-resolved in place.

Users running one of the distributions not using systemd-resolved yet, but still wanting to use systemd-wireguard will be competent enough to take care of a potentially missing systemd-resolved on their own.

@poettering

some more nitpicks.

What's the current state of the kernel patch going upstream?

src/network/networkd-util.c
+
+ r = safe_atou16(s, port);
+ if (r < 0 || *port == 0)
+ return -EINVAL;
@poettering
poettering Nov 15, 2016 Member

as a general rule: please don't clobber return values on failure, also see CODING_STYLE. Specifically, this means: invoke safe_atou16() with a temporary variable as second argument, and write it to *port only on success.

src/network/networkd-util.c
+ return 0;
+}
+
+int cidr_from_string(const char *s, int *family, union in_addr_union *addr, unsigned char *prefixlen) {
@poettering
poettering Nov 15, 2016 Member

i'd call this parse_address_and_prefixlen() I must say.

The kernel exposes this as "address" and "prefixlen" fields, and doesn't use the term "cidr", and I think we should follow suit here...

src/network/networkd-util.c
+ r = in_addr_from_string_auto(address, family, addr);
+ if (r < 0) {
+ return r;
+ }
@poettering
poettering Nov 15, 2016 Member

no {} around single-line if blocks please

src/network/networkd-util.c
+ if (r < 0 ||
+ (*family == AF_INET && *prefixlen > 32) ||
+ (*family == AF_INET6 && *prefixlen > 128))
+ return -EINVAL;
@poettering
poettering Nov 15, 2016 Member

please do not clobber return values on error (see above)

src/network/netdev/wireguard.h
+} WireguardEndpoint;
+
+
+struct Wireguard {
@poettering
poettering Nov 15, 2016 Member

spurious double newline

src/network/netdev/wireguard.c
+ int r, i = 0;
+ Wireguard *w;
+ WireguardEndpoint *endpoint;
+ struct addrinfo hints = {
@poettering
poettering Nov 15, 2016 Member

this can be made static const, no?

src/network/netdev/wireguard.c
+}
+
+
+int config_parse_wireguard_private_key(const char *unit,
@poettering
poettering Nov 15, 2016 Member

spurious double newline

src/network/netdev/wireguard.c
+
+ r = cidr_from_string(word, &family, &addr, &prefixlen);
+
+ if (r < 0) {
@poettering
poettering Nov 15, 2016 Member

please keep function calls and their error handling together, i.e. drop the newline above

src/network/netdev/wireguard.c
+ }
+
+
+ peer->fields.persistent_keepalive_interval = keepalive;
@poettering
poettering Nov 15, 2016 Member

spurious double newline

@Mic92
Contributor
Mic92 commented Nov 15, 2016

Addressed the issues.

@Mic92
Contributor
Mic92 commented Nov 22, 2016

the code handle resolving errors now.

@Mic92 Mic92 changed the title from networkd: add basic support for wireguard vpn to networkd: add support for wireguard vpn Nov 30, 2016
Mic92 added some commits Nov 12, 2016
@Mic92 Mic92 networkd: add support for wireguard vpn fcd4ea7
@Mic92 Mic92 wireguard: exponential backoff on resolve error
3bb1bb5
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment