From 28a004b53d71615a9a674d78d1e027452ef798a5 Mon Sep 17 00:00:00 2001 From: Ian Duffy Date: Wed, 21 Oct 2026 17:25:57 +0100 Subject: [PATCH 01/12] 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 43243211..98f3e391 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 edc03abc..9c45d4dc 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 7585cf8b..9322cfb2 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 92162df0..9c8693c0 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 a7b4a13a..d6e3a780 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 ee724667..5a707582 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 55bb9182..d2c109e2 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 c0415954..d2d031d6 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 f1729307..17f4759f 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/12] 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 da5d690c..c570b6ad 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 cbea23d6..914dd7b9 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 8086e6f2..ceabd26d 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 87c0faaa7b48f119a2737ac00564c478cf350e23 Mon Sep 17 00:00:00 2001 From: Ian Duffy Date: Wed, 21 Oct 2026 17:29:43 +0100 Subject: [PATCH 03/12] feat(ENG-13680): add a NuGet credential provider `cloudsmith credential-helper install nuget` writes a `nuget-plugin-cloudsmith` launcher (`.bat` on Windows) on PATH. NuGet 6.13+ finds it and speaks the cross-platform plugin protocol to `cloudsmith credential-helper nuget`. The plugin answers for Cloudsmith hosts and the NuGet domains of the Workspace that is written into the launcher. Co-Authored-By: Claude Opus 5.5 # Conflicts: # CHANGELOG.md # cloudsmith_cli/cli/commands/credential_helper/manage.py # cloudsmith_cli/cli/tests/commands/test_credential_helper_install.py # cloudsmith_cli/core/api/init.py # cloudsmith_cli/credential_helpers/launchers.py # Conflicts: # cloudsmith_cli/cli/commands/credential_helper/manage.py # cloudsmith_cli/cli/tests/commands/test_credential_helper_install.py --- CHANGELOG.md | 1 + .../commands/credential_helper/__init__.py | 5 + .../cli/commands/credential_helper/manage.py | 2 + .../cli/commands/credential_helper/nuget.py | 94 ++++ cloudsmith_cli/cli/decorators.py | 1 + .../test_credential_helper_install.py | 4 +- .../cli/tests/test_credential_helper_nuget.py | 470 ++++++++++++++++++ cloudsmith_cli/core/api/init.py | 6 +- cloudsmith_cli/credential_helpers/common.py | 10 +- .../credential_helpers/launchers.py | 25 +- .../credential_helpers/nuget/__init__.py | 4 + .../credential_helpers/nuget/installer.py | 146 ++++++ .../credential_helpers/nuget/runtime.py | 343 +++++++++++++ 13 files changed, 1095 insertions(+), 16 deletions(-) create mode 100644 cloudsmith_cli/cli/commands/credential_helper/nuget.py create mode 100644 cloudsmith_cli/cli/tests/test_credential_helper_nuget.py create mode 100644 cloudsmith_cli/credential_helpers/nuget/__init__.py create mode 100644 cloudsmith_cli/credential_helpers/nuget/installer.py create mode 100644 cloudsmith_cli/credential_helpers/nuget/runtime.py diff --git a/CHANGELOG.md b/CHANGELOG.md index 6ff5c88d..eab93b96 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -14,6 +14,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0. - Added `--ssl-verify-strict/--without-ssl-verify-strict` (env `CLOUDSMITH_SSL_VERIFY_STRICT`: `1` enables strict verification, the default; `0` disables it; config key `ssl_verify_strict`) to control strict X.509 certificate verification (`VERIFY_X509_STRICT`). Python 3.13 and later turn this on by default, so certificates from TLS-intercepting proxies that miss some extensions fail verification. The CLI still verifies certificates against the trusted CA bundle. The option applies to API, SAML, upload, download, MCP, and update connections. - `cloudsmith update` now accepts the connection options, such as `--proxy` and `--without-ssl-verify-strict`. The background update check uses the proxy and SSL options of the command that runs it. - Added `--proxy` (env `CLOUDSMITH_PROXY`, config key `proxy`) to set the proxy for all HTTP requests of the CLI. `--api-proxy`, `CLOUDSMITH_API_PROXY` and `api_proxy` remain compatibility aliases. The hidden `CLOUDSMITH_ALLOWED_PROXY_SUFFIXES` setting replaces `CLOUDSMITH_ALLOWED_API_PROXY_SUFFIXES`, which also remains an alias. +- Added a NuGet credential provider. Run `cloudsmith credential-helper install nuget --workspace ` once. Then `dotnet restore`, MSBuild and Visual Studio authenticate to Cloudsmith feeds with your CLI credentials. NuGet finds the provider on PATH. This needs .NET SDK 9.0.200 or later. The provider also covers the NuGet custom domains of the Workspace. To name feed hosts yourself, use `--domain`. To skip the domain lookup, use `--no-discover`. No token is stored in `nuget.config`. ### Changed diff --git a/cloudsmith_cli/cli/commands/credential_helper/__init__.py b/cloudsmith_cli/cli/commands/credential_helper/__init__.py index bcf76528..412fd3c2 100644 --- a/cloudsmith_cli/cli/commands/credential_helper/__init__.py +++ b/cloudsmith_cli/cli/commands/credential_helper/__init__.py @@ -13,6 +13,7 @@ from .docker import docker as docker_cmd from .generic import generic as generic_cmd from .manage import install_cmd, list_cmd, uninstall_cmd +from .nuget import nuget as nuget_cmd from .pnpm import pnpm as pnpm_cmd from .terraform import terraform as terraform_cmd @@ -37,6 +38,9 @@ def credential_helper(): \b # Install cargo credential helper $ cloudsmith credential-helper install cargo + \b + # Install NuGet credential provider + $ cloudsmith credential-helper install nuget \b # Test Docker credential helper directly $ echo "docker.cloudsmith.io" | cloudsmith credential-helper docker @@ -54,5 +58,6 @@ def credential_helper(): credential_helper.add_command(list_cmd, name="list") credential_helper.add_command(cargo_cmd, name="cargo") credential_helper.add_command(terraform_cmd, name="terraform") +credential_helper.add_command(nuget_cmd, name="nuget") main.add_command(credential_helper, name="credential-helper") diff --git a/cloudsmith_cli/cli/commands/credential_helper/manage.py b/cloudsmith_cli/cli/commands/credential_helper/manage.py index c570b6ad..f2871d7d 100644 --- a/cloudsmith_cli/cli/commands/credential_helper/manage.py +++ b/cloudsmith_cli/cli/commands/credential_helper/manage.py @@ -17,6 +17,7 @@ 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.nuget.installer import NuGetInstaller from cloudsmith_cli.credential_helpers.pnpm.installer import PNPMInstaller from cloudsmith_cli.credential_helpers.terraform.installer import ( TerraformHelperExeNotFound, @@ -46,6 +47,7 @@ "pnpm": PNPMInstaller, "cargo": CargoInstaller, "terraform": TerraformInstaller, + "nuget": NuGetInstaller, } diff --git a/cloudsmith_cli/cli/commands/credential_helper/nuget.py b/cloudsmith_cli/cli/commands/credential_helper/nuget.py new file mode 100644 index 00000000..98a22770 --- /dev/null +++ b/cloudsmith_cli/cli/commands/credential_helper/nuget.py @@ -0,0 +1,94 @@ +# Copyright 2026 Cloudsmith Ltd +""" +NuGet credential provider command. + +Implements the NuGet cross-platform authentication plugin protocol for +Cloudsmith feeds. + +See: + https://learn.microsoft.com/en-us/nuget/reference/extensibility/nuget-cross-platform-plugins +""" + +import sys + +import click + +from ....core.credentials.chain import CredentialProviderChain +from ....credential_helpers.nuget import execute +from ...decorators import common_api_auth_options, initialise_api + + +def _utf8(stream, **kwargs): + """Switch a text stream to UTF-8, which the NuGet plugin protocol requires.""" + try: + stream.reconfigure(encoding="utf-8", **kwargs) + except (AttributeError, ValueError, OSError): + pass + return stream + + +@click.command( + context_settings={"ignore_unknown_options": True, "allow_interspersed_args": False} +) +@click.option( + "--domain", + "domains", + multiple=True, + help="A feed host to authenticate, as well as the Workspace's NuGet domains.", +) +@click.option( + "--no-discover", + is_flag=True, + help="Do not look up the Workspace's NuGet domains.", +) +@click.argument("nuget_args", nargs=-1, type=click.UNPROCESSED) +@common_api_auth_options +@initialise_api +def nuget(opts, domains, no_discover, nuget_args): + """ + NuGet credential provider for Cloudsmith feeds. + + NuGet (dotnet, MSBuild, Visual Studio) runs this command through the + ``nuget-plugin-cloudsmith`` launcher that ``cloudsmith credential-helper + install nuget`` puts on PATH, passing ``-Plugin`` after ``--``. It then + speaks the NuGet cross-platform plugin protocol on stdin/stdout. + + Provides credentials for all Cloudsmith NuGet feeds: ``*.cloudsmith.io``, + ``*.cloudsmith.com``, and any NuGet custom domains configured for the + Workspace (requires a Workspace - ``--workspace``, CLOUDSMITH_WORKSPACE, + or ``workspace`` in ``config.ini``; legacy aliases are also accepted - and + a valid API key/token). Any other feed is declined so NuGet falls back to + its other credential sources. + + \b + Examples: + + \b + # Called by NuGet via the launcher + $ nuget-plugin-cloudsmith -Plugin + + \b + Environment variables: + CLOUDSMITH_API_KEY: API key for authentication (optional) + CLOUDSMITH_WORKSPACE: Workspace slug (CLOUDSMITH_ORG is also accepted) + """ + protocol_stdout = _utf8(sys.stdout, newline="\n") + # Other output, such as the HTTP trace of --debug, must not reach NuGet. + sys.stdout = sys.stderr + exit_code, stderr = execute( + nuget_args, + _utf8(sys.stdin, errors="replace"), + protocol_stdout, + credential=opts.credential, + api_host=opts.api_host, + workspace=opts.org, + refresh_credential=lambda: CredentialProviderChain().resolve( + opts.credential_context + ), + domains=domains, + discover=not no_discover, + ) + + if stderr is not None: + click.echo(stderr, err=True) + sys.exit(exit_code) diff --git a/cloudsmith_cli/cli/decorators.py b/cloudsmith_cli/cli/decorators.py index 9c45d4dc..96fa9413 100644 --- a/cloudsmith_cli/cli/decorators.py +++ b/cloudsmith_cli/cli/decorators.py @@ -735,6 +735,7 @@ def wrapper(ctx, *args, **kwargs): click.secho(message, fg="yellow", err=True) opts.credential = credential + opts.credential_context = context kwargs["opts"] = opts return ctx.invoke(f, *args, **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 914dd7b9..768a3786 100644 --- a/cloudsmith_cli/cli/tests/commands/test_credential_helper_install.py +++ b/cloudsmith_cli/cli/tests/commands/test_credential_helper_install.py @@ -22,8 +22,8 @@ from ....credential_helpers.launchers import ( UnsafeLauncherValue, _launcher_content, - _launcher_filename, is_on_path, + launcher_filename, remove_launcher, resolve_bin_dir, write_launcher, @@ -94,7 +94,7 @@ def test_launcher_filename_and_content( ``NotImplementedError`` on Python < 3.12). """ assert ( - _launcher_filename(f"{format}-credential-cloudsmith", windows=windows) + launcher_filename(f"{format}-credential-cloudsmith", windows=windows) == expected_name ) assert ( diff --git a/cloudsmith_cli/cli/tests/test_credential_helper_nuget.py b/cloudsmith_cli/cli/tests/test_credential_helper_nuget.py new file mode 100644 index 00000000..e22204f6 --- /dev/null +++ b/cloudsmith_cli/cli/tests/test_credential_helper_nuget.py @@ -0,0 +1,470 @@ +# Copyright 2026 Cloudsmith Ltd +"""Tests for the `cloudsmith credential-helper nuget` command and installer.""" + +from __future__ import annotations + +import io +import json +import os +import sys +from unittest.mock import patch + +import click.testing +import cloudsmith_api +import pytest + +from ...core.api.exceptions import ApiException +from ...core.credentials.models import CredentialResult +from ...credential_helpers.common import REFUSAL_MESSAGE +from ...credential_helpers.nuget.installer import NuGetInstaller +from ...credential_helpers.nuget.runtime import ( + PluginSession, + is_plugin_mode, + is_supported_source, + negotiate_version, +) +from ..commands.credential_helper.manage import install_cmd, list_cmd +from ..commands.credential_helper.nuget import nuget +from ..commands.main import main + +CLOUDSMITH_FEED = "https://nuget.cloudsmith.io/acme/repo/v3/index.json" +CUSTOM_FEED = "https://nuget.acme.com/repo/v3/index.json" +NUGET_ORG = "https://api.nuget.org/v3/index.json" +FORMAT_DOMAINS = "cloudsmith_cli.credential_helpers.common.get_format_domains" +LAUNCHER = "nuget-plugin-cloudsmith" + + +@pytest.fixture() +def runner(): + """Return a CliRunner.""" + return click.testing.CliRunner() + + +@pytest.fixture() +def credential(): + """Return a resolved credential.""" + return CredentialResult(api_key="k_abc", source_name="test") + + +def _message(request_id, method, payload=None, message_type="Request") -> dict: + message = {"RequestId": request_id, "Type": message_type, "Method": method} + if payload is not None: + message["Payload"] = payload + return message + + +HANDSHAKE = _message( + "h", "Handshake", {"ProtocolVersion": "2.0.0", "MinimumProtocolVersion": "1.0.0"} +) + + +def _credentials_request(uri=CLOUDSMITH_FEED) -> dict: + return _message( + "c", + "GetAuthenticationCredentials", + {"Uri": uri, "IsRetry": False, "IsNonInteractive": True}, + ) + + +def _session(*messages, credential=None, workspace=None): + """Run a plugin session over *messages*; return (code, stderr, responses).""" + stdin = io.StringIO("".join(json.dumps(m) + "\n" for m in messages)) + stdout = io.StringIO() + code, stderr = PluginSession( + stdin, stdout, credential=credential, workspace=workspace + ).run() + sent = [json.loads(line) for line in stdout.getvalue().splitlines()] + return code, stderr, sent + + +def _run_cli(runner, args, *messages, command=nuget) -> list[dict]: + """Run the CLI over *messages* and return the messages on stdout.""" + stdin = "".join(json.dumps(m) + "\n" for m in messages) + result = runner.invoke(command, args=args, input=stdin, catch_exceptions=False) + return [json.loads(line) for line in result.stdout.splitlines()] + + +def _responses(sent) -> dict: + """Map RequestId -> message for everything the plugin answered.""" + return {m["RequestId"]: m for m in sent if m["Type"] != "Request"} + + +@pytest.mark.parametrize( + "args,expected", + [(["-Plugin"], True), (["/plugin"], True), (["-Uri", CLOUDSMITH_FEED], False)], +) +def test_is_plugin_mode(args, expected): + assert is_plugin_mode(args) is expected + + +@pytest.mark.parametrize( + "uri,workspace,expected", + [ + (CLOUDSMITH_FEED, None, True), + (NUGET_ORG, "acme", False), + (CUSTOM_FEED, None, False), + (CUSTOM_FEED, "acme", True), + ("http://nuget.cloudsmith.io/acme/repo/v3/index.json", None, False), + ("https://attacker.example?x=.cloudsmith.io", None, False), + (123, None, False), + ("https://attacker.example\\@nuget.cloudsmith.io/v3/index.json", None, False), + ], +) +def test_is_supported_source(credential, uri, workspace, expected): + """Only https Cloudsmith feeds, and NuGet domains of a Workspace, match.""" + with patch(FORMAT_DOMAINS, return_value=["nuget.acme.com"]): + assert ( + is_supported_source(uri, credential=credential, workspace=workspace) + is expected + ) + + +@pytest.mark.parametrize( + "uri,expected", + [ + (CUSTOM_FEED, True), + (CLOUDSMITH_FEED, True), + ("https://nuget.other.com/v3/index.json", False), + ], +) +def test_is_supported_source_without_discovery(credential, uri, expected): + """Explicit domains and Cloudsmith hosts match with no API lookup.""" + with patch(FORMAT_DOMAINS, side_effect=AssertionError("looked up")): + assert ( + is_supported_source( + uri, + credential=credential, + workspace="acme", + domains={"nuget.acme.com"}, + discover=False, + ) + is expected + ) + + +@pytest.mark.parametrize( + "payload,expected", + [ + ({"ProtocolVersion": "3.0.0", "MinimumProtocolVersion": "1.0.0"}, "2.0.0"), + ({"ProtocolVersion": "1.0.0", "MinimumProtocolVersion": "1.0.0"}, None), + ({"ProtocolVersion": "x"}, None), + ], +) +def test_negotiate_version(payload, expected): + assert negotiate_version(payload) == expected + + +def test_plugin_full_conversation(credential): + code, stderr, sent = _session( + HANDSHAKE, + _message("i", "Initialize", {"RequestTimeout": "00:00:05"}), + _message("o", "GetOperationClaims", {}), + _credentials_request(), + _message("x", "Close"), + credential=credential, + ) + responses = _responses(sent) + + assert (code, stderr) == (0, None) + assert responses["h"]["Payload"] == { + "ResponseCode": "Success", + "ProtocolVersion": "2.0.0", + } + assert responses["i"]["Payload"] == {"ResponseCode": "Success"} + assert responses["o"]["Payload"] == {"Claims": ["Authentication"]} + assert responses["c"]["Payload"] == { + "ResponseCode": "Success", + "Username": "token", + "Password": "k_abc", + "AuthenticationTypes": ["Basic"], + } + assert "x" not in responses + + +def test_plugin_claims_no_download_operations(credential): + claims = _message( + "o", + "GetOperationClaims", + {"PackageSourceRepository": CLOUDSMITH_FEED, "ServiceIndex": "{}"}, + ) + _, _, sent = _session(claims, credential=credential) + assert _responses(sent)["o"]["Payload"] == {"Claims": []} + + +def test_plugin_lookup_failure_is_not_found(credential, tmp_path): + """A failed domain lookup declines the feed, so NuGet tries other sources.""" + with ( + patch( + "cloudsmith_cli.credential_helpers.domains.get_cache_path", + return_value=tmp_path / "acme.json", + ), + patch( + "cloudsmith_cli.core.api.workspaces.list_domains", + side_effect=ApiException(0, detail="Connection refused"), + ), + ): + code, _, sent = _session( + _credentials_request(CUSTOM_FEED), credential=credential, workspace="acme" + ) + assert code == 0 + assert _responses(sent)["c"]["Payload"]["ResponseCode"] == "NotFound" + + +def test_plugin_reports_missing_credentials_for_a_cloudsmith_feed(): + code, stderr, sent = _session(_credentials_request()) + + payload = _responses(sent)["c"]["Payload"] + assert payload == {"ResponseCode": "Error", "Message": REFUSAL_MESSAGE} + assert (code, stderr) == (1, REFUSAL_MESSAGE) + + +@pytest.mark.parametrize( + "first_line,answered", + [ + ("not json", {"c": "Response"}), + (json.dumps(_message("u", "Unknown")), {"u": "Fault", "c": "Response"}), + ( + json.dumps( + _message("ours", "Handshake", {"ResponseCode": "Error"}, "Response") + ), + {}, + ), + ], + ids=["malformed-line", "unknown-method", "handshake-rejected"], +) +def test_plugin_session_handles_unexpected_messages(credential, first_line, answered): + stdin = io.StringIO(f"{first_line}\n{json.dumps(_credentials_request())}\n") + stdout = io.StringIO() + with patch( + "cloudsmith_cli.credential_helpers.nuget.runtime.uuid.uuid4", + return_value="ours", + ): + PluginSession(stdin, stdout, credential=credential).run() + + sent = [json.loads(line) for line in stdout.getvalue().splitlines()] + assert {m["RequestId"]: m["Type"] for m in sent[1:]} == answered + + +class _BrokenPipe(io.StringIO): + def write(self, _): + raise BrokenPipeError("gone") + + +def test_plugin_transport_failure_exits_without_a_message(credential): + """NuGet can close the pipes before the handshake; that is not a credential error.""" + code, stderr = PluginSession( + io.StringIO(), _BrokenPipe(), credential=credential + ).run() + assert (code, stderr) == (1, None) + + +def test_cli_speaks_the_plugin_protocol(runner): + sent = _run_cli( + runner, ["-k", "k_abc", "-Plugin"], HANDSHAKE, _credentials_request() + ) + + assert sent[0]["Method"] == "Handshake" + assert _responses(sent)["c"]["Payload"]["Password"] == "k_abc" + + +def test_cli_reads_input_that_is_not_utf8(runner): + """On Windows, NuGet writes to the plugin in the console code page.""" + proxy_login = json.dumps( + _message("p", "SetCredentials", {"ProxyPassword": "\u00e9t\u00e9"}), + ensure_ascii=False, + ) + stdin = f"{proxy_login}\n{json.dumps(_credentials_request())}\n".encode("cp1252") + result = runner.invoke( + nuget, args=["-k", "k_abc", "-Plugin"], input=stdin, catch_exceptions=False + ) + + sent = [json.loads(line) for line in result.stdout.splitlines()] + assert _responses(sent)["c"]["Payload"]["Password"] == "k_abc" + + +def test_cli_looks_up_domains_with_the_proxy_and_tls_settings(runner, tmp_path): + """A domain-cache miss reaches the API through the configured proxy.""" + seen = {} + + def list_domains(workspace, credential=None): + config = cloudsmith_api.Configuration() + seen.update(proxy=config.proxy, verify_ssl=config.verify_ssl) + record = {"domain_name": "nuget.acme.com", "domain_scope": "WORKSPACE"} + return [{**record, "domain_type": "NATIVE_NUGET"}] + + with ( + patch( + "cloudsmith_cli.credential_helpers.domains.get_cache_path", + return_value=tmp_path / "acme.json", + ), + patch("cloudsmith_cli.core.api.workspaces.list_domains", list_domains), + ): + args = ["-k", "k_abc", "-w", "acme", "--api-proxy", "http://proxy:3128", "-S"] + sent = _run_cli( + runner, [*args, "--", "-Plugin"], _credentials_request(CUSTOM_FEED) + ) + + assert seen == {"proxy": "http://proxy:3128", "verify_ssl": False} + assert _responses(sent)["c"]["Payload"]["ResponseCode"] == "Success" + + +@pytest.mark.parametrize( + "extra_args", [[], ["--api-headers", "Authorization=user:pass"]] +) +def test_cli_writes_only_protocol_messages_to_stdout_with_debug(runner, extra_args): + """Debug output goes to stderr, so it cannot break the protocol stream. + + With --debug, http.client prints each API request with print(). + """ + + def lookup_with_http_debug(*args, **kwargs): + print("send: b'GET /v2/workspaces/acme/domains/ HTTP/1.1'") + return True + + args = ["--debug", "credential-helper", "nuget", "-k", "k_abc", *extra_args] + with patch( + "cloudsmith_cli.credential_helpers.nuget.runtime.is_cloudsmith_domain", + lookup_with_http_debug, + ): + sent = _run_cli( + runner, [*args, "--", "-Plugin"], _credentials_request(), command=main + ) + assert _responses(sent)["c"]["Payload"]["Password"] == "k_abc" + + +def test_cli_retries_with_a_freshly_resolved_credential(runner): + retry = _credentials_request() + retry["Payload"]["IsRetry"] = True + renewed = CredentialResult(api_key="k_renewed", source_name="test") + with patch( + "cloudsmith_cli.cli.commands.credential_helper.nuget.CredentialProviderChain" + ) as chain: + chain.return_value.resolve.return_value = renewed + sent = _run_cli(runner, ["-k", "k_abc", "--", "-Plugin"], retry) + + assert _responses(sent)["c"]["Payload"]["Password"] == "k_renewed" + + +def test_cli_matches_explicit_domains(runner): + args = ["-k", "k_abc", "-w", "acme", "--domain", "NuGet.Acme.com", "--no-discover"] + with patch(FORMAT_DOMAINS, side_effect=AssertionError("looked up")): + sent = _run_cli( + runner, [*args, "--", "-Plugin"], _credentials_request(CUSTOM_FEED) + ) + + assert _responses(sent)["c"]["Payload"]["Password"] == "k_abc" + + +def test_cli_without_nuget_args_prints_usage(runner): + result = runner.invoke(nuget, args=["-k", "k_abc"], catch_exceptions=False) + assert result.exit_code == 1 + assert result.stdout == "" + assert "credential-helper install nuget" in result.stderr + + +@pytest.fixture() +def bin_dir(tmp_path, monkeypatch): + """Return a launcher directory that is on PATH.""" + path = tmp_path / "bin" + monkeypatch.setenv("PATH", str(path)) + for name in [name for name in os.environ if name.startswith("CLOUDSMITH_")]: + monkeypatch.delenv(name) + monkeypatch.delattr(sys, "frozen", raising=False) + return path + + +def test_installer_writes_the_launcher(bin_dir): + actions = NuGetInstaller().install(bin_dir=str(bin_dir)) + + assert (bin_dir / LAUNCHER).read_text() == ( + '#!/bin/sh\nexec cloudsmith credential-helper nuget -- "$@"\n' + ) + assert actions == [f"wrote launcher {bin_dir / LAUNCHER}"] + + +def test_installer_dry_run_and_uninstall_of_a_missing_launcher(bin_dir): + installer = NuGetInstaller() + + assert installer.install(bin_dir=str(bin_dir), dry_run=True) == [ + f"would write launcher {bin_dir / LAUNCHER}" + ] + assert installer.uninstall(bin_dir=str(bin_dir)) == [ + f"launcher not found at {bin_dir / LAUNCHER} (nothing to remove)" + ] + + +@pytest.fixture() +def on_windows(): + with ( + patch( + "cloudsmith_cli.credential_helpers.launchers.is_windows", return_value=True + ), + patch( + "cloudsmith_cli.credential_helpers.nuget.installer.is_windows", + return_value=True, + ), + ): + yield + + +@pytest.mark.usefixtures("on_windows") +def test_installer_uses_a_bat_launcher_on_windows(bin_dir): + """NuGet only discovers .exe and .bat plugins on Windows, not .cmd.""" + installer = NuGetInstaller() + installer.install( + bin_dir=str(bin_dir), + domains=("nuget.acme.com",), + launcher_env={"CLOUDSMITH_WORKSPACE": "acme"}, + ) + assert (bin_dir / f"{LAUNCHER}.bat").read_bytes() == ( + b"@echo off\r\n" + b"setlocal DisableDelayedExpansion\r\n" + b'set "CLOUDSMITH_WORKSPACE=acme"\r\n' + b'cloudsmith credential-helper nuget --domain "nuget.acme.com" -- %*\r\n' + ) + installer.uninstall(bin_dir=str(bin_dir)) + assert not (bin_dir / f"{LAUNCHER}.bat").exists() + + +def test_installer_passes_explicit_domains_to_the_plugin(bin_dir): + """NuGet sends every feed to the plugin, so the launcher names the hosts.""" + NuGetInstaller().install( + bin_dir=str(bin_dir), domains=("nuget.acme.com", "a b"), discover=False + ) + assert ( + "exec cloudsmith credential-helper nuget --domain nuget.acme.com " + "--domain 'a b' --no-discover -- \"$@\"" + ) in (bin_dir / LAUNCHER).read_text() + + +def test_list_command_reports_the_launcher(runner, bin_dir): + with patch( + "cloudsmith_cli.credential_helpers.nuget.installer.resolve_bin_dir", + return_value=bin_dir, + ): + before = runner.invoke(list_cmd, args=["-F", "json"], catch_exceptions=False) + NuGetInstaller().install(bin_dir=str(bin_dir)) + after = runner.invoke(list_cmd, args=["-F", "json"], catch_exceptions=False) + + def nuget_entry(result): + data = json.loads(result.stdout)["data"] + return next(entry for entry in data if entry["helper"] == "nuget") + + assert nuget_entry(before)["launcher"] is None + assert nuget_entry(after)["launcher"] == str(bin_dir / LAUNCHER) + assert nuget_entry(after)["hosts"] == ["all Cloudsmith NuGet feeds (nuget-plugin)"] + + +def test_install_command_keeps_install_time_settings(runner, bin_dir): + result = runner.invoke( + install_cmd, + args=["nuget", "--bin-dir", str(bin_dir), "-w", "acme"], + env={"CLOUDSMITH_API_KEY": "k_secret"}, + catch_exceptions=False, + ) + + assert result.exit_code == 0, result.output + assert (bin_dir / LAUNCHER).read_text() == ( + "#!/bin/sh\nexport CLOUDSMITH_WORKSPACE=acme\n" + 'exec cloudsmith credential-helper nuget -- "$@"\n' + ) diff --git a/cloudsmith_cli/core/api/init.py b/cloudsmith_cli/core/api/init.py index 8edf0b04..91219123 100644 --- a/cloudsmith_cli/core/api/init.py +++ b/cloudsmith_cli/core/api/init.py @@ -50,11 +50,11 @@ def initialise_api( config.api_key.pop("X-Api-Key", None) config.headers["Authorization"] = f"Bearer {credential.api_key}" if config.debug: - click.echo("SAML access token config value set") + click.echo("SAML access token config value set", err=True) else: config.api_key["X-Api-Key"] = credential.api_key if config.debug: - click.echo("User API key config value set") + click.echo("User API key config value set", err=True) auth_header = headers and config.headers.get("Authorization") if auth_header and " " in auth_header: @@ -65,7 +65,7 @@ def initialise_api( config.username, config.password = values.split(":") if config.debug: - click.echo("Username and password config values set") + click.echo("Username and password config values set", err=True) # Important! Some of the attributes set above (e.g. error_retry_max) are not # present in the cloudsmith_api.Configuration class declaration. diff --git a/cloudsmith_cli/credential_helpers/common.py b/cloudsmith_cli/credential_helpers/common.py index d6a0e98b..b29dbcc8 100644 --- a/cloudsmith_cli/credential_helpers/common.py +++ b/cloudsmith_cli/credential_helpers/common.py @@ -92,7 +92,7 @@ def is_standard_cloudsmith_domain(url): def is_cloudsmith_domain( - url, credential=None, api_host=None, backend_kind=None, org=None + url, credential=None, api_host=None, backend_kind=None, org=None, configure_api=True ): """ Check if a URL points to a Cloudsmith service. @@ -110,6 +110,8 @@ def is_cloudsmith_domain( org: Workspace slug whose custom domains to match against, as the CLI resolved it from --workspace, CLOUDSMITH_WORKSPACE, CLOUDSMITH_ORG, workspace in config.ini, or a legacy config alias + configure_api: Passed to the domain lookup. Pass ``False`` when the + caller has already configured the SDK. Returns: bool: True if this is a Cloudsmith domain @@ -137,13 +139,17 @@ def is_cloudsmith_domain( backend_kind, credential=credential, api_host=api_host, + configure_api=configure_api, ) } else: hosts = { d.host.lower() for d in get_workspace_domains( - org, credential=credential, api_host=api_host + org, + credential=credential, + api_host=api_host, + configure_api=configure_api, ) } return hostname in hosts diff --git a/cloudsmith_cli/credential_helpers/launchers.py b/cloudsmith_cli/credential_helpers/launchers.py index ceabd26d..e2e27be3 100644 --- a/cloudsmith_cli/credential_helpers/launchers.py +++ b/cloudsmith_cli/credential_helpers/launchers.py @@ -24,9 +24,9 @@ def is_windows() -> bool: return os.name == "nt" -def _launcher_filename(name: str, *, windows: bool) -> str: +def launcher_filename(name: str, *, windows: bool, windows_suffix: str = ".cmd") -> str: """Return the launcher file name for the platform (``.cmd`` on Windows).""" - return f"{name}.cmd" if windows else name + return f"{name}{windows_suffix}" if windows else name # A batch file expands % even inside quotes and cannot hold " or a line break. @@ -119,6 +119,7 @@ def write_launcher( name: str, target_cmd: str, dry_run=False, + windows_suffix=".cmd", env: dict[str, str] | None = None, ) -> Path: """Write a launcher script for *name* in *bin_dir* that execs *target_cmd*. @@ -132,6 +133,8 @@ def write_launcher( target_cmd: The command the launcher forwards to (e.g. ``cloudsmith credential-helper docker``). + windows_suffix: + The launcher file extension on Windows. NuGet discovers only ``.bat``. env: Environment variables that the launcher sets. @@ -141,16 +144,14 @@ def write_launcher( The path of the written file. """ windows = is_windows() + filename = launcher_filename(name, windows=windows, windows_suffix=windows_suffix) content = _launcher_content(target_cmd, windows=windows, env=env) if dry_run: - if windows: - return bin_dir / f"{name}.cmd" - else: - return bin_dir / name + return bin_dir / filename bin_dir = Path(bin_dir) bin_dir.mkdir(parents=True, exist_ok=True) - dest = bin_dir / _launcher_filename(name, windows=windows) + dest = bin_dir / filename dest.write_text(content, encoding="utf-8", newline="") if not windows: dest.chmod(0o755) @@ -158,7 +159,9 @@ def write_launcher( return dest -def remove_launcher(bin_dir: Path, name: str, dry_run=False) -> bool: +def remove_launcher( + bin_dir: Path, name: str, dry_run=False, windows_suffix=".cmd" +) -> bool: """Remove a launcher previously created by :func:`write_launcher`. Parameters @@ -167,13 +170,17 @@ def remove_launcher(bin_dir: Path, name: str, dry_run=False) -> bool: Directory that contains (or contained) the launcher. name: Base name of the helper binary (without extension). + windows_suffix: + The launcher file extension on Windows. Returns ------- bool ``True`` if a file was removed, ``False`` if no file was found. """ - target = Path(bin_dir) / _launcher_filename(name, windows=is_windows()) + target = Path(bin_dir) / launcher_filename( + name, windows=is_windows(), windows_suffix=windows_suffix + ) if target.exists(): if not dry_run: diff --git a/cloudsmith_cli/credential_helpers/nuget/__init__.py b/cloudsmith_cli/credential_helpers/nuget/__init__.py new file mode 100644 index 00000000..ac0d1d6a --- /dev/null +++ b/cloudsmith_cli/credential_helpers/nuget/__init__.py @@ -0,0 +1,4 @@ +# Copyright 2026 Cloudsmith Ltd +from .runtime import execute + +__all__ = ["execute"] diff --git a/cloudsmith_cli/credential_helpers/nuget/installer.py b/cloudsmith_cli/credential_helpers/nuget/installer.py new file mode 100644 index 00000000..33b66b8a --- /dev/null +++ b/cloudsmith_cli/credential_helpers/nuget/installer.py @@ -0,0 +1,146 @@ +# Copyright 2026 Cloudsmith Ltd +"""Installer for the NuGet credential provider. + +Writes a ``nuget-plugin-cloudsmith`` launcher into a directory on ``PATH``. +NuGet 6.13+ (.NET SDK 9.0.200+, Visual Studio 17.13+) discovers any +executable named ``nuget-plugin-*`` on ``PATH`` as an authentication plugin, +so no NuGet configuration file changes. On Windows NuGet only discovers +``.exe`` and ``.bat`` files, so the launcher is a ``.bat``. +""" + +from __future__ import annotations + +from ..common import path_warning +from ..launchers import ( + helper_command, + is_on_path, + is_windows, + launcher_arg, + launcher_filename, + remove_launcher, + resolve_bin_dir, + write_launcher, +) + + +class NuGetInstaller: + """Manages installation of the NuGet credential provider for Cloudsmith.""" + + LAUNCHER_NAME = "nuget-plugin-cloudsmith" + WINDOWS_SUFFIX = ".bat" + # NuGet can run the plugin from an IDE or a build agent without the + # install's environment, so the launcher keeps the install-time settings. + KEEPS_INSTALL_TIME_SETTINGS = True + + name = "nuget" + summary = "NuGet credential provider for Cloudsmith feeds" + + @classmethod + def _resolve_target_cmd(cls, domains: tuple[str, ...], discover: bool) -> str: + """Return the command the launcher forwards to. + + ``--`` stops the CLI from parsing NuGet's ``-Plugin`` argument as its + own option. + """ + args = [f"--domain {launcher_arg('--domain', domain)}" for domain in domains] + if not discover: + args.append("--no-discover") + return " ".join([helper_command(cls.name), *args, "--"]) + + def _launcher_path(self, target_dir): + return target_dir / launcher_filename( + self.LAUNCHER_NAME, windows=is_windows(), windows_suffix=self.WINDOWS_SUFFIX + ) + + def install( + self, + *, + bin_dir: str | None = None, + domains: tuple[str, ...] = (), + discover: bool = True, + launcher_env: dict[str, str] | None = None, + dry_run: bool = False, + # Accepted for a uniform installer interface. + **kwargs, + ) -> list[str]: + """Install the NuGet credential provider. + + Parameters + ---------- + bin_dir: + Override for the directory to install the launcher. It must be on + ``PATH`` for NuGet to discover the plugin. + domains: + Feed hosts that the plugin always authenticates. NuGet sends + every feed to the plugin, so the launcher passes them to it. + discover: + When ``False``, the plugin does not look up the Workspace's NuGet + domains through the Cloudsmith API. + launcher_env: + ``CLOUDSMITH_*`` settings that the launcher sets. + dry_run: + When ``True``, return planned actions without writing any files. + + Returns + ------- + list[str] + Human-readable descriptions of actions taken (or planned). + """ + target_dir = resolve_bin_dir(bin_dir) + launcher_path = write_launcher( + target_dir, + self.LAUNCHER_NAME, + self._resolve_target_cmd(domains, discover), + dry_run=dry_run, + windows_suffix=self.WINDOWS_SUFFIX, + env=launcher_env, + ) + actions = [ + ( + f"would write launcher {launcher_path}" + if dry_run + else f"wrote launcher {launcher_path}" + ) + ] + if launcher_env: + actions.append(f"launcher sets {', '.join(launcher_env)}") + + if not is_on_path(target_dir): + actions.append( + path_warning(target_dir, "NuGet", self.LAUNCHER_NAME, is_windows()) + ) + + return actions + + def uninstall( + self, *, bin_dir: str | None = None, dry_run: bool = False + ) -> list[str]: + """Remove the NuGet credential provider launcher.""" + target_dir = resolve_bin_dir(bin_dir) + launcher_path = self._launcher_path(target_dir) + + removed = remove_launcher( + target_dir, + self.LAUNCHER_NAME, + dry_run=dry_run, + windows_suffix=self.WINDOWS_SUFFIX, + ) + if not removed: + return [f"launcher not found at {launcher_path} (nothing to remove)"] + if dry_run: + return [f"would remove launcher {launcher_path}"] + return [f"removed launcher {launcher_path}"] + + def status(self) -> dict: + """Return current installation status. + + The plugin is host-agnostic: NuGet sends it every feed URL, so + ``hosts`` is a single marker when the launcher exists. + """ + launcher_path = self._launcher_path(resolve_bin_dir()) + if not launcher_path.exists(): + return {"launcher": None, "hosts": []} + return { + "launcher": str(launcher_path), + "hosts": ["all Cloudsmith NuGet feeds (nuget-plugin)"], + } diff --git a/cloudsmith_cli/credential_helpers/nuget/runtime.py b/cloudsmith_cli/credential_helpers/nuget/runtime.py new file mode 100644 index 00000000..591a2580 --- /dev/null +++ b/cloudsmith_cli/credential_helpers/nuget/runtime.py @@ -0,0 +1,343 @@ +# Copyright 2026 Cloudsmith Ltd +""" +NuGet credential provider runtime. + +Transport-light protocol logic for NuGet credential providers. This module is +intentionally free of Click/sys imports so it can be unit-tested without +invoking the CLI machinery. + +NuGet (``dotnet``, MSBuild and Visual Studio) runs the plugin with +``-Plugin`` and speaks the cross-platform authentication plugin protocol: a +bidirectional conversation of newline-delimited JSON messages on stdin/stdout +that opens with a symmetric handshake, where each side sends a ``Handshake`` +request and answers the other's. + +See: + https://learn.microsoft.com/en-us/nuget/reference/extensibility/nuget-cross-platform-plugins +""" + +import json +import logging +import uuid +from urllib.parse import urlsplit + +from ..backends import BackendKind +from ..common import ( + REFUSAL_MESSAGE, + extract_hostname, + is_cloudsmith_domain, + is_standard_cloudsmith_domain, +) + +logger = logging.getLogger(__name__) + +#: Plugin protocol version spoken; authentication needs at least 2.0.0. +PROTOCOL_VERSION = "2.0.0" +MINIMUM_PROTOCOL_VERSION = "2.0.0" + +_NOT_APPLICABLE_MESSAGE = "Not a Cloudsmith NuGet feed." + +USAGE_MESSAGE = ( + "This is the Cloudsmith NuGet credential provider. NuGet runs it with " + "-Plugin once it is installed with `cloudsmith credential-helper install nuget`." +) + + +def is_plugin_mode(args) -> bool: + """Return True when NuGet asked for the plugin protocol (``-Plugin``).""" + return any(arg.lstrip("-/").lower() == "plugin" for arg in args) + + +def is_supported_source( + uri, + credential=None, + api_host=None, + workspace=None, + domains=frozenset(), + discover=True, +) -> bool: + """Return True when *uri* is a Cloudsmith NuGet feed. + + Standard ``*.cloudsmith.io``/``*.cloudsmith.com`` hosts always match. + Other hosts match when the Workspace lists them as NuGet domains (looked + up via the Cloudsmith API and cached, with the SDK as the caller configured + it). Only ``https`` feeds match, so the API key is never sent in clear + text. The host comes from the URI's authority, so a query or fragment + cannot name a Cloudsmith host. A backslash is refused because .NET reads + it as a path separator. The hosts in *domains* always match. When + *discover* is False, no other custom domain matches and the API is not + called. + """ + if not isinstance(uri, str) or "\\" in uri: + return False + try: + parts = urlsplit(uri) + except ValueError: + return False + if parts.scheme.lower() != "https" or not parts.hostname: + return False + if parts.hostname in domains: + return True + if not discover: + return is_standard_cloudsmith_domain(parts.hostname) + + return is_cloudsmith_domain( + parts.hostname, + credential=credential, + api_host=api_host, + backend_kind=BackendKind.NUGET, + org=workspace, + configure_api=False, + ) + + +def _parse_version(value) -> tuple[int, ...] | None: + """Parse a ``major.minor.patch`` SemVer string, ignoring any suffix.""" + if not isinstance(value, str): + return None + core = value.split("-", 1)[0].split("+", 1)[0] + try: + return tuple(int(part) for part in core.split(".")) + except ValueError: + return None + + +def negotiate_version(payload) -> str | None: + """Return the protocol version for a Handshake request, or None if incompatible.""" + if not isinstance(payload, dict): + return None + theirs = _parse_version(payload.get("ProtocolVersion")) + their_minimum = _parse_version(payload.get("MinimumProtocolVersion")) + if theirs is None or their_minimum is None: + return None + ours = tuple(int(part) for part in PROTOCOL_VERSION.split(".")) + if not their_minimum <= ours <= theirs: + return None + return PROTOCOL_VERSION + + +class PluginSession: + """One NuGet plugin conversation over newline-delimited JSON streams.""" + + def __init__( + self, + stdin, + stdout, + credential=None, + api_host=None, + workspace=None, + refresh_credential=None, + domains=(), + discover=True, + ): + self.stdin = stdin + self.stdout = stdout + self.credential = credential + self.api_host = api_host + self.workspace = workspace + self.refresh_credential = refresh_credential + self.domains = frozenset(extract_hostname(domain) for domain in domains) + self.discover = discover + self.refused = False + self._handshake_id = str(uuid.uuid4()) + + def _send(self, request_id, message_type, method, payload=None) -> None: + """Write one message and flush it; NuGet reads line by line.""" + message = {"RequestId": request_id, "Type": message_type, "Method": method} + if payload is not None: + message["Payload"] = payload + self.stdout.write(json.dumps(message) + "\n") + self.stdout.flush() + + @staticmethod + def _handle_handshake(payload) -> dict: + version = negotiate_version(payload) + if version is None: + return {"ResponseCode": "Error"} + return {"ResponseCode": "Success", "ProtocolVersion": version} + + @staticmethod + def _handle_operation_claims(payload) -> dict: + # An authentication plugin is queried with no package source; a query + # for a specific source asks about package download, which this + # plugin does not offer. + payload = payload if isinstance(payload, dict) else {} + if payload.get("PackageSourceRepository") or payload.get("ServiceIndex"): + return {"Claims": []} + return {"Claims": ["Authentication"]} + + def _handle_get_credentials(self, payload) -> dict: + payload = payload if isinstance(payload, dict) else {} + if payload.get("IsRetry") and self.refresh_credential: + # NuGet retries after the feed rejected the last credential, which + # can have expired since the plugin started. + self.credential = self.refresh_credential() + uri = payload.get("Uri") + # A failed custom-domain lookup matches no host, so NuGet gets + # NotFound and tries its other credential sources. + if not is_supported_source( + uri, + credential=self.credential, + api_host=self.api_host, + workspace=self.workspace, + domains=self.domains, + discover=self.discover, + ): + return {"ResponseCode": "NotFound", "Message": _NOT_APPLICABLE_MESSAGE} + + if not self.credential or not self.credential.api_key: + self.refused = True + return {"ResponseCode": "Error", "Message": REFUSAL_MESSAGE} + + return { + "ResponseCode": "Success", + "Username": "token", + "Password": self.credential.api_key, + "AuthenticationTypes": ["Basic"], + } + + def handle_request(self, method, payload) -> dict | None: + """Return the response payload for a request, or None for a fault.""" + if method == "Handshake": + return self._handle_handshake(payload) + if method == "GetOperationClaims": + return self._handle_operation_claims(payload) + if method == "GetAuthenticationCredentials": + return self._handle_get_credentials(payload) + if method in ( + "Initialize", + "SetLogLevel", + "SetCredentials", + "MonitorNuGetProcessExit", + ): + # Nothing to configure: credentials come from the CLI's own + # provider chain and every request is answered immediately. + return {"ResponseCode": "Success"} + return None + + def _dispatch(self, message) -> bool: + """Handle one decoded message; return False when the session ends.""" + if not isinstance(message, dict): + return True + + request_id = message.get("RequestId") + message_type = message.get("Type") + method = message.get("Method") + + if message_type == "Response": + if request_id == self._handshake_id: + payload = message.get("Payload") + code = ( + payload.get("ResponseCode") if isinstance(payload, dict) else None + ) + if code != "Success": + logger.debug("NuGet rejected the plugin handshake: %r", code) + return False + return True + + if message_type != "Request" or not request_id: + # Progress, Cancel and Fault messages need no answer. + return True + + if method == "Close": + return False + + response = self.handle_request(method, message.get("Payload")) + if response is None: + self._send( + request_id, + "Fault", + method, + {"Message": f"Unsupported request method {method!r}"}, + ) + else: + self._send(request_id, "Response", method, response) + return True + + def run(self) -> tuple[int, str | None]: + """ + Run the conversation until NuGet sends Close or closes stdin. + + Returns: + A (exit_code, stderr_text) tuple. Outcomes are reported in-band; + the exit code only flags a session that refused a Cloudsmith feed + for lack of credentials, or that broke at the transport level. + """ + try: + # The handshake is symmetric: NuGet waits for ours as well as for + # our answer to its own. + self._send( + self._handshake_id, + "Request", + "Handshake", + { + "ProtocolVersion": PROTOCOL_VERSION, + "MinimumProtocolVersion": MINIMUM_PROTOCOL_VERSION, + }, + ) + + for line in self.stdin: + line = line.strip() + if not line: + continue + try: + message = json.loads(line) + except ValueError as exc: + logger.debug("Ignoring malformed NuGet message: %s", exc) + continue + if not self._dispatch(message): + break + except (OSError, ValueError) as exc: + # Protocol boundary: a broken pipe, a closed stream or undecodable + # input ends the session cleanly rather than with a traceback. + # NuGet has gone away, so a credential message would mislead. + logger.debug("nuget plugin session failed: %s", exc, exc_info=True) + return (1, None) + + if self.refused: + return (1, REFUSAL_MESSAGE) + return (0, None) + + +def execute( + args, + stdin, + stdout, + credential=None, + api_host=None, + workspace=None, + refresh_credential=None, + domains=(), + discover=True, +) -> tuple[int, str | None]: + """ + Run the NuGet credential provider for the given NuGet arguments. + + Args: + args: The arguments NuGet passed (``["-Plugin"]``) + stdin: A text stream to read plugin messages from + stdout: A text stream to write plugin messages to + credential: Pre-resolved CredentialResult from the provider chain + api_host: Cloudsmith API host URL + workspace: Workspace slug whose custom domains to match against + refresh_credential: Callable that resolves the credential again when + NuGet retries a request + domains: Feed hosts that always match + discover: When False, do not look up the Workspace's NuGet domains + + Returns: + A (exit_code, stderr_text) tuple. + """ + if not is_plugin_mode(args): + return (1, USAGE_MESSAGE) + + return PluginSession( + stdin, + stdout, + credential=credential, + api_host=api_host, + workspace=workspace, + refresh_credential=refresh_credential, + domains=domains, + discover=discover, + ).run() From 0352b88e0ab7d90a76294e94d846fa6c094c532f Mon Sep 17 00:00:00 2001 From: Ian Duffy Date: Wed, 28 Oct 2026 09:36:50 +0000 Subject: [PATCH 04/12] 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 05/12] 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 2f2464b9..b999909e 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 7a28b06c..7cc93f79 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 8086e6f2..ceabd26d 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 9a3096c9284aa664be5ea47c633d8f1d4bbf34c2 Mon Sep 17 00:00:00 2001 From: Ian Duffy Date: Tue, 3 Nov 2026 10:48:08 +0000 Subject: [PATCH 06/12] Test that list finds a NuGet launcher in the default directory The test runs `install nuget` with no --bin-dir and does not patch the resolver, so `list` must look in ~/.cloudsmith/bin. Co-Authored-By: Claude Opus 5.5 --- .../cli/tests/test_credential_helper_nuget.py | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/cloudsmith_cli/cli/tests/test_credential_helper_nuget.py b/cloudsmith_cli/cli/tests/test_credential_helper_nuget.py index e22204f6..e237427c 100644 --- a/cloudsmith_cli/cli/tests/test_credential_helper_nuget.py +++ b/cloudsmith_cli/cli/tests/test_credential_helper_nuget.py @@ -7,6 +7,7 @@ import json import os import sys +from pathlib import Path from unittest.mock import patch import click.testing @@ -437,21 +438,20 @@ def test_installer_passes_explicit_domains_to_the_plugin(bin_dir): ) in (bin_dir / LAUNCHER).read_text() -def test_list_command_reports_the_launcher(runner, bin_dir): - with patch( - "cloudsmith_cli.credential_helpers.nuget.installer.resolve_bin_dir", - return_value=bin_dir, - ): - before = runner.invoke(list_cmd, args=["-F", "json"], catch_exceptions=False) - NuGetInstaller().install(bin_dir=str(bin_dir)) - after = runner.invoke(list_cmd, args=["-F", "json"], catch_exceptions=False) +def test_list_command_reports_the_launcher(runner, bin_dir, monkeypatch): + """`list` finds the launcher where `install` puts it by default.""" + monkeypatch.setattr(Path, "home", staticmethod(lambda: bin_dir.parent)) + before = runner.invoke(list_cmd, args=["-F", "json"], catch_exceptions=False) + runner.invoke(install_cmd, args=["nuget"], catch_exceptions=False) + after = runner.invoke(list_cmd, args=["-F", "json"], catch_exceptions=False) def nuget_entry(result): data = json.loads(result.stdout)["data"] return next(entry for entry in data if entry["helper"] == "nuget") + default_dir = bin_dir.parent / ".cloudsmith" / "bin" assert nuget_entry(before)["launcher"] is None - assert nuget_entry(after)["launcher"] == str(bin_dir / LAUNCHER) + assert nuget_entry(after)["launcher"] == str(default_dir / LAUNCHER) assert nuget_entry(after)["hosts"] == ["all Cloudsmith NuGet feeds (nuget-plugin)"] From 58c0b9e5dee21e617103d339a364b6837199f744 Mon Sep 17 00:00:00 2001 From: Ian Duffy Date: Tue, 3 Nov 2026 10:53:03 +0000 Subject: [PATCH 07/12] Send Progress while a NuGet request runs A worker thread answers GetAuthenticationCredentials. The main thread sends Progress at half of the Initialize request timeout until the answer is ready. It keeps reading stdin, so a Cancel gets a Cancel at once and the late answer is not sent. Co-Authored-By: Claude Opus 5.5 --- .../cli/tests/test_credential_helper_nuget.py | 75 ++++++++++- .../credential_helpers/nuget/runtime.py | 118 ++++++++++++++++-- 2 files changed, 178 insertions(+), 15 deletions(-) diff --git a/cloudsmith_cli/cli/tests/test_credential_helper_nuget.py b/cloudsmith_cli/cli/tests/test_credential_helper_nuget.py index e237427c..e43fe796 100644 --- a/cloudsmith_cli/cli/tests/test_credential_helper_nuget.py +++ b/cloudsmith_cli/cli/tests/test_credential_helper_nuget.py @@ -7,6 +7,7 @@ import json import os import sys +import threading from pathlib import Path from unittest.mock import patch @@ -23,6 +24,7 @@ is_plugin_mode, is_supported_source, negotiate_version, + parse_timespan, ) from ..commands.credential_helper.manage import install_cmd, list_cmd from ..commands.credential_helper.nuget import nuget @@ -67,10 +69,10 @@ def _credentials_request(uri=CLOUDSMITH_FEED) -> dict: ) -def _session(*messages, credential=None, workspace=None): +def _session(*messages, credential=None, workspace=None, stdout=None): """Run a plugin session over *messages*; return (code, stderr, responses).""" stdin = io.StringIO("".join(json.dumps(m) + "\n" for m in messages)) - stdout = io.StringIO() + stdout = io.StringIO() if stdout is None else stdout code, stderr = PluginSession( stdin, stdout, credential=credential, workspace=workspace ).run() @@ -246,6 +248,75 @@ def test_plugin_session_handles_unexpected_messages(credential, first_line, answ assert {m["RequestId"]: m["Type"] for m in sent[1:]} == answered +class _MessageWatcher(io.StringIO): + """A stdout that sets ``seen`` when the plugin sends a message of one type.""" + + def __init__(self, message_type): + super().__init__() + self.message_type = message_type + self.seen = threading.Event() + + def write(self, text): + if json.loads(text)["Type"] == self.message_type: + self.seen.set() + return super().write(text) + + +def test_plugin_sends_progress_while_a_request_runs(credential): + """Progress before each Initialize deadline keeps NuGet waiting.""" + stdout = _MessageWatcher("Progress") + initialize = _message("i", "Initialize", {"RequestTimeout": "00:00:00.0200000"}) + with patch( + "cloudsmith_cli.credential_helpers.nuget.runtime.is_cloudsmith_domain", + lambda *args, **kwargs: stdout.seen.wait(timeout=1), + ): + _, _, sent = _session( + initialize, + _credentials_request(CUSTOM_FEED), + credential=credential, + workspace="acme", + stdout=stdout, + ) + + answers = [m for m in sent if m["RequestId"] == "c"] + assert answers[0] == { + "RequestId": "c", + "Type": "Progress", + "Method": "GetAuthenticationCredentials", + "Payload": {}, + } + assert {m["Type"] for m in answers[:-1]} == {"Progress"} + assert answers[-1]["Type"] == "Response" + assert answers[-1]["Payload"]["ResponseCode"] == "Success" + + +def test_plugin_answers_a_cancel_while_a_request_runs(credential): + """A Cancel gets a Cancel at once, and the late result is not sent.""" + stdout = _MessageWatcher("Cancel") + cancel = _message("c", "GetAuthenticationCredentials", message_type="Cancel") + with patch( + "cloudsmith_cli.credential_helpers.nuget.runtime.is_cloudsmith_domain", + lambda *args, **kwargs: stdout.seen.wait(timeout=1), + ): + _, _, sent = _session( + _credentials_request(CUSTOM_FEED), + cancel, + credential=credential, + workspace="acme", + stdout=stdout, + ) + + assert [(m["RequestId"], m["Type"]) for m in sent[1:]] == [("c", "Cancel")] + + +@pytest.mark.parametrize( + "value,expected", + [("00:00:05", 5.0), ("1.02:03:04.5", 93784.5), ("5", None), (None, None)], +) +def test_parse_timespan(value, expected): + assert parse_timespan(value) == expected + + class _BrokenPipe(io.StringIO): def write(self, _): raise BrokenPipeError("gone") diff --git a/cloudsmith_cli/credential_helpers/nuget/runtime.py b/cloudsmith_cli/credential_helpers/nuget/runtime.py index 591a2580..f70d0c0b 100644 --- a/cloudsmith_cli/credential_helpers/nuget/runtime.py +++ b/cloudsmith_cli/credential_helpers/nuget/runtime.py @@ -16,9 +16,15 @@ https://learn.microsoft.com/en-us/nuget/reference/extensibility/nuget-cross-platform-plugins """ +import collections import json import logging +import queue +import re +import threading +import time import uuid +from concurrent.futures import Future, ThreadPoolExecutor from urllib.parse import urlsplit from ..backends import BackendKind @@ -35,6 +41,14 @@ PROTOCOL_VERSION = "2.0.0" MINIMUM_PROTOCOL_VERSION = "2.0.0" +#: The request timeout in seconds until NuGet sends ``Initialize``. +DEFAULT_REQUEST_TIMEOUT = 5.0 + +#: A .NET TimeSpan in the constant format, ``[d.]hh:mm:ss[.fffffff]``. +_TIMESPAN = re.compile(r"(?:(\d+)\.)?(\d+):(\d+):(\d+(?:\.\d+)?)") + +_END_OF_INPUT = object() + _NOT_APPLICABLE_MESSAGE = "Not a Cloudsmith NuGet feed." USAGE_MESSAGE = ( @@ -102,6 +116,17 @@ def _parse_version(value) -> tuple[int, ...] | None: return None +def parse_timespan(value) -> float | None: + """Return the seconds in a .NET TimeSpan string such as ``00:00:05``, or None.""" + match = _TIMESPAN.fullmatch(value) if isinstance(value, str) else None + if match is None: + return None + days, hours, minutes, seconds = match.groups() + return ((int(days or 0) * 24 + int(hours)) * 60 + int(minutes)) * 60 + float( + seconds + ) + + def negotiate_version(payload) -> str | None: """Return the protocol version for a Handshake request, or None if incompatible.""" if not isinstance(payload, dict): @@ -116,6 +141,15 @@ def negotiate_version(payload) -> str | None: return PROTOCOL_VERSION +def _is_cancel(message, request_id) -> bool: + """Return True when *message* cancels the request *request_id*.""" + return ( + isinstance(message, dict) + and message.get("Type") == "Cancel" + and message.get("RequestId") == request_id + ) + + class PluginSession: """One NuGet plugin conversation over newline-delimited JSON streams.""" @@ -139,7 +173,12 @@ def __init__( self.domains = frozenset(extract_hostname(domain) for domain in domains) self.discover = discover self.refused = False + self.request_timeout = DEFAULT_REQUEST_TIMEOUT self._handshake_id = str(uuid.uuid4()) + self._inbox = queue.Queue() + self._backlog = collections.deque() + # One worker thread runs one request at a time, also after a Cancel. + self._worker = ThreadPoolExecutor(max_workers=1) def _send(self, request_id, message_type, method, payload=None) -> None: """Write one message and flush it; NuGet reads line by line.""" @@ -166,6 +205,13 @@ def _handle_operation_claims(payload) -> dict: return {"Claims": []} return {"Claims": ["Authentication"]} + def _handle_initialize(self, payload) -> dict: + payload = payload if isinstance(payload, dict) else {} + request_timeout = parse_timespan(payload.get("RequestTimeout")) + if request_timeout: + self.request_timeout = request_timeout + return {"ResponseCode": "Success"} + def _handle_get_credentials(self, payload) -> dict: payload = payload if isinstance(payload, dict) else {} if payload.get("IsRetry") and self.refresh_credential: @@ -204,17 +250,67 @@ def handle_request(self, method, payload) -> dict | None: return self._handle_operation_claims(payload) if method == "GetAuthenticationCredentials": return self._handle_get_credentials(payload) + if method == "Initialize": + return self._handle_initialize(payload) if method in ( - "Initialize", "SetLogLevel", "SetCredentials", "MonitorNuGetProcessExit", ): # Nothing to configure: credentials come from the CLI's own - # provider chain and every request is answered immediately. + # provider chain. return {"ResponseCode": "Success"} return None + def _answer_with_progress(self, request_id, method, payload) -> None: + """Answer a request from the worker, with Progress before each deadline. + + A Cancel for the request ends the wait. Other messages wait in the backlog. + """ + future = self._worker.submit(self.handle_request, method, payload) + future.add_done_callback(self._inbox.put) + interval = self.request_timeout / 2 + progress_due = time.monotonic() + interval + while True: + try: + item = self._inbox.get( + timeout=max(0.0, progress_due - time.monotonic()) + ) + except queue.Empty: + self._send(request_id, "Progress", method, {}) + progress_due = time.monotonic() + interval + continue + if item is future: + self._send(request_id, "Response", method, future.result()) + return + if _is_cancel(item, request_id): + self._send(request_id, "Cancel", method) + return + if not isinstance(item, Future): + self._backlog.append(item) + + def _read_messages(self) -> None: + """Queue each stdin message, then the end of input, for the main thread.""" + try: + for line in self.stdin: + line = line.strip() + if not line: + continue + try: + self._inbox.put(json.loads(line)) + except ValueError as exc: + logger.debug("Ignoring malformed NuGet message: %s", exc) + except (OSError, ValueError) as exc: + self._inbox.put(exc) + self._inbox.put(_END_OF_INPUT) + + def _next_message(self): + """Return the next message, or raise the error that ended stdin.""" + message = self._backlog.popleft() if self._backlog else self._inbox.get() + if isinstance(message, Exception): + raise message + return message + def _dispatch(self, message) -> bool: """Handle one decoded message; return False when the session ends.""" if not isinstance(message, dict): @@ -242,8 +338,11 @@ def _dispatch(self, message) -> bool: if method == "Close": return False - response = self.handle_request(method, message.get("Payload")) - if response is None: + if method == "GetAuthenticationCredentials": + # A domain lookup or a credential renewal can take longer than + # NuGet's request timeout. + self._answer_with_progress(request_id, method, message.get("Payload")) + elif (response := self.handle_request(method, message.get("Payload"))) is None: self._send( request_id, "Fault", @@ -276,15 +375,8 @@ def run(self) -> tuple[int, str | None]: }, ) - for line in self.stdin: - line = line.strip() - if not line: - continue - try: - message = json.loads(line) - except ValueError as exc: - logger.debug("Ignoring malformed NuGet message: %s", exc) - continue + threading.Thread(target=self._read_messages, daemon=True).start() + while (message := self._next_message()) is not _END_OF_INPUT: if not self._dispatch(message): break except (OSError, ValueError) as exc: From 4589130543231336c8a4673939ac1a6d380ee628 Mon Sep 17 00:00:00 2001 From: Ian Duffy Date: Tue, 3 Nov 2026 10:57:45 +0000 Subject: [PATCH 08/12] Resolve NuGet credentials after the handshake `initialise_api` and `resolve_credentials` take a `defer` option. The NuGet command uses it, so it sends the handshake before the provider chain runs. The first credentials request resolves the credential in the worker thread, which sends Progress. The command keeps the credential on `opts`, where telemetry redacts it. Co-Authored-By: Claude Opus 5.5 --- .../cli/commands/credential_helper/nuget.py | 15 ++++--- cloudsmith_cli/cli/decorators.py | 17 +++++--- .../cli/tests/test_credential_helper_nuget.py | 40 +++++++++++++++++-- .../credential_helpers/nuget/runtime.py | 12 ++++-- 4 files changed, 67 insertions(+), 17 deletions(-) diff --git a/cloudsmith_cli/cli/commands/credential_helper/nuget.py b/cloudsmith_cli/cli/commands/credential_helper/nuget.py index 98a22770..7c3b3b01 100644 --- a/cloudsmith_cli/cli/commands/credential_helper/nuget.py +++ b/cloudsmith_cli/cli/commands/credential_helper/nuget.py @@ -27,6 +27,12 @@ def _utf8(stream, **kwargs): return stream +def _resolve_credential(opts): + """Resolve the credential and keep it on *opts*, where telemetry redacts it.""" + opts.credential = CredentialProviderChain().resolve(opts.credential_context) + return opts.credential + + @click.command( context_settings={"ignore_unknown_options": True, "allow_interspersed_args": False} ) @@ -43,7 +49,9 @@ def _utf8(stream, **kwargs): ) @click.argument("nuget_args", nargs=-1, type=click.UNPROCESSED) @common_api_auth_options -@initialise_api +# NuGet gives the handshake a deadline, so a slow credential, such as an OIDC +# exchange, resolves at the first credentials request. +@initialise_api(defer_credentials=True) def nuget(opts, domains, no_discover, nuget_args): """ NuGet credential provider for Cloudsmith feeds. @@ -79,12 +87,9 @@ def nuget(opts, domains, no_discover, nuget_args): nuget_args, _utf8(sys.stdin, errors="replace"), protocol_stdout, - credential=opts.credential, api_host=opts.api_host, workspace=opts.org, - refresh_credential=lambda: CredentialProviderChain().resolve( - opts.credential_context - ), + refresh_credential=lambda: _resolve_credential(opts), domains=domains, discover=not no_discover, ) diff --git a/cloudsmith_cli/cli/decorators.py b/cloudsmith_cli/cli/decorators.py index 96fa9413..fd578930 100644 --- a/cloudsmith_cli/cli/decorators.py +++ b/cloudsmith_cli/cli/decorators.py @@ -618,8 +618,13 @@ def _warn_on_oidc_detector_controls(order, disabled): ) -def resolve_credentials(f): - """Resolve credentials via the provider chain. Depends on initialise_session.""" +def resolve_credentials(f=None, *, defer=False): + """Resolve credentials via the provider chain. Depends on initialise_session. + + With ``defer=True``, the command resolves them from ``opts.credential_context``. + """ + if f is None: + return functools.partial(resolve_credentials, defer=defer) @click.option( "--oidc-audience", @@ -707,7 +712,7 @@ def wrapper(ctx, *args, **kwargs): ) chain = CredentialProviderChain() - credential = chain.resolve(context) + credential = None if defer else chain.resolve(context) if context.keyring_refresh_failed and not is_auth_command: if credential: @@ -743,8 +748,10 @@ def wrapper(ctx, *args, **kwargs): return initialise_session(wrapper) -def initialise_api(f): +def initialise_api(f=None, *, defer_credentials=False): """Initialise the Cloudsmith API for use. Depends on resolve_credentials.""" + if f is None: + return functools.partial(initialise_api, defer_credentials=defer_credentials) @click.option( "-R", @@ -821,7 +828,7 @@ def call_print_rate_limit_info_with_opts(rate_info): kwargs["opts"] = opts return ctx.invoke(f, *args, **kwargs) - return resolve_credentials(wrapper) + return resolve_credentials(wrapper, defer=defer_credentials) def initialise_mcp(f): diff --git a/cloudsmith_cli/cli/tests/test_credential_helper_nuget.py b/cloudsmith_cli/cli/tests/test_credential_helper_nuget.py index e43fe796..2c251ef7 100644 --- a/cloudsmith_cli/cli/tests/test_credential_helper_nuget.py +++ b/cloudsmith_cli/cli/tests/test_credential_helper_nuget.py @@ -29,6 +29,7 @@ from ..commands.credential_helper.manage import install_cmd, list_cmd from ..commands.credential_helper.nuget import nuget from ..commands.main import main +from ..config import OPTIONS CLOUDSMITH_FEED = "https://nuget.cloudsmith.io/acme/repo/v3/index.json" CUSTOM_FEED = "https://nuget.acme.com/repo/v3/index.json" @@ -404,17 +405,48 @@ def lookup_with_http_debug(*args, **kwargs): assert _responses(sent)["c"]["Payload"]["Password"] == "k_abc" -def test_cli_retries_with_a_freshly_resolved_credential(runner): +def test_cli_retries_with_a_freshly_resolved_credential(runner, credential): retry = _credentials_request() + retry["RequestId"] = "r" retry["Payload"]["IsRetry"] = True renewed = CredentialResult(api_key="k_renewed", source_name="test") with patch( "cloudsmith_cli.cli.commands.credential_helper.nuget.CredentialProviderChain" ) as chain: - chain.return_value.resolve.return_value = renewed - sent = _run_cli(runner, ["-k", "k_abc", "--", "-Plugin"], retry) + chain.return_value.resolve.side_effect = [credential, renewed] + sent = _run_cli( + runner, ["-k", "k_abc", "--", "-Plugin"], _credentials_request(), retry + ) + + assert _responses(sent)["c"]["Payload"]["Password"] == "k_abc" + assert _responses(sent)["r"]["Payload"]["Password"] == "k_renewed" + + +def test_cli_resolves_the_credential_at_the_first_credentials_request(runner): + """The handshake does not wait for a slow credential, such as an OIDC exchange.""" + oidc = CredentialResult(api_key="k_oidc", source_name="test") + with ( + patch("cloudsmith_cli.cli.decorators.CredentialProviderChain") as chain, + patch( + "cloudsmith_cli.cli.commands.credential_helper.nuget.CredentialProviderChain", + chain, + ), + ): + chain.return_value.resolve.return_value = oidc + _run_cli(runner, ["--", "-Plugin"], HANDSHAKE) + assert not chain.return_value.resolve.called + + requests = (_credentials_request(), _credentials_request()) + sent = _run_cli(runner, ["--", "-Plugin"], *requests) + + assert chain.return_value.resolve.call_count == 1 + assert _responses(sent)["c"]["Payload"]["Password"] == "k_oidc" + - assert _responses(sent)["c"]["Payload"]["Password"] == "k_renewed" +def test_cli_keeps_the_resolved_credential_on_the_options(runner): + """Telemetry redacts ``opts.credential``, so the late credential goes there.""" + _run_cli(runner, ["-k", "k_abc", "--", "-Plugin"], _credentials_request()) + assert OPTIONS.value.credential.api_key == "k_abc" def test_cli_matches_explicit_domains(runner): diff --git a/cloudsmith_cli/credential_helpers/nuget/runtime.py b/cloudsmith_cli/credential_helpers/nuget/runtime.py index f70d0c0b..5f809fa0 100644 --- a/cloudsmith_cli/credential_helpers/nuget/runtime.py +++ b/cloudsmith_cli/credential_helpers/nuget/runtime.py @@ -170,6 +170,7 @@ def __init__( self.api_host = api_host self.workspace = workspace self.refresh_credential = refresh_credential + self._credential_resolved = credential is not None self.domains = frozenset(extract_hostname(domain) for domain in domains) self.discover = discover self.refused = False @@ -214,10 +215,14 @@ def _handle_initialize(self, payload) -> dict: def _handle_get_credentials(self, payload) -> dict: payload = payload if isinstance(payload, dict) else {} - if payload.get("IsRetry") and self.refresh_credential: + if self.refresh_credential and ( + payload.get("IsRetry") or not self._credential_resolved + ): + # The first request resolves a credential that the caller did not give. # NuGet retries after the feed rejected the last credential, which # can have expired since the plugin started. self.credential = self.refresh_credential() + self._credential_resolved = True uri = payload.get("Uri") # A failed custom-domain lookup matches no host, so NuGet gets # NotFound and tries its other credential sources. @@ -412,8 +417,9 @@ def execute( credential: Pre-resolved CredentialResult from the provider chain api_host: Cloudsmith API host URL workspace: Workspace slug whose custom domains to match against - refresh_credential: Callable that resolves the credential again when - NuGet retries a request + refresh_credential: Callable that resolves the credential at the + first request when *credential* is None, and again when NuGet + retries a request domains: Feed hosts that always match discover: When False, do not look up the Workspace's NuGet domains From 7530face44cd25c3565d678373d9b5edcde38275 Mon Sep 17 00:00:00 2001 From: Ian Duffy Date: Tue, 3 Nov 2026 11:11:32 +0000 Subject: [PATCH 09/12] Say where NuGet status looks for the launcher Co-Authored-By: Claude Opus 5.5 --- cloudsmith_cli/credential_helpers/nuget/installer.py | 1 + 1 file changed, 1 insertion(+) diff --git a/cloudsmith_cli/credential_helpers/nuget/installer.py b/cloudsmith_cli/credential_helpers/nuget/installer.py index 33b66b8a..2a75bda7 100644 --- a/cloudsmith_cli/credential_helpers/nuget/installer.py +++ b/cloudsmith_cli/credential_helpers/nuget/installer.py @@ -137,6 +137,7 @@ def status(self) -> dict: The plugin is host-agnostic: NuGet sends it every feed URL, so ``hosts`` is a single marker when the launcher exists. """ + # resolve_bin_dir() gives ~/.cloudsmith/bin, where `install` writes by default. launcher_path = self._launcher_path(resolve_bin_dir()) if not launcher_path.exists(): return {"launcher": None, "hosts": []} From b385219028b068acaff39a31d1fa2310acfb3593 Mon Sep 17 00:00:00 2001 From: Ian Duffy Date: Tue, 3 Nov 2026 11:17:51 +0000 Subject: [PATCH 10/12] End the NuGet session at once when NuGet goes away Close, the end of stdin or a stdin error now ends the session while a request runs. The plugin sends no more Progress and drops the late answer. Each request runs in a daemon thread, so a slow lookup cannot delay the exit. A request that raises gets a Fault. The tests now play NuGet: they send Close or end stdin only after the plugin answers each request. Co-Authored-By: Claude Opus 5.5 --- .../cli/tests/test_credential_helper_nuget.py | 149 +++++++++++++++--- .../credential_helpers/nuget/runtime.py | 54 +++++-- 2 files changed, 174 insertions(+), 29 deletions(-) diff --git a/cloudsmith_cli/cli/tests/test_credential_helper_nuget.py b/cloudsmith_cli/cli/tests/test_credential_helper_nuget.py index 2c251ef7..d607a833 100644 --- a/cloudsmith_cli/cli/tests/test_credential_helper_nuget.py +++ b/cloudsmith_cli/cli/tests/test_credential_helper_nuget.py @@ -3,6 +3,7 @@ from __future__ import annotations +import collections import io import json import os @@ -70,21 +71,77 @@ def _credentials_request(uri=CLOUDSMITH_FEED) -> dict: ) +class _NuGetFeed: + """NuGet's end of stdin: it sends Close or ends the input only after each answer. + + A message can be a raw line, which needs no answer. + """ + + def __init__(self, messages): + self.messages = messages + self.requests = collections.Counter() + self.answers = collections.Counter() + self.answered = threading.Condition() + + def record(self, send): + """Wrap ``PluginSession._send`` so that the feed sees each answer.""" + + def send_and_record(session, request_id, message_type, method, payload=None): + send(session, request_id, message_type, method, payload) + if message_type not in ("Request", "Progress"): + with self.answered: + self.answers[request_id] += 1 + self.answered.notify_all() + + return send_and_record + + def _wait_for_answers(self): + with self.answered: + self.answered.wait_for(lambda: self.answers >= self.requests, timeout=2) + + def lines(self): + """Yield each message as the line that NuGet writes.""" + for message in self.messages: + if isinstance(message, str): + yield message + "\n" + continue + if message.get("Method") == "Close": + self._wait_for_answers() + elif message.get("Type") == "Request": + self.requests[message["RequestId"]] += 1 + yield json.dumps(message, ensure_ascii=False) + "\n" + self._wait_for_answers() + + def _session(*messages, credential=None, workspace=None, stdout=None): """Run a plugin session over *messages*; return (code, stderr, responses).""" - stdin = io.StringIO("".join(json.dumps(m) + "\n" for m in messages)) + feed = _NuGetFeed(messages) stdout = io.StringIO() if stdout is None else stdout - code, stderr = PluginSession( - stdin, stdout, credential=credential, workspace=workspace - ).run() + with patch.object(PluginSession, "_send", feed.record(PluginSession._send)): + code, stderr = PluginSession( + feed.lines(), stdout, credential=credential, workspace=workspace + ).run() sent = [json.loads(line) for line in stdout.getvalue().splitlines()] return code, stderr, sent -def _run_cli(runner, args, *messages, command=nuget) -> list[dict]: +def _run_cli(runner, args, *messages, command=nuget, encoding="utf-8") -> list[dict]: """Run the CLI over *messages* and return the messages on stdout.""" - stdin = "".join(json.dumps(m) + "\n" for m in messages) - result = runner.invoke(command, args=args, input=stdin, catch_exceptions=False) + feed = _NuGetFeed(messages) + read_fd, write_fd = os.pipe() + + def write_feed(): + with open(write_fd, "w", encoding=encoding) as pipe: + for line in feed.lines(): + pipe.write(line) + pipe.flush() + + threading.Thread(target=write_feed, daemon=True).start() + with ( + open(read_fd, "rb") as stdin, + patch.object(PluginSession, "_send", feed.record(PluginSession._send)), + ): + result = runner.invoke(command, args=args, input=stdin, catch_exceptions=False) return [json.loads(line) for line in result.stdout.splitlines()] @@ -237,15 +294,12 @@ def test_plugin_reports_missing_credentials_for_a_cloudsmith_feed(): ids=["malformed-line", "unknown-method", "handshake-rejected"], ) def test_plugin_session_handles_unexpected_messages(credential, first_line, answered): - stdin = io.StringIO(f"{first_line}\n{json.dumps(_credentials_request())}\n") - stdout = io.StringIO() with patch( "cloudsmith_cli.credential_helpers.nuget.runtime.uuid.uuid4", return_value="ours", ): - PluginSession(stdin, stdout, credential=credential).run() + _, _, sent = _session(first_line, _credentials_request(), credential=credential) - sent = [json.loads(line) for line in stdout.getvalue().splitlines()] assert {m["RequestId"]: m["Type"] for m in sent[1:]} == answered @@ -310,6 +364,64 @@ def test_plugin_answers_a_cancel_while_a_request_runs(credential): assert [(m["RequestId"], m["Type"]) for m in sent[1:]] == [("c", "Cancel")] +def _lines_then(lines, error): + """Yield *lines* as NuGet's stdin, then raise *error* if it is given.""" + yield from lines + if error is not None: + raise error + + +@pytest.mark.parametrize( + "after,error,code", + [ + ([json.dumps(_message("x", "Close"))], None, 0), + ([], None, 0), + ([], OSError("stdin closed"), 1), + ], + ids=["close", "end-of-input", "stdin-error"], +) +def test_plugin_ends_the_session_while_a_request_runs(credential, after, error, code): + """NuGet has gone, so the plugin drops the late answer and does not wait for it.""" + entered, release = threading.Event(), threading.Event() + daemon = [] + + def slow_lookup(*args, **kwargs): + daemon.append(threading.current_thread().daemon) + entered.set() + return release.wait(timeout=1) + + lines = [json.dumps(_credentials_request(CUSTOM_FEED)), *after] + stdout = io.StringIO() + with patch( + "cloudsmith_cli.credential_helpers.nuget.runtime.is_cloudsmith_domain", + slow_lookup, + ): + result = PluginSession( + _lines_then(lines, error), stdout, credential=credential, workspace="acme" + ).run() + assert entered.wait(timeout=1) + release.set() + + sent = [json.loads(line) for line in stdout.getvalue().splitlines()] + assert result == (code, None) + assert [m["Method"] for m in sent] == ["Handshake"] + assert daemon == [True] + + +@pytest.mark.filterwarnings("ignore::pytest.PytestUnhandledThreadExceptionWarning") +def test_plugin_faults_a_request_that_fails(credential): + """A request that raises gets a Fault, so NuGet does not wait for it.""" + with patch( + "cloudsmith_cli.credential_helpers.nuget.runtime.is_cloudsmith_domain", + side_effect=RuntimeError("lookup failed"), + ): + _, _, sent = _session( + _credentials_request(CUSTOM_FEED), credential=credential, workspace="acme" + ) + + assert _responses(sent)["c"]["Type"] == "Fault" + + @pytest.mark.parametrize( "value,expected", [("00:00:05", 5.0), ("1.02:03:04.5", 93784.5), ("5", None), (None, None)], @@ -342,16 +454,15 @@ def test_cli_speaks_the_plugin_protocol(runner): def test_cli_reads_input_that_is_not_utf8(runner): """On Windows, NuGet writes to the plugin in the console code page.""" - proxy_login = json.dumps( - _message("p", "SetCredentials", {"ProxyPassword": "\u00e9t\u00e9"}), - ensure_ascii=False, - ) - stdin = f"{proxy_login}\n{json.dumps(_credentials_request())}\n".encode("cp1252") - result = runner.invoke( - nuget, args=["-k", "k_abc", "-Plugin"], input=stdin, catch_exceptions=False + proxy_login = _message("p", "SetCredentials", {"ProxyPassword": "\u00e9t\u00e9"}) + sent = _run_cli( + runner, + ["-k", "k_abc", "-Plugin"], + proxy_login, + _credentials_request(), + encoding="cp1252", ) - sent = [json.loads(line) for line in result.stdout.splitlines()] assert _responses(sent)["c"]["Payload"]["Password"] == "k_abc" diff --git a/cloudsmith_cli/credential_helpers/nuget/runtime.py b/cloudsmith_cli/credential_helpers/nuget/runtime.py index 5f809fa0..e838a648 100644 --- a/cloudsmith_cli/credential_helpers/nuget/runtime.py +++ b/cloudsmith_cli/credential_helpers/nuget/runtime.py @@ -24,7 +24,6 @@ import threading import time import uuid -from concurrent.futures import Future, ThreadPoolExecutor from urllib.parse import urlsplit from ..backends import BackendKind @@ -150,6 +149,26 @@ def _is_cancel(message, request_id) -> bool: ) +def _ends_session(message) -> bool: + """Return True for input that ends the session: Close, end of input or an error.""" + if message is _END_OF_INPUT or isinstance(message, Exception): + return True + return ( + isinstance(message, dict) + and message.get("Type") == "Request" + and message.get("Method") == "Close" + and bool(message.get("RequestId")) + ) + + +class _Answer: + """A worker's answer to one request. It stays a Fault if the request fails.""" + + def __init__(self): + self.message_type = "Fault" + self.payload = {"Message": "The plugin could not answer the request."} + + class PluginSession: """One NuGet plugin conversation over newline-delimited JSON streams.""" @@ -178,8 +197,8 @@ def __init__( self._handshake_id = str(uuid.uuid4()) self._inbox = queue.Queue() self._backlog = collections.deque() - # One worker thread runs one request at a time, also after a Cancel. - self._worker = ThreadPoolExecutor(max_workers=1) + # One request runs at a time, also after a Cancel. + self._request_lock = threading.Lock() def _send(self, request_id, message_type, method, payload=None) -> None: """Write one message and flush it; NuGet reads line by line.""" @@ -267,13 +286,25 @@ def handle_request(self, method, payload) -> dict | None: return {"ResponseCode": "Success"} return None + def _work(self, answer, method, payload) -> None: + """Answer one request in a daemon thread, which cannot delay the exit.""" + try: + with self._request_lock: + answer.payload = self.handle_request(method, payload) + answer.message_type = "Response" + finally: + self._inbox.put(answer) + def _answer_with_progress(self, request_id, method, payload) -> None: - """Answer a request from the worker, with Progress before each deadline. + """Answer a request from a worker thread, with Progress before each deadline. A Cancel for the request ends the wait. Other messages wait in the backlog. + Close, end of input or a stdin error ends the session at once. """ - future = self._worker.submit(self.handle_request, method, payload) - future.add_done_callback(self._inbox.put) + answer = _Answer() + threading.Thread( + target=self._work, args=(answer, method, payload), daemon=True + ).start() interval = self.request_timeout / 2 progress_due = time.monotonic() + interval while True: @@ -285,14 +316,17 @@ def _answer_with_progress(self, request_id, method, payload) -> None: self._send(request_id, "Progress", method, {}) progress_due = time.monotonic() + interval continue - if item is future: - self._send(request_id, "Response", method, future.result()) + if item is answer: + self._send(request_id, answer.message_type, method, answer.payload) return if _is_cancel(item, request_id): self._send(request_id, "Cancel", method) return - if not isinstance(item, Future): - self._backlog.append(item) + if _ends_session(item): + # NuGet has gone, so the session ends before the backlog. + self._backlog.appendleft(item) + return + self._backlog.append(item) def _read_messages(self) -> None: """Queue each stdin message, then the end of input, for the main thread.""" From 6738c00292937903ff609f393f8280f3f615633d Mon Sep 17 00:00:00 2001 From: Ian Duffy Date: Tue, 3 Nov 2026 11:19:57 +0000 Subject: [PATCH 11/12] Stop keyring prompts when NuGet forbids interaction The credential callback takes the request's IsNonInteractive value. When it is true, the command calls keyring.forbid_password_prompts(), so an encrypted keyring refuses to prompt also at a terminal. Co-Authored-By: Claude Opus 5.5 --- .../cli/commands/credential_helper/nuget.py | 9 +++++++-- .../cli/tests/test_credential_helper_nuget.py | 18 ++++++++++++++++++ cloudsmith_cli/core/keyring.py | 13 +++++++++++-- cloudsmith_cli/core/tests/test_keyring.py | 9 +++++++++ .../credential_helpers/nuget/runtime.py | 8 ++++++-- 5 files changed, 51 insertions(+), 6 deletions(-) diff --git a/cloudsmith_cli/cli/commands/credential_helper/nuget.py b/cloudsmith_cli/cli/commands/credential_helper/nuget.py index 7c3b3b01..e84637ca 100644 --- a/cloudsmith_cli/cli/commands/credential_helper/nuget.py +++ b/cloudsmith_cli/cli/commands/credential_helper/nuget.py @@ -13,6 +13,7 @@ import click +from ....core import keyring from ....core.credentials.chain import CredentialProviderChain from ....credential_helpers.nuget import execute from ...decorators import common_api_auth_options, initialise_api @@ -27,8 +28,10 @@ def _utf8(stream, **kwargs): return stream -def _resolve_credential(opts): +def _resolve_credential(opts, non_interactive): """Resolve the credential and keep it on *opts*, where telemetry redacts it.""" + if non_interactive: + keyring.forbid_password_prompts() opts.credential = CredentialProviderChain().resolve(opts.credential_context) return opts.credential @@ -89,7 +92,9 @@ def nuget(opts, domains, no_discover, nuget_args): protocol_stdout, api_host=opts.api_host, workspace=opts.org, - refresh_credential=lambda: _resolve_credential(opts), + refresh_credential=lambda non_interactive: _resolve_credential( + opts, non_interactive + ), domains=domains, discover=not no_discover, ) diff --git a/cloudsmith_cli/cli/tests/test_credential_helper_nuget.py b/cloudsmith_cli/cli/tests/test_credential_helper_nuget.py index d607a833..d785a36f 100644 --- a/cloudsmith_cli/cli/tests/test_credential_helper_nuget.py +++ b/cloudsmith_cli/cli/tests/test_credential_helper_nuget.py @@ -16,6 +16,7 @@ import cloudsmith_api import pytest +from ...core import keyring as core_keyring from ...core.api.exceptions import ApiException from ...core.credentials.models import CredentialResult from ...credential_helpers.common import REFUSAL_MESSAGE @@ -554,6 +555,23 @@ def test_cli_resolves_the_credential_at_the_first_credentials_request(runner): assert _responses(sent)["c"]["Payload"]["Password"] == "k_oidc" +@pytest.mark.parametrize("non_interactive", [True, False]) +def test_cli_passes_the_interaction_policy_to_the_keyring(runner, non_interactive): + """When NuGet forbids prompts, the keyring does not ask for its password.""" + request = _credentials_request() + request["Payload"]["IsNonInteractive"] = non_interactive + no_prompt = [] + with patch( + "cloudsmith_cli.cli.commands.credential_helper.nuget.CredentialProviderChain" + ) as chain: + chain.return_value.resolve.side_effect = lambda context: no_prompt.append( + core_keyring.current_settings().no_prompt + ) + _run_cli(runner, ["--", "-Plugin"], request) + + assert no_prompt == [non_interactive] + + def test_cli_keeps_the_resolved_credential_on_the_options(runner): """Telemetry redacts ``opts.credential``, so the late credential goes there.""" _run_cli(runner, ["-k", "k_abc", "--", "-Plugin"], _credentials_request()) diff --git a/cloudsmith_cli/core/keyring.py b/cloudsmith_cli/core/keyring.py index d2c109e2..5564bab8 100644 --- a/cloudsmith_cli/core/keyring.py +++ b/cloudsmith_cli/core/keyring.py @@ -12,13 +12,14 @@ @dataclasses.dataclass(frozen=True) class KeyringSettings: - """The CLOUDSMITH_*KEYRING* settings that the CLI parsed.""" + """The CLOUDSMITH_*KEYRING* settings that the CLI parsed, and the prompt policy.""" no_keyring: bool = False backend: str | None = None key: str | None = None file_path: str | None = None directory: str | None = None + no_prompt: bool = False class _Current: @@ -38,6 +39,12 @@ def current_settings(): return _Current.settings +def forbid_password_prompts(): + """Make the keyring backend refuse to prompt for a password in this process.""" + if not _Current.settings.no_prompt: + configure(dataclasses.replace(_Current.settings, no_prompt=True)) + + def should_use_keyring(): """Check if keyring should be used based on CLOUDSMITH_NO_KEYRING.""" return not _Current.settings.no_keyring @@ -139,7 +146,9 @@ def _disable_password_prompts(backend): key. A chain calls a member only when the earlier members did not handle the call, so the error occurs only for a member that is in use. """ - if is_interactive(os.environ, controlling_terminal_mode()): + if not _Current.settings.no_prompt and is_interactive( + os.environ, controlling_terminal_mode() + ): return for member in getattr(backend, "backends", None) or [backend]: if hasattr(member, "_unlock"): diff --git a/cloudsmith_cli/core/tests/test_keyring.py b/cloudsmith_cli/core/tests/test_keyring.py index d2d031d6..ffee41e4 100644 --- a/cloudsmith_cli/core/tests/test_keyring.py +++ b/cloudsmith_cli/core/tests/test_keyring.py @@ -951,6 +951,15 @@ def test_supplied_key_uses_keyring_without_terminal( assert store_oidc_token(*OIDC_ARGS, "token-data") is True assert get_oidc_token(*OIDC_ARGS) == "token-data" + def test_forbidden_prompts_skip_keyring_at_a_terminal( + self, tmp_path, mock_get_user + ): + """A caller that must not prompt, such as NuGet, stops a terminal prompt too.""" + core_keyring.forbid_password_prompts() + backend = CryptFileKeyring() + with _keyring_session(backend, _keyring_env(tmp_path), has_terminal=True): + assert store_oidc_token(*OIDC_ARGS, "token-data") is False + def test_wrong_key_reports_not_found(self, tmp_path, mock_get_user): _write_oidc_entry(CryptFileKeyring(), tmp_path) core_keyring.configure(KeyringSettings(key="wrong-password")) diff --git a/cloudsmith_cli/credential_helpers/nuget/runtime.py b/cloudsmith_cli/credential_helpers/nuget/runtime.py index e838a648..4bdac29f 100644 --- a/cloudsmith_cli/credential_helpers/nuget/runtime.py +++ b/cloudsmith_cli/credential_helpers/nuget/runtime.py @@ -240,7 +240,10 @@ def _handle_get_credentials(self, payload) -> dict: # The first request resolves a credential that the caller did not give. # NuGet retries after the feed rejected the last credential, which # can have expired since the plugin started. - self.credential = self.refresh_credential() + self.credential = self.refresh_credential( + # A plugin must not prompt unless NuGet allows it. + non_interactive=payload.get("IsNonInteractive", True) + ) self._credential_resolved = True uri = payload.get("Uri") # A failed custom-domain lookup matches no host, so NuGet gets @@ -453,7 +456,8 @@ def execute( workspace: Workspace slug whose custom domains to match against refresh_credential: Callable that resolves the credential at the first request when *credential* is None, and again when NuGet - retries a request + retries a request. It takes the request's ``IsNonInteractive`` + value as ``non_interactive``. domains: Feed hosts that always match discover: When False, do not look up the Workspace's NuGet domains From 57bbd5c9f389510587787960b7a45a61d5d5729c Mon Sep 17 00:00:00 2001 From: Ian Duffy Date: Tue, 3 Nov 2026 11:30:46 +0000 Subject: [PATCH 12/12] Decline other feeds before a NuGet credential lookup, and retry a failed one Co-Authored-By: Claude Opus 5.5 --- .../cli/tests/test_credential_helper_nuget.py | 29 ++++++++++++++++++- .../credential_helpers/nuget/runtime.py | 11 +++++-- 2 files changed, 37 insertions(+), 3 deletions(-) diff --git a/cloudsmith_cli/cli/tests/test_credential_helper_nuget.py b/cloudsmith_cli/cli/tests/test_credential_helper_nuget.py index d785a36f..89eeb8c7 100644 --- a/cloudsmith_cli/cli/tests/test_credential_helper_nuget.py +++ b/cloudsmith_cli/cli/tests/test_credential_helper_nuget.py @@ -10,7 +10,7 @@ import sys import threading from pathlib import Path -from unittest.mock import patch +from unittest.mock import Mock, patch import click.testing import cloudsmith_api @@ -410,6 +410,33 @@ def slow_lookup(*args, **kwargs): @pytest.mark.filterwarnings("ignore::pytest.PytestUnhandledThreadExceptionWarning") +def test_plugin_resolves_again_after_a_failed_resolution(credential): + """A first failure does not stay, so a later request can get the credential.""" + resolve = Mock(side_effect=[None, credential]) + session = PluginSession(io.StringIO(), io.StringIO(), refresh_credential=resolve) + payload = _credentials_request()["Payload"] + + first = session.handle_request("GetAuthenticationCredentials", payload) + second = session.handle_request("GetAuthenticationCredentials", payload) + + assert first["ResponseCode"] == "Error" + assert second["Password"] == "k_abc" + + +def test_plugin_without_discovery_declines_a_feed_before_it_resolves(credential): + """The feed of another provider does not wait for a credential.""" + resolve = Mock(return_value=credential) + session = PluginSession( + io.StringIO(), io.StringIO(), refresh_credential=resolve, discover=False + ) + payload = _credentials_request(CUSTOM_FEED)["Payload"] + + response = session.handle_request("GetAuthenticationCredentials", payload) + + assert response["ResponseCode"] == "NotFound" + assert not resolve.called + + def test_plugin_faults_a_request_that_fails(credential): """A request that raises gets a Fault, so NuGet does not wait for it.""" with patch( diff --git a/cloudsmith_cli/credential_helpers/nuget/runtime.py b/cloudsmith_cli/credential_helpers/nuget/runtime.py index 4bdac29f..ccc18b58 100644 --- a/cloudsmith_cli/credential_helpers/nuget/runtime.py +++ b/cloudsmith_cli/credential_helpers/nuget/runtime.py @@ -234,6 +234,13 @@ def _handle_initialize(self, payload) -> dict: def _handle_get_credentials(self, payload) -> dict: payload = payload if isinstance(payload, dict) else {} + uri = payload.get("Uri") + # Without discovery the host check needs no credential, so another + # provider's feed does not wait for one. + if not self.discover and not is_supported_source( + uri, domains=self.domains, discover=False + ): + return {"ResponseCode": "NotFound", "Message": _NOT_APPLICABLE_MESSAGE} if self.refresh_credential and ( payload.get("IsRetry") or not self._credential_resolved ): @@ -244,8 +251,8 @@ def _handle_get_credentials(self, payload) -> dict: # A plugin must not prompt unless NuGet allows it. non_interactive=payload.get("IsNonInteractive", True) ) - self._credential_resolved = True - uri = payload.get("Uri") + # A failed resolution is not kept, so a later request tries again. + self._credential_resolved = self.credential is not None # A failed custom-domain lookup matches no host, so NuGet gets # NotFound and tries its other credential sources. if not is_supported_source(