Refuse secure erase if ATA command does not work

Adds dependency upon smartmontools's binary smartctl to
query the block devices via ATA mode which fails on pass-thru
buses such as ATA over SCSI and ATA over USB, in an effort
to prevent the initiation of ATA secure erase with one
of these interfaces in place which may render the disk
unreachable after security options are enabled for
ATA Secure Erase or upon the Secure Erase command being
sent to the Hard Disk.

Change-Id: I7635a197eb000650e919fac386b38ac15ef17041
Story: #2002546
Task: #22109
Depends-On: Ibbfd168844524d91927bdd6e67d973e0bd519bf2

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.