Implemented Grostl #10

gsingh93 · 2017-01-17T03:09:58Z

This PR contains an implementation of Grostl. I mainly used this document as a reference for implementation. This page contains a guide for different optimizations one can do in an implementation, as well as many optimized reference implementations, but I chose to implement none of these. The goal of my implementation is that it should be correct, readable, and well tested. This is because none of the other implementations are readable or well-tested, which made using them as a reference difficult. After this is merged, we can work on optimizations, as we'd have a nice reference implementation to test against.

One way Grostl differs from most other hash functions is that it allows you to specify a variable digest size between 1 and 64 bytes. However the block size is different between digests of size 1 to 32 and 33 to 64. In order to prevent people from using the wrong block size, I've added GrostlSmall and GrostlBig which should prevent you from making this mistake when you specify your custom OutputSize. I've also provided type aliases for the common digest sizes, 224, 256, 384, and 512.

While the code is done, two minor things I plan on adding are more comments/documentation, more/better tests (especially around edge cases like padding), and cleaning up some of the assertions. It should be fine for you to start reviewing now, as these are minor changes.

…nction

…atrix to its own mod

…aliases

gsingh93 · 2017-01-17T03:11:05Z

grostl/src/lib.rs

+pub type Grostl224 = GrostlSmall<U28>;
+pub type Grostl256 = GrostlSmall<U32>;
+pub type Grostl384 = GrostlSmall<U48>;
+pub type Grostl512 = GrostlSmall<U64>;


Oops. The fact that GrostlSmall compiles with a size greater than U32 is a problem...

Thank you for your work!

Unfortunately it seems that according to E0122 "trait bounds are not (yet) enforced in type definitions" and because of the bug I've missed it. (also you have typo in GrostlSmall which got through because of that, you needed to use Cmp<U33>)

So you'll need to write structs explicitly like this:

#[derive(Default)] pub struct GrostlSmall<OutputSize> where OutputSize: ArrayLength<u8> + Cmp<U33>, Compare<OutputSize, U33>: Same<Less> { grostl: Grostl<OutputSize, U64>, } impl<OutputSize> Digest for GrostlSmall<OutputSize> { type OutputSize = OutputSize; type BlockSize = U64; fn input(&mut self, input: &[u8]) { self.grostl.process(input); } fn result(mut self) -> GenericArray<u8, Self::OutputSize> { self.grostl.finalize() } }

It's probably not worth to implement Digest for Grostl in this situation.

I will review your code shortly.

newpavlov

It's just a cursory review. I haven't looked deep into implementation yet, but it looks good.

Though formatting differs from used in other crates. But it's my fault, as I haven't got time to make rustfmt config. (personally I am not fond of multiline approach of rustfmt)

Also, please add LICENSE files as in other crates.

newpavlov · 2017-01-17T15:46:11Z

grostl/Cargo.toml

@@ -0,0 +1,18 @@
+[package]
+name = "grostl"
+version = "0.3.0"


It's better to start from 0.1.0 as we don't require version synchronization across crates.

newpavlov · 2017-01-17T15:46:28Z

grostl/Cargo.toml

+authors = ["The Rust-Crypto Project Developers"]
+license = "MIT/Apache-2.0"
+description = "Grostl hash function"
+documentation = "https://docs.rs/md5"


Wrong link to docs.

newpavlov · 2017-01-17T15:48:02Z

grostl/src/grostl.rs

+use generic_array::typenum::{Quot, U8};
+use matrix::Matrix;
+
+const SBOX: [u8; 256] = [


I think it's better to move constants to the separate module, in other crates I've used consts.rs for this purpose.

newpavlov · 2017-01-17T15:57:25Z

grostl/src/grostl.rs

+const SHIFTS_Q_WIDE: [u8; 8] = [1, 3, 5, 11, 0, 2, 4, 6];
+
+pub struct Grostl<OutputSize, BlockSize: ArrayLength<u8>>
+    where BlockSize::ArrayType: Copy


Add #[derive(Copy, Clone)] to Grostl and GrostlState. You'll need to add several BlockSize::ArrayType: Copy trait bounds for that to work.

newpavlov · 2017-01-17T15:59:55Z

grostl/src/grostl.rs

+        let block_bytes = BlockSize::to_usize();
+        if block_bytes == 64 {
+            false
+        } else {


Maybe use match statement with unreachable! macro?

newpavlov · 2017-01-17T16:05:24Z

grostl/src/grostl.rs

+#[cfg(test)]
+mod test {
+    use super::{xor_generic_array, C_P, C_Q, Grostl, SHIFTS_P};
+    use generic_array::typenum::{U32, U64};


I think it's better to move this test module to the tests.rs and use:

#[cfg(test)] mod tests

In the lib.rs.

On the second thought it will require to make tested methods public, they still will not be accessible to the crate user as grostl field will be private, but perhaps it's better to leave it as is. Your choice.

newpavlov · 2017-01-17T16:09:29Z

grostl/src/grostl.rs

+
+struct GrostlState<OutputSize, BlockSize: ArrayLength<u8>> {
+    state: GenericArray<u8, BlockSize>,
+    rounds: u8,


You also can impl Default for GrostlState. This will allow you to simplify code and tests a bit and you will be able to simply use #[derive(Default)] on Grostl.

newpavlov · 2017-01-17T16:20:20Z

grostl/tests/lib.rs

+extern crate grostl;
+
+use crypto_tests::hash::{Test, main_test};
+use generic_array::typenum::{U28, U32, U48, U64};


You can use type aliases instead of using GrostlSmall and GrostlBig directly. In my opinion it will look a bit nicer.

newpavlov · 2017-01-18T17:09:57Z

By the way, correct name for Grøstl is Groestl, not Grostl. So it's better to rename crate. :)

Adding "grostl" to kewords should help with searchability.

gsingh93 · 2017-01-22T07:21:32Z

Ok, I believe I've addressed all of your comments. The crate and all structs should be renamed, the Small/Big issue should be fixed, and the other minor code changes were made. I decided to leave the tests in the same mod, as those were unit tests which need access to the private functions. The tests in tests are integration tests and only use the public API.

I still need to add some tests to ensure the padding is correct, I haven't convinced myself that it is. I'm also planning on adding a benchmark, as I see there's one in the utils crate.

Also, what names do I put for the copyright in the LICENSE file? My own?

newpavlov

Several minor comments.

newpavlov · 2017-01-22T12:30:50Z

groestl/LICENSE-MIT

@@ -0,0 +1,27 @@
+Copyright (c) 2006-2009 Graydon Hoare
+Copyright (c) 2009-2013 Mozilla Foundation
+Copyright (c) 2016 Artyom Pavlov


It's your work, so it's better to attribute yourself.

newpavlov · 2017-01-22T12:39:10Z

groestl/src/groestl.rs

+    fn result(mut self) -> GenericArray<u8, Self::OutputSize> {
+        self.buffer.next(1)[0] = 128;
+        if self.buffer.remaining() >= 8 {
+            let block_bytes = self.block_bytes();


You can use standard_padding method from digest-buffer to reduce amount of code.

newpavlov · 2017-01-22T12:41:52Z

groestl/src/groestl.rs

+
+impl<OutputSize, BlockSize> Digest for Groestl<OutputSize, BlockSize>
+    where OutputSize: ArrayLength<u8>,
+          BlockSize: ArrayLength<u8> + Div<U8>,


Not sure if it's worth to implement Digest for Groestl. You can move input and result to struct's impl. It'll reduce amount of code a bit.

newpavlov · 2017-01-22T12:45:27Z

groestl/src/groestl.rs

+    #[test]
+    fn test_shift_bytes() {
+        let g: Groestl<U32, U64> = Groestl::default();
+        let s = g.state;


You do not use g anywhere after that. Maybe just write let s = GroestlState::<U32, U64>::default();? The same goes for other tests as well.

newpavlov · 2017-01-22T12:53:34Z

groestl/src/groestl.rs

+            48, 57, 2, 11, 20, 29, 38, 47,
+            56, 1, 10, 19, 28, 37, 46, 55,
+        ];
+        assert_eq!(&*block, &expected[..]);


You can write it as assert_eq!(&block[..], &expected[..]);, which will be a bit clearer in my opinion. The same goes for other tests. I'll probably create later PR for generic-array with implementation of PartialEq<[T]>.

newpavlov · 2017-01-22T12:59:31Z

groestl/src/lib.rs

+//! let mut hasher = GroestlBig::<U52>::new();
+//! hasher.input(b"my message");
+//! let result = hasher.result();
+//!}


Unnecessary }.

newpavlov · 2017-01-22T13:00:34Z

groestl/src/lib.rs

+//! ```
+//!
+//! ```rust,ignore
+//! use groestl::{Digest, Groestl256};


Should be GroestlSmall and GroestlBig, not Groestl256.

newpavlov · 2017-01-22T13:01:14Z

groestl/src/lib.rs

+mod matrix;
+
+#[derive(Default)]
+pub struct GroestlSmall<OutputSize>


You forgot to derive Copy and Clone.

newpavlov · 2017-01-22T13:01:53Z

groestl/src/lib.rs

+}
+
+#[derive(Default)]
+pub struct GroestlBig<OutputSize>


Copy and Clone.

gsingh93 · 2017-01-22T20:57:11Z

Ok, I made those changes and I added some benchmarks. Since I'm using the standard_padding function now, I'm going to assume everything is fine and I don't need to write any extra tests, so I think this should be good to go.

newpavlov · 2017-01-22T21:47:23Z

Thank you for you work!

Unfortunately performance is a bit lower than I expected (slightly less than 1 MB/s), but I will leave it for future improvements, I'll add your links to the related issue.

I will publish this crate in the next few days.

Add AArch64 sha1 and sha2 extension support

gsingh93 added 22 commits January 11, 2017 20:29

Added stub Grostl implementation and tests

d96e552

Implemented compress

00fb505

Implement input and result Digest functions

f5912ec

Initialize Grostl state to correct value

e4f0b21

Implemented Grostl padding function

445422e

Ensure max OutputSize for GrostlBig is 512 bits

b6a9b72

Moved Grostl implementation to grostl.rs

213c4ea

Implemented Grostl finalize

11709d9

Move all bounds into where clauses

c47721b

Implemented Grostl P, Q, and sub_bytes

90e8f89

Implemented a generic Matrix type to use in the Grostl compression fu…

08358c8

…nction

grostl: Added shift_bytes, test_shift_bytes, fixed some bugs, moved M…

3e70c21

…atrix to its own mod

grostl: implemented add_round_constant

8075c9e

grostl: implemented mix_bytes

3fed52d

grostl: convert BlockSize and OutputSize from bits to bytes

9da68da

grostl: fixed bug where matrix row and column were flipped

ed01531

grostl: handle overflow in Matrix::mul_array

13f1a98

grostl: fixed padding bugs

e5a4dee

grostl: implemented more unit tests and fixed various bugs

c763327

grostl: use digest buffer to be able to handle partial input

c38209d

grostl: handle padding if less than 8 bytes are remaining, fix tests

9fb3b4d

grostl: don't depend on std, add crate doc block, define more Grostl …

5982723

…aliases

gsingh93 commented Jan 17, 2017

View reviewed changes

newpavlov requested changes Jan 17, 2017

View reviewed changes

grostl: cleanup based on review comments

6e0ce08

newpavlov reviewed Jan 22, 2017

View reviewed changes

newpavlov approved these changes Jan 22, 2017

View reviewed changes

grostl: more review cleanup

3be3a90

gsingh93 added 2 commits January 22, 2017 12:45

grostl: added benchmarks

5cef829

grostl: removed Digest implementation for Groestl

fab5981

newpavlov merged commit 86e3886 into RustCrypto:master Jan 22, 2017

newpavlov mentioned this pull request Jan 22, 2017

Add Grostl #8

Closed

newpavlov mentioned this pull request Feb 2, 2017

Missing hash functions #1

Open

18 tasks

linkmauve pushed a commit to linkmauve/hashes that referenced this pull request Jan 4, 2020

Merge pull request RustCrypto#10 from linkmauve/aarch64

4085cde

Add AArch64 sha1 and sha2 extension support

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implemented Grostl #10

Implemented Grostl #10

gsingh93 commented Jan 17, 2017 •

edited

Loading

gsingh93 Jan 17, 2017

newpavlov Jan 17, 2017 •

edited

Loading

newpavlov left a comment •

edited

Loading

newpavlov Jan 17, 2017

newpavlov Jan 17, 2017

newpavlov Jan 17, 2017

newpavlov Jan 17, 2017

newpavlov Jan 17, 2017

newpavlov Jan 17, 2017

newpavlov Jan 18, 2017 •

edited

Loading

newpavlov Jan 17, 2017

newpavlov Jan 17, 2017

newpavlov commented Jan 18, 2017 •

edited

Loading

gsingh93 commented Jan 22, 2017 •

edited

Loading

newpavlov left a comment

newpavlov Jan 22, 2017

newpavlov Jan 22, 2017

newpavlov Jan 22, 2017 •

edited

Loading

newpavlov Jan 22, 2017

newpavlov Jan 22, 2017 •

edited

Loading

newpavlov Jan 22, 2017

newpavlov Jan 22, 2017

newpavlov Jan 22, 2017

newpavlov Jan 22, 2017

gsingh93 commented Jan 22, 2017

newpavlov commented Jan 22, 2017

Implemented Grostl #10

Implemented Grostl #10

Conversation

gsingh93 commented Jan 17, 2017 • edited Loading

Choose a reason for hiding this comment

newpavlov Jan 17, 2017 • edited Loading

Choose a reason for hiding this comment

newpavlov left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

newpavlov Jan 18, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

newpavlov commented Jan 18, 2017 • edited Loading

gsingh93 commented Jan 22, 2017 • edited Loading

newpavlov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

newpavlov Jan 22, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

newpavlov Jan 22, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gsingh93 commented Jan 22, 2017

newpavlov commented Jan 22, 2017

gsingh93 commented Jan 17, 2017 •

edited

Loading

newpavlov Jan 17, 2017 •

edited

Loading

newpavlov left a comment •

edited

Loading

newpavlov Jan 18, 2017 •

edited

Loading

newpavlov commented Jan 18, 2017 •

edited

Loading

gsingh93 commented Jan 22, 2017 •

edited

Loading

newpavlov Jan 22, 2017 •

edited

Loading

newpavlov Jan 22, 2017 •

edited

Loading