About using existing cryptography libraries in keytstore

[syjn99]

I'd like to discuss about our own implementation of crypgoraphic primitives using in ream-keystore package. Although I believe that our implementation is correct and good to read (double-checked with the specification), I would like to suggest to use the existing libraries for these implementation.

Rust Crypto team provides the most of the primitives we need to build a validator client, inclduing HMAC, PBKDF2, and scrypt. For example, hmac.rs can be simply replaced like the following diff (this passes all unit tests):

diff --git a/crates/crypto/keystore/src/hmac.rs b/crates/crypto/keystore/src/hmac.rs
index d7cdbcd..9625a3c 100644
--- a/crates/crypto/keystore/src/hmac.rs
+++ b/crates/crypto/keystore/src/hmac.rs
@@ -1,44 +1,14 @@
-use alloy_primitives::{B256, B512};
-use sha2::{Digest, Sha256, digest::crypto_common::BlockSizeUser};
+use alloy_primitives::B256;
+use hmac::{Hmac, Mac};
+use sha2::Sha256;
 
-// Going off of this
-// https://en.wikipedia.org/wiki/HMAC#:~:text=In%20cryptography%2C%20an%20HMAC%20(sometimes,and%20a%20secret%20cryptographic%20key.
 pub fn hmac_sha_256(key: &[u8], message: &[u8]) -> B256 {
-    let block_sized_key = compute_block_sized_key_sha_256(key);
+    // HMAC-SHA256 can handle keys of any length, so this should never fail.
+    let mut mac = Hmac::<Sha256>::new_from_slice(key).expect("Could not derive HMAC");
+    mac.update(message);
+    let result = mac.finalize().into_bytes();
 
-    let outer_padded_key = block_sized_key
-        .iter()
-        .map(|&b| b ^ 0x5c)
-        .collect::<Vec<_>>();
-    let inner_padded_key = block_sized_key
-        .iter()
-        .map(|&b| b ^ 0x36)
-        .collect::<Vec<_>>();
-
-    // Compute inner hash
-    let mut inner_hasher = Sha256::new();
-    inner_hasher.update(&inner_padded_key);
-    inner_hasher.update(message);
-    let inner_hash = inner_hasher.finalize();
-
-    // Compute outer hash
-    let mut outer_hasher = Sha256::new();
-    outer_hasher.update(&outer_padded_key);
-    outer_hasher.update(inner_hash);
-
-    B256::from_slice(&outer_hasher.finalize())
-}
-
-fn compute_block_sized_key_sha_256(key: &[u8]) -> B512 {
-    let block_size = Sha256::block_size();
-    if key.len() > block_size {
-        let mut hasher = Sha256::new();
-        hasher.update(key);
-        return B512::from_slice(&hasher.finalize());
-    }
-    let mut padded_key = vec![0u8; block_size];
-    padded_key[..key.len()].copy_from_slice(key);
-    B512::from_slice(&padded_key)
+    B256::new(result.into())
 }
 
 #[cfg(test)]

Also, as HMAC is just a building block for PBKDF2 and scrypt, we can just delete hmac.rs file and use Rust Crypto's implementation by importing it.

There are many pros and some con using external libraries:

Pros

1. Unless there's a clear reason to re-implement cryptographic functions, using the existing and also well-verified code is the rule of thumbs. I think the outer implementation provides more security and performance in terms of its widely usage.
2. We don't need an abstraction for this. In the past, we needed a fine-grained layer for abstraction for the case of BLS cryptography.
3. We can significantly debloat our codebase, including some tests with public test vectors.

Con

1. Managing dependency: I think this couldn't be a big problem because Rust Crypto's implementation is quite well modularized.

---

@Patchoulis @KolbyML
If we all are on the same line for this discussion, I'd love to open a new issue and work on this issue. (I mentioned relevant contributors but any feedbacks from anyone are welcomed!)

[Patchoulis]

We could use the HMAC implementation, but I did a bit of benchmarking in regards to scrypt for example, and our implementation is actually faster than the scrypt crate by 30%(And actually runs it in parallel if the parallelization parameter is greater than 1, which is actually an open issue on their repo).

In general, the biggest thing really which I think 100% warrants us using an external library should be if there's any possibility of hardware acceleration.

It'd be a bad idea to make a software implementation of sha256 because a lot of CPUs have support for hardware acceleration in regards to it. I don't know for certain if HMAC is in a similar situation, but if it is, we should 100% use the external library. There's a similar situation for AES(hence why it's imported instead of implemented ourselves), since implementing it ourselves would prevent us from using their native hardware acceleration.

Sometimes it's better to use external libraries(especially if it involves a ton of oddly specific edgecases that bloats up the code, or if the external library has many different configurations for different cpus), but doing it ourselves can be good in situations where we could do it faster or better than the external library itself(provided that it's not too difficult to maintain).

[syjn99]

Thanks! And apologize for the late reply: I've just arrived in Paris so suffering from jet lag :)

And actually runs it in parallel if the parallelization parameter is greater than 1, which is actually an RustCrypto/password-hashes#79 on their repo

I didn't notice that scrypt implementation in Rust Crypto doesn't benefit from parallelization. 30% is quite a reasonable number if we built upon parallelization. Would you mind sharing the result? Is there any link in our Github that I'm missing?

I don't know for certain if HMAC is in a similar situation

In this case, as HMAC is a simple scheme that is built on a cryptographic hash function (in case of us it's sha256), I'm pretty sure acceleration of sha256 is enough.

but doing it ourselves can be good in situations where we could do it faster or better than the external library itself(provided that it's not too difficult to maintain).

Agreed on this point, provided we have better implementation of scrypt in terms of performance. But in case of the lower layer building blocks (pbdkf2, salsa20, and hmac), what's the prior reason for maintaining the codebase for ourselves? I didn't investigate much about this, butpbkdf2 provides parallel feature.

[KolbyML]

Overall I think something like this issue isn't a priority and wouldn't help us reach any of our MVP's. So regardless of the discuss I don't think anybody on the team should work on this nomatter the discussion for at least until we have production grade software on mainnet. The only exception to this would be if there was a security vulnerability or if the implementation doesn't work.

I think as a team we should focus on issues that move the needle forward. We have limited resources, etc, etc.

[syjn99]

Make sense, then I think I can close this discussion