[MIR] Wireguard

Update: I believe I'm able to create a better DEP8 test using a VM and network namespaces, establishing a wireguard vpn between them.

Hi Andreas, thank you for this high quality MIR!

Unfortunately this is a tentative MIR team NACK, as it duplicates functionality that we already have in Ubuntu main.

I understand you point of openvpn != wireguard and therefore we need wireguard VPN support in main, too.

But netplan.io has been supporting wireguard setups since v0.100, that is available in Focal+ for both of it's backend renders (systemd-networkd & NetworkManager). systemd-networkd + netplan.io being the default network management tool in Ubuntu since Bionic, I do not see a valid reason to integrate the additional and duplicated wg/wg-quick configuration functionality that this package provides, as stated by xnox in #9 of LP: #1892798 as well.

https://netplan.io/reference/#properties-for-device-type-tunnels%3A
https://github.com/canonical/netplan/blob/main/examples/wireguard.yaml

If netplan is lacking any required functionality, we should rather work on improving that, IMO.

It feels awkward to have half the wireguard code in main (kernel), and leave the configuration bits out in favor of our own tools, which probably didn't exist when the wireguard userspace tooling was created. If you go to the wireguard site, it won't teach you how to use systemd-networkd or netplan to configure wireguard: it will talk about wg and wg-quick, which ubuntu users won't have supported in the next LTS release.

If you google for "how to configure wireguard", none of the hits (checked first page only) talk about systemd-networkd or netplan.

The netplan documentation mentions wireguard, but does not say how to generate the keys: https://netplan.io/reference/#properties-for-device-type-tunnels%3A

This documentation can be improved, as you say, but won't change the fact that we will be the odd ones out by not using upstream's tooling.

Doesn't the "duplicated functionality" argument apply to the major bits of wireguard code that are in the kernel in main already? Shall we remove it from the kernel then and go back to having the dkms build only?

I agree that's pretty weird. And especially for wg(8), that's not just a configuration tool; that's the low level inspection tool. Netplan can configure IP addresses; are you going to move ip(8) out of main too? If ip(8) is in main, then wg(8) should be in main. Netplan doesn't replace the low level inspection tools. It's a high level thing.

My personal opinion is that we do need tools to setup and configure wireguard from start to finish in Main. That does include tooling to generate the keys.

It would be nice to further develop wireguard package such that by default it integrates with the default Ubuntu networking stack (netplan.io, networkd, resolved).

W.r.t. documentation not pointing to things that work best on ubuntu, we should too work on improving that.

Indeed our current default networking setup is lacking some functionality from "wg", that is the key generation and display of dynamic information (like "lastest-handshake" and "transfer-rx/tx" from "wg show"), while most of the other static information of a current WireGuard setup is also available via the "netplan get" command. So is the configuration part (given the required keys) via netplan YAML configuration using either the systemd-networkd or NetworkManager backend.

So we need to make a call if we want to accept some duplicated maintenance (configuration & static info parts) in favor of gaining the full upstream standard tools support, incl. key generation (which we probably need).

Thank you for all your input, I will consult the rest of the MIR team about this!

A compromise could be splitting up the package to keep out wg-quick, but I wonder if that's worth the effort, as wg-quick is a pretty small and clear script only (in addition to some systemd unit configuration).

Status changed to 'Confirmed' because the bug affects multiple users.

I wrote this test that I intend to propose as a DEP8 test. Here is the output:

$ sudo ./vpn
Setting things up
Generating keys
Generating wireguard config
Cleaning up old namespaces
Creating new namespaces left_ns and right_ns and adding loopback interface to them
Creating veth interface connecting both namespaces
Bringing up LEFT wireguard interface in namespace left_ns
[#] ip link add wg_left type wireguard
[#] wg setconf wg_left /dev/fd/63
[#] ip -4 address add 10.0.5.1/24 dev wg_left
[#] ip link set mtu 1420 up dev wg_left
Bringing up RIGHT wireguard interface in namespace right_ns
[#] ip link add wg_right type wireguard
[#] wg setconf wg_right /dev/fd/63
[#] ip -4 address add 10.0.5.2/24 dev wg_right
[#] ip link set mtu 1420 up dev wg_right

This is the config
left_ns namespace:
[Interface]
ListenPort = 3001
PrivateKey = WDwCnk1LaTwsLSWT3DUsrgu9676RxjBdX+PPglV1tGA=

[Peer]
PublicKey = +69yT8PzWVd1l8IR8Y5yc25Qsi0OoIB+i75HTlvVVjM=
AllowedIPs = 10.0.5.2/32
Endpoint = 10.0.1.2:3002

right_ns namespace:
[Interface]
ListenPort = 3002
PrivateKey = WK5M7T1HVu12Q8SCW9FZpgaxTjXXMTzjM5QT7Q+qNV8=

[Peer]
PublicKey = qfg1hEQp9EK951ysQhzEi2F9ahW/KndYPkIRulAlIm8=
AllowedIPs = 10.0.5.1/32
Endpoint = 10.0.1.1:3001

Testing gateway ping
Pinging right gateway, from left_ns namespace
PING 10.0.5.2 (10.0.5.2) 56(84) bytes of data.
64 bytes from 10.0.5.2: icmp_seq=1 ttl=64 time=0.495 ms

--- 10.0.5.2 ping statistics ---
1 packets transmitted, 1 received, 0% packet loss, time 0ms
rtt min/avg/max/mdev = 0.495/0.495/0.495/0.000 ms

Pinging left gateway, from right_ns namespace
PING 10.0.5.1 (10.0.5.1) 56(84) bytes of data.
64 bytes from 10.0.5.1: icmp_seq=1 ttl=64 time=0.061 ms

--- 10.0.5.1 ping statistics ---
1 packets transmitted, 1 received, 0% packet loss, time 0ms
rtt min/avg/max/mdev = 0.061/0.061/0.061/0.000 ms

Testing wireguard interface ping
Pinging right wireguard IP from left_ns namespace
PING 10.0.1.2 (10.0.1.2) 56(84) bytes of data.
64 bytes from 10.0.1.2: icmp_seq=1 ttl=64 time=0.015 ms

--- 10.0.1.2 ping statistics ---
1 packets transmitted, 1 received, 0% packet loss, time 0ms
rtt min/avg/max/mdev = 0.015/0.015/0.015/0.000 ms

Pinging left wireguard IP from right_ns namesapce
PING 10.0.1.1 (10.0.1.1) 56(84) bytes of data.
64 bytes from 10.0.1.1: icmp_seq=1 ttl=64 time=0.046 ms

--- 10.0.1.1 ping statistics ---
1 packets transmitted, 1 received, 0% packet loss, time 0ms
rtt min/avg/max/mdev = 0.046/0.046/0.046/0.000 ms

Testing vpn stats
Namespace left_ns
  latest handshake: Now
  transfer: 348 B received, 404 B sent
Namespace right_ns
  latest handshake: Now
  transfer: 404 B received, 348 B sent

It's suitable to run in a single VM. I'll create a PR soon.

> It's suitable to run in a single VM. I'll create a PR soon.

Nice, that will help to catch changes in other packages impacting
wireguard to be spotted early on.

BTW - the MIR team discussion has come to the conclusion that we can
and want to have it along with the functionality that exists in
netplan.
Lukas will start a full review of it soon'ish.

I just realized that we have a better DEP8 test already running, but in another source package: src:wireguard-linux-compat, which produces bin:wireguard-dkms. With wireguard now being in the kernel, I wonder if we still need the dkms one. Maybe for cases where someone wants to try a newer version before the kernel is updated? Not sure.

In any case, we have good DEP8 coverage already, but split among different source packages:
- src:wireguard-linux-compat: sets up a wireguard vpn using network namespaces, and uses ping to test it, similar to what I have done above
- src:wireguard: just generates keys, and that test is marked correctly as superficial
- src:netplan.io: also sets up a wireguard vpn, and exercises it

These are all run together when a new wireguard upload happens. In that sense, we have good coverage, even if we decide to drop the dkms package. The netplan test would still exercise wireguard, but I think it does not use the wg or wg-quick tools to set it up, so we would still benefit from having another end-to-end test using those tools. So I can still add my test to src:wireguard, or copy over the one from src:wireguard-linux-compat.

With the additional DEP-8 tests and as xnox says, better integration with our default units, I do not think the duplication is harmful (apart from the extra maintainance on us) compared to what most users will do: google/look on stackoverflow and use a semi-official (due to wg being in the kernel) supported integration.

Review for Package: src:wireguard

[Summary]
Thank you for all the comments and input on this MIR! I've revisited my initial
opinion after consulting with the rest of the MIR team and came to the
conclusion that we should accept a certain degree of duplicated maintenance here
in order to be able to have the low-level WireGuard standard tools in the
supported set of packages and especially to be able to have the full stack of
tools supported to create a WireGuard connection, that includes key generation.

MIR team ACK under the constraint to resolve the below listed
required TODOs and as much as possible having a look at the
recommended TODOs.

This does need a security review, so I'll assign ubuntu-security

List of specific binary packages to be promoted to main: wireguard-tools
Specific binary packages built, but NOT to be promoted to main: wireguard-dkms

Notes:
I suggest the server team to reach out to @unit193 as the MOTU who maintained
this package previously, to coordinate the next steps with him and keep him in
the loop.

Required TODOs:
- does NOT have a test suite that runs at build time, we should add at least
  the trivial autopkgtest generating and checking keys, as suggested
- does NOT have a non-trivial test suite that runs as autopkgtest, we should
  integrate more testing (LP: #1952102) as suggested by adding the new "vpn"
  test and/or copying the non-trivial autopkgtest from wireguard-linux-compat
- Resolve MIR dependencies:
  + nftables: we could switch Recommends to iptables, but nftables is the future
    please refer to (LP: #1887187)
  + wireguard-dkms: recommended by wireguard-tools, it's part of the same source
    package, but we probably want to drop that, as we have the WireGuard modules
    in the kernel. Or at least we'd want to change Recommends: wireguard-dkms to
    Suggests: wireguard-dkms (LP: #1873288)

Recommended TODOs:
- improve integration with Ubuntu's default networking stack (LP: #1892798)
- The package should get a team bug subscriber before being promoted
- Ubuntu does carry a delta, but it is reasonable and maintenance under
  control, LP: #1890201 should be revisited to check if we still need this
  delta now that our kernels support WireGuard natively

[Duplication]
There is netplan.io in main providing some of the same functionality, especially
for configuring and setting up wireguard tunnels. But it is lacking the others
like generating the key material and inspection of low level dynamic config.
Furthermore, most documentation that can be found online points to using the "wg"
and "wg-quick" tools, so we want to support those, too.

[Dependencies]
OK:
- no -dev/-debug/-doc packages that need exclusion
- No dependencies in main that are only superficially tested requiring
  more tests now.

Problems:
- Other dependencies to MIR due to this:
  - checked with check-mir
  - not listed in seeded-in-ubuntu
  - none of the (potentially auto-generated) dependencies (Depends
    and Recommends) that are present after build are not in main
  + nftables: we could switch Recommends to iptables, but nftables is the future
    please refer to (LP: #1887187)
  + wireguard-dkms: recommended by wireguard-too...

Glad to hear the result. Thanks for working through this and hearing me out on IRC as well.

With regards to the TODO:

> I suggest the server team to reach out to @unit193 as the MOTU who maintained

Unit193 is really top-notch and knows the project well, is an active participant with upstream, and generally is pretty on top of things. I don't know whether MIRing this means some sort of hand off involved, but I'd say that to the extent you can keep him in the fold, it's some nice expertise to keep around.

> - does NOT have a test suite that runs at build time, we should add at least
> the trivial autopkgtest generating and checking keys, as suggested
> - does NOT have a non-trivial test suite that runs as autopkgtest, we should
> integrate more testing (LP: #1952102) as suggested by adding the new "vpn"
> test and/or copying the non-trivial autopkgtest from wireguard-linux-compat

Let me know if you guys need help scripting these up. Indeed taking the wireguard-linux-compat case is probably a good place to start. But if you want something more elaborate and need a hand, just poke me on IRC.

> + wireguard-dkms: recommended by wireguard-tools, it's part of the same source
> package, but we probably want to drop that, as we have the WireGuard modules
> in the kernel. Or at least we'd want to change Recommends: wireguard-dkms to
> Suggests: wireguard-dkms (LP: #1873288)

I'd suggest you sync up with @apw about this. He was involved in some of the earlier discussions about this. And @unit193 too. Details are a bit fuzzy to me, but I think there's something interesting happening with the `wireguard` metapackage pulling in `wireguard-tools` and a `wireguard-modules` virtual package. That `wireguard-modules` virtual package is then satisfied by wireguard-dkms, wireguard-linux-compat, and the various Canonical kernel packages. Or something like that. I don't see a need for this to change. But...

> recommended by wireguard-tools, it's part of the same source package

This part confused me. Many many eons ago, WireGuard was one repo, with src/* having dkms kernel sources and src/tools/* containing the tools package. For a long long time now, this has been split up. But I wonder if the wireguard-tools package still has something left over from the days when dkms was mixed with it?

There are different source packages involved:

- src:wireguard produces bin:wireguard-tools and is the subject of this MIR
- src:wireguard-linux-compat produces bin:wireguard-dkms. It may be the same upstream source, but it's NOT the same source package as src:wireguard

I wondered if we should drop src:wireguard-linux-compat, since it only builds the dkms package, and we have wireguard in the kernel already. I reached out to apw, and he commented that in such cases, where a dkms module is merged with the kernel, we (canonical) tend to follow debian and drop the dkms when debian does. I was pointed at the existing debian bug requesting to drop the wireguard-dkms package: https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=989406
It could lead to confusion, as some users might wonder if they have to install the dkms package or not, but I discussed this with the server team and the conclusion was that we can leave it where it is (universe), and only act if it starts to introduce problems.

I also think we can drop the ubuntu delta, since it was just reordering the dependencies (wireguard-dkms (>= 0.0.20200121-2) | wireguard-modules (>= 0.0.20191219)). Indeed, canonical's kernel provides "wireguard-modules (= 1.0.0)", so we can list wireguard-modules first just like in debian:

$ dpkg -s linux-image-generic|grep Depends
Depends: linux-image-5.13.0-21-generic, linux-modules-extra-5.13.0-21-generic, linux-firmware, intel-microcode, amd64-microcode

Jason, this is the DEP8 test I wanted to add to src:wireguard before I saw that src:wireguard-linux-compat had one already: https://git.launchpad.net/~ahasenack/ubuntu/+source/wireguard/tree/debian/tests/wireguard-wgquick?h=jammy-wireguard-dep8

I also copied over the one from src:wireguard-linux-compat (netns-mini), and I was wondering about moving the keygen test into the build process, since the other dep8 tests for sure call keygen anyway. In that way, we would have:
- build test: keygen (using the built binary)
- dep8 tests: two good tests: one using wg only, the other using wg and wg-quick.

I'll go over the remaining issues and address them. Thanks for the MIR review, Lukas!

> I was pointed at the existing debian bug requesting to drop the wireguard-dkms package

The place where we still want wireguard-dkms, though, is for when people are running Ubuntu on strange kernels that might not have it out of the box. These are, of course, becoming increasingly rare. Probably the thing to do here is just to drop that package at the same time Debian does, whenever Debian does.

> Jason, this is the DEP8 test I wanted to add to src:wireguard before I saw that src:wireguard-linux-compat had one already: https://git.launchpad.net/~ahasenack/ubuntu/+source/wireguard/tree/debian/tests/wireguard-wgquick?h=jammy-wireguard-dep8

That looks fine to me, though I find the use of "right" and "left" a bit too IPsec for my tastes :-). One thing you could do is do all the keygen inline with the script. For example:

key1="$(pp wg genkey)"
key2="$(pp wg genkey)"
pub1="$(pp wg pubkey <<<"$key1")"
pub2="$(pp wg pubkey <<<"$key2")"

If I recall correctly, the netns-mini test I made does this, though that uses a slightly different topology, avoiding veth. Your test also uses wg-quick, which is neat, so maybe combining everything into one would be a decent idea, depending on how motivated you are.

There's also this monster set of tests in the kernel tree, if you're looking for trouble: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/tools/testing/selftests/wireguard/netns.sh

On Fri, Nov 26, 2021 at 02:09:26PM -0000, Jason A. Donenfeld wrote:
> Unit193 is really top-notch and knows the project well, is an active
> participant with upstream, and generally is pretty on top of things. I
> don't know whether MIRing this means some sort of hand off involved, but
> I'd say that to the extent you can keep him in the fold, it's some nice
> expertise to keep around.

Indeed, this is one of the potential downsides to the MIR: when we follow
through, we'll probably have *fewer* updates to the package, unless we
grant Unit 193 either core-dev or per-package upload privileges. That's
not something the MIR team can do, but we can suggest it strongly. :)

Thanks

> One thing you could do is do all the keygen inline with the script. For example:
> key1="$(pp wg genkey)"
> ...

Looks good, I don't need the keys in a file, and I can then use the var names in the config file I generate for wg-quick.

Taking care of these:
- build time test: https://bugs.launchpad.net/ubuntu/+source/wireguard/+bug/1952767
- dep8 test: https://bugs.launchpad.net/bugs/1952102

Required for 22.04, setting Critical + Milestone 22.02 (FeatureFreeze)

I reviewed wireguard 1.0.20210914-1ubuntu2 as checked into jammy.
This shouldn't be considered a full audit but rather a quick
gauge of maintainability.

wireguard is the user space component of the WireGuard VPN, an
in-kernel vpn. The tools provided are for querying and configuring
the state of the kernel portion of WireGuard.

- No directly applicable CVEs.
- No significant Build-Depends.
- pre/post inst/rm scripts deal with the wq-quick systemd unit
- The wg-quick systemd unit in not enabled by default; it is a
  templated oneshot service to make automatic connections on boot.
- No dbus services
- No setuid binaries
- wg and wg-quick are the binaries in added in PATH
- No sudo fragments.
- No polkit files.
- No udev rules.
- tests:
  - No unit tests, a couple of build time tests of key generation
  - Some autopkgtests to test basic functionality, no real
    negative tests
  - it is good to see built-in fuzzing support.
- No cron jobs.
- Build logs are clean

- Processes spawned:
  - there are lots of wrapped calls to popen(); fortunately they
    are confined to contributed or android tools only, and not
    included in the wg binary.
- Memory management is performed okay.
- File IO is okay, primarily used from the command line to read
  and write keys and read configuration. Attempts to protect
  against writing world accessible keys.
- Logging is done through perror(), strerror(), and gai_strerror(),
  and is okay.
- Environment variable use is limited.
- No use of privileged functions on Linux
- Use of cryptography / random number sources:
  - uses getrandom()
  - curve25519 implementations are embedded code copies,
    implementations are good.
- No use of temp files in C code, wg-quick uses a static name
  for writing out a config file before moving it into place.
- networking for the userspace component looks to be limited to
  resolving ip addresses and talking via netlink to configure
  and query the kernel code, and looks okay.
- No use of WebKit.
- No use of PolicyKit.

- No cppcheck warnings.
- No Coverity results that weren't false positives.
- shellcheck on wg-quick was mostly clean:
  - line 338 uses the variable $i as a loop index in multiple nested
    loops; it appears to work correctly, but is mildly confusing
    to read.
  - quoting issues that are likely false positives

The wg-quick shell script feels like it is at that point of
complexity where it might be worth re-implementing in a less
error prone programming language than bash.

The /usr/share/docs/wireguard-tools/examples directory contains
all of the stuff in contrib/ which is of varying quality, but
doesn't really provide any example configurations.

Security team ACK for promoting wireguard to main.

One other non-security opinionated comment: having the wireguard meta package pull in the dkms package will likely cause people to install them unnecessarily. While many people will read the documentation first and realize they only need to install wireguard-tools, it's likely others will hear that WireGuard is supported in Ubuntu and assume `apt install wireguard` will do the right thing.

> One other non-security opinionated comment: having the wireguard meta package pull in the dkms
> package will likely cause people to install them unnecessarily.

I asked about this in the bug[1], and on irc[2], but got no response.

1. https://bugs.launchpad.net/ubuntu/+source/wireguard/+bug/1873288/comments/23
2. https://irclogs.ubuntu.com/2021/12/07/%23ubuntu-devel.html#t10:58

If you happen to have a kernel installed that has the virtual provides for wireguard-modules, then dkms won't be pulled in.

$ dpkg -s linux-image-generic|grep wireguard-modules
Provides: virtualbox-guest-modules (= 5.13.0-28), wireguard-modules (= 1.0.0), zfs-modules (= 2.0.6-1ubuntu2)

Seed was changed

ubuntu-server will be subscribed to this package

Andreas wrote:
> If you happen to have a kernel installed that has the virtual provides
> for wireguard-modules, then dkms won't be pulled in.

Oh nice, I missed that, thanks for pointing it out. That definitely covers my complaint there.

I had a chat with apw in #ubuntu-devel[1], and it looks like keeping the current order of dkms first, then modules, is correct.

This is the reason, in summary: "so that if you install this with a personal kernel, or a kernel without support you get the dkms not another kernel."

The scenario could be you have no kernel which provides wireguard-modules. Then you apt install wireguard. If wireguard-modules comes first, apt will select some binary kernel package that has the modules, and install it. But you are still without the modules, because you have to boot the kernel. With dkms first, you at least get the chance to build the modules via dkms, and start using wireguard right away.

If you have a kernel which provides wireguard-modules, then the "dkms-wireguard | wireguard-modules" will be satisfied, and dkms won't be installed.

That all of course ignores the fact that installed kernel doesn't necessarily mean running kernel, but that's another issue.

My seed change is then for bin:wireguard-tools. not bin:wireguard, because I don't want wireguard-dkms to appear in component mismatches. But maybe this is a tooling issue?

Archive Admins, given the above, what do you prefer?

1. https://irclogs.ubuntu.com/2022/02/23/%23ubuntu-devel.html (not a super direct link because the log hasn't updated yet, but the conversation should be near the top)

apw's reasoning is correct.

I do wonder, though, if at some point we can start looking into sunsetting the dkms package entirely and the wireguard-linux-compat backport with it. It's been mainlined for a good deal of time now. We'd have to do some analysis of which kernels people run Ubuntu with are so old that they don't have it, and I'm not sure how exactly to perform that analysis. But maybe it's something to consider down the road.

Override component to main
wireguard 1.0.20210914-1ubuntu2 in jammy: universe/misc -> main
wireguard 1.0.20210914-1ubuntu2 in jammy amd64: universe/net/optional/100% -> main
wireguard 1.0.20210914-1ubuntu2 in jammy arm64: universe/net/optional/100% -> main
wireguard 1.0.20210914-1ubuntu2 in jammy armhf: universe/net/optional/100% -> main
wireguard 1.0.20210914-1ubuntu2 in jammy i386: universe/net/optional/100% -> main
wireguard 1.0.20210914-1ubuntu2 in jammy ppc64el: universe/net/optional/100% -> main
wireguard 1.0.20210914-1ubuntu2 in jammy riscv64: universe/net/optional/100% -> main
wireguard 1.0.20210914-1ubuntu2 in jammy s390x: universe/net/optional/100% -> main
wireguard-tools 1.0.20210914-1ubuntu2 in jammy amd64: universe/net/optional/100% -> main
wireguard-tools 1.0.20210914-1ubuntu2 in jammy arm64: universe/net/optional/100% -> main
wireguard-tools 1.0.20210914-1ubuntu2 in jammy armhf: universe/net/optional/100% -> main
wireguard-tools 1.0.20210914-1ubuntu2 in jammy ppc64el: universe/net/optional/100% -> main
wireguard-tools 1.0.20210914-1ubuntu2 in jammy riscv64: universe/net/optional/100% -> main
wireguard-tools 1.0.20210914-1ubuntu2 in jammy s390x: universe/net/optional/100% -> main
14 publications overridden.