Repository navigation
Judge enclave raw messages and FROST shares only on what the enclave can read - #1124
Conversation
…can read The enclave signs a raw Sign message as given, so it cannot tell what that message spends or which event it is. It judged the amount, address and event kind rules on fields the host sent with the request, and passed them when the host sent none; FROST shares carried none at all. SigningRequest no longer carries amount, destination or event kind, on either side of the wire. The policy engine now sees what the enclave itself read: an analyzed PSBT, or an unread request. An unread request fails MaxAmountSats and the address rules, with the policy's action deciding the outcome as for any failed rule. A MaxFeeSats anywhere in the loaded policy refuses it outright, since it may pay any fee. AllowedEventKinds fails every request, a PSBT included, so an Or holding it can no longer let a spend past the caps beside it. The rate, hour and weekend rules apply as before. The wire digest is updated on both sides, docs/ENCLAVE.md says which rules can judge which requests, and the stale enclave sign-psbt CLI example is removed.
|
No actionable comments were generated in the recent review.
 |
| Layer / File(s) | Summary |
|---|---|
Policy inputs and request contract keep-enclave/enclave/src/policy.rs, keep-enclave/enclave/src/vsock_server.rs, keep-enclave/host/src/protocol.rs, keep-cli/src/commands/enclave.rs, keep-enclave/host/src/backend.rs, keep-enclave/host/src/mock.rs, keep-enclave/host/tests/integration_tests.rs |
SigningContext distinguishes unread requests from analyzed spends. SigningRequest removes the event-kind, amount, and destination fields. Request examples, wire-format digests, and initializers are updated. |
Policy evaluation and unit tests keep-enclave/enclave/src/policy.rs |
Amount, fee, address, and event-kind rules use the new context semantics. Fee-cap detection checks nested rules. Tests cover unread requests, spend data, and fee-bound analysis. |
Signing paths and policy validation keep-enclave/enclave/src/vsock_server.rs, docs/ENCLAVE.md |
Raw and FROST policy checks use unread content. PSBT checks use spend amount, fee, and destination. Signing tests and documentation describe the updated policy outcomes. |
Priority:
Estimated code review effort: 3 (Moderate) | ~25 minutes
Change: Bug fix
Sequence Diagram(s)
sequenceDiagram
participant Host
participant VsockServer
participant PolicyEngine
Host->>VsockServer: Send raw, FROST, or PSBT signing request
alt Raw or FROST request
VsockServer->>PolicyEngine: Evaluate Contents::Unread
else PSBT request
VsockServer->>PolicyEngine: Evaluate Contents::Spend with amount, fee, and destination
end
PolicyEngine-->>VsockServer: Return policy decision
VsockServer-->>Host: Return signing response
Possibly related PRs
- privkeyio/keep#1122: Changes
SigningContextand rate-rule evaluation, which share the policy evaluation flow updated here. - privkeyio/keep#1123: Adds FROST policy checks that this change updates to use unread content.
- privkeyio/keep#1109: Adds enclave PSBT analysis whose spend data this change uses in policy evaluation.
Suggested reviewers: wksantiago
Merge Risk: 
d50d9
-
Raw and FROST signing follow the documented policy restrictions. No material merge-blocking risk was identified.
Pre-merge checks: 
-
Check name Status Explanation Resolution Docstring Coverage Docstring coverage is 29.41% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documenta… Write docstrings for the functions missing them to satisfy the coverage threshold. -
Check name Status Explanation Description Check Check skipped - CodeRabbit’s high-level summary is enabled. Title check The title clearly and concisely describes the main policy change for raw messages and FROST shares. Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request.
Full details: Docstring Coverage
-
Explanation
Docstring coverage is 29.41% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 17 functions across 3 files. (1 skipped: 1 unsupported.)
Resolution
Write docstrings for the functions missing them to satisfy the coverage threshold.
-
Finishing Touches: 💡 1
-
📝 Generate docstrings: 💡
-
- Commit to this branch
- Create a new PR
🧪 Generate unit tests (beta)
-
- Commit to this branch
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
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.
A rabbit checks the signing flow,
Unread requests have rules to know.
PSBT spends bring amounts clear,
Fee and address checks appear.
The bunny hops; the tests all glow.
Comment @coderabbitai help to get the list of available commands.
Summary
The enclave signs a raw
Signmessage as given, so it cannot read what the message spends or which event it is. Until now it judged the amount, address and event kind rules on fields the host sent alongside the request, and FROST shares were judged with none of them. This makes the policy engine judge only what the enclave itself read.SigningRequestdropsamount_sats,destinationandevent_kindon both the host and the enclave. The wire digest is updated on both sides.SigningContextnow holdsContents::Spend { amount_sats, fee_sats, destination }for an analyzed PSBT orContents::Unreadfor a raw message or FROST share.MaxAmountSats,AddressAllowlistandAddressBlocklist; the policy's action then decides, as for any failed rule.MaxFeeSatsanywhere in the loaded policy, at any depth and under any action, refuses an unread request, since it may pay any fee.AllowedEventKindsfails every request, a PSBT included, so anOrholding it cannot let a spend past the caps beside it. The fee bound analysis counts it as unable to pass.docs/ENCLAVE.mdsays which rules can judge which requests, and thekeep enclave sign-psbtexample, a command that does not exist, is removed.An enclave whose policy has amount, fee or address rules now signs spends only through
SignPsbt. Raw signing still works under rate, hour and weekend rules.Testing
cargo +1.89.0 test --manifest-path keep-enclave/enclave/Cargo.toml(default andallow-plaintext-import) andcargo +1.89.0 test -p keep-enclave-hostSummary by CodeRabbit
MaxFeeSatsrule rejects raw and FROST requests, regardless of its action or position in the policy.AllowedEventKindsrules fail all requests.