Re: [PATCH 1/1] Added network namespacing support to wq-quick

["Jason A. Donenfeld" <Jason@zx2c4.com>]

Thanks for the patch! I've generally been hesitant to add network
namespacing stuff to wg-quick, because it seemed a bit too complex for
the scope, but your idea here of thinking of it as simply the socket
netns, while the link stays in the namespace of the running process is
kind of nice, and I'm willing to consider this.

Would be interested in feedback on the idea from Luis (CC'd) too.

Some comments on the implementation, below:

On Wed, Mar 4, 2020 at 7:14 AM <endre.szabo@wg-ml-rkaofgr.redir.email> wrote:
> Please go easy on me, this is my first time sending a patch.
>
> --Endre

The patch needs a Signed-off-by: line. Git will automatically add this
using the lowercase -s option to git-commit, such as:

$ git commit -a -s -m "blah"

>   contrib/highlighter/gui/highlight.cpp |  1 +
>   contrib/highlighter/highlight.c       |  1 +
>   contrib/highlighter/highlighter.h     |  1 +

Thanks for not forgetting the highlighters. However, you'll still need
to add parsing of this field to the actual parser. This might involve
looking up what valid netns names are.

> +NetNS \(em Controls in which network namespace the WireGuard UDP socket
> is added to. The
> +namespace has to be created before WireGuard use.
> +.IP \(bu

Let's call this "SocketNamespace".

SocketNamespace \(em the namespace of the interface's UDP sockets,
though not the interface itself.

> +            NetNS) NETNS="$value"; continue ;;

Might need some validation after?

> +    if [[ -n $NETNS ]]; then
> +        if ! ip netns pids "${NETNS}" > /dev/null; then
> +            ret=$?
> +            echo "[!] Target namespace '${NETNS}' not found"
> +            exit $ret

`ip -n NS` will fail if NS doesn't exist, right? Might as well allow
that to fail organically rather than the TOCTOU here.

> +        elif ! cmd ip -n "${NETNS}" link add "$INTERFACE" type
> wireguard; then

Rather than putting the whole thing in an if [[ -n $NETNS ]] like
that, you can instead use this pattern:

local knetns unetns
[[ -z $NETNS ]] || knetns="-n $NETNS" unetns="ip netns exec $NETNS"
cmd ip $knetns link add ...
cmd $unetns wireguard-go ...

Also, ${NETNS} --> $NETNS.

> +            ret=$?
> +            [[ -e /sys/module/wireguard ]] || ! command -v
> "${WG_QUICK_USERSPACE_IMPLEMENTATION:-wireguard-go}" >/dev/null && exit $ret

That should be run in the right namespace, right? via `ip netns exec`

> +        cmd ip -n "${NETNS}" link set "$INTERFACE" netns 1

Using "1" here isn't correct, since we want people to be able to run
wg-quick itself in a network namespace. Instead use `$$`, which will
expand to the shell's PID, which has an associated netns.
_______________________________________________
WireGuard mailing list
WireGuard@lists.zx2c4.com
https://lists.zx2c4.com/mailman/listinfo/wireguard