From f1a7f2aa69ac29f124247e0db24987a6e78bf546 Mon Sep 17 00:00:00 2001 From: Qubasa Date: Fri, 25 Apr 2025 13:38:03 +0200 Subject: [PATCH 1/2] clan-cli: Expose private_key to Machine class, in the future we should merge Machine and Host class --- pkgs/clan-cli/clan_cli/machines/machines.py | 3 +++ pkgs/clan-cli/clan_cli/ssh/host.py | 13 ++++++------- pkgs/clan-cli/clan_cli/ssh/parse.py | 3 +++ pkgs/clan-cli/clan_cli/tests/hosts.py | 3 ++- pkgs/clan-cli/clan_cli/tests/test_secrets_upload.py | 2 +- 5 files changed, 15 insertions(+), 9 deletions(-) diff --git a/pkgs/clan-cli/clan_cli/machines/machines.py b/pkgs/clan-cli/clan_cli/machines/machines.py index 942102f4c..76366e923 100644 --- a/pkgs/clan-cli/clan_cli/machines/machines.py +++ b/pkgs/clan-cli/clan_cli/machines/machines.py @@ -32,6 +32,7 @@ class Machine: cached_deployment: None | dict[str, Any] = None override_target_host: None | str = None override_build_host: None | str = None + private_key: Path | None = None host_key_check: HostKeyCheck = HostKeyCheck.STRICT def get_id(self) -> str: @@ -150,6 +151,7 @@ class Machine: self.name, self.target_host_address, self.host_key_check, + private_key=self.private_key, meta={"machine": self}, ) @@ -168,6 +170,7 @@ class Machine: build_host, self.host_key_check, forward_agent=True, + private_key=self.private_key, meta={"machine": self, "target_host": self.target_host}, ) diff --git a/pkgs/clan-cli/clan_cli/ssh/host.py b/pkgs/clan-cli/clan_cli/ssh/host.py index 56a4fca44..0cfc6af7e 100644 --- a/pkgs/clan-cli/clan_cli/ssh/host.py +++ b/pkgs/clan-cli/clan_cli/ssh/host.py @@ -6,6 +6,7 @@ import shlex import socket import subprocess from dataclasses import dataclass, field +from pathlib import Path from shlex import quote from typing import Any @@ -27,7 +28,7 @@ class Host: host: str user: str | None = None port: int | None = None - key: str | None = None + private_key: Path | None = None forward_agent: bool = False command_prefix: str | None = None host_key_check: HostKeyCheck = HostKeyCheck.ASK @@ -54,7 +55,7 @@ class Host: host=host.host, user=host.user, port=host.port, - key=host.key, + private_key=host.private_key, forward_agent=host.forward_agent, command_prefix=host.command_prefix, host_key_check=host.host_key_check, @@ -176,6 +177,9 @@ class Host: ssh_opts.extend(self.host_key_check.to_ssh_opt()) + if self.private_key: + ssh_opts.extend(["-i", str(self.private_key)]) + return ssh_opts def ssh_cmd( @@ -201,11 +205,6 @@ class Host: if tty: ssh_opts.extend(["-t"]) - if self.port: - ssh_opts.extend(["-p", str(self.port)]) - if self.key: - ssh_opts.extend(["-i", self.key]) - if tor_socks: packages.append("netcat") ssh_opts.append("-o") diff --git a/pkgs/clan-cli/clan_cli/ssh/parse.py b/pkgs/clan-cli/clan_cli/ssh/parse.py index ee3e0aee9..02d1bd9c0 100644 --- a/pkgs/clan-cli/clan_cli/ssh/parse.py +++ b/pkgs/clan-cli/clan_cli/ssh/parse.py @@ -1,5 +1,6 @@ import re import urllib.parse +from pathlib import Path from typing import Any from clan_cli.errors import ClanError @@ -13,6 +14,7 @@ def parse_deployment_address( host_key_check: HostKeyCheck, forward_agent: bool = True, meta: dict[str, Any] | None = None, + private_key: Path | None = None, ) -> Host: parts = host.split("?", maxsplit=1) endpoint, maybe_options = parts if len(parts) == 2 else (parts[0], "") @@ -58,6 +60,7 @@ def parse_deployment_address( hostname, user=user, port=port, + private_key=private_key, host_key_check=host_key_check, command_prefix=machine_name, forward_agent=forward_agent, diff --git a/pkgs/clan-cli/clan_cli/tests/hosts.py b/pkgs/clan-cli/clan_cli/tests/hosts.py index 7d08b9d59..9cfeb4fce 100644 --- a/pkgs/clan-cli/clan_cli/tests/hosts.py +++ b/pkgs/clan-cli/clan_cli/tests/hosts.py @@ -1,5 +1,6 @@ import os import pwd +from pathlib import Path import pytest from clan_cli.ssh.host import Host @@ -15,7 +16,7 @@ def hosts(sshd: Sshd) -> list[Host]: "127.0.0.1", port=sshd.port, user=login, - key=sshd.key, + private_key=Path(sshd.key), host_key_check=HostKeyCheck.NONE, ) ] diff --git a/pkgs/clan-cli/clan_cli/tests/test_secrets_upload.py b/pkgs/clan-cli/clan_cli/tests/test_secrets_upload.py index 677a34e08..6dd220525 100644 --- a/pkgs/clan-cli/clan_cli/tests/test_secrets_upload.py +++ b/pkgs/clan-cli/clan_cli/tests/test_secrets_upload.py @@ -40,7 +40,7 @@ def test_secrets_upload( config = flake.machines["vm1"] config["nixpkgs"]["hostPlatform"] = "x86_64-linux" host = hosts[0] - addr = f"{host.user}@{host.host}:{host.port}?StrictHostKeyChecking=no&UserKnownHostsFile=/dev/null&IdentityFile={host.key}" + addr = f"{host.user}@{host.host}:{host.port}?StrictHostKeyChecking=no&UserKnownHostsFile=/dev/null&IdentityFile={host.private_key}" config["clan"]["networking"]["targetHost"] = addr config["clan"]["core"]["facts"]["secretUploadDirectory"] = str(flake.path / "facts") flake.refresh() From 3214d27f0bd7b0473b4cbb134cd83a3ad011c197 Mon Sep 17 00:00:00 2001 From: Qubasa Date: Fri, 25 Apr 2025 14:00:40 +0200 Subject: [PATCH 2/2] clan-cli: Improve remote destination depth validation with detailed error messaging --- pkgs/clan-cli/clan_cli/ssh/upload.py | 35 ++++++++++++++++++++++++---- 1 file changed, 31 insertions(+), 4 deletions(-) diff --git a/pkgs/clan-cli/clan_cli/ssh/upload.py b/pkgs/clan-cli/clan_cli/ssh/upload.py index 3cdba60b4..955b4882c 100644 --- a/pkgs/clan-cli/clan_cli/ssh/upload.py +++ b/pkgs/clan-cli/clan_cli/ssh/upload.py @@ -18,10 +18,37 @@ def upload( dir_mode: int = 0o700, file_mode: int = 0o400, ) -> None: - # Check if the remote destination is at least 3 directories deep - if len(remote_dest.parts) < 3: - msg = f"The remote destination must be at least 3 directories deep. Got: {remote_dest}. Reason: The directory will be deleted with 'rm -rf'." - raise ClanError(msg) + # Check the depth of the remote destination path to prevent accidental deletion + # of important directories like /home/user when uploading a directory, + # as the process involves `rm -rf` on the destination. + if local_src.is_dir(): + # Calculate the depth (number of components after the root '/') + # / -> depth 0 + # /a -> depth 1 + # /a/b -> depth 2 + # /a/b/c -> depth 3 + depth = len(remote_dest.parts) - 1 + + # General rule: destination must be at least 3 levels deep for safety. + is_too_shallow = depth < 3 + + # Exceptions: Allow depth 2 if the path starts with /tmp/, /root/, or /etc/. + # This allows destinations like /tmp/mydir or /etc/conf.d, but not /tmp or /etc directly. + is_allowed_exception = depth >= 2 and ( + str(remote_dest).startswith("/tmp/") + or str(remote_dest).startswith("/root/") + or str(remote_dest).startswith("/etc/") + ) + + # Raise error if the path is too shallow and not an allowed exception. + if is_too_shallow and not is_allowed_exception: + msg = ( + f"When uploading a directory, the remote destination '{remote_dest}' is considered unsafe " + f"(depth {depth}). It must be at least 3 levels deep (e.g., /path/to/dir), " + f"or at least 2 levels deep starting with /tmp/, /root/, or /etc/ (e.g., /tmp/mydir). " + f"Reason: The existing destination '{remote_dest}' will be recursively deleted ('rm -rf') before upload." + ) + raise ClanError(msg) # Create the tarball from the temporary directory with TemporaryDirectory(prefix="facts-upload-") as tardir: