openstack/ironic-python-agent
Code Issues Proposed changes

Refuse secure erase if ATA command does not work

Browse Source 
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
This commit is contained in:
Julia Kreger 2018-06-13 12:13:20 -07:00
parent 80be07ae79
commit aef703b879
5 changed files with 165 additions and 3 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

3
Dockerfile
View File

@ -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

1
imagebuild/tinyipa/build_files/finalreqs.lst
View File

@ -14,3 +14,4 @@ udev-lib.tcz
util-linux.tcz
glib2.tcz
iproute2.tcz
smartmontools.tcz

39
ironic_python_agent/hardware.py
View File

@ -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

115
ironic_python_agent/tests/unit/test_hardware.py
View File

@ -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(

10
releasenotes/notes/adds-smartctl-ata-check-to-secure-erase-caebba4f25821575.yaml Normal file
View File

@ -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.