From 7d721c7a9127dda595f4e4440c321b8d0f29243c Mon Sep 17 00:00:00 2001 From: Dave Page Date: Tue, 9 Jun 2026 11:59:02 +0100 Subject: [PATCH 1/5] Disable SSH agent when tunnel identity file/password is given. #9814 SSHTunnelForwarder was called without allow_agent, which defaults to True in sshtunnel/paramiko, so the SSH agent was always probed even when the user supplied an identity file or password - causing repeated agent authentication attempts/denials. Pass allow_agent=False on both the identity-file and password code paths. --- web/pgadmin/utils/driver/psycopg3/server_manager.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/web/pgadmin/utils/driver/psycopg3/server_manager.py b/web/pgadmin/utils/driver/psycopg3/server_manager.py index 00738355ee4..b3dccfaab43 100644 --- a/web/pgadmin/utils/driver/psycopg3/server_manager.py +++ b/web/pgadmin/utils/driver/psycopg3/server_manager.py @@ -593,6 +593,7 @@ def create_ssh_tunnel(self, tunnel_password): ssh_username=self.tunnel_username, ssh_pkey=get_complete_file_path(self.tunnel_identity_file), ssh_private_key_password=tunnel_password, + allow_agent=False, remote_bind_address=(self.host, self.port), logger=ssh_logger, set_keepalive=int(self.tunnel_keep_alive) @@ -602,6 +603,7 @@ def create_ssh_tunnel(self, tunnel_password): (self.tunnel_host, int(self.tunnel_port)), ssh_username=self.tunnel_username, ssh_password=tunnel_password, + allow_agent=False, remote_bind_address=(self.host, self.port), logger=ssh_logger, set_keepalive=int(self.tunnel_keep_alive) From 6061705e36af241784d8e78d54f795ad8a95fb17 Mon Sep 17 00:00:00 2001 From: Dave Page Date: Mon, 17 Aug 2026 13:05:15 +0100 Subject: [PATCH 2/5] Only disable the SSH agent when a credential is actually available Passing allow_agent=False unconditionally regressed the case the option was meant to leave alone: tunnel_password is None whenever the user picked password authentication and left the field empty, which the UI supports for prompting on connection, and with no password and no key sshtunnel raises ValueError from _consolidate_auth() before it connects. ValueError is not a BaseSSHTunnelForwarderError, so it escaped the handler and propagated instead of returning the (False, message) tuple callers expect, which is worse than the behaviour before the change. allow_agent is now conditional on there actually being a credential to offer, so the agent is still bypassed in the case #9814 reported whilst an empty password or an unusable identity file falls back to the previous behaviour. ValueError is caught alongside BaseSSHTunnelForwarderError as well, so any future pre-flight validation failure in sshtunnel is still reported cleanly. Tests cover all four credential combinations and the ValueError path. --- .../utils/driver/psycopg3/server_manager.py | 21 +++- .../tests/test_ssh_tunnel_allow_agent.py | 118 ++++++++++++++++++ 2 files changed, 135 insertions(+), 4 deletions(-) create mode 100644 web/pgadmin/utils/tests/test_ssh_tunnel_allow_agent.py diff --git a/web/pgadmin/utils/driver/psycopg3/server_manager.py b/web/pgadmin/utils/driver/psycopg3/server_manager.py index b3dccfaab43..79d53622605 100644 --- a/web/pgadmin/utils/driver/psycopg3/server_manager.py +++ b/web/pgadmin/utils/driver/psycopg3/server_manager.py @@ -587,13 +587,22 @@ def create_ssh_tunnel(self, tunnel_password): ssh_logger.setLevel(logging.DEBUG) for h in current_app.logger.handlers: ssh_logger.addHandler(h) + # Only keep sshtunnel away from the agent when we have a + # credential of our own for it to use (#9814). Given neither a + # usable key nor a password it raises ValueError from + # _consolidate_auth() before it ever connects, so leaving the + # agent enabled in that case preserves the previous behaviour + # for anyone relying on "Prompt for Password?" or on the agent + # itself to authenticate. if self.tunnel_authentication == 1: + tunnel_identity_file = get_complete_file_path( + self.tunnel_identity_file) self.tunnel_object = SSHTunnelForwarder( (self.tunnel_host, int(self.tunnel_port)), ssh_username=self.tunnel_username, - ssh_pkey=get_complete_file_path(self.tunnel_identity_file), + ssh_pkey=tunnel_identity_file, ssh_private_key_password=tunnel_password, - allow_agent=False, + allow_agent=not tunnel_identity_file, remote_bind_address=(self.host, self.port), logger=ssh_logger, set_keepalive=int(self.tunnel_keep_alive) @@ -603,7 +612,7 @@ def create_ssh_tunnel(self, tunnel_password): (self.tunnel_host, int(self.tunnel_port)), ssh_username=self.tunnel_username, ssh_password=tunnel_password, - allow_agent=False, + allow_agent=not tunnel_password, remote_bind_address=(self.host, self.port), logger=ssh_logger, set_keepalive=int(self.tunnel_keep_alive) @@ -612,7 +621,11 @@ def create_ssh_tunnel(self, tunnel_password): self.tunnel_object.daemon_forward_servers = True self.tunnel_object.start() self.tunnel_created = True - except BaseSSHTunnelForwarderError as e: + except (BaseSSHTunnelForwarderError, ValueError) as e: + # sshtunnel raises a bare ValueError rather than one of its own + # exceptions when it finds no credential to authenticate with, so + # catch that too and report it the same way instead of letting it + # escape as an unhandled exception. current_app.logger.exception(e) return False, gettext( "Failed to create the SSH tunnel. Possible causes:\n" diff --git a/web/pgadmin/utils/tests/test_ssh_tunnel_allow_agent.py b/web/pgadmin/utils/tests/test_ssh_tunnel_allow_agent.py new file mode 100644 index 00000000000..8a892ae06f9 --- /dev/null +++ b/web/pgadmin/utils/tests/test_ssh_tunnel_allow_agent.py @@ -0,0 +1,118 @@ +########################################################################## +# +# pgAdmin 4 - PostgreSQL Tools +# +# Copyright (C) 2013 - 2026, The pgAdmin Development Team +# This software is released under the PostgreSQL Licence +# +########################################################################## + +"""Tests for the SSH agent handling in ServerManager.create_ssh_tunnel(). + +Probing the agent when pgAdmin already has an identity file or a password of +its own produces repeated prompts or denials (#9814), so the agent is disabled +whenever a credential is available. It must stay enabled when none is, because +sshtunnel raises ValueError from _consolidate_auth() if it is left with +nothing at all to authenticate with, and that would be a worse failure than +the one being fixed: ValueError is not a BaseSSHTunnelForwarderError, so it +would escape the handler that turns tunnel failures into a friendly message. +""" + +from unittest.mock import MagicMock, patch + +import config +from pgadmin.utils.route import BaseTestGenerator + +import pgadmin.utils.driver.psycopg3.server_manager as server_manager + + +class SSHTunnelAllowAgentTestCase(BaseTestGenerator): + """allow_agent must follow whether a credential is actually present.""" + + scenarios = [ + ('Identity file present disables the agent', dict( + tunnel_authentication=1, + resolved_identity_file='/tmp/id_rsa', + stored_password=None, + expected_allow_agent=False, + )), + ('Unusable identity file leaves the agent enabled', dict( + tunnel_authentication=1, + resolved_identity_file=None, + stored_password=None, + expected_allow_agent=True, + )), + ('Tunnel password disables the agent', dict( + tunnel_authentication=0, + resolved_identity_file=None, + stored_password='encrypted', + expected_allow_agent=False, + )), + ('No credential at all leaves the agent enabled', dict( + tunnel_authentication=0, + resolved_identity_file=None, + stored_password=None, + expected_allow_agent=True, + )), + ('A missing credential is reported, not raised', dict( + tunnel_authentication=0, + resolved_identity_file=None, + stored_password=None, + expected_allow_agent=True, + forwarder_error=ValueError( + 'No password or public key available!'), + )), + ] + + # Overridden per scenario where the forwarder is meant to fail. + forwarder_error = None + + def setUp(self): + # Deliberately no server connection: this exercises the argument + # marshalling in create_ssh_tunnel(), not a real tunnel. + if not config.SUPPORT_SSH_TUNNEL: + self.skipTest('SSH tunnelling is disabled in this configuration.') + + def _make_manager(self): + manager = server_manager.ServerManager.__new__( + server_manager.ServerManager) + manager.tunnel_authentication = self.tunnel_authentication + manager.tunnel_host = 'tunnel.example.com' + manager.tunnel_port = 22 + manager.tunnel_username = 'tunneluser' + manager.tunnel_identity_file = 'id_rsa' + manager.tunnel_keep_alive = 0 + manager.host = 'db.example.com' + manager.port = 5432 + manager.tunnel_object = None + manager.tunnel_created = False + return manager + + def runTest(self): + manager = self._make_manager() + forwarder = MagicMock(side_effect=self.forwarder_error) \ + if self.forwarder_error else MagicMock() + + # A request context, not just an app context: the failure path calls + # gettext(), and pgAdmin's locale selector reads the request. + with self.app.test_request_context(), \ + patch.object(server_manager, 'SSHTunnelForwarder', forwarder), \ + patch.object(server_manager, 'User', MagicMock()), \ + patch.object(server_manager, 'current_user', MagicMock()), \ + patch.object(server_manager, 'get_complete_file_path', + return_value=self.resolved_identity_file), \ + patch.object(server_manager, 'get_crypt_key', + return_value=(True, 'crypt-key')), \ + patch.object(server_manager, 'decrypt', + return_value=b'tunnelpassword'): + success, error = manager.create_ssh_tunnel(self.stored_password) + + forwarder.assert_called_once() + self.assertEqual(forwarder.call_args.kwargs['allow_agent'], + self.expected_allow_agent) + + if self.forwarder_error: + self.assertFalse(success) + self.assertIn('Failed to create the SSH tunnel', error) + else: + self.assertTrue(success, msg=error) From 71464bbb51999be7e876d2d7a9e1ca3e8dfd3c51 Mon Sep 17 00:00:00 2001 From: Dave Page Date: Wed, 23 Sep 2026 16:38:33 +0100 Subject: [PATCH 3/5] Test that an empty SSH tunnel password leaves the agent enabled create_ssh_tunnel() skips decryption for an empty string separately from None, so cover that path too, as suggested in review. --- web/pgadmin/utils/tests/test_ssh_tunnel_allow_agent.py | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/web/pgadmin/utils/tests/test_ssh_tunnel_allow_agent.py b/web/pgadmin/utils/tests/test_ssh_tunnel_allow_agent.py index 8a892ae06f9..80eb3683c0a 100644 --- a/web/pgadmin/utils/tests/test_ssh_tunnel_allow_agent.py +++ b/web/pgadmin/utils/tests/test_ssh_tunnel_allow_agent.py @@ -54,6 +54,12 @@ class SSHTunnelAllowAgentTestCase(BaseTestGenerator): stored_password=None, expected_allow_agent=True, )), + ('An empty tunnel password leaves the agent enabled', dict( + tunnel_authentication=0, + resolved_identity_file=None, + stored_password='', + expected_allow_agent=True, + )), ('A missing credential is reported, not raised', dict( tunnel_authentication=0, resolved_identity_file=None, From 93645c3536ddf1fa94788e8b84029f19a69f7cf1 Mon Sep 17 00:00:00 2001 From: Dave Page Date: Wed, 23 Sep 2026 17:08:34 +0100 Subject: [PATCH 4/5] Assert the SSH tunnel credential reaches sshtunnel, not just the flag Check ssh_pkey on the identity-file path and ssh_password on the password path as well as allow_agent, as suggested in review. --- .../utils/tests/test_ssh_tunnel_allow_agent.py | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-) diff --git a/web/pgadmin/utils/tests/test_ssh_tunnel_allow_agent.py b/web/pgadmin/utils/tests/test_ssh_tunnel_allow_agent.py index 80eb3683c0a..ddf12c4ce93 100644 --- a/web/pgadmin/utils/tests/test_ssh_tunnel_allow_agent.py +++ b/web/pgadmin/utils/tests/test_ssh_tunnel_allow_agent.py @@ -114,8 +114,18 @@ def runTest(self): success, error = manager.create_ssh_tunnel(self.stored_password) forwarder.assert_called_once() - self.assertEqual(forwarder.call_args.kwargs['allow_agent'], - self.expected_allow_agent) + kwargs = forwarder.call_args.kwargs + self.assertEqual(kwargs['allow_agent'], self.expected_allow_agent) + + # The credential itself must reach sshtunnel too, not just the flag. + if self.tunnel_authentication == 1: + self.assertEqual(kwargs['ssh_pkey'], self.resolved_identity_file) + else: + # A stored password is decrypted; an absent or empty one is + # passed through untouched. + expected_password = 'tunnelpassword' \ + if self.stored_password else self.stored_password + self.assertEqual(kwargs['ssh_password'], expected_password) if self.forwarder_error: self.assertFalse(success) From 08e8681b815e0c0dc383c295aeffe695cdcee98b Mon Sep 17 00:00:00 2001 From: Dave Page Date: Thu, 24 Sep 2026 13:29:29 +0100 Subject: [PATCH 5/5] Keep the SSH agent disabled when the configured identity file is missing get_complete_file_path() returns None for a key file that no longer exists, which re-enabled the agent and let sshtunnel authenticate with some other key instead of reporting the missing file. Base allow_agent on the configured path instead, as suggested in review. --- .../utils/driver/psycopg3/server_manager.py | 17 +++++++++-------- .../tests/test_ssh_tunnel_allow_agent.py | 19 +++++++++++++++---- 2 files changed, 24 insertions(+), 12 deletions(-) diff --git a/web/pgadmin/utils/driver/psycopg3/server_manager.py b/web/pgadmin/utils/driver/psycopg3/server_manager.py index 79d53622605..ea1b2995daf 100644 --- a/web/pgadmin/utils/driver/psycopg3/server_manager.py +++ b/web/pgadmin/utils/driver/psycopg3/server_manager.py @@ -587,13 +587,14 @@ def create_ssh_tunnel(self, tunnel_password): ssh_logger.setLevel(logging.DEBUG) for h in current_app.logger.handlers: ssh_logger.addHandler(h) - # Only keep sshtunnel away from the agent when we have a - # credential of our own for it to use (#9814). Given neither a - # usable key nor a password it raises ValueError from - # _consolidate_auth() before it ever connects, so leaving the - # agent enabled in that case preserves the previous behaviour - # for anyone relying on "Prompt for Password?" or on the agent - # itself to authenticate. + # Keep sshtunnel away from the agent whenever the user has + # configured a credential of their own (#9814). The identity file + # check uses the configured path, not the resolved one, so a + # missing key file is reported as a failure rather than silently + # authenticating with some other key from the agent. With no + # identity file or password configured the agent stays enabled, + # preserving the previous behaviour for anyone relying on + # "Prompt for Password?" or on the agent itself. if self.tunnel_authentication == 1: tunnel_identity_file = get_complete_file_path( self.tunnel_identity_file) @@ -602,7 +603,7 @@ def create_ssh_tunnel(self, tunnel_password): ssh_username=self.tunnel_username, ssh_pkey=tunnel_identity_file, ssh_private_key_password=tunnel_password, - allow_agent=not tunnel_identity_file, + allow_agent=not self.tunnel_identity_file, remote_bind_address=(self.host, self.port), logger=ssh_logger, set_keepalive=int(self.tunnel_keep_alive) diff --git a/web/pgadmin/utils/tests/test_ssh_tunnel_allow_agent.py b/web/pgadmin/utils/tests/test_ssh_tunnel_allow_agent.py index ddf12c4ce93..69210076fc0 100644 --- a/web/pgadmin/utils/tests/test_ssh_tunnel_allow_agent.py +++ b/web/pgadmin/utils/tests/test_ssh_tunnel_allow_agent.py @@ -11,7 +11,9 @@ Probing the agent when pgAdmin already has an identity file or a password of its own produces repeated prompts or denials (#9814), so the agent is disabled -whenever a credential is available. It must stay enabled when none is, because +whenever one is configured, even if the identity file turns out to be missing, +since falling back to the agent would authenticate with some other key. It +must stay enabled when no credential is configured at all, because sshtunnel raises ValueError from _consolidate_auth() if it is left with nothing at all to authenticate with, and that would be a worse failure than the one being fixed: ValueError is not a BaseSSHTunnelForwarderError, so it @@ -36,10 +38,17 @@ class SSHTunnelAllowAgentTestCase(BaseTestGenerator): stored_password=None, expected_allow_agent=False, )), - ('Unusable identity file leaves the agent enabled', dict( + ('Missing identity file still disables the agent', dict( tunnel_authentication=1, resolved_identity_file=None, stored_password=None, + expected_allow_agent=False, + )), + ('No identity file configured leaves the agent enabled', dict( + tunnel_authentication=1, + configured_identity_file=None, + resolved_identity_file=None, + stored_password=None, expected_allow_agent=True, )), ('Tunnel password disables the agent', dict( @@ -70,8 +79,10 @@ class SSHTunnelAllowAgentTestCase(BaseTestGenerator): )), ] - # Overridden per scenario where the forwarder is meant to fail. + # Overridden per scenario where the forwarder is meant to fail, or where + # no identity file is configured. forwarder_error = None + configured_identity_file = 'id_rsa' def setUp(self): # Deliberately no server connection: this exercises the argument @@ -86,7 +97,7 @@ def _make_manager(self): manager.tunnel_host = 'tunnel.example.com' manager.tunnel_port = 22 manager.tunnel_username = 'tunneluser' - manager.tunnel_identity_file = 'id_rsa' + manager.tunnel_identity_file = self.configured_identity_file manager.tunnel_keep_alive = 0 manager.host = 'db.example.com' manager.port = 5432