From 31da1af5e46b14393807f91d98b3d24edc1aac8e Mon Sep 17 00:00:00 2001 From: tegwick Date: Wed, 9 Sep 2026 14:09:54 +0200 Subject: [PATCH] Record the verified principal type on approver entries (schema v4) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit An entry stored subject_id, assurance and evidence_ref but nothing about what kind of principal bound the approval, and subject_id is a naming convention rather than a verified claim. /entries is not restricted by principal type — only /consume is — and the approval-engine-operator client holds approval:approve, so a service can supply approver evidence today. Whether it may is gate-house doctrine; that it is legible is ours. Add entries.principal_type, populate it from the verified token, surface it on the object and the audit evidence path (not the claim, which stays least-disclosure), and migrate v3 stores leaving legacy rows null rather than back-filling a claim nobody made. Also corrects two statements in the requirements issued to informed-decision: agent tokens are not barred from approval:approve, and an empty assurance object is accepted rather than refused. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01HyybaE7DUXrWYrhbnESCTe Assistant: claude-code Assistant-Model: opus Assistant-Process: 1275879@bnt-lap001 Assistant-Session: eb464208-f821-41b2-bc5a-a6c33d92a8ad --- approval_engine/api.py | 1 + approval_engine/store.py | 26 ++++++--- docs/approver-surface-requirements.md | 31 ++++++++--- docs/caller-authentication.md | 13 ++++- tests/test_auth.py | 77 +++++++++++++++++++++++++++ tests/test_cas.py | 32 +++++++++-- 6 files changed, 160 insertions(+), 20 deletions(-) diff --git a/approval_engine/api.py b/approval_engine/api.py index 43825a7..98cc724 100644 --- a/approval_engine/api.py +++ b/approval_engine/api.py @@ -145,6 +145,7 @@ class App: identity.subject, assurance=json.dumps(identity.assurance, sort_keys=True), evidence_ref=identity.evidence_ref, + principal_type=identity.principal_type, ) return 200, obj.as_dict() if rest == ["revoke"] and method == "POST": diff --git a/approval_engine/store.py b/approval_engine/store.py index 455e8d4..4aac8f5 100644 --- a/approval_engine/store.py +++ b/approval_engine/store.py @@ -35,7 +35,7 @@ AUDIT_SCHEMA = "audit-core.event.v1alpha1" SOURCE = "approval-engine" SCOPE = "netkingdom-approvals" EVENT_CLASSES = ("issuance", "use", "supersession", "revocation", "heartbeat") -LATEST_SCHEMA_VERSION = 3 +LATEST_SCHEMA_VERSION = 4 SCHEMA = """ CREATE TABLE IF NOT EXISTS approvals ( @@ -66,6 +66,7 @@ CREATE TABLE IF NOT EXISTS entries ( approved_at TEXT NOT NULL, assurance TEXT, evidence_ref TEXT, + principal_type TEXT, PRIMARY KEY (approval_id, subject_id) ); CREATE TABLE IF NOT EXISTS outbox ( @@ -230,6 +231,15 @@ class Engine: conn.execute( "ALTER TABLE approvals ADD COLUMN pdp_path INTEGER NOT NULL DEFAULT 0" ) + entry_columns = { + row["name"] for row in conn.execute("PRAGMA table_info(entries)").fetchall() + } + if "principal_type" not in entry_columns: + # v4. Legacy entries stay NULL. An entry recorded before this + # column existed carries no verified statement about what kind + # of principal bound it, and defaulting it to 'human' would + # manufacture approver evidence that was never presented. + conn.execute("ALTER TABLE entries ADD COLUMN principal_type TEXT") outbox_columns = { row["name"] for row in conn.execute("PRAGMA table_info(outbox)").fetchall() } @@ -423,10 +433,11 @@ class Engine: "approved_at": e["approved_at"], "assurance": e["assurance"], "evidence_ref": e["evidence_ref"], + "principal_type": e["principal_type"], } for e in self._conn() .execute( - "SELECT subject_id, approved_at, assurance, evidence_ref " + "SELECT subject_id, approved_at, assurance, evidence_ref, principal_type " "FROM entries WHERE approval_id=? ORDER BY approved_at", (row["id"],), ) @@ -485,6 +496,7 @@ class Engine: *, assurance: str | None = None, evidence_ref: str | None = None, + principal_type: str | None = None, ) -> Approval: if not subject_id: raise Unprocessable("subject_id is required") @@ -501,9 +513,10 @@ class Engine: raise Conflict(f"cannot add entries in status {row['status']}") try: conn.execute( - "INSERT INTO entries (approval_id, subject_id, approved_at, assurance, evidence_ref) " - "VALUES (?,?,?,?,?)", - (approval_id, subject_id, now, assurance, evidence_ref), + "INSERT INTO entries " + "(approval_id, subject_id, approved_at, assurance, evidence_ref, principal_type) " + "VALUES (?,?,?,?,?,?)", + (approval_id, subject_id, now, assurance, evidence_ref, principal_type), ) except sqlite3.IntegrityError as exc: conn.rollback() @@ -767,7 +780,7 @@ class Engine: a future schema that relaxes it. """ rows = conn.execute( - "SELECT subject_id, approved_at, assurance, evidence_ref " + "SELECT subject_id, approved_at, assurance, evidence_ref, principal_type " "FROM entries WHERE approval_id=? ORDER BY approved_at, subject_id", (approval_id,), ).fetchall() @@ -780,6 +793,7 @@ class Engine: "approved_at": r["approved_at"], **({"assurance": r["assurance"]} if r["assurance"] else {}), **({"evidence_ref": r["evidence_ref"]} if r["evidence_ref"] else {}), + **({"principal_type": r["principal_type"]} if r["principal_type"] else {}), } for r in seen.values() ] diff --git a/docs/approver-surface-requirements.md b/docs/approver-surface-requirements.md index b01275b..c41bd73 100644 --- a/docs/approver-surface-requirements.md +++ b/docs/approver-surface-requirements.md @@ -53,11 +53,22 @@ the deployed issuer's `/jwks` (RS256) and carry: The **access** token is what we validate. `id_token` appears nowhere in this codebase; the ID token belongs to the login client and is never evidence here. -Agents may assemble a memo but must never complete a binding act — that is -`informed-decision`'s own design principle 10, and it matches this engine: an -agent token cannot hold `approval:approve` under the requested registrations, -and an entry recorded under an agent subject would be indistinguishable from a -human's in the evidence chain. +**Correction to our 2026-09-09 message.** We said an agent token cannot hold +`approval:approve` under the requested registrations. That is wrong twice: the +`approval-engine-operator` service client holds `approval:approve`, and this +engine does not restrict `/entries` by principal type at all — only `/consume` +is restricted, to `service`/`agent`. So a non-human principal can supply +approver evidence today. + +Whether it *should* be able to is approval doctrine and belongs to +`gate-house`, not to us and not to you. What we have done instead is make it +visible: schema v4 records the verified `principal_type` on every entry +(`tests/test_auth.py::test_entry_records_the_verified_principal_type`, +`::test_non_human_approver_is_recorded_as_such`). Your design principle 10 +("humans bind, agents draft") is therefore enforceable in the evidence chain +rather than assumed — read `entries[].principal_type` and do not infer the +answer from the shape of `subject_id`. Entries written before v4 are `null` and +must not be read as `human`. ## 3. Open question A — the human client cannot read the approval it renders @@ -100,9 +111,13 @@ on the human client — the requirement here is that the resulting token's (authentication method, `acr`/`amr`, `auth_time` at minimum), rather than leaving MFA as a property of the login that nothing downstream can see. -`informed-decision` and `key-cape` own that shape between them. This engine will -store whatever they agree on and will not silently accept an empty object as -evidence of anything. +`informed-decision` and `key-cape` own that shape between them. To be exact +about what this engine does and does not do: an empty `assurance` object +(`{}`) is *accepted* — validation checks only that the claim is present and is +an object. So an empty assurance is not refused here; it simply produces an +entry that evidences nothing about how the person authenticated. If MFA has to +be *provable* from the approval record, the shape has to carry it, because +nothing downstream reconstructs it. ## 5. Open question C — `view_hash` has nowhere to go today diff --git a/docs/caller-authentication.md b/docs/caller-authentication.md index f5a5d1b..75694ad 100644 --- a/docs/caller-authentication.md +++ b/docs/caller-authentication.md @@ -20,8 +20,17 @@ cross-tenant reads and mutations are rejected before object lookup. | explicit heartbeat | `approval:emit` | Create additionally requires `binding.actor == sub`. Approval-entry subject, -assurance, and evidence reference are derived from the verified JWT, never the -request body. KeyCape owns client registration and scope grants; approval-engine +assurance, evidence reference, and **principal type** are derived from the +verified JWT, never the request body. + +`principal_type` is recorded on the entry (schema v4) because `subject_id` +alone cannot answer what kind of principal bound the approval — `user:alice` is +a naming convention, not a verified claim. This engine does not restrict +`/entries` to human principals: whether a service or agent may supply approver +evidence is approval doctrine and belongs to `gate-house`, and the +`approval-engine-operator` registration holds `approval:approve` today. What +this engine owes is that the evidence chain says which it was. Entries written +before v4 stay `null` rather than being back-filled into a claim nobody made. KeyCape owns client registration and scope grants; approval-engine only verifies and enforces them. Requested registrations are: - audience/resource server `approval-engine` with the scopes above; diff --git a/tests/test_auth.py b/tests/test_auth.py index bfda2a3..8acbfa5 100644 --- a/tests/test_auth.py +++ b/tests/test_auth.py @@ -346,3 +346,80 @@ def test_human_principal_cannot_consume(engine): assert status == 200 assert claim["consumed"] is False assert claim["valid_now"] is True + + +def test_entry_records_the_verified_principal_type(engine): + """Approver evidence must say what kind of principal bound the approval. + + `subject_id` alone cannot answer it: `user:alice` is a naming convention, + not a verified claim. The recorded value comes from the token and from + nowhere else, so an evidence reader can tell a human bind from a service + one without trusting a string's shape. + """ + from approval_engine.api import App + + creator = Identity( + subject="agt-secrets-engine", + issuer="test", + audiences=("approval-engine",), + principal_type="service", + tenant="tenant:platform", + roles=frozenset(), + scopes=frozenset({"approval:create", "approval:read"}), + assurance={"level": "aal1"}, + evidence_ref="service", + ) + human = Identity( + subject="user:alice", + issuer="test", + audiences=("approval-engine",), + principal_type="human", + tenant="tenant:platform", + roles=frozenset(), + scopes=frozenset({"approval:approve"}), + assurance={"level": "aal2", "amr": ["pwd", "otp"]}, + evidence_ref="human", + ) + app = App(engine, StaticTokenAuthenticator({"creator": creator, "human": human})) + _, created = call( + app, + "POST", + "/v1/approvals", + {"binding": binding(), "validity": validity()}, + authorization="Bearer creator", + ) + _, approved = call( + app, + "POST", + f"/v1/approvals/{created['id']}/entries", + {"principal_type": "service"}, + authorization="Bearer human", + ) + entry = approved["entries"][0] + assert entry["subject_id"] == "user:alice" + assert entry["principal_type"] == "human" + + _, claim = call( + app, "GET", f"/v1/approvals/{created['id']}/claim", authorization="Bearer creator" + ) + assert "approvers" not in claim + + +def test_non_human_approver_is_recorded_as_such(app): + """A service principal holding `approval:approve` is not refused — but it + is not silently indistinguishable from a human either. + + Whether a non-human may supply approver evidence at all is approval + doctrine and belongs to gate-house; the `approval-engine-operator` + registration holds `approval:approve` today. This engine's obligation is + that the evidence chain records which it was. + """ + _, created = call( + app, + "POST", + "/v1/approvals", + {"binding": binding(), "validity": validity()}, + ) + _, approved = call(app, "POST", f"/v1/approvals/{created['id']}/entries", {}) + assert approved["status"] == "approved" + assert approved["entries"][0]["principal_type"] == "service" diff --git a/tests/test_cas.py b/tests/test_cas.py index 3dce2be..fef2ffa 100644 --- a/tests/test_cas.py +++ b/tests/test_cas.py @@ -6,7 +6,7 @@ from pathlib import Path import pytest from approval_engine.errors import Conflict, Unprocessable -from approval_engine.store import Engine +from approval_engine.store import LATEST_SCHEMA_VERSION, Engine from tests.conftest import FROZEN, approve, binding, validity @@ -174,8 +174,8 @@ def test_successor_inherits_the_pdp_path_declaration(engine): assert successor.pdp_path is True -def test_v2_database_migrates_to_v3_preserving_approvals(): - """Schema v3 (GH-DEC-2026-008) must not disturb approvals issued under v2.""" +def test_v2_database_migrates_forward_preserving_approvals(): + """A v2 store must upgrade to the current schema without disturbing approvals.""" with tempfile.TemporaryDirectory() as tmp: path = Path(tmp) / "v2.sqlite" eng = Engine(path, clock=lambda: FROZEN) @@ -189,7 +189,7 @@ def test_v2_database_migrates_to_v3_preserving_approvals(): upgraded = Engine(path, clock=lambda: FROZEN) version = int(upgraded._conn().execute("PRAGMA user_version").fetchone()[0]) - assert version == 3 + assert version == LATEST_SCHEMA_VERSION survivor = upgraded.get(obj.id) assert survivor.status == "approved" assert survivor.pdp_digest == "sha256:" + "12" * 32 @@ -197,4 +197,28 @@ def test_v2_database_migrates_to_v3_preserving_approvals(): # a digest that happens to be present assert survivor.pdp_path is False assert upgraded.claim(obj.id)["binding"]["pdp_path"] is False + # v4: a legacy entry carries no verified statement about the kind of + # principal that bound it, and must not be back-filled into one + assert survivor.entries[0]["principal_type"] is None + upgraded.close() + + +def test_v3_entries_migrate_to_v4_without_inventing_a_principal_type(): + """The v4 column is added; pre-v4 approver entries stay unclassified.""" + with tempfile.TemporaryDirectory() as tmp: + path = Path(tmp) / "v3.sqlite" + eng = Engine(path, clock=lambda: FROZEN) + obj = eng.create(binding(), validity()) + eng.add_entry(obj.id, "user:alice", principal_type="human") + eng._conn().execute("ALTER TABLE entries DROP COLUMN principal_type") + eng._conn().execute("PRAGMA user_version=3") + eng._conn().commit() + eng.close() + + upgraded = Engine(path, clock=lambda: FROZEN) + assert int(upgraded._conn().execute("PRAGMA user_version").fetchone()[0]) == 4 + survivor = upgraded.get(obj.id) + assert survivor.status == "approved" + assert survivor.entries[0]["subject_id"] == "user:alice" + assert survivor.entries[0]["principal_type"] is None upgraded.close()