From 1a87b2da821331482f78869e81103ab2afcc1856 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 11 Sep 2026 14:31:17 +0000 Subject: [PATCH] fix(mandate): fail closed on grant content the service cannot evaluate A mandate is an authorization document, so an input this version does not understand may be one that RESTRICTS the grant. Both places that could happen resolved as "more authority", which is the wrong direction for an authorization decision to fail in. Unknown top-level fields. MandateDocument carried Pydantic's default extra="ignore". The signature is computed over model_dump(), so a mandate issued at a newer schema version lost its unknown fields before canonicalization and failed as "one or more signatures are invalid", indistinguishable from tampering. MandateDocument and IssueMandateRequest now set extra="forbid": an unknown field is a parse error naming the field. Unevaluated conditions. `conditions` is inside the signed grant and nothing in this service evaluates it. An issuer could sign {"env": "staging"} and /verify would answer valid: true in production, with no way for a caller to tell that verdict from one over a mandate carrying no conditions at all. /verify gains a sixth check, conditions_evaluable, which fails when conditions are present and unevaluated. A caller holding its own evaluator passes conditions_evaluated=true to assert it did that work; the flag is an assertion, not a bypass. The principle is standards-independent, and the same one argued publicly in ocsf#1756: an unevaluable restriction cannot bound authority, so it must not read as satisfied. Breaking for external callers submitting populated conditions to /verify. Nothing in this repository does: the smoke script sends an empty map, the overspend demo sends none. The gateway's enforcement path is unaffected, it calls /mandates/{id}/spend directly rather than /verify. Seven tests cover both failure modes, the escape hatch, and that conditions stay inside the signature so stripping them cannot buy a clean verdict. Five of the seven fail against the previous code. Mandate suite 59 passed / 6 skipped, gateway mandate tests 17 passed, ruff check and format clean. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01XyU3PRkgvMxgimkPBC5726 --- CHANGELOG.md | 1 + mandate/app/routers/verify.py | 26 ++++ mandate/app/schemas.py | 52 ++++++- mandate/tests/test_strict_unknown_fields.py | 162 ++++++++++++++++++++ 4 files changed, 236 insertions(+), 5 deletions(-) create mode 100644 mandate/tests/test_strict_unknown_fields.py diff --git a/CHANGELOG.md b/CHANGELOG.md index 828e0112..76a09a45 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,7 @@ Format follows [Keep a Changelog](https://keepachangelog.com/en/1.1.0/). ## [Unreleased] ### Fixed +- **The mandate service treated grant content it could not evaluate as permission granted, in two places.** `MandateDocument` carried Pydantic's default `extra="ignore"`, so an unrecognized top-level field was silently dropped. Because the signature is computed over `model_dump()` (`signing._build_signable_payload`), a mandate issued at a newer schema version lost its unknown fields before canonicalization and then failed with "one or more signatures are invalid", indistinguishable from tampering. Worse, `conditions` is a signed part of the grant that **nothing in this service evaluates**: an issuer could sign `{"env": "staging"}` and `POST /api/v1/mandates/verify` would answer `valid: true` in production, with no way for the caller to tell that verdict apart from one over a mandate carrying no conditions at all. Both now fail closed. `MandateDocument` and `IssueMandateRequest` set `extra="forbid"`, so an unknown field is a parse error naming the field rather than a silent widening of authority. `/verify` gains a sixth check, `conditions_evaluable`, which fails when conditions are present and unevaluated; a caller holding its own evaluator (request context this service does not have) passes `conditions_evaluated=true` to assert it did that work. The principle is standards-independent and the same one argued publicly in `ocsf#1756`: an unevaluable restriction cannot bound authority, so it must not read as satisfied. **Breaking for external callers** that submit mandates with populated `conditions` to `/verify`, which will now report invalid; nothing in this repository does (the smoke script sends an empty map, the overspend demo sends none). The gateway's live enforcement path is unaffected: it calls `/mandates/{id}/spend` directly, not `/verify`. Seven new tests cover both failure modes, the escape hatch, and that conditions stay inside the signature so stripping them cannot buy a clean verdict; five of the seven fail against the previous code. Full mandate suite 59 passed / 6 skipped, gateway mandate tests 17 passed, ruff clean. - **The OCSF sink declared no capabilities, so on praxis-proxy/policy PR #84's new head it silently stopped emitting agent identity, the delegation chain and security labels.** `0651258` made audit sinks filtered like any other plugin: the engine pairs each handler with its own `plugins:` capability set in a new `AttachedSink` and hands `handle` a filtered `Extensions`. Correct, and this crate declared nothing. Of the six typed fields it maps, `request`, `mcp` and `completion` are ungated, but `agent` sits behind `read_agent`, `delegation` behind `read_delegation`, and security labels behind `read_labels`. Reproduced through `examples/panic_drive.rs`, which drives a real engine through `load_config` (the unit tests build `Extensions` directly and never reach the filter): at the old pin `499ee91` the record carries `ai_agent.uid: agent-7`; at `5b76fa6` with no capabilities it carries no `ai_agent` block at all, and still chains, still signs and still verifies offline. That is the failure mode this project exists to prevent, since a verifier cannot distinguish "no delegation occurred" from "the sink was not permitted to see it". Both demo config variants and both README wiring examples now declare the three capabilities, with a note on why omitting them is an evidence loss rather than a load error. The pin moves to `5b76fa6` and `AuditHandler::handle` / `on_effect` / `as_audit_handler` are unchanged there, so the crate needed no source change. Verified the way CI runs it, toolchain 1.96.1 with a cpex sibling at `64c8eba`: warning-free `--locked` checks and 34 tests green on each host, `emit_sample` byte-identical to the AID-EMIT-1 section 12 conformance vector on both. The same head also answers our review comment: `emit_decision` is the single verdict finalizer, route-resolution denials seed a decision log, and reconciled effects take their place in the current stream rather than under the sequence numbers of the run that crashed. - **Port results no longer ask PR #84 to list the `plugin_settings` rename as a breaking change.** `PRAXIS-PORT-RESULTS.md` observation 3 recommended a line in #84's breaking-changes list for the `plugin_settings:` to `engine_settings:` rename being a load error on PPE. Teryl answered that the rename is not a #84 change: it shipped in praxis-proxy/policy #55 on 2026-08-31 and is documented under the 0.2.0 Removed section of the upstream CHANGELOG and in `docs/upgrade-apl.md`, verified here against the `3e7734e` head. The observation now carries that answer inline, the same way observation 1 carries its fix, and the rename stays in the "What changed" list as the porting note it is. Docs only, no product impact. - **The PPE audit-key workaround is gone: `audit_stream_namespace` loads from the file again.** `integrations/cpex-ocsf-audit` pinned praxis-proxy/policy PR #84 at `20798ae`, a head whose engine-settings allowlist rejected every audit key `docs/auditing.md` documents, so `examples/panic_drive.rs` set the stream namespace in code on PPE rather than in the YAML (reported upstream as `PRAXIS-PORT-RESULTS.md` observation 1, shipped with #513). Teryl fixed it in `3e7734e`, "accept the documented engine_settings audit keys at load". The pin moves to that head and the workaround comes out: `set_host_identity` now sets only the epoch, on both hosts, which is the one value that genuinely cannot live in a static file because it must stay monotonic across boots. Verified the way CI runs it, on toolchain 1.96.1 with a cpex sibling at the pinned `64c8eba`: warning-free `--locked` builds and 34 tests green on each host, `emit_sample` byte-identical so the AID-EMIT-1 section 12 conformance vector is untouched, and `panic_drive` driven end to end on PPE through `parse_config` and `load_config` with the namespace taken from the YAML, landing the real contained panic on `gw-1:decision` with verdict deny and violation `plugin_panic`. README, `PRAXIS-PORT-RESULTS.md` and the pin comment no longer describe a workaround that does not exist. diff --git a/mandate/app/routers/verify.py b/mandate/app/routers/verify.py index 3fc6f097..3f4e3a63 100644 --- a/mandate/app/routers/verify.py +++ b/mandate/app/routers/verify.py @@ -12,6 +12,8 @@ 3. not_expired — valid_until is None OR now < valid_until 4. within_spend_limit — spent_cents <= spend_limit.limit_cents (when a limit exists) 5. scope_sufficient — if required_scope provided, mandate.scope is a superset + 6. conditions_evaluable: the mandate carries no condition this service + cannot evaluate (the check explains why it fails closed) """ import logging @@ -120,6 +122,30 @@ async def verify_mandate( else: checks["scope_sufficient"] = True + # 6. Conditions check: fail closed on a restriction nothing evaluated. + # + # `conditions` is part of the signed grant, so an issuer writing + # {"env": "staging"} has expressed a real limit on the authority. This + # service has no evaluator for it. Reporting `valid: true` anyway would + # answer "is this mandate good?" while ignoring a clause of the mandate, + # and a caller cannot tell that answer apart from one where the issuer + # imposed no conditions at all. + # + # So an unevaluated condition is a failed check, not a warning. A caller + # holding its own evaluator (a gateway with request context this service + # does not have) asserts `conditions_evaluated=true` and takes + # responsibility for that half of the verdict. + if not mandate.conditions or body.conditions_evaluated: + checks["conditions_evaluable"] = True + else: + checks["conditions_evaluable"] = False + errors.append( + f"Mandate carries {len(mandate.conditions)} condition(s) this service " + f"cannot evaluate: {sorted(mandate.conditions)}. Evaluate them against " + f"the request context and resubmit with conditions_evaluated=true, or " + f"treat the mandate as unverified." + ) + valid = all(checks.values()) return VerifyMandateResult( valid=valid, diff --git a/mandate/app/schemas.py b/mandate/app/schemas.py index 15997e38..8caff20c 100644 --- a/mandate/app/schemas.py +++ b/mandate/app/schemas.py @@ -19,7 +19,7 @@ from enum import StrEnum from typing import Any -from pydantic import BaseModel, Field, field_validator +from pydantic import BaseModel, ConfigDict, Field, field_validator # ── Enums ───────────────────────────────────────────────────────────────── @@ -99,7 +99,22 @@ class MandateExceedance(BaseModel): class MandateDocument(BaseModel): - """Canonical MongoDB document shape for a mandate.""" + """Canonical MongoDB document shape for a mandate. + + `extra="forbid"`: an unrecognized top-level field is a parse error, not a + silently dropped one. A mandate is an authorization document, so a field + this version cannot evaluate might be one that *restricts* the grant, and + dropping it would widen authority past what the issuer signed. + + Fail-closed also makes version skew diagnosable. The signature is computed + over `model_dump()` (see `signing._build_signable_payload`), so under the + old `extra="ignore"` default a mandate issued at a newer schema version + lost its unknown fields before canonicalization and failed with "one or + more signatures are invalid", indistinguishable from tampering. It now + fails naming the field it did not recognize. + """ + + model_config = ConfigDict(extra="forbid") mandate_id: str = Field(description="Human-readable ID: mnd_<8-char-hex>") schema_version: str = "1.1" @@ -110,7 +125,14 @@ class MandateDocument(BaseModel): scope: list[str] = Field(description="Permission scopes, e.g. ['read:audit', 'write:policies']") conditions: dict[str, Any] = Field( - default_factory=dict, description="ABAC conditions (env, tier, etc.)" + default_factory=dict, + description=( + "ABAC conditions (env, tier, etc.), part of the signed grant. " + "NOTHING IN THIS SERVICE EVALUATES THEM YET: verification reports " + "them as unevaluated and fails closed rather than reporting a " + "mandate valid while ignoring restrictions its issuer signed. " + "A caller that has its own evaluator passes conditions_evaluated=true." + ), ) policy_hash: str | None = Field(None, description="SHA-256 of the linked policy rules JSON") spend_limit: SpendLimit | None = Field( @@ -144,7 +166,14 @@ def scope_not_empty(cls, v: list[str]) -> list[str]: class IssueMandateRequest(BaseModel): - """Body for POST /api/v1/mandates.""" + """Body for POST /api/v1/mandates. + + `extra="forbid"` for the same reason as `MandateDocument`, one step + earlier: a caller that misspells a restricting field should be told, not + issued a broader mandate than it asked for. + """ + + model_config = ConfigDict(extra="forbid") subject_agent_id: str subject_org_id: str @@ -248,6 +277,18 @@ class VerifyMandateRequest(BaseModel): """Body for POST /api/v1/mandates/verify — accepts a full mandate payload.""" mandate: MandateResponse + conditions_evaluated: bool = Field( + default=False, + description=( + "Set true only if the caller has already evaluated the mandate's " + "`conditions` against its own request context and they hold. This " + "service has no evaluator, so by default a mandate carrying any " + "condition fails verification: an unevaluable restriction cannot " + "bound authority, and reporting such a mandate valid would mean " + "answering a question about the grant while ignoring part of it. " + "The flag asserts the caller did the work; it is not a bypass." + ), + ) class VerifyMandateResult(BaseModel): @@ -256,7 +297,8 @@ class VerifyMandateResult(BaseModel): checks: dict[str, bool] = Field( description=( "Individual check results: signatures_valid, status_active, " - "not_expired, scope_sufficient, within_spend_limit" + "not_expired, scope_sufficient, within_spend_limit, " + "conditions_evaluable" ) ) error: str | None = None diff --git a/mandate/tests/test_strict_unknown_fields.py b/mandate/tests/test_strict_unknown_fields.py new file mode 100644 index 00000000..6c338dc1 --- /dev/null +++ b/mandate/tests/test_strict_unknown_fields.py @@ -0,0 +1,162 @@ +"""Strict handling of grant content this version cannot evaluate. + +A mandate is an authorization document. Two ways it can carry a restriction +the service does not understand, and both used to resolve as "more authority", +which is the wrong direction for an authorization decision to fail in: + + 1. An unrecognized top-level field, silently dropped by Pydantic's default + ``extra="ignore"``. + 2. A populated ``conditions`` map, which is inside the signed grant and which + nothing in this service evaluates. + +These tests pin both to fail closed. They are written sync + ``asyncio.run`` +for the same reason as ``test_signing.py``: no pytest-asyncio dependency. +""" + +import asyncio +from datetime import UTC, datetime, timedelta + +import pytest +from cryptography.hazmat.primitives import serialization +from cryptography.hazmat.primitives.asymmetric import ec +from pydantic import ValidationError + +from common.config.settings import settings +from mandate.app.routers.verify import verify_mandate +from mandate.app.schemas import ( + IssueMandateRequest, + MandateDocument, + MandateIssuer, + MandateResponse, + MandateStatus, + MandateSubject, + VerifyMandateRequest, +) +from mandate.app.signing import sign_mandate + + +@pytest.fixture +def fresh_local_key(monkeypatch): + priv = ec.generate_private_key(ec.SECP256R1()) + pem = priv.private_bytes( + encoding=serialization.Encoding.PEM, + format=serialization.PrivateFormat.PKCS8, + encryption_algorithm=serialization.NoEncryption(), + ).decode() + monkeypatch.setattr(settings, "forensic_signing_key_pem", pem, raising=False) + monkeypatch.setattr(settings, "forensic_signing_key_id", "", raising=False) + return pem + + +def _mandate_kwargs(**overrides): + now = datetime.now(UTC) + base = { + "mandate_id": "mnd_deadbeef", + "status": MandateStatus.active, + "issuer": MandateIssuer(org_id="org_test", user_id="user_test"), + "subject": MandateSubject(agent_id="agt_test", org_id="org_test"), + "scope": ["read:audit"], + "valid_from": now, + "valid_until": now + timedelta(days=1), + "signatures": [], + "created_at": now, + "updated_at": now, + } + base.update(overrides) + return base + + +def _signed(mandate: MandateDocument) -> MandateDocument: + mandate.signatures = [asyncio.run(sign_mandate(mandate))] + return mandate + + +def _verify(mandate: MandateDocument, **body_kwargs): + request = VerifyMandateRequest(mandate=MandateResponse(**mandate.model_dump()), **body_kwargs) + return asyncio.run(verify_mandate(request, required_scope=None)) + + +# --- 1. Unrecognized top-level fields --- + + +def test_unknown_field_is_rejected_not_dropped(): + """A field this version does not know is a parse error. + + Under ``extra="ignore"`` this constructed fine and lost the field, so a + mandate issued at a newer schema version verified against a narrower + reading of its own grant. + """ + with pytest.raises(ValidationError) as exc: + MandateDocument(**_mandate_kwargs(geo_restriction={"allow": ["US"]})) + assert "geo_restriction" in str(exc.value) + + +def test_unknown_field_on_issue_request_is_rejected(): + """Same rule one step earlier, so a misspelled limit is not a wide grant.""" + with pytest.raises(ValidationError) as exc: + IssueMandateRequest( + subject_agent_id="agt_test", + subject_org_id="org_test", + scope=["read:audit"], + spend_limmit={"limit_cents": 5000}, # codespell:ignore + ) + assert "spend_limmit" in str(exc.value) # codespell:ignore + + +def test_known_fields_still_construct(): + """Guard against the rule being too broad to issue an ordinary mandate.""" + m = MandateDocument(**_mandate_kwargs(conditions={"env": "prod"})) + assert m.conditions == {"env": "prod"} + + +# --- 2. Conditions nothing evaluates --- + + +def test_mandate_without_conditions_verifies(fresh_local_key): + m = _signed(MandateDocument(**_mandate_kwargs())) + result = _verify(m) + assert result.checks["conditions_evaluable"] is True + assert result.valid is True + + +def test_unevaluated_conditions_fail_closed(fresh_local_key): + """The bug this file exists for. + + An issuer signs {"env": "staging"}. Nothing here evaluates it. Before this + change the endpoint answered ``valid: true`` in production, and a caller + could not tell that verdict apart from one over a mandate with no + conditions at all. + """ + m = _signed(MandateDocument(**_mandate_kwargs(conditions={"env": "staging"}))) + result = _verify(m) + + assert result.checks["conditions_evaluable"] is False + assert result.valid is False + assert "env" in result.error + # Every other check still passes: the mandate is well-formed and correctly + # signed. It is the unevaluated restriction alone that sinks the verdict. + assert result.checks["signatures_valid"] is True + assert result.checks["status_active"] is True + assert result.checks["not_expired"] is True + + +def test_caller_can_assert_it_evaluated_conditions(fresh_local_key): + """The escape is an assertion of work done, not a bypass of the check.""" + m = _signed(MandateDocument(**_mandate_kwargs(conditions={"env": "staging"}))) + result = _verify(m, conditions_evaluated=True) + assert result.checks["conditions_evaluable"] is True + assert result.valid is True + + +def test_conditions_stay_inside_the_signature(fresh_local_key): + """Conditions are part of the grant, so editing them breaks the signature. + + Without this, failing closed on conditions would be theatre: an attacker + would strip the field and get a clean verdict. + """ + m = _signed(MandateDocument(**_mandate_kwargs(conditions={"env": "staging"}))) + stripped = m.model_copy(update={"conditions": {}}) + + result = _verify(stripped) + assert result.checks["signatures_valid"] is False + assert result.valid is False