New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
wireguard: add support for default routing #31250
Conversation
@zx2c4 I wonder, if we should use |
@Mic92 I'd advise against using This code needs my copyright line and license re-added, however, since a lot of that is very clearly copy and pasted. Also, it's not okay to ship distro-specific tools that use the |
@@ -0,0 +1,48 @@ | |||
#! /usr/bin/env bash |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This has code from wg-quick and therefore needs my copyright readded.
# Copyright (C) 2016-2017 Jason A. Donenfeld <Jason@zx2c4.com>. All Rights Reserved.
#
# This file is licensed under the GPLv2 as published by the Free Software Foundation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Somebody must have already told you that "All Rights Reserved" contradicts the rights granted by GPLv2… There is an anecdote about that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
zx2c4@thinkpad ~/Projects/linux $ grep -ri 'all rights reserved' | wc -l
8372
Have your lawyers talk to the Linux Foundation's lawyers about that then. I'm more likely to go with whatever decision lawyers make than with what a random guy on a github issue claims.
Until that happens, you can respect my copyright line, or not use my code. I won't waste my time arguing fake legal nonsense on a github issue.
@@ -227,13 +227,16 @@ let | |||
|
|||
(optionals (values.allowedIPsAsRoutes != false) (map (peer: | |||
(map (allowedIP: | |||
"${ipCommand} route replace ${allowedIP} dev ${name} table ${values.table}" | |||
if (allowedIP == "0.0.0.0/0") || (builtins.match "[0:]+/0" allowedIP != null) | |||
then "${pkgs.wireguard}/bin/wg-default add ${name} ${allowedIP}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please do not ship distro-specific tools that use the wg-
prefix namespace. This will likely clash with upstream.
Instead, call your utility something like: nixos-set-wg-default-route
-- long, descriptive, distro-specific.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, this added behavior that isn't configurable and magically kicks in when specific allowedIPs are set is undesirable because not everyone would want to use fwmarks and ip rules in this case.
@@ -63,6 +63,8 @@ let | |||
postInstall = '' | |||
substituteInPlace $out/lib/systemd/system/wg-quick@.service \ | |||
--replace /usr/bin $out/bin | |||
substituteAll ${./wg-default.sh} "$out"/bin/wg-default | |||
chmod a+x "$out"/bin/wg-default |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wg-default
is not appropriate as a name. See earlier comment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We already have the allowedIPsAsRoutes
option that disables adding routes to the default routing table. Also, all commands can be executed in the postSetup
script option.
Exposing the fwmark wg setting as a NixOS option and setting the mentioned options to the specific use case would be a better solution IMHO.
@@ -227,13 +227,16 @@ let | |||
|
|||
(optionals (values.allowedIPsAsRoutes != false) (map (peer: | |||
(map (allowedIP: | |||
"${ipCommand} route replace ${allowedIP} dev ${name} table ${values.table}" | |||
if (allowedIP == "0.0.0.0/0") || (builtins.match "[0:]+/0" allowedIP != null) | |||
then "${pkgs.wireguard}/bin/wg-default add ${name} ${allowedIP}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, this added behavior that isn't configurable and magically kicks in when specific allowedIPs are set is undesirable because not everyone would want to use fwmarks and ip rules in this case.
@fpletz That seems like a reasonable policy decision from NixOS's perspective. In Wondering what kind of configuration nobs you envision for doing the fwmark dance in an optional way. What will their behaviors be? What is a reasonable way to approach that? I'm interested in seeing what you guys do here, since it will likely be an inspiration of sorts for other network management utilities. |
Closing. I think the implementation here has some hints as to the right way to go about it, but this PR itself isn't the solution. |
Motivation for this change
Currently, when the allowedIP is set to 0.0.0.0/0 or ::/0, the default route is replaced to go through the wireguard wireguard. This is undesirable because the packets from the wireguard interface itself is looped back into the interface, making the interface unable to connect to its endpoint(s). This change replicates the bahaviour of the wg-quick script in this case, by routing all packets with a specific fwmark set by the wireguard interface to the default interface.
Things done
build-use-sandbox
innix.conf
on non-NixOS)nix-shell -p nox --run "nox-review wip"
./result/bin/
)