diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 6d406d2..1f50747 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -124,12 +124,15 @@ jobs: # G13 is N/A on SQLite, the action's default store: SQLite has no clock of its own # to diverge from; G15 is N/A because neither document declares `max_attempts`. # G16 is graded on both: verify brings its own precondition provider (SPEC-v0.7 §8.9). - # G18 likewise since v0.8 item 2, which supplies its own approver identity: whether + # G17 is N/A on both since v0.8 item 3: neither document names an + # `approver_role`, which is a statement about what the operator wrote + # (SPEC-v0.8 §3.5, §11.7). G18 is graded since item 2, which supplies its + # own approver identity: whether # an operator configured one is a fact about their application and never an N/A # reason here (SPEC-v0.8 §11.7). Both counts moved by one when it landed. - test "$AUTHORITY_NA" = "2" + test "$AUTHORITY_NA" = "4" test "$TEMPLATES" = "verified 9/9" - test "$TEMPLATES_NA" = "8" + test "$TEMPLATES_NA" = "10" test -s verify-badge.json test -s verify-report.json test -s verify-report.xml diff --git a/CHANGELOG.md b/CHANGELOG.md index a47a2cc..225889f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,6 +11,139 @@ any change to one appears here. ### Added +- **Break-glass is a grant, and there is no flag** (`docs/SPEC-v0.8.md` §5). An incident needs + authority nobody was granted in advance. The wrong answer is a setting: a setting leaves no + record, expires never, cannot be revoked and cannot be narrowed. `authority.py` already has + grants that are all five, so break-glass is a delegation beneath an **envelope** the policy + declared in advance. + + ```yaml + authority: + break_glass: + incident-payments: + subject: {agent: "oncall-*"} # who a grant opened here may be FOR + actions: ["payments.*"] + constraints: {amount_lte: 50000} + max_ttl: PT4H # the longest expiry a grant beneath it may carry + controls: [incident-response] # whose approver_role gates who may OPEN it + ``` + + ``` + ctrlrun break-glass --envelope incident-payments --file grant.yaml --reason "INC-4412" + ``` + + **The envelope decides nothing, by construction.** It lives in `Authority.envelopes`, a mapping + separate from `grants`, because the candidate set is every entry of `grants` unconditionally: an + envelope living there would decide actions, which is the opposite of what it is for. The test + asserts it is absent from the candidate set rather than merely unmatched. + + **It is covered by the policy hash, `max_ttl` included.** The argument for declaring the widest + authority an incident can reach in a file is that somebody reviewed it *before* the incident, and + that argument is only true if widening it moves every receipt. + + **There is no `--as`.** Whoever opens one is the principal the deployment's approver identity + resolves, gated by the envelope's `controls:`; a deployment that names no approver identity + cannot open one at all. An assertion typed at a shell is exactly what break-glass must not + accept. + + **What it is afterwards**: recorded, with `created_via: break-glass`; expiring, and bounded by + `max_ttl` from the moment it was opened; revocable, and revoking it stops everything beneath it; + attenuable, obeying `child ⊆ parent` on every dimension. A setting has none of those. + + **`Receipt.authority_grant_id`** now names the grant that decided the action, for **every** + action decided by authority and not only under break-glass. A field exercised only on the rare + path is one nobody notices breaking. + + `created_via` gains its third value, which is a public change rather than an addition: the + vocabulary is a closed set and a record carrying an unknown value is unreadable, which answers + `authority_unreadable` for every action in the deployment. It moves with every reader in one + commit. + + No guarantee id. The roadmap assigned five to v0.8 and G22 to G24 to v0.9, so inventing a sixth + would collide or renumber, and a renumber is the maintainer's change. Its evidence is its tests, + and one of them greps the shipped package for sixteen names a flag would be spelled as. + +- **M-of-N approvals** (`docs/SPEC-v0.8.md` §4). An action may require more than one yes, and what + the threshold counts is **distinct verified principals**: a second answer from a principal that + already answered is recorded, moves that entry's `granted_at`, and does not move the count. + + ```yaml + schema: ctrlrun.policy/v6 + actions: + payments.refund: + decision: approve + approvals_required: 2 + ``` + + **The count is decided where the row is written, on all three stores**, and never by a read + followed by a write: SQLite counts inside its `BEGIN IMMEDIATE` transaction, Postgres + compare-and-sets on the approver list it read and retries, and the in-memory store holds its + lock. Two processes answering at the same instant produce two approvers or one, never a + threshold reached twice. + + **A yes that cannot be attributed does not count.** `approvals_required` above 1 in a deployment + that names no approver identity is a denial, not a silent downgrade to one approval: the kernel + cannot tell two anonymous yeses apart, so it refuses rather than counting them. `ctrlrun approve` + records no verified approver and therefore never counts toward a threshold, which the CLI says + at the moment it is used rather than leaving to be discovered. + + `ApprovalStore.grant_approval` now returns `Approval | None`, where `None` means **recorded and + still short of N**. Nothing is granted, no `APPROVAL_GRANTED` event is written, and a consume + attempted below the threshold is refused as `pending` with nothing reserved. + + Needs `ctrlrun.policy/v6`. `ctrlrun verify` grades **G19** under `ctrlrun.guarantees/v4`, `N/A` + where every action in the document takes one approval. + +- **Entitlement from the control registry** (`docs/SPEC-v0.8.md` §3). A control may now name the + role that answers for it, and an approval whose recorded entitlement does not cover the roles + the request pinned is refused, with the control named in the message, the exception and the + `APPROVAL_INVALIDATED` event. + + ```yaml + schema: ctrlrun.policy/v6 + controls: + card-data-handling: + title: Cardholder data changes are approved by a named owner + approver_role: payments-owner + ``` + + **CTRLRun does not interpret the role.** It does not know what `payments-owner` means, does not + check that such a role exists anywhere, and makes no compliance claim on the strength of one, + exactly as it does not interpret `source:`. What changed about `SPEC-v0.6.md` §7.3's + "attribution, not prevention" is one sentence: a control still decides no *action*, and now + decides **who may answer an approval the decision already required**. + + **Omission is not entitlement, and a control naming no role gates nobody.** Two sentences that + mean opposite things: a principal whose claims lack the role is not entitled, because a missing + claim is a statement about a person and the kernel refuses to invent one; a control with no + `approver_role` gates nobody, because a missing role is a statement about the operator's + document and inventing one there would refuse every approval in every deployment that has + controls and has not heard of v0.8. + + **Roles are matched byte for byte**, in both claim shapes. No case folding, no trimming, no + prefix matching and no pattern grammar: a wildcard in a role would be an entitlement nobody + wrote. Where an evaluation cites several controls, **every** required role must be held, because + any-of lets the weakest control in the set decide who may answer. + + **`ClaimValue` gains a tuple of strings**, amending `SPEC-v0.3.md` §2.1. A roles claim is a JSON + array at every issuer anybody deploys, and the old rule meant such a claim arrived *absent*, so + its holder was silently unentitled. `JWTIdentityProvider` carries an array-of-strings claim now + instead of dropping it, and says so at WARNING rather than DEBUG when it drops anything else it + was asked to carry. Safe for hashes: `v0.3 §2.2` keeps claims out of an action's canonical form. + + **The check is bounded and the bound is stated.** What the kernel refuses is an approval whose + *recorded* entitlement does not cover the role; what entitled it was decided where the credential + was verified, which is the operator MCP server (`ctrlrun mcp-operator --approver-roles-claim`) + and an embedding application. `docs/SPEC-mcp-operator.md` §4.3 and §10 are amended to say that, + and §4.3 now carries a three-row table instead of one sentence, because the sentence covered + two unconfigured cases that behave in opposite ways: a control naming no role admits any + verified human, and a control naming a role in a deployment with no claim to read roles from + refuses **everyone**. The server warns about the second at startup rather than at the first + refusal. + + Needs `ctrlrun.policy/v6`. `ctrlrun verify` grades **G17** under `ctrlrun.guarantees/v4`, `N/A` + with a reason that is true of a document naming no approver role. + - **The approver is a principal** (`docs/SPEC-v0.8.md` §2, §4.1). `Approval.approver` is a string whose only check is that it is not empty, and `adapter.py` has always conceded what that string often is: a channel, wherever the framework's primitive does not identify a person. A deployment diff --git a/docs/SPEC-mcp-operator.md b/docs/SPEC-mcp-operator.md index 149a6f1..392a465 100644 --- a/docs/SPEC-mcp-operator.md +++ b/docs/SPEC-mcp-operator.md @@ -172,6 +172,7 @@ Then, in this order: | The provider returned `None` | Refused, `-41007`. A decline is a refusal here, with no `context()` to fall back to and nothing that may be backfilled | | It returned a `Principal` with `expires_at` set and now past it | Refused, `-41014` `ctrlrun.principal_expired`. `v0.3 §2.3`'s check, applied at the one entry point that never reaches `Control.execute` | | It returned a `Principal` with `user is None` | Refused, `-41013` `ctrlrun.not_a_human` (§3.2) | +| It returned a `Principal` the cited control's `approver_role` does not cover | Refused, `-41015` `ctrlrun.not_entitled`, with the control and the role named (`SPEC-v0.8.md` §3.7). Its own code for `-41014`'s reason: the answer tells a human which role they were missing, and `-41007` deliberately says nothing about why | | Otherwise | The write proceeds, attributed to it (§5.4) | **No receipt and no events for any row above.** `v0.3 §3.2`'s last paragraph is the rule: a @@ -240,9 +241,28 @@ answered yes to one exact `action_hash`; it becomes permission only when `Contro consumes it, and `Control.execute` evaluates the principal's expiry, then authority, then policy, then the approval, in that order (`v0.3 §4.3.1`), every time, for the action the grant names. A second authority evaluation here would be evaluating the *approver's* authority against the -*agent's* action, which is a different question that this release does not answer (see §10: -authenticating the approver's entitlement to approve is not in scope, and the honest place for it -is a separation-of-duties model that does not exist yet). +*agent's* action, which is a different question and one this document still does not answer. + +**Amended by `SPEC-v0.8.md` §3, and the amendment is narrower than it sounds.** Since v0.8 this +server checks the approver's **entitlement**: where the deployment names an approver identity and +the cited control names an `approver_role`, an answer from a credential that does not carry that +role is refused here, with the control named, and the roles the answer satisfied are recorded on +the approval row. That is not an authority evaluation and it is not separation of duties: it is +one string from the operator's own control registry compared against one claim on a verified +credential, and CTRLRun interprets neither. + +**The unconfigured cases are two, they are not the same, and the earlier wording here stated +one of them backwards.** `SPEC-v0.8.md` §3.5's rule is that omission is not entitlement and it is +not refusal either, and which one a deployment gets depends on which half is missing: + +| What the deployment configured | What this server does | +|---|---| +| No approver identity, or a cited control naming no `approver_role` | The paragraph above is unchanged and describes the deployment: any human whose credential the provider verifies can answer any pending request | +| A control naming an `approver_role`, and no `--approver-roles-claim` | **Every answer to that request is refused**, here and again at consumption. No claim can be read, so no role is held, and half a check fails closed (`SPEC-v0.8.md` §3.4). The server warns at startup rather than at the first refusal | +| A control naming an `approver_role`, and a claim the credential does not carry | That answer is refused, `-41015`, with the control and the role named | + +Both of the first two are true of real configurations, which is why both are here, and the +difference between them is the whole of §3.5. `resolve` is the same shape: it states what happened at a remote. It is `v0.1 §5.2`'s human authority, and the store already refuses to apply it to anything but an `AMBIGUOUS` record. @@ -429,6 +449,7 @@ enforcing, and answering one changes nothing in the world. | A write tool with no principal, a declined or rejected credential | HTTP 403, `-41007` | | A write tool whose principal has no `user` | HTTP 403, `-41013` | | A write tool whose principal has expired | HTTP 403, `-41014` | +| `approve` whose principal lacks a role a cited control requires | HTTP 403, `-41015` | | `approve`/`deny` on an unknown, answered or expired request | HTTP 200, `-41003`, with the store's reason | | `resolve` on a record that is not `AMBIGUOUS`, or an unknown key | HTTP 200, `-41003`, with the store's reason | | `resolve` with a blank reason | HTTP 200, `-32602` | @@ -626,6 +647,7 @@ neither is reachable from the gateway. |---|---|---| | `-41013` | `ctrlrun.not_a_human` | 403 | | `-41014` | `ctrlrun.principal_expired` | 403 | +| `-41015` | `ctrlrun.not_entitled` | 403 | Reused unchanged: `-41003` `ctrlrun.approval_denied` for a store refusal about an approval or an effect, and `-41007` `ctrlrun.no_principal`. @@ -694,13 +716,22 @@ did before the move to `ctrlrun.reporting`, exit code included. Everything `v0.6 §11` excludes, plus: -- **Authenticating the approver's *entitlement*.** This server authenticates *who* is answering; - it does not check that they were allowed to. `v0.3 §13` and `v0.5`'s do-not-build list already - exclude authenticating the approver, separation of duties, M-of-N and break-glass, and nothing - here changes that. The honest statement is in §4.3: any human whose credential the provider - verifies can answer any pending request, exactly as any human who can run `ctrlrun approve` - can today. **This is attribution, not authorization**, and no document may describe it as the - latter. +- **Authenticating the approver's *entitlement*.** ~~This server authenticates *who* is + answering; it does not check that they were allowed to.~~ **Amended by `SPEC-v0.8.md` §3**, + and the strikethrough is deliberate: this line was true of every release up to 0.7.0, and a + reader of an older deployment's documentation should be able to see which sentence applied. + + What is true now: where an approver identity and an `approver_role` are configured, this server + **does** check entitlement, refuses an answer the credential is not entitled to give, and + records what the answer satisfied. Where they are not, the old sentence still holds exactly: + any human whose credential the provider verifies can answer any pending request, exactly as any + human who can run `ctrlrun approve` can. + + **What is still out of scope**: separation of duties as a model, and evaluating the approver's + *authority* against the agent's action. M-of-N and break-glass arrive with v0.8's items 4 and 5 + and are not this server's. And the check is bounded the way `SPEC-v0.8.md` §3.8 bounds it: what + it compares is one operator-written string against one claim, and what the kernel later refuses + is an approval whose **recorded** entitlement does not cover the role. - **stdio transport.** An MCP server launched over stdio by the assistant has no credential to verify — the process is whatever the client started, and every candidate identity is asserted by it. That is `--principal-from-client-info` (`v0.3 §8.1`) in a fourth costume, and it is the @@ -729,6 +760,7 @@ Everything `v0.6 §11` excludes, plus: | A write tool, credential declined or rejected | `-41007`, store unchanged | | A write tool, credential names no human | `-41013`, store unchanged | | A write tool, credential expired | `-41014`, store unchanged | +| `approve`, credential missing a required `approver_role` | `-41015`, store unchanged: no grant, no event, no partial row | | `resolve` with no reason | `-32602`, store unchanged | | An unknown tool, an unknown method, a bad argument | A JSON-RPC error, store unchanged | | Anything the store refuses | The store's refusal, unchanged. Store unchanged, **except** that answering a lapsed request records the lapse (§4.2) | diff --git a/docs/SPEC-v0.8.md b/docs/SPEC-v0.8.md index 672df03..c2a146b 100644 --- a/docs/SPEC-v0.8.md +++ b/docs/SPEC-v0.8.md @@ -565,8 +565,21 @@ expectation they failed. ### 3.4 A missing claim is not a role The approver's roles are read from the claim `ApproverIdentity.roles_claim` names, on the resolved -principal's claims. Three facts about the code shape every rule below, and each was found by -reading it rather than assumed: +principal's claims. + +**One surface may name it twice, and the flag wins.** The operator MCP server has its own +`--approver-roles-claim`, because it resolves the approver with its own provider and a deployment +may run it against an issuer the embedding application does not use. Where both are set the flag +wins; where only the `Control`'s is set the server reads that. This is stated rather than left to +be discovered because **it is a flag that changes the outcome of an entitlement decision**, and the +rule that no flag relaxes a check is only true if every flag that touches one is written down. It +cannot relax the check: naming a claim that carries no role refuses *more*, never less, and the +kernel's refusal at consumption reads what was recorded either way. The server warns at startup +where a cited control names a role and neither source names a claim, because that configuration +refuses every approval it gates. + +Three facts about the code shape every rule below, and each was found by reading it rather than +assumed: - `ClaimValue` is `str | int | bool` and `Principal.claims` **refuses containers** (`action.py`), whose stated reason is that "a provider flattens or drops a structured claim @@ -663,8 +676,11 @@ type, because a test asserting a type alone cannot tell which of §2.7's four re ### 3.8 Where entitlement is decided twice, on purpose The grant surface computes `entitled` from the request's `required_roles` and refuses on the spot -where the approver holds none of them, so a human learns at the moment they answer rather than at -the moment an agent retries. +where the approver does not hold **every** one of them, so a human learns at the moment they answer +rather than at the moment an agent retries. Not "holds none of them", which was the earlier wording +here and is a different rule: §3.6 is all-of, an approver holding one of two required roles is +refused, and a surface applying any-of would admit an answer the kernel then refuses at +consumption, which is the worst of both halves. **What each half is, said in the register `CONTRIBUTING.md` demands.** The consumption check **prevents consumption** of an approval whose recorded entitlement does not cover the roles the @@ -760,12 +776,31 @@ once" is the requirement, and rejecting the second answer would make a human thi lost. Once the record has reached `granted`, a further grant is refused exactly as it is today, by `check_answerable`, which refuses any record that is not `PENDING` and which `v0.1 §4.2` freezes. -**What does not count**, each refused before the count moves, and each with its own test asserting -the count did not move: +**What does not count, and where it stops counting.** Three answers must not authorize an action, +and building item 4 established that only one of them can be excluded from the count itself. The +draft of this paragraph said all three were "refused before the count moves"; §14.4 records why +that was wrong and what replaced it. -- the requester's own yes (§4.1); -- an approver who satisfies none of the required roles (§3.8); -- an approver the provider could not verify (§2.7). +| Answer | Where it is refused | Does the count move? | +|---|---|---| +| An approver the provider could not verify (§2.7) | In the count. `count_grant` records no approver for a grant carrying none | No. The row is unchanged | +| The requester's own yes (§4.1) | At consumption, `approver_is_requester` | **Yes**, and the row shows who gave it | +| An approver satisfying a required role they do not hold (§3.8) | At the granting surface, and again at consumption, `approver_unentitled` | **Yes**, at consumption; the grant surface refuses before the row is written at all | + +The two that count are still refused, every time, on every path, and the receipt names which +refusal fired. What they do not do is make the record unreachable: a yes that never lands leaves +the request `pending` for ever, and at N=1 that turns a refusal a reader can act on into a silence +they cannot. + +**A partial grant is visible in one place only, and that is deliberate.** `ctrlrun.inspection/v2` +is unchanged (§11.3) and §11.2 adds no event type, so a request that one of two humans has answered +reads identically to one nobody has answered in `ctrlrun inspect --json` and in the event log. What +reports it is the answer each grant surface returns, to the person who just answered. The reason is +that the alternative is an event type meaning "somebody answered and it changed nothing", which is +a row every reader of the chain has to learn to ignore, for a fact with a half-life of minutes. The +cost is stated here rather than left to be discovered: **an operator cannot currently see how many +of N a pending request holds without asking the store for the row.** If that turns out to matter it +is an inspection schema bump, on its own version line, not an event. **A denial denies the request, whole.** One `deny_approval` moves the record to `denied` however many grants it holds. A request that absorbs a no while it waits for enough yeses is a request that @@ -1522,6 +1557,19 @@ forbids (the third). a self-approved grant: refused `approver_is_requester`, the executor not called, **the approval still granted and nothing reserved**. Without the last two assertions the test cannot tell a refusal before the store call from one after, which is what let the deferral look correct. +- **T283b:** `entitled` is validated before it is converted. `str` is a `Sequence`, so + `tuple("abc")` is three control ids: a corrupted column was accepted as an approver entitled for + controls that do not exist, which made `_approvers_from_json`'s promise to raise false for the + shape a corruption most easily takes. +- **T296b:** the report still adds up. An observe-mode approval refusal, and an observe-mode + **denial by a human**, are each counted by `ctrlrun stats`. `would_have.blocked_reason` is a + closed vocabulary because `v0.3 §6.4` buckets counts on it, and recording each mismatch's own + reason put them in no bucket at all. Driven with no `ApproverIdentity` anywhere, because the + receipts that stopped counting have nothing to do with v0.8, and the denial one was already + uncounted before it. +- **T296c:** a resumed leg carries the approvers onto its receipt. That leg is the **only** receipt + an MCP multi round-trip or an ACS action ever gets (`SPEC-mcp-operator.md` §8.3), so without it + §2.5's "carried onto the receipt" is false for exactly the actions that get one receipt. - **T292:** the migration ledger reaches `HEAD` and the column round-trips through a file-backed store. The 0.6.1-built upgrade in both directions is `test_preconditions.py`'s T264, and §14.2 says why this one does not duplicate it. @@ -1586,14 +1634,18 @@ forbids (the third). `granted_at` moves. - **T312:** a grant arriving after the record reached `granted` is refused exactly as at 0.7.0, by `check_answerable` (§4.2). -- **T313:** the concurrency case, deterministic, and it is aimed at the defect §4.3 names. Separate +- **T313:** the concurrency case, deterministic, and it is aimed at the defect §4.3 names. It + lives in `tests/test_attempt_integrity.py` with the proxy and the child-process harness the + window needs, and `tests/test_m_of_n.py` names it where a reader of the item's tests looks. Separate OS processes against Postgres, with the window opened **between the read of `approvers` and the update**; N grants never become N+1 and one principal never fills two slots. The test is written so that it **fails against a compare-and-set on `status`**, because that is the shape the store has today and the one a reviewer found: a test that passes against it has not opened the window. -- **T314:** the requester's own yes does not count, and the count is asserted unmoved (§4.2). -- **T315:** an unentitled yes does not count, and the count is asserted unmoved. -- **T316:** an unverifiable yes does not count, and the count is asserted unmoved. +- **T314:** the requester's own yes is refused at consumption with `approver_is_requester`, and + the row shows the count *did* move, which §4.2's table and §14.4 explain. +- **T315:** an unentitled yes is refused with `approver_unentitled`, and the same about the count. +- **T316:** an unverifiable yes does not count, and the count is asserted unmoved: this is the one + of the three that the count itself excludes. - **T317:** one denial denies a request holding N-1 grants. - **T318:** expiry: a request with N-1 grants expires as a whole, and the grants do not extend it. - **T319:** `approvals_required: 1` is 0.7.0 exactly, driven through the whole path and compared. @@ -1782,6 +1834,7 @@ the section it cites, and only then is there code. |---|---|---|---| | Two CLI options | 1 | `ctrlrun revoke --created-by`, `--under` | The incident operation is a query over rows that already exist, and writing it under pressure is how a script revokes the wrong subtree. `--created-by` and not `--by`, because `--by` already means who performed the revocation (§7.2). | | One configuration object | 2 | `ctrlrun.approval.ApproverIdentity(provider, roles_claim=None)`, and `Control(approver_identity=...)` with a read-only `Control.approver_identity` | Opt in, then fail closed, needs one switch. A provider without a roles claim cannot answer §3, so the two travel together or a deployment has a silent half-check (§2.3). | +| One server flag | 3 | `ctrlrun mcp-operator --approver-roles-claim`, and `OperatorConfig.approver_roles_claim` | The server resolves the approver with its own provider, against an issuer the application need not share. It takes precedence over `ApproverIdentity.roles_claim` and falls back to it; §3.4 says why a flag touching an entitlement decision is named here. | | One record type | 2 | `ctrlrun.approval.VerifiedApprover(agent, user, issuer, entitled, granted_at)`, read back on `ApprovalRecord.approvers` | `ApprovalRecord` is rebuilt from columns, so a value the consume-side check reads must be one (`v0.6 §3.7`). | | One amendment to `v0.3 §2.1` | 3 | `ClaimValue` gains `tuple[str, ...]`; `_frozen_claims` accepts a sequence of strings and normalises it to a tuple; `JWTIdentityProvider` carries an array-of-strings claim instead of dropping it | A roles claim is a JSON array at every issuer anybody deploys, and `Principal.claims` refuses containers today, so §3 is unbuildable without this. The normalisation is the half that keeps it readable: JSON has no tuple, so a stored claim returns as a list, and without it every receipt carrying one would raise on read, against `Receipt.from_dict`'s never-raises contract. Safe for hashes: `v0.3 §2.2` keeps claims out of an action's canonical form (§3.4). | | One record type | 3 | `ctrlrun.approval.RequiredRole(control, role)` | The refusal must name the control (§3.7), so the pair survives to the refusal; a role alone leaves an operator grepping a registry. | @@ -2123,10 +2176,128 @@ questions. ### 14.3 Item 3: entitlement from the control registry +**The tests were written after the implementation, and that is worth recording rather than +hiding.** Every other item in this milestone wrote a red suite first, as `CONTRIBUTING.md` asks; +this one did not, and all forty-eight passed on their first run. What carried the weight instead +was the mutation table, and it found within minutes what the ordering had cost: **case-folding the +*string* claim branch left every test green**, because T303 parametrised only the list shape and +nothing exercised the other branch. Both shapes are covered now, with a positive control, and the +lesson is the one the discipline exists for: a test written against code that already works tests +the code you wrote rather than the rule you meant. + +**One mutation is an equivalent mutant, and it is declared rather than claimed.** Recording +`required` instead of the computed `entitled` at the grant is indistinguishable, because the +refusal fires first whenever any role is missing, so past that point `entitled` provably equals the +full required set. `CONTRIBUTING.md` says to say so in the table rather than dress it up as closed. + +**`RequiredRole` lives in `approval.py` and `Control` builds the pairs.** The obvious home for the +lookup was `Policy.required_roles(controls)`, and it was written and then reverted: `policy.py` +imports `action` and `effect`, `approval.py` imports `action`, `errors` and `identity`, and neither +imports the other. Putting the accessor on `Policy` meant a new `policy` to `approval` edge for one +dataclass, and §11.1 freezes no such accessor. `Control` already holds both the evaluation and the +registry, which is where the join belongs. + +**The entitlement refusal reaches its event by the carrier, not by a new keyword.** `§3.7` requires +`APPROVAL_INVALIDATED` to name the control and the role, and the first attempt added a `detail` +keyword to `ApprovalMismatch`: a public name §11 does not list, on the closed error set `v0.6 §9.2` +freezes. The precondition hashes already travel to that event on `_Compared`, so the pair travels +the same way. + +**The grant-side refusal needed a policy of its own to be testable.** `test_mcp_operator.py`'s +document cites no control with a role, so the first version of that test asserted a 403 it could +never get: the server had nothing to refuse. The two §3.8 tests carry their own gated document, and +the request is created through `Control.execute` rather than through the provider, because +`_required_roles` is set around **that** call and a request built straight from the provider pins +nothing. A test that skipped the pinning would have asserted a refusal no deployment reaches. + +**What G17 moved.** It is `N/A` on both shipped examples, because neither names an `approver_role`, +which is a true statement about those documents (§3.5). Nine tests and two pins in +`.github/workflows/ci.yml` moved by one as a result, which is the same class of change G18 forced +in item 2 and the second time that CI file has been the thing no local gate would catch. + ### 14.4 Item 4: M-of-N +**"Does not count" cannot mean "is not counted", for two of the three.** §4.2's first draft said +the requester's own yes, an unentitled yes and an unverifiable yes were each *refused before the +count moved*. Building it showed that reading destroys G18. At `approvals_required: 1`, excluding +the requester's yes from the count leaves the record `pending` for ever: the consumption check +never reaches `approver_is_requester`, because there is nothing granted to check, and the refusal a +reader gets is `pending` — "nobody has answered yet" — about a request the requester answered. G18 +is shipped, `verify` grades it, and it says a self-approval is *refused as one*. + +So the store counts every verified approver, and all three refusals live at consumption where the +kernel can name which one fired. One rule, in one place, and the store stays a store rather than +acquiring a second opinion about who may answer. §4.2 now carries the table. + +**The same argument, run the other way, is why an unverifiable yes *is* excluded from the count.** +There is nothing to refuse it by: a row with no verified approver records nothing that could be +compared against a requester or a role, and at N=1 it must still grant, because that is 0.7.0 +unchanged and R1 promises a deployment naming no approver identity is untouched. + +**The read-back had to grow, and an independent review found that it had not.** `v0.7 §6.4` reads +the stored request back after the provider returns, because a provider that builds its own +`ApprovalRequest` and a store that drops a column both produce a row missing what the kernel +pinned. Items 3 and 4 pin two more fields by exactly the same route and added no check, so both +were lost in both ways with nothing refused: a row pinning `required_roles=()` satisfies every +control trivially, and one pinning `approvals_required=1` grants on a single yes. An action ran +under a policy demanding two approvals from a named role, approved once, by somebody holding no +role. `§4.5`'s sentence that no such path exists was false when it was written; the read-back now +covers all three fields and `approval_unrecorded` is its refusal. + +**The count's window is not the same thing as two processes answering at once.** Item 4's serial +tests all pass against a store with no compare-and-set whatever, which the mutation table caught by +being caught for the wrong reason: removing Postgres's condition turned a *sequential* test red. +T313 opens the real window, between the read of `approvers` and the update of it, and fails against +the compare-and-set on `status` that the store had. The store conformance case learned the same +lesson one layer down: it accepted a `processes` argument and never used it. + ### 14.5 Item 5: break-glass as a grant +**`delegable` is read at three sites and they are not interchangeable.** §5.2 point 4 named all +three before the code was written, and building it confirmed each has a different failure: without +the creation-root site nothing is created; without the rule-3 chain scan nothing can be delegated +beneath a break-glass grant; without rule 6 the grant is created, looks right in every record, and +authorises nothing on every evaluation, refused `authority_escalation` with no dimension named. +Only the third is silent, which is why T326b asserts **evaluation** and not creation. + +**The exemption is applied where the value is read, never written onto the grant.** A +`delegable=True` on the parsed envelope would have been three lines shorter and would have moved +the policy hash, because `_canonical_grant`'s closed field list always emits `delegable`. The hash +has to be a statement about the document: T332 pins that an envelope renders `delegable: false`, +the parser default, and that a deployment where break-glass evaluates correctly hashes identically +to one where the runtime rule is absent. + +**A break-glass grant is not delegable unless it says so.** Writing item 5's tests found this and +it is not a defect: `delegable` on the grant in `--file` means what it means everywhere, and §5.2 +point 4 exempts the **envelope**, which carries no such key, and changes nothing about the grant +opened beneath it. An operator who wants to sub-delegate during an incident writes `delegable: true` +in that file, and `max_ttl` still bounds the whole subtree in time. + +**`resources:` is contained only where the parent constrains it**, which is `v0.3 §5.4` unchanged +and worth stating because an envelope is where it surprises: an envelope declaring no `resources:` +does not bound them, so an envelope intended to bound resources must say so. Omission is not +unlimited *for the child* — a child may not drop a dimension its parent constrains — and it is +also not a constraint the parent never expressed. + +**Open, and the maintainer's call: `ctrlrun break-glass` cannot succeed in any configuration +the CLI can load.** `Control.from_file` wires no `ApproverIdentity` -- there is no configuration +key for one, and §2.6.1 rules out giving the CLI a provider of its own -- so the command §11.1 adds +for this item always exits 1 saying an approver identity is needed, which is advice the CLI cannot +act on. It fails closed, and the gated path is the only path: `delegate --parent ` is +refused by name. But it means the shell example in §5.3 does not run today, and break-glass is +reachable only from an embedding application that built its own `Control`. + +Three ways out, none of them a build item's to choose: a configuration key naming an approver +identity provider, which is surface §11.1 does not list; a credential option on this one command, +which is the same surface with a narrower blast radius; or striking the shell example and saying +the command serves deployments that configure one in code. **Asked rather than decided**, on the +rule that an item which disagrees with its specification stops. + +**The absence test had to read code rather than text.** `approval.py` explains in a comment that a +public `_granting_principal` would be "`trust_approver` spelled as a context manager", which is +prose arguing the flag away. A grep that cannot tell that from a flag pushes the argument out of +the tree, so T338 tokenizes comments and strings out, and its control plants a flag and finds it. + ### 14.6 Item 6: credential revocation, consumed ### 14.7 Item 7: a policy change is a protected action diff --git a/examples/medical_workbench.py b/examples/medical_workbench.py index a097e6e..05b6d86 100644 --- a/examples/medical_workbench.py +++ b/examples/medical_workbench.py @@ -136,6 +136,15 @@ def step(request_json: str) -> str: approved = store.grant_approval( pending.request_id, "human:demo-medical-reviewer" ) + if approved is None: + # SPEC-v0.8 §4.4. This policy asks for one approval, so this + # branch is unreachable here; it raises rather than passing, + # because an example that silently carried `None` forward would + # fail later somewhere that says nothing about why. + raise SystemExit( + "the demo policy asks for one approval and this grant " + "recorded without reaching it" + ) from None approval = approved.approval_id approval_version = snapshot["version"] result.update(outcome="approved", action_hash=approved.action_hash) diff --git a/pyproject.toml b/pyproject.toml index cb1ad8f..3b27f66 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -68,6 +68,11 @@ ctrlrun = "ctrlrun.cli.main:main" [project.optional-dependencies] dev = [ "pytest>=8", + # The suite is 3700 tests of temp databases, scratch schemas and loopback sockets, each + # owning its own, and running them one at a time cost eleven minutes of wall clock for no + # isolation the tests did not already have. `scripts/check.sh` passes `-n auto`; `-p + # no:randomly -n0` is how an ordering bug is reproduced. + "pytest-xdist>=3", "mypy>=2.3", "ruff>=0.16", # SPEC-v0.4 §4.3 - T115 validates `ctrlrun verify --junit` against the checked-in @@ -133,6 +138,12 @@ markers = [ # unmarked test that appends one, which is what "no `authority:` section behaves # exactly as v0.2" means mechanically inside one pytest process. "authority: builds an `authority:` section (SPEC-v0.3 §4.1)", + # A test that reproduces a timing window through the TCP proxy in + # `tests/failure_injection.py`: a held statement, a killed COMMIT, a partition. These do + # not share a box. Run under `-n auto` on a loaded CI runner, T155b's COMMIT was never + # killed and the test reported "the window never opened" -- correctly, because it had not. + # `scripts/check.sh` runs everything else in parallel and these on their own. + "serial: reproduces a timing window and must not share a machine with other workers", ] [tool.mypy] diff --git a/scripts/check.sh b/scripts/check.sh index c242365..d3be044 100755 --- a/scripts/check.sh +++ b/scripts/check.sh @@ -25,6 +25,17 @@ run() { run ruff format --check run ruff check run mypy --strict src -run pytest +# `-n auto` across cores, `--dist loadfile` so a file's tests stay on one worker: a suite +# whose fixtures are per-file (the Postgres schema fixtures especially) pays fewer setups that +# way, and an ordering assumption inside a file still holds. PYTEST_ARGS overrides for a single +# test or a serial reproduction. +# shellcheck disable=SC2086 +run pytest -n auto --dist loadfile -m "not serial" ${PYTEST_ARGS:-} +# The windows, on their own. `tests/failure_injection.py`'s proxy holds one statement, kills one +# COMMIT or partitions one connection, and the assertion is about what a store did inside that +# window. Seven other workers on the same box turn that into a race, which is how T155b came to +# report "the window never opened" on one Python version and pass on three. +# shellcheck disable=SC2086 +run pytest -m serial ${PYTEST_ARGS:-} printf '\nall checks passed\n' diff --git a/src/ctrlrun/action.py b/src/ctrlrun/action.py index dc57cf6..134e879 100644 --- a/src/ctrlrun/action.py +++ b/src/ctrlrun/action.py @@ -23,10 +23,18 @@ "str | int | bool | tuple[FrozenValue, ...] | Mapping[str, FrozenValue] | None" ) -#: The claim value types SPEC-v0.3 §2.1 allows. `float` is absent for v0.1 §2.3's reason, and -#: containers are absent because a provider flattens or drops a structured claim rather than -#: storing one here. -ClaimValue: TypeAlias = "str | int | bool" +#: The claim value types SPEC-v0.3 §2.1 allows, amended by SPEC-v0.8 §3.4. `float` is absent for +#: v0.1 §2.3's reason. +#: +#: **A tuple of strings is admitted since v0.8, and §3.4 argues it.** A roles claim is a JSON +#: array at every issuer anybody deploys, and the rule this replaced -- that a provider flattens +#: or drops a structured claim -- meant such a claim arrived *absent*, so its holder was silently +#: unentitled. Flattening into a delimited string was rejected: that is structure encoded in a +#: string, and one role named `a,b` away from a defect. +#: +#: Safe where a hash is concerned: `v0.3 §2.2` keeps `claims` out of an action's canonical form, +#: so no action hash and no approval binding moves. +ClaimValue: TypeAlias = "str | int | bool | tuple[str, ...]" #: A `Principal` with no claims. Shared, because it is immutable — but it is handed out by a #: `default_factory` rather than used as a bare default: Python 3.11 refuses *any* unhashable @@ -113,11 +121,27 @@ def _frozen_claims(claims: object) -> Mapping[str, ClaimValue]: f"principal.claims[{key!r}] is a float; use an int or a decimal string, for the " "reason v0.1 §2.3 rejects one in an argument" ) + if isinstance(value, list | tuple): + # SPEC-v0.8 §3.4's second half. JSON has no tuple, so a claim written as one comes + # back a **list**: unamended, every stored action and every receipt carrying an array + # claim would raise here on read, which for a receipt breaks `Receipt.from_dict`'s + # never-raises contract by name (`v0.7 §6.11`). Normalised on the way in instead. + # + # `list | tuple` and never `Sequence`: `str` is a `Sequence`, so the wider check would + # turn the one-role claim "payments-owner" into twenty-one single characters. + items = tuple(value) + if not all(isinstance(item, str) and item for item in items): + raise InvalidArgument( + f"principal.claims[{key!r}] is a list holding something other than a " + "non-empty string; a structured claim is carried as a list of strings or " + "not at all (SPEC-v0.8 §3.4)" + ) + result[key] = items + continue if not isinstance(value, str | int): # bool is a subclass of int, and is allowed raise InvalidArgument( f"principal.claims[{key!r}] is {type(value).__name__}; a claim value must be " - "str, int or bool — a structured claim is flattened or dropped by the provider " - "that read it" + "str, int, bool or a list of strings (SPEC-v0.3 §2.1, amended by v0.8 §3.4)" ) result[key] = value return MappingProxyType(result) diff --git a/src/ctrlrun/adapter.py b/src/ctrlrun/adapter.py index f65193d..907d6d7 100644 --- a/src/ctrlrun/adapter.py +++ b/src/ctrlrun/adapter.py @@ -301,7 +301,27 @@ def wait(self, request_id: str, timeout: timedelta | None = None) -> Approval | ) if answer.granted: - return self._store.grant_approval(request_id, answer.approver) + granted = self._store.grant_approval(request_id, answer.approver) + # SPEC-v0.8 §4.4, and the same trap as the scripted provider: `None` from the store + # means recorded and short of N, while `None` from `wait` means `v0.1 §4.3`'s + # "answered, no". Returning it straight through would report a partial grant as a + # **denial**, and `@protect(wait=True)` would raise `ActionDenied` for a request a + # second human is still answering. + # + # This `wait` is not a polling loop: it puts the question to the framework once. So + # the truthful answer is that nobody completed it here, which is `ApprovalTimeout`, + # naming what is still outstanding. The answer **is** recorded, and another surface + # can complete it. + if granted is not None: + return granted + outstanding = self._store.get_approval(request_id) + recorded = 0 if outstanding is None else len(outstanding.approvers) + needed = 1 if outstanding is None else outstanding.request.approvals_required + raise ApprovalTimeout( + f"approval request {request_id} holds {recorded} of {needed} approvals; this " + "answer was recorded and the request needs another principal", + request_id=request_id, + ) self._store.deny_approval(request_id, answer.approver) return None diff --git a/src/ctrlrun/approval.py b/src/ctrlrun/approval.py index 9c1e4d4..0691cf0 100644 --- a/src/ctrlrun/approval.py +++ b/src/ctrlrun/approval.py @@ -12,10 +12,10 @@ import logging import secrets import time -from collections.abc import Callable, Iterable, Iterator, Mapping +from collections.abc import Callable, Iterable, Iterator, Mapping, Sequence from contextlib import contextmanager from contextvars import ContextVar -from dataclasses import dataclass +from dataclasses import dataclass, replace from datetime import UTC, datetime, timedelta from enum import StrEnum from typing import Any, Final, Protocol, runtime_checkable @@ -85,6 +85,43 @@ class ApprovalStatus(StrEnum): #: asserts the reason and never the type alone. APPROVER_UNVERIFIED: Final = "approver_unverified" APPROVER_IS_REQUESTER: Final = "approver_is_requester" +APPROVER_UNENTITLED: Final = "approver_unentitled" + +#: SPEC-v0.8 §4.2 — `approvals_required` above one where no approver identity is configured. +#: A value of `ActionDenied.reason` and of `ACTION_DENIED.data.reason`, not a new error type. +APPROVALS_UNVERIFIABLE: Final = "approvals_unverifiable" + +#: SPEC-v0.8 §3.3, §4.5, extending `v0.7 §6.4`'s read-back to the two fields items 3 and 4 pin. +#: The request carries what was in force when it was built: the roles the cited controls demand +#: and how many distinct principals must answer. Both travel to the provider through a context +#: variable, so **a provider that builds its own `ApprovalRequest` and a store that drops the +#: columns each produce a row pinning neither** -- and a row pinning neither is a row that gates +#: nobody and grants on one yes, which is the silent downgrade §4.5 says does not exist. +APPROVAL_UNRECORDED: Final = "approval_unrecorded" + + +@dataclass(frozen=True) +class RequiredRole: + """One control's demand about who may answer (SPEC-v0.8 §3.3). + + **The pair and not the role alone**, because §3.7 requires a refusal to name the control: a + refusal naming only a role leaves an operator grepping a registry to find out which written + expectation they failed. + """ + + control: str + role: str + + def __post_init__(self) -> None: + if not self.control or not self.role: + raise InvalidArgument("a required role must name a control and a role") + + def to_dict(self) -> dict[str, str]: + return {"control": self.control, "role": self.role} + + @classmethod + def from_dict(cls, document: Mapping[str, Any]) -> RequiredRole: + return cls(control=str(document["control"]), role=str(document["role"])) @dataclass(frozen=True) @@ -118,7 +155,10 @@ def __post_init__(self) -> None: # arrives from a JSON column: mypy is right that a `tuple[str, ...]` cannot be a `str`, # and a corrupted row is exactly the case where the declaration is not true. given: object = self.entitled - if isinstance(given, str | bytes) or not isinstance(given, Iterable): + # `list | tuple` and not `Iterable`: a JSON object round-trips to a `dict`, whose + # iteration yields its keys, so `{"card-data-handling": 0}` in a tampered column read + # as an entitlement. Nothing this package writes produces one. + if not isinstance(given, list | tuple): raise InvalidArgument( f"a verified approver's 'entitled' must be a list of control ids, got " f"{type(given).__name__}" @@ -151,7 +191,10 @@ def from_dict(cls, document: Mapping[str, Any]) -> VerifiedApprover: user=document.get("user"), issuer=document.get("issuer"), granted_at=datetime.fromisoformat(str(document["granted_at"])), - entitled=tuple(document.get("entitled") or ()), + # Raw, never `tuple(...)`: `__post_init__` is the guard, and wrapping the value + # here pre-empts it. `tuple("c1")` is three single-character control ids that + # entitle nothing, and each one passes the "non-empty string" check below it. + entitled=document.get("entitled") or (), ) @@ -167,9 +210,10 @@ class ApproverIdentity: reads what a proxy set for the agent, and a deployment where one object answers both doors is one where the agent's own token can grant the agent's own approvals. - `roles_claim` names the claim an issuer puts roles in. It is item 3's, and nothing reads it - yet; it lives here because a provider without it cannot answer §3, so a deployment that sets - one and forgets the other would have a silent half-check. + `roles_claim` names the claim this issuer puts roles in (§3.4). It lives here because it is + a property of the issuer and not of any one surface: the operator MCP server reads it where + `--approver-roles-claim` does not override it, and a surface that reads neither holds no + roles and so satisfies no control that names one. """ provider: IdentityProvider @@ -199,6 +243,105 @@ def resolve(self, context: IdentityContext) -> Principal | None: return self.provider.resolve(context) +def count_grant( + record: ApprovalRecord, verified: VerifiedApprover | None, now: datetime +) -> tuple[tuple[VerifiedApprover, ...], bool]: + """What a grant makes of a record: its approvers afterwards, and whether N is reached. + + **One implementation, so three stores cannot come to count differently** (`v0.1 §4.2`'s + argument for `check_consumable`, applied to the other transition). + + - An unverified grant records no approver and reaches nothing: a store that cannot say who + answered has not collected one of N (§2.7, §4.5). + - A second grant from the same resolved principal **updates that entry and does not count** + (§4.2, G19): counted once is the requirement, and rejecting the answer would make a human + think it was lost. + - `granted` is reached when the distinct verified approvers reach the threshold the request + pinned, and never before. + """ + required = max(1, record.request.approvals_required) + if verified is None: + # **At N=1 an unverified answer still grants, and that is 0.7.0 unchanged**: a deployment + # that verifies nobody is the one R1 promises is untouched, and the row it produces is + # what `Control` refuses at consumption with `approver_unverified` where an approver + # identity *is* configured (§2.7). Making it `pending` instead would leave that + # deployment unable to approve anything and would take G18's shape with it. + # + # Above one it advances nothing, which is §4.5: a store that records no approver never + # reaches N and never behaves as N=1. A threshold above one already requires an approver + # identity (§4.2), so every legitimate answer there is a verified one. + return record.approvers, required == 1 + # **The requester's own yes is counted here and refused at consumption**, which is not what + # a first draft of §4.2 said, and the correction is recorded in §14.4. Excluding it from the + # count looks stricter and is weaker: at N=1 the record would never reach `granted`, so the + # consumption refusal would be `pending`, and **G18 would never fire** on the deployment + # shape it was written for. G18 is shipped, graded by `verify`, and says a self-approval is + # refused as one. + # + # So the store counts every verified approver, and the three "does not count" rules are all + # refusals at consumption: §4.1's requester, §3.6's entitlement and §2.7's verification. One + # rule, in one place, and the store stays a store. + kept = [approver for approver in record.approvers if approver.principal != verified.principal] + kept.append(replace(verified, granted_at=now)) + approvers = tuple(kept) + return approvers, len(approvers) >= required + + +def roles_held(principal: Principal, roles_claim: str | None) -> frozenset[str]: + """The roles this principal holds, per SPEC-v0.8 §3.4's rules, one per line. + + Every rule here is a refusal to invent a role, and the failure they exist to prevent is a + **silently** unentitled approver: a claim the issuer sent in a shape nothing carries, read as + absent, and an approval refused for a reason nobody can act on. + + - no `roles_claim` configured: nothing. A deployment naming roles in its policy and no claim + to read them from has configured half a check, and half a check fails closed. + - the claim is absent: nothing. A missing claim is a statement about a person, and the kernel + does not invent one. + - a string: one role, matched byte for byte later. No folding, no trimming, no pattern. + - a tuple of strings: that set, each matched byte for byte. + - an int or a bool, which is everything else a `Principal` can carry: nothing, never coerced + and never stringified. `True` is not the role `"True"`. + """ + if not roles_claim: + return frozenset() + value = principal.claims.get(roles_claim) + if isinstance(value, str): + return frozenset({value}) if value else frozenset() + if isinstance(value, tuple): + return frozenset(item for item in value if item) + if value is not None: + # SPEC-v0.8 §3.4's failure mode by name: the issuer sent the claim, in a shape that + # carries no role, and every rule above reads it as absent. Silence here is an approval + # refused for a reason nobody can act on. Not a refusal, because `None` is the honest + # answer to "which roles does this principal hold"; the refusal is the caller's. + _LOG.warning( + "the claim %r carries %s, which names no role: this principal holds none, and an " + "approval it gives is refused by any control that requires one (SPEC-v0.8 §3.4)", + roles_claim, + type(value).__name__, + ) + return frozenset() + + +def entitled_controls(required: Sequence[RequiredRole], held: frozenset[str]) -> tuple[str, ...]: + """The control ids `held` satisfies, in the order they were required (SPEC-v0.8 §3.6).""" + return tuple(role.control for role in required if role.role in held) + + +def unsatisfied(required: Sequence[RequiredRole], entitled: Sequence[str]) -> RequiredRole | None: + """The first required role this entitlement does not cover, or `None` (SPEC-v0.8 §3.6). + + **Every one must be satisfied, not any.** Any-of lets the weakest control in the set decide + who may answer, and makes adding a control to a rule a way of *widening* who may approve it. + """ + covered = set(entitled) + for role in required: + if role.control not in covered: + return role + return None + + #: SPEC-v0.8 §2.5: how a surface that resolved an approver hands that principal to the store #: call that records it. **Package-internal on purpose** (§2.5.1): a public one would be an #: unauthenticated way to assert a verified approver, which is `trust_approver` spelled as a @@ -213,6 +356,12 @@ def resolve(self, context: IdentityContext) -> Principal | None: @contextmanager def _granting_principal(principal: Principal, *, entitled: Iterable[str] = ()) -> Iterator[None]: """Record `principal` as the verified approver of any grant made inside this block.""" + if isinstance(entitled, str | bytes): + # `Iterable[str]` admits a `str`, and `tuple("c1")` is three control ids named 'c', + # '1' — each a non-empty string, so every check downstream passes on nonsense. + raise InvalidArgument( + "entitled must be a sequence of control ids, not a string; pass ('c1',) or ['c1']" + ) token = _GRANTING_PRINCIPAL.set((principal, tuple(entitled))) try: yield @@ -271,6 +420,18 @@ class ApprovalRequest: #: the human's decision and the effect; a change landing after the comparison and before #: the reservation is not refused (§6.7). precondition_fingerprint: str | None = None + #: SPEC-v0.8 §3.3 — the roles the controls cited by the evaluation that sent this action to + #: approval required, pinned **at request time**, for `v0.6 §7.1`'s reason: the approval binds + #: to what the human was shown, the store has no policy, and a command a human answers with + #: must not fail because a policy file two hosts away became malformed. Where the policy moved + #: in between, the roles the human was asked under are the ones that bind, and the receipt's + #: own `policy_hash` records that it moved. + required_roles: tuple[RequiredRole, ...] = () + #: SPEC-v0.8 §4.2 — how many distinct verified principals must answer, pinned at request time + #: like the roles and for the same reason, and because **this is what lets the store enforce + #: it**: the store has no policy, and a store that had to ask one what N is would be a store + #: that loads policy files. + approvals_required: int = 1 def __post_init__(self) -> None: if not self.request_id: @@ -284,6 +445,19 @@ def __post_init__(self) -> None: f"approval {self.request_id} expires at or before it was created; " "a request nobody can answer is not a request" ) + # SPEC-v0.8 §4.2, §12. Refused here rather than coerced: `count_grant` clamped a + # corrupt value with `max(1, ...)` and the SQLite read turned a `0` into `1` with + # `or 1`, so a row whose threshold had been tampered to nothing read back as an + # ordinary single-approval request and nobody could tell. `approvers` and + # `required_roles` both raise out of the store read on a corrupted column; this is + # the third field and it was the one that did not. Policy load refuses these values + # too, so the only way here is a row a store did not write. + threshold: object = self.approvals_required + if isinstance(threshold, bool) or not isinstance(threshold, int) or threshold < 1: + raise InvalidArgument( + f"approval {self.request_id} requires {threshold!r} approvals; the threshold " + "is an integer of at least 1 (SPEC-v0.8 §4.2)" + ) @dataclass(frozen=True) @@ -471,8 +645,18 @@ def get_approval(self, approval_id: str) -> ApprovalRecord | None: """The stored record, or `None` if there is no such approval.""" ... - def grant_approval(self, approval_id: str, approver: str) -> Approval: - """Move `pending → granted`. Anything else raises `ApprovalMismatch`.""" + def grant_approval(self, approval_id: str, approver: str) -> Approval | None: + """Record one answer. `Approval` where it reached the threshold, `None` where it did not. + + SPEC-v0.8 §4.4: `None` means **recorded and still pending**, which is M-of-N below N. A + public API change and not a new method (`v0.6 §9.2`): every implementation that only ever + grants at N=1 returns an `Approval` every time, as it does today. + + **`None` here is not `None` from `ApprovalProvider.wait`**, which `v0.1 §4.3` fixes as + "answered, no". A provider that returned this straight through would report a partial + grant as a denial, and `@protect(wait=True)` would raise `ActionDenied` for a request a + second human is still answering. + """ ... def deny_approval(self, approval_id: str, approver: str) -> None: @@ -547,6 +731,28 @@ def policy_in_force(policy_hash: str | None) -> Iterator[None]: _PRECONDITION_SCHEMA: Final = "ctrlrun.precondition/v1" +#: SPEC-v0.8 §3.3 — the third traveller on `_POLICY_AT_REQUEST`'s route, and for the same reason: +#: `ApprovalProvider.request(action, ttl)` is frozen by `v0.1 §9.1`, so a provider cannot be handed +#: a policy, and `Control` is the only object holding both. +_REQUIRED_ROLES: ContextVar[tuple[RequiredRole, ...]] = ContextVar( + "ctrlrun_required_roles", default=() +) + +#: SPEC-v0.8 §4.2 — the threshold, on the same route and for the same reason. +_APPROVALS_REQUIRED: ContextVar[int] = ContextVar("ctrlrun_approvals_required", default=1) + + +@contextmanager +def _required_roles(roles: tuple[RequiredRole, ...], required: int = 1) -> Iterator[None]: + """Record `roles` and the threshold on any request built inside this block (§3.3, §4.2).""" + tokens = (_REQUIRED_ROLES.set(roles), _APPROVALS_REQUIRED.set(required)) + try: + yield + finally: + _REQUIRED_ROLES.reset(tokens[0]) + _APPROVALS_REQUIRED.reset(tokens[1]) + + @contextmanager def _precondition_at_request(fingerprint: str | None) -> Iterator[None]: """Record `fingerprint` on any request built inside this block (SPEC-v0.7 §6.2).""" @@ -584,6 +790,8 @@ def build_request(action: Action, ttl: timedelta, now: datetime) -> ApprovalRequ action=action, policy_hash=_POLICY_AT_REQUEST.get(), precondition_fingerprint=_PRECONDITION_AT_REQUEST.get(), + required_roles=_REQUIRED_ROLES.get(), + approvals_required=_APPROVALS_REQUIRED.get(), created_at=now, expires_at=now + ttl, ) @@ -707,7 +915,15 @@ def wait(self, request_id: str, timeout: timedelta | None = None) -> Approval | self._step += 1 self.polls += 1 if outcome is ScriptedOutcome.GRANT: - return self._store.grant_approval(request_id, self._approver) + granted = self._store.grant_approval(request_id, self._approver) + # SPEC-v0.8 §4.4: `None` from the store is "recorded, short of N"; `None` from + # `wait` is `v0.1 §4.3`'s "answered, no". Returning it straight through would + # report a partial grant as a **denial**, and `@protect(wait=True)` would raise + # `ActionDenied` for a request a second human is still answering. So this keeps + # polling, and an exhausted script times out as it does for an unanswered one. + if granted is not None: + return granted + continue if outcome is ScriptedOutcome.DENY: self._store.deny_approval(request_id, self._approver) return None diff --git a/src/ctrlrun/authority.py b/src/ctrlrun/authority.py index 6458adb..9eadd59 100644 --- a/src/ctrlrun/authority.py +++ b/src/ctrlrun/authority.py @@ -30,7 +30,7 @@ import secrets from collections.abc import Iterable, Mapping, Sequence from dataclasses import dataclass, field, replace -from datetime import datetime +from datetime import datetime, timedelta from types import MappingProxyType from typing import Any, Final, Literal @@ -97,7 +97,19 @@ #: §5.2, §5.7 — `api` for `Control.delegate`, where `by` came from wherever the application's #: identity came from, and `cli` for `ctrlrun delegate`, where it came from a shell. A reader of #: the evidence can tell an act from an assertion, which is the whole reason the field exists. -_CREATED_VIA: Final[Mapping[str, Literal["api", "cli"]]] = {"api": "api", "cli": "cli"} +#: SPEC-v0.3 §5.3, SPEC-v0.8 §5.3, §11.1. The vocabulary is **closed** and a record carrying a +#: value outside it is unreadable, which makes `_candidates` raise and answers +#: `authority_unreadable` for **every action in the deployment**. So the third value is a public +#: change that moves the `Literal`, this mapping and every reader together, in one commit; a +#: deployment that wrote `break-glass` rows under a reader that knew two values would deny +#: everything, which is fail-closed and useless. +CreatedVia = Literal["api", "cli", "break-glass"] + +_CREATED_VIA: Final[Mapping[str, CreatedVia]] = { + "api": "api", + "cli": "cli", + "break-glass": "break-glass", +} #: SPEC-v0.3 §5.3 — the creation-time vocabulary. Disjoint from the evaluation reasons above, #: and never used interchangeably with them: a test asserting `authority_escalation` on a @@ -125,7 +137,14 @@ } _AUTHORITY_KEY: Final = "authority" -_AUTHORITY_KEYS: Final = frozenset({"max_delegation_depth", "grants"}) +_AUTHORITY_KEYS: Final = frozenset({"max_delegation_depth", "grants", "break_glass"}) + +#: SPEC-v0.8 §5.2 — an envelope's keys: a grant's, minus the two it may not carry, plus the two +#: that make it an envelope. `delegable:` and `expires_at:` are **named refusals** rather than +#: silent omissions, because the grant parser accepts both and an operator writing +#: `delegable: true` would then owe an `expires_at` an envelope does not have. +_ENVELOPE_ONLY_KEYS: Final = frozenset({"max_ttl", "controls"}) +_ENVELOPE_REFUSED_KEYS: Final = frozenset({"delegable", "expires_at"}) _GRANT_KEYS: Final = frozenset( { "id", @@ -428,7 +447,7 @@ class Delegation: depth: int grant: Grant created_by: Principal - created_via: Literal["api", "cli"] + created_via: CreatedVia created_at: datetime revoked_at: datetime | None = None revoked_by: str | None = None @@ -708,6 +727,12 @@ class _Walk: missing_parent_id: str | None = None cycle_at: str | None = None depth_exceeded: int | None = None + #: SPEC-v0.8 §5.2 point 4 — did the root come from `Authority.envelopes`? `delegable` is + #: read at three sites that each decide something, and an envelope carries no such key, + #: so an envelope ancestor **counts as** delegable at all three. "Counts as", and never a + #: `delegable=True` written onto the parsed grant: the rendered value must come from the + #: document, or the policy hash would move because of a runtime rule (T332). + root_is_envelope: bool = False @property def complete(self) -> bool: @@ -720,6 +745,19 @@ def ancestors(self) -> tuple[Grant, ...]: above.extend(node.grant for node in reversed(self.nodes[1:])) return tuple(above) + @property + def undelegable_ancestor(self) -> bool: + """Is any ancestor one that may not be delegated beneath? (SPEC-v0.8 §5.2 point 4.) + + The root is exempt where it is an envelope, which is the whole of the rule: an envelope + exists only to be a parent, and what bounds the population it can reach is `max_ttl` + rather than `delegable`. Every other ancestor is read exactly as before. + """ + above = self.ancestors + if self.root_is_envelope and above: + above = above[1:] + return any(not ancestor.delegable for ancestor in above) + @property def ancestor_ids(self) -> tuple[str, ...]: above: list[str] = [] if self.root_id is None else [self.root_id] @@ -748,6 +786,36 @@ def _by_grant_id(result: AuthorityResult) -> str: return result.grant_id or "" +@dataclass(frozen=True) +class BreakGlassEnvelope: + """The widest authority an incident may reach, declared in advance (SPEC-v0.8 §5.2). + + An envelope is a `Grant` in every respect the parser knows, plus the longest expiry a grant + beneath it may carry and the controls whose `approver_role` gates who may open one. It is + **not** in `Authority.grants`, so `_candidates` cannot return it and it decides no action: + that is T330 true by construction rather than by a filter somebody can delete. + + It carries no `delegable` and no `expires_at`. `delegable` is what ordinarily says a grant + may be delegated beneath, and the reason behind that rule -- that nothing else bounds the + population a delegable grant can reach -- is met here by `max_ttl`, which bounds every child + in time and is required. An envelope exists only to be a parent. + """ + + grant: Grant + max_ttl: timedelta + controls: tuple[str, ...] = () + + def __post_init__(self) -> None: + if self.max_ttl <= timedelta(0): + raise InvalidArgument( + f"break-glass envelope {self.grant.id!r}: 'max_ttl' must be a positive duration" + ) + + @property + def id(self) -> str: + return self.grant.id + + class Authority: """The `authority:` section, loaded and evaluable (SPEC-v0.3 §4). @@ -761,17 +829,27 @@ def __init__( *, max_delegation_depth: int = DEFAULT_MAX_DELEGATION_DEPTH, source: str = "", + envelopes: Mapping[str, BreakGlassEnvelope] | None = None, ) -> None: if max_delegation_depth < 0: raise InvalidArgument("max_delegation_depth must be a non-negative int") self._grants = MappingProxyType(dict(grants)) self._max_delegation_depth = max_delegation_depth self._source = source + # SPEC-v0.8 §5.2: a **separate mapping**, and that is the whole design. `_candidates` + # returns every entry of `_grants` unconditionally, so an envelope living there would + # decide actions, which is the opposite of what it is for. + self._envelopes = MappingProxyType(dict(envelopes or {})) @property def grants(self) -> Mapping[str, Grant]: return self._grants + @property + def envelopes(self) -> Mapping[str, BreakGlassEnvelope]: + """The break-glass envelopes (SPEC-v0.8 §5.2). Never consulted by `evaluate`.""" + return self._envelopes + @property def max_delegation_depth(self) -> int: return self._max_delegation_depth @@ -874,6 +952,105 @@ def evaluate(self, action: Action, *, now: datetime, store: StateStore) -> Autho # --- delegation (SPEC-v0.3 §5) ----------------------------------------------------- + def plan_break_glass( + self, + envelope_id: str, + grant: Grant, + *, + by: Principal, + store: StateStore, + now: datetime, + ) -> Delegation: + """The delegation `Control.break_glass` would write, or a refusal (SPEC-v0.8 §5.3). + + `plan_delegation` with two differences, and both are §5.3.1's: + + - **`envelope_id` resolves only in `envelopes`.** An id naming an ordinary grant is + refused by name, whether or not it also names nothing here. Without that, + `--envelope ` would reach a path where rule 4 is skipped for a + grant that has no `controls:` to gate it instead, which is strictly weaker than what + `ctrlrun delegate` requires beneath the same grant (T334b). + - **Rule 4 does not apply.** An envelope's subject names the agents a break-glass grant + may be *for*; the principal opening one is a human. What gates the opener is the + envelope's `controls:`, checked by `Control` where the approver identity is. + + Every other rule of `v0.3 §5.3` applies unchanged: unknown parent, expiry, containment, + depth. And one this adds: a grant beneath an envelope **must** carry an expiry, and one + beyond `max_ttl` is refused. + """ + envelope = self._envelopes.get(envelope_id) + if envelope is None: + named = ( + " it names a grant, and a grant is not an envelope: opening one beneath it " + "would skip the subject check that 'ctrlrun delegate' applies there, and a " + "grant has no 'controls:' to gate the opener instead" + if envelope_id in self._grants + else " no envelope of that name is declared" + ) + raise AuthorityEscalation( + f"no break-glass envelope {envelope_id!r};{named}. An envelope is declared " + "under 'authority: break_glass:' and names the controls that gate who may open " + "it (SPEC-v0.8 §5.2, §5.3.1)", + reason=UNKNOWN_PARENT, + parent_id=envelope_id, + ) + if by.expires_at is not None and now > by.expires_at: + raise IdentityError( + f"the opening principal's credential expired at {by.expires_at}; an expired " + "credential may not create authority (SPEC-v0.3 §5.3 rule 0)" + ) + if grant.id: + raise InvalidArgument( + f"the grant passed to break_glass() carries id {grant.id!r}; a delegation's id " + "is assigned, not chosen (SPEC-v0.3 §5.2)" + ) + if grant.expires_at is None: + # §5.3's one added rule. An ordinary delegation may carry no expiry; a break-glass + # grant that outlives the incident is the thing this section exists to prevent. + raise AuthorityEscalation( + f"a grant opened beneath {envelope_id!r} must carry 'expires_at'; break-glass " + "authority that outlives the incident is what an envelope exists to prevent " + "(SPEC-v0.8 §5.3)", + reason=CONTAINMENT, + parent_id=envelope_id, + dimension="expires_at", + ) + if grant.expires_at > now + envelope.max_ttl: + raise AuthorityEscalation( + f"a grant opened beneath {envelope_id!r} expires at {grant.expires_at}, beyond " + f"its max_ttl of {envelope.max_ttl} from now ({now + envelope.max_ttl})", + reason=CONTAINMENT, + parent_id=envelope_id, + dimension="expires_at", + ) + depth = 1 + if depth > self._max_delegation_depth: + raise AuthorityEscalation( + f"a delegation of {envelope_id!r} would be at depth {depth}, beyond " + f"max_delegation_depth {self._max_delegation_depth}", + reason=MAX_DEPTH, + parent_id=envelope_id, + ) + dimension = contained_dimension(envelope.grant, grant) + if dimension is not None: + raise AuthorityEscalation( + f"the break-glass grant is not contained in {envelope_id!r} on {dimension!r}; " + "omission never means unlimited (SPEC-v0.3 §5.4)", + reason=CONTAINMENT, + parent_id=envelope_id, + dimension=dimension, + ) + delegation_id = new_delegation_id() + return Delegation( + delegation_id=delegation_id, + parent_id=envelope_id, + depth=depth, + grant=replace(grant, id=delegation_id), + created_by=by, + created_via="break-glass", + created_at=now, + ) + def plan_delegation( self, parent_id: str, grant: Grant, *, by: Principal, store: StateStore, now: datetime ) -> Delegation: @@ -960,6 +1137,27 @@ def _parent_for_creation( """§5.3 rules 1 to 3, and the walked depth rule 5 needs.""" # §5.2 — a root grant wins any collision: an id is resolved against the document first # and the store second. + if parent_id in self._envelopes: + # **SPEC-v0.8 §5.3.1, the direction an independent review found open.** §5.3.1 + # guards `break-glass --envelope `; nothing guarded `delegate --parent + # `, which is strictly worse. An earlier build of this method + # returned the envelope's grant here, and `plan_delegation` then created authority + # beneath it with **no expiry requirement, no `max_ttl`, no entitlement check and + # no `created_via` saying what it was** -- a permanent break-glass grant, opened + # from a shell by anyone whose `--as` matched the envelope's subject, which is a + # pattern over the agents the grant may be *for*. + # + # `plan_break_glass` resolves envelopes itself and applies §5.3's rules. This path + # refuses them by name. The delegable-exemption of §5.2 point 4 lives at the two + # read sites that walk an existing chain, where it cannot create anything. + raise AuthorityEscalation( + f"{parent_id!r} is a break-glass envelope, not a grant. Authority beneath an " + "envelope is opened with 'ctrlrun break-glass --envelope', which requires an " + "expiry inside its max_ttl and checks the opener against the controls that " + "gate it; delegating beneath one directly would skip both (SPEC-v0.8 §5.3.1)", + reason=UNKNOWN_PARENT, + parent_id=parent_id, + ) root = self._grants.get(parent_id) if root is not None: if not root.delegable: @@ -1013,8 +1211,13 @@ def _parent_for_creation( # new delegations appearing anywhere beneath it rather than only one level down. invalid = walk.missing_parent_id is not None or walk.cycle_at is not None if not invalid: - invalid = any(node.is_revoked for node in walk.nodes[1:]) or any( - ancestor.is_expired(now) or not ancestor.delegable for ancestor in walk.ancestors + # SPEC-v0.8 §5.2 point 4, second site: the same exemption, or a delegation + # **beneath** a break-glass grant is refused `parent_not_valid` and §5.4's + # attenuation bullet cannot hold (T337). + invalid = ( + any(node.is_revoked for node in walk.nodes[1:]) + or any(ancestor.is_expired(now) for ancestor in walk.ancestors) + or walk.undelegable_ancestor ) if invalid: raise AuthorityEscalation( @@ -1050,6 +1253,18 @@ def _walk(self, leaf: Delegation, *, store: StateStore) -> _Walk: root = self._grants.get(parent_id) if root is not None: return _Walk(tuple(nodes), root=root, root_id=parent_id) + # SPEC-v0.8 §5.2 point 2: a break-glass delegation names an **envelope** as its + # parent, and the walk resolved roots out of `_grants` alone. Without this its + # chain has no root at all and every action under it is refused + # `authority_escalation` with a missing parent. + envelope = self._envelopes.get(parent_id) + if envelope is not None: + return _Walk( + tuple(nodes), + root=envelope.grant, + root_id=parent_id, + root_is_envelope=True, + ) if parent_id in seen: return _Walk(tuple(nodes), cycle_at=parent_id) record = store.get_delegation(parent_id) @@ -1099,6 +1314,24 @@ def _check_chain(self, leaf: Delegation, *, store: StateStore, now: datetime) -> return _ChainCheck( depth, AuthorityResult(False, AUTHORITY_ESCALATION, dimension=dimension) ) + # SPEC-v0.8 §5.2, and `v0.3 §5.6`'s stated purpose: **a narrowed root narrows + # everything beneath it.** `max_ttl` was checked once, at creation, and is not a §5.4 + # containment row, so an operator who narrowed an envelope while an incident was still + # running narrowed nothing: a four-hour grant opened under `PT4H` kept running under + # `PT15M`. Every other envelope dimension already narrows live grants here, because the + # envelope is the chain's root parent; this is the one that did not, and it is the one + # bound an operator reaches for first. + envelope = None if walk.root_id is None else self._envelopes.get(walk.root_id) + if envelope is not None and walk.nodes: + opened = walk.nodes[-1] + if ( + opened.grant.expires_at is None + or opened.grant.expires_at > opened.created_at + envelope.max_ttl + ): + return _ChainCheck( + depth, + AuthorityResult(False, AUTHORITY_ESCALATION, dimension="max_ttl"), + ) if walk.cycle_at is not None: # A chain that loops is a store somebody has edited by hand, and it belongs with # the other unreadable-record cases rather than with the ordinary escalations. @@ -1116,10 +1349,15 @@ def _check_chain(self, leaf: Delegation, *, store: StateStore, now: datetime) -> return _ChainCheck( depth, AuthorityResult(False, AUTHORITY_ESCALATION, depth_exceeded=depth) ) - if any(not ancestor.delegable for ancestor in walk.ancestors): + if walk.undelegable_ancestor: # Rule 6 exists because `delegable` is not a §5.4 row and rule 4 would therefore # never see it. An operator setting `delegable: false` on a root grant is shutting # down a chain they believe is compromised. + # + # SPEC-v0.8 §5.2 point 4, third site: this runs on **every evaluation**, so without + # the envelope exemption a break-glass grant is created successfully and then + # authorises nothing, refused `authority_escalation` with no dimension named, which + # is the least diagnosable refusal in this file (T326b). return _ChainCheck(depth, AuthorityResult(False, AUTHORITY_ESCALATION)) return _ChainCheck(depth, None) @@ -1127,6 +1365,15 @@ def _check_chain(self, leaf: Delegation, *, store: StateStore, now: datetime) -> # --- loading (SPEC-v0.3 §4.1, §4.2) ---------------------------------------------------- +def _canonical_envelope(envelope: BreakGlassEnvelope) -> dict[str, PlainValue]: + """One envelope in the shape `policy_hash` is taken over (SPEC-v0.8 §5.2).""" + rendered = _canonical_grant(envelope.grant) + fields: dict[str, PlainValue] = dict(rendered) if isinstance(rendered, dict) else {} + fields["max_ttl"] = int(envelope.max_ttl.total_seconds()) + fields["controls"] = list(envelope.controls) + return fields + + def canonical_grants(authority: Authority | None) -> PlainValue: """This authority's grants, in the shape `policy_hash` is taken over (SPEC-v0.6 §7.1). @@ -1156,6 +1403,20 @@ def canonical_grants(authority: Authority | None) -> PlainValue: "grants": { grant_id: _canonical_grant(grant) for grant_id, grant in authority.grants.items() }, + # SPEC-v0.8 §5.2, §11.1. **The envelope is the widest authority an incident can reach**, + # and the argument for declaring it in the policy is that it was evidenced before the + # incident by a document somebody reviewed. That argument is only true if widening it + # moves the hash, so `max_ttl` is rendered here with the rest. + # + # Rendered through `_canonical_grant` like any grant, whose closed field list always + # emits `delegable`. An envelope carries no such key, so it hashes `delegable: false` -- + # the parser default every grant omitting the key already hashes as -- and the read-site + # rule of §5.2 point 4 does not touch the rendered value. That is deliberate: the hash + # is a statement about the document, never about a runtime decision (T332). + "break_glass": { + envelope_id: _canonical_envelope(envelope) + for envelope_id, envelope in authority.envelopes.items() + }, } @@ -1212,10 +1473,10 @@ def _optional_from_yaml( # as well as in `Policy`, because §8.3's `--authority` document is never read by the # policy loader at all. require_v3(document, str(schema), source) - return _from_section(document[_AUTHORITY_KEY], source) + return _from_section(document[_AUTHORITY_KEY], source, standalone=standalone) -def _from_section(section: object, source: str) -> Authority: +def _from_section(section: object, source: str, *, standalone: bool = False) -> Authority: where = f"{source}: authority" if not isinstance(section, Mapping): raise PolicyError( @@ -1250,7 +1511,110 @@ def _from_section(section: object, source: str) -> Authority: "can resolve later" ) grants[grant.id] = grant - return Authority(grants, max_delegation_depth=depth, source=source) + if standalone and "break_glass" in section: + # SPEC-v0.8 §5.2. Not "allowed but its citations must resolve": this document shape has + # no control registry to resolve them against, so the only envelope it could express is + # an **ungated** one, and the single deployment unable to state the gate would be the + # one whose break-glass anybody verified could open. + raise PolicyError( + f"{where}: a standalone authority document may not declare 'break_glass'. An " + "envelope names the controls whose approver_role gates who may open it, and this " + "document shape carries no control registry to resolve them against; move the " + "authority section into the policy document, where the registry is (SPEC-v0.8 §5.2)" + ) + envelopes = _parse_envelopes(section.get("break_glass"), grants, where) + return Authority(grants, max_delegation_depth=depth, source=source, envelopes=envelopes) + + +def _parse_envelopes( + block: object, grants: Mapping[str, Grant], where: str +) -> dict[str, BreakGlassEnvelope]: + """SPEC-v0.8 §5.2's `break_glass:` mapping, by id.""" + if block is None: + return {} + if not isinstance(block, Mapping): + raise PolicyError( + f"{where}: 'break_glass' must be a mapping of envelope id to envelope, got " + f"{_type_name(block)}" + ) + envelopes: dict[str, BreakGlassEnvelope] = {} + for identifier, entry in block.items(): + spot = f"{where} break_glass[{identifier!r}]" + if not isinstance(identifier, str) or not identifier: + raise PolicyError(f"{spot}: an envelope id must be a non-empty string") + if identifier in grants: + # §5.2 and §5.3.1: rule 4 is substituted only for a parent that came from + # `break_glass:`, so "which mapping did this id come from" must always have an + # answer. An id in both makes it undecidable, and the resolution that favoured + # `grants:` would silently skip the gate. + raise PolicyError( + f"{spot}: {identifier!r} is declared in both 'grants' and 'break_glass'. " + "Which mapping a parent came from decides whether the envelope's controls " + "gate who may open it (SPEC-v0.8 §5.3.1), so one id cannot be in both" + ) + if not isinstance(entry, Mapping): + raise PolicyError(f"{spot}: an envelope must be a mapping, got {_type_name(entry)}") + for refused in sorted(_ENVELOPE_REFUSED_KEYS): + if refused in entry: + raise PolicyError( + f"{spot}: an envelope may not carry {refused!r}. An envelope exists only " + "to be a parent, so 'delegable' is what it means rather than a key, and " + "what bounds it in time is 'max_ttl', which every grant beneath it obeys " + "(SPEC-v0.8 §5.2)" + ) + _reject_unknown_keys( + entry, (_GRANT_KEYS - _ENVELOPE_REFUSED_KEYS - {"id"}) | _ENVELOPE_ONLY_KEYS, spot + ) + if "max_ttl" not in entry: + raise PolicyError( + f"{spot}: 'max_ttl' is required. It is what bounds every grant opened beneath " + "this envelope in time, and a break-glass grant that outlives the incident is " + "what SPEC-v0.8 §5 exists to prevent" + ) + max_ttl = _parse_duration(entry["max_ttl"], f"{spot}: max_ttl") + controls = entry.get("controls", []) + if not isinstance(controls, list) or not all( + isinstance(item, str) and item for item in controls + ): + raise PolicyError(f"{spot}: 'controls' must be a list of control ids, got {controls!r}") + grant = _parse_grant( + {key: value for key, value in entry.items() if key not in _ENVELOPE_ONLY_KEYS} + | {"id": identifier}, + spot, + ) + try: + envelopes[identifier] = BreakGlassEnvelope( + grant=grant, max_ttl=max_ttl, controls=tuple(controls) + ) + except InvalidArgument as exc: + raise PolicyError(f"{spot}: {exc}") from exc + return envelopes + + +def _parse_duration(value: object, where: str) -> timedelta: + """An ISO-8601 duration, the subset `max_ttl` needs: `PTH`, `PTM`, `PTS`, `PD`. + + Written here rather than taken from a dependency, because the core is stdlib plus `pyyaml` + and `click`, and because a permissive parser would accept a month or a year, which are not + durations a clock can add without a calendar. + """ + if not isinstance(value, str) or not value: + raise PolicyError(f"{where}: must be an ISO-8601 duration string, got {value!r}") + match = _DURATION.fullmatch(value) + if match is None: + raise PolicyError( + f"{where}: {value!r} is not a duration this reader accepts. Write it as 'PT4H', " + "'PT30M', 'PT90S' or 'P2D'; months and years are not durations a clock can add" + ) + days, hours, minutes, seconds = (int(part or 0) for part in match.groups()) + found = timedelta(days=days, hours=hours, minutes=minutes, seconds=seconds) + if found <= timedelta(0): + raise PolicyError(f"{where}: {value!r} is not a positive duration") + return found + + +#: The subset of ISO-8601 above. Anchored, so `P1MT1H` is refused rather than read as an hour. +_DURATION: Final = re.compile(r"P(?:(\d+)D)?(?:T(?:(\d+)H)?(?:(\d+)M)?(?:(\d+)S)?)?") def grant_from_yaml(text: str, *, source: str = "") -> Grant: diff --git a/src/ctrlrun/cli/main.py b/src/ctrlrun/cli/main.py index e77a911..e40590b 100644 --- a/src/ctrlrun/cli/main.py +++ b/src/ctrlrun/cli/main.py @@ -369,6 +369,22 @@ def approve(request_id: str, store_url: str | None) -> None: approval = store.grant_approval(request_id, CLI_APPROVER) except CTRLRunError as exc: raise _fail(exc) from exc + if approval is None: + # SPEC-v0.8 §4.4: recorded, and still short of the threshold the request pinned. No + # `APPROVAL_GRANTED` event, because nothing was granted yet: an event naming a grant that + # did not happen is the false-green shape in the evidence log (`v0.6 §7.2.3`'s argument). + after = store.get_approval(request_id) + recorded = 0 if after is None else len(after.approvers) + needed = 1 if after is None else after.request.approvals_required + click.echo(f"recorded {request_id}: {recorded} of {needed} approvals") + # §2.6: a CLI grant records no verified approver, so under M-of-N it never counts. Said + # here rather than left for an operator to infer from a number that does not move. + if recorded < needed: + click.echo( + "this answer carries no verified approver, so it will not count where the " + "deployment names an approver identity (SPEC-v0.8 §2.6)" + ) + return if record is not None: store.append_event( _event( @@ -930,6 +946,52 @@ def delegate( click.echo(f"revoke it with: ctrlrun revoke {created.delegation_id}") +@main.command("break-glass") +@click.option( + "--envelope", + required=True, + help="The break-glass envelope declared under 'authority: break_glass:'.", +) +@click.option( + "--file", + "grant_file", + required=True, + type=click.Path(exists=True, dir_okay=False, path_type=Path), + help="A one-grant YAML document, as --file takes for delegate. 'expires_at' is required.", +) +@click.option("--reason", default="", help="Free text recorded on the DELEGATION_CREATED event.") +@click.option("--json", "as_json", is_flag=True, help="Emit one JSON object instead.") +@STORE_URL_OPTION +def break_glass( + envelope: str, grant_file: Path, reason: str, as_json: bool, store_url: str | None +) -> None: + """Open a break-glass grant beneath a declared envelope. + + An incident needs authority nobody was granted in advance, and the wrong answer is a flag: + a flag leaves no record, expires never and cannot be revoked. What this creates is an + ordinary delegation, bounded by the envelope on every dimension, expiring within its + 'max_ttl', revocable, attenuable, and named on the receipt of every action taken under it. + + **There is no --as.** Whoever opens one is the principal the deployment's approver identity + resolves, and a deployment that names none cannot open one at all: an assertion typed at a + shell is exactly what break-glass must not accept (SPEC-v0.8 §5.3.1). + """ + try: + control = _control_on(store_url) + grant = grant_from_yaml(grant_file.read_text(encoding="utf-8"), source=str(grant_file)) + created = control._break_glass(envelope, grant, reason=reason) + except AuthorityEscalation as exc: + raise click.ClickException(f"{exc.reason}: {exc}") from exc + except CTRLRunError as exc: + raise _fail(exc) from exc + if as_json: + click.echo(json.dumps(_delegation_dict(created), ensure_ascii=False)) + return + click.echo(f"opened {created.delegation_id} beneath {created.parent_id}") + click.echo(f"expires {created.grant.expires_at}") + click.echo(f"revoke it with: ctrlrun revoke {created.delegation_id}") + + @main.command() @click.argument("delegation_id", required=False) @click.option("--by", "by", default=CLI_APPROVER, show_default=True, help="Who revoked it.") @@ -1179,6 +1241,14 @@ def _delegation_dict(delegation: Delegation) -> dict[str, Any]: @click.option("--identity-jwt-leeway", type=float, default=60.0, show_default=True) @click.option("--identity-jwt-jwks-min-refresh", type=float, default=30.0, show_default=True) @click.option("--identity-jwt-http-timeout", type=float, default=5.0, show_default=True) +@click.option( + "--approver-roles-claim", + default=None, + help=( + "Which verified claim carries this issuer's roles, for the approver entitlement of " + "SPEC-v0.8 §3. Without it no role can be read, so any cited control naming one refuses." + ), +) @STORE_URL_OPTION def mcp_operator( listen: str, @@ -1198,6 +1268,7 @@ def mcp_operator( identity_jwt_audience: str | None, identity_jwt_token_type: str | None, identity_jwt_header: str, + approver_roles_claim: str | None, identity_jwt_agent_claim: str, identity_jwt_user_claim: str | None, identity_jwt_claims: tuple[str, ...], @@ -1246,6 +1317,7 @@ def mcp_operator( identity_jwt_leeway=identity_jwt_leeway, identity_jwt_jwks_min_refresh=identity_jwt_jwks_min_refresh, identity_jwt_http_timeout=identity_jwt_http_timeout, + approver_roles_claim=approver_roles_claim, ) except (ValueError, CTRLRunError) as exc: raise click.ClickException(str(exc)) from exc diff --git a/src/ctrlrun/conformance/store/suites.py b/src/ctrlrun/conformance/store/suites.py index d24bbe3..1776457 100644 --- a/src/ctrlrun/conformance/store/suites.py +++ b/src/ctrlrun/conformance/store/suites.py @@ -29,7 +29,13 @@ from typing import Any from ...action import Action, Principal -from ...approval import ApprovalStatus, build_request +from ...approval import ( + ApprovalStatus, + RequiredRole, + _granting_principal, + _required_roles, + build_request, +) from ...effect import COMMITTED_EFFECT, IN_PROGRESS_EFFECT, EffectState from ...errors import ( AmbiguousEffect, @@ -1020,6 +1026,169 @@ def continuation_taken_once_cross_process( return passed("taken-once-cross-process", title) +@case("verified-approver", "the approver columns round-trip and one principal counts once") +def approval_verified_approver(backend: StoreBackend, processes: int = CONTENDERS) -> CaseResult: + """SPEC-v0.8 §2.5, §3.6, §4.2: what a store must do with the three columns v0.8 added. + + Three things a store can each get wrong on its own, and every one of them is an approval + nobody gave: + + - the roles the request pinned come back as they were written, so the kernel compares against + what was in force at the request and not at the answer; + - a verified approver is recorded with the entitlement the granting surface computed; + - **the same principal answering twice is one approver.** A store that appended would reach a + threshold of two on one person's yes, which is the defect `approvals_required` exists to + prevent, and the second grant is not an error: a human whose answer was rejected believes + it was lost. + + A store that ignores the columns entirely is refused by `Control` at consumption rather than + silently trusted (§2.4), which is the fail-closed direction; this case says so out loud so a + third-party store learns it here and not from a deployment. + """ + title = approval_verified_approver.title + store = backend.open() + action = an_action(payment_id="txn_verified") + roles = (RequiredRole(control="card-data-handling", role="payments-owner"),) + with _required_roles(roles, 2): + request = build_request(action, timedelta(minutes=15), store_now(store)) + store.put_approval_request(request) + + pinned = store.get_approval(request.request_id) + if pinned is None or pinned.request.required_roles != roles: + return failed( + "verified-approver", + title, + "the roles the request pinned did not come back: wrote " + f"{roles}, read {None if pinned is None else pinned.request.required_roles}", + ) + if pinned.request.approvals_required != 2: + return failed( + "verified-approver", + title, + f"approvals_required came back {pinned.request.approvals_required}, not 2", + ) + + alice = Principal(agent="human:alice", user="alice@example.com") + for door in ("mcp-operator", "cli"): + with _granting_principal(alice, entitled=["card-data-handling"]): + partial_grant = store.grant_approval(request.request_id, f"{door}:alice") + if partial_grant is not None: + return failed( + "verified-approver", + title, + f"the {door} grant produced an Approval at 1 of 2 approvals", + ) + + after = store.get_approval(request.request_id) + if after is None or len(after.approvers) != 1: + return failed( + "verified-approver", + title, + "one principal answering twice is " + f"{0 if after is None else len(after.approvers)} approvers, not 1", + ) + if after.status is not ApprovalStatus.PENDING: + return failed( + "verified-approver", + title, + f"the request is {after.status} after one principal's two answers, not pending", + ) + if after.approvers[0].entitled != ("card-data-handling",): + return failed( + "verified-approver", + title, + f"the recorded entitlement is {after.approvers[0].entitled}, not the control it " + "was granted for", + ) + + bob = Principal(agent="human:bob", user="bob@example.com") + with _granting_principal(bob, entitled=["card-data-handling"]): + reached = store.grant_approval(request.request_id, "mcp-operator:bob") + if reached is None: + return failed("verified-approver", title, "a second distinct principal did not reach 2") + final = store.get_approval(request.request_id) + if final is None or final.status is not ApprovalStatus.GRANTED: + return failed( + "verified-approver", + title, + f"the request is {None if final is None else final.status} at 2 of 2, not granted", + ) + store.close() + return _contended_count(backend, processes, title) + + +#: SPEC-v0.8 §4.5 — the one reason this case is `not_applicable`, and it rests on the store's +#: own declaration that its storage cannot be opened from another process, which §2.4 already +#: allows for `url()`. A sequential pass is not evidence about a count: every assertion above +#: holds on a store that reads and then writes with nothing in between. +NO_CONTENTION = ( + "this backend's storage cannot be opened from another process, so the count cannot be " + "contended; what passed above is the sequential half only" +) + + +def _contended_count(backend: StoreBackend, processes: int, title: str) -> CaseResult: + """The half that is about a **count**: N processes, and one principal in two of them. + + SPEC-v0.8 §4.3. A store deciding the threshold by a read and then a write passes every + sequential assertion in this case and fails here, which is the whole reason `processes` is + a parameter: an earlier draft accepted it and never used it, so the case asserted nothing + about concurrency while sitting in a suite named for it. + + `alice` answers from **two** of the contenders and `bob` from the rest. Whatever the + interleaving, the row must end with exactly two approvers, because there are exactly two + principals; a store that appends reaches three or more, and one that loses an update + reaches one. + """ + store = backend.open() + action = an_action(payment_id="txn_contended") + with _required_roles((), 2): + request = build_request(action, timedelta(minutes=15), store_now(store)) + store.put_approval_request(request) + store.close() + + people = [("human:alice", "alice@example.com")] * 2 + [("human:bob", "bob@example.com")] * max( + 1, processes - 2 + ) + outcome = race( + backend, + len(people), + [ + { + "kind": "answer", + "answer": "grant", + "approval_id": request.request_id, + "who": f"conformance-{index}", + "agent": agent, + "user": user, + } + for index, (agent, user) in enumerate(people) + ], + ) + if isinstance(outcome, str): + if not storage_is_confined(backend): + return dishonest("verified-approver", title, "url()") + return na("verified-approver", title, NO_CONTENTION) + + after = backend.open() + try: + record = after.get_approval(request.request_id) + finally: + after.close() + if record is None: + return failed("verified-approver", title, "the contended request did not come back") + agents = sorted({approver.agent for approver in record.approvers}) + if len(record.approvers) != 2 or agents != ["human:alice", "human:bob"]: + return failed( + "verified-approver", + title, + f"{len(people)} contenders and two principals left " + f"{[approver.agent for approver in record.approvers]}: a count decided by a read " + "and then a write either loses one of them or counts one of them twice", + ) + return passed("verified-approver", title) + + # --- resolution (v0.1 §7 T10; §5.2) --------------------------------------------------------- @@ -2015,6 +2184,7 @@ def _clocked(backend: StoreBackend, clock: Callable[[], datetime]) -> StateStore approval_expiry, approval_atomic, approval_checked_first, + approval_verified_approver, ), "resolution": (resolution_only_ambiguous, resolution_two_targets, resolution_attribution), "outcome": (outcome_no_failed_on_refusal, outcome_no_not_executed), diff --git a/src/ctrlrun/conformance/store/worker.py b/src/ctrlrun/conformance/store/worker.py index b7fd018..6fa76e8 100644 --- a/src/ctrlrun/conformance/store/worker.py +++ b/src/ctrlrun/conformance/store/worker.py @@ -89,7 +89,19 @@ def contend_on(payload: dict[str, Any]) -> dict[str, Any]: store.take_continuation(payload["continuation"]) elif kind == "answer": if payload["answer"] == "grant": - store.grant_approval(payload["approval_id"], payload["who"]) + # SPEC-v0.8 §4.3: where the job names a principal, the grant carries it, so a + # store's count is contended by **distinct verified approvers** rather than by + # anonymous writers it can deduplicate on nothing. + agent = payload.get("agent") + if agent: + from ...action import Principal + from ...approval import _granting_principal + + who = Principal(agent=agent, user=payload.get("user")) + with _granting_principal(who): + store.grant_approval(payload["approval_id"], payload["who"]) + else: + store.grant_approval(payload["approval_id"], payload["who"]) else: store.deny_approval(payload["approval_id"], payload["who"]) else: # pragma: no cover - a payload the kit did not write diff --git a/src/ctrlrun/control.py b/src/ctrlrun/control.py index 57d1dcc..4b3b7a0 100644 --- a/src/ctrlrun/control.py +++ b/src/ctrlrun/control.py @@ -19,11 +19,14 @@ from dataclasses import dataclass, replace from datetime import UTC, datetime, timedelta from pathlib import Path -from typing import Any, Final, Literal, NoReturn, ParamSpec, TypeVar, cast +from typing import Any, Final, NoReturn, ParamSpec, TypeVar, cast from .action import Action, Principal from .approval import ( + APPROVAL_UNRECORDED, + APPROVALS_UNVERIFIABLE, APPROVER_IS_REQUESTER, + APPROVER_UNENTITLED, APPROVER_UNVERIFIED, DEFAULT_APPROVAL_TTL, Approval, @@ -34,13 +37,26 @@ ApprovalVerdict, ApproverIdentity, LocalApprovalProvider, + RequiredRole, VerifiedApprover, _precondition_at_request, _precondition_fingerprint, + _required_roles, check_consumable, + entitled_controls, policy_in_force, + roles_held, + unsatisfied, +) +from .authority import ( + Authority, + AuthorityResult, + BreakGlassEnvelope, + CreatedVia, + Delegation, + Grant, + _optional_from_yaml, ) -from .authority import Authority, AuthorityResult, Delegation, Grant, _optional_from_yaml from .effect import ( _EXECUTOR_RUN, COMMITTED_EFFECT, @@ -184,6 +200,12 @@ class _Invocation: _CONTEXT: ContextVar[_Invocation] = ContextVar("ctrlrun_context") +#: SPEC-v0.8 §5.4 — the grant that decided the action being recorded, for the receipt. A +#: context variable rather than an argument threaded through `_record`'s dozen call sites, +#: on the precedent of `_PRESENTED_APPROVAL` beneath it: `Control` is shared across calls and +#: holds no per-call state, and a context variable is per-call by construction. +_AUTHORITY_GRANT_ID: ContextVar[str | None] = ContextVar("ctrlrun_authority_grant_id") + _PRESENTED_APPROVAL: ContextVar[str] = ContextVar("ctrlrun_approval") #: SPEC-v0.7 §4.3. The token of the attempt whose executor is running, and nothing else. Set @@ -373,19 +395,23 @@ class _Compared: per receipt, on a path that has just read the record. """ - __slots__ = ("approvers", "at_recheck", "at_request", "error") + __slots__ = ("approvers", "at_recheck", "at_request", "error", "unentitled") def __init__(self, at_request: str | None = None) -> None: self.at_request = at_request self.at_recheck: str | None = None self.error: str | None = None self.approvers: tuple[VerifiedApprover, ...] = () + #: SPEC-v0.8 §3.7 — the required role an approver did not hold, so the event can name the + #: control as well as the reason. + self.unentitled: RequiredRole | None = None def reset(self) -> None: self.at_request = None self.at_recheck = None self.error = None self.approvers = () + self.unentitled = None def data(self) -> dict[str, Any]: data: dict[str, Any] = { @@ -641,10 +667,22 @@ def evaluate(self, action: Action) -> Evaluation: return self._policy.evaluate(action) def _authority_result(self, action: Action) -> AuthorityResult | None: - """The authority axis for this action, or `None` where there is no section (§4.1).""" + """The authority axis for this action, or `None` where there is no section (§4.1). + + SPEC-v0.8 §5.4: it also remembers which grant decided, for the receipt. Here rather + than at the four call sites, because a site that forgot would produce a receipt whose + `authority_grant_id` was the **previous** action's, and a stale id on the evidence is + worse than none. Set on every call, including to `None`, for the same reason. + """ if self._authority is None: + _AUTHORITY_GRANT_ID.set(None) return None - return self._authority.evaluate(action, now=self._clock(), store=self._store) + result = self._authority.evaluate(action, now=self._clock(), store=self._store) + # Only where one passed: `grant_id` is also set on a refusal, and a committed receipt + # is the only thing this field is read on. §4.6's `min` already picked which grant of + # several decided, so this is that decision and not a guess about it. + _AUTHORITY_GRANT_ID.set(result.grant_id if result.passed else None) + return result def _authority_data(self, result: AuthorityResult) -> dict[str, Any]: """SPEC-v0.3 §7 — the ids travel here, and never in `decision_reason`. @@ -824,6 +862,15 @@ def execute( reconciler = _reconciler(reconcile, reconcile_eagerly, "execute") started_at = self._clock() + # SPEC-v0.8 §5.4. **Cleared at the top of the call, not only set at the authority + # gate.** §4.3.1 puts `principal_expired` first, so `execute` records a denied receipt + # *before* `_authority_result` runs; with only the gate setting this, that receipt + # carried the **previous** action's grant id. An independent review demonstrated it: a + # committed action under a break-glass grant, then a refusal for a lapsed credential, + # and the refusal's receipt named the grant that never decided it. Async tasks inherit + # a copy of the context at creation, so a task started after a break-glass action + # carried that id into an unrelated refusal too. + _AUTHORITY_GRANT_ID.set(None) # SPEC-v0.3 §6.2 — the counterfactual for an observed run, or `None` in enforce mode. # Every branch below reads it to choose between refusing and recording. observation = _Observation() if self._observing else None @@ -1152,7 +1199,18 @@ def _observe_secure( if evaluation.decision is Decision.APPROVE: approval_id = _PRESENTED_APPROVAL.get(None) if approval_id is None: - observation.block(BLOCKED_APPROVAL_REQUIRED) + # SPEC-v0.8 §4.2, §11.1. **The threshold refusal reaches observe mode too**, + # and it reached it nowhere: `_refuse_unverifiable` lives in `_presented`, + # which observe mode never calls, so a deployment piloting a two-approver + # policy with no approver identity was told `approval_required` -- that a human + # would have been asked. Enforce mode denies every one of those actions before + # anybody is asked, and reporting which is the one thing observe mode is for. + required = self._policy.approvals_required(action.name) + observation.block( + APPROVALS_UNVERIFIABLE + if required > 1 and self._approver_identity is None + else BLOCKED_APPROVAL_REQUIRED + ) if approval_id is None and effect_key is None: return None, None try: @@ -2277,6 +2335,14 @@ def _presented( action is suspended awaiting a human, which is not a terminal state (§6.1). The `APPROVAL_REQUESTED` event is the evidence. """ + # SPEC-v0.8 §4.2: a threshold above one in a deployment that verifies nobody has no + # referent for "distinct principals": the count could never move, or distinctness would + # fall back to the string §4.1 forbids. Denied here, before a human is asked, because + # asking somebody to answer a request that can never be completed spends their attention + # on nothing (`v0.3 §4.3`'s argument for refusing before the approval gate). + required = self._policy.approvals_required(action.name) + if required > 1 and self._approver_identity is None: + self._refuse_unverifiable(action, evaluation, started_at, effect_key, required) presented = _PRESENTED_APPROVAL.get(None) if presented is not None: return presented @@ -2291,8 +2357,24 @@ def _presented( # `Control` is the only object holding both a policy and a provider, and the provider # protocol takes neither, so it travels the way a presented approval does. The # fingerprint travels beside it, by the same route and for the same reason. + # SPEC-v0.8 §3.3: which controls the evaluation that sent this action to approval cited, + # and what role each demands. Built here rather than on `Policy`, because `Control` is + # the only object that holds both the evaluation and the registry, and §11.1 freezes no + # accessor for it. + roles = tuple( + RequiredRole(control=identifier, role=control.approver_role) + for identifier, control in ( + (identifier, self._policy.controls.get(identifier)) + for identifier in evaluation.controls + ) + if control is not None and control.approver_role + ) try: - with policy_in_force(self._policy_hash), _precondition_at_request(fingerprint): + with ( + policy_in_force(self._policy_hash), + _precondition_at_request(fingerprint), + _required_roles(roles, self._policy.approvals_required(action.name)), + ): request = self._approvals.request(action, self._approval_ttl) except Exception: if fingerprint is not None: @@ -2318,7 +2400,16 @@ def _presented( effect_key, approval_id=request.request_id, ) - if fingerprint is not None and not self._recorded(request, fingerprint): + # **Asked whenever anything was pinned**, not only for a fingerprint. The earlier + # spelling was `if fingerprint is not None`, which meant a deployment with no + # preconditions at all -- the common one -- never read the row back, and so never saw a + # threshold or a role list that failed to reach it. + missing = ( + self._unpinned(request, fingerprint, roles, required) + if fingerprint is not None or roles or required > 1 + else None + ) + if missing is not None: # SPEC-v0.7 §6.4: **never a skip**, and without this it was one. A provider that # builds its own `ApprovalRequest` (`build_request` is package-internal) and a store # that does not persist the column both leave an approval that was requested with a @@ -2326,7 +2417,7 @@ def _presented( # names no provider, is 0.6.1's path: consumed with nothing compared. The request # pass is where that is visible, so it is where it is refused. self._refuse_unrecorded_request( - action, evaluation, started_at, effect_key, request, fingerprint + action, evaluation, started_at, effect_key, request, fingerprint, missing ) raise ApprovalRequired( f"{action.name} requires approval: run 'ctrlrun approve {request.request_id}', " @@ -2335,6 +2426,37 @@ def _presented( action_id=action.action_id, ) + def _refuse_unverifiable( + self, + action: Action, + evaluation: Evaluation, + started_at: datetime, + effect_key: str | None, + required: int, + ) -> None: + """§4.2's refusal: `approvals_required` above one, and nobody to count (§12). + + Not a load error: the policy is loadable and correct, and what is missing is the + `Control` it was deployed in, which the loader cannot see. + """ + message = ( + f"{action.name}: 'approvals_required: {required}' needs an approver identity, and " + "this deployment names none; distinct principals cannot be counted where nobody is " + "verified (SPEC-v0.8 §4.2)" + ) + self._append( + EventType.ACTION_DENIED, action, {"reason": APPROVALS_UNVERIFIABLE}, effect_key + ) + self._record( + action, + Evaluation(Decision.DENY, APPROVALS_UNVERIFIABLE, evaluation.controls), + ReceiptResult.DENIED, + started_at, + error=message, + effect_key=effect_key, + ) + raise ActionDenied(message, reason=APPROVALS_UNVERIFIABLE, action_id=action.action_id) + def _refuse_unfetched_request( self, action: Action, @@ -2376,8 +2498,14 @@ def _refuse_unfetched_request( ) raise ActionDenied(message, reason=_PRECONDITION_UNAVAILABLE, action_id=action.action_id) - def _recorded(self, request: ApprovalRequest, fingerprint: str) -> bool: - """Did the fingerprint reach the record a later presentation will read? (§6.4) + def _unpinned( + self, + request: ApprovalRequest, + fingerprint: str | None, + roles: tuple[RequiredRole, ...], + required: int, + ) -> str | None: + """What the kernel pinned and the stored row does not carry, or `None` (§6.4, §4.5). **The read-back, and only the read-back.** An earlier build also compared the returned `ApprovalRequest`, and the review found that guard subsumed: a presentation reads the @@ -2388,9 +2516,37 @@ def _recorded(self, request: ApprovalRequest, fingerprint: str) -> bool: rather than defence (`CONTRIBUTING.md`, the first of the four shapes of a false green). One `get_approval`, on the request pass only. + + **Three fields, not one, and the two new ones are v0.8's.** `v0.7 §6.4` wrote this for + the precondition fingerprint and named both ways of losing it: a provider that builds + its own `ApprovalRequest`, and a store that does not persist the column. Items 3 and 4 + pin the required roles and the threshold by exactly the same route, and an independent + review found that both were lost in exactly the same two ways, with no refusal anywhere: + a row pinning `required_roles=()` satisfies `unsatisfied` trivially, and a row pinning + `approvals_required=1` grants on one yes. An action then executed under a policy + demanding two approvals from a named role, approved once by somebody holding no role. + + So the read-back covers every field this method pins. A row missing any of them cannot + be compared against what was in force, and `v0.7 §6.4`'s rule is that such a request is + refused where it is visible rather than skipped. """ record = self._store.get_approval(request.request_id) - return record is not None and record.request.precondition_fingerprint == fingerprint + if record is None: + return "the request was not recorded at all" + stored = record.request + if fingerprint is not None and stored.precondition_fingerprint != fingerprint: + return "the precondition fingerprint" + if stored.required_roles != roles: + return ( + f"the roles the cited controls require ({[role.control for role in roles]}); " + f"the row carries {[role.control for role in stored.required_roles]}" + ) + if stored.approvals_required != required: + return ( + f"the threshold of {required} approvals; the row carries " + f"{stored.approvals_required}" + ) + return None def _refuse_unrecorded_request( self, @@ -2399,7 +2555,8 @@ def _refuse_unrecorded_request( started_at: datetime, effect_key: str | None, request: ApprovalRequest, - fingerprint: str, + fingerprint: str | None, + missing: str, ) -> NoReturn: """Refuse, and leave nothing behind that another path could spend (SPEC-v0.7 §6.4). @@ -2416,16 +2573,23 @@ def _refuse_unrecorded_request( """ withdrawn = self._withdraw(request) compared = _Compared() - compared.at_recheck = fingerprint + if fingerprint is not None: + compared.at_recheck = fingerprint + # The fingerprint keeps its own reason, because `precondition_missing` is in the + # vocabulary `v0.7` froze and every reader of an older receipt reads it that way. What + # items 3 and 4 pin gets its own, for the reason every refusal here does: a test that + # asserts a status cannot tell which guard ran. + reason = _PRECONDITION_MISSING if fingerprint is not None else APPROVAL_UNRECORDED outcome = ( f"the request is withdrawn ({withdrawn})" if withdrawn in _WITHDRAWALS else f"the request could not be withdrawn ({withdrawn})" ) _LOG.warning( - "%s: the precondition fingerprint was not recorded with approval request %s, so %s " - "and the action is refused (SPEC-v0.7 §6.4)", + "%s: %s was not recorded with approval request %s, so %s and the action is refused " + "(SPEC-v0.7 §6.4, SPEC-v0.8 §4.5)", action.name, + missing, request.request_id, outcome, ) @@ -2433,7 +2597,8 @@ def _refuse_unrecorded_request( EventType.APPROVAL_INVALIDATED, action, { - "reason": _PRECONDITION_MISSING, + "reason": reason, + "missing": missing, "action_hash": action.action_hash, "withdrawn": withdrawn, **compared.data(), @@ -2444,14 +2609,13 @@ def _refuse_unrecorded_request( self._append( EventType.ACTION_DENIED, action, - {"reason": _PRECONDITION_MISSING}, + {"reason": reason}, effect_key, approval_id=request.request_id, ) message = ( - f"{action.name}: the precondition fingerprint was not recorded with approval " - f"request {request.request_id}, so no presentation of it could compare anything; " - f"{outcome}" + f"{action.name}: {missing} was not recorded with approval request " + f"{request.request_id}, so no presentation of it could compare anything; {outcome}" ) self._record( action, @@ -2463,7 +2627,7 @@ def _refuse_unrecorded_request( effect_key=effect_key, compared=compared, ) - raise ActionDenied(message, reason=_PRECONDITION_MISSING, action_id=action.action_id) + raise ActionDenied(message, reason=reason, action_id=action.action_id) def _withdraw(self, request: ApprovalRequest) -> str: """Make a request nobody may answer, with the methods a store already has (§6.4). @@ -2637,12 +2801,20 @@ def _check_approver( # `check_consumable` refuses it at every later presentation, and a lapsed grant whose # approver is fine still reports `expired` with its event and the store's own write, # because the check passes and `_take` decides. §2.4.1 carries the table. - self._refuse_approver(action, approval_id, record.approvers if record is not None else ()) + assert record is not None + self._refuse_approver( + action, approval_id, record.approvers, compared, record.request.required_roles + ) def _refuse_approver( - self, action: Action, approval_id: str, approvers: tuple[VerifiedApprover, ...] + self, + action: Action, + approval_id: str, + approvers: tuple[VerifiedApprover, ...], + compared: _Compared, + required: tuple[RequiredRole, ...] = (), ) -> None: - """§2.7 and §4.1's two refusals, over whatever the row recorded.""" + """§2.7, §3.6 and §4.1's refusals, over whatever the row recorded.""" if not approvers: raise ApprovalMismatch( f"approval {approval_id} carries no verified approver, and this deployment " @@ -2650,6 +2822,20 @@ def _refuse_approver( reason=APPROVER_UNVERIFIED, approval_id=approval_id, ) + for approver in approvers: + # SPEC-v0.8 §3.6: **each** approver satisfies **every** required role. A control that + # says who may answer is not satisfied by a committee in which one member could, which + # is why this is inside the loop and `unsatisfied` is all-of rather than any-of. + missing = unsatisfied(required, approver.entitled) + if missing is not None: + compared.unentitled = missing + raise ApprovalMismatch( + f"approval {approval_id} was granted by an approver who does not hold the " + f"role {missing.role!r} required by control {missing.control!r}; the approval " + "is left granted", + reason=APPROVER_UNENTITLED, + approval_id=approval_id, + ) requester = (action.principal.agent, action.principal.user) for approver in approvers: if approver.principal == requester: @@ -2723,6 +2909,13 @@ def _invalidated( """`APPROVAL_INVALIDATED`'s data: the reason, and for a precondition refusal the two fingerprints it compared, hashes only, and the provider's failure by type (§6.2).""" data: dict[str, Any] = {"reason": mismatch.reason, "action_hash": action.action_hash} + # SPEC-v0.8 §3.7: an entitlement refusal names the control and the role on the event as + # well as in the message, because an operator reading the evidence should not have to + # parse a sentence to find out which written expectation was not met. By the carrier the + # precondition hashes already travel on, so no error type grows a keyword (§11.2). + if mismatch.reason == APPROVER_UNENTITLED and compared.unentitled is not None: + data["control"] = compared.unentitled.control + data["role"] = compared.unentitled.role if mismatch.reason in _PRECONDITION_REASONS: data.update(compared.data()) return data @@ -2766,6 +2959,157 @@ def delegate(self, parent_id: str, grant: Grant, *, by: Principal) -> Delegation """ return self._delegate(parent_id, grant, by=by, via="api") + def _break_glass(self, envelope_id: str, grant: Grant, *, reason: str = "") -> Delegation: + """Open a break-glass grant beneath a declared envelope (SPEC-v0.8 §5.3). + + There is no flag. What this creates is an ordinary delegation: recorded, bounded by the + envelope on every dimension `contained_dimension` knows, expiring, revocable and + attenuable, and named on the receipt of every action taken under it. A setting that + skipped a check would have none of those five properties, which is the whole argument + of §5.1. + + **The opener is a verified principal and never an assertion, and there is no parameter + that says otherwise.** An earlier build took `by: Principal | None`, which was an + unauthenticated way to assert an opener *and the roles it holds*: passing a principal + whose claims carried the envelope's role opened it in a deployment whose provider + resolved somebody else entirely. `§11.2` keeps `_granting_principal` package-internal + for exactly that reason, and a public `by=` was the same hole with a docstring. The + opener is whoever the `ApproverIdentity` resolves, and a deployment that names none + cannot open one at all. + + **Private, like `_delegate`.** `§11.2` adds no public `Control` method in v0.8; the + surface item 5 adds is the CLI command, which calls this the way `ctrlrun delegate` + calls `_delegate`. + + `reason` is free text on the `DELEGATION_CREATED` event. The kernel does not interpret + it, exactly as it does not interpret `source:`. + """ + authority = self._require_authority("break-glass") + envelope = authority.envelopes.get(envelope_id) + opener = self._opener_for(envelope_id, envelope) + now = self._clock() + try: + planned = authority.plan_break_glass( + envelope_id, grant, by=opener, store=self._store, now=now + ) + except IdentityError: + self._append_delegation( + EventType.DELEGATION_REJECTED, + {"reason": PRINCIPAL_EXPIRED, "parent_id": envelope_id}, + ) + raise + except AuthorityEscalation as escalation: + data: dict[str, Any] = {"reason": escalation.reason, "parent_id": envelope_id} + if escalation.dimension is not None: + data["dimension"] = escalation.dimension + self._append_delegation(EventType.DELEGATION_REJECTED, data) + raise + self._store.put_delegation(planned.to_record()) + self._append_delegation( + EventType.DELEGATION_CREATED, + { + "delegation_id": planned.delegation_id, + "parent_id": planned.parent_id, + "depth": planned.depth, + "created_by_agent": opener.agent, + "created_by_user": opener.user, + "created_via": "break-glass", + "reason": reason, + }, + ) + _LOG.warning( + "break-glass %s opened beneath %s by %s until %s: %s", + planned.delegation_id, + envelope_id, + opener.agent, + planned.grant.expires_at, + reason or "no reason given", + ) + return planned + + def _opener_for(self, envelope_id: str, envelope: BreakGlassEnvelope | None) -> Principal: + """Who is opening this envelope, and may they? (SPEC-v0.8 §5.3.1.) + + Rule 4 does not apply to an envelope: its subject names the agents a break-glass grant + may be **for**, and the opener is a human. What gates the opener is the envelope's + `controls:`, whose `approver_role` this checks against the roles the opener holds. + + An unknown envelope is left to `plan_break_glass` to refuse, so `--envelope` naming an + ordinary grant gets §5.3.1's message rather than one about a missing configuration. + """ + identity = self._approver_identity + if identity is None: + raise InvalidArgument( + "opening a break-glass envelope needs an approver identity: the envelope names " + "the controls that gate who may open it, and with nobody resolved there is no " + "principal to check them against. Build the Control with " + "approver_identity=ApproverIdentity(provider, roles_claim=...) " + "(SPEC-v0.8 §5.3.1). Note that `Control.from_file`, which is what the CLI " + "builds, wires none: see SPEC-v0.8 §14.5, which records that as open" + ) + opener = identity.resolve( + IdentityContext(action="ctrlrun.break-glass", environment=self.environment) + ) + if opener is None: + raise IdentityError( + "the approver identity resolved nobody, so this break-glass envelope has no " + "opener to check against its controls (SPEC-v0.8 §5.3.1)" + ) + if envelope is None: + return opener + # **Every cited control must resolve and must name a role.** Elsewhere a control that + # names no `approver_role` gates nobody (§3.5), and that is right where the citation is + # on an *action*: the control is documentation and the approval decides. Here the + # citation **is** the gate, so the same rule reads the opposite way -- a typo in an + # envelope's `controls:` silently admitted any verified principal, which an independent + # review demonstrated with one transposed letter. `Authority.from_yaml` parses the + # section without a registry to check against, so it is checked here, where both are. + unresolved = [ + identifier + for identifier in envelope.controls + if (control := self._policy.controls.get(identifier)) is None + or not control.approver_role + ] + if unresolved: + raise InvalidArgument( + f"break-glass envelope {envelope_id!r} cites {unresolved}, which " + + ( + "name no control in this policy's registry" + if any(self._policy.controls.get(name) is None for name in unresolved) + else "declare no 'approver_role'" + ) + + ". An envelope's controls are what gate who may open it, so a citation that " + "resolves to nothing would gate nobody (SPEC-v0.8 §5.3.1)" + ) + required = tuple( + RequiredRole(control=identifier, role=control.approver_role) + for identifier, control in ( + (identifier, self._policy.controls.get(identifier)) + for identifier in envelope.controls + ) + if control is not None and control.approver_role + ) + held = roles_held(opener, identity.roles_claim) + missing = unsatisfied(required, entitled_controls(required, held)) + if missing is not None: + self._append_delegation( + EventType.DELEGATION_REJECTED, + { + "reason": APPROVER_UNENTITLED, + "parent_id": envelope_id, + "control": missing.control, + "role": missing.role, + }, + ) + raise AuthorityEscalation( + f"{opener.agent!r} does not hold the role {missing.role!r} required by control " + f"{missing.control!r}, which gates who may open {envelope_id!r} " + "(SPEC-v0.8 §5.3.1)", + reason=APPROVER_UNENTITLED, + parent_id=envelope_id, + ) + return opener + def revoke(self, delegation_id: str, *, by: str | None = None) -> None: """Revoke one delegation (SPEC-v0.3 §5.7). @@ -2790,7 +3134,7 @@ def revoke(self, delegation_id: str, *, by: str | None = None) -> None: ) def _delegate( - self, parent_id: str, grant: Grant, *, by: Principal, via: Literal["api", "cli"] + self, parent_id: str, grant: Grant, *, by: Principal, via: CreatedVia ) -> Delegation: """The one implementation behind `Control.delegate` and `ctrlrun delegate`. @@ -2993,6 +3337,13 @@ def _record( # both new fields null, which is what makes "would_have present on every observed run # and absent on every refused one" true in both directions. receipt = Receipt( + # SPEC-v0.8 §5.4 — which grant let this through, on **every** action decided by + # authority and not only under break-glass. `AuthorityResult.grant_id` already + # reaches the events; what nothing did was put it on the receipt, so answering + # "what did this grant let through" meant joining events by hand. A field that + # existed only under break-glass would be one nothing exercises on the ordinary + # path, and so one nobody would notice breaking. + authority_grant_id=_AUTHORITY_GRANT_ID.get(None), receipt_id=new_receipt_id(), action_id=action.action_id, action=action.name, diff --git a/src/ctrlrun/gateway/operator.py b/src/ctrlrun/gateway/operator.py index dbb4dba..5e12973 100644 --- a/src/ctrlrun/gateway/operator.py +++ b/src/ctrlrun/gateway/operator.py @@ -34,7 +34,14 @@ from typing import Any, Final from ..action import Principal -from ..approval import ApprovalRecord, ApprovalStatus, _granting_principal +from ..approval import ( + ApprovalRecord, + ApprovalStatus, + _granting_principal, + entitled_controls, + roles_held, + unsatisfied, +) from ..control import Control from ..effect import RESOLVED_BY_HUMAN, EffectState from ..errors import CTRLRunError, IdentityError, InvalidArgument @@ -99,6 +106,11 @@ #: Reused unchanged from `v0.2 §6.10`. NO_PRINCIPAL: Final = (-41007, "ctrlrun.no_principal", 403) + +#: SPEC-v0.8 §3.8 — the credential is verified and does not carry the role the request pinned. +#: A 403 and not a 400: the caller is who they say they are, and the answer is that this is not +#: theirs to give. +_NOT_ENTITLED: Final = -41015 STORE_REFUSED: Final = (-41003, "ctrlrun.approval_denied", 200) _METHOD_NOT_FOUND: Final = -32601 @@ -141,6 +153,10 @@ class OperatorConfig: identity_jwt_audience: str | None = None identity_jwt_token_type: str | None = None identity_jwt_header: str = "authorization" + #: SPEC-v0.8 §3.4, §11.1 — which claim this deployment's issuer puts roles in. `None` means + #: no role can be read, so any control naming one refuses: a deployment naming roles in its + #: policy and no claim to read them from has configured half a check. + approver_roles_claim: str | None = None identity_jwt_agent_claim: str = "sub" identity_jwt_user_claim: str | None = None identity_jwt_claims: tuple[str, ...] = () @@ -399,6 +415,43 @@ def __init__( self._control = control self._identity = identity self._clock = clock + self._warn_about_unreadable_roles() + + def _roles_claim(self) -> str | None: + """Which claim roles are read from: `--approver-roles-claim`, else the `Control`'s. + + SPEC-v0.8 §3.4 puts the claim name on `ApproverIdentity` because it is a property of + the issuer that verifies approvers, and this server is one surface reading it. The flag + wins where both are set, because a flag is what an operator changes to debug a + deployment, and the two disagreeing is itself worth the warning below. + """ + if self._config.approver_roles_claim: + return self._config.approver_roles_claim + identity = self._control.approver_identity + return None if identity is None else identity.roles_claim + + def _warn_about_unreadable_roles(self) -> None: + """SPEC-mcp-operator §4.3: half a check, named at startup and not at the first refusal. + + A policy whose cited controls name an `approver_role` and a deployment with no claim to + read roles from refuses **every** answer to those requests. That is the fail-closed + direction and it is correct; what is not acceptable is discovering it when a human is + told no at three in the morning. + """ + if self._roles_claim(): + return + policy = self._control.policy + gated = sorted( + identifier for identifier, control in policy.controls.items() if control.approver_role + ) + if not gated: + return + _LOG.warning( + "controls %s name an approver_role and no claim is configured to read roles from: " + "every approval they gate will be refused. Set --approver-roles-claim, or pass " + "roles_claim= on the Control's ApproverIdentity (SPEC-v0.8 §3.4)", + ", ".join(gated), + ) @property def config(self) -> OperatorConfig: @@ -784,7 +837,36 @@ def _approve(self, request_id: str, who: str, principal: Principal) -> dict[str, # The principal its own provider verified is recorded beside that string, so an # approval granted here is consumable in a deployment that checks (§2.7). It is recorded # whether or not the deployment checks, because it is true either way. - with _granting_principal(principal): + # + # SPEC-v0.8 §3.8: and the entitlement the request pinned is computed here, where the + # credential is, and refused here where it is not held. This half is the **courtesy**: a + # human learns at the moment they answer rather than at the moment an agent retries. The + # guarantee is `Control`'s check at consumption, which reads what this recorded. + required = () if record is None else record.request.required_roles + held = roles_held(principal, self._roles_claim()) + entitled = entitled_controls(required, held) + missing = unsatisfied(required, entitled) + if missing is not None: + # §3.7: named in the operator's log as well as in the answer, because the answer + # goes to the person who was refused and this goes to whoever configured the claim. + _LOG.warning( + "%s answered %s without the role %r that control %r requires; roles were read " + "from the claim %r and held %s", + principal.agent, + request_id, + missing.role, + missing.control, + self._roles_claim(), + sorted(held) or "none", + ) + raise _Refused( + _NOT_ENTITLED, + "ctrlrun.not_entitled", + 403, + f"answering this request needs the role {missing.role!r}, required by control " + f"{missing.control!r}, and the credential presented does not carry it", + ) + with _granting_principal(principal, entitled=entitled): approval = store.grant_approval(request_id, who) # `grant_approval` refuses an unknown id (`v0.1 §4.1`, `check_answerable`) and nothing # deletes an approval row, so the record exists by here. This is an invariant check and @@ -797,6 +879,16 @@ def _approve(self, request_id: str, who: str, principal: Principal) -> dict[str, 500, f"{request_id} was granted and then could not be read back", ) + if approval is None: + # SPEC-v0.8 §4.4: recorded, still short of N. No `APPROVAL_GRANTED`, because nothing + # was granted; the answer is a fact the caller needs and not an event about a grant. + after = store.get_approval(request_id) + return { + "status": "pending", + "request_id": request_id, + "approvals_required": record.request.approvals_required, + "approvals_recorded": 0 if after is None else len(after.approvers), + } store.append_event( self._event( EventType.APPROVAL_GRANTED, diff --git a/src/ctrlrun/jwt_identity.py b/src/ctrlrun/jwt_identity.py index 0aaacd9..c900c51 100644 --- a/src/ctrlrun/jwt_identity.py +++ b/src/ctrlrun/jwt_identity.py @@ -571,8 +571,21 @@ def _principal( value = claims.get(name) if isinstance(value, str | int | bool): # bool is a subclass of int selected[name] = value + elif isinstance(value, list) and all(isinstance(item, str) and item for item in value): + # SPEC-v0.8 §3.4: a roles claim is a JSON array at every issuer anybody deploys, and + # dropping it here is what made its holder silently unentitled. Carried as a tuple, + # which is what `Principal.claims` now admits. + selected[name] = tuple(value) elif value is not None: - _LOG.debug("claim %r is %s, which a Principal cannot carry", name, type(value).__name__) + # **A warning and not a DEBUG line**, because this is where a silently unentitled + # approver begins: a claim an operator asked for by name, that the issuer sent in a + # shape this cannot carry, and that every later check then reads as absent (§3.4). + _LOG.warning( + "claim %r is %s, which a Principal cannot carry, so it is absent: an approver " + "role read from this claim will not be held by anybody (SPEC-v0.8 §3.4)", + name, + type(value).__name__, + ) expires_at = claims.get("exp") return Principal( agent=agent.strip(), diff --git a/src/ctrlrun/policy.py b/src/ctrlrun/policy.py index 32d7f36..1e381b1 100644 --- a/src/ctrlrun/policy.py +++ b/src/ctrlrun/policy.py @@ -54,6 +54,12 @@ #: would renew without a ceiling, which is the behaviour the key exists to bound. POLICY_SCHEMA_V5: Final = "ctrlrun.policy/v5" +#: SPEC-v0.8 §11.3: `v6` adds `approver_role` on a control entry, and items 4 and 5 add +#: their keys under it. The version moves once, here, for the reason §11.4 gives: an older +#: reader must refuse a document whose keys it would otherwise ignore, and a reader that +#: ignored `approver_role` would run a deployment believing nobody was gated. +POLICY_SCHEMA_V6: Final = "ctrlrun.policy/v6" + #: All of them, newest last, for the message an unknown schema produces. **In version order**, #: which `_at_least` reads: a version added out of order would make every gate below lie. SUPPORTED_SCHEMAS: Final = ( @@ -62,6 +68,7 @@ POLICY_SCHEMA_V3, POLICY_SCHEMA_V4, POLICY_SCHEMA_V5, + POLICY_SCHEMA_V6, ) @@ -117,7 +124,7 @@ def _at_least(schema: str, minimum: str) -> bool: #: §3.1's key sets are closed and a `version:` an older reader silently dropped would be a typo #: that never surfaced. #: The closed key set of one registry entry (§7.3, and §3.1's rule). -_CONTROL_KEYS: Final = frozenset({"title", "source"}) +_CONTROL_KEYS: Final = frozenset({"title", "source", "approver_role"}) _V4_TOP_LEVEL_KEYS: Final[Mapping[str, str]] = { "controls": ( @@ -155,6 +162,16 @@ def _at_least(schema: str, minimum: str) -> bool: ), } +#: SPEC-v0.8 §4.2 — the M-of-N threshold, gated for `max_attempts`'s reason: an older reader +#: would ignore it and consume on the first grant, which is a deployment believing two humans +#: answered when one did. +_V6_ENTRY_KEYS: Final[Mapping[str, str]] = { + "approvals_required": ( + "an older reader would ignore the threshold and consume on the first grant, which is a " + "deployment believing several humans answered when one did" + ), +} + _RULE_KEYS: Final = frozenset({"when", "decision", "controls"}) #: SPEC-v0.2 §3.1 — the keys `ctrlrun.policy/v2` adds to an action entry. The gateway has no @@ -164,6 +181,7 @@ def _at_least(schema: str, minimum: str) -> bool: frozenset({"decision", "rules", "controls", "data"}) | _V2_ENTRY_KEYS | frozenset(_V5_ENTRY_KEYS) + | frozenset(_V6_ENTRY_KEYS) ) #: And the closed key set of the `mcp` mapping, which is one key wide. @@ -475,6 +493,15 @@ class PolicyControl: id: str title: str source: str | None = None + #: SPEC-v0.8 §3.2 — which role may answer an approval this control was cited on. An opaque + #: string: CTRLRun does not know what it means, does not check that such a role exists, and + #: makes no compliance claim on the strength of one, exactly as it does not interpret + #: `source`. What it does is decide **who may answer an approval the decision already + #: required**, which is the first thing a control has ever decided (§3.2). + #: + #: `None` means this control gates nothing, which is 0.7.0's behaviour for it and is the + #: opposite answer to a principal whose claims carry no role (§3.5). + approver_role: str | None = None def _in_registry_order(cited: tuple[str, ...], order: tuple[str, ...]) -> tuple[str, ...]: @@ -522,6 +549,9 @@ class _ActionPolicy: #: SPEC-v0.7 §5.3 — how many attempts may execute on one effect key, the first included, or #: `None` where the entry names no ceiling. `None` is today's behaviour and is not a number. max_attempts: int | None = None + #: SPEC-v0.8 §4.2 — how many distinct verified principals must answer, or `None` where the + #: entry names none. `None` is one, which is 0.7.0. + approvals_required: int | None = None def data_scope(self, arguments: Mapping[str, Any]) -> frozenset[str]: """The labels present in **the arguments actually supplied** (SPEC-v0.6 §7.4). @@ -741,7 +771,7 @@ def _from_document(cls, document: object, source: str, text: str | None = None) raise PolicyError( f"{source}: 'version' must be a non-empty string, got {_type_name(version)}" ) - controls = _parse_controls(document.get("controls"), source) + controls = _parse_controls(document.get("controls"), source, schema) actions: dict[str, _ActionPolicy] = {} for name, entry in entries.items(): if not isinstance(name, str) or not name: @@ -790,6 +820,18 @@ def mcp_options(self, action_name: str) -> McpOptions: entry = self.actions.get(action_name) return _DEFAULT_MCP_OPTIONS if entry is None else entry.mcp + def approvals_required(self, action_name: str) -> int: + """How many distinct verified principals must answer this action (SPEC-v0.8 §4.2). + + One where the entry names none, which is 0.7.0, and one for an action no entry names: + such an action is denied `unknown_action` before an approval exists, so the number is + never read. + """ + entry = self.actions.get(action_name) + if entry is None or entry.approvals_required is None: + return 1 + return entry.approvals_required + def max_attempts(self, action_name: str) -> int | None: """This action's attempt ceiling, or `None` (SPEC-v0.7 §5.3). @@ -1118,7 +1160,7 @@ def _checked_control_id(identifier: str, source: str) -> None: ) -def _parse_controls(value: object, source: str) -> dict[str, PolicyControl]: +def _parse_controls(value: object, source: str, schema: str) -> dict[str, PolicyControl]: """The top-level `controls:` registry (SPEC-v0.6 §7.3).""" if value is None: return {} @@ -1147,8 +1189,33 @@ def _parse_controls(value: object, source: str) -> dict[str, PolicyControl]: raise PolicyError( f"{where}: 'source' must be a non-empty string, got {_type_name(cited)}" ) + role = entry.get("approver_role") + if "approver_role" in entry: + if not _at_least(schema, POLICY_SCHEMA_V6): + raise PolicyError( + f"{where}: 'approver_role' needs 'schema: {POLICY_SCHEMA_V6}'; this document " + f"declares {schema!r}, and an older reader would load it, gate nobody, and " + "report a deployment as checking entitlement when it is not" + ) + if not isinstance(role, str) or not role.strip(): + raise PolicyError( + f"{where}: 'approver_role' must be a non-empty string, got {_type_name(role)}" + ) + if role != role.strip(): + # Refused rather than trimmed, because §3.4 matches a role against a claim byte + # for byte: trimming here would make the document and the comparison disagree, + # and accepting it as written means a role nobody's credential can ever carry, + # which refuses every approval the control gates and says nothing about why. + raise PolicyError( + f"{where}: 'approver_role' has leading or trailing whitespace ({role!r}); " + "roles are matched byte for byte against a claim, so this one would match " + "nothing and refuse every approval this control gates" + ) registry[identifier] = PolicyControl( - id=identifier, title=title, source=cited if isinstance(cited, str) else None + id=identifier, + title=title, + source=cited if isinstance(cited, str) else None, + approver_role=role if isinstance(role, str) else None, ) return registry @@ -1265,8 +1332,15 @@ def _parse_entry( f"{where}: {key!r}{_at_line(line_of(key))} needs 'schema: {POLICY_SCHEMA_V5}'; " f"this document declares {schema!r}, and {consequence}" ) + for key, consequence in _V6_ENTRY_KEYS.items(): + if key in entry and not _at_least(schema, POLICY_SCHEMA_V6): + raise PolicyError( + f"{where}: {key!r}{_at_line(line_of(key))} needs 'schema: {POLICY_SCHEMA_V6}'; " + f"this document declares {schema!r}, and {consequence}" + ) labels = _parse_data(entry.get("data"), where) ceiling = _parse_max_attempts(entry, where, line_of) + required = _parse_approvals_required(entry, where, line_of) if has_decision: return _ActionPolicy( @@ -1278,6 +1352,7 @@ def _parse_entry( controls=cited, data=MappingProxyType(labels), max_attempts=ceiling, + approvals_required=required, ) rules = entry["rules"] @@ -1294,6 +1369,7 @@ def _parse_entry( controls=cited, data=MappingProxyType(labels), max_attempts=ceiling, + approvals_required=required, ) @@ -1302,6 +1378,37 @@ def _at_line(line: int | None) -> str: return "" if line is None else f" on line {line}" +def _parse_approvals_required( + entry: Mapping[Any, Any], where: str, line_of: Callable[[str], int | None] +) -> int | None: + """The M-of-N threshold, validated at load (SPEC-v0.8 §4.2). + + `v0.7 §5.3`'s rules for `max_attempts`, for the same reason: a document that cannot say how + many approvals it requires is a document nobody should deploy, and finding out when the first + grant consumes is finding out late. + + `bool` is refused although Python makes it an `int` (`v0.1 §3.2`). `0` is refused rather than + read as "no approval needed", which is what `decision: allow` says, or as "never", which is + `decision: deny`. Absent means 1, which is 0.7.0. + """ + if "approvals_required" not in entry: + return None + value = entry["approvals_required"] + at = _at_line(line_of("approvals_required")) + if isinstance(value, bool) or not isinstance(value, int): + raise PolicyError( + f"{where}: 'approvals_required'{at} must be an integer of at least 1, got " + f"{_type_name(value)} {value!r}. It counts the distinct verified principals that " + "must answer; remove the key for one" + ) + if value < 1: + raise PolicyError( + f"{where}: 'approvals_required'{at} must be at least 1, got {value}. Zero approvals " + "is 'decision: allow', and an action nobody may approve is 'decision: deny'" + ) + return value + + def _parse_max_attempts( entry: Mapping[Any, Any], where: str, line_of: Callable[[str], int | None] ) -> int | None: diff --git a/src/ctrlrun/postgres.py b/src/ctrlrun/postgres.py index 7706f79..fa5c905 100644 --- a/src/ctrlrun/postgres.py +++ b/src/ctrlrun/postgres.py @@ -49,6 +49,7 @@ _verified_approver_now, check_answerable, check_consumable, + count_grant, ) from .effect import ( COMMITTED_EFFECT, @@ -92,6 +93,8 @@ _resolved, _result_json, _result_value, + _roles_from_json, + _roles_json, _transitioned, _utc_now, ) @@ -317,6 +320,13 @@ def _moved(found: EffectRecord, was: EffectRecord, effect_key: str) -> CTRLRunEr return DuplicateEffect(moved, state=IN_PROGRESS_EFFECT, effect_key=effect_key) +#: SPEC-v0.8 §4.3 — how many times a grant re-reads after losing its compare-and-set. Bounded, +#: because an unbounded retry against a hot approval is a spin nobody can see; N humans answering +#: one request cannot exceed N collisions, and this is comfortably above any N a human workflow +#: has. +_GRANT_ATTEMPTS: Final = 8 + + class PostgresStateStore: """Approvals, effects and evidence in a Postgres schema (SPEC-v0.6 §4). @@ -696,7 +706,7 @@ def _read_approval(self, connection: Any, approval_id: str) -> ApprovalRecord | cursor.execute( "SELECT approval_id, action_hash, status, action_json, approver, created_at, " "granted_at, expires_at, consumed_at, policy_hash_at_approval, " - "precondition_fingerprint, approvers " + "precondition_fingerprint, approvers, required_roles, approvals_required " f"FROM {self._q}.approvals WHERE approval_id = %s", (approval_id,), ) @@ -712,6 +722,8 @@ def _read_approval(self, connection: Any, approval_id: str) -> ApprovalRecord | expires_at=datetime.fromisoformat(str(row[7])), policy_hash=None if row[9] is None else str(row[9]), precondition_fingerprint=None if row[10] is None else str(row[10]), + required_roles=_roles_from_json(None if row[12] is None else str(row[12])), + approvals_required=int(row[13]) if row[13] is not None else 1, ), status=ApprovalStatus(row[2]), approver=row[4], @@ -1467,8 +1479,8 @@ def put_approval_request(self, request: ApprovalRequest) -> None: f"INSERT INTO {self._q}.approvals(" "approval_id, action_hash, status, action_json, " "approver, created_at, granted_at, expires_at, consumed_at, " - "policy_hash_at_approval, precondition_fingerprint) " - "VALUES(%s,%s,%s,%s,NULL,%s,NULL,%s,NULL,%s,%s)", + "policy_hash_at_approval, precondition_fingerprint, required_roles, " + "approvals_required) VALUES(%s,%s,%s,%s,NULL,%s,NULL,%s,NULL,%s,%s,%s,%s)", ( request.request_id, request.action_hash, @@ -1478,6 +1490,8 @@ def put_approval_request(self, request: ApprovalRequest) -> None: _iso(request.expires_at), request.policy_hash, request.precondition_fingerprint, + _roles_json(request.required_roles), + request.approvals_required, ), ) except Exception as duplicate: @@ -1513,51 +1527,76 @@ def find_denied_request(self, action_hash: str) -> ApprovalRequest | None: return _newest_denied(self.approvals_for(action_hash), action_hash, self._clock()) - def grant_approval(self, approval_id: str, approver: str) -> Approval: + def grant_approval(self, approval_id: str, approver: str) -> Approval | None: + """Record one answer, counting toward the threshold the request pinned (SPEC-v0.8 §4.2). + + **The compare-and-set is on `approvers` and not on `status`, and that is the whole of + §4.3.** This store runs READ COMMITTED with an explicit `BEGIN` and reads with a plain + `SELECT`, so at N-1 the status does not change: two concurrent grants both read + `pending`, both update `WHERE status = 'pending'`, both see `rowcount == 1`, and each + writes an `approvers` value computed from the row it read before the other wrote. That is + a lost update, and one principal fills two slots. `_consume_locked` documents the + identical defect, measured at 8 of 8, and says the condition has to be in the statement. + + So the condition is the value being changed. `IS NOT DISTINCT FROM` and not `=`, because + the first grant compares against `NULL`. A miss means somebody else answered first, which + is information rather than an error to swallow: the retry re-reads, and it converges + because a fresh statement in READ COMMITTED sees the winner's commit. + """ approver = _approver(approver) connection = self._connection() - now = self._clock() - connection.execute("BEGIN") - self._use_schema(connection) - try: - record = self._answerable(connection, approval_id, now) - # SPEC-v0.8 §2.5: the verified approver the granting surface resolved, appended to - # whatever the row already holds. - verified = _verified_approver_now(now) - approvers = (*record.approvers, verified) if verified else record.approvers - granted = replace( - record, - status=ApprovalStatus.GRANTED, - approver=approver, - granted_at=now, - approvers=approvers, - ) - with connection.cursor() as cursor: - # Conditional on what `check_answerable` saw. Unconditional, a concurrent - # `deny_approval` was silently overwritten and `find_granted_approval` then - # returned an approval a human had refused. - cursor.execute( - f"UPDATE {self._q}.approvals SET status=%s, approver=%s, granted_at=%s, " - "approvers=%s WHERE approval_id=%s AND status=%s", - ( - str(ApprovalStatus.GRANTED), - approver, - _iso(now), - _approvers_json(approvers), - approval_id, - str(record.status), - ), + for _ in range(_GRANT_ATTEMPTS): + now = self._clock() + connection.execute("BEGIN") + self._use_schema(connection) + try: + record = self._answerable(connection, approval_id, now) + verified = _verified_approver_now(now) + approvers, reached = count_grant(record, verified, now) + status = ApprovalStatus.GRANTED if reached else record.status + granted = replace( + record, + status=status, + approver=approver if reached else record.approver, + granted_at=now if reached else record.granted_at, + approvers=approvers, ) - if cursor.rowcount != 1: - raise ApprovalMismatch( - f"approval {approval_id} was answered by somebody else first", - reason="answered", + with connection.cursor() as cursor: + # Conditional on what this transaction read, which is both halves: the + # status `check_answerable` saw, so a concurrent `deny_approval` is not + # silently overwritten, and the `approvers` value the count was computed + # from, so a concurrent grant is not lost. + cursor.execute( + f"UPDATE {self._q}.approvals SET status=%s, approver=%s, granted_at=%s, " + "approvers=%s WHERE approval_id=%s AND status=%s " + "AND approvers IS NOT DISTINCT FROM %s", + ( + str(status), + granted.approver, + _iso(granted.granted_at) if granted.granted_at else None, + _approvers_json(approvers), + approval_id, + str(record.status), + _approvers_json(record.approvers), + ), ) - except BaseException: - self._rollback(connection) - raise - self._commit(connection) - return granted.as_approval() + missed = cursor.rowcount != 1 + except BaseException: + self._rollback(connection) + raise + if missed: + # Somebody else answered between this transaction's read and its write. Roll + # back and read again rather than raise: their answer is as valid as this one, + # and the next pass counts both. + self._rollback(connection) + continue + self._commit(connection) + return granted.as_approval() if reached else None + raise ApprovalMismatch( + f"approval {approval_id} was answered by somebody else first, {_GRANT_ATTEMPTS} " + "times running; nothing was recorded for this answer", + reason="answered", + ) def deny_approval(self, approval_id: str, approver: str) -> None: approver = _approver(approver) diff --git a/src/ctrlrun/receipt.py b/src/ctrlrun/receipt.py index 28edab6..5ba3892 100644 --- a/src/ctrlrun/receipt.py +++ b/src/ctrlrun/receipt.py @@ -27,7 +27,12 @@ # downward (ARCHITECTURE §6): a receipt records what an approval verified, and the record # type it records is that module's. from .approval import APPROVAL_DENIED as APPROVAL_DENIED_REASON -from .approval import VerifiedApprover +from .approval import ( + APPROVAL_UNRECORDED, + APPROVALS_UNVERIFIABLE, + APPROVER_UNENTITLED, + VerifiedApprover, +) from .errors import CTRLRunError, InvalidArgument from .policy import Decision @@ -143,6 +148,17 @@ "precondition_unavailable", "approver_unverified", "approver_is_requester", + # **Items 3 and 4's reasons, and their absence was the same defect one item later.** + # The paragraph above records `approval_denied` landing in no bucket and being fixed + # here; `approver_unentitled` and `approvals_unverifiable` were then coined without + # being added here, so an observe-mode run that would have refused an unentitled + # approver reported `would_have_been_blocked = 0`. A set maintained by hand is a set + # the next reason is missed from, which is why + # `test_every_approval_refusal_reason_is_counted_by_stats` enumerates them from + # `approval.py` instead of restating them. + APPROVER_UNENTITLED, + APPROVALS_UNVERIFIABLE, + APPROVAL_UNRECORDED, } ) @@ -546,7 +562,7 @@ def from_dict(cls, document: Mapping[str, Any]) -> Receipt: principal=Principal( agent=principal["agent"], user=principal["user"], - claims=principal.get("claims") or {}, + claims=_claims_of(principal.get("claims")), issuer=principal.get("issuer"), expires_at=None if expires_at is None else datetime.fromisoformat(expires_at), ), @@ -604,6 +620,33 @@ def from_json(cls, line: str) -> Receipt: return cls.from_dict(document) +def _claims_of(value: object) -> dict[str, Any]: + """`Principal.claims` out of a document, never raising (`v0.7 §6.11`, SPEC-v0.8 §3.4). + + `_frozen_claims` refuses a claim JSON can hold — an array of numbers, a float, a nested + object — and `Principal` runs it on construction, so a receipt carrying one raised out of + `from_dict` and blinded every reader of the chain rather than the one field. Dropped here + for `_approvers_of`'s reason, and the drop is visible: the receipt reads back with fewer + claims than the principal that produced it, and its stored hash no longer matches. + """ + if not isinstance(value, Mapping): + return {} + # `bool | int | str` and no float branch: a float is not an `int` in Python, so it falls + # through to the drop below with every other shape. A branch that cannot fire is + # documentation, not defence. + kept: dict[str, Any] = {} + for name, claim in value.items(): + if not isinstance(name, str) or not name: + continue + if isinstance(claim, bool | int | str): + kept[name] = claim + elif isinstance(claim, list | tuple) and all( + isinstance(item, str) and item for item in claim + ): + kept[name] = tuple(claim) + return kept + + def _approvers_of(value: object) -> tuple[VerifiedApprover, ...]: """`Receipt.approvers` out of a document, never raising (SPEC-v0.8 §2.5, `v0.7 §6.11`). diff --git a/src/ctrlrun/state.py b/src/ctrlrun/state.py index fe2c5fb..70b1df8 100644 --- a/src/ctrlrun/state.py +++ b/src/ctrlrun/state.py @@ -37,10 +37,12 @@ ApprovalRequest, ApprovalStatus, ApprovalStore, + RequiredRole, VerifiedApprover, _verified_approver_now, check_answerable, check_consumable, + count_grant, ) from .effect import ( COMMITTED_EFFECT, @@ -735,6 +737,26 @@ def close(self) -> None: ... +def _roles_json(roles: tuple[RequiredRole, ...]) -> str | None: + """The roles a request pinned, as canonical JSON, or `None` where it pinned none.""" + if not roles: + return None + return json.dumps([role.to_dict() for role in roles], sort_keys=True) + + +def _roles_from_json(text: str | None) -> tuple[RequiredRole, ...]: + """What the column holds, or `()`. A corrupted column raises, for `_approvers_from_json`'s + reason: this is authority about to be spent, not evidence a reader walks past.""" + if not text: + return () + try: + return tuple(RequiredRole.from_dict(item) for item in json.loads(text)) + except (ValueError, TypeError, KeyError, AttributeError) as exc: + raise InvalidArgument( + f"the approvals row carries an unreadable 'required_roles' column: {exc}" + ) from exc + + def _approvers_json(approvers: tuple[VerifiedApprover, ...]) -> str | None: """The verified approvers as one canonical JSON array, or `None` where there are none. @@ -906,7 +928,7 @@ def find_denied_request(self, action_hash: str) -> ApprovalRequest | None: records = list(self._approvals.values()) return _newest_denied(records, action_hash, now) - def grant_approval(self, approval_id: str, approver: str) -> Approval: + def grant_approval(self, approval_id: str, approver: str) -> Approval | None: approver = _approver(approver) with self._lock: record = self._answerable(approval_id) @@ -914,16 +936,20 @@ def grant_approval(self, approval_id: str, approver: str) -> Approval: # SPEC-v0.8 §2.5: whatever the granting surface verified, or nothing where it # verified nobody. A store that did not read this records no approver, and that is # refused at consumption rather than skipped. + # + # SPEC-v0.8 §4.2: and `count_grant` decides whether this grant reaches the + # threshold, in the one implementation all three stores apply. verified = _verified_approver_now(now) + approvers, reached = count_grant(record, verified, now) granted = replace( record, - status=ApprovalStatus.GRANTED, - approver=approver, - granted_at=now, - approvers=(*record.approvers, verified) if verified else record.approvers, + status=ApprovalStatus.GRANTED if reached else record.status, + approver=approver if reached else record.approver, + granted_at=now if reached else record.granted_at, + approvers=approvers, ) self._approvals[approval_id] = granted - return granted.as_approval() + return granted.as_approval() if reached else None def deny_approval(self, approval_id: str, approver: str) -> None: approver = _approver(approver) @@ -1460,8 +1486,8 @@ def put_approval_request(self, request: ApprovalRequest) -> None: try: self._connection().execute( "INSERT INTO approvals(approval_id, action_hash, status, action_json, " - "created_at, expires_at, policy_hash_at_approval, precondition_fingerprint) " - "VALUES(?,?,?,?,?,?,?,?)", + "created_at, expires_at, policy_hash_at_approval, precondition_fingerprint, " + "required_roles, approvals_required) VALUES(?,?,?,?,?,?,?,?,?,?)", ( request.request_id, request.action_hash, @@ -1471,6 +1497,8 @@ def put_approval_request(self, request: ApprovalRequest) -> None: _iso(request.expires_at), request.policy_hash, request.precondition_fingerprint, + _roles_json(request.required_roles), + request.approvals_required, ), ) except sqlite3.IntegrityError as exc: @@ -1584,31 +1612,34 @@ def approvals_for(self, action_hash: str) -> tuple[ApprovalRecord, ...]: found = (self._read_approval(connection, row["approval_id"]) for row in rows) return tuple(record for record in found if record is not None) - def grant_approval(self, approval_id: str, approver: str) -> Approval: + def grant_approval(self, approval_id: str, approver: str) -> Approval | None: approver = _approver(approver) connection = self._connection() now = self._clock() connection.execute("BEGIN IMMEDIATE") try: record = self._answerable(connection, approval_id, now) - # SPEC-v0.8 §2.5: the verified approver, appended to whatever the row already - # holds, inside the same `BEGIN IMMEDIATE` that serialises the status transition. + # SPEC-v0.8 §2.5, §4.2: the verified approver and the count, inside the same + # `BEGIN IMMEDIATE` that serialises the read and the write. That serialisation is + # what makes the count a property of the store's write rather than of a read + # followed by one (§4.3). verified = _verified_approver_now(now) - approvers = (*record.approvers, verified) if verified else record.approvers + approvers, reached = count_grant(record, verified, now) + status = ApprovalStatus.GRANTED if reached else record.status granted = replace( record, - status=ApprovalStatus.GRANTED, - approver=approver, - granted_at=now, + status=status, + approver=approver if reached else record.approver, + granted_at=now if reached else record.granted_at, approvers=approvers, ) connection.execute( "UPDATE approvals SET status=?, approver=?, granted_at=?, approvers=? " "WHERE approval_id=?", ( - str(ApprovalStatus.GRANTED), - approver, - _iso(now), + str(status), + granted.approver, + _iso(granted.granted_at) if granted.granted_at else None, _approvers_json(approvers), approval_id, ), @@ -1617,7 +1648,7 @@ def grant_approval(self, approval_id: str, approver: str) -> Approval: self._unwind(connection) raise connection.commit() - return granted.as_approval() + return granted.as_approval() if reached else None def deny_approval(self, approval_id: str, approver: str) -> None: approver = _approver(approver) @@ -1676,6 +1707,13 @@ def _read_approval( expires_at=datetime.fromisoformat(row["expires_at"]), policy_hash=row["policy_hash_at_approval"], precondition_fingerprint=row["precondition_fingerprint"], + required_roles=_roles_from_json(row["required_roles"]), + # `if ... is None else` and never `or 1`: `or` swallows a tampered `0`, + # which `ApprovalRequest.__post_init__` exists to refuse (§4.2, §12). `None` + # is the honest absent value, written by every row predating the column. + approvals_required=( + 1 if row["approvals_required"] is None else row["approvals_required"] + ), ), status=ApprovalStatus(row["status"]), approver=row["approver"], diff --git a/src/ctrlrun/verify/guarantees.py b/src/ctrlrun/verify/guarantees.py index 2f278c4..0b71cc2 100644 --- a/src/ctrlrun/verify/guarantees.py +++ b/src/ctrlrun/verify/guarantees.py @@ -107,6 +107,14 @@ class Guarantee: "a moved fingerprint is refused", ("v0.1 §4.2", "v0.7 §8 T253", "v0.7 §8 T254"), ), + Guarantee( + "G17", + # 30 characters against `report._TITLE_WIDTH`'s 32. It says what the refusal is about and + # not what it prevents: what the kernel refuses is an approval whose **recorded** + # entitlement does not cover the role, and §3.8 is the paragraph that bounds the claim. + "an unentitled approver refused", + ("v0.6 §7.3", "v0.8 §10 T297", "v0.8 §10 T298"), + ), Guarantee( "G18", # 28 characters, because `report._TITLE_WIDTH` is 32 and a wider title breaks the @@ -117,6 +125,13 @@ class Guarantee: "the requester cannot approve", ("v0.3 §4.2", "v0.8 §10 T285", "v0.8 §10 T286"), ), + Guarantee( + "G19", + # 30 characters. "counts once" and not "is refused": a second yes from one principal is + # recorded and not rejected, and what it does not do is move the count (§4.2). + "one principal counts once", + ("v0.3 §4.2", "v0.8 §10 T311", "v0.8 §10 T310"), + ), ) #: By id, for `--only` and for the report. Insertion order is catalogue order. @@ -136,6 +151,16 @@ class Guarantee: # --- N/A reasons: statements about the configuration, never about a failed run (§2.1) --- NO_APPROVE_RULE: Final = "no action requires approval" + +#: SPEC-v0.8 §3.5, §11.7 — G17's own `N/A`, and a statement about the operator's **document**: +#: a control that names no `approver_role` gates nobody, which is the answer §3.5 gives and the +#: opposite of what a missing claim on a principal means. +NO_APPROVER_ROLE: Final = "no cited control names an approver role" + +#: SPEC-v0.8 §4.2, §11.7 — G19's own `N/A`, and a statement about the operator's **document**: +#: a document where every action takes one yes has no count to get wrong. Not "M-of-N is not +#: configured", which would be a sentence about a deployment verify cannot see. +NO_M_OF_N: Final = "no action requires more than one approval" NO_EFFECT_TEMPLATE: Final = "no action declares an `effect:` template" #: The sentence that makes G3's N/A actionable rather than mysterious (§2.2). It travels in @@ -267,6 +292,7 @@ class Guarantee: "GUARANTEES", "NOT_SELECTED", "NO_ACTIONS", + "NO_APPROVER_ROLE", "NO_APPROVE_RULE", "NO_AUTHORITY_SECTION", "NO_CEILING_DECLARED", diff --git a/src/ctrlrun/verify/scenarios.py b/src/ctrlrun/verify/scenarios.py index 5031bf6..217733f 100644 --- a/src/ctrlrun/verify/scenarios.py +++ b/src/ctrlrun/verify/scenarios.py @@ -47,7 +47,9 @@ ApprovalStatus, ApproverIdentity, LocalApprovalProvider, + RequiredRole, _granting_principal, + _required_roles, ) from ..authority import ( AUTHORITY_EXPIRED, @@ -730,6 +732,8 @@ def select( needs_effect: bool = False, needs_renewal: bool = False, needs_ceiling: bool = False, + needs_approver_role: bool = False, + needs_threshold: bool = False, ceiling_bound: int | None = None, grant_filter: Callable[[Grant], bool] | None = None, mutation: Mapping[str, Any] | None = None, @@ -754,6 +758,16 @@ def select( ceiling = self.policy.max_attempts(name) if needs_renewal and ceiling is not None and ceiling < 2: continue + if needs_approver_role and not self._roles_for(name): + # SPEC-v0.8 §3.5, for G17: an action whose cited controls name no role gates + # nobody, so it cannot exercise the refusal, and picking it would make G17's + # `N/A` reason a statement about this selection rather than about the document. + continue + if needs_threshold and self.policy.approvals_required(name) < 2: + # SPEC-v0.8 §4.2, for G19: an action that takes one yes has no count to get + # wrong. Selecting it would make G19's `N/A` reason a statement about this + # selection rather than about the document, which is §11.7's rule. + continue if needs_ceiling and ceiling is None: continue if ceiling_bound is not None and ceiling is not None and ceiling > ceiling_bound: @@ -859,6 +873,38 @@ def control( ) return control, store, recorder, moving + def _roles_for(self, action_name: str) -> tuple[RequiredRole, ...]: + """The roles this action's cited controls require (SPEC-v0.8 §3.3), by name. + + Off the entry's own citations rather than an evaluation, so it can be asked before a + selection exists: `§3.5`'s question is whether the **document** gates anything. + """ + entry = self.policy.actions.get(action_name) + cited = () if entry is None else entry.controls + return tuple( + RequiredRole(control=identifier, role=control.approver_role) + for identifier, control in ( + (identifier, self.policy.controls.get(identifier)) for identifier in cited + ) + if control is not None and control.approver_role + ) + + def _required_roles(self, selection: _Selection) -> tuple[RequiredRole, ...]: + """The roles this selection's decision would pin on a request (SPEC-v0.8 §3.3). + + Read from the same evaluation `Control` reads, so verify grades what the deployment would + do rather than a rule of its own. + """ + evaluation = self.policy.evaluate(selection.build()) + return tuple( + RequiredRole(control=identifier, role=control.approver_role) + for identifier, control in ( + (identifier, self.policy.controls.get(identifier)) + for identifier in evaluation.controls + ) + if control is not None and control.approver_role + ) + def _control_for( self, gid: str, @@ -3018,6 +3064,111 @@ def body(detail: dict[str, Any]) -> None: finally: store.close() + # --- G17: an unentitled approver is refused ------------------------------------------- + + def g17(self) -> GuaranteeResult: + """SPEC-v0.8 §3.6, §11.7. Graded where the document names an approver role. + + **The `N/A` reason is about the document**, which is what `verify/guarantees.py` requires + of every reason in it: a policy whose cited controls name no `approver_role` gates nobody, + which is §3.5's answer and a true statement about what the operator wrote. Whether that + operator configured an approver identity is a fact about their application, which verify + cannot see and which it therefore supplies for itself (§11.7). + + Both halves, `v0.4 §1.3`. The observable: an approval recorded without the control's role + is refused, and the refusal names the control. The control: the same action, approved by + an approver the role covers, commits. A check that refused every approval would pass the + first and fail the second. + """ + selection = self.select(decisions=(Decision.APPROVE,), needs_approver_role=True) + if selection is None: + # **Over the whole document, not over one selection.** `select` is deterministic by + # codepoint, so on a document with two approve rules where the ungated one sorts + # first, asking about that one alone would report "no cited control names an + # approver role" of a document that gates entitlement. `v0.7 §8.9` makes an untrue + # `N/A` reason a false green, and every reason here is a statement about the + # operator's document. + if self.select(decisions=(Decision.APPROVE,)) is None: + return self.na("G17", self.unselected(reg.NO_APPROVE_RULE)) + return self.na("G17", reg.NO_APPROVER_ROLE) + roles = self._required_roles(selection) + wanted = roles[0] + approver = Principal( + agent=f"{selection.principal.agent}-approver", + user=selection.principal.user, + issuer=selection.principal.issuer, + ) + identity = ApproverIdentity(_VerifyApproverProvider(approver)) + control, store, recorder, _ = self._control_for( + "G17", selection, approver_identity=identity + ) + + def body(detail: dict[str, Any]) -> None: + detail["approved_by_verify"] = True + detail["required_role"] = f"{wanted.control}:{wanted.role}" + action = selection.build() + # **Through the pinning route, not straight from the provider.** `_REQUIRED_ROLES` is + # set inside `Control._presented`, so a request built from `control.approvals.request` + # pins nothing, `unsatisfied((), ...)` refuses nothing, and this scenario would FAIL + # on every document that gates anything while passing on the shipped examples, which + # are `N/A`. §14.3 records the same lesson for the operator tests; verify's own + # scenario is where it was not applied. + with _required_roles(roles, self.policy.approvals_required(selection.action)): + request = control.approvals.request(action, DEFAULT_APPROVAL_TTL) + # Recorded entitled for nothing, which is what a surface that verified the credential + # and found no role records (§3.8). + with _granting_principal(approver): + store.grant_approval(request.request_id, APPROVER) + executor = _Executor() + refusal = self.refused( + lambda: self.execute( + control, action, executor, selection.effect_key, request.request_id + ), + (ApprovalMismatch,), + "ApprovalMismatch(reason='approver_unentitled')", + "an action ran under an approval nobody was recorded as entitled to give", + ) + reason = getattr(refusal, "reason", "") + _expect( + reason == "approver_unentitled", + "ApprovalMismatch(reason='approver_unentitled')", + f"ApprovalMismatch(reason={reason!r})", + ) + _expect( + executor.calls == 0, + "the executor is not reached", + f"the executor was called {executor.calls} times", + ) + _expect( + _named_event(recorder, EventType.APPROVAL_INVALIDATED, reason="approver_unentitled") + and any( + event.data.get("control") == wanted.control + for event in recorder.events + if event.type is EventType.APPROVAL_INVALIDATED + ), + f"APPROVAL_INVALIDATED naming control {wanted.control!r}", + f"events were {recorder.types()}", + ) + + with _required_roles(roles, self.policy.approvals_required(selection.action)): + second = control.approvals.request(selection.build(), DEFAULT_APPROVAL_TTL) + with _granting_principal(approver, entitled=[role.control for role in roles]): + store.grant_approval(second.request_id, APPROVER) + committed = _Executor() + receipt = self.execute( + control, action, committed, selection.effect_key, second.request_id + ) + _expect_control( + receipt.result is ReceiptResult.COMMITTED and committed.calls == 1, + "the same action, approved by an entitled approver, commits", + f"it ended {receipt.result} after {committed.calls} executor calls", + ) + + try: + return self.graded("G17", selection, store, recorder, body) + finally: + store.close() + # --- G18: an approver who is the requester is refused --------------------------------- def g18(self) -> GuaranteeResult: @@ -3110,6 +3261,114 @@ def body(detail: dict[str, Any]) -> None: finally: store.close() + # --- G19: one principal counts once --------------------------------------------------- + + def g19(self) -> GuaranteeResult: + """SPEC-v0.8 §4.2, §11.7. Graded where the document asks for more than one approval. + + **The `N/A` reason is about the document**: an action that takes one yes has no count to + get wrong, which is a true statement about what the operator wrote and not a claim about + a deployment verify cannot see (§11.7). + + Both halves, `v0.4 §1.3`. The observable: one principal answers twice, through two doors + and under two different approver strings, and the action is still refused as `pending`. + The control: N distinct principals answer and it commits. A count that never reached N + would pass the first and fail the second, and a count that moved on the duplicate would + fail the first, which is why neither half is evidence alone. + + The two grants carry **different `approver` strings**, because a scenario in which the + strings match proves only that the row was deduplicated on a string, and §4.2's rule is + about the resolved principal. + """ + selection = self.select(decisions=(Decision.APPROVE,), needs_threshold=True) + if selection is None: + # Over the whole document, exactly as G17 does it: a document with an ungated + # approve rule sorting before a gated one would otherwise report the threshold + # reason about a document that does name one. + if self.select(decisions=(Decision.APPROVE,)) is None: + return self.na("G19", self.unselected(reg.NO_APPROVE_RULE)) + return self.na("G19", reg.NO_M_OF_N) + needed = self.policy.approvals_required(selection.action) + roles = self._required_roles(selection) + entitled = [role.control for role in roles] + approvers = tuple( + Principal( + agent=f"{selection.principal.agent}-approver-{index}", + user=selection.principal.user, + issuer=selection.principal.issuer, + ) + for index in range(1, needed + 1) + ) + identity = ApproverIdentity(_VerifyApproverProvider(approvers[0])) + control, store, recorder, _ = self._control_for( + "G19", selection, approver_identity=identity + ) + + def body(detail: dict[str, Any]) -> None: + detail["approved_by_verify"] = True + detail["approvals_required"] = needed + action = selection.build() + # Through the pinning route: `_APPROVALS_REQUIRED` is read inside `build_request`, + # so a request built outside `Control._presented` would pin 1 and this scenario + # would grade a threshold the document does not ask for (§14.3). + with _required_roles(roles, needed): + request = control.approvals.request(action, DEFAULT_APPROVAL_TTL) + for door in ("mcp-operator", "cli"): + with _granting_principal(approvers[0], entitled=entitled): + store.grant_approval(request.request_id, f"{door}:{APPROVER}") + record = store.get_approval(request.request_id) + _expect( + record is not None and len(record.approvers) == 1, + "one principal answering twice is recorded once", + f"the row carries {0 if record is None else len(record.approvers)} approvers", + ) + _expect( + record is not None and record.status is ApprovalStatus.PENDING, + f"the request is still pending at 1 of {needed}", + f"the request is {None if record is None else record.status}", + ) + executor = _Executor() + refusal = self.refused( + lambda: self.execute( + control, action, executor, selection.effect_key, request.request_id + ), + (ApprovalMismatch,), + "ApprovalMismatch(reason='pending')", + "an action ran on a count one principal reached alone", + ) + reason = getattr(refusal, "reason", "") + _expect( + reason == "pending", + "ApprovalMismatch(reason='pending')", + f"ApprovalMismatch(reason={reason!r})", + ) + _expect( + executor.calls == 0, + "the executor is not reached", + f"the executor was called {executor.calls} times", + ) + + # The control: N distinct principals, and the same action commits. + with _required_roles(roles, needed): + second = control.approvals.request(selection.build(), DEFAULT_APPROVAL_TTL) + for index, approver in enumerate(approvers, start=1): + with _granting_principal(approver, entitled=entitled): + store.grant_approval(second.request_id, f"{APPROVER}-{index}") + committed = _Executor() + receipt = self.execute( + control, action, committed, selection.effect_key, second.request_id + ) + _expect_control( + receipt.result is ReceiptResult.COMMITTED and committed.calls == 1, + f"the same action, approved by {needed} distinct principals, commits", + f"it ended {receipt.result} after {committed.calls} executor calls", + ) + + try: + return self.graded("G19", selection, store, recorder, body) + finally: + store.close() + #: G12's loopback address: the literal, never `localhost` and never `::1` (SPEC-v0.7 §8.9). _LOOPBACK: Final = "127.0.0.1" diff --git a/src/ctrlrun/webhook.py b/src/ctrlrun/webhook.py index 42a2b0d..618bc9e 100644 --- a/src/ctrlrun/webhook.py +++ b/src/ctrlrun/webhook.py @@ -352,7 +352,25 @@ def handle_inbound( # get to weaken it: `grant_approval` refuses a record that is consumed, denied or # expired, so a replay arriving *after* consumption cannot resurrect anything. if decision == "grant": - store.grant_approval(path_request_id, approver) + # SPEC-v0.8 §4.4: `None` is **recorded and still short of N**, not a failure. 200 + # either way, because the answer was recorded either way, and the body says which: + # a sender told a flat "ok" for an answer that moved nothing has been told the + # opposite of what happened. + granted = store.grant_approval(path_request_id, approver) + if granted is None: + after = store.get_approval(path_request_id) + recorded = 0 if after is None else len(after.approvers) + needed = 1 if after is None else after.request.approvals_required + if recorded == 0: + # §2.6 and §4.5: this endpoint resolves nobody, so its answer carries no + # verified approver and counts toward no threshold. "recorded: 0 of 2" + # alone reads as a bug in the count rather than a fact about this door. + return 200, ( + f"recorded: {recorded} of {needed} approvals; this endpoint verifies " + "no approver, so its answer counts toward no threshold " + "(SPEC-v0.8 §2.6)" + ) + return 200, f"recorded: {recorded} of {needed} approvals" else: store.deny_approval(path_request_id, approver) except CTRLRunError as refused: diff --git a/tests/test_approver.py b/tests/test_approver.py index 3e0a5f7..c92ee7a 100644 --- a/tests/test_approver.py +++ b/tests/test_approver.py @@ -727,10 +727,10 @@ def test_T283b_a_corrupted_entitled_column_is_refused_and_not_exploded(store, cl assert fine.entitled == ("card-data-handling",) -# --- T297: the resumed leg is the only receipt some actions get ------------------------------ +# --- T296c: the resumed leg is the only receipt some actions get ------------------------------ -def test_T297_a_resumed_leg_carries_the_approvers_onto_its_receipt(store, clock): +def test_T296c_a_resumed_leg_carries_the_approvers_onto_its_receipt(store, clock): """§2.5 and `SPEC-mcp-operator.md` §8.3, which is why this is not a nicety. A suspended action writes no receipt on the leg that suspended, so the resumed leg's is the diff --git a/tests/test_attempt_integrity.py b/tests/test_attempt_integrity.py index cbe8950..2664d67 100644 --- a/tests/test_attempt_integrity.py +++ b/tests/test_attempt_integrity.py @@ -40,10 +40,17 @@ import pytest +from ctrlrun.action import Action, Principal from ctrlrun.effect import EffectState from ctrlrun.errors import CTRLRunError from failure_injection import Proxy, statement_of, upstream_of +#: Every window in this file is opened by the proxy -- the armed hold -- and measured against +#: what a store did inside it. Sharing a machine with seven other pytest workers turns +#: those into races the test loses: T155b reported "the window never opened", which was +#: true. `scripts/check.sh` runs these on their own. +pytestmark = pytest.mark.serial + URL = os.environ.get("CTRLRUN_TEST_POSTGRES") postgres = pytest.mark.skipif( @@ -93,7 +100,7 @@ def emit(self, record): log.addHandler(Branches()) log.setLevel(logging.WARNING) - OPS = ("reserve", "begin", "fail", "commit", "ambiguous", "resolve") + OPS = ("reserve", "begin", "fail", "commit", "ambiguous", "resolve", "grant") for step in job["steps"]: if step["op"] not in OPS: raise SystemExit("unknown step %r" % step["op"]) @@ -133,6 +140,18 @@ def emit(self, record): store.commit_effect(key, actor, {"refund": "re_1"}) elif op == "ambiguous": store.mark_ambiguous(key, actor, "the outcome was lost") + elif op == "grant": + # SPEC-v0.8 §4.3, T313. The principal is the step's, because the whole + # question is whether two *distinct* ones both survive the window. + from ctrlrun.action import Principal + from ctrlrun.approval import _granting_principal + + who = Principal(agent=step["agent"], user=step["user"]) + with _granting_principal(who, entitled=step.get("entitled") or ()): + approval = store.grant_approval(step["approval_id"], step["who"]) + done["state"] = "granted" if approval is not None else "pending" + record = store.get_approval(step["approval_id"]) + done["attempt"] = None if record is None else len(record.approvers) else: resolved = store.resolve_effect(key, EffectState(step["to"]), step["resolver"]) done["attempt"] = resolved.attempt @@ -978,6 +997,115 @@ def stale_write(proxy, home, schema: str, key: str, held_steps, rival_steps): return outcome, rival, read +# --- T313: the M-of-N count, in the window between its read and its write --------------------- + + +@postgres +def test_T313_two_processes_granting_in_the_window_produce_two_approvers(proxy, schema, home): + """SPEC-v0.8 §4.3. The window is between the read of `approvers` and the update of it. + + Two OS processes, two distinct verified principals, one request needing two yeses. The proxy + holds ALICE's `UPDATE` after her connection has already read the row — an empty approver + list — and BOB's grant lands in that window. When ALICE's update is released it must not + write the list she read over the one BOB wrote. + + **It fails against a compare-and-set on `status`,** which is the shape the store had and the + shape a reviewer found: `status` is still `pending` when ALICE's held update lands, so the + condition holds, her write succeeds, and the row ends with one approver and a threshold of + two that a third yes would have to fill. Nobody would ever know: two humans answered and the + record says one did. + + `CONTRIBUTING.md`'s fourth mutation shape is the reason this test exists at all. Item 4's serial + tests pass against a store with no compare-and-set whatever, because a read and a write with + nothing in between is correct right up until something *is* in between. + """ + from ctrlrun.approval import RequiredRole, _required_roles, build_request + + request_id = None + setup = direct(schema) + try: + action = Action( + name="payments.refund", + arguments={"amount": 100, "payment_id": "EU-42"}, + principal=Principal(agent="ops-agent", user="ada"), + environment="production", + ) + with _required_roles((RequiredRole(control="c1", role="payments-owner"),), 2): + request = build_request(action, timedelta(minutes=15), T0) + setup.put_approval_request(request) + request_id = request.request_id + finally: + setup.close() + + def grant(name: str, agent: str, at: datetime) -> dict: + return step( + "grant", + "", + "", + at, + approval_id=request_id, + who=f"mcp-operator:{name}", + agent=agent, + user=f"{name}@example.com", + entitled=["c1"], + ) + + held = Child( + home, + "alice", + url=proxy.url(URL), + schema=schema, + gated=True, + steps=[grant("alice", "human:alice", T0 + timedelta(seconds=1))], + ) + try: + held.wait_ready() + proxy.reset_counters() + proxy.arm(approvals_statement(b"UPDATE", schema)) + held.go() + assert proxy.holding.wait(BOUND), ( + "alice's UPDATE never reached the proxy, so the window between the count's read and " + "its write was never opened and this test has proved nothing" + ) + rival = Child( + home, + "bob", + url=URL, + schema=schema, + gated=False, + steps=[grant("bob", "human:bob", T0 + timedelta(seconds=2))], + ).result() + assert rival["error"] is None, f"bob did not complete: {rival}" + assert rival["results"][0]["attempt"] == 1, ( + f"bob's grant recorded {rival['results'][0]['attempt']} approvers, not 1: the window " + "was opened somewhere other than where this test believes" + ) + assert held.running(), "alice finished while her UPDATE was being held" + proxy.release() + outcome = held.result() + finally: + proxy.release() + held.kill() + + assert proxy.holds == 1 + assert outcome["error"] is None, f"alice's grant was refused: {outcome}" + + after = direct(schema) + try: + record = after.get_approval(request_id) + finally: + after.close() + assert record is not None + agents = sorted(approver.agent for approver in record.approvers) + assert agents == ["human:alice", "human:bob"], ( + f"the row ended with {agents}: one human's yes was written over the other's inside the " + "window, and the request still needs a third answer that two people already gave" + ) + assert str(record.status) == "granted", ( + f"two distinct principals answered and the request is {record.status}" + ) + + REUSED = "act_reused" diff --git a/tests/test_break_glass.py b/tests/test_break_glass.py new file mode 100644 index 0000000..323d226 --- /dev/null +++ b/tests/test_break_glass.py @@ -0,0 +1,892 @@ +"""T326 to T339: break-glass as a grant, never a flag (SPEC-v0.8 §5). + +Parametrised over the three stores for the same reason every authority test is: an envelope is +a document, but everything opened beneath one is a row, and a store that loses the parent link +loses the containment the envelope exists to impose. +""" + +from __future__ import annotations + +import os +import pathlib +import uuid +from datetime import UTC, datetime, timedelta +from typing import Any + +import pytest + +from ctrlrun.action import Action, Principal +from ctrlrun.approval import ApproverIdentity +from ctrlrun.authority import Authority, canonical_grants, grant_from_yaml +from ctrlrun.control import Control +from ctrlrun.errors import AuthorityEscalation, InvalidArgument, PolicyError +from ctrlrun.identity import StaticIdentityProvider +from ctrlrun.policy import Policy +from ctrlrun.state import InMemoryStateStore, SQLiteStateStore + +pytestmark = pytest.mark.authority + +POSTGRES_URL = os.environ.get("CTRLRUN_TEST_POSTGRES") + +POLICY = """ +schema: ctrlrun.policy/v6 +environment: prod +controls: + incident-response: + title: Only an incident commander opens break-glass + approver_role: incident-commander +actions: + payments.refund: + decision: allow +authority: + grants: + - id: everyday + subject: {agent: "ops-agent"} + actions: ["payments.read"] + break_glass: + incident-payments: + subject: {agent: "oncall-*"} + actions: ["payments.*"] + environments: ["prod"] + resources: ["payment:*"] + constraints: {amount_lte: 50000} + max_ttl: PT4H + controls: [incident-response] +""" + +NOW = datetime(2026, 9, 12, 12, 0, tzinfo=UTC) +KEY = "refund:EU-42" +COMMANDER = Principal( + agent="human:ada", + user="ada@example.com", + issuer="https://issuer.example", + claims={"roles": ("incident-commander",)}, +) +BYSTANDER = Principal(agent="human:bob", user="bob@example.com", claims={"roles": ("viewer",)}) +ONCALL = Principal(agent="oncall-agent", user="ada") + + +class _Clock: + def __init__(self, now: datetime = NOW) -> None: + self.now = now + + def __call__(self) -> datetime: + return self.now + + def advance(self, by: timedelta) -> None: + self.now += by + + +class _Fixed: + """An approver identity provider. Refuses what a real one refuses: it reads a credential + it was given and invents nobody.""" + + def __init__(self, principal: Principal | None) -> None: + self._principal = principal + + def resolve(self, context: Any) -> Principal | None: + return self._principal + + +class _Executor: + def __init__(self) -> None: + self.calls = 0 + + def __call__(self) -> dict[str, str]: + self.calls += 1 + return {"ok": "yes"} + + +@pytest.fixture +def clock() -> _Clock: + return _Clock() + + +@pytest.fixture( + params=[ + "in-memory", + "sqlite", + pytest.param( + "postgres", + marks=pytest.mark.skipif( + POSTGRES_URL is None, reason="CTRLRUN_TEST_POSTGRES is not set" + ), + ), + ] +) +def store(request, tmp_path, clock): + if request.param == "in-memory": + made = InMemoryStateStore(clock=clock) + elif request.param == "sqlite": + made = SQLiteStateStore(tmp_path / "state.db", clock=clock) + else: + from ctrlrun.postgres import PostgresStateStore + + schema = f"glass_{uuid.uuid4().hex[:12]}" + PostgresStateStore.create_schema(POSTGRES_URL, schema) + made = PostgresStateStore(POSTGRES_URL, schema=schema, clock=clock) + yield made + made.close() + if request.param == "postgres": + from ctrlrun.postgres import PostgresStateStore + + PostgresStateStore.drop_schema(POSTGRES_URL, schema) + + +def _control(store, clock, *, opener=COMMANDER, policy=POLICY): + return Control( + Policy.from_yaml(policy), + store, + clock=clock, + authority=Authority.from_yaml(policy), + approver_identity=( + None if opener is False else ApproverIdentity(_Fixed(opener), roles_claim="roles") + ), + identity=StaticIdentityProvider(agent=ONCALL.agent, user=ONCALL.user), + ) + + +def _grant(expires: datetime | None = None, **overrides: Any): + document = { + "subject": {"agent": "oncall-agent"}, + "actions": ["payments.refund"], + "environments": ["prod"], + "resources": ["payment:EU-*"], + "constraints": {"amount_lte": 1000}, + } + document.update(overrides) + if expires is not None: + document["expires_at"] = expires.isoformat() + lines = _yaml(document) + return grant_from_yaml(lines) + + +def _yaml(document: dict[str, Any]) -> str: + import yaml + + return yaml.safe_dump(document) + + +def _action(control, amount: int = 100) -> Action: + return Action( + name="payments.refund", + arguments={"amount": amount, "payment_id": "EU-42"}, + principal=ONCALL, + resource="payment:EU-42", + environment=control.environment, + ) + + +# --- T326, T326b: it is created, and it authorises ------------------------------------------- + + +def test_T326_a_break_glass_grant_is_created_beneath_its_envelope(store, clock): + """§5.3. Recorded, with a parent, a depth and a provenance that says what it was.""" + control = _control(store, clock) + + opened = control._break_glass( + "incident-payments", _grant(clock.now + timedelta(hours=2)), reason="INC-4412" + ) + + assert opened.parent_id == "incident-payments" + assert opened.depth == 1 + assert opened.created_via == "break-glass" + assert opened.created_by.agent == COMMANDER.agent + record = store.get_delegation(opened.delegation_id) + assert record is not None and record.created_via == "break-glass" + + +def test_T326b_an_action_under_a_live_break_glass_grant_is_allowed(store, clock): + """§5.2 point 4, third site. **Created is not authorised**, and this is the difference. + + `_check_chain`'s rule 6 reads `delegable` over every ancestor including the root, on every + evaluation. An envelope carries no such key, so without the exemption the grant is created + and then authorises nothing, refused `authority_escalation` with no dimension named, which + is the least diagnosable refusal in `authority.py`. + """ + control = _control(store, clock) + control._break_glass("incident-payments", _grant(clock.now + timedelta(hours=2))) + executor = _Executor() + + receipt = control.execute(_action(control), executor, KEY) + + assert executor.calls == 1, "the grant authorised nothing" + assert str(receipt.result) == "committed" + + +# --- T327: it expires ------------------------------------------------------------------------ + + +def test_T327_after_its_expiry_the_action_is_denied(store, clock): + """§5.4. By the existing expiry check, with the existing reason.""" + from ctrlrun.errors import ActionDenied + + control = _control(store, clock) + control._break_glass("incident-payments", _grant(clock.now + timedelta(hours=2))) + assert control.execute(_action(control), _Executor(), KEY).result + + clock.advance(timedelta(hours=3)) + executor = _Executor() + with pytest.raises(ActionDenied) as refused: + control.execute(_action(control), executor, "refund:EU-43") + + assert refused.value.reason == "authority_expired" + assert executor.calls == 0 + + +# --- T328: an expiry is required, and bounded ------------------------------------------------ + + +def test_T328_a_grant_with_no_expiry_is_refused(store, clock): + """§5.3's one added rule. An ordinary delegation may carry none; this may not.""" + control = _control(store, clock) + + with pytest.raises(AuthorityEscalation) as refused: + control._break_glass("incident-payments", _grant(None)) + + assert refused.value.reason == "containment" + assert refused.value.dimension == "expires_at" + + +def test_T328_a_grant_beyond_max_ttl_is_refused(store, clock): + control = _control(store, clock) + + with pytest.raises(AuthorityEscalation) as refused: + control._break_glass("incident-payments", _grant(clock.now + timedelta(hours=5))) + + assert refused.value.reason == "containment" + assert refused.value.dimension == "expires_at" + assert list(store.delegations()) == [] or all( + record.parent_id != "incident-payments" for record in store.delegations() + ) + + +# --- T329: containment, one refusal per dimension -------------------------------------------- + + +@pytest.mark.parametrize( + ("overrides", "dimension"), + [ + ({"subject": {"agent": "*"}}, "subject"), + ({"actions": ["*"]}, "actions"), + ({"environments": ["prod", "staging"]}, "environments"), + ({"constraints": {"amount_lte": 90000}}, "constraints"), + ({"resources": ["*"]}, "resources"), + ], +) +def test_T329_a_grant_wider_than_its_envelope_is_refused_per_dimension( + store, clock, overrides, dimension +): + """§5.3, driven through `contained_dimension` so a dimension added later cannot escape. + + `expires_at` is the sixth and has its own test above, because an envelope carries none and + what bounds a child in time is `max_ttl`. + """ + control = _control(store, clock) + + with pytest.raises(AuthorityEscalation) as refused: + control._break_glass( + "incident-payments", _grant(clock.now + timedelta(hours=1), **overrides) + ) + + assert refused.value.reason == "containment" + assert refused.value.dimension == dimension + + +def test_T329_every_dimension_contained_dimension_knows_has_a_case(): + """The test that keeps the table above honest (SPEC-v0.8 §10.5). + + A dimension added to `contained_dimension` and not to the parameters above would be a + dimension nothing drives, which is exactly how a containment check comes to be decoration. + """ + import inspect + + from ctrlrun.authority import contained_dimension + + source = inspect.getsource(contained_dimension) + named = { + word.strip("\"'") + for word in source.split() + if word.strip("\"',()") + in { + "subject", + "actions", + "resources", + "constraints", + "environments", + "expires_at", + } + } + covered = {"subject", "actions", "environments", "constraints", "resources", "expires_at"} + missing = {word.strip("\"',()") for word in named} - covered + assert not missing, f"contained_dimension knows {missing}, and no case drives them" + + +# --- T330: the envelope decides nothing ------------------------------------------------------ + + +def test_T330_the_envelope_is_not_a_candidate(store, clock): + """§5.2, **by construction**: it is not in `Authority._grants`, so `_candidates` cannot + return it. Asserted as absence from the set and not merely as a denial, because a grant + that is present and unmatched passes a denial test too.""" + authority = Authority.from_yaml(POLICY) + + assert "incident-payments" not in authority.grants + assert "incident-payments" in authority.envelopes + candidates = {grant_id for grant_id, _, _ in authority._candidates(store)} + assert "incident-payments" not in candidates + + +def test_T330_a_deployment_with_only_an_envelope_evaluates_as_0_7_0(store, clock): + """The behavioural half: an envelope authorises nothing on its own.""" + from ctrlrun.errors import ActionDenied + + control = _control(store, clock) + executor = _Executor() + + with pytest.raises(ActionDenied) as refused: + control.execute(_action(control), executor, KEY) + + assert refused.value.reason == "no_authority" + assert executor.calls == 0 + + +# --- T331: the walk resolves an envelope root ------------------------------------------------ + + +def test_T331_an_unknown_envelope_is_refused_by_name(store, clock): + control = _control(store, clock) + + with pytest.raises(AuthorityEscalation) as refused: + control._break_glass("no-such-envelope", _grant(clock.now + timedelta(hours=1))) + + assert refused.value.reason == "unknown_parent" + assert "no-such-envelope" in str(refused.value) + + +# --- T332: the policy hash covers the envelope ----------------------------------------------- + + +def test_T332_widening_max_ttl_moves_the_policy_hash(): + """§5.2. The argument for declaring the envelope in the policy is that it was evidenced + before the incident, and that is only true if widening it moves the hash.""" + from ctrlrun.action import canonical_bytes + + narrow = canonical_bytes(canonical_grants(Authority.from_yaml(POLICY))) + wide = canonical_bytes(canonical_grants(Authority.from_yaml(POLICY.replace("PT4H", "PT8H")))) + + assert narrow != wide + + +def test_T332_the_envelope_hashes_as_the_document_wrote_it(): + """And the read-site rule of §5.2 point 4 does not move it. + + An envelope renders `delegable: false`, the parser default every grant omitting the key + already hashes as. The runtime rule that an envelope ancestor *counts as* delegable is + applied where `delegable` is read and never written onto the parsed grant, so the hash + stays a statement about the document. + """ + rendered = canonical_grants(Authority.from_yaml(POLICY)) + assert isinstance(rendered, dict) + envelope = rendered["break_glass"]["incident-payments"] + + assert envelope["delegable"] is False + assert envelope["max_ttl"] == 4 * 60 * 60 + assert envelope["controls"] == ["incident-response"] + + +# --- T332b: what an envelope may not carry, and where it may not live ------------------------ + + +@pytest.mark.parametrize("key", ["delegable", "expires_at"]) +def test_T332b_an_envelope_may_not_carry_delegable_or_expires_at(key): + value = "true" if key == "delegable" else "2026-01-01T00:00:00Z" + with pytest.raises(PolicyError) as refused: + Authority.from_yaml( + POLICY.replace(" max_ttl: PT4H", f" {key}: {value}\n max_ttl: PT4H") + ) + + assert key in str(refused.value) + + +def test_T332b_an_id_in_both_mappings_is_refused_naming_both(): + with pytest.raises(PolicyError) as refused: + Authority.from_yaml(POLICY.replace(" incident-payments:", " everyday:")) + + assert "everyday" in str(refused.value) + assert "grants" in str(refused.value) and "break_glass" in str(refused.value) + + +def test_T332b_a_standalone_authority_document_may_not_declare_break_glass(): + """§5.2: that shape has no control registry, so the only envelope it could express is an + ungated one, and the single deployment unable to state the gate would be the one whose + break-glass anybody verified could open.""" + document = POLICY[POLICY.index("authority:") :] + with pytest.raises(PolicyError) as refused: + Authority.from_yaml(f"schema: ctrlrun.policy/v6\n{document}", standalone=True) + + assert "break_glass" in str(refused.value) + assert "registry" in str(refused.value) + + +# --- T333: the receipt names the grant, break-glass or not ----------------------------------- + + +def test_T333_the_receipt_names_the_break_glass_grant(store, clock): + control = _control(store, clock) + opened = control._break_glass("incident-payments", _grant(clock.now + timedelta(hours=2))) + + receipt = control.execute(_action(control), _Executor(), KEY) + + assert receipt.authority_grant_id == opened.delegation_id + + +def test_T333_an_ordinary_grant_is_named_too(store, clock): + """§5.4: the field is not break-glass-specific. One that existed only under break-glass + would be one nothing exercises on the ordinary path.""" + policy = POLICY.replace('actions: ["payments.read"]', 'actions: ["payments.*"]').replace( + 'subject: {agent: "ops-agent"}', 'subject: {agent: "oncall-agent"}' + ) + control = _control(store, clock, policy=policy) + + receipt = control.execute(_action(control), _Executor(), KEY) + + assert receipt.authority_grant_id == "everyday" + + +# --- T334, T334b: who may open one ------------------------------------------------------------ + + +def test_T334_the_opener_is_the_resolved_principal_not_the_envelopes_subject(store, clock): + """§5.3.1. The envelope's subject is `oncall-*` and the opener is `human:ada`. Under rule 4 + as `plan_delegation` writes it that is `not_the_subject`; what gates the opener instead is + the envelope's `controls:`.""" + control = _control(store, clock) + + opened = control._break_glass("incident-payments", _grant(clock.now + timedelta(hours=1))) + + assert opened.created_by.agent == "human:ada" + assert opened.grant.subject.agent == "oncall-agent" + + +def test_T334_an_opener_without_the_control_role_is_refused(store, clock): + control = _control(store, clock, opener=BYSTANDER) + + with pytest.raises(AuthorityEscalation) as refused: + control._break_glass("incident-payments", _grant(clock.now + timedelta(hours=1))) + + assert refused.value.reason == "approver_unentitled" + assert "incident-response" in str(refused.value) + assert "incident-commander" in str(refused.value) + + +def test_T334_with_no_approver_identity_it_cannot_be_opened_at_all(store, clock): + """Opt in, then fail closed. With nobody resolved there is no principal to check the + envelope's controls against, and an unchecked opener is the flag §5 refuses.""" + control = _control(store, clock, opener=False) + + with pytest.raises(InvalidArgument) as refused: + control._break_glass("incident-payments", _grant(clock.now + timedelta(hours=1))) + + assert "approver identity" in str(refused.value) + + +def test_T334b_an_ordinary_grant_id_is_not_an_envelope(store, clock): + """§5.3.1. `--envelope everyday` would otherwise reach a path where rule 4 is skipped for + a grant that has no `controls:` to gate the opener instead, which is strictly weaker than + what `ctrlrun delegate` requires beneath the same grant.""" + control = _control(store, clock) + + with pytest.raises(AuthorityEscalation) as refused: + control._break_glass("everyday", _grant(clock.now + timedelta(hours=1))) + + assert refused.value.reason == "unknown_parent" + assert "a grant is not an envelope" in str(refused.value) + + +# --- T335: the third created_via value is readable -------------------------------------------- + + +def test_T335_a_break_glass_row_does_not_make_the_deployment_unreadable(store, clock): + """§5.3, §11.1. The vocabulary is a closed `Literal`, and an unknown value makes + `_candidates` raise and answers `authority_unreadable` for **every action in the + deployment**. This is the test for that failure mode.""" + control = _control(store, clock) + control._break_glass("incident-payments", _grant(clock.now + timedelta(hours=2))) + + authority = Authority.from_yaml(POLICY) + result = authority.evaluate(_action(control), now=clock.now, store=store) + + assert result.reason != "authority_unreadable" + assert result.passed + + +# --- T336, T337: revocation and attenuation --------------------------------------------------- + + +def test_T336_revoking_it_stops_everything_beneath(store, clock): + from ctrlrun.errors import ActionDenied + + control = _control(store, clock) + opened = control._break_glass( + "incident-payments", _grant(clock.now + timedelta(hours=2), delegable=True) + ) + beneath = control.delegate( + opened.delegation_id, + _grant(clock.now + timedelta(hours=1), constraints={"amount_lte": 500}), + by=ONCALL, + ) + + control.revoke(opened.delegation_id, by="human:ada") + + for record in (opened, beneath): + assert store.get_delegation(record.delegation_id) is not None + with pytest.raises(ActionDenied) as refused: + control.execute(_action(control), _Executor(), KEY) + assert refused.value.reason in ("authority_revoked", "no_authority") + + +def test_T337_a_delegation_beneath_it_is_created_and_attenuates(store, clock): + """§5.2 point 4, second site, and §5.4's attenuation bullet. + + `_parent_for_creation`'s rule-3 chain scan reads `delegable` over every ancestor including + the envelope, so without the exemption this is refused `parent_not_valid` and the bullet + cannot hold. Creation **and then evaluation**, because the two read `delegable` at + different sites and one can pass while the other refuses. + """ + control = _control(store, clock) + # `delegable: true` on the break-glass grant itself, exactly as on any grant somebody + # intends to be delegated beneath: §5.2 point 4 exempts the **envelope**, which carries no + # such key, and changes nothing about the grant opened under it. + opened = control._break_glass( + "incident-payments", _grant(clock.now + timedelta(hours=2), delegable=True) + ) + + beneath = control.delegate( + opened.delegation_id, + _grant(clock.now + timedelta(hours=1), constraints={"amount_lte": 500}), + by=ONCALL, + ) + + assert beneath.depth == 2 + executor = _Executor() + assert str(control.execute(_action(control, amount=100), executor, KEY).result) == "committed" + assert executor.calls == 1 + + # And it cannot widen what it was given. + with pytest.raises(AuthorityEscalation) as refused: + control.delegate( + opened.delegation_id, + _grant(clock.now + timedelta(hours=1), constraints={"amount_lte": 5000}), + by=ONCALL, + ) + assert refused.value.reason == "containment" + + +def test_T337_a_delegation_beneath_it_cannot_outlive_it(store, clock): + control = _control(store, clock) + opened = control._break_glass( + "incident-payments", _grant(clock.now + timedelta(hours=1), delegable=True) + ) + + with pytest.raises(AuthorityEscalation) as refused: + control.delegate(opened.delegation_id, _grant(clock.now + timedelta(hours=3)), by=ONCALL) + + assert refused.value.reason == "containment" + assert refused.value.dimension == "expires_at" + + +# --- what an independent review found, each with the test that would have caught it --------- + + +def test_T334b_delegate_may_not_name_an_envelope_as_its_parent(store, clock): + """**The direction §5.3.1 did not guard, and it is the worse one.** + + §5.3.1 guards `break-glass --envelope `. Nothing guarded `delegate --parent `: `_parent_for_creation` resolved envelopes unconditionally and handed the + envelope's grant to `plan_delegation`, which applies none of §5.3's rules. The result was a + break-glass grant with **no expiry at all**, no `max_ttl`, no entitlement check and a + `created_via` saying `cli`, openable from a shell by anyone whose `--as` matched the + envelope's subject -- which is a pattern over the agents the grant may be *for*. + """ + control = _control(store, clock) + + with pytest.raises(AuthorityEscalation) as refused: + control.delegate("incident-payments", _grant(None), by=ONCALL) + + assert refused.value.reason == "unknown_parent" + assert "not a grant" in str(refused.value) + assert list(store.delegations()) == [] + + +def test_T334b_the_cli_delegate_path_is_refused_too(store, clock): + """The same, through `_delegate`, which is what `ctrlrun delegate --as` calls.""" + control = _control(store, clock) + + with pytest.raises(AuthorityEscalation): + control._delegate("incident-payments", _grant(None), by=ONCALL, via="cli") + + assert list(store.delegations()) == [] + + +def test_T334_an_envelope_citing_an_unknown_control_cannot_be_opened(store, clock): + """**A typo gated nobody.** One transposed letter in `controls:` and any verified principal + opened the envelope. + + Elsewhere a control naming no `approver_role` gates nobody (§3.5), and that is right where + the citation is on an *action*. Here the citation **is** the gate, so the same omission + reads the opposite way and must fail closed. `Authority.from_yaml` parses the section with + no registry to check against, so it is checked where both are. + """ + control = _control( + store, + clock, + opener=BYSTANDER, + policy=POLICY.replace("[incident-response]", "[incident-respones]"), + ) + + with pytest.raises(InvalidArgument) as refused: + control._break_glass("incident-payments", _grant(clock.now + timedelta(hours=1))) + + assert "incident-respones" in str(refused.value) + assert list(store.delegations()) == [] + + +def test_T334_an_envelope_citing_a_control_with_no_role_cannot_be_opened(store, clock): + """The other half of the same hole: the control resolves and gates nothing.""" + policy = POLICY.replace(" approver_role: incident-commander\n", "") + control = _control(store, clock, opener=BYSTANDER, policy=policy) + + with pytest.raises(InvalidArgument) as refused: + control._break_glass("incident-payments", _grant(clock.now + timedelta(hours=1))) + + assert "approver_role" in str(refused.value) + + +def test_T334_there_is_no_parameter_that_asserts_the_opener(): + """§5.3.1, §11.2. `by=` was an unauthenticated way to assert an opener **and its roles**. + + Passing a principal whose claims carried the envelope's role opened it in a deployment + whose provider resolved somebody else entirely. The signature is the test: a keyword that + does not exist cannot be passed. + """ + import inspect + + from ctrlrun.control import Control + + parameters = inspect.signature(Control._break_glass).parameters + + assert "by" not in parameters, ( + "a caller-supplied opener is an assertion, and the entitlement check reads the roles " + "off whatever is asserted (SPEC-v0.8 §5.3.1)" + ) + assert not hasattr(Control, "break_glass"), ( + "§11.2 adds no public Control method in v0.8; the surface is the CLI command" + ) + + +def test_T333_a_refusal_before_the_authority_gate_names_no_grant(store, clock): + """§5.4. The grant id is per-call state, and it was only ever **set**, never cleared. + + §4.3.1 puts `principal_expired` first, so a denied receipt is recorded before the authority + gate runs. After one committed action under a break-glass grant, the next call's refusal + carried that grant's id -- naming a grant that never decided it, which is worse than + naming none. + """ + from ctrlrun.errors import IdentityError + + control = _control(store, clock) + opened = control._break_glass("incident-payments", _grant(clock.now + timedelta(hours=2))) + committed = control.execute(_action(control), _Executor(), KEY) + assert committed.authority_grant_id == opened.delegation_id + + lapsed = Action( + name="payments.refund", + arguments={"amount": 100, "payment_id": "EU-42"}, + principal=Principal( + agent="oncall-agent", user="ada", expires_at=clock.now - timedelta(minutes=5) + ), + resource="payment:EU-42", + environment=control.environment, + ) + with pytest.raises(IdentityError): + control.execute(lapsed, _Executor(), "refund:EU-99") + + denied = [receipt for receipt in store.receipts() if str(receipt.result) == "denied"] + assert denied and denied[-1].authority_grant_id is None, ( + f"the refusal named {denied[-1].authority_grant_id!r}, a grant that never decided it" + ) + + +def test_T327_narrowing_max_ttl_cuts_a_grant_already_open(store, clock): + """§5.2, and `v0.3 §5.6`'s purpose: a narrowed root narrows everything beneath it. + + `max_ttl` was checked once, at creation, and is not a §5.4 containment row, so an operator + who narrowed an envelope while an incident was still running narrowed nothing. Every other + envelope dimension already cuts live grants at evaluation, because the envelope is the + chain's root parent; this was the one that did not, and it is the bound an operator reaches + for first. + """ + from ctrlrun.errors import ActionDenied + + control = _control(store, clock) + control._break_glass("incident-payments", _grant(clock.now + timedelta(hours=3))) + assert str(control.execute(_action(control), _Executor(), KEY).result) == "committed" + + # The same store, the same open grant, an envelope narrowed to fifteen minutes. + narrowed = _control(store, clock, policy=POLICY.replace("PT4H", "PT15M")) + clock.advance(timedelta(minutes=30)) + executor = _Executor() + + with pytest.raises(ActionDenied) as refused: + narrowed.execute(_action(narrowed), executor, "refund:EU-43") + + assert refused.value.reason == "authority_escalation" + assert executor.calls == 0 + + +def test_T327_widening_max_ttl_does_not_extend_a_grant_already_open(store, clock): + """The other direction, which must not change anything: the grant's own `expires_at` is + what expires it, and an envelope widened afterwards does not hand it more time.""" + from ctrlrun.errors import ActionDenied + + control = _control(store, clock) + control._break_glass("incident-payments", _grant(clock.now + timedelta(hours=1))) + clock.advance(timedelta(hours=2)) + + widened = _control(store, clock, policy=POLICY.replace("PT4H", "PT8H")) + with pytest.raises(ActionDenied) as refused: + widened.execute(_action(widened), _Executor(), KEY) + + assert refused.value.reason == "authority_expired" + + +# --- T338: THE absence test ------------------------------------------------------------------- + + +#: The names the milestone's plan forbids by name, plus the ones an implementer reaches for +#: under time pressure. A claim about the environment is a claim until something greps for it. +#: Names a flag would be spelled as **in code**: an identifier, a keyword argument, an +#: attribute. Matched with comments and string literals removed, because `approval.py` argues +#: in prose that a public `_granting_principal` would be "`trust_approver` spelled as a context +#: manager", and a grep that cannot tell prose from a flag pushes the argument out of the tree. +FORBIDDEN_IDENTIFIERS = ( + "skip_entitlement", + "trust_approver", + "allow_self_approval", + "break_glass=True", + "break_glass = True", + "ignore_revocations", + "skip_approver", + "disable_entitlement", +) + +#: And the names it would be spelled as **in a string**: a `click.option`, an environment +#: variable, a dict key. An independent review found that seven of the original sixteen +#: patterns could only ever appear as string literals and the tokenizer dropped exactly those, +#: so they were unmatchable by construction -- and the control test planted an identifier, so +#: nothing noticed. These are matched against string tokens only. +FORBIDDEN_STRINGS = ( + "CTRLRUN_SKIP", + "CTRLRUN_ALLOW_SELF", + "CTRLRUN_BREAK_GLASS", + "CTRLRUN_TRUST", + "--skip-entitlement", + "--allow-self-approval", + "--break-glass-force", + "--no-approver", + "--trust-approver", + "skip_entitlement", + "allow_self_approval", +) + + +def _tokens(path: pathlib.Path) -> tuple[list[tuple[int, str]], list[tuple[int, str]]]: + """This module's code lines and its string literals, separately, each numbered. + + `tokenize`, not a regex: a docstring spans lines and a `#` inside a string is not a + comment, and both mistakes go the unsafe way here. + + Two lists rather than one, because the two classes of name need opposite treatment. An + identifier must be matched with strings **out**, or prose arguing a flag away reads as the + flag. A `--flag` or an environment variable can only ever *be* a string, so matching it + with strings out matches nothing at all. Comments are dropped from both: a comment naming + `--skip-entitlement` to say it does not exist is the same prose problem one level down. + """ + import io + import tokenize + + code: dict[int, list[str]] = {} + strings: list[tuple[int, str]] = [] + with path.open("rb") as handle: + for token in tokenize.tokenize(io.BytesIO(handle.read()).readline): + if token.type == tokenize.COMMENT: + continue + if token.type == tokenize.STRING: + strings.append((token.start[0], token.string)) + continue + code.setdefault(token.start[0], []).append(token.string) + return ([(number, " ".join(parts)) for number, parts in sorted(code.items())], strings) + + +def test_T338_no_flag_environment_variable_or_option_skips_a_check(): + """§5.1 and the fourth rule of v0.8: break-glass is a grant, not a flag. + + A flag leaves no record, expires never, cannot be revoked and cannot be attenuated. The + whole of §5 rests on there being no such setting anywhere, and that sentence is a claim + until something greps for it. This is the grep, in the suite rather than in a PR body, so + it runs on every change rather than once when somebody remembered. + + It reads `src/` only: this file names every pattern while asserting none exists. + """ + root = pathlib.Path(__file__).resolve().parent.parent / "src" / "ctrlrun" + found: list[str] = [] + for path in sorted(root.rglob("*.py")): + where = path.relative_to(root.parent.parent) + code, strings = _tokens(path) + for line, text in code: + found += [f"{where}:{line}: {name}" for name in FORBIDDEN_IDENTIFIERS if name in text] + for line, text in strings: + found += [f"{where}:{line}: {name}" for name in FORBIDDEN_STRINGS if name in text] + + assert not found, ( + "a setting that relaxes a check exists in the shipped package:\n " + + "\n ".join(found) + + "\nSPEC-v0.8 §5.1: a flag leaves no record, expires never, and cannot be revoked" + ) + + +def test_T338_the_grep_would_find_one_in_every_spelling_a_flag_takes(tmp_path): + """The control for the test above (`v0.4 §1.3`). + + A grep that matches nothing passes whether or not the tree is clean. The first version of + this control planted an identifier only, so it never exercised the seven patterns that can + appear solely as strings -- which were being discarded, and were therefore dead patterns + the control could not see. It now plants one of each spelling, and one in a comment that + must not count. + """ + planted = tmp_path / "ctrlrun" / "planted.py" + planted.parent.mkdir(parents=True) + planted.write_text( + "import os\n" + "import click\n" + "\n" + "ALLOW = dict(skip_entitlement=True)\n" + 'OPTION = click.option("--allow-self-approval", is_flag=True)\n' + 'ENV = os.environ.get("CTRLRUN_BREAK_GLASS")\n' + "# --no-approver is named here in a comment and must NOT count\n", + encoding="utf-8", + ) + code, strings = _tokens(planted) + + identifiers = {name for name in FORBIDDEN_IDENTIFIERS for _, text in code if name in text} + literals = {name for name in FORBIDDEN_STRINGS for _, text in strings if name in text} + + assert identifiers == {"skip_entitlement"}, identifiers + assert literals == {"--allow-self-approval", "CTRLRUN_BREAK_GLASS"}, ( + "a flag spelled as a click option or an environment variable is invisible to this " + f"test, so its absence from the tree is not evidence: found {literals}" + ) + assert not any("--no-approver" in text for _, text in code + strings), ( + "a name in a comment counted, so prose arguing a flag away reads as the flag" + ) diff --git a/tests/test_cross_host.py b/tests/test_cross_host.py index bb6149a..afe75a8 100644 --- a/tests/test_cross_host.py +++ b/tests/test_cross_host.py @@ -54,6 +54,12 @@ from ctrlrun.errors import AmbiguousEffect, CTRLRunError, DuplicateEffect, NotExecuted from failure_injection import Proxy, upstream_of +#: Every window in this file is opened by the proxy -- the killed COMMIT and the partition -- +#: and measured against what a store did inside it. Sharing a machine with seven other pytest +#: workers turns those into races the test loses: T155b reported "the window never opened", +#: which was true. `scripts/check.sh` runs these on their own. +pytestmark = pytest.mark.serial + URL = os.environ.get("CTRLRUN_TEST_POSTGRES") postgres = pytest.mark.skipif( diff --git a/tests/test_demo.py b/tests/test_demo.py index ab72c18..076ba35 100644 --- a/tests/test_demo.py +++ b/tests/test_demo.py @@ -1095,6 +1095,9 @@ def test_the_cli_offers_exactly_the_commands_the_spec_freezes(): # SPEC-v0.3 §5.7 — build-list item 3. "delegate", "revoke", + # SPEC-v0.8 §5.3 — break-glass is a grant, so opening one is a delegation with its own + # command rather than a flag on this list. + "break-glass", # SPEC-v0.3 §6.4, §6.5 — build-list item 4. "stats", "verify", diff --git a/tests/test_entitlement.py b/tests/test_entitlement.py new file mode 100644 index 0000000..c0d17de --- /dev/null +++ b/tests/test_entitlement.py @@ -0,0 +1,703 @@ +"""T297 to T309: entitlement from the control registry (SPEC-v0.8 §3). + +A control names the role that may answer an approval the decision already required. Omission is +not entitlement, and a control naming no role gates nobody: two sentences that mean opposite +things, and the pair R2 exists for. +""" + +from __future__ import annotations + +import logging +import os +import pathlib +import uuid +from datetime import UTC, datetime, timedelta +from typing import Any + +import pytest + +from ctrlrun.action import Action, Principal +from ctrlrun.approval import ( + ApproverIdentity, + RequiredRole, + VerifiedApprover, + _granting_principal, + entitled_controls, + roles_held, + unsatisfied, +) +from ctrlrun.control import Control, with_approval +from ctrlrun.errors import ApprovalMismatch, ApprovalRequired, InvalidArgument, PolicyError +from ctrlrun.identity import IdentityContext, StaticIdentityProvider +from ctrlrun.policy import Policy +from ctrlrun.receipt import EventType +from ctrlrun.state import InMemoryStateStore, SQLiteStateStore + +pytestmark = pytest.mark.authority + +POLICY = """ +schema: ctrlrun.policy/v6 +controls: + card-data-handling: + title: Cardholder data changes are approved by a named owner + source: PCI DSS 7.2.1 + approver_role: payments-owner + separation-of-duties: + title: A second pair of eyes + approver_role: second-pair + change-log: + title: Changes are written down +actions: + payments.refund: + decision: approve + controls: [card-data-handling] + payments.large: + decision: approve + controls: [card-data-handling, separation-of-duties] + payments.logged: + decision: approve + controls: [change-log] +""" + +KEY = "refund:EU-42" +UNENTITLED = "approver_unentitled" +AGENT = Principal(agent="ops-agent", user="ada") + +#: The approver, and the roles it holds, vary per test; this is the shape they share. +APPROVER = Principal(agent="human:bob", user="bob@example.com", issuer="https://issuer.example") + +POSTGRES_URL = os.environ.get("CTRLRUN_TEST_POSTGRES") + + +class _Clock: + def __init__(self) -> None: + self.now = datetime(2026, 9, 12, 10, 0, tzinfo=UTC) + + def __call__(self) -> datetime: + return self.now + + def advance(self, delta: timedelta) -> None: + self.now += delta + + +class _Fixed: + def __init__(self, principal: Principal | None) -> None: + self.principal = principal + + def resolve(self, context: IdentityContext) -> Principal | None: + return self.principal + + +class _Executor: + def __init__(self) -> None: + self.calls = 0 + + def __call__(self) -> str: + self.calls += 1 + return "done" + + +@pytest.fixture +def clock(): + return _Clock() + + +@pytest.fixture( + params=[ + "in-memory", + "sqlite", + pytest.param( + "postgres", + marks=pytest.mark.skipif( + not POSTGRES_URL, + reason="CTRLRUN_TEST_POSTGRES is not set; no server to run against", + ), + ), + ] +) +def store(request, clock, tmp_path): + """Every shipped store: `required_roles` is written and read by each of them. + + Item 2's mutation table found what a single-store file hides, and the column this item pins + the roles in is written by the same three write paths. + """ + if request.param == "in-memory": + made = InMemoryStateStore(clock=clock) + elif request.param == "sqlite": + made = SQLiteStateStore(tmp_path / "state.db", clock=clock) + else: + from ctrlrun.postgres import PostgresStateStore + + schema = f"entitle_{uuid.uuid4().hex[:12]}" + PostgresStateStore.create_schema(POSTGRES_URL, schema) + made = PostgresStateStore(POSTGRES_URL, schema=schema, clock=clock) + yield made + made.close() + if request.param == "postgres": + from ctrlrun.postgres import PostgresStateStore + + PostgresStateStore.drop_schema(POSTGRES_URL, schema) + + +def _control(store, clock, *, roles_claim="roles", policy=POLICY, provider=None): + identity = ( + None + if provider is False + else ApproverIdentity(_Fixed(provider or APPROVER), roles_claim=roles_claim) + ) + return Control( + Policy.from_yaml(policy), + store, + clock=clock, + approver_identity=identity, + identity=StaticIdentityProvider(agent=AGENT.agent, user=AGENT.user), + ) + + +def _action(control, name: str = "payments.refund", **arguments: Any) -> Action: + return Action( + name=name, + arguments=arguments or {"amount": 100, "payment_id": "EU-42"}, + principal=AGENT, + environment=control.environment, + ) + + +def _requested(control, action, key: str | None = KEY) -> str: + with pytest.raises(ApprovalRequired) as pending: + control.execute(action, _Executor(), key) + return pending.value.request_id + + +def _grant(store, request_id, *, entitled=(), principal=APPROVER): + with _granting_principal(principal, entitled=entitled): + return store.grant_approval(request_id, "mcp-operator:bob") + + +def _present(control, action, request_id, executor=None, key: str | None = KEY): + with with_approval(request_id): + return control.execute(action, executor or _Executor(), key) + + +def _invalidated(store, request_id): + return [ + event + for event in store.events() + if event.type is EventType.APPROVAL_INVALIDATED and event.approval_id == request_id + ] + + +# --- T297, T298: the refusal names the control, and the control passes ------------------------- + + +def test_T297_an_unentitled_approver_is_refused_and_the_control_is_named(store, clock): + """§3.7: the reason, the message and the event all name the control and the role. + + Asserted by value and never by type: an entitlement refusal and an unverified one share + `ApprovalMismatch`, and a test asserting only the type cannot tell which guard fired. + """ + control = _control(store, clock) + action = _action(control) + request_id = _requested(control, action) + _grant(store, request_id, entitled=()) + executor = _Executor() + + with pytest.raises(ApprovalMismatch) as refused: + _present(control, action, request_id, executor) + + assert refused.value.reason == UNENTITLED + assert "payments-owner" in str(refused.value) + assert "card-data-handling" in str(refused.value) + assert executor.calls == 0 + data = _invalidated(store, request_id)[-1].data + assert data["control"] == "card-data-handling" + assert data["role"] == "payments-owner" + assert str(store.get_approval(request_id).status) == "granted" + + +def test_T298_an_entitled_approver_approves_and_the_action_runs(store, clock): + """The positive control. A check that refused everything would pass T297 alone.""" + control = _control(store, clock) + action = _action(control) + request_id = _requested(control, action) + _grant(store, request_id, entitled=("card-data-handling",)) + executor = _Executor() + + receipt = _present(control, action, request_id, executor) + + assert executor.calls == 1 + assert str(receipt.result) == "committed" + + +# --- T299: the claim shapes ------------------------------------------------------------------- + + +def test_T299_a_tuple_claim_entitles_for_each_of_its_roles(): + """§3.4: the case `ClaimValue`'s amendment exists for.""" + holder = Principal(agent="human:bob", claims={"roles": ["payments-owner", "second-pair"]}) + + held = roles_held(holder, "roles") + + assert held == frozenset({"payments-owner", "second-pair"}) + required = ( + RequiredRole("card-data-handling", "payments-owner"), + RequiredRole("separation-of-duties", "second-pair"), + ) + assert entitled_controls(required, held) == ( + "card-data-handling", + "separation-of-duties", + ) + assert unsatisfied(required, entitled_controls(required, held)) is None + + +def test_T299_a_jwt_array_claim_is_carried_rather_than_dropped(): + """§3.4's first half, at the provider: an array claim arrived *absent* before v0.8.""" + principal = Principal(agent="human:bob", claims={"roles": ["payments-owner"]}) + + assert principal.claims["roles"] == ("payments-owner",) + + +def test_T299b_a_tuple_claim_survives_the_json_round_trip(store, clock): + """§3.4's second half: JSON has no tuple, so a stored claim comes back a **list**. + + Unamended, `_frozen_claims` refused a list, so every stored action and every receipt carrying + an array claim raised on read, and for a receipt that breaks `Receipt.from_dict`'s + never-raises contract by name (`v0.7 §6.11`). + """ + from ctrlrun.receipt import Receipt, verify_chain + + holder = Principal(agent="ops-agent", user="ada", claims={"roles": ["payments-owner", "x"]}) + control = Control( + Policy.from_yaml(POLICY), + store, + clock=clock, + identity=StaticIdentityProvider(agent="ops-agent", user="ada"), + ) + action = Action( + name="payments.logged", + arguments={"amount": 1}, + principal=holder, + environment=control.environment, + ) + request_id = _requested(control, action, "refund:EU-99") + store.grant_approval(request_id, "cli:local") + _present(control, action, request_id, key="refund:EU-99") + + stored = store.get_approval(request_id) + assert stored.request.action.principal.claims["roles"] == ("payments-owner", "x") + + written = [r for r in store.receipts() if r.action_id == action.action_id][-1] + assert written.principal.claims["roles"] == ("payments-owner", "x") + reparsed = Receipt.from_dict(written.to_dict()) + assert reparsed.principal.claims["roles"] == ("payments-owner", "x") + report = verify_chain(store) + assert report.ok, report.breaks + + +# --- T300, T301: the two omissions ------------------------------------------------------------ + + +def test_T300_a_principal_whose_claims_lack_the_role_is_not_entitled(store, clock, caplog): + """Omission A (§3.4). A missing claim is a statement about a person, and the kernel refuses + to invent one; the warning exists because the commonest cause is a provider nobody told.""" + bare = Principal(agent="human:bob", user="bob@example.com") + control = _control(store, clock, provider=bare) + action = _action(control) + request_id = _requested(control, action) + with caplog.at_level(logging.WARNING, logger="ctrlrun"): + _grant(store, request_id, entitled=(), principal=bare) + + with pytest.raises(ApprovalMismatch) as refused: + _present(control, action, request_id) + + assert refused.value.reason == UNENTITLED + # The docstring above claimed a warning existed and nothing asserted one, which is the + # shape of false green this repository keeps finding: a sentence about behaviour, in a + # test that would pass if the behaviour were deleted. A principal with no claims at all + # holds no role and there is nothing silent about that, so what is asserted is the + # refusal's own record: the event names the control and the role (§3.7). + assert not [record for record in caplog.records if "names no role" in record.message], ( + "an absent claim is not a misconfiguration to warn about" + ) + invalidated = _invalidated(store, request_id) + assert invalidated and invalidated[0].data["control"] == "card-data-handling" + assert invalidated[0].data["role"] == "payments-owner" + + +def test_T300_a_claim_in_a_shape_that_carries_no_role_warns(store, clock, caplog): + """§3.4's *silent* failure, which is the one the warning is for. + + The issuer did send the claim. It sent an integer, every rule in `roles_held` reads it as + absent, and without this the operator sees an approval refused for a role their identity + provider believes it is sending. + """ + numeric = Principal(agent="human:bob", user="bob@example.com", claims={"roles": 7}) + with caplog.at_level(logging.WARNING, logger="ctrlrun"): + held = roles_held(numeric, "roles") + + assert held == frozenset() + assert [record for record in caplog.records if "names no role" in record.message], ( + f"no warning named the shape; records were {[r.message for r in caplog.records]}" + ) + assert "'roles'" in caplog.text and "int" in caplog.text + + +def test_T301_a_control_naming_no_role_gates_nobody(store, clock): + """Omission B (§3.5), and the opposite answer. + + Deleting either of T300 or T301 leaves the other passing under a reading that is wrong in + the other direction, which is the pair R2 exists for. + """ + control = _control(store, clock) + action = _action(control, "payments.logged", amount=5) + request_id = _requested(control, action, "refund:EU-77") + _grant(store, request_id, entitled=()) + executor = _Executor() + + receipt = _present(control, action, request_id, executor, key="refund:EU-77") + + assert executor.calls == 1 + assert str(receipt.result) == "committed" + + +# --- T302, T303: what a role is and is not ---------------------------------------------------- + + +@pytest.mark.parametrize("value", [3, True, "", 0]) +def test_T302_an_int_a_bool_or_an_empty_claim_entitles_nothing(value): + """§3.4: never coerced, never stringified. `True` is not the role `"True"`.""" + holder = Principal(agent="human:bob", claims={"roles": value}) + + assert roles_held(holder, "roles") == frozenset() + + +@pytest.mark.parametrize("value", ["payments-owner ", "PAYMENTS-OWNER", "payments", "owner"]) +@pytest.mark.parametrize("shape", ["string", "list"]) +def test_T303_matching_is_byte_for_byte(value, shape): + """§3.4: no folding, no trimming, no prefix matching, no pattern grammar. + + **Both claim shapes**, because they are two branches. The first version parametrised only the + list shape, and the mutation table found what that hid: folding and trimming the *string* + branch left every case green, since nothing exercised it. + """ + claim = value if shape == "string" else [value] + holder = Principal(agent="human:bob", claims={"roles": claim}) + required = (RequiredRole("card-data-handling", "payments-owner"),) + + assert entitled_controls(required, roles_held(holder, "roles")) == () + + +@pytest.mark.parametrize("shape", ["string", "list"]) +def test_T303_the_exact_role_matches_in_both_shapes(shape): + """The positive control for the pair above: a matcher that matched nothing would pass it.""" + claim = "payments-owner" if shape == "string" else ["payments-owner"] + holder = Principal(agent="human:bob", claims={"roles": claim}) + required = (RequiredRole("card-data-handling", "payments-owner"),) + + assert entitled_controls(required, roles_held(holder, "roles")) == ("card-data-handling",) + + +# --- T304: half a check fails closed ---------------------------------------------------------- + + +def test_T304_no_roles_claim_configured_entitles_nobody(store, clock): + """§3.4's last bullet: a deployment naming roles in its policy and no claim to read them + from has configured half a check.""" + holder = Principal(agent="human:bob", claims={"roles": ["payments-owner"]}) + + assert roles_held(holder, None) == frozenset() + + +def test_T304_a_provider_that_carries_no_claims_entitles_nobody(store, clock): + """The shipped case: `HeaderIdentityProvider` carries no claims at all, by design, so the + operator server run with `--principal-header` produces a verified-but-unentitled approver.""" + from ctrlrun.identity import HeaderIdentityProvider + + provider = HeaderIdentityProvider(agent_header="x-agent", user_header="x-user") + resolved = provider.resolve( + IdentityContext( + action="payments.refund", + environment="production", + headers={"x-agent": "human:bob", "x-user": "bob@example.com"}, + ) + ) + + assert resolved is not None + assert roles_held(resolved, "roles") == frozenset() + + +# --- T305: every cited control must be satisfied ---------------------------------------------- + + +def test_T305_holding_one_of_two_required_roles_is_refused(store, clock): + """§3.6: all-of and never any-of, which would let the weakest control decide who may answer. + + The test asserts the refusal **and the control named**, so it cannot pass under an any-of + reading by asserting only that something was refused. + """ + control = _control(store, clock) + action = _action(control, "payments.large", amount=900000) + request_id = _requested(control, action, "refund:EU-88") + _grant(store, request_id, entitled=("card-data-handling",)) + + with pytest.raises(ApprovalMismatch) as refused: + _present(control, action, request_id, key="refund:EU-88") + + assert refused.value.reason == UNENTITLED + assert "separation-of-duties" in str(refused.value) + assert _invalidated(store, request_id)[-1].data["control"] == "separation-of-duties" + + +# --- T306: the roles are pinned at request time ----------------------------------------------- + + +def test_T306_the_roles_in_force_at_the_request_are_the_ones_applied(store, clock): + """§3.3, on `v0.6 §7.1`'s rule: the approval binds to what the human was shown. + + The policy's role changes between the request and the presentation. The approval was granted + entitled for the control the *request* pinned, and it is still consumable: a `Control` loaded + with a moved policy does not re-derive what the human was asked under. + """ + control = _control(store, clock) + action = _action(control) + request_id = _requested(control, action) + _grant(store, request_id, entitled=("card-data-handling",)) + + moved = POLICY.replace("approver_role: payments-owner", "approver_role: someone-else") + after = _control(store, clock, policy=moved) + executor = _Executor() + + receipt = _present(after, action, request_id, executor) + + assert executor.calls == 1 + assert str(receipt.result) == "committed" + assert store.get_approval(request_id).request.required_roles == ( + RequiredRole("card-data-handling", "payments-owner"), + ) + + +# --- T307: two defences, two tests ------------------------------------------------------------ + + +def test_T307_the_consume_side_refuses_a_row_a_store_wrote_without_checking(store, clock): + """§3.8: the guarantee is the consumption check, and this is the case the grant-side + courtesy cannot reach. + + The row is written entitled for a control that is not the one the request pinned, which is + what a store that recorded whatever it was handed would hold. + """ + control = _control(store, clock) + action = _action(control) + request_id = _requested(control, action) + _grant(store, request_id, entitled=("some-other-control",)) + + with pytest.raises(ApprovalMismatch) as refused: + _present(control, action, request_id) + + assert refused.value.reason == UNENTITLED + + +# --- T307b: the shapes a column can hold, and the one a reader must survive ------------------- + + +@pytest.mark.parametrize("given", ["card-data-handling", 7, {"card-data-handling": 1}, [""], [3]]) +def test_T307_a_row_whose_entitled_is_not_a_list_of_ids_is_refused(given): + """§3.6. `tuple("c1")` is three control ids that entitle nothing and refuse nothing. + + The guard lives in `__post_init__`, and the defect this covers was a caller that wrapped + the value in `tuple(...)` **before** the guard saw it, so a string arrived as a tuple of + non-empty strings and every check downstream passed on nonsense. A mapping is refused for + the same reason: iterating one yields its keys. + """ + document = {"agent": "human:bob", "granted_at": "2026-01-01T00:00:00+00:00", "entitled": given} + with pytest.raises(InvalidArgument): + VerifiedApprover.from_dict(document) + + +def test_T307_a_string_cannot_be_handed_to_the_granting_context(): + """The same hazard on the write side (§2.5).""" + with pytest.raises(InvalidArgument), _granting_principal(APPROVER, entitled="card-data"): + pass # pragma: no cover - the context manager refuses before the body + + +def test_T307_a_receipt_carrying_a_tampered_claim_still_parses(): + """`v0.7 §6.11`: `Receipt.from_dict` never raises, and §3.4 gave claims a new refusal. + + `_frozen_claims` refuses an array of numbers, which is a shape JSON holds and a tampered + row can carry. Running it inside `from_dict` meant one bad field blinded every reader of + the chain rather than costing that field, which is the opposite of what a receipt is for. + """ + from ctrlrun.receipt import _claims_of + + kept = _claims_of( + {"roles": [1, 2], "team": ["payments"], "level": 3, "on": True, "nested": {"a": 1}, "": "x"} + ) + + assert kept == {"team": ("payments",), "level": 3, "on": True} + assert Principal(agent="a", user=None, claims=kept).claims["team"] == ("payments",) + + +# --- the vocabulary check that stops this being found a third time ---------------------------- + + +def test_every_approval_refusal_reason_is_counted_by_stats(): + """`ctrlrun stats` must have a bucket for every reason an approval can be refused with. + + Enumerated from `approval.py` rather than restated here, which is the whole point: a set + written out by hand is a set somebody adds a reason without. It has been missed twice. Item + 2 shipped `approval_denied` in no bucket, so an observe-mode run in which a **human said + no** was counted nowhere; items 3 and 4 then coined `approver_unentitled` and + `approvals_unverifiable` and did the same thing again. This test fails on the next one. + """ + import re + + from ctrlrun import approval + from ctrlrun.receipt import BLOCKED_BY_STATE + + source = pathlib.Path(approval.__file__).read_text(encoding="utf-8") + names = re.findall(r"^([A-Z][A-Z_]*): Final = \"([a-z_]+)\"", source, re.MULTILINE) + reasons = { + value + for name, value in names + if name.startswith(("APPROVAL_", "APPROVER_", "APPROVALS_", "HASH_", "UNKNOWN_")) + } + assert reasons, "the enumeration found nothing, so this test is asserting nothing" + + missing = sorted(reason for reason in reasons if reason not in BLOCKED_BY_STATE) + assert not missing, ( + f"these refusal reasons are in no `ctrlrun stats` bucket: {missing}. An observe-mode " + "run that would have refused for one of them reports would_have_been_blocked = 0, " + "which is a report saying nothing happened about the thing that did" + ) + + +# --- T308: the registry loads, and refuses what it must --------------------------------------- + + +@pytest.mark.parametrize("role", [" payments-owner", "payments-owner ", "payments-owner\n"]) +def test_T308_a_padded_approver_role_is_refused_at_load(role): + """A role that can never match is a control that refuses everything and explains nothing.""" + document = ( + "schema: ctrlrun.policy/v6\n" + "controls:\n" + " card-data-handling:\n" + " title: t\n" + f' approver_role: "{role}"\n' + "actions:\n" + " payments.refund:\n" + " decision: approve\n" + " controls: [card-data-handling]\n" + ) + with pytest.raises(PolicyError) as refused: + Policy.from_yaml(document) + + assert "whitespace" in str(refused.value) + + +def test_T308_the_registry_carries_approver_role_under_v6(): + policy = Policy.from_yaml(POLICY) + + assert policy.controls["card-data-handling"].approver_role == "payments-owner" + assert policy.controls["change-log"].approver_role is None + + +@pytest.mark.parametrize("value", ["", " ", "[]", "3"]) +def test_T308_a_malformed_approver_role_is_refused_naming_the_key(value): + document = POLICY.replace("approver_role: payments-owner", f"approver_role: {value}") + + with pytest.raises(PolicyError) as refused: + Policy.from_yaml(document) + + assert "approver_role" in str(refused.value) + + +def test_T308_the_key_needs_v6(): + """`v0.6 §9.5`'s rule: an older reader refuses the document rather than ignoring the key, + because a reader that ignored this would gate nobody and report a deployment as checking.""" + document = POLICY.replace("ctrlrun.policy/v6", "ctrlrun.policy/v5") + + with pytest.raises(PolicyError) as refused: + Policy.from_yaml(document) + + assert "approver_role" in str(refused.value) + assert "ctrlrun.policy/v6" in str(refused.value) + + +def test_T308_an_unknown_key_on_a_control_entry_is_still_refused(): + """The closed key set (`v0.6 §7.3`) grew by one and is still closed.""" + document = POLICY.replace( + " approver_role: payments-owner", " approver_role: payments-owner\n owner: bob" + ) + + with pytest.raises(PolicyError): + Policy.from_yaml(document) + + +# --- T309: G17 in verify ---------------------------------------------------------------------- + + +GATED_DOCUMENT = """ +schema: ctrlrun.policy/v6 +environment: production +controls: + card-data-handling: + title: Cardholder data changes are approved by a named owner + approver_role: payments-owner +actions: + aaa.untouched: + decision: approve + zzz.refund: + decision: approve + controls: [card-data-handling] +""" + + +def test_T309_G17_passes_on_a_document_that_gates_entitlement(tmp_path, monkeypatch): + """**The test that was missing, and its absence let a broken scenario ship.** + + Nothing anywhere graded G17 as PASS: every other assertion was about its `N/A` count, so the + entire scenario body was uncovered and `ctrlrun verify` exited 1 on any document that used + the feature item 3 ships. The scenario built its request straight from the provider, outside + `Control._presented`, so nothing was pinned and nothing was refused. + + The document also puts an **ungated** approve action first by codepoint, which is the second + thing the scenario got wrong: asking about whichever action `select` reached first reported + "no cited control names an approver role" of a document that gates one. + """ + from ctrlrun.verify import run + + monkeypatch.chdir(tmp_path) + path = tmp_path / "ctrlrun.yaml" + path.write_text(GATED_DOCUMENT, encoding="utf-8") + + report = run(path) + + results = {guarantee.id: guarantee for guarantee in report.guarantees} + assert str(results["G17"].status) in ("Status.PASS", "pass"), ( + f"G17 reported {results['G17'].status}: {results['G17'].counterexample}" + ) + + +def test_T309_G17_is_not_applicable_only_where_the_document_gates_nothing(tmp_path, monkeypatch): + """The other half: the `N/A` reason is about the document, per §11.7.""" + from ctrlrun.verify import run + + monkeypatch.chdir(tmp_path) + path = tmp_path / "ctrlrun.yaml" + path.write_text( + GATED_DOCUMENT.replace(" controls: [card-data-handling]\n", ""), encoding="utf-8" + ) + + report = run(path) + + result = {guarantee.id: guarantee for guarantee in report.guarantees}["G17"] + assert "not_applicable" in str(result.status).lower() + assert result.reason == "no cited control names an approver role" + + +def test_T309_G17_is_in_the_catalogue_with_a_document_true_na_reason(): + from ctrlrun.verify import guarantees as reg + + assert "G17" in reg.BY_ID + assert reg.BY_ID["G17"].title == "an unentitled approver refused" + assert reg.NO_APPROVER_ROLE == "no cited control names an approver role" diff --git a/tests/test_jwt_identity.py b/tests/test_jwt_identity.py index 68b23b3..f55f471 100644 --- a/tests/test_jwt_identity.py +++ b/tests/test_jwt_identity.py @@ -170,16 +170,28 @@ def test_T88_only_the_named_claims_reach_the_principal(keypair, clock): def test_T88_a_claim_whose_value_is_not_scalar_is_dropped(keypair, clock, caplog): + """Amended by SPEC-v0.8 §3.4: a **list of strings** is carried now, as a tuple. + + A roles claim is a JSON array at every issuer anybody deploys, and dropping it is what made + its holder silently unentitled. Everything else a `Principal` cannot carry is still dropped, + and now says so at WARNING rather than DEBUG, because this is where an unentitled approver + begins. + """ private, public = keypair - provider = _provider(public, clock, claim_names=["email", "roles", "level", "active"]) + provider = _provider(public, clock, claim_names=["email", "roles", "level", "active", "nested"]) - token = _sign(private, clock, roles=["a", "b"], level=3, active=True) + token = _sign(private, clock, roles=["a", "b"], level=3, active=True, nested={"deep": 1}) with caplog.at_level(logging.DEBUG, logger="ctrlrun"): principal = provider.resolve(_context(token)) - assert set(principal.claims) == {"email", "level", "active"} + assert set(principal.claims) == {"email", "roles", "level", "active"} + assert principal.claims["roles"] == ("a", "b") assert principal.claims["level"] == 3 assert principal.claims["active"] is True + assert "nested" not in principal.claims + assert any( + record.levelname == "WARNING" and "nested" in record.message for record in caplog.records + ) def test_T88_a_claim_name_is_a_flat_key_and_no_nesting_is_traversed(keypair, clock): diff --git a/tests/test_m_of_n.py b/tests/test_m_of_n.py new file mode 100644 index 0000000..ba42798 --- /dev/null +++ b/tests/test_m_of_n.py @@ -0,0 +1,756 @@ +"""T310 to T325: M-of-N on distinct verified principals (SPEC-v0.8 §4.2). + +N distinct resolved principals, counted once each, decided by the store's write and never by a +read followed by one. Written before the implementation: a red suite is the specification. +""" + +from __future__ import annotations + +import json +import os +import uuid +from dataclasses import replace +from datetime import UTC, datetime, timedelta +from typing import Any + +import pytest + +from ctrlrun.action import Action, Principal +from ctrlrun.approval import ApproverIdentity, _granting_principal +from ctrlrun.control import Control, with_approval +from ctrlrun.errors import ( + ActionDenied, + ApprovalMismatch, + ApprovalRequired, + InvalidArgument, + PolicyError, +) +from ctrlrun.identity import IdentityContext, StaticIdentityProvider +from ctrlrun.policy import Policy +from ctrlrun.state import InMemoryStateStore, SQLiteStateStore +from ctrlrun.webhook import sign + +pytestmark = pytest.mark.authority + +POLICY = """ +schema: ctrlrun.policy/v6 +actions: + payments.refund: + decision: approve + approvals_required: 2 + payments.single: + decision: approve +""" + +KEY = "refund:EU-42" +AGENT = Principal(agent="ops-agent", user="ada") +ALICE = Principal(agent="human:alice", user="alice@example.com", issuer="https://issuer.example") +BOB = Principal(agent="human:bob", user="bob@example.com", issuer="https://issuer.example") + +POSTGRES_URL = os.environ.get("CTRLRUN_TEST_POSTGRES") + + +class _Clock: + def __init__(self) -> None: + self.now = datetime(2026, 9, 12, 10, 0, tzinfo=UTC) + + def __call__(self) -> datetime: + return self.now + + def advance(self, delta: timedelta) -> None: + self.now += delta + + +class _Fixed: + def __init__(self, principal: Principal | None = None) -> None: + self.principal = principal + + def resolve(self, context: IdentityContext) -> Principal | None: + return self.principal + + +class _Executor: + def __init__(self) -> None: + self.calls = 0 + + def __call__(self) -> str: + self.calls += 1 + return "done" + + +@pytest.fixture +def clock(): + return _Clock() + + +@pytest.fixture( + params=[ + "in-memory", + "sqlite", + pytest.param( + "postgres", + marks=pytest.mark.skipif( + not POSTGRES_URL, + reason="CTRLRUN_TEST_POSTGRES is not set; no server to run against", + ), + ), + ] +) +def store(request, clock, tmp_path): + """Every shipped store: the count is decided by each one's own write (§4.3).""" + if request.param == "in-memory": + made = InMemoryStateStore(clock=clock) + elif request.param == "sqlite": + made = SQLiteStateStore(tmp_path / "state.db", clock=clock) + else: + from ctrlrun.postgres import PostgresStateStore + + schema = f"mofn_{uuid.uuid4().hex[:12]}" + PostgresStateStore.create_schema(POSTGRES_URL, schema) + made = PostgresStateStore(POSTGRES_URL, schema=schema, clock=clock) + yield made + made.close() + if request.param == "postgres": + from ctrlrun.postgres import PostgresStateStore + + PostgresStateStore.drop_schema(POSTGRES_URL, schema) + + +def _control(store, clock, *, verifying=True, policy=POLICY): + extra = {} if clock is None else {"clock": clock} + return Control( + Policy.from_yaml(policy), + store, + **extra, + approver_identity=ApproverIdentity(_Fixed(ALICE)) if verifying else None, + identity=StaticIdentityProvider(agent=AGENT.agent, user=AGENT.user), + ) + + +def _action(control, name: str = "payments.refund", **arguments: Any) -> Action: + return Action( + name=name, + arguments=arguments or {"amount": 100, "payment_id": "EU-42"}, + principal=AGENT, + environment=control.environment, + ) + + +def _requested(control, action, key: str | None = KEY) -> str: + with pytest.raises(ApprovalRequired) as pending: + control.execute(action, _Executor(), key) + return pending.value.request_id + + +def _grant(store, request_id, principal, *, approver=None, entitled=()): + with _granting_principal(principal, entitled=entitled): + return store.grant_approval(request_id, approver or f"mcp-operator:{principal.user}") + + +def _present(control, action, request_id, executor=None, key: str | None = KEY): + with with_approval(request_id): + return control.execute(action, executor or _Executor(), key) + + +# --- T310: N distinct principals, and not before ---------------------------------------------- + + +def test_T310_the_approval_is_consumable_only_after_the_nth(store, clock): + """§4.2. At N-1 the record is still `pending`, which is what the consume refuses on.""" + control = _control(store, clock) + action = _action(control) + request_id = _requested(control, action) + + first = _grant(store, request_id, ALICE) + assert first is None, "a partial grant is not an Approval" + assert str(store.get_approval(request_id).status) == "pending" + + executor = _Executor() + with pytest.raises(ApprovalMismatch) as refused: + _present(control, action, request_id, executor) + assert refused.value.reason == "pending" + assert executor.calls == 0 + assert store.get_effect(KEY) is None + + second = _grant(store, request_id, BOB) + assert second is not None, "the Nth grant produces the Approval" + assert str(store.get_approval(request_id).status) == "granted" + + receipt = _present(control, action, request_id, executor) + assert executor.calls == 1 + assert str(receipt.result) == "committed" + + +# --- T311: G19, one principal counts once ----------------------------------------------------- + + +def test_T311_a_second_grant_from_the_same_principal_counts_once(store, clock): + """G19. The strings differ and the principal is the same, which is the whole point. + + Not an error and not a duplicate row: rejecting the second answer would make a human think + their answer was lost, and counting it would be the defect. + """ + control = _control(store, clock) + action = _action(control) + request_id = _requested(control, action) + + _grant(store, request_id, ALICE, approver="mcp-operator:alice") + clock.advance(timedelta(minutes=1)) + again = _grant(store, request_id, ALICE, approver="cli:alice-from-a-different-door") + + assert again is None, "one principal cannot reach N alone" + record = store.get_approval(request_id) + assert str(record.status) == "pending" + assert len(record.approvers) == 1 + assert record.approvers[0].granted_at == clock.now, "the entry moves, the count does not" + + +# --- T313 lives elsewhere ------------------------------------------------------------------- +# +# SPEC-v0.8 §4.3's concurrency case needs the TCP proxy, the spawned child processes and the +# armed hold that open the window between the count's read and its write, and all three live in +# `tests/test_attempt_integrity.py`. It is +# `test_T313_two_processes_granting_in_the_window_produce_two_approvers`, and it fails against a +# compare-and-set on `status` alone, which is the shape this store had. +# +# Nothing in this file reproduces that window, and the fourth mutation shape in +# `CONTRIBUTING.md` is why +# that is written down rather than left to be noticed: every test here passes against a store +# with no compare-and-set whatever. + + +# --- T312: after N, a further grant is refused as it always was ------------------------------- + + +def test_T312_a_grant_after_the_record_is_granted_is_refused(store, clock): + """§4.2, by `check_answerable`, which `v0.1 §4.2` freezes.""" + control = _control(store, clock) + request_id = _requested(control, _action(control)) + _grant(store, request_id, ALICE) + _grant(store, request_id, BOB) + + with pytest.raises(ApprovalMismatch): + _grant(store, request_id, Principal(agent="human:carol", user="carol@example.com")) + + +# --- T314, T315, T316: what does not count ---------------------------------------------------- + + +def test_T314_the_requesters_own_yes_is_refused_at_consumption(store, clock): + """§4.1 and §14.4: it is counted by the store and refused by `Control`, not uncounted. + + A first draft of §4.2 said this yes "does not count". Excluding it from the count looks + stricter and is weaker: at N=1 the record would never reach `granted`, the consumption + refusal would be `pending`, and **G18 would never fire** on the deployment shape it was + written for. So the store counts every verified approver and the refusal is `Control`'s, + which is one rule in one place. + """ + control = _control(store, clock) + action = _action(control) + request_id = _requested(control, action) + + _grant(store, request_id, ALICE) + _grant(store, request_id, Principal(agent=AGENT.agent, user=AGENT.user)) + + with pytest.raises(ApprovalMismatch) as refused: + _present(control, action, request_id) + + assert refused.value.reason == "approver_is_requester" + assert str(store.get_approval(request_id).status) == "granted", ( + "the store counted it; what refuses it is §4.1 at consumption" + ) + + +def test_T315_an_unentitled_yes_is_refused_at_consumption(store, clock): + """§3.8, §14.4. §10.4 has named this test since the spec was written and it did not exist. + + The same shape as T314 and for the same reason: the store counts every verified approver, + and the refusal is `Control`'s, by its own reason, naming the control. Both of a request's + two approvers here are entitled to nothing, so the count reaches two and consumption still + refuses: a threshold is not a way to outvote an entitlement. + """ + control = _control(store, clock, policy=GATED_POLICY) + action = _action(control) + request_id = _requested(control, action) + + _grant(store, request_id, ALICE) + _grant(store, request_id, BOB) + + record = store.get_approval(request_id) + assert str(record.status) == "granted", "two distinct principals reached the threshold" + assert len(record.approvers) == 2 + + with pytest.raises(ApprovalMismatch) as refused: + _present(control, action, request_id) + + assert refused.value.reason == "approver_unentitled" + assert str(store.get_approval(request_id).status) == "granted", ( + "the approval is left granted; nothing is consumed for an action that did not run" + ) + + +def test_T316_an_unverifiable_yes_does_not_count(store, clock): + """§2.7: a grant a surface could not resolve records no approver, so it cannot be one of N.""" + control = _control(store, clock) + action = _action(control) + request_id = _requested(control, action) + + _grant(store, request_id, ALICE) + store.grant_approval(request_id, "cli:local") + + record = store.get_approval(request_id) + assert len(record.approvers) == 1, "an unverified answer is not a verified approver" + assert str(record.status) == "pending" + + +# --- T317, T318: a denial, and expiry --------------------------------------------------------- + + +def test_T317_one_denial_denies_a_request_holding_n_minus_one(store, clock): + """§4.2: a request that absorbs a no while it waits for yeses asked the wrong question.""" + control = _control(store, clock) + action = _action(control) + request_id = _requested(control, action) + _grant(store, request_id, ALICE) + + store.deny_approval(request_id, "mcp-operator:bob") + + assert str(store.get_approval(request_id).status) == "denied" + with pytest.raises(ActionDenied): + _present(control, action, request_id) + + +def test_T318_grants_do_not_extend_the_requests_expiry(store, clock): + """§4.2: expiry is the request's, and a partial grant does not renew it.""" + control = _control(store, clock) + action = _action(control) + request_id = _requested(control, action) + _grant(store, request_id, ALICE) + clock.advance(timedelta(hours=48)) + + with pytest.raises(ApprovalMismatch) as refused: + _grant(store, request_id, BOB) + + assert refused.value.reason == "expired" + + +# --- T319: N = 1 is 0.7.0 --------------------------------------------------------------------- + + +def test_T319_one_required_is_unchanged(store, clock): + """The positive control for the whole item: absent means 1, and 1 is what 0.7.0 did.""" + control = _control(store, clock) + action = _action(control, "payments.single", amount=5) + request_id = _requested(control, action, "refund:EU-1") + + granted = _grant(store, request_id, ALICE) + + assert granted is not None, "at N=1 the first grant is the Approval, as it always was" + executor = _Executor() + receipt = _present(control, action, request_id, executor, key="refund:EU-1") + assert executor.calls == 1 + assert str(receipt.result) == "committed" + + +# --- T320, T321: the key, and the deployment it needs ----------------------------------------- + + +@pytest.mark.parametrize("value", ["0", "-1", "true", "1.0", '"2"']) +def test_T320_a_malformed_threshold_is_refused_at_load(value): + """§4.2, on `v0.7 §5.3`'s precedent: a malformed threshold fails the policy, not the action.""" + document = POLICY.replace("approvals_required: 2", f"approvals_required: {value}") + + with pytest.raises(PolicyError) as refused: + Policy.from_yaml(document) + + assert "approvals_required" in str(refused.value) + + +def test_T320_the_key_needs_v6(): + document = POLICY.replace("ctrlrun.policy/v6", "ctrlrun.policy/v5") + + with pytest.raises(PolicyError) as refused: + Policy.from_yaml(document) + + assert "approvals_required" in str(refused.value) + + +def test_T321_a_threshold_above_one_with_no_approver_identity_is_denied(store, clock): + """§4.2: "distinct principals" has no referent in a deployment that verifies nobody. + + Denied at evaluation and not at load: the policy is loadable and correct, and what is missing + is the `Control` it is deployed in, which the loader cannot see. + """ + control = _control(store, clock, verifying=False) + action = _action(control) + executor = _Executor() + + with pytest.raises(ActionDenied) as refused: + control.execute(action, executor, KEY) + + assert refused.value.reason == "approvals_unverifiable" + assert "approvals_required" in str(refused.value) + assert executor.calls == 0 + + +# --- T322, T323: the widened return, and its callers ------------------------------------------ + + +def test_T322_no_provider_wait_returns_none_for_a_partial_grant(store, clock): + """`v0.1 §4.3`: `None` from `wait` means "answered, no", never "still waiting". + + Both shipped providers returned `grant_approval`'s result straight through, so a partial + grant would have reached `Control` as a **denial** and `@protect(wait=True)` would have + raised `ActionDenied` for a request a second human was still answering. + """ + from ctrlrun.approval import ScriptedApprovalProvider, ScriptedOutcome + from ctrlrun.errors import ApprovalTimeout + + control = _control(store, clock) + action = _action(control) + request_id = _requested(control, action) + + provider = ScriptedApprovalProvider(store, [ScriptedOutcome.GRANT], clock=clock) + with _granting_principal(ALICE), pytest.raises(ApprovalTimeout): + provider.wait(request_id, timedelta(seconds=1)) + + assert str(store.get_approval(request_id).status) == "pending" + + +# --- T324: a store that records one approver only --------------------------------------------- + + +def test_T324_a_store_that_records_one_approver_never_reaches_n(store, clock): + """§4.5: it fails closed, and never behaves as N=1.""" + control = _control(store, clock) + action = _action(control) + request_id = _requested(control, action) + + store.grant_approval(request_id, "cli:local") + store.grant_approval(request_id, "cli:local-again") + + record = store.get_approval(request_id) + assert str(record.status) == "pending", "an unverified grant never reaches N" + with pytest.raises(ApprovalMismatch): + _present(control, action, request_id) + + +@pytest.mark.parametrize("threshold", [0, -3, "2", 1.0, True]) +def test_T320_a_corrupted_threshold_is_refused_on_read_not_clamped(threshold): + """§4.2, §12. The other two columns raise on a corrupted value; this one was coerced. + + `count_grant` clamped with `max(1, ...)` and the SQLite read turned `0` into `1` with + `or 1`, so a row tampered to nothing read back as an ordinary single-approval request. + A store is the one place this can arrive from, which is the threat model `approvers` + is already written against. + """ + from ctrlrun.approval import ApprovalRequest + + now = datetime(2026, 1, 1, tzinfo=UTC) + with pytest.raises(InvalidArgument): + ApprovalRequest( + request_id="apr_corrupt", + action_hash="sha256:00", + action=None, + created_at=now, + expires_at=now + timedelta(minutes=15), + approvals_required=threshold, + ) + + +# --- T323: the three callers of the widened return --------------------------------------------- + + +def test_T323_the_cli_says_how_many_more_are_needed(tmp_path, clock): + """§4.4. `None` means recorded and short of N, and each caller says so in its own idiom. + + Against SQLite on disk, because the CLI opens the store by URL. + """ + from click.testing import CliRunner + + from ctrlrun.cli.main import main + + # The real clock on both sides: the CLI opens its own store and reads its own `now`, so a + # request pinned to a frozen test clock is already expired by the time it runs. + database = tmp_path / "state.db" + opened = SQLiteStateStore(database) + try: + control = _control(opened, None) + request_id = _requested(control, _action(control)) + finally: + opened.close() + + result = CliRunner().invoke( + main, + ["approve", request_id, "--store-url", f"sqlite:///{database}"], + catch_exceptions=False, + ) + + assert result.exit_code == 0, result.output + assert "0 of 2 approvals" in result.output + # §2.6: and it says why this answer will never be one of the two, rather than leaving an + # operator to infer it from a number that does not move. + assert "no verified approver" in result.output + + +def test_T323_the_webhook_answers_200_with_the_count(store, clock): + """§4.4's caller table: 200, because the answer *was* recorded, and the body says what of.""" + from ctrlrun.webhook import handle_inbound + + control = _control(store, clock) + request_id = _requested(control, _action(control)) + record = store.get_approval(request_id) + body = json.dumps( + { + "request_id": request_id, + "action_hash": record.action_hash, + "decision": "grant", + "approver": "slack:U123", + } + ).encode() + secret = "s" * 32 + status, text = handle_inbound( + store, + request_id, + body, + sign(body, secret, at=datetime.now(UTC)), + secret=secret, + replay_window=timedelta(seconds=300), + ) + + assert status == 200 + assert text.startswith("recorded: 0 of 2 approvals"), ( + f"the sender was told {text!r} for an answer that moved nothing toward the threshold" + ) + # Zero, and the body says why zero: this door verifies nobody, so its answer is recorded + # and counts for nothing. A bare count here reads as a broken counter. + assert "counts toward no threshold" in text + assert str(store.get_approval(request_id).status) == "pending" + + +# --- T322b: what observe mode says about a threshold it cannot verify ------------------------- + + +OBSERVE_POLICY = """ +schema: ctrlrun.policy/v6 +mode: observe +actions: + payments.refund: + decision: approve + approvals_required: 2 +""" + + +def test_T322_observe_mode_reports_the_threshold_it_could_not_verify(clock): + """§4.2, §11.1. Observe mode exists to say what enforce mode would have done. + + It said `approval_required`, which is "a human would have been asked". Enforce mode denies + every one of these before anybody is asked, because a threshold above one with no approver + identity has no referent for "distinct principals". An operator piloting a two-approver + policy got no signal that the thing they were piloting denies everything. + """ + store = InMemoryStateStore(clock=clock) + control = Control( + Policy.from_yaml(OBSERVE_POLICY), + store, + clock=clock, + identity=StaticIdentityProvider(agent=AGENT.agent, user=AGENT.user), + ) + executor = _Executor() + + receipt = control.execute(_action(control), executor, KEY) + + assert executor.calls == 1, "observe mode runs the action; that is what it is" + assert receipt.would_have is not None + assert receipt.would_have.blocked_reason == "approvals_unverifiable" + + from ctrlrun.reporting import stats_document + + counted = stats_document(list(store.receipts()), mode="observe", boundary=None) + assert counted["would_have_been_blocked"] == 1, f"the refusal is in no stats bucket: {counted}" + + +def test_T322_observe_mode_still_says_approval_required_where_one_yes_would_do(clock): + """The other half: a threshold of one is the ordinary case and keeps the ordinary reason.""" + store = InMemoryStateStore(clock=clock) + control = Control( + Policy.from_yaml(OBSERVE_POLICY.replace(" approvals_required: 2\n", "")), + store, + clock=clock, + identity=StaticIdentityProvider(agent=AGENT.agent, user=AGENT.user), + ) + + receipt = control.execute(_action(control), _Executor(), KEY) + + assert receipt.would_have is not None + assert receipt.would_have.blocked_reason == "approval_required" + + +# --- T324b: the read-back, because pinning through a context variable can be lost ------------- + + +class _OwnRequestProvider: + """A third-party provider that builds its own `ApprovalRequest`, as the protocol allows. + + `ApprovalProvider` and `ApprovalRequest` are both exported from `ctrlrun`, `build_request` + is package-internal, and nothing obliges a provider to use it. This one carries the fields + it knows about and drops the two v0.8 added, which is what any provider written against + 0.7.0 does. + """ + + def __init__(self, store, clock): + self._store = store + self._clock = clock + + def request(self, action, ttl): + from ctrlrun.approval import ApprovalRequest + + now = self._clock.now + request = ApprovalRequest( + request_id="apr_own_request_provider", + action_hash=action.action_hash, + action=action, + created_at=now, + expires_at=now + ttl, + ) + self._store.put_approval_request(request) + return request + + def wait(self, request, timeout): # pragma: no cover - never reached + raise AssertionError("the request pass refuses before anybody is asked") + + +class _DroppingStore(InMemoryStateStore): + """A store that accepts the request and does not persist what v0.8 pins. + + SPEC-v0.8 §4.5 says there is no path on which a store that ignores the column behaves as + though N were 1. There was: this one. + """ + + def put_approval_request(self, request): + return super().put_approval_request( + replace(request, required_roles=(), approvals_required=1) + ) + + +GATED_POLICY = """ +schema: ctrlrun.policy/v6 +controls: + card-data-handling: + title: t + approver_role: payments-owner +actions: + payments.refund: + decision: approve + approvals_required: 2 + controls: [card-data-handling] +""" + + +def test_T324_a_provider_that_drops_the_pinned_fields_is_refused(clock): + """§4.5, §3.3. The review's first reproduction: it committed, approved once, by nobody + holding the role.""" + store = InMemoryStateStore(clock=clock) + control = Control( + Policy.from_yaml(GATED_POLICY), + store, + clock=clock, + approvals=_OwnRequestProvider(store, clock), + approver_identity=ApproverIdentity(_Fixed(ALICE)), + identity=StaticIdentityProvider(agent=AGENT.agent, user=AGENT.user), + ) + action = _action(control) + executor = _Executor() + + with pytest.raises(ActionDenied) as refused: + control.execute(action, executor, KEY) + + assert refused.value.reason == "approval_unrecorded" + assert executor.calls == 0 + assert "threshold of 2" in str(refused.value) or "roles" in str(refused.value) + # The request the provider already recorded is withdrawn, so a human answering it cannot + # leave a grant some later call spends with nothing compared (v0.7 §6.4). + record = store.get_approval("apr_own_request_provider") + assert record is not None and str(record.status) == "denied" + + +def test_T324_a_store_that_drops_the_pinned_columns_is_refused(clock): + """§4.5's sentence, made true. The review's second reproduction.""" + store = _DroppingStore(clock=clock) + control = Control( + Policy.from_yaml(GATED_POLICY), + store, + clock=clock, + approver_identity=ApproverIdentity(_Fixed(ALICE)), + identity=StaticIdentityProvider(agent=AGENT.agent, user=AGENT.user), + ) + executor = _Executor() + + with pytest.raises(ActionDenied) as refused: + control.execute(_action(control), executor, KEY) + + assert refused.value.reason == "approval_unrecorded" + assert executor.calls == 0 + assert store.get_effect(KEY) is None, "nothing was reserved for a request nobody may answer" + + +# --- T325: G19 in the catalogue --------------------------------------------------------------- + + +VERIFY_DOCUMENT = """ +schema: ctrlrun.policy/v6 +environment: production +actions: + aaa.single: + decision: approve + zzz.refund: + decision: approve + approvals_required: 2 +""" + + +def test_T325_G19_is_in_the_catalogue(): + from ctrlrun.verify import guarantees as reg + + assert "G19" in reg.BY_ID + assert len(reg.BY_ID["G19"].title) <= 32 + + +def test_T325_G19_passes_on_a_document_that_asks_for_two(tmp_path, monkeypatch): + """The catalogue assertion above is not evidence: it grades nothing. + + G17 shipped with exactly that shape of coverage and its scenario was broken on every + document that used the feature, because nothing ever ran the body. This runs it. The + document also sorts an action needing **one** approval first by codepoint, so a scenario + asking about whichever action `select` reached first would report "no action requires more + than one approval" of a document that asks for two. + """ + from ctrlrun.verify import run + + monkeypatch.chdir(tmp_path) + path = tmp_path / "ctrlrun.yaml" + path.write_text(VERIFY_DOCUMENT, encoding="utf-8") + + report = run(path) + + result = {guarantee.id: guarantee for guarantee in report.guarantees}["G19"] + assert str(result.status) in ("Status.PASS", "pass"), ( + f"G19 reported {result.status}: {result.counterexample}" + ) + assert result.detail.get("approvals_required") == 2 + + +def test_T325_G19_is_not_applicable_only_where_every_action_takes_one(tmp_path, monkeypatch): + """§11.7: the reason is a statement about the operator's document.""" + from ctrlrun.verify import guarantees as reg + from ctrlrun.verify import run + + monkeypatch.chdir(tmp_path) + path = tmp_path / "ctrlrun.yaml" + path.write_text(VERIFY_DOCUMENT.replace(" approvals_required: 2\n", ""), encoding="utf-8") + + report = run(path) + + result = {guarantee.id: guarantee for guarantee in report.guarantees}["G19"] + assert "not_applicable" in str(result.status).lower() + assert result.reason == reg.NO_M_OF_N diff --git a/tests/test_mcp_operator.py b/tests/test_mcp_operator.py index efe1482..6bf3246 100644 --- a/tests/test_mcp_operator.py +++ b/tests/test_mcp_operator.py @@ -101,11 +101,13 @@ class Recording: satisfy "the read succeeded" while breaking the rule the test is about. """ - def __init__(self, *, agent="approver-app", user="alice", expires_at=None): + def __init__(self, *, agent="approver-app", user="alice", expires_at=None, roles=None): self.calls: list[str] = [] self.agent = agent self.user = user self.expires_at = expires_at + #: SPEC-v0.8 §3.4 — what this credential's issuer put in the roles claim, or nothing. + self.roles = roles def resolve(self, context): self.calls.append(context.action) @@ -117,6 +119,7 @@ def resolve(self, context): user=self.user, issuer="https://proxy.example/", expires_at=self.expires_at, + claims={"roles": self.roles} if self.roles else {}, ) @@ -196,6 +199,71 @@ def test_T288_a_denial_records_the_principal_too(server, control): assert [who.agent for who in record.approvers] == ["approver-app"] +#: SPEC-v0.8 §3.2 — a registry with a role on it, for the two §3.8 tests. Its own document, so +#: the rest of this file keeps grading the server it was written for. +GATED_POLICY = """ +schema: ctrlrun.policy/v6 +controls: + card-data-handling: + title: Cardholder data changes are approved by a named owner + approver_role: payments-owner +actions: + stripe.refund: + decision: approve + controls: [card-data-handling] +""" + + +@pytest.fixture +def gated(store, tmp_path): + return Control(Policy.from_yaml(GATED_POLICY), store, LocalApprovalProvider(store)) + + +def _pending_on(control): + """Propose through `Control.execute`, so the request carries the roles §3.3 pins.""" + action = _action(control) + with pytest.raises(ApprovalRequired) as raised: + control.execute(action, lambda: "re_1", "refund:txn_1") + return raised.value.request_id + + +def test_T307_the_server_refuses_an_answer_the_credential_is_not_entitled_to(gated): + """SPEC-v0.8 §3.8's other half: the courtesy, where the credential actually is. + + The guarantee is `Control`'s check at consumption; this refuses at the moment a human + answers, so they learn then rather than when an agent retries. Two defences, two tests, on + `CONTRIBUTING.md`'s first shape of a false green. + """ + server = OperatorServer(_config(approver_roles_claim="roles"), gated, Recording()) + # Through `Control.execute`, because that is where the roles are pinned onto the request + # (§3.3): a request built straight from the provider carries none, and a test that did so + # would assert a refusal the deployment never reaches. + request_id = _pending_on(gated) + + document, status = _call(server, "approve", {"request_id": request_id}, credential="alice") + + assert status == 403, document + assert "role" in json.dumps(document) + record = gated.store.get_approval(request_id) + assert str(record.status) == "pending", "a refused answer is not an answer" + + +def test_T307_an_entitled_credential_is_recorded_with_what_it_satisfied(gated): + """The positive control, and what the consume-side check then reads.""" + server = OperatorServer( + _config(approver_roles_claim="roles"), + gated, + Recording(roles=("payments-owner",)), + ) + request_id = _pending_on(gated) + + document, status = _call(server, "approve", {"request_id": request_id}, credential="alice") + + assert status == 200, document + record = gated.store.get_approval(request_id) + assert record.approvers[0].entitled == ("card-data-handling",) + + def _rpc(server, method, params=None, *, request_id=1): body = {"jsonrpc": "2.0", "id": request_id, "method": method} if params is not None: diff --git a/tests/test_policy_versioning.py b/tests/test_policy_versioning.py index 46a798b..afcd7ea 100644 --- a/tests/test_policy_versioning.py +++ b/tests/test_policy_versioning.py @@ -1591,6 +1591,9 @@ def test_T177c_the_command_list_is_exactly_the_one_the_spec_froze(): # a v0.6 feature than that one: §9.4's claim is about the surface *this milestone* grew, # and a subcommand landing before the tag does not retroactively make it one. "scan", + # SPEC-v0.8 §5.3, §11.1. Opening a break-glass envelope is an act, and acts get + # commands. It is a v0.8 addition on the v0.8 line, not a v0.6 one. + "break-glass", ] assert sorted(cli.main.commands) == sorted(frozen_by_v0_6 + after_v0_6) diff --git a/tests/test_store_conformance.py b/tests/test_store_conformance.py index c5cb3cd..18ca809 100644 --- a/tests/test_store_conformance.py +++ b/tests/test_store_conformance.py @@ -20,6 +20,7 @@ from ctrlrun.conformance.store import SUITES, run from ctrlrun.conformance.store.backends import InMemoryBackend, SQLiteBackend from ctrlrun.conformance.store.fixtures import FIXTURES +from ctrlrun.conformance.store.suites import NO_CONTENTION # --- T140: every fixture fails its named suite, and every suite has a fixture ------------- @@ -152,6 +153,11 @@ def test_T141_in_memory_reports_only_the_honest_reasons(tmp_path): assert reasons == { "this backend's storage cannot be opened from another process", "this backend's storage does not outlive the object that holds it", + # SPEC-v0.8 §4.5. Its own sentence rather than the first one above, because it says + # something the others do not: the sequential half of that case **did** run and pass, + # and a reader who saw only "cannot be opened from another process" would not know + # which half of a two-part case they had evidence for. + NO_CONTENTION, NO_CLOCK, }, reasons for suite in report.suites: diff --git a/tests/test_verify.py b/tests/test_verify.py index 991522d..43e921e 100644 --- a/tests/test_verify.py +++ b/tests/test_verify.py @@ -177,13 +177,13 @@ def test_T101_a_policy_with_no_approve_rule_makes_G1_and_G2_not_applicable(tmp_p # because this document names no `max_attempts` (SPEC-v0.7 §8.9). The rest are applicable, # G14 among them, and the count is over those. assert report.applicable == 9 - assert report.not_applicable == 8 + assert report.not_applicable == 10 text = report.to_text() # The fraction is passes over applicable and never the catalogue size: with eight N/As a # seventeen-guarantee catalogue must not report seventeen over seventeen. assert f"{len(reg.GUARANTEES)}/{len(reg.GUARANTEES)}" not in text assert f"{report.passed}/{report.applicable} declared guarantees pass." in text - assert "8 not applicable: G1, G2, G8, G9, G13, G15, G16, G18." in text + assert "10 not applicable: G1, G2, G8, G9, G13, G15, G16, G17, G18, G19." in text def test_T101b_zero_applicable_guarantees_is_not_a_pass(tmp_path): @@ -861,14 +861,14 @@ def test_the_v1_payments_template_reports_nine_over_nine(): report = run(V1_PAYMENTS) assert report.exit_code == 0 - assert (report.passed, report.applicable, report.not_applicable) == (9, 9, 8) + assert (report.passed, report.applicable, report.not_applicable) == (9, 9, 10) text = report.to_text() assert "9/9 declared guarantees pass." in text # G13 is N/A on SQLite, which has no clock of its own; G14 and G15 join G3, G4 and G5 where # the effect template lives in the @protect decorator verify does not read, and where the # document names no `max_attempts`. G16 and G18 are graded: verify brings its own provider # for the first and its own approver identity for the second (§8.9, §11.7). - assert "8 not applicable: G3, G4, G5, G8, G9, G13, G14, G15." in text + assert "10 not applicable: G3, G4, G5, G8, G9, G13, G14, G15, G17, G19." in text assert "10/10" not in text diff --git a/tests/test_verify_action.py b/tests/test_verify_action.py index d7415b9..55a8e41 100644 --- a/tests/test_verify_action.py +++ b/tests/test_verify_action.py @@ -139,8 +139,8 @@ def test_T118_ci_asserts_the_two_shapes_the_specification_names(): assert 'test "$AUTHORITY" = "verified 15/15"' in script assert 'test "$TEMPLATES" = "verified 9/9"' in script - assert 'test "$AUTHORITY_NA" = "2"' in script - assert 'test "$TEMPLATES_NA" = "8"' in script + assert 'test "$AUTHORITY_NA" = "4"' in script + assert 'test "$TEMPLATES_NA" = "10"' in script @pytest.mark.authority @@ -155,10 +155,10 @@ def test_T118_the_two_configurations_really_do_report_those_shapes(): assert authority.badge["message"] == "verified 15/15" # G13 and G15: SQLite has no clock of its own to diverge from, and the document declares # no `max_attempts` (SPEC-v0.7 §8.9). - assert authority.not_applicable == 2 + assert authority.not_applicable == 4 assert templates.badge is not None assert templates.badge["message"] == "verified 9/9" - assert templates.not_applicable == 8 + assert templates.not_applicable == 10 def test_T118_the_action_uploads_the_report_and_writes_a_job_summary(): @@ -211,7 +211,7 @@ def test_T119_the_colour_is_about_failures_and_has_no_amber_for_not_applicable( from ctrlrun.verify import scenarios passing = run(V1_PAYMENTS) - assert passing.not_applicable == 8 + assert passing.not_applicable == 10 assert passing.badge is not None assert passing.badge["color"] == BADGE_PASS_COLOR diff --git a/tests/test_verify_report.py b/tests/test_verify_report.py index 209ae16..1fd4375 100644 --- a/tests/test_verify_report.py +++ b/tests/test_verify_report.py @@ -109,9 +109,9 @@ def test_T113_the_summary_is_the_last_line_and_names_the_not_applicable_ids(tmp_ # G13 is N/A on every SQLite run: SQLite has no clock of its own. G14 needs the effect # template this document keeps in the @protect decorator, and G15 a `max_attempts` it does # not declare (SPEC-v0.7 §8.9). - assert "8 not applicable: G3, G4, G5, G8, G9, G13, G14, G15." in last + assert "10 not applicable: G3, G4, G5, G8, G9, G13, G14, G15, G17, G19." in last # The fraction is passes over applicable. A report with eight N/As does not say 17/17. - assert "17/17" not in text + assert "18/18" not in text def test_T113_a_failing_report_names_the_subject_and_prints_the_counterexample( @@ -137,9 +137,9 @@ def test_T113_a_failing_report_names_the_subject_and_prints_the_counterexample( # SPEC-v0.8 item 2: G18 joins the catalogue. It is graded wherever the document sends # an action to approval, which the first two of these do, and `N/A` for G1's reason # where nothing does. So the first two gain a pass and the third gains an N/A. - (ALL_APPLICABLE, "13/13 declared guarantees pass. 4 not applicable"), - (WITH_NOT_APPLICABLE, "9/9 declared guarantees pass. 8 not applicable"), - (EMPTY, "0/0 declared guarantees pass. 17 not applicable"), + (ALL_APPLICABLE, "13/13 declared guarantees pass. 6 not applicable"), + (WITH_NOT_APPLICABLE, "9/9 declared guarantees pass. 10 not applicable"), + (EMPTY, "0/0 declared guarantees pass. 19 not applicable"), ], ids=["passing", "some-na", "all-na"], ) @@ -443,14 +443,18 @@ def broken(self, name, vector, decision, expected_reason): assert "internal error" in result.stderr -def test_T116_a_run_with_eight_not_applicable_still_exits_0(tmp_path, monkeypatch): - """N/A never changes the exit code by itself.""" +def test_T116_a_run_with_several_not_applicable_still_exits_0(tmp_path, monkeypatch): + """N/A never changes the exit code by itself. + + The count moves whenever the catalogue grows a guarantee this document cannot exercise, so + it is read off the report rather than pinned: what T116 is about is the exit code. + """ monkeypatch.chdir(tmp_path) result = _cli(tmp_path, WITH_NOT_APPLICABLE) assert result.exit_code == 0 - assert "8 not applicable" in result.stdout + assert "10 not applicable" in result.stdout def test_T116_json_and_junit_can_be_combined(tmp_path, monkeypatch):