Merge pull request StackStorm#2755 from lakshmi-kannan/STORM-2178/cle…

…ar_error_message_for_passphrase_protected_keys Clear error message when passphrase not supplied with encrypted private key
ddevalco · Jun 17, 2016 · 9966144 · 9966144
2 parents c220652 + b762b3a
commit 9966144
Show file tree

Hide file tree

Showing 5 changed files with 92 additions and 1 deletion.
diff --git a/CHANGELOG.rst b/CHANGELOG.rst
@@ -66,6 +66,8 @@ in development
 * API and CLI allow rules to be filtered by their enable state. (improvement)
 * Fix SSH bastion host support by ensuring the bastion parameter is passed to the paramiko ssh
   client. (bug-fix) #2543 [Adam Mielke]
+* Send out a clear error message when SSH private key is passphrase protected but user fails to
+  supply passphrase with private_key when running a remote SSH action. (improvement)
 * Admins will now be able pass ``--show_secrets`` when listing apikeys to get the ``key_hash``
   un-masked on the CLI. (new-feature)
 

diff --git a/st2actions/st2actions/runners/ssh/parallel_ssh.py b/st2actions/st2actions/runners/ssh/parallel_ssh.py
@@ -245,7 +245,6 @@ def _connect(self, host, results, raise_on_any_error=False):
             LOG.exception(error)
             if raise_on_any_error:
                 raise
-            error = ' '.join([self.CONNECT_ERROR, str(ex)])
             error_dict = self._generate_error_result(exc=ex, message=error)
             self._bad_hosts[hostname] = error_dict
             results[hostname] = error_dict

diff --git a/st2actions/st2actions/runners/ssh/paramiko_ssh.py b/st2actions/st2actions/runners/ssh/paramiko_ssh.py
@@ -554,6 +554,12 @@ def _connect(self, host, socket=None):
         if self.key_files:
             conninfo['key_filename'] = self.key_files
 
+            passphrase_reqd = self._is_key_file_needs_passphrase(self.key_files)
+            if passphrase_reqd and not self.passphrase:
+                msg = ('Private key file %s is passphrase protected. Supply a passphrase.' %
+                       self.key_files)
+                raise paramiko.ssh_exception.PasswordRequiredException(msg)
+
             if self.passphrase:
                 # Optional passphrase for unlocking the private key
                 conninfo['password'] = self.passphrase
@@ -579,6 +585,18 @@ def _connect(self, host, socket=None):
 
         return client
 
+    @staticmethod
+    def _is_key_file_needs_passphrase(file):
+        for cls in [paramiko.RSAKey, paramiko.DSSKey, paramiko.ECDSAKey]:
+            try:
+                cls.from_private_key_file(file, password=None)
+            except paramiko.ssh_exception.PasswordRequiredException:
+                return True
+            except paramiko.ssh_exception.SSHException:
+                continue
+
+        return False
+
     def __repr__(self):
         return ('<ParamikoSSHClient hostname=%s,port=%s,username=%s,id=%s>' %
                 (self.hostname, self.port, self.username, id(self)))
diff --git a/st2actions/tests/unit/test_parallel_ssh.py b/st2actions/tests/unit/test_parallel_ssh.py
@@ -29,6 +29,8 @@
 class ParallelSSHTests(unittest2.TestCase):
 
     @patch('paramiko.SSHClient', Mock)
+    @patch.object(ParamikoSSHClient, '_is_key_file_needs_passphrase',
+                  MagicMock(return_value=False))
     def test_connect_with_password(self):
         hosts = ['localhost', '127.0.0.1']
         client = ParallelSSHClient(hosts=hosts,
@@ -49,6 +51,8 @@ def test_connect_with_password(self):
             client._hosts_client[host].client.connect.assert_called_once_with(**expected_conn)
 
     @patch('paramiko.SSHClient', Mock)
+    @patch.object(ParamikoSSHClient, '_is_key_file_needs_passphrase',
+                  MagicMock(return_value=False))
     def test_connect_with_random_ports(self):
         hosts = ['localhost:22', '127.0.0.1:55', 'st2build001']
         client = ParallelSSHClient(hosts=hosts,
@@ -71,6 +75,8 @@ def test_connect_with_random_ports(self):
             client._hosts_client[hostname].client.connect.assert_called_once_with(**expected_conn)
 
     @patch('paramiko.SSHClient', Mock)
+    @patch.object(ParamikoSSHClient, '_is_key_file_needs_passphrase',
+                  MagicMock(return_value=False))
     def test_connect_with_key(self):
         hosts = ['localhost', '127.0.0.1', 'st2build001']
         client = ParallelSSHClient(hosts=hosts,
@@ -93,6 +99,8 @@ def test_connect_with_key(self):
             client._hosts_client[hostname].client.connect.assert_called_once_with(**expected_conn)
 
     @patch('paramiko.SSHClient', Mock)
+    @patch.object(ParamikoSSHClient, '_is_key_file_needs_passphrase',
+                  MagicMock(return_value=False))
     def test_connect_with_bastion(self):
         hosts = ['localhost', '127.0.0.1']
         client = ParallelSSHClient(hosts=hosts,
@@ -108,6 +116,8 @@ def test_connect_with_bastion(self):
 
     @patch('paramiko.SSHClient', Mock)
     @patch.object(ParamikoSSHClient, 'run', MagicMock(return_value=('/home/ubuntu', '', 0)))
+    @patch.object(ParamikoSSHClient, '_is_key_file_needs_passphrase',
+                  MagicMock(return_value=False))
     def test_run_command(self):
         hosts = ['localhost', '127.0.0.1', 'st2build001']
         client = ParallelSSHClient(hosts=hosts,
@@ -123,6 +133,8 @@ def test_run_command(self):
             client._hosts_client[hostname].run.assert_called_with('pwd', **expected_kwargs)
 
     @patch('paramiko.SSHClient', Mock)
+    @patch.object(ParamikoSSHClient, '_is_key_file_needs_passphrase',
+                  MagicMock(return_value=False))
     def test_run_command_timeout(self):
         # Make sure stdout and stderr is included on timeout
         hosts = ['localhost', '127.0.0.1', 'st2build001']
@@ -149,6 +161,8 @@ def test_run_command_timeout(self):
     @patch('paramiko.SSHClient', Mock)
     @patch.object(ParamikoSSHClient, 'put', MagicMock(return_value={}))
     @patch.object(os.path, 'exists', MagicMock(return_value=True))
+    @patch.object(ParamikoSSHClient, '_is_key_file_needs_passphrase',
+                  MagicMock(return_value=False))
     def test_put(self):
         hosts = ['localhost', '127.0.0.1', 'st2build001']
         client = ParallelSSHClient(hosts=hosts,
@@ -167,6 +181,8 @@ def test_put(self):
 
     @patch('paramiko.SSHClient', Mock)
     @patch.object(ParamikoSSHClient, 'delete_file', MagicMock(return_value={}))
+    @patch.object(ParamikoSSHClient, '_is_key_file_needs_passphrase',
+                  MagicMock(return_value=False))
     def test_delete_file(self):
         hosts = ['localhost', '127.0.0.1', 'st2build001']
         client = ParallelSSHClient(hosts=hosts,
@@ -180,6 +196,8 @@ def test_delete_file(self):
 
     @patch('paramiko.SSHClient', Mock)
     @patch.object(ParamikoSSHClient, 'delete_dir', MagicMock(return_value={}))
+    @patch.object(ParamikoSSHClient, '_is_key_file_needs_passphrase',
+                  MagicMock(return_value=False))
     def test_delete_dir(self):
         hosts = ['localhost', '127.0.0.1', 'st2build001']
         client = ParallelSSHClient(hosts=hosts,
@@ -197,6 +215,8 @@ def test_delete_dir(self):
                                                                          **expected_kwargs)
 
     @patch('paramiko.SSHClient', Mock)
+    @patch.object(ParamikoSSHClient, '_is_key_file_needs_passphrase',
+                  MagicMock(return_value=False))
     def test_host_port_info(self):
         client = ParallelSSHClient(hosts=['dummy'],
                                    user='ubuntu',
@@ -218,6 +238,8 @@ def test_host_port_info(self):
     @patch.object(ParamikoSSHClient, 'run', MagicMock(
         return_value=(json.dumps({'foo': 'bar'}), '', 0))
     )
+    @patch.object(ParamikoSSHClient, '_is_key_file_needs_passphrase',
+                  MagicMock(return_value=False))
     def test_run_command_json_output_transformed_to_object(self):
         hosts = ['127.0.0.1']
         client = ParallelSSHClient(hosts=hosts,

diff --git a/st2actions/tests/unit/test_paramiko_ssh.py b/st2actions/tests/unit/test_paramiko_ssh.py
@@ -44,6 +44,8 @@ def setUp(self):
         self.ssh_cli = ParamikoSSHClient(**conn_params)
 
     @patch('paramiko.SSHClient', Mock)
+    @patch.object(ParamikoSSHClient, '_is_key_file_needs_passphrase',
+                  MagicMock(return_value=False))
     def test_create_with_password(self):
         conn_params = {'hostname': 'dummy.host.org',
                        'username': 'ubuntu',
@@ -61,6 +63,8 @@ def test_create_with_password(self):
         mock.client.connect.assert_called_once_with(**expected_conn)
 
     @patch('paramiko.SSHClient', Mock)
+    @patch.object(ParamikoSSHClient, '_is_key_file_needs_passphrase',
+                  MagicMock(return_value=False))
     def test_deprecated_key_argument(self):
         conn_params = {'hostname': 'dummy.host.org',
                        'username': 'ubuntu',
@@ -89,6 +93,8 @@ def test_key_files_and_key_material_arguments_are_mutual_exclusive(self):
                                 ParamikoSSHClient, **conn_params)
 
     @patch('paramiko.SSHClient', Mock)
+    @patch.object(ParamikoSSHClient, '_is_key_file_needs_passphrase',
+                  MagicMock(return_value=False))
     def test_key_material_argument(self):
         path = os.path.join(get_resources_base_path(),
                             'ssh', 'dummy_rsa')
@@ -113,6 +119,8 @@ def test_key_material_argument(self):
         mock.client.connect.assert_called_once_with(**expected_conn)
 
     @patch('paramiko.SSHClient', Mock)
+    @patch.object(ParamikoSSHClient, '_is_key_file_needs_passphrase',
+                  MagicMock(return_value=False))
     def test_key_material_argument_invalid_key(self):
         conn_params = {'hostname': 'dummy.host.org',
                        'username': 'ubuntu',
@@ -125,6 +133,8 @@ def test_key_material_argument_invalid_key(self):
                                 expected_msg, mock.connect)
 
     @patch('paramiko.SSHClient', Mock)
+    @patch.object(ParamikoSSHClient, '_is_key_file_needs_passphrase',
+                  MagicMock(return_value=True))
     def test_passphrase_no_key_provided(self):
         conn_params = {'hostname': 'dummy.host.org',
                        'username': 'ubuntu',
@@ -135,6 +145,18 @@ def test_passphrase_no_key_provided(self):
                                 **conn_params)
 
     @patch('paramiko.SSHClient', Mock)
+    def test_passphrase_not_provided_for_encrypted_key_file(self):
+        path = os.path.join(get_resources_base_path(),
+                            'ssh', 'dummy_rsa_passphrase')
+        conn_params = {'hostname': 'dummy.host.org',
+                       'username': 'ubuntu',
+                       'key_files': path}
+        mock = ParamikoSSHClient(**conn_params)
+        self.assertRaises(paramiko.ssh_exception.PasswordRequiredException, mock.connect)
+
+    @patch('paramiko.SSHClient', Mock)
+    @patch.object(ParamikoSSHClient, '_is_key_file_needs_passphrase',
+                  MagicMock(return_value=True))
     def test_key_with_passphrase_success(self):
         path = os.path.join(get_resources_base_path(),
                             'ssh', 'dummy_rsa_passphrase')
@@ -179,6 +201,8 @@ def test_key_with_passphrase_success(self):
         mock.client.connect.assert_called_once_with(**expected_conn)
 
     @patch('paramiko.SSHClient', Mock)
+    @patch.object(ParamikoSSHClient, '_is_key_file_needs_passphrase',
+                  MagicMock(return_value=True))
     def test_passphrase_and_no_key(self):
         conn_params = {'hostname': 'dummy.host.org',
                        'username': 'ubuntu',
@@ -189,6 +213,8 @@ def test_passphrase_and_no_key(self):
                                 ParamikoSSHClient, **conn_params)
 
     @patch('paramiko.SSHClient', Mock)
+    @patch.object(ParamikoSSHClient, '_is_key_file_needs_passphrase',
+                  MagicMock(return_value=True))
     def test_incorrect_passphrase(self):
         path = os.path.join(get_resources_base_path(),
                             'ssh', 'dummy_rsa_passphrase')
@@ -207,6 +233,8 @@ def test_incorrect_passphrase(self):
                                 expected_msg, mock.connect)
 
     @patch('paramiko.SSHClient', Mock)
+    @patch.object(ParamikoSSHClient, '_is_key_file_needs_passphrase',
+                  MagicMock(return_value=False))
     def test_key_material_contains_path_not_contents(self):
         conn_params = {'hostname': 'dummy.host.org',
                        'username': 'ubuntu'}
@@ -228,6 +256,8 @@ def test_key_material_contains_path_not_contents(self):
                                     expected_msg, mock.connect)
 
     @patch('paramiko.SSHClient', Mock)
+    @patch.object(ParamikoSSHClient, '_is_key_file_needs_passphrase',
+                  MagicMock(return_value=False))
     def test_create_with_key(self):
         conn_params = {'hostname': 'dummy.host.org',
                        'username': 'ubuntu',
@@ -245,6 +275,8 @@ def test_create_with_key(self):
         mock.client.connect.assert_called_once_with(**expected_conn)
 
     @patch('paramiko.SSHClient', Mock)
+    @patch.object(ParamikoSSHClient, '_is_key_file_needs_passphrase',
+                  MagicMock(return_value=False))
     def test_create_with_key_via_bastion(self):
         conn_params = {'hostname': 'dummy.host.org',
                        'bastion_host': 'bastion.host.org',
@@ -273,6 +305,8 @@ def test_create_with_key_via_bastion(self):
         mock.client.connect.assert_called_once_with(**expected_conn)
 
     @patch('paramiko.SSHClient', Mock)
+    @patch.object(ParamikoSSHClient, '_is_key_file_needs_passphrase',
+                  MagicMock(return_value=False))
     def test_create_with_password_and_key(self):
         conn_params = {'hostname': 'dummy.host.org',
                        'username': 'ubuntu',
@@ -292,6 +326,8 @@ def test_create_with_password_and_key(self):
         mock.client.connect.assert_called_once_with(**expected_conn)
 
     @patch('paramiko.SSHClient', Mock)
+    @patch.object(ParamikoSSHClient, '_is_key_file_needs_passphrase',
+                  MagicMock(return_value=False))
     def test_create_without_credentials(self):
         """
         Initialize object with no credentials.
@@ -313,6 +349,8 @@ def test_create_without_credentials(self):
         mock.client.connect.assert_called_once_with(**expected_conn)
 
     @patch('paramiko.SSHClient', Mock)
+    @patch.object(ParamikoSSHClient, '_is_key_file_needs_passphrase',
+                  MagicMock(return_value=False))
     def test_create_without_credentials_use_default_key(self):
         # No credentials are provided by default stanley ssh key exists so it should use that
         cfg.CONF.set_override(name='ssh_key_file', override='stanley_rsa', group='system_user')
@@ -338,6 +376,8 @@ def test_create_without_credentials_use_default_key(self):
                   MagicMock(return_value=StringIO('')))
     @patch.object(os.path, 'exists', MagicMock(return_value=True))
     @patch.object(os, 'stat', MagicMock(return_value=None))
+    @patch.object(ParamikoSSHClient, '_is_key_file_needs_passphrase',
+                  MagicMock(return_value=False))
     def test_basic_usage_absolute_path(self):
         """
         Basic execution.
@@ -370,6 +410,8 @@ def test_basic_usage_absolute_path(self):
         mock.close()
 
     @patch('paramiko.SSHClient', Mock)
+    @patch.object(ParamikoSSHClient, '_is_key_file_needs_passphrase',
+                  MagicMock(return_value=False))
     def test_delete_script(self):
         """
         Provide a basic test with 'delete' action.
@@ -387,6 +429,8 @@ def test_delete_script(self):
         mock.close()
 
     @patch('paramiko.SSHClient', Mock)
+    @patch.object(ParamikoSSHClient, '_is_key_file_needs_passphrase',
+                  MagicMock(return_value=False))
     @patch.object(ParamikoSSHClient, 'exists', return_value=False)
     def test_put_dir(self, *args):
         mock = self.ssh_cli
@@ -410,6 +454,8 @@ def test_put_dir(self, *args):
         mock_cli.open_sftp().put.assert_has_calls(calls, any_order=True)
 
     @patch('paramiko.SSHClient', Mock)
+    @patch.object(ParamikoSSHClient, '_is_key_file_needs_passphrase',
+                  MagicMock(return_value=False))
     def test_consume_stdout(self):
         # Test utf-8 decoding of ``stdout`` still works fine when reading CHUNK_SIZE splits a
         # multi-byte utf-8 character in the middle. We should wait to collect all bytes
@@ -431,6 +477,8 @@ def test_consume_stdout(self):
         self.assertEqual(u'\U00010348', stdout.getvalue())
 
     @patch('paramiko.SSHClient', Mock)
+    @patch.object(ParamikoSSHClient, '_is_key_file_needs_passphrase',
+                  MagicMock(return_value=False))
     def test_consume_stderr(self):
         # Test utf-8 decoding of ``stderr`` still works fine when reading CHUNK_SIZE splits a
         # multi-byte utf-8 character in the middle. We should wait to collect all bytes
@@ -458,6 +506,8 @@ def test_consume_stderr(self):
                   MagicMock(return_value=StringIO('')))
     @patch.object(os.path, 'exists', MagicMock(return_value=True))
     @patch.object(os, 'stat', MagicMock(return_value=None))
+    @patch.object(ParamikoSSHClient, '_is_key_file_needs_passphrase',
+                  MagicMock(return_value=False))
     def test_sftp_connection_is_only_established_if_required(self):
         # Verify that SFTP connection is lazily established only if and when needed.
         conn_params = {'hostname': 'dummy.host.org',