-
Notifications
You must be signed in to change notification settings - Fork 188
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
Fix 'OpenSSL::Digest.{digest,hexdigest,base64digest} algorithm, data' #1889
Conversation
It was not consistent with hashing Contents or converting Digest to hexadecimal
Digest and hexdigest were not implemented correctly, leading to an error: `ArgumentError (wrong number of arguments (given 2, expected 1))` Base64digest was missing. The order of arguments for Digest::Class.digest is different than hexdigest and base64digest on purpose, matching the C implementation in MRI. In lib/mri/openssl/digest.rb the arguments are reversed for this in the Ruby method signature. Co-authored-by: Chris Seaton <chris.seaton@shopify.com>
Hello Bart de Water, thanks for contributing a PR to our project! We use the Oracle Contributor Agreement to make the copyright of contributions clear. We don't have a record of you having signed this yet, based on your email address bart -(dot)- dewater -(at)- shopify -(dot)- com. You can sign it at that link. If you think you've already signed it, please comment below and we'll check. |
Bart de Water has signed the Oracle Contributor Agreement (based on email address bart -(dot)- dewater -(at)- shopify -(dot)- com) so can contribute to this repository. |
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.
This looks great, thank you for the fix, I'll integrate it.
def self.hexdigest(message) | ||
digest = new | ||
def self.hexdigest(*parameters, message) | ||
digest = new(*parameters) | ||
digest.update message | ||
digest.hexdigest | ||
end |
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.
Digest.hexdigest
seems defined as Digest.hexencode(Digest::Class.digest(*args))
in MRI:
https://github.com/ruby/ruby/blob/37c2cd3fa47c709570e22ec4dac723ca211f423a/ext/digest/digest.c#L478-L490
And then
truffleruby/lib/mri/openssl/digest.rb
Lines 34 to 36 in 59b78fd
def self.digest(name, data) | |
super(data, name) | |
end |
I'll add a commit to do that.
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.
Yes this definitely was a brainbender 😅 also had a fun time with how Digest::Class
and Digest::Instance
are used depending on using the stdlib Digest or the OpenSSL algorithm modules:
OpenSSL::Digest::SHA256.ancestors
=> [OpenSSL::Digest::SHA256, OpenSSL::Digest, Digest::Class, Digest::Instance, Object, Kernel, BasicObject]
Digest::SHA256.ancestors
=> [Digest::SHA256, Digest::Base, Digest::Class, Digest::Instance, Object, Kernel, BasicObject]
Digest::SHA256.ancestors - OpenSSL::Digest::SHA256.ancestors
=> [Digest::SHA256, Digest::Base]
digest = new(*parameters) | ||
digest.update message | ||
digest.base64digest | ||
end |
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.
Just a note: this was already defined at line 29 above:
https://github.com/oracle/truffleruby/pull/1889/files#diff-048c3691437021dc9c9997d1a3aa7792R29
This file is pretty hard to follow due to reopening classes and part of the code from MRI.
Maybe I should try to convince MRI devs to define more of Digest in Ruby :)
…dle 2 arguments (#1889). PullRequest: truffleruby/1295
Merged in 619ad90, thank you! |
I ran into
digest
not working while testing webauthn-ruby at9b08086b
. The test suite failed with examples such as:While fixing this particular error I also noticed
hexdigest
andbase64digest
not working as expected, so I fixed these as well. More details are in the commit messages.Shopify#1