diff --git a/Dockerfile b/Dockerfile index 2296addbf..e34f096fa 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 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 55753740d..62090c125 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 2f0a8354a..838dc3ccb 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,7 @@ class TestGenericHardwareManager(base.IronicAgentTest): (create_hdparm_info( supported=True, enabled=False, frozen=False, enhanced_erase=False), ''), + (SMARTCTL_NORMAL_OUTPUT, ''), ('', ''), ('', ''), (create_hdparm_info( @@ -1282,6 +1297,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', @@ -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,7 @@ class TestGenericHardwareManager(base.IronicAgentTest): mocked_execute.side_effect = [ (hdparm_output, ''), + (SMARTCTL_UNAVAILABLE_OUTPUT, ''), (SHRED_OUTPUT_1_ITERATION_ZERO_TRUE, '') ] @@ -1322,6 +1371,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') ]) @@ -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 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.