From 78c1343a540060527ff56cee92a9072146034ecb Mon Sep 17 00:00:00 2001 From: Julia Kreger Date: Fri, 7 Apr 2023 13:38:21 -0700 Subject: [PATCH] Fix Bandit errors Bandit 1.7.5 released with a timeout check for all requests and urllib calls. Fixed those. In the process, then exposed a bandit b310 issue, which was already covered by the code, but explicitly marked it as such. Also, enables bandit checks to be voting for CI.. Change-Id: If0e87790191f5f3648366d571e1d85dd7393a548 --- ironic_python_agent/config.py | 6 ++++++ ironic_python_agent/extensions/standby.py | 2 +- .../nvidia/nvidia_fw_update.py | 9 ++++++++- ironic_python_agent/inspector.py | 6 ++++-- ironic_python_agent/ironic_api_client.py | 1 + .../tests/unit/test_inspector.py | 9 ++++++--- .../notes/bandit-fixes-a971142075b29ca9.yaml | 17 +++++++++++++++++ zuul.d/project.yaml | 4 ++-- 8 files changed, 45 insertions(+), 9 deletions(-) create mode 100644 releasenotes/notes/bandit-fixes-a971142075b29ca9.yaml diff --git a/ironic_python_agent/config.py b/ironic_python_agent/config.py index cd6de31f6..1c4359c2a 100644 --- a/ironic_python_agent/config.py +++ b/ironic_python_agent/config.py @@ -332,6 +332,12 @@ cli_opts = [ default=True, help='If the MD5 algorithm is enabled for file checksums. ' 'Will be changed to False in the future.'), + cfg.IntOpt('http_request_timeout', + default=APARAMS.get('ipa-http-request-timeout', 30), + min=1, + help='Time in seconds to wait for an HTTP request TCP socket ' + 'used by an API request to a remote service to enter ' + 'a state where a request can be transmitted.'), ] CONF.register_cli_opts(cli_opts) diff --git a/ironic_python_agent/extensions/standby.py b/ironic_python_agent/extensions/standby.py index 90affd752..92ee5d674 100644 --- a/ironic_python_agent/extensions/standby.py +++ b/ironic_python_agent/extensions/standby.py @@ -371,7 +371,7 @@ class ImageDownload(object): # Realistically, this should never happen, but for # compatability... # TODO(TheJulia): Remove for a 2024 release. - self._hash_algo = hashlib.new('md5') + self._hash_algo = hashlib.new('md5') # nosec else: self._hash_algo = new_algo except ValueError as e: diff --git a/ironic_python_agent/hardware_managers/nvidia/nvidia_fw_update.py b/ironic_python_agent/hardware_managers/nvidia/nvidia_fw_update.py index 73cefe5e9..185ef4d16 100644 --- a/ironic_python_agent/hardware_managers/nvidia/nvidia_fw_update.py +++ b/ironic_python_agent/hardware_managers/nvidia/nvidia_fw_update.py @@ -23,11 +23,15 @@ from urllib import request from ironic_lib.common.i18n import _ from ironic_lib.exception import IronicException from oslo_concurrency import processutils +from oslo_config import cfg from oslo_log import log from oslo_utils import fileutils from ironic_python_agent import utils + +CONF = cfg.CONF + FW_VERSION_REGEX = r'FW Version:\s*\t*(?P\d+\.\d+\.\d+)' RUNNING_FW_VERSION_REGEX = \ r'FW Version\(Running\):\s*\t*(?P\d+\.\d+\.\d+)' @@ -415,7 +419,10 @@ class NvidiaNicFirmwareBinary(object): try: LOG.info('Downloading file: %s to %s', self.url, self.dest_file_path) - url_data = request.urlopen(self.url) + # NOTE(TheJulia: nosec b310 rule below is covered by _process_url + url_data = request.urlopen( + self.url, + timeout=CONF.image_download_connection_timeout) # nosec except urlError.URLError as url_error: LOG.error('Failed to open URL data: %s', url_error) raise url_error diff --git a/ironic_python_agent/inspector.py b/ironic_python_agent/inspector.py index c2d98d5de..b4660164c 100644 --- a/ironic_python_agent/inspector.py +++ b/ironic_python_agent/inspector.py @@ -139,8 +139,10 @@ def call_inspector(data, failures): wait=tenacity.wait_fixed(_RETRY_WAIT), reraise=True) def _post_to_inspector(): - return requests.post(CONF.inspection_callback_url, data=data, - verify=verify, cert=cert) + return requests.post( + CONF.inspection_callback_url, data=data, + verify=verify, cert=cert, + timeout=CONF.http_request_timeout) resp = _post_to_inspector() if resp.status_code >= 400: diff --git a/ironic_python_agent/ironic_api_client.py b/ironic_python_agent/ironic_api_client.py index a2e7e1d00..e0204d282 100644 --- a/ironic_python_agent/ironic_api_client.py +++ b/ironic_python_agent/ironic_api_client.py @@ -80,6 +80,7 @@ class APIClient(object): data=data, verify=verify, cert=cert, + timeout=CONF.http_request_timeout, **kwargs) def _get_ironic_api_version_header(self, version=None): diff --git a/ironic_python_agent/tests/unit/test_inspector.py b/ironic_python_agent/tests/unit/test_inspector.py index b5861c750..f44121305 100644 --- a/ironic_python_agent/tests/unit/test_inspector.py +++ b/ironic_python_agent/tests/unit/test_inspector.py @@ -163,7 +163,8 @@ class TestCallInspector(base.IronicAgentTest): mock_post.assert_called_once_with('url', cert=None, verify=True, - data='{"data": 42, "error": null}') + data='{"data": 42, "error": null}', + timeout=30) self.assertEqual(mock_post.return_value.json.return_value, res) def test_send_failure(self, mock_post): @@ -176,7 +177,8 @@ class TestCallInspector(base.IronicAgentTest): mock_post.assert_called_once_with('url', cert=None, verify=True, - data='{"data": 42, "error": "boom"}') + data='{"data": 42, "error": "boom"}', + timeout=30) self.assertEqual(mock_post.return_value.json.return_value, res) def test_inspector_error(self, mock_post): @@ -188,7 +190,8 @@ class TestCallInspector(base.IronicAgentTest): mock_post.assert_called_once_with('url', cert=None, verify=True, - data='{"data": 42, "error": null}') + data='{"data": 42, "error": null}', + timeout=30) self.assertIsNone(res) @mock.patch.object(inspector, '_RETRY_WAIT', 0.01) diff --git a/releasenotes/notes/bandit-fixes-a971142075b29ca9.yaml b/releasenotes/notes/bandit-fixes-a971142075b29ca9.yaml new file mode 100644 index 000000000..b2548e338 --- /dev/null +++ b/releasenotes/notes/bandit-fixes-a971142075b29ca9.yaml @@ -0,0 +1,17 @@ +--- +fixes: + - | + Fixes timeout declarations for Bandit 1.7.5 rule additions. + - | + Adds a new configuration option ``http_request_timeout`` + to allow for operators to set the amount of time to wait + for a new request socket to wait. This helps prevent prevent + a possible hanged connection should the initial packets be + lost in tranist. +other: + - | + Adds a new configuration option ``http_request_timeout`` + which is also accessible utilizing the kernel command line + option ``ipa-http-request-timeout``. This option helps prevent + failed connections from hanging the agent. The default is 30 + seconds. diff --git a/zuul.d/project.yaml b/zuul.d/project.yaml index ceff4cfb9..1e7e5efe8 100644 --- a/zuul.d/project.yaml +++ b/zuul.d/project.yaml @@ -9,6 +9,7 @@ check: jobs: - openstack-tox-functional + - ipa-tox-bandit - ipa-tox-examples # NOTE(iurygregory) Only run this two jobs since we are testing # wholedisk + partition on tempest @@ -26,11 +27,10 @@ # Non-voting jobs - ipa-tempest-ironic-inspector-src: voting: false - - ipa-tox-bandit: - voting: false gate: jobs: - openstack-tox-functional + - ipa-tox-bandit - ipa-tox-examples - ipa-tempest-bios-ipmi-direct-src - ipa-tempest-uefi-redfish-vmedia-src