diff --git a/pkgs/clan-cli/clan_cli/flash/flash.py b/pkgs/clan-cli/clan_cli/flash/flash.py index dc110a163..8fd36c153 100644 --- a/pkgs/clan-cli/clan_cli/flash/flash.py +++ b/pkgs/clan-cli/clan_cli/flash/flash.py @@ -60,7 +60,9 @@ def flash_machine( extra_args = [] system_config_nix: dict[str, Any] = {} - generate_vars_for_machine(machine, generator_name=None, regenerate=False) + generate_vars_for_machine( + machine, generator_name=None, regenerate=False, fix=False + ) generate_facts([machine], service=None, regenerate=False) if system_config.wifi_settings: diff --git a/pkgs/clan-cli/clan_cli/secrets/groups.py b/pkgs/clan-cli/clan_cli/secrets/groups.py index c45266422..d52054bdc 100644 --- a/pkgs/clan-cli/clan_cli/secrets/groups.py +++ b/pkgs/clan-cli/clan_cli/secrets/groups.py @@ -103,7 +103,7 @@ def update_group_keys(flake_dir: Path, group: str) -> list[Path]: if (secret / "groups" / group).is_symlink(): updated_paths += update_keys( secret, - sorted(secrets.collect_keys_for_path(secret)), + secrets.collect_keys_for_path(secret), ) return updated_paths diff --git a/pkgs/clan-cli/clan_cli/secrets/secrets.py b/pkgs/clan-cli/clan_cli/secrets/secrets.py index 3374c7668..52c633ede 100644 --- a/pkgs/clan-cli/clan_cli/secrets/secrets.py +++ b/pkgs/clan-cli/clan_cli/secrets/secrets.py @@ -1,7 +1,5 @@ import argparse -import functools import getpass -import operator import os import shutil import sys @@ -45,7 +43,7 @@ def update_secrets( changed_files.extend( update_keys( secret_path, - sorted_keys(collect_keys_for_path(secret_path)), + collect_keys_for_path(secret_path), ) ) return changed_files @@ -147,7 +145,7 @@ def encrypt_secret( ) secret_path = secret_path / "secret" - encrypt_file(secret_path, value, sorted_keys(recipient_keys)) + encrypt_file(secret_path, value, sorted(recipient_keys)) files_to_commit.append(secret_path) if git_commit: commit_files( @@ -231,7 +229,7 @@ def allow_member( changed.extend( update_keys( group_folder.parent, - sorted_keys(collect_keys_for_path(group_folder.parent)), + collect_keys_for_path(group_folder.parent), ) ) return changed @@ -257,12 +255,7 @@ def disallow_member(group_folder: Path, name: str) -> list[Path]: if len(os.listdir(group_folder.parent)) == 0: group_folder.parent.rmdir() - return update_keys( - target.parent.parent, sorted_keys(collect_keys_for_path(group_folder.parent)) - ) - - -sorted_keys = functools.partial(sorted, key=operator.itemgetter(0)) + return update_keys(target.parent.parent, collect_keys_for_path(group_folder.parent)) def has_secret(secret_path: Path) -> bool: diff --git a/pkgs/clan-cli/clan_cli/secrets/sops.py b/pkgs/clan-cli/clan_cli/secrets/sops.py index b3a90483c..2e81d3b4e 100644 --- a/pkgs/clan-cli/clan_cli/secrets/sops.py +++ b/pkgs/clan-cli/clan_cli/secrets/sops.py @@ -4,7 +4,7 @@ import json import os import shutil import subprocess -from collections.abc import Iterator +from collections.abc import Iterable, Iterator from contextlib import contextmanager, suppress from dataclasses import dataclass from pathlib import Path @@ -182,8 +182,9 @@ def sops_manifest(keys: list[tuple[str, KeyType]]) -> Iterator[Path]: yield Path(manifest.name) -def update_keys(secret_path: Path, keys: list[tuple[str, KeyType]]) -> list[Path]: - with sops_manifest(keys) as manifest: +def update_keys(secret_path: Path, keys: Iterable[tuple[str, KeyType]]) -> list[Path]: + keys_sorted = sorted(keys) + with sops_manifest(keys_sorted) as manifest: secret_path = secret_path / "secret" time_before = secret_path.stat().st_mtime cmd = nix_shell( diff --git a/pkgs/clan-cli/clan_cli/vars/check.py b/pkgs/clan-cli/clan_cli/vars/check.py index bf0e8feee..38d10663e 100644 --- a/pkgs/clan-cli/clan_cli/vars/check.py +++ b/pkgs/clan-cli/clan_cli/vars/check.py @@ -8,7 +8,9 @@ from clan_cli.machines.machines import Machine log = logging.getLogger(__name__) -def check_vars(machine: Machine, generator_name: None | str = None) -> bool: +def vars_status( + machine: Machine, generator_name: None | str = None +) -> tuple[list[tuple[str, str]], list[tuple[str, str]], list[tuple[str, str]]]: secret_vars_module = importlib.import_module(machine.secret_vars_module) secret_vars_store = secret_vars_module.SecretStore(machine=machine) public_vars_module = importlib.import_module(machine.public_vars_module) @@ -16,6 +18,8 @@ def check_vars(machine: Machine, generator_name: None | str = None) -> bool: missing_secret_vars = [] missing_public_vars = [] + # signals if a var needs to be updated (eg. needs re-encryption due to new users added) + outdated_secret_vars = [] if generator_name: generators = [generator_name] else: @@ -23,24 +27,39 @@ def check_vars(machine: Machine, generator_name: None | str = None) -> bool: for generator_name in generators: shared = machine.vars_generators[generator_name]["share"] for name, file in machine.vars_generators[generator_name]["files"].items(): - if file["secret"] and not secret_vars_store.exists( - generator_name, name, shared=shared - ): + if file["secret"]: + if not secret_vars_store.exists(generator_name, name, shared=shared): + log.info( + f"Secret var '{name}' for service '{generator_name}' in machine {machine.name} is missing." + ) + missing_secret_vars.append((generator_name, name)) + else: + needs_update, msg = secret_vars_store.needs_fix( + generator_name, name, shared=shared + ) + if needs_update: + log.info( + f"Secret var '{name}' for service '{generator_name}' in machine {machine.name} needs update: {msg}" + ) + outdated_secret_vars.append((generator_name, name)) + + elif not public_vars_store.exists(generator_name, name, shared=shared): log.info( - f"Secret fact '{name}' for service '{generator_name}' in machine {machine.name} is missing." - ) - missing_secret_vars.append((generator_name, name)) - if not file["secret"] and not public_vars_store.exists( - generator_name, name, shared=shared - ): - log.info( - f"Public fact '{name}' for service '{generator_name}' in machine {machine.name} is missing." + f"Public var '{name}' for service '{generator_name}' in machine {machine.name} is missing." ) missing_public_vars.append((generator_name, name)) log.debug(f"missing_secret_vars: {missing_secret_vars}") log.debug(f"missing_public_vars: {missing_public_vars}") - return not (missing_secret_vars or missing_public_vars) + log.debug(f"outdated_secret_vars: {outdated_secret_vars}") + return missing_secret_vars, missing_public_vars, outdated_secret_vars + + +def check_vars(machine: Machine, generator_name: None | str = None) -> bool: + missing_secret_vars, missing_public_vars, outdated_secret_vars = vars_status( + machine, generator_name=generator_name + ) + return not (missing_secret_vars or missing_public_vars or outdated_secret_vars) def check_command(args: argparse.Namespace) -> None: diff --git a/pkgs/clan-cli/clan_cli/vars/generate.py b/pkgs/clan-cli/clan_cli/vars/generate.py index 78e1dc462..178da5068 100644 --- a/pkgs/clan-cli/clan_cli/vars/generate.py +++ b/pkgs/clan-cli/clan_cli/vars/generate.py @@ -318,11 +318,49 @@ def _check_can_migrate( ) +def ensure_consistent_state( + machine: Machine, + generator_name: str | None, + fix: bool, +) -> None: + """ + Apply local updates to secrets like re-encrypting with missing keys + when new users were added. + """ + + if generator_name is None: + generators = list(machine.vars_generators.keys()) + else: + generators = [generator_name] + outdated = [] + for generator_name in generators: + for name, file in machine.vars_generators[generator_name]["files"].items(): + shared = machine.vars_generators[generator_name]["share"] + if file["secret"] and machine.secret_vars_store.exists( + generator_name, name + ): + needs_update, msg = machine.secret_vars_store.needs_fix( + generator_name, name, shared=shared + ) + if needs_update: + outdated.append((generator_name, name, msg)) + if not fix and outdated: + msg = ( + "The local state of some secret vars is inconsistent and needs to be updated.\n" + "Rerun 'clan vars generate' passing '--fix' to apply the necessary changes." + "Problems to fix:\n" + "\n".join(o[2] for o in outdated if o[2]) + ) + raise ClanError(msg) + + def generate_vars_for_machine( machine: Machine, generator_name: str | None, regenerate: bool, + fix: bool, ) -> bool: + ensure_consistent_state(machine, generator_name, fix) closure = get_closure(machine, generator_name, regenerate) if len(closure) == 0: return False @@ -347,13 +385,14 @@ def generate_vars( machines: list[Machine], generator_name: str | None, regenerate: bool, + fix: bool = False, ) -> bool: was_regenerated = False for machine in machines: errors = [] try: was_regenerated |= generate_vars_for_machine( - machine, generator_name, regenerate + machine, generator_name, regenerate, fix ) machine.flush_caches() except Exception as exc: @@ -376,7 +415,7 @@ def generate_command(args: argparse.Namespace) -> None: machines = get_all_machines(args.flake, args.option) else: machines = get_selected_machines(args.flake, args.option, args.machines) - generate_vars(machines, args.service, args.regenerate) + generate_vars(machines, args.service, args.regenerate, args.fix) def register_generate_parser(parser: argparse.ArgumentParser) -> None: @@ -403,4 +442,12 @@ def register_generate_parser(parser: argparse.ArgumentParser) -> None: help="whether to regenerate facts for the specified machine", default=None, ) + + parser.add_argument( + "--fix", + action=argparse.BooleanOptionalAction, + help="whether to fix local state inconsistencies, for example if a secret is not encrypted with the correct keys", + default=False, + ) + parser.set_defaults(func=generate_command) diff --git a/pkgs/clan-cli/clan_cli/vars/secret_modules/__init__.py b/pkgs/clan-cli/clan_cli/vars/secret_modules/__init__.py index bec54847d..1134685a3 100644 --- a/pkgs/clan-cli/clan_cli/vars/secret_modules/__init__.py +++ b/pkgs/clan-cli/clan_cli/vars/secret_modules/__init__.py @@ -12,6 +12,27 @@ class SecretStoreBase(StoreBase): def needs_upload(self) -> bool: return True + def needs_fix( + self, + generator_name: str, + name: str, + shared: bool, + ) -> tuple[bool, str | None]: + """ + Check if local state needs updating, eg. secret needs to be re-encrypted with new keys + """ + return False, None + + def fix( + self, + generator_name: str, + name: str, + shared: bool, + ) -> None: + """ + Update local state, eg make sure secret is encrypted with correct keys + """ + @abstractmethod def upload(self, output_dir: Path) -> None: pass diff --git a/pkgs/clan-cli/clan_cli/vars/secret_modules/sops.py b/pkgs/clan-cli/clan_cli/vars/secret_modules/sops.py index b38a23750..840fec357 100644 --- a/pkgs/clan-cli/clan_cli/vars/secret_modules/sops.py +++ b/pkgs/clan-cli/clan_cli/vars/secret_modules/sops.py @@ -1,13 +1,23 @@ import json from dataclasses import dataclass from pathlib import Path +from typing import override from clan_cli.errors import ClanError from clan_cli.machines.machines import Machine -from clan_cli.secrets.folders import sops_machines_folder, sops_secrets_folder +from clan_cli.secrets.folders import ( + sops_machines_folder, + sops_secrets_folder, + sops_users_folder, +) from clan_cli.secrets.machines import add_machine, add_secret, has_machine -from clan_cli.secrets.secrets import decrypt_secret, encrypt_secret, has_secret -from clan_cli.secrets.sops import generate_private_key +from clan_cli.secrets.secrets import ( + collect_keys_for_path, + decrypt_secret, + encrypt_secret, + has_secret, +) +from clan_cli.secrets.sops import KeyType, generate_private_key from . import SecretStoreBase @@ -56,6 +66,18 @@ class SecretStore(SecretStoreBase): def store_name(self) -> str: return "sops" + def user_has_access( + self, user: str, generator_name: str, secret_name: str, shared: bool + ) -> bool: + secret_path = self.secret_path(generator_name, secret_name, shared=shared) + secret = json.loads((secret_path / "secret").read_text()) + recipients = [r["recipient"] for r in (secret["sops"].get("age") or [])] + users_folder_path = sops_users_folder(self.machine.flake_dir) + user_pubkey = json.loads((users_folder_path / user / "key.json").read_text())[ + "publickey" + ] + return user_pubkey in recipients + def machine_has_access( self, generator_name: str, secret_name: str, shared: bool ) -> bool: @@ -131,3 +153,53 @@ class SecretStore(SecretStoreBase): if not shared: return True return self.machine_has_access(generator_name, name, shared) + + def collect_keys_for_secret(self, path: Path) -> set[tuple[str, KeyType]]: + from clan_cli.secrets.secrets import ( + collect_keys_for_path, + collect_keys_for_type, + ) + + keys = collect_keys_for_path(path) + for group in self.machine.deployment["sops"]["defaultGroups"]: + keys.update( + collect_keys_for_type( + self.machine.flake_dir / "sops" / "groups" / group / "machines" + ) + ) + keys.update( + collect_keys_for_type( + self.machine.flake_dir / "sops" / "groups" / group / "users" + ) + ) + return keys + + @override + def needs_fix( + self, generator_name: str, name: str, shared: bool + ) -> tuple[bool, str | None]: + secret_path = self.secret_path(generator_name, name, shared) + recipients_ = json.loads((secret_path / "secret").read_text())["sops"]["age"] + current_recipients = {r["recipient"] for r in recipients_} + wanted_recipients = { + key[0] for key in self.collect_keys_for_secret(secret_path) + } + needs_update = current_recipients != wanted_recipients + recipients_to_add = wanted_recipients - current_recipients + var_id = f"{generator_name}/{name}" + msg = ( + f"One or more recipient keys were added to secret{' shared' if shared else ''} var '{var_id}', but it was never re-encrypted. " + f"This could have been a malicious actor trying to add their keys, please investigate. " + f"Added keys: {', '.join(recipients_to_add)}" + ) + return needs_update, msg + + @override + def fix(self, generator_name: str, name: str, shared: bool) -> None: + from clan_cli.secrets.secrets import update_keys + + secret_path = self.secret_path(generator_name, name, shared) + update_keys( + secret_path, + collect_keys_for_path(secret_path), + ) diff --git a/pkgs/clan-cli/tests/test_vars.py b/pkgs/clan-cli/tests/test_vars.py index 09fdcdf71..e2f59a4eb 100644 --- a/pkgs/clan-cli/tests/test_vars.py +++ b/pkgs/clan-cli/tests/test_vars.py @@ -191,6 +191,31 @@ def test_generate_secret_var_sops_with_default_group( ) assert sops_store.exists("my_generator", "my_secret") assert sops_store.get("my_generator", "my_secret").decode() == "hello\n" + # add another user and check if secret gets re-encrypted + from clan_cli.secrets.sops import generate_private_key + + _, pubkey_uschi = generate_private_key() + cli.run( + [ + "secrets", + "users", + "add", + "--flake", + str(flake.path), + "uschi", + pubkey_uschi, + ] + ) + cli.run(["secrets", "groups", "add-user", "my_group", "uschi"]) + with pytest.raises(ClanError): + cli.run(["vars", "generate", "--flake", str(flake.path), "my_machine"]) + # apply fix + cli.run(["vars", "generate", "--flake", str(flake.path), "my_machine", "--fix"]) + # check if new user can access the secret + monkeypatch.setenv("USER", "uschi") + assert sops_store.user_has_access( + "uschi", "my_generator", "my_secret", shared=False + ) @pytest.mark.impure @@ -746,6 +771,7 @@ def test_stdout_of_generate( Machine(name="my_machine", flake=FlakeId(str(flake.path))), "my_generator", regenerate=False, + fix=False, ) assert "Updated var my_generator/my_value" in output.out @@ -757,6 +783,7 @@ def test_stdout_of_generate( Machine(name="my_machine", flake=FlakeId(str(flake.path))), "my_generator", regenerate=True, + fix=False, ) assert "Updated var my_generator/my_value" in output.out assert "old: world" in output.out @@ -767,6 +794,7 @@ def test_stdout_of_generate( Machine(name="my_machine", flake=FlakeId(str(flake.path))), "my_generator", regenerate=True, + fix=False, ) assert "Updated" not in output.out assert "hello" in output.out @@ -775,6 +803,7 @@ def test_stdout_of_generate( Machine(name="my_machine", flake=FlakeId(str(flake.path))), "my_secret_generator", regenerate=False, + fix=False, ) assert "Updated secret var my_secret_generator/my_secret" in output.out assert "hello" not in output.out @@ -789,6 +818,7 @@ def test_stdout_of_generate( Machine(name="my_machine", flake=FlakeId(str(flake.path))), "my_secret_generator", regenerate=True, + fix=False, ) assert "Updated secret var my_secret_generator/my_secret" in output.out assert "world" not in output.out @@ -891,6 +921,7 @@ def test_fails_when_files_are_left_from_other_backend( Machine(name="my_machine", flake=FlakeId(str(flake.path))), generator, regenerate=False, + fix=False, ) my_secret_generator["files"]["my_secret"]["secret"] = False my_value_generator["files"]["my_value"]["secret"] = True @@ -902,6 +933,7 @@ def test_fails_when_files_are_left_from_other_backend( Machine(name="my_machine", flake=FlakeId(str(flake.path))), generator, regenerate=False, + fix=False, )