make local ssh functionality work by copying and chmoding the ssh files#78
make local ssh functionality work by copying and chmoding the ssh files#78
Conversation
There was a problem hiding this comment.
Pull request overview
This PR aims to make APX SSH usage work reliably in local/container environments by staging SSH key/known_hosts files (copy + chmod/chown) into a runtime location, and updating the integration test to mount SSH files under /run/keys without an in-container prep script.
Changes:
- Added SSH runtime staging helpers in
mcp-local/utils/apx.pyand invoked them during SSH mount auto-discovery. - Introduced a default runtime keys directory (
/tmp/apx-ssh) for staged SSH materials. - Updated
mcp-local/tests/test_mcp.pyto mountssh-key.pemandknown_hostsas individual files under/run/keysand removed the container-sidecp/chmodprep step.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
mcp-local/utils/apx.py |
Adds runtime staging of SSH key/known_hosts and wires it into mount auto-discovery. |
mcp-local/tests/test_mcp.py |
Adjusts integration test mounts to /run/keys/* and removes in-container prep commands. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| runtime_keys_dir = runtime_keys_dir or APX_RUNTIME_KEYS_DIR | ||
| runtime_keys_dir.mkdir(parents=True, exist_ok=True) | ||
|
|
||
| staged_key_path = runtime_keys_dir / "ssh-key.pem" | ||
| staged_known_hosts_path = runtime_keys_dir / "known_hosts" | ||
| current_uid = os.getuid() | ||
| current_gid = os.getgid() | ||
|
|
||
| shutil.copyfile(key_path, staged_key_path) | ||
| os.chown(staged_key_path, current_uid, current_gid) | ||
| os.chmod(staged_key_path, 0o600) | ||
|
|
||
| shutil.copyfile(known_hosts_path, staged_known_hosts_path) | ||
| os.chown(staged_known_hosts_path, current_uid, current_gid) | ||
| os.chmod(staged_known_hosts_path, 0o644) |
There was a problem hiding this comment.
Staging SSH material into a fixed, shared path under /tmp with predictable filenames (/tmp/apx-ssh/ssh-key.pem, known_hosts) is vulnerable to symlink/clobber attacks (especially if the process runs as root) and can also race/overwrite when multiple requests run concurrently. Consider creating a per-invocation temp directory with restrictive permissions (e.g., via tempfile.mkdtemp/TemporaryDirectory, chmod 0700) and writing files using non-following/atomic creation to avoid existing symlinks, then cleaning up after use if possible.
| return _stage_apx_ssh_runtime_files( | ||
| key_path=key_path, | ||
| known_hosts_path=known_hosts_path, | ||
| runtime_keys_dir=runtime_keys_dir, | ||
| ) |
There was a problem hiding this comment.
_stage_apx_ssh_runtime_files() can raise (copy/chown/chmod failures), and resolve_apx_ssh_mount_env() does not catch those exceptions. Since apx_recipe_run() calls resolve_apx_ssh_mount_env() outside a try/except, a staging error could crash the tool instead of returning a structured config error. Wrap the staging call in a try/except and convert failures into key_path=None/known_hosts_path=None plus a helpful *_reason so the caller can return mount guidance.
| } | ||
|
|
||
| key_mode = key_stat.st_mode & 0o777 | ||
| if key_stat.st_uid == os.getuid() and key_mode == 0o600: |
There was a problem hiding this comment.
The permission check in prepare_apx_ssh_paths is overly strict: it only treats 0o600 as acceptable. SSH typically accepts keys with no group/other permissions (e.g., 0400 is common) as well. Consider changing the check to allow any mode where (mode & 0o077) == 0 (and owner matches), to avoid unnecessary staging/copying and potential failures on otherwise-valid keys.
| if key_stat.st_uid == os.getuid() and key_mode == 0o600: | |
| if key_stat.st_uid == os.getuid() and (key_mode & 0o077) == 0: |
| if key_path and known_hosts_path: | ||
| prepared_paths = prepare_apx_ssh_paths(key_path, known_hosts_path) | ||
| key_path = prepared_paths["key_path"] | ||
| known_hosts_path = prepared_paths["known_hosts_path"] | ||
| os.environ["SSH_KEY_PATH"] = key_path | ||
| os.environ["KNOWN_HOSTS_PATH"] = known_hosts_path |
There was a problem hiding this comment.
The new staging via prepare_apx_ssh_paths() is only reached when env vars were auto-selected; if SSH_KEY_PATH and KNOWN_HOSTS_PATH are already set, the function returns earlier and this block never runs. That means user-provided env paths can still hit SSH permission/ownership failures. Consider restructuring resolve_apx_ssh_mount_env() so this preparation step runs whenever both paths are present (after existence checks), not only after auto-discovery.
| ssh_key_path = external_ssh_key_path | ||
| known_hosts_path = external_known_hosts_path | ||
| pub_key_path = temp_keys_path / "ssh-key.pem.pub" | ||
|
|
||
| if external_ssh_key_path: | ||
| shutil.copyfile(external_ssh_key_path, pem_path) | ||
| else: | ||
| if not ssh_key_path: | ||
| ssh_key_path = str(temp_keys_path / "ssh-key.pem") | ||
| subprocess.run( | ||
| ["ssh-keygen", "-t", "ed25519", "-N", "", "-f", str(pem_path)], | ||
| ["ssh-keygen", "-t", "ed25519", "-N", "", "-f", ssh_key_path], | ||
| check=True, | ||
| capture_output=True, | ||
| text=True, | ||
| ) | ||
| if pub_key_path.exists(): | ||
| pub_key_path.unlink() | ||
|
|
||
| if external_known_hosts_path: | ||
| shutil.copyfile(external_known_hosts_path, known_hosts_path) | ||
| else: | ||
| known_hosts_path.write_text( | ||
| if not known_hosts_path: | ||
| known_hosts_path = str(temp_keys_path / "known_hosts") | ||
| Path(known_hosts_path).write_text( | ||
| "172.17.0.1 ssh-ed25519 AAAAC3NzaC1lZDI1NTE5AAAAIFakeKnownHostKeyForIntegrationTestsOnly\n", | ||
| encoding="utf-8", | ||
| ) | ||
|
|
||
| os.chmod(pem_path, 0o600) | ||
| os.chmod(ssh_key_path, 0o600) | ||
| os.chmod(known_hosts_path, 0o644) |
There was a problem hiding this comment.
This test unconditionally chmods ssh_key_path/known_hosts_path. When APX_TEST_SSH_KEY_PATH or APX_TEST_KNOWN_HOSTS_PATH is set, these may point to real user files, so the test will mutate host SSH materials (or fail if permissions are restricted). Consider only chmod-ing files that the test created in the temp directory, or copying external files into temp_keys_path before chmod/mounting.
No description provided.