fix: avoid double hashing JMT keys #170
Conversation
WalkthroughThe changes in this pull request primarily focus on modifying how Changes
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 (
|
![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 (2)
crates/common/src/test_utils.rs (2)
63-63: Add test cases for edge-case IDs.With the change to use raw IDs for key generation, ensure test coverage for:
- IDs with special characters
- Very long IDs
- Unicode IDs
- Similar looking IDs (e.g., with different Unicode normalizations)
Consider adding test helper methods for generating various types of test IDs:
impl TestTreeState { pub fn test_with_special_chars_id(&mut self) -> String { "test!@#$%^&*()_+".to_string() } pub fn test_with_unicode_id(&mut self) -> String { "test_🦀_κόσμε".to_string() } }Also applies to: 97-97
219-219: Enhance random ID generation for better test coverage.The current random string generation uses only alphanumeric characters. With the switch to raw IDs, consider expanding the character set to include special characters and Unicode for more comprehensive testing.
Consider replacing the current random string generation with a more comprehensive approach:
-let random_string: String = (0..10) - .map(|_| rng.sample(rand::distributions::Alphanumeric) as char) - .collect(); +let random_string = generate_test_id(rng, TestIdType::Complex);Also applies to: 247-247
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
crates/common/src/test_utils.rs(4 hunks)crates/common/src/transaction_builder.rs(3 hunks)crates/common/src/tree/snarkable_tree.rs(4 hunks)crates/node_types/prover/src/prover/mod.rs(1 hunks)
🔇 Additional comments (3)
crates/common/src/tree/snarkable_tree.rs (1)
36-36: Verify the security implications of using raw IDs for key generation.
The change from using hashed IDs to raw IDs for KeyHash generation could have security implications:
- Key Predictability: Using raw IDs makes keys more predictable
- Key Uniqueness: Collision resistance now depends on ID uniqueness rather than hash properties
- Key Length: Raw IDs might be longer than their hash equivalents
Also applies to: 54-54, 64-64, 101-101
✅ Verification successful
Let me gather more specific information about the key hash implementation and its usage.
Let me gather more specific information about the KeyHash implementation and transaction ID generation.
Let me gather more information about the transaction ID validation and hash generation.
Based on the gathered information, I can now provide a final response regarding the security implications of using raw IDs for key generation.
Using raw transaction IDs for key generation is secure in this implementation
The implementation uses the KeyHash::with::<Hasher> function to generate key hashes from transaction IDs, which provides several security guarantees:
- The IDs are hashed before being used as keys, preventing direct exposure of raw IDs
- The hash function is cryptographically secure through the generic
Hashertype parameter - The key generation is part of a larger append-only hashchain system that maintains integrity through cryptographic commitments
- The system uses Jellyfish Merkle Trees with versioning, providing additional security through state verification
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for potential ID collisions in the codebase
# Look for patterns where IDs might conflict
# Search for ID generation/validation patterns
rg -A 3 'id: String|id: &str|id\.to_string\(\)'
# Search for any existing ID validation or sanitization
ast-grep --pattern 'fn $name($params) {
$$$
ensure!($condition, $message)
$$$
}'
Length of output: 13447
Script:
#!/bin/bash
# Check KeyHash implementation and usage
rg -A 5 "impl.*KeyHash"
# Check transaction ID generation/validation
rg -A 5 "transaction\.id"
# Look for any hash-related security measures
ast-grep --pattern 'struct KeyHash {
$$$
}'
# Check for any validation or sanitization of transaction IDs
ast-grep --pattern 'fn $name($params) -> $ret {
$$$
hash($expr)
$$$
}'
Length of output: 131070
Script:
#!/bin/bash
# Check KeyHash implementation and usage
ast-grep --pattern 'impl KeyHash {
$$$
}'
# Look for transaction ID generation/validation
ast-grep --pattern 'fn $name($params) -> $ret {
$$$
transaction.id
$$$
}'
# Check for any hash-related security measures on transaction IDs
rg -A 3 "with::<Hasher>\(&"
# Look for any transaction ID validation
rg -A 3 "validate.*id|verify.*id"
Length of output: 103693
Script:
#!/bin/bash
# Check for transaction ID validation in hashchain
rg -A 5 "fn create_account|fn register_service" crates/common/src/hashchain.rs
# Look for KeyHash implementation details
rg -A 5 "impl KeyHash" crates/common/src/
# Check for any transaction ID format requirements or validation
rg -A 3 "transaction.*id.*format|validate.*transaction.*id"
# Look for any hash-related security measures in the codebase
rg -A 3 "hash.*security|secure.*hash"
Length of output: 87091
crates/common/src/transaction_builder.rs (1)
193-193: 🛠️ Refactor suggestion
Enhance error handling for invalid IDs.
With the change to use raw IDs for key generation, the error handling should be enhanced to handle invalid ID cases consistently across all methods.
Consider adding ID validation before key generation:
+fn validate_id(id: &str) -> Result<(), anyhow::Error> {
+ if id.is_empty() {
+ bail!("ID cannot be empty");
+ }
+ // Add more validation as needed
+ Ok(())
+}
pub fn add_key(
&mut self,
id: &str,
key: VerifyingKey,
signing_key: &SigningKey,
key_idx: usize,
) -> UncommittedTransaction {
+ validate_id(id).expect("Invalid ID");
let key_hash = KeyHash::with::<Hasher>(id);Also applies to: 230-230, 307-307
crates/node_types/prover/src/prover/mod.rs (1)
462-462: LGTM! Change aligns with avoiding double hashing of JMT keys.
The modification correctly implements the PR's objective by directly using the id string to create the KeyHash instead of performing an intermediate hashing step. This change is consistent with similar modifications across other components mentioned in the AI summary.
Let's verify the consistency of this change with other components that interact with JMT keys:
✅ Verification successful
Verified: Change correctly implements direct key hashing
The verification confirms that:
- All
KeyHash::withusages across the codebase directly use the identifier without pre-hashing - The remaining
Digest::hashoperations with IDs are used for different purposes (signatures, challenges) and not for key generation - The change maintains consistency with similar usage patterns in other components (
transaction_builder.rs,snarkable_tree.rs,test_utils.rs)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the consistent usage of KeyHash::with across the codebase
# Expected: All KeyHash::with calls should directly use the identifier without pre-hashing
# Search for KeyHash::with usage patterns
rg "KeyHash::with" -A 3
# Search for any remaining Digest::hash calls that might be used with KeyHash
rg "Digest::hash.*KeyHash::with" -A 3
Length of output: 4123
Script:
#!/bin/bash
# Let's verify if there are any direct usages of Digest::hash with strings that might be related to key hashing
rg "Digest::hash.*id" -A 3
# Also check for any KeyHash related code to ensure we're not missing any implementation details
ast-grep --pattern 'impl $_ for KeyHash { $$ }'
# And check the KeyHash struct definition
ast-grep --pattern 'struct KeyHash { $$ }'
Length of output: 1272
Summary by CodeRabbit
get_hashchainmethod to improve how hash chains are retrieved, enhancing its interaction with the underlying data structure.get_hashchainin theProverstruct to reflect changes in key hash generation.