[WireGuard] Wireguard on OpenWRT/LEDE (here: Luci)

[WireGuard] Wireguard on OpenWRT/LEDE (here: Luci)

[Dan Lüdtke]

Hi everyone,

I created a LuCi integration for Wireguard. Feedback and beta testing =
appreciated.

--> https://github.com/danrl/luci-proto-wireguard

Thanks,

Dan=

Re: [WireGuard] Wireguard on OpenWRT/LEDE (here: Luci)

[Jason A. Donenfeld]

 Hey Dan,

This looks beautiful! Thanks for writing it. I'm not a LEDE dev, but I
do have a small bit of preliminary feedback, below.

Thanks for writing this. I'm thrilled!

Jason


1. The first screenshot should be cropped before the start of the
"Peers" section creeps in. Otherwise it looks like there are two
sections for peers. This confused me a bit at first.

2.
- listen_port.placeholder = "500"
+ listen_port.placeholder = "51820"

3. "addresses.placeholder = "fd00:13:37::1/64"" -- might be nice to
give a v4 example too.

4.
- "cryptography for post-quantum resistance")
+ "cryptography for post-quantum resistance.")

5. "Names are resolved prior to establishing the connection for the
first time." It might be more accurate to say that they're resolved at
the time the interface is configured/brought up.

6.
- endpoint.placeholder = "vpn.example.com:500"
+ endpoint.placeholder = "vpn.example.com:51820"

7.
- persistent_keepalive.datatype = "range(0, 3600)"
+ persistent_keepalive.datatype = "range(0, 65535)"

8. This one is more important than it may seem at first, but please
keep the terminology straight and explicit:
- translate("Keep Alive"),
+ translate("Persistent Keep Alive"),

9. ${iface} should be quoted -- "${iface}"

10. 1>/dev/null 2>/dev/null can be shortened to the more idiomatic
>/dev/null 2>&1

11. The existing logic for adding a device is to add it if it doesn't
exist, and otherwise to flush the addresses. Is it a good idea to
flush the routes too? Or simply delete and re-add? Or is a simple
flush of the addresses standard LEDE behavior? I'll defer to Baptiste
(CC'd) on this.

12. mkdir -p is non fatal if the path already exists, so you don't
need to do the -d check before.

13. Touching the configuration file and then recursively chmodding is
not the right way to do things. It opens you up to other types of
security vulnerabilities. Instead, use umask.

Putting 12 and 13 together, we have:

- [ -d "${wg_dir}" ] || mkdir -p "${wg_dir}"
- [ -f "${wg_cfg}" ] || touch "${wg_cfg}"
- chmod -R 0600 "${wg_dir}"
+ umask 077
+ mkdir -p "${wg_dir}"

14.
- allowed_ips=$(echo $allowed_ips | tr ' ' ',')
+ allowed_ips="$(echo "$allowed_ips" | tr ' ' ',')"

You might also want to replace all instances of ,, with , after this
transformation until there aren't any ,, left  just to maintain
sanity.

15. On tear-down, why not just unconditionally delete?

Re: [WireGuard] Wireguard on OpenWRT/LEDE (here: Luci)

[Dan Lüdtke]

Thanks for the fast reply!


Changes applied. See diff here:
--> =
https://github.com/danrl/luci-proto-wireguard/commit/6f90ff4ecc21f39721b3d=
a357962c0ebe0d20750

Screenshots updated.



> On 1 Nov 2016, at 21:09, Jason A. Donenfeld <Jason@zx2c4.com> wrote:
>=20
> 3. "addresses.placeholder =3D "fd00:13:37::1/64"" -- might be nice to
> give a v4 example too.

Not a big fan of legacy IP in new software. Also, Not sure how to have =
multiple placeholders. If anyone knows a clean solution: Count me in. In =
the meantime I'd prefer IP over legacy IP here.


> 9. ${iface} should be quoted -- "${iface}"

Where exactly? So I quoted all I could find. Should be fine now?!


> 11. The existing logic for adding a device is to add it if it doesn't
> exist, and otherwise to flush the addresses. Is it a good idea to
> flush the routes too? Or simply delete and re-add? Or is a simple
> flush of the addresses standard LEDE behavior? I'll defer to Baptiste
> (CC'd) on this.

Not sure either. Also, anyone knows it OpenWRT and LEDE will join after =
the c-base talks? Will commit to openwrt on github. Everyone is free to =
copy from there.

>=20
> 14.
> - allowed_ips=3D$(echo $allowed_ips | tr ' ' ',')
> + allowed_ips=3D"$(echo "$allowed_ips" | tr ' ' ',')"

I went with multiple AllowedIPs=3D lines now, introduced early October.


Thanks again for the feedback!

Dan=

Re: [WireGuard] Wireguard on OpenWRT/LEDE (here: Luci)

[Jason A. Donenfeld]

On Tue, Nov 1, 2016 at 9:57 PM, Dan L=C3=BCdtke <mail@danrl.com> wrote:
>> 11. The existing logic for adding a device is to add it if it doesn't
>> exist, and otherwise to flush the addresses. Is it a good idea to
>> flush the routes too? Or simply delete and re-add? Or is a simple
>> flush of the addresses standard LEDE behavior? I'll defer to Baptiste
>> (CC'd) on this.
>
> Not sure either.

In your latest commit, it looks like you've just added flushing of the
routes too. If you're headed down this track, just do it properly by:

- if ! ip link show dev "${iface}" >/dev/null 2>&1; then
-  ip link add dev "${iface}" type wireguard
- else
-  ip address flush dev "${iface}"
-  ip route flush dev "${iface}"
- fi
+ ip link del dev "${iface}" 2>/dev/null
+ ip link add dev "${iface}" type wireguard

Re: [WireGuard] Wireguard on OpenWRT/LEDE (here: Luci)

[Dan Lüdtke]

Done!

> - if ! ip link show dev "${iface}" >/dev/null 2>&1; then
> -  ip link add dev "${iface}" type wireguard
> - else
> -  ip address flush dev "${iface}"
> -  ip route flush dev "${iface}"
> - fi
> + ip link del dev "${iface}" 2>/dev/null
> + ip link add dev "${iface}" type wireguard