From aaf76e2cfb88c6cae50c8b46c65edce0a30e6a8f Mon Sep 17 00:00:00 2001 From: Will Szumski Date: Thu, 10 May 2018 21:53:44 +0100 Subject: [PATCH] rework ATA secure erase hdparm versions prior to 9.51 interpret the value, NULL, as a password with string value: "NULL". Example output of hdparm with NULL password: [root@localhost ~]# hdparm --user-master u --security-unlock NULL /dev/sda security_password="NULL" /dev/sda: Issuing SECURITY_UNLOCK command, password="NULL", user=user SECURITY_UNLOCK: Input/output error Example output of hdparm with "" as password: [root@localhost ~]# hdparm --user-master u --security-unlock "" /dev/sda security_password="" /dev/sda: Issuing SECURITY_UNLOCK command, password="", user=user Note the values of security_password in the output above. The output was observed on a CentOS 7 system, which ships hdparm 9.43 in the offical repositories. This change attempts to unlock the drive with the empty string if an unlock with NULL was unsucessful. Issuing a security-unlock will cause a state transition from SEC4 (security enabled, locked, not frozen) to SEC5 (security enabled, unlocked, not frozen). In order to check that a password unlock attempt was successful it makes sense to check that the drive is in the unlocked state (a necessary condition for SEC5). Only after all unlock attempts fail, do we consider the drive out of our control. The conditions to check the drive is in the right state have been adjusted to ensure that the drive is in the SEC5 state prior to issuing a secure erase. Previously, on the "recovery from previous fail" path, the security state was asserted to be "not enabled" after an unlock - this could never have been the case. A good overview of the ATA security states can be found here: http://www.admin-magazine.com/Archive/2014/19/Using-the-ATA-security-features-of-modern-hard-disks-and-SSDs Change-Id: Ic24b706a04ff6c08d750b9e3d79eb79eab2952ad Story: 2001762 Task: 12161 Story: 2001763 Task: 12162 --- ironic_python_agent/hardware.py | 74 ++++++++++------- .../tests/unit/test_hardware.py | 80 ++++++++++++++----- 2 files changed, 106 insertions(+), 48 deletions(-) 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')