technion / ruby-argon2

A Ruby gem offering bindings for Argon2 password hashing
MIT License
229 stars 30 forks source link

Minimum costs #19

Closed bdewater closed 7 years ago

bdewater commented 7 years ago

I want to ensure my test suite is fast, so for the test env I want to use minimum costs. Based on this I think the following should work:

irb(main):007:0> Argon2::Password.new({t_cost: 1, m_cost: 1}).create('test')
Argon2::ArgonHashFail: Argon2::ArgonHashFail
    from /Users/bartdewater/.gem/ruby/2.3.3/gems/argon2-1.1.2/lib/argon2/ffi_engine.rb:59:in `block in hash_argon2i_encode'
    from /Users/bartdewater/.gem/ruby/2.3.3/gems/argon2-1.1.2/lib/argon2/ffi_engine.rb:55:in `initialize'
    from /Users/bartdewater/.gem/ruby/2.3.3/gems/argon2-1.1.2/lib/argon2/ffi_engine.rb:55:in `new'
    from /Users/bartdewater/.gem/ruby/2.3.3/gems/argon2-1.1.2/lib/argon2/ffi_engine.rb:55:in `hash_argon2i_encode'
    from /Users/bartdewater/.gem/ruby/2.3.3/gems/argon2-1.1.2/lib/argon2.rb:24:in `create'
    from (irb):7
    from /opt/rubies/2.3.3/bin/irb:11:in `<main>'

Only from m_cost: 3 I don't get an error. Perhaps a MIN_COST constant (like the Bcrypt gem) could be introduced to make it a bit easier?

technion commented 7 years ago

Hi,

Thanks for reporting this issue! The minimum costing is actually enforced by the C library. You'll note the reference command line tool doesn't work with m = 1 either.

The bug you've found is that it really should be raising

ArgonHashFail: ARGON2_MEMORY_TOO_LITTLE

And being specific with you. I'll look into it.

technion commented 7 years ago

It looks like a signed integer issue and I'm pushing a fix shortly for the lack of specificity on the exception.

Unfortunately I'm reluctant to add a constant that's based on the reference library, because it's a horribly moving target. Which is what we saw in this case - the C library used to return an unsigned integer and errors had a positive error code, and now it uses negative numbers. I would be happy to revisit this if a version is ever marked as stable in future.

The sensible exception should make this obvious to anyone who hits the problem in future.

bdewater commented 7 years ago

Thanks for the swift reply! 🙇

I have zero experience with C extensions so this might be a naive suggestion... but wouldn't it be possible to get the constant values from C that are used in determining the validity of these values and expose them as Ruby datatypes? Or would that require a refactoring in the C code first?

technion commented 7 years ago

but wouldn't it be possible to get the constant values from C

My C wrapper has broken before because constants were moved around or renamed within the official library. So regrettably the answer is yes this is possible, but creates a headache on a moving target.