From 76ef947c741652eb9a7dd5709f56cb50bfaf6f07 Mon Sep 17 00:00:00 2001 From: Nuwan Goonasekera <2070605+nuwang@users.noreply.github.com> Date: Thu, 17 Sep 2026 13:36:36 +0530 Subject: [PATCH] Honor falsy config values, and configured OpenStack credentials over the environment _get_config_value treated every falsy value as "not configured", so s3_validate_certs: False (and ec2_validate_certs) silently left certificate verification switched on, and a 0 for any numeric option was quietly swapped for its default. Only None and the empty string now count as unset - what an absent value looks like coming from YAML, a blank environment variable or a blank ini option - so False and 0 reach the SDK as configured. A misconfigured 0 therefore errors where it used to be masked. The OpenStack provider filled in every credential field from the OS_* environment whenever the config did not name it, and Keystone password authentication won when both a password and an application credential were present. A provider configured with only an application credential, running in a process that carried OS_USERNAME and OS_PASSWORD, therefore authenticated as that ambient identity rather than the credential it was given - in Galaxy, a user-defined store could end up acting as the server. Resolve the two credential sets separately: the environment completes only the set the config names, so a configured os_username with the password in OS_PASSWORD still works, and is consulted for both sets only when neither is configured. Both surfaced in review of galaxyproject/galaxy#23098. --- CHANGELOG.rst | 21 ++++ cloudbridge/base/provider.py | 26 +++-- cloudbridge/providers/openstack/provider.py | 31 ++++-- docs/topics/setup.rst | 115 +++++++++++--------- tests/test_cloud_helpers.py | 26 +++++ tests/test_openstack_credentials.py | 109 +++++++++++++++++++ 6 files changed, 263 insertions(+), 65 deletions(-) create mode 100644 tests/test_openstack_credentials.py diff --git a/CHANGELOG.rst b/CHANGELOG.rst index 518ce61b..65c7b14a 100644 --- a/CHANGELOG.rst +++ b/CHANGELOG.rst @@ -22,6 +22,27 @@ ``DuplicateResourceException``, so a retry never appends a second copy; and ``remove_metadata_item`` returns ``False`` when there was nothing to remove, as its callers always assumed. +* **A config value of ``False`` or ``0`` is now honored instead of being + replaced by the default.** ``_get_config_value`` treated every falsy value + as "not configured", so ``s3_validate_certs: False`` (and + ``ec2_validate_certs``) silently left certificate verification switched on, + and a ``0`` for any numeric option was quietly swapped for its default. + Only ``None`` and the empty string now count as unset - what an absent + value looks like coming from YAML, a blank environment variable or a blank + ini option. A value that was previously ignored for being falsy now takes + effect; a misconfigured ``0`` (say for ``multipart_max_concurrency``) + therefore errors where it used to be masked. +* **Explicitly configured OpenStack credentials take precedence over the + ``OS_*`` environment.** The provider filled in every credential field from + the environment whenever the config did not name it, and Keystone password + authentication was preferred when both a password and an application + credential were present. So a provider configured with only an application + credential, running in a process that carried ``OS_USERNAME`` and + ``OS_PASSWORD``, authenticated as that ambient identity rather than the + credential it was given. The two credential sets are now resolved + separately: the environment only completes the set the config names (a + configured ``os_username`` with the password in ``OS_PASSWORD`` still + works), and is consulted for both only when neither is configured. 4.4.1 - August 21, 2026 (sha 093ef669598d9f324be28d400a851396739cf1d8) ---------------------------------------------------------------------- diff --git a/cloudbridge/base/provider.py b/cloudbridge/base/provider.py index 38266d5c..98683f3e 100644 --- a/cloudbridge/base/provider.py +++ b/cloudbridge/base/provider.py @@ -193,6 +193,13 @@ def _get_config_value(self, key: str, """ A convenience method to extract a configuration value. + The config dict is consulted first, then an attribute of the same + name on the config object, then the ``[]`` section of + the cloudbridge ini file. A value is taken from the first source + that has it set; only ``None`` and the empty string count as unset, + so ``False`` and ``0`` are returned as configured rather than + replaced by the default. + :type key: str :param key: a field to look for in the ``self.config`` field @@ -204,12 +211,15 @@ def _get_config_value(self, key: str, """ log.debug("Getting config key %s, with supplied default value: %s", key, default_value) - value = default_value - if isinstance(self.config, dict) and self.config.get(key): - value = self.config.get(key, default_value) - elif hasattr(self.config, key) and getattr(self.config, key): - value = getattr(self.config, key) - elif (self._config_parser.has_option(self.PROVIDER_ID, key) and - self._config_parser.get(self.PROVIDER_ID, key)): + value = self.config.get(key) if isinstance(self.config, dict) else None + if not self._is_set(value): + value = getattr(self.config, key, None) + if (not self._is_set(value) and + self._config_parser.has_option(self.PROVIDER_ID, key)): value = self._config_parser.get(self.PROVIDER_ID, key) - return value + return value if self._is_set(value) else default_value + + @staticmethod + def _is_set(value: Any) -> bool: + """Whether a config value was supplied, as opposed to left blank.""" + return value is not None and value != '' diff --git a/cloudbridge/providers/openstack/provider.py b/cloudbridge/providers/openstack/provider.py index 43b94788..ab43da9c 100644 --- a/cloudbridge/providers/openstack/provider.py +++ b/cloudbridge/providers/openstack/provider.py @@ -44,14 +44,29 @@ def __init__(self, config: dict[str, Any]) -> None: super(OpenStackCloudProvider, self).__init__(config) # Initialize cloud connection fields - self.app_cred_id = self._get_config_value( - 'os_application_credential_id', get_env('OS_APPLICATION_CREDENTIAL_ID')) - self.app_cred_secret = self._get_config_value( - 'os_application_credential_secret', get_env('OS_APPLICATION_CREDENTIAL_SECRET')) - self.username = self._get_config_value( - 'os_username', get_env('OS_USERNAME')) - self.password = self._get_config_value( - 'os_password', get_env('OS_PASSWORD')) + # Credentials come from the config first and the OS_* environment + # second. The two credential sets are resolved separately: the + # environment only completes the set the config names, so a provider + # configured with an application credential never picks up the + # process's own OS_USERNAME/OS_PASSWORD and signs in as that identity + # instead (and vice versa). With neither set configured, both are read + # from the environment as before. + app_cred_id = self._get_config_value('os_application_credential_id') + app_cred_secret = self._get_config_value( + 'os_application_credential_secret') + username = self._get_config_value('os_username') + password = self._get_config_value('os_password') + env_app_cred = not (username or password) + env_password = not (app_cred_id or app_cred_secret) + self.app_cred_id = app_cred_id or ( + get_env('OS_APPLICATION_CREDENTIAL_ID') if env_app_cred else None) + self.app_cred_secret = app_cred_secret or ( + get_env('OS_APPLICATION_CREDENTIAL_SECRET') if env_app_cred + else None) + self.username = username or ( + get_env('OS_USERNAME') if env_password else None) + self.password = password or ( + get_env('OS_PASSWORD') if env_password else None) self.project_name = self._get_config_value( 'os_project_name', get_env('OS_PROJECT_NAME') or get_env('OS_TENANT_NAME')) diff --git a/docs/topics/setup.rst b/docs/topics/setup.rst index f1ee7d4b..abb12db1 100644 --- a/docs/topics/setup.rst +++ b/docs/topics/setup.rst @@ -16,8 +16,12 @@ available credentials in one of following ways: Providing access credentials through a dictionary ------------------------------------------------- You can initialize a simple config as follows. The key names are the same -as the environment variables, in lower case. Note that the config dictionary -will override environment values. +as the environment variables, in lower case. A value is looked up in the +config dictionary first, then in the `CloudBridge config file`_, and only +then in the environment; ``None`` and the empty string count as not set, so +``False`` and ``0`` are honored as configured. + +.. _CloudBridge config file: #providing-access-credentials-in-a-cloudbridge-config-file .. code-block:: python @@ -174,53 +178,60 @@ GCP OpenStack ~~~~~~~~~ -+-------------------------+--------------------------------------------------------------+ -| Variable | Description | -+=========================+==============================================================+ -| os_auth_url | Required. OpenStack authentication endpoint. | -| | eg: https://my-openstack.com:5000/v3 | -+-------------------------+--------------------------------------------------------------+ -| os_username | Required. Username for authentication. | -+-------------------------+--------------------------------------------------------------+ -| os_password | Required. password for authentication. | -+-------------------------+--------------------------------------------------------------+ -| os_project_name | Required. The project in which to manage resources. | -+-------------------------+--------------------------------------------------------------+ -| os_region_name | Required. Region in which to manage resources. | -+-------------------------+--------------------------------------------------------------+ -| os_zone_name | Default Availability Zone in which to manage resources. | -| | If not provided, will default to the first available zone | -| | in the region. This zone will be the default for all services| -| | unless overwritten by service-specific zone configs | -+-------------------------+--------------------------------------------------------------+ -| os_compute_zone_name | Default Availability Zone for Compute servies. | -| | If not provided, will default to `os_zone_name` | -+-------------------------+--------------------------------------------------------------+ -| os_networking_zone_name | Default Availability Zone for Networking servies. | -| | If not provided, will default to `os_zone_name` | -+-------------------------+--------------------------------------------------------------+ -| os_security_zone_name | Default Availability Zone for Security servies. | -| | If not provided, will default to `os_zone_name` | -+-------------------------+--------------------------------------------------------------+ -| os_storage_zone_name | Default Availability Zone for Storage servies. | -| | If not provided, will default to `os_zone_name` | -+-------------------------+--------------------------------------------------------------+ -| nova_service_name | Service name for the NOVA client. | -+-------------------------+--------------------------------------------------------------+ -| os_auth_token | Authentication token, if applicable. | -+-------------------------+--------------------------------------------------------------+ -| os_compute_api_version | Compute API version, if applicable. | -+-------------------------+--------------------------------------------------------------+ -| os_volume_api_version | Volume API version, if applicable. | -+-------------------------+--------------------------------------------------------------+ -| os_storage_url | Storage endpoint URL, if applicable | -+-------------------------+--------------------------------------------------------------+ -| os_project_domain_id | Project domain id for authentication. | -+-------------------------+--------------------------------------------------------------+ -| os_project_domain_name | Project domain name for authentication. | -+-------------------------+--------------------------------------------------------------+ -| os_user_domain_name | User domain name for authentication. | -+-------------------------+--------------------------------------------------------------+ ++----------------------------------+--------------------------------------------------------------+ +| Variable | Description | ++==================================+==============================================================+ +| os_auth_url | Required. OpenStack authentication endpoint. | +| | eg: https://my-openstack.com:5000/v3 | ++----------------------------------+--------------------------------------------------------------+ +| os_username | Username for password authentication. Required unless an | +| | application credential is given. | ++----------------------------------+--------------------------------------------------------------+ +| os_password | Password for password authentication. Required unless an | +| | application credential is given. | ++----------------------------------+--------------------------------------------------------------+ +| os_application_credential_id | Keystone application credential ID. Together with the secret,| +| | an alternative to a username and password. | ++----------------------------------+--------------------------------------------------------------+ +| os_application_credential_secret | Keystone application credential secret. | ++----------------------------------+--------------------------------------------------------------+ +| os_project_name | Required. The project in which to manage resources. | ++----------------------------------+--------------------------------------------------------------+ +| os_region_name | Required. Region in which to manage resources. | ++----------------------------------+--------------------------------------------------------------+ +| os_zone_name | Default Availability Zone in which to manage resources. | +| | If not provided, will default to the first available zone | +| | in the region. This zone will be the default for all services| +| | unless overwritten by service-specific zone configs | ++----------------------------------+--------------------------------------------------------------+ +| os_compute_zone_name | Default Availability Zone for Compute servies. | +| | If not provided, will default to `os_zone_name` | ++----------------------------------+--------------------------------------------------------------+ +| os_networking_zone_nam e | Default Availability Zone for Networking servies. | +| | If not provided, will default to `os_zone_name` | ++----------------------------------+--------------------------------------------------------------+ +| os_security_zone_name | Default Availability Zone for Security servies. | +| | If not provided, will default to `os_zone_name` | ++----------------------------------+--------------------------------------------------------------+ +| os_storage_zone_name | Default Availability Zone for Storage servies. | +| | If not provided, will default to `os_zone_name` | ++----------------------------------+--------------------------------------------------------------+ +| nova_service_name | Service name for the NOVA client. | ++----------------------------------+--------------------------------------------------------------+ +| os_auth_token | Authentication token, if applicable. | ++----------------------------------+--------------------------------------------------------------+ +| os_compute_api_version | Compute API version, if applicable. | ++----------------------------------+--------------------------------------------------------------+ +| os_volume_api_version | Volume API version, if applicable. | ++----------------------------------+--------------------------------------------------------------+ +| os_storage_url | Storage endpoint URL, if applicable | ++----------------------------------+--------------------------------------------------------------+ +| os_project_domain_id | Project domain id for authentication. | ++----------------------------------+--------------------------------------------------------------+ +| os_project_domain_name | Project domain name for authentication. | ++----------------------------------+--------------------------------------------------------------+ +| os_user_domain_name | User domain name for authentication. | ++----------------------------------+--------------------------------------------------------------+ Providing access credentials through environment variables ---------------------------------------------------------- @@ -345,6 +356,12 @@ OpenStack | OS_USER_DOMAIN_NAME | | +----------------------------------+-----------+ +``OS_USERNAME``/``OS_PASSWORD`` and ``OS_APPLICATION_CREDENTIAL_ID``/ +``OS_APPLICATION_CREDENTIAL_SECRET`` are alternatives. When a config +dictionary names either set, the environment only completes that set: a +provider configured with an application credential does not pick up an +``OS_USERNAME`` and ``OS_PASSWORD`` that happen to be in its environment. + Once the environment variables are set, you can create a connection as follows, replacing ``ProviderList.AWS`` with the desired provider (AZURE, GCP, or OPENSTACK): diff --git a/tests/test_cloud_helpers.py b/tests/test_cloud_helpers.py index 8f08553b..e1a872d0 100644 --- a/tests/test_cloud_helpers.py +++ b/tests/test_cloud_helpers.py @@ -98,3 +98,29 @@ def test_type_validation(self): int_value = self.provider._get_config_value( 'default_result_limit', None) self.assertIsInstance(int_value, int) + + def test_config_value_set_to_false_is_honored(self): + # A boolean option turned off must not be mistaken for an unset one: + # `s3_validate_certs: False` has to reach the SDK as False, not as + # the default of True. + self.provider.config['falsy_bool_check'] = False + # pylint:disable=protected-access + self.assertIs( + self.provider._get_config_value('falsy_bool_check', True), False) + + def test_config_value_set_to_zero_is_honored(self): + self.provider.config['falsy_int_check'] = 0 + # pylint:disable=protected-access + self.assertEqual( + self.provider._get_config_value('falsy_int_check', 4), 0) + + def test_config_value_none_or_blank_falls_back_to_default(self): + # None and the empty string are what an absent value looks like + # coming from YAML, a blank environment variable or a blank ini + # option, so those alone mean "not configured". + for unset in (None, ''): + self.provider.config['unset_check'] = unset + # pylint:disable=protected-access + self.assertEqual( + self.provider._get_config_value('unset_check', 'default'), + 'default', repr(unset)) diff --git a/tests/test_openstack_credentials.py b/tests/test_openstack_credentials.py new file mode 100644 index 00000000..f41f594f --- /dev/null +++ b/tests/test_openstack_credentials.py @@ -0,0 +1,109 @@ +"""Which OpenStack credentials a provider authenticates with. + +The provider reads credentials from its config dict and, failing that, from +the ``OS_*`` environment. These tests pin the precedence between the two: +credentials configured explicitly win over whatever happens to be in the +process environment, so a provider built for one identity never signs in as +another. Nothing here touches a network: the Keystone version probe is +patched and keystoneauth plugins are plain objects until used. +""" + +import os +import unittest +from unittest import mock + +from keystoneauth1.identity import v3 + +from cloudbridge.providers.openstack.provider import OpenStackCloudProvider + +AUTH_URL = 'https://keystone.example.org:5000/v3' + +PASSWORD_ENV = { + 'OS_USERNAME': 'ambient-user', + 'OS_PASSWORD': 'ambient-password', + 'OS_PROJECT_NAME': 'ambient-project', +} + +APP_CRED_ENV = { + 'OS_APPLICATION_CREDENTIAL_ID': 'ambient-app-cred-id', + 'OS_APPLICATION_CREDENTIAL_SECRET': 'ambient-app-cred-secret', +} + +ALL_CREDENTIAL_VARS = tuple(PASSWORD_ENV) + tuple(APP_CRED_ENV) + + +def _environment(**values): + """The process environment with only the given OS_* credentials set.""" + env = {k: v for k, v in os.environ.items() if k not in ALL_CREDENTIAL_VARS} + env.update(values) + return mock.patch.dict(os.environ, env, clear=True) + + +def _provider(**config): + # A configured zone keeps the compute service from asking Nova for one + # while the provider is being built. + return OpenStackCloudProvider( + dict(config, os_auth_url=AUTH_URL, os_zone_name='nova')) + + +def _keystone_auth(provider): + with mock.patch.object(OpenStackCloudProvider, '_keystone_version', + new_callable=mock.PropertyMock, return_value=3): + # pylint:disable=protected-access + return provider._keystone_session.auth + + +class OpenStackCredentialPrecedenceTestCase(unittest.TestCase): + + def test_configured_application_credential_ignores_ambient_password(self): + # The process may carry the server's own OS_USERNAME/OS_PASSWORD; a + # provider configured with an application credential must use that + # credential, not the ambient identity. + with _environment(**PASSWORD_ENV): + provider = _provider( + os_application_credential_id='configured-id', + os_application_credential_secret='configured-secret') + self.assertIsNone(provider.username) + self.assertIsNone(provider.password) + auth = _keystone_auth(provider) + self.assertIsInstance(auth, v3.ApplicationCredential) + self.assertEqual(auth.auth_methods[0].application_credential_id, + 'configured-id') + + def test_configured_password_ignores_ambient_application_credential(self): + with _environment(**APP_CRED_ENV): + provider = _provider(os_username='configured-user', + os_password='configured-password', + os_project_name='configured-project') + self.assertIsNone(provider.app_cred_id) + self.assertIsNone(provider.app_cred_secret) + auth = _keystone_auth(provider) + self.assertIsInstance(auth, v3.Password) + self.assertEqual(auth.auth_methods[0].username, 'configured-user') + + def test_environment_is_used_when_nothing_is_configured(self): + with _environment(**PASSWORD_ENV): + provider = _provider() + self.assertEqual(provider.username, 'ambient-user') + self.assertEqual(provider.password, 'ambient-password') + auth = _keystone_auth(provider) + self.assertIsInstance(auth, v3.Password) + + def test_environment_completes_a_partially_configured_credential(self): + # Keeping the secret out of the config file and in the environment is + # legitimate: the environment fills in the missing half of the set + # that is configured, and only that set. + with _environment(**PASSWORD_ENV, **APP_CRED_ENV): + provider = _provider(os_username='configured-user') + self.assertEqual(provider.username, 'configured-user') + self.assertEqual(provider.password, 'ambient-password') + self.assertIsNone(provider.app_cred_id) + self.assertIsNone(provider.app_cred_secret) + + with _environment(**PASSWORD_ENV, **APP_CRED_ENV): + provider = _provider(os_application_credential_id='configured-id') + self.assertEqual(provider.app_cred_id, 'configured-id') + self.assertEqual(provider.app_cred_secret, + 'ambient-app-cred-secret') + self.assertIsNone(provider.username) + self.assertIsNone(provider.password)