Repository navigation
Add a sha3_256 module with SHA3-256 - #4919
Conversation
66beddc to
a175470
Compare
rustaceanrob
changed the title
sha3 module with SHA3-256sha3_256 module with SHA3-256
There was a problem hiding this comment.
I validated the implementation using additional test vectors from NIST CAVP SHA3-256 suite
They can be found in my branch here. All inputs pass successfully.
I’ve left some small suggestions in the review comments
a175470 to
ab8cc89
Compare
My friend how were you able to add 156 cases? I am unsure to what extend other reviewers would like this hash tested, but great work and thanks for further validation. |
All the cases come from https://csrc.nist.gov/CSRC/media/Projects/Cryptographic-Algorithm-Validation-Program/documents/sha3/sha-3bytetestvectors.zip I didn’t include all of the very long message cases from |
|
concept ACK adding this hash. @jrakibi great find!! Yeah, for sure, we should throw all the test cases we can find at this. (But honestly, personally, I think that if you just test the empty string and any string longer than the input to the compression function, and your code looks roughly like a hash function should, it's overwhelmingly likely that your hash is correct.) |
Right, but how did you go from the I can either 1. add the |
yaa this is the python script to go from #!/usr/bin/env python3
import sys
import re
def parse_rsp_file(filename):
with open(filename, 'r') as f:
content = f.read()
pattern = r'Len\s*=\s*(\d+)\s*\nMsg\s*=\s*([a-fA-F0-9]*)\s*\nMD\s*=\s*([a-fA-F0-9]+)'
matches = re.findall(pattern, content)
test_cases = []
for length, msg, md in matches:
if length == "0" and msg == "00":
msg = ""
test_cases.append({'input': msg, 'output': md.lower()})
return test_cases
def main():
test_cases = parse_rsp_file(sys.argv[1])
print("const TESTS: [TestCase; {}] = [".format(len(test_cases)))
for case in test_cases:
print(f" TestCase {{")
print(f" input: \"{case['input']}\",")
print(f" output: \"{case['output']}\"")
print(f" }},")
print("];")
if __name__ == "__main__":
main()Then you can run it for the two files |
ab8cc89 to
ab28065
Compare
|
6f878c7 includes the NIST test vectors, many short and some long, within the file. Thanks a bunch for that script @jrakibi
These cases are now covered by all the NIST test vectors. |
ab28065 to
6f878c7
Compare
|
There is no |
|
You could add the new hash to the |
6f878c7 to
cf49e8a
Compare
I had a dull moment. I added the |
cf49e8a to
75a9647
Compare
87bce8f to
e4de9b0
Compare
There was a problem hiding this comment.
All my comments are nits mate except the _ function names. It looks good to me. Code review only, no crypto review but since its so well tested I don't think it needs deep crypto knowledge. I don't know anything about SIMD either.
EDIT: oh drats, this shows up below all the nits because I used "Add single comment" instead of "Start review" - hope I didn't trigger you too hard.
There is a substantial amount of bitcoin listening nodes only reachable by Tor. This is backed by some relatively old research, but I would imagine this remains the case today. When it comes to peer-to-peer gossip, Tor nodes are reachable by a 32-byte ed25519 public key. In practice, most applications will run a Tor daemon, and other applications will communicate with Tor via a Socks5 proxy. With Socks5, you may connect to an IPv4, IPv6, or domain. The problem is to go from Tor public key to onion address (domain), you must take a Sha3-256 hash of a public key and some concatinated data. In effect this makes Tor nodes unreachable for those that do not want to use `RustCrypto` or god forbid the Arti project. Here I propose we add this hash to `bitcoin_hashes`. While it is not explicitly a hash used in bitcoin, many bitcoin nodes use Tor, and more importantly, accept inbound connections using Tor. Others are of course free to use this hash however their heart desires. As far as implementation, from a high level, the hash is a number of permutation operations on a state array of 1600 bits. These bits can be visualized/represented as a 5x5 matrix of 64-bit "lanes." I will leave the rest to the code comment in the file. The Keccak team designed this function to be robust theoretically but easy to implement. Each of these lanes may be mutated directly, and I also try to abuse `for_each` to give SIMD hints. To review the implementation, you may follow the psuedo-code provided by the Keccak team, linked below. ref Psuedo-code: https://keccak.team/keccak_specs_summary.html Full Keccak reference: https://keccak.team/files/Keccak-reference-3.0.pdf Tor spec: https://spec.torproject.org/rend-spec/encoding-onion-addresses.html Tor post: https://archive.torproject.org/websites/lists.torproject.org/pipermail/tor-dev/2017-January/011816.html P2P data: https://github.com/virtu/p2p-metrics
All tests are sourced from NIST via the Cryptographic Algorithm Validation Program. They include namely: 1. the null data case 2. Input cases just above, equal, and below the rate block in bytes 3. Additional randomly sized vectors ref: https://csrc.nist.gov/projects/cryptographic-algorithm-validation-program/secure-hashing
e4de9b0 to
5524934
Compare
Nor do I. From what I understand making function calls inline and using Otherwise I think this is sufficiently tested and I don't have anything else I'd like to iterate on. |
Maybe overkill given the bitcoin ecosystem use case, but we could take a similar strategy as the chacha20 crate and target the experimental portable SIMD interface. That allowed the modern rust compiler to squeeze the most SIMD juice without writing machine code. And has the promise 🤞 of being dropped for the std lib version in the future. Basic idea is to copy what the interface would be for Might want to add a simple |
|
Can we do all that as a follow up if you guys decide to do it? Makes it easier to get this in. |
Yes, preferred
My estimate is this would be called at a maximum around every ~100 ms if someone really needs to connect to Tor nodes. I do think we should follow up with |
|
I couldn't help but run a bench. I would still rather add this in a separate PR, but I thought I would share the results. They are uneventful, which is definitely de-motivating to optimize further, but there seems to be room for improvement. For the immediately pressing use-case the current implementation appears fine. diff --git a/hashes/src/sha3_256/benches.rs b/hashes/src/sha3_256/benches.rs
new file mode 100644
index 00000000..f27f0d9e
--- /dev/null
+++ b/hashes/src/sha3_256/benches.rs
@@ -0,0 +1,33 @@
+use test::Bencher;
+
+use crate::{sha256, Hash, HashEngine};
+
+#[bench]
+pub fn sha3_256_10(bh: &mut Bencher) {
+ let mut engine = crate::sha3_256::Hash::engine();
+ let bytes = [1u8; 10];
+ bh.iter(|| {
+ engine.input(&bytes);
+ });
+ bh.bytes = bytes.len() as u64;
+}
+
+#[bench]
+pub fn sha3_256_1k(bh: &mut Bencher) {
+ let mut engine = crate::sha3_256::Hash::engine();
+ let bytes = [1u8; 1024];
+ bh.iter(|| {
+ engine.input(&bytes);
+ });
+ bh.bytes = bytes.len() as u64;
+}
+
+#[bench]
+pub fn sha3_256_64k(bh: &mut Bencher) {
+ let mut engine = crate::sha3_256::Hash::engine();
+ let bytes = [1u8; 65536];
+ bh.iter(|| {
+ engine.input(&bytes);
+ });
+ bh.bytes = bytes.len() as u64;
+}
diff --git a/hashes/src/sha3_256/mod.rs b/hashes/src/sha3_256/mod.rs
index eccf027a..ee150beb 100644
--- a/hashes/src/sha3_256/mod.rs
+++ b/hashes/src/sha3_256/mod.rs
@@ -17,6 +17,8 @@
//
// To read this file, follow the example code: https://keccak.team/keccak_specs_summary.html
// For a detailed specification: https://keccak.team/files/Keccak-reference-3.0.pdf
+#[cfg(bench)]
+mod benches;
use core::fmt;
crate::internal_macros::general_hash_type! { |
5 participants
My first commit message motivates this change. The brief explanation is Tor nodes are advertised by ED25519 public key, but are often discoverable as
.onionaddresses. To go from public key to onion address, one must takeSHA3(".onion checksum" | PUBKEY | VERSION)for a checksum. I will leave the rest for the commit message, as well as all reference material.