Allow configuring shred's final overwrite with zeros

Introduce shred_final_overwrite_with_zeros, a new configuration option
to control whether devices will receive a final overwrite with zeros
during cleaning. Additionally, rename erase_devices_iterations to
shred_random_overwrite_iterations to clarify the true meaning of this
configuration option.

Also, ensure a warning is raised in the logs to raise awareness around
the potential security risk of running cleaning with iterations=0 and
overwrite_with_zeros=False.

Change-Id: I0dd3f488ab2cd0df778f34a5a23948fa0c6c4334
Closes-Bug: #1568811
Depends-On: I7053034f5b5bc6737b535ee601e6fb71284d4a83

etc/ironic/ironic.conf.sample

@@ -907,9 +907,19 @@
 # set to 0, will not run during cleaning. (integer value)
 #erase_devices_priority = <None>
 
-# Number of iterations to be run for erasing devices. (integer
+# During shred, overwrite all block devices N times with
-# value)
+# random data. This is only used if a device could not be ATA
-#erase_devices_iterations = 1
+# Secure Erased. Defaults to 1. (integer value)
+# Minimum value: 0
+# Deprecated group/name - [deploy]/erase_devices_iterations
+#shred_random_overwrite_iterations = 1
+
+# Whether to write zeros to a node's block devices after
+# writing random data. This will write zeros to the device
+# even when deploy.shred_random_overwrite_interations is 0.
+# This option is only used if a device could not be ATA Secure
+# Erased. Defaults to True. (boolean value)
+#shred_final_overwrite_with_zeros = true
 
 # Whether to power off a node after deploy failure. Defaults
 # to True. (boolean value)

ironic/drivers/modules/deploy_utils.py

@@ -49,12 +49,12 @@ from ironic import objects
 
 deploy_opts = [
     cfg.StrOpt('http_url',
-               help='ironic-conductor node\'s HTTP server URL. '
+               help=_("ironic-conductor node's HTTP server URL. "
-                    'Example: http://192.1.2.3:8080',
+                      "Example: http://192.1.2.3:8080"),
                deprecated_group='pxe'),
     cfg.StrOpt('http_root',
                default='/httpboot',
-               help='ironic-conductor node\'s HTTP root path.',
+               help=_("ironic-conductor node's HTTP root path."),
                deprecated_group='pxe'),
     cfg.IntOpt('erase_devices_priority',
                help=_('Priority to run in-band erase devices via the Ironic '
@@ -62,9 +62,23 @@ deploy_opts = [
                       'set in the ramdisk (defaults to 10 for the '
                       'GenericHardwareManager). If set to 0, will not run '
                       'during cleaning.')),
-    cfg.IntOpt('erase_devices_iterations',
+    # TODO(mmitchell): Remove the deprecated name/group during Ocata cycle.
+    cfg.IntOpt('shred_random_overwrite_iterations',
+               deprecated_name='erase_devices_iterations',
+               deprecated_group='deploy',
                default=1,
-               help=_('Number of iterations to be run for erasing devices.')),
+               min=0,
+               help=_('During shred, overwrite all block devices N times with '
+                      'random data. This is only used if a device could not '
+                      'be ATA Secure Erased. Defaults to 1.')),
+    cfg.BoolOpt('shred_final_overwrite_with_zeros',
+                default=True,
+                help=_("Whether to write zeros to a node's block devices "
+                       "after writing random data. This will write zeros to "
+                       "the device even when "
+                       "deploy.shred_random_overwrite_interations is 0. This "
+                       "option is only used if a device could not be ATA "
+                       "Secure Erased. Defaults to True.")),
     cfg.BoolOpt('power_off_after_deploy_failure',
                 default=True,
                 help=_('Whether to power off a node after deploy failure. '
@@ -97,6 +111,19 @@ SUPPORTED_CAPABILITIES = {
 
 DISK_LAYOUT_PARAMS = ('root_gb', 'swap_mb', 'ephemeral_gb')
 
+
+def warn_about_unsafe_shred_parameters():
+    iterations = CONF.deploy.shred_random_overwrite_iterations
+    overwrite_with_zeros = CONF.deploy.shred_final_overwrite_with_zeros
+    if iterations == 0 and overwrite_with_zeros is False:
+        LOG.warning(_LW('With shred_random_overwrite_iterations set to 0 and '
+                        'shred_final_overwrite_with_zeros set to False, disks '
+                        'may NOT be shredded at all, unless they support ATA '
+                        'Secure Erase. This is a possible SECURITY ISSUE!'))
+
+
+warn_about_unsafe_shred_parameters()
+
 # All functions are called from deploy() directly or indirectly.
 # They are split for stub-out.
 
@@ -629,8 +656,12 @@ def agent_add_clean_params(task):
     :param task: a TaskManager instance.
     """
     info = task.node.driver_internal_info
-    passes = CONF.deploy.erase_devices_iterations
+
-    info['agent_erase_devices_iterations'] = passes
+    random_iterations = CONF.deploy.shred_random_overwrite_iterations
+    info['agent_erase_devices_iterations'] = random_iterations
+    zeroize = CONF.deploy.shred_final_overwrite_with_zeros
+    info['agent_erase_devices_zeroize'] = zeroize
+
     task.node.driver_internal_info = info
     task.node.save()
 

ironic/tests/unit/drivers/modules/test_deploy_utils.py

@@ -1287,7 +1287,8 @@ class OtherFunctionTestCase(db_base.DbTestCase):
                                     log_calls=calls)
 
     def test_set_failed_state_no_poweroff(self):
-        cfg.CONF.deploy.power_off_after_deploy_failure = False
+        cfg.CONF.set_override('power_off_after_deploy_failure', False,
+                              'deploy')
         exc_state = exception.InvalidState('invalid state')
         exc_param = exception.InvalidParameterValue('invalid parameter')
         mock_call = mock.call(mock.ANY)
@@ -1343,6 +1344,37 @@ class OtherFunctionTestCase(db_base.DbTestCase):
         mock_clean_up_caches.assert_called_once_with(None, 'master_dir',
                                                      [('uuid', 'path')])
 
+    @mock.patch.object(utils, 'LOG', autospec=True)
+    def test_warn_about_unsafe_shred_parameters_defaults(self, log_mock):
+        utils.warn_about_unsafe_shred_parameters()
+        self.assertFalse(log_mock.warning.called)
+
+    @mock.patch.object(utils, 'LOG', autospec=True)
+    def test_warn_about_unsafe_shred_parameters_zeros(self, log_mock):
+        cfg.CONF.set_override('shred_random_overwrite_iterations', 0, 'deploy')
+        cfg.CONF.set_override('shred_final_overwrite_with_zeros', True,
+                              'deploy')
+        utils.warn_about_unsafe_shred_parameters()
+        self.assertFalse(log_mock.warning.called)
+
+    @mock.patch.object(utils, 'LOG', autospec=True)
+    def test_warn_about_unsafe_shred_parameters_random_no_zeros(self,
+                                                                log_mock):
+        cfg.CONF.set_override('shred_random_overwrite_iterations', 1, 'deploy')
+        cfg.CONF.set_override('shred_final_overwrite_with_zeros', False,
+                              'deploy')
+        utils.warn_about_unsafe_shred_parameters()
+        self.assertFalse(log_mock.warning.called)
+
+    @mock.patch.object(utils, 'LOG', autospec=True)
+    def test_warn_about_unsafe_shred_parameters_produces_a_warning(self,
+                                                                   log_mock):
+        cfg.CONF.set_override('shred_random_overwrite_iterations', 0, 'deploy')
+        cfg.CONF.set_override('shred_final_overwrite_with_zeros', False,
+                              'deploy')
+        utils.warn_about_unsafe_shred_parameters()
+        self.assertTrue(log_mock.warning.called)
+
 
 class VirtualMediaDeployUtilsTestCase(db_base.DbTestCase):
 
@@ -1680,12 +1712,16 @@ class AgentMethodsTestCase(db_base.DbTestCase):
             self.assertEqual(states.CLEANWAIT, response)
 
     def test_agent_add_clean_params(self):
-        cfg.CONF.deploy.erase_devices_iterations = 2
+        cfg.CONF.set_override('shred_random_overwrite_iterations', 2, 'deploy')
+        cfg.CONF.set_override('shred_final_overwrite_with_zeros', False,
+                              'deploy')
         with task_manager.acquire(
                 self.context, self.node.uuid, shared=False) as task:
             utils.agent_add_clean_params(task)
-            self.assertEqual(task.node.driver_internal_info.get(
+            self.assertEqual(2, task.node.driver_internal_info.get(
-                'agent_erase_devices_iterations'), 2)
+                'agent_erase_devices_iterations'))
+            self.assertEqual(False, task.node.driver_internal_info.get(
+                'agent_erase_devices_zeroize'))
 
     @mock.patch('ironic.dhcp.neutron.NeutronDHCPApi.delete_cleaning_ports',
                 autospec=True)
@@ -1749,8 +1785,10 @@ class AgentMethodsTestCase(db_base.DbTestCase):
                 utils.prepare_inband_cleaning(task, manage_boot=manage_boot))
             prepare_cleaning_ports_mock.assert_called_once_with(task)
             power_mock.assert_called_once_with(task, states.REBOOT)
-            self.assertEqual(task.node.driver_internal_info.get(
+            self.assertEqual(1, task.node.driver_internal_info.get(
-                             'agent_erase_devices_iterations'), 1)
+                             'agent_erase_devices_iterations'))
+            self.assertEqual(True, task.node.driver_internal_info.get(
+                             'agent_erase_devices_zeroize'))
             if manage_boot:
                 prepare_ramdisk_mock.assert_called_once_with(
                     mock.ANY, mock.ANY, {'a': 'b', 'c': 'd'})

releasenotes/notes/shred-final-overwrite-with-zeros-50b5ba5b19c0da27.yaml

@@ -0,0 +1,14 @@
+---
+features:
+  - A new configuration option, `shred_final_overwrite_with_zeros` is now
+    available. This option controls the final overwrite with zeros done on
+    all block devices for a node under cleaning. This feature was previously
+    always enabled and not configurable. This option is only used when a
+    block device could not be ATA Secure Erased.
+deprecations:
+  - The [deploy]/erase_devices_iterations config is deprecated and will
+    be removed in the Ocata cycle. It has been replaced by the
+    [deploy]/shred_random_overwrite_iterations config. This configuration
+    option controls the number of times block devices are overwritten with
+    random data. This option is only used when a block device could not be
+    ATA Secure Erased.