Merge "rework ATA secure erase"

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:
+        # At this point, we could be SEC1,2,4,5,6
-            # 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))
 
         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:
+        # At this point, we could be in SEC1,4,5
-            utils.execute('hdparm', '--user-master', 'u',
+
-                          '--security-set-pass', 'NULL', block_device.name)
+        # Attempt to unlock the drive in the event it has already been
-        except processutils.ProcessExecutionError as e:
+        # locked by a previous failed attempt. We try the empty string as
-            raise errors.BlockDeviceEraseError('Security password set '
+        # versions of hdparm < 9.51, interpreted NULL as the literal string,
-                                               'failed for device '
+        # "NULL", as opposed to the empty string.
-                                               '%(name)s: %(err)s' %
+        unlock_passwords = ['NULL', '']
-                                               {'name': block_device.name,
+        for password in unlock_passwords:
-                                                'err': e})
+            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):

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'),
-            '',
+            '',  # security-set-pass
-            (hdparm_output_not_enabled, ''),
+            processutils.ProcessExecutionError()  # security-erase
-            '',
-            processutils.ProcessExecutionError()
         ]
 
         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,
+def create_hdparm_info(supported=False, enabled=False, locked=False,
-                       enhanced_erase=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')