diff --git a/CHANGELOG.md b/CHANGELOG.md index 828e011..76a09a4 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 3fc6f09..3f4e3a6 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 15997e3..8caff20 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 0000000..6c338dc --- /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