diff --git a/ironic_python_agent/hardware.py b/ironic_python_agent/hardware.py index 0f53a442c..2a4f8eef1 100644 --- a/ironic_python_agent/hardware.py +++ b/ironic_python_agent/hardware.py @@ -898,39 +898,57 @@ class GenericHardwareManager(HardwareManager): if 'supported' not in security_lines: return False - if 'enabled' in security_lines: - # Attempt to unlock the drive in the event it has already been - # locked by a previous failed attempt. - try: - utils.execute('hdparm', '--user-master', 'u', - '--security-unlock', 'NULL', block_device.name) - security_lines = self._get_ata_security_lines(block_device) - except processutils.ProcessExecutionError as e: - raise errors.BlockDeviceEraseError('Security password set ' - 'failed for device ' - '%(name)s: %(err)s' % - {'name': block_device.name, - 'err': e}) - - if 'enabled' in security_lines: - raise errors.BlockDeviceEraseError( - ('Block device {} already has a security password set' - ).format(block_device.name)) + # At this point, we could be SEC1,2,4,5,6 if 'not frozen' not in security_lines: + # In SEC2 or 6 raise errors.BlockDeviceEraseError( ('Block device {} is frozen and cannot be erased' ).format(block_device.name)) - try: - utils.execute('hdparm', '--user-master', 'u', - '--security-set-pass', 'NULL', block_device.name) - except processutils.ProcessExecutionError as e: - raise errors.BlockDeviceEraseError('Security password set ' - 'failed for device ' - '%(name)s: %(err)s' % - {'name': block_device.name, - 'err': e}) + # At this point, we could be in SEC1,4,5 + + # Attempt to unlock the drive in the event it has already been + # locked by a previous failed attempt. We try the empty string as + # versions of hdparm < 9.51, interpreted NULL as the literal string, + # "NULL", as opposed to the empty string. + unlock_passwords = ['NULL', ''] + for password in unlock_passwords: + if 'not locked' in security_lines: + break + try: + utils.execute('hdparm', '--user-master', 'u', + '--security-unlock', password, + block_device.name) + except processutils.ProcessExecutionError as e: + LOG.info('Security unlock failed for device ' + '%(name)s using password "%(password)s": %(err)s', + {'name': block_device.name, + 'password': password, + 'err': e}) + security_lines = self._get_ata_security_lines(block_device) + + # If the unlock failed we will still be in SEC4, otherwise, we will be + # in SEC1 or SEC5 + + if 'not locked' not in security_lines: + # In SEC4 + raise errors.BlockDeviceEraseError( + ('Block device {} already has a security password set' + ).format(block_device.name)) + + # At this point, we could be in SEC1 or 5 + if 'not enabled' in security_lines: + # SEC1. Try to transition to SEC5 by setting empty user + # password. + try: + utils.execute('hdparm', '--user-master', 'u', + '--security-set-pass', 'NULL', block_device.name) + except processutils.ProcessExecutionError as e: + error_msg = ('Security password set failed for device ' + '{name}: {err}' + ).format(name=block_device.name, err=e) + raise errors.BlockDeviceEraseError(error_msg) # Use the 'enhanced' security erase option if it's supported. erase_option = '--security-erase' @@ -949,10 +967,12 @@ class GenericHardwareManager(HardwareManager): # Verify that security is now 'not enabled' security_lines = self._get_ata_security_lines(block_device) if 'not enabled' not in security_lines: + # Not SEC1 - fail raise errors.BlockDeviceEraseError( ('An unknown error occurred erasing block device {}' ).format(block_device.name)) + # In SEC1 security state return True def get_bmc_address(self): diff --git a/ironic_python_agent/tests/unit/test_hardware.py b/ironic_python_agent/tests/unit/test_hardware.py index 95c22fc1d..555487456 100644 --- a/ironic_python_agent/tests/unit/test_hardware.py +++ b/ironic_python_agent/tests/unit/test_hardware.py @@ -113,7 +113,7 @@ HDPARM_INFO_TEMPLATE = ( '\tMaster password revision code = 65534\n' '\t%(supported)s\n' '\t%(enabled)s\n' - '\tnot\tlocked\n' + '\t%(locked)s\n' '\t%(frozen)s\n' '\tnot\texpired: security count\n' '\t%(enhanced_erase)s\n' @@ -1441,15 +1441,49 @@ class TestGenericHardwareManager(base.IronicAgentTest): 'shred', '--force', '--zero', '--verbose', '--iterations', '1', '/dev/sda') + @mock.patch.object(utils, 'execute', autospec=True) + def test_erase_block_device_ata_security_unlock_fallback_pass( + self, mocked_execute): + hdparm_output = create_hdparm_info( + supported=True, enabled=True, locked=True + ) + hdparm_output_unlocked = create_hdparm_info( + supported=True, enabled=True, frozen=False, enhanced_erase=False) + hdparm_output_not_enabled = create_hdparm_info( + supported=True, enabled=False, frozen=False, enhanced_erase=False) + mocked_execute.side_effect = [ + (hdparm_output, ''), + processutils.ProcessExecutionError(), # NULL fails to unlock + (hdparm_output, ''), # recheck security lines + None, # security unlock with "" + (hdparm_output_unlocked, ''), + '', + (hdparm_output_not_enabled, '') + ] + + block_device = hardware.BlockDevice('/dev/sda', 'big', 1073741824, + True) + + self.hardware.erase_block_device(self.node, block_device) + + mocked_execute.assert_any_call('hdparm', '--user-master', 'u', + '--security-unlock', '', '/dev/sda') + @mock.patch.object(hardware.GenericHardwareManager, '_shred_block_device', autospec=True) @mock.patch.object(utils, 'execute', autospec=True) def test_erase_block_device_ata_security_enabled( self, mocked_execute, mock_shred): + # Tests that an exception is thrown if all of the recovery passwords + # fail to unlock the device without throwing exception hdparm_output = create_hdparm_info( - supported=True, enabled=True, frozen=False, enhanced_erase=False) + supported=True, enabled=True, locked=True) mocked_execute.side_effect = [ + (hdparm_output, ''), + None, + (hdparm_output, ''), + None, (hdparm_output, ''), None, (hdparm_output, '') @@ -1457,12 +1491,15 @@ class TestGenericHardwareManager(base.IronicAgentTest): block_device = hardware.BlockDevice('/dev/sda', 'big', 1073741824, True) - self.assertRaises( errors.IncompatibleHardwareMethodError, self.hardware.erase_block_device, self.node, block_device) + mocked_execute.assert_any_call('hdparm', '--user-master', 'u', + '--security-unlock', '', '/dev/sda') + mocked_execute.assert_any_call('hdparm', '--user-master', 'u', + '--security-unlock', 'NULL', '/dev/sda') self.assertFalse(mock_shred.called) @mock.patch.object(hardware.GenericHardwareManager, '_shred_block_device', @@ -1471,7 +1508,7 @@ class TestGenericHardwareManager(base.IronicAgentTest): def test_erase_block_device_ata_security_enabled_unlock_attempt( self, mocked_execute, mock_shred): hdparm_output = create_hdparm_info( - supported=True, enabled=True, frozen=False, enhanced_erase=False) + supported=True, enabled=True, locked=True) hdparm_output_not_enabled = create_hdparm_info( supported=True, enabled=False, frozen=False, enhanced_erase=False) @@ -1493,34 +1530,36 @@ class TestGenericHardwareManager(base.IronicAgentTest): @mock.patch.object(utils, 'execute', autospec=True) def test__ata_erase_security_enabled_unlock_exception( self, mocked_execute): + # test that an exception is thrown when security unlock fails with + # ProcessExecutionError hdparm_output = create_hdparm_info( - supported=True, enabled=True, frozen=False, enhanced_erase=False) - + supported=True, enabled=True, locked=True) mocked_execute.side_effect = [ (hdparm_output, ''), - processutils.ProcessExecutionError() + processutils.ProcessExecutionError(), + (hdparm_output, ''), + processutils.ProcessExecutionError(), + (hdparm_output, ''), ] block_device = hardware.BlockDevice('/dev/sda', 'big', 1073741824, True) - self.assertRaises(errors.BlockDeviceEraseError, self.hardware._ata_erase, block_device) + mocked_execute.assert_any_call('hdparm', '--user-master', 'u', + '--security-unlock', '', '/dev/sda') + mocked_execute.assert_any_call('hdparm', '--user-master', 'u', + '--security-unlock', 'NULL', '/dev/sda') @mock.patch.object(utils, 'execute', autospec=True) def test__ata_erase_security_enabled_set_password_exception( self, mocked_execute): hdparm_output = create_hdparm_info( - supported=True, enabled=True, frozen=False, enhanced_erase=False) - hdparm_output_not_enabled = create_hdparm_info( supported=True, enabled=False, frozen=False, enhanced_erase=False) mocked_execute.side_effect = [ (hdparm_output, ''), - '', - (hdparm_output_not_enabled, ''), - '', processutils.ProcessExecutionError() ] @@ -1534,17 +1573,14 @@ class TestGenericHardwareManager(base.IronicAgentTest): @mock.patch.object(utils, 'execute', autospec=True) def test__ata_erase_security_erase_exec_exception( self, mocked_execute): + # Exception on security erase hdparm_output = create_hdparm_info( - supported=True, enabled=True, frozen=False, enhanced_erase=False) - hdparm_output_not_enabled = create_hdparm_info( supported=True, enabled=False, frozen=False, enhanced_erase=False) mocked_execute.side_effect = [ (hdparm_output, '', '-1'), - '', - (hdparm_output_not_enabled, ''), - '', - processutils.ProcessExecutionError() + '', # security-set-pass + processutils.ProcessExecutionError() # security-erase ] block_device = hardware.BlockDevice('/dev/sda', 'big', 1073741824, @@ -2003,8 +2039,8 @@ class TestModuleFunctions(base.IronicAgentTest): mock.call('iscsistart', '-f')]) -def create_hdparm_info(supported=False, enabled=False, frozen=False, - enhanced_erase=False): +def create_hdparm_info(supported=False, enabled=False, locked=False, + frozen=False, enhanced_erase=False): def update_values(values, state, key): if not state: @@ -2013,12 +2049,14 @@ def create_hdparm_info(supported=False, enabled=False, frozen=False, values = { 'supported': '\tsupported', 'enabled': '\tenabled', + 'locked': '\tlocked', 'frozen': '\tfrozen', 'enhanced_erase': '\tsupported: enhanced erase', } update_values(values, supported, 'supported') update_values(values, enabled, 'enabled') + update_values(values, locked, 'locked') update_values(values, frozen, 'frozen') update_values(values, enhanced_erase, 'enhanced_erase')