wireguard: non-invasive fix for permanent disconnects on unstable network (e.g. laptops) from dyndns endpoints

[seb314]

Motivation for this change

When dns resolution fails with a permanent error ("Name or service not
known" instead of "Temporary failure in name resolution"), in the current
setup for dynamic dns with dynamicEndpointRefreshSeconds, wireguard
won't retry despite WG_ENDPOINT_RESOLUTION_RETRIES=infinity.

Ideally, dns would probably never report a permanent error for an
existing name, but unfortunately this does happen (maybe especially
with dynamic dns?) and cannot easily be fixed by the wireguard setup's
admin.

I can't think of a scenario where it is essential to not retry after a
negative dns response (given that the endpoint has been configured, the
dns name quite certainly exists), right?. On the other hand, a machine
that drops out of the vpn can be very annoying...

-> This change should improve reliability/connectivity.

somewhat related thread: #63869

Things done

• Built on platform(s)
  • x86_64-linux (built & deployed a cherry-picked version on top of roughly nixos-unstable)
  • aarch64-linux
  • x86_64-darwin
  • aarch64-darwin
• For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
• Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
• Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
• Tested execution of all binary files (usually in ./result/bin/)
• 21.11 Release Notes (or backporting 21.05 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.

[jian-lin]

Thanks for your work.

I also have this issue. Say wireguard is set on your laptop, wireguard-{interface}-peer-{publickey}-refresh.service will become dead if your laptop lose network connection (e.g. switch to a new wifi) or your laptop doesn't connect to the network before wireguard-{interface}-peer-{publickey}-refresh.service starts. The latter case is possible if you don't set a connection available for all users in NetworkManager, in which case network-online.target will be reached before a connection is established.

wireguard won't retry despite WG_ENDPOINT_RESOLUTION_RETRIES=infinity. Ideally, dns would probably never report a permanent error for an existing name, but unfortunately this does happen (maybe especially with dynamic dns?)

According to the code, wireguard won't retry if it gets the EAI_NONAME error ( the corresponding error message is "Name or service not known" ), which will appear when the network connection is lose and is considered as a permanent error instead of a transient one.

Adding retry in the systemd unit does fix this issue. I do not known if there is a better way, like only retrying when the network is back online.

[seb314]

Thanks for you further analysis @jian-lin !

I guess the basic periodic retry shouldn't cause significant system load unless there is a really large number of peers with dynamic IPs, but something smarter might be nicer (e.g. exponential backoff or when network connectivity is restored, as you suggested). We should be sure that such "smartness" doesn't introduce a new edge case where we accidentally fail to retry, though.

[stale]

I marked this as stale due to inactivity. → More info

[zarelit]

I'm having the same problem, exactly on a laptop with no permanent connection. If time allows I should be able to review soon.

@jian-lin I think it's okay to keep the same interval, after all the invariant is "I want this peer to be refreshed every X seconds" and when the connection is brought back up you can expected it to have the right IP after X seconds

[jian-lin]

@zarelit I use a better workaround, patching wireguard-tools. By better, I mean wireguard peer services are not restarted. That patch is as follows.

Don't know if there is a way to distinguish between no network connection and EAI_NONAME.

From 1b33a0d5eb3b578767162ef465b5994afeb15dac Mon Sep 17 00:00:00 2001
From: linj <linj.dev@outlook.com>
Date: Wed, 24 Nov 2021 21:48:43 +0000
Subject: [PATCH 1/1] make EAI_NONAME transient

When a notebook loses its network connection, EAI_NONAME will appear if
you look up DNS. If dynamicEndpointRefreshSeconds is enabled, this error
will cause the peer systemd service to fail (but with 0 as exit code due
to the while loop) and clean all your wireguard peers due to the
ExecStopPost command. So your wireguard network will not work even after
your network is connected again. You have to restart your main wireguard
service to make it work again.

By applying this patch, this program will not exit if it get EAI_NONAME
error, whose error message is "Name or service not known". Instead, it
will treat this error like other transient errors and retry after a few
seconds. So your peer systemd unit will not exit and your wireguard peer
will not be cleaned. You wireguard network will be useable when you
connect to the network again.
---
 src/config.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/config.c b/config.c
index 211e887..51b90d9 100644
--- a/config.c
+++ b/config.c
@@ -252,7 +252,7 @@ static inline bool parse_endpoint(struct sockaddr *endpoint, const char *value)
 		 *
 		 * So this is what we do, except FreeBSD removed EAI_NODATA some time ago, so that's conditional.
 		 */
-		if (ret == EAI_NONAME || ret == EAI_FAIL ||
+		if (ret == EAI_FAIL ||
 			#ifdef EAI_NODATA
 				ret == EAI_NODATA ||
 			#endif
-- 
2.33.1

[zarelit]

Tested on a intermittently-connected laptop and works.

The script that refreshes the endpoints quits with error when it's not possible to resolve the name of the peer and so the unit dies. This will happen 100% on a laptop with no automatic connection at boot.

The PR does not change the script and just guarantees that the refresh operation is tried every dynamicEndpointRefreshSeconds even when the script fails so it's a safe merge unless I'm missing a usecase where having the peers not refreshed is somehow expected.

Somewhat related to #165474 i.e. lifecycle of wireguard-related units.

Also thanks @seb314

[zarelit]

Also wrt/ #63869 (comment) This means that the restart of the unit is to handle the DNS not available case (e.g. because of lack of connection) and other transient failure are still handled by wg itself

[nixos-discourse]

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/prs-already-reviewed/2617/507

[seb314]

@zarelit thanks for the review!

Is there anything that can be done or improved about this PR that would improve the chances of it getting merged?

[zarelit]

Both @nh2 and @WilliButz worked on dynamically refreshed peers and systemd units that didn't restart in this module so maybe they can help us here

[nixos-discourse]

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/prs-already-reviewed/2617/574

[seb314]

I changed the title to something more descriptive, in case the previous title was part of the reason why the PR has been stuck. (If there is something wrong with the new title, please let me know).

[seb314]

@jian-lin @Majiir I added an (optional) option dynamicEndpointRefreshRestartSeconds to set the retry delay directly.
If the option is not set, the value dynamicEndpointRefreshSeconds will be used, so the default behaviour is unchanged vs the previous version of this PR.

[seb314]

@jian-lin @majir @zarelit could you maybe check if the updated version (with 2821cd9) works for you too?

If it works for you too, and you can mark your review as approved, we might have a better chance to get someone with merge permissions to look at the PR and maybe merge it :-)

[zarelit]

Tried out latest version (2022-09-27) with peers with mixed settings and works well

[jian-lin]

Suggest some small changes.

[jian-lin]

LGTM

[zarelit]

LGTM

[RaitoBezarius]

Looks sane to me (untested as I do not have any dynamic endpoint of this type).

[pwaller]

LGTM, untested but I have this problem and this looks to me like a suitable fix.

[Artturin]

squash the commits

the pr can be tested with https://nixos.wiki/wiki/Nixpkgs/Reviewing_changes#Modules

[seb314]

squash the commits

done

[pwaller]

Friendly ping, would love to see this land.

[pwaller]

🎉 thanks @Artturin !

[github-actions]

Successfully created backport PR #204134 for release-22.11.

[seb314]

Thanks @Artturin and thank you all for review, testing & support!