diff --git a/DEVELOPER.md b/DEVELOPER.md index d2a0108..0f7c769 100644 --- a/DEVELOPER.md +++ b/DEVELOPER.md @@ -733,7 +733,7 @@ ip route show default iptables -t nat -S POSTROUTING iptables -S FORWARD ls -la /var/lib/uvm/vms -cat /var/lib/uvm/state.json +sudo cat /var/lib/uvm/state.json # Contains plaintext guest passwords; do not share. ``` Do not use these commands to mutate UVM resources manually while a lifecycle diff --git a/README.md b/README.md index 7eaad26..e9c680d 100644 --- a/README.md +++ b/README.md @@ -236,9 +236,9 @@ State writes are locked and atomic. Lifecycle operations share an additional operation lock so concurrent `create`, `stop`, `destroy`, and `install` commands cannot overwrite each other's state or binaries. -The registry stores guest usernames and passwords in plaintext as requested, -so `state.json` is mode `0600`. Treat it as a secret, do not include it in bug -reports, and replace the default `root` password immediately. +The registry stores guest usernames and passwords in plaintext, so `state.json` +is mode `0600`. Treat it as a secret, do not include it in bug reports, and +replace the default `root` password immediately. Credential provisioning applies only to VMs created by this version. Existing VM disks are not modified automatically. diff --git a/tests/test_images.py b/tests/test_images.py index 0bf8565..09ee22a 100644 --- a/tests/test_images.py +++ b/tests/test_images.py @@ -10,6 +10,7 @@ from uvm.config import Settings from uvm.errors import UvmError from uvm.images import ( _FIRECRACKER_DEMO_PUBLIC_KEY, + _GuestFile, _enable_ssh_password_authentication, _set_shadow_password, _without_firecracker_demo_key, @@ -96,6 +97,8 @@ class ImageStoreTests(unittest.TestCase): "HostKey /etc/ssh/ssh_host_ecdsa_key\n" "HostKey /etc/ssh/ssh_host_ed25519_key\n" "PasswordAuthentication no\n" + "KbdInteractiveAuthentication no\n" + "ChallengeResponseAuthentication no\n" "PermitRootLogin yes\n" "Match User root\n" " PasswordAuthentication yes\n" @@ -105,6 +108,16 @@ class ImageStoreTests(unittest.TestCase): self.assertNotIn("/insecure/shared-key", updated) self.assertNotIn("PermitRootLogin prohibit-password", updated) + def test_non_root_credentials_preserve_the_existing_root_login_policy(self) -> None: + updated = _enable_ssh_password_authentication( + "PermitRootLogin no\nChallengeResponseAuthentication yes\n", + "service", + ) + + self.assertIn("PermitRootLogin no", updated) + self.assertIn("Match User service\n PasswordAuthentication yes", updated) + self.assertNotIn("ChallengeResponseAuthentication yes", updated) + def test_removes_only_the_public_firecracker_demo_key(self) -> None: own_key = "ssh-ed25519 AAAA-own-key developer@example" @@ -150,3 +163,27 @@ class ImageStoreTests(unittest.TestCase): Path(temporary_directory) / "authorized_keys", required=False, ) + + def test_refuses_to_replace_files_with_unpreserved_security_metadata(self) -> None: + runner = PasswordHashRunner() + store = ImageStore(Settings(), runner=runner) # type: ignore[arg-type] + + for guest_file in ( + _GuestFile(Path("/tmp/shadow"), 0o640, 0, 42, links=2), + _GuestFile( + Path("/tmp/shadow"), + 0o640, + 0, + 42, + has_extended_attributes=True, + ), + ): + with self.subTest(guest_file=guest_file): + with self.assertRaises(UvmError): + store._write_guest_file( + Path("/tmp/rootfs.ext4"), + "/etc/shadow", + guest_file, + ) + + self.assertEqual(runner.calls, []) diff --git a/tests/test_state.py b/tests/test_state.py index dcb5cfe..bdeb7d2 100644 --- a/tests/test_state.py +++ b/tests/test_state.py @@ -6,9 +6,11 @@ import tempfile import unittest from dataclasses import replace from pathlib import Path +from unittest.mock import patch from uvm.config import Settings from uvm.domain import VmRecord +from uvm.errors import StateError from uvm.state import StateStore @@ -97,3 +99,14 @@ class StateStoreTests(unittest.TestCase): self.store.load() self.assertEqual(stat.S_IMODE(self.settings.state_path.stat().st_mode), 0o600) + + def test_load_explains_when_registry_permissions_require_root(self) -> None: + self.base.mkdir(parents=True) + self.settings.state_path.write_text('{"vms": {}}', encoding="utf-8") + self.settings.state_path.chmod(0o644) + + with ( + patch("uvm.state.os.chmod", side_effect=PermissionError), + self.assertRaisesRegex(StateError, "Run this command with sudo"), + ): + self.store.load() diff --git a/uvm/images.py b/uvm/images.py index 013bd24..f31801d 100644 --- a/uvm/images.py +++ b/uvm/images.py @@ -24,6 +24,8 @@ _FIRECRACKER_DEMO_PUBLIC_KEY = ( ) _MODE_PATTERN = re.compile(r"Mode:\s+0*([0-7]+)") _OWNER_PATTERN = re.compile(r"User:\s+(\d+)\s+Group:\s+(\d+)") +_FLAGS_PATTERN = re.compile(r"Flags:\s+0x([0-9a-fA-F]+)") +_LINKS_PATTERN = re.compile(r"Links:\s+(\d+)") _HOST_KEY_TYPES = ( ("rsa", "3072"), ("ecdsa", "256"), @@ -43,6 +45,9 @@ class _GuestFile: mode: int uid: int gid: int + flags: int | None = None + links: int = 1 + has_extended_attributes: bool = False class ImageStore: @@ -184,7 +189,14 @@ class ImageStore: ) mode_match = _MODE_PATTERN.search(stat_result.stdout) owner_match = _OWNER_PATTERN.search(stat_result.stdout) - if mode_match is None or owner_match is None: + flags_match = _FLAGS_PATTERN.search(stat_result.stdout) + links_match = _LINKS_PATTERN.search(stat_result.stdout) + if ( + mode_match is None + or owner_match is None + or flags_match is None + or links_match is None + ): output = f"{stat_result.stdout}\n{stat_result.stderr}".lower() if not required and "file not found" in output: return None @@ -198,14 +210,35 @@ class ImageStore: ) if not destination.is_file(): raise UvmError(f"could not read guest rootfs file: {guest_path}") + attributes = self._runner.run( + ("debugfs", "-R", f"ea_list {guest_path}", disk), + check=False, + capture=True, + ) + diagnostics = [ + line + for line in attributes.stderr.splitlines() + if line and not line.startswith("debugfs ") + ] + if attributes.returncode != 0 or diagnostics: + raise UvmError(f"could not inspect guest rootfs metadata: {guest_path}") return _GuestFile( path=destination, mode=int(mode_match.group(1), 8), uid=int(owner_match.group(1)), gid=int(owner_match.group(2)), + flags=int(flags_match.group(1), 16), + links=int(links_match.group(1)), + has_extended_attributes=bool(attributes.stdout.strip()), ) def _write_guest_file(self, disk: Path, guest_path: str, source: _GuestFile) -> None: + if source.links != 1: + raise UvmError(f"cannot safely replace hard-linked guest file: {guest_path}") + if source.has_extended_attributes: + raise UvmError( + f"cannot safely replace guest file with extended attributes: {guest_path}" + ) self._runner.run(("debugfs", "-w", "-R", f"rm {guest_path}", disk), capture=True) self._runner.run( ("debugfs", "-w", "-R", f"write {source.path} {guest_path}", disk), @@ -226,6 +259,17 @@ class ImageStore: ), capture=True, ) + if source.flags is not None: + self._runner.run( + ( + "debugfs", + "-w", + "-R", + f"set_inode_field {guest_path} flags 0x{source.flags:x}", + disk, + ), + capture=True, + ) verification = source.path.with_name(f"{source.path.name}.verify") written = self._read_guest_file(disk, guest_path, verification) @@ -235,6 +279,7 @@ class ImageStore: or written.mode != source.mode or written.uid != source.uid or written.gid != source.gid + or (source.flags is not None and written.flags != source.flags) ): raise UvmError(f"could not verify updated guest rootfs file: {guest_path}") @@ -315,7 +360,14 @@ def _set_shadow_password(contents: str, username: str, password_hash: str) -> st def _enable_ssh_password_authentication(contents: str, username: str) -> str: - managed_directives = {"hostkey", "passwordauthentication", "permitrootlogin"} + managed_directives = { + "challengeresponseauthentication", + "hostkey", + "kbdinteractiveauthentication", + "passwordauthentication", + } + if username == "root": + managed_directives.add("permitrootlogin") unmanaged_lines = [] for line in contents.splitlines(): stripped = line.lstrip() @@ -328,13 +380,17 @@ def _enable_ssh_password_authentication(contents: str, username: str) -> str: "HostKey /etc/ssh/ssh_host_ecdsa_key", "HostKey /etc/ssh/ssh_host_ed25519_key", "PasswordAuthentication no", + "KbdInteractiveAuthentication no", + "ChallengeResponseAuthentication no", ] if username == "root": directives.append("PermitRootLogin yes") return ( "# Managed by uvm\n" + "\n".join(directives) - + f"\nMatch User {username}\n PasswordAuthentication yes\nMatch all\n" + + f"\nMatch User {username}\n" + + " PasswordAuthentication yes\n" + + "Match all\n" + "\n".join(unmanaged_lines).rstrip() + "\n" ) diff --git a/uvm/state.py b/uvm/state.py index c1c342a..ccea924 100644 --- a/uvm/state.py +++ b/uvm/state.py @@ -98,6 +98,10 @@ class StateStore: os.chmod(path, 0o600) with path.open(encoding="utf-8") as state_file: raw = json.load(state_file) + except PermissionError as error: + raise StateError( + f"cannot access protected VM registry {path}. Run this command with sudo." + ) from error except (OSError, json.JSONDecodeError) as error: raise StateError(f"cannot read {path}: {error}") from error return State.from_dict(raw)