diff --git a/CHANGELOG.md b/CHANGELOG.md index ffbf17a4b..a5dca5300 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -90,6 +90,8 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0. - `cloudsmith credential-helper install` and `uninstall` no longer rewrite a config file when they make no change to its data. Previously, the CLI always rewrote a file in a different formatting, such as the tab-indented `config.json` that Docker writes. The command also made a `.bak` copy and reported changes that it did not make. - When the API host name or the proxy host name does not exist, for example because of a typo in `--api-host`, commands now fail immediately. Previously, they retried for approximately 62 seconds without output. The CLI still retries a DNS failure that the operating system reports as temporary. - When the CLI waits before it retries a failed connection or a 5xx response, it now writes `Request failed: retrying in N second(s) ...` to stderr. Previously, it waited without output. +- `cloudsmith whoami --verbose` now shows a credential from OIDC auto-discovery as `Authentication Method: OIDC Auto-Discovery`, with its source, for example `OIDC via GitHub Actions (org: acme, service: ci-service)`. Since 1.26.0, it showed these credentials as `SAML Token (primary)` with no source. In the JSON output, `auth.active_method` is now `oidc` instead of `sso_token`, and the new `auth.oidc.source` field gives the source. +- The source of a cached OIDC token now names the CI vendor, for example `OIDC via GitHub Actions [cached] (org: acme, service: ci-service)`. The OIDC token cache now stores the name of the vendor. A token that an earlier version cached still shows `OIDC [cached]` until it expires. - `cloudsmith mcp start` now stops without an error when the MCP client closes the connection while the server writes a reply. Previously, the command stopped with `BrokenPipeError` and exit status 1. - The CLI now exits with status 1 when you interrupt it with Ctrl+C after the reader of its stderr has closed the pipe. Before, the CLI sent an error report and exited with status 120. - `cloudsmith check rates` no longer stops with `TypeError` when the API does not give a value for an interval, limit, remaining count, reset time or throttled status. The command now shows `None` for that value. diff --git a/cloudsmith_cli/cli/commands/whoami.py b/cloudsmith_cli/cli/commands/whoami.py index 4c52357b6..0df3bb498 100644 --- a/cloudsmith_cli/cli/commands/whoami.py +++ b/cloudsmith_cli/cli/commands/whoami.py @@ -10,8 +10,11 @@ from .main import main -def _get_active_method(api_config): - """Inspect API config to determine SAML, API key, or no auth.""" +def _get_active_method(api_config, credential=None): + """Inspect the credential and API config to determine OIDC, SAML, API key, or no auth.""" + # An exchanged OIDC token is sent as a bearer token, like a SAML session. + if credential is not None and credential.source_name == "oidc": + return "oidc" headers = getattr(api_config, "headers", {}) or {} if headers.get("Authorization", "").startswith("Bearer "): return "sso_token" @@ -36,6 +39,17 @@ def _get_api_key_source(opts): return {"configured": False, "source": None, "source_key": None} +def _get_oidc_source(opts): + """Return where the OIDC token came from, such as the CI vendor.""" + credential = getattr(opts, "credential", None) + if credential and credential.source_name == "oidc": + return { + "configured": True, + "source": credential.source_detail or credential.source_name, + } + return {"configured": False, "source": None} + + def _get_sso_status(api_host, profile=None): """Return SAML token status from the system keyring.""" enabled = keyring.should_use_keyring() @@ -72,8 +86,11 @@ def _get_verbose_auth_data(opts, api_host, profile=None): api_key_info["created"] = utils.fmt_datetime(created) if created else None return { - "active_method": _get_active_method(opts.api_config), + "active_method": _get_active_method( + opts.api_config, getattr(opts, "credential", None) + ), "api_key": api_key_info, + "oidc": _get_oidc_source(opts), "sso": sso_info, } @@ -111,13 +128,11 @@ def _print_verbose_text(data): if ak.get("source"): click.echo(f" Source: {ak['source']}") click.echo(" Note: SAML token is being used instead") + elif active == "oidc": + click.secho("Authentication Method: OIDC Auto-Discovery", fg="cyan", bold=True) + click.echo(f" Source: {auth['oidc']['source']}") elif active == "api_key": - if ak.get("source_key") == "oidc": - click.secho( - "Authentication Method: OIDC Auto-Discovery", fg="cyan", bold=True - ) - else: - click.secho("Authentication Method: API Key", fg="cyan", bold=True) + click.secho("Authentication Method: API Key", fg="cyan", bold=True) for label, field in [ ("Source", "source"), ("Token Slug", "slug"), diff --git a/cloudsmith_cli/cli/tests/commands/test_whoami.py b/cloudsmith_cli/cli/tests/commands/test_whoami.py index 187432235..4bdad6ee3 100644 --- a/cloudsmith_cli/cli/tests/commands/test_whoami.py +++ b/cloudsmith_cli/cli/tests/commands/test_whoami.py @@ -4,10 +4,12 @@ import click.testing import pytest +from ....core.credentials.models import CredentialResult from ...commands.whoami import whoami HOST = "https://api.example.com" ARGS = ["--api-host", HOST, "--api-key", "fake-api-key"] +OIDC_SOURCE = "OIDC via GitHub Actions (org: acme, service: ci-service)" @pytest.fixture() @@ -24,6 +26,37 @@ def invoke_whoami(runner, user_brief, extra_args=None): return runner.invoke(whoami, ARGS + (extra_args or [])) +def invoke_verbose_whoami(runner, credential, extra_args=None): + """Invoke whoami --verbose with the credential chain resolving to credential.""" + with ( + patch( + "cloudsmith_cli.cli.commands.whoami.get_user_brief", + return_value=(True, "ci-service", None, "CI service"), + ), + patch( + "cloudsmith_cli.core.credentials.chain.CredentialProviderChain.resolve", + return_value=credential, + ), + patch( + "cloudsmith_cli.cli.commands.whoami.keyring.has_sso_tokens", + return_value=False, + ), + ): + return runner.invoke( + whoami, ["--api-host", HOST, "--verbose", *(extra_args or [])] + ) + + +def oidc_credential(): + """Return a credential as the OIDC provider resolves it after an exchange.""" + return CredentialResult( + api_key="exchanged-token", + source_name="oidc", + source_detail=OIDC_SOURCE, + auth_type="bearer", + ) + + class TestWhoamiCommand: """Tests for the cloudsmith whoami command.""" @@ -71,3 +104,47 @@ def test_json_unauthenticated_exits_one(self, runner): "".join(line for line in result.output.splitlines() if line.startswith("{")) ) assert payload["data"]["is_authenticated"] is False + + +class TestWhoamiVerboseAuthMethod: + """whoami --verbose names the method and the source of the credential in use.""" + + def test_oidc_names_the_oidc_source(self, runner): + """An OIDC token is sent as a bearer token, but it is not a SAML session.""" + result = invoke_verbose_whoami(runner, oidc_credential()) + + assert result.exit_code == 0, result.output + assert "Authentication Method: OIDC Auto-Discovery" in result.output + assert f" Source: {OIDC_SOURCE}" in result.output + assert "SAML Token" not in result.output + + def test_json_oidc_names_the_oidc_source(self, runner): + """The JSON output gives the method as oidc, with the source of the token.""" + result = invoke_verbose_whoami( + runner, oidc_credential(), extra_args=["--output-format", "json"] + ) + + assert result.exit_code == 0, result.output + payload = json.loads( + "".join(line for line in result.output.splitlines() if line.startswith("{")) + ) + assert payload["data"]["auth"]["active_method"] == "oidc" + assert payload["data"]["auth"]["oidc"] == { + "configured": True, + "source": OIDC_SOURCE, + } + + def test_saml_session_is_still_a_saml_token(self, runner): + """A bearer token from the keyring is a SAML session.""" + saml = CredentialResult( + api_key="access-token", + source_name="keyring", + source_detail="System Keyring", + auth_type="bearer", + ) + + result = invoke_verbose_whoami(runner, saml) + + assert result.exit_code == 0, result.output + assert "Authentication Method: SAML Token (primary)" in result.output + assert "OIDC" not in result.output diff --git a/cloudsmith_cli/core/credentials/oidc/cache.py b/cloudsmith_cli/core/credentials/oidc/cache.py index ab5a4c401..ed59e6e5a 100644 --- a/cloudsmith_cli/core/credentials/oidc/cache.py +++ b/cloudsmith_cli/core/credentials/oidc/cache.py @@ -12,6 +12,7 @@ import logging import os import time +from dataclasses import dataclass from ...cache_utils import atomic_write_json @@ -22,6 +23,17 @@ _CACHE_DIR_NAME = "oidc_token_cache" +@dataclass(frozen=True) +class CachedToken: + """A cached exchanged token, and the CI vendor whose OIDC token it came from. + + ``detector`` is ``None`` for an entry that an older CLI wrote. + """ + + token: str + detector: str | None = None + + def _get_cache_dir() -> str: """Return the cache directory path without creating it.""" from ....cli.config import get_default_config_path @@ -56,7 +68,7 @@ def _decode_jwt_exp(token: str) -> float | None: return None -def get_cached_token(api_host: str, org: str, service_slug: str) -> str | None: +def get_cached_token(api_host: str, org: str, service_slug: str) -> CachedToken | None: """Return a cached token if it exists and is not expired.""" token = _get_from_keyring(api_host, org, service_slug) if token: @@ -64,7 +76,7 @@ def get_cached_token(api_host: str, org: str, service_slug: str) -> str | None: return _get_from_disk(api_host, org, service_slug) -def _get_from_keyring(api_host: str, org: str, service_slug: str) -> str | None: +def _get_from_keyring(api_host: str, org: str, service_slug: str) -> CachedToken | None: """Try to get token from keyring.""" try: from ...keyring import get_oidc_token @@ -97,14 +109,14 @@ def _get_from_keyring(api_host: str, org: str, service_slug: str) -> str | None: else: logger.debug("Using keyring OIDC token (no expiry information)") - return token + return CachedToken(token, data.get("detector")) except Exception: # pylint: disable=broad-exception-caught logger.debug("Failed to read OIDC token from keyring", exc_info=True) return None -def _get_from_disk(api_host: str, org: str, service_slug: str) -> str | None: +def _get_from_disk(api_host: str, org: str, service_slug: str) -> CachedToken | None: """Try to get token from disk cache.""" cache_dir = _get_cache_dir() cache_file = os.path.join(cache_dir, _cache_key(api_host, org, service_slug)) @@ -139,7 +151,7 @@ def _get_from_disk(api_host: str, org: str, service_slug: str) -> str | None: else: logger.debug("Using disk cached OIDC token (no expiry information)") - return token + return CachedToken(token, data.get("detector")) except (ValueError, TypeError, OSError): logger.debug("Failed to read OIDC token from disk cache", exc_info=True) @@ -147,8 +159,17 @@ def _get_from_disk(api_host: str, org: str, service_slug: str) -> str | None: return None -def store_cached_token(api_host: str, org: str, service_slug: str, token: str) -> None: - """Cache a token in keyring (if available) or filesystem.""" +def store_cached_token( + api_host: str, + org: str, + service_slug: str, + token: str, + detector: str | None = None, +) -> None: + """Cache a token in keyring (if available) or filesystem. + + *detector* is the name of the CI vendor whose OIDC token was exchanged. + """ expires_at = _decode_jwt_exp(token) data = { @@ -157,6 +178,7 @@ def store_cached_token(api_host: str, org: str, service_slug: str, token: str) - "api_host": api_host, "org": org, "service_slug": service_slug, + "detector": detector, "cached_at": time.time(), } diff --git a/cloudsmith_cli/core/credentials/providers/oidc_provider.py b/cloudsmith_cli/core/credentials/providers/oidc_provider.py index e8373169d..29d70fb67 100644 --- a/cloudsmith_cli/core/credentials/providers/oidc_provider.py +++ b/cloudsmith_cli/core/credentials/providers/oidc_provider.py @@ -107,10 +107,13 @@ def resolve( # pylint: disable=too-many-return-statements cached = get_cached_token(context.api_host, org, service_slug) if cached: logger.debug("OidcProvider: Using cached OIDC token") + vendor = f" via {cached.detector}" if cached.detector else "" return CredentialResult( - api_key=cached, + api_key=cached.token, source_name="oidc", - source_detail=f"OIDC [cached] (org: {org}, service: {service_slug})", + source_detail=( + f"OIDC{vendor} [cached] (org: {org}, service: {service_slug})" + ), auth_type="bearer", ) @@ -184,7 +187,13 @@ def resolve( # pylint: disable=too-many-return-statements if not cloudsmith_token: return None - store_cached_token(context.api_host, org, service_slug, cloudsmith_token) + store_cached_token( + context.api_host, + org, + service_slug, + cloudsmith_token, + detector=detector.name, + ) return CredentialResult( api_key=cloudsmith_token, diff --git a/cloudsmith_cli/core/tests/test_oidc_cache.py b/cloudsmith_cli/core/tests/test_oidc_cache.py new file mode 100644 index 000000000..306be9171 --- /dev/null +++ b/cloudsmith_cli/core/tests/test_oidc_cache.py @@ -0,0 +1,65 @@ +"""Tests for the OIDC token cache.""" + +import json +import time +from unittest.mock import patch + +import jwt +import pytest + +from cloudsmith_cli.core.credentials.oidc import cache + +API_HOST = "https://api.cloudsmith.io" + + +def _token() -> str: + return jwt.encode({"exp": time.time() + 3600}, key="", algorithm="none") + + +@pytest.fixture() +def disk_only(tmp_path): + """Use a cache directory in tmp_path, with no keyring.""" + with ( + patch( + "cloudsmith_cli.cli.config.get_default_config_path", + return_value=str(tmp_path), + ), + patch.object(cache, "_store_in_keyring", return_value=False), + patch.object(cache, "_get_from_keyring", return_value=None), + ): + yield tmp_path + + +def test_disk_cache_keeps_the_vendor(disk_only): + """A token cached on disk comes back with the name of the CI vendor.""" + token = _token() + cache.store_cached_token( + API_HOST, "acme", "ci-service", token, detector="GitHub Actions" + ) + + assert cache.get_cached_token(API_HOST, "acme", "ci-service") == ( + cache.CachedToken(token, detector="GitHub Actions") + ) + + +def test_disk_cache_entry_without_a_vendor(disk_only): + """An entry that an older CLI wrote has no vendor name.""" + token = _token() + cache.store_cached_token(API_HOST, "acme", "ci-service", token) + + assert cache.get_cached_token(API_HOST, "acme", "ci-service") == ( + cache.CachedToken(token) + ) + + +def test_keyring_cache_keeps_the_vendor(): + """A token cached in the keyring comes back with the name of the CI vendor.""" + token = _token() + entry = json.dumps( + {"token": token, "expires_at": time.time() + 3600, "detector": "CircleCI"} + ) + + with patch("cloudsmith_cli.core.keyring.get_oidc_token", return_value=entry): + assert cache.get_cached_token(API_HOST, "acme", "ci-service") == ( + cache.CachedToken(token, detector="CircleCI") + ) diff --git a/cloudsmith_cli/core/tests/test_oidc_provider.py b/cloudsmith_cli/core/tests/test_oidc_provider.py index a44959054..8f3b84438 100644 --- a/cloudsmith_cli/core/tests/test_oidc_provider.py +++ b/cloudsmith_cli/core/tests/test_oidc_provider.py @@ -8,7 +8,7 @@ import pytest from cloudsmith_cli.core.credentials.models import CredentialContext, CredentialResult -from cloudsmith_cli.core.credentials.oidc.cache import _cache_key +from cloudsmith_cli.core.credentials.oidc.cache import CachedToken, _cache_key from cloudsmith_cli.core.credentials.oidc.exchange import OidcExchangeError from cloudsmith_cli.core.credentials.providers.oidc_provider import OidcProvider @@ -24,7 +24,7 @@ def test_cached_oidc_token_is_a_bearer_credential(): """Cached exchanged tokens retain their HTTP authorization scheme.""" with patch( "cloudsmith_cli.core.credentials.oidc.cache.get_cached_token", - return_value="cached-token", + return_value=CachedToken("cached-token"), ): credential = OidcProvider().resolve(_context()) @@ -32,6 +32,67 @@ def test_cached_oidc_token_is_a_bearer_credential(): assert credential.auth_type == "bearer" +def test_cached_oidc_token_names_the_vendor(): + """The source of a cached token names the CI vendor whose token was exchanged.""" + with patch( + "cloudsmith_cli.core.credentials.oidc.cache.get_cached_token", + return_value=CachedToken("cached-token", detector="GitHub Actions"), + ): + credential = OidcProvider().resolve(_context()) + + assert credential.api_key == "cached-token" + assert credential.source_detail == ( + "OIDC via GitHub Actions [cached] (org: cloudsmith, service: github-actions)" + ) + + +def test_cached_oidc_token_without_a_vendor_keeps_the_old_source(): + """A token that an older CLI cached has no vendor name.""" + with patch( + "cloudsmith_cli.core.credentials.oidc.cache.get_cached_token", + return_value=CachedToken("cached-token"), + ): + credential = OidcProvider().resolve(_context()) + + assert credential.source_detail == ( + "OIDC [cached] (org: cloudsmith, service: github-actions)" + ) + + +def test_exchanged_oidc_token_is_cached_with_the_vendor(): + """The cache keeps the vendor name, so a cached token can name it later.""" + detector = Mock() + detector.name = "GitHub Actions" + detector.get_token.return_value = "vendor-token" + + with ( + patch( + "cloudsmith_cli.core.credentials.oidc.cache.get_cached_token", + return_value=None, + ), + patch( + "cloudsmith_cli.core.credentials.oidc.cache.store_cached_token", + ) as store, + patch( + "cloudsmith_cli.core.credentials.oidc.detectors.detect_environment", + return_value=detector, + ), + patch( + "cloudsmith_cli.core.credentials.oidc.exchange.exchange_oidc_token", + return_value="exchanged-token", + ), + ): + OidcProvider().resolve(_context()) + + store.assert_called_once_with( + "https://api.cloudsmith.io", + "cloudsmith", + "github-actions", + "exchanged-token", + detector="GitHub Actions", + ) + + def test_exchanged_oidc_token_is_a_bearer_credential(): """Freshly exchanged tokens use Bearer authentication.""" detector = Mock(name="github-actions")