From 28a004b53d71615a9a674d78d1e027452ef798a5 Mon Sep 17 00:00:00 2001 From: Ian Duffy Date: Wed, 21 Oct 2026 17:25:57 +0100 Subject: [PATCH 01/16] refactor(no-ticket): read the keyring settings through Click The keyring module read CLOUDSMITH_NO_KEYRING, CLOUDSMITH_KEYRING_BACKEND, CLOUDSMITH_KEYRING_KEY, CLOUDSMITH_KEYRING_FILE_PATH and CLOUDSMITH_KEYRING_DIR with os.environ.get(). These settings have no flags, so Click did not know about them. - `keyring_options` on the `main` group adds hidden options that read the five variables. It gives the values to the keyring module through `keyring.configure(KeyringSettings(...))`. - The keyring module reads its settings from `configure()`, not from the environment. It still copies them to PYTHON_KEYRING_BACKEND and KEYRING_PROPERTY_*, because the keyring library reads only those. - The path settings are expanded and made absolute when Click parses them, not when the keyring uses them. - CLOUDSMITH_NO_KEYRING keeps its parsing: only 1, true and yes, in any case, turn off the keyring. Other values leave it on, as before. - A root conftest fixture resets the settings before each test, so tests no longer depend on the keyring variables of the developer shell. Co-Authored-By: Claude Opus 5.5 --- cloudsmith_cli/cli/commands/main.py | 1 + cloudsmith_cli/cli/decorators.py | 62 +++++- .../cli/tests/commands/test_domains.py | 3 +- .../cli/tests/test_credential_helper_cargo.py | 4 +- cloudsmith_cli/cli/tests/test_decorators.py | 35 ++++ cloudsmith_cli/conftest.py | 7 + cloudsmith_cli/core/keyring.py | 50 +++-- cloudsmith_cli/core/tests/test_keyring.py | 185 +++++------------- .../core/tests/test_keyring_provider.py | 8 +- 9 files changed, 198 insertions(+), 157 deletions(-) diff --git a/cloudsmith_cli/cli/commands/main.py b/cloudsmith_cli/cli/commands/main.py index 432432115..98f3e3914 100644 --- a/cloudsmith_cli/cli/commands/main.py +++ b/cloudsmith_cli/cli/commands/main.py @@ -64,6 +64,7 @@ def print_version(opts): envvar="CLOUDSMITH_NO_UPDATE_CHECK", help="Disable the once-a-day check for a newer version of the CLI.", ) +@decorators.keyring_options @decorators.common_cli_config_options @decorators.common_cli_output_options @click.pass_context diff --git a/cloudsmith_cli/cli/decorators.py b/cloudsmith_cli/cli/decorators.py index edc03abcb..9c45d4dc7 100644 --- a/cloudsmith_cli/cli/decorators.py +++ b/cloudsmith_cli/cli/decorators.py @@ -11,7 +11,7 @@ from cloudsmith_cli.cli import validators from cloudsmith_cli.core.utils import ColorMode, TTYMode, color_enabled -from ..core import update_check +from ..core import keyring, update_check from ..core.certificate_errors import describe_certificate_error from ..core.credentials.chain import CredentialProviderChain from ..core.credentials.models import CredentialContext @@ -118,6 +118,66 @@ def wrapper(ctx, *args, **kwargs): return wrapper +def _absolute_path(ctx, param, value): + """Expand ``~`` and environment variables in *value* and make it absolute.""" + if not value: + return value + return os.path.abspath(os.path.expanduser(os.path.expandvars(value))) + + +def keyring_options(f): + """Add hidden options that read the keyring settings from the environment.""" + + @click.option( + "--no-keyring-env", + "no_keyring", + envvar="CLOUDSMITH_NO_KEYRING", + hidden=True, + ) + @click.option( + "--keyring-backend-env", + "keyring_backend", + envvar="CLOUDSMITH_KEYRING_BACKEND", + hidden=True, + ) + @click.option( + "--keyring-key-env", + "keyring_key", + envvar="CLOUDSMITH_KEYRING_KEY", + hidden=True, + ) + @click.option( + "--keyring-file-path-env", + "keyring_file_path", + envvar="CLOUDSMITH_KEYRING_FILE_PATH", + callback=_absolute_path, + hidden=True, + ) + @click.option( + "--keyring-dir-env", + "keyring_dir", + envvar="CLOUDSMITH_KEYRING_DIR", + callback=_absolute_path, + hidden=True, + ) + @click.pass_context + @functools.wraps(f) + def wrapper(ctx, *args, **kwargs): + keyring.configure( + keyring.KeyringSettings( + no_keyring=(kwargs.pop("no_keyring") or "").strip().lower() + in ("1", "true", "yes"), + backend=kwargs.pop("keyring_backend"), + key=kwargs.pop("keyring_key"), + file_path=kwargs.pop("keyring_file_path"), + directory=kwargs.pop("keyring_dir"), + ) + ) + return ctx.invoke(f, *args, **kwargs) + + return wrapper + + def common_cli_config_options(f): """Add common CLI config options to commands.""" diff --git a/cloudsmith_cli/cli/tests/commands/test_domains.py b/cloudsmith_cli/cli/tests/commands/test_domains.py index 7585cf8b7..9322cfb21 100644 --- a/cloudsmith_cli/cli/tests/commands/test_domains.py +++ b/cloudsmith_cli/cli/tests/commands/test_domains.py @@ -8,6 +8,7 @@ from ....cli import config as cli_config from ....cli.commands.domains import list_domains +from ....core import keyring as core_keyring from ....core.api.exceptions import ApiException from ....credential_helpers.domains import ( domain_from_payload, @@ -35,7 +36,7 @@ def hermetic_environment(monkeypatch, tmp_path): monkeypatch.delenv("CLOUDSMITH_API_HOST", raising=False) monkeypatch.delenv("CLOUDSMITH_CONFIG_FILE", raising=False) monkeypatch.delenv("CLOUDSMITH_API_KEY", raising=False) - monkeypatch.setenv("CLOUDSMITH_NO_KEYRING", "1") + core_keyring.configure(core_keyring.KeyringSettings(no_keyring=True)) monkeypatch.delattr(cli_config.OPTIONS, "value", raising=False) monkeypatch.setattr(cli_config.ConfigReader, "config_files", ["config.ini"]) monkeypatch.setattr(cli_config.ConfigReader, "config_searchpath", ["."]) diff --git a/cloudsmith_cli/cli/tests/test_credential_helper_cargo.py b/cloudsmith_cli/cli/tests/test_credential_helper_cargo.py index 92162df0b..9c8693c05 100644 --- a/cloudsmith_cli/cli/tests/test_credential_helper_cargo.py +++ b/cloudsmith_cli/cli/tests/test_credential_helper_cargo.py @@ -11,6 +11,7 @@ import pytest import tomlkit +from ...core import keyring as core_keyring from ...core.credentials.models import CredentialResult from ...credential_helpers.backends import BackendKind from ...credential_helpers.cargo.installer import CargoInstaller @@ -355,6 +356,7 @@ def test_cli_accepts_the_cargo_plugin_flag_and_extra_provider_args(runner): def test_cli_exits_non_zero_with_a_hint_when_no_credential_resolves(runner): """A refused `get` still answers Cargo in-band, then exits 1 with a hint.""" + core_keyring.configure(core_keyring.KeyringSettings(no_keyring=True)) with patch( "cloudsmith_cli.credential_helpers.cargo.runtime.is_cloudsmith_domain", return_value=True, @@ -363,7 +365,7 @@ def test_cli_exits_non_zero_with_a_hint_when_no_credential_resolves(runner): cargo, args=[], input=json.dumps(_request()) + "\n", - env={"CLOUDSMITH_API_KEY": "", "CLOUDSMITH_NO_KEYRING": "1"}, + env={"CLOUDSMITH_API_KEY": ""}, catch_exceptions=False, ) diff --git a/cloudsmith_cli/cli/tests/test_decorators.py b/cloudsmith_cli/cli/tests/test_decorators.py index a7b4a13af..d6e3a780f 100644 --- a/cloudsmith_cli/cli/tests/test_decorators.py +++ b/cloudsmith_cli/cli/tests/test_decorators.py @@ -4,6 +4,7 @@ import click.testing import pytest +from ...core import keyring as core_keyring from ...core.mcp.server import create_ssl_context from ...core.session import create_requests_session from ...core.tests.test_certificate_errors import certificate_error @@ -12,6 +13,7 @@ from ..decorators import ( initialise_api, initialise_session, + keyring_options, report_retry, resolve_credentials, ) @@ -289,3 +291,36 @@ def leaf(opts): ) assert result.exit_code == 0, result.output assert seen == [PROXY] + + +def test_keyring_options_configure_the_keyring(monkeypatch, tmp_path): + """The keyring settings come from the environment, with absolute paths.""" + + @click.command() + @keyring_options + def command(): + pass + + monkeypatch.chdir(tmp_path) + monkeypatch.setenv("HOME", str(tmp_path / "home")) + monkeypatch.setenv("USERPROFILE", str(tmp_path / "home")) + monkeypatch.setenv("KEYRING_SUBDIR", "store") + result = click.testing.CliRunner().invoke( + command, + env={ + "CLOUDSMITH_NO_KEYRING": "yes", + "CLOUDSMITH_KEYRING_BACKEND": "keyring.backends.null.Keyring", + "CLOUDSMITH_KEYRING_KEY": "secret", + "CLOUDSMITH_KEYRING_FILE_PATH": "keyring.cfg", + "CLOUDSMITH_KEYRING_DIR": "~/$KEYRING_SUBDIR", + }, + ) + + assert result.exit_code == 0, result.output + assert core_keyring.current_settings() == core_keyring.KeyringSettings( + no_keyring=True, + backend="keyring.backends.null.Keyring", + key="secret", + file_path=str(tmp_path / "keyring.cfg"), + directory=str(tmp_path / "home" / "store"), + ) diff --git a/cloudsmith_cli/conftest.py b/cloudsmith_cli/conftest.py index ee7246677..5a7075828 100644 --- a/cloudsmith_cli/conftest.py +++ b/cloudsmith_cli/conftest.py @@ -1,6 +1,7 @@ import cloudsmith_api import pytest +from cloudsmith_cli.core import keyring as core_keyring from cloudsmith_cli.core import session, telemetry #: A syntactically valid DSN that is never contacted: tests that report use @@ -19,6 +20,12 @@ def no_real_error_reports(monkeypatch): monkeypatch.setattr(telemetry, "_transport", None) +@pytest.fixture(autouse=True) +def default_keyring_settings(): + """Start each test with the default keyring settings.""" + core_keyring.configure(core_keyring.KeyringSettings()) + + @pytest.fixture() def telemetry_config_dir(monkeypatch, tmp_path_factory): """An empty config search path, isolated from the developer's config.ini. diff --git a/cloudsmith_cli/core/keyring.py b/cloudsmith_cli/core/keyring.py index 55bb91828..d2c109e29 100644 --- a/cloudsmith_cli/core/keyring.py +++ b/cloudsmith_cli/core/keyring.py @@ -1,3 +1,4 @@ +import dataclasses import functools import getpass import os @@ -9,10 +10,37 @@ ACCESS_TOKEN_KEY = "cloudsmith_cli-access_token-{api_host}" +@dataclasses.dataclass(frozen=True) +class KeyringSettings: + """The CLOUDSMITH_*KEYRING* settings that the CLI parsed.""" + + no_keyring: bool = False + backend: str | None = None + key: str | None = None + file_path: str | None = None + directory: str | None = None + + +class _Current: + """The keyring settings of this process.""" + + settings = KeyringSettings() + + +def configure(settings): + """Use *settings* for the keyring in this process.""" + _Current.settings = settings + _prepare_keyring_backend.cache_clear() + + +def current_settings(): + """Return the keyring settings of this process.""" + return _Current.settings + + def should_use_keyring(): - """Check if keyring should be used based on CLOUDSMITH_NO_KEYRING env var.""" - env_value = os.environ.get("CLOUDSMITH_NO_KEYRING", "").strip().lower() - return env_value not in ("1", "true", "yes") + """Check if keyring should be used based on CLOUDSMITH_NO_KEYRING.""" + return not _Current.settings.no_keyring ACCESS_TOKEN_REFRESH_ATTEMPTED_AT_KEY = ( @@ -52,27 +80,23 @@ def _get_value_with_fallback(template, api_host, profile): def _sync_keyring_backend_env(): """Allow CLOUDSMITH_KEYRING_BACKEND to alias PYTHON_KEYRING_BACKEND.""" - alias_value = os.environ.get("CLOUDSMITH_KEYRING_BACKEND") + alias_value = _Current.settings.backend if alias_value and "PYTHON_KEYRING_BACKEND" not in os.environ: os.environ["PYTHON_KEYRING_BACKEND"] = alias_value def _sync_keyring_property_env(): """Allow CLOUDSMITH_KEYRING_KEY to alias KEYRING_PROPERTY_KEYRING_KEY.""" - alias_value = os.environ.get("CLOUDSMITH_KEYRING_KEY") + alias_value = _Current.settings.key if alias_value and "KEYRING_PROPERTY_KEYRING_KEY" not in os.environ: os.environ["KEYRING_PROPERTY_KEYRING_KEY"] = alias_value -def _expand_path(value): - return os.path.expanduser(os.path.expandvars(value)) - - def _sync_keyring_file_path_env(): """Allow CLOUDSMITH_KEYRING_FILE_PATH to alias KEYRING_PROPERTY_FILE_PATH.""" - alias_value = os.environ.get("CLOUDSMITH_KEYRING_FILE_PATH") + alias_value = _Current.settings.file_path if alias_value and "KEYRING_PROPERTY_FILE_PATH" not in os.environ: - os.environ["KEYRING_PROPERTY_FILE_PATH"] = _expand_path(alias_value) + os.environ["KEYRING_PROPERTY_FILE_PATH"] = alias_value def _apply_keyring_file_location(backend): @@ -89,13 +113,13 @@ def _apply_keyring_file_location(backend): if file_path: backend.file_path = file_path return - dir_value = os.environ.get("CLOUDSMITH_KEYRING_DIR") + dir_value = _Current.settings.directory if not dir_value: return filename = getattr(backend, "filename", None) if not filename: return - backend.file_path = os.path.join(_expand_path(dir_value), filename) + backend.file_path = os.path.join(dir_value, filename) def _refuse_password_prompt(): diff --git a/cloudsmith_cli/core/tests/test_keyring.py b/cloudsmith_cli/core/tests/test_keyring.py index c04159548..d2d031d61 100644 --- a/cloudsmith_cli/core/tests/test_keyring.py +++ b/cloudsmith_cli/core/tests/test_keyring.py @@ -14,6 +14,7 @@ from .. import keyring as core_keyring from ..keyring import ( + KeyringSettings, delete_oidc_token, delete_sso_tokens, get_access_token, @@ -246,11 +247,7 @@ def test_get_refresh_token_when_error_raised( @freeze_time("2024-06-01 10:00:00") def test_store_sso_tokens(self, mock_get_user, mock_set_password): - # Ensure keyring is enabled - env = os.environ.copy() - env.pop("CLOUDSMITH_NO_KEYRING", None) - with patch.dict(os.environ, env, clear=True): - result = store_sso_tokens(self.api_host, "access_token", "refresh_token") + result = store_sso_tokens(self.api_host, "access_token", "refresh_token") assert result is True refresh_key = ( @@ -274,8 +271,8 @@ def test_store_sso_tokens_returns_false_when_keyring_disabled( self, mock_get_user, mock_set_password ): """Verify store_sso_tokens returns False when keyring disabled.""" - with patch.dict(os.environ, {"CLOUDSMITH_NO_KEYRING": "1"}): - result = store_sso_tokens(self.api_host, "access_token", "refresh_token") + core_keyring.configure(KeyringSettings(no_keyring=True)) + result = store_sso_tokens(self.api_host, "access_token", "refresh_token") assert result is False mock_set_password.assert_not_called() @@ -326,12 +323,9 @@ def test_get_refresh_token_with_profile(self, mock_get_user, mock_get_password): @freeze_time("2024-06-01 10:00:00") def test_store_sso_tokens_with_profile(self, mock_get_user, mock_set_password): - env = os.environ.copy() - env.pop("CLOUDSMITH_NO_KEYRING", None) - with patch.dict(os.environ, env, clear=True): - result = store_sso_tokens( - self.api_host, "access_token", "refresh_token", profile="staging" - ) + result = store_sso_tokens( + self.api_host, "access_token", "refresh_token", profile="staging" + ) assert result is True assert mock_set_password.call_count == 3 @@ -588,25 +582,11 @@ class TestShouldUseKeyring: """Tests for the should_use_keyring function.""" def test_returns_true_by_default(self): - """When env var is not set, keyring should be used.""" - env = os.environ.copy() - env.pop("CLOUDSMITH_NO_KEYRING", None) - with patch.dict(os.environ, env, clear=True): - assert should_use_keyring() is True - - @pytest.mark.parametrize( - "env_value", ["1", "true", "True", "TRUE", "yes", "Yes", "YES"] - ) - def test_returns_false_when_env_var_is_truthy(self, env_value): - """Keyring should not be used when CLOUDSMITH_NO_KEYRING is truthy.""" - with patch.dict(os.environ, {"CLOUDSMITH_NO_KEYRING": env_value}): - assert should_use_keyring() is False + assert should_use_keyring() is True - @pytest.mark.parametrize("env_value", ["0", "false", "False", "no", "No", ""]) - def test_returns_true_when_env_var_is_falsy(self, env_value): - """Keyring should be used when CLOUDSMITH_NO_KEYRING is falsy.""" - with patch.dict(os.environ, {"CLOUDSMITH_NO_KEYRING": env_value}): - assert should_use_keyring() is True + def test_returns_false_when_disabled(self): + core_keyring.configure(KeyringSettings(no_keyring=True)) + assert should_use_keyring() is False class TestKeyringBackendAlias: @@ -617,9 +597,9 @@ class TestKeyringBackendAlias: def test_sets_python_keyring_backend_when_unset( self, mock_get_user, mock_get_password ): + core_keyring.configure(KeyringSettings(backend="keyring.backends.null.Keyring")) env = os.environ.copy() env.pop("PYTHON_KEYRING_BACKEND", None) - env["CLOUDSMITH_KEYRING_BACKEND"] = "keyring.backends.null.Keyring" with patch.dict(os.environ, env, clear=True): get_access_token(self.api_host) assert ( @@ -629,9 +609,9 @@ def test_sets_python_keyring_backend_when_unset( def test_does_not_override_existing_python_keyring_backend( self, mock_get_user, mock_get_password ): + core_keyring.configure(KeyringSettings(backend="keyring.backends.null.Keyring")) env = os.environ.copy() env["PYTHON_KEYRING_BACKEND"] = "keyring.backends.SecretService.Keyring" - env["CLOUDSMITH_KEYRING_BACKEND"] = "keyring.backends.null.Keyring" with patch.dict(os.environ, env, clear=True): get_access_token(self.api_host) assert ( @@ -642,7 +622,6 @@ def test_does_not_override_existing_python_keyring_backend( def test_no_op_when_alias_not_set(self, mock_get_user, mock_get_password): env = os.environ.copy() env.pop("PYTHON_KEYRING_BACKEND", None) - env.pop("CLOUDSMITH_KEYRING_BACKEND", None) with patch.dict(os.environ, env, clear=True): get_access_token(self.api_host) assert "PYTHON_KEYRING_BACKEND" not in os.environ @@ -663,9 +642,9 @@ class TestKeyringPropertyAlias: def test_sets_keyring_property_when_unset( self, mock_get_user, mock_get_password, mock_get_keyring ): + core_keyring.configure(KeyringSettings(key="super-secret")) env = os.environ.copy() env.pop("KEYRING_PROPERTY_KEYRING_KEY", None) - env["CLOUDSMITH_KEYRING_KEY"] = "super-secret" with patch.dict(os.environ, env, clear=True): get_access_token(self.api_host) assert os.environ["KEYRING_PROPERTY_KEYRING_KEY"] == "super-secret" @@ -673,9 +652,9 @@ def test_sets_keyring_property_when_unset( def test_does_not_override_existing_keyring_property( self, mock_get_user, mock_get_password, mock_get_keyring ): + core_keyring.configure(KeyringSettings(key="alias-secret")) env = os.environ.copy() env["KEYRING_PROPERTY_KEYRING_KEY"] = "native-secret" - env["CLOUDSMITH_KEYRING_KEY"] = "alias-secret" with patch.dict(os.environ, env, clear=True): get_access_token(self.api_host) assert os.environ["KEYRING_PROPERTY_KEYRING_KEY"] == "native-secret" @@ -685,7 +664,6 @@ def test_no_op_when_alias_not_set( ): env = os.environ.copy() env.pop("KEYRING_PROPERTY_KEYRING_KEY", None) - env.pop("CLOUDSMITH_KEYRING_KEY", None) with patch.dict(os.environ, env, clear=True): get_access_token(self.api_host) assert "KEYRING_PROPERTY_KEYRING_KEY" not in os.environ @@ -703,9 +681,9 @@ class TestKeyringFilePathAlias: api_host = "https://example.com" def test_sets_keyring_property_when_unset(self, mock_get_user, mock_get_password): + core_keyring.configure(KeyringSettings(file_path="/secure/path/keyring.cfg")) env = os.environ.copy() env.pop("KEYRING_PROPERTY_FILE_PATH", None) - env["CLOUDSMITH_KEYRING_FILE_PATH"] = "/secure/path/keyring.cfg" with patch.dict(os.environ, env, clear=True): get_access_token(self.api_host) assert ( @@ -715,9 +693,9 @@ def test_sets_keyring_property_when_unset(self, mock_get_user, mock_get_password def test_does_not_override_existing_keyring_property( self, mock_get_user, mock_get_password ): + core_keyring.configure(KeyringSettings(file_path="/alias/path/keyring.cfg")) env = os.environ.copy() env["KEYRING_PROPERTY_FILE_PATH"] = "/native/path/keyring.cfg" - env["CLOUDSMITH_KEYRING_FILE_PATH"] = "/alias/path/keyring.cfg" with patch.dict(os.environ, env, clear=True): get_access_token(self.api_host) assert ( @@ -727,41 +705,10 @@ def test_does_not_override_existing_keyring_property( def test_no_op_when_alias_not_set(self, mock_get_user, mock_get_password): env = os.environ.copy() env.pop("KEYRING_PROPERTY_FILE_PATH", None) - env.pop("CLOUDSMITH_KEYRING_FILE_PATH", None) with patch.dict(os.environ, env, clear=True): get_access_token(self.api_host) assert "KEYRING_PROPERTY_FILE_PATH" not in os.environ - def test_expands_user_and_env_vars(self, mock_get_user, mock_get_password): - env = os.environ.copy() - env.pop("KEYRING_PROPERTY_FILE_PATH", None) - env["KEYRING_STORE_ROOT"] = "/secure" - env["CLOUDSMITH_KEYRING_FILE_PATH"] = "$KEYRING_STORE_ROOT/keyring.cfg" - with patch.dict(os.environ, env, clear=True): - get_access_token(self.api_host) - assert os.environ["KEYRING_PROPERTY_FILE_PATH"] == "/secure/keyring.cfg" - - def test_expands_home_directory(self, mock_get_user, mock_get_password): - env = os.environ.copy() - env.pop("KEYRING_PROPERTY_FILE_PATH", None) - env["CLOUDSMITH_KEYRING_FILE_PATH"] = "~/keyring.cfg" - with patch.dict(os.environ, env, clear=True): - get_access_token(self.api_host) - assert os.environ["KEYRING_PROPERTY_FILE_PATH"] == os.path.expanduser( - "~/keyring.cfg" - ) - - def test_expands_home_inside_env_var_value(self, mock_get_user, mock_get_password): - env = os.environ.copy() - env.pop("KEYRING_PROPERTY_FILE_PATH", None) - env["KEYRING_STORE_ROOT"] = "~" - env["CLOUDSMITH_KEYRING_FILE_PATH"] = "$KEYRING_STORE_ROOT/keyring.cfg" - with patch.dict(os.environ, env, clear=True): - get_access_token(self.api_host) - assert os.environ["KEYRING_PROPERTY_FILE_PATH"] == os.path.expanduser( - "~/keyring.cfg" - ) - class TestKeyringDirEnv: """Tests for CLOUDSMITH_KEYRING_DIR redirecting file-backed backends.""" @@ -773,9 +720,9 @@ def test_sets_backend_file_path( ): backend = mock_get_keyring.return_value backend.filename = "cryptfile_pass.cfg" + core_keyring.configure(KeyringSettings(directory="/secure/dir")) env = os.environ.copy() env.pop("KEYRING_PROPERTY_FILE_PATH", None) - env["CLOUDSMITH_KEYRING_DIR"] = "/secure/dir" with patch.dict(os.environ, env, clear=True): get_access_token(self.api_host) assert backend.file_path == os.path.join( @@ -787,9 +734,9 @@ def test_native_file_path_wins_over_dir( ): backend = mock_get_keyring.return_value backend.filename = "cryptfile_pass.cfg" + core_keyring.configure(KeyringSettings(directory="/secure/dir")) env = os.environ.copy() env["KEYRING_PROPERTY_FILE_PATH"] = "/native/path/keyring.cfg" - env["CLOUDSMITH_KEYRING_DIR"] = "/secure/dir" with patch.dict(os.environ, env, clear=True): get_access_token(self.api_host) assert backend.file_path == "/native/path/keyring.cfg" @@ -799,10 +746,13 @@ def test_alias_file_path_wins_over_dir( ): backend = mock_get_keyring.return_value backend.filename = "cryptfile_pass.cfg" + core_keyring.configure( + KeyringSettings( + file_path="/alias/path/keyring.cfg", directory="/secure/dir" + ) + ) env = os.environ.copy() env.pop("KEYRING_PROPERTY_FILE_PATH", None) - env["CLOUDSMITH_KEYRING_FILE_PATH"] = "/alias/path/keyring.cfg" - env["CLOUDSMITH_KEYRING_DIR"] = "/secure/dir" with patch.dict(os.environ, env, clear=True): get_access_token(self.api_host) assert backend.file_path == "/alias/path/keyring.cfg" @@ -813,9 +763,9 @@ def test_no_op_for_backends_without_filename( backend = mock_get_keyring.return_value del backend.filename backend.file_path = "untouched" + core_keyring.configure(KeyringSettings(directory="/secure/dir")) env = os.environ.copy() env.pop("KEYRING_PROPERTY_FILE_PATH", None) - env["CLOUDSMITH_KEYRING_DIR"] = "/secure/dir" with patch.dict(os.environ, env, clear=True): get_access_token(self.api_host) assert backend.file_path == "untouched" @@ -828,40 +778,10 @@ def test_no_op_when_dir_not_set( backend.file_path = "untouched" env = os.environ.copy() env.pop("KEYRING_PROPERTY_FILE_PATH", None) - env.pop("CLOUDSMITH_KEYRING_DIR", None) with patch.dict(os.environ, env, clear=True): get_access_token(self.api_host) assert backend.file_path == "untouched" - def test_expands_user_and_env_vars( - self, mock_get_user, mock_get_password, mock_get_keyring - ): - backend = mock_get_keyring.return_value - backend.filename = "keyring_pass.cfg" - env = os.environ.copy() - env.pop("KEYRING_PROPERTY_FILE_PATH", None) - env["KEYRING_STORE_ROOT"] = "/secure" - env["CLOUDSMITH_KEYRING_DIR"] = "$KEYRING_STORE_ROOT/store" - with patch.dict(os.environ, env, clear=True): - get_access_token(self.api_host) - assert backend.file_path == os.path.join( - "/secure/store", "keyring_pass.cfg" - ) - - def test_expands_home_directory( - self, mock_get_user, mock_get_password, mock_get_keyring - ): - backend = mock_get_keyring.return_value - backend.filename = "keyring_pass.cfg" - env = os.environ.copy() - env.pop("KEYRING_PROPERTY_FILE_PATH", None) - env["CLOUDSMITH_KEYRING_DIR"] = "~/store" - with patch.dict(os.environ, env, clear=True): - get_access_token(self.api_host) - assert backend.file_path == os.path.join( - os.path.expanduser("~/store"), "keyring_pass.cfg" - ) - class TestKeyringFileRelocation: """Tests for file relocation with a real cryptfile backend.""" @@ -871,16 +791,9 @@ class TestKeyringFileRelocation: @staticmethod def _relocation_env(tmp_path): env = os.environ.copy() - for var in ( - "KEYRING_PROPERTY_FILE_PATH", - "KEYRING_PROPERTY_KEYRING_KEY", - "CLOUDSMITH_KEYRING_FILE_PATH", - "CLOUDSMITH_KEYRING_DIR", - "CLOUDSMITH_KEYRING_KEY", - ): + for var in ("KEYRING_PROPERTY_FILE_PATH", "KEYRING_PROPERTY_KEYRING_KEY"): env.pop(var, None) env["XDG_DATA_HOME"] = str(tmp_path / "default") - env["CLOUDSMITH_KEYRING_KEY"] = "test-password" return env def _roundtrip_token(self, backend, env): @@ -899,10 +812,11 @@ def test_dir_relocates_storage_before_unlock( ): backend = CryptFileKeyring() mock_get_keyring.return_value = backend - env = self._relocation_env(tmp_path) - env["CLOUDSMITH_KEYRING_DIR"] = str(tmp_path / "secure") + core_keyring.configure( + KeyringSettings(key="test-password", directory=str(tmp_path / "secure")) + ) - self._roundtrip_token(backend, env) + self._roundtrip_token(backend, self._relocation_env(tmp_path)) assert (tmp_path / "secure" / backend.filename).is_file() assert not (tmp_path / "default").exists() @@ -913,10 +827,11 @@ def test_file_path_alias_relocates_storage_before_unlock( backend = CryptFileKeyring() mock_get_keyring.return_value = backend target = tmp_path / "secure" / "tokens.cfg" - env = self._relocation_env(tmp_path) - env["CLOUDSMITH_KEYRING_FILE_PATH"] = str(target) + core_keyring.configure( + KeyringSettings(key="test-password", file_path=str(target)) + ) - self._roundtrip_token(backend, env) + self._roundtrip_token(backend, self._relocation_env(tmp_path)) assert target.is_file() assert not (tmp_path / "default").exists() @@ -963,16 +878,16 @@ def test_has_sso_tokens_returns_false_when_keyring_disabled( ): """has_sso_tokens should short-circuit when keyring is disabled.""" mock_get_password.return_value = "some_token" - with patch.dict(os.environ, {"CLOUDSMITH_NO_KEYRING": "1"}): - assert has_sso_tokens(self.api_host) is False + core_keyring.configure(KeyringSettings(no_keyring=True)) + assert has_sso_tokens(self.api_host) is False mock_get_password.assert_not_called() OIDC_ARGS = ("https://example.com", "my-org", "my-service") -def _keyring_env(tmp_path, **overrides): - return {"XDG_DATA_HOME": str(tmp_path), **overrides} +def _keyring_env(tmp_path): + return {"XDG_DATA_HOME": str(tmp_path)} @contextmanager @@ -1031,24 +946,24 @@ def test_store_without_terminal_skips_keyring( def test_supplied_key_uses_keyring_without_terminal( self, tmp_path, mock_get_user, backend_cls ): - env = _keyring_env(tmp_path, CLOUDSMITH_KEYRING_KEY="supplied-password") - with _keyring_session(backend_cls(), env): + core_keyring.configure(KeyringSettings(key="supplied-password")) + with _keyring_session(backend_cls(), _keyring_env(tmp_path)): assert store_oidc_token(*OIDC_ARGS, "token-data") is True assert get_oidc_token(*OIDC_ARGS) == "token-data" def test_wrong_key_reports_not_found(self, tmp_path, mock_get_user): _write_oidc_entry(CryptFileKeyring(), tmp_path) - env = _keyring_env(tmp_path, CLOUDSMITH_KEYRING_KEY="wrong-password") - with _keyring_session(CryptFileKeyring(), env): + core_keyring.configure(KeyringSettings(key="wrong-password")) + with _keyring_session(CryptFileKeyring(), _keyring_env(tmp_path)): assert get_oidc_token(*OIDC_ARGS) is None assert delete_oidc_token(*OIDC_ARGS) is False def test_key_unlocks_once_per_process(self, tmp_path, mock_get_user): _write_oidc_entry(CryptFileKeyring(), tmp_path) backend = CryptFileKeyring() - env = _keyring_env(tmp_path, CLOUDSMITH_KEYRING_KEY="earlier-password") + core_keyring.configure(KeyringSettings(key="earlier-password")) with ( - _keyring_session(backend, env), + _keyring_session(backend, _keyring_env(tmp_path)), patch.object(backend, "_unlock", wraps=backend._unlock) as unlock_mock, ): store_oidc_token(*OIDC_ARGS, "token-data") @@ -1074,12 +989,10 @@ def test_settings_reach_the_storage_member(self, tmp_path, mock_get_user): _write_oidc_entry(AltEncryptedKeyring(), tmp_path) storage_member = CryptFileKeyring() chain = _chain(storage_member, AltEncryptedKeyring()) - env = _keyring_env( - tmp_path, - CLOUDSMITH_KEYRING_KEY="supplied-password", - CLOUDSMITH_KEYRING_DIR=str(tmp_path / "secure"), + core_keyring.configure( + KeyringSettings(key="supplied-password", directory=str(tmp_path / "secure")) ) - with _keyring_session(chain, env): + with _keyring_session(chain, _keyring_env(tmp_path)): assert store_oidc_token(*OIDC_ARGS, "token-data") is True assert get_oidc_token(*OIDC_ARGS) == "token-data" assert (tmp_path / "secure" / storage_member.filename).is_file() @@ -1089,8 +1002,8 @@ def test_lower_member_with_an_existing_file_is_skipped( ): _write_oidc_entry(AltEncryptedKeyring(), tmp_path) chain = _chain(CryptFileKeyring(), AltEncryptedKeyring()) - env = _keyring_env(tmp_path, CLOUDSMITH_KEYRING_KEY="supplied-password") - with _keyring_session(chain, env): + core_keyring.configure(KeyringSettings(key="supplied-password")) + with _keyring_session(chain, _keyring_env(tmp_path)): assert get_oidc_token(*OIDC_ARGS) is None def test_write_fallback_cannot_prompt_for_a_new_file(self, tmp_path, mock_get_user): diff --git a/cloudsmith_cli/core/tests/test_keyring_provider.py b/cloudsmith_cli/core/tests/test_keyring_provider.py index f1729307c..17f4759f4 100644 --- a/cloudsmith_cli/core/tests/test_keyring_provider.py +++ b/cloudsmith_cli/core/tests/test_keyring_provider.py @@ -23,14 +23,13 @@ def mock_get_keyring(): class TestKeyringProvider: def test_returns_none_when_keyring_disabled(self): provider = KeyringProvider() - with patch.dict(os.environ, {"CLOUDSMITH_NO_KEYRING": "1"}): - result = provider.resolve(CredentialContext()) - assert result is None + keyring.configure(keyring.KeyringSettings(no_keyring=True)) + result = provider.resolve(CredentialContext()) + assert result is None def test_returns_none_when_no_token(self): provider = KeyringProvider() env = os.environ.copy() - env.pop("CLOUDSMITH_NO_KEYRING", None) with ( patch.dict(os.environ, env, clear=True), patch.object(keyring, "should_use_keyring", return_value=True), @@ -42,7 +41,6 @@ def test_returns_none_when_no_token(self): def test_returns_bearer_token(self): provider = KeyringProvider() env = os.environ.copy() - env.pop("CLOUDSMITH_NO_KEYRING", None) with ( patch.dict(os.environ, env, clear=True), patch.object(keyring, "should_use_keyring", return_value=True), From 6db23806d0af24ac488b7c9500052b77b70fec22 Mon Sep 17 00:00:00 2001 From: Ian Duffy Date: Wed, 21 Oct 2026 13:57:10 +0100 Subject: [PATCH 02/16] feat(ENG-15016): let credential-helper launchers keep install-time settings A package manager can run its credential helper from an IDE, Docker Desktop or a build agent that does not have the environment of the install. This change adds the shared code that writes the install-time CLOUDSMITH_* settings into a launcher. - `write_launcher(env=...)` in `launchers.py` writes the settings as `export` lines, or as `set` lines in a Windows batch file. The settings replace the values of the caller. - `batch_value()` refuses a value that a batch file cannot hold. A non-ASCII path under the user profile becomes `%USERPROFILE%`. - `install` passes `launcher_env` to an installer that sets `KEEPS_INSTALL_TIME_SETTINGS`. It holds each option with a CLOUDSMITH_* environment variable whose value came from a flag or the environment, also on a parent command, so the keyring settings of `main` are kept too. The API key, the custom headers, the keyring key and the proxy never go into it. No installer sets `KEEPS_INSTALL_TIME_SETTINGS` yet. Co-Authored-By: Claude Opus 5.5 --- .../cli/commands/credential_helper/manage.py | 46 ++++++++ .../test_credential_helper_install.py | 107 ++++++++++++++++++ .../credential_helpers/launchers.py | 84 ++++++++++++-- 3 files changed, 226 insertions(+), 11 deletions(-) diff --git a/cloudsmith_cli/cli/commands/credential_helper/manage.py b/cloudsmith_cli/cli/commands/credential_helper/manage.py index da5d690cb..c570b6ad7 100644 --- a/cloudsmith_cli/cli/commands/credential_helper/manage.py +++ b/cloudsmith_cli/cli/commands/credential_helper/manage.py @@ -12,9 +12,11 @@ from urllib.parse import urlsplit import click +from click.core import ParameterSource from cloudsmith_cli.credential_helpers.cargo.installer import CargoInstaller from cloudsmith_cli.credential_helpers.generic import PartialInstallError +from cloudsmith_cli.credential_helpers.launchers import UnsafeLauncherValue from cloudsmith_cli.credential_helpers.pnpm.installer import PNPMInstaller from cloudsmith_cli.credential_helpers.terraform.installer import ( TerraformHelperExeNotFound, @@ -134,6 +136,46 @@ def _terraform_proxy_next_steps(opts) -> list[str]: ] +# Other users can read a launcher, so it never holds a secret. A proxy URL can +# hold credentials. +_NEVER_KEPT = ("api_key", "api_headers", "keyring_key", "proxy") + + +def _main_envvar(option) -> str: + """Return the main environment variable of *option*.""" + names = option.envvar + return names if isinstance(names, str) else names[0] + + +def _launcher_settings_from_cli(ctx) -> dict[str, str]: + """Return the ``CLOUDSMITH_*`` settings that a launcher keeps. + + An option with a ``CLOUDSMITH_*`` environment variable is kept when its + value came from a flag or from the environment. A value from a config + file is not kept, because the helper reads that file itself. An option + of this command replaces the same option of a parent command. + """ + contexts = [] + while ctx is not None: + contexts.insert(0, ctx) + ctx = ctx.parent + + supplied = (ParameterSource.COMMANDLINE, ParameterSource.ENVIRONMENT) + settings = {} + for context in contexts: + for option in context.command.params: + if option.name in _NEVER_KEPT or not option.envvar: + continue + envvar = _main_envvar(option) + source = context.get_parameter_source(option.name) + if envvar.startswith("CLOUDSMITH_") and source in supplied: + value = context.params[option.name] + settings[envvar] = ( + str(value).lower() if isinstance(value, bool) else value + ) + return settings + + _REPO_FLAGS = ("-r", "--repo", "--repository") @@ -310,12 +352,16 @@ def install_cmd( helper_args = _terraform_helper_args(ctx, opts, repo) install_kwargs["helper_args"] = helper_args + if getattr(installer, "KEEPS_INSTALL_TIME_SETTINGS", False): + install_kwargs["launcher_env"] = _launcher_settings_from_cli(ctx) + try: actions = installer.install(**install_kwargs) except ( TerraformrcConflictError, TerraformHelperExeNotFound, TerraformPluginDirError, + UnsafeLauncherValue, ) as exc: raise click.ClickException(str(exc)) except OSError as exc: diff --git a/cloudsmith_cli/cli/tests/commands/test_credential_helper_install.py b/cloudsmith_cli/cli/tests/commands/test_credential_helper_install.py index cbea23d6b..914dd7b96 100644 --- a/cloudsmith_cli/cli/tests/commands/test_credential_helper_install.py +++ b/cloudsmith_cli/cli/tests/commands/test_credential_helper_install.py @@ -20,6 +20,7 @@ from ....core.credentials.models import CredentialResult from ....credential_helpers.docker.installer import DockerInstaller from ....credential_helpers.launchers import ( + UnsafeLauncherValue, _launcher_content, _launcher_filename, is_on_path, @@ -42,6 +43,13 @@ def runner(): return click.testing.CliRunner() +@pytest.fixture(autouse=True) +def clear_cloudsmith_env(monkeypatch): + """Keep the CLOUDSMITH_* settings of the developer shell out of the installs.""" + for name in [name for name in os.environ if name.startswith("CLOUDSMITH_")]: + monkeypatch.delenv(name) + + # --------------------------------------------------------------------------- # 1. launcher filename + content (pure, platform-parameterised) # --------------------------------------------------------------------------- @@ -95,6 +103,105 @@ def test_launcher_filename_and_content( ) +@pytest.mark.parametrize( + "profile,value,expected", + [ + ("C:\\Users\\jo", "C:\\Users\\jo\\a b&c!", "C:\\Users\\jo\\a b&c!"), + ( + "C:\\Users\\José", + "c:\\users\\josé\\cs\\config.ini", + "%USERPROFILE%\\cs\\config.ini", + ), + ], +) +def test_windows_launcher_settings_are_ascii(monkeypatch, profile, value, expected): + """cmd.exe reads a batch file in the console code page, not UTF-8. + + Only a non-ASCII path under the user profile is written as %USERPROFILE%, + because another account that runs the package manager has a different + profile. + """ + monkeypatch.setenv("USERPROFILE", profile) + content = _launcher_content("cmd", windows=True, env={"CLOUDSMITH_X": value}) + assert content == ( + "@echo off\r\nsetlocal DisableDelayedExpansion\r\n" + f'set "CLOUDSMITH_X={expected}"\r\ncmd %*\r\n' + ) + + +@pytest.mark.parametrize("value", ["%PATH%", "D:\\Données"]) +def test_windows_launcher_refuses_a_value_a_batch_file_cannot_hold(monkeypatch, value): + monkeypatch.setenv("USERPROFILE", "C:\\Users\\José") + with pytest.raises(UnsafeLauncherValue, match="CLOUDSMITH_PROFILE"): + _launcher_content("cmd", windows=True, env={"CLOUDSMITH_PROFILE": value}) + + +def test_install_passes_flag_and_environment_settings_to_the_launcher( + runner, monkeypatch, tmp_path +): + """Settings from flags and CLOUDSMITH_* variables go to the launcher. + + A value read from a config file does not, because the helper reads that + file itself. Secrets and the proxy never go to the launcher. A flag on + the parent command also counts. + """ + from ....cli.commands.credential_helper import manage + from ....cli.commands.main import main + + launcher_env = {} + + class SettingsInstaller: + KEEPS_INSTALL_TIME_SETTINGS = True + + def install(self, **kwargs): + launcher_env.update(kwargs["launcher_env"]) + return [] + + monkeypatch.setitem(manage._INSTALLERS, "settings", SettingsInstaller) + config = tmp_path / "config.ini" + config.write_text("[default]\nssl_verify_strict = false\n") + result = runner.invoke( + main, + args=[ + "-P", + "work", + "credential-helper", + "install", + "settings", + "-w", + "acme", + "--config-file", + str(config), + "--api-host", + "https://api.example.com", + "--proxy", + "http://proxy.example.com:3128", + "-S", + ], + env={ + "CLOUDSMITH_API_KEY": "k_secret", + "CLOUDSMITH_API_HEADERS": "X-Secret=1", + "CLOUDSMITH_KEYRING_KEY": "keyring_secret", + "CLOUDSMITH_KEYRING_DIR": "/var/keyring", + "CLOUDSMITH_OIDC_AUDIENCE": "cloudsmith-test", + "CLOUDSMITH_SERVICE_SLUG": "ci-bot", + }, + catch_exceptions=False, + ) + + assert result.exit_code == 0, result.output + assert launcher_env == { + "CLOUDSMITH_WORKSPACE": "acme", + "CLOUDSMITH_PROFILE": "work", + "CLOUDSMITH_CONFIG_FILE": str(config.resolve()), + "CLOUDSMITH_API_HOST": "https://api.example.com", + "CLOUDSMITH_WITHOUT_API_SSL_VERIFY": "true", + "CLOUDSMITH_OIDC_AUDIENCE": "cloudsmith-test", + "CLOUDSMITH_SERVICE_SLUG": "ci-bot", + "CLOUDSMITH_KEYRING_DIR": "/var/keyring", + } + + # --------------------------------------------------------------------------- # 2. write_launcher / remove_launcher — end-to-end on the host platform # --------------------------------------------------------------------------- diff --git a/cloudsmith_cli/credential_helpers/launchers.py b/cloudsmith_cli/credential_helpers/launchers.py index 8086e6f28..ceabd26db 100644 --- a/cloudsmith_cli/credential_helpers/launchers.py +++ b/cloudsmith_cli/credential_helpers/launchers.py @@ -29,10 +29,49 @@ def _launcher_filename(name: str, *, windows: bool) -> str: return f"{name}.cmd" if windows else name -def launcher_arg(value: str) -> str: +# A batch file expands % even inside quotes and cannot hold " or a line break. +_UNSAFE_IN_BATCH = ('"', "%", "\r", "\n") + + +class UnsafeLauncherValue(ValueError): + """Raised when a value cannot go into a launcher script.""" + + +def batch_value(name: str, value: str) -> str: + """Return *value* in a form that a batch file holds without change. + + ``cmd.exe`` reads a batch file in the console code page, so the file must + be ASCII. A path under the user profile is written as ``%USERPROFILE%``, + which ``cmd.exe`` expands to the correct Unicode path when it runs. An + ASCII path stays literal, because another account that runs the package + manager has a different profile. + """ + if any(char in value for char in _UNSAFE_IN_BATCH): + raise UnsafeLauncherValue( + f"{name} {value!r} cannot go into the launcher: a batch file cannot " + "hold a '\"', a '%' or a line break." + ) + profile = os.environ.get("USERPROFILE", "") + lowered = value.lower() + if ( + not value.isascii() + and profile + and (lowered == profile.lower() or lowered.startswith(profile.lower() + "\\")) + ): + value = "%USERPROFILE%" + value[len(profile) :] + if not value.isascii(): + raise UnsafeLauncherValue( + f"{name} {value!r} cannot go into the launcher: outside the user " + "profile, a batch file can only hold ASCII characters. Set it in " + "the environment that runs the package manager instead." + ) + return value + + +def launcher_arg(name: str, value: str) -> str: """Return *value* quoted as one argument of a launcher command.""" if is_windows(): - return f'"{value}"' + return f'"{batch_value(name, value)}"' return shlex.quote(value) @@ -43,23 +82,45 @@ def helper_command(helper: str) -> str: ``cloudsmith``, so the launcher runs the absolute executable. """ if getattr(sys, "frozen", False): - return f"{launcher_arg(sys.executable)} credential-helper {helper}" + executable = launcher_arg("cloudsmith", sys.executable) + return f"{executable} credential-helper {helper}" return f"cloudsmith credential-helper {helper}" -def _launcher_content(target_cmd: str, *, windows: bool) -> str: +def _launcher_content( + target_cmd: str, *, windows: bool, env: dict[str, str] | None = None +) -> str: """Return the launcher script body for the platform. Windows uses a ``.cmd`` batch file (``@echo off`` keeps stdout clean for Docker's credential JSON); Unix uses a ``#!/bin/sh`` script that ``exec``s - the target so signals and the exit code pass straight through. + the target so signals and the exit code pass straight through. The + ``env`` variables replace the caller's values, so the helper gives the + same result as the install. """ + env = env or {} if windows: - return f"@echo off\r\n{target_cmd} %*\r\n" - return f'#!/bin/sh\nexec {target_cmd} "$@"\n' + settings = "".join( + f'set "{name}={batch_value(name, value)}"\r\n' + for name, value in env.items() + ) + if settings: + # Keep "!" literal and the settings local to the launcher. + settings = f"setlocal DisableDelayedExpansion\r\n{settings}" + return f"@echo off\r\n{settings}{target_cmd} %*\r\n" + settings = "".join( + f"export {name}={shlex.quote(value)}\n" for name, value in env.items() + ) + return f'#!/bin/sh\n{settings}exec {target_cmd} "$@"\n' -def write_launcher(bin_dir: Path, name: str, target_cmd: str, dry_run=False) -> Path: +def write_launcher( + bin_dir: Path, + name: str, + target_cmd: str, + dry_run=False, + env: dict[str, str] | None = None, +) -> Path: """Write a launcher script for *name* in *bin_dir* that execs *target_cmd*. Parameters @@ -71,6 +132,8 @@ def write_launcher(bin_dir: Path, name: str, target_cmd: str, dry_run=False) -> target_cmd: The command the launcher forwards to (e.g. ``cloudsmith credential-helper docker``). + env: + Environment variables that the launcher sets. Returns ------- @@ -78,6 +141,7 @@ def write_launcher(bin_dir: Path, name: str, target_cmd: str, dry_run=False) -> The path of the written file. """ windows = is_windows() + content = _launcher_content(target_cmd, windows=windows, env=env) if dry_run: if windows: return bin_dir / f"{name}.cmd" @@ -87,9 +151,7 @@ def write_launcher(bin_dir: Path, name: str, target_cmd: str, dry_run=False) -> bin_dir.mkdir(parents=True, exist_ok=True) dest = bin_dir / _launcher_filename(name, windows=windows) - dest.write_text( - _launcher_content(target_cmd, windows=windows), encoding="utf-8", newline="" - ) + dest.write_text(content, encoding="utf-8", newline="") if not windows: dest.chmod(0o755) From a3667bb8347f74e2d42fbcb086fe5f8462ee3c42 Mon Sep 17 00:00:00 2001 From: Ian Duffy Date: Wed, 21 Oct 2026 14:00:15 +0100 Subject: [PATCH 03/16] feat(ENG-15016): keep install-time settings in the Docker credential helper Docker runs `docker-credential-cloudsmith` with the environment of the Docker client, for example Docker Desktop or an IDE. The flags and CLOUDSMITH_* variables of the install were lost, so later pulls used another profile, Workspace or API host. - The Docker installer now keeps the install-time settings in the launcher. - An install with a profile or a Workspace writes its own helper, `cloudsmith--`, so that installs can coexist. An install without them writes `cloudsmith`, as before. - The install shows a warning when it moves a host from a different Cloudsmith helper. - `uninstall docker -P -w ` removes only the helper of that install. The scope comes only from flags, so that a plain `uninstall docker` removes the unscoped helper, as before. - `list` shows the hosts of all Cloudsmith helpers and the launcher that they use. Co-Authored-By: Claude Opus 5.5 --- CHANGELOG.md | 1 + .../cli/commands/credential_helper/manage.py | 26 +++- cloudsmith_cli/cli/decorators.py | 6 +- .../test_credential_helper_install.py | 111 ++++++++++++++ .../credential_helpers/docker/installer.py | 136 +++++++++++++----- 5 files changed, 238 insertions(+), 42 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 6ff5c88db..5eab5f754 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -25,6 +25,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0. - `cloudsmith credential-helper install terraform` now writes `-w ` into the `terraformrc` args, not `--org`. A block that has `--org` continues to work. The install also writes `--api-host` and `--proxy` when you set them, so the helper finds the domains on that API host through that proxy. A proxy URL with a user name or password is not written, because Terraform passes the args on the command line of the helper; set `CLOUDSMITH_PROXY` where `terraform init` runs instead. Without a Workspace, the install tells you how to set one. - `cloudsmith credential-helper install` and `uninstall` now use `~/.cloudsmith/bin` when you do not give `--bin-dir`. Before, they used the directory of the `cloudsmith` executable, which a package manager can overwrite or make read-only. Terraform still uses its plugin directory. If the directory is not on `PATH`, `install` shows the command that adds it. To remove a helper that an earlier version installed, give its directory with `--bin-dir`. - The hint shown for a `401 - Unauthorized` error when no API key or SAML session is set no longer suggests `cloudsmith token`. It now suggests `CLOUDSMITH_API_KEY` or `cloudsmith auth`. +- `cloudsmith credential-helper install docker` now writes the `CLOUDSMITH_*` settings that you give as a flag or an environment variable into the launcher. These settings are the Workspace, profile, config and credentials files, API host, TLS verification, OIDC and keyring settings. Docker then gives the same result from Docker Desktop, an IDE or a new shell. The launcher never contains API keys, custom headers or the proxy URL. An install with a profile or a Workspace writes its own launcher, for example `docker-credential-cloudsmith--`, so that the custom domains of different Workspaces can use different helpers. A host that more than one install configures, such as `docker.cloudsmith.io`, uses the helper of the last install, and the install shows a warning when it moves such a host. An install without a profile or a Workspace writes `docker-credential-cloudsmith`, as before. `uninstall docker -P -w ` removes only the helper of that install. Without these flags, `uninstall docker` removes `docker-credential-cloudsmith`, as before. `list` shows the hosts of all Cloudsmith helpers. - Messages, help text and the README now say SAML instead of SSO. JSON keys and values, such as `"method": "sso"`, do not change. ### Deprecated diff --git a/cloudsmith_cli/cli/commands/credential_helper/manage.py b/cloudsmith_cli/cli/commands/credential_helper/manage.py index c570b6ad7..e114656ab 100644 --- a/cloudsmith_cli/cli/commands/credential_helper/manage.py +++ b/cloudsmith_cli/cli/commands/credential_helper/manage.py @@ -31,6 +31,8 @@ from ....credential_helpers.docker.installer import DockerInstaller from ... import utils from ...decorators import ( + PROFILE_ENVVAR, + WORKSPACE_ENVVARS, common_api_auth_options, common_cli_config_options, common_cli_output_options, @@ -355,6 +357,12 @@ def install_cmd( if getattr(installer, "KEEPS_INSTALL_TIME_SETTINGS", False): install_kwargs["launcher_env"] = _launcher_settings_from_cli(ctx) + # The helper name uses the same profile and Workspace as the launcher. + if helper == "docker": + launcher_env = install_kwargs["launcher_env"] + install_kwargs["profile"] = launcher_env.get(PROFILE_ENVVAR) + install_kwargs["workspace"] = launcher_env.get(WORKSPACE_ENVVARS[0]) + try: actions = installer.install(**install_kwargs) except ( @@ -426,10 +434,17 @@ def install_cmd( default=False, help="Show what would be done without making any changes.", ) +@click.option( + "-w", + "--workspace", + help="Docker only: remove the helper that was installed for this Workspace.", +) @common_cli_config_options @common_cli_output_options @click.pass_context -def uninstall_cmd(ctx, opts, helper: str, bin_dir: str | None, dry_run: bool) -> None: +def uninstall_cmd( + ctx, opts, helper: str, bin_dir: str | None, dry_run: bool, workspace: str | None +) -> None: """Uninstall a credential helper launcher and remove its config entries. HELPER is the name of the credential helper to uninstall (e.g. ``docker``). @@ -445,8 +460,15 @@ def uninstall_cmd(ctx, opts, helper: str, bin_dir: str | None, dry_run: bool) -> $ cloudsmith credential-helper uninstall docker --dry-run """ installer = _get_installer(helper) + uninstall_kwargs = {"bin_dir": bin_dir, "dry_run": dry_run} + # The scope comes only from flags, so that a shell with CLOUDSMITH_PROFILE + # or CLOUDSMITH_WORKSPACE can still remove the unscoped helper. + if helper == "docker": + if ctx.get_parameter_source("profile") == ParameterSource.COMMANDLINE: + uninstall_kwargs["profile"] = ctx.meta.get("profile") + uninstall_kwargs["workspace"] = workspace try: - actions = installer.uninstall(bin_dir=bin_dir, dry_run=dry_run) + actions = installer.uninstall(**uninstall_kwargs) except OSError as exc: raise click.ClickException( f"Failed to uninstall {helper!r} credential helper: {exc}" diff --git a/cloudsmith_cli/cli/decorators.py b/cloudsmith_cli/cli/decorators.py index 9c45d4dc7..f2a7131bc 100644 --- a/cloudsmith_cli/cli/decorators.py +++ b/cloudsmith_cli/cli/decorators.py @@ -198,7 +198,7 @@ def common_cli_config_options(f): "-P", "--profile", default=None, - envvar="CLOUDSMITH_PROFILE", + envvar=PROFILE_ENVVAR, help="The name of the profile to use for configuration.", ) @click.pass_context @@ -384,6 +384,8 @@ def wrapper(ctx, *args, **kwargs): return wrapper +PROFILE_ENVVAR = "CLOUDSMITH_PROFILE" +WORKSPACE_ENVVARS = ("CLOUDSMITH_WORKSPACE", "CLOUDSMITH_ORG") PROXY_ENVVARS = ("CLOUDSMITH_PROXY", "CLOUDSMITH_API_PROXY") PROXY_SUFFIX_ENVVARS = ( "CLOUDSMITH_ALLOWED_PROXY_SUFFIXES", @@ -635,7 +637,7 @@ def resolve_credentials(f): "-o", "--owner", "org", - envvar=["CLOUDSMITH_WORKSPACE", "CLOUDSMITH_ORG"], + envvar=list(WORKSPACE_ENVVARS), help="The Cloudsmith Workspace slug.", ) @click.option( diff --git a/cloudsmith_cli/cli/tests/commands/test_credential_helper_install.py b/cloudsmith_cli/cli/tests/commands/test_credential_helper_install.py index 914dd7b96..e638ddfa0 100644 --- a/cloudsmith_cli/cli/tests/commands/test_credential_helper_install.py +++ b/cloudsmith_cli/cli/tests/commands/test_credential_helper_install.py @@ -399,6 +399,93 @@ def test_docker_installer_install(tmp_path, monkeypatch): assert (bin_dir / "docker-credential-cloudsmith").exists() +def test_docker_install_scopes_the_helper_to_the_install_time_settings( + tmp_path, monkeypatch +): + """The profile and Workspace name the helper, so installs can coexist.""" + monkeypatch.setenv("DOCKER_CONFIG", str(tmp_path / ".docker")) + monkeypatch.delattr(sys, "frozen", raising=False) + bin_dir = tmp_path / "bin" + monkeypatch.setenv("PATH", str(bin_dir)) + installer = DockerInstaller() + installer.install(bin_dir=str(bin_dir), discover=False) + + actions = installer.install( + bin_dir=str(bin_dir), + discover=False, + profile="Meta", + workspace="iduffy_demo", + launcher_env={ + "CLOUDSMITH_PROFILE": "Meta", + "CLOUDSMITH_WORKSPACE": "iduffy_demo", + "CLOUDSMITH_API_HOST": "https://api.example.com", + }, + ) + + name = "cloudsmith-meta-iduffy-demo" + assert (bin_dir / f"docker-credential-{name}").read_text() == ( + "#!/bin/sh\n" + "export CLOUDSMITH_PROFILE=Meta\n" + "export CLOUDSMITH_WORKSPACE=iduffy_demo\n" + "export CLOUDSMITH_API_HOST=https://api.example.com\n" + 'exec cloudsmith credential-helper docker "$@"\n' + ) + assert (bin_dir / "docker-credential-cloudsmith").exists() + cfg = json.loads((tmp_path / ".docker" / "config.json").read_text()) + assert cfg["credHelpers"] == {"docker.cloudsmith.io": name} + assert ( + "WARNING: credHelpers['docker.cloudsmith.io'] changes from 'cloudsmith'" + f" to {name!r}" + ) in actions + + +def test_docker_dry_run_shows_the_scoped_launcher_and_its_settings( + tmp_path, monkeypatch +): + monkeypatch.setenv("DOCKER_CONFIG", str(tmp_path / ".docker")) + bin_dir = tmp_path / "bin" + + actions = DockerInstaller().install( + bin_dir=str(bin_dir), + discover=False, + dry_run=True, + workspace="acme", + launcher_env={"CLOUDSMITH_WORKSPACE": "acme", "CLOUDSMITH_NO_KEYRING": "1"}, + ) + + assert actions[:2] == [ + f"would write launcher {bin_dir / 'docker-credential-cloudsmith-acme'}", + "launcher sets CLOUDSMITH_WORKSPACE, CLOUDSMITH_NO_KEYRING", + ] + assert not bin_dir.exists() + + +def test_docker_uninstall_takes_the_scope_from_flags_only( + runner, tmp_path, monkeypatch +): + """Without flags, uninstall removes the unscoped helper, as before.""" + from ....cli.commands.credential_helper.manage import install_cmd, uninstall_cmd + + monkeypatch.setenv("DOCKER_CONFIG", str(tmp_path / ".docker")) + bin_dir = tmp_path / "bin" + DockerInstaller().install(bin_dir=str(bin_dir), domains=("legacy.example.com",)) + args = ["docker", "--bin-dir", str(bin_dir)] + env = {"CLOUDSMITH_WORKSPACE": "acme"} + installed = runner.invoke( + install_cmd, [*args, "--domain", "acme.example.com", "--no-discover"], env=env + ) + + unscoped = runner.invoke(uninstall_cmd, args, env=env) + scoped_launchers = sorted(path.name for path in bin_dir.iterdir()) + scoped = runner.invoke(uninstall_cmd, [*args, "-w", "acme"]) + + assert [installed.exit_code, unscoped.exit_code, scoped.exit_code] == [0, 0, 0] + assert scoped_launchers == ["docker-credential-cloudsmith-acme"] + assert not list(bin_dir.iterdir()) + cfg = json.loads((tmp_path / ".docker" / "config.json").read_text()) + assert "credHelpers" not in cfg + + def test_pnpm_installer_install(tmp_path: Path, monkeypatch: MonkeyPatch): """install sets default+extra domains, preserves foreign entries, writes the launcher.""" npm_path = tmp_path / ".npmrc" @@ -701,6 +788,30 @@ def test_docker_installer_status_type_contract(tmp_path, monkeypatch): assert not isinstance(launcher, Path) +def test_docker_installer_status_reports_scoped_helpers(tmp_path, monkeypatch): + monkeypatch.setenv("DOCKER_CONFIG", str(tmp_path / ".docker")) + bin_dir = tmp_path / "bin" + installer = DockerInstaller() + installer.install(bin_dir=str(bin_dir), discover=False) + installer.install( + bin_dir=str(bin_dir), + domains=("acme.example.com",), + discover=False, + workspace="acme", + ) + + with patch( + "cloudsmith_cli.credential_helpers.docker.installer.resolve_bin_dir", + return_value=bin_dir, + ): + status = installer.status() + + assert status == { + "launcher": str(bin_dir / "docker-credential-cloudsmith-acme"), + "hosts": ["docker.cloudsmith.io", "acme.example.com"], + } + + def test_pnpm_installer_status_type_contract(tmp_path: Path, monkeypatch: MonkeyPatch): """status()['launcher'] is str when installed and None when not — never a Path. diff --git a/cloudsmith_cli/credential_helpers/docker/installer.py b/cloudsmith_cli/credential_helpers/docker/installer.py index 32bec276b..a0b34046c 100644 --- a/cloudsmith_cli/credential_helpers/docker/installer.py +++ b/cloudsmith_cli/credential_helpers/docker/installer.py @@ -11,6 +11,7 @@ import json import logging import os +import re from pathlib import Path from typing import TYPE_CHECKING @@ -45,6 +46,29 @@ def _docker_config_path() -> Path: return Path.home() / ".docker" / "config.json" +def _helper_name(profile: str | None, workspace: str | None) -> str: + """Return the ``credHelpers`` value for an install with these settings. + + Each profile and Workspace gets its own launcher, so that installs for + different configurations can coexist. + """ + segments = [ + re.sub(r"[^a-z0-9-]+", "-", value.lower()).strip("-") + for value in (profile, workspace) + if value + ] + return "-".join(["cloudsmith", *filter(None, segments)]) + + +def _is_cloudsmith_helper(value) -> bool: + return value == "cloudsmith" or str(value).startswith("cloudsmith-") + + +def _launcher_path(target_dir: Path, helper: str) -> Path: + name = f"docker-credential-{helper}" + return target_dir / (f"{name}.cmd" if is_windows() else name) + + class DockerInstaller: """Manages installation of the Docker credential helper for Cloudsmith. @@ -61,8 +85,10 @@ class DockerInstaller: """ LAUNCHER_NAME = "docker-credential-cloudsmith" - HELPER_VALUE = "cloudsmith" DEFAULT_HOST = "docker.cloudsmith.io" + # Docker runs the helper from Docker Desktop, IDEs and other shells, so + # the launcher keeps the install-time settings. + KEEPS_INSTALL_TIME_SETTINGS = True name = "docker" summary = "Docker credential helper for Cloudsmith registries" @@ -77,6 +103,9 @@ def install( org: str | None = None, credential: CredentialResult | None = None, api_host: str | None = None, + profile: str | None = None, + workspace: str | None = None, + launcher_env: dict[str, str] | None = None, dry_run: bool = False, ) -> list[str]: """Install the Docker credential helper. @@ -105,6 +134,10 @@ def install( Resolved credential used for custom-domain discovery. api_host: Cloudsmith API host URL override. + profile, workspace: + The install-time profile and Workspace, which name the helper. + launcher_env: + ``CLOUDSMITH_*`` settings that the launcher sets. dry_run: When ``True``, compute and return planned actions without writing any files. @@ -117,6 +150,9 @@ def install( """ target_dir = resolve_bin_dir(bin_dir) config_path = _docker_config_path() + launcher_env = launcher_env or {} + helper = _helper_name(profile, workspace) + launcher_name = f"docker-credential-{helper}" actions: list[str] = [] @@ -179,58 +215,80 @@ def install( deduped.append(h) hosts = deduped + replaced: list[str] = [] + def mutate(config: dict) -> None: helpers = config.get("credHelpers") if not isinstance(helpers, dict): helpers = config["credHelpers"] = {} for host in hosts: - helpers[host] = self.HELPER_VALUE + current = helpers.get(host) + if current != helper and _is_cloudsmith_helper(current): + replaced.append( + f"WARNING: credHelpers[{host!r}] changes from {current!r}" + f" to {helper!r}" + ) + helpers[host] = helper + + launcher_settings = ( + [f"launcher sets {', '.join(launcher_env)}"] if launcher_env else [] + ) if dry_run: - if os.name == "nt": - launcher_path = target_dir / f"{self.LAUNCHER_NAME}.cmd" - else: - launcher_path = target_dir / self.LAUNCHER_NAME + launcher_path = write_launcher( + target_dir, + launcher_name, + helper_command(self.name), + dry_run=True, + env=launcher_env, + ) actions.append(f"would write launcher {launcher_path}") + actions.extend(launcher_settings) would_change = merge_config_file(config_path, mutate, dry_run=True) for host in hosts: if would_change: actions.append( - f"would set credHelpers[{host!r}]={self.HELPER_VALUE!r}" - f" in {config_path}" + f"would set credHelpers[{host!r}]={helper!r} in {config_path}" ) else: actions.append( f"credHelpers[{host!r}] already set" f" in {config_path} (no change)" ) - return actions + return actions + replaced # Real install launcher_path = write_launcher( - target_dir, self.LAUNCHER_NAME, helper_command(self.name) + target_dir, launcher_name, helper_command(self.name), env=launcher_env ) actions.append(f"wrote launcher {launcher_path}") + actions.extend(launcher_settings) changed = merge_config_file(config_path, mutate) if changed: actions.extend( - f"set credHelpers[{host!r}]={self.HELPER_VALUE!r} in {config_path}" + f"set credHelpers[{host!r}]={helper!r} in {config_path}" for host in hosts ) else: actions.append(f"config.json already up to date ({config_path})") + actions.extend(replaced) if not is_on_path(target_dir): actions.append( - path_warning(target_dir, "Docker", self.LAUNCHER_NAME, is_windows()) + path_warning(target_dir, "Docker", launcher_name, is_windows()) ) return actions def uninstall( - self, *, bin_dir: str | None = None, dry_run: bool = False + self, + *, + bin_dir: str | None = None, + profile: str | None = None, + workspace: str | None = None, + dry_run: bool = False, ) -> list[str]: """Uninstall the Docker credential helper. @@ -244,6 +302,9 @@ def uninstall( Defaults to :func:`resolve_bin_dir` auto-detection. Pass the same value that was given to :meth:`install` so the correct launcher file is found and removed. + profile, workspace: + The settings of the install to remove. Without them, the helper + of an install without them is removed. dry_run: When ``True``, return planned actions without writing any files. @@ -254,23 +315,21 @@ def uninstall( """ target_dir = resolve_bin_dir(bin_dir) config_path = _docker_config_path() + helper = _helper_name(profile, workspace) + launcher_name = f"docker-credential-{helper}" def mutate(config: dict) -> None: helpers = config.get("credHelpers") if not isinstance(helpers, dict): return - removed = [k for k, v in helpers.items() if v == self.HELPER_VALUE] + removed = [k for k, v in helpers.items() if v == helper] for key in removed: del helpers[key] if removed and not helpers: del config["credHelpers"] actions: list[str] = [] - - if os.name == "nt": - launcher_path = target_dir / f"{self.LAUNCHER_NAME}.cmd" - else: - launcher_path = target_dir / self.LAUNCHER_NAME + launcher_path = _launcher_path(target_dir, helper) if dry_run: if launcher_path.exists(): @@ -284,14 +343,14 @@ def mutate(config: dict) -> None: if would_change: actions.append( f"would remove credHelpers entries with value" - f" {self.HELPER_VALUE!r} from {config_path}" + f" {helper!r} from {config_path}" ) else: actions.append(f"no credHelpers entries to remove from {config_path}") return actions # Real uninstall - removed = remove_launcher(target_dir, self.LAUNCHER_NAME) + removed = remove_launcher(target_dir, launcher_name) if removed: actions.append(f"removed launcher {launcher_path}") else: @@ -300,8 +359,7 @@ def mutate(config: dict) -> None: changed = merge_config_file(config_path, mutate) if changed: actions.append( - f"removed credHelpers entries with value" - f" {self.HELPER_VALUE!r} from {config_path}" + f"removed credHelpers entries with value {helper!r} from {config_path}" ) else: actions.append(f"no credHelpers entries to remove from {config_path}") @@ -317,31 +375,33 @@ def status(self) -> dict: A dict with keys: ``"launcher"`` - The :class:`~pathlib.Path` of the launcher if it exists, - else ``None``. + The path of the launcher that the first host uses, else of + the unscoped launcher, if it exists. Else ``None``. ``"hosts"`` List of hostnames in ``config.json``'s ``credHelpers`` block - whose value equals ``"cloudsmith"``. + that use a ``cloudsmith`` or ``cloudsmith-*`` helper. """ - target_dir = resolve_bin_dir() - if os.name == "nt": - launcher_path: Path | None = target_dir / f"{self.LAUNCHER_NAME}.cmd" - else: - launcher_path = target_dir / self.LAUNCHER_NAME - - if launcher_path is not None and not launcher_path.exists(): - launcher_path = None - config_path = _docker_config_path() - hosts: list[str] = [] + helpers: dict = {} if config_path.exists(): try: data = json.loads(config_path.read_text(encoding="utf-8")) if isinstance(data, dict): - helpers = data.get("credHelpers", {}) - hosts = [k for k, v in helpers.items() if v == self.HELPER_VALUE] + helpers = { + k: v + for k, v in data.get("credHelpers", {}).items() + if _is_cloudsmith_helper(v) + } except (json.JSONDecodeError, OSError): pass + hosts = list(helpers) + + target_dir = resolve_bin_dir() + candidates = [ + _launcher_path(target_dir, helper) + for helper in [*helpers.values(), "cloudsmith"] + ] + launcher_path = next((path for path in candidates if path.exists()), None) return { "launcher": str(launcher_path) if launcher_path is not None else None, From 796438ae023f7325bc5ba398e47f614141964b04 Mon Sep 17 00:00:00 2001 From: Ian Duffy Date: Wed, 21 Oct 2026 23:19:39 +0100 Subject: [PATCH 04/16] fix(ENG-15016): keep each Docker helper name to one install - Read the profile and the Workspace of an install from its launcher settings. - Stop an install or an uninstall when the launcher with that name has a different profile or Workspace, for example `-P acme` and `-w acme`. - Take the uninstall profile also from a `-P` on a parent command. - Do not keep the colour settings in a launcher. A `ColorMode` value caused a TypeError in the install. Co-Authored-By: Claude Opus 5.5 --- CHANGELOG.md | 2 +- .../cli/commands/credential_helper/manage.py | 39 +++++--- cloudsmith_cli/cli/decorators.py | 6 +- .../test_credential_helper_install.py | 97 ++++++++++++------- .../credential_helpers/docker/installer.py | 74 ++++++++++---- .../credential_helpers/launchers.py | 15 +++ 6 files changed, 163 insertions(+), 70 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 5eab5f754..858e416c3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -25,7 +25,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0. - `cloudsmith credential-helper install terraform` now writes `-w ` into the `terraformrc` args, not `--org`. A block that has `--org` continues to work. The install also writes `--api-host` and `--proxy` when you set them, so the helper finds the domains on that API host through that proxy. A proxy URL with a user name or password is not written, because Terraform passes the args on the command line of the helper; set `CLOUDSMITH_PROXY` where `terraform init` runs instead. Without a Workspace, the install tells you how to set one. - `cloudsmith credential-helper install` and `uninstall` now use `~/.cloudsmith/bin` when you do not give `--bin-dir`. Before, they used the directory of the `cloudsmith` executable, which a package manager can overwrite or make read-only. Terraform still uses its plugin directory. If the directory is not on `PATH`, `install` shows the command that adds it. To remove a helper that an earlier version installed, give its directory with `--bin-dir`. - The hint shown for a `401 - Unauthorized` error when no API key or SAML session is set no longer suggests `cloudsmith token`. It now suggests `CLOUDSMITH_API_KEY` or `cloudsmith auth`. -- `cloudsmith credential-helper install docker` now writes the `CLOUDSMITH_*` settings that you give as a flag or an environment variable into the launcher. These settings are the Workspace, profile, config and credentials files, API host, TLS verification, OIDC and keyring settings. Docker then gives the same result from Docker Desktop, an IDE or a new shell. The launcher never contains API keys, custom headers or the proxy URL. An install with a profile or a Workspace writes its own launcher, for example `docker-credential-cloudsmith--`, so that the custom domains of different Workspaces can use different helpers. A host that more than one install configures, such as `docker.cloudsmith.io`, uses the helper of the last install, and the install shows a warning when it moves such a host. An install without a profile or a Workspace writes `docker-credential-cloudsmith`, as before. `uninstall docker -P -w ` removes only the helper of that install. Without these flags, `uninstall docker` removes `docker-credential-cloudsmith`, as before. `list` shows the hosts of all Cloudsmith helpers. +- `cloudsmith credential-helper install docker` now writes the `CLOUDSMITH_*` settings that you give as a flag or an environment variable into the launcher. These settings include the Workspace, profile, config and credentials files, API host, TLS verification, OIDC and keyring settings. Docker then gives the same result from Docker Desktop, an IDE or a new shell. The launcher never contains API keys, custom headers or the proxy URL. An install with a profile or a Workspace writes its own launcher, for example `docker-credential-cloudsmith--`, so that the custom domains of different Workspaces can use different helpers. A host that more than one install configures, such as `docker.cloudsmith.io`, uses the helper of the last install, and the install shows a warning when it moves such a host. An install without a profile or a Workspace writes `docker-credential-cloudsmith`, as before. Two installs can get the same name, for example with `-P acme` and with `-w acme`. Then the second install stops and shows the command that removes the first. `uninstall docker -P -w ` removes only the helper of that install. Without these flags, `uninstall docker` removes `docker-credential-cloudsmith`, as before. `list` shows the hosts of all Cloudsmith helpers. - Messages, help text and the README now say SAML instead of SSO. JSON keys and values, such as `"method": "sso"`, do not change. ### Deprecated diff --git a/cloudsmith_cli/cli/commands/credential_helper/manage.py b/cloudsmith_cli/cli/commands/credential_helper/manage.py index e114656ab..cc23cc841 100644 --- a/cloudsmith_cli/cli/commands/credential_helper/manage.py +++ b/cloudsmith_cli/cli/commands/credential_helper/manage.py @@ -28,11 +28,12 @@ TerraformrcConflictError, ) -from ....credential_helpers.docker.installer import DockerInstaller +from ....credential_helpers.docker.installer import ( + DockerInstaller, + HelperScopeConflict, +) from ... import utils from ...decorators import ( - PROFILE_ENVVAR, - WORKSPACE_ENVVARS, common_api_auth_options, common_cli_config_options, common_cli_output_options, @@ -139,8 +140,15 @@ def _terraform_proxy_next_steps(opts) -> list[str]: # Other users can read a launcher, so it never holds a secret. A proxy URL can -# hold credentials. -_NEVER_KEPT = ("api_key", "api_headers", "keyring_key", "proxy") +# hold credentials. The colour settings are for the terminal of the install. +_NEVER_KEPT = ( + "api_key", + "api_headers", + "keyring_key", + "proxy", + "color", + "force_color_env", +) def _main_envvar(option) -> str: @@ -178,6 +186,15 @@ def _launcher_settings_from_cli(ctx) -> dict[str, str]: return settings +def _profile_flag(ctx) -> str | None: + """Return the ``-P`` value of this command or of a parent command.""" + while ctx is not None: + if ctx.get_parameter_source("profile") == ParameterSource.COMMANDLINE: + return ctx.params["profile"] + ctx = ctx.parent + return None + + _REPO_FLAGS = ("-r", "--repo", "--repository") @@ -357,12 +374,6 @@ def install_cmd( if getattr(installer, "KEEPS_INSTALL_TIME_SETTINGS", False): install_kwargs["launcher_env"] = _launcher_settings_from_cli(ctx) - # The helper name uses the same profile and Workspace as the launcher. - if helper == "docker": - launcher_env = install_kwargs["launcher_env"] - install_kwargs["profile"] = launcher_env.get(PROFILE_ENVVAR) - install_kwargs["workspace"] = launcher_env.get(WORKSPACE_ENVVARS[0]) - try: actions = installer.install(**install_kwargs) except ( @@ -370,6 +381,7 @@ def install_cmd( TerraformHelperExeNotFound, TerraformPluginDirError, UnsafeLauncherValue, + HelperScopeConflict, ) as exc: raise click.ClickException(str(exc)) except OSError as exc: @@ -464,11 +476,12 @@ def uninstall_cmd( # The scope comes only from flags, so that a shell with CLOUDSMITH_PROFILE # or CLOUDSMITH_WORKSPACE can still remove the unscoped helper. if helper == "docker": - if ctx.get_parameter_source("profile") == ParameterSource.COMMANDLINE: - uninstall_kwargs["profile"] = ctx.meta.get("profile") + uninstall_kwargs["profile"] = _profile_flag(ctx) uninstall_kwargs["workspace"] = workspace try: actions = installer.uninstall(**uninstall_kwargs) + except HelperScopeConflict as exc: + raise click.ClickException(str(exc)) except OSError as exc: raise click.ClickException( f"Failed to uninstall {helper!r} credential helper: {exc}" diff --git a/cloudsmith_cli/cli/decorators.py b/cloudsmith_cli/cli/decorators.py index f2a7131bc..9c45d4dc7 100644 --- a/cloudsmith_cli/cli/decorators.py +++ b/cloudsmith_cli/cli/decorators.py @@ -198,7 +198,7 @@ def common_cli_config_options(f): "-P", "--profile", default=None, - envvar=PROFILE_ENVVAR, + envvar="CLOUDSMITH_PROFILE", help="The name of the profile to use for configuration.", ) @click.pass_context @@ -384,8 +384,6 @@ def wrapper(ctx, *args, **kwargs): return wrapper -PROFILE_ENVVAR = "CLOUDSMITH_PROFILE" -WORKSPACE_ENVVARS = ("CLOUDSMITH_WORKSPACE", "CLOUDSMITH_ORG") PROXY_ENVVARS = ("CLOUDSMITH_PROXY", "CLOUDSMITH_API_PROXY") PROXY_SUFFIX_ENVVARS = ( "CLOUDSMITH_ALLOWED_PROXY_SUFFIXES", @@ -637,7 +635,7 @@ def resolve_credentials(f): "-o", "--owner", "org", - envvar=list(WORKSPACE_ENVVARS), + envvar=["CLOUDSMITH_WORKSPACE", "CLOUDSMITH_ORG"], help="The Cloudsmith Workspace slug.", ) @click.option( diff --git a/cloudsmith_cli/cli/tests/commands/test_credential_helper_install.py b/cloudsmith_cli/cli/tests/commands/test_credential_helper_install.py index e638ddfa0..973e946b7 100644 --- a/cloudsmith_cli/cli/tests/commands/test_credential_helper_install.py +++ b/cloudsmith_cli/cli/tests/commands/test_credential_helper_install.py @@ -18,12 +18,16 @@ from cloudsmith_cli.credential_helpers.pnpm.installer import PNPMInstaller from ....core.credentials.models import CredentialResult -from ....credential_helpers.docker.installer import DockerInstaller +from ....credential_helpers.docker.installer import ( + DockerInstaller, + HelperScopeConflict, +) from ....credential_helpers.launchers import ( UnsafeLauncherValue, _launcher_content, _launcher_filename, is_on_path, + parse_launcher_env, remove_launcher, resolve_bin_dir, write_launcher, @@ -136,14 +140,20 @@ def test_windows_launcher_refuses_a_value_a_batch_file_cannot_hold(monkeypatch, _launcher_content("cmd", windows=True, env={"CLOUDSMITH_PROFILE": value}) +@pytest.mark.parametrize("windows", [False, True]) +def test_parse_launcher_env_reads_the_settings_of_a_launcher(windows): + env = {"CLOUDSMITH_PROFILE": "it's a b", "CLOUDSMITH_WORKSPACE": "x=y!"} + assert parse_launcher_env(_launcher_content("cmd", windows=windows, env=env)) == env + + def test_install_passes_flag_and_environment_settings_to_the_launcher( runner, monkeypatch, tmp_path ): """Settings from flags and CLOUDSMITH_* variables go to the launcher. A value read from a config file does not, because the helper reads that - file itself. Secrets and the proxy never go to the launcher. A flag on - the parent command also counts. + file itself. Secrets, the proxy and the colour settings never go to the + launcher. A flag on the parent command also counts. """ from ....cli.commands.credential_helper import manage from ....cli.commands.main import main @@ -181,6 +191,8 @@ def install(self, **kwargs): env={ "CLOUDSMITH_API_KEY": "k_secret", "CLOUDSMITH_API_HEADERS": "X-Secret=1", + "CLOUDSMITH_COLOR": "never", + "CLOUDSMITH_FORCE_COLOR": "1", "CLOUDSMITH_KEYRING_KEY": "keyring_secret", "CLOUDSMITH_KEYRING_DIR": "/var/keyring", "CLOUDSMITH_OIDC_AUDIENCE": "cloudsmith-test", @@ -413,8 +425,6 @@ def test_docker_install_scopes_the_helper_to_the_install_time_settings( actions = installer.install( bin_dir=str(bin_dir), discover=False, - profile="Meta", - workspace="iduffy_demo", launcher_env={ "CLOUDSMITH_PROFILE": "Meta", "CLOUDSMITH_WORKSPACE": "iduffy_demo", @@ -437,6 +447,14 @@ def test_docker_install_scopes_the_helper_to_the_install_time_settings( "WARNING: credHelpers['docker.cloudsmith.io'] changes from 'cloudsmith'" f" to {name!r}" ) in actions + with patch( + "cloudsmith_cli.credential_helpers.docker.installer.resolve_bin_dir", + return_value=bin_dir, + ): + assert installer.status() == { + "launcher": str(bin_dir / f"docker-credential-{name}"), + "hosts": ["docker.cloudsmith.io"], + } def test_docker_dry_run_shows_the_scoped_launcher_and_its_settings( @@ -449,7 +467,6 @@ def test_docker_dry_run_shows_the_scoped_launcher_and_its_settings( bin_dir=str(bin_dir), discover=False, dry_run=True, - workspace="acme", launcher_env={"CLOUDSMITH_WORKSPACE": "acme", "CLOUDSMITH_NO_KEYRING": "1"}, ) @@ -463,29 +480,65 @@ def test_docker_dry_run_shows_the_scoped_launcher_and_its_settings( def test_docker_uninstall_takes_the_scope_from_flags_only( runner, tmp_path, monkeypatch ): - """Without flags, uninstall removes the unscoped helper, as before.""" + """Without flags, uninstall removes the unscoped helper, as before. + + A -P flag on the root command also counts. + """ from ....cli.commands.credential_helper.manage import install_cmd, uninstall_cmd + from ....cli.commands.main import main monkeypatch.setenv("DOCKER_CONFIG", str(tmp_path / ".docker")) bin_dir = tmp_path / "bin" DockerInstaller().install(bin_dir=str(bin_dir), domains=("legacy.example.com",)) args = ["docker", "--bin-dir", str(bin_dir)] - env = {"CLOUDSMITH_WORKSPACE": "acme"} + env = {"CLOUDSMITH_PROFILE": "work", "CLOUDSMITH_WORKSPACE": "acme"} installed = runner.invoke( install_cmd, [*args, "--domain", "acme.example.com", "--no-discover"], env=env ) unscoped = runner.invoke(uninstall_cmd, args, env=env) scoped_launchers = sorted(path.name for path in bin_dir.iterdir()) - scoped = runner.invoke(uninstall_cmd, [*args, "-w", "acme"]) + scoped = runner.invoke( + main, ["-P", "work", "credential-helper", "uninstall", *args, "-w", "acme"] + ) assert [installed.exit_code, unscoped.exit_code, scoped.exit_code] == [0, 0, 0] - assert scoped_launchers == ["docker-credential-cloudsmith-acme"] + assert scoped_launchers == ["docker-credential-cloudsmith-work-acme"] assert not list(bin_dir.iterdir()) cfg = json.loads((tmp_path / ".docker" / "config.json").read_text()) assert "credHelpers" not in cfg +def test_docker_refuses_the_helper_of_another_install(runner, tmp_path, monkeypatch): + """-P acme and -w acme give the same helper name.""" + from ....cli.commands.credential_helper.manage import uninstall_cmd + + monkeypatch.setenv("DOCKER_CONFIG", str(tmp_path / ".docker")) + bin_dir = tmp_path / "bin" + installer = DockerInstaller() + installer.install( + bin_dir=str(bin_dir), + discover=False, + launcher_env={"CLOUDSMITH_PROFILE": "acme"}, + ) + launcher = bin_dir / "docker-credential-cloudsmith-acme" + content = launcher.read_text() + + with pytest.raises(HelperScopeConflict, match="uninstall docker -P acme$"): + installer.install( + bin_dir=str(bin_dir), + discover=False, + launcher_env={"CLOUDSMITH_WORKSPACE": "acme"}, + ) + result = runner.invoke( + uninstall_cmd, ["docker", "--bin-dir", str(bin_dir), "-w", "acme"] + ) + + assert result.exit_code == 1 + assert "uninstall docker -P acme" in result.output + assert launcher.read_text() == content + + def test_pnpm_installer_install(tmp_path: Path, monkeypatch: MonkeyPatch): """install sets default+extra domains, preserves foreign entries, writes the launcher.""" npm_path = tmp_path / ".npmrc" @@ -788,30 +841,6 @@ def test_docker_installer_status_type_contract(tmp_path, monkeypatch): assert not isinstance(launcher, Path) -def test_docker_installer_status_reports_scoped_helpers(tmp_path, monkeypatch): - monkeypatch.setenv("DOCKER_CONFIG", str(tmp_path / ".docker")) - bin_dir = tmp_path / "bin" - installer = DockerInstaller() - installer.install(bin_dir=str(bin_dir), discover=False) - installer.install( - bin_dir=str(bin_dir), - domains=("acme.example.com",), - discover=False, - workspace="acme", - ) - - with patch( - "cloudsmith_cli.credential_helpers.docker.installer.resolve_bin_dir", - return_value=bin_dir, - ): - status = installer.status() - - assert status == { - "launcher": str(bin_dir / "docker-credential-cloudsmith-acme"), - "hosts": ["docker.cloudsmith.io", "acme.example.com"], - } - - def test_pnpm_installer_status_type_contract(tmp_path: Path, monkeypatch: MonkeyPatch): """status()['launcher'] is str when installed and None when not — never a Path. diff --git a/cloudsmith_cli/credential_helpers/docker/installer.py b/cloudsmith_cli/credential_helpers/docker/installer.py index a0b34046c..0be989a4c 100644 --- a/cloudsmith_cli/credential_helpers/docker/installer.py +++ b/cloudsmith_cli/credential_helpers/docker/installer.py @@ -1,7 +1,7 @@ # Copyright 2026 Cloudsmith Ltd """Installer for the Docker credential helper. -Manages writing/removing the ``docker-credential-cloudsmith`` launcher and +Manages writing/removing the ``docker-credential-cloudsmith*`` launchers and patching ``~/.docker/config.json`` to enable the helper for Cloudsmith registry hosts. """ @@ -12,6 +12,7 @@ import logging import os import re +import shlex from pathlib import Path from typing import TYPE_CHECKING @@ -23,6 +24,7 @@ helper_command, is_on_path, is_windows, + parse_launcher_env, remove_launcher, resolve_bin_dir, write_launcher, @@ -46,33 +48,69 @@ def _docker_config_path() -> Path: return Path.home() / ".docker" / "config.json" -def _helper_name(profile: str | None, workspace: str | None) -> str: - """Return the ``credHelpers`` value for an install with these settings. +# The profile and the Workspace of an install name its helper. +_SCOPE_ENVVARS = ("CLOUDSMITH_PROFILE", "CLOUDSMITH_WORKSPACE") +_SCOPE_FLAGS = ("-P", "-w") + + +class HelperScopeConflict(ValueError): + """Raised when the launcher of another profile or Workspace has the name.""" + + +def _helper_name(scope: tuple[str | None, ...]) -> str: + """Return the ``credHelpers`` value for an install with this scope. Each profile and Workspace gets its own launcher, so that installs for different configurations can coexist. """ segments = [ re.sub(r"[^a-z0-9-]+", "-", value.lower()).strip("-") - for value in (profile, workspace) + for value in scope if value ] return "-".join(["cloudsmith", *filter(None, segments)]) +def _check_scope(launcher_path: Path, scope: tuple[str | None, ...]) -> None: + """Raise :class:`HelperScopeConflict` if another scope wrote the launcher. + + Different scopes can give the same name, for example ``-P acme`` and + ``-w acme``. + """ + if not launcher_path.exists(): + return + env = parse_launcher_env(launcher_path.read_text(encoding="utf-8")) + installed = tuple(env.get(name) for name in _SCOPE_ENVVARS) + if installed != scope: + flags = "".join( + f" {flag} {shlex.quote(value)}" + for flag, value in zip(_SCOPE_FLAGS, installed) + if value + ) + raise HelperScopeConflict( + f"{launcher_path} is the helper of an install with a different" + " profile or Workspace. To remove it, run: cloudsmith" + f" credential-helper uninstall docker{flags}" + ) + + def _is_cloudsmith_helper(value) -> bool: return value == "cloudsmith" or str(value).startswith("cloudsmith-") +def _launcher_name(helper: str) -> str: + return f"docker-credential-{helper}" + + def _launcher_path(target_dir: Path, helper: str) -> Path: - name = f"docker-credential-{helper}" + name = _launcher_name(helper) return target_dir / (f"{name}.cmd" if is_windows() else name) class DockerInstaller: """Manages installation of the Docker credential helper for Cloudsmith. - This installer writes a ``docker-credential-cloudsmith`` launcher binary + This installer writes a ``docker-credential-cloudsmith*`` launcher binary and patches ``~/.docker/config.json`` to route the configured registry hosts through the Cloudsmith credential helper. @@ -84,7 +122,6 @@ class DockerInstaller: print(action) """ - LAUNCHER_NAME = "docker-credential-cloudsmith" DEFAULT_HOST = "docker.cloudsmith.io" # Docker runs the helper from Docker Desktop, IDEs and other shells, so # the launcher keeps the install-time settings. @@ -103,8 +140,6 @@ def install( org: str | None = None, credential: CredentialResult | None = None, api_host: str | None = None, - profile: str | None = None, - workspace: str | None = None, launcher_env: dict[str, str] | None = None, dry_run: bool = False, ) -> list[str]: @@ -134,10 +169,9 @@ def install( Resolved credential used for custom-domain discovery. api_host: Cloudsmith API host URL override. - profile, workspace: - The install-time profile and Workspace, which name the helper. launcher_env: - ``CLOUDSMITH_*`` settings that the launcher sets. + ``CLOUDSMITH_*`` settings that the launcher sets. The profile and + the Workspace in them name the helper. dry_run: When ``True``, compute and return planned actions without writing any files. @@ -151,8 +185,10 @@ def install( target_dir = resolve_bin_dir(bin_dir) config_path = _docker_config_path() launcher_env = launcher_env or {} - helper = _helper_name(profile, workspace) - launcher_name = f"docker-credential-{helper}" + scope = tuple(launcher_env.get(name) for name in _SCOPE_ENVVARS) + helper = _helper_name(scope) + launcher_name = _launcher_name(helper) + _check_scope(_launcher_path(target_dir, helper), scope) actions: list[str] = [] @@ -315,8 +351,10 @@ def uninstall( """ target_dir = resolve_bin_dir(bin_dir) config_path = _docker_config_path() - helper = _helper_name(profile, workspace) - launcher_name = f"docker-credential-{helper}" + scope = (profile, workspace) + helper = _helper_name(scope) + launcher_name = _launcher_name(helper) + _check_scope(_launcher_path(target_dir, helper), scope) def mutate(config: dict) -> None: helpers = config.get("credHelpers") @@ -375,8 +413,8 @@ def status(self) -> dict: A dict with keys: ``"launcher"`` - The path of the launcher that the first host uses, else of - the unscoped launcher, if it exists. Else ``None``. + The path of the first launcher that exists, of the helpers + of the hosts and then of the unscoped helper. Else ``None``. ``"hosts"`` List of hostnames in ``config.json``'s ``credHelpers`` block that use a ``cloudsmith`` or ``cloudsmith-*`` helper. diff --git a/cloudsmith_cli/credential_helpers/launchers.py b/cloudsmith_cli/credential_helpers/launchers.py index ceabd26db..675884764 100644 --- a/cloudsmith_cli/credential_helpers/launchers.py +++ b/cloudsmith_cli/credential_helpers/launchers.py @@ -114,6 +114,21 @@ def _launcher_content( return f'#!/bin/sh\n{settings}exec {target_cmd} "$@"\n' +def parse_launcher_env(content: str) -> dict[str, str]: + """Return the environment variables that a launcher body sets.""" + env = {} + for line in content.splitlines(): + if line.startswith("export "): + setting = shlex.split(line)[1] + elif line.startswith('set "'): + setting = line[len('set "') : -1] + else: + continue + name, _, value = setting.partition("=") + env[name] = value + return env + + def write_launcher( bin_dir: Path, name: str, From 34a43a1ef074429952139177ac94a5fa884b3963 Mon Sep 17 00:00:00 2001 From: Ian Duffy Date: Thu, 22 Oct 2026 17:29:46 +0100 Subject: [PATCH 05/16] fix(ENG-15016): list each Docker helper and accept --org in uninstall - Show one `list` entry for each Cloudsmith Docker helper, with its launcher and its hosts. A host whose helper has no launcher shows "not installed". - Accept `--org`, `--organization`, `-o` and `--owner` for the Workspace in `uninstall`, as `install` does. Co-Authored-By: Claude Opus 5.5 --- CHANGELOG.md | 2 +- .../cli/commands/credential_helper/manage.py | 11 ++++- .../test_credential_helper_install.py | 46 ++++++++++------- .../credential_helpers/docker/installer.py | 49 +++++++++---------- 4 files changed, 63 insertions(+), 45 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 858e416c3..1fb41bff9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -25,7 +25,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0. - `cloudsmith credential-helper install terraform` now writes `-w ` into the `terraformrc` args, not `--org`. A block that has `--org` continues to work. The install also writes `--api-host` and `--proxy` when you set them, so the helper finds the domains on that API host through that proxy. A proxy URL with a user name or password is not written, because Terraform passes the args on the command line of the helper; set `CLOUDSMITH_PROXY` where `terraform init` runs instead. Without a Workspace, the install tells you how to set one. - `cloudsmith credential-helper install` and `uninstall` now use `~/.cloudsmith/bin` when you do not give `--bin-dir`. Before, they used the directory of the `cloudsmith` executable, which a package manager can overwrite or make read-only. Terraform still uses its plugin directory. If the directory is not on `PATH`, `install` shows the command that adds it. To remove a helper that an earlier version installed, give its directory with `--bin-dir`. - The hint shown for a `401 - Unauthorized` error when no API key or SAML session is set no longer suggests `cloudsmith token`. It now suggests `CLOUDSMITH_API_KEY` or `cloudsmith auth`. -- `cloudsmith credential-helper install docker` now writes the `CLOUDSMITH_*` settings that you give as a flag or an environment variable into the launcher. These settings include the Workspace, profile, config and credentials files, API host, TLS verification, OIDC and keyring settings. Docker then gives the same result from Docker Desktop, an IDE or a new shell. The launcher never contains API keys, custom headers or the proxy URL. An install with a profile or a Workspace writes its own launcher, for example `docker-credential-cloudsmith--`, so that the custom domains of different Workspaces can use different helpers. A host that more than one install configures, such as `docker.cloudsmith.io`, uses the helper of the last install, and the install shows a warning when it moves such a host. An install without a profile or a Workspace writes `docker-credential-cloudsmith`, as before. Two installs can get the same name, for example with `-P acme` and with `-w acme`. Then the second install stops and shows the command that removes the first. `uninstall docker -P -w ` removes only the helper of that install. Without these flags, `uninstall docker` removes `docker-credential-cloudsmith`, as before. `list` shows the hosts of all Cloudsmith helpers. +- `cloudsmith credential-helper install docker` now writes the `CLOUDSMITH_*` settings that you give as a flag or an environment variable into the launcher. These settings include the Workspace, profile, config and credentials files, API host, TLS verification, OIDC and keyring settings. Docker then gives the same result from Docker Desktop, an IDE or a new shell. The launcher never contains API keys, custom headers or the proxy URL. An install with a profile or a Workspace writes its own launcher, for example `docker-credential-cloudsmith--`, so that the custom domains of different Workspaces can use different helpers. A host that more than one install configures, such as `docker.cloudsmith.io`, uses the helper of the last install, and the install shows a warning when it moves such a host. An install without a profile or a Workspace writes `docker-credential-cloudsmith`, as before. Two installs can get the same name, for example with `-P acme` and with `-w acme`. Then the second install stops and shows the command that removes the first. `uninstall docker -P -w ` removes only the helper of that install. Without these flags, `uninstall docker` removes `docker-credential-cloudsmith`, as before. `list` shows each Cloudsmith helper with its launcher and its hosts. - Messages, help text and the README now say SAML instead of SSO. JSON keys and values, such as `"method": "sso"`, do not change. ### Deprecated diff --git a/cloudsmith_cli/cli/commands/credential_helper/manage.py b/cloudsmith_cli/cli/commands/credential_helper/manage.py index cc23cc841..568aabe62 100644 --- a/cloudsmith_cli/cli/commands/credential_helper/manage.py +++ b/cloudsmith_cli/cli/commands/credential_helper/manage.py @@ -449,6 +449,11 @@ def install_cmd( @click.option( "-w", "--workspace", + "--org", + "--organization", + "-o", + "--owner", + "workspace", help="Docker only: remove the helper that was installed for this Workspace.", ) @common_cli_config_options @@ -532,14 +537,16 @@ def list_cmd(ctx, opts) -> None: data = [] for name, cls in sorted(_INSTALLERS.items()): installer = cls() - st = installer.status() - data.append( + # Docker can have one helper for each profile and Workspace. + statuses = getattr(installer, "statuses", None) + data.extend( { "helper": name, "summary": installer.summary, "launcher": st.get("launcher"), "hosts": st.get("hosts", []), } + for st in (statuses() if statuses else [installer.status()]) ) if utils.maybe_print_as_json(opts, data): diff --git a/cloudsmith_cli/cli/tests/commands/test_credential_helper_install.py b/cloudsmith_cli/cli/tests/commands/test_credential_helper_install.py index 973e946b7..da0c9ebc3 100644 --- a/cloudsmith_cli/cli/tests/commands/test_credential_helper_install.py +++ b/cloudsmith_cli/cli/tests/commands/test_credential_helper_install.py @@ -420,7 +420,9 @@ def test_docker_install_scopes_the_helper_to_the_install_time_settings( bin_dir = tmp_path / "bin" monkeypatch.setenv("PATH", str(bin_dir)) installer = DockerInstaller() - installer.install(bin_dir=str(bin_dir), discover=False) + installer.install( + bin_dir=str(bin_dir), domains=("legacy.example.com",), discover=False + ) actions = installer.install( bin_dir=str(bin_dir), @@ -440,9 +442,11 @@ def test_docker_install_scopes_the_helper_to_the_install_time_settings( "export CLOUDSMITH_API_HOST=https://api.example.com\n" 'exec cloudsmith credential-helper docker "$@"\n' ) - assert (bin_dir / "docker-credential-cloudsmith").exists() cfg = json.loads((tmp_path / ".docker" / "config.json").read_text()) - assert cfg["credHelpers"] == {"docker.cloudsmith.io": name} + assert cfg["credHelpers"] == { + "docker.cloudsmith.io": name, + "legacy.example.com": "cloudsmith", + } assert ( "WARNING: credHelpers['docker.cloudsmith.io'] changes from 'cloudsmith'" f" to {name!r}" @@ -451,10 +455,16 @@ def test_docker_install_scopes_the_helper_to_the_install_time_settings( "cloudsmith_cli.credential_helpers.docker.installer.resolve_bin_dir", return_value=bin_dir, ): - assert installer.status() == { - "launcher": str(bin_dir / f"docker-credential-{name}"), - "hosts": ["docker.cloudsmith.io"], - } + assert installer.statuses() == [ + { + "launcher": str(bin_dir / f"docker-credential-{name}"), + "hosts": ["docker.cloudsmith.io"], + }, + { + "launcher": str(bin_dir / "docker-credential-cloudsmith"), + "hosts": ["legacy.example.com"], + }, + ] def test_docker_dry_run_shows_the_scoped_launcher_and_its_settings( @@ -499,7 +509,7 @@ def test_docker_uninstall_takes_the_scope_from_flags_only( unscoped = runner.invoke(uninstall_cmd, args, env=env) scoped_launchers = sorted(path.name for path in bin_dir.iterdir()) scoped = runner.invoke( - main, ["-P", "work", "credential-helper", "uninstall", *args, "-w", "acme"] + main, ["-P", "work", "credential-helper", "uninstall", *args, "-o", "acme"] ) assert [installed.exit_code, unscoped.exit_code, scoped.exit_code] == [0, 0, 0] @@ -804,7 +814,7 @@ def test_pnpm_installer_uninstall(tmp_path: Path, monkeypatch): def test_docker_installer_status_type_contract(tmp_path, monkeypatch): - """status()['launcher'] is str when installed and None when not — never a Path. + """statuses()[0]['launcher'] is a str when installed, else None, never a Path. Retained guard: the -F json Path-serialization regression. """ @@ -819,7 +829,7 @@ def test_docker_installer_status_type_contract(tmp_path, monkeypatch): "cloudsmith_cli.credential_helpers.docker.installer.resolve_bin_dir", return_value=bin_dir, ): - result_before = installer.status() + [result_before] = installer.statuses() assert result_before["launcher"] is None assert not isinstance(result_before["launcher"], Path) @@ -830,7 +840,7 @@ def test_docker_installer_status_type_contract(tmp_path, monkeypatch): "cloudsmith_cli.credential_helpers.docker.installer.resolve_bin_dir", return_value=bin_dir, ): - result_after = installer.status() + [result_after] = installer.statuses() launcher = result_after["launcher"] assert launcher is not None @@ -1348,11 +1358,13 @@ def test_output_format_json( Retained guard: list -F json serialises a launcher path (str), not a Path object. """ - def _docker_stub_status_fn(_self): - return { - "launcher": "/some/bin/docker-credential-cloudsmith", - "hosts": ["docker.cloudsmith.io"], - } + def _docker_stub_statuses_fn(_self): + return [ + { + "launcher": "/some/bin/docker-credential-cloudsmith", + "hosts": ["docker.cloudsmith.io"], + } + ] def _pnpm_stub_status_fn(_self): return { @@ -1362,7 +1374,7 @@ def _pnpm_stub_status_fn(_self): monkeypatch.setenv("DOCKER_CONFIG", str(tmp_path / ".docker")) monkeypatch.setenv("NPM_CONFIG_USERCONFIG", str(tmp_path / ".npmrc")) - monkeypatch.setattr(DockerInstaller, "status", _docker_stub_status_fn) + monkeypatch.setattr(DockerInstaller, "statuses", _docker_stub_statuses_fn) monkeypatch.setattr(PNPMInstaller, "status", _pnpm_stub_status_fn) from ....cli.commands.credential_helper import manage as manage_mod diff --git a/cloudsmith_cli/credential_helpers/docker/installer.py b/cloudsmith_cli/credential_helpers/docker/installer.py index 0be989a4c..9e77abeb9 100644 --- a/cloudsmith_cli/credential_helpers/docker/installer.py +++ b/cloudsmith_cli/credential_helpers/docker/installer.py @@ -404,44 +404,43 @@ def mutate(config: dict) -> None: return actions - def status(self) -> dict: - """Return current installation status. + def statuses(self) -> list[dict]: + """Return the status of each Cloudsmith helper in ``config.json``. Returns ------- - dict - A dict with keys: + list[dict] + One dict for each ``cloudsmith`` or ``cloudsmith-*`` helper, in the + order of the ``credHelpers`` block, with keys: ``"launcher"`` - The path of the first launcher that exists, of the helpers - of the hosts and then of the unscoped helper. Else ``None``. + The path of the launcher of the helper, or ``None`` if it does + not exist. ``"hosts"`` - List of hostnames in ``config.json``'s ``credHelpers`` block - that use a ``cloudsmith`` or ``cloudsmith-*`` helper. + List of hostnames that use the helper. + + Without such a helper, the list has only the unscoped helper. """ config_path = _docker_config_path() - helpers: dict = {} + hosts_by_helper: dict[str, list[str]] = {} if config_path.exists(): try: data = json.loads(config_path.read_text(encoding="utf-8")) if isinstance(data, dict): - helpers = { - k: v - for k, v in data.get("credHelpers", {}).items() - if _is_cloudsmith_helper(v) - } + for host, helper in data.get("credHelpers", {}).items(): + if _is_cloudsmith_helper(helper): + hosts_by_helper.setdefault(helper, []).append(host) except (json.JSONDecodeError, OSError): pass - hosts = list(helpers) target_dir = resolve_bin_dir() - candidates = [ - _launcher_path(target_dir, helper) - for helper in [*helpers.values(), "cloudsmith"] - ] - launcher_path = next((path for path in candidates if path.exists()), None) - - return { - "launcher": str(launcher_path) if launcher_path is not None else None, - "hosts": hosts, - } + statuses = [] + for helper, hosts in (hosts_by_helper or {"cloudsmith": []}).items(): + launcher_path = _launcher_path(target_dir, helper) + statuses.append( + { + "launcher": str(launcher_path) if launcher_path.exists() else None, + "hosts": hosts, + } + ) + return statuses From 0352b88e0ab7d90a76294e94d846fa6c094c532f Mon Sep 17 00:00:00 2001 From: Ian Duffy Date: Wed, 28 Oct 2026 09:36:50 +0000 Subject: [PATCH 06/16] fix(no-ticket): end the pnpm .npmrc with a newline (#528) The pnpm install wrote .npmrc without a final newline. A shell append such as `echo ... >> ~/.npmrc` then joined the last tokenHelper line. Write a final newline after each line. Add the newline to an existing .npmrc that does not have one. Show each tokenHelper line that a real install sets, as the dry run does. Co-authored-by: Claude Opus 5.5 Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: cloudsmith-iduffy <178375997+cloudsmith-iduffy@users.noreply.github.com> From 66fb58855795534ac0a94cc2b97824cc1da686ef Mon Sep 17 00:00:00 2001 From: Ian Duffy Date: Wed, 21 Oct 2026 13:57:10 +0100 Subject: [PATCH 07/16] feat(ENG-15016): let credential-helper launchers keep install-time settings A package manager can run its credential helper from an IDE, Docker Desktop or a build agent that does not have the environment of the install. This change adds the shared code that writes the install-time CLOUDSMITH_* settings into a launcher. - `write_launcher(env=...)` in `launchers.py` writes the settings as `export` lines, or as `set` lines in a Windows batch file. The settings replace the values of the caller. - `batch_value()` refuses a value that a batch file cannot hold. A non-ASCII path under the user profile becomes `%USERPROFILE%`. - `install` passes `launcher_env` to an installer that sets `KEEPS_INSTALL_TIME_SETTINGS`. It holds each option with a CLOUDSMITH_* environment variable whose value came from a flag or the environment, also on a parent command, so the keyring settings of `main` are kept too. The API key, the custom headers, the keyring key and the proxy never go into it. No installer sets `KEEPS_INSTALL_TIME_SETTINGS` yet. Co-Authored-By: Claude Opus 5.5 --- .../cli/commands/credential_helper/manage.py | 46 ++++++++ .../test_credential_helper_install.py | 107 ++++++++++++++++++ .../credential_helpers/launchers.py | 84 ++++++++++++-- 3 files changed, 226 insertions(+), 11 deletions(-) diff --git a/cloudsmith_cli/cli/commands/credential_helper/manage.py b/cloudsmith_cli/cli/commands/credential_helper/manage.py index 2f2464b9b..b999909e1 100644 --- a/cloudsmith_cli/cli/commands/credential_helper/manage.py +++ b/cloudsmith_cli/cli/commands/credential_helper/manage.py @@ -12,9 +12,11 @@ from urllib.parse import urlsplit import click +from click.core import ParameterSource from cloudsmith_cli.credential_helpers.cargo.installer import CargoInstaller from cloudsmith_cli.credential_helpers.generic import PartialInstallError +from cloudsmith_cli.credential_helpers.launchers import UnsafeLauncherValue from cloudsmith_cli.credential_helpers.pnpm.installer import PNPMInstaller from cloudsmith_cli.credential_helpers.terraform.installer import ( TerraformHelperExeNotFound, @@ -134,6 +136,46 @@ def _terraform_proxy_next_steps(opts) -> list[str]: ] +# Other users can read a launcher, so it never holds a secret. A proxy URL can +# hold credentials. +_NEVER_KEPT = ("api_key", "api_headers", "keyring_key", "proxy") + + +def _main_envvar(option) -> str: + """Return the main environment variable of *option*.""" + names = option.envvar + return names if isinstance(names, str) else names[0] + + +def _launcher_settings_from_cli(ctx) -> dict[str, str]: + """Return the ``CLOUDSMITH_*`` settings that a launcher keeps. + + An option with a ``CLOUDSMITH_*`` environment variable is kept when its + value came from a flag or from the environment. A value from a config + file is not kept, because the helper reads that file itself. An option + of this command replaces the same option of a parent command. + """ + contexts = [] + while ctx is not None: + contexts.insert(0, ctx) + ctx = ctx.parent + + supplied = (ParameterSource.COMMANDLINE, ParameterSource.ENVIRONMENT) + settings = {} + for context in contexts: + for option in context.command.params: + if option.name in _NEVER_KEPT or not option.envvar: + continue + envvar = _main_envvar(option) + source = context.get_parameter_source(option.name) + if envvar.startswith("CLOUDSMITH_") and source in supplied: + value = context.params[option.name] + settings[envvar] = ( + str(value).lower() if isinstance(value, bool) else value + ) + return settings + + _REPO_FLAGS = ("-r", "--repo", "--repository") @@ -310,12 +352,16 @@ def install_cmd( helper_args = _terraform_helper_args(ctx, opts, repo) install_kwargs["helper_args"] = helper_args + if getattr(installer, "KEEPS_INSTALL_TIME_SETTINGS", False): + install_kwargs["launcher_env"] = _launcher_settings_from_cli(ctx) + try: actions = installer.install(**install_kwargs) except ( TerraformrcConflictError, TerraformHelperExeNotFound, TerraformPluginDirError, + UnsafeLauncherValue, ) as exc: raise click.ClickException(str(exc)) except OSError as exc: diff --git a/cloudsmith_cli/cli/tests/commands/test_credential_helper_install.py b/cloudsmith_cli/cli/tests/commands/test_credential_helper_install.py index 7a28b06ca..7cc93f794 100644 --- a/cloudsmith_cli/cli/tests/commands/test_credential_helper_install.py +++ b/cloudsmith_cli/cli/tests/commands/test_credential_helper_install.py @@ -20,6 +20,7 @@ from ....core.credentials.models import CredentialResult from ....credential_helpers.docker.installer import DockerInstaller from ....credential_helpers.launchers import ( + UnsafeLauncherValue, _launcher_content, _launcher_filename, is_on_path, @@ -42,6 +43,13 @@ def runner(): return click.testing.CliRunner() +@pytest.fixture(autouse=True) +def clear_cloudsmith_env(monkeypatch): + """Keep the CLOUDSMITH_* settings of the developer shell out of the installs.""" + for name in [name for name in os.environ if name.startswith("CLOUDSMITH_")]: + monkeypatch.delenv(name) + + # --------------------------------------------------------------------------- # 1. launcher filename + content (pure, platform-parameterised) # --------------------------------------------------------------------------- @@ -95,6 +103,105 @@ def test_launcher_filename_and_content( ) +@pytest.mark.parametrize( + "profile,value,expected", + [ + ("C:\\Users\\jo", "C:\\Users\\jo\\a b&c!", "C:\\Users\\jo\\a b&c!"), + ( + "C:\\Users\\José", + "c:\\users\\josé\\cs\\config.ini", + "%USERPROFILE%\\cs\\config.ini", + ), + ], +) +def test_windows_launcher_settings_are_ascii(monkeypatch, profile, value, expected): + """cmd.exe reads a batch file in the console code page, not UTF-8. + + Only a non-ASCII path under the user profile is written as %USERPROFILE%, + because another account that runs the package manager has a different + profile. + """ + monkeypatch.setenv("USERPROFILE", profile) + content = _launcher_content("cmd", windows=True, env={"CLOUDSMITH_X": value}) + assert content == ( + "@echo off\r\nsetlocal DisableDelayedExpansion\r\n" + f'set "CLOUDSMITH_X={expected}"\r\ncmd %*\r\n' + ) + + +@pytest.mark.parametrize("value", ["%PATH%", "D:\\Données"]) +def test_windows_launcher_refuses_a_value_a_batch_file_cannot_hold(monkeypatch, value): + monkeypatch.setenv("USERPROFILE", "C:\\Users\\José") + with pytest.raises(UnsafeLauncherValue, match="CLOUDSMITH_PROFILE"): + _launcher_content("cmd", windows=True, env={"CLOUDSMITH_PROFILE": value}) + + +def test_install_passes_flag_and_environment_settings_to_the_launcher( + runner, monkeypatch, tmp_path +): + """Settings from flags and CLOUDSMITH_* variables go to the launcher. + + A value read from a config file does not, because the helper reads that + file itself. Secrets and the proxy never go to the launcher. A flag on + the parent command also counts. + """ + from ....cli.commands.credential_helper import manage + from ....cli.commands.main import main + + launcher_env = {} + + class SettingsInstaller: + KEEPS_INSTALL_TIME_SETTINGS = True + + def install(self, **kwargs): + launcher_env.update(kwargs["launcher_env"]) + return [] + + monkeypatch.setitem(manage._INSTALLERS, "settings", SettingsInstaller) + config = tmp_path / "config.ini" + config.write_text("[default]\nssl_verify_strict = false\n") + result = runner.invoke( + main, + args=[ + "-P", + "work", + "credential-helper", + "install", + "settings", + "-w", + "acme", + "--config-file", + str(config), + "--api-host", + "https://api.example.com", + "--proxy", + "http://proxy.example.com:3128", + "-S", + ], + env={ + "CLOUDSMITH_API_KEY": "k_secret", + "CLOUDSMITH_API_HEADERS": "X-Secret=1", + "CLOUDSMITH_KEYRING_KEY": "keyring_secret", + "CLOUDSMITH_KEYRING_DIR": "/var/keyring", + "CLOUDSMITH_OIDC_AUDIENCE": "cloudsmith-test", + "CLOUDSMITH_SERVICE_SLUG": "ci-bot", + }, + catch_exceptions=False, + ) + + assert result.exit_code == 0, result.output + assert launcher_env == { + "CLOUDSMITH_WORKSPACE": "acme", + "CLOUDSMITH_PROFILE": "work", + "CLOUDSMITH_CONFIG_FILE": str(config.resolve()), + "CLOUDSMITH_API_HOST": "https://api.example.com", + "CLOUDSMITH_WITHOUT_API_SSL_VERIFY": "true", + "CLOUDSMITH_OIDC_AUDIENCE": "cloudsmith-test", + "CLOUDSMITH_SERVICE_SLUG": "ci-bot", + "CLOUDSMITH_KEYRING_DIR": "/var/keyring", + } + + # --------------------------------------------------------------------------- # 2. write_launcher / remove_launcher — end-to-end on the host platform # --------------------------------------------------------------------------- diff --git a/cloudsmith_cli/credential_helpers/launchers.py b/cloudsmith_cli/credential_helpers/launchers.py index 8086e6f28..ceabd26db 100644 --- a/cloudsmith_cli/credential_helpers/launchers.py +++ b/cloudsmith_cli/credential_helpers/launchers.py @@ -29,10 +29,49 @@ def _launcher_filename(name: str, *, windows: bool) -> str: return f"{name}.cmd" if windows else name -def launcher_arg(value: str) -> str: +# A batch file expands % even inside quotes and cannot hold " or a line break. +_UNSAFE_IN_BATCH = ('"', "%", "\r", "\n") + + +class UnsafeLauncherValue(ValueError): + """Raised when a value cannot go into a launcher script.""" + + +def batch_value(name: str, value: str) -> str: + """Return *value* in a form that a batch file holds without change. + + ``cmd.exe`` reads a batch file in the console code page, so the file must + be ASCII. A path under the user profile is written as ``%USERPROFILE%``, + which ``cmd.exe`` expands to the correct Unicode path when it runs. An + ASCII path stays literal, because another account that runs the package + manager has a different profile. + """ + if any(char in value for char in _UNSAFE_IN_BATCH): + raise UnsafeLauncherValue( + f"{name} {value!r} cannot go into the launcher: a batch file cannot " + "hold a '\"', a '%' or a line break." + ) + profile = os.environ.get("USERPROFILE", "") + lowered = value.lower() + if ( + not value.isascii() + and profile + and (lowered == profile.lower() or lowered.startswith(profile.lower() + "\\")) + ): + value = "%USERPROFILE%" + value[len(profile) :] + if not value.isascii(): + raise UnsafeLauncherValue( + f"{name} {value!r} cannot go into the launcher: outside the user " + "profile, a batch file can only hold ASCII characters. Set it in " + "the environment that runs the package manager instead." + ) + return value + + +def launcher_arg(name: str, value: str) -> str: """Return *value* quoted as one argument of a launcher command.""" if is_windows(): - return f'"{value}"' + return f'"{batch_value(name, value)}"' return shlex.quote(value) @@ -43,23 +82,45 @@ def helper_command(helper: str) -> str: ``cloudsmith``, so the launcher runs the absolute executable. """ if getattr(sys, "frozen", False): - return f"{launcher_arg(sys.executable)} credential-helper {helper}" + executable = launcher_arg("cloudsmith", sys.executable) + return f"{executable} credential-helper {helper}" return f"cloudsmith credential-helper {helper}" -def _launcher_content(target_cmd: str, *, windows: bool) -> str: +def _launcher_content( + target_cmd: str, *, windows: bool, env: dict[str, str] | None = None +) -> str: """Return the launcher script body for the platform. Windows uses a ``.cmd`` batch file (``@echo off`` keeps stdout clean for Docker's credential JSON); Unix uses a ``#!/bin/sh`` script that ``exec``s - the target so signals and the exit code pass straight through. + the target so signals and the exit code pass straight through. The + ``env`` variables replace the caller's values, so the helper gives the + same result as the install. """ + env = env or {} if windows: - return f"@echo off\r\n{target_cmd} %*\r\n" - return f'#!/bin/sh\nexec {target_cmd} "$@"\n' + settings = "".join( + f'set "{name}={batch_value(name, value)}"\r\n' + for name, value in env.items() + ) + if settings: + # Keep "!" literal and the settings local to the launcher. + settings = f"setlocal DisableDelayedExpansion\r\n{settings}" + return f"@echo off\r\n{settings}{target_cmd} %*\r\n" + settings = "".join( + f"export {name}={shlex.quote(value)}\n" for name, value in env.items() + ) + return f'#!/bin/sh\n{settings}exec {target_cmd} "$@"\n' -def write_launcher(bin_dir: Path, name: str, target_cmd: str, dry_run=False) -> Path: +def write_launcher( + bin_dir: Path, + name: str, + target_cmd: str, + dry_run=False, + env: dict[str, str] | None = None, +) -> Path: """Write a launcher script for *name* in *bin_dir* that execs *target_cmd*. Parameters @@ -71,6 +132,8 @@ def write_launcher(bin_dir: Path, name: str, target_cmd: str, dry_run=False) -> target_cmd: The command the launcher forwards to (e.g. ``cloudsmith credential-helper docker``). + env: + Environment variables that the launcher sets. Returns ------- @@ -78,6 +141,7 @@ def write_launcher(bin_dir: Path, name: str, target_cmd: str, dry_run=False) -> The path of the written file. """ windows = is_windows() + content = _launcher_content(target_cmd, windows=windows, env=env) if dry_run: if windows: return bin_dir / f"{name}.cmd" @@ -87,9 +151,7 @@ def write_launcher(bin_dir: Path, name: str, target_cmd: str, dry_run=False) -> bin_dir.mkdir(parents=True, exist_ok=True) dest = bin_dir / _launcher_filename(name, windows=windows) - dest.write_text( - _launcher_content(target_cmd, windows=windows), encoding="utf-8", newline="" - ) + dest.write_text(content, encoding="utf-8", newline="") if not windows: dest.chmod(0o755) From b7f68a528445aef8aa7c1446f034cc963871930f Mon Sep 17 00:00:00 2001 From: Ian Duffy Date: Mon, 2 Nov 2026 11:38:45 +0100 Subject: [PATCH 08/16] fix(no-ticket): stop retrying unknown hosts and show backoff retries (#542) Do not retry a request when DNS reports that the API host or the proxy host does not exist. Retry a temporary DNS failure as before. Write one line to stderr when the CLI waits before a backoff retry. Co-authored-by: Claude Opus 5.5 From 25fd194add707a27a1e269b1c2c82d072507a7f0 Mon Sep 17 00:00:00 2001 From: Ian Duffy Date: Wed, 21 Oct 2026 13:57:10 +0100 Subject: [PATCH 09/16] feat(ENG-15016): let credential-helper launchers keep install-time settings A package manager can run its credential helper from an IDE, Docker Desktop or a build agent that does not have the environment of the install. This change adds the shared code that writes the install-time CLOUDSMITH_* settings into a launcher. - `write_launcher(env=...)` in `launchers.py` writes the settings as `export` lines, or as `set` lines in a Windows batch file. The settings replace the values of the caller. - `batch_value()` refuses a value that a batch file cannot hold. A non-ASCII path under the user profile becomes `%USERPROFILE%`. - `install` passes `launcher_env` to an installer that sets `KEEPS_INSTALL_TIME_SETTINGS`. It holds each option with a CLOUDSMITH_* environment variable whose value came from a flag or the environment, also on a parent command, so the keyring settings of `main` are kept too. The API key, the custom headers, the keyring key and the proxy never go into it. No installer sets `KEEPS_INSTALL_TIME_SETTINGS` yet. Co-Authored-By: Claude Opus 5.5 --- .../cli/commands/credential_helper/manage.py | 46 ++++++++ .../test_credential_helper_install.py | 107 ++++++++++++++++++ .../credential_helpers/launchers.py | 84 ++++++++++++-- 3 files changed, 226 insertions(+), 11 deletions(-) diff --git a/cloudsmith_cli/cli/commands/credential_helper/manage.py b/cloudsmith_cli/cli/commands/credential_helper/manage.py index 60697fda3..3e0101dc7 100644 --- a/cloudsmith_cli/cli/commands/credential_helper/manage.py +++ b/cloudsmith_cli/cli/commands/credential_helper/manage.py @@ -12,10 +12,12 @@ from urllib.parse import urlsplit import click +from click.core import ParameterSource from cloudsmith_cli.core.cache_utils import ConfigFileParseError from cloudsmith_cli.credential_helpers.cargo.installer import CargoInstaller from cloudsmith_cli.credential_helpers.generic import PartialInstallError +from cloudsmith_cli.credential_helpers.launchers import UnsafeLauncherValue from cloudsmith_cli.credential_helpers.pnpm.installer import PNPMInstaller from cloudsmith_cli.credential_helpers.terraform.installer import ( TerraformHelperExeNotFound, @@ -135,6 +137,46 @@ def _terraform_proxy_next_steps(opts) -> list[str]: ] +# Other users can read a launcher, so it never holds a secret. A proxy URL can +# hold credentials. +_NEVER_KEPT = ("api_key", "api_headers", "keyring_key", "proxy") + + +def _main_envvar(option) -> str: + """Return the main environment variable of *option*.""" + names = option.envvar + return names if isinstance(names, str) else names[0] + + +def _launcher_settings_from_cli(ctx) -> dict[str, str]: + """Return the ``CLOUDSMITH_*`` settings that a launcher keeps. + + An option with a ``CLOUDSMITH_*`` environment variable is kept when its + value came from a flag or from the environment. A value from a config + file is not kept, because the helper reads that file itself. An option + of this command replaces the same option of a parent command. + """ + contexts = [] + while ctx is not None: + contexts.insert(0, ctx) + ctx = ctx.parent + + supplied = (ParameterSource.COMMANDLINE, ParameterSource.ENVIRONMENT) + settings = {} + for context in contexts: + for option in context.command.params: + if option.name in _NEVER_KEPT or not option.envvar: + continue + envvar = _main_envvar(option) + source = context.get_parameter_source(option.name) + if envvar.startswith("CLOUDSMITH_") and source in supplied: + value = context.params[option.name] + settings[envvar] = ( + str(value).lower() if isinstance(value, bool) else value + ) + return settings + + _REPO_FLAGS = ("-r", "--repo", "--repository") @@ -311,6 +353,9 @@ def install_cmd( helper_args = _terraform_helper_args(ctx, opts, repo) install_kwargs["helper_args"] = helper_args + if getattr(installer, "KEEPS_INSTALL_TIME_SETTINGS", False): + install_kwargs["launcher_env"] = _launcher_settings_from_cli(ctx) + try: actions = installer.install(**install_kwargs) except ( @@ -318,6 +363,7 @@ def install_cmd( TerraformrcConflictError, TerraformHelperExeNotFound, TerraformPluginDirError, + UnsafeLauncherValue, ) as exc: raise click.ClickException(str(exc)) except OSError as exc: diff --git a/cloudsmith_cli/cli/tests/commands/test_credential_helper_install.py b/cloudsmith_cli/cli/tests/commands/test_credential_helper_install.py index 9264fb87e..d0a7b136d 100644 --- a/cloudsmith_cli/cli/tests/commands/test_credential_helper_install.py +++ b/cloudsmith_cli/cli/tests/commands/test_credential_helper_install.py @@ -21,6 +21,7 @@ from ....core.credentials.models import CredentialResult from ....credential_helpers.docker.installer import DockerInstaller from ....credential_helpers.launchers import ( + UnsafeLauncherValue, _launcher_content, _launcher_filename, is_on_path, @@ -43,6 +44,13 @@ def runner(): return click.testing.CliRunner() +@pytest.fixture(autouse=True) +def clear_cloudsmith_env(monkeypatch): + """Keep the CLOUDSMITH_* settings of the developer shell out of the installs.""" + for name in [name for name in os.environ if name.startswith("CLOUDSMITH_")]: + monkeypatch.delenv(name) + + # --------------------------------------------------------------------------- # 1. launcher filename + content (pure, platform-parameterised) # --------------------------------------------------------------------------- @@ -96,6 +104,105 @@ def test_launcher_filename_and_content( ) +@pytest.mark.parametrize( + "profile,value,expected", + [ + ("C:\\Users\\jo", "C:\\Users\\jo\\a b&c!", "C:\\Users\\jo\\a b&c!"), + ( + "C:\\Users\\José", + "c:\\users\\josé\\cs\\config.ini", + "%USERPROFILE%\\cs\\config.ini", + ), + ], +) +def test_windows_launcher_settings_are_ascii(monkeypatch, profile, value, expected): + """cmd.exe reads a batch file in the console code page, not UTF-8. + + Only a non-ASCII path under the user profile is written as %USERPROFILE%, + because another account that runs the package manager has a different + profile. + """ + monkeypatch.setenv("USERPROFILE", profile) + content = _launcher_content("cmd", windows=True, env={"CLOUDSMITH_X": value}) + assert content == ( + "@echo off\r\nsetlocal DisableDelayedExpansion\r\n" + f'set "CLOUDSMITH_X={expected}"\r\ncmd %*\r\n' + ) + + +@pytest.mark.parametrize("value", ["%PATH%", "D:\\Données"]) +def test_windows_launcher_refuses_a_value_a_batch_file_cannot_hold(monkeypatch, value): + monkeypatch.setenv("USERPROFILE", "C:\\Users\\José") + with pytest.raises(UnsafeLauncherValue, match="CLOUDSMITH_PROFILE"): + _launcher_content("cmd", windows=True, env={"CLOUDSMITH_PROFILE": value}) + + +def test_install_passes_flag_and_environment_settings_to_the_launcher( + runner, monkeypatch, tmp_path +): + """Settings from flags and CLOUDSMITH_* variables go to the launcher. + + A value read from a config file does not, because the helper reads that + file itself. Secrets and the proxy never go to the launcher. A flag on + the parent command also counts. + """ + from ....cli.commands.credential_helper import manage + from ....cli.commands.main import main + + launcher_env = {} + + class SettingsInstaller: + KEEPS_INSTALL_TIME_SETTINGS = True + + def install(self, **kwargs): + launcher_env.update(kwargs["launcher_env"]) + return [] + + monkeypatch.setitem(manage._INSTALLERS, "settings", SettingsInstaller) + config = tmp_path / "config.ini" + config.write_text("[default]\nssl_verify_strict = false\n") + result = runner.invoke( + main, + args=[ + "-P", + "work", + "credential-helper", + "install", + "settings", + "-w", + "acme", + "--config-file", + str(config), + "--api-host", + "https://api.example.com", + "--proxy", + "http://proxy.example.com:3128", + "-S", + ], + env={ + "CLOUDSMITH_API_KEY": "k_secret", + "CLOUDSMITH_API_HEADERS": "X-Secret=1", + "CLOUDSMITH_KEYRING_KEY": "keyring_secret", + "CLOUDSMITH_KEYRING_DIR": "/var/keyring", + "CLOUDSMITH_OIDC_AUDIENCE": "cloudsmith-test", + "CLOUDSMITH_SERVICE_SLUG": "ci-bot", + }, + catch_exceptions=False, + ) + + assert result.exit_code == 0, result.output + assert launcher_env == { + "CLOUDSMITH_WORKSPACE": "acme", + "CLOUDSMITH_PROFILE": "work", + "CLOUDSMITH_CONFIG_FILE": str(config.resolve()), + "CLOUDSMITH_API_HOST": "https://api.example.com", + "CLOUDSMITH_WITHOUT_API_SSL_VERIFY": "true", + "CLOUDSMITH_OIDC_AUDIENCE": "cloudsmith-test", + "CLOUDSMITH_SERVICE_SLUG": "ci-bot", + "CLOUDSMITH_KEYRING_DIR": "/var/keyring", + } + + # --------------------------------------------------------------------------- # 2. write_launcher / remove_launcher — end-to-end on the host platform # --------------------------------------------------------------------------- diff --git a/cloudsmith_cli/credential_helpers/launchers.py b/cloudsmith_cli/credential_helpers/launchers.py index 8086e6f28..ceabd26db 100644 --- a/cloudsmith_cli/credential_helpers/launchers.py +++ b/cloudsmith_cli/credential_helpers/launchers.py @@ -29,10 +29,49 @@ def _launcher_filename(name: str, *, windows: bool) -> str: return f"{name}.cmd" if windows else name -def launcher_arg(value: str) -> str: +# A batch file expands % even inside quotes and cannot hold " or a line break. +_UNSAFE_IN_BATCH = ('"', "%", "\r", "\n") + + +class UnsafeLauncherValue(ValueError): + """Raised when a value cannot go into a launcher script.""" + + +def batch_value(name: str, value: str) -> str: + """Return *value* in a form that a batch file holds without change. + + ``cmd.exe`` reads a batch file in the console code page, so the file must + be ASCII. A path under the user profile is written as ``%USERPROFILE%``, + which ``cmd.exe`` expands to the correct Unicode path when it runs. An + ASCII path stays literal, because another account that runs the package + manager has a different profile. + """ + if any(char in value for char in _UNSAFE_IN_BATCH): + raise UnsafeLauncherValue( + f"{name} {value!r} cannot go into the launcher: a batch file cannot " + "hold a '\"', a '%' or a line break." + ) + profile = os.environ.get("USERPROFILE", "") + lowered = value.lower() + if ( + not value.isascii() + and profile + and (lowered == profile.lower() or lowered.startswith(profile.lower() + "\\")) + ): + value = "%USERPROFILE%" + value[len(profile) :] + if not value.isascii(): + raise UnsafeLauncherValue( + f"{name} {value!r} cannot go into the launcher: outside the user " + "profile, a batch file can only hold ASCII characters. Set it in " + "the environment that runs the package manager instead." + ) + return value + + +def launcher_arg(name: str, value: str) -> str: """Return *value* quoted as one argument of a launcher command.""" if is_windows(): - return f'"{value}"' + return f'"{batch_value(name, value)}"' return shlex.quote(value) @@ -43,23 +82,45 @@ def helper_command(helper: str) -> str: ``cloudsmith``, so the launcher runs the absolute executable. """ if getattr(sys, "frozen", False): - return f"{launcher_arg(sys.executable)} credential-helper {helper}" + executable = launcher_arg("cloudsmith", sys.executable) + return f"{executable} credential-helper {helper}" return f"cloudsmith credential-helper {helper}" -def _launcher_content(target_cmd: str, *, windows: bool) -> str: +def _launcher_content( + target_cmd: str, *, windows: bool, env: dict[str, str] | None = None +) -> str: """Return the launcher script body for the platform. Windows uses a ``.cmd`` batch file (``@echo off`` keeps stdout clean for Docker's credential JSON); Unix uses a ``#!/bin/sh`` script that ``exec``s - the target so signals and the exit code pass straight through. + the target so signals and the exit code pass straight through. The + ``env`` variables replace the caller's values, so the helper gives the + same result as the install. """ + env = env or {} if windows: - return f"@echo off\r\n{target_cmd} %*\r\n" - return f'#!/bin/sh\nexec {target_cmd} "$@"\n' + settings = "".join( + f'set "{name}={batch_value(name, value)}"\r\n' + for name, value in env.items() + ) + if settings: + # Keep "!" literal and the settings local to the launcher. + settings = f"setlocal DisableDelayedExpansion\r\n{settings}" + return f"@echo off\r\n{settings}{target_cmd} %*\r\n" + settings = "".join( + f"export {name}={shlex.quote(value)}\n" for name, value in env.items() + ) + return f'#!/bin/sh\n{settings}exec {target_cmd} "$@"\n' -def write_launcher(bin_dir: Path, name: str, target_cmd: str, dry_run=False) -> Path: +def write_launcher( + bin_dir: Path, + name: str, + target_cmd: str, + dry_run=False, + env: dict[str, str] | None = None, +) -> Path: """Write a launcher script for *name* in *bin_dir* that execs *target_cmd*. Parameters @@ -71,6 +132,8 @@ def write_launcher(bin_dir: Path, name: str, target_cmd: str, dry_run=False) -> target_cmd: The command the launcher forwards to (e.g. ``cloudsmith credential-helper docker``). + env: + Environment variables that the launcher sets. Returns ------- @@ -78,6 +141,7 @@ def write_launcher(bin_dir: Path, name: str, target_cmd: str, dry_run=False) -> The path of the written file. """ windows = is_windows() + content = _launcher_content(target_cmd, windows=windows, env=env) if dry_run: if windows: return bin_dir / f"{name}.cmd" @@ -87,9 +151,7 @@ def write_launcher(bin_dir: Path, name: str, target_cmd: str, dry_run=False) -> bin_dir.mkdir(parents=True, exist_ok=True) dest = bin_dir / _launcher_filename(name, windows=windows) - dest.write_text( - _launcher_content(target_cmd, windows=windows), encoding="utf-8", newline="" - ) + dest.write_text(content, encoding="utf-8", newline="") if not windows: dest.chmod(0o755) From 1f34ef5c620752f63090d1e9cb4b5d9b35885043 Mon Sep 17 00:00:00 2001 From: Ian Duffy Date: Tue, 3 Nov 2026 10:42:06 +0000 Subject: [PATCH 10/16] Save the resolved TLS setting in the launcher Keep the error report controls after the install tests clear the CLOUDSMITH_* variables. Co-Authored-By: Claude Opus 5.5 --- .../cli/commands/credential_helper/manage.py | 10 ++++++++-- .../tests/commands/test_credential_helper_install.py | 11 ++++++++++- 2 files changed, 18 insertions(+), 3 deletions(-) diff --git a/cloudsmith_cli/cli/commands/credential_helper/manage.py b/cloudsmith_cli/cli/commands/credential_helper/manage.py index 3e0101dc7..344757537 100644 --- a/cloudsmith_cli/cli/commands/credential_helper/manage.py +++ b/cloudsmith_cli/cli/commands/credential_helper/manage.py @@ -141,6 +141,8 @@ def _terraform_proxy_next_steps(opts) -> list[str]: # hold credentials. _NEVER_KEPT = ("api_key", "api_headers", "keyring_key", "proxy") +_TLS_ENVVARS = ("CLOUDSMITH_SSL_VERIFY", "CLOUDSMITH_WITHOUT_API_SSL_VERIFY") + def _main_envvar(option) -> str: """Return the main environment variable of *option*.""" @@ -148,7 +150,7 @@ def _main_envvar(option) -> str: return names if isinstance(names, str) else names[0] -def _launcher_settings_from_cli(ctx) -> dict[str, str]: +def _launcher_settings_from_cli(ctx, opts) -> dict[str, str]: """Return the ``CLOUDSMITH_*`` settings that a launcher keeps. An option with a ``CLOUDSMITH_*`` environment variable is kept when its @@ -174,6 +176,10 @@ def _launcher_settings_from_cli(ctx) -> dict[str, str]: settings[envvar] = ( str(value).lower() if isinstance(value, bool) else value ) + # A -S flag overrides CLOUDSMITH_SSL_VERIFY. Two environment variables do + # not keep that precedence, so the launcher keeps only the resolved value. + if [settings.pop(name) for name in _TLS_ENVVARS if name in settings]: + settings["CLOUDSMITH_SSL_VERIFY"] = str(opts.ssl_verify).lower() return settings @@ -354,7 +360,7 @@ def install_cmd( install_kwargs["helper_args"] = helper_args if getattr(installer, "KEEPS_INSTALL_TIME_SETTINGS", False): - install_kwargs["launcher_env"] = _launcher_settings_from_cli(ctx) + install_kwargs["launcher_env"] = _launcher_settings_from_cli(ctx, opts) try: actions = installer.install(**install_kwargs) diff --git a/cloudsmith_cli/cli/tests/commands/test_credential_helper_install.py b/cloudsmith_cli/cli/tests/commands/test_credential_helper_install.py index d0a7b136d..2c584f582 100644 --- a/cloudsmith_cli/cli/tests/commands/test_credential_helper_install.py +++ b/cloudsmith_cli/cli/tests/commands/test_credential_helper_install.py @@ -18,6 +18,7 @@ from cloudsmith_cli.credential_helpers.pnpm.installer import PNPMInstaller from ....cli.commands.credential_helper import credential_helper +from ....core import telemetry from ....core.credentials.models import CredentialResult from ....credential_helpers.docker.installer import DockerInstaller from ....credential_helpers.launchers import ( @@ -49,6 +50,13 @@ def clear_cloudsmith_env(monkeypatch): """Keep the CLOUDSMITH_* settings of the developer shell out of the installs.""" for name in [name for name in os.environ if name.startswith("CLOUDSMITH_")]: monkeypatch.delenv(name) + monkeypatch.setenv(telemetry.NO_TELEMETRY_ENV, "1") + monkeypatch.setenv(telemetry.DSN_ENV, "") + + +def test_cleared_install_settings_keep_the_error_report_controls(): + assert os.environ[telemetry.NO_TELEMETRY_ENV] == "1" + assert os.environ[telemetry.DSN_ENV] == "" # --------------------------------------------------------------------------- @@ -186,6 +194,7 @@ def install(self, **kwargs): "CLOUDSMITH_KEYRING_DIR": "/var/keyring", "CLOUDSMITH_OIDC_AUDIENCE": "cloudsmith-test", "CLOUDSMITH_SERVICE_SLUG": "ci-bot", + "CLOUDSMITH_SSL_VERIFY": "1", }, catch_exceptions=False, ) @@ -196,7 +205,7 @@ def install(self, **kwargs): "CLOUDSMITH_PROFILE": "work", "CLOUDSMITH_CONFIG_FILE": str(config.resolve()), "CLOUDSMITH_API_HOST": "https://api.example.com", - "CLOUDSMITH_WITHOUT_API_SSL_VERIFY": "true", + "CLOUDSMITH_SSL_VERIFY": "false", "CLOUDSMITH_OIDC_AUDIENCE": "cloudsmith-test", "CLOUDSMITH_SERVICE_SLUG": "ci-bot", "CLOUDSMITH_KEYRING_DIR": "/var/keyring", From 7c98fbead5f07244c1cde5250f3b682c8e1536bc Mon Sep 17 00:00:00 2001 From: Ian Duffy Date: Tue, 3 Nov 2026 10:44:53 +0000 Subject: [PATCH 11/16] Name the bin directory in the Docker scope conflict command Parse the whole Unix launcher, so a value with a line break does not stop a reinstall or an uninstall. Prove that the launcher keeps the resolved TLS setting. Co-Authored-By: Claude Opus 5.5 --- .../test_credential_helper_install.py | 73 ++++++++++++++++++- .../credential_helpers/docker/installer.py | 1 + .../credential_helpers/launchers.py | 19 +++-- 3 files changed, 85 insertions(+), 8 deletions(-) diff --git a/cloudsmith_cli/cli/tests/commands/test_credential_helper_install.py b/cloudsmith_cli/cli/tests/commands/test_credential_helper_install.py index da664abfd..02aa56022 100644 --- a/cloudsmith_cli/cli/tests/commands/test_credential_helper_install.py +++ b/cloudsmith_cli/cli/tests/commands/test_credential_helper_install.py @@ -5,6 +5,7 @@ import json import os +import shlex import stat import sys from pathlib import Path @@ -477,6 +478,28 @@ def test_docker_install_scopes_the_helper_to_the_install_time_settings( ] +def test_docker_launcher_keeps_the_resolved_tls_setting(runner, tmp_path, monkeypatch): + """-S overrides CLOUDSMITH_SSL_VERIFY=1, so the launcher turns verification off.""" + from ....cli.commands.credential_helper.manage import install_cmd + + monkeypatch.setenv("DOCKER_CONFIG", str(tmp_path / ".docker")) + monkeypatch.delattr(sys, "frozen", raising=False) + bin_dir = tmp_path / "bin" + + result = runner.invoke( + install_cmd, + ["docker", "--bin-dir", str(bin_dir), "--no-discover", "-S"], + env={"CLOUDSMITH_SSL_VERIFY": "1"}, + ) + + assert result.exit_code == 0, result.output + assert (bin_dir / "docker-credential-cloudsmith").read_text() == ( + "#!/bin/sh\n" + "export CLOUDSMITH_SSL_VERIFY=false\n" + 'exec cloudsmith credential-helper docker "$@"\n' + ) + + def test_docker_dry_run_shows_the_scoped_launcher_and_its_settings( tmp_path, monkeypatch ): @@ -544,7 +567,7 @@ def test_docker_refuses_the_helper_of_another_install(runner, tmp_path, monkeypa launcher = bin_dir / "docker-credential-cloudsmith-acme" content = launcher.read_text() - with pytest.raises(HelperScopeConflict, match="uninstall docker -P acme$"): + with pytest.raises(HelperScopeConflict, match="docker -P acme --bin-dir "): installer.install( bin_dir=str(bin_dir), discover=False, @@ -559,6 +582,54 @@ def test_docker_refuses_the_helper_of_another_install(runner, tmp_path, monkeypa assert launcher.read_text() == content +def test_docker_reinstalls_a_launcher_with_a_line_break_in_a_setting( + tmp_path, monkeypatch +): + """A quoted value in a Unix launcher can hold a line break.""" + monkeypatch.setenv("DOCKER_CONFIG", str(tmp_path / ".docker")) + bin_dir = tmp_path / "bin" + launcher_env = { + "CLOUDSMITH_CONFIG_FILE": str(tmp_path / "a\nb"), + "CLOUDSMITH_WORKSPACE": "acme", + } + installer = DockerInstaller() + installer.install(bin_dir=str(bin_dir), discover=False, launcher_env=launcher_env) + + installer.install(bin_dir=str(bin_dir), discover=False, launcher_env=launcher_env) + installer.uninstall(bin_dir=str(bin_dir), workspace="acme") + + assert not list(bin_dir.iterdir()) + + +def test_docker_conflict_command_removes_the_conflicting_launcher( + runner, tmp_path, monkeypatch +): + """The command in the conflict message names the --bin-dir of the launcher.""" + from ....cli.commands.main import main + + monkeypatch.setenv("HOME", str(tmp_path / "home")) + monkeypatch.setenv("DOCKER_CONFIG", str(tmp_path / ".docker")) + bin_dir = tmp_path / "bin" + installer = DockerInstaller() + installer.install( + bin_dir=str(bin_dir), + discover=False, + launcher_env={"CLOUDSMITH_PROFILE": "acme"}, + ) + with pytest.raises(HelperScopeConflict) as conflict: + installer.install( + bin_dir=str(bin_dir), + discover=False, + launcher_env={"CLOUDSMITH_WORKSPACE": "acme"}, + ) + + command = shlex.split(str(conflict.value).partition("run: ")[2]) + result = runner.invoke(main, command[1:]) + + assert result.exit_code == 0, result.output + assert not list(bin_dir.iterdir()) + + def test_pnpm_installer_install(tmp_path: Path, monkeypatch: MonkeyPatch): """install sets default+extra domains, preserves foreign entries, writes the launcher.""" npm_path = tmp_path / ".npmrc" diff --git a/cloudsmith_cli/credential_helpers/docker/installer.py b/cloudsmith_cli/credential_helpers/docker/installer.py index 1dcdc8d7a..496d7c525 100644 --- a/cloudsmith_cli/credential_helpers/docker/installer.py +++ b/cloudsmith_cli/credential_helpers/docker/installer.py @@ -91,6 +91,7 @@ def _check_scope(launcher_path: Path, scope: tuple[str | None, ...]) -> None: f"{launcher_path} is the helper of an install with a different" " profile or Workspace. To remove it, run: cloudsmith" f" credential-helper uninstall docker{flags}" + f" --bin-dir {shlex.quote(str(launcher_path.parent))}" ) diff --git a/cloudsmith_cli/credential_helpers/launchers.py b/cloudsmith_cli/credential_helpers/launchers.py index 675884764..b26f8da76 100644 --- a/cloudsmith_cli/credential_helpers/launchers.py +++ b/cloudsmith_cli/credential_helpers/launchers.py @@ -16,6 +16,7 @@ import os import shlex import sys +from itertools import pairwise from pathlib import Path @@ -116,14 +117,18 @@ def _launcher_content( def parse_launcher_env(content: str) -> dict[str, str]: """Return the environment variables that a launcher body sets.""" + if content.startswith("@echo off"): + settings = [ + line[len('set "') : -1] + for line in content.splitlines() + if line.startswith('set "') + ] + else: + # A quoted value can hold a line break, so split the whole script. + words = shlex.split(content) + settings = [word for prev, word in pairwise(words) if prev == "export"] env = {} - for line in content.splitlines(): - if line.startswith("export "): - setting = shlex.split(line)[1] - elif line.startswith('set "'): - setting = line[len('set "') : -1] - else: - continue + for setting in settings: name, _, value = setting.partition("=") env[name] = value return env From 2c7169d17b5cea25f0f9b35897f002ff3b48ba1a Mon Sep 17 00:00:00 2001 From: Ian Duffy Date: Tue, 3 Nov 2026 10:54:35 +0000 Subject: [PATCH 12/16] Let a root -P flag override CLOUDSMITH_PROFILE Install, the launcher settings and uninstall now use the same profile. Accept --oidc-org in uninstall. Quote the conflict command for cmd.exe on Windows. Co-Authored-By: Claude Opus 5.5 --- .../cli/commands/credential_helper/manage.py | 4 ++ cloudsmith_cli/cli/decorators.py | 5 +- .../test_credential_helper_install.py | 65 +++++++++++++++++++ .../credential_helpers/docker/installer.py | 14 +++- 4 files changed, 85 insertions(+), 3 deletions(-) diff --git a/cloudsmith_cli/cli/commands/credential_helper/manage.py b/cloudsmith_cli/cli/commands/credential_helper/manage.py index 987ee5099..d627d4820 100644 --- a/cloudsmith_cli/cli/commands/credential_helper/manage.py +++ b/cloudsmith_cli/cli/commands/credential_helper/manage.py @@ -190,6 +190,9 @@ def _launcher_settings_from_cli(ctx, opts) -> dict[str, str]: # not keep that precedence, so the launcher keeps only the resolved value. if [settings.pop(name) for name in _TLS_ENVVARS if name in settings]: settings["CLOUDSMITH_SSL_VERIFY"] = str(opts.ssl_verify).lower() + # Keep the profile that the install used to load the config files. + if "CLOUDSMITH_PROFILE" in settings: + settings["CLOUDSMITH_PROFILE"] = contexts[-1].meta["profile"] return settings @@ -459,6 +462,7 @@ def install_cmd( "--workspace", "--org", "--organization", + "--oidc-org", "-o", "--owner", "workspace", diff --git a/cloudsmith_cli/cli/decorators.py b/cloudsmith_cli/cli/decorators.py index e2022716a..f13065498 100644 --- a/cloudsmith_cli/cli/decorators.py +++ b/cloudsmith_cli/cli/decorators.py @@ -219,7 +219,10 @@ def common_cli_config_options(f): def wrapper(ctx, *args, **kwargs): # pylint: disable=missing-docstring opts = config.get_or_create_options(ctx) - profile = kwargs.pop("profile") or ctx.meta.get("profile") + profile = kwargs.pop("profile") + # A -P flag of a parent command overrides CLOUDSMITH_PROFILE. + if ctx.get_parameter_source("profile") != ParameterSource.COMMANDLINE: + profile = ctx.meta.get("profile") or profile config_file = kwargs.pop("config_file") or ctx.meta.get("config_file") creds_file = kwargs.pop("credentials_file") or ctx.meta.get("creds_file") diff --git a/cloudsmith_cli/cli/tests/commands/test_credential_helper_install.py b/cloudsmith_cli/cli/tests/commands/test_credential_helper_install.py index 02aa56022..407f0b646 100644 --- a/cloudsmith_cli/cli/tests/commands/test_credential_helper_install.py +++ b/cloudsmith_cli/cli/tests/commands/test_credential_helper_install.py @@ -24,6 +24,7 @@ from ....credential_helpers.docker.installer import ( DockerInstaller, HelperScopeConflict, + _check_scope, ) from ....credential_helpers.launchers import ( UnsafeLauncherValue, @@ -630,6 +631,70 @@ def test_docker_conflict_command_removes_the_conflicting_launcher( assert not list(bin_dir.iterdir()) +def test_docker_conflict_command_uses_windows_quoting(tmp_path, monkeypatch): + """cmd.exe does not read single quotes as quotes.""" + monkeypatch.setattr( + "cloudsmith_cli.credential_helpers.docker.installer.is_windows", lambda: True + ) + bin_dir = tmp_path / "my bin" + bin_dir.mkdir() + launcher = bin_dir / "docker-credential-cloudsmith-acme.cmd" + launcher.write_text( + _launcher_content("cmd", windows=True, env={"CLOUDSMITH_PROFILE": "acme"}) + ) + + with pytest.raises(HelperScopeConflict) as conflict: + _check_scope(launcher, (None, "acme")) + + assert str(conflict.value).endswith(f' -P acme --bin-dir "{bin_dir}"') + + +def test_docker_root_profile_flag_overrides_the_environment( + runner, tmp_path, monkeypatch +): + """A -P flag on the root command overrides CLOUDSMITH_PROFILE.""" + from ....cli.commands.main import main + + monkeypatch.setenv("DOCKER_CONFIG", str(tmp_path / ".docker")) + bin_dir = tmp_path / "bin" + args = ["docker", "--bin-dir", str(bin_dir), "-w", "acme"] + env = {"CLOUDSMITH_PROFILE": "other"} + + installed = runner.invoke( + main, + ["-P", "work", "credential-helper", "install", *args, "--no-discover"], + env=env, + ) + launchers = sorted(path.name for path in bin_dir.iterdir()) + uninstalled = runner.invoke( + main, ["-P", "work", "credential-helper", "uninstall", *args], env=env + ) + + assert installed.exit_code == 0, installed.output + assert uninstalled.exit_code == 0, uninstalled.output + assert launchers == ["docker-credential-cloudsmith-work-acme"] + assert not list(bin_dir.iterdir()) + + +def test_docker_uninstall_accepts_the_oidc_org_alias(runner, tmp_path, monkeypatch): + from ....cli.commands.credential_helper.manage import uninstall_cmd + + monkeypatch.setenv("DOCKER_CONFIG", str(tmp_path / ".docker")) + bin_dir = tmp_path / "bin" + DockerInstaller().install( + bin_dir=str(bin_dir), + discover=False, + launcher_env={"CLOUDSMITH_WORKSPACE": "acme"}, + ) + + result = runner.invoke( + uninstall_cmd, ["docker", "--bin-dir", str(bin_dir), "--oidc-org", "acme"] + ) + + assert result.exit_code == 0, result.output + assert not list(bin_dir.iterdir()) + + def test_pnpm_installer_install(tmp_path: Path, monkeypatch: MonkeyPatch): """install sets default+extra domains, preserves foreign entries, writes the launcher.""" npm_path = tmp_path / ".npmrc" diff --git a/cloudsmith_cli/credential_helpers/docker/installer.py b/cloudsmith_cli/credential_helpers/docker/installer.py index 496d7c525..97185a539 100644 --- a/cloudsmith_cli/credential_helpers/docker/installer.py +++ b/cloudsmith_cli/credential_helpers/docker/installer.py @@ -13,6 +13,7 @@ import os import re import shlex +import subprocess from pathlib import Path from typing import TYPE_CHECKING @@ -71,6 +72,12 @@ def _helper_name(scope: tuple[str | None, ...]) -> str: return "-".join(["cloudsmith", *filter(None, segments)]) +def _shell_quote(value: str) -> str: + """Return *value* quoted as one argument for the shell of the user.""" + # cmd.exe does not read single quotes as quotes. + return subprocess.list2cmdline([value]) if is_windows() else shlex.quote(value) + + def _check_scope(launcher_path: Path, scope: tuple[str | None, ...]) -> None: """Raise :class:`HelperScopeConflict` if another scope wrote the launcher. @@ -83,7 +90,7 @@ def _check_scope(launcher_path: Path, scope: tuple[str | None, ...]) -> None: installed = tuple(env.get(name) for name in _SCOPE_ENVVARS) if installed != scope: flags = "".join( - f" {flag} {shlex.quote(value)}" + f" {flag} {_shell_quote(value)}" for flag, value in zip(_SCOPE_FLAGS, installed) if value ) @@ -91,7 +98,7 @@ def _check_scope(launcher_path: Path, scope: tuple[str | None, ...]) -> None: f"{launcher_path} is the helper of an install with a different" " profile or Workspace. To remove it, run: cloudsmith" f" credential-helper uninstall docker{flags}" - f" --bin-dir {shlex.quote(str(launcher_path.parent))}" + f" --bin-dir {_shell_quote(str(launcher_path.parent))}" ) @@ -296,6 +303,9 @@ def mutate(config: dict) -> None: return actions + replaced # Real install + # The caller resolves -S and CLOUDSMITH_SSL_VERIFY into one + # CLOUDSMITH_SSL_VERIFY value, so the launcher keeps the TLS setting + # of the install. launcher_path = write_launcher( target_dir, launcher_name, helper_command(self.name), env=launcher_env ) From cb77e5eb77c4ca9497481dc6e2b4f75f2b120873 Mon Sep 17 00:00:00 2001 From: Ian Duffy Date: Tue, 3 Nov 2026 11:02:41 +0000 Subject: [PATCH 13/16] Quote cmd.exe metacharacters in the Docker conflict command Co-Authored-By: Claude Opus 5.5 --- .../test_credential_helper_install.py | 20 ++++++++++++++++++- .../credential_helpers/docker/installer.py | 9 ++++++--- 2 files changed, 25 insertions(+), 4 deletions(-) diff --git a/cloudsmith_cli/cli/tests/commands/test_credential_helper_install.py b/cloudsmith_cli/cli/tests/commands/test_credential_helper_install.py index 407f0b646..926d5bfc8 100644 --- a/cloudsmith_cli/cli/tests/commands/test_credential_helper_install.py +++ b/cloudsmith_cli/cli/tests/commands/test_credential_helper_install.py @@ -646,7 +646,25 @@ def test_docker_conflict_command_uses_windows_quoting(tmp_path, monkeypatch): with pytest.raises(HelperScopeConflict) as conflict: _check_scope(launcher, (None, "acme")) - assert str(conflict.value).endswith(f' -P acme --bin-dir "{bin_dir}"') + assert str(conflict.value).endswith(f' -P "acme" --bin-dir "{bin_dir}"') + + +def test_docker_conflict_command_quotes_cmd_metacharacters(tmp_path, monkeypatch): + """cmd.exe splits a command at an & that is not in double quotes.""" + monkeypatch.setattr( + "cloudsmith_cli.credential_helpers.docker.installer.is_windows", lambda: True + ) + bin_dir = tmp_path / "bin&tools" + bin_dir.mkdir() + launcher = bin_dir / "docker-credential-cloudsmith-work-team.cmd" + launcher.write_text( + _launcher_content("cmd", windows=True, env={"CLOUDSMITH_PROFILE": "work&team"}) + ) + + with pytest.raises(HelperScopeConflict) as conflict: + _check_scope(launcher, (None, "acme")) + + assert str(conflict.value).endswith(f' -P "work&team" --bin-dir "{bin_dir}"') def test_docker_root_profile_flag_overrides_the_environment( diff --git a/cloudsmith_cli/credential_helpers/docker/installer.py b/cloudsmith_cli/credential_helpers/docker/installer.py index 97185a539..c2f8dd55c 100644 --- a/cloudsmith_cli/credential_helpers/docker/installer.py +++ b/cloudsmith_cli/credential_helpers/docker/installer.py @@ -13,7 +13,6 @@ import os import re import shlex -import subprocess from pathlib import Path from typing import TYPE_CHECKING @@ -74,8 +73,12 @@ def _helper_name(scope: tuple[str | None, ...]) -> str: def _shell_quote(value: str) -> str: """Return *value* quoted as one argument for the shell of the user.""" - # cmd.exe does not read single quotes as quotes. - return subprocess.list2cmdline([value]) if is_windows() else shlex.quote(value) + if not is_windows(): + return shlex.quote(value) + # cmd.exe reads & | < > ^ as text only in double quotes. A backslash + # before the closing quote must be doubled, or it escapes the quote. + trailing_backslashes = "\\" * (len(value) - len(value.rstrip("\\"))) + return f'"{value}{trailing_backslashes}"' def _check_scope(launcher_path: Path, scope: tuple[str | None, ...]) -> None: From c35a039fa5aa61a7ab3b081904ebb8ded42740ea Mon Sep 17 00:00:00 2001 From: Ian Duffy Date: Tue, 3 Nov 2026 11:08:50 +0000 Subject: [PATCH 14/16] Treat an empty Docker profile or Workspace as none Co-Authored-By: Claude Opus 5.5 --- .../commands/test_credential_helper_install.py | 16 ++++++++++++++++ .../credential_helpers/docker/installer.py | 5 +++-- 2 files changed, 19 insertions(+), 2 deletions(-) diff --git a/cloudsmith_cli/cli/tests/commands/test_credential_helper_install.py b/cloudsmith_cli/cli/tests/commands/test_credential_helper_install.py index 926d5bfc8..53e5fd7ac 100644 --- a/cloudsmith_cli/cli/tests/commands/test_credential_helper_install.py +++ b/cloudsmith_cli/cli/tests/commands/test_credential_helper_install.py @@ -602,6 +602,22 @@ def test_docker_reinstalls_a_launcher_with_a_line_break_in_a_setting( assert not list(bin_dir.iterdir()) +def test_docker_uninstall_removes_a_launcher_with_an_empty_profile( + tmp_path, monkeypatch +): + """An empty CLOUDSMITH_PROFILE gives the helper of an install without a profile.""" + monkeypatch.setenv("DOCKER_CONFIG", str(tmp_path / ".docker")) + bin_dir = tmp_path / "bin" + installer = DockerInstaller() + installer.install( + bin_dir=str(bin_dir), discover=False, launcher_env={"CLOUDSMITH_PROFILE": ""} + ) + + installer.uninstall(bin_dir=str(bin_dir)) + + assert not list(bin_dir.iterdir()) + + def test_docker_conflict_command_removes_the_conflicting_launcher( runner, tmp_path, monkeypatch ): diff --git a/cloudsmith_cli/credential_helpers/docker/installer.py b/cloudsmith_cli/credential_helpers/docker/installer.py index c2f8dd55c..e98eb5795 100644 --- a/cloudsmith_cli/credential_helpers/docker/installer.py +++ b/cloudsmith_cli/credential_helpers/docker/installer.py @@ -90,8 +90,9 @@ def _check_scope(launcher_path: Path, scope: tuple[str | None, ...]) -> None: if not launcher_path.exists(): return env = parse_launcher_env(launcher_path.read_text(encoding="utf-8")) - installed = tuple(env.get(name) for name in _SCOPE_ENVVARS) - if installed != scope: + # An empty value gives the same helper name as no value. + installed = tuple(env.get(name) or None for name in _SCOPE_ENVVARS) + if installed != tuple(value or None for value in scope): flags = "".join( f" {flag} {_shell_quote(value)}" for flag, value in zip(_SCOPE_FLAGS, installed) From 68e9a03b5ad38453cee998605e64465fe68aa743 Mon Sep 17 00:00:00 2001 From: Ian Duffy Date: Thu, 5 Nov 2026 11:37:21 +0000 Subject: [PATCH 15/16] Keep the settings of deprecated spellings in the Docker launcher A deprecated spelling such as --org, -S or CLOUDSMITH_WITHOUT_API_SSL_VERIFY is now a separate hidden option. The install now keeps its value in the launcher. Before, the install dropped the value or stopped with a KeyError. uninstall -w now uses the same deprecated spellings as install. They show a warning and do not show in --help. Co-Authored-By: Claude Opus 5.5 --- .../cli/commands/credential_helper/manage.py | 50 +++++++++++++++---- .../test_credential_helper_install.py | 27 ++++++++++ 2 files changed, 66 insertions(+), 11 deletions(-) diff --git a/cloudsmith_cli/cli/commands/credential_helper/manage.py b/cloudsmith_cli/cli/commands/credential_helper/manage.py index d627d4820..763dc8126 100644 --- a/cloudsmith_cli/cli/commands/credential_helper/manage.py +++ b/cloudsmith_cli/cli/commands/credential_helper/manage.py @@ -33,7 +33,7 @@ DockerInstaller, HelperScopeConflict, ) -from ... import utils +from ... import deprecation, utils from ...decorators import ( common_api_auth_options, common_cli_config_options, @@ -160,6 +160,24 @@ def _main_envvar(option) -> str: return names if isinstance(names, str) else names[0] +# A deprecated spelling is a separate option that sets the value of the option +# that replaces it. +_DEPRECATED_SPELLINGS = { + "org": deprecation.WORKSPACE_ALIASES, + "ssl_verify": deprecation.SSL_VERIFY_ALIASES, + "allowed_proxy_suffixes": deprecation.PROXY_SUFFIXES_ALIASES, +} + + +def _sources(context, option) -> set: + """Return the sources of the value of *option* and its deprecated spellings.""" + names = [option.name] + [ + deprecation.alias_param_name(spelling) + for spelling in _DEPRECATED_SPELLINGS.get(option.name, ()) + ] + return {context.get_parameter_source(name) for name in names} + + def _launcher_settings_from_cli(ctx, opts) -> dict[str, str]: """Return the ``CLOUDSMITH_*`` settings that a launcher keeps. @@ -177,11 +195,16 @@ def _launcher_settings_from_cli(ctx, opts) -> dict[str, str]: settings = {} for context in contexts: for option in context.command.params: - if option.name in _NEVER_KEPT or not option.envvar: + # The command removes the deprecated spellings from ctx.params. + if ( + option.name in _NEVER_KEPT + or not option.envvar + or option.name not in context.params + ): continue envvar = _main_envvar(option) - source = context.get_parameter_source(option.name) - if envvar.startswith("CLOUDSMITH_") and source in supplied: + sources = _sources(context, option) + if envvar.startswith("CLOUDSMITH_") and not sources.isdisjoint(supplied): value = context.params[option.name] settings[envvar] = ( str(value).lower() if isinstance(value, bool) else value @@ -460,19 +483,21 @@ def install_cmd( @click.option( "-w", "--workspace", - "--org", - "--organization", - "--oidc-org", - "-o", - "--owner", "workspace", help="Docker only: remove the helper that was installed for this Workspace.", ) +@deprecation.alias_options(deprecation.WORKSPACE_ALIASES) @common_cli_config_options @common_cli_output_options @click.pass_context def uninstall_cmd( - ctx, opts, helper: str, bin_dir: str | None, dry_run: bool, workspace: str | None + ctx, + opts, + helper: str, + bin_dir: str | None, + dry_run: bool, + workspace: str | None, + **workspace_aliases, ) -> None: """Uninstall a credential helper launcher and remove its config entries. @@ -489,12 +514,15 @@ def uninstall_cmd( $ cloudsmith credential-helper uninstall docker --dry-run """ installer = _get_installer(helper) + alias_workspace = deprecation.pop_alias_options( + ctx, workspace_aliases, deprecation.WORKSPACE_ALIASES + ) uninstall_kwargs = {"bin_dir": bin_dir, "dry_run": dry_run} # The scope comes only from flags, so that a shell with CLOUDSMITH_PROFILE # or CLOUDSMITH_WORKSPACE can still remove the unscoped helper. if helper == "docker": uninstall_kwargs["profile"] = _profile_flag(ctx) - uninstall_kwargs["workspace"] = workspace + uninstall_kwargs["workspace"] = workspace or alias_workspace try: actions = installer.uninstall(**uninstall_kwargs) except (ConfigFileParseError, HelperScopeConflict) as exc: diff --git a/cloudsmith_cli/cli/tests/commands/test_credential_helper_install.py b/cloudsmith_cli/cli/tests/commands/test_credential_helper_install.py index 2167c5910..c517ac563 100644 --- a/cloudsmith_cli/cli/tests/commands/test_credential_helper_install.py +++ b/cloudsmith_cli/cli/tests/commands/test_credential_helper_install.py @@ -501,6 +501,31 @@ def test_docker_launcher_keeps_the_resolved_tls_setting(runner, tmp_path, monkey ) +def test_docker_launcher_keeps_the_settings_of_deprecated_spellings( + runner, tmp_path, monkeypatch +): + """--org and CLOUDSMITH_WITHOUT_API_SSL_VERIFY set the same values as -w and -S.""" + from ....cli.commands.credential_helper.manage import install_cmd + + monkeypatch.setenv("DOCKER_CONFIG", str(tmp_path / ".docker")) + monkeypatch.delattr(sys, "frozen", raising=False) + bin_dir = tmp_path / "bin" + + result = runner.invoke( + install_cmd, + ["docker", "--bin-dir", str(bin_dir), "--no-discover", "--org", "acme"], + env={"CLOUDSMITH_WITHOUT_API_SSL_VERIFY": "1"}, + ) + + assert result.exit_code == 0, result.output + assert (bin_dir / "docker-credential-cloudsmith-acme").read_text() == ( + "#!/usr/bin/env sh\n" + "export CLOUDSMITH_WORKSPACE=acme\n" + "export CLOUDSMITH_SSL_VERIFY=false\n" + 'exec cloudsmith credential-helper docker "$@"\n' + ) + + def test_docker_dry_run_shows_the_scoped_launcher_and_its_settings( tmp_path, monkeypatch ): @@ -711,6 +736,7 @@ def test_docker_root_profile_flag_overrides_the_environment( def test_docker_uninstall_accepts_the_oidc_org_alias(runner, tmp_path, monkeypatch): + """--oidc-org works, but warns that it is deprecated.""" from ....cli.commands.credential_helper.manage import uninstall_cmd monkeypatch.setenv("DOCKER_CONFIG", str(tmp_path / ".docker")) @@ -727,6 +753,7 @@ def test_docker_uninstall_accepts_the_oidc_org_alias(runner, tmp_path, monkeypat assert result.exit_code == 0, result.output assert not list(bin_dir.iterdir()) + assert "`--oidc-org` is deprecated" in result.stderr def test_pnpm_installer_install(tmp_path: Path, monkeypatch: MonkeyPatch): From 263eaa4590b0c58f265b1620221068935d42821b Mon Sep 17 00:00:00 2001 From: Ian Duffy Date: Thu, 5 Nov 2026 11:49:20 +0000 Subject: [PATCH 16/16] Name each Docker helper in list and read launchers safely list now shows the credHelpers name of each Docker helper. It also shows a scoped launcher after a later install moves all of its hosts. A launcher that the CLI cannot parse now stops install and uninstall with its path and a fix, not with a ValueError. The CHANGELOG entry is shorter, and a Fixed entry tells that a root -P flag now overrides CLOUDSMITH_PROFILE. Co-Authored-By: Claude Opus 5.5 --- CHANGELOG.md | 3 +- .../cli/commands/credential_helper/manage.py | 3 + .../test_credential_helper_install.py | 57 +++++++++++++------ .../credential_helpers/docker/installer.py | 50 +++++++++++----- 4 files changed, 80 insertions(+), 33 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 10be6ec8d..d0a5f62a6 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -16,7 +16,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0. ### Changed - After a SAML login, `cloudsmith auth` saves the Workspace to `config.ini`. It writes to the `[profile:NAME]` section of the selected profile, or to `[default]` when you do not select a profile. The CLI writes to the file or directory that `--config-file` gives, or else to your user-level `config.ini`. If the `config.ini` does not exist, the CLI creates it. -- `cloudsmith credential-helper install docker` now writes the `CLOUDSMITH_*` settings that you give as a flag or an environment variable into the launcher. These settings include the Workspace, profile, config and credentials files, API host, TLS verification, OIDC and keyring settings. Docker then gives the same result from Docker Desktop, an IDE or a new shell. The launcher never contains API keys, custom headers or the proxy URL. An install with a profile or a Workspace writes its own launcher, for example `docker-credential-cloudsmith--`, so that the custom domains of different Workspaces can use different helpers. A host that more than one install configures, such as `docker.cloudsmith.io`, uses the helper of the last install, and the install shows a warning when it moves such a host. An install without a profile or a Workspace writes `docker-credential-cloudsmith`, as before. Two installs can get the same name, for example with `-P acme` and with `-w acme`. Then the second install stops and shows the command that removes the first. `uninstall docker -P -w ` removes only the helper of that install. Without these flags, `uninstall docker` removes `docker-credential-cloudsmith`, as before. `list` shows each Cloudsmith helper with its launcher and its hosts. +- `cloudsmith credential-helper install docker` now keeps the `CLOUDSMITH_*` settings of the install, such as the profile, Workspace and API host, in the launcher. Docker then gets the same credentials from Docker Desktop, an IDE or a new shell. The launcher never contains API keys, custom headers or the proxy URL. An install with a profile or a Workspace writes its own helper, for example `docker-credential-cloudsmith--`. To remove it, run `cloudsmith credential-helper uninstall docker -P -w `. An install without them writes `docker-credential-cloudsmith`, as before. - Using `#!/usr/bin/env sh` as shebang path for unix credential helpers. - The minimum version of `cloudsmith-api` is now 2.0.42. @@ -86,6 +86,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0. - `cloudsmith mcp start` now stops without an error when the MCP client closes the connection while the server writes a reply. Previously, the command stopped with `BrokenPipeError` and exit status 1. - The CLI now exits with status 1 when you interrupt it with Ctrl+C after the reader of its stderr has closed the pipe. Before, the CLI sent an error report and exited with status 120. - `cloudsmith check rates` no longer stops with `TypeError` when the API does not give a value for an interval, limit, remaining count, reset time or throttled status. The command now shows `None` for that value. +- A `-P` flag before the command, for example `cloudsmith -P meta whoami`, now overrides `CLOUDSMITH_PROFILE`. Before, the environment variable took precedence. ## [1.30.0] - 2026-10-22 diff --git a/cloudsmith_cli/cli/commands/credential_helper/manage.py b/cloudsmith_cli/cli/commands/credential_helper/manage.py index 763dc8126..554ccbefb 100644 --- a/cloudsmith_cli/cli/commands/credential_helper/manage.py +++ b/cloudsmith_cli/cli/commands/credential_helper/manage.py @@ -583,6 +583,7 @@ def list_cmd(ctx, opts) -> None: { "helper": name, "summary": installer.summary, + "cred_helper": st.get("name"), "launcher": st.get("launcher"), "hosts": st.get("hosts", []), } @@ -599,6 +600,8 @@ def list_cmd(ctx, opts) -> None: summary = entry["summary"] click.echo(f"{name} ({summary})", err=use_stderr) + if entry["cred_helper"]: + click.echo(f" helper : {entry['cred_helper']}", err=use_stderr) if launcher: click.echo(f" launcher : {launcher}", err=use_stderr) else: diff --git a/cloudsmith_cli/cli/tests/commands/test_credential_helper_install.py b/cloudsmith_cli/cli/tests/commands/test_credential_helper_install.py index c517ac563..ea75ede81 100644 --- a/cloudsmith_cli/cli/tests/commands/test_credential_helper_install.py +++ b/cloudsmith_cli/cli/tests/commands/test_credential_helper_install.py @@ -367,6 +367,34 @@ def test_list_finds_a_launcher_installed_to_the_default_dir( ) +def test_list_shows_each_docker_helper_by_name(runner, tmp_path, monkeypatch): + """A scoped launcher stays in `list` after another install takes its hosts.""" + from ....cli.commands.credential_helper.manage import list_cmd + + monkeypatch.setattr(Path, "home", staticmethod(lambda: tmp_path)) + monkeypatch.setenv("DOCKER_CONFIG", str(tmp_path / ".docker")) + for workspace in ("acme", "beta"): + DockerInstaller().install( + discover=False, launcher_env={"CLOUDSMITH_WORKSPACE": workspace} + ) + + result = runner.invoke(list_cmd, ["-F", "json"], catch_exceptions=False) + + bin_dir = tmp_path / ".cloudsmith" / "bin" + assert [ + (entry["cred_helper"], entry["launcher"], entry["hosts"]) + for entry in json.loads(result.stdout)["data"] + if entry["helper"] == "docker" + ] == [ + ( + "cloudsmith-beta", + str(bin_dir / "docker-credential-cloudsmith-beta"), + ["docker.cloudsmith.io"], + ), + ("cloudsmith-acme", str(bin_dir / "docker-credential-cloudsmith-acme"), []), + ] + + # --------------------------------------------------------------------------- # 4. is_on_path # --------------------------------------------------------------------------- @@ -469,10 +497,12 @@ def test_docker_install_scopes_the_helper_to_the_install_time_settings( ): assert installer.statuses() == [ { + "name": name, "launcher": str(bin_dir / f"docker-credential-{name}"), "hosts": ["docker.cloudsmith.io"], }, { + "name": "cloudsmith", "launcher": str(bin_dir / "docker-credential-cloudsmith"), "hosts": ["legacy.example.com"], }, @@ -673,39 +703,32 @@ def test_docker_conflict_command_removes_the_conflicting_launcher( def test_docker_conflict_command_uses_windows_quoting(tmp_path, monkeypatch): - """cmd.exe does not read single quotes as quotes.""" + """cmd.exe reads only double quotes, and splits a command at an unquoted &.""" monkeypatch.setattr( "cloudsmith_cli.credential_helpers.docker.installer.is_windows", lambda: True ) - bin_dir = tmp_path / "my bin" + bin_dir = tmp_path / "my bin&tools" bin_dir.mkdir() - launcher = bin_dir / "docker-credential-cloudsmith-acme.cmd" + launcher = bin_dir / "docker-credential-cloudsmith-work-team.cmd" launcher.write_text( - _launcher_content("cmd", windows=True, env={"CLOUDSMITH_PROFILE": "acme"}) + _launcher_content("cmd", windows=True, env={"CLOUDSMITH_PROFILE": "work&team"}) ) with pytest.raises(HelperScopeConflict) as conflict: _check_scope(launcher, (None, "acme")) - assert str(conflict.value).endswith(f' -P "acme" --bin-dir "{bin_dir}"') + assert str(conflict.value).endswith(f' -P "work&team" --bin-dir "{bin_dir}"') -def test_docker_conflict_command_quotes_cmd_metacharacters(tmp_path, monkeypatch): - """cmd.exe splits a command at an & that is not in double quotes.""" - monkeypatch.setattr( - "cloudsmith_cli.credential_helpers.docker.installer.is_windows", lambda: True - ) - bin_dir = tmp_path / "bin&tools" - bin_dir.mkdir() - launcher = bin_dir / "docker-credential-cloudsmith-work-team.cmd" - launcher.write_text( - _launcher_content("cmd", windows=True, env={"CLOUDSMITH_PROFILE": "work&team"}) - ) +def test_docker_names_a_launcher_that_it_cannot_read(tmp_path): + """A launcher with an unclosed quote stops the install with its path.""" + launcher = tmp_path / "docker-credential-cloudsmith-acme" + launcher.write_text("#!/bin/sh\nexport CLOUDSMITH_WORKSPACE='acme\n") with pytest.raises(HelperScopeConflict) as conflict: _check_scope(launcher, (None, "acme")) - assert str(conflict.value).endswith(f' -P "work&team" --bin-dir "{bin_dir}"') + assert str(launcher) in str(conflict.value) def test_docker_root_profile_flag_overrides_the_environment( diff --git a/cloudsmith_cli/credential_helpers/docker/installer.py b/cloudsmith_cli/credential_helpers/docker/installer.py index e98eb5795..02e093718 100644 --- a/cloudsmith_cli/credential_helpers/docker/installer.py +++ b/cloudsmith_cli/credential_helpers/docker/installer.py @@ -89,7 +89,13 @@ def _check_scope(launcher_path: Path, scope: tuple[str | None, ...]) -> None: """ if not launcher_path.exists(): return - env = parse_launcher_env(launcher_path.read_text(encoding="utf-8")) + try: + env = parse_launcher_env(launcher_path.read_text(encoding="utf-8")) + except ValueError as exc: + raise HelperScopeConflict( + f"Cannot read the settings in {launcher_path} ({exc}). Delete the" + " file, then run the command again." + ) from exc # An empty value gives the same helper name as no value. installed = tuple(env.get(name) or None for name in _SCOPE_ENVVARS) if installed != tuple(value or None for value in scope): @@ -106,6 +112,19 @@ def _check_scope(launcher_path: Path, scope: tuple[str | None, ...]) -> None: ) +def _cred_helpers() -> dict: + """Return the ``credHelpers`` block of ``config.json``, or ``{}``.""" + config_path = _docker_config_path() + if not config_path.exists(): + return {} + try: + data = json.loads(config_path.read_text(encoding="utf-8")) + except (ValueError, OSError): + return {} + cred_helpers = data.get("credHelpers") if isinstance(data, dict) else None + return cred_helpers if isinstance(cred_helpers, dict) else {} + + def _is_cloudsmith_helper(value) -> bool: return value == "cloudsmith" or str(value).startswith("cloudsmith-") @@ -424,14 +443,17 @@ def mutate(config: dict[str, object]) -> None: return actions def statuses(self) -> list[dict]: - """Return the status of each Cloudsmith helper in ``config.json``. + """Return the status of each Cloudsmith helper. Returns ------- list[dict] One dict for each ``cloudsmith`` or ``cloudsmith-*`` helper, in the - order of the ``credHelpers`` block, with keys: + order of the ``credHelpers`` block, then one for each other scoped + launcher in the default bin directory, with keys: + ``"name"`` + The ``credHelpers`` value of the helper. ``"launcher"`` The path of the launcher of the helper, or ``None`` if it does not exist. @@ -440,26 +462,24 @@ def statuses(self) -> list[dict]: Without such a helper, the list has only the unscoped helper. """ - config_path = _docker_config_path() hosts_by_helper: dict[str, list[str]] = {} - if config_path.exists(): - try: - data = json.loads(config_path.read_text(encoding="utf-8")) - if isinstance(data, dict): - cred_helpers = data.get("credHelpers", {}) - if isinstance(cred_helpers, dict): - for host, helper in cred_helpers.items(): - if _is_cloudsmith_helper(helper): - hosts_by_helper.setdefault(helper, []).append(host) - except (ValueError, OSError): - pass + for host, helper in _cred_helpers().items(): + if _is_cloudsmith_helper(helper): + hosts_by_helper.setdefault(helper, []).append(host) target_dir = resolve_bin_dir() + # A scoped launcher stays when a later install moves all of its hosts. + pattern = _launcher_path(target_dir, "cloudsmith-*") + for launcher_path in sorted(target_dir.glob(pattern.name)): + name = launcher_path.name.removesuffix(pattern.suffix) + hosts_by_helper.setdefault(name.removeprefix(_launcher_name("")), []) + statuses = [] for helper, hosts in (hosts_by_helper or {"cloudsmith": []}).items(): launcher_path = _launcher_path(target_dir, helper) statuses.append( { + "name": helper, "launcher": str(launcher_path) if launcher_path.exists() else None, "hosts": hosts, }