diff --git a/ironic_python_agent/config.py b/ironic_python_agent/config.py index 7fb1d88f8..383d290d1 100644 --- a/ironic_python_agent/config.py +++ b/ironic_python_agent/config.py @@ -262,6 +262,31 @@ cli_opts = [ help='Pre-shared token to use when working with the ' 'ironic API. This value is typically supplied by ' 'ironic automatically.'), + cfg.StrOpt('image_server_user', + default=None, + help='Pre-shared username used in Basic Auth process ' + 'against the http(s) server that hosts the disk image.' + 'This variable can be also configured via image_info.' + 'Value coming from image_info takes precedence over' + 'value coming from command line or configuration file.'), + cfg.StrOpt('image_server_password', + default=None, + help='Pre-shared password used in Basic Auth process ' + 'against the http(s) server that hosts the disk image.' + 'This variable can be also configured via image_info.' + 'Value coming from image_info takes precedence over' + 'value coming from command line or configuration file.'), + cfg.StrOpt('image_server_auth_strategy', + default="noauth", + help='Option to select authentication strategy used during' + 'communication with the server that hosts the disk images' + 'and related checksums. This option also turns' + 'image_server_password and image_server_user' + 'into mandatory variables for those authentication' + 'strategies that require username + password credentials.' + 'This variable can be also configured via image_info.' + 'Value coming from image_info takes precedence over' + 'value coming from command line or configuration file.'), cfg.BoolOpt('agent_token_required', default=APARAMS.get('ipa-agent-token-required', False), help='Control to enforce if API command requests should ' diff --git a/ironic_python_agent/extensions/standby.py b/ironic_python_agent/extensions/standby.py index 90e0d1350..6cc2be740 100644 --- a/ironic_python_agent/extensions/standby.py +++ b/ironic_python_agent/extensions/standby.py @@ -47,6 +47,93 @@ def _image_location(image_info): return os.path.join(tempfile.gettempdir(), image_info['id']) +def _verify_basic_auth_creds(user, password, image_id): + """Verify the basic auth credentials used for image download are present. + + :param user: Basic auth username + :param password: Basic auth password + :param image_id: id of the image that is being acted upon + + :raises ImageDownloadError if the credentials are not present + """ + expected_creds = {'user': user, 'password': password} + missing_creds = [] + for key, value in expected_creds.items(): + if not value: + missing_creds.append(key) + if missing_creds: + raise errors.ImageDownloadError( + image_id, + "Missing {} fields from HTTP(S) " + "basic auth config".format(missing_creds) + ) + + +def _gen_auth_from_image_info_user_pass(image_info, image_id): + """This function is used to pass the credentials to the chosen + + credential verifier and in case the verification is successful + generate the compatible authentication object that will be used + with the request(s). This function handles the authentication object + generation for authentication strategies that are username+password + based. Credentials are collected via image_info. + + :param image_info: Image information dictionary. + :param image_id: id of the image that is being acted upon + + :return: Authentication object used directly by the request library + :rtype: requests.auth.HTTPBasicAuth + """ + image_server_user = None + image_server_password = None + + if image_info.get('image_server_auth_strategy') == 'http_basic': + image_server_user = image_info.get('image_server_user') + image_server_password = image_info.get('image_server_password') + _verify_basic_auth_creds( + image_server_user, + image_server_password, + image_id + ) + else: + return None + + return requests.auth.HTTPBasicAuth(image_server_user, + image_server_password) + + +def _gen_auth_from_oslo_conf_user_pass(image_id): + """This function is used to pass the credentials to the chosen + + credential verifier and in case the verification is successful + generate the compatible authentication object that will be used + with the request(s). This function handles the authentication object + generation for authentication strategies that are username+password + based. Credentials are collected from the oslo.config framework. + + :param image_id: id of the image that is being acted upon + + :return: Authentication object used directly by the request library + :rtype: requests.auth.HTTPBasicAuth + """ + + image_server_user = None + image_server_password = None + + if CONF.image_server_auth_strategy == 'http_basic': + _verify_basic_auth_creds( + CONF.image_server_user, + CONF.image_server_password, + image_id) + image_server_user = CONF.image_server_user + image_server_password = CONF.image_server_password + else: + return None + + return requests.auth.HTTPBasicAuth(image_server_user, + image_server_password) + + def _download_with_proxy(image_info, url, image_id): """Opens a download stream for the given URL. @@ -56,13 +143,31 @@ def _download_with_proxy(image_info, url, image_id): :raises: ImageDownloadError if the download stream was not started properly. + + :return: HTTP(s) server response for the image/hash download request + :rtype: requests.Response """ + no_proxy = image_info.get('no_proxy') if no_proxy: os.environ['no_proxy'] = no_proxy proxies = image_info.get('proxies', {}) verify, cert = utils.get_ssl_client_options(CONF) resp = None + image_download_attributes = { + "stream": True, + "proxies": proxies, + "verify": verify, + "cert": cert, + "timeout": CONF.image_download_connection_timeout + } + # NOTE(Adam) `image_info` is prioritized over `oslo.conf` for credential + # collection and auth strategy selection + auth_object = _gen_auth_from_image_info_user_pass(image_info, image_id) + if auth_object is None: + auth_object = _gen_auth_from_oslo_conf_user_pass(image_id) + if auth_object is not None: + image_download_attributes['auth'] = auth_object for attempt in range(CONF.image_download_connection_retries + 1): try: # NOTE(TheJulia) The get request below does the following: @@ -78,9 +183,8 @@ def _download_with_proxy(image_info, url, image_id): # exactly just as the timeout value exists. The risk in transitory # failure is more so once we've started the download and we are # processing the incoming data. - resp = requests.get(url, stream=True, proxies=proxies, - verify=verify, cert=cert, - timeout=CONF.image_download_connection_timeout) + # B113 issue is covered is the image_download_attributs list + resp = requests.get(url, **image_download_attributes) # nosec if resp.status_code != 200: msg = ('Received status code {} from {}, expected 200. ' 'Response body: {} Response headers: {}').format( diff --git a/ironic_python_agent/tests/unit/extensions/test_standby.py b/ironic_python_agent/tests/unit/extensions/test_standby.py index e0284ea1d..53c303630 100644 --- a/ironic_python_agent/tests/unit/extensions/test_standby.py +++ b/ironic_python_agent/tests/unit/extensions/test_standby.py @@ -204,6 +204,86 @@ class TestStandbyExtension(base.IronicAgentTest): expected_loc = os.path.join(tempfile.gettempdir(), 'fake_id') self.assertEqual(expected_loc, location) + def test_verify_basic_auth_creds(self): + image_info = _build_fake_image_info() + self.assertIsNone(standby._verify_basic_auth_creds("SpongeBob", + "SquarePants", + image_info['id'])) + + def test_gen_auth_from_image_info_user_pass_success(self): + image_info = _build_fake_image_info() + image_info['image_server_auth_strategy'] = 'http_basic' + image_info['image_server_user'] = 'SpongeBob' + image_info['image_server_password'] = 'SquarePants' + exp_auth = requests.auth.HTTPBasicAuth('SpongeBob', 'SquarePants') + return_auth = \ + standby._gen_auth_from_image_info_user_pass(image_info, + image_info['id']) + self.assertEqual(exp_auth, return_auth) + + def test_gen_auth_from_image_info_user_pass_none(self): + image_info = _build_fake_image_info() + image_info['image_server_auth_strategy'] = '' + image_info['image_server_user'] = 'SpongeBob' + image_info['image_server_password'] = 'SquarePants' + return_auth = \ + standby._gen_auth_from_image_info_user_pass(image_info, + image_info['id']) + self.assertIsNone(return_auth) + + def test_gen_auth_from_oslo_conf_user_pass_success(self): + image_info = _build_fake_image_info() + CONF.set_override('image_server_auth_strategy', 'http_basic') + CONF.set_override('image_server_password', 'SpongeBob') + CONF.set_override('image_server_user', 'SquarePants') + correct_auth = \ + requests.auth.HTTPBasicAuth(CONF['image_server_user'], + CONF['image_server_password']) + return_auth = \ + standby._gen_auth_from_oslo_conf_user_pass(image_info['id']) + self.assertEqual(correct_auth, return_auth) + + def test_gen_auth_from_oslo_conf_user_pass_none(self): + image_info = _build_fake_image_info() + CONF.set_override('image_server_auth_strategy', 'noauth') + CONF.set_override('image_server_password', 'SpongeBob') + CONF.set_override('image_server_user', 'SquarePants') + return_auth = \ + standby._gen_auth_from_oslo_conf_user_pass(image_info['id']) + self.assertIsNone(return_auth) + + def test_verify_basic_auth_creds_empty_user(self): + image_info = _build_fake_image_info() + self.assertRaises(errors.ImageDownloadError, + standby._verify_basic_auth_creds, + "", + "SquarePants", + image_info['id']) + + def test_verify_basic_auth_creds_empty_password(self): + image_info = _build_fake_image_info() + self.assertRaises(errors.ImageDownloadError, + standby._verify_basic_auth_creds, + "SpongeBob", + "", + image_info['id']) + + def test_verify_basic_auth_creds_none_user(self): + image_info = _build_fake_image_info() + self.assertRaises(errors.ImageDownloadError, + standby._verify_basic_auth_creds, + None, + "SquarePants", + image_info['id']) + + def test_verify_basic_auth_creds_none_password(self): + image_info = _build_fake_image_info() + self.assertRaises(errors.ImageDownloadError, + standby._verify_basic_auth_creds, + "SpongeBob", + None, + image_info['id']) + @mock.patch('ironic_lib.disk_utils.fix_gpt_partition', autospec=True) @mock.patch('ironic_lib.disk_utils.trigger_device_rescan', autospec=True) @mock.patch('ironic_lib.disk_utils.convert_image', autospec=True) @@ -504,6 +584,106 @@ class TestStandbyExtension(base.IronicAgentTest): standby._download_image, image_info) + @mock.patch('hashlib.new', autospec=True) + @mock.patch('builtins.open', autospec=True) + @mock.patch('requests.get', autospec=True) + def test_download_image_basic_auth_conf_success(self, requests_mock, + open_mock, hash_mock): + image_info = _build_fake_image_info() + CONF.set_override('image_server_auth_strategy', 'http_basic') + CONF.set_override('image_server_password', 'SpongeBob') + CONF.set_override('image_server_user', 'SquarePants') + user = CONF.image_server_user + password = CONF.image_server_password + correct_auth = requests.auth.HTTPBasicAuth(user, password) + response = requests_mock.return_value + response.status_code = 200 + response.iter_content.return_value = ['some', 'content'] + file_mock = mock.Mock() + open_mock.return_value.__enter__.return_value = file_mock + file_mock.read.return_value = None + hexdigest_mock = hash_mock.return_value.hexdigest + hexdigest_mock.return_value = image_info['os_hash_value'] + + standby._download_image(image_info) + requests_mock.assert_called_once_with(image_info['urls'][0], + cert=None, verify=True, + stream=True, proxies={}, + timeout=60, auth=correct_auth) + write = file_mock.write + write.assert_any_call('some') + write.assert_any_call('content') + self.assertEqual(2, write.call_count) + + @mock.patch('hashlib.new', autospec=True) + @mock.patch('builtins.open', autospec=True) + @mock.patch('requests.get', autospec=True) + def test_download_image_basic_auth_image_info_success(self, + requests_mock, + open_mock, + hash_mock): + image_info = _build_fake_image_info() + image_info['image_server_auth_strategy'] = 'http_basic' + image_info['image_server_password'] = 'SpongeBob' + image_info['image_server_user'] = 'SquarePants' + user = image_info['image_server_user'] + password = image_info['image_server_password'] + correct_auth = requests.auth.HTTPBasicAuth(user, password) + response = requests_mock.return_value + response.status_code = 200 + response.iter_content.return_value = ['some', 'content'] + file_mock = mock.Mock() + open_mock.return_value.__enter__.return_value = file_mock + file_mock.read.return_value = None + hexdigest_mock = hash_mock.return_value.hexdigest + hexdigest_mock.return_value = image_info['os_hash_value'] + + standby._download_image(image_info) + requests_mock.assert_called_once_with(image_info['urls'][0], + cert=None, verify=True, + stream=True, proxies={}, + timeout=60, auth=correct_auth) + write = file_mock.write + write.assert_any_call('some') + write.assert_any_call('content') + self.assertEqual(2, write.call_count) + + def test_download_image_bad_basic_auth_conf_credential(self): + self.config(image_download_connection_retry_interval=0) + image_info = _build_fake_image_info() + CONF.set_override('image_server_auth_strategy', 'http_basic') + self.assertRaises(errors.ImageDownloadError, + standby._download_image, + image_info) + + def test_download_image_bad_basic_auth_image_info_credential(self): + self.config(image_download_connection_retry_interval=0) + image_info = _build_fake_image_info() + image_info['image_server_auth_strategy'] = 'http_basic' + self.assertRaises(errors.ImageDownloadError, + standby._download_image, + image_info) + + def test_download_image_bad_basic_auth_mixed_credential(self): + self.config(image_download_connection_retry_interval=0) + image_info = _build_fake_image_info() + image_info['image_server_auth_strategy'] = 'http_basic' + CONF.set_override('image_server_password', 'SpongeBob') + CONF.set_override('image_server_user', 'SquarePants') + self.assertRaises(errors.ImageDownloadError, + standby._download_image, + image_info) + + def test_download_image_bad_basic_auth_mixed_credential_second(self): + self.config(image_download_connection_retry_interval=0) + image_info = _build_fake_image_info() + CONF.set_override('image_server_auth_strategy', 'http_basic') + image_info['image_server_password'] = 'SpongeBob' + image_info['image_server_user'] = 'SquarePants' + self.assertRaises(errors.ImageDownloadError, + standby._download_image, + image_info) + @mock.patch('hashlib.new', autospec=True) @mock.patch('builtins.open', autospec=True) @mock.patch('requests.get', autospec=True) diff --git a/releasenotes/notes/basic-auth-for-user-image-server-150835e7567444da.yaml b/releasenotes/notes/basic-auth-for-user-image-server-150835e7567444da.yaml new file mode 100644 index 000000000..67b0af20c --- /dev/null +++ b/releasenotes/notes/basic-auth-for-user-image-server-150835e7567444da.yaml @@ -0,0 +1,27 @@ +--- +features: + - | + Introducing basic authentication and configurable authentication strategy + support for image and image checksum download processes. This feature + introduces 3 new variables that could be set (either via oslo.config or + image_info) to select the authentication strategy an provide credentials + for HTTP(S) basic authentication. The 3 variables are structured in way + that 1 of them 'image_server_auth_strategy' (string) provides the ability + to select between authentication strategies by specifying the name of + the strategy. Currently the only supported authentication strategy is the + 'http-basic' which will make IPA use HTTP(S) basic authentication also + known as the 'RFC 7617' standard. The other 2 variables + 'image_server_password' and 'image_server_user' provide username and + password credentials for image download processes. The + 'image_server_password' and 'image_server_user' are not strategy specific + and could be reused for any username + password based authentication + strategy, but for the moment these 2 variables are only used for the + 'http-basic' strategy. 'image_server_basic_auth' not just enables the + feature but enforces checks on the values of the 2 related credentials. + When the 'http-basic' strategy is enabled for image server download + workflow the download logic will make sure to raise an exception in case + any of the credentials are None or an empty string. Values coming from + 'image_info' are prioritized over values coming from the 'oslo.config' + framework and the 2 different credential source can't be mixed. Passing 1 + or 2 out of the 3 from and source and the remaining values from an other + source will result in a exception.