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

feat: implement Digest for DomainSeparatedHasher #119

Merged
merged 3 commits into from
Jul 27, 2022

Conversation

stringhandler
Copy link
Contributor

@stringhandler stringhandler commented Jul 25, 2022

Implements Digest for DomainSeparatedHasher with a blank default label
Deleted GenericHashDomain

Motivation

This means you can use a DomainSeparatedHasher in any function that already uses Digest. Unfortunately, the label must be blank, so the domain should be different for struct.

I also added a macro hashing_domain for creating domains quickly (e.g. for tests)

hash_domain!(MyDemoHasher, "macro.test");
// Generates

 pub struct MyDemoHasher {}

        impl DomainSeparation for MyDemoHasher {
            fn version() -> u8 {
                1
            }

            fn domain() -> &'static str {
                "macro.test"
            }
        }   

   

After adding the hash_domain macro it is now easy enough to create a new domain when you need it, removing the need for GenericHashDomain. I found that devs were trying to build on top of it instead of creating new domains, so IMO it's more confusing keeping it in

UPDATE: Removed GenericHashDomain
UPDATE: Added create_hasher! macro

@@ -254,6 +254,49 @@ impl<D: Digest, M: DomainSeparation> DomainSeparatedHasher<D, M> {
}
}

/// Implements Digest so that it can be used for other crates
Copy link
Contributor

Choose a reason for hiding this comment

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

The digest crate already offers a default impl for

impl<D: FixedOutput + Default + Update + HashMarker> Digest for D 

Is this intended to be a stopgap so digest is available while we look at impl the other trait bounds?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I suppose I could implement Update and Reset individually instead, but I am not sure that covers all of them

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried implementing them individually, but because we need to push a prefix when we reset, the Reset needs Update, and FixedOutput needs Reset and it ended up being a lot more code than implementing the whole of Digest.

hansieodendaal
hansieodendaal previously approved these changes Jul 26, 2022
Copy link
Contributor

@hansieodendaal hansieodendaal left a comment

Choose a reason for hiding this comment

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

Hi @stringhandler, LGTM, but please consider to merge this PR into your branch.

Comment on lines +586 to +592
let mut hasher = DomainSeparatedHasher::<Blake256, MyDemoHasher>::new("");
hasher.update(&[0, 0, 0]);
let hash = hasher.finalize();
assert_eq!(
to_hex(hash.as_ref()),
"5faa7d48b551362bbee8a02c43e6ab634ed47c58ecf7b353f9afedfe3d574608"
);
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it is important to show that this equality only holds if an empty label is supplied for the same hash domain, thus add a failing test for a hasher with a label.

Otherwise LGTM

Copy link
Member

@sdbondi sdbondi left a comment

Choose a reason for hiding this comment

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

Implementing Digest and the hash_domain! macro is a great improvement - I have a concern about how create_hasher! should be used.

create_hasher!($digest, $name, $domain, "", 1)
};
($digest: ty, $domain:expr) => {
create_hasher!($digest, __TempHashDomain, $domain, "", 1)
Copy link
Member

Choose a reason for hiding this comment

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

What is __TempHashDomain?

($digest:ty, $name:ident, $domain:expr, $label:expr, $version: expr) => {{
use $crate::hash_domain;

hash_domain!($name, $domain, $version);
Copy link
Member

@sdbondi sdbondi Jul 27, 2022

Choose a reason for hiding this comment

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

My only concern about create_hasher! is that users will duplicate the domain separation domain label.

fn hash_foo(foo:&Foo) -> Hash {
   let hasher = create_hasher!(Blake256, FooBarHasher, "com.foobar", "foo");
   hasher.chain(foo.as_bytes()).finalize()
}

fn hash_bar(bar:&Bar) -> Hash {
   let hasher = create_hasher!(Blake256, FooBarHasher, "com.foobar", "bar");
   hasher.chain(bar.as_bytes()).finalize()
}

Perhaps create_hasher! should take in a domain type?

define_hash_domain!(FooBarHashDomain, "com.foobar");

fn hash_foo(foo:&Foo) -> Hash {
   let hasher = create_hasher!(Blake256, FooBarHashDomain, "foo");
   hasher.chain(foo.as_bytes()).finalize()
}

fn hash_bar(bar:&Bar) -> Hash {
   let hasher = create_hasher!(Blake256, FooBarHashDomain, "bar");
   hasher.chain(bar.as_bytes()).finalize()
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The macro isn't really needed unless you are creating a new struct. (The DomainSeparation).

In your second example you can just create a method

fn create_hasher<D:Digest, Dom:DomainSeparation>(label: &'static str) -> ...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here's an example of create_hasher!, but I agree it's probably better to leave it out and create the method.

     create_hasher!(Blake2b, "com.tari.cipher_seed_mac")
            .chain(plaintext.clone())
            .chain([CIPHER_SEED_VERSION])
            .chain(self.salt)
            .chain(passphrase.as_bytes())
            .finalize()

Choose a reason for hiding this comment

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

same concern as @sdbondi, it's nice to add a suggestive comment not the use the long form macro create_hasher!(), maybe add another macro next to it, that would differ by name instead of only arguments

Copy link

@agubarev agubarev Jul 27, 2022

Choose a reason for hiding this comment

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

users might not even notice that calling the long form in multiple places will define nested structs in different places where it's called, which will work of course, because the data will match, but could be unintentional duplication

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.

5 participants