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

Add hash customization traits #1334

Merged
merged 4 commits into from
Apr 12, 2024

Conversation

sylvainpelissier
Copy link
Contributor

@sylvainpelissier sylvainpelissier commented Jul 17, 2023

Some hash function constructions allow to have a customization string to have domain separation in hash functions. This is the case for CSHAKE and Blake2:

With these new traits, the construction of hash functions with customization strings would be simpler. From currently for CSHAKE:

let mut hasher = CShake128::from_core(CShake128Core::new(b"test")); 

To:

let mut hasher = CShake128:new_personalization(b"test"));

@tarcieri
Copy link
Member

tarcieri commented Jul 17, 2023

nit: BLAKE2 calls this a "personalization string" which I think might be a bit more clear vs "custom"

@sylvainpelissier
Copy link
Contributor Author

Should I change the function and/or parameter names ?

@tarcieri
Copy link
Member

IMO it'd be clearer, but I'm curious what @newpavlov thinks

@newpavlov
Copy link
Member

I don't have a particular preference, so I am fine with both options.

I wonder if we should use an associated type, instead of simple byte slice. For example, cSHAKE also takes the "function-name" argument, which also can be viewed as a customization parameter. IIRC BLAKE2 in addition to the personalization string also can take salt and perform even finer-grained personalization.

@sylvainpelissier
Copy link
Contributor Author

For SHA3, the "function-name" argument is more an internal parameter used for example by TupleHash to instantiate CSHAKE. It may not be accessible from outside. For Blake2 I'm not aware of the salt usage.

@sylvainpelissier
Copy link
Contributor Author

IMO it'd be clearer, but I'm curious what @newpavlov thinks

I made the changes.

@tarcieri
Copy link
Member

I guess BLAKE3 calls it a "context string", and also has some additional stipulations about how it be used (i.e. hardcoded at compile time)

@sylvainpelissier
Copy link
Contributor Author

Is there anything I need to add to this PR ?

digest/src/lib.rs Outdated Show resolved Hide resolved
@tarcieri
Copy link
Member

@sylvainpelissier sorry for the belated reply!

I think it'd be good if you could also open a PR to https://github.com/RustCrypto/hashes which impls these traits, so we can see if e.g. the potential name conflict I was asking about would occur in practice.

Looks like this also needs a rebase.

digest/src/lib.rs Outdated Show resolved Hide resolved
@tarcieri
Copy link
Member

I've changed my mind on the name "personalization", especially since it's used by BLAKE2 but not BLAKE3.

I know I didn't like "custom" before, but "customization" seems ok? It also goes well with e.g. cSHAKE.

@sylvainpelissier sylvainpelissier force-pushed the custom_hash branch 3 times, most recently from 7793e0e to 3704a14 Compare March 14, 2024 15:26
@sylvainpelissier
Copy link
Contributor Author

@sylvainpelissier sorry for the belated reply!

I think it'd be good if you could also open a PR to https://github.com/RustCrypto/hashes which impls these traits, so we can see if e.g. the potential name conflict I was asking about would occur in practice.

Looks like this also needs a rebase.

I'm not able to make a PR in hashes. There is an existing error when compiling hashes with the latest commit of trait (6c5f56e) without my modifications:

$ cargo build
   Compiling digest v0.11.0-pre.8 (https://github.com/RustCrypto/traits?rev=6c5f56e206ff0bbf6ac8b51a6cf4730d278d7c8b#6c5f56e2)
   Compiling sha3 v0.11.0-pre.3 (/home/sylvain/software/hashes/sha3)
   Compiling k12 v0.4.0-pre (/home/sylvain/software/hashes/k12)
error[E0277]: the trait bound `TurboShake128ReaderCore: digest::core_api::XofReaderCore` is not satisfied
   --> k12/src/lib.rs:207:11
    |
207 |     tshk: XofReaderCoreWrapper<TurboShake128ReaderCore>,
    |           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ the trait `digest::core_api::XofReaderCore` is not implemented for `TurboShake128ReaderCore`
    |
    = help: the trait `digest::core_api::XofReaderCore` is implemented for `KangarooTwelveReaderCore`
note: required by a bound in `digest::core_api::XofReaderCoreWrapper`
   --> /home/sylvain/.cargo/registry/src/index.crates.io-6f17d22bba15001f/digest-0.11.0-pre.8/src/core_api/xof_reader.rs:12:8
    |
10  | pub struct XofReaderCoreWrapper<T>
    |            -------------------- required by a bound in this struct
11  | where
12  |     T: XofReaderCore,
    |        ^^^^^^^^^^^^^ required by this bound in `XofReaderCoreWrapper`

For more information about this error, try `rustc --explain E0277`.
error: could not compile `k12` (lib) due to 1 previous error
warning: build failed, waiting for other jobs to finish...

Would you prefer I rebase my modification at digest-v0.11.0-pre.8

@tarcieri
Copy link
Member

Can you still open a PR so we can see the error in context? It's hard to tell what's wrong without code.

@sylvainpelissier
Copy link
Contributor Author

It seems there is no naming conflict in hashes. Is there still something missing ?

@tarcieri
Copy link
Member

I would still suggest changing the name to new_customized (sorry about backpedaling)

@sylvainpelissier
Copy link
Contributor Author

Ah yes, I made the change to new_customized

@tarcieri
Copy link
Member

@sylvainpelissier can you also change the trait name from Personalized* to Customized* and any other remaining instances of "personalized"?

@sylvainpelissier
Copy link
Contributor Author

Yes right. It should be good now.

digest/CHANGELOG.md Outdated Show resolved Hide resolved
@tarcieri tarcieri requested a review from newpavlov April 12, 2024 14:50
@tarcieri
Copy link
Member

Needs rustfmt

digest/CHANGELOG.md Outdated Show resolved Hide resolved
@newpavlov
Copy link
Member

Thank you!

@newpavlov newpavlov merged commit a593570 into RustCrypto:master Apr 12, 2024
9 checks passed
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