Add pwhash argon2i #261

lmctv · 2017-02-12T12:51:05Z

As suggested by @reaperhulk, I'm early-posting the argon2i bindings I'm working on, to add the needed testing context to the #260 PR and starting to implement #233.

While I think the implementation is reasonably complete, documentation is completely missing.

codecov-io · 2017-02-12T13:04:18Z

Codecov Report

Merging #261 into master will increase coverage by 0.02%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #261      +/-   ##
==========================================
+ Coverage   99.85%   99.88%   +0.02%     
==========================================
  Files          36       37       +1     
  Lines        1418     1698     +280     
  Branches       69       90      +21     
==========================================
+ Hits         1416     1696     +280     
  Misses          1        1              
  Partials        1        1

Impacted Files	Coverage Δ
src/nacl/bindings/__init__.py	`100% <ø> (ø)`	⬆️
src/nacl/pwhash.py	`100% <100%> (ø)`	⬆️
tests/test_pwhash.py	`100% <100%> (ø)`	⬆️
src/nacl/bindings/crypto_pwhash.py	`100% <100%> (ø)`	⬆️
tests/test_shorthash.py	`100% <0%> (ø)`	⬆️
tests/test_generichash.py	`100% <0%> (ø)`	⬆️
src/nacl/bindings/crypto_box.py	`100% <0%> (ø)`	⬆️
src/nacl/bindings/crypto_shorthash.py	`100% <0%> (ø)`	⬆️
src/nacl/hash.py	`100% <0%> (ø)`	⬆️
... and 4 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 05cedd6...b0edee3. Read the comment docs.

lmctv · 2017-02-12T13:20:16Z

I don't understand what's happening in jenkin's windows environments, since all of the symbols I'm using seem to be correctly exported, at least from a libsodium's include perspective.

reaperhulk · 2017-02-12T13:43:13Z

libsodium is precompiled on the windows builder and appears to be out of date. Looks like I need to upgrade it.

lmctv · 2017-02-12T14:17:29Z

Thank you for letting me know. Those Jenkins errors got me worried!

reaperhulk · 2017-02-14T13:52:15Z

Jenkins, retest this please

reaperhulk · 2017-02-14T13:53:28Z

Jenkins, retest this please

phc-winner-argon2/argon2 hasher built from https://github.com/P-H-C/phc-winner-argon2 sources The python driver used to generate the json data is available as a gihub gist at https://gist.github.com/lmctv/b035a96ca7c867464ab62482dc81555c For raw kdf usage, fix salt length to 16, as required by libsodium's API

both at cffi level and at python nacl.bindings level.

* insert argon2i examples * add some notes about modular crypt() format

for argon2i and scrypt parameters exposed since version 1.0.12 of libsodium.

and remove now unneeded local definitions.

lmctv · 2017-03-19T19:58:12Z

@reaperhulk is jenkin's

_sodium.obj : error LNK2019: unresolved external symbol _crypto_pwhash_argon2i_bytes_max

caused by a too old libsodium library as happened on february?

lmctv · 2017-03-19T21:40:14Z

@reaperhulk I think this is ready for review now (assuming the windows problem stems from outdated libs)

reaperhulk · 2017-03-19T23:37:23Z

No doubt. I haven't had a chance to upgrade the windows builders to 1.0.12 yet.

reaperhulk · 2017-03-21T13:33:34Z

Jenkins, retest this please

reaperhulk · 2017-03-21T13:49:06Z

Eeeh, jenkins is being a pain but hypothetically libsodium is now upgraded. Rebase/push a commit and we can confirm that.

reaperhulk · 2017-03-21T15:49:53Z

Looks like there's an issue on 32-bit related to maximum memory limit and I need to do one fix for 64-bit python 2.7 on my end.

lmctv · 2017-03-21T15:54:34Z

Strange failures:

jenkins py27 {w32,w64}:

c:\libsodium-1.0.12-msvc\include\sodium\crypto_hash_sha512.h(12) :
fatal error C1083: Cannot open include file: 'stdint.h': No such file or directory

jenkins py3{3,4,5} w32:

nacl.exceptions.ValueError: memlimit must be at most 32768 bytes

Must see what happens with crypto_pwhash_argon2i_MEMLIMIT_MAX on a 32bit linux system.

reaperhulk · 2017-03-21T15:55:07Z

the 64-bit error should be resolved. I had a missing header.

lmctv · 2017-06-08T14:47:58Z

Any kind of review would surely help both me, if you find missing or wrong stuff, and the committers in reviewing and eventually approving the PR.

maqp · 2017-06-08T22:55:01Z

Thoughts on key derivation API

opslimit and memlimit (and their default values) should probably be explained in the documentation
Argument for adjusting parallelism seems to be missing from the API
I'd love to have all different parameters available in the API so that the test vectors could be replicated easily. (This means parameters for secret and associated data would also need to be added (preferably between memlimit and encoder). I understand duplicate tests are not very useful, but it could help users to check that the library is being used correctly.

I found no other major issues in usability. My expertise in reviewing the bindings and implementation are non-existent and IANAC, so proper review is up to others. That being said, thank you for your effort!

lmctv · 2017-06-09T20:17:48Z

@maqp You are right about the need to at least expose the named opslimit and memlimit constants; will comply as soon as I can.

As for the limited API, we have to live within the limits of what libsodium itself exposes; in this case, the simplified argon2i API documented at

https://download.libsodium.org/doc/password_hashing/the_argon2i_function.html

While preparing this PR, I've even tried to open an issue suggesting to also export the low-level full phc
API, jedisct1/libsodium#488 but the issue has been rejected.

Thank you for your time.

reaperhulk · 2017-06-16T16:53:27Z

src/nacl/bindings/crypto_pwhash.py

+    given as input.
+
+    :param outlen: the length of the derived key
+    :type outlen: output key length


This should say int

I'm away from my workstation for a couple of days; will add the needed change in a few days.

reaperhulk · 2017-06-16T16:58:30Z

tests/data/modular_crypt_argon2i_hashes.json

@@ -0,0 +1,92 @@
+[


I may have asked this before, but remind me where these test vectors are from?

reaperhulk · 2017-06-16T17:02:13Z

Oops, I think #260 is where these test vectors come from actually? I assume you intend to rebase this PR after #260 merges still, is that correct?

lmctv · 2017-06-16T20:28:11Z

I posted two distict PRs to keep myself honest with the test vectors; but I think this could as well get in all at once.

I've generated the vectors scripting the command line interface from mainline argon2, since the RFC vectors used different sized salts, which are not supported by libsodium; if you think it could be useful, I could add the generator script.

lmctv · 2017-06-16T21:51:59Z

@reaperhulk I just remembered I've already put the driver script I've used to generate the vectors in #260 online at https://gist.github.com/lmctv/b035a96ca7c867464ab62482dc81555c

To use the script, and generate a set of test vectors, you need a executable
copy of the argon2 cli utility as can be compiled from the sources available at https://github.com/P-H-C/phc-winner-argon2 .

I'm unsure about what is the right place for the gist script in pynacl sources; any suggestions?

lmctv · 2017-06-17T08:26:36Z

@reaperhulk what about putting the vector generation driver inside the docs?

reaperhulk · 2017-06-17T16:56:24Z

We do that with cryptography, although we don't currently have a place for it in the docs here. If you want to add a new section in the style we use on the other project that would be good.

lmctv · 2017-06-17T17:27:04Z

Just took a look at the general structure of the vectors sectoin of cryptography docs; it seems at least a crude version for pynacl is within reach of my writing skills...

for crypto_pwhash_argon2i() binding.

lmctv · 2017-06-25T08:03:09Z

@reaperhulk would you mind taking a look at the present status of #287 ? I think that would be the right place for adding a "how this project is tested" heading and then describe where the test vectors come from, and the instructions/code needed to generate those vectors we didn't find a canonical source for.

lmctv · 2017-06-26T18:27:26Z

@reaperhulk just a little note on merge strategy: since the single-commit in #260 was not altered in this PR, even if merging separately there should be non need to rebasing #261 after merging #260.

lmctv · 2017-06-26T18:45:50Z

@reaperhulk P.S. for the separate merge to work, I think #260 must be merged as-is and not rebased on top of current master...

reaperhulk

Okay, verified the vectors, looked it over again, and left one comment. It'd also possibly be nice to note that there are some known weaknesses with data-independent memory hard functions that data dependent functions like scrypt (or argon2d) do not suffer from. See: https://www.youtube.com/watch?v=YtfVLzUkwME

Essentially: there is a reasonable argument to be made that argon2i is not actually the PBKDF you should use. It is reasonable to still prefer scrypt.

reaperhulk · 2017-06-27T08:31:28Z

docs/password_hashing.rst

    >>> Eves_box = secret.SecretBox(Eves_key)
    >>> intercepted = Eves_box.decrypt(encrypted)
    Traceback (most recent call last):
        ...
    nacl.exceptions.CryptoError: Decryption failed. Ciphertext failed ...

+
+Contrary to the hashed password storage case, where a serialization


Small rephrasing:

Contrary to the hashed password storage case where a serialization format is well-defined, in the raw key derivation case the library user must take care to store (and retrieve) both a reference to the kdf used to derive the secret key and all the derivation parameters. These parameters are needed to later generate the same secret key from the password.

reaperhulk

here we go 😮

lmctv · 2017-06-27T22:15:50Z

@reaperhulk thank you very much for your continued help and support.
@maqp thank you very much for your helpful pre-review.

reaperhulk mentioned this pull request Feb 12, 2017

Upgrade libsodium on windows builders #262

Closed

lmctv force-pushed the add-pwhash-argon2i branch from f5f00ff to 81b678d Compare March 14, 2017 09:40

This was referenced Mar 15, 2017

Upgrade libsodium to 1.0.12 #271

Closed

Upgrade libsodium sources to version 1.0.12 #277

Merged

Add some notes about modular crypt() format usage #263

Closed

lmctv added 5 commits March 19, 2017 15:43

Add low-level bindings for argon2i password hasher

1942b6b

both at cffi level and at python nacl.bindings level.

Add API level bindings for argon2i password hash

438b9fe

Add tests for argon2i password hash

c15b2fc

Update password hashing docs

0d2c5fd

* insert argon2i examples * add some notes about modular crypt() format

lmctv force-pushed the add-pwhash-argon2i branch from 81b678d to 0d2c5fd Compare March 19, 2017 15:29

lmctv added 2 commits March 19, 2017 18:53

Import definitions needed to get limits

b56b539

for argon2i and scrypt parameters exposed since version 1.0.12 of libsodium.

Use libsodium's names for min and max limits

350f928

and remove now unneeded local definitions.

lmctv changed the title ~~WIP - Add pwhash argon2i~~ Add pwhash argon2i Mar 19, 2017

lmctv mentioned this pull request Mar 19, 2017

Expose Argon2 #233

Closed

Add a a reference to libsodium issue

aec09ae

lmctv added 2 commits June 11, 2017 15:06

Add description of opslimit and memlimit constants

d0ee1f1

Reflow overlong lines

1b7d9c7

lmctv mentioned this pull request Jun 11, 2017

Add SealedBox public key constructs implementation #297

Merged

reaperhulk reviewed Jun 16, 2017

View reviewed changes

Correct parameter type in docstring

2975d7a

for crypto_pwhash_argon2i() binding.

This was referenced Jun 25, 2017

Add missing information about test vectors #305

Closed

Move around and split some docs #287

Merged

reaperhulk previously approved these changes Jun 27, 2017

View reviewed changes

lmctv added 2 commits June 27, 2017 20:06

Split overlong sentence as suggested in review.

77f4041

Add a note about tradeoffs between scrpyt and argon2i

b0edee3

lmctv dismissed reaperhulk’s stale review via b0edee3 June 27, 2017 18:27

reaperhulk approved these changes Jun 27, 2017

View reviewed changes

reaperhulk merged commit 70256f7 into pyca:master Jun 27, 2017

lmctv mentioned this pull request Jun 27, 2017

Add argon2i hashes generated via the command line #260

Closed

lmctv deleted the add-pwhash-argon2i branch June 28, 2017 06:26

lmctv mentioned this pull request Oct 15, 2017

Add argon2id to argon2i changelog entry #359

Merged

github-actions bot locked as resolved and limited conversation to collaborators Oct 6, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add pwhash argon2i #261

Add pwhash argon2i #261

lmctv commented Feb 12, 2017 •

edited

codecov-io commented Feb 12, 2017 •

edited by codecov bot

lmctv commented Feb 12, 2017

reaperhulk commented Feb 12, 2017

lmctv commented Feb 12, 2017

reaperhulk commented Feb 14, 2017

reaperhulk commented Feb 14, 2017

lmctv commented Mar 19, 2017

lmctv commented Mar 19, 2017

reaperhulk commented Mar 19, 2017

reaperhulk commented Mar 21, 2017

reaperhulk commented Mar 21, 2017

reaperhulk commented Mar 21, 2017

lmctv commented Mar 21, 2017

reaperhulk commented Mar 21, 2017

lmctv commented Jun 8, 2017

maqp commented Jun 8, 2017

lmctv commented Jun 9, 2017 •

edited

reaperhulk Jun 16, 2017

lmctv Jun 16, 2017

reaperhulk Jun 16, 2017

reaperhulk commented Jun 16, 2017

lmctv commented Jun 16, 2017 •

edited

lmctv commented Jun 16, 2017 •

edited

lmctv commented Jun 17, 2017

reaperhulk commented Jun 17, 2017

lmctv commented Jun 17, 2017

lmctv commented Jun 25, 2017

lmctv commented Jun 26, 2017

lmctv commented Jun 26, 2017

reaperhulk left a comment

reaperhulk Jun 27, 2017

reaperhulk left a comment

lmctv commented Jun 27, 2017

Add pwhash argon2i #261

Add pwhash argon2i #261

Conversation

lmctv commented Feb 12, 2017 • edited

codecov-io commented Feb 12, 2017 • edited by codecov bot

Codecov Report

lmctv commented Feb 12, 2017

reaperhulk commented Feb 12, 2017

lmctv commented Feb 12, 2017

reaperhulk commented Feb 14, 2017

reaperhulk commented Feb 14, 2017

lmctv commented Mar 19, 2017

lmctv commented Mar 19, 2017

reaperhulk commented Mar 19, 2017

reaperhulk commented Mar 21, 2017

reaperhulk commented Mar 21, 2017

reaperhulk commented Mar 21, 2017

lmctv commented Mar 21, 2017

reaperhulk commented Mar 21, 2017

lmctv commented Jun 8, 2017

maqp commented Jun 8, 2017

lmctv commented Jun 9, 2017 • edited

reaperhulk Jun 16, 2017

Choose a reason for hiding this comment

lmctv Jun 16, 2017

Choose a reason for hiding this comment

reaperhulk Jun 16, 2017

Choose a reason for hiding this comment

reaperhulk commented Jun 16, 2017

lmctv commented Jun 16, 2017 • edited

lmctv commented Jun 16, 2017 • edited

lmctv commented Jun 17, 2017

reaperhulk commented Jun 17, 2017

lmctv commented Jun 17, 2017

lmctv commented Jun 25, 2017

lmctv commented Jun 26, 2017

lmctv commented Jun 26, 2017

reaperhulk left a comment

Choose a reason for hiding this comment

reaperhulk Jun 27, 2017

Choose a reason for hiding this comment

reaperhulk left a comment

Choose a reason for hiding this comment

lmctv commented Jun 27, 2017

lmctv commented Feb 12, 2017 •

edited

codecov-io commented Feb 12, 2017 •

edited by codecov bot

lmctv commented Jun 9, 2017 •

edited

lmctv commented Jun 16, 2017 •

edited

lmctv commented Jun 16, 2017 •

edited