diff --git a/README.md b/README.md index d76ff86..c263835 100644 --- a/README.md +++ b/README.md @@ -104,6 +104,13 @@ hands over: exec "$VENV_DIR/bin/" install "$@" ``` +It also hands the virtualenv and the checkout to the daemon account with the +ops group writable, so an operator can pull, hotfix and import a driver to +test it without sudo. That grants nothing new: they already decide what the +daemons run, through the configs they edit and the restarts they trigger. git +is told separately, since it refuses a repository whose owner is not the +caller no matter the permissions. + Everything else needs no root. `doctor` compares the installed unit and polkit rule against what `install` would write now, so a host whose unit predates a template change is reported rather than silently stale. diff --git a/src/instrumentctl/installer.py b/src/instrumentctl/installer.py index 758d2db..e8d5495 100644 --- a/src/instrumentctl/installer.py +++ b/src/instrumentctl/installer.py @@ -16,6 +16,7 @@ import os import pwd import shutil +import stat import subprocess import tempfile from importlib import resources @@ -181,6 +182,44 @@ def _sudoers(product: Product, paths: Paths) -> None: shutil.chown(product.sudoers, "root", "root") +def _hand_to_operators(tree: Path, product: Product) -> None: + """Give a tree to the daemon account, writable by the ops group. + + Operators already decide what the daemons run, through the configs they + edit and the restarts they trigger, so letting them pull and hotfix the + checkout grants nothing new. It spares a sudo for every edit and lets them + import a driver to test it. + """ + if not tree.is_dir(): + return + for target in (tree, *tree.rglob("*")): + if target.is_symlink(): + continue + shutil.chown(target, product.user, product.ops_group) + mode = stat.S_IMODE(target.stat().st_mode) + # Setgid on directories so anything created later keeps the group, + # rather than the creating operator's own + target.chmod(mode | (0o2770 if target.is_dir() else 0o660)) + + +def _git_safe_directory(repo: Path) -> None: + """Let operators run git here despite the tree belonging to the daemon. + + Since 2.35.2 git refuses a repository whose owner is not the caller, which + group permissions do not satisfy. + """ + if not (repo / ".git").exists(): + return + listed = subprocess.run(["git", "config", "--system", "--get-all", + "safe.directory"], check=False, capture_output=True, + text=True) + if str(repo) in listed.stdout.split(): + return + subprocess.run(["git", "config", "--system", "--add", "safe.directory", + str(repo)], check=False) + _say("git trusts", str(repo)) + + def install(paths: Paths, args: argparse.Namespace) -> int: """Converge this host onto what the instrument needs. Root only.""" product = paths.product @@ -198,10 +237,10 @@ def install(paths: Paths, args: argparse.Namespace) -> int: _directories(product, paths) _secrets_and_role(product, paths, args.host_role) _artefacts(product, paths) - shutil.chown(paths.venv, product.user, product.user) - for child in paths.venv.rglob("*"): - shutil.chown(child, product.user, product.user) - _say("owned", f"{paths.venv} by {product.user}") + for tree in (paths.venv, paths.repo): + _hand_to_operators(tree, product) + _say("owned", f"{tree} by {product.user}:{product.ops_group}") + _git_safe_directory(paths.repo) subprocess.run(["systemctl", "daemon-reload"], check=False) print(f"\nHost setup complete. Operators in {product.ops_group} can now " diff --git a/tests/test_ownership.py b/tests/test_ownership.py new file mode 100644 index 0000000..515eb10 --- /dev/null +++ b/tests/test_ownership.py @@ -0,0 +1,71 @@ +"""Handing the venv and checkout to operators.""" +from __future__ import annotations + +import os +import pathlib +import stat +from unittest import mock + +import pytest + +from instrumentctl import installer +from instrumentctl.product import Product + + +@pytest.fixture(name="tree") +def tree_fixture(tmp_path: pathlib.Path) -> pathlib.Path: + """A venv-shaped tree: an executable, a plain file, nested directories.""" + (tmp_path / "bin").mkdir() + (tmp_path / "lib" / "pkg").mkdir(parents=True) + (tmp_path / "bin" / "python3").touch(mode=0o755) + (tmp_path / "lib" / "pkg" / "mod.py").touch(mode=0o644) + return tmp_path + + +def hand_over(tree: pathlib.Path, product: Product) -> None: + """Run the real mode changes; chown needs accounts a test does not have.""" + with mock.patch.object(installer.shutil, "chown", lambda *a, **k: None): + installer._hand_to_operators(tree, product) + + +def mode(path: pathlib.Path) -> int: + return stat.S_IMODE(path.stat().st_mode) + + +def test_operators_can_write_every_file(tree: pathlib.Path, product: Product) -> None: + hand_over(tree, product) + for path in (tree / "bin" / "python3", tree / "lib" / "pkg" / "mod.py"): + assert mode(path) & stat.S_IWGRP, path + + +def test_operators_can_write_every_directory(tree: pathlib.Path, + product: Product) -> None: + hand_over(tree, product) + for path in (tree, tree / "bin", tree / "lib" / "pkg"): + assert mode(path) & stat.S_IWGRP, path + + +def test_executables_stay_executable(tree: pathlib.Path, product: Product) -> None: + """Group-write must not cost the venv its interpreter.""" + hand_over(tree, product) + assert mode(tree / "bin" / "python3") & stat.S_IXUSR + + +def test_plain_files_do_not_become_executable(tree: pathlib.Path, + product: Product) -> None: + hand_over(tree, product) + assert not mode(tree / "lib" / "pkg" / "mod.py") & stat.S_IXUSR + + +@pytest.mark.skipif(os.geteuid() != 0 or os.uname().sysname == "Darwin", + reason="macOS strips setgid for a non-root caller") +def test_directories_are_setgid(tree: pathlib.Path, product: Product) -> None: + """So a file an operator adds keeps the group the daemon reads as.""" + hand_over(tree, product) + assert mode(tree / "bin") & stat.S_ISGID + + +def test_a_missing_tree_is_not_an_error(tmp_path: pathlib.Path, + product: Product) -> None: + """install runs before the checkout exists on a fresh host.""" + hand_over(tmp_path / "absent", product)