From 3055adce36acb45d109bc8425529ebfb7e2cfb1d Mon Sep 17 00:00:00 2001 From: Sofiane Date: Fri, 28 Aug 2026 22:08:31 +0200 Subject: [PATCH] fix(vpn/ipsec): bound shutdown when charon-nm ignores SIGTERM `charon-nm` (strongSwan's NetworkManager plugin subprocess) does not always handle SIGTERM correctly, in particular when a VPN connection attempt hasn't fully completed. When that happens it keeps running inside NetworkManager's cgroup even after NetworkManager.service itself reports as stopped (its `Type=dbus` completion condition -- losing its D-Bus name -- doesn't require its whole cgroup to be empty). Setting NetworkManager.service's own TimeoutStopSec is not enough: since the unit is already considered stopped as soon as its main process exits, that per-unit timer never gets a chance to force-kill the leftover charon-nm. What actually hangs shutdown/reboot is a different, later mechanism: once every unit has been stopped, systemd's low-level systemd-shutdown does one final SIGTERM-then- SIGKILL sweep over any processes still around, and the wait before that SIGKILL is governed by DefaultTimeoutStopSec (system-wide default: 90s), not any per-unit TimeoutStopSec. Bound both: keep NetworkManager.service's own TimeoutStopSec (bounds `systemctl stop NetworkManager.service` in isolation, documents intent) and lower the system-wide DefaultTimeoutStopSec, which is what actually bounds the real-world hang. Add a regression test that simulates a charon-nm process stuck ignoring SIGTERM in NetworkManager's cgroup and asserts a full machine shutdown completes well within the fixed timeout instead of the default 90s. Closes #205 --- modules/vpn/ipsec/networkmanager.nix | 27 ++++++ tests/default.nix | 1 + tests/vpn-ipsec-shutdown.nix | 120 +++++++++++++++++++++++++++ 3 files changed, 148 insertions(+) create mode 100644 tests/vpn-ipsec-shutdown.nix diff --git a/modules/vpn/ipsec/networkmanager.nix b/modules/vpn/ipsec/networkmanager.nix index 37c4d800..a078ec77 100644 --- a/modules/vpn/ipsec/networkmanager.nix +++ b/modules/vpn/ipsec/networkmanager.nix @@ -231,6 +231,33 @@ in } ]; + # `charon-nm` (strongSwan's NetworkManager VPN plugin subprocess) does not always + # handle SIGTERM correctly, in particular when a VPN connection attempt hasn't + # fully completed. When that happens, it keeps running inside NetworkManager's + # cgroup even after NetworkManager.service itself reports as stopped (its + # `Type=dbus` completion condition -- losing its D-Bus name -- doesn't require + # its whole cgroup to be empty), showing + # "NetworkManager.service: Unit process N (charon-nm) remains running after unit + # stopped." This is the same underlying strongSwan bug as the suspend/resume + # workaround above; it's tracked upstream (e.g. + # https://bugs.archlinux.org/task/68116) and we don't have the resources to fix + # strongSwan itself. + # + # Setting NetworkManager.service's own TimeoutStopSec is not sufficient here: + # since the unit is already considered stopped as soon as its main process + # exits, that per-unit timer never gets a chance to force-kill the leftover + # charon-nm. It's kept below anyway (bounds `systemctl stop + # NetworkManager.service` itself, and documents intent), but the process the + # bug report is actually about -- shutdown/reboot hanging on "waiting for + # charon-nm to stop" -- is bound by a *different*, later mechanism: once every + # unit has been stopped, systemd's low-level `systemd-shutdown` does one final + # SIGTERM-then-SIGKILL sweep over any processes still around, and the wait + # before that SIGKILL is `DefaultTimeoutStopSec` (system-wide default: 90s), + # not any per-unit TimeoutStopSec. Lower that instead to actually bound + # shutdown/reboot. See https://github.com/cloud-gouv/securix/issues/205. + systemd.services.NetworkManager.serviceConfig.TimeoutStopSec = mkDefault "10s"; + systemd.settings.Manager.DefaultTimeoutStopSec = mkDefault "10s"; + environment.etc = { "strongswan.conf".text = '' charon-nm { diff --git a/tests/default.nix b/tests/default.nix index 3f6f74b8..6d319281 100644 --- a/tests/default.nix +++ b/tests/default.nix @@ -9,4 +9,5 @@ idempotent-autoinstall = import ./idempotent-autoinstall.nix { inherit pkgs libSecurix; }; portail = import ./portail.nix { inherit pkgs libSecurix; }; tools = import ./tools.nix { inherit pkgs libSecurix; }; + vpn-ipsec-shutdown = import ./vpn-ipsec-shutdown.nix { inherit pkgs libSecurix; }; } diff --git a/tests/vpn-ipsec-shutdown.nix b/tests/vpn-ipsec-shutdown.nix new file mode 100644 index 00000000..005c85c5 --- /dev/null +++ b/tests/vpn-ipsec-shutdown.nix @@ -0,0 +1,120 @@ +# SPDX-FileCopyrightText: 2025 Antoine Eiche +# SPDX-FileContributor: 2026 Sofiane Beloucif +# +# SPDX-License-Identifier: MIT + +# Regression test for https://github.com/cloud-gouv/securix/issues/205: +# a hung `charon-nm` process (strongSwan's NetworkManager IPsec plugin) must +# not block `NetworkManager.service` from stopping for more than a bounded +# amount of time. +{ pkgs, libSecurix }: +let + terminal = libSecurix.mkTerminal { + name = "vpn-ipsec-shutdown"; + userSpecificModule = { }; + vpnProfiles = { }; + modules = [ + { + securix = { + # graphical-interface.variant is read unconditionally by some + # sub-modules (e.g. cinnamon.nix) regardless of `enable`, so a + # value must be provided even though we don't need a graphical + # session for this test. + graphical-interface.variant = "sway"; + self = { + mainDisk = "/dev/nvme0n1"; + machine = { + hardwareSKU = "x280"; + inventoryId = 0; + }; + }; + vpn.ipsec.enable = true; + }; + } + ]; + }; +in +pkgs.testers.nixosTest { + name = "vpn-ipsec-shutdown"; + nodes = { + securix-unbranded-0 = { + imports = terminal.modules; + # The default test VM (1 vCPU / 1024 MiB) is too tight for Sécurix's + # full module closure (in particular the sizeable package set pulled + # in unconditionally by modules/tools) under nested virtualisation, + # where udev/systemd device processing can fall far enough behind to + # blow past the default device-unit timeouts. Give the VM more room. + virtualisation.memorySize = 4096; + virtualisation.cores = 4; + }; + }; + testScript = '' + import time + + securix_unbranded_0.wait_for_unit("NetworkManager.service") + + # Our mitigation must bound how long systemd waits for + # NetworkManager.service to stop... + securix_unbranded_0.succeed( + "systemctl show NetworkManager.service -p TimeoutStopUSec | grep -q '=10s'" + ) + # ...but what actually bounds shutdown/reboot itself is the system-wide + # default, since systemd-shutdown's final kill sweep isn't governed by + # any per-unit TimeoutStopSec. See the comment in + # modules/vpn/ipsec/networkmanager.nix for why. + securix_unbranded_0.succeed( + "systemctl show -p DefaultTimeoutStopUSec | grep -q '=10s'" + ) + + # Simulate a `charon-nm` process still alive in NetworkManager's cgroup + # and ignoring SIGTERM, like the real strongSwan bug this mitigates. + # Use a real file named `charon-nm` (rather than `exec -a` to rename + # argv[0]) so its `comm` is "charon-nm" regardless of the shell used to + # launch it. + # + # It must never fork a child to sleep/wait: a forked child is placed in + # whatever cgroup its parent is in *at fork time*. Since we move this + # process into NetworkManager's cgroup only *after* spawning it (see + # below), a child forked beforehand would stay in its original cgroup + # and never receive the SIGTERM systemd broadcasts to NetworkManager's + # cgroup -- only the process directly moved there does. A signal-trapping + # busy loop avoids forking altogether, so it (and only it) reliably + # keeps ignoring SIGTERM once moved. + # + # NOTE: the write/chmod and the backgrounded spawn are deliberately two + # separate machine.succeed() calls. The test driver's execute() pipes the + # command's stdout through `base64 -w0; echo` and waits for that pipe to + # close. Backgrounding a whole `a && b && c &` chain in a single call + # backgrounds the *entire* and-or list as one job; only the final command + # (setsid ...) has its own local `>/dev/null` redirection, so the pipe + # ends up not fully closing and execute() hangs forever. Backgrounding + # only the already fully-redirected `setsid ...` command by itself, as + # its own call, avoids that. + securix_unbranded_0.succeed( + "printf '#!/bin/sh\\ntrap : TERM\\nwhile :; do :; done\\n' > /root/charon-nm && " + "chmod +x /root/charon-nm" + ) + securix_unbranded_0.succeed("setsid /root/charon-nm /dev/null 2>&1 &") + securix_unbranded_0.wait_until_succeeds("pgrep -x charon-nm") + securix_unbranded_0.succeed( + "pid=$(pgrep -x charon-nm); " + "echo $pid > /sys/fs/cgroup/system.slice/NetworkManager.service/cgroup.procs" + ) + + # Actually shut the machine down, rather than just `systemctl stop`ping + # NetworkManager.service in isolation: the real bug manifests during the + # system-wide shutdown sequence, where systemd waits for every unit's + # cgroup to empty before it can proceed. A standalone `systemctl stop` + # can return as soon as the unit's main process exits, without actually + # waiting on stray processes like our simulated charon-nm, so it would + # not reliably exercise (or catch a regression in) TimeoutStopSec here. + start = time.monotonic() + securix_unbranded_0.shutdown() + elapsed = time.monotonic() - start + + assert elapsed < 60, ( + f"shutting down took {elapsed:.1f}s, expected it to be bounded by " + "TimeoutStopSec (10s) rather than the default 90s" + ) + ''; +}