GNOME Bugzilla – Bug 602715
[GChecksum] Please add support for SHA512
Last modified: 2012-11-22 03:26:30 UTC
I would like to see support for SHA512 in GLib. The beecrypt project has a LGPL-2.1+ version which could probably be used as the base: http://beecrypt.cvs.sourceforge.net/viewvc/beecrypt/beecrypt/sha512.c?view=markup
Created attachment 229172 [details] [review] Add SHA512 support in GChecksum This patch implements SHA512 in GChecksum, following FIPS-180-2 standard <http://csrc.nist.gov/publications/fips/fips180-2/fips180-2.pdf>. Proper unit tests included. Better late than never :) I think we should also add SHA384 support after landing this. It shares a lot of code with the SHA512 implmentation.
Review of attachment 229172 [details] [review]: looks generally okay to me. if you want to add SHA-384, you probably want to do so within this cycle, to keep the GChecksumType ordered (a minor nitpick, I know). I am not entirely sure it's worth adding it, though: I am not aware of anything using the truncated versions of SHA-2.
Thanks for reviewing so quickly. I'm not aware either of anything using SHA384, but I thought it could be added for the sake of completeness and, as you said, maintaining GChecksumType order. I don't particularly need SHA384 but I'm willing to implement it now if it helps landing SHA512 (which I do need).
you could just leave a hole in the enum so it could be filled in later if someone needed it
Created attachment 229177 [details] [review] Adds SHA512 and SHA384 support in GChecksum This is a new patch adding both SHA-512 and SHA384. It is basically the previous patch with a few lines of SHA-384 specific bits, and the corresponding SHA-384 test cases. I didn't mark previous patch as obsolete because I'm ok with merging any of the two. I do think though, that we should take the opportunity and include SHA-384 now that it is done, and being so few code on top of SHA-512. I prefer this to adding a placeholder in the enum. BTW, if you are curious, I used sha512sum <http://linux.die.net/man/1/sha512sum> and sha384sum <http://linux.die.net/man/1/sha384sum> commands to generate the expected results of tests.
Does somebody think is worth reviewing my second patch, or should I go ahead and merge the first one (maybe adding the placeholder in the enum as Dan suggested) ?
Review of attachment 229177 [details] [review]: yeah, I don't think SHA-384 is worth the aggravation, even if the patch looks simple enough. we don't have the SHA-224 short form either, and it has never been raised as an issue. if you really, *really* want all the spectrum of crypto-level hashing, then I have a copy of NSS to sell you anyway. I'd say to go with SHA-512, i.e. your first patch - but before committing it let's wait for another opinion at least.
I don't have a strong opinion, other than the standard "well, are there any glib-using apps using SHA-384?"
I doubt there is, otherwise I would expect to see a bug similar to this one requesting SHA-384 support. I can merge the first patch then (SHA-512 only). Emmanuele, what about the enum, shall we make room for an eventual SHA-384? I guess not. There is no placeholder for SHA-224 either, so... Thanks for your help guys.
I tried to merge but don't have commit access to glib module. Can someone with right permissions help me merge patch from comment 1? Thanks
Pushed as 6e2046207a2fe1c013bbf348b87d54a1375dea7a commit 6e2046207a2fe1c013bbf348b87d54a1375dea7a Author: Eduardo Lima Mitev <elima@igalia.com> Date: Fri Nov 16 18:20:09 2012 +0100 gchecksum: Adds SHA512 support https://bugzilla.gnome.org/show_bug.cgi?id=602715
why you should use sha-384 instead of 512 or 256: https://plus.google.com/114471118004229223857/posts/hzoUxXhhkBf