Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

FSB hash function #256

Merged
merged 21 commits into from
Jul 18, 2021
Merged

FSB hash function #256

merged 21 commits into from
Jul 18, 2021

Conversation

iquerejeta
Copy link
Contributor

As an exercise, I've implemented the FSB hash function taking as a reference the paper, so I might have missed out on some efficiency tricks in the reference implementation.

If you'd be happy to have this code in the repo, I'm happy to maintain it and make the necessary changes to take it to the repo quality standards!

@iquerejeta iquerejeta mentioned this pull request May 2, 2021
18 tasks
.gitignore Outdated
Comment on lines 4 to 5
.idea
.DS_Store
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Speaking as a fellow IntelliJ user...

I'd suggest adding the following to your personal ~/.gitconfig instead:

[core]
  excludesfile = /home/myuser/.gitignore

...and then putting these kinds of directives in there.

That avoids every other project having to add .gitignore directives for every editor.

fsb/Cargo.toml Outdated
@@ -0,0 +1,16 @@
[package]
name = "fsb_rust"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

RustCrypto owns the fsb name. You can use that.

@newpavlov can you add rustcrypto:hashes as an owner?

fsb/Cargo.toml Outdated Show resolved Hide resolved
@tarcieri
Copy link
Member

tarcieri commented May 3, 2021

Looks like a promising start!

@iquerejeta can you rebase? You'll probably want to git checkout --theirs Cargo.lock and have it recomputed from there.

#[allow(dead_code)]
#[macro_use]
mod macros;
mod pi;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

GitHub won't even let me leave comments on that module, but... wow 😮

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not my work! Copied that file from the reference implementation

@tarcieri
Copy link
Member

tarcieri commented May 3, 2021

@iquerejeta it looks like you need to bump clippy MSRV in .github/workflows/workspace.yml L19

Also can copy/modify a README.md from one of the other crates? (e.g. blake2) Thanks!

@iquerejeta
Copy link
Contributor Author

iquerejeta commented May 3, 2021

I've rebased, hopefully correctly 😄

  • Missing the bump on clippy

qq, should I squash all commits to have a cleaner history?

@tarcieri
Copy link
Member

tarcieri commented May 3, 2021

We use GitHub's "Squash and merge" feature so you don't need to worry about squashing it yourself

@iquerejeta
Copy link
Contributor Author

iquerejeta commented May 16, 2021

Bumped version. I got a security audit failing in the repo, but can't really tell what the problems are. Also I got a bunch of PRs from the bot, requesting to bump dependencies on other crates (e.g. sha1 or sha2). Should I go ahead and merge all these PRs?
Thanks!

@tarcieri
Copy link
Member

Tests appear to be passing now.

Copy link
Member

@newpavlov newpavlov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I haven't checked the implementation itself, so I have only several surface-level comments for now.

@@ -16,7 +16,7 @@ jobs:
- uses: actions/checkout@v1
- uses: actions-rs/toolchain@v1
with:
toolchain: 1.41.0 # MSRV
toolchain: 1.47.0 # MSRV
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can remove the "MSRV" comment from here, since it's not about MSRV, but about pinning clippy version.

@@ -81,6 +81,10 @@ dependencies = [
"generic-array",
]

[[package]]
name = "fsb_rust"
version = "0.1.0"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since you've already changed the crate name, re-generate the lock file, so it will use fsb instead of fsb_rust? I have shared ownership of the fsb crate with the hashes group, so we can publish it immediately after merge.

fsb/Cargo.toml Outdated
categories = ["cryptography", "no-std"]

[dependencies]
whirlpool = "0.9"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suggest using whirlpool = { version = "0.9", path = "../whirlpool", default-features = false } here.

fsb/README.md Outdated
[![crate][crate-image]][crate-link]
[![Docs][docs-image]][docs-link]
[![Build Status][build-image]][build-link]
-->
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't forget to uncomment these lines?

@@ -0,0 +1,21 @@
#![allow(non_snake_case)]
#[allow(dead_code)]
#[macro_use]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it truly necessary to allow dead code? It also would be nice to add crate-level docs, no_std and other attributes (see other crates for reference).

Copy link
Contributor Author

@iquerejeta iquerejeta Jun 5, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added no_std, and I depend on alloc now for dynamically sized arrays. We use them here.

assert_eq!(
result[..],
hex!("6e8ce7998e4c46a4ca7c5e8f6498a5778140d14b")[..]
);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can simply use the following code here:

assert_eq!(
    Fsb160::digest(msg_1)[..],
    hex!("6e8ce7998e4c46a4ca7c5e8f6498a5778140d14b")[..],
);

@tarcieri tarcieri merged commit a1e8900 into RustCrypto:master Jul 18, 2021
@tarcieri
Copy link
Member

@iquerejeta thank you!

@iquerejeta
Copy link
Contributor Author

Pleasure! I'm happy to change the tests using the use digest::new_test; macro, but when I tried it out I couldn't figure out how to generate the data (e.g. hashes/sha2/tests/data/sha256.blb), any tips?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants