Skip to content

fix(security): key request_hash with an HMAC under the audit key (chain_version 4) - #627

Open
Georgefifth wants to merge 2 commits into
lacs-project:mainfrom
Georgefifth:fix/268-hmac-request-hash
Open

Georgefifth wants to merge 2 commits into
lacs-project:mainfrom
Georgefifth:fix/268-hmac-request-hash

Conversation

@Georgefifth

Copy link
Copy Markdown
Contributor

Problem

request_hash is a single unsalted SHA-256 over action_name || 0x00 || canonical_json(params) with unredacted params, and sysknife audit export serialises the column. For a low-entropy, partly-public parameter set — ConfigureWifi, where the SSID broadcasts and only the passphrase is unknown — a holder of an export can brute-force the preimage offline. Scope 1 of #268 shipped in #318; this is Scope 2.

Solution

  • request_hash is now HMAC-SHA256(key, action || 0x00 || canonical), keyed by RequestHashKey: HKDF-SHA256 over the audit key material with its own domain tag (sysknife-request-hash-v1\x1f, added to the prefix-free tag test). Message framing and the 64-hex column width are unchanged, so the wire format and the execute-time binding check keep working — preview and execute both hash through DaemonState.request_hash_key.
  • New rows are signed under chain_version = 4 (same field set as v3; the tag records which hash scheme the row's request_hash uses). Both writers (SQLite transactions.rs, Postgres store/postgres.rs) derive the stored version from the signed identity as before. verify_rows already treats the column as opaque, so offline verification is unaffected.
  • Rows written under v1–v3 keep their original hashes — they are inside signed bytes, and rewriting them would break verification. The fix is forward-only; docs/cli.md and docs/the-audit-chain.md state the residual exposure for legacy rows plainly.
  • Wiring: DaemonState loads the audit key once (open_full) or receives it (open_with_audit, Postgres path) and derives the HMAC key from the same material the chain signatures use, so the two cannot drift.

Tests, per the issue's checklist:

  • HMAC-bound round trip: a_chain_spanning_all_four_encodings_verifies (v1→v4 in one walk), plus the existing store-level migration tests which now append v4 onto v1/v2 fixtures and re-verify.
  • Migration negative case: the frozen v1, v2 and v3 golden vectors (a_legacy_v1_row_on_disk_still_verifies, a_row_written_by_the_previous_release_still_verifies, a_v3_row_on_disk_still_verifies) all still pass untouched — the bump invalidates nothing on disk.
  • New goldens: a_v4_row_on_disk_still_verifies (with a nibble-flip rejection half, as the v2 golden has) and request_hash_golden_vector (the derivation is now a wire contract across upgrades).
  • v3_and_v4_encodings_do_not_alias, request_hash_is_not_the_unsigned_sha256 (the ConfigureWifi shape from the issue), request_hash_is_deterministic_and_key_bound, request_hash_key_is_derived_not_the_signing_seed.

Both goldens were mutation-checked: reverting the HMAC to a bare SHA-256 reds the three request_hash tests; making the v4 encoder sign chain_version = 3 reds the v4 golden and the alias test.

Result

  • cargo fmt --all -- --check, cargo clippy --workspace --all-features --all-targets -- -D warnings: clean.
  • cargo nextest run --workspace: 1999 passed, 6 skipped (frontend not in scope).
  • Live Postgres (SYSKNIFE_TEST_POSTGRES_URL, --run-ignored=all): 1149 passed, 0 skipped.
  • tests/release/audit-export-confidentiality.test.sh: passes (the hash-before-redact order guard still holds).
  • tests/evidence/workspace-tests.json regenerated (1993 → 1999); README, docs/introduction.md and docs/distro-support.md updated in the same commit. check_evidence_claims.py and check_public_claims.sh pass.

hmac and hkdf were already in Cargo.lock (via sqlx-postgres); this adds them as direct daemon dependencies at the locked versions.

Closes #268

@Georgefifth

Copy link
Copy Markdown
Contributor Author

Note on the red security-audit check: it fails on RUSTSEC-2026-0381 (webpki-roots 1.0.7, published 2026-10-19) plus pre-existing unmaintained/unsound warnings. webpki-roots comes in via sqlx's TLS stack and is already pinned at 1.0.7 on main — this PR's Cargo.lock change adds only hmac/hkdf as direct daemon dependencies (both already in the lock via sqlx-postgres). So the failure is an upstream advisory that predates this branch, not something the change introduces; I left the bump out of scope.

Comment thread crates/sysknife-daemon/src/audit_chain.rs Dismissed

@vladimirrott vladimirrott left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you, @Georgefifth. This is the careful version of #268 I was hoping for: an HKDF-derived key under its own domain tag instead of reusing the Ed25519 seed as an HMAC key, the same framing and width so nothing downstream changes shape, chain_version = 4 with each older row still verified under its own rule, and hmac/hkdf already in the lockfile, so no new code arrives. Reviewed at 9b406d5.

What I checked:

  • The crypto: HKDF-SHA256(ikm = seed, info = "sysknife-request-hash-v1\x1f"), and the new tag joins the prefix-free domain-tag test. No client needs the key: the CLI, MCP and daemon-test pass the preview's request_hash through as an opaque value, and only the daemon recomputes it.
  • The auditor path: verify_rows treats request_hash as an opaque signed field, so audit verify --pubkey still needs no secret. verify_audit_chain_with_pubkey_intact_after_inserts writes v4 rows and passes.
  • Old chains: the v1 to v3 goldens are untouched, and the mixed-chain test covers all four encodings.
  • Gates on the merged tree: fmt, clippy --all-targets, doc, the test baseline (1,999, matching the evidence file and the three doc figures), and the Postgres contract (running 11 tests, 11 passed).
  • Mutations: HMAC → bare SHA-256 inside RequestHashKey, a constant key, v4 rows dispatched as v3, and the v4 encoder signing 3 all fail your tests.

One blocking item. The live path has no test that it is keyed. Preview and execute call compute_request_hash (dispatcher.rs:2313, :2720). With its body changed back to a bare sha256(action \0 canonical), the daemon and CLI suites pass, 1,496 of 1,496. The dispatcher tests near :5204 check determinism and key ordering, and a bare SHA-256 satisfies both. So a later refactor that skips the key would reopen #268 with CI green. A test that runs a real preview and asserts its request_hash equals state.request_hash_key.request_hash(...) and differs from the unsalted SHA-256 would pin it. Revert compute_request_hash to the old body and it should fail.

Optional, not blocking:

  • Upgrading strands pending previews. A preview stored by 0.25 holds the SHA-256 value, so its execute after the upgrade fails closed with stale_approval: action or parameters differ from the approved preview. That's safe, and your golden-test comment already names it. One CHANGELOG sentence ("preview anything pending again after upgrading") would save someone a confusing minute.
  • The v4 golden's only negative twin rotates chain_hash. A frozen v4 row relabelled chain_version = 3 being refused would pin the version field itself.
  • Stale text: docs/the-audit-chain.md:127 still says caller_principal is signed "only under chain_version = 3", and the test comment near audit_chain.rs:2758 says "only in the v3 arm".

The red security-audit was main's (three webpki advisories), fixed by #628 this afternoon. Update the branch and it goes green. Once the dispatcher test is in, I'll re-run that same mutation against it.

The request_hash column was a single unsalted SHA-256 over the action name
and the unredacted params, and sysknife audit export serialises it. For a
low-entropy, partly-public parameter set (ConfigureWifi: the SSID broadcasts,
leaving the passphrase as the only unknown) any holder of an export could
brute-force the preimage offline.

The hash is now an HMAC-SHA256 under a key derived from the audit key via
HKDF with its own domain tag, so the value cannot be verified or cracked
without key material that never leaves the host. The message framing is
unchanged, and the preview/execute binding still works because both sides
now hash with the same derived key.

New rows are written under chain_version = 4 (same signed field set as v3;
the bump records which hash scheme the row's request_hash uses). Rows written
under v1-v3 keep their original hashes: they are inside signed bytes, so
rewriting them would break verification. The fix is forward-only and the docs
say so plainly.

Closes lacs-project#268
@Georgefifth
Georgefifth force-pushed the fix/268-hmac-request-hash branch from 9b406d5 to aeddeb3 Compare October 26, 2026 22:40
@Georgefifth

Copy link
Copy Markdown
Contributor Author

Rebased onto main (which picks up #628, so security-audit should go green) and addressed everything at aeddeb3:

Blocking — added preview_request_hash_is_keyed_not_a_bare_sha256 in the dispatcher tests. It drives a real preview over the socket and pins the response hash two ways: equals state.request_hash_key.request_hash(...) over the canonical framing, and differs from the unsalted SHA-256 of the same preimage. Mutation-checked in the shape you described: reverting compute_request_hash's body to sha256(action \0 canonical) (keeping the key parameter, so everything still compiles) fails this test, and restoring the HMAC body makes it pass again.

Optional 1 — CHANGELOG entry now ends with: "After upgrading, re-preview anything still pending: a preview stored by an older release carries the old unsalted hash, so its execute fails closed with stale_approval rather than running."

Optional 2 — the v4 golden test now also relabels the frozen row chain_version = 3 and asserts Broken, pinning the version field itself alongside the rotated-hash twin.

Optional 3 — both stale spots fixed: docs/the-audit-chain.md now says the principal is signed "under chain_version = 3 and every later encoding that carries it (v4 today)", and the census test comment says "in the v3 and v4 arms".

Baseline: suite is 2009 on the rebased tree (2002 + this PR's 7); regenerated the artifact and the three prose figures in the same commit. Re-verified locally: fmt, clippy -D warnings, rustdoc -D warnings, workspace nextest 2009 passed / 6 skipped, live Postgres (--run-ignored=all) 1150 passed, release confidentiality shell test, evidence + public-claims screens.

vladimirrott
vladimirrott previously approved these changes Oct 27, 2026

@vladimirrott vladimirrott left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you, @Georgefifth. preview_request_hash_is_keyed_not_a_bare_sha256 is the test I asked for and a bit more: it drives a real preview over the socket and pins the hash from both sides, equal to the HMAC under the daemon's key and unequal to the old SHA-256. You also took all three optional items, and the relabel-to-v3 half of the v4 golden pins the version field the way I hoped. Reviewed at aeddeb3.

I approve it.

What I checked on this push:

  • The new test bites on the line that matters. I had my merge gate run it in a container, unmutated and then with compute_request_hash (dispatcher.rs:1022) reverted to a bare SHA-256 that ignores the key:
mutation: s/^    key\.request_hash(action_name, &canonical)$/    { let _ = key; hex::encode(Sha256::digest(format!("{action_name}\\x00{canonical}").as_bytes())) }/
`preview_request_hash_is_keyed_not_a_bare_sha256` passes clean and fails under the mutation (rc=101)
test dispatcher::tests::preview_request_hash_is_keyed_not_a_bare_sha256 ... FAILED
  • The rebase carried no new production code. git range-diff between 9b406d5 and aeddeb3 shows the new dispatcher test, the v4 golden's relabel half, one test comment, the CHANGELOG sentence, the audit-chain doc line and the moved test count. Nothing else in crates/**/src changed.
  • The board. rust, postgres-contract, docs-and-hygiene, security-audit and the rest pass at this head, so the count of 2,009 agrees with the evidence file and the three prose figures.

One check is red, and it's on me: CodeQL raised rust/hard-coded-cryptographic-value at audit_chain.rs:3337. That line is the 0x42 seed in request_hash_key_is_derived_not_the_signing_seed, inside #[cfg(test)] mod tests, so it's a test fixture and not a key anything ships with. I'll dismiss the alert as test code and merge after that.

#678 is still there for you if you want it after this one.

Main moved to 2,004 Rust tests (lacs-project#626, lacs-project#684); this branch adds seven, so
the artifact and the three published figures read 2,011. No other
conflict.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01UpyrhbDTW1gBwzGAC44GYU

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

audit export publishes request_hash, an unsalted hash over unredacted params, with no statement of its sensitivity

3 participants