Expose lower level of blake2b API #159
Conversation
This will support arbitrary length of message to be hashed without having to load all the message into memory beforehand.
|
Unfortunately thanks to the existing API, this is less than ideal. The standard Ruby
My takeaway is the existing API is kind of not great, and that the existing So, perhaps we should do that, then we could call your I think it's very confusing to have both This would be a breaking API change, and would necessitate a major version bump. But I think it's the "right" way to handle this, and I'm not opposed to doing that. |
|
Note we still support Ruby 2.2, and the |
Agree, previously I was about to use the existing
ok, will suppress rubocop rule on this. Thank you. |
so we have similar API with ruby's ::Digest. Also this has minimal breaking changes, in which #digest essentially still has 1 arity and will be only callable without params when the initial state for hash calculation is initialized (after calling #reset). Added new tests to define the new behavior.
|
@tarcieri I end up breaking the arity :D |
lib/rbnacl/hash/blake2b_state.rb
Outdated
| @@ -0,0 +1,17 @@ | |||
| # encoding: binary | |||
There was a problem hiding this comment.
Would suggest just defining this inside blake2b.rb instead of making a separate file
lib/rbnacl/hash/blake2b.rb
Outdated
| end | ||
|
|
||
| # Calculate a Blake2b digest | ||
| # | ||
| # @param [String] message Message to be hashed | ||
| # | ||
| # @return [String] Blake2b digest of the string as raw bytes | ||
| def digest(message) | ||
| def digest(*message) | ||
| return final_digest if message.empty? |
There was a problem hiding this comment.
Ugh, I don't like this. I'd rather this method not have two responsibilities. I'd suggest just renaming final_digest to digest, and making the rest of this method a completely separate class method.
There was a problem hiding this comment.
Sure, I taken out the current digest into class method, I also extract the validation from initialize, since I don't think I want to instantiate the object and add attr_reader just to read the options back. Is this looks ok for you? I will rebase these commits later if needed
This is effectively changing #digest behavior, but thanks to the upper RbNaCl::Hash.blake2b API, the invocation is essentially the same. And extract options validation from #initialize, so we don't have to instantiate Blake2b from self.digest. Also put Blake2bState together with Blake2b definition.
| # @raise [RbNaCl::LengthError] Invalid length specified for one or more options | ||
| # | ||
| # @return [Hash] opts Configuration hash with sanitized values | ||
| def self.validate_opts(opts) |
There was a problem hiding this comment.
Can you make this method private? e.g.
class << self
private :validate_opts
endThere was a problem hiding this comment.
What about something like this?
index 1265ec3..703bce7 100644
--- a/lib/rbnacl/hash/blake2b.rb
+++ b/lib/rbnacl/hash/blake2b.rb
@@ -103,6 +103,13 @@ module RbNaCl
opts
end
+ private_class_method :validate_opts
+
+ def self.new(opts = {})
+ opts = validate_opts(opts)
+ super
+ end
+
# Create a new Blake2b hash object
#
# @param [Hash] opts Blake2b configuration
@@ -117,7 +124,6 @@ module RbNaCl
#
# @return [RbNaCl::Hash::Blake2b] A Blake2b hasher object
def initialize(opts = {})
- opts = self.class.validate_opts(opts)
@key = opts[:key]
@key_size = opts[:key_size]
@digest_size = opts[:digest_size]
lib/rbnacl/hash/blake2b.rb
Outdated
| raise(CryptoError, "Hashing failed!") | ||
| end | ||
|
|
||
| def <<(message) |
There was a problem hiding this comment.
Can you use alias for this?
lib/rbnacl/hash/blake2b.rb
Outdated
|
|
||
| # The crypto_generichash_blake2b_state struct representation | ||
| # ref: jedisct1/libsodium/src/libsodium/include/sodium/crypto_generichash_blake2b.h#L23 | ||
| class Blake2bState < FFI::Struct |
There was a problem hiding this comment.
This can just be called State, since it's already in the RbNaCl::Blake2b namespace
but then also need to move its invocation to self.new. Blake2bState is internal within Blake2b so adapt accordingly. Use alias instead of defining new method for `<<`.
|
This looks good to me, but I'll wait a bit for merging in case @namelessjon would like to opine. Note this is a breaking change and will require a major version bump. |
|
Okay, merged, will bump the version to 5.0.0 and release this |
|
Released in RbNaCl 5.0.0 🎉 |
|
Nice, thanks! |
## 5.0.0 (2017-06-13) * [#159](RubyCrypto/rbnacl#159) Support the BLAKE2b Initialize-Update-Finalize API. ([@fudanchii])
Hi,
Wondering if we could expose the low-level API of blake2b.
This would be really useful in the case of hashing really large file or data stream.
Thanks!