diff --git a/app/src/github_runner_image_builder/logging.py b/app/src/github_runner_image_builder/logging.py index 771da30e..b3fa134c 100644 --- a/app/src/github_runner_image_builder/logging.py +++ b/app/src/github_runner_image_builder/logging.py @@ -27,5 +27,4 @@ def configure(log_level: str | int) -> None: logging.basicConfig( level=log_level_normalized, handlers=(log_handler,), - encoding="utf-8", ) diff --git a/charmcraft.yaml b/charmcraft.yaml index 56824386..f166394e 100644 --- a/charmcraft.yaml +++ b/charmcraft.yaml @@ -114,6 +114,15 @@ config: The password section of the clouds.yaml contents, used to authenticate the OpenStack \ client (e.g. myverysecurepassword). See https://docs.openstack.org/python-openstackclient/\ queens/configuration/index.html for more information. + DEPRECATED: Use openstack-password-secret instead for better security. + openstack-password-secret: + type: secret + description: | + The password section of the clouds.yaml contents, used to authenticate the OpenStack + client. A Juju user secret ID should be passed in the format of secret:. + The secret must contain a 'password' key with the OpenStack password as its value. + Example: juju add-secret openstack-password password=. + This option takes precedence over openstack-password if both are set. openstack-project-domain-name: type: string default: "" diff --git a/docs/changelog.md b/docs/changelog.md index d7356b48..4d65d883 100644 --- a/docs/changelog.md +++ b/docs/changelog.md @@ -1,6 +1,11 @@ +## [#219 Use Juju secrets](https://github.com/canonical/github-runner-image-builder-operator/pull/219) (2026-04-17) +* Add new `openstack-password-secret` configuration option to securely store OpenStack passwords using Juju secrets. +* Deprecated `openstack-password` configuration option (still supported for backward compatibility). +* Users can migrate to the new secret-based approach by setting `openstack-password-secret` instead of `openstack-password`. + ## [#206 Add resolute image support] * Add resolute image support. diff --git a/src/state.py b/src/state.py index 506d68f2..cd11b230 100644 --- a/src/state.py +++ b/src/state.py @@ -36,6 +36,10 @@ OPENSTACK_AUTH_URL_CONFIG_NAME = "openstack-auth-url" # Bandit thinks this is a hardcoded password OPENSTACK_PASSWORD_CONFIG_NAME = "openstack-password" # nosec: hardcoded_password_string +# Bandit thinks this is a hardcoded password +OPENSTACK_PASSWORD_SECRET_CONFIG_NAME = ( + "openstack-password-secret" # nosec: hardcoded_password_string +) OPENSTACK_PROJECT_DOMAIN_CONFIG_NAME = "openstack-project-domain-name" OPENSTACK_PROJECT_CONFIG_NAME = "openstack-project-name" OPENSTACK_USER_DOMAIN_CONFIG_NAME = "openstack-user-domain-name" @@ -628,14 +632,46 @@ def _parse_openstack_clouds_config(charm: ops.CharmBase) -> OpenstackCloudsConfi The openstack clouds yaml. """ auth_url = typing.cast(str, charm.config.get(OPENSTACK_AUTH_URL_CONFIG_NAME)) + password_secret_id = typing.cast(str, charm.config.get(OPENSTACK_PASSWORD_SECRET_CONFIG_NAME)) password = typing.cast(str, charm.config.get(OPENSTACK_PASSWORD_CONFIG_NAME)) project_domain = typing.cast(str, charm.config.get(OPENSTACK_PROJECT_DOMAIN_CONFIG_NAME)) project = typing.cast(str, charm.config.get(OPENSTACK_PROJECT_CONFIG_NAME)) user_domain = typing.cast(str, charm.config.get(OPENSTACK_USER_DOMAIN_CONFIG_NAME)) user = typing.cast(str, charm.config.get(OPENSTACK_USER_CONFIG_NAME)) - if not all((auth_url, password, project_domain, project, user_domain, user)): + + # Check if we have the required configs, password can come from either source + if not all((auth_url, project_domain, project, user_domain, user)): raise InvalidCloudConfigError("Please supply all OpenStack configurations.") + # Prefer the secret-based password if provided + if password_secret_id: + if not password_secret_id.startswith("secret:"): + raise InvalidCloudConfigError( + f"Invalid value '{password_secret_id}' for openstack-password-secret. " + "Expected a Juju secret ID in the format 'secret:'." + ) + try: + secret = charm.model.get_secret(id=password_secret_id) + except ops.SecretNotFoundError as exc: + raise InvalidCloudConfigError( + f"OpenStack password secret not found: {password_secret_id}." + ) from exc + except ops.ModelError as exc: + raise InvalidCloudConfigError( + "Charm does not have access to the OpenStack password secret. " + "Please grant the charm read access to the secret." + ) from exc + secret_content = secret.get_content(refresh=True) + password = secret_content.get("password", "") + if not password: + raise InvalidCloudConfigError( + f"Secret {password_secret_id} does not contain a 'password' key." + ) + elif not password: + raise InvalidCloudConfigError( + "Please supply OpenStack password via openstack-password or openstack-password-secret." + ) + clouds_config = OpenstackCloudsConfig( clouds={ CLOUD_NAME: _CloudsConfig( diff --git a/tests/conftest.py b/tests/conftest.py index 96f5e0d2..5d157ff6 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -3,6 +3,8 @@ """Fixtures for github runner charm.""" +import os + from pytest import Parser @@ -29,52 +31,60 @@ def pytest_addoption(parser: Parser): "--openstack-network-name-amd64", action="store", help="The Openstack network to create testing instances under.", + default=os.getenv("OPENSTACK_NETWORK_NAME_AMD64"), ) parser.addoption( "--openstack-flavor-name-amd64", action="store", help="The Openstack flavor to create testing instances with.", + default=os.getenv("OPENSTACK_FLAVOR_NAME_AMD64"), ) parser.addoption( "--openstack-auth-url-amd64", action="store", help="The URL to Openstack authentication service, i.e. keystone.", + default=os.getenv("OPENSTACK_AUTH_URL_AMD64"), ) parser.addoption( "--openstack-project-domain-name-amd64", action="store", help="The Openstack project domain name to use.", + default=os.getenv("OPENSTACK_PROJECT_DOMAIN_NAME_AMD64"), ) parser.addoption( "--openstack-project-name-amd64", action="store", help="The Openstack project name to use.", + default=os.getenv("OPENSTACK_PROJECT_NAME_AMD64"), ) parser.addoption( "--openstack-user-domain-name-amd64", action="store", help="The Openstack user domain name to use.", + default=os.getenv("OPENSTACK_USER_DOMAIN_NAME_AMD64"), ) parser.addoption( "--openstack-username-amd64", action="store", help="The Openstack user to authenticate as.", + default=os.getenv("OPENSTACK_USERNAME_AMD64"), ) parser.addoption( "--openstack-region-name-amd64", action="store", help="The Openstack region to authenticate to.", + default=os.getenv("OPENSTACK_REGION_NAME_AMD64"), ) # Shared private endpoint options parser.addoption( "--proxy", action="store", help="The HTTP proxy URL to apply on the Openstack runners.", - default=None, + default=os.getenv("PROXY"), ) parser.addoption( "--no-proxy", action="store", help="The no proxy URL(s) to apply on the Openstack runners.", - default=None, + default=os.getenv("NO_PROXY"), ) diff --git a/tests/integration/conftest.py b/tests/integration/conftest.py index 1d130d93..450b4506 100644 --- a/tests/integration/conftest.py +++ b/tests/integration/conftest.py @@ -38,6 +38,7 @@ EXTERNAL_BUILD_NETWORK_CONFIG_NAME, OPENSTACK_AUTH_URL_CONFIG_NAME, OPENSTACK_PASSWORD_CONFIG_NAME, + OPENSTACK_PASSWORD_SECRET_CONFIG_NAME, OPENSTACK_PROJECT_CONFIG_NAME, OPENSTACK_PROJECT_DOMAIN_CONFIG_NAME, OPENSTACK_USER_CONFIG_NAME, @@ -332,12 +333,27 @@ async def script_secret_fixture(test_configs) -> _Secret: return _Secret(id=secret_id, name=secret_name) -@pytest.fixture(scope="module", name="app_config") -def app_config_fixture( +@pytest_asyncio.fixture(scope="module", name="openstack_password_secret") +async def openstack_password_secret_fixture( + test_configs: TestConfigs, + private_endpoint_configs: PrivateEndpointConfigs, +) -> _Secret: + """The OpenStack password Juju secret.""" + secret_name = f"openstack-password-{uuid4().hex}" + secret_id = await test_configs.model.add_secret( + name=secret_name, + data_args=[f"password={private_endpoint_configs['password']}"], + ) # note secret_id already contains "secret:" prefix + return _Secret(id=secret_id, name=secret_name) + + +@pytest_asyncio.fixture(scope="module", name="app_config") +async def app_config_fixture( private_endpoint_configs: PrivateEndpointConfigs, image_configs: ImageConfigs, openstack_metadata: OpenstackMeta, arch: state.Arch, + openstack_password_secret: _Secret, ) -> dict: """The image builder application config.""" return { @@ -346,7 +362,7 @@ def app_config_fixture( BUILD_INTERVAL_CONFIG_NAME: 12, REVISION_HISTORY_LIMIT_CONFIG_NAME: 5, OPENSTACK_AUTH_URL_CONFIG_NAME: private_endpoint_configs["auth_url"], - OPENSTACK_PASSWORD_CONFIG_NAME: private_endpoint_configs["password"], + OPENSTACK_PASSWORD_SECRET_CONFIG_NAME: openstack_password_secret.id, OPENSTACK_PROJECT_CONFIG_NAME: private_endpoint_configs["project_name"], OPENSTACK_PROJECT_DOMAIN_CONFIG_NAME: private_endpoint_configs["project_domain_name"], OPENSTACK_USER_CONFIG_NAME: private_endpoint_configs["username"], @@ -372,6 +388,7 @@ async def app_fixture( base_machine_constraint: str, test_configs: TestConfigs, script_secret: _Secret, + openstack_password_secret: _Secret, ) -> AsyncGenerator[Application, None]: """The deployed application fixture.""" logger.info("Deploying image builder: %s", test_configs.dispatch_time) @@ -381,6 +398,7 @@ async def app_fixture( constraints=base_machine_constraint, config=app_config, ) + await app.model.grant_secret(openstack_password_secret.name, app.name) await app.model.grant_secret(script_secret.name, app.name) await app.set_config( { @@ -398,23 +416,28 @@ async def app_fixture( await test_configs.model.remove_application(app_name=app.name) -@pytest_asyncio.fixture(scope="module", name="app_on_charmhub") -async def app_on_charmhub_fixture( - test_configs: TestConfigs, - app_config: dict, - base_machine_constraint: str, - ops_test, -) -> AsyncGenerator[Application, None]: - """Fixture for deploying the charm from charmhub.""" - # Normally we would use latest/stable, but upgrading - # from stable is currently broken, and therefore we are using edge. Change this in the future. +async def _prepare_charmhub_app_config( + ops_test, app_config: dict, openstack_password: str +) -> tuple[str, dict, set[str]]: + """Prepare the application config for charmhub deployment. + + Args: + ops_test: The pytest operator test instance. + app_config: The base application configuration. + openstack_password: The plaintext OpenStack password, used as a fallback when the + charmhub revision does not yet expose openstack-password-secret. + + Returns: + A tuple of (channel, prepared_config, config_options). + + """ charmhub_channel = "edge" ret_code, stdout, stderr = await ops_test.juju( "info", "--format", "json", "--channel", charmhub_channel, "github-runner-image-builder" ) assert ret_code == 0, f"Failed to get charm info: {stderr}" charmhub_info = json.loads(stdout.strip()) - charmhub_config_options = charmhub_info["charm"]["config"]["Options"].keys() + charmhub_config_options = set(charmhub_info["charm"]["config"]["Options"].keys()) charmhub_app_config = {k: v for k, v in app_config.items() if k in charmhub_config_options} # We might need to test using the legacy config options. @@ -422,14 +445,55 @@ async def app_on_charmhub_fixture( for opt in (EXTERNAL_BUILD_FLAVOR_CONFIG_NAME, EXTERNAL_BUILD_NETWORK_CONFIG_NAME): if (legacy_opt := f"{legacy_config_prefix}{opt}") in charmhub_config_options: charmhub_app_config[legacy_opt] = app_config[opt] + + # If the charmhub revision doesn't expose openstack-password-secret yet, fall back to the + # legacy openstack-password option so the charm has credentials during initial deployment. + if ( + OPENSTACK_PASSWORD_SECRET_CONFIG_NAME not in charmhub_config_options + and OPENSTACK_PASSWORD_CONFIG_NAME in charmhub_config_options + ): + charmhub_app_config[OPENSTACK_PASSWORD_CONFIG_NAME] = openstack_password + + return charmhub_channel, charmhub_app_config, charmhub_config_options + + +@pytest_asyncio.fixture(scope="module", name="app_on_charmhub") +async def app_on_charmhub_fixture( # pylint: disable=too-many-arguments,too-many-positional-arguments + test_configs: TestConfigs, + app_config: dict, + base_machine_constraint: str, + ops_test, + openstack_password_secret: _Secret, + private_endpoint_configs: PrivateEndpointConfigs, +) -> AsyncGenerator[Application, None]: + """Fixture for deploying the charm from charmhub.""" + # Normally we would use latest/stable, but upgrading + # from stable is currently broken, and therefore we are using edge. Change this in the future. + charmhub_channel, charmhub_app_config, charmhub_config_options = ( + await _prepare_charmhub_app_config( + ops_test, app_config, private_endpoint_configs["password"] + ) + ) + + # Deploy without the secret-backed config so the charm doesn't try to read the secret + # before the grant is in place. + deploy_config = { + k: v for k, v in charmhub_app_config.items() if k != OPENSTACK_PASSWORD_SECRET_CONFIG_NAME + } app: Application = await test_configs.model.deploy( "github-runner-image-builder", application_name=f"image-builder-operator-{test_configs.test_id}", constraints=base_machine_constraint, - config=charmhub_app_config, + config=deploy_config, channel=charmhub_channel, ) + if OPENSTACK_PASSWORD_SECRET_CONFIG_NAME in charmhub_config_options: + # Grant access first, then set the config to trigger a config-changed hook + # after the charm already has read permissions for the secret. + await app.model.grant_secret(openstack_password_secret.name, app.name) + await app.set_config({OPENSTACK_PASSWORD_SECRET_CONFIG_NAME: openstack_password_secret.id}) + await test_configs.model.wait_for_idle(apps=[app.name], idle_period=30, timeout=60 * 30) yield app diff --git a/tests/unit/factories.py b/tests/unit/factories.py index 153480a0..2a760a20 100644 --- a/tests/unit/factories.py +++ b/tests/unit/factories.py @@ -18,6 +18,7 @@ EXTERNAL_BUILD_NETWORK_CONFIG_NAME, OPENSTACK_AUTH_URL_CONFIG_NAME, OPENSTACK_PASSWORD_CONFIG_NAME, + OPENSTACK_PASSWORD_SECRET_CONFIG_NAME, OPENSTACK_PROJECT_CONFIG_NAME, OPENSTACK_PROJECT_DOMAIN_CONFIG_NAME, OPENSTACK_USER_CONFIG_NAME, @@ -76,6 +77,7 @@ class Meta: # pylint: disable=too-few-public-methods """Configuration for factory.""" # noqa: DCO060 model = MagicMock + exclude = ["_setup_mock_openstack_secret"] app = MockAppFactory() unit = MockUnitFactory() @@ -86,7 +88,8 @@ class Meta: # pylint: disable=too-few-public-methods EXTERNAL_BUILD_FLAVOR_CONFIG_NAME: "test-flavor", EXTERNAL_BUILD_NETWORK_CONFIG_NAME: "test-network", OPENSTACK_AUTH_URL_CONFIG_NAME: "http://testing-auth/keystone", - OPENSTACK_PASSWORD_CONFIG_NAME: "test-password", + OPENSTACK_PASSWORD_CONFIG_NAME: "", + OPENSTACK_PASSWORD_SECRET_CONFIG_NAME: "secret:test-secret-id", OPENSTACK_PROJECT_DOMAIN_CONFIG_NAME: "test-project-domain", OPENSTACK_PROJECT_CONFIG_NAME: "test-project-name", OPENSTACK_USER_DOMAIN_CONFIG_NAME: "test-user-domain", @@ -98,6 +101,16 @@ class Meta: # pylint: disable=too-few-public-methods } ) + @factory.post_generation + def _setup_mock_openstack_secret( # noqa: DCO020 + obj: MagicMock, create: bool, extracted: typing.Any, **kwargs: typing.Any + ) -> None: + mock_secret = MagicMock() + mock_secret.get_content.return_value = { + "password": "test-password" # nosec: hardcoded_password_string + } + obj.model.get_secret.return_value = mock_secret + class CloudAuthFactory(factory.DictFactory): """Mock cloud auth dict object factory.""" # noqa: DCO060 diff --git a/tests/unit/test_state.py b/tests/unit/test_state.py index e451ace4..6d0572ec 100644 --- a/tests/unit/test_state.py +++ b/tests/unit/test_state.py @@ -7,6 +7,7 @@ # pylint:disable=protected-access import os +import secrets from unittest.mock import MagicMock import ops @@ -374,7 +375,6 @@ def test__parse_runner_version(version: str, expected_version: str): "missing_config", [ pytest.param(state.OPENSTACK_AUTH_URL_CONFIG_NAME), - pytest.param(state.OPENSTACK_PASSWORD_CONFIG_NAME), pytest.param(state.OPENSTACK_PROJECT_DOMAIN_CONFIG_NAME), pytest.param(state.OPENSTACK_PROJECT_CONFIG_NAME), pytest.param(state.OPENSTACK_USER_DOMAIN_CONFIG_NAME), @@ -396,6 +396,7 @@ def test__parse_openstack_clouds_config_invalid(missing_config: str): assert "Please supply all OpenStack configurations." in str(exc) +@pytest.mark.usefixtures("patch_juju_version_33") def test__parse_openstack_clouds_config(): """ arrange: given mock charm from factory. @@ -422,6 +423,103 @@ def test__parse_openstack_clouds_config(): ) +@pytest.mark.parametrize( + "exception, expected_error", + [ + pytest.param( + ops.SecretNotFoundError, + "OpenStack password secret not found:", + id="secret-not-found", + ), + pytest.param( + ops.ModelError, + "Please grant the charm read access to the secret.", + id="access-denied", + ), + ], +) +@pytest.mark.usefixtures("patch_juju_version_33") +def test__parse_openstack_clouds_config_secret_error(exception: Exception, expected_error: str): + """ + arrange: given a mocked model get_secret method that raises a given error. + act: when _parse_openstack_clouds_config is called. + assert: InvalidCloudConfigError is raised. + """ + charm = factories.MockCharmFactory() + charm.model.get_secret = MagicMock(side_effect=exception) + + with pytest.raises(state.InvalidCloudConfigError) as exc: + state._parse_openstack_clouds_config(charm) + + assert expected_error in str(exc) + + +@pytest.mark.usefixtures("patch_juju_version_33") +def test__parse_openstack_clouds_config_missing_password_key(): + """ + arrange: given a secret that does not contain a 'password' key. + act: when _parse_openstack_clouds_config is called. + assert: InvalidCloudConfigError is raised. + """ + charm = factories.MockCharmFactory() + mock_secret = MagicMock() + mock_secret.get_content.return_value = {} + charm.model.get_secret.return_value = mock_secret + + with pytest.raises(state.InvalidCloudConfigError) as exc: + state._parse_openstack_clouds_config(charm) + + assert "does not contain a 'password' key" in str(exc) + + +def test__parse_openstack_clouds_config_legacy_password(): + """ + arrange: given a charm with the legacy openstack-password config (string). + act: when _parse_openstack_clouds_config is called. + assert: the clouds config is parsed correctly using the legacy password. + """ + charm = factories.MockCharmFactory() + test_password = secrets.token_hex(16) + charm.config[state.OPENSTACK_PASSWORD_CONFIG_NAME] = test_password + charm.config[state.OPENSTACK_PASSWORD_SECRET_CONFIG_NAME] = "" + + clouds_config = state._parse_openstack_clouds_config(charm) + + assert clouds_config.clouds[state.CLOUD_NAME].auth.password == test_password + + +def test__parse_openstack_clouds_config_no_password(): + """ + arrange: given a charm with neither password config set. + act: when _parse_openstack_clouds_config is called. + assert: InvalidCloudConfigError is raised. + """ + charm = factories.MockCharmFactory() + charm.config[state.OPENSTACK_PASSWORD_CONFIG_NAME] = "" + charm.config[state.OPENSTACK_PASSWORD_SECRET_CONFIG_NAME] = "" + + with pytest.raises(state.InvalidCloudConfigError) as exc: + state._parse_openstack_clouds_config(charm) + + assert "Please supply OpenStack password" in str(exc) + + +@pytest.mark.usefixtures("patch_juju_version_33") +def test__parse_openstack_clouds_config_invalid_secret_id_format(): + """ + arrange: given a charm with openstack-password-secret set to a non-secret-id value. + act: when _parse_openstack_clouds_config is called. + assert: InvalidCloudConfigError is raised with guidance on the expected format. + """ + charm = factories.MockCharmFactory() + charm.config[state.OPENSTACK_PASSWORD_SECRET_CONFIG_NAME] = "not-a-secret-id" + + with pytest.raises(state.InvalidCloudConfigError) as exc: + state._parse_openstack_clouds_config(charm) + + assert "secret:" in str(exc) + + # pylint: enable=undefined-variable,unused-variable @@ -625,3 +723,4 @@ def test__parse_script_secrets_from_config(secret: str, expected_secrets_map: di mock_charm.config = {state.SCRIPT_SECRET_CONFIG_NAME: secret} assert state._parse_script_secrets(charm=mock_charm) == expected_secrets_map + assert state._parse_script_secrets(charm=mock_charm) == expected_secrets_map diff --git a/tox.ini b/tox.ini index 7cda05cf..cdff0cc9 100644 --- a/tox.ini +++ b/tox.ini @@ -148,6 +148,16 @@ pass_env = PYTEST_ADDOPTS OPENSTACK_PASSWORD_AMD64 OPENSTACK_PASSWORD_ARM64 + OPENSTACK_FLAVOR_NAME_AMD64 + OPENSTACK_NETWORK_NAME_AMD64 + OPENSTACK_AUTH_URL_AMD64 + OPENSTACK_PROJECT_DOMAIN_NAME_AMD64 + OPENSTACK_PROJECT_NAME_AMD64 + OPENSTACK_USER_DOMAIN_NAME_AMD64 + OPENSTACK_USERNAME_AMD64 + OPENSTACK_REGION_NAME_AMD64 + PROXY + NO_PROXY deps = -r{[vars]tst_path}integration/requirements.txt -r{toxinidir}/requirements.txt