diff --git a/CHANGELOG.md b/CHANGELOG.md index 8b320cd..c9889bc 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -14,6 +14,10 @@ and versions are tracked in the repo-root `VERSION` file. ### Changed - Include `uv.lock` freshness in the authoritative local validation aggregate (#424). +- Clarify that extension capabilities are informational metadata, and require + fully qualified or distribution-prefixed extension allowlist entries (#397). +- Match distribution-prefixed extension allowlists against the exact installed + metadata spelling and precompute the approved distribution set. - Classify the compatibility facade in a reviewed API manifest and add the smaller `base_cli.core` starting surface for new applications (#242). diff --git a/docs/extensions.md b/docs/extensions.md index a58d8fb..e497827 100644 --- a/docs/extensions.md +++ b/docs/extensions.md @@ -50,10 +50,15 @@ telemetry = "acme_cli.telemetry:install [base-cli-api-v1, base-cli-cap-tracing]" `ExtensionDescriptor.api_version` and `.capabilities` expose this metadata. `ExtensionDiscovery` rejects an unsupported API version before loading the -entry point and reports the supported versions in the error. This is a -negotiation boundary, not an installation-order heuristic; the consumer can -select another plugin release or widen its `supported_api_versions` policy -after running its compatibility suite. +entry point and reports the supported versions in the error. Capabilities are +currently **informational inventory metadata**: they do not grant or restrict +permissions and do not sandbox an in-process Python extension. A consumer that +needs a capability policy must inspect the descriptor and make that decision +before calling `load()`. + +This is a negotiation boundary, not an installation-order heuristic; the +consumer can select another plugin release or widen its +`supported_api_versions` policy after running its compatibility suite. An entry point must declare at most one `base-cli-api-vN` extra. Discovery skips an entry point with malformed metadata on a per-entry-point basis, so a @@ -72,10 +77,14 @@ runtime environment change. `load_all()` isolates broken third-party imports and returns an `ExtensionLoadResult` for every descriptor so one broken plugin does not hide healthy extensions. -Use `allowlist={"base_cli.commands:audit"}` to restrict names, or -`ExtensionDiscovery(disabled=True)` to disable discovery entirely. Allowlist -entries may be a bare entry-point name, a fully-qualified `group:name`, or a -distribution name. +Use `allowlist={"base_cli.commands:audit"}` to restrict one entry point, or +`allowlist={"dist:acme-cli"}` to trust all entry points from one distribution. +Distribution allowlist values use the exact installed metadata spelling, +including case; base-cli does not silently normalize case, hyphens, or +underscores for a security decision. +`ExtensionDiscovery(disabled=True)` disables discovery entirely. Bare entry +point names and unqualified distribution names are rejected because they can +silently trust an unintended group or distribution. Loading uses the exact entry-point source and distribution identity approved by discovery rather than re-selecting an entry point by a potentially colliding diff --git a/docs/security-threat-model.md b/docs/security-threat-model.md index b7c46e6..d013acf 100644 --- a/docs/security-threat-model.md +++ b/docs/security-threat-model.md @@ -74,7 +74,7 @@ not a replacement for explicit `sensitive=True` declarations. | Symlink, traversal, replacement, or mount races redirect cleanup | Exclusive runtime-leaf ownership, retained descriptors, identity checks, no-follow traversal, run-ID containment, and fail-closed cleanup; `tests/test_cleanup_security.py`, `tests/test_app_security_boundaries.py`, and adversarial regression tests | A same-account process with the same filesystem authority can race user-owned paths. Use a private cache root and avoid sharing runtime trees between mutually hostile users. | | Insecure permissions expose runtime files | POSIX `0600`/`0700` modes; Windows uses inherited user-profile ACLs and warns when secure handle operations are unavailable | A custom Windows cache root or network filesystem may not inherit private ACLs. Consumers must provision and verify permissions. | | Malicious or accidental config content changes execution | Explicit config paths must be readable regular files; YAML is parsed as data; generic profile has no implicit product files; consumer profiles own schema validation and precedence | The consumer still decides which files, environment variables, and values are trusted. Validate schema, reject unexpected keys, and do not treat config as a secret store. | -| A plugin executes unwanted code or changes command behavior | Lazy discovery, deterministic ordering, duplicate detection, explicit `load`/`load_all`, `allowlist`, and `disabled`; `tests/test_extensions.py` | Loaded extensions have the process's authority and are not sandboxed. Pin and review distributions, disable discovery by default where possible, and use an allowlist. | +| A plugin executes unwanted code or changes command behavior | Lazy discovery, deterministic ordering, duplicate detection, explicit `load`/`load_all`, canonical `group:name` or `dist:distribution` allowlists, and `disabled`; `tests/test_extensions.py` | Loaded extensions have the process's authority and are not sandboxed. Capability extras are informational metadata, not authorization. Pin and review distributions, disable discovery by default where possible, and use an allowlist. | | Concurrency corrupts logs, history, metadata, or retention state | Atomic replacement, sidecar/process locks, per-run leases, bounded complete-bundle retention, and concurrency/adversarial tests | The framework cannot make consumer databases or external stores transactional. Use a transactional writer and define recovery for multi-process consumers. Unknown run liveness fails closed. | | Child/inherited runs mutate or delete a parent's artifacts | Inherited runtimes do not own a new bundle, do not finalize parent metadata, and are excluded from cleanup/retention; inherited-run tests cover startup, success, and failure | Parent and child processes still share the user's filesystem authority. Consumers must validate parent provenance and avoid inheriting from untrusted paths. | | Telemetry leaks argv, config, paths, or secrets | Telemetry is opt-in; only run ID, CLI name, environment, dry-run, outcome, exit code, and duration are attached; missing/broken exporters are no-ops; `tests/test_integrations.py` | A consumer-supplied tracer/exporter can add arbitrary attributes or send to an untrusted endpoint. Review provider configuration, use TLS/authentication, and never attach raw command data. | diff --git a/lib/python/base_cli/extensions.py b/lib/python/base_cli/extensions.py index f3599c7..8913957 100644 --- a/lib/python/base_cli/extensions.py +++ b/lib/python/base_cli/extensions.py @@ -98,7 +98,12 @@ def __init__(self, descriptor: ExtensionDescriptor, cause: BaseException) -> Non @dataclass(frozen=True) class ExtensionDescriptor: - """Stable metadata for one entry point, before its object is loaded.""" + """Stable metadata for one entry point, before its object is loaded. + + ``capabilities`` is inventory metadata only. It lets a consumer inspect + what an extension claims before loading it; it does not grant permission, + restrict the in-process Python object, or provide a sandbox. + """ group: str name: str @@ -175,7 +180,12 @@ def __init__( if entry_points is not None and paths is not None: raise ValueError("pass either entry_points or paths, not both") self.disabled = disabled - self.allowlist = frozenset(allowlist) if allowlist is not None else None + self.allowlist = _normalise_allowlist(allowlist) + self._distribution_allowlist = ( + frozenset(value.removeprefix("dist:") for value in self.allowlist if value.startswith("dist:")) + if self.allowlist is not None + else None + ) self._entry_points = entry_points self._paths = tuple(Path(path) for path in paths) if paths is not None else None self.supported_api_versions = frozenset(str(version) for version in supported_api_versions) @@ -315,11 +325,11 @@ def _raw_entry_points(self) -> tuple[Any, ...]: def _allowed(self, descriptor: ExtensionDescriptor) -> bool: if self.allowlist is None: return True - return bool( - descriptor.name in self.allowlist - or descriptor.key in self.allowlist - or (descriptor.distribution is not None and descriptor.distribution in self.allowlist) - ) + if descriptor.key in self.allowlist: + return True + if descriptor.distribution is None: + return False + return self._distribution_allowlist is not None and descriptor.distribution in self._distribution_allowlist def _load_descriptor(self, descriptor: ExtensionDescriptor) -> Any: entry_point = self._entry_point_cache.get(descriptor) @@ -340,6 +350,38 @@ def _validate_group(group: str) -> None: raise ValueError(f"Unsupported extension group '{group}'. Expected one of: {expected}.") +def _normalise_allowlist(values: Iterable[str] | None) -> frozenset[str] | None: + """Validate canonical allowlist entries before discovery begins. + + A fully-qualified ``group:name`` entry selects one entry point. A + ``dist:name`` entry selects all entry points from one distribution. The + distribution spelling is matched exactly, including case; callers should + copy the ``Name`` value from installed metadata rather than relying on + implicit case or punctuation normalization. Bare names are rejected + because they can accidentally trust a same-named entry point from another + group or distribution. + """ + + if values is None: + return None + normalised: set[str] = set() + for value in values: + if not isinstance(value, str) or not value.strip(): + raise ValueError("allowlist entries must be non-empty strings") + value = value.strip() + if value.startswith("dist:"): + if not value.removeprefix("dist:"): + raise ValueError("distribution allowlist entries must use dist:") + normalised.add(value) + continue + group, separator, name = value.partition(":") + if not separator or group not in ENTRY_POINT_GROUPS or not name: + expected = "group:name or dist:distribution" + raise ValueError(f"allowlist entry {value!r} must use {expected}; bare names are not allowed") + normalised.add(f"{group}:{name}") + return frozenset(normalised) + + def _normalise_entry_points(values: Any) -> tuple[Any, ...]: if isinstance(values, Mapping): return tuple(entry_point for group in values.values() for entry_point in group) diff --git a/tests/test_extensions.py b/tests/test_extensions.py index 7fa2b00..29e13a6 100644 --- a/tests/test_extensions.py +++ b/tests/test_extensions.py @@ -85,7 +85,7 @@ def test_allowlist_loads_the_exact_approved_source_across_collisions_and_refresh denied.load = mock.Mock(return_value="denied") trusted.load = mock.Mock(return_value="trusted") entries = (trusted, denied) if trusted_first else (denied, trusted) - discovery = base_cli.ExtensionDiscovery(entry_points=entries, allowlist={"trusted"}) + discovery = base_cli.ExtensionDiscovery(entry_points=entries, allowlist={"dist:trusted"}) descriptors = discovery.list_commands() self.assertEqual(len(descriptors), 1) @@ -104,7 +104,7 @@ def test_allowlist_loads_the_exact_approved_source_across_collisions_and_refresh def test_allowlisted_entry_point_identity_change_fails_closed(self) -> None: trusted = _entry_point("shared", "same.module:plugin", distribution="trusted") trusted.load = mock.Mock(return_value="trusted") - discovery = base_cli.ExtensionDiscovery(entry_points=(trusted,), allowlist={"trusted"}) + discovery = base_cli.ExtensionDiscovery(entry_points=(trusted,), allowlist={"dist:trusted"}) self.assertEqual(len(discovery.list_commands()), 1) trusted.dist.name = "denied" @@ -112,6 +112,16 @@ def test_allowlisted_entry_point_identity_change_fails_closed(self) -> None: discovery.load(base_cli.COMMAND_ENTRY_POINT_GROUP, "shared") trusted.load.assert_not_called() + def test_distribution_allowlist_requires_exact_metadata_spelling(self) -> None: + legitimate = _entry_point("shared", "same.module:plugin", distribution="trusted-pkg") + case_variant = _entry_point("case", "same.module:plugin", distribution="TRUSTED-PKG") + discovery = base_cli.ExtensionDiscovery( + entry_points=(legitimate, case_variant), + allowlist={"dist:trusted-pkg"}, + ) + + self.assertEqual([item.name for item in discovery.list_commands()], ["shared"]) + def test_refresh_cannot_race_an_inflight_descriptor_load(self) -> None: entry_point = _entry_point("shared", "same.module:plugin", distribution="trusted") entered = threading.Event() @@ -123,7 +133,7 @@ def load() -> str: return "trusted" entry_point.load = load - discovery = base_cli.ExtensionDiscovery(entry_points=(entry_point,), allowlist={"trusted"}) + discovery = base_cli.ExtensionDiscovery(entry_points=(entry_point,), allowlist={"dist:trusted"}) result: list[object] = [] def worker() -> None: @@ -210,6 +220,10 @@ def test_api_version_and_capabilities_are_negotiated_from_entry_point_extras(sel self.assertEqual(descriptor.api_version, "1") self.assertEqual(descriptor.capabilities, ("metrics", "tracing")) + compatible.load = mock.Mock(return_value="loaded") + self.assertEqual(discovery.load(base_cli.PLUGIN_ENTRY_POINT_GROUP, "telemetry"), "loaded") + compatible.load.assert_called_once_with() + incompatible = _entry_point( "future", "two:install", @@ -221,6 +235,13 @@ def test_api_version_and_capabilities_are_negotiated_from_entry_point_extras(sel with self.assertRaisesRegex(base_cli.ExtensionCompatibilityError, "API version '2'"): future.load(base_cli.PLUGIN_ENTRY_POINT_GROUP, "future") + def test_allowlist_rejects_ambiguous_bare_names(self) -> None: + with self.assertRaisesRegex(ValueError, "bare names are not allowed"): + base_cli.ExtensionDiscovery(allowlist={"audit"}) + + with self.assertRaisesRegex(ValueError, "group:name or dist:distribution"): + base_cli.ExtensionDiscovery(allowlist={"unknown:audit"}) + def test_real_distribution_metadata_is_discovered_from_a_path(self) -> None: with tempfile.TemporaryDirectory() as tmpdir: root = Path(tmpdir)