From d3cd481600bf95ee0dbad6fa9ab3841c2e92b3e6 Mon Sep 17 00:00:00 2001 From: lassulus Date: Fri, 4 Jul 2025 14:35:33 +0200 Subject: [PATCH 1/2] fix(flake): handle file paths with line numbers in cache existence check The is_cached method now correctly handles store paths that have line numbers appended (e.g., /nix/store/file.nix:123:456). Previously, these paths would fail the existence check because the exact path with line numbers doesn't exist as a file. The fix adds a helper method that: - First checks if the exact path exists - If not, and the path contains colons, validates that the suffix consists only of numbers (line:column format) - If valid, strips the line numbers and checks the base file path This ensures that cached references to specific file locations are properly validated while avoiding false positives with files that have colons in their names. --- pkgs/clan-cli/clan_lib/flake/flake.py | 21 ++++- .../clan_lib/flake/flake_cache_test.py | 78 +++++++++++++++++++ 2 files changed, 97 insertions(+), 2 deletions(-) diff --git a/pkgs/clan-cli/clan_lib/flake/flake.py b/pkgs/clan-cli/clan_lib/flake/flake.py index 2d82a8acb..e4beaf2c2 100644 --- a/pkgs/clan-cli/clan_lib/flake/flake.py +++ b/pkgs/clan-cli/clan_lib/flake/flake.py @@ -345,6 +345,23 @@ class FlakeCacheEntry: msg = f"Cannot insert {value} into cache, already have {self.value}" raise TypeError(msg) + def _check_path_exists(self, path_str: str) -> bool: + """Check if a path exists, handling potential line number suffixes.""" + path = Path(path_str) + if path.exists(): + return True + + # Try stripping line numbers if the path doesn't exist + # Handle format: /path/to/file:123 or /path/to/file:123:456 + if ":" in path_str: + parts = path_str.split(":") + if len(parts) >= 2: + # Check if all parts after the first colon are numbers + if all(part.isdigit() for part in parts[1:]): + base_path = parts[0] + return Path(base_path).exists() + return False + def is_cached(self, selectors: list[Selector]) -> bool: selector: Selector @@ -353,12 +370,12 @@ class FlakeCacheEntry: # Check if it's a regular nix store path nix_store_dir = os.environ.get("NIX_STORE_DIR", "/nix/store") if self.value.startswith(nix_store_dir): - return Path(self.value).exists() + return self._check_path_exists(self.value) # Check if it's a test store path test_store = os.environ.get("CLAN_TEST_STORE") if test_store and self.value.startswith(test_store): - return Path(self.value).exists() + return self._check_path_exists(self.value) # if self.value is not dict but we request more selectors, we assume we are cached and an error will be thrown in the select function if isinstance(self.value, str | float | int | None): diff --git a/pkgs/clan-cli/clan_lib/flake/flake_cache_test.py b/pkgs/clan-cli/clan_lib/flake/flake_cache_test.py index 625abf913..9849752ed 100644 --- a/pkgs/clan-cli/clan_lib/flake/flake_cache_test.py +++ b/pkgs/clan-cli/clan_lib/flake/flake_cache_test.py @@ -296,3 +296,81 @@ def test_cache_gc(tmp_path: Path, monkeypatch: pytest.MonkeyPatch) -> None: assert my_flake._cache.is_cached("testfile") # noqa: SLF001 subprocess.run(["nix-collect-garbage"], check=True) assert not my_flake._cache.is_cached("testfile") # noqa: SLF001 + + +def test_cache_path_with_line_numbers( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + """Test that is_cached correctly handles store paths with line numbers appended. + + This is a regression test for the bug where cached store paths with line numbers + (e.g., /nix/store/path:123) are not properly checked for existence. + """ + # Create a temporary store + test_store = tmp_path / "test-store" + test_store.mkdir() + + # Set CLAN_TEST_STORE environment variable + monkeypatch.setenv("CLAN_TEST_STORE", str(test_store)) + + # Create a fake store path + fake_store_path = test_store / "abc123-source-file.nix" + fake_store_path.write_text("# nix source file\n{ foo = 123; }") + + # Create cache entries for paths with line numbers + cache = FlakeCacheEntry() + + # Test single line number format + path_with_line = f"{fake_store_path}:42" + selectors = parse_selector("testPath1") + cache.insert(path_with_line, selectors) + + # Test line:column format + path_with_line_col = f"{fake_store_path}:42:10" + selectors2 = parse_selector("testPath2") + cache.insert(path_with_line_col, selectors2) + + # Test path with colon but non-numeric suffix (should not be treated as line number) + path_with_colon = test_store / "file:with:colons" + path_with_colon.write_text("test") + selectors3 = parse_selector("testPath3") + cache.insert(str(path_with_colon), selectors3) + + # Before the fix: These would return True even though the exact paths don't exist + # After the fix: They check the base file path exists + assert cache.is_cached(parse_selector("testPath1")), ( + "Path with line number should be cached when base file exists" + ) + assert cache.is_cached(parse_selector("testPath2")), ( + "Path with line:column should be cached when base file exists" + ) + assert cache.is_cached(parse_selector("testPath3")), ( + "Path with colons in filename should be cached when file exists" + ) + + # Now delete the base file + fake_store_path.unlink() + + # After deletion, paths with line numbers should not be cached + assert not cache.is_cached(parse_selector("testPath1")), ( + "Path with line number should not be cached when base file doesn't exist" + ) + assert not cache.is_cached(parse_selector("testPath2")), ( + "Path with line:column should not be cached when base file doesn't exist" + ) + + # Path with colons in name still exists + assert cache.is_cached(parse_selector("testPath3")), ( + "Path with colons in filename should still be cached" + ) + + # Test with regular /nix/store paths + monkeypatch.delenv("CLAN_TEST_STORE", raising=False) + cache2 = FlakeCacheEntry() + nix_path_with_line = "/nix/store/fake-source.nix:123" + cache2.insert(nix_path_with_line, parse_selector("nixPath")) + + # Should return False because neither the exact path nor base path exists + assert not cache2.is_cached(parse_selector("nixPath")), ( + "Nix store path with line number should not be cached when file doesn't exist" + ) From 2f0f9a9fbac7a9c249756158409b74969673cc4d Mon Sep 17 00:00:00 2001 From: lassulus Date: Sun, 29 Jun 2025 10:51:18 +0200 Subject: [PATCH 2/2] vars/password-store: replace passBackend option with passPackage The `clan.core.vars.settings.passBackend` option has been replaced with `clan.vars.password-store.passPackage` to provide better type safety and clearer configuration. Changes: - Remove problematic mkRemovedOptionModule that caused circular dependency - Add proper option definition with assertion-based migration - Users setting the old option get clear migration instructions - Normal evaluation continues to work for users not using the old option Migration: Replace `clan.core.vars.settings.passBackend = "passage"` with `clan.vars.password-store.passPackage = pkgs.passage` --- nixosModules/clanCore/vars/default.nix | 12 ++ .../clanCore/vars/secret/password-store.nix | 9 +- nixosModules/clanCore/vars/settings-opts.nix | 22 +-- pkgs/clan-cli/clan_cli/machines/update.py | 2 - pkgs/clan-cli/clan_cli/tests/test_vars.py | 13 ++ pkgs/clan-cli/clan_cli/vars/generate.py | 9 +- .../vars/secret_modules/password_store.py | 157 +++++++++--------- pkgs/clan-cli/default.nix | 3 + 8 files changed, 129 insertions(+), 98 deletions(-) diff --git a/nixosModules/clanCore/vars/default.nix b/nixosModules/clanCore/vars/default.nix index 0fa5c6165..afe8962ad 100644 --- a/nixosModules/clanCore/vars/default.nix +++ b/nixosModules/clanCore/vars/default.nix @@ -40,6 +40,18 @@ in }; config = { + # Check for removed passBackend option usage + assertions = [ + { + assertion = config.clan.core.vars.settings.passBackend == null; + message = '' + The option `clan.core.vars.settings.passBackend' has been removed. + Use clan.vars.password-store.passPackage instead. + Set it to pkgs.pass for GPG or pkgs.passage for age encryption. + ''; + } + ]; + # check all that all non-secret files have no owner/group/mode set warnings = lib.foldl' ( warnings: generator: diff --git a/nixosModules/clanCore/vars/secret/password-store.nix b/nixosModules/clanCore/vars/secret/password-store.nix index 8b02be38d..d79c46cf4 100644 --- a/nixosModules/clanCore/vars/secret/password-store.nix +++ b/nixosModules/clanCore/vars/secret/password-store.nix @@ -62,6 +62,13 @@ in location where the tarball with the password-store secrets will be uploaded to and the manifest ''; }; + passPackage = lib.mkOption { + type = lib.types.package; + default = pkgs.pass; + description = '' + Password store package to use. Can be pkgs.pass for GPG-based storage or pkgs.passage for age-based storage. + ''; + }; }; config = { clan.core.vars.settings = @@ -76,7 +83,7 @@ in else if file.config.neededFor == "services" then "/run/secrets/${file.config.generatorName}/${file.config.name}" else if file.config.neededFor == "activation" then - "${config.clan.password-store.secretLocation}/activation/${file.config.generatorName}/${file.config.name}" + "${config.clan.vars.password-store.secretLocation}/activation/${file.config.generatorName}/${file.config.name}" else if file.config.neededFor == "partitioning" then "/run/partitioning-secrets/${file.config.generatorName}/${file.config.name}" else diff --git a/nixosModules/clanCore/vars/settings-opts.nix b/nixosModules/clanCore/vars/settings-opts.nix index 276da4e9e..226888111 100644 --- a/nixosModules/clanCore/vars/settings-opts.nix +++ b/nixosModules/clanCore/vars/settings-opts.nix @@ -15,17 +15,6 @@ ''; }; - passBackend = lib.mkOption { - type = lib.types.enum [ - "passage" - "pass" - ]; - default = "pass"; - description = '' - password-store backend to use. Valid options are `pass` and `passage` - ''; - }; - secretModule = lib.mkOption { type = lib.types.str; internal = true; @@ -65,4 +54,15 @@ the python import path to the public module ''; }; + + # Legacy option that guides migration + passBackend = lib.mkOption { + type = lib.types.nullOr lib.types.str; + default = null; + visible = false; + description = '' + DEPRECATED: This option has been removed. Use clan.vars.password-store.passPackage instead. + Set it to pkgs.pass for GPG or pkgs.passage for age encryption. + ''; + }; } diff --git a/pkgs/clan-cli/clan_cli/machines/update.py b/pkgs/clan-cli/clan_cli/machines/update.py index e0f539446..75c543c8c 100644 --- a/pkgs/clan-cli/clan_cli/machines/update.py +++ b/pkgs/clan-cli/clan_cli/machines/update.py @@ -124,8 +124,6 @@ def update_command(args: argparse.Namespace) -> None: f"clanInternals.machines.{system}.{{{','.join(machine_names)}}}.config.clan.core.vars.generators.*.{{share,dependencies,migrateFact,prompts}}", f"clanInternals.machines.{system}.{{{','.join(machine_names)}}}.config.clan.core.vars.generators.*.files.*.{{secret,deploy,owner,group,mode,neededFor}}", f"clanInternals.machines.{system}.{{{','.join(machine_names)}}}.config.clan.core.facts.secretUploadDirectory", - f"clanInternals.machines.{system}.{{{','.join(machine_names)}}}.config.clan.vars.password-store.secretLocation", - f"clanInternals.machines.{system}.{{{','.join(machine_names)}}}.config.clan.core.vars.settings.passBackend", ] ) diff --git a/pkgs/clan-cli/clan_cli/tests/test_vars.py b/pkgs/clan-cli/clan_cli/tests/test_vars.py index 181b2a543..274dd38fc 100644 --- a/pkgs/clan-cli/clan_cli/tests/test_vars.py +++ b/pkgs/clan-cli/clan_cli/tests/test_vars.py @@ -403,6 +403,19 @@ def test_generate_secret_var_password_store( shutil.copytree(test_root / "data" / "password-store", password_store_dir) monkeypatch.setenv("PASSWORD_STORE_DIR", str(password_store_dir)) + # Initialize password store as a git repository + import subprocess + + subprocess.run(["git", "init"], cwd=password_store_dir, check=True) + subprocess.run( + ["git", "config", "user.email", "test@example.com"], + cwd=password_store_dir, + check=True, + ) + subprocess.run( + ["git", "config", "user.name", "Test User"], cwd=password_store_dir, check=True + ) + machine = Machine(name="my_machine", flake=Flake(str(flake.path))) assert not check_vars(machine.name, machine.flake) cli.run(["vars", "generate", "--flake", str(flake.path), "my_machine"]) diff --git a/pkgs/clan-cli/clan_cli/vars/generate.py b/pkgs/clan-cli/clan_cli/vars/generate.py index 925c97b1a..2d1ef9c08 100644 --- a/pkgs/clan-cli/clan_cli/vars/generate.py +++ b/pkgs/clan-cli/clan_cli/vars/generate.py @@ -82,11 +82,6 @@ class Generator: files = [] gen_files = files_data.get(gen_name, {}) for file_name, file_data in gen_files.items(): - # Handle mode conversion properly - mode = file_data["mode"] - if isinstance(mode, str): - mode = int(mode, 8) - var = Var( id=f"{gen_name}/{file_name}", name=file_name, @@ -94,7 +89,9 @@ class Generator: deploy=file_data["deploy"], owner=file_data["owner"], group=file_data["group"], - mode=mode, + mode=file_data["mode"] + if isinstance(file_data["mode"], int) + else int(file_data["mode"], 8), needed_for=file_data["neededFor"], ) files.append(var) diff --git a/pkgs/clan-cli/clan_cli/vars/secret_modules/password_store.py b/pkgs/clan-cli/clan_cli/vars/secret_modules/password_store.py index 98060a997..d23cf18a4 100644 --- a/pkgs/clan-cli/clan_cli/vars/secret_modules/password_store.py +++ b/pkgs/clan-cli/clan_cli/vars/secret_modules/password_store.py @@ -1,9 +1,7 @@ import io import logging -import os import tarfile from collections.abc import Iterable -from itertools import chain from pathlib import Path from tempfile import TemporaryDirectory @@ -12,7 +10,6 @@ from clan_cli.vars._types import StoreBase from clan_cli.vars.generate import Generator, Var from clan_lib.cmd import CmdOut, Log, RunOpts, run from clan_lib.machines.machines import Machine -from clan_lib.nix import nix_shell from clan_lib.ssh.remote import Remote log = logging.getLogger(__name__) @@ -26,31 +23,64 @@ class SecretStore(StoreBase): def __init__(self, machine: Machine) -> None: self.machine = machine self.entry_prefix = "clan-vars" + self._store_dir: Path | None = None @property def store_name(self) -> str: return "password_store" @property - def _store_backend(self) -> str: - backend = self.machine.select("config.clan.core.vars.settings.passBackend") - return backend + def store_dir(self) -> Path: + """Get the password store directory, cached after first access.""" + if self._store_dir is None: + result = self._run_pass( + "git", "rev-parse", "--show-toplevel", options=RunOpts(check=False) + ) + if result.returncode != 0: + msg = "Password store must be a git repository" + raise ValueError(msg) + self._store_dir = Path(result.stdout.strip()) + return self._store_dir @property - def _password_store_dir(self) -> Path: - if self._store_backend == "passage": - lookup = os.environ.get("PASSAGE_DIR") - default = Path.home() / ".passage/store" - else: - lookup = os.environ.get("PASSWORD_STORE_DIR") - default = Path.home() / ".password-store" - return Path(lookup) if lookup else default + def _pass_command(self) -> str: + out_path = self.machine.select( + "config.clan.vars.password-store.passPackage.outPath" + ) + main_program = ( + self.machine.select( + "config.clan.vars.password-store.passPackage.?meta.?mainProgram" + ) + .get("meta", {}) + .get("mainProgram") + ) + + if main_program: + binary_path = Path(out_path) / "bin" / main_program + if binary_path.exists(): + return str(binary_path) + + # Look for common password store binaries + bin_dir = Path(out_path) / "bin" + if bin_dir.exists(): + for binary in ["pass", "passage"]: + binary_path = bin_dir / binary + if binary_path.exists(): + return str(binary_path) + + # If only one binary exists, use it + binaries = [f for f in bin_dir.iterdir() if f.is_file()] + if len(binaries) == 1: + return str(binaries[0]) + + msg = "Could not find password store binary in package" + raise ValueError(msg) def entry_dir(self, generator: Generator, name: str) -> Path: return Path(self.entry_prefix) / self.rel_dir(generator, name) def _run_pass(self, *args: str, options: RunOpts | None = None) -> CmdOut: - cmd = nix_shell(packages=["pass"], cmd=[self._store_backend, *args]) + cmd = [self._pass_command, *args] return run(cmd, options) def _set( @@ -68,9 +98,11 @@ class SecretStore(StoreBase): return self._run_pass("show", pass_name).stdout.encode() def exists(self, generator: Generator, name: str) -> bool: - extension = "age" if self._store_backend == "passage" else "gpg" - filename = f"{self.entry_dir(generator, name)}.{extension}" - return (self._password_store_dir / filename).exists() + pass_name = str(self.entry_dir(generator, name)) + # Check if the file exists with either .age or .gpg extension + age_file = self.store_dir / f"{pass_name}.age" + gpg_file = self.store_dir / f"{pass_name}.gpg" + return age_file.exists() or gpg_file.exists() def delete(self, generator: Generator, name: str) -> Iterable[Path]: pass_name = str(self.entry_dir(generator, name)) @@ -79,66 +111,31 @@ class SecretStore(StoreBase): def delete_store(self) -> Iterable[Path]: machine_dir = Path(self.entry_prefix) / "per-machine" / self.machine.name - if not (self._password_store_dir / machine_dir).exists(): - # The directory may not exist if the machine - # has no vars, or they have been deleted already. - return [] - pass_call = ["rm", "--force", "--recursive", str(machine_dir)] - self._run_pass(*pass_call, options=RunOpts(check=True)) + # Check if the directory exists in the password store before trying to delete + result = self._run_pass("ls", str(machine_dir), options=RunOpts(check=False)) + if result.returncode == 0: + self._run_pass( + "rm", + "--force", + "--recursive", + str(machine_dir), + options=RunOpts(check=True), + ) return [] def generate_hash(self) -> bytes: - hashes = [] - hashes.append( - run( - nix_shell( - ["git"], - [ - "git", - "-C", - str(self._password_store_dir), - "log", - "-1", - "--format=%H", - self.entry_prefix, - ], - ), - RunOpts(check=False), - ) - .stdout.strip() - .encode() + result = self._run_pass( + "git", + "log", + "-1", + "--format=%H", + self.entry_prefix, + options=RunOpts(check=False), ) - shared_dir = self._password_store_dir / self.entry_prefix / "shared" - machine_dir = ( - self._password_store_dir - / self.entry_prefix - / "per-machine" - / self.machine.name - ) - for symlink in chain(shared_dir.glob("**/*"), machine_dir.glob("**/*")): - if symlink.is_symlink(): - hashes.append( - run( - nix_shell( - ["git"], - [ - "git", - "-C", - str(self._password_store_dir), - "log", - "-1", - "--format=%H", - str(symlink), - ], - ), - RunOpts(check=False), - ) - .stdout.strip() - .encode() - ) + git_hash = result.stdout.strip().encode() - # we sort the hashes to make sure that the order is always the same - hashes.sort() + if not git_hash: + return b"" from clan_cli.vars.generate import Generator @@ -149,22 +146,24 @@ class SecretStore(StoreBase): for generator in generators: for file in generator.files: manifest.append(f"{generator.name}/{file.name}".encode()) - manifest += hashes + + manifest.append(git_hash) return b"\n".join(manifest) def needs_upload(self, host: Remote) -> bool: local_hash = self.generate_hash() + if not local_hash: + return True + remote_hash = host.run( - # TODO get the path to the secrets from the machine [ "cat", - f"{self.machine.select('config.clan.vars.password-store.secretLocation')}/.{self._store_backend}_info", + f"{self.machine.select('config.clan.vars.password-store.secretLocation')}/.pass_info", ], RunOpts(log=Log.STDERR, check=False), ).stdout.strip() if not remote_hash: - print("remote hash is empty") return True return local_hash.decode() != remote_hash @@ -233,7 +232,9 @@ class SecretStore(StoreBase): out_file.parent.mkdir(parents=True, exist_ok=True) out_file.write_bytes(self.get(generator, file.name)) - (output_dir / f".{self._store_backend}_info").write_bytes(self.generate_hash()) + hash_data = self.generate_hash() + if hash_data: + (output_dir / ".pass_info").write_bytes(hash_data) def upload(self, host: Remote, phases: list[str]) -> None: if "partitioning" in phases: diff --git a/pkgs/clan-cli/default.nix b/pkgs/clan-cli/default.nix index 1e04822c7..be9dd935a 100644 --- a/pkgs/clan-cli/default.nix +++ b/pkgs/clan-cli/default.nix @@ -2,6 +2,7 @@ # callPackage args gnupg, installShellFiles, + pass, jq, lib, nix, @@ -58,6 +59,7 @@ let testDependencies = testRuntimeDependencies ++ [ gnupg + pass stdenv.cc # Compiler used for certain native extensions (pythonRuntime.withPackages pyTestDeps) ]; @@ -213,6 +215,7 @@ pythonRuntime.pkgs.buildPythonApplication { pkgs.shellcheck-minimal pkgs.mkpasswd pkgs.xkcdpass + pkgs.pass nix-select ]; };