Skip to content
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

Closed
wants to merge 1 commit into from

Conversation

shaunren
Copy link
Contributor

@shaunren shaunren commented Nov 4, 2017

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
  • Tested using sandboxing (nix.useSandbox on NixOS, or option build-use-sandbox in nix.conf on non-NixOS)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nox --run "nox-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Fits CONTRIBUTING.md.

@Mic92
Copy link
Member

Mic92 commented Nov 7, 2017

@zx2c4 I wonder, if we should use wg-quick instead.

@zx2c4
Copy link
Contributor

zx2c4 commented Nov 8, 2017

@Mic92 I'd advise against using wg-quick. Baking wg(8), not wg-quick(8), directly into your network management tool is how wireguard was meant to be used.

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 wg- prefix namespace. This will likely clash with upstream. Instead, call your utility something like: nixos-set-wg-default-route -- long, descriptive, distro-specific.

@@ -0,0 +1,48 @@
#! /usr/bin/env bash
Copy link
Contributor

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.

Copy link
Contributor

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.

Copy link
Contributor

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}"
Copy link
Contributor

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.

Copy link
Member

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
Copy link
Contributor

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.

@globin globin requested a review from fpletz November 8, 2017 10:26
Copy link
Member

@fpletz fpletz left a 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}"
Copy link
Member

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.

@zx2c4
Copy link
Contributor

zx2c4 commented Nov 9, 2017

@fpletz That seems like a reasonable policy decision from NixOS's perspective. In wg-quick(8), I do the magic thing because that's just a quick and dirty bash script (originally from the examples/ directory!) that isn't supposed to be a real network manager. But in Nix, you really want things to have sensible configuration.

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.

@grahamc
Copy link
Member

grahamc commented Aug 1, 2019

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants