nixos/wireguard add endpointFile option

[mutantmell]

Description of changes

There are two motivating scenarios for this change:

1. In scenarios where the peer's endpoint might change over time, such as with a dynamic ip address, this provides a mechanism for communicating that change via updating a file and restarting wireguard, typically with a small systemd daemon.

2. This provides a way to interate with peers that may not want ip addresses or endpoints to be solicited in public configuration files.

I have been running this change on a server with no noticed issues, see here: https://github.com/mutantmell/dotfiles/blob/main/modules/overrides/wireguard.nix

Things done

• Built on platform(s)
  • x86_64-linux
  • aarch64-linux
  • x86_64-darwin
  • aarch64-darwin
• For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
• Tested, as applicable:
  • NixOS test(s) (look inside nixos/tests)
  • and/or package tests
  • or, for functions and "core" functionality, tests in lib/tests or pkgs/test
  • made sure NixOS tests are linked to the relevant packages
• Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
• Tested basic functionality of all binary files (usually in ./result/bin/)
• 23.05 Release Notes (or backporting 22.11 Release notes)
  • (Package updates) Added a release notes entry if the change is major or breaking
  • (Module updates) Added a release notes entry if the change is significant
  • (Module addition) Added a release notes entry if adding a new NixOS module
• Fits CONTRIBUTING.md.

[GabrielDougherty]

question: does this support IPv6? say I put this in the endpointFile:

[2001:db8::1]:23542

would that work?

[mutantmell]

I made a local change to the file-config test to make it use IPv6, and it worked. In general, this should support anything that's usable by directly specifying it in endpoint.

[GabrielDougherty]

Diff looks good to me

[GabrielDougherty]

suggestion (non-blocking): add a test that uses an endpoint file with an IPv6 address in it

[mutantmell]

I could modify the existing test to have peer0 specify peer1's ipv6 endpoint, would that suffice, or is that too much in a single test?

[GabrielDougherty]

would that suffice

Yes - IMO that is fine since there are existing tests that don't use the IP file

[RaitoBezarius]

I am sorry, I don't understand very well the motivations of these changes.

First, there is a mention of changing IPs and DNS resolution, why the set of dynamicEndpointRefresh options not enough for this usecase?

Second, endpoint not ending up in public configuration, it seems to me that this is a completely different matter. Why not doing something like builtins.readFile ./endpoint.txt and not checking in the endpoint.txt file?

Also, if you want to provide this completely at runtime, I would argue this might be beyond this module's scope and maybe we should check if wireguard in systemd-networkd support or not this usecase?

[mutantmell]

I'm going to respond to these out of order:

Also, if you want to provide this completely at runtime, I would argue this might be beyond this module's scope and maybe we should check if wireguard in systemd-networkd support or not this usecase?

The answer for this depends:

1. From the perspective of the configuration available in the network file, the answer is no: the endpoint is only configurable as a constant.
2. From the perspective of "is networkd designed to handle dynamic situations", the answer is yes: that's that the /run/systemd/network/ volatile directory is intended for.

Second, endpoint not ending up in public configuration, it seems to me that this is a completely different matter. Why not doing something like builtins.readFile ./endpoint.txt and not checking in the endpoint.txt file?

I consider this to be an anti-pattern; I lost a computer recently that was configured using this pattern (have a secrets folder in .gitignore), and had I not been able to recover the hard drive I would have had a much harder time recovering. I've migrated to sops-nix in my current dotfiles, and I've used NixOps historically, both of which use files deployed to /run/secrets as the common mechanism for secrets management. The change here was relatively small, so I made it locally and wanted to see if there was interest in maintaining up upstream.

(I'm using flakes, which broke support for git-crypt, so that option is out of the window).

First, there is a mention of changing IPs and DNS resolution, why the set of dynamicEndpointRefresh options not enough for this usecase?

I mean dynamic ip address as in "my ISP changes my assigned ipv4 every month", and having a file to write that (which triggers a refresh of networkd) is a workflow that would work well for me. I consider this to be a mostly-secondary concern, the primary concern is to respect people's request for privacy with my desire to have a public backup of my config on github for potential disaster recovery :)

Here's the kicker: If this contribution doesn't meet the quality bar, I'm perfectly happy to maintain this fork of upstream locally. I hadn't realized that the /run/systemd/network directory existed for this sort of use-case, so I might just change tack and do that. Let me know if you think this is out of scope for what this package is doing, or if there's anything you want/need from me in case you do want it.

[RaitoBezarius]

I'm going to respond to these out of order:

Also, if you want to provide this completely at runtime, I would argue this might be beyond this module's scope and maybe we should check if wireguard in systemd-networkd support or not this usecase?

The answer for this depends:

1. From the perspective of the configuration available in the network file, the answer is no: the endpoint is only configurable as a constant.
2. From the perspective of "is networkd designed to handle dynamic situations", the answer is yes: that's that the /run/systemd/network/ volatile directory is intended for.

First, there is a mention of changing IPs and DNS resolution, why the set of dynamicEndpointRefresh options not enough for this usecase?

I mean dynamic ip address as in "my ISP changes my assigned ipv4 every month", and having a file to write that (which triggers a refresh of networkd) is a workflow that would work well for me. I consider this to be a mostly-secondary concern, the primary concern is to respect people's request for privacy with my desire to have a public backup of my config on github for potential disaster recovery :)

Thank you for the long exposition of the motivations, it's really helpful to understand where you're coming from.

Second, endpoint not ending up in public configuration, it seems to me that this is a completely different matter. Why not doing something like builtins.readFile ./endpoint.txt and not checking in the endpoint.txt file?

I consider this to be an anti-pattern; I lost a computer recently that was configured using this pattern (have a secrets folder in .gitignore), and had I not been able to recover the hard drive I would have had a much harder time recovering. I've migrated to sops-nix in my current dotfiles, and I've used NixOps historically, both of which use files deployed to /run/secrets as the common mechanism for secrets management. The change here was relatively small, so I made it locally and wanted to see if there was interest in maintaining up upstream.

(I'm using flakes, which broke support for git-crypt, so that option is out of the window).

It seems to me this feature is built out of piling on the top of workarounds and workarounds because sanctioned ways to do things are not working as intended, alas.

I do not have authority to say no to a change, that being said, accepting such a change means that similar change in similar areas are fair game and I certainly do not want multiplication of similar options to work around similar problems, hence my question (and the fact I am not really in favor of).

Here's the kicker: If this contribution doesn't meet the quality bar,

First of all, there's no quality bar beyond the one that reviewers put efforts into asking you to get the PR to and I do think it is possible to make the PR in good shape.

This is more of an architecture design problem.

It seems like the usecase is better covered by manual invocations of wg set xx endpoint yy somewhat, but again, I get it that it's a bit annoying to inject stuff manually and all.

I'm perfectly happy to maintain this fork of upstream locally. I hadn't realized that the /run/systemd/network directory existed for this sort of use-case, so I might just change tack and do that. Let me know if you think this is out of scope for what this package is doing, or if there's anything you want/need from me in case you do want it.

Honestly, I feel like this is a OK-change because it's quite small and you could make it so that static endpoint could be turned into endpointFile by writing a path in the Nix store and endpointFile could exist without any concern regarding the mutual exclusive logic because it would fail for redefinition.

It is just I am not in favor of it, but this is a weak opinion.

[mutantmell]

It seems to me this feature is built out of piling on the top of workarounds and workarounds because sanctioned ways to do things are not working as intended, alas.

That's a bit how I feel too, but I also feel like various parts of NixOS have embraced files as a communication method for sensitive information, so this feels like it might fit, and might have a home upstream.

This is more of an architecture design problem.

That's a large part of what I mean by "quality bar" -- does it fit the overall vision for the ecosystem? I don't know! To me, a successful end for this PR could easily be "thanks, but this doesn't fit what we want for upstream!"

Honestly, I feel like this is a OK-change because it's quite small and you could make it so that static endpoint could be turned into endpointFile by writing a path in the Nix store and endpointFile could exist without any concern regarding the mutual exclusive logic because it would fail for redefinition.

I'd be happy to make this change, but I will wait until someone else weighs in on whether or not it is wanted.

And thanks again for taking the time to review this!

[RaitoBezarius]

It seems to me this feature is built out of piling on the top of workarounds and workarounds because sanctioned ways to do things are not working as intended, alas.

That's a bit how I feel too, but I also feel like various parts of NixOS have embraced files as a communication method for sensitive information, so this feels like it might fit, and might have a home upstream.

This is more of an architecture design problem.

That's a large part of what I mean by "quality bar" -- does it fit the overall vision for the ecosystem? I don't know! To me, a successful end for this PR could easily be "thanks, but this doesn't fit what we want for upstream!"

Honestly, I feel like this is a OK-change because it's quite small and you could make it so that static endpoint could be turned into endpointFile by writing a path in the Nix store and endpointFile could exist without any concern regarding the mutual exclusive logic because it would fail for redefinition.

I'd be happy to make this change, but I will wait until someone else weighs in on whether or not it is wanted.

And thanks again for taking the time to review this!

Thank you for your answer, I have no real strong opinion, I am interested into @ncfavier opinion who reviewed a lot of change for this module IIRC.

Don't hesitate to send the PR on Matrix or Discourse if you want some people chiming in.

[zx2c4]

This seems very weird and niche. Just use wg set ... like everyone else for this. You can even do that from PostUp.

[mutantmell]

This seems very weird and niche. Just use wg set ... like everyone else for this. You can even do that from PostUp.

That's where this PR came from -- the wg set command I was building up ended up looking a lot like the command that was built up in this module. I used a file to store the endpoint for reasons discussed above, so I made a small change to this module, and wanted to see if upstream wanted it, which they clearly do not.