Skip to content
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

HMAC-SHA256/512 non 32 byte keys supporting #166

Merged
merged 8 commits into from
Feb 15, 2018

Conversation

gdwrd
Copy link
Contributor

@gdwrd gdwrd commented Dec 14, 2017

Hi,
Here is my PR with non 32 keys supporting.
HMAC RFC 2104: https://tools.ietf.org/html/rfc2104#section-3
Thanks!

@gdwrd
Copy link
Contributor Author

gdwrd commented Dec 14, 2017

#165

@gdwrd
Copy link
Contributor Author

gdwrd commented Dec 19, 2017

Hi @tarcieri, what do you think about my PR?

@tarcieri
Copy link
Contributor

tarcieri commented Dec 19, 2017

I think if you're doing this much, you might as well expose the full IUF API... and ideally make it API-compatible with OpenSSL::HMAC

@gdwrd
Copy link
Contributor Author

gdwrd commented Dec 21, 2017

@tarcieri You mean, change hmac512256 libsodium calls to Init/Update/Final API? And what about OpenSSL::HMAC?

@tarcieri
Copy link
Contributor

@nsheremet you've done most of the work here to expose the IUF API (i.e. initialize, update, finalize or as the OpenSSL::HMAC, or perhaps more generally the Ruby Digest API calls them #initialize, #update, and #digest), but it's not a particularly friendly or idiomatic API. I think if you're adding access to this API it probably should be, and would ideally implement something more akin to a Ruby Digest or OpenSSL::HMAC

@gdwrd
Copy link
Contributor Author

gdwrd commented Dec 21, 2017

@tarcieri hm. I will add IUF Api soon.

@gdwrd
Copy link
Contributor Author

gdwrd commented Jan 11, 2018

@tarcieri Hi, I pushed some updates. Can you review, please?

@tarcieri
Copy link
Contributor

I can review it soon

@gdwrd
Copy link
Contributor Author

gdwrd commented Jan 23, 2018

Hi @tarcieri, did you see the PR?

@tarcieri
Copy link
Contributor

@nsheremet yes, sorry, I haven't had time to review it

# @params [#to_str] message message to construct an authenticator for
def update(message)
self.class.auth_hmacsha256_update(@state, message, message.bytesize)
self.class.auth_hmacsha256_final(@state.clone, @authenticator)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should probably be moved to #digest below so as to allow #update to be called multiple times. I'm also curious how this handles #update being called after it's been finalized. That should probably raise an exception.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @tarcieri. You can use this update multiple times.
But the point is, #update method without calling this line self.class.auth_hmacsha256_final(@state.clone, @authenticator) every time will be rewriting the @state. I can add spec test to compare with openssl multiple usage. But I know that it's work because I test it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

here is RbNaCl example:

pry(main)> key = ['07c0'].pack('H*')
pry(main)> hmac = RbNaCl::HMAC::SHA256.new(key)
pry(main)> hmac.update 'abcd'
pry(main)> hmac.update '1'
pry(main)> hmac.digest
=> "G\xCC[\xBDI\x1A\xA3\x95\xCD\xA2@\xC2\xB2L\xCD\xE9\x03kV\xDA\x7F\xAB@\xBE\xB5\xA7V\x1C\x11G\xBE\xD8"

OpenSSL example:

pry(main)> key = ['07c0'].pack('H*')
pry(main)> hmac = OpenSSL::HMAC.new(key, d)
pry(main)> hmac.update 'abcd'
pry(main)> hmac.update '1'
pry(main)> hmac.digest
=> "G\xCC[\xBDI\x1A\xA3\x95\xCD\xA2@\xC2\xB2L\xCD\xE9\x03kV\xDA\x7F\xAB@\xBE\xB5\xA7V\x1C\x11G\xBE\xD8"

# @params [#to_str] message message to construct an authenticator for
def update(message)
self.class.auth_hmacsha512_update(@state, message, message.bytesize)
self.class.auth_hmacsha512_final(@state.clone, @authenticator)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

here is the answer #166 (comment)

# @params [#to_str] message message to construct an authenticator for
def update(message)
self.class.auth_hmacsha512256_update(@state, message, message.bytesize)
self.class.auth_hmacsha512256_final(@state.clone, @authenticator)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And ditto here too.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

here is the answer #166 (comment)

@tarcieri
Copy link
Contributor

This looks mostly good but I think all three primitives you're implementing here should both handle multiple invocations of #update and raise if updated after they've been finalized.

@gdwrd
Copy link
Contributor Author

gdwrd commented Feb 12, 2018

@tarcieri I also can make that after you call #update method, it will return hexdigest. The same functionality as in openssl #update method.

@tarcieri
Copy link
Contributor

@nsheremet sorry I misunderstood how this works. I think the change you suggest (to match the OpenSSL::HMAC behavior) sounds good

@gdwrd
Copy link
Contributor Author

gdwrd commented Feb 12, 2018

@tarcieri I updated #update method. Done.

@gdwrd
Copy link
Contributor Author

gdwrd commented Feb 13, 2018

@tarcieri so, will you merge this PR?

end
end

# The crupto_auth_hmacsha256_state struct representation
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typo

# encoding: binary
# frozen_string_literal: true

RSpec.shared_examples "hmac" do
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tiny nit but I'd personally prefer "HMAC" here

@tarcieri
Copy link
Contributor

One typo and one nit, otherwise it looks good, thanks for your contribution!

@gdwrd
Copy link
Contributor Author

gdwrd commented Feb 14, 2018

@tarcieri I fixed typos.

@gdwrd
Copy link
Contributor Author

gdwrd commented Feb 15, 2018

@tarcieri I think that's all 😄

@tarcieri tarcieri merged commit 4cba02e into RubyCrypto:master Feb 15, 2018
@tarcieri
Copy link
Contributor

Thanks!

@gdwrd
Copy link
Contributor Author

gdwrd commented Feb 15, 2018

@tarcieri I am glad to help!

@gdwrd gdwrd deleted the hmac_keysize_fix branch February 15, 2018 17:10
@tarcieri tarcieri mentioned this pull request Nov 8, 2018
netbsd-srcmastr pushed a commit to NetBSD/pkgsrc that referenced this pull request Dec 17, 2018
## [6.0.0] (2018-11-08)

[6.0.0]: RubyCrypto/rbnacl#182

* [#180](RubyCrypto/rbnacl#180)
  Deprecate rbnacl-libsodium.
  ([@tarcieri])

* [#176](RubyCrypto/rbnacl#176)
  Add support for XChaCha20-Poly1305.
  ([@AnIrishDuck])

* [#174](RubyCrypto/rbnacl#174)
  Fix buffer size type in `randombytes_buf` binding.
  ([@elijh])

* [#172](RubyCrypto/rbnacl#172)
  Add support for argon2id digest.
  ([@trofi])

* [#166](RubyCrypto/rbnacl#166)
  Support for non-32-byte HMAC-SHA256/512 keys.
  ([@nsheremet])
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants