From ae2909dd0f83d7896803773513ce614a498be486 Mon Sep 17 00:00:00 2001 From: Mathieu Benoit Date: Tue, 8 Sep 2026 09:41:17 -0400 Subject: [PATCH] =?UTF-8?q?[FIX]=20vpn=20:=20ne=20plus=20demander=20la=20r?= =?UTF-8?q?oute=20par=20d=C3=A9faut=20=C3=A0=20qui=20ne=20la=20pose=20pas?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Le formulaire demandait « tout le trafic ? » à un pilote dont le SERVEUR décide du routage, ne faisait rien de la réponse, et `status` la jugeait quand même : un ✗ permanent sur un tunnel sain, et un profil annonçant « tout le trafic » sans l'obtenir. Un drapeau, sur le modèle de celui du MTU, dit quels pilotes posent cette route. Les autres ne sont ni interrogés ni jugés, et un drapeau laissé à vrai n'est plus conservé. L'honorer serait pire qu'inutile : forcer une route par défaut contre une passerelle en tunnel scindé donne un trou noir, une passerelle ne routant pas ce qu'elle n'a pas annoncé. Les routes déclarées, elles, restent honorées — le formulaire le dit. --- EN --- The form asked "all traffic?" of a driver whose SERVER decides the routing, did nothing with the answer, and `status` judged it anyway: a permanent ✗ on a healthy tunnel, and a profile announcing "all traffic" without getting it. A flag, modelled on the MTU one, says which drivers lay that route. The others are neither asked nor judged, and a flag left true is no longer kept. Honouring it would be worse than useless: forcing a default route against a split-tunnel gateway gives a black hole, a gateway not routing what it never advertised. Declared routes are still honoured — the form says so. Assisted-by: Claude Opus 5 --- script/todo/todo_i18n.py | 13 ++++++ script/todo/vpn_menu.py | 23 +++++++++-- script/vpn/README.base.md | 21 ++++++++-- script/vpn/README.fr.md | 9 ++++- script/vpn/README.md | 12 +++++- script/vpn/drivers/base.py | 11 +++++- script/vpn/drivers/openconnect.py | 6 +++ test/test_vpn_drivers.py | 41 +++++++++++++++++++ test/test_vpn_menu.py | 66 +++++++++++++++++++++++++++++++ 9 files changed, 191 insertions(+), 11 deletions(-) diff --git a/script/todo/todo_i18n.py b/script/todo/todo_i18n.py index 0eb16b2..a0fd31a 100644 --- a/script/todo/todo_i18n.py +++ b/script/todo/todo_i18n.py @@ -11810,6 +11810,19 @@ TRANSLATIONS = { " state a dead tunnel left behind." ), }, + "The gateway decides what enters this tunnel. To force a network" + " through it anyway, add it to the routes above \u2014 0.0.0.0/0 for all.": { + "fr": ( + "La passerelle décide de ce qui entre dans ce tunnel. Pour y" + " forcer un réseau tout de même, l'ajouter aux routes" + " ci-dessus — 0.0.0.0/0 pour tout." + ), + "en": ( + "The gateway decides what enters this tunnel. To force a" + " network through it anyway, add it to the routes above —" + " 0.0.0.0/0 for all." + ), + }, "SSO helper": { "fr": "greffon SSO", "en": "SSO helper", diff --git a/script/todo/vpn_menu.py b/script/todo/vpn_menu.py index 2dda221..b9cea94 100644 --- a/script/todo/vpn_menu.py +++ b/script/todo/vpn_menu.py @@ -117,6 +117,14 @@ NOT_CONNECTED_NOTE = ( " a dead tunnel left behind." ) +# Ce qu'on dit à la place de la question « tout le trafic ? », pour un +# pilote dont le SERVEUR décide du routage. Nomme l'échappatoire, sans quoi +# le retrait de la question se lit comme une capacité perdue. +SERVER_ROUTES_NOTE = ( + "The gateway decides what enters this tunnel. To force a network" + " through it anyway, add it to the routes above — 0.0.0.0/0 for all." +) + MASTER_PASSWORD_WARNING = ( "The vault MASTER password is stored in the configuration in clear" " text. Remove it and type it on demand." @@ -566,10 +574,17 @@ class VpnMenuMixin: t("Networks to reach, comma-separated"), ", ".join(draft.get("routes", [])), ) - draft["default_route"] = self._vpn_ask_flag( - t("Send ALL traffic through the tunnel?"), - draft.get("default_route", False), - ) + if driver_cls.uses_default_route: + draft["default_route"] = self._vpn_ask_flag( + t("Send ALL traffic through the tunnel?"), + draft.get("default_route", False), + ) + else: + # Ni demandée, ni conservée : un drapeau laissé à vrai sur un + # pilote qui l'ignore reste un champ sans effet, et le profil + # continuerait d'annoncer « tout le trafic » à la liste. + draft["default_route"] = False + print(f" {t(SERVER_ROUTES_NOTE)}") draft["probe"] = self._vpn_ask( t("Witness address reachable only through the tunnel (optional)"), draft.get("probe", ""), diff --git a/script/vpn/README.base.md b/script/vpn/README.base.md index ae13a87..0d831bc 100644 --- a/script/vpn/README.base.md +++ b/script/vpn/README.base.md @@ -605,8 +605,16 @@ says so when it takes it. unknown server certificate raises a question, and openconnect would read the answer from the standard input the password arrives on. With it, openconnect refuses at once **and** prints the `--servercert sha256:…` line to paste into -the profile's `oc_servercert`. Routes belong to the server, through -`vpnc-script`; the profile can add to them, not replace them. +the profile's `oc_servercert`. + +Routes belong to the server, through `vpnc-script`; the profile can add to +them, not replace them. For that reason the form does **not** ask this +driver « send ALL traffic through the tunnel? », and `status` does not judge +it: the gateway decides what enters the tunnel, and forcing a default route +against a split-tunnel gateway would not give all traffic but a black hole — +a gateway does not route what it never advertised. To force a network +through anyway, add it to `routes`, which this driver does honour, with +`0.0.0.0/0` for everything. Set **`oc_sso`** when the concentrator authenticates through a **web form** (SAML / SSO — Azure AD, Okta, Duo). There is then no password to send, and @@ -695,7 +703,14 @@ sur l'entrée standard par laquelle arrive le mot de passe. Avec lui, openconnect refuse tout de suite **et** imprime la ligne `--servercert sha256:…` à recopier dans le champ `oc_servercert` du profil. Les routes appartiennent au serveur, via `vpnc-script` ; le profil peut en -ajouter, pas les remplacer. +ajouter, pas les remplacer. Pour cette raison le formulaire ne demande +**pas** à ce pilote « envoyer TOUT le trafic dans le tunnel ? », et `status` +ne le juge pas : c'est la passerelle qui décide de ce qui entre dans le +tunnel, et forcer une route par défaut contre une passerelle en tunnel +scindé ne donnerait pas tout le trafic mais un trou noir — une passerelle ne +route pas ce qu'elle n'a jamais annoncé. Pour y forcer un réseau tout de +même, l'ajouter à `routes`, que ce pilote honore, avec `0.0.0.0/0` pour +tout. Cocher **`oc_sso`** quand le concentrateur authentifie par un **formulaire web** (SAML / SSO — Azure AD, Okta, Duo). Il n'y a alors aucun mot de passe à diff --git a/script/vpn/README.fr.md b/script/vpn/README.fr.md index f062551..4aaf57d 100644 --- a/script/vpn/README.fr.md +++ b/script/vpn/README.fr.md @@ -340,7 +340,14 @@ sur l'entrée standard par laquelle arrive le mot de passe. Avec lui, openconnect refuse tout de suite **et** imprime la ligne `--servercert sha256:…` à recopier dans le champ `oc_servercert` du profil. Les routes appartiennent au serveur, via `vpnc-script` ; le profil peut en -ajouter, pas les remplacer. +ajouter, pas les remplacer. Pour cette raison le formulaire ne demande +**pas** à ce pilote « envoyer TOUT le trafic dans le tunnel ? », et `status` +ne le juge pas : c'est la passerelle qui décide de ce qui entre dans le +tunnel, et forcer une route par défaut contre une passerelle en tunnel +scindé ne donnerait pas tout le trafic mais un trou noir — une passerelle ne +route pas ce qu'elle n'a jamais annoncé. Pour y forcer un réseau tout de +même, l'ajouter à `routes`, que ce pilote honore, avec `0.0.0.0/0` pour +tout. Cocher **`oc_sso`** quand le concentrateur authentifie par un **formulaire web** (SAML / SSO — Azure AD, Okta, Duo). Il n'y a alors aucun mot de passe à diff --git a/script/vpn/README.md b/script/vpn/README.md index fa966ef..173ce01 100644 --- a/script/vpn/README.md +++ b/script/vpn/README.md @@ -321,8 +321,16 @@ says so when it takes it. unknown server certificate raises a question, and openconnect would read the answer from the standard input the password arrives on. With it, openconnect refuses at once **and** prints the `--servercert sha256:…` line to paste into -the profile's `oc_servercert`. Routes belong to the server, through -`vpnc-script`; the profile can add to them, not replace them. +the profile's `oc_servercert`. + +Routes belong to the server, through `vpnc-script`; the profile can add to +them, not replace them. For that reason the form does **not** ask this +driver « send ALL traffic through the tunnel? », and `status` does not judge +it: the gateway decides what enters the tunnel, and forcing a default route +against a split-tunnel gateway would not give all traffic but a black hole — +a gateway does not route what it never advertised. To force a network +through anyway, add it to `routes`, which this driver does honour, with +`0.0.0.0/0` for everything. Set **`oc_sso`** when the concentrator authenticates through a **web form** (SAML / SSO — Azure AD, Okta, Duo). There is then no password to send, and diff --git a/script/vpn/drivers/base.py b/script/vpn/drivers/base.py index d09c4cd..a85fc1f 100644 --- a/script/vpn/drivers/base.py +++ b/script/vpn/drivers/base.py @@ -301,6 +301,15 @@ class VpnDriver: # Faux quand la technologie ne prend pas le MTU du profil — le demander # serait une question sans effet. uses_mtu = True + # Faux quand la technologie ne pose pas la route par défaut elle-même — + # c'est alors le SERVEUR qui décide de ce qui passe par le tunnel. Le + # champ n'est ni demandé ni jugé : demander « tout le trafic ? » à qui + # n'a pas la main dessus, puis sanctionner la réponse par un ✗ sur un + # tunnel sain, est la pire des trois façons de traiter la question. + # + # L'échappatoire reste : `routes` est honorée par tous les pilotes, et + # « 0.0.0.0/0 » y demande explicitement ce que ce drapeau n'offre plus. + uses_default_route = True def __init__(self, profile: dict, secrets: dict | None = None): self.profile = profile @@ -859,7 +868,7 @@ class VpnDriver: + (f" (attendu {iface})" if not ok else ""), ) ) - if self.profile.get("default_route"): + if self.profile.get("default_route") and self.uses_default_route: info = route_to("1.1.1.1") checks.append( ( diff --git a/script/vpn/drivers/openconnect.py b/script/vpn/drivers/openconnect.py index b1f7f1d..0dffa4a 100644 --- a/script/vpn/drivers/openconnect.py +++ b/script/vpn/drivers/openconnect.py @@ -172,6 +172,12 @@ class OpenconnectDriver(VpnDriver): # Le serveur pousse les routes : exiger une route déclarée serait une # fausse exigence. needs_routes = False + # Et pour la même raison, la route par défaut ne se demande pas ici : + # c'est le concentrateur qui décide de ce qui entre dans le tunnel, et + # `vpnc-script` pose ce qu'il pousse. La forcer côté client contre une + # passerelle en tunnel scindé ne donnerait pas « tout le trafic » mais + # un trou noir — la passerelle ne route pas ce qu'elle n'a pas annoncé. + uses_default_route = False defaults = { "port": 443, "oc_user": "", diff --git a/test/test_vpn_drivers.py b/test/test_vpn_drivers.py index 6936d76..903f501 100644 --- a/test/test_vpn_drivers.py +++ b/test/test_vpn_drivers.py @@ -416,6 +416,47 @@ class Openconnect(unittest.TestCase): self.assertEqual(driver.profile["routes"], []) +class TheDefaultRouteVerdict(unittest.TestCase): + """`status` ne juge que ce que le pilote CONTRÔLE. + + Un pilote dont le serveur décide du routage ne pose pas la route par + défaut. La juger quand même rendait un ✗ permanent sur un tunnel + parfaitement sain, et un profil qui la demandait n'obtenait rien. + """ + + def _driver(self, name, **overrides): + # `driver` explicite : les échantillons ne le portent pas, et la + # validation retomberait sur le pilote par défaut. + profile = dict( + SAMPLES[name][0], + name="t-route", + driver=name, + default_route=True, + ) + profile.update(overrides) + return DRIVERS[name](profiles.validate(profile), {}) + + @staticmethod + def _labels(driver): + """Les libellés que rend `check_routes`, interrogé directement. + + `standard_status` ne l'appelle que sur un tunnel MONTÉ : passer par + lui ne rendrait aucun verdict de route sur une machine de test, et + le test passerait pour la mauvaise raison. + """ + return [label for label, _ok, _d in driver.check_routes("vpn-t")] + + def test_a_server_routed_driver_is_not_judged_on_it(self): + driver = self._driver("openconnect") + self.assertFalse(driver.uses_default_route) + self.assertNotIn("route par défaut", self._labels(driver)) + + def test_a_driver_that_lays_it_is_still_judged(self): + driver = self._driver("wireguard") + self.assertTrue(driver.uses_default_route) + self.assertIn("route par défaut", self._labels(driver)) + + class _OpenconnectNoHelper(DRIVERS["openconnect"]): """Le pilote openconnect, mais sans greffon SSO. diff --git a/test/test_vpn_menu.py b/test/test_vpn_menu.py index c521ad5..1a376e8 100644 --- a/test/test_vpn_menu.py +++ b/test/test_vpn_menu.py @@ -672,6 +672,72 @@ def largeur_affichee(texte): ) +class TheDefaultRouteQuestion(MenuBase): + """« Tout le trafic ? » n'est posée qu'aux pilotes qui posent la route. + + Demander à qui n'a pas la main dessus, puis sanctionner la réponse par + un ✗ sur un tunnel sain, était la pire des trois façons de traiter la + question : le champ existait, ne servait à rien, et faisait échouer le + diagnostic. + """ + + def filling(self, seed, *answers): + """Déroule le formulaire et rend (questions posées, sortie).""" + prompts = [] + suite = iter(answers) + + def fake(prompt=""): + prompts.append(prompt.strip()) + return next(suite) + + with patch("builtins.input", fake): + out = io.StringIO() + with redirect_stdout(out): + try: + self.todo._vpn_edit_profile(seed=dict(seed)) + except StopIteration: + pass + return prompts, out.getvalue() + + OPENCONNECT = { + "name": "oc", + "driver": "openconnect", + "server": "ssl.vpn.example-campus.net", + "oc_user": "someone", + } + WIREGUARD = { + "name": "wg", + "driver": "wireguard", + "server": "gw.example-campus.net", + "wg_address": "10.9.0.2/32", + "wg_peer_key": WG_PUBLIC, + } + + def test_a_server_routed_driver_is_not_asked(self): + prompts, printed = self.filling(self.OPENCONNECT, *[""] * 20) + self.assertFalse([p for p in prompts if "trafic" in p], prompts) + self.assertIn("passerelle décide", printed) + + def test_the_escape_hatch_is_named(self): + """Retirer la question sans dire par quoi la remplacer se lirait + comme une capacité perdue : `routes` reste honorée.""" + _, printed = self.filling(self.OPENCONNECT, *[""] * 20) + self.assertIn("0.0.0.0/0", printed) + + def test_a_driver_that_lays_the_route_is_still_asked(self): + prompts, _ = self.filling(self.WIREGUARD, *[""] * 20) + self.assertTrue([p for p in prompts if "trafic" in p], prompts) + + def test_the_flag_is_not_kept_on_a_driver_that_ignores_it(self): + """Un drapeau laissé à vrai resterait un champ sans effet, et la + liste des profils continuerait d'annoncer « tout le trafic ».""" + seed = dict(self.OPENCONNECT, default_route=True) + self.filling(seed, *([""] * 8 + ["n"])) + saved = profiles.load("oc") + self.assertIsNotNone(saved, "profil non enregistré") + self.assertFalse(saved["default_route"]) + + class ShowingWhatIsConnected(MenuBase): """L'état de chaque profil, dans la liste qui sert à choisir.