feature(common): better serialization of cryptographic payloads #161
Conversation
WalkthroughThe pull request introduces several changes primarily focused on enhancing cryptographic capabilities within the 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 (8)
crates/common/src/serialization.rs (3)
10-36: Consider abstracting common serialization logic to adhere to DRY principleThe modules
raw_or_hex,raw_or_hex_fixed,raw_or_b64, andraw_or_b64_fixedcontain similarserializeanddeserializefunctions with minor differences in encoding (hexadecimal vs. Base64) and handling fixed-size arrays. This code duplication can be reduced by abstracting the common logic into generic functions or utilizing traits. This will improve maintainability and reduce the potential for errors.Example refactor suggestion:
pub mod raw_or_encoding { use serde::{self, Deserialize, Deserializer, Serializer}; pub enum Encoding { Hex, Base64, } pub fn serialize<S, T: AsRef<[u8]>>( bytes: T, serializer: S, encoding: Encoding, ) -> Result<S::Ok, S::Error> where S: Serializer, { if serializer.is_human_readable() { let encoded_str = match encoding { Encoding::Hex => hex::encode(bytes.as_ref()), Encoding::Base64 => base64::encode(bytes.as_ref()), }; serializer.serialize_str(&encoded_str) } else { serializer.serialize_bytes(bytes.as_ref()) } } pub fn deserialize<'de, D>( deserializer: D, encoding: Encoding, ) -> Result<Vec<u8>, D::Error> where D: Deserializer<'de>, { if deserializer.is_human_readable() { let encoded_str = String::deserialize(deserializer)?; let decoded = match encoding { Encoding::Hex => hex::decode(&encoded_str), Encoding::Base64 => base64::decode(&encoded_str), } .map_err(serde::de::Error::custom)?; Ok(decoded) } else { Vec::<u8>::deserialize(deserializer) } } pub fn serialize_fixed<S, const N: usize>( bytes: &[u8; N], serializer: S, encoding: Encoding, ) -> Result<S::Ok, S::Error> where S: Serializer, { serialize(bytes.as_slice(), serializer, encoding) } pub fn deserialize_fixed<'de, D, const N: usize>( deserializer: D, encoding: Encoding, ) -> Result<[u8; N], D::Error> where D: Deserializer<'de>, { let vec = deserialize(deserializer, encoding)?; let len = vec.len(); vec.try_into() .map_err(|_| serde::de::Error::custom(format!("Expected {} bytes, got {}", N, len))) } }You can then replace the individual modules with calls to this generalized module.
Also applies to: 38-59, 61-87, 90-111
31-31: Simplify decoding by passing strings directlyIn
raw_or_hex::deserializeandraw_or_b64::deserialize, you can pass the string slice directly to the decoding functions instead of callingas_bytes(). The decoding functions accept&stror&[u8], making theas_bytes()call unnecessary.Apply this diff to simplify the code:
--- a/crates/common/src/serialization.rs +++ b/crates/common/src/serialization.rs @@ -30,7 +30,7 @@ pub fn deserialize<'de, D>(deserializer: D) -> Result<Vec<u8>, D::Error> let hex_str = String::deserialize(deserializer)?; - hex::decode(hex_str.as_bytes()).map_err(serde::de::Error::custom) + hex::decode(&hex_str).map_err(serde::de::Error::custom) } else { Vec::<u8>::deserialize(deserializer) } @@ -82,7 +82,7 @@ pub fn deserialize<'de, D>(deserializer: D) -> Result<Vec<u8>, D::Error> let base64_string = String::deserialize(deserializer)?; - BASE64.decode(base64_string.as_bytes()).map_err(serde::de::Error::custom) + BASE64.decode(&base64_string).map_err(serde::de::Error::custom) } else { Vec::<u8>::deserialize(deserializer) }Also applies to: 83-83
Line range hint
403-509: Add unit tests for failure cases and edge conditionsThe test cases provided cover typical success scenarios. Consider adding tests for failure cases, such as invalid algorithm names, corrupted byte arrays, and mismatched key and signature types. This will enhance test coverage and ensure robustness against erroneous inputs.
crates/common/src/keys.rs (3)
Line range hint
28-71: Ensure consistent error messages and handlingThroughout the
Signature,VerifyingKey, andSigningKeyimplementations, error messages use phrases like "Unexpected algorithm for Signature" or "Invalid signature type". For better clarity and debugging, consider including the provided algorithm or type in the error message.Apply this diff to improve error messages:
- _ => bail!("Unexpected algorithm for Signature"), + _ => bail!("Unexpected algorithm '{}' for Signature", algorithm),Repeat similar changes for other error messages where applicable.
Also applies to: 91-153, 296-350
373-377: Simplify matching inPartialEqimplementationIn the
PartialEqimplementation forSigningKey, consider usingmatches!or directly comparing theto_bytes()output to simplify the code and enhance readability.Example refactor:
impl PartialEq for SigningKey { fn eq(&self, other: &Self) -> bool { - match (self, other) { - (SigningKey::Ed25519(a), SigningKey::Ed25519(b)) => a.as_bytes() == b.as_bytes(), - (SigningKey::Secp256k1(a), SigningKey::Secp256k1(b)) => a == b, - (SigningKey::Secp256r1(a), SigningKey::Secp256r1(b)) => a == b, - _ => false, - } + self.algorithm() == other.algorithm() && self.to_bytes() == other.to_bytes() } }
490-498: Handle potential panics in test casesIn the test cases
test_verifying_key_from_string_ed25519andtest_verifying_key_from_string_secp256k1, the use ofunwrap()can cause the test to panic if decoding fails unexpectedly. Consider usingexpect()to provide a clear error message or handle the error appropriately to improve test diagnostics.Also applies to: 503-511
crates/common/src/lib.rs (1)
6-6: Module declaration order for better code organizationConsider grouping related module declarations together or ordering them alphabetically for better readability and maintainability.
Example:
pub mod digest; pub mod hashchain; pub mod hasher; pub mod keys; +pub mod serialization; pub mod operation; pub mod transaction; pub mod tree;crates/common/src/test_utils.rs (1)
91-91: Update comments to reflect code changesIn the comments where
vk.as_bytes()was changed tovk.to_bytes(), update the comments if they mentionas_bytes()to prevent confusion.Also applies to: 224-224
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (10)
Cargo.toml(1 hunks)crates/common/Cargo.toml(1 hunks)crates/common/src/digest.rs(1 hunks)crates/common/src/keys.rs(8 hunks)crates/common/src/lib.rs(1 hunks)crates/common/src/serialization.rs(1 hunks)crates/common/src/test_utils.rs(2 hunks)crates/common/src/transaction_builder.rs(1 hunks)crates/common/src/tree.rs(1 hunks)crates/node_types/prover/src/webserver.rs(2 hunks)
🔇 Additional comments (10)
crates/common/Cargo.toml (1)
21-21: LGTM! Verify ECDSA dependency usage
The addition of the ECDSA dependency aligns with the PR's objective of improving cryptographic payload serialization.
Let's verify the ECDSA dependency usage in the codebase:
✅ Verification successful
ECDSA dependency is properly used in cryptographic operations
The ECDSA dependency is correctly utilized in crates/common/src/keys.rs for both secp256k1 and p256 curve implementations, specifically for digital signatures and key management. The imports show proper integration with the cryptographic functionality.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check usage of ECDSA types/functions in the codebase
# Expect: Usage of ECDSA types/functions in cryptographic serialization code
rg -l "use.*ecdsa::"
Length of output: 48
Script:
#!/bin/bash
# Check the actual usage of ECDSA in keys.rs
rg "ecdsa::" crates/common/src/keys.rs -B 2 -A 2
Length of output: 437
Cargo.toml (1)
103-103: LGTM! Verify ECDSA version security
The ECDSA dependency with DER feature is appropriate for standardized cryptographic encoding.
Let's check for any security advisories:
✅ Verification successful
ECDSA 0.16.0 dependency verified as secure
The GitHub Security Advisory Database shows no known vulnerabilities for the ECDSA crate. The version 0.16.0 with DER feature can be safely used.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for security advisories for ECDSA 0.16.0
# Expect: No critical security issues
gh api graphql -f query='
{
securityVulnerabilities(first: 5, ecosystem: RUST, package: "ecdsa") {
nodes {
advisory {
summary
severity
publishedAt
}
vulnerableVersionRange
firstPatchedVersion {
identifier
}
}
}
}'
Length of output: 339
crates/node_types/prover/src/webserver.rs (2)
113-122: LGTM! Improved server logging and error handling
Good improvements:
- Now logging the actual bound port instead of configured port
- Better error handling with proper context
- Cleaner separation of server binding and serving logic
39-39: Consider keeping dynamic port allocation for testing
Changing from port 0 (dynamic) to a fixed port (50524) might impact test parallelization and deployment flexibility. Consider making this configurable or keeping dynamic allocation for tests.
Let's check if this port is used in tests:
✅ Verification successful
Fixed port is acceptable as it's not used in tests
After reviewing the codebase, the change from dynamic port to fixed port 50524 appears safe because:
- The port is only used as a default in the WebServerConfig
- Tests don't rely on this port number - they focus on the prover logic without testing the web server
- The port can still be configured through WebServerConfig if needed
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check usage of port 50524 in test files
# Expect: No hardcoded usage in tests to avoid conflicts
rg -l "50524" | grep -i "test"
Length of output: 28
Script:
#!/bin/bash
# Let's check for test files that might be using any port configuration
rg -A 5 -B 5 "port.*=|=.*port" --type rust | grep -i "test"
# Also check for any test configurations or test setup files
fd -e rs -e toml | xargs rg "test.*port|port.*test" -l
# Check the specific file's test module if it exists
rg -A 5 "mod test" crates/node_types/prover/src/webserver.rs
Length of output: 179
Script:
#!/bin/bash
# Let's check the webserver.rs file content to understand the context better
cat crates/node_types/prover/src/webserver.rs
# Also check for any integration tests that might be using this server
fd -e rs -g "*test*.rs" -g "*integration*.rs" crates/node_types/prover/
Length of output: 6667
Script:
#!/bin/bash
# Let's check the test file to see if it uses the webserver
cat crates/node_types/prover/src/prover/tests.rs
# Also check for any configuration files that might override the port
fd -e toml -e yaml -e json | xargs rg "port.*50524|50524.*port"
Length of output: 7923
crates/common/src/serialization.rs (2)
3-8: Ensure security considerations for sensitive data serialization
When serializing cryptographic payloads, it's important to ensure that sensitive data is handled securely. Consider whether the bytes field in CryptoPayload might contain sensitive key material or data that should be protected. If so, ensure that appropriate measures are taken to prevent accidental exposure, such as zeroizing memory after use or encrypting sensitive fields.
Line range hint 113-122: Consider implementing Hash and Eq traits consistently
In the VerifyingKey implementation, the Hash trait relies on self.to_bytes() for hashing. Ensure that the Eq and PartialEq implementations are consistent with the Hash implementation to maintain hash map invariants.
Run the following script to check consistency of Eq and Hash implementations:
Also applies to: 291-292
crates/common/src/keys.rs (1)
81-88: Implement serde attributes consistently across enums
Ensure that the Signature, VerifyingKey, and SigningKey enums consistently derive Serialize and Deserialize or provide appropriate custom implementations. Currently, Signature and VerifyingKey derive these traits, but SigningKey does not. This inconsistency might lead to serialization issues when using SigningKey.
Also applies to: 188-201, 381-396
crates/common/src/digest.rs (1)
6-6: Verify that Digest serialization aligns with expectations
The addition of #[serde(with = "raw_or_hex_fixed")] for the Digest struct's internal array alters its serialization behavior. Ensure that this change is compatible with any existing serialized data formats and that it does not break deserialization of previously serialized Digest instances.
crates/common/src/tree.rs (1)
279-279: LGTM: Improved cryptographic key serialization
The change from as_bytes() to to_bytes() aligns with the PR's objective of better cryptographic payload serialization. This change ensures consistent byte representation across the codebase.
Let's verify the consistent usage of to_bytes() across the codebase:
✅ Verification successful
Consistent byte serialization methods across cryptographic contexts
The codebase shows a clear pattern in the usage of these methods:
as_bytes()is primarily used for string-like types and general data serialization (e.g., with Redis, RocksDB)to_bytes()is consistently used for cryptographic types (keys, signatures, hashes)
The change in the PR aligns with this pattern, as it switches to to_bytes() for a cryptographic key serialization.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining instances of as_bytes() used with cryptographic keys
rg "as_bytes\(\)" --type rust
# Search for the new to_bytes() usage pattern
rg "to_bytes\(\)" --type rust
Length of output: 5299
crates/common/src/transaction_builder.rs (1)
136-136: Verify consistent key serialization across the codebase.
The change from as_bytes() to to_bytes() aligns with the broader refactor for better cryptographic payload serialization. However, since this affects the hash computation for account creation credentials, we need to ensure:
- Consistent serialization across all key operations
- Compatibility with existing signatures
Let's verify the consistent use of the new serialization method:
✅ Verification successful
The key serialization change is consistent across the codebase
The search results confirm that the change from as_bytes() to to_bytes() for key serialization is consistently applied across the codebase:
- The
to_bytes()method is used consistently for all cryptographic key serialization in hash computations, particularly in:- Account creation in
transaction_builder.rs - Test utilities in
test_utils.rs - Key directory tree operations in
tree.rs
- Account creation in
- The remaining
as_bytes()calls are appropriately used for non-key data like IDs and strings - The implementation in
keys.rsshows proper key serialization methods for all supported key types (Ed25519, Secp256k1, Secp256r1)
The change is part of a well-implemented cryptographic serialization refactor that maintains consistency across all key operations.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining as_bytes() calls on cryptographic keys
# and verify consistent use of to_bytes()
# Check for any remaining as_bytes() calls on VerifyingKey
rg "VerifyingKey.*as_bytes\(\)"
# Check for consistent use of to_bytes()
rg "VerifyingKey.*to_bytes\(\)"
# Look for potential key serialization in tests
rg -A 3 "create_account.*SigningKey"
Length of output: 197
Script:
#!/bin/bash
# Let's broaden our search to understand the key serialization patterns
# Check for any as_bytes() usage in the codebase
rg "as_bytes\(\)" -A 2
# Check for to_bytes() usage in the codebase
rg "to_bytes\(\)" -A 2
# Look for key-related type definitions and implementations
ast-grep --pattern 'impl $_ for $key where $key: $_'
# Check for hash computation patterns
rg "Digest::hash" -A 2
Length of output: 16091
"as_" prefix should be used when nothing needs to be allocated, which is not the case here.
Before it was logging the configured port, which could be 0 (to take an arbitrary free one) and devs wouldn't be able to see the port chosen by axum.
Setting it to something fixed, so external tools (e.g. API testing tool) can be configured accordingly.
jns-ps
force-pushed
the
serialization-overhaul
branch
from
3a8f87d to
54a2bd2
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (5)
crates/common/src/serde.rs (2)
83-83: SimplifyBASE64.decodeinputWhile
BASE64.decodeaccepts&[u8], passing a&strcan improve readability sincebase64_stringis already aString.Apply this diff to simplify the code:
- BASE64.decode(base64_string.as_bytes()).map_err(serde::de::Error::custom) + BASE64.decode(&base64_string).map_err(serde::de::Error::custom)
10-35: Refactor to reduce code duplicationThe
raw_or_hexandraw_or_b64modules have similarserializeanddeserializefunctions with minor differences in encoding. Consider abstracting the common logic to a generic function or trait to eliminate duplication and improve maintainability.Also applies to: 61-88
crates/common/src/keys.rs (3)
370-379: ImplementEqtrait forSigningKeySince
SigningKeyimplementsPartialEq, it's advisable to also implementEqfor completeness and to meet Rust's equality trait requirements.Implement
EqforSigningKey:impl Eq for SigningKey {}
244-246: SimplifyFrom<SigningKey>implementationIn the
From<SigningKey>implementation forVerifyingKey, you can streamline the match arms by directly calling.into()without dereferencing.Apply this diff to simplify the code:
- match sk { - SigningKey::Ed25519(sk) => (*sk).into(), + match sk { + SigningKey::Ed25519(sk) => sk.as_ref().into(),
403-485: Expand test coverage for error casesThe tests cover successful parsing and equality checks. Consider adding tests for error cases, such as invalid algorithms or corrupted key bytes, to ensure robust error handling.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (11)
Cargo.toml(1 hunks)crates/common/Cargo.toml(1 hunks)crates/common/src/digest.rs(1 hunks)crates/common/src/keys.rs(8 hunks)crates/common/src/lib.rs(1 hunks)crates/common/src/operation.rs(2 hunks)crates/common/src/serde.rs(1 hunks)crates/common/src/test_utils.rs(2 hunks)crates/common/src/transaction_builder.rs(1 hunks)crates/common/src/tree.rs(1 hunks)crates/node_types/prover/src/webserver.rs(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (8)
- crates/common/src/lib.rs
- crates/common/Cargo.toml
- crates/common/src/transaction_builder.rs
- crates/common/src/test_utils.rs
- Cargo.toml
- crates/node_types/prover/src/webserver.rs
- crates/common/src/digest.rs
- crates/common/src/tree.rs
🔇 Additional comments (2)
crates/common/src/serde.rs (1)
31-31:
Incorrect use of hex::decode function
The hex::decode function expects a &str, but hex_str.as_bytes() provides a &[u8]. This may cause a type mismatch or unexpected behavior.
Apply this diff to fix the issue:
- hex::decode(hex_str.as_bytes()).map_err(serde::de::Error::custom)
+ hex::decode(&hex_str).map_err(serde::de::Error::custom)Likely invalid or redundant comment.
crates/common/src/operation.rs (1)
30-30: Verify the impact of serialization change on AddData
Adding #[serde(with = "raw_or_b64")] changes the serialization format of the data field to use Base64 encoding when human-readable. Please ensure this change is backward-compatible and won't break existing clients or stored data relying on the previous format.
jns-ps
changed the title
Summary by CodeRabbit
Release Notes
New Features
ecdsadependency.Bug Fixes
Documentation
These updates enhance the security and functionality of the application, particularly in cryptographic operations and data handling.