-
Notifications
You must be signed in to change notification settings - Fork 7
Use Digest::SHA instead of Digest::SHA1 #11
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
base: master
Are you sure you want to change the base?
Conversation
|
A slightly simpler patch is used in Debian as well: https://salsa.debian.org/debian/razor/-/blob/debian/1%252.85-7/debian/patches/use-Digest-SHA.patch |
Yes, however the Debian patch does not provide any backwards compatibility, which may or may not be desired by upstream. |
Indeed, I just ran into this while hacking on #13 and wanted to add this data point that most people probably use this change in some way already. |
|
@robert-scheck Updated commit, rebased: michal-josef-spacek@d84debb |
Switch from Digest::SHA1 to Digest::SHA, because: Digest::SHA is a bit faster than Digest::SHA1, Digest::SHA1 has been removed from some Linux distributions, Digest::SHA is a core library (as of Perl >= 5.10.0) and Digest::SHA1 is not (and never will be). See also: - https://src.fedoraproject.org/rpms/perl-Razor-Agent/c/75fa8a6c1f1fdf779312dac68f331a288bd2920f?branch=rawhide - https://stackoverflow.com/questions/3420720/what-are-the-advantages-of-digestsha-over-digestsha1 Original author: Warren Togami <[email protected]> Co-authored-by: Warren Togami <[email protected]>
1a8dc0e to
620122d
Compare
|
@toddr Could we merge this? |
|
@toddr, what's missing to get this merged? Happy to perform changes, if you let me know what needs to be changed… |
|
I took a brief look at it this weekend. My major concern is that if you are trying to get it to load and then fall back to the older module, I don't think it is going to do that right now. Is that your goal? Or are you just trying to use the new module? |
Goal is to use the new module and fall back to the older one if it fails. But I don't know if that's what you would like to see anyway. |
| $sha1->add($corrected_length); | ||
| $sig .= substr $sha1->hexdigest, 0, 4; | ||
| $sig = substr sha1_hex($host), 0, 12; | ||
| $sig .= substr sha1_hex($corrected_length), 0, 4; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I may be missing something about the inner workings of SHA1 but to me it looks like this code behaves differently to the old one:
The old code created a SHA1 object, added the $host, took the first 12 characters of the digest as generated up to that point, added $corrected_length and then took the first four characters of the digest calculated at this new point in time.
The new code calculates two different digests, one over $host and one over $corrected_length, the latter does not include state from the previous digest.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I totally forgot to update this: In the meantime I verified that both code paths calculate the same hash.
| $ctx->add($digest); | ||
| $digest = $ctx->hexdigest; | ||
| my $digest = sha1_hex($iv2, $text); | ||
| $digest = sha1_hex($iv1, $digest); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO this is a bit confusing due to the reuse of $digest. I'd either use $digest1 and $digest2 or just make that a sha1_hex($iv1, sha1_hex($iv2, $text))
Switch from
Digest::SHA1toDigest::SHA, because:Digest::SHAis a bit faster thanDigest::SHA1,Digest::SHA1has been removed from some Linux distributions,Digest::SHAis a core library (as of Perl >= 5.10.0) andDigest::SHA1is not (and never will be). See also:Original author: Warren Togami ([email protected])