From 50c7fdc45c2de132013887fbdb7b906a8ec746fd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ricardo=20Ara=C3=BAjo=20Santos?= Date: Tue, 6 Sep 2016 10:47:46 -0300 Subject: [PATCH] Fixes parameters validation in SSH power manager. Parameters validation was not in conformity with the docs, and it was not possible to pass a SSH key with a passphrase. This patch fixes the validation allowing the user to pass both parameters, and adds tests for this validation. Change-Id: I1ba1eb8393c7921f4cab154b69b856418af32173 Closes-Bug: #1607527 --- ironic/drivers/modules/ssh.py | 36 ++++++++++------ ironic/tests/unit/db/utils.py | 8 +++- ironic/tests/unit/drivers/modules/test_ssh.py | 42 ++++++++++++++++++- .../notes/bug-1607527-75885e145db62d69.yaml | 4 ++ 4 files changed, 75 insertions(+), 15 deletions(-) create mode 100644 releasenotes/notes/bug-1607527-75885e145db62d69.yaml diff --git a/ironic/drivers/modules/ssh.py b/ironic/drivers/modules/ssh.py index fbf8803d0b..72fa7893a9 100644 --- a/ironic/drivers/modules/ssh.py +++ b/ironic/drivers/modules/ssh.py @@ -58,14 +58,18 @@ REQUIRED_PROPERTIES = { "vmware, parallels, xenserver. Required.") } OTHER_PROPERTIES = { - 'ssh_key_contents': _("private key(s). One of this, ssh_key_filename, " - "or ssh_password must be specified."), + 'ssh_key_contents': _("private key(s). If ssh_password is also specified " + "it will be used for unlocking the private key. Do " + "not specify ssh_key_filename when this property is " + "specified."), 'ssh_key_filename': _("(list of) filename(s) of optional private key(s) " - "for authentication. One of this, ssh_key_contents, " - "or ssh_password must be specified."), + "for authentication. If ssh_password is also " + "specified it will be used for unlocking the " + "private key. Do not specify ssh_key_contents when " + "this property is specified."), 'ssh_password': _("password to use for authentication or for unlocking a " - "private key. One of this, ssh_key_contents, or " - "ssh_key_filename must be specified."), + "private key. At least one of this, ssh_key_contents, " + "or ssh_key_filename must be specified."), 'ssh_port': _("port on the node to connect to; default is 22. Optional."), 'vbox_use_headless': _("True or False (Default). Optional. " "In the case of VirtualBox 3.2 and above, allows " @@ -392,17 +396,25 @@ def _parse_driver_info(node): cmd_set = _get_command_sets(virt_type, use_headless) res['cmd_set'] = cmd_set - # Only one credential may be set (avoids complexity around having - # precedence etc). - if len([v for v in (password, key_filename, key_contents) if v]) != 1: + # Set at least one credential method. + if len([v for v in (password, key_filename, key_contents) if v]) == 0: raise exception.InvalidParameterValue(_( - "SSHPowerDriver requires one and only one of ssh_password, " + "SSHPowerDriver requires at least one of ssh_password, " "ssh_key_contents and ssh_key_filename to be set.")) + + # Set only credential file or content but not both + if key_filename and key_contents: + raise exception.InvalidParameterValue(_( + "SSHPowerDriver requires one and only one of " + "ssh_key_contents and ssh_key_filename to be set.")) + if password: res['password'] = password - elif key_contents: + + if key_contents: res['key_contents'] = key_contents - else: + + if key_filename: if not os.path.isfile(key_filename): raise exception.InvalidParameterValue(_( "SSH key file %s not found.") % key_filename) diff --git a/ironic/tests/unit/db/utils.py b/ironic/tests/unit/db/utils.py index 8c9c62f855..fc626a87ae 100644 --- a/ironic/tests/unit/db/utils.py +++ b/ironic/tests/unit/db/utils.py @@ -53,9 +53,15 @@ def get_test_ssh_info(auth_type='password', virt_type='virsh'): result['ssh_key_filename'] = '/not/real/file' elif 'key' == auth_type: result['ssh_key_contents'] = '--BEGIN PRIVATE ...blah' - elif 'too_many' == auth_type: + elif 'file_with_passphrase' == auth_type: result['ssh_password'] = 'fake' result['ssh_key_filename'] = '/not/real/file' + elif 'key_with_passphrase' == auth_type: + result['ssh_password'] = 'fake' + result['ssh_key_contents'] = '--BEGIN PRIVATE ...blah' + elif 'too_many' == auth_type: + result['ssh_key_contents'] = '--BEGIN PRIVATE ...blah' + result['ssh_key_filename'] = '/not/real/file' else: # No auth details (is invalid) pass diff --git a/ironic/tests/unit/drivers/modules/test_ssh.py b/ironic/tests/unit/drivers/modules/test_ssh.py index f4e302db72..008c0a8b07 100644 --- a/ironic/tests/unit/drivers/modules/test_ssh.py +++ b/ironic/tests/unit/drivers/modules/test_ssh.py @@ -96,6 +96,45 @@ class SSHValidateParametersTestCase(db_base.DbTestCase): self.assertEqual('1be26c0b-03f2-4d2e-ae87-c02d7f33c123', info['uuid']) + def test__parse_driver_info_good_file_with_passphrase(self): + # make sure we get back the expected things + d_info = db_utils.get_test_ssh_info('file_with_passphrase') + tempdir = tempfile.mkdtemp() + key_path = tempdir + '/foo' + open(key_path, 'wt').close() + d_info['ssh_key_filename'] = key_path + node = obj_utils.get_test_node( + self.context, + driver='fake_ssh', + driver_info=d_info) + info = ssh._parse_driver_info(node) + self.assertEqual('1.2.3.4', info['host']) + self.assertEqual('admin', info['username']) + self.assertEqual('fake', info['password']) + self.assertEqual(key_path, info['key_filename']) + self.assertEqual(22, info['port']) + self.assertEqual('virsh', info['virt_type']) + self.assertIsNotNone(info['cmd_set']) + self.assertEqual('1be26c0b-03f2-4d2e-ae87-c02d7f33c123', + info['uuid']) + + def test__parse_driver_info_good_key_with_passphrase(self): + # make sure we get back the expected things + node = obj_utils.get_test_node( + self.context, + driver='fake_ssh', + driver_info=db_utils.get_test_ssh_info('key_with_passphrase')) + info = ssh._parse_driver_info(node) + self.assertEqual('1.2.3.4', info['host']) + self.assertEqual('admin', info['username']) + self.assertEqual('fake', info['password']) + self.assertEqual('--BEGIN PRIVATE ...blah', info['key_contents']) + self.assertEqual(22, info['port']) + self.assertEqual('virsh', info['virt_type']) + self.assertIsNotNone(info['cmd_set']) + self.assertEqual('1be26c0b-03f2-4d2e-ae87-c02d7f33c123', + info['uuid']) + def test__parse_driver_info_bad_file(self): # A filename that doesn't exist errors. info = db_utils.get_test_ssh_info('file') @@ -107,11 +146,10 @@ class SSHValidateParametersTestCase(db_base.DbTestCase): exception.InvalidParameterValue, ssh._parse_driver_info, node) def test__parse_driver_info_too_many(self): - info = db_utils.get_test_ssh_info('too_many') node = obj_utils.get_test_node( self.context, driver='fake_ssh', - driver_info=info) + driver_info=db_utils.get_test_ssh_info('too_many')) self.assertRaises( exception.InvalidParameterValue, ssh._parse_driver_info, node) diff --git a/releasenotes/notes/bug-1607527-75885e145db62d69.yaml b/releasenotes/notes/bug-1607527-75885e145db62d69.yaml new file mode 100644 index 0000000000..ba52d09f36 --- /dev/null +++ b/releasenotes/notes/bug-1607527-75885e145db62d69.yaml @@ -0,0 +1,4 @@ +--- +fixes: + - Fixes bug where the user tries to configure the ssh power driver using a + private key protected by a passphrase.