diff --git a/Dockerfile b/Dockerfile index b7127080f..85e5735c8 100644 --- a/Dockerfile +++ b/Dockerfile @@ -27,7 +27,7 @@ RUN proxy.sh apt-get update && \ 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 \ - iptables lshw && \ + smartmontools iptables lshw && \ 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 e1400d2c5..4134b8c22 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 6a3a9db06..1b11a5920 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 c1ab4674e..714f988d1 100644 --- a/ironic_python_agent/tests/unit/test_hardware.py +++ b/ironic_python_agent/tests/unit/test_hardware.py @@ -348,6 +348,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): @@ -1235,6 +1249,7 @@ class TestGenericHardwareManager(base.IronicAgentTest): (create_hdparm_info( supported=True, enabled=False, frozen=False, enhanced_erase=False), ''), + (SMARTCTL_NORMAL_OUTPUT, ''), ('', ''), ('', ''), (create_hdparm_info( @@ -1247,6 +1262,36 @@ 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', + '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( + 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', @@ -1260,6 +1305,7 @@ class TestGenericHardwareManager(base.IronicAgentTest): mocked_execute.side_effect = [ (hdparm_output, ''), + (SMARTCTL_UNAVAILABLE_OUTPUT, ''), (SHRED_OUTPUT_1_ITERATION_ZERO_TRUE, '') ] @@ -1268,6 +1314,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') ]) @@ -1279,6 +1327,7 @@ class TestGenericHardwareManager(base.IronicAgentTest): mocked_execute.side_effect = [ (hdparm_output, ''), + (SMARTCTL_UNAVAILABLE_OUTPUT, ''), (SHRED_OUTPUT_1_ITERATION_ZERO_TRUE, '') ] @@ -1287,6 +1336,54 @@ 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') + ]) + + @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, '') + ] + + 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') ]) @@ -1302,6 +1399,7 @@ class TestGenericHardwareManager(base.IronicAgentTest): mocked_execute.side_effect = [ (hdparm_output, ''), + (SMARTCTL_NORMAL_OUTPUT, ''), (SHRED_OUTPUT_2_ITERATIONS_ZERO_FALSE, '') ] @@ -1310,6 +1408,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') ]) @@ -1325,6 +1425,7 @@ class TestGenericHardwareManager(base.IronicAgentTest): mocked_execute.side_effect = [ (hdparm_output, ''), + (SMARTCTL_UNAVAILABLE_OUTPUT, ''), (SHRED_OUTPUT_0_ITERATIONS_ZERO_FALSE, '') ] @@ -1333,6 +1434,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') ]) @@ -1418,6 +1521,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 "" @@ -1446,6 +1550,7 @@ class TestGenericHardwareManager(base.IronicAgentTest): mocked_execute.side_effect = [ (hdparm_output, ''), + (SMARTCTL_NORMAL_OUTPUT, ''), None, (hdparm_output, ''), None, @@ -1479,6 +1584,7 @@ class TestGenericHardwareManager(base.IronicAgentTest): mocked_execute.side_effect = [ (hdparm_output, ''), + (SMARTCTL_NORMAL_OUTPUT, ''), '', (hdparm_output_not_enabled, ''), '', @@ -1501,6 +1607,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(), @@ -1525,6 +1632,7 @@ class TestGenericHardwareManager(base.IronicAgentTest): mocked_execute.side_effect = [ (hdparm_output, ''), + (SMARTCTL_NORMAL_OUTPUT, ''), processutils.ProcessExecutionError() ] @@ -1545,6 +1653,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'), @@ -1567,7 +1676,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, @@ -1593,6 +1703,7 @@ class TestGenericHardwareManager(base.IronicAgentTest): mocked_execute.side_effect = [ (hdparm_output_before, ''), + (SMARTCTL_NORMAL_OUTPUT, ''), ('', ''), ('', ''), (hdparm_output_after, ''), @@ -1627,6 +1738,7 @@ class TestGenericHardwareManager(base.IronicAgentTest): mocked_execute.side_effect = [ (hdparm_output_before, ''), + (SMARTCTL_NORMAL_OUTPUT, ''), ('', ''), ('', ''), (hdparm_output_after, ''), @@ -1648,6 +1760,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 000000000..331f36eba --- /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.