diff --git a/src/ctrlrun/decision.py b/src/ctrlrun/decision.py new file mode 100644 index 00000000..8f33fcf9 --- /dev/null +++ b/src/ctrlrun/decision.py @@ -0,0 +1,59 @@ +# SPDX-FileCopyrightText: 2026 The CTRLRun contributors +# SPDX-License-Identifier: Apache-2.0 +"""The decision vocabulary, below everything that produces or records one. + +**Why this module exists rather than these two names living in `policy.py`.** They did, and it +put a cycle in the module map: `state.py` imports `receipt.py`, `receipt.py` imported +`policy.py` for exactly these two names, `policy.py` reaches `authority.py` from inside two +functions, and `authority.py` imports `state.py`. A v0.7 review found it and +`docs/ARCHITECTURE.md` §6 has recorded it since 2026-09-12, as a named item before v1.0 rather +than something to change in a release pass. + +Nothing was ever broken at run time, which is why it survived five milestones: the two edges out +of `policy.py` are function-level and run after every module is loaded, so `import ctrlrun` +resolves in one order and the suite passes. What it cost was §6's own rule, **dependencies point +downward only** -- with a cycle in place that sentence describes import order rather than the +module map, and the map is what tells a contributor what a module may know about. + +**Why this edge and not one of the other three.** `receipt.py` is the module everything else +records through; the table in §6 lists it as used by *everything else*. An evidence type reaching +**up** into the decider is the edge that most contradicts the map, and what it reached up for was +pure vocabulary: a three-member `StrEnum` and a reason string, no behaviour either way. The other +candidate was moving `policy.py`'s two deferred imports, but `_canonical_authority` and +`hash_with_authority` genuinely need authority's canonicalization, and relocating them decides +who owns the policy hash, which is a real behavioural question rather than a placement one. + +**No public name moves.** `policy.py` re-exports both, so `from ctrlrun.policy import Decision` +still resolves, `SPEC-v0.1.md` §8's frozen `__init__` block is literally unchanged, and +`from ctrlrun import Decision` is the same object it always was. The cycle was the only thing +that changed shape. + +This module imports nothing from the package, and `test_the_module_graph_has_no_cycle` fails if +it ever does. +""" + +from __future__ import annotations + +from enum import StrEnum +from typing import Final + + +class Decision(StrEnum): + """What may happen to an action: exactly three outcomes in v0.1 (SPEC-v0.1 §3.3). + + `StrEnum`, so a member renders as its value in receipts and CLI output (SPEC-v0.1 §6.1). + """ + + ALLOW = "allow" + APPROVE = "approve" + DENY = "deny" + + +#: SPEC-v0.8 §8.4 — the refusal a deployment gets under a policy nobody approved. Its own +#: reason, never folded into `unknown_action` or a generic denial: "this policy was never +#: approved" and "this policy denies this action" are different facts and an operator acts on +#: them differently. +#: +#: It sits beside `Decision` rather than in `policy.py` because `receipt.py` buckets it with the +#: other refusal reasons and was the second half of the import that made the cycle. +POLICY_UNAPPROVED: Final = "policy_unapproved" diff --git a/src/ctrlrun/jwt_identity.py b/src/ctrlrun/jwt_identity.py index 56ba5c40..0619cd10 100644 --- a/src/ctrlrun/jwt_identity.py +++ b/src/ctrlrun/jwt_identity.py @@ -50,6 +50,11 @@ from .identity import IdentityContext from .revocation import FEED_STALE, RevocationFeed +# `_NoRedirects` is re-exported, not merely used: it was defined here until v0.12, and both +# `tests/test_revocation_feed.py` and anything else reaching for `jwt_identity._NoRedirects` +# still resolve. It moved down to break the layering cycle §6 forbids; it did not change. +from .revocation import _NoRedirects as _NoRedirects + _LOG = logging.getLogger("ctrlrun") #: The module this provider needs, and the extra that carries it. @@ -486,30 +491,6 @@ def _fetch(self) -> Mapping[str, Any]: return document -class _NoRedirects(urllib.request.HTTPRedirectHandler): - """A redirect handler that redirects nowhere (SPEC-v0.3 §3.4). - - `urllib.request.build_opener` does **not** drop `HTTPRedirectHandler` when it is handed an - `HTTPSHandler` — the default classes it removes are only the ones an argument is an - instance or subclass of, and the two are unrelated. An opener built that way still follows - a 302, and `HTTPRedirectHandler` permits `http`, `https` and `ftp` targets: an open - redirect on the issuer's domain would make this process fetch its signing keys, in - cleartext, from wherever the redirect pointed. Those keys are cached for the life of the - process, so every token the attacker then signs verifies, with an arbitrary `agent` and - `user`. That is the whole authority model, bypassed at the one input that decides who - everybody is. - - Subclassing and refusing is the reliable way to say "no redirects": passing an instance of - a subclass *does* displace the default, which passing an unrelated handler does not. - """ - - def redirect_request( - self, req: Any, fp: Any, code: int, msg: str, headers: Any, newurl: str - ) -> None: - _LOG.warning("the JWK Set at %s redirected to %s; refusing to follow", req.full_url, newurl) - return None - - class _Key: """One usable verification key, and the algorithm it constrains itself to, if any.""" diff --git a/src/ctrlrun/policy.py b/src/ctrlrun/policy.py index 81137823..5d495cc9 100644 --- a/src/ctrlrun/policy.py +++ b/src/ctrlrun/policy.py @@ -21,7 +21,6 @@ from collections.abc import Callable, Iterable, Mapping from dataclasses import dataclass, field from datetime import date, datetime, time -from enum import StrEnum from functools import cached_property, partial from pathlib import Path from types import MappingProxyType @@ -30,6 +29,13 @@ import yaml from .action import Action, PlainValue, canonical_bytes + +# Re-exported, not merely used. `SPEC-v0.1.md` §8 freezes `from .policy import Decision, Policy` +# in `__init__.py`, and `adapter.py` imports `Decision` from here too. Both names moved down to +# break the module cycle §6 records, and both still resolve from this module because that block +# is a frozen public surface and a cycle is not a reason to move a published import path. +from .decision import POLICY_UNAPPROVED as POLICY_UNAPPROVED +from .decision import Decision as Decision from .effect import template_placeholders from .errors import InvalidArgument, PolicyError @@ -185,12 +191,6 @@ def _at_least(schema: str, minimum: str) -> bool: #: rules were mutually exclusive. POLICY_CHANGE_ACTION: Final = "ctrlrun.policy.change" -#: SPEC-v0.8 §8.4 — the refusal a deployment gets under a policy nobody approved. Its own -#: reason, never folded into `unknown_action` or a generic denial: "this policy was never -#: approved" and "this policy denies this action" are different facts and an operator acts on -#: them differently. -POLICY_UNAPPROVED: Final = "policy_unapproved" - #: SPEC-v0.10 §4.5 — the two upstream refusals, separately observable because "the server #: changed" and "nobody has checked" are different findings an operator fixes differently. #: `UPSTREAM_UNVERIFIED` is the fail-closed half and the one to get right: a pin that does @@ -334,17 +334,6 @@ def _refuse_reserved(names: Iterable[str], where: str, what: str) -> None: } -class Decision(StrEnum): - """What may happen to an action: exactly three outcomes in v0.1 (SPEC-v0.1 §3.3). - - `StrEnum`, so a member renders as its value in receipts and CLI output (SPEC-v0.1 §6.1). - """ - - ALLOW = "allow" - APPROVE = "approve" - DENY = "deny" - - @dataclass(frozen=True) class Evaluation: """A decision and the reason it was reached, e.g. `rule[1]` or `unknown_action`.""" diff --git a/src/ctrlrun/receipt.py b/src/ctrlrun/receipt.py index d447dee3..376a3c82 100644 --- a/src/ctrlrun/receipt.py +++ b/src/ctrlrun/receipt.py @@ -35,8 +35,12 @@ APPROVER_UNENTITLED, VerifiedApprover, ) + +# `decision.py` imports nothing from the package, so this is downward and the cycle +# `state -> receipt -> policy -> authority -> state` that ARCHITECTURE §6 recorded is gone. +# These two names were the whole of what a receipt needed from the decider. +from .decision import POLICY_UNAPPROVED, Decision from .errors import CTRLRunError, InvalidArgument -from .policy import POLICY_UNAPPROVED, Decision #: SPEC-v0.3 §12.2. The bump landed with build-list item 1, because that is when the first v2 #: field appeared — the principal's claims, issuer and expiry. `execution` and `would_have` diff --git a/src/ctrlrun/revocation.py b/src/ctrlrun/revocation.py index 4cf35801..31c5ac36 100644 --- a/src/ctrlrun/revocation.py +++ b/src/ctrlrun/revocation.py @@ -36,6 +36,45 @@ _LOG = logging.getLogger("ctrlrun.revocation") +#: `_NoRedirects` logs here and not to this module's `_LOG`. It moved down from `jwt_identity.py` +#: to break the layering cycle `jwt_identity <-> revocation`, and it warned on the `ctrlrun` +#: logger from both callers before the move, because `revocation.py` was importing the class from +#: there. Keeping that name keeps every existing handler and filter pointed at the same place: a +#: refactor that silently re-routes a security warning is a refactor that loses it. +_REDIRECT_LOG = logging.getLogger("ctrlrun") + + +class _NoRedirects(urllib.request.HTTPRedirectHandler): + """A redirect handler that redirects nowhere (SPEC-v0.3 §3.4). + + **Defined here, below both callers, and used by `jwt_identity.py` too.** It lived in + `jwt_identity.py` and `revocation.py` imported it from inside `_opener` -- deliberately, to + avoid a second copy, and that deferred import was a layering cycle `ARCHITECTURE.md` §6 + forbids. One copy was always right; the direction was wrong. `jwt_identity.py` already + imports this module at module level, so defining it here needs no new module and no new edge. + + `urllib.request.build_opener` does **not** drop `HTTPRedirectHandler` when it is handed an + `HTTPSHandler` — the default classes it removes are only the ones an argument is an + instance or subclass of, and the two are unrelated. An opener built that way still follows + a 302, and `HTTPRedirectHandler` permits `http`, `https` and `ftp` targets: an open + redirect on the issuer's domain would make this process fetch its signing keys, in + cleartext, from wherever the redirect pointed. Those keys are cached for the life of the + process, so every token the attacker then signs verifies, with an arbitrary `agent` and + `user`. That is the whole authority model, bypassed at the one input that decides who + everybody is. The revocation feed is the same argument one step along: a redirect there + decides which revocations this process never hears about. + + Subclassing and refusing is the reliable way to say "no redirects": passing an instance of + a subclass *does* displace the default, which passing an unrelated handler does not. + """ + + def redirect_request( + self, req: Any, fp: Any, code: int, msg: str, headers: Any, newurl: str + ) -> None: + _REDIRECT_LOG.warning("%s redirected to %s; refusing to follow", req.full_url, newurl) + return None + + def _utc_now() -> datetime: return datetime.now(UTC) @@ -348,11 +387,10 @@ def refresh(self) -> None: self._read_at = now def _opener(self) -> Any: - """HTTPS, and follows nothing. `jwt_identity._NoRedirects`, reused deliberately: two - copies of this handler would be two things to keep correct at the one input that - decides who everybody is.""" - from .jwt_identity import _NoRedirects - + """HTTPS, and follows nothing. `_NoRedirects` is defined in this module: two copies of + this handler would be two things to keep correct at the one input that decides who + everybody is, and importing it from `jwt_identity.py` was the layering cycle §6 + forbids.""" return urllib.request.build_opener( urllib.request.HTTPSHandler(context=ssl.create_default_context()), _NoRedirects() ) diff --git a/tests/test_module_graph.py b/tests/test_module_graph.py new file mode 100644 index 00000000..bc6b411a --- /dev/null +++ b/tests/test_module_graph.py @@ -0,0 +1,190 @@ +# SPDX-FileCopyrightText: 2026 The CTRLRun contributors +# SPDX-License-Identifier: Apache-2.0 +"""`docs/ARCHITECTURE.md` §6's rule, as a test rather than as a sentence. + +§6 says **dependencies point downward only**, and from v0.7 to v0.11 that was false: a review +found the cycle `state -> receipt -> policy -> authority -> state`, §6 was amended to record it, +and it stayed for five milestones. Nothing was broken at run time, because the two edges out of +`policy.py` are function-level and run after every module is loaded, so `import ctrlrun` resolved +in one order and every test passed. + +**That is the whole reason this file exists.** The rule had no guard, so the only thing that could +contradict it was a human reading the imports, and the one who did had to write a paragraph +instead of a failing test. A claim about the module map needs a test as much as a claim about +behaviour, which is the rule `SPEC-v0.11.md` §13.3 states for claims about what a feature does +*not* do. + +**Module-level imports only, deliberately.** A function-level import is a real edge for layering +and *not* an edge for import order, and conflating them is what let the cycle read as harmless. +This walks the AST, so it sees what a reader of the file sees, without importing anything. +""" + +from __future__ import annotations + +import ast +from pathlib import Path + +import pytest + +PACKAGE = Path(__file__).resolve().parent.parent / "src" / "ctrlrun" + + +def _module_name(path: Path) -> str: + """`src/ctrlrun/gateway/server.py` -> `gateway.server`, and `__init__.py` -> its package.""" + relative = path.relative_to(PACKAGE).with_suffix("") + parts = list(relative.parts) + if parts[-1] == "__init__": + parts.pop() + return ".".join(parts) + + +def _edges(*, deferred: bool = False) -> dict[str, set[str]]: + """Every module-level intra-package import, as a graph. + + **Resolving `level` correctly is the whole of this function, and getting it wrong invents + cycles.** A first version treated `from .. import transport` inside `gateway/outcome.py` as + `gateway.transport` instead of the top-level `transport`, and reported + `gateway.outcome -> gateway.transport -> gateway.outcome`, a cycle that does not exist. A + detector that invents edges is worse than none, because the fix for a phantom cycle is a + refactor nobody needed. + + `level` counts the dots. Level 1 is the module's own package; each dot above that drops one + component. `from . import x` and `from .. import x` carry no `module`, so the imported name + is itself the submodule. + """ + graph: dict[str, set[str]] = {} + for path in sorted(PACKAGE.rglob("*.py")): + name = _module_name(path) + parts = name.split(".")[:-1] if "." in name else [] + targets: set[str] = set() + tree = ast.parse(path.read_text(encoding="utf-8"), filename=str(path)) + # `deferred=False` is the import-order graph: what runs at `import ctrlrun`. + # `deferred=True` adds function-level imports, which is the **layering** graph -- a + # deferred import is still one module knowing about another, which is what §6's map says. + for node in ast.walk(tree) if deferred else tree.body: + if isinstance(node, ast.ImportFrom) and node.level: + climb = node.level - 1 + if climb > len(parts): + continue # reaches above the package; not an intra-package edge + base = parts[: len(parts) - climb] + if node.module: + targets.add(".".join([*base, node.module])) + else: + # `from . import x` / `from .. import x`: each name is a submodule. + targets.update(".".join([*base, alias.name]) for alias in node.names) + elif isinstance(node, ast.Import): + for alias in node.names: + if alias.name.startswith("ctrlrun."): + targets.add(alias.name.removeprefix("ctrlrun.")) + graph[name] = {t for t in targets if t and t != name} + return graph + + +def _cycles(graph: dict[str, set[str]]) -> list[list[str]]: + """Every elementary cycle, by depth-first search, reported as the path that closes it.""" + found: list[list[str]] = [] + seen: set[str] = set() + + def walk(node: str, path: list[str], on_path: set[str]) -> None: + for target in sorted(graph.get(node, ())): + if target in on_path: + found.append([*path[path.index(target) :], target]) + elif target not in seen: + walk(target, [*path, target], on_path | {target}) + + for start in sorted(graph): + if start not in seen: + walk(start, [start], {start}) + seen.add(start) + return found + + +def test_the_import_order_graph_has_no_cycle() -> None: + """No cycle among module-level imports: what actually runs at `import ctrlrun`. + + **This is not the test that catches §6's recorded cycle, and saying so matters.** Run against + the tree as 0.11.0 shipped it, this passes, because the two edges out of `policy.py` are + function-level. That is the same fact §6 states, and it is why a reviewer had to find the + cycle by reading. The layering test below is the one that fails there. + """ + cycles = _cycles(_edges()) + + assert cycles == [], "module-level import cycle: " + ( + "; ".join(" -> ".join(cycle) for cycle in cycles) + ) + + +#: The one layering cycle this repository has decided to keep, and the only one. +#: +#: `authority.py` imports `Condition` and `parse_conditions` from `policy.py` deliberately and at +#: module level: a grant's `constraints:` is a rule's `when:` syntax and `SPEC-v0.3.md` §4.5 says +#: the two axes MUST share one evaluator. §6 records that as an exception. The reverse edge is +#: `policy.py` reaching `authority.py` from inside `_canonical_authority` and +#: `hash_with_authority`, and it cannot be removed by relocation: `_from_section` *constructs* an +#: `Authority` and `canonical_grants` *consumes* one, so neither moves below `policy.py`, and +#: moving the two callers up to `control.py` would change what `policy_hash` is taken over -- +#: which is evidence in every receipt, not an implementation detail. +#: +#: So it stays, named, until someone decides that question. An allow-list of one fails the moment +#: a second appears, which a plain "no cycles" assertion softened to a skip never would. +RECORDED_LAYERING_CYCLES: frozenset[frozenset[str]] = frozenset( + {frozenset({"policy", "authority"})} +) + + +def test_the_layering_graph_has_only_the_one_recorded_cycle() -> None: + """§6's actual rule, including deferred imports, with one documented exception. + + A function-level import is a real edge for the module map and not one for import order, and + conflating them is exactly what let `state -> receipt -> policy -> authority -> state` read as + harmless for five milestones. This is the assertion that fails on 0.11.0's tree. + """ + found = {frozenset(cycle) for cycle in _cycles(_edges(deferred=True))} + + assert found == RECORDED_LAYERING_CYCLES, ( + "ARCHITECTURE §6 says dependencies point downward only. New: " + f"{sorted(map(sorted, found - RECORDED_LAYERING_CYCLES))}. " + f"Gone, so remove it from RECORDED_LAYERING_CYCLES: " + f"{sorted(map(sorted, RECORDED_LAYERING_CYCLES - found))}" + ) + + +def test_the_decision_vocabulary_depends_on_nothing_in_the_package() -> None: + """`decision.py` is the floor the cycle was broken against, and a floor that grows an + intra-package import is not a floor. `errors.py` holds the same position and is checked + with it, since it is what `decision.py` would reach for first.""" + graph = _edges() + + assert graph["decision"] == set(), f"decision.py imports {sorted(graph['decision'])}" + assert graph["errors"] == set(), f"errors.py imports {sorted(graph['errors'])}" + + +def test_a_receipt_does_not_import_the_decider() -> None: + """The specific edge that closed the cycle, pinned by name. + + The graph test above fails on *any* cycle, which is the guard that matters. This one names + the edge that was there, so a change reintroducing it fails with the reason rather than with + a path a reader has to re-derive. + """ + graph = _edges() + + assert "policy" not in graph["receipt"], ( + "receipt.py imports policy.py again; that edge is what made " + "state -> receipt -> policy -> authority -> state" + ) + + +@pytest.mark.parametrize("cycle", [("a", "b"), ("a", "b", "c")]) +def test_the_cycle_detector_finds_a_cycle_it_is_given(cycle: tuple[str, ...]) -> None: + """The positive control. A detector that returns `[]` on everything passes the test above + on any codebase, which is the shape of green this project keeps refusing. + """ + graph = {node: {cycle[(index + 1) % len(cycle)]} for index, node in enumerate(cycle)} + + assert _cycles(graph), f"the detector missed {' -> '.join(cycle)}" + + +def test_the_cycle_detector_passes_a_graph_that_is_a_dag() -> None: + """The other half of the control: it must not report a cycle on a diamond, where two paths + reach one module without any edge pointing back.""" + assert _cycles({"a": {"b", "c"}, "b": {"d"}, "c": {"d"}, "d": set()}) == []