diff --git a/docs/networking.md b/docs/networking.md index 41a034b..6d8c941 100644 --- a/docs/networking.md +++ b/docs/networking.md @@ -154,6 +154,74 @@ that would be a one-way door. The original is kept at `/etc/network/interfaces.before-ekklesia-classroom`. +### What 0.2.5 got wrong, and what 0.2.6 changed + +The prayer-room laptop's Wi-Fi is **hidden** — `ROGC Classroom`, an SSID +with a space in it, announced by nothing. `--adopt` built a connection, +failed to bring it up, rolled back, and left the adapter belonging to +nobody. Four separate defects, all fixed: + +**1. The hidden flag was thrown away.** The stanza said `wpa-scan-ssid 1` +and the parser read it — into `options`, where the only accessor ignored +it. The generated profile had `802-11-wireless.hidden: no`, and +NetworkManager will not associate with an SSID it has not seen in a scan. +A hidden one is never in a scan. `credentials()` returned a pair; there +is now a `profile()` that returns the whole network, and `add()` writes +`802-11-wireless.hidden` in **both** directions rather than only when +true — a default that happens to be right is not an answer. + +**2. `ifdown` ran after the stanza was commented out.** ifupdown reads +`/etc/network/interfaces` to find out how to bring an interface down. +With the stanza gone it says *"Ignoring unknown interface"* and does +nothing — so ifupdown's own `wpa_supplicant` and DHCP client were still +holding the radio when NetworkManager was asked to associate with it. +Two supplicants on one interface is a fight NetworkManager loses. The +order is now: **`ifdown` first, while the file still says how; rewrite +the file second.** + +**3. The rollback left the adapter unmanaged.** `restore()` ended with +`nmcli device set managed no`, which is right while ifupdown can +actually bring the interface up. When `ifup` fails too, those two lines +are a wireless adapter neither manager owns: no connection, and a panel +that cannot even scan. That is the state the laptop was found in, and it +is why `sudo nmcli device set wlp2s0 managed yes` was a manual recovery +step. The rollback now escalates — if ifupdown cannot get the network +back either, the interface is handed to NetworkManager rather than left +with neither, and it says so loudly. + +**4. A `#` in a password was silently truncated.** The parser cut every +line at its first `#`. `interfaces(5)` is explicit that only whole-line +comments exist — *"end-of-line comments are NOT supported"* — so a +pre-shared key containing `#` became a prefix of itself, and the only +symptom is an association that fails exactly the way a wrong password +fails. + +Two smaller ones went with them. The rollback un-comments its own marked +lines in place instead of copying the backup over the whole file, because +the backup is written once and never refreshed — a whole-file restore +silently reverted any *wired* change an administrator made in between. +And `nmcli -t` output is now split on its own escaping rules rather than +on `:`, with `LC_ALL=C`, so a network whose name contains a colon cannot +make a successful migration look like a failed one. + +`--adopt` also now refuses a network it cannot recreate — WPA-Enterprise +has no pre-shared key to carry across — *before* taking the interface +away from ifupdown to find out, and reports any `unmanaged-devices` line +in NetworkManager's own configuration, which survives the stanza going +away and would otherwise leave the interface belonging to nobody after a +restart. + +Verified against a real NetworkManager 1.52.1: the keyfile it writes +carries `hidden=true`, `ssid=ROGC Classroom` with its space, and a psk +containing `#` intact, at 0600 root:root with no `connection.permissions`. +`tests/test-classroom-wifi-adopt.sh` drives the whole hand-over against +stubbed tools and asserts the ordering, the rollback and the managed +state. + +**The prayer-room laptop must not be migrated again.** Its stanza is +already gone, so `--adopt` reports "nothing to adopt" and exits 0 without +touching anything; there is a test for exactly that. + After the migration the panel shows the connection at the next login, the room can scan, and the credential protection below applies to it like any other system connection. diff --git a/lib/classroom_network.py b/lib/classroom_network.py index 005f2b5..30deb45 100644 --- a/lib/classroom_network.py +++ b/lib/classroom_network.py @@ -36,10 +36,24 @@ import re # ifupdown's own spelling. Values may be quoted, and a psk may be a # 64-character hex key rather than a passphrase. -SETTINGS = ("wpa-ssid", "wpa-psk", "wpa-key-mgmt", "wpa-conf", "wpa-proto") +SETTINGS = ("wpa-ssid", "wpa-psk", "wpa-key-mgmt", "wpa-conf", "wpa-proto", + "wpa-scan-ssid") + +# Whether the access point broadcasts its name. +# +# wpa-scan-ssid 1 is ifupdown's spelling of wpa_supplicant's scan_ssid=1: +# probe for this network by name, because it is not in a passive scan. +# NetworkManager spells the same thing 802-11-wireless.hidden, and a +# migration that drops it produces a connection that is correct in every +# visible respect and can never associate — NetworkManager will not +# connect to an SSID it has not seen in a scan, and a hidden one is never +# in a scan. That is what happened to the prayer-room laptop on 0.2.5. +HIDDEN = "wpa-scan-ssid" _STANZA = re.compile(r"^\s*(iface|auto|allow-hotplug|source|source-directory)\s+(.*)$") +TRUTHY = {"1", "yes", "true", "on"} + def _unquote(value): value = (value or "").strip() @@ -63,7 +77,19 @@ def parse(text): current = None for number, raw in enumerate(lines): - line = raw.split("#", 1)[0].rstrip() + # A comment is a WHOLE line, and only a whole line. + # + # interfaces(5): "Lines starting with '#' are ignored. Note that + # end-of-line comments are NOT supported." This used to cut every + # line at its first '#', which is a silent corruption of exactly + # one value: a pre-shared key containing a '#' was truncated, the + # migrated connection got a password that was a prefix of the + # real one, and the only symptom is an association that fails the + # way a wrong password fails. + if raw.lstrip().startswith("#"): + continue + + line = raw.rstrip() match = _STANZA.match(line) if match: @@ -129,12 +155,57 @@ def credentials(stanza): network inline returns no SSID: that one is not ours to move, and the caller must leave it alone rather than guess. """ + found = profile(stanza) + + if found is None: + return None, None + + return found["ssid"], found["psk"] + + +def profile(stanza): + """Everything needed to recreate this stanza in NetworkManager. + + A dict, or None when the stanza is not ours to move. Returning the + whole network rather than a pair is the fix for the 0.2.5 failure: + `wpa-scan-ssid` was sitting in the parsed options the whole time and + the only accessor threw it away, so the hand-over could not have + preserved it however carefully it was written. + + `key_mgmt` is "wpa-psk", "none", or None meaning "this file says + something this command does not understand". The caller must refuse + that last one rather than guess: a WPA-Enterprise network recreated + as a PSK network is a migration that cannot work and has already + taken the interface away from ifupdown to find out. + """ options = stanza.get("options") or {} if "wpa-conf" in options and "wpa-ssid" not in options: - return None, None + return None - return options.get("wpa-ssid") or None, options.get("wpa-psk") or None + ssid = options.get("wpa-ssid") or None + + if not ssid: + return None + + psk = options.get("wpa-psk") or None + declared = (options.get("wpa-key-mgmt") or "").strip().upper() + + if declared in ("", "WPA-PSK", "WPA-PSK-SHA256", "SAE", "WPA-PSK SAE"): + key_mgmt = "wpa-psk" if psk else "none" + elif declared == "NONE": + key_mgmt = "none" + else: + # WPA-EAP, IEEE8021X and anything else this has never seen. + key_mgmt = None + + return { + "ssid": ssid, + "psk": psk, + # A network that has to be probed for by name. See HIDDEN. + "hidden": (options.get(HIDDEN) or "").strip().lower() in TRUTHY, + "key_mgmt": key_mgmt, + } def comment_out(text, stanza, marker="# ekklesia-classroom: "): @@ -160,3 +231,31 @@ def comment_out(text, stanza, marker="# ekklesia-classroom: "): break return "".join(lines) + + +def uncomment(text, marker="# ekklesia-classroom: "): + """Undo comment_out, in the file as it stands now. + + Returns (text, count). The rollback uses this rather than copying the + backup over the top, because the backup is kept once and never + refreshed: an administrator who corrected a static Ethernet address + between the migration and the rollback would have had that correction + silently reverted by a whole-file restore. This touches only the + lines this package marked, so everything else in the file — wired + stanzas, source lines, an administrator's own comments — is exactly + as the machine left it. + """ + lines = (text or "").splitlines(keepends=True) + restored = 0 + + for number, line in enumerate(lines): + stripped = line.lstrip() + + if not stripped.startswith(marker): + continue + + indent = line[:len(line) - len(stripped)] + lines[number] = indent + stripped[len(marker):] + restored += 1 + + return "".join(lines), restored diff --git a/scripts/classroom-wifi b/scripts/classroom-wifi index 8da11d8..b207aa0 100755 --- a/scripts/classroom-wifi +++ b/scripts/classroom-wifi @@ -77,6 +77,11 @@ def nmcli(*args, check=True): result = subprocess.run( ["nmcli"] + list(args), capture_output=True, text=True, timeout=60, check=False, + # nmcli translates its output and its terse field separator rules + # are the only thing here that is parsed. C is the one locale in + # which what this reads back is what nmcli is documented to + # print, on a machine in any country. + env={**os.environ, "LC_ALL": "C"}, ) if check and result.returncode != 0: @@ -86,14 +91,57 @@ def nmcli(*args, check=True): return result +def terse(line): + """One line of `nmcli -t` output, split into its fields. + + nmcli escapes a colon inside a value as `\\:` and a backslash as + `\\\\`, so splitting on ":" is wrong for any network whose name + contains one. Everything here that compares a connection name to a + field goes through this, because getting it wrong does not produce an + error — it produces a migration that decides it failed and rolls a + working network back. + """ + fields = [] + current = [] + escaped = False + + for character in line: + if escaped: + current.append(character) + escaped = False + elif character == "\\": + escaped = True + elif character == ":": + fields.append("".join(current)) + current = [] + else: + current.append(character) + + fields.append("".join(current)) + + return fields + + +def connections(): + """Every saved connection, as (name, uuid, type).""" + result = nmcli("-t", "-f", "NAME,UUID,TYPE", "connection", "show", + check=False) + + found = [] + + for line in (result.stdout or "").splitlines(): + fields = terse(line) + + if len(fields) >= 3: + found.append((fields[0], fields[1], fields[2])) + + return found + + def listing(): - result = nmcli("-t", "-f", "NAME,TYPE,UUID", "connection", "show") - - for line in result.stdout.splitlines(): - parts = line.split(":") - - if len(parts) >= 2 and "wireless" in parts[1]: - print(f" {parts[0]}") + for name, _uuid, kind in connections(): + if "wireless" in kind: + print(f" {name}") return 0 @@ -105,7 +153,7 @@ def forget(name): return 0 -def add(ssid, password, name=None): +def add(ssid, password, name=None, hidden=False): name = name or ssid # Deliberately no connection.permissions. That single omission is the @@ -121,6 +169,15 @@ def add(ssid, password, name=None): # Stored in the connection file, which is root-only, rather than # in a login keyring that belongs to whoever is signed in. "802-11-wireless-security.psk-flags", "0", + # WHETHER THE ACCESS POINT BROADCASTS ITS NAME. + # + # Always passed, in both directions, rather than only when it is + # true. NetworkManager's default is "no", and a default that + # happens to be right is not the same as an answer: this is the + # setting the prayer-room laptop's migration left at the default + # and could therefore never associate. Written down either way so + # that the connection file says what was meant. + "802-11-wireless.hidden", "yes" if hidden else "no", ] if password: @@ -131,9 +188,13 @@ def add(ssid, password, name=None): nmcli(*arguments) - path = os.path.join(CONNECTIONS, f"{name}.nmconnection") + if hidden: + say(f"{name} is a hidden network; NetworkManager will probe for it " + "by name") - if os.path.exists(path): + path = connection_file(name) + + if path and os.path.exists(path): # NetworkManager already writes 0600 root:root. Said out loud and # enforced anyway, because this is the file the whole arrangement # rests on. @@ -148,6 +209,28 @@ def add(ssid, password, name=None): return 0 +def connection_file(name): + """Where NetworkManager actually put a connection, or None. + + Asked rather than constructed. NetworkManager escapes characters in + the filename it derives from a connection name, so + f"{name}.nmconnection" is a guess that happens to be right for a + one-word SSID and wrong for "ROGC Classroom" — which is the network + this command exists to migrate, and the file whose permissions the + whole arrangement rests on. + """ + result = nmcli("-t", "-f", "NAME,FILENAME", "connection", "show", + check=False) + + for line in (result.stdout or "").splitlines(): + fields = terse(line) + + if len(fields) >= 2 and fields[0] == name and fields[-1]: + return fields[-1] + + return None + + def run(*command): """Run a command, tolerating one that is not installed. @@ -198,14 +281,71 @@ def carrying(name, device): "--active", check=False) for line in (result.stdout or "").splitlines(): - parts = line.rsplit(":", 1) + fields = terse(line) - if len(parts) == 2 and parts[0] == name and parts[1] == device: + if len(fields) == 2 and fields[0] == name and fields[1] == device: return True return False +def device_state(device): + """NetworkManager's word for what it is doing with this interface.""" + result = nmcli("-t", "-f", "DEVICE,STATE", "device", "status", check=False) + + for line in (result.stdout or "").splitlines(): + fields = terse(line) + + if len(fields) >= 2 and fields[0] == device: + return fields[1] + + return "unknown" + + +def unmanaged_by_config(device): + """Configuration files that name this device as unmanaged. + + Separate from the ifupdown plugin, and the reason the prayer-room + laptop still needed `nmcli device set wlp2s0 managed yes` by hand + after its stanza was gone. A keyfile saying `unmanaged-devices` is + not undone by commenting out an interfaces stanza, and a hand-over + that does not notice it hands the interface to nobody. + """ + naming = [] + candidates = ["/etc/NetworkManager/NetworkManager.conf"] + + for directory in ("/etc/NetworkManager/conf.d", + "/usr/lib/NetworkManager/conf.d", + "/run/NetworkManager/conf.d"): + try: + candidates += sorted( + os.path.join(directory, entry) + for entry in os.listdir(directory) + if entry.endswith(".conf") + ) + except OSError: + continue + + for path in candidates: + try: + with open(path, "r", encoding="utf-8", errors="replace") as handle: + text = handle.read() + except OSError: + continue + + for line in text.splitlines(): + stripped = line.strip() + + if not stripped.startswith("unmanaged-devices"): + continue + + if device in stripped or "except:" in stripped: + naming.append(path) + break + + return naming + + def find_stanza(): """The one wireless stanza ifupdown holds, or None, plus the file.""" if not os.path.exists(INTERFACES): @@ -249,20 +389,33 @@ def check(): return 0 device = stanza["name"] - ssid, _ = ifupdown.credentials(stanza) + found = ifupdown.profile(stanza) say(f"NOTE: {device} is configured in {INTERFACES}, so ifupdown owns " "it and") say(" NetworkManager will report it unmanaged — the room's panel") say(" will say there is no connection and will not find networks.") - if ssid: + if found and found["hidden"]: + say(" Its access point does not broadcast its name, which " + "NetworkManager") + say(" has to be told; --adopt carries that across.") + + for path in unmanaged_by_config(device): + say(f" {path} also names this device unmanaged; --adopt " + "reports it") + say(" rather than editing it.") + + if found is None: + say(" Its credentials are not in that file, so it has to be " + "moved by hand.") + elif found["key_mgmt"] is None: + say(" Its security is not a pre-shared key, so it has to be " + "moved by hand.") + else: say(" To move it, while watching, over SSH:") say(" classroom-wifi --adopt --dry-run") say(" classroom-wifi --adopt") - else: - say(" Its credentials are not in that file, so it has to be " - "moved by hand.") return 0 @@ -312,17 +465,37 @@ def adopt(dry_run=False): return 0 device = stanza["name"] - ssid, psk = ifupdown.credentials(stanza) + found = ifupdown.profile(stanza) - if not ssid: + if found is None: say(f"{device} is configured through a wpa_supplicant.conf rather " "than named in this file") say("that one is not ours to move; adopt it by hand") return 1 + if found["key_mgmt"] is None: + say(f"{device} uses a security this command does not recreate " + f"({stanza['options'].get('wpa-key-mgmt')})") + say("refusing to guess; a network moved wrong is a network that " + "cannot come back") + return 1 + + ssid = found["ssid"] + psk = found["psk"] + # Never printed. The room's password is not diagnostic output. say(f"{device}: ifupdown holds a wireless network " - f"({'with' if psk else 'without'} a saved password)") + f"({'with' if psk else 'without'} a saved password" + f"{', hidden' if found['hidden'] else ''})") + + # A keyfile naming this device unmanaged survives the stanza going + # away, so the hand-over would end with the interface belonging to + # nobody. Said before anything is touched, and not edited: a file an + # administrator wrote is not this command's to rewrite. + for path in unmanaged_by_config(device): + say(f"WARNING: {path} names {device} unmanaged") + say(" NetworkManager will not keep it after a restart " + "until that is changed") # Refuse to start a hand-over that cannot be undone. ifdown releases # the interface and ifup is the whole of the rollback; without them @@ -342,17 +515,32 @@ def adopt(dry_run=False): "run would refuse") say(f"would create a NetworkManager system connection for {ssid!r}") - say(f"would comment the {device} stanza out of {INTERFACES}") + say(" 802-11-wireless.hidden: " + f"{'yes' if found['hidden'] else 'no'}") + say(f" 802-11-wireless-security.key-mgmt: {found['key_mgmt']}") + say(f"would run `ifdown {device}` while its stanza is still in " + f"{INTERFACES}") + say(f"would then comment that stanza out of {INTERFACES}") say("would hand the interface over, and put it back if that failed") return 0 - existing = nmcli("-t", "-f", "NAME", "connection", "show", check=False) - already = ssid in (existing.stdout or "").splitlines() + already = any(name == ssid for name, _uuid, _kind in connections()) if already: + # Reused as it stands. An existing connection of this name may be + # a teacher's own, or one a previous attempt left behind with the + # wrong hidden setting — which is exactly the state the + # prayer-room laptop was in — so it is corrected rather than + # trusted. Every setting the migration decides is written again. say(f"NetworkManager already has a connection named {ssid!r}") + say("correcting it to match what ifupdown holds") + nmcli("connection", "modify", ssid, + "802-11-wireless.hidden", "yes" if found["hidden"] else "no", + "connection.autoconnect", "yes", + "802-11-wireless-security.psk-flags", "0", + check=False) else: - if add(ssid, psk) != 0: + if add(ssid, psk, hidden=found["hidden"]) != 0: return 1 backup = f"{INTERFACES}.before-ekklesia-classroom" @@ -366,20 +554,51 @@ def adopt(dry_run=False): # timeout, an interrupt — the file goes back and ifupdown gets the # interface again. Nothing below may raise past this. try: + # RELEASE THE INTERFACE BEFORE TAKING ITS STANZA AWAY. + # + # ifdown reads /etc/network/interfaces to find out how to bring + # an interface down. Run after the stanza is commented out it + # reports "Ignoring unknown interface" and does nothing at all — + # so ifupdown's own wpa_supplicant and DHCP client were still + # running, still holding the radio, when NetworkManager was asked + # to associate with it. Two supplicants on one interface is a + # fight NetworkManager loses, and it is the second half of why + # the prayer-room laptop rolled back. + # + # The order is therefore: down first, while the file still says + # how; rewrite the file second. The window in which the machine + # is offline is the same either way. + result = run("ifdown", device) + + if result is not None and result.returncode != 0: + # Not fatal. A machine whose interface is already down is a + # perfectly good starting point, and ifdown says so with a + # non-zero status. + say(f"ifdown {device}: " + f"{(result.stderr or result.stdout or '').strip()}") + + say(f"released {device} from ifupdown") + with open(INTERFACES, "w", encoding="utf-8") as handle: handle.write(ifupdown.comment_out(text, stanza)) - say(f"took {device} out of ifupdown's hands") + say(f"took {device} out of ifupdown's hands in {INTERFACES}") # NetworkManager only re-reads which interfaces ifupdown claims - # when its configuration is reloaded. The link is still - # ifupdown's at this point, and NetworkManager does not touch a - # device it considers unmanaged, so this cannot drop it. + # when its configuration is reloaded. nmcli("general", "reload", "conf", check=False) - - run("ifdown", device) nmcli("device", "set", device, "managed", "yes", check=False) - nmcli("connection", "up", ssid, check=False) + + # On the device, by name. `connection up ` alone lets + # NetworkManager choose an interface, and a machine with two + # radios could bring the network up on the wrong one — which + # `carrying` would then correctly report as a failure, and the + # migration would roll back a network that was working. + started = nmcli("connection", "up", ssid, "ifname", device, + check=False) + + if started.returncode != 0: + say(f"nmcli: {(started.stderr or '').strip()}") deadline = time.monotonic() + ADOPT_TIMEOUT @@ -387,12 +606,15 @@ def adopt(dry_run=False): if carrying(ssid, device) and online(): say(f"{device} is NetworkManager's now, and the room is " "online") + say(f"NetworkManager reports {device} " + f"{device_state(device)}") say("the panel will show the connection at the next login") return 0 time.sleep(3) - say("NetworkManager did not get the network back") + say("NetworkManager did not get the network back " + f"({device} is {device_state(device)})") except BaseException as error: # noqa: BLE001 - see the comment above say(f"the migration failed part way through: {error}") @@ -403,6 +625,48 @@ def adopt(dry_run=False): return 1 +def put_interfaces_back(): + """Undo this command's edit to /etc/network/interfaces. + + Un-commenting the marked lines where they are, rather than copying + the backup over the whole file. The backup is written once, on the + first attempt, and never refreshed — so a whole-file restore silently + reverts anything an administrator changed about the WIRED network in + between. The marker names exactly the lines this command commented + out, and nothing else in the file is touched. + + The backup is still the fallback, for a file that has been replaced + since rather than edited. + """ + backup = f"{INTERFACES}.before-ekklesia-classroom" + + try: + with open(INTERFACES, "r", encoding="utf-8") as handle: + current = handle.read() + except OSError as error: + say(f"cannot read {INTERFACES}: {error}") + current = None + + if current is not None: + restored, count = ifupdown.uncomment(current) + + if count: + with open(INTERFACES, "w", encoding="utf-8") as handle: + handle.write(restored) + + say(f"put {count} line{'' if count == 1 else 's'} back in " + f"{INTERFACES}, and changed nothing else in it") + return True + + if os.path.exists(backup): + shutil.copy2(backup, INTERFACES) + say(f"no marked lines to restore; copied {backup} back instead") + return True + + say(f"nothing of this command's is in {INTERFACES} to undo") + return False + + def restore(device, ssid=None): """Put the interface back under ifupdown. @@ -410,11 +674,7 @@ def restore(device, ssid=None): decide what that means for an exit code: for a failed migration it is still a failure, and for `--rollback` it is a success. """ - backup = f"{INTERFACES}.before-ekklesia-classroom" - - if os.path.exists(backup): - shutil.copy2(backup, INTERFACES) - say(f"restored {INTERFACES} from {backup}") + put_interfaces_back() nmcli("general", "reload", "conf", check=False) @@ -423,7 +683,12 @@ def restore(device, ssid=None): if device: nmcli("device", "set", device, "managed", "no", check=False) - run("ifup", device) + result = run("ifup", device) + + if result is not None and result.returncode != 0: + say(f"ifup {device}: " + f"{(result.stderr or result.stdout or '').strip()}") + say(f"ifupdown has {device} again") for _ in range(10): @@ -433,6 +698,44 @@ def restore(device, ssid=None): time.sleep(3) + # THE ADAPTER IS NEVER LEFT BELONGING TO NOBODY. + # + # Above, the device has deliberately been handed back to ifupdown and + # NetworkManager told to leave it alone. That is right while ifupdown + # can actually bring it up. When ifup has failed too, the same two + # lines are a wireless adapter that neither manager owns: no + # connection, and a panel that cannot even scan for one. That is the + # state the prayer-room laptop was found in after 0.2.5, and it is + # the state that made `nmcli device set wlp2s0 managed yes` a manual + # recovery step. + # + # So the last resort is to give it to NetworkManager rather than to + # leave it with neither. A room able to pick its network from the + # panel is recoverable by somebody standing in front of it; an + # unmanaged radio is not. + if device: + say("WARNING: ifupdown did not bring the network back either") + say(f" handing {device} to NetworkManager so the adapter " + "is not left unmanaged") + + nmcli("device", "set", device, "managed", "yes", check=False) + + if ssid: + nmcli("connection", "up", ssid, "ifname", device, check=False) + + for _ in range(5): + if online(): + say("the room is on the network, through NetworkManager") + say(f"NOTE: {INTERFACES} still describes {device}, so " + "ifupdown will") + say(" claim it again at the next boot. Decide which " + "one owns it.") + return True + + time.sleep(3) + + say(f" {device} is {device_state(device)}") + say("WARNING: the machine may be offline. The original " f"{INTERFACES} is restored; try `ifup {device}` or a reboot") return False @@ -449,6 +752,8 @@ def main(): help="remove a saved connection") parser.add_argument("--open", action="store_true", help="the network has no password") + parser.add_argument("--hidden", action="store_true", + help="the access point does not broadcast its name") parser.add_argument("--adopt", action="store_true", help="move an installer-configured Wi-Fi from " "ifupdown into NetworkManager") @@ -494,7 +799,8 @@ def main(): say("no password given") return 1 - return add(arguments.ssid, password, arguments.name) + return add(arguments.ssid, password, arguments.name, + hidden=arguments.hidden) if __name__ == "__main__": diff --git a/tests/test-classroom-wifi-adopt.sh b/tests/test-classroom-wifi-adopt.sh new file mode 100755 index 0000000..662bd53 --- /dev/null +++ b/tests/test-classroom-wifi-adopt.sh @@ -0,0 +1,411 @@ +#!/bin/bash +# The Wi-Fi hand-over, driven end to end against stubbed tools. +# +# tests/test-classroom-network-adopt.sh covers the parsing. This covers +# the part that actually failed on the prayer-room laptop during the +# 0.2.5 setup: the ORDER of the hand-over, and what is carried across +# with the network. +# +# The laptop's own configuration was +# +# allow-hotplug wlp2s0 +# iface wlp2s0 inet dhcp +# wpa-ssid "ROGC Classroom" +# wpa-psk +# wpa-scan-ssid 1 +# +# and the migration produced a NetworkManager connection with +# 802-11-wireless.hidden: no, which can never associate with an access +# point that does not broadcast its name. It also ran `ifdown` after the +# stanza had been commented out, where ifdown has nothing left to read +# and does nothing, so ifupdown's own supplicant was still holding the +# radio. Then the rollback left the adapter unmanaged. +# +# nmcli, ifup, ifdown and curl are stubs that record what they were +# asked and can be told to fail, so the sequence is asserted rather than +# described. No real interface, no real NetworkManager, no laptop. +set -uo pipefail + +ROOT="$(cd "$(dirname "${BASH_SOURCE[0]}")/.." && pwd)" + +python3 - "$ROOT" <<'PY' +import importlib.machinery +import importlib.util +import os +import shutil +import sys +import tempfile +from pathlib import Path + +root = Path(sys.argv[1]) + +passed = failed = 0 + + +def check(label, condition, detail=""): + global passed, failed + if condition: + print(f" ok {label}") + passed += 1 + else: + print(f" FAIL {label}") + if detail: + print(f" {detail}") + failed += 1 + + +def load(): + """classroom-wifi as a module. It has no .py, so name the loader.""" + path = root / "scripts" / "classroom-wifi" + loader = importlib.machinery.SourceFileLoader("classroom_wifi", str(path)) + spec = importlib.util.spec_from_loader("classroom_wifi", loader) + module = importlib.util.module_from_spec(spec) + loader.exec_module(module) + return module + + +# The laptop's file, with the hidden network and an SSID with a space in +# it. Also a wired stanza, which nothing here may touch. +LAPTOP = """# This file describes the network interfaces available on your system +source /etc/network/interfaces.d/* + +auto lo +iface lo inet loopback + +auto enp3s0 +iface enp3s0 inet static +\taddress 10.40.0.90/24 +\tgateway 10.40.0.1 + +allow-hotplug wlp2s0 +iface wlp2s0 inet dhcp +\twpa-ssid "ROGC Classroom" +\twpa-psk s#cret-with-a-hash +\twpa-scan-ssid 1 +""" + +BROADCAST = """allow-hotplug wlp2s0 +iface wlp2s0 inet dhcp +\twpa-ssid Sanctuary +\twpa-psk ordinary-secret +""" + +ENTERPRISE = """allow-hotplug wlp2s0 +iface wlp2s0 inet dhcp +\twpa-ssid Campus +\twpa-key-mgmt WPA-EAP +""" + +STUB = r"""#!/bin/bash +# Records the call, and the state of the interfaces file at the moment of +# it, so the test can assert ordering rather than trust it. +printf '%s' "$(basename "$0")" >> "$LOG" +for argument in "$@"; do printf ' %s' "$argument" >> "$LOG"; done +printf '\n' >> "$LOG" + +if [[ -f "$INTERFACES" ]]; then + cp "$INTERFACES" "$SNAPSHOTS/$(basename "$0").$$.interfaces" +fi + +case "$(basename "$0")" in +nmcli) + case "$*" in + *"-f NAME,UUID,TYPE connection show"*) cat "$FAKE_CONNECTIONS" ;; + *"-f NAME,FILENAME connection show"*) cat "$FAKE_FILENAMES" ;; + *"connection show --active"*) cat "$FAKE_ACTIVE" ;; + *"device status"*) cat "$FAKE_DEVICES" ;; + *"networking connectivity"*) cat "$FAKE_CONNECTIVITY" ;; + *"connection up"*) exit "${UP_STATUS:-0}" ;; + esac + exit 0 + ;; +ifdown) exit "${IFDOWN_STATUS:-0}" ;; +ifup) exit "${IFUP_STATUS:-0}" ;; +curl) exit "${CURL_STATUS:-0}" ;; +esac +exit 0 +""" + + +class Rig: + """A machine mid-migration, with every external tool stubbed.""" + + def __init__(self, interfaces_text, **environment): + self.directory = Path(tempfile.mkdtemp(prefix="classroom-wifi-")) + self.bin = self.directory / "bin" + self.snapshots = self.directory / "snapshots" + self.bin.mkdir() + self.snapshots.mkdir() + + self.interfaces = self.directory / "interfaces" + self.interfaces.write_text(interfaces_text) + + self.log = self.directory / "calls.log" + self.log.write_text("") + + for tool in ("nmcli", "ifdown", "ifup", "curl"): + target = self.bin / tool + target.write_text(STUB) + target.chmod(0o755) + + # What the stubbed nmcli answers with. Written as files so a test + # can change an answer partway through if it needs to. + self.answers = { + "FAKE_CONNECTIONS": "", + "FAKE_FILENAMES": "", + "FAKE_ACTIVE": "ROGC Classroom:wlp2s0\n", + "FAKE_DEVICES": "wlp2s0:connected\n", + "FAKE_CONNECTIVITY": "full\n", + } + + self.environment = { + "LOG": str(self.log), + "INTERFACES": str(self.interfaces), + "SNAPSHOTS": str(self.snapshots), + } + + for key, value in self.answers.items(): + path = self.directory / key + path.write_text(value) + self.environment[key] = str(path) + + self.environment.update({k: str(v) for k, v in environment.items()}) + + def answer(self, key, value): + Path(self.environment[key]).write_text(value) + + def run(self, action, **kwargs): + saved_path = os.environ.get("PATH", "") + saved = {k: os.environ.get(k) for k in self.environment} + os.environ["PATH"] = f"{self.bin}:{saved_path}" + os.environ.update(self.environment) + + module = load() + module.INTERFACES = str(self.interfaces) + # The poll loops are real time; nothing here needs to wait. + module.time.sleep = lambda _seconds: None + module.ADOPT_TIMEOUT = 0.2 + + try: + return getattr(module, action)(**kwargs), module + finally: + os.environ["PATH"] = saved_path + for key, value in saved.items(): + if value is None: + os.environ.pop(key, None) + else: + os.environ[key] = value + + @property + def calls(self): + return self.log.read_text().splitlines() + + def snapshot_at(self, tool): + """The interfaces file as it was when `tool` ran.""" + for path in sorted(self.snapshots.glob(f"{tool}.*.interfaces")): + return path.read_text() + return None + + def clean(self): + shutil.rmtree(self.directory, ignore_errors=True) + + +print("1. the hidden network survives the hand-over") +# The 0.2.5 regression itself. wpa-scan-ssid 1 was sitting in the parsed +# options and the only accessor threw it away. +rig = Rig(LAPTOP) +status, module = rig.run("adopt") +added = [c for c in rig.calls if c.startswith("nmcli connection add")] +check("the migration reports success", status == 0, str(status)) +check("a connection is created", len(added) == 1, str(rig.calls)) +check("and it is told the access point does not broadcast its name", + added and "802-11-wireless.hidden yes" in added[0], + added[0] if added else "no connection was added") +check("the SSID keeps its space", + added and "ssid ROGC Classroom" in added[0], added[0] if added else "") +check("it is still a system connection, with no permissions", + added and "connection.permissions" not in added[0]) +check("the secret is stored in the connection file", + added and "802-11-wireless-security.psk-flags 0" in added[0]) +check("the password is not truncated at the # inside it", + added and "wifi-sec.psk s#cret-with-a-hash" in added[0], + added[0] if added else "") +rig.clean() + +print("2. the interface is released BEFORE its stanza is taken away") +# ifdown reads /etc/network/interfaces to know how to bring an interface +# down. Run after the stanza is commented out it reports "Ignoring +# unknown interface" and does nothing, so ifupdown's wpa_supplicant kept +# the radio and NetworkManager could not have it. +rig = Rig(LAPTOP) +rig.run("adopt") +order = [c.split()[0] for c in rig.calls] +at_ifdown = rig.snapshot_at("ifdown") +check("ifdown is called at all", "ifdown" in order, str(order)) +check("the stanza is still live in the file when ifdown runs", + at_ifdown is not None + and "\niface wlp2s0 inet dhcp\n" in at_ifdown, + "ifdown ran against a file whose stanza was already commented out") +check("and the file is commented out afterwards", + "# ekklesia-classroom: iface wlp2s0" in rig.interfaces.read_text()) +check("NetworkManager is only told to manage it after the file changed", + order.index("ifdown") < len(order) - 1) +managed = [c for c in rig.calls if "device set" in c] +check("the device is explicitly handed to NetworkManager", + any("managed yes" in c for c in managed), str(managed)) +up = [c for c in rig.calls if "connection up" in c] +check("the connection is brought up on that interface by name", + up and "ifname wlp2s0" in up[0], str(up)) +rig.clean() + +print("3. an ordinary broadcast network is not marked hidden") +# The flag has to be an answer in both directions, not something only +# ever switched on. +rig = Rig(BROADCAST) +rig.run("adopt") +added = [c for c in rig.calls if c.startswith("nmcli connection add")] +check("hidden is written as no", added and "802-11-wireless.hidden no" in added[0], + added[0] if added else "") +rig.clean() + +print("4. a failed hand-over puts the file back, and only the file") +# The backup is written once and never refreshed, so a whole-file restore +# would silently revert a wired change made in between. The rollback +# un-comments its own marker instead. +rig = Rig(LAPTOP, UP_STATUS="1", FAKE_CONNECTIVITY="none\n", CURL_STATUS="1") +status, _ = rig.run("adopt") +after = rig.interfaces.read_text() +check("the migration reports failure", status == 1, str(status)) +check("the wireless stanza is live again", + "\niface wlp2s0 inet dhcp\n" in after, after) +check("the allow-hotplug line came back too", + "\nallow-hotplug wlp2s0\n" in after) +check("nothing of the marker is left behind", + "ekklesia-classroom:" not in after, after) +check("the file is byte for byte what it was", after == LAPTOP, + "restored file differs from the original") +check("the wired stanza was never touched", + "\taddress 10.40.0.90/24\n" in after) +rig.clean() + +print("5. a rollback preserves an unrelated change made since") +# The case a whole-file copy gets wrong: the wired address is corrected +# after the migration, and then the migration fails. +rig = Rig(LAPTOP, UP_STATUS="1", FAKE_CONNECTIVITY="none\n", CURL_STATUS="1") + + +def edit_between(module): + original = module.put_interfaces_back + + def wrapped(): + text = rig.interfaces.read_text() + rig.interfaces.write_text( + text.replace("10.40.0.90/24", "10.40.0.91/24")) + return original() + + module.put_interfaces_back = wrapped + + +saved_run = Rig.run + + +def run_with_edit(self, action, **kwargs): + saved_path = os.environ.get("PATH", "") + os.environ["PATH"] = f"{self.bin}:{saved_path}" + os.environ.update(self.environment) + module = load() + module.INTERFACES = str(self.interfaces) + module.time.sleep = lambda _seconds: None + module.ADOPT_TIMEOUT = 0.2 + edit_between(module) + try: + return getattr(module, action)(**kwargs), module + finally: + os.environ["PATH"] = saved_path + + +run_with_edit(rig, "adopt") +after = rig.interfaces.read_text() +check("the corrected wired address survived the rollback", + "10.40.0.91/24" in after, after) +check("and the wireless stanza is live again", + "\niface wlp2s0 inet dhcp\n" in after) +rig.clean() + +print("6. a failed hand-over never leaves the adapter unmanaged") +# The state the prayer-room laptop was found in: ifupdown had been told +# to take the interface back, ifup failed too, and NetworkManager had +# been told to leave it alone. Nobody owned the radio, and recovering it +# needed `nmcli device set wlp2s0 managed yes` typed by hand. +rig = Rig(LAPTOP, UP_STATUS="1", IFUP_STATUS="1", + FAKE_CONNECTIVITY="none\n", CURL_STATUS="1") +status, _ = rig.run("adopt") +# Only what the rollback did. The hand-over itself sets managed yes on +# the way in, which is not the question here. +rollback_calls = rig.calls[ + max(index for index, call in enumerate(rig.calls) + if call.startswith("ifup ")):] +sets = [c for c in rollback_calls if "device set" in c] +check("ifupdown is offered the interface first", + any("managed no" in c for c in rig.calls), str(rig.calls)) +check("and when ifup fails too it is given to NetworkManager", + sets and "managed yes" in sets[-1], + "the last word on the adapter was 'managed no', which owns nothing") +check("so the adapter is never left belonging to nobody", + "managed yes" in rig.calls[-1] or any("managed yes" in c + for c in sets), + str(rollback_calls)) +check("and the migration still reports failure", status == 1, str(status)) +rig.clean() + +print("7. a network this cannot recreate is refused before anything moves") +# WPA-Enterprise has no pre-shared key to carry across. Guessing would +# take the interface away from ifupdown to find that out. +rig = Rig(ENTERPRISE) +status, _ = rig.run("adopt") +check("it refuses", status == 1, str(status)) +check("nothing was added", not any("connection add" in c for c in rig.calls), + str(rig.calls)) +check("the file is untouched", rig.interfaces.read_text() == ENTERPRISE) +check("ifdown was never run", not any(c.startswith("ifdown") + for c in rig.calls), str(rig.calls)) +rig.clean() + +print("8. a machine already migrated is not migrated again") +# The prayer-room laptop, after tomorrow. Its stanza is gone, so there is +# nothing to adopt and --adopt must be a no-op rather than an error. +MIGRATED = LAPTOP.replace("allow-hotplug wlp2s0", + "# ekklesia-classroom: allow-hotplug wlp2s0") +MIGRATED = "\n".join( + ("# ekklesia-classroom: " + line) + if line.startswith("iface wlp2s0") or line.startswith("\twpa-") + else line + for line in MIGRATED.splitlines() +) + "\n" +rig = Rig(MIGRATED) +status, _ = rig.run("adopt") +check("it reports nothing to do, and succeeds", status == 0, str(status)) +check("no connection was created", + not any("connection add" in c for c in rig.calls), str(rig.calls)) +check("nothing was brought down", not any(c.startswith("ifdown") + for c in rig.calls)) +check("the file is untouched", rig.interfaces.read_text() == MIGRATED) +rig.clean() + +print("9. --check changes nothing at all") +rig = Rig(LAPTOP) +status, _ = rig.run("check") +check("it succeeds", status == 0) +check("the file is untouched", rig.interfaces.read_text() == LAPTOP) +check("it runs no command that could change anything", + not any(c.startswith(("ifup", "ifdown")) for c in rig.calls), + str(rig.calls)) +check("and no nmcli that writes", + not any("connection add" in c or "connection modify" in c + or "device set" in c for c in rig.calls), str(rig.calls)) +rig.clean() + +print() +print(f"{passed} passed, {failed} failed") +raise SystemExit(1 if failed else 0) +PY