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

crypto/hash/digest reorganization #7503

Closed
Seldaek opened this issue Jun 30, 2013 · 10 comments
Closed

crypto/hash/digest reorganization #7503

Seldaek opened this issue Jun 30, 2013 · 10 comments
Labels
C-cleanup Category: PRs that clean code up or issues documenting cleanup.

Comments

@Seldaek
Copy link
Contributor

Seldaek commented Jun 30, 2013

Right now extra has md4/sha1/sha2 implemented, yet many more hashing algos exist so I'm thinking consolidating it now might make sense. sha1/sha2 are in extra/crypto/ but exported as extra::{sha1,sha2}, they both implement the Digest trait. md4 is simply in extra/ and has its own API.

Therefore I would like to suggest the following changes (and volunteer to realize them should they be accepted):

  • move extra/md4.rs to extra/crypto/md4.rs and make it implement Digest
  • potentially move all three in an extra::crypto::{md4,sha1,sha2} module, although I'd rather call it extra::hash:: because not all hash algos are to be considered cryptographically secure. Perhaps that is the reason md4 is not in crypto right now, but I think it'd be best to clean this up now before more hashes get added and the BC requirements becomes stronger.
@Seldaek
Copy link
Contributor Author

Seldaek commented Jun 30, 2013

Ping @DaGenix - since you moved the stuff into crypto/, I assume you would have an opinion on this.

@bluss
Copy link
Member

bluss commented Jun 30, 2013

Just a thought, maybe the digest types should implement io::Writer, when that trait arrives in its new form. Or there could be a trivial wrapper for it.

@DaGenix
Copy link

DaGenix commented Jun 30, 2013

I exported Sha1/Sha2 as extra::{Sha1,Sha2} instead of extra::crypto::{Sha1,Sha2} only because as best as I could tell, that was what was being done in other places in libextra / libstd. Longer term it does seem like as there are more functions implemented, exporting them all together under extra::crypto (or something similar) would be easier to maintain than exporting them all in the main namespace.

When I created the Digest trait, I left the md4 code alone simply because its current implementation can't support the Digest trait I created - the md4 code wants to process an entire message at a time, while the Digest trait specifies an interface to process chunks of a message. I didn't want to bundle the md4 code alongside the Sha1/Sha2 code until they supported the same traits. Updating the md4 code to support processing a message incrementally and implement the same traits as Sha1/Sha2 sounds like a great idea to me.

There are a wide variety of different cryptographic functions that I would imagine would be nice to have in libextra some day: symmetric ciphers, asymmetric ciphers, message authentication codes, key derivation functions, key agreement protocols, etc. I think the question is how to organize it all. A few options I can think of (and there are probably more):

  1. Put each specific implementation directly into the crypto sub-module - extra::crypto::Sha1, extra::crypto::Aes, etc.
  2. Group each type of function into its own sub-module of extra - extra::digest::; extra::symmetriccipher::, extra::mac::*, etc
  3. Group each type of function into its own sub-module of crypto - extra::crypto::digest::; extra::crypto::symmetriccipher::, extra::crypto::mac::*, etc

Personally, I think I prefer #3, though, there are pros and cons for every approach.

Another question that doesn't seem to have a widely agreed upon answer is what to call a function like Sha2 - a Hash or a Digest. Personally, I prefer Digest since I think the term hash function is a bit ambiguous: If someone says "hash function," does that mean a hash function suitable for use with a hash table or a hash function suitable for securing communication?

Well, those are my initial thoughts. I don't really feel too strongly about anything above, though. I'm fairly new to Rust, so, I'm not sure if any particular options are more appropriate for Rust style than others.

@Seldaek
Copy link
Contributor Author

Seldaek commented Jun 30, 2013

md4: OK I did not realize the Digest interface meant it had to be able to process messages in chunks. Makes sense.

Regarding naming, Digest IMO is ok, but as said above though crypto can be misleading if we shove all sorts of hashing algorithms in there. Older/simpler algorithms with known vulnerabilities should not be treated as cryptographically secure, and labeling crypto sort of infers that.

As for the grouping, I think I would favor 1 because having to remember what group the algorithm you want to use belongs to can be a pain and isn't very developer-friendly.

I am also very new to rust, but I would be happy to help with this if the core folks set a direction.

@DaGenix
Copy link

DaGenix commented Jun 30, 2013

I agree that crypto isn't a perfect name. What other options are there?

If the goal is the name the module structure to differentiate broken algorithms from non-broken ones, how do we handle something like Sha1? Sha1 is considered broken for use in digital signatures, but, I think, is still considered secure for its use in HMAC functions: http://www.schneier.com/blog/archives/2005/02/sha1_broken.html. That article is old, but, I couldn't find anything newer. Regardless, even if that no longer applies to Sha1, how can the module structure handle an algorithm that is insecure for one purpose but secure for another?

Sha2 is generally considered to still be secure. So, it would got in a module indicating that its secure. But, what happens if Sha2 is broken next week? Does it get moved to a new module to indicate that its no longer secure? That would brake any applications that are making use of the algorithm.

@Seldaek
Copy link
Contributor Author

Seldaek commented Jun 30, 2013

Well I would use extra::hash (or extra::digest) because that's more generic and does not convey a notion of security. I was not suggesting that we split the algos by level of security, I think it's better people educate themselves a little bit about that, because indeed we can't just move stuff around like that after 1.0.

@DaGenix
Copy link

DaGenix commented Jul 3, 2013

Ah, I guess I misunderstood you earlier. That sounds reasonable to me.

@msullivan
Copy link
Contributor

Nominating for backwards compatible.

@catamorphism
Copy link
Contributor

libextra is out of scope for milestones. De-nominating

@emberian
Copy link
Member

Closing since we've decided to remove these from libextra.

flip1995 pushed a commit to flip1995/rust that referenced this issue Jul 29, 2021
Rustup

r? `@ghost`

changelog: none
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-cleanup Category: PRs that clean code up or issues documenting cleanup.
Projects
None yet
Development

No branches or pull requests

6 participants