Record refused sign requests in the audit log - #946
Conversation
|
Warning Review limit reached
Next review available in: 34 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
WalkthroughThe PR adds ChangesSigning refusal audit
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant SigningNode
participant PreSignHook
participant SigningAuditLog
participant Requester
SigningNode->>PreSignHook: evaluate signing request
PreSignHook-->>SigningNode: refuse request
SigningNode->>SigningAuditLog: record SignRequestRefused
SigningNode->>Requester: notify refusal
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`:
- Around line 521-538: Add a shared refusal-recording helper around the existing
audit_log.log_signing_operation call, then invoke it in the structured-payload
mismatch path before send_session_error and from the existing refusal path.
Ensure both policy refusal paths create a SignRequestRefused audit entry with
the same request context.
🪄 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: c65acd40-0cd0-4988-8495-b5bf450b67b2
📒 Files selected for processing (2)
keep-frost-net/src/audit.rskeep-frost-net/src/node/signing.rs
|
All four addressed, and the eviction one was right to raise as a decision rather than a nit: left alone it would have made this change net-negative on the exact configuration it exists to serve. Refusals now live in their own bounded ring. The two streams have different natural bounds, which is what makes sharing one queue wrong rather than merely tight: accepted-path entries are limited by how many sessions can be open, while a refusal creates no session, so a peer can trigger one per request indefinitely. In a shared FIFO the unbounded stream evicts the bounded one, and it does that on a node with co-signing switched off, whose history is the history most worth keeping. Recording refusals is only an improvement if it cannot destroy the record it was added to strengthen. Reads merge both rings on timestamp so the log still presents as one chronological account. Verified by falsification: reverting to the shared queue makes the flood test fail on the surviving-entry assertion. The structured-payload refusal is audited too. That asymmetry was the right thing to flag, and the direction matters: the case being skipped was the sharper of the two. A policy refusal is a configuration saying no; a structured-payload mismatch is a requester whose body does not produce the digest it asked us to sign. Same trust position, same reachability, more alarming, and it was the one going unrecorded. The missing reason is a fair hit and worth naming precisely: the doc comment asserted the reason stays in the log line, and the log line did not carry it. The claim was written from what the code should have done rather than from what it did, so the comment was describing an intention. The error is now on the warning, and the sibling refusal twenty lines above already did this correctly, which is where the wording came from. The duplicated comment came from a restore that went wrong. Falsifying the audit call means removing it and putting it back, my backup write failed silently, and I reconstructed the block by re-applying rather than reverting, because reverting would have taken the new test with it. Re-applying on top of an already-present comment is how it doubled. The lesson is the one already in the invariants doc and I did not follow it: commit before falsifying, so the restore is a revert rather than a reconstruction. Discriminant distinctness is now pinned. That gap was real and self-concealing: the log writes and verifies through the same function, so it agrees with itself whatever the numbering is, and no existing test could have caught a collision. On the question of whether the accept path is meaningfully bounded: sessions do complete and free slots, so a determined member can churn the accepted ring serially on a default-hooks node. That reading is correct. It does not change the conclusion here, because the refusal path needed no session at all and so had no backpressure whatsoever, but it is worth recording that the accepted ring's bound is a rate limit rather than a hard ceiling. |
|
Addressed, and the shared-helper half is worth taking on its own merits. The structured-payload path was already recorded as of a5f0fb4, which landed after this comment was written. Independent review had reached the same conclusion, which is some evidence the asymmetry was the obvious thing to notice: recording a configuration refusal while a requester whose body does not produce the digest it asked us to sign goes unrecorded gets the priority backwards. The helper is now in as well. Two identical call sites recording the same thing is exactly the shape that drifts, and the drift would be silent: nothing fails when one path stops recording, which is the property that made the original gap invisible. Both notified refusals now go through Not folded in: the duress freeze and the pre-admission early returns. Those sit before peer admission, so auditing them would let any sender write to the log, which is the flooding problem the separate refusal ring exists to bound. Recording them would reintroduce it one layer further out. The two paths audited here are both a known member's behaviour. 451 library tests pass, formatter and clippy clean. |
1 participant
Summary
Every audit operation records something that happened, so the log answered what this node signed but not what it was asked to sign and declined. A peer probing a co-signer left a warning and nothing else, and a warning is not something a holder still has to consult a week later. That matters more now that mobile refuses a class of requests it previously accepted.
Two refusals sit past peer admission and both are now recorded. The pre-sign policy refusal, which also covers the kill switch and the desktop approval prompt since they refuse by returning an error from the same hook. And the structured-payload mismatch, which fires when a requester's body does not produce the digest it asked us to sign. The second is the sharper signal of the two: a configuration saying no versus an attempted cross-domain relabel. Recording one without the other would have left the more alarming case invisible.
Refusals go in their own bounded ring rather than the shared queue, and that is the part worth reviewing. The two streams have different natural bounds: accepted-path entries are limited by how many sessions can be open at once, while a refusal creates no session, so a peer can trigger one per request indefinitely. Sharing a single FIFO means the unbounded stream walks the bounded one out of the log, and it does that precisely on a node with co-signing switched off, which is the configuration whose history is worth keeping. Recording refusals is only an improvement if it cannot destroy the record it was added to strengthen. Reads merge both rings on timestamp, so splitting the storage is a retention decision and does not change what the log reads like.
The new operation carries no reason. A policy returns a formatted string and a custom hook could build one from requester-supplied content; an audit log is the wrong place to accept text an adversary influences. The reason now genuinely does go to the log line, which the first version of this claimed while omitting the error from the warning.
The entry is written before the requester is notified, deliberately. The recorded fact is that we refused, which holds whether or not the peer could be told, and a refusal that could not be delivered is the one most worth keeping. That differs from the accept path, which logs after its send because there the recorded fact is the send itself; the divergence is now noted at both sites.
Discriminants are appended, never renumbered, and that is now documented and tested. The values are covered by the entry HMAC, so renumbering one would fail verification on every entry written under the old numbering, reading as tampering rather than a version skew.
Test plan
Four tests. A real refusal driven end to end through a node with a refusing policy, a trusted peer and a raw-labelled request, asserting the entry exists for that session and that the whole log still verifies. A refusal flood against a small cap, asserting an accepted-path entry survives and that refusals stay inside their own bound. Discriminant distinctness, which nothing else pins, since the log writes and verifies through the same function and so agrees with itself either way. Plus the existing HMAC tests.
Falsified rather than assumed, twice. Removing the audit call makes the first fail with an empty entry list. Reverting the refusal ring to the shared queue makes the flood test fail on the surviving-entry assertion. Both restored and passing. 451 library tests pass, formatter and clippy clean.
Note for local runs: the crate's integration tests need the
testingfeature, so a barecargo test -p keep-frost-netfails on unrelated unresolved imports first.