Record which peer asked for a signature in the audit log - #949
Conversation
WalkthroughThe signing audit model now records optional requester share indexes. HMAC generation and verification authenticate requester presence and value. Signing paths attribute resolved requesters for refusals and commitments while leaving other operations unattributed. ChangesRequester-aware signing audit logging
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@keep-frost-net/src/node/signing.rs`:
- Line 530: Update the refusal handling around record_refusal to pass the
already resolved requester as Some(requester) instead of None, matching the
pre-sign refusal path and attributing the inbound refusal to the verified peer
identity.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: fb5e0406-ead6-4000-bfa3-2c4db37b1af0
📒 Files selected for processing (2)
keep-frost-net/src/audit.rskeep-frost-net/src/node/signing.rs
1 participant
Summary
Entries recorded what this node signed but not who asked for it. For a single holder that is enough, since there is only one party who could have asked. It stops being enough the moment several principals share a signer, which is the shape an agent deployment takes: the first question about any entry is which agent produced it, and no field answered it.
Both entries written while handling an inbound request now carry the requester's share index: the policy refusal and the structured-payload mismatch. The second is the one most worth attributing, since it fires when a requester's body does not produce the digest it asked us to sign, which is an attempted relabel rather than a configuration saying no.
The field is covered by the entry HMAC, because a field the HMAC does not cover is one anyone with write access can change, and this is exactly the claim an audit log exists to make unforgeable. That is safe to add here only because nothing persists these entries: the log is an in-memory queue rebuilt on every start, read by one status endpoint. There are no stored entries whose verification could break. The vault's hash-chained audit log is a different type in a different crate and is untouched.
Adding a variable-width field at the end exposed an existing weakness in the preimage. The participant list is written without a length prefix; every field after it used to be fixed width, so the total length still determined where the list ended. A trailing optional removed that, and two different records began hashing identically: two participants plus an index and a one-byte operation are also the bytes of three participants plus a larger index and a different operation. The list is now length-prefixed.
Two claims from the first version of this change were wrong and are corrected rather than quietly dropped. The requester is not attribution for the whole round: the session id is derived from the message, threshold and sorted participants, none of which mention the requester, and participant selection does not depend on who asked, so two peers requesting the same digest derive the same session id. A duplicate request is also answered from the cached commitment before the requester is resolved, so a second peer can drive a round whose remaining entries would join back to the first. The field now documents that it attributes the entry it sits on and nothing more. And the index is only as trustworthy as peer admission, which is weaker than it sounds, because member transport keys are derived from the group public key; it identifies which member slot a request arrived under rather than proving who sat in it.
The value is now also emitted on the tracing line, alongside every sibling field.
Test plan
Five tests. Reattributing an entry breaks its HMAC. An unattributed entry does not verify as one attributed to share index zero. Two different records do not share a tag, using the exact values that collided. And both inbound-request call sites assert the recorded peer, which nothing did before: the existing refusal test exercised the attributed path with peer 2 and asserted nothing about it, so either call site could have passed nothing and the suite would have stayed green.
Falsified rather than assumed, three ways. Unbinding the requester from the HMAC fails the first two. Removing the length prefix reproduces the collision exactly. Passing nothing at either call site fails both wiring tests.
One earlier claim was withdrawn because falsification refuted it: the tag byte distinguishing present from absent is not what prevents a collision with share index zero. Removing it leaves those tests passing. It stays to keep the field self-delimiting, and the comment no longer claims more than that.
455 library tests pass, workspace builds, formatter and clippy clean. Follow-ups filed rather than fixed here: carrying the requester on the session so downstream entries are attributed, and the transport-key derivation that bounds what any of this can prove.