feat(daemon): sign the acting account into approval events - #491
Georgefifth wants to merge 1 commit into
Conversation
The approval-event chain signs seq/key_id/kind/transaction_id/
receipt_digest/created_at and no account, so a grant and a consume by
different uids leave one indistinguishable signed trace. The transaction
chain already signs the requester (ChainIdentity::V3); the event chain
did not.
Add EventIdentity { LegacyV1, V2 { caller_principal } } beside
ChainIdentity with the same discipline: an appended suffix on the
six-field message, prefix-free framing, exhaustive match. event_message
selects per row, so `sysknife audit verify` stays correct over a mixed
legacy+V2 chain.
Migration 4 on both backends adds chain_version (NOT NULL DEFAULT 1, so
every historical row lands on LegacyV1 without a rewrite) and a nullable
caller_principal, never backfilled because rewriting an old row's
message would report the whole chain as Broken. The stored chain_version
is derived from the same identity that was signed, so the column is
provably the version whose message produced the signature.
Thread CallerPrincipal through approve_transaction (approver),
claim_approved_for_execution (executor), and revoke_unconsumed_approval
(revoker) across TransactionStore, the AuditStore trait, both backends,
and the dispatcher call sites. cancel_queued signs the cancelling
account (handle_cancel holds the caller); only the daemon-side stale
sweep records Unattributed.
Status events stay LegacyV1: they are written from spawned execution
tasks with no caller in scope, and signing them with the creator's
principal would be a claim the code cannot support.
Tests: a committed legacy golden-vector hash (a literal, not recomputed)
fails if the six-field encoding is edited even though in-process
round-trips stay green; V2 sign/verify, grant-vs-consume by different
uids distinguishable, stored-principal tamper Broken, V2->LegacyV1
downgrade Broken, blank/absent principal on V2 Broken, unknown version
CannotVerify (exit 2), mixed chain Intact; SQLite + live-Postgres
round-trip persistence; migration-count assertions 3->4. Baseline
1873 -> 1883.
Closes lacs-project#249
vladimirrott
left a comment
There was a problem hiding this comment.
Reviewed at 1237a196d5b25ca35ab6e5cf6b47314d6775272e.
The committed golden vector is the part I would point other contributors at. a_legacy_event_row_signed_by_the_previous_release_still_verifies spells out the key, every field and the 128-hex signature as literals, so an edit to the six-field encoding fails the test instead of moving both sides of a round trip together. You also posted the design in #249 before writing code, and the cancel_queued call you flagged there is the right one: handle_cancel holds the caller, so signing Unattributed on that path would throw away an identity the daemon has.
maintainer screen 491 returns DO NOT EXECUTE, so everything below ran in rootless podman with --network=none against the merge of your head with main at f22a6abd (clean, merge commit 744431ce).
What I verified
The Postgres contract, 11 of 11, against postgres:17-alpine in a pod with no network (output trimmed to the summary and the test this PR changes):
$ out="$(timeout 3000 podman run --rm --pod r23pg -v /tmp/r23m491:/repo:ro -v /tmp/r23target491:/target:z -v "$HOME/.cargo:/cargo:O" -w /repo -e CARGO_HOME=/cargo -e CARGO_TARGET_DIR=/target -e CARGO_NET_OFFLINE=true -e 'SYSKNIFE_TEST_POSTGRES_URL=postgres://sysknife:sysknife@127.0.0.1:5432/sysknife_test?sslmode=disable' -e SYSKNIFE_REQUIRE_POSTGRES=1 docker.io/library/rust:1-slim cargo test -p sysknife-daemon --test postgres_store --locked --offline -- --include-ignored 2>&1)"; rc=$?; echo "POSTGRES CONTRACT rc=$rc"; printf '%s\n' "$out" | grep -v level=warning | grep -E '^(running|test |test result:)'
POSTGRES CONTRACT rc=0
running 11 tests
test migrates_legacy_schema_and_enforces_store_contract ... ok
test result: ok. 11 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.38s
Two guards bite. With the caller_principal field dropped from the V2 signed bytes (the event_version field kept), editing_a_stored_event_principal_reports_broken fails; that run is the sign line in the table further down. With the Postgres approve_transaction made to sign Unattributed, the contract catches it:
$ out="$(timeout 3000 podman run --rm --pod r23pg -v /tmp/r23m491_pg:/repo:ro -v /tmp/r23target491:/target:z -v "$HOME/.cargo:/cargo:O" -w /repo -e CARGO_HOME=/cargo -e CARGO_TARGET_DIR=/target -e CARGO_NET_OFFLINE=true -e 'SYSKNIFE_TEST_POSTGRES_URL=postgres://sysknife:sysknife@127.0.0.1:5432/sysknife_test?sslmode=disable' -e SYSKNIFE_REQUIRE_POSTGRES=1 docker.io/library/rust:1-slim cargo test -p sysknife-daemon --test postgres_store --locked --offline -- --include-ignored 2>&1)"; rc=$?; echo "PG APPROVER->Unattributed rc=$rc"; grep -q 'Compiling sysknife-daemon' <<<"$out" && echo "(rebuilt)"; printf '%s\n' "$out" | grep -v level=warning | grep -E '^test result:|left:|right:|FAILED$'
PG APPROVER->Unattributed rc=101
(rebuilt)
test migrates_legacy_schema_and_enforces_store_contract ... FAILED
left: Some("none:unattributed")
right: Some("uid:1000")
test result: FAILED. 10 passed; 1 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.33s
Every mutated run below checks that sysknife-daemon recompiled, because a copied tree keeps its old mtimes and cargo will otherwise reuse the previous binary.
The six lib failures you will see in the table below are the same six on your unmutated head (five watermark tests that pass one at a time and fail together under plain cargo test, and a load-sensitive executor test). CI uses nextest, which gives each test its own process, and CI is green.
The test count is consistent: ten new test functions, none removed, and 1,873 + 10 = 1,883 in all four places.
Blocking: nothing checks which account the handlers pass
Every new principal assertion calls the store directly with a CallerPrincipal the test chose. None goes through handle_approve, handle_execute or handle_cancel, the code that decides which account gets signed. I replaced caller.principal() with CallerPrincipal::Unattributed at the handle_approve call site, then at the handle_execute call site, and in a third copy made the SQLite cancel_queued sign Unattributed instead of the canceller. Every daemon test target gives the same result on all three as on your head. The loop touches each tree's daemon sources, runs cargo test -p sysknife-daemon --offline --lib, asserts the crate rebuilt, and prints any failure outside the six baseline names:
=== sign rc=101
(rebuilt sysknife-daemon)
audit_chain::tests::editing_a_stored_event_principal_reports_broken
test result: FAILED. 908 passed; 7 failed; 0 ignored; 0 measured; 0 filtered out; finished in 5.63s
=== approve rc=101
(rebuilt sysknife-daemon)
test result: FAILED. 909 passed; 6 failed; 0 ignored; 0 measured; 0 filtered out; finished in 5.63s
=== execute rc=101
(rebuilt sysknife-daemon)
auth::tests::token_file_with_trailing_newline_still_matches
test result: FAILED. 908 passed; 7 failed; 0 ignored; 0 measured; 0 filtered out; finished in 5.62s
=== cancel rc=101
(rebuilt sysknife-daemon)
test result: FAILED. 910 passed; 5 failed; 0 ignored; 0 measured; 0 filtered out; finished in 2.01s
=== head rc=101
(rebuilt sysknife-daemon)
test result: FAILED. 909 passed; 6 failed; 0 ignored; 0 measured; 0 filtered out; finished in 5.61s
The auth token-file failure under execute is unrelated: that test passed three runs out of three on its own against the same mutated tree. The integration targets under tests/ give the same failing set on head and on all three mutations.
So a daemon that signs none:unattributed for every grant and every consume would pass the suite, and #249 exists to rule that out. The file already has the pattern: the_recorded_principal_is_the_one_the_connection_was_attributed_to drives a connection as uid 4242 and reads the stored row back. This test, placed next to it, passes on your head and fails on each dispatcher mutation:
#[tokio::test]
async fn handlers_sign_the_connection_account_into_approval_events() {
let dir = tempdir().unwrap();
let state = test_state(&dir);
let audit = std::sync::Arc::clone(&state.audit);
let (client, server) = tokio::net::UnixStream::pair().unwrap();
tokio::spawn(async move {
unix_connection_handler(
server,
state,
runner(),
uid_caller_with(4242, CallerRole::Admin),
)
.await;
});
let mut framed = FramedStream::new(client);
let (txid, receipt) = preview_and_approve(&mut framed, "GetMemoryInfo", json!({})).await;
framed
.send(
&serde_json::to_vec(&json!({
"type": "execute",
"request_id": "r-exec",
"transaction_id": txid,
"action_name": "GetMemoryInfo",
"params": {},
"approval_receipt": receipt
}))
.unwrap(),
)
.await
.unwrap();
let _exec: Value = serde_json::from_slice(&framed.recv().await.unwrap()).unwrap();
let events = audit.fetch_event_rows().await.unwrap();
let principal = |kind: &str| {
events
.iter()
.find(|e| e.kind == kind)
.unwrap_or_else(|| panic!("no {kind} event"))
.caller_principal
.clone()
};
assert_eq!(principal("approval_granted").as_deref(), Some("uid:4242"));
assert_eq!(principal("approval_consumed").as_deref(), Some("uid:4242"));
}My copy of it printed its events on the way, which is why the output below has PROBE lines:
=== r23m491_probe rc=0
(rebuilt sysknife-daemon)
PROBE execute response type: "job_started"
PROBE event kind=approval_granted principal=Some("uid:4242") version=2
PROBE event kind=approval_consumed principal=Some("uid:4242") version=2
test dispatcher::tests::probe_249_handlers_sign_the_connection_account_into_approval_events ... ok
=== r23m491_approve_probe rc=101
(rebuilt sysknife-daemon)
PROBE event kind=approval_granted principal=Some("none:unattributed") version=2
PROBE event kind=approval_consumed principal=Some("uid:4242") version=2
left: Some("none:unattributed")
right: Some("uid:4242")
test dispatcher::tests::probe_249_handlers_sign_the_connection_account_into_approval_events ... FAILED
=== r23m491_execute_probe rc=101
(rebuilt sysknife-daemon)
PROBE event kind=approval_granted principal=Some("uid:4242") version=2
PROBE event kind=approval_consumed principal=Some("none:unattributed") version=2
left: Some("none:unattributed")
right: Some("uid:4242")
test dispatcher::tests::probe_249_handlers_sign_the_connection_account_into_approval_events ... FAILED
The cancel path needs the same treatment: a cancel request on that connection after an approve, asserting the approval_revoked row says uid:4242. I have not written or run that one, so take the shape from the test above rather than from me. New tests move the baseline again; CONTRIBUTING.md lets you leave that count to me if you cannot build the GUI members.
Also needed before merge, and small
docs/the-audit-chain.md still describes the event chain as it was. "The approval-event chain" section lists the three kinds and says nothing about chain_version on events, what V2 signs, or that status events stay on the legacy encoding. The transaction-chain half of the same page has a version table (line 52 onward) you can mirror. An auditor reading sysknife audit verify output reaches that page first.
Optional
docs/automatic-rollback.md:130 still says the transaction row records "who approved what". #249 quoted that line as wrong. After this PR the approver lives in the event chain, so one sentence pointing there would make it true.
I will write the CHANGELOG entry at merge.
Problem
The approval-event chain signs
seq, key_id, kind, transaction_id, receipt_digest, created_at— and no account.ChainIdentity::V3already signs the requester into the transaction chain, but the event rows forapproval_granted/approval_consumed/approval_revokedcarry nobody. Two accounts approving and consuming the same receipt leave one indistinguishable signed trace: on a host where the account that spends a receipt differs from the one that granted it, the chain cannot say so.Closes #249.
What changed
New event encoding, per-row selectable (
EventIdentity)audit_chain.rs:EventIdentity { LegacyV1, V2 { caller_principal } }besideChainIdentity, with the same discipline — an appended suffix on the six-field message, prefix-free framing, exhaustive match so a forgotten generation arm is a compile error.event_message/canonical_bytesselect per row, sosysknife audit verifystays correct over a mixed legacy+V2 chain.EVENT_VERSION_LEGACY = 1/EVENT_VERSION_V2 = 2constants; a V2 row with empty or absent principal isBroken; an unknown version isCannotVerify(exit 2), mirroring the transaction chain's split.Migration 4 on both backends (was the next free version on each)
DEFAULT 1puts every historical row on LegacyV1 without touching it — no backfill, because rewriting an old row's message would report the whole chain as Broken. New approval events are written at version 2 with the acting account signed in; the storedchain_versionis derived from the sameidentitythat was signed, so the column is provably the version whose message produced the signature.Threading —
CallerPrincipal(the type the transaction chain already signs) goes intoapprove_transaction(signs the approver),claim_approved_for_execution(signs the executor),revoke_unconsumed_approval(signs the revoker) acrossTransactionStore, theAuditStoretrait, both backends, and the dispatcher call sites from theCallerAttributionthe handlers already hold.Status events stay LegacyV1. They are written from
update_statusinside spawned execution tasks with no caller in scope; signing them with the transaction creator's principal would be a claim the code cannot support. One judgment call worth flagging:cancel_queuedrevokes an unconsumed approval fromhandle_cancel, which does hold the caller — so its revoke event signs the cancelling account, notUnattributed. Only the daemon-side stale sweep (cleanup_stale_queued, no connection to attribute) recordsUnattributed, which is exactly what that principal renders as.Tests
Mutation run (the committed fixture is a real oracle). The legacy golden vector's hash is a committed literal, computed once under the fixed test key — not recomputed by the test. Mutating the LegacyV1 encoding (injecting one extra field) makes it fail even though the in-process round-trip tests stay green:
New coverage:
audit_chain.rs): committed legacy fixture verifies; V2 signs/verifies with the acting account; grant-by-uid:1000 + consume-by-uid:1001 are two distinguishable signed records; editing a stored principal reports Broken; downgrading V2→LegacyV1 reports Broken; blank/absent principal on a V2 row is Broken; unknown version is CannotVerify (exit 2); mixed legacy+V2 chain verifies intact.transactions.rs): over the real SQLite store, the principal survives the approve→claim round-trip into persisted rows (alice grants, bob executes, chain intact); status events persist as LegacyV1 with no principal and the mixed chain still verifies.postgres_store.rs): same acceptance assertions on the Postgres backend — event rows name uid:1000 (grant) and uid:1001 (consume), both at the V2 encoding; migration 4 applies and is idempotent.Migration-count assertions updated 3→4 in both backends' migration tests (they pin
MAX(version)/COUNT(*), which is what they exist for).