Add the "missing" WireGuard rc.d script

[crest_freebsd_rlwinm.de]

Summary

This rc.d script reimplements most features of wg-quick(8) in FreeBSD
/bin/sh instead of bash.

The following features have not been reimplemented:

• The daemon syncing a routing table the AllowedIPs settings.
• Saving the active configuration back to a config file.

The PreUp/PostUp/PreDown/PostDown hooks are implemented, but the hooks
are executed in sh instead of bash. Additionally the successful execution
is tracked to avoid (re-)execution of hooks because it's expected that users will rerun rc.d scripts.

Since the WireGuard rc.d script is supposed to make using WireGuard on FreeBSD as easy as possible it handles the creation and destruction of WireGuard interfaces instead of forcing users to configure the netif script to clone and rename WireGuard interfaces I added one feature not present in wg-quick(8): the Sticky setting. Sticky interfaces are only deconfigured (IP addresses removed, link brought down, configuration replaced with the empty configuration), but not destroyed by service wireguard stop $ifn. To avoid having to add a FreeBSD specific setting to the configuration files the rc.d script also looks at the sticky bit of interface configuration files.

The rc.d script uses per interface lock files (implemented using lockf) to protect against race conditions (e.g. restarting an interface via devd and ifstated at the same time), because otherwise users would have to implement similar locking in their {Pre,Post}{Up,Down} hooks.

Test Plan

• Create /etc/wireguard using install -d -m 750 -o root -g wheel /etc/wireguard
• Create a test interface configuration e.g. ${EDITOR:-vi} /etc/wireguard/wg-test.conf
• Start the rc.d script: service wireguard start.
• Restart just one interface: service wireguard restart wg-test
• Stop the rc.d script: service wireguard stop.

The proposed test plan covers only the basic operations since there is no way to foresee what users will put in their hooks.

[freebsd_igalic.co]

first pass…

some of the formatting looks really off. Especially the case statements.
Please make sure you're using tabs everywhere.

I'll have more comments when I'm less tired, but for now we'll need entries in /etc/defaults/rc.conf and in rc.conf(5)

libexec/rc/rc.d/wireguard

57

maybe this function should go into /etc/network.subr

[crest_freebsd_rlwinm.de]

The interface name restriction function is a judgment call restricting users from creating problematic (for shell scripts) interface names. Applying the same restrictions to existing services like netif and routing could break (partly) working configurations.

libexec/rc/rc.d/wireguard

57

This function is a judgment call restricting users from creating problematic (for shell scripts) interface names. Applying the same restrictions to existing services like netif and routing could break (partly) working configurations.

[michael_galassi.us]

This is my first review here, I hope to not be stepping on any toes. If I have, please correct me so I might do better next time. Overall your wireguard startup mechanism looks good and my suggestions are strictly cosmetic in nature. I would not be at all disappointed if this patch were committed unaltered.

A general comment is that there are several instances similar to:

	if ! exists; then
		warn "Missing WireGuard interface $INTERFACE to bring down."
	elif ! in_wg; then
		warn "Refusing to bring down non-WireGuard interface $INTERFACE."

I wonder if the code wouldn't be overall more easily read if those lines were factored out into a small helper function so we only every saw validate_if $INTERFACE <caller function name>?

libexec/rc/rc.d/wireguard

3

I believe that we've discontinued the use of these rcs/cvs tags in our shell scripts.

233

Perhaps lose the 6085 string in the middle of this line of hash marks?

527

I don't believe that the enter is needed if state was already $1, maybe move this line above the 'fi' on the preceding line?

687

The inconsistent use of white-space in the next dozen or so lines is a bit distracting.

[pauamma_gundo.com]

No manual page to review, yet manpages is a group reviewer. Did a file get accidentally left out?

[crest_freebsd_rlwinm.de]

In D41318#980524, @pauamma_gundo.com wrote:

No manual page to review, yet manpages is a group reviewer. Did a file get accidentally left out?

There is no change to man pages yet, but the new rc.conf(5) variables have to be documented (e.g. duplicating what service wireguard describe would print) as man page.

[crest_freebsd_rlwinm.de]

libexec/rc/rc.d/wireguard

3

Correct. This line got overcome by events.

[crest_freebsd_rlwinm.de]

libexec/rc/rc.d/wireguard

233

Will do.

527

It doesn't hurt as it's a cheap function to call. It can be argued both ways, but I prefer it this way because it produces better debug logs if debugging is enabled. Not calling enter would only log that the hook was skipped, but not why. To me this worth the cost of checking for the state file twice even if it triggers some reader's desire to optimise the "useless" call away.

[crest_freebsd_rlwinm.de]

libexec/rc/rc.d/wireguard

687

Depending on your editor whitespace settings it arranges the code into a 2D table which helped me reason about it and saved vertical screen space for split editing. Is there a canonical style(9) like guide for shell instead of C to follow on how to reformat this?

[rpokala]

libexec/rc/rc.d/wireguard

687

I'm not sure if it's in style(9), but the mere fact that tab can be interpreted in different ways, suggests that space should be used.

[kevans]

Resigning from this; I tried to provide feedback over IRC, but that was seemingly not well-received (and questions unanswered) and I'm not interested in reviewing this as-is. I'd much prefer splitting it into two scripts, one with, e.g., verbs, that manages wireguard interfaces and then the rc script that simply drives that in an obvious way. The last objection I heard was that there's too much state to pass around, but it's not at all clear why unless this is trying to mix way too much rc.conf configuration in with wg config.

[crest_freebsd_rlwinm.de]

libexec/rc/rc.d/wireguard

687

Spaces can't be used because shell heredocs can only strip tabs not spaces from the beginning of lines.

[zarychtam_plan-b.pwste.edu.pl]

That's the way how DragonFlyBSD devs solved the problem
https://gitweb.dragonflybsd.org/dragonfly.git/commitdiff/451640b7cf6bcf7826b901ac9a51647442adb96b