From 03d22c4d06386c40babb365752efe5fc323775b3 Mon Sep 17 00:00:00 2001 From: Alex Blank Date: Fri, 29 May 2026 14:04:52 +0200 Subject: [PATCH] fix: set SSH key ownership to container user with mode 600 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Mount SSH keys as bind (not ro) so docker exec --user root can chown - Add apply_ssh_permissions() to permission_fixer.py - Call apply_ssh_permissions() after container start for all instance types - Derive container user from home_dir (/root → root, /home/user → user) - Tests: apply_ssh_permissions unit tests + start_instance integration tests Quality gates: pytest 236 passed (6 pre-existing failures), tsc --noEmit clean --- apps/api/src/api/tool_instances.py | 32 ++- apps/api/src/services/permission_fixer.py | 63 +++++ apps/api/tests/unit/test_permission_fixer.py | 37 +++ .../tests/unit/test_tool_instances_legacy.py | 245 ++++++++++++++++++ 4 files changed, 374 insertions(+), 3 deletions(-) diff --git a/apps/api/src/api/tool_instances.py b/apps/api/src/api/tool_instances.py index 73888bf..e718354 100644 --- a/apps/api/src/api/tool_instances.py +++ b/apps/api/src/api/tool_instances.py @@ -75,7 +75,7 @@ from src.services.manifest_compiler import ( merge_with_config, resolve_base, ) -from src.services.permission_fixer import apply_mount_permissions +from src.services.permission_fixer import apply_mount_permissions, apply_ssh_permissions from src.services.readiness_probe import execute_probe from src.services.ssh_keys import cleanup_ssh_key_files, prepare_ssh_key_files @@ -1415,7 +1415,7 @@ async def start_instance( { "source": ssh_dir, "target": ssh_target, - "type": "ro", + "type": "bind", } ) logger.debug( @@ -1493,7 +1493,7 @@ async def start_instance( { "source": ssh_dir, "target": "/root/.ssh", - "type": "ro", + "type": "bind", } ) logger.debug( @@ -1651,6 +1651,32 @@ async def start_instance( result["error"], ) + # Fix SSH key ownership/permissions inside the container + if instance.ssh_key_ids and instance.container_id: + container_user = ( + "root" + if home_dir == "/root" + else home_dir[6:] if home_dir.startswith("/home/") else "root" + ) + ssh_target = os.path.join(home_dir, ".ssh") + logger.debug( + "Applying SSH permissions for user %s on %s in instance %s", + container_user, + ssh_target, + instance.id, + ) + ssh_perm_result = apply_ssh_permissions( + instance.container_id, + ssh_target, + container_user, + ) + if not ssh_perm_result["success"]: + logger.warning( + "SSH permission fix failed for instance %s: %s", + instance.id, + ssh_perm_result["error"], + ) + # Execute readiness probe if configured tool_type = await session.get(ToolType, instance.tool_type_id) if tool_type and instance.container_id: diff --git a/apps/api/src/services/permission_fixer.py b/apps/api/src/services/permission_fixer.py index b0c8603..3161f1b 100644 --- a/apps/api/src/services/permission_fixer.py +++ b/apps/api/src/services/permission_fixer.py @@ -104,6 +104,69 @@ def apply_mount_permissions( return results +def apply_ssh_permissions( + container_id: str, + ssh_target: str, + container_user: str, + timeout: int = 10, +) -> dict[str, Any]: + """Fix SSH directory ownership and permissions in a running container. + + Runs chown and chmod on the ~/.ssh directory so the container user + can use the keys (SSH requires the private key to be owned by the + user with mode 600). + + Args: + container_id: Docker container ID or name. + ssh_target: Absolute path to the .ssh directory inside the container. + container_user: The container user that should own the keys. + timeout: Max seconds per docker exec command. + + Returns: + Result dict with keys: success, error. + """ + result: dict[str, Any] = {"success": True, "error": None} + try: + # Ensure directory is owned by the container user + _run_in_container( + container_id, + ["chown", "-R", f"{container_user}:{container_user}", ssh_target], + timeout, + ) + # Set directory permissions + _run_in_container( + container_id, + ["chmod", "700", ssh_target], + timeout, + ) + # Set private key permissions + _run_in_container( + container_id, + [ + "sh", + "-c", + f"find {ssh_target} -name 'id_*' -type f -exec chmod 600 {{}} +", + ], + timeout, + ) + logger.debug( + "Applied SSH permissions for user %s on %s in container %s", + container_user, + ssh_target, + container_id, + ) + except PermissionFixError as exc: + result["success"] = False + result["error"] = str(exc) + logger.warning( + "SSH permission fix failed for container %s (target=%s): %s", + container_id, + ssh_target, + exc, + ) + return result + + class PermissionFixError(Exception): """Raised when a permission fix command fails.""" diff --git a/apps/api/tests/unit/test_permission_fixer.py b/apps/api/tests/unit/test_permission_fixer.py index df868d2..e863eb1 100644 --- a/apps/api/tests/unit/test_permission_fixer.py +++ b/apps/api/tests/unit/test_permission_fixer.py @@ -7,6 +7,7 @@ import pytest from src.services.permission_fixer import ( PermissionFixError, apply_mount_permissions, + apply_ssh_permissions, check_root_user_available, _run_in_container, ) @@ -132,6 +133,42 @@ class TestRunInContainer: _run_in_container("abc123", ["chown", "x"], 10) +class TestApplySshPermissions: + """Tests for apply_ssh_permissions.""" + + @patch("src.services.permission_fixer._run_in_container") + def test_applies_chown_chmod_and_file_mode(self, mock_run) -> None: + result = apply_ssh_permissions("abc123", "/home/user/.ssh", "user") + + assert result["success"] is True + assert mock_run.call_count == 3 + chown_call = mock_run.call_args_list[0] + chmod_call = mock_run.call_args_list[1] + file_mode_call = mock_run.call_args_list[2] + + assert chown_call[0][1] == ["chown", "-R", "user:user", "/home/user/.ssh"] + assert chmod_call[0][1] == ["chmod", "700", "/home/user/.ssh"] + assert file_mode_call[0][1][0] == "sh" + assert "find /home/user/.ssh -name 'id_*' -type f -exec chmod 600" in file_mode_call[0][1][2] + + @patch("src.services.permission_fixer._run_in_container") + def test_uses_root_user(self, mock_run) -> None: + result = apply_ssh_permissions("abc123", "/root/.ssh", "root") + + assert result["success"] is True + chown_call = mock_run.call_args_list[0] + assert chown_call[0][1] == ["chown", "-R", "root:root", "/root/.ssh"] + + @patch("src.services.permission_fixer._run_in_container") + def test_reports_failure(self, mock_run) -> None: + mock_run.side_effect = PermissionFixError("chown failed") + + result = apply_ssh_permissions("abc123", "/home/user/.ssh", "user") + + assert result["success"] is False + assert "chown failed" in result["error"] + + class TestCheckRootUserAvailable: """Tests for check_root_user_available.""" diff --git a/apps/api/tests/unit/test_tool_instances_legacy.py b/apps/api/tests/unit/test_tool_instances_legacy.py index 2e22e0e..03f2655 100644 --- a/apps/api/tests/unit/test_tool_instances_legacy.py +++ b/apps/api/tests/unit/test_tool_instances_legacy.py @@ -701,6 +701,251 @@ class TestStartInstanceLegacyFallback: mock_execute_compose.assert_called_once() +class TestStartInstanceSshPermissions: + """SSH key mounts trigger permission fixes after container starts.""" + + @patch("src.api.tool_instances.write_compose_file") + @patch("src.api.tool_instances.apply_ssh_permissions") + @patch("src.api.tool_instances.wait_for_container_running") + @patch("src.api.tool_instances.execute_compose_command") + @patch("src.api.tool_instances.get_container_id") + @patch("src.api.tool_instances.get_container_name") + @patch("src.api.tool_instances.connect_container_to_network") + @patch("src.api.tool_instances._sanitize_compose_file") + @patch("src.api.tool_instances._get_user") + @patch("src.api.tool_instances._get_owned_project") + async def test_manifest_instance_applies_ssh_permissions( + self, + mock_get_project, + mock_get_user, + mock_sanitize, + mock_connect_network, + mock_get_container_name, + mock_get_container_id, + mock_execute_compose, + mock_wait_container, + mock_apply_ssh, + mock_write_compose, + mock_session, + fake_user_id, + fake_project_id, + fake_repo_id, + fake_instance_id, + fake_tool_type_id, + ) -> None: + """Manifest instance with SSH keys calls apply_ssh_permissions.""" + from src.models.tool_definition_manifest import ToolDefinitionManifest + + manifest_id = uuid.uuid4() + ssh_key_id = str(uuid.uuid4()) + + mock_get_user.return_value = AsyncMock() + mock_get_project.return_value = AsyncMock() + mock_execute_compose.return_value = (0, "started", "") + mock_get_container_id.return_value = "abc123" + mock_get_container_name.return_value = "test-container" + mock_connect_network.return_value = True + mock_wait_container.return_value = { + "success": True, + "status": "running", + "waited_seconds": 0.5, + } + mock_apply_ssh.return_value = {"success": True, "error": None} + + instance = ToolInstance( + id=fake_instance_id, + name="manifest-instance", + repository_id=fake_repo_id, + tool_type_id=fake_tool_type_id, + compose_path="/data/instances/manifest-instance/docker-compose.yml", + status="stopped", + clone_mode="mount", + ssh_key_ids=[ssh_key_id], + created_at=datetime.now(), + updated_at=datetime.now(), + ) + tool_type = ToolType( + id=fake_tool_type_id, + name="manifest-tool", + display_name="Manifest Tool", + default_port=8080, + definition_type="manifest", + manifest_id=manifest_id, + dockerfile_template=None, + compose_template=None, + ) + repo = GitRepository( + id=fake_repo_id, + project_id=fake_project_id, + name="test-repo", + path="/data/repos/test-repo", + remote_url=None, + ssh_key_id=None, + ) + manifest_def = ToolDefinitionManifest( + id=manifest_id, + name="test-manifest", + display_name="Test Manifest", + interface_type="web", + manifest={"user": {"name": "user", "uid": 1001, "gid": 1001}}, + ) + ssh_key = SSHKey( + id=uuid.UUID(ssh_key_id), + user_id=fake_user_id, + name="test-key", + public_key="ssh-ed25519 AAA test@test", + private_key_encrypted="enc", + ) + + async def _get(model, pk): + if model is ToolInstance and pk == fake_instance_id: + return instance + if model is ToolType and pk == fake_tool_type_id: + return tool_type + if model is GitRepository and pk == fake_repo_id: + return repo + if model is User and pk == fake_user_id: + return User(id=fake_user_id, email="test@example.com") + if model is ToolDefinitionManifest and pk == manifest_id: + return manifest_def + if model is SSHKey and pk == uuid.UUID(ssh_key_id): + return ssh_key + return None + + mock_session.get.side_effect = _get + + with patch("os.path.exists", return_value=True): + with patch( + "src.api.tool_instances._prepare_manifest_instance" + ) as mock_prepare: + mock_prepare.return_value = ( + "headquarter/test:latest", + "services:\n app:\n image: test", + {"name": "test-manifest", "user": {"name": "user"}}, + "/home/user", + ) + result = await start_instance( + project_id=fake_project_id, + repo_id=fake_repo_id, + instance_id=fake_instance_id, + data=None, + user_id=fake_user_id, + session=mock_session, + ) + + assert result["status"] == "running" + mock_apply_ssh.assert_called_once_with("abc123", "/home/user/.ssh", "user") + + @patch("src.api.tool_instances.apply_ssh_permissions") + @patch("src.api.tool_instances.wait_for_container_running") + @patch("src.api.tool_instances.execute_compose_command") + @patch("src.api.tool_instances.get_container_id") + @patch("src.api.tool_instances.get_container_name") + @patch("src.api.tool_instances.connect_container_to_network") + @patch("src.api.tool_instances._sanitize_compose_file") + @patch("src.api.tool_instances._get_user") + @patch("src.api.tool_instances._get_owned_project") + async def test_legacy_instance_applies_ssh_permissions( + self, + mock_get_project, + mock_get_user, + mock_sanitize, + mock_connect_network, + mock_get_container_name, + mock_get_container_id, + mock_execute_compose, + mock_wait_container, + mock_apply_ssh, + mock_session, + fake_user_id, + fake_project_id, + fake_repo_id, + fake_instance_id, + fake_tool_type_id, + ) -> None: + """Legacy instance with SSH keys calls apply_ssh_permissions.""" + ssh_key_id = str(uuid.uuid4()) + + mock_get_user.return_value = AsyncMock() + mock_get_project.return_value = AsyncMock() + mock_execute_compose.return_value = (0, "started", "") + mock_get_container_id.return_value = "abc123" + mock_get_container_name.return_value = "test-container" + mock_connect_network.return_value = True + mock_wait_container.return_value = { + "success": True, + "status": "running", + "waited_seconds": 0.5, + } + mock_apply_ssh.return_value = {"success": True, "error": None} + + instance = ToolInstance( + id=fake_instance_id, + name="legacy-instance", + repository_id=fake_repo_id, + tool_type_id=fake_tool_type_id, + compose_path="/data/instances/legacy-instance/docker-compose.yml", + status="stopped", + clone_mode="mount", + ssh_key_ids=[ssh_key_id], + created_at=datetime.now(), + updated_at=datetime.now(), + ) + tool_type = ToolType( + id=fake_tool_type_id, + name="legacy-tool", + display_name="Legacy Tool", + default_port=8080, + definition_type="legacy", + manifest_id=None, + dockerfile_template=None, + compose_template="services:\n app:\n image: nginx", + ) + repo = GitRepository( + id=fake_repo_id, + project_id=fake_project_id, + name="test-repo", + path="/data/repos/test-repo", + remote_url=None, + ssh_key_id=None, + ) + ssh_key = SSHKey( + id=uuid.UUID(ssh_key_id), + user_id=fake_user_id, + name="test-key", + public_key="ssh-ed25519 AAA test@test", + private_key_encrypted="enc", + ) + + async def _get(model, pk): + if model is ToolInstance and pk == fake_instance_id: + return instance + if model is ToolType and pk == fake_tool_type_id: + return tool_type + if model is GitRepository and pk == fake_repo_id: + return repo + if model is User and pk == fake_user_id: + return User(id=fake_user_id, email="test@example.com") + if model is SSHKey and pk == uuid.UUID(ssh_key_id): + return ssh_key + return None + + mock_session.get.side_effect = _get + + with patch("os.path.exists", return_value=True): + result = await start_instance( + project_id=fake_project_id, + repo_id=fake_repo_id, + instance_id=fake_instance_id, + data=None, + user_id=fake_user_id, + session=mock_session, + ) + + assert result["status"] == "running" + mock_apply_ssh.assert_called_once_with("abc123", "/root/.ssh", "root") + + class TestStartInstanceManifestBranch: """Manifest branch is taken ONLY when definition_type == 'manifest'."""