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

nixos/modules/networking/wg-quick Add wg-quick options support #53043

Merged
merged 1 commit into from Apr 24, 2019

Conversation

exi
Copy link
Contributor

@exi exi commented Dec 29, 2018

This is an implementation of wireguard support using wg-quick config
generation.

This seems preferable to the existing wireguard support because
it handles many more routing and resolvconf edge cases than the
current wireguard support.

I'm aware that there are situations where you want the explicit control over wireguard provided by the existing networking.wireguard module but other distributions are also shipping wg-quick@.service units to make it very hard to do the wrong thing.

Our hand baked wireguard setups already diverge from wg-quick in significant ways and for the worse.
Wrong ipv6 routing and missing dns handling make it very easy to accidentally pass traffic past wireguard even if allowedIps is set to ::/0.

I'm not advocating to remove networking.wireguard but i would like to offer a way to get the "easy and secure by default" behavior that wg-quick provides.

It also includes workarounds to make key files work.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option 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/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Assured whether relevant documentation is up to date
  • Fits CONTRIBUTING.md.

@exi
Copy link
Contributor Author

exi commented Dec 29, 2018

@fpletz pretty sure you will have some opinion on this as well.

description = "The IP addresses of the interface.";
};

dns = mkOption {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this use networking.nameservers?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No because wg-quick overrides everything by design and only while the VPN tunnel is up.
This ensures that users don't leak dns queries

@exi
Copy link
Contributor Author

exi commented Dec 30, 2018

@zx2c4 I say you doing some similar work only after i opened this PR.
Any thoughts?

I feel like using wg-quick to get the default and safe behaviour around routing/dns/etc. is not a bad approach.
This should help to avoid "works on my machine" issues between distros.

@exi exi force-pushed the wg-quick branch 2 times, most recently from a82dddf to daba929 Compare December 30, 2018 14:21
@picnoir
Copy link
Member

picnoir commented Jan 23, 2019

Hey @exi, are you still working on this?

@exi
Copy link
Contributor Author

exi commented Jan 23, 2019

@NinjaTrappeur sure but nobody responded to my PR

@zx2c4
Copy link
Contributor

zx2c4 commented Jan 23, 2019

Sorry for the delay in responding here. I haven't read the commit yet but I vaguely recall being poked, at some point, about my opinion replacing the nixos backend with a wg-quick powered backend.

My general feeling about this is that wg-quick is a pile of hacks, and that distributions that already have control over the networking stack can do much better with something custom and focused. I'm happy to help the nix module if there are particular problems with it that need fixing.

Alternatively, if man power is short and you do prefer just relying on the wg-quick stuff, I guess that's fine too, but it probably won't be as slick as the optimal nix-centric integration.

There was some discussion in #31250 IIRC.

@picnoir
Copy link
Member

picnoir commented Jan 23, 2019

See also #52411

@exi
Copy link
Contributor Author

exi commented Jan 28, 2019

I have been using this approach for some time now and would like to have it merged.
I agree that it would be ideal to have a proper solution the nixos way but would also like to point out that VPNs are used very differently.
The way i set this up currently is that you can freely start/stop the VPN systemd unit to enable/disable the VPN.

We would have to replicate something like this because for many VPN use-cases, people don't want to invoke nix just to enable/disable their VPN.

This is an implementation of wireguard support using wg-quick config
generation.

This seems preferrable to the existing wireguard support because
it handles many more routing and resolvconf edge cases than the
current wireguard support.

It also includes work-arounds to make key files work.

This has one quirk:
We need to set reverse path checking in the firewall to false because
it interferes with the way wg-quick sets up its routing.
@exi
Copy link
Contributor Author

exi commented Apr 20, 2019

@globin ping

associated routes to. Setting this is useful for e.g. policy routing
("ip rule") or virtual routing and forwarding ("ip vrf"). Both numeric
table IDs and table names (/etc/rt_tables) can be used. Defaults to
"main".
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should the default be declared in the option's default instead?

@globin
Copy link
Member

globin commented Apr 24, 2019

I'd prefer this to use the same option names as in networking.wireguard.

Otherwise I think this preferable to networking.wireguard for simple setups and should be included for the time being. Most probably this will become obsolete when we switch to networkd by default, but this can easily be deprecated in that case.

@exi
Copy link
Contributor Author

exi commented Apr 24, 2019

@globin I would prefer to keep the names identical to every wg-quick config out there but i don't really have a strong opinion. If you still prefer to make it match our own special snowflake names i could do that as well.
Just tell me which one you prefer.

@globin
Copy link
Member

globin commented Apr 24, 2019

Ah then fine by me, when deprecating we can easily add the renames.

@globin globin merged commit b2c1ed6 into NixOS:master Apr 24, 2019
@asymmetric
Copy link
Contributor

Would it make sense to backport this to 19.03?

@andrew-d
Copy link
Contributor

I would love to see this backported; it's a huge quality-of-life improvement for using Wireguard, and it doesn't break anything.

environment.systemPackages = [ pkgs.wireguard-tools ];
# This is forced to false for now because the default "--validmark" rpfilter we apply on reverse path filtering
# breaks the wg-quick routing because wireguard packets leave with a fwmark from wireguard.
networking.firewall.checkReversePath = false;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think a service module should silently change the firewall behaviour

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd like to request @fpletz opinion on this :
is it something we should keep as-is for the next release?

@asymmetric
Copy link
Contributor

Should this module configure the firewall to allow incoming UDP packets on the specified port?

@c0bw3b
Copy link
Contributor

c0bw3b commented May 1, 2019

Not automatically, the policy is that opening ports should always be an explicit user configuration (SSH being the only exception to avoid locking oneself out of a remote machine).

The module description could indicate that additional firewall config is needed.
BTW I have not tested wg but I'm not sure I see why wg marks are not considered valid marks. Does setting checkReversePath to loose change anything?

@exi
Copy link
Contributor Author

exi commented May 1, 2019

The module description could indicate that additional firewall config is needed.
BTW I have not tested wg but I'm not sure I see why wg marks are not considered valid marks. Does setting checkReversePath to loose change anything?

The reason it is not considered a valid mark is because we receive a packet from the network without a mark but sent the response with a mark because wg-quick uses firewall marks only for outgoing packages because it makes it really easy to bypass the default routing table.
It needs to bypass the default routing table because you can configure wireguard to be your default route. In this case, all outgoing packets would use the wireguard default route except the ones with the firewall mark.

However this causes us to use a different return path from the perspective of rp_filter, which considers (incoming, nomark) and (outgoing, mark) as two different paths as long as --validmark is set. And if it considers these to be two different paths, it will drop the packets.

@exi
Copy link
Contributor Author

exi commented May 6, 2019

Should this module configure the firewall to allow incoming UDP packets on the specified port?

Not sure, for wg-quick we have outgoing packets first. So incoming packets would fall into a RELATED state and i presume (without checking) that related packets are already allowed.

@asymmetric
Copy link
Contributor

Is it not supposed to be used on the server?

@exi
Copy link
Contributor Author

exi commented May 6, 2019

Is it not supposed to be used on the server?

Yes you are right. We may need some complex logic to figure out what port to open where.
I honestly would leave that to the user to explicitly open a port. I don't think this is something we could get right by default.

@asymmetric
Copy link
Contributor

Yeah I don't think we should open a port either, as it seems to be against the general guidelines.

Maybe just mention in the module that whatever port one specifies should be opened for a server-like peer.

@andrew-d
Copy link
Contributor

andrew-d commented May 6, 2019

Maybe have a configureNetworking option that defaults to false? That way, it's untouched by default, but if you're just doing the simple case of enabling a Wireguard server, you can set one option and it opens the right listenPort (if given), sets checkReversePath = false, etc. etc.? If we wanted to get really fancy, we could also create a systemd unit that waits for the Wireguard interface to come up, dynamically looks up the listening port (for cases where you don't specify one), and then inserts a firewall rule?

I dunno; no strong opinions here, but I think it's worth optimizing for the easy case here; most people that set up a Wireguard server will need to set the rpfilter/port networking options anyway.

Other thoughts:

  • mkDefault for checkReversePath in this module?
  • assertions to ensure that it's set in the cases where we know that it's needed?

@c0bw3b
Copy link
Contributor

c0bw3b commented May 6, 2019

Maybe just mention in the module that whatever port one specifies should be opened for a server-like peer.

Yes that's the usual approach for modules that needs some firewall rules : description should explain it has to be configured by the user. Then it's left to the user to open the right port on the right interfaces (or all interfaces) according to his own setup.

  • assertions to ensure that it's set in the cases where we know that it's needed?

That could be an extra precaution.

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

9 participants