Keyshare cleans state #164
Conversation
WalkthroughThe pull request involves multiple changes primarily centered around the removal of the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Keyshare
participant Context
User->>Keyshare: Trigger E3RequestComplete
Keyshare->>Keyshare: clear_secret()
Keyshare->>Context: Notify to stop actor
Assessment against linked issues
Possibly related PRs
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
ryardley
changed the base branch from
main
to
ry/128-keymanagement-encryption
ryardley
changed the title
ryardley
changed the title
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
packages/ciphernode/keyshare/src/keyshare.rs (1)
72-74: Consider adding documentation for the clear_secret method.
The implementation is correct and aligns with the PR objective to clean up keyshare state. Consider adding a doc comment to explain its purpose and relationship with E3 round completion.
+ /// Clears the encrypted secret from memory. This is called when an E3 round completes
+ /// to ensure proper cleanup of sensitive data.
fn clear_secret(&mut self) {
self.secret = None;
}📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
packages/ciphernode/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (8)
- packages/ciphernode/Cargo.toml (0 hunks)
- packages/ciphernode/data/Cargo.toml (0 hunks)
- packages/ciphernode/data/src/data_store.rs (1 hunks)
- packages/ciphernode/enclave/Cargo.toml (0 hunks)
- packages/ciphernode/enclave/src/main.rs (1 hunks)
- packages/ciphernode/enclave_node/Cargo.toml (0 hunks)
- packages/ciphernode/keyshare/src/keyshare.rs (4 hunks)
- packages/ciphernode/tests/Cargo.toml (0 hunks)
💤 Files with no reviewable changes (5)
- packages/ciphernode/Cargo.toml
- packages/ciphernode/data/Cargo.toml
- packages/ciphernode/enclave/Cargo.toml
- packages/ciphernode/enclave_node/Cargo.toml
- packages/ciphernode/tests/Cargo.toml
✅ Files skipped from review due to trivial changes (1)
- packages/ciphernode/data/src/data_store.rs
🔇 Additional comments (5)
packages/ciphernode/enclave/src/main.rs (2)
Line range hint 39-47: Verify keyshare cleanup implementation in command handlers.
Given that this PR aims to ensure keyshares are cleaned up both in memory and persistence, please verify that the command handlers (particularly in start::execute) properly implement this cleanup functionality.
Let's check the implementation:
#!/bin/bash
# Description: Verify keyshare cleanup implementation
# Test 1: Check for keyshare cleanup related code in start command
ast-grep --pattern 'fn execute($_, $_) {
$$$
clear_secret
$$$
}'
# Test 2: Look for E3RequestComplete event handling
rg -A 5 "E3RequestComplete"56-56: LGTM! Attribute updated correctly.
The change from #[actix_rt::main] to #[actix::main] aligns with the PR objective to remove the deprecated actix_rt dependency.
Let's verify the dependency change in Cargo.toml:
✅ Verification successful
Dependencies updated correctly for actix migration
The verification confirms:
actixdependency is properly added with workspace inheritanceactix-rtdependency has been removed- The attribute change from
#[actix_rt::main]to#[actix::main]is properly supported by the dependencies
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify actix dependency is properly configured
# Test 1: Check if actix_rt is removed from dependencies
rg -A 5 "actix-rt =" packages/ciphernode/enclave/Cargo.toml || echo "actix-rt dependency removed ✓"
# Test 2: Check if actix is added to dependencies
rg -A 5 "actix =" packages/ciphernode/enclave/Cargo.toml || echo "Please verify actix dependency is added"
Length of output: 8928
Script:
#!/bin/bash
# Let's check the dependencies in Cargo.toml correctly
rg "actix =" packages/ciphernode/enclave/Cargo.toml
rg "actix-rt =" packages/ciphernode/enclave/Cargo.toml
# Also check if the file exists
ls -l packages/ciphernode/enclave/Cargo.toml
Length of output: 273
packages/ciphernode/keyshare/src/keyshare.rs (3)
8-8: LGTM: Import addition is correct.
The E3RequestComplete import is properly added and aligns with its usage in the new handler implementation.
115-115: LGTM: Event handling is consistent.
The E3RequestComplete event handling follows the established pattern and correctly forwards the event data.
197-204: 🛠️ Refactor suggestion
Consider error handling and operation ordering.
While the implementation addresses the core requirement of cleaning up keyshare state, there are a few considerations:
- The
checkpoint()operation could fail silently. Consider handling potential errors to ensure state is properly persisted. - The actor stops immediately after checkpoint request, which might not guarantee state persistence.
Let's verify the checkpoint implementation:
Consider this safer implementation:
fn handle(&mut self, _: E3RequestComplete, ctx: &mut Self::Context) -> Self::Result {
self.clear_secret();
- self.checkpoint();
- ctx.notify(Die);
+ if let Err(e) = self.repository().store(&self.snapshot()).await {
+ warn!("Failed to persist keyshare cleanup state: {}", e);
+ }
+ // Ensure we only stop after state is handled
+ ctx.notify(Die);
}
ryardley
force-pushed
the
ry/152-keyshare-cleans-state
branch
from
5af1317 to
717e047
Compare
ryardley
changed the base branch from
ry/128-keymanagement-encryption
to
main
ryardley
changed the title
Closes: #152
Closes: #161
Currently when an E3 round has finished and we clean up keyshares in memory we should also nullify the persisted keyshare
This PR:
E3RequestComplete(When deleting keyshare ensure that persisted state is cleaned up #152)Summary by CodeRabbit
Summary by CodeRabbit
New Features
clear_secretin the Keyshare implementation for enhanced event handling.E3RequestCompleteevent.Bug Fixes
actix-rtdependency across multiple packages.Refactor
Chores
Cargo.tomlfiles by removing unnecessary dependencies.