Skip to content

Remote: Automatically manage known hosts keys #451

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
jan-janssen merged 6 commits into from
Apr 11, 2026
Merged

Remote: Automatically manage known hosts keys #451

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
0cf2c65
Remote: Automatically manage known hosts keys
jan-janssen Aug 13, 2025
41a42a0
work with strings
jan-janssen Aug 13, 2025
8868a61
add more tests
jan-janssen Aug 13, 2025
6dd12d0
fixes
jan-janssen Aug 13, 2025
28e97e9
Merge commit '3ab449af65a54a03abb944e51ca4e64d6ab7b272' into known_hosts
jan-janssen Apr 11, 2026
ccf1586
move test config
jan-janssen Apr 11, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 10 additions & 4 deletions src/pysqa/base/remote.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,9 +93,12 @@ def __init__(
)
self._ssh_host = config["ssh_host"]
self._ssh_username = config["ssh_username"]
self._ssh_known_hosts = os.path.abspath(
os.path.expanduser(config["known_hosts"])
)
if "known_hosts" in config:
self._ssh_known_hosts = os.path.abspath(
os.path.expanduser(config["known_hosts"])
Comment on lines +96 to +98
Copy link

Copilot AI Apr 11, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

known_hosts is treated as present solely based on the key existing in config. If a user sets known_hosts: "" (or it gets parsed as an empty string), abspath(expanduser("")) becomes the current working directory and load_host_keys() will later try to read a directory path. Consider using known_hosts = config.get("known_hosts") and only setting _ssh_known_hosts when it is a non-empty path (otherwise treat it as unset).

Suggested change
if "known_hosts" in config:
self._ssh_known_hosts = os.path.abspath(
os.path.expanduser(config["known_hosts"])
known_hosts = config.get("known_hosts")
if known_hosts:
self._ssh_known_hosts = os.path.abspath(
os.path.expanduser(known_hosts)

Copilot uses AI. Check for mistakes.
)
else:
self._ssh_known_hosts = ""
self._ssh_ask_for_password: Union[bool, str] = False
self._ssh_key = (
os.path.abspath(os.path.expanduser(config["ssh_key"]))
Expand Down Expand Up @@ -346,7 +349,10 @@ def _open_ssh_connection(self) -> paramiko.SSHClient:
paramiko.SSHClient: The SSH connection object.
"""
ssh = paramiko.SSHClient()
ssh.load_host_keys(self._ssh_known_hosts)
if len(self._ssh_known_hosts) > 0:
ssh.load_host_keys(self._ssh_known_hosts)
else:
ssh.set_missing_host_key_policy(paramiko.AutoAddPolicy())
Comment on lines +352 to +355
Copy link

Copilot AI Apr 11, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When known_hosts is unset this enables AutoAddPolicy(), which silently trusts the first presented host key (MITM risk). Consider at least emitting a warning (and/or using WarningPolicy) so users know host key verification is disabled. Also, when known_hosts is set, load_host_keys() will raise if the file doesn’t exist; it would be more robust to os.path.exists()-check and fall back to the missing-host-key policy with a warning rather than crashing.

Suggested change
if len(self._ssh_known_hosts) > 0:
ssh.load_host_keys(self._ssh_known_hosts)
else:
ssh.set_missing_host_key_policy(paramiko.AutoAddPolicy())
if len(self._ssh_known_hosts) > 0 and os.path.exists(self._ssh_known_hosts):
ssh.load_host_keys(self._ssh_known_hosts)
else:
if len(self._ssh_known_hosts) > 0:
warnings.warn(
"Configured SSH known_hosts file does not exist: "
+ self._ssh_known_hosts
+ ". Host key verification will use WarningPolicy instead.",
UserWarning,
)
else:
warnings.warn(
"SSH known_hosts is not configured. Host key verification is "
"disabled for unknown hosts and WarningPolicy will be used.",
UserWarning,
)
ssh.set_missing_host_key_policy(paramiko.WarningPolicy())

Copilot uses AI. Check for mistakes.
if (
self._ssh_key is not None
and self._ssh_key_passphrase is not None
Expand Down
1 change: 0 additions & 1 deletion tests/static/remote_rebex/queue.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@ queue_type: REMOTE
queue_primary: remote
ssh_host: test.rebex.net
ssh_username: demo
known_hosts: ~/.ssh/known_hosts
ssh_password: password
ssh_remote_config_dir: /
ssh_remote_path: /
Expand Down
14 changes: 14 additions & 0 deletions tests/static/remote_rebex_hosts/queue.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
queue_type: REMOTE
queue_primary: remote
ssh_host: test.rebex.net
ssh_username: demo
known_hosts: ~/.ssh/known_hosts
Copy link

Copilot AI Apr 11, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pointing known_hosts at ~/.ssh/known_hosts makes tests/configs environment-dependent (often missing on CI, and contents vary per machine). For stable tests, prefer a repository-local known_hosts fixture file (checked into tests/static/...) or omit known_hosts in this test config and mock host-key verification when needed.

Suggested change
known_hosts: ~/.ssh/known_hosts

Copilot uses AI. Check for mistakes.
ssh_password: password
ssh_remote_config_dir: /
ssh_remote_path: /
ssh_local_path: /home/localuser/projects/
ssh_continous_connection: False
ssh_port: 22
python_executable: python3
queues:
remote: {cores_max: 100, cores_min: 10, run_time_max: 259200}
27 changes: 22 additions & 5 deletions tests/unit/base/test_remote.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -151,36 +151,53 @@ def test_remote_command_continous_connection(self):
output = remote._adapter._execute_remote_command(command="pwd")
self.assertEqual(output, "/\n")

def test_remote_command_individual_connections_hosts(self):
path = os.path.dirname(os.path.abspath(__file__))
remote = QueueAdapter(directory=os.path.join(path, "../../static/remote_rebex_hosts"))
remote._adapter._ssh_remote_path = path
remote._adapter._open_ssh_connection()
output = remote._adapter._execute_remote_command(command="pwd")
Comment on lines +155 to +159
Copy link

Copilot AI Apr 11, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These new "_hosts" integration tests depend on the runner’s ~/.ssh/known_hosts containing the correct key for test.rebex.net. That makes CI/local runs brittle and environment-dependent. Prefer using a repo-provided known_hosts fixture (or a temp file created during the test), or mock Paramiko’s host-key handling so the test doesn’t rely on a developer/CI home directory.

Copilot uses AI. Check for mistakes.
self.assertEqual(output, "/\n")

Comment on lines +154 to +161
Copy link
Copy Markdown

@coderabbitai coderabbitai bot Aug 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Avoid opening a redundant SSH connection in the individual-connections host test.

When _ssh_continous_connection is False, _execute_remote_command opens and closes its own connection. Calling _open_ssh_connection beforehand (Line 158) creates an unused connection and may leak resources.

Apply this diff:

 def test_remote_command_individual_connections_hosts(self):
     path = os.path.dirname(os.path.abspath(__file__))
     remote = QueueAdapter(directory=os.path.join(path, "config/remote_rebex_hosts"))
     remote._adapter._ssh_remote_path = path
-    remote._adapter._open_ssh_connection()
     output = remote._adapter._execute_remote_command(command="pwd")
     self.assertEqual(output, "/\n")

Optionally, make the same change in test_remote_command_individual_connections above (Lines 141–143) for consistency.

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
def test_remote_command_individual_connections_hosts(self):
path = os.path.dirname(os.path.abspath(__file__))
remote = QueueAdapter(directory=os.path.join(path, "config/remote_rebex_hosts"))
remote._adapter._ssh_remote_path = path
remote._adapter._open_ssh_connection()
output = remote._adapter._execute_remote_command(command="pwd")
self.assertEqual(output, "/\n")
def test_remote_command_individual_connections_hosts(self):
path = os.path.dirname(os.path.abspath(__file__))
remote = QueueAdapter(directory=os.path.join(path, "config/remote_rebex_hosts"))
remote._adapter._ssh_remote_path = path
output = remote._adapter._execute_remote_command(command="pwd")
self.assertEqual(output, "/\n")
🤖 Prompt for AI Agents 
In tests/test_remote.py around lines 154 to 161, the test calls
remote._adapter._open_ssh_connection() before executing the command which is
redundant when _ssh_continous_connection is False and causes a leaked unused
connection; remove the explicit call to _open_ssh_connection() (line 158) so
_execute_remote_command manages its own connect/disconnect, and also apply the
same removal to the similar test at lines ~141–143 for consistency.

def test_remote_command_continous_connection_hosts(self):
path = os.path.dirname(os.path.abspath(__file__))
remote = QueueAdapter(directory=os.path.join(path, "../../static/remote_rebex_hosts"))
remote._adapter._ssh_remote_path = path
remote._adapter._ssh_continous_connection = True
remote._adapter._open_ssh_connection()
output = remote._adapter._execute_remote_command(command="pwd")
self.assertEqual(output, "/\n")

def test_submit_job(self):
path = os.path.dirname(os.path.abspath(__file__))
remote = QueueAdapter(directory=os.path.join(path, "../../static/remote_rebex"))
remote = QueueAdapter(directory=os.path.join(path, "../../static/remote_rebex_hosts"))
remote._adapter._ssh_remote_path = path
output = remote._adapter.submit_job(working_directory=os.path.join(path, "../../static/empty"), command="echo 1")
Comment on lines 171 to 175
Copy link

Copilot AI Apr 11, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This (and the other Rebex tests below) now use remote_rebex_hosts, which requires ~/.ssh/known_hosts to exist and include the Rebex host key. That will likely fail on clean CI images and reduces coverage of the new behavior where known_hosts is omitted. Consider switching these back to remote_rebex (no known_hosts) and keeping host-key verification behavior covered via a deterministic fixture or a mocked Paramiko client.

Copilot uses AI. Check for mistakes.
self.assertEqual(output, 1)

def test_transferfile_individual_connections(self):
path = os.path.dirname(os.path.abspath(__file__))
remote = QueueAdapter(directory=os.path.join(path, "../../static/remote_rebex"))
remote = QueueAdapter(directory=os.path.join(path, "../../static/remote_rebex_hosts"))
remote._adapter._ssh_remote_path = path
self.assertIsNone(remote._adapter.transfer_file(file="readme.txt", transfer_back=True))
Comment on lines 178 to 182
Copy link

Copilot AI Apr 11, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using remote_rebex_hosts here makes the test dependent on an external ~/.ssh/known_hosts file (which may not exist in CI). Either use the config without known_hosts for these integration tests, or supply a known_hosts fixture under tests/static and reference it from the YAML.

Copilot uses AI. Check for mistakes.

def test_transferfile_continous_connection(self):
path = os.path.dirname(os.path.abspath(__file__))
remote = QueueAdapter(directory=os.path.join(path, "../../static/remote_rebex"))
remote = QueueAdapter(directory=os.path.join(path, "../../static/remote_rebex_hosts"))
remote._adapter._ssh_remote_path = path
remote._adapter._ssh_continous_connection = True
self.assertIsNone(remote._adapter.transfer_file(file="readme.txt", transfer_back=True))

def test_get_transport(self):
path = os.path.dirname(os.path.abspath(__file__))
remote = QueueAdapter(directory=os.path.join(path, "../../static/remote_rebex"))
remote = QueueAdapter(directory=os.path.join(path, "../../static/remote_rebex_hosts"))
self.assertIsNotNone(get_transport(remote._adapter._open_ssh_connection()))
Comment on lines 191 to 194
Copy link

Copilot AI Apr 11, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test will be flaky in environments where ~/.ssh/known_hosts is absent or doesn’t contain test.rebex.net. For stability, avoid relying on the user/CI home directory: use a repo-local known_hosts fixture or mock Paramiko’s host key checks/policies.

Copilot uses AI. Check for mistakes.
with self.assertRaises(ValueError):
get_transport(ssh=FakeSSH())

def test_get_job_from_remote(self):
path = os.path.dirname(os.path.abspath(__file__))
remote = QueueAdapter(directory=os.path.join(path, "../../static/remote_rebex"))
remote = QueueAdapter(directory=os.path.join(path, "../../static/remote_rebex_hosts"))
remote._adapter._ssh_remote_path = path
remote._adapter._ssh_local_path = path
remote._adapter._ssh_delete_file_on_remote = True
Comment on lines 199 to 203
Copy link

Copilot AI Apr 11, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same issue as above: remote_rebex_hosts makes this unit test suite depend on ~/.ssh/known_hosts contents. Consider using remote_rebex (no known_hosts) for functional coverage and cover the known_hosts path via a deterministic fixture/mock so tests remain hermetic.

Copilot uses AI. Check for mistakes.
Expand Down
6 changes: 3 additions & 3 deletions tests/unit/base/test_remote_auth.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,21 +19,21 @@
class TestRemoteQueueAdapterAuth(unittest.TestCase):
def test_password_auth(self):
path = os.path.dirname(os.path.abspath(__file__))
remote = QueueAdapter(directory=os.path.join(path, "../../static/remote_rebex"))
remote = QueueAdapter(directory=os.path.join(path, "../../static/remote_rebex_hosts"))
remote._adapter._ssh_ask_for_password = False
remote._adapter._ssh_key = None
self.assertIsNotNone(remote._adapter._open_ssh_connection())
Comment on lines 20 to 25
Copy link

Copilot AI Apr 11, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All auth tests now use remote_rebex_hosts, which makes the suite depend on ~/.ssh/known_hosts existing and containing the Rebex host key. That’s not guaranteed in CI and can cause unrelated failures. Prefer using the config without known_hosts, or point known_hosts at a repo-local fixture file for deterministic behavior.

Copilot uses AI. Check for mistakes.

def test_key_auth(self):
path = os.path.dirname(os.path.abspath(__file__))
remote = QueueAdapter(directory=os.path.join(path, "../../static/remote_rebex"))
remote = QueueAdapter(directory=os.path.join(path, "../../static/remote_rebex_hosts"))
remote._adapter._ssh_password = None
with self.assertRaises(ValueError):
remote._adapter._open_ssh_connection()

def test_two_factor_auth(self):
path = os.path.dirname(os.path.abspath(__file__))
remote = QueueAdapter(directory=os.path.join(path, "../../static/remote_rebex"))
remote = QueueAdapter(directory=os.path.join(path, "../../static/remote_rebex_hosts"))
remote._adapter._ssh_two_factor_authentication = True
with self.assertRaises(paramiko.ssh_exception.AuthenticationException):
remote._adapter._open_ssh_connection()
Loading