From 66a9b22d665a264ac8d87333298cc5360bfe3245 Mon Sep 17 00:00:00 2001 From: Louis Opter Date: Tue, 21 Jan 2025 20:57:15 +0000 Subject: [PATCH 1/9] clan-cli: secrets/sops: fix error message in maybe_get_admin_public_key --- pkgs/clan-cli/clan_cli/secrets/sops.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkgs/clan-cli/clan_cli/secrets/sops.py b/pkgs/clan-cli/clan_cli/secrets/sops.py index 9c37aad12..26adf3f6b 100644 --- a/pkgs/clan-cli/clan_cli/secrets/sops.py +++ b/pkgs/clan-cli/clan_cli/secrets/sops.py @@ -337,7 +337,7 @@ def maybe_get_admin_public_key() -> None | SopsKey: if len(keyring) > 1: last_3 = [f"{key.key_type.name.lower()}:{key.pubkey}" for key in keyring[:3]] msg = ( - f"Found more than {len(keyring)} public keys in your " + f"Found {len(keyring)} public keys in your " f"environment/system and cannot decide which one to " f"use, first {len(last_3)}:\n\n" f"- {'\n- '.join(last_3)}\n\n" From 7be310062fdc63e86a715e2ec06e1d454e113d9a Mon Sep 17 00:00:00 2001 From: Louis Opter Date: Tue, 21 Jan 2025 20:58:36 +0000 Subject: [PATCH 2/9] clan-cli: honor clan.core.sops.defaultGroups option in `vars fix` --- pkgs/clan-cli/clan_cli/vars/secret_modules/sops.py | 13 +++++++++++++ 1 file changed, 13 insertions(+) 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 1eb18e99f..e08b0eb29 100644 --- a/pkgs/clan-cli/clan_cli/vars/secret_modules/sops.py +++ b/pkgs/clan-cli/clan_cli/vars/secret_modules/sops.py @@ -7,15 +7,18 @@ from clan_cli.errors import ClanError from clan_cli.machines.machines import Machine from clan_cli.secrets import sops from clan_cli.secrets.folders import ( + sops_groups_folder, 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 ( + allow_member, collect_keys_for_path, decrypt_secret, encrypt_secret, + groups_folder, has_secret, ) from clan_cli.ssh.upload import upload @@ -284,6 +287,16 @@ class SecretStore(StoreBase): continue secret_path = self.secret_path(generator, file.name) + + for group in self.machine.deployment["sops"]["defaultGroups"]: + allow_member( + groups_folder(secret_path), + sops_groups_folder(self.machine.flake_dir), + group, + # we just want to create missing symlinks, we call update_keys below: + do_update_keys=False, + ) + update_keys( secret_path, collect_keys_for_path(secret_path), From 26d07cf1f589edda9a751a03cf7888c1278239a2 Mon Sep 17 00:00:00 2001 From: Louis Opter Date: Wed, 22 Jan 2025 18:10:17 +0000 Subject: [PATCH 3/9] clan-cli: secrets: commit changes when group members are added or removed --- pkgs/clan-cli/clan_cli/secrets/groups.py | 25 +++++++++++++++++++----- 1 file changed, 20 insertions(+), 5 deletions(-) diff --git a/pkgs/clan-cli/clan_cli/secrets/groups.py b/pkgs/clan-cli/clan_cli/secrets/groups.py index 5744eecb6..f58908ade 100644 --- a/pkgs/clan-cli/clan_cli/secrets/groups.py +++ b/pkgs/clan-cli/clan_cli/secrets/groups.py @@ -118,19 +118,22 @@ def add_member( raise ClanError(msg) user_target.unlink() user_target.symlink_to(os.path.relpath(source, user_target.parent)) - return update_group_keys(flake_dir, group_folder.parent.name) + changed_files = [user_target] + changed_files.extend(update_group_keys(flake_dir, group_folder.parent.name)) + return changed_files -def remove_member(flake_dir: Path, group_folder: Path, name: str) -> None: +def remove_member(flake_dir: Path, group_folder: Path, name: str) -> list[Path]: target = group_folder / name if not target.exists(): msg = f"{name} does not exist in group in {group_folder}: " msg += list_directory(group_folder) raise ClanError(msg) target.unlink() + updated_files = [target] if len(os.listdir(group_folder)) > 0: - update_group_keys(flake_dir, group_folder.parent.name) + updated_files.extend(update_group_keys(flake_dir, group_folder.parent.name)) if len(os.listdir(group_folder)) == 0: group_folder.rmdir() @@ -138,6 +141,8 @@ def remove_member(flake_dir: Path, group_folder: Path, name: str) -> None: if len(os.listdir(group_folder.parent)) == 0: group_folder.parent.rmdir() + return updated_files + def add_user(flake_dir: Path, group: str, name: str) -> None: updated_files = add_member( @@ -155,7 +160,12 @@ def add_user_command(args: argparse.Namespace) -> None: def remove_user(flake_dir: Path, group: str, name: str) -> None: - remove_member(flake_dir, users_folder(flake_dir, group), name) + updated_files = remove_member(flake_dir, users_folder(flake_dir, group), name) + commit_files( + updated_files, + flake_dir, + f"Remove user {name} from group {group}", + ) def remove_user_command(args: argparse.Namespace) -> None: @@ -181,7 +191,12 @@ def add_machine_command(args: argparse.Namespace) -> None: def remove_machine(flake_dir: Path, group: str, name: str) -> None: - remove_member(flake_dir, machines_folder(flake_dir, group), name) + updated_files = remove_member(flake_dir, machines_folder(flake_dir, group), name) + commit_files( + updated_files, + flake_dir, + f"Remove machine {name} from group {group}", + ) def remove_machine_command(args: argparse.Namespace) -> None: From e2c6b7bffc0df09a60d06c38d122273c074891ca Mon Sep 17 00:00:00 2001 From: Louis Opter Date: Wed, 22 Jan 2025 18:15:27 +0000 Subject: [PATCH 4/9] clan-cli: secrets key generate: fix call to action message So that it makes sense when an already existing PGP key is specified. --- pkgs/clan-cli/clan_cli/secrets/key.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/pkgs/clan-cli/clan_cli/secrets/key.py b/pkgs/clan-cli/clan_cli/secrets/key.py index 104c008ee..b8aff5670 100644 --- a/pkgs/clan-cli/clan_cli/secrets/key.py +++ b/pkgs/clan-cli/clan_cli/secrets/key.py @@ -33,9 +33,9 @@ def generate_key() -> sops.SopsKey: def generate_command(args: argparse.Namespace) -> None: key = generate_key() - print("Also add your age public key to the repository with:") key_type = key.key_type.name.lower() - print(f"clan secrets users add --{key_type}-key ") + print(f"Add your {key_type} public key to the repository with:") + print(f"clan secrets users add --{key_type}-key {key.pubkey}") def show_command(args: argparse.Namespace) -> None: From 5fc9440a1e3e3781f3a2d772980a77ce901f7c26 Mon Sep 17 00:00:00 2001 From: Louis Opter Date: Wed, 22 Jan 2025 18:16:35 +0000 Subject: [PATCH 5/9] clan-cli: secrets: truly catch broken symlinks in `collect_keys_for_type` --- pkgs/clan-cli/clan_cli/secrets/secrets.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkgs/clan-cli/clan_cli/secrets/secrets.py b/pkgs/clan-cli/clan_cli/secrets/secrets.py index 22a21b7aa..f576833d2 100644 --- a/pkgs/clan-cli/clan_cli/secrets/secrets.py +++ b/pkgs/clan-cli/clan_cli/secrets/secrets.py @@ -99,7 +99,7 @@ def collect_keys_for_type(folder: Path) -> set[tuple[str, sops.KeyType]]: if not p.is_symlink(): continue try: - target = p.resolve() + target = p.resolve(strict=True) except FileNotFoundError: log.warning(f"Ignoring broken symlink {p}") continue From 36a54ead12f189ca160e5c9e5b671c890dd1de77 Mon Sep 17 00:00:00 2001 From: Louis Opter Date: Sat, 1 Feb 2025 22:41:05 +0000 Subject: [PATCH 6/9] =?UTF-8?q?clan-cli:=20improve=20tests=20on=20`clan=20?= =?UTF-8?q?secrets=20=E2=80=A6`?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When users or groups are updated : - Check that keys are properly updated on sops secrets; - Check that no dangling symlinks are left behind in sops secrets. And when an user is removed from the clan, check that it is removed from the groups it belonged to. This doesn't check this works for vars explicitly, since they share the same logic, see `secret_paths.extend(list_vars_secrets(flake_dir))` in commit f2856cb773f11b3d32f246b9b07d79419575ab3f. Those improvements allow us to validate that #2659 is indeed fixed, and tell us that we need to make the same kind of fixes for machines and groups. For groups this is straightforward, and for machines, when one is deleted, I wanna discuss first whether we want to delete all its secrets as well. --- pkgs/clan-cli/tests/test_secrets_cli.py | 248 ++++++++++++++++++++++-- 1 file changed, 229 insertions(+), 19 deletions(-) diff --git a/pkgs/clan-cli/tests/test_secrets_cli.py b/pkgs/clan-cli/tests/test_secrets_cli.py index 85003a541..939fb541a 100644 --- a/pkgs/clan-cli/tests/test_secrets_cli.py +++ b/pkgs/clan-cli/tests/test_secrets_cli.py @@ -11,6 +11,7 @@ from typing import TYPE_CHECKING import pytest from clan_cli.errors import ClanError +from clan_cli.secrets.folders import sops_secrets_folder from fixtures_flakes import FlakeForTest from helpers import cli from stdout import CaptureOutput @@ -26,9 +27,18 @@ def _test_identities( test_flake: FlakeForTest, capture_output: CaptureOutput, age_keys: list["KeyPair"], + monkeypatch: pytest.MonkeyPatch, ) -> None: sops_folder = test_flake.path / "sops" + what_singular = what[:-1] + test_secret_name = f"{what_singular}_secret" + + # fake some admin user that's different from the identity, we are going to + # try to add/remove/update from the clan, this way we can check that keys + # are properly updated on secrets when an identity changes. + admin_age_key = age_keys[2] + cli.run( [ "secrets", @@ -42,6 +52,18 @@ def _test_identities( ) assert (sops_folder / what / "foo" / "key.json").exists() + cli.run( + [ + "secrets", + "users", + "add", + "--flake", + str(test_flake.path), + "admin", + admin_age_key.pubkey, + ] + ) + with pytest.raises(ClanError): # raises "foo already exists" cli.run( [ @@ -55,18 +77,44 @@ def _test_identities( ] ) - # rotate the key - cli.run( - [ - "secrets", - what, - "add", - "--flake", - str(test_flake.path), - "-f", - "foo", - age_keys[1].privkey, - ] + with monkeypatch.context(): + monkeypatch.setenv("SOPS_NIX_SECRET", "deadfeed") + monkeypatch.setenv("SOPS_AGE_KEY", admin_age_key.privkey) + cli.run( + [ + "secrets", + "set", + "--flake", + str(test_flake.path), + f"--{what_singular}", + "foo", + test_secret_name, + ] + ) + assert_sops_file_recipients( + test_flake.path, + test_secret_name, + expected_age_recipients_keypairs=[age_keys[0], admin_age_key], + ) + + with monkeypatch.context(): + monkeypatch.setenv("SOPS_AGE_KEY", admin_age_key.privkey) + cli.run( + [ + "secrets", + what, + "add", + "--flake", + str(test_flake.path), + "-f", + "foo", + age_keys[1].privkey, + ] + ) + assert_sops_file_recipients( + test_flake.path, + test_secret_name, + expected_age_recipients_keypairs=[age_keys[1], admin_age_key], ) with capture_output as output: @@ -96,26 +144,52 @@ def _test_identities( cli.run(["secrets", what, "list", "--flake", str(test_flake.path)]) assert "foo" not in output.out + if what == "machines": + # lopter@(2025-02-02): Let's address #2659 first and then figure out + # what we wanna do about secrets when a machine is deleted. + return + + user_symlink = sops_folder / "secrets" / test_secret_name / what / "foo" + err_msg = ( + f"Symlink to {what_singular} foo's key in secret " + f"{test_secret_name} was not cleaned up after " + f"{what_singular} foo was removed." + ) + assert not user_symlink.exists(follow_symlinks=False), err_msg + def test_users( - test_flake: FlakeForTest, capture_output: CaptureOutput, age_keys: list["KeyPair"] + test_flake: FlakeForTest, + capture_output: CaptureOutput, + age_keys: list["KeyPair"], + monkeypatch: pytest.MonkeyPatch, ) -> None: - _test_identities("users", test_flake, capture_output, age_keys) + _test_identities("users", test_flake, capture_output, age_keys, monkeypatch) def test_machines( - test_flake: FlakeForTest, capture_output: CaptureOutput, age_keys: list["KeyPair"] + test_flake: FlakeForTest, + capture_output: CaptureOutput, + age_keys: list["KeyPair"], + monkeypatch: pytest.MonkeyPatch, ) -> None: - _test_identities("machines", test_flake, capture_output, age_keys) + _test_identities("machines", test_flake, capture_output, age_keys, monkeypatch) def test_groups( - test_flake: FlakeForTest, capture_output: CaptureOutput, age_keys: list["KeyPair"] + test_flake: FlakeForTest, + capture_output: CaptureOutput, + age_keys: list["KeyPair"], + monkeypatch: pytest.MonkeyPatch, ) -> None: with capture_output as output: cli.run(["secrets", "groups", "list", "--flake", str(test_flake.path)]) assert output.out == "" + machine1_age_key = age_keys[0] + user1_age_key = age_keys[1] + admin_age_key = age_keys[2] + with pytest.raises(ClanError): # machine does not exist yet cli.run( [ @@ -148,7 +222,7 @@ def test_groups( "--flake", str(test_flake.path), "machine1", - age_keys[0].pubkey, + machine1_age_key.pubkey, ] ) cli.run( @@ -184,7 +258,18 @@ def test_groups( "--flake", str(test_flake.path), "user1", - age_keys[0].pubkey, + user1_age_key.pubkey, + ] + ) + cli.run( + [ + "secrets", + "users", + "add", + "--flake", + str(test_flake.path), + "admin", + admin_age_key.pubkey, ] ) cli.run( @@ -205,6 +290,37 @@ def test_groups( assert "user1" in out assert "machine1" in out + secret_name = "foo" + + with monkeypatch.context(): + monkeypatch.setenv("SOPS_NIX_SECRET", "deafbeef") + monkeypatch.setenv("SOPS_AGE_KEY", admin_age_key.privkey) + cli.run( + [ + "secrets", + "set", + "--flake", + str(test_flake.path), + "--group", + "group1", + secret_name, + ] + ) + + assert_sops_file_recipients( + test_flake.path, + secret_name, + expected_age_recipients_keypairs=[ + machine1_age_key, + user1_age_key, + admin_age_key, + ], + err_msg=( + f"The secret `{secret_name}` owned by group1 was not encrypted " + f"with all members of the group." + ), + ) + cli.run( [ "secrets", @@ -216,6 +332,59 @@ def test_groups( "user1", ] ) + assert_sops_file_recipients( + test_flake.path, + secret_name, + expected_age_recipients_keypairs=[machine1_age_key, admin_age_key], + err_msg=( + f"The secret `{secret_name}` owned by group1 is still encrypted for " + f"`user1` even though this user has been removed from the group." + ), + ) + + # re-add the user to the group + cli.run( + [ + "secrets", + "groups", + "add-user", + "--flake", + str(test_flake.path), + "group1", + "user1", + ] + ) + assert_sops_file_recipients( + test_flake.path, + secret_name, + expected_age_recipients_keypairs=[ + machine1_age_key, + user1_age_key, + admin_age_key, + ], + ) + # and instead of removing the user from the group, remove the + # user instead, it should also remove it from the group: + cli.run( + [ + "secrets", + "users", + "remove", + "--flake", + str(test_flake.path), + "user1", + ] + ) + assert_sops_file_recipients( + test_flake.path, + secret_name, + expected_age_recipients_keypairs=[machine1_age_key, admin_age_key], + err_msg=( + f"The secret `{secret_name}` owned by group1 is still encrypted " + f"for `user1` even though this user has been deleted." + ), + ) + cli.run( [ "secrets", @@ -227,9 +396,50 @@ def test_groups( "machine1", ] ) + assert_sops_file_recipients( + test_flake.path, + secret_name, + expected_age_recipients_keypairs=[admin_age_key], + err_msg=( + f"The secret `{secret_name}` owned by group1 is still encrypted for " + f"`machine1` even though this machine has been removed from the group." + ), + ) + groups = os.listdir(test_flake.path / "sops" / "groups") assert len(groups) == 0 + # Check if the symlink to the group was removed our foo test secret: + group_symlink = test_flake.path / "sops/secrets/foo/groups/group1" + err_msg = ( + "Symlink to group1's key in foo secret " + "was not cleaned up after group1 was removed" + ) + assert not group_symlink.exists(follow_symlinks=False), err_msg + + +def assert_sops_file_recipients( + flake_path: Path, + secret_name: str, + expected_age_recipients_keypairs: list["KeyPair"], + err_msg: str | None = None, +) -> None: + """Checks that the recipients of a SOPS file matches expectations. + + :param err_msg: in case of failure, if you gave an error message then it + will be displayed, otherwise pytest will display the two different sets + of recipients. + """ + sops_file = sops_secrets_folder(flake_path) / secret_name / "secret" + with sops_file.open("rb") as fp: + sops_data = json.load(fp) + age_recipients = {each["recipient"] for each in sops_data["sops"]["age"]} + expected_age_recipients = {pair.pubkey for pair in expected_age_recipients_keypairs} + if not err_msg: + assert age_recipients == expected_age_recipients + return + assert age_recipients == expected_age_recipients, err_msg + @contextmanager def use_age_key(key: str, monkeypatch: pytest.MonkeyPatch) -> Iterator[None]: From 81cf521d8c66dfdfd811506418e95de9ed0030b0 Mon Sep 17 00:00:00 2001 From: Louis Opter Date: Wed, 22 Jan 2025 18:16:54 +0000 Subject: [PATCH 7/9] clan-cli: secrets: properly update secrets when an user is removed Fixes #2659. --- pkgs/clan-cli/clan_cli/secrets/secrets.py | 16 ++++++++++ pkgs/clan-cli/clan_cli/secrets/users.py | 36 ++++++++++++++++------- 2 files changed, 42 insertions(+), 10 deletions(-) diff --git a/pkgs/clan-cli/clan_cli/secrets/secrets.py b/pkgs/clan-cli/clan_cli/secrets/secrets.py index f576833d2..d0c5786f1 100644 --- a/pkgs/clan-cli/clan_cli/secrets/secrets.py +++ b/pkgs/clan-cli/clan_cli/secrets/secrets.py @@ -82,6 +82,11 @@ def update_secrets( for path in secret_paths: if not filter_secrets(path): continue + # clean-up non-existent users, groups, and machines + # from the secret before we update it: + changed_files.extend(cleanup_dangling_symlinks(path / "users")) + changed_files.extend(cleanup_dangling_symlinks(path / "groups")) + changed_files.extend(cleanup_dangling_symlinks(path / "machines")) changed_files.extend( update_keys( path, @@ -91,6 +96,17 @@ def update_secrets( return changed_files +def cleanup_dangling_symlinks(folder: Path) -> list[Path]: + if not folder.exists(): + return [] + removed: list[Path] = [] + for link in folder.iterdir(): + if link.is_symlink() and not link.exists(): + link.unlink() + removed.append(folder / link) + return removed + + def collect_keys_for_type(folder: Path) -> set[tuple[str, sops.KeyType]]: if not folder.exists(): return set() diff --git a/pkgs/clan-cli/clan_cli/secrets/users.py b/pkgs/clan-cli/clan_cli/secrets/users.py index e22ba69b4..658e40ba0 100644 --- a/pkgs/clan-cli/clan_cli/secrets/users.py +++ b/pkgs/clan-cli/clan_cli/secrets/users.py @@ -1,5 +1,7 @@ import argparse import json +import logging +import os import sys from pathlib import Path @@ -7,14 +9,14 @@ from clan_cli.completions import add_dynamic_completer, complete_secrets, comple from clan_cli.errors import ClanError from clan_cli.git import commit_files -from . import secrets, sops +from . import groups, secrets, sops from .folders import ( list_objects, remove_object, + sops_groups_folder, sops_secrets_folder, sops_users_folder, ) -from .groups import get_groups from .secrets import update_secrets from .sops import read_key, write_key from .types import ( @@ -24,6 +26,8 @@ from .types import ( user_name_type, ) +log = logging.getLogger(__name__) + def add_user( flake_dir: Path, @@ -34,12 +38,12 @@ def add_user( ) -> None: path = sops_users_folder(flake_dir) / name - groups = get_groups(flake_dir, "users", name) + groupnames = [p.name for p in groups.get_groups(flake_dir, "users", name)] def filter_user_secrets(secret: Path) -> bool: if secret.joinpath("users", name).exists(): return True - return any(secret.joinpath("groups", group.name).exists() for group in groups) + return any(secret.joinpath("groups", name).exists() for name in groupnames) write_key(path, key, key_type, overwrite=force) paths = [path] @@ -53,12 +57,24 @@ def add_user( def remove_user(flake_dir: Path, name: str) -> None: - removed_paths = remove_object(sops_users_folder(flake_dir), name) - commit_files( - removed_paths, - flake_dir, - f"Remove user {name}", - ) + updated_paths: list[Path] = [] + # Remove the user from any group where it belonged: + groups_dir = sops_groups_folder(flake_dir) + if groups_dir.exists(): + for group in os.listdir(groups_dir): + group_folder = groups_dir / group + if not group_folder.is_dir(): + continue + memberships = group_folder / "users" + if not (memberships / name).exists(): + continue + log.info(f"Removing user {name} from group {group}") + updated_paths.extend(groups.remove_member(flake_dir, memberships, name)) + # Remove the user's key: + updated_paths.extend(remove_object(sops_users_folder(flake_dir), name)) + # Remove the user from any secret where it was used: + updated_paths.extend(update_secrets(flake_dir)) + commit_files(updated_paths, flake_dir, f"Remove user {name}") def get_user(flake_dir: Path, name: str) -> sops.SopsKey: From ef442ef31663c818c1b2baa4974ffc3fa3bd2e45 Mon Sep 17 00:00:00 2001 From: Louis Opter Date: Sat, 1 Feb 2025 22:39:22 +0000 Subject: [PATCH 8/9] clan-cli: secrets users add: make sure only one key argument was passed This is slightly better input validation FWIW. --- pkgs/clan-cli/clan_cli/secrets/users.py | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/pkgs/clan-cli/clan_cli/secrets/users.py b/pkgs/clan-cli/clan_cli/secrets/users.py index 658e40ba0..2d7598532 100644 --- a/pkgs/clan-cli/clan_cli/secrets/users.py +++ b/pkgs/clan-cli/clan_cli/secrets/users.py @@ -131,15 +131,19 @@ def add_command(args: argparse.Namespace) -> None: if args.flake is None: msg = "Could not find clan flake toplevel directory" raise ClanError(msg) + keys_args = (args.age_key, args.agekey, args.pgp_key) + keys_count = sum(1 if key else 0 for key in keys_args) + if keys_count != 1: + err_msg = ( + f"Please provide one key (got {keys_count}) through `--pgp-key`, " + f"`--age-key`, or as a positional (age key) argument." + ) + raise ClanError(err_msg) if args.age_key or args.agekey: key_type = sops.KeyType.AGE - elif args.pgp_key: - key_type = sops.KeyType.PGP else: - msg = "BUG!: key type not set" - raise ValueError(msg) + key_type = sops.KeyType.PGP key = args.agekey or args.age_key or args.pgp_key - assert key is not None, "key is None" add_user(args.flake.path, args.user, key, key_type, args.force) From 48268f796085fd7e87bbbbccb4e6e0d34413e135 Mon Sep 17 00:00:00 2001 From: Louis Opter Date: Sun, 2 Feb 2025 20:31:59 +0000 Subject: [PATCH 9/9] clan-cli: do not skip `secrets.update_secrets` when a group is removed We need to remove all keys that were in the group from affected secrets. With this change we now take `group_name` as an argument in `{add,remove}_member`, which is a little bit more readable than `group_folder.parent.name`, and helps DRY the code a bit. --- pkgs/clan-cli/clan_cli/secrets/groups.py | 50 ++++++++++++++++++------ pkgs/clan-cli/clan_cli/secrets/users.py | 4 +- pkgs/clan-cli/tests/test_secrets_cli.py | 2 +- 3 files changed, 43 insertions(+), 13 deletions(-) diff --git a/pkgs/clan-cli/clan_cli/secrets/groups.py b/pkgs/clan-cli/clan_cli/secrets/groups.py index f58908ade..ee7b4cf06 100644 --- a/pkgs/clan-cli/clan_cli/secrets/groups.py +++ b/pkgs/clan-cli/clan_cli/secrets/groups.py @@ -1,5 +1,6 @@ import argparse import os +from collections.abc import Callable from pathlib import Path from clan_cli.completions import ( @@ -103,13 +104,19 @@ def update_group_keys(flake_dir: Path, group: str) -> list[Path]: def add_member( - flake_dir: Path, group_folder: Path, source_folder: Path, name: str + flake_dir: Path, + group_name: str, + get_group_folder: Callable[[Path, str], Path], + get_source_folder: Callable[[Path], Path], + name: str, ) -> list[Path]: + source_folder = get_source_folder(flake_dir) source = source_folder / name if not source.exists(): msg = f"{name} does not exist in {source_folder}: " msg += list_directory(source_folder) raise ClanError(msg) + group_folder = get_group_folder(flake_dir, group_name) group_folder.mkdir(parents=True, exist_ok=True) user_target = group_folder / name if user_target.exists(): @@ -119,11 +126,18 @@ def add_member( user_target.unlink() user_target.symlink_to(os.path.relpath(source, user_target.parent)) changed_files = [user_target] - changed_files.extend(update_group_keys(flake_dir, group_folder.parent.name)) + group_name = group_folder.parent.name + changed_files.extend(update_group_keys(flake_dir, group_name)) return changed_files -def remove_member(flake_dir: Path, group_folder: Path, name: str) -> list[Path]: +def remove_member( + flake_dir: Path, + group_name: str, + get_group_folder: Callable[[Path, str], Path], + name: str, +) -> list[Path]: + group_folder = get_group_folder(flake_dir, group_name) target = group_folder / name if not target.exists(): msg = f"{name} does not exist in group in {group_folder}: " @@ -132,21 +146,24 @@ def remove_member(flake_dir: Path, group_folder: Path, name: str) -> list[Path]: target.unlink() updated_files = [target] - if len(os.listdir(group_folder)) > 0: - updated_files.extend(update_group_keys(flake_dir, group_folder.parent.name)) - if len(os.listdir(group_folder)) == 0: group_folder.rmdir() if len(os.listdir(group_folder.parent)) == 0: group_folder.parent.rmdir() + updated_files.extend(update_group_keys(flake_dir, group_name)) + return updated_files def add_user(flake_dir: Path, group: str, name: str) -> None: updated_files = add_member( - flake_dir, users_folder(flake_dir, group), sops_users_folder(flake_dir), name + flake_dir, + group, + users_folder, + sops_users_folder, + name, ) commit_files( updated_files, @@ -160,7 +177,12 @@ def add_user_command(args: argparse.Namespace) -> None: def remove_user(flake_dir: Path, group: str, name: str) -> None: - updated_files = remove_member(flake_dir, users_folder(flake_dir, group), name) + updated_files = remove_member( + flake_dir, + group, + users_folder, + name, + ) commit_files( updated_files, flake_dir, @@ -175,8 +197,9 @@ def remove_user_command(args: argparse.Namespace) -> None: def add_machine(flake_dir: Path, group: str, name: str) -> None: updated_files = add_member( flake_dir, - machines_folder(flake_dir, group), - sops_machines_folder(flake_dir), + group, + machines_folder, + sops_machines_folder, name, ) commit_files( @@ -191,7 +214,12 @@ def add_machine_command(args: argparse.Namespace) -> None: def remove_machine(flake_dir: Path, group: str, name: str) -> None: - updated_files = remove_member(flake_dir, machines_folder(flake_dir, group), name) + updated_files = remove_member( + flake_dir, + group, + machines_folder, + name, + ) commit_files( updated_files, flake_dir, diff --git a/pkgs/clan-cli/clan_cli/secrets/users.py b/pkgs/clan-cli/clan_cli/secrets/users.py index 2d7598532..44a37417d 100644 --- a/pkgs/clan-cli/clan_cli/secrets/users.py +++ b/pkgs/clan-cli/clan_cli/secrets/users.py @@ -69,7 +69,9 @@ def remove_user(flake_dir: Path, name: str) -> None: if not (memberships / name).exists(): continue log.info(f"Removing user {name} from group {group}") - updated_paths.extend(groups.remove_member(flake_dir, memberships, name)) + updated_paths.extend( + groups.remove_member(flake_dir, group, groups.users_folder, name) + ) # Remove the user's key: updated_paths.extend(remove_object(sops_users_folder(flake_dir), name)) # Remove the user from any secret where it was used: diff --git a/pkgs/clan-cli/tests/test_secrets_cli.py b/pkgs/clan-cli/tests/test_secrets_cli.py index 939fb541a..a28f5153b 100644 --- a/pkgs/clan-cli/tests/test_secrets_cli.py +++ b/pkgs/clan-cli/tests/test_secrets_cli.py @@ -409,7 +409,7 @@ def test_groups( groups = os.listdir(test_flake.path / "sops" / "groups") assert len(groups) == 0 - # Check if the symlink to the group was removed our foo test secret: + # Check if the symlink to the group was removed from our foo test secret: group_symlink = test_flake.path / "sops/secrets/foo/groups/group1" err_msg = ( "Symlink to group1's key in foo secret "