Merge "Refuse secure erase if ATA command does not work"

5 files changed, 165 insertions, 3 deletions

diff --git a/Dockerfile b/Dockerfile
index 2296addb..e34f096f 100644
--- a/Dockerfile
+++ b/Dockerfile
@@ -26,7 +26,8 @@ RUN proxy.sh apt-get update && \
     proxy.sh apt-get install -y --no-install-recommends netbase gdisk \
         python2.7 python2.7-dev python-pip qemu-utils parted hdparm \
         util-linux genisoimage git gcc bash coreutils tgt dmidecode \
-        ipmitool psmisc dosfstools bsdmainutils open-iscsi udev && \
+        ipmitool psmisc dosfstools bsdmainutils open-iscsi udev \
+        smartmontools && \
     proxy.sh apt-get --only-upgrade -t jessie-backports install -y qemu-utils
 
 # Some cleanup
diff --git a/imagebuild/tinyipa/build_files/finalreqs.lst b/imagebuild/tinyipa/build_files/finalreqs.lst
index e1400d2c..4134b8c2 100644
--- a/imagebuild/tinyipa/build_files/finalreqs.lst
+++ b/imagebuild/tinyipa/build_files/finalreqs.lst
@@ -14,3 +14,4 @@ udev-lib.tcz
 util-linux.tcz
 glib2.tcz
 iproute2.tcz
+smartmontools.tcz
diff --git a/ironic_python_agent/hardware.py b/ironic_python_agent/hardware.py
index 55753740..62090c12 100644
--- a/ironic_python_agent/hardware.py
+++ b/ironic_python_agent/hardware.py
@@ -888,6 +888,42 @@ class GenericHardwareManager(HardwareManager):
 
         return security_lines
 
+    def _smartctl_security_check(self, block_device):
+        """Checks if we can query security via smartctl.
+
+            :param block_device: A block_device object
+
+            :returns: True if we can query the block device via ATA
+                      or the smartctl binary is not present.
+                      False if we cannot query the device.
+        """
+        try:
+            # NOTE(TheJulia): smartctl has a concept of drivers being how
+            # to query or interpret data from the device. We want to use `ata`
+            # instead of `scsi` or `sat` as smartctl will not be able to read
+            # a bridged device that it doesn't understand, and accordingly
+            # return an error code.
+            output = utils.execute('smartctl', '-d', 'ata', block_device.name,
+                                   '-g', 'security',
+                                   check_exit_code=[0, 127])[0]
+            if 'Unavailable' in output:
+                # Smartctl is reporting it is unavailable, lets return false.
+                LOG.debug('Smartctl has reported that security is '
+                          'unavailable on device %s.', block_device.name)
+                return False
+            return True
+        except processutils.ProcessExecutionError:
+            # Things don't look so good....
+            LOG.warning('Refusing to permit ATA Secure Erase as direct '
+                        'ATA commands via the `smartctl` utility with device '
+                        '%s do not succeed.', block_device.name)
+            return False
+        except OSError:
+            # Processutils can raise OSError if a path is not found,
+            # and it is okay that we tollerate that since it was the
+            # prior behavior.
+            return True
+
     def _ata_erase(self, block_device):
 
         def __attempt_unlock_drive(block_device, security_lines=None):
@@ -920,7 +956,8 @@ class GenericHardwareManager(HardwareManager):
         # can try another mechanism. Below here, if secure erase is supported
         # but fails in some way, error out (operators of hardware that supports
         # secure erase presumably expect this to work).
-        if 'supported' not in security_lines:
+        if (not self._smartctl_security_check(block_device)
+                or 'supported' not in security_lines):
             return False
 
         # At this point, we could be SEC1,2,4,5,6
diff --git a/ironic_python_agent/tests/unit/test_hardware.py b/ironic_python_agent/tests/unit/test_hardware.py
index 2f0a8354..838dc3cc 100644
--- a/ironic_python_agent/tests/unit/test_hardware.py
+++ b/ironic_python_agent/tests/unit/test_hardware.py
@@ -349,6 +349,20 @@ LSHW_JSON_OUTPUT = ("""
 }
 """, "")
 
+SMARTCTL_NORMAL_OUTPUT = ("""
+smartctl 6.2 2017-02-27 r4394 [x86_64-linux-3.10.0-693.21.1.el7.x86_64] (local build)
+Copyright (C) 2002-13, Bruce Allen, Christian Franke, www.smartmontools.org
+
+ATA Security is:  Disabled, NOT FROZEN [SEC1]
+""")  # noqa
+
+SMARTCTL_UNAVAILABLE_OUTPUT = ("""
+smartctl 6.2 2017-02-27 r4394 [x86_64-linux-3.10.0-693.21.1.el7.x86_64] (local build)
+Copyright (C) 2002-13, Bruce Allen, Christian Franke, www.smartmontools.org
+
+ATA Security is:  Unavailable
+""")  # noqa
+
 
 class FakeHardwareManager(hardware.GenericHardwareManager):
     def __init__(self, hardware_support):
@@ -1270,6 +1284,35 @@ class TestGenericHardwareManager(base.IronicAgentTest):
             (create_hdparm_info(
                 supported=True, enabled=False, frozen=False,
                 enhanced_erase=False), ''),
+            (SMARTCTL_NORMAL_OUTPUT, ''),
+            ('', ''),
+            ('', ''),
+            (create_hdparm_info(
+                supported=True, enabled=False, frozen=False,
+                enhanced_erase=False), ''),
+        ]
+
+        block_device = hardware.BlockDevice('/dev/sda', 'big', 1073741824,
+                                            True)
+        self.hardware.erase_block_device(self.node, block_device)
+        mocked_execute.assert_has_calls([
+            mock.call('hdparm', '-I', '/dev/sda'),
+            mock.call('smartctl', '-d', 'ata', '/dev/sda', '-g', 'security',
+                      check_exit_code=[0, 127]),
+            mock.call('hdparm', '--user-master', 'u', '--security-set-pass',
+                      'NULL', '/dev/sda'),
+            mock.call('hdparm', '--user-master', 'u', '--security-erase',
+                      'NULL', '/dev/sda'),
+            mock.call('hdparm', '-I', '/dev/sda'),
+        ])
+
+    @mock.patch.object(utils, 'execute', autospec=True)
+    def test_erase_block_device_ata_success_no_smartctl(self, mocked_execute):
+        mocked_execute.side_effect = [
+            (create_hdparm_info(
+                supported=True, enabled=False, frozen=False,
+                enhanced_erase=False), ''),
+            OSError('boom'),
             ('', ''),
             ('', ''),
             (create_hdparm_info(
@@ -1282,6 +1325,8 @@ class TestGenericHardwareManager(base.IronicAgentTest):
         self.hardware.erase_block_device(self.node, block_device)
         mocked_execute.assert_has_calls([
             mock.call('hdparm', '-I', '/dev/sda'),
+            mock.call('smartctl', '-d', 'ata', '/dev/sda', '-g', 'security',
+                      check_exit_code=[0, 127]),
             mock.call('hdparm', '--user-master', 'u', '--security-set-pass',
                       'NULL', '/dev/sda'),
             mock.call('hdparm', '--user-master', 'u', '--security-erase',
@@ -1295,6 +1340,7 @@ class TestGenericHardwareManager(base.IronicAgentTest):
 
         mocked_execute.side_effect = [
             (hdparm_output, ''),
+            (SMARTCTL_UNAVAILABLE_OUTPUT, ''),
             (SHRED_OUTPUT_1_ITERATION_ZERO_TRUE, '')
         ]
 
@@ -1303,6 +1349,8 @@ class TestGenericHardwareManager(base.IronicAgentTest):
         self.hardware.erase_block_device(self.node, block_device)
         mocked_execute.assert_has_calls([
             mock.call('hdparm', '-I', '/dev/sda'),
+            mock.call('smartctl', '-d', 'ata', '/dev/sda', '-g', 'security',
+                      check_exit_code=[0, 127]),
             mock.call('shred', '--force', '--zero', '--verbose',
                       '--iterations', '1', '/dev/sda')
         ])
@@ -1314,6 +1362,53 @@ class TestGenericHardwareManager(base.IronicAgentTest):
 
         mocked_execute.side_effect = [
             (hdparm_output, ''),
+            (SMARTCTL_UNAVAILABLE_OUTPUT, ''),
+            (SHRED_OUTPUT_1_ITERATION_ZERO_TRUE, '')
+        ]
+
+        block_device = hardware.BlockDevice('/dev/sda', 'big', 1073741824,
+                                            True)
+        self.hardware.erase_block_device(self.node, block_device)
+        mocked_execute.assert_has_calls([
+            mock.call('hdparm', '-I', '/dev/sda'),
+            mock.call('smartctl', '-d', 'ata', '/dev/sda', '-g', 'security',
+                      check_exit_code=[0, 127]),
+            mock.call('shred', '--force', '--zero', '--verbose',
+                      '--iterations', '1', '/dev/sda')
+        ])
+
+    @mock.patch.object(utils, 'execute', autospec=True)
+    def test_erase_block_device_smartctl_unsupported_shred(self,
+                                                           mocked_execute):
+        hdparm_output = create_hdparm_info(
+            supported=True, enabled=False, frozen=False, enhanced_erase=False)
+
+        mocked_execute.side_effect = [
+            (hdparm_output, ''),
+            (SMARTCTL_UNAVAILABLE_OUTPUT, ''),
+            (SHRED_OUTPUT_1_ITERATION_ZERO_TRUE, '')
+        ]
+
+        block_device = hardware.BlockDevice('/dev/sda', 'big', 1073741824,
+                                            True)
+        self.hardware.erase_block_device(self.node, block_device)
+        mocked_execute.assert_has_calls([
+            mock.call('hdparm', '-I', '/dev/sda'),
+            mock.call('smartctl', '-d', 'ata', '/dev/sda', '-g', 'security',
+                      check_exit_code=[0, 127]),
+            mock.call('shred', '--force', '--zero', '--verbose',
+                      '--iterations', '1', '/dev/sda')
+        ])
+
+    @mock.patch.object(utils, 'execute', autospec=True)
+    def test_erase_block_device_smartctl_fails_security_fallback_to_shred(
+            self, mocked_execute):
+        hdparm_output = create_hdparm_info(
+            supported=True, enabled=False, frozen=False, enhanced_erase=False)
+
+        mocked_execute.side_effect = [
+            (hdparm_output, ''),
+            processutils.ProcessExecutionError(),
             (SHRED_OUTPUT_1_ITERATION_ZERO_TRUE, '')
         ]
 
@@ -1322,6 +1417,8 @@ class TestGenericHardwareManager(base.IronicAgentTest):
         self.hardware.erase_block_device(self.node, block_device)
         mocked_execute.assert_has_calls([
             mock.call('hdparm', '-I', '/dev/sda'),
+            mock.call('smartctl', '-d', 'ata', '/dev/sda', '-g', 'security',
+                      check_exit_code=[0, 127]),
             mock.call('shred', '--force', '--zero', '--verbose',
                       '--iterations', '1', '/dev/sda')
         ])
@@ -1337,6 +1434,7 @@ class TestGenericHardwareManager(base.IronicAgentTest):
 
         mocked_execute.side_effect = [
             (hdparm_output, ''),
+            (SMARTCTL_NORMAL_OUTPUT, ''),
             (SHRED_OUTPUT_2_ITERATIONS_ZERO_FALSE, '')
         ]
 
@@ -1345,6 +1443,8 @@ class TestGenericHardwareManager(base.IronicAgentTest):
         self.hardware.erase_block_device(self.node, block_device)
         mocked_execute.assert_has_calls([
             mock.call('hdparm', '-I', '/dev/sda'),
+            mock.call('smartctl', '-d', 'ata', '/dev/sda', '-g', 'security',
+                      check_exit_code=[0, 127]),
             mock.call('shred', '--force', '--verbose',
                       '--iterations', '2', '/dev/sda')
         ])
@@ -1360,6 +1460,7 @@ class TestGenericHardwareManager(base.IronicAgentTest):
 
         mocked_execute.side_effect = [
             (hdparm_output, ''),
+            (SMARTCTL_UNAVAILABLE_OUTPUT, ''),
             (SHRED_OUTPUT_0_ITERATIONS_ZERO_FALSE, '')
         ]
 
@@ -1368,6 +1469,8 @@ class TestGenericHardwareManager(base.IronicAgentTest):
         self.hardware.erase_block_device(self.node, block_device)
         mocked_execute.assert_has_calls([
             mock.call('hdparm', '-I', '/dev/sda'),
+            mock.call('smartctl', '-d', 'ata', '/dev/sda', '-g', 'security',
+                      check_exit_code=[0, 127]),
             mock.call('shred', '--force', '--verbose',
                       '--iterations', '0', '/dev/sda')
         ])
@@ -1453,6 +1556,7 @@ class TestGenericHardwareManager(base.IronicAgentTest):
             supported=True, enabled=False, frozen=False, enhanced_erase=False)
         mocked_execute.side_effect = [
             (hdparm_output, ''),
+            (SMARTCTL_NORMAL_OUTPUT, ''),
             processutils.ProcessExecutionError(),  # NULL fails to unlock
             (hdparm_output, ''),  # recheck security lines
             None,  # security unlock with ""
@@ -1481,6 +1585,7 @@ class TestGenericHardwareManager(base.IronicAgentTest):
 
         mocked_execute.side_effect = [
             (hdparm_output, ''),
+            (SMARTCTL_NORMAL_OUTPUT, ''),
             None,
             (hdparm_output, ''),
             None,
@@ -1514,6 +1619,7 @@ class TestGenericHardwareManager(base.IronicAgentTest):
 
         mocked_execute.side_effect = [
             (hdparm_output, ''),
+            (SMARTCTL_NORMAL_OUTPUT, ''),
             '',
             (hdparm_output_not_enabled, ''),
             '',
@@ -1536,6 +1642,7 @@ class TestGenericHardwareManager(base.IronicAgentTest):
             supported=True, enabled=True, locked=True)
         mocked_execute.side_effect = [
             (hdparm_output, ''),
+            (SMARTCTL_NORMAL_OUTPUT, ''),
             processutils.ProcessExecutionError(),
             (hdparm_output, ''),
             processutils.ProcessExecutionError(),
@@ -1560,6 +1667,7 @@ class TestGenericHardwareManager(base.IronicAgentTest):
 
         mocked_execute.side_effect = [
             (hdparm_output, ''),
+            (SMARTCTL_NORMAL_OUTPUT, ''),
             processutils.ProcessExecutionError()
         ]
 
@@ -1580,6 +1688,7 @@ class TestGenericHardwareManager(base.IronicAgentTest):
             supported=True, locked=True, frozen=False, enhanced_erase=False)
         mocked_execute.side_effect = [
             (hdparm_output, '', '-1'),
+            (SMARTCTL_NORMAL_OUTPUT, ''),
             '',  # security-set-pass
             processutils.ProcessExecutionError(),  # security-erase
             (hdparm_unlocked_output, '', '-1'),
@@ -1602,7 +1711,8 @@ class TestGenericHardwareManager(base.IronicAgentTest):
             supported=True, enabled=False, frozen=True, enhanced_erase=False)
 
         mocked_execute.side_effect = [
-            (hdparm_output, '')
+            (hdparm_output, ''),
+            (SMARTCTL_NORMAL_OUTPUT, '')
         ]
 
         block_device = hardware.BlockDevice('/dev/sda', 'big', 1073741824,
@@ -1628,6 +1738,7 @@ class TestGenericHardwareManager(base.IronicAgentTest):
 
         mocked_execute.side_effect = [
             (hdparm_output_before, ''),
+            (SMARTCTL_NORMAL_OUTPUT, ''),
             ('', ''),
             ('', ''),
             (hdparm_output_after, ''),
@@ -1662,6 +1773,7 @@ class TestGenericHardwareManager(base.IronicAgentTest):
 
         mocked_execute.side_effect = [
             (hdparm_output_before, ''),
+            (SMARTCTL_NORMAL_OUTPUT, ''),
             ('', ''),
             ('', ''),
             (hdparm_output_after, ''),
@@ -1683,6 +1795,7 @@ class TestGenericHardwareManager(base.IronicAgentTest):
                 (create_hdparm_info(
                     supported=True, enabled=False, frozen=False,
                     enhanced_erase=enhanced_erase), ''),
+                (SMARTCTL_NORMAL_OUTPUT, ''),
                 ('', ''),
                 ('', ''),
                 (create_hdparm_info(
diff --git a/releasenotes/notes/adds-smartctl-ata-check-to-secure-erase-caebba4f25821575.yaml b/releasenotes/notes/adds-smartctl-ata-check-to-secure-erase-caebba4f25821575.yaml
new file mode 100644
index 00000000..331f36eb
--- /dev/null
+++ b/releasenotes/notes/adds-smartctl-ata-check-to-secure-erase-caebba4f25821575.yaml
@@ -0,0 +1,10 @@
+---
+fixes:
+  - |
+    Adds an additional check if the ``smartctl`` utility is present from the
+    ``smartmontools`` package, which performs an ATA disk specific check that
+    should prevent ATA Secure Erase from being performed if a pass-thru
+    device is detected that requires a non-ATA command signling sequence.
+    Devices such as these can be `smart` disk interfaces such as
+    RAID controllers and USB disk adapters, which can cause failures
+    when attempting to Secure Erase, which may render the disk unreachable.