feat(common) Allow adding signed data #141
Conversation
9e29688 to
84ffa15
Compare
WalkthroughThe pull request introduces several modifications to the Changes
Possibly related PRs
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 (
|
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (5)
crates/common/src/test_utils.rs (1)
127-144: LGTM with suggestions for improvementThe new
add_signed_data_to_accountmethod is well-structured and consistent with other methods in theTestTreeStatestruct. However, there are a few suggestions for improvement:
Error Handling: Consider replacing
unwrap()calls with the?operator to propagate errors instead of potentially panicking. This would make the function more robust and easier to debug.Documentation: Add a documentation comment explaining the purpose and usage of this method.
Here's a suggested implementation with improved error handling and documentation:
/// Adds signed data to the specified account's hashchain. /// /// # Arguments /// * `data` - The data to be signed and added /// * `account` - The account to which the signed data will be added /// /// # Returns /// * `Ok(())` if the operation was successful /// * `Err` if there was an issue retrieving the signing key or performing the operation pub fn add_signed_data_to_account( &mut self, data: &[u8], account: &mut TestAccount, ) -> Result<()> { let signing_key = self.signing_keys.get(&account.hashchain.id) .ok_or_else(|| anyhow!("Signing key not found for account"))?; let signature = signing_key.sign(data); let op = Operation::new_add_signed_data( account.hashchain.id.clone(), data.to_vec(), signature, 0, )?; account.hashchain.perform_operation(op)?; Ok(()) }This implementation improves error handling and adds a helpful documentation comment.
crates/common/src/hashchain.rs (1)
134-134: Clarify handling ofAddSignedDatain valid keys construction.In the loop within
verify_last_entry, theOperation::AddSignedData(_)variant is matched but left with an empty block. IfAddSignedDataoperations do not influence the set of valid keys, consider adding a comment to explicitly state that no action is required. This enhances code readability and helps future maintainers understand the control flow.crates/common/src/operation.rs (3)
Line range hint
320-331: Missing Validation for EmptyvalueinAddSignedDataThe
validatemethod checks for emptyidandsignature.signaturefields but does not validate ifargs.valueis empty. An emptyvaluemight not be meaningful in this context.Consider adding a validation check for
args.valueto ensure it's not empty.Apply this diff to add the validation:
if signature.signature.is_empty() { return Err(GeneralError::MissingArgumentError( "signature is empty".to_string(), ) .into()); + } + + if args.value.is_empty() { + return Err(GeneralError::MissingArgumentError( + "value is empty".to_string(), + ) + .into()); }
86-95: Improve Documentation forAddSignedDataArgsCurrently, each field within
AddSignedDataArgsis documented, but the struct itself lacks a comprehensive description.Consider adding a struct-level comment to explain the purpose of
AddSignedDataArgsand how it should be used.Apply this diff to enhance the documentation:
#[derive(Clone, Serialize, Deserialize, Debug, PartialEq)] +/// Arguments for adding signed data to an account. pub struct AddSignedDataArgs {
218-218: Clarify Return Value inget_public_keyIn the
get_public_keymethod, theAddSignedDatavariant returnsNone:Operation::RegisterService(_) | Operation::AddSignedData(_) => None,If
AddSignedDataoperations may involve a public key, consider whether it would be appropriate to return it here. Alternatively, update the documentation to clarify whyNoneis returned for this variant.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
- crates/common/src/hashchain.rs (6 hunks)
- crates/common/src/operation.rs (9 hunks)
- crates/common/src/test_utils.rs (1 hunks)
- crates/common/src/tree.rs (3 hunks)
- crates/node_types/prover/src/prover.rs (1 hunks)
🧰 Additional context used
🔇 Additional comments (8)
crates/common/src/hashchain.rs (3)
233-249:verify_signature_at_key_idxmethod correctly encapsulates signature verification.The new
verify_signature_at_key_idxmethod enhances modularity by encapsulating the signature verification logic. It also provides detailed error messages, including the invalid key and the set of valid keys, which aids in debugging.
293-296: Ensure proper key index handling invalidate_new_operation.In
validate_new_operation, the inclusion ofOperation::AddSignedDatain the signature verification block is appropriate. Verify that thekey_idxprovided insignature.key_idxcorrectly corresponds to the intended key in the hashchain, and that key revocation status is accurately checked.You can run the following script to confirm that
key_idxvalues are used consistently:
175-181: Verify the message used for signature verification inAddSignedData.When handling
Operation::AddSignedData(args)inverify_last_entry, the message used for signature verification isargs.value. Ensure that this is the intended data for signature verification. If the signature should cover the entire operation or a specific serialization, consider adjusting the message accordingly for consistency with other operations.Run the following script to check where
AddSignedDatais used and howargs.valueis constructed:✅ Verification successful
Verified:
args.valueis correctly used for signature verification inAddSignedData.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Find all usages of AddSignedData and inspect args.value # Search for AddSignedData usages and display surrounding context rg --type rust -A 5 -B 2 'Operation::AddSignedData\('Length of output: 7892
Script:
#!/bin/bash # Description: Locate the definition of AddSignedDataArgs and inspect assignments to `value` # Find the definition of AddSignedDataArgs rg --type rust 'struct AddSignedDataArgs' -A 10 # Find all assignments to `value` within AddSignedDataArgs rg --type rust 'AddSignedDataArgs.*value\s*='Length of output: 777
Script:
#!/bin/bash # Description: Find all instantiations of AddSignedDataArgs and inspect the value field # Find all instances where AddSignedDataArgs is instantiated rg --type rust 'AddSignedDataArgs\s*{' -A 5 # Optionally, display the lines where AddSignedDataArgs is constructed with their surrounding context rg --type rust 'AddSignedDataArgs\s*{' -A 5 -B 2Length of output: 308
Script:
#!/bin/bash # Description: Locate all instantiations of AddSignedDataArgs and inspect the value field # Corrected regex to find struct instantiations with escaped { rg --type rust 'AddSignedDataArgs\s*\{' -A 5 -B 2 # Additionally, search for builder patterns or other instantiation methods if applicable rg --type rust 'AddSignedDataArgs::new\s*\(' -A 3 -B 2Length of output: 3680
crates/common/src/operation.rs (2)
276-283: Consistent Handling of Signatures inwithout_signatureMethodIn the
without_signaturemethod, forAddSignedData, thesignaturefield is replaced with an empty signature but retains thekey_idx:signature: SignatureBundle { key_idx: args.signature.key_idx, signature: Vec::new(), },Ensure that this approach is consistent with other operations and that retaining
key_idxwithout the signature does not introduce any unintended side effects.
192-203:⚠️ Potential issueMissing Signature Generation in
new_add_signed_dataThe
new_add_signed_datamethod constructs anAddSignedDataoperation but does not generate a signature forargs.value. This means thesignaturefield must be provided externally, which might not be the intended behavior.Consider generating the signature within this method to encapsulate the operation fully and prevent misuse.
Apply this diff to generate the signature within the method:
pub fn new_add_signed_data( id: String, value: Vec<u8>, - signature: Vec<u8>, key_idx: usize, ) -> Result<Self> { + let signature = signing_key.sign(&value).to_vec(); Ok(Operation::AddSignedData(AddSignedDataArgs { id, value, signature: SignatureBundle { key_idx, signature }, })) }Likely invalid or redundant comment.
crates/common/src/tree.rs (3)
19-20: ImportingAddSignedDataArgsinto scopeThe addition of
AddSignedDataArgsto the imports from theoperationmodule ensures that the necessary structures are available for handling the newAddSignedDataoperation.
378-379: Confirm consistent handling ofAddSignedDatainprocess_operationThe code adds
Operation::AddSignedData(AddSignedDataArgs { id, .. })to the match arms processed similarly toAddKeyandRevokeKey. Ensure thatAddSignedDatarequires the same processing steps as the other operations, and that all necessary validation and updates to the hashchain are correctly implemented for this operation.
714-716: Test correctly covers addition of signed data to accountThe test
test_multiple_inserts_and_updatesnow includes adding signed data toaccount2usingadd_signed_data_to_account. This expands the test coverage to include the new functionality and verifies that accounts can handle signed data correctly.
When solely relying on an external signature, this leaves prism vulnerable to replay attacks. As a result, ops to add data are always signed with a valid chain key and can optionally have an additional signature for the data to be added.
84ffa15 to
c253341
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (5)
crates/common/src/test_utils.rs (2)
135-142: LGTM with a suggestion: Consider parameterizing the signing key.The method provides a clear interface for adding signed data to an account. However, creating a new random signing key for each call might not be suitable for all test scenarios. Consider allowing the caller to optionally provide a signing key, falling back to a random key if not provided. This would increase the flexibility of the method for various test cases.
Example:
pub fn add_signed_data_to_account( &mut self, data: &[u8], account: &mut TestAccount, signing_key: Option<&SigningKey>, ) -> Result<()> { let signing_key = signing_key.cloned().unwrap_or_else(create_mock_signing_key); self.add_data_to_account(data, account, Some(&signing_key)) }
144-167: LGTM with a minor suggestion: Improve error handling.The implementation correctly handles both signed and unsigned data cases and properly creates and executes the operation on the hashchain. However, there's room for improvement in error handling:
- The
unwrap()call on line 165 could be replaced with proper error handling to provide more context if an error occurs.Consider updating the code as follows:
account.hashchain.perform_operation(op) .map_err(|e| anyhow!("Failed to perform operation on hashchain: {}", e))?;This change would provide more context in case of an error and propagate it up the call stack, allowing for better error handling and debugging in test scenarios.
crates/node_types/prover/src/prover/mod.rs (1)
459-459: LGTM! Consider adding a comment for clarity.The addition of
Operation::AddData(_)to this match arm is correct and consistent with the existing logic. It ensures thatAddDataoperations are validated against the existing hashchain, including signature checks, similar toAddKeyandRevokeKeyoperations.Consider adding a brief comment explaining why these specific operations require validation against the existing hashchain. This would improve code readability and maintainability. For example:
// These operations modify existing accounts and require validation against the current hashchain Operation::AddKey(_) | Operation::RevokeKey(_) | Operation::AddData(_) => { // ... (existing code) }crates/common/src/tree.rs (2)
725-735: LGTM: Comprehensive test coverage for new data operationsThe additions to the
test_multiple_inserts_and_updatestest case are well-structured and cover both unsigned and signed data operations. This ensures good test coverage for the new functionality.Consider adding assertions to verify the content of the added data in the final state of
account2.hashchain. This would provide an extra layer of validation for the new operations.
390-391: Enhance AddData implementation and provide more contextThe addition of the
AddDataoperation is a good start, but there are a few areas that could use clarification or enhancement:
- Could you provide more context on how the data is actually added to the hashchain? This implementation detail is not visible in the current file.
- Consider adding a clear distinction between signed and unsigned data in the main processing logic, not just in the tests.
- Implement or clarify the verification process for added data, especially for signed data.
- Update the documentation to reflect these new operations and their specific behaviors.
These enhancements will make the implementation more robust and easier to understand for other developers.
Also applies to: 725-735
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
- crates/common/src/hashchain.rs (7 hunks)
- crates/common/src/operation.rs (10 hunks)
- crates/common/src/test_utils.rs (2 hunks)
- crates/common/src/tree.rs (3 hunks)
- crates/node_types/prover/src/prover/mod.rs (1 hunks)
🧰 Additional context used
📓 Learnings (1)
crates/common/src/operation.rs (1)
Learnt from: distractedm1nd PR: deltadevsde/prism#141 File: crates/common/src/operation.rs:311-313 Timestamp: 2024-10-16T12:03:50.680Z Learning: When implementing `AddSignedData` in `crates/common/src/operation.rs`, ensure that signatures cover the entire operation (including metadata like account ID) to prevent replay attacks or unauthorized data modifications. Including a `value_signature: Option<(VerifyingKey, Signature)>` allows for externally signed data while ensuring proper validation.
🔇 Additional comments (14)
crates/common/src/test_utils.rs (2)
127-133: LGTM: Clear and concise implementation for adding unsigned data.The method provides a straightforward public interface for adding unsigned data to an account. It correctly delegates the implementation to the private helper method
add_data_to_account.
126-167: Overall assessment: Well-implemented feature addition with minor improvement opportunities.The new methods
add_unsigned_data_to_account,add_signed_data_to_account, and the private helperadd_data_to_accountare well-implemented and enhance the test utilities by adding support for signed and unsigned data operations. These additions align well with the existing codebase and follow similar patterns.Key points:
- The public methods provide clear interfaces for adding signed and unsigned data.
- The private helper method correctly handles both cases and interacts properly with the hashchain.
- There are opportunities for minor improvements in flexibility (parameterizing the signing key) and error handling.
These changes significantly expand the testing capabilities of the
TestTreeStatestruct, allowing for more comprehensive testing of data addition operations in the key directory tree implementation.crates/common/src/operation.rs (6)
16-20: LGTM: New operation type added and comment improvedThe addition of the
AddDatavariant to theOperationenum aligns with the PR objective of allowing signed data to be added. The updated comment forAddKeyalso improves clarity.
45-52: LGTM: New SignatureBundle struct addedThe new
SignatureBundlestruct is well-designed for handling external signatures, which aligns with the PR objective of allowing signed data to be added.
95-106: LGTM: AddDataArgs struct added with proper signature coverageThe new
AddDataArgsstruct is well-designed for theAddDataoperation. The inclusion of bothvalue_signatureandop_signatureallows for proper signature coverage, preventing replay attacks and unauthorized data modifications as discussed in the learnings.
203-229: LGTM: new_add_signed_data method addedThe
new_add_signed_datamethod is well-implemented and consistent with the PR objective. It properly handles both the operation signature and the optional value signature, aligning with the security considerations discussed in the learnings.
329-345: LGTM: Proper signature verification for AddData operationThe
verify_user_signaturemethod has been updated to handle the newAddDataoperation correctly. It verifies both the operation signature and the optional value signature, which aligns with the security considerations mentioned in the learnings. The handling of cases where no value signature is provided is also appropriate.
29-31:⚠️ Potential issueConsider using a fixed-size integer for
key_idxThe change from
u64tousizeforkey_idxmight introduce portability issues across different platforms (32-bit vs 64-bit). Consider using a fixed-size integer type likeu64to ensure consistent behavior across all platforms.pub struct HashchainSignatureBundle { /// Index of the key in the hashchain - pub key_idx: usize, + pub key_idx: u64,Likely invalid or redundant comment.
crates/common/src/tree.rs (2)
19-20: LGTM: New imports and Operation enum updateThe addition of
AddDataArgsimport and the inclusion ofAddDatain theOperationenum are consistent with the new functionality for adding data. These changes align well with the described modifications in the AI-generated summary.
390-391: Verify identical handling for different operationsThe inclusion of
Operation::AddDatain the match arm is correct and aligns with the new functionality. However, the identical handling forAddKey,RevokeKey, andAddDataoperations raises a question:Are there any operation-specific checks or validations that should be performed for
AddDatathat differ fromAddKeyorRevokeKey?Please review if any additional checks or different logic should be implemented for the
AddDataoperation.crates/common/src/hashchain.rs (4)
240-258: Well-structured extraction of signature verification logicThe new
verify_signature_at_key_idxmethod cleanly encapsulates the signature verification process, enhancing code maintainability and readability by avoiding duplication.
302-308: Consistent pattern matching for signature extractionUsing pattern matching to extract the
signaturefromAddKey,RevokeKey, andAddDataoperations improves code clarity and reduces redundancy.
134-134: Confirm necessity of handlingAddDatain prior entriesIn the loop over previous entries within
verify_last_entry, theOperation::AddDatacase currently performs no action:Operation::AddData(_) => {}Please confirm that no additional validation or state updates are required for
AddDataoperations in prior entries.
213-215: 🛠️ Refactor suggestionAvoid unnecessary cloning in
get_valid_keysmethodCloning
self.entriescreates an unnecessary copy of the data, which could impact performance. Consider iterating over references instead:- for entry in self.entries.clone() { + for entry in &self.entries {This change eliminates the need to clone the entire entries vector.
Likely invalid or redundant comment.
Introduces a new operation type,
AddData, allowing users to add arbitrary data to accounts. The added data can optionally be signed as well.Summary by CodeRabbit
New Features
AddData, allowing users to add arbitrary signed data to accounts.Bug Fixes
AddDataoperation to ensure data integrity.Documentation
AddDataoperation, ensuring functionality remains intact.