diff --git a/CHANGELOG.md b/CHANGELOG.md index 147fb191..a61e03ee 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,12 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0. ## [Unreleased] +### Fixed + +- The CLI no longer prompts for a keyring password when no user can answer. A run is non-interactive when `CI` is set or when no controlling terminal is available. In such a run, a keyring call that needs a password that was not supplied fails without a prompt. OIDC tokens then use the on-disk cache, and `cloudsmith auth` prints why it did not store the SSO tokens. Previously, headless Linux agents printed `GetPassWarning: Can not control echo on the terminal` after each OIDC exchange. To keep a keyring in CI, set `CLOUDSMITH_KEYRING_BACKEND` to a backend that does not prompt, or to an encrypted backend together with `CLOUDSMITH_KEYRING_KEY`. +- `CLOUDSMITH_KEYRING_KEY`, `CLOUDSMITH_KEYRING_DIR`, and `CLOUDSMITH_KEYRING_FILE_PATH` now apply to the default keyring on headless Linux. That keyring is a chain of backends. Previously, the CLI applied these settings to the chain and not to the encrypted backend that stores the values, so the settings had no effect. +- A `CLOUDSMITH_KEYRING_KEY` that does not unlock the keyring file no longer stops commands such as `logout` and `whoami` with a `ValueError`. The CLI treats the keyring as not available. The CLI also unlocks an encrypted keyring file once for each run, not once for each keyring call. + ## [1.27.0] - 2026-09-08 ### Added diff --git a/cloudsmith_cli/cli/tests/test_webserver.py b/cloudsmith_cli/cli/tests/test_webserver.py index 6f6f1e8b..170b10bb 100644 --- a/cloudsmith_cli/cli/tests/test_webserver.py +++ b/cloudsmith_cli/cli/tests/test_webserver.py @@ -6,6 +6,7 @@ import click import pytest +from keyring.errors import KeyringError from ...core.api.exceptions import ApiException from ..webserver import AuthenticationWebRequestHandler, AuthenticationWebServer @@ -137,11 +138,26 @@ def test_store_sso_tokens_receives_profile(self, mock_handler): profile="staging", ) - def test_message_shown_when_keyring_disabled(self, mock_handler): - """Verify message is shown when store_sso_tokens returns False.""" + @pytest.mark.parametrize( + "store_result,message", + [ + ( + {"return_value": False}, + "SSO tokens not stored (CLOUDSMITH_NO_KEYRING is set)", + ), + ( + {"side_effect": KeyringError("no prompt")}, + "SSO tokens not stored: no prompt", + ), + ], + ) + def test_message_shown_when_tokens_not_stored( + self, mock_handler, store_result, message + ): + """Verify message is shown when store_sso_tokens does not store tokens.""" with ( patch( - "cloudsmith_cli.cli.webserver.store_sso_tokens", return_value=False + "cloudsmith_cli.cli.webserver.store_sso_tokens", **store_result ) as mock_store, patch("click.echo") as mock_echo, patch.object(mock_handler, "_return_success_response"), @@ -164,14 +180,11 @@ def test_message_shown_when_keyring_disabled(self, mock_handler): mock_handler.do_GET() - # store_sso_tokens should be called (returns False) + # store_sso_tokens should be called mock_store.assert_called_once() # Message should be displayed to stderr - mock_echo.assert_called_once_with( - "SSO tokens not stored (CLOUDSMITH_NO_KEYRING is set)", - err=True, - ) + mock_echo.assert_called_once_with(message, err=True) def test_access_token_stored_on_server_instance(self, mock_handler): """Verify the SSO access token is stored on the server instance for direct use.""" diff --git a/cloudsmith_cli/cli/webserver.py b/cloudsmith_cli/cli/webserver.py index 54466377..fdcd3cb5 100644 --- a/cloudsmith_cli/cli/webserver.py +++ b/cloudsmith_cli/cli/webserver.py @@ -240,13 +240,18 @@ def _store_authentication_result(self, access_token, refresh_token): if self.server_instance: self.server_instance.sso_access_token = access_token - if not store_sso_tokens( - self.api_host, access_token, refresh_token, profile=self.profile - ): - click.echo( - "SSO tokens not stored (CLOUDSMITH_NO_KEYRING is set)", - err=True, - ) + from keyring.errors import KeyringError + + try: + if not store_sso_tokens( + self.api_host, access_token, refresh_token, profile=self.profile + ): + click.echo( + "SSO tokens not stored (CLOUDSMITH_NO_KEYRING is set)", + err=True, + ) + except KeyringError as exc: + click.echo(f"SSO tokens not stored: {exc}", err=True) if self.refresh_api_on_success and self.server_instance: self.server_instance.refresh_api_config_after_auth() diff --git a/cloudsmith_cli/core/keyring.py b/cloudsmith_cli/core/keyring.py index 7b247a66..360c344f 100644 --- a/cloudsmith_cli/core/keyring.py +++ b/cloudsmith_cli/core/keyring.py @@ -1,8 +1,11 @@ +import functools import getpass import os import sys from datetime import datetime, timedelta, timezone +from .utils import controlling_terminal_mode, is_interactive + ACCESS_TOKEN_KEY = "cloudsmith_cli-access_token-{api_host}" @@ -95,6 +98,31 @@ def _apply_keyring_file_location(backend): backend.file_path = os.path.join(_expand_path(dir_value), filename) +def _refuse_password_prompt(): + from keyring.errors import KeyringError + + raise KeyringError( + "The keyring backend needs a password, but this session cannot prompt " + "for one. Set CLOUDSMITH_KEYRING_BACKEND to a backend that does not " + "prompt, or to an encrypted backend together with CLOUDSMITH_KEYRING_KEY." + ) + + +def _disable_password_prompts(backend): + """Make encrypted backends raise KeyringError where they would prompt. + + The encrypted file backends call these methods only when they have no + 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()): + return + for member in getattr(backend, "backends", None) or [backend]: + if hasattr(member, "_unlock"): + member._unlock = member._get_new_password = _refuse_password_prompt + + +@functools.cache def _prepare_keyring_backend(): """Resolve env var aliases and apply them to the keyring backend. @@ -102,25 +130,36 @@ def _prepare_keyring_backend(): without calling super(), so KEYRING_PROPERTY_* env vars (e.g. the keyring_key password for those encrypted file backends) never reach them through the library's own documented mechanism. Apply them here - instead, once the backend has been resolved. + instead, once the backend has been resolved. For a chain backend, + apply them to the member that stores values, not to the chain. + + Run once per process, because each key assignment unlocks the file + again, and the unlock is slow. """ import keyring + from keyring.errors import KeyringError _sync_keyring_backend_env() _sync_keyring_property_env() _sync_keyring_file_path_env() - backend = keyring.get_keyring() - _apply_keyring_file_location(backend) - backend.set_properties_from_env() + storage_backend = _effective_backend() + _apply_keyring_file_location(storage_backend) + try: + storage_backend.set_properties_from_env() + except ValueError as exc: + raise KeyringError( + "CLOUDSMITH_KEYRING_KEY does not unlock the keyring file." + ) from exc + _disable_password_prompts(keyring.get_keyring()) def _get_value(key): import keyring from keyring.errors import KeyringError - _prepare_keyring_backend() username = _get_username() try: + _prepare_keyring_backend() return keyring.get_password(key, username) except KeyringError: return None @@ -275,9 +314,9 @@ def _delete_value(key): import keyring from keyring.errors import KeyringError - _prepare_keyring_backend() username = _get_username() try: + _prepare_keyring_backend() keyring.delete_password(key, username) return True except KeyringError: diff --git a/cloudsmith_cli/core/tests/test_keyring.py b/cloudsmith_cli/core/tests/test_keyring.py index f6b60e68..2a53956a 100644 --- a/cloudsmith_cli/core/tests/test_keyring.py +++ b/cloudsmith_cli/core/tests/test_keyring.py @@ -1,24 +1,31 @@ import getpass import importlib import os +from contextlib import contextmanager from datetime import datetime, timedelta, timezone from unittest.mock import ANY, Mock, call, patch import jwt import pytest from freezegun import freeze_time +from keyring.backends.chainer import ChainerBackend +from keyrings.alt.file import EncryptedKeyring as AltEncryptedKeyring from keyrings.cryptfile.cryptfile import CryptFileKeyring from .. import keyring as core_keyring +from ..utils import TTYMode from ..keyring import ( + delete_oidc_token, delete_sso_tokens, get_access_token, + get_oidc_token, get_refresh_attempted_at, get_refresh_token, has_sso_tokens, should_refresh_access_token, should_use_keyring, store_access_token, + store_oidc_token, store_refresh_token, store_sso_tokens, update_refresh_attempted_at, @@ -59,7 +66,9 @@ def mock_delete_password(): def mock_get_keyring(): import keyring + core_keyring._prepare_keyring_backend.cache_clear() with patch.object(keyring, "get_keyring") as get_keyring_mock: + get_keyring_mock.return_value.backends = [] yield get_keyring_mock @@ -957,3 +966,137 @@ def test_has_sso_tokens_returns_false_when_keyring_disabled( with patch.dict(os.environ, {"CLOUDSMITH_NO_KEYRING": "1"}): 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} + + +@contextmanager +def _keyring_session(backend, env, has_terminal=False, typed_password=None): + import keyring + + if typed_password is None: + prompt = {"side_effect": AssertionError("prompted for a keyring password")} + else: + prompt = {"return_value": typed_password} + with ( + patch.dict(os.environ, env, clear=True), + patch.object(keyring, "get_keyring", return_value=backend), + patch.object(keyring.core, "_keyring_backend", backend), + patch.object( + core_keyring, + "controlling_terminal_mode", + return_value=TTYMode.ENABLED if has_terminal else TTYMode.DISABLED, + ), + patch.object(getpass, "getpass", **prompt) as getpass_mock, + ): + yield getpass_mock + if typed_password is None: + getpass_mock.assert_not_called() + + +def _write_oidc_entry(backend, tmp_path): + oidc_key = core_keyring.OIDC_TOKEN_KEY.format( + api_host=OIDC_ARGS[0], org=OIDC_ARGS[1], service_slug=OIDC_ARGS[2] + ) + with patch.dict(os.environ, _keyring_env(tmp_path), clear=True): + backend.keyring_key = "earlier-password" + backend.set_password(oidc_key, "test_user", "token-data") + + +def _chain(*members): + class Chain(ChainerBackend): + backends = list(members) + + return Chain() + + +class TestNonInteractivePasswordPrompt: + """Tests that a run without a user never prompts for a keyring password.""" + + @pytest.mark.parametrize("backend_cls", [CryptFileKeyring, AltEncryptedKeyring]) + def test_store_without_terminal_skips_keyring( + self, tmp_path, mock_get_user, backend_cls + ): + backend = backend_cls() + with _keyring_session(backend, _keyring_env(tmp_path)): + assert store_oidc_token(*OIDC_ARGS, "token-data") is False + assert not os.path.exists(backend.file_path) + + @pytest.mark.parametrize("backend_cls", [CryptFileKeyring, AltEncryptedKeyring]) + 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): + 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): + 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") + with ( + _keyring_session(backend, env), + patch.object(backend, "_unlock", wraps=backend._unlock) as unlock_mock, + ): + store_oidc_token(*OIDC_ARGS, "token-data") + get_oidc_token(*OIDC_ARGS) + get_oidc_token(*OIDC_ARGS) + assert unlock_mock.call_count == 1 + + def test_terminal_without_ci_prompts_as_before(self, tmp_path, mock_get_user): + with _keyring_session( + CryptFileKeyring(), + _keyring_env(tmp_path), + has_terminal=True, + typed_password="typed-password", + ) as getpass_mock: + assert store_oidc_token(*OIDC_ARGS, "token-data") is True + assert getpass_mock.called + + +class TestChainedBackendProperties: + """Tests for keyring settings and password prompts in a chain backend.""" + + 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"), + ) + with _keyring_session(chain, env): + 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() + + def test_lower_member_with_an_existing_file_is_skipped( + self, tmp_path, mock_get_user + ): + _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): + assert get_oidc_token(*OIDC_ARGS) is None + + def test_write_fallback_cannot_prompt_for_a_new_file(self, tmp_path, mock_get_user): + primary = Mock() + primary.set_password.side_effect = NotImplementedError + fallback = CryptFileKeyring() + with _keyring_session(_chain(primary, fallback), _keyring_env(tmp_path)): + assert store_oidc_token(*OIDC_ARGS, "token-data") is False + assert not os.path.exists(fallback.file_path) diff --git a/cloudsmith_cli/core/tests/test_terminal_color.py b/cloudsmith_cli/core/tests/test_terminal_color.py index f1c9889c..efc57038 100644 --- a/cloudsmith_cli/core/tests/test_terminal_color.py +++ b/cloudsmith_cli/core/tests/test_terminal_color.py @@ -1,12 +1,15 @@ from enum import Flag, auto from typing import ClassVar +from unittest.mock import patch import pytest +from cloudsmith_cli.core import utils from cloudsmith_cli.core.utils import ( ColorMode, TTYMode, color_enabled, + controlling_terminal_mode, is_interactive, ) @@ -121,3 +124,25 @@ def test_no_interactive_environment_variables( assert is_interactive(env, ttyMode) == want_interactive, ( f"interactive suppression check failed for environment {env} wanted {desired}" ) + + +class TestControllingTerminalMode: + """Tests for the check that a prompt can reach a user.""" + + def test_disabled_when_dev_tty_cannot_open(self): + with ( + patch.object(utils.sys, "platform", "linux"), + patch.object(utils, "open", side_effect=OSError, create=True), + ): + assert controlling_terminal_mode() is TTYMode.DISABLED + + @pytest.mark.parametrize( + "platform,terminal", [("linux", "/dev/tty"), ("win32", "CONIN$")] + ) + def test_enabled_when_the_terminal_opens(self, platform, terminal): + with ( + patch.object(utils.sys, "platform", platform), + patch.object(utils, "open", create=True) as open_mock, + ): + assert controlling_terminal_mode() is TTYMode.ENABLED + assert open_mock.call_args.args[0] == terminal diff --git a/cloudsmith_cli/core/utils.py b/cloudsmith_cli/core/utils.py index c3fca8c9..e14742ef 100644 --- a/cloudsmith_cli/core/utils.py +++ b/cloudsmith_cli/core/utils.py @@ -2,6 +2,7 @@ import hashlib import os +import sys from enum import Enum, auto import click @@ -61,6 +62,20 @@ def is_interactive(env: dict[str, str], tty_mode: TTYMode) -> bool: return True +def controlling_terminal_mode() -> TTYMode: + """Return whether a prompt can reach a user through the terminal. + + Stdin can be a pipe while a user is at the terminal, so check the + terminal that getpass uses: the console on Windows, else /dev/tty. + """ + terminal = "CONIN$" if sys.platform == "win32" else "/dev/tty" + try: + with open(terminal, "rb"): + return TTYMode.ENABLED + except OSError: + return TTYMode.DISABLED + + def get_github_website(): """Get the URL for the GitHub project.""" return "https://github.com/cloudsmith-io/cloudsmith-cli"