Keep the installer's proxy and clean up node uninstall

deploy/node/README.md

@@ -21,8 +21,8 @@ The node installer runs as root and prepares the host for one node per installat
 - It never installs Docker, KVM or packages and never changes device permissions. It refuses SELinux-enforcing hosts and changes nothing when a check fails. The enrollment token comes only on standard input, never in arguments or the environment.
 - Files the service account owns are read, written and deleted only with that account's credentials. The one exception is root removing the account's home after `userdel`, when no process can still run as that account. That work runs in a child that starts its own session with `/dev/null` as input, joins a new session keyring and dies with its parent; root shows its output only as plain text (terminal controls become `?`). SIGINT, SIGHUP and SIGTERM stop that child and what it started.
 - Root never runs a file the service account can write, opens a URL it wrote, or follows a link in its home. Capture the trusted bootstrap bytes before dropping to the service account and pass them through the fork; the service account writes its own retained generation helper. Never open the caller's private download directory to it or let root write into service-owned state.
-- The generated bootstrap passes only the six standard HTTP/HTTPS proxy and bypass variables through sudo and gives both spellings the lowercase value when present, even if empty, so curl, urllib and the Go registration command follow the same rules. The installation child keeps just those names beside its fixed environment. Proxy values stay out of arguments, saved configuration, service units and diagnostics; never use broad sudo environment inheritance. This covers installation downloads only, not the node service.
-- `--uninstall` removes a node only after Core rejects its credential, except for a node that never registered and with `--force`, which Web offers when the old Core address no longer responds. It never touches sandboxes, volumes or images (the Runtime image and the microsandbox store stay), deletes the account only when the installer created it and no node remains, and otherwise removes only the groups it added.
+- The generated bootstrap passes only the six standard HTTP/HTTPS proxy and bypass variables through sudo and gives both spellings the lowercase value when present, even if empty, so curl, urllib and the Go registration command follow the same rules. The installation child keeps just those names beside its fixed environment. Proxy values stay out of arguments, unit files and diagnostics; never use broad sudo environment inheritance. The installation child uses them for downloads. The node service keeps the settings of the latest run in `/etc/oac-node/<installation-id>.env` (root, mode 0600, read through `EnvironmentFile=`; removed when a run has no proxy and by uninstall), with loopback added to `NO_PROXY`.
+- `--uninstall` removes a node only after Core rejects its credential, except for a node that never registered and with `--force`, which Web offers when the old Core address no longer responds. It never touches sandboxes, volumes or Docker images (the Runtime image stays, with the command that removes it). Once Core confirmed the removal it also deletes the node's microsandbox store, which `--force` never does (it prints the command instead). It deletes the account only when the installer created it and no node remains, and otherwise removes only the groups it added.
 - Refuse resources of an older product name for the same installation ID; never adopt them or remove another installation's resources.
 
 ## Download contract

deploy/node/node_install.py

@@ -759,11 +759,7 @@ def service_child(account, function, arguments, parent, mask, read_ends, output_
         # service. Keep only standard HTTP proxy settings across the UID boundary.
         # curl/urllib prefer lowercase, while the Go registration command prefers
         # uppercase. Give both spellings one value, including an explicit empty value.
-        proxies = {}
-        for name in ("http_proxy", "https_proxy", "no_proxy"):
-            value = os.environ.get(name, os.environ.get(name.upper()))
-            if value is not None:
-                proxies[name] = proxies[name.upper()] = value
+        proxies = proxy_settings(os.environ)
         os.environ.clear()
         os.environ.update(HOME=account.pw_dir, USER=account.pw_name, LOGNAME=account.pw_name, PATH=SAFE_PATH,
                           LANG="C.UTF-8", DOCKER_CONFIG=str(CHILD_DOCKER_CONFIG))
@@ -790,6 +786,48 @@ def service_child(account, function, arguments, parent, mask, read_ends, output_
 run_as = as_service_user
 
 
+def proxy_settings(environment):
+    """The standard proxy variables, each under both spellings with one value (an explicit empty value included)."""
+    settings = {}
+    for name in ("http_proxy", "https_proxy", "no_proxy"):
+        value = environment.get(name, environment.get(name.upper()))
+        if value is not None:
+            settings[name] = settings[name.upper()] = value
+    return settings
+
+
+def service_environment(environment):
+    """The proxy settings the node service keeps, as an EnvironmentFile; empty without an HTTP(S) proxy.
+
+    Loopback always stays direct. Systemd reads quoted values without expanding anything."""
+    settings = proxy_settings(environment)
+    if not (settings.get("http_proxy") or settings.get("https_proxy")):
+        return ""
+    direct = "localhost,127.0.0.1,::1"
+    for name in ("no_proxy", "NO_PROXY"):
+        settings[name] = ",".join(filter(None, (settings.get(name, "").strip(), direct)))
+    if any(set(value) & {"\n", "\r", "\0"} for value in settings.values()):
+        raise InstallError("A proxy setting contains a line break; fix it, then rerun the command." + NOTHING_CHANGED)
+    return "".join(name + '="' + value.replace("\\", "\\\\").replace('"', '\\"') + '"\n' for name, value in sorted(settings.items()) if value)
+
+
+def write_service_environment(path, content):
+    """Keep the installer's proxy settings in a root-only file the unit reads; True when they changed."""
+    if path.is_symlink() or (path.exists() and not path.is_file()):
+        raise InstallError(str(path) + " must be a regular file")
+    current = path.read_text() if path.exists() else ""
+    if current == content:
+        return False
+    if not content:
+        path.unlink()
+        return True
+    with distribution.temporary_file(path) as temporary:
+        with os.fdopen(os.open(temporary, os.O_WRONLY | os.O_CREAT | os.O_EXCL, 0o600), "w") as stream:
+            stream.write(content)
+        os.replace(temporary, path)
+    return True
+
+
 def child_docker_config():
     """Create the root-owned empty directory the service user's docker calls read."""
     path = CHILD_DOCKER_CONFIG
@@ -1064,7 +1102,7 @@ def prepare_account(account, record, group):
     return service_account()
 
 
-def system_unit(root, provider):
+def system_unit(root, provider, environment):
     def quote(value):
         return '"' + str(value).replace("\\", "\\\\").replace('"', '\\"').replace("%", "%%") + '"'
     after = "network-online.target docker.service" if provider == "docker" else "network-online.target"
@@ -1074,6 +1112,7 @@ def quote(value):
             + "\nStartLimitIntervalSec=0\n\n[Service]\nType=exec\nUser=" + SERVICE_USER
             + "\nExecStart=:" + quote(root / provider_assets.artifacts(provider, ("node",))[0]) + " run --config " + quote(root / "provider.json")
             + " --state-dir " + quote(root / "state/node") + "\nWorkingDirectory=" + str(root).replace("%", "%%")
+            + "\nEnvironmentFile=-" + str(environment).replace("%", "%%")
             + "\nRestart=on-failure\nRestartSec=5s\nRestartPreventExitStatus=78\nKillMode=process\nUMask=0077"
             + "\n\n[Install]\nWantedBy=multi-user.target\n")
 
@@ -1105,6 +1144,7 @@ def install_system(args, token):
     host_checks()
     # Checks that change nothing run first, so a refusal leaves no trace, not even a lock.
     record = node_record(args.installation_id)
+    proxy = service_environment(os.environ)
     configuration = None
     if record is None:
         install_display.step("Reading the Core deployment specification")
@@ -1136,9 +1176,13 @@ def install_system(args, token):
                              "core_url": args.core_url, "node_root": str(root), "unit": str(unit)}))
         args.system, args.provider = True, provider
         run_as(account, prepare_service_node, args, token, helper_archive)
-        root_file(unit, system_unit(root, provider))
+        environment = SYSTEM_RECORDS / (args.installation_id + ".env")
+        root_file(unit, system_unit(root, provider, environment))
+        changed = write_service_environment(environment, proxy)
         install_display.step("Starting the node service")
         checked(["systemctl", "daemon-reload"], "Cannot reload systemd")
+        if changed:  # A running service reads its environment only when it starts; a stopped one is left to enable.
+            checked(["systemctl", "try-restart", unit.name], "Cannot restart the node service with the new proxy settings. Inspect sudo journalctl -u " + unit.name)
         checked(["systemctl", "enable", "--now", unit.name], "Cannot start the node service; its identity is kept. Inspect sudo journalctl -u " + unit.name)
         checked(["systemctl", "is-active", "--quiet", unit.name], "Node service is unavailable; inspect sudo journalctl -u " + unit.name)
         install_display.step("Waiting for Core connection and provider readiness")
@@ -1226,8 +1270,8 @@ def release_docker_network(installation_id):
     checked(list(DOCKER) + ["network", "rm", network], "Cannot remove Docker network " + network)
 
 
-def remove_node_files(root, installation_id):
-    """Delete the node's state directory; keep the Runtime image and the microsandbox store.
+def remove_node_files(root, installation_id, delete_store):
+    """Delete the node's state directory and, once Core confirmed the removal, its microsandbox store; keep the Runtime image.
 
     Runs as the node's own user, so a link it planted can never reach another user's files."""
     no_links(root)
@@ -1240,11 +1284,14 @@ def remove_node_files(root, installation_id):
     if isinstance(image, str) and re.fullmatch(r"sha256:[0-9a-f]{64}", image):
         print("Kept the Runtime image " + image + "; remove it with `sudo docker image rm " + image + "` if no other node uses it.")
     if runtime_home.exists():
-        sudo_mode = str(Path.home()) == str(SERVICE_HOME)
-        remove = ("sudo -u " + SERVICE_USER + " rm -rf " if sudo_mode else "rm -rf ") + str(runtime_home)
-        print("Kept the microsandbox store " + str(runtime_home) + " with its images and any sandbox state. When no "
-              "microVM runs (`pgrep -u " + os.environ.get("USER", "") + "` is empty), remove it with `" + remove
-              + "`, then rerun this uninstall command.")
+        no_links(runtime_home)
+        if delete_store and private_json(runtime_home / "oac-installation.json") == {"installation_id": installation_id}:
+            shutil.rmtree(runtime_home)
+            print("Deleted the microsandbox store " + str(runtime_home) + ".")
+        else:
+            sudo_mode = str(Path.home()) == str(SERVICE_HOME)
+            print("Kept the microsandbox store " + str(runtime_home) + " with its images and any sandbox state. Delete it with `"
+                  + ("sudo -u " + SERVICE_USER + " rm -rf " if sudo_mode else "rm -rf ") + str(runtime_home) + "`.")
 
 
 def uninstall_system(args):
@@ -1285,8 +1332,10 @@ def uninstall_system(args):
                 with contextlib.suppress(InstallError):
                     checked(["systemctl", "reset-failed", unit.name], "Cannot reset " + unit.name)
             release_docker_network(args.installation_id)
-            run_as(account, remove_node_files, root, args.installation_id)
+            # --force skips Core's confirmation, so it never deletes the store.
+            run_as(account, remove_node_files, root, args.installation_id, not args.force)
             (SYSTEM_RECORDS / (args.installation_id + ".json")).unlink(missing_ok=True)
+            (SYSTEM_RECORDS / (args.installation_id + ".env")).unlink(missing_ok=True)
             print("Node for installation " + args.installation_id + " uninstalled from this host.")
         release_account(account, account_record)
 

deploy/node/test_node_install.py

@@ -871,6 +871,62 @@ def test_uninstall_waits_for_core_to_reject_the_node(self):
         self.assertFalse((system / "units" / ("oac-node-" + self.args.installation_id + ".service")).exists())
         self.assertFalse((system / "etc").exists())
 
+    def uninstall_output(self, force):
+        output = io.StringIO()
+        rejected = urllib.error.HTTPError("https://core.example", 401, "", {}, None)
+        with mock.patch.object(installer, "open_request", side_effect=rejected), mock.patch.object(installer.sys, "stdout", output):
+            installer.uninstall_system(SimpleNamespace(installation_id=self.args.installation_id, force=force))
+        return output.getvalue()
+
+    def test_uninstall_deletes_the_store_once_core_confirmed_the_removal(self):
+        self.sudo_host()
+        installer.install_system(self.args, "synthetic-once-token")
+        store = self.home / "m"
+        store.mkdir()
+        (store / "oac-installation.json").write_text(json.dumps({"installation_id": self.args.installation_id}))
+        (store / "image").write_text("layers")
+        output = self.uninstall_output(False)
+        self.assertFalse(store.exists())
+        self.assertIn("Deleted the microsandbox store " + str(store), output)
+        self.assertNotIn("Kept the microsandbox store", output)
+
+    def test_forced_uninstall_keeps_the_store_and_prints_the_command(self):
+        self.sudo_host()
+        installer.install_system(self.args, "synthetic-once-token")
+        store = self.home / "m"
+        store.mkdir()
+        (store / "oac-installation.json").write_text(json.dumps({"installation_id": self.args.installation_id}))
+        output = self.uninstall_output(True)
+        self.assertTrue(store.is_dir())
+        self.assertIn("Kept the microsandbox store " + str(store), output)
+        self.assertIn("rm -rf " + str(store) + "`", output)
+
+    def test_rerun_updates_the_service_proxy_and_restarts_the_service(self):
+        system = self.sudo_host()
+        unit = "oac-node-" + self.args.installation_id + ".service"
+        environment = system / "etc" / (self.args.installation_id + ".env")
+        with mock.patch.dict(os.environ, {"https_proxy": "http://proxy-one.invalid:3128", "http_proxy": ""}):
+            installer.install_system(self.args, "synthetic-once-token")
+        self.assertIn("proxy-one", environment.read_text())
+        self.assertEqual(stat.S_IMODE(environment.stat().st_mode), 0o600)
+        self.assertNotIn("proxy-one", (system / "units" / unit).read_text())
+        self.calls.clear()
+        with mock.patch.dict(os.environ, {"https_proxy": "http://proxy-two.invalid:3128", "http_proxy": ""}):
+            installer.install_system(self.args, "")
+        self.assertIn("proxy-two", environment.read_text())
+        self.assertNotIn("proxy-one", environment.read_text())
+        self.assertIn(["systemctl", "try-restart", unit], [call for call, _ in self.calls])
+        self.uninstall_output(False)
+        self.assertFalse(environment.exists())
+
+    def test_uninstall_keeps_a_store_it_did_not_create(self):
+        self.sudo_host()
+        installer.install_system(self.args, "synthetic-once-token")
+        store = self.home / "m"
+        store.mkdir()
+        self.assertIn("Kept the microsandbox store", self.uninstall_output(False))
+        self.assertTrue(store.is_dir())
+
     def test_uninstall_leaves_an_adopted_account_as_found(self):
         system = self.sudo_host()
         (self.home / "service").mkdir()
@@ -958,13 +1014,13 @@ def test_uninstall_prints_only_an_image_id_from_the_service_home(self):
         (root / "provider.json").write_text(json.dumps({"native": {"image": "x\nFinish with: sudo sh"}}))
         output = io.StringIO()
         with mock.patch.object(installer.sys, "stdout", output):
-            installer.remove_node_files(root, self.args.installation_id)
+            installer.remove_node_files(root, self.args.installation_id, False)
         self.assertNotIn("Finish", output.getvalue())
         root.mkdir()
         (root / "provider.json").write_text(json.dumps({"native": {"image": "sha256:" + "b" * 64}}))
         output = io.StringIO()
         with mock.patch.object(installer.sys, "stdout", output):
-            installer.remove_node_files(root, self.args.installation_id)
+            installer.remove_node_files(root, self.args.installation_id, False)
         self.assertIn("`sudo docker image rm sha256:", output.getvalue())
 
     def test_service_user_step_gets_its_own_session_keyring(self):

deploy/node/test_node_proxy.py

@@ -8,6 +8,7 @@
 import pwd
 import select
 import socket
+import stat
 import ssl
 import subprocess
 import sys
@@ -193,11 +194,27 @@ def test_rejected_proxy_does_not_reveal_credentials(self):
                                         "http://downloads.invalid", expect_failure=True)
             self.assertIn("HTTP 407", error)
 
-    def test_installer_proxy_is_not_in_service_unit(self):
-        with mock.patch.dict(os.environ, {"https_proxy": "http://fixture:private%20password@proxy.invalid"}):
-            unit = installer.system_unit(Path("/var/lib/oac-node/.oac/nodes/fixture"), "docker")
-        self.assertIn("User=oac-node", unit)
-        self.assertNotIn("proxy", unit.lower())
+    def test_service_keeps_the_installer_proxy_in_a_private_file(self):
+        secret = "http://fixture:private%20password@proxy.invalid:3128"
+        content = installer.service_environment({"https_proxy": secret, "NO_PROXY": "internal.invalid"})
+        self.assertIn('https_proxy="' + secret + '"\n', content)
+        self.assertIn('HTTPS_PROXY="' + secret + '"\n', content)
+        self.assertIn('no_proxy="internal.invalid,localhost,127.0.0.1,::1"\n', content)
+        self.assertEqual(installer.service_environment({"no_proxy": "internal.invalid"}), "")
+        with tempfile.TemporaryDirectory() as directory:
+            path = Path(directory) / "node.env"
+            unit = installer.system_unit(Path("/var/lib/oac-node/.oac/nodes/fixture"), "docker", path)
+            self.assertIn("EnvironmentFile=-" + str(path) + "\n", unit)
+            self.assertNotIn("private", unit)
+            self.assertTrue(installer.write_service_environment(path, content))
+            self.assertEqual(stat.S_IMODE(path.stat().st_mode), 0o600)
+            self.assertFalse(installer.write_service_environment(path, content))
+            # A rerun with other settings replaces them; one without a proxy removes the file.
+            other = installer.service_environment({"HTTP_PROXY": "http://other.invalid:3128"})
+            self.assertTrue(installer.write_service_environment(path, other))
+            self.assertNotIn("private", path.read_text())
+            self.assertTrue(installer.write_service_environment(path, ""))
+            self.assertFalse(path.exists())
 
 
 if __name__ == "__main__":