x/crypto/argon2: docs on thread parameter might make users inadvertently create non-portable hashes

[m90]

Both IDKey and Key from golang.org/x/crypto/argon2 take a threads parameter which is documented as following:

The number of threads can be adjusted to the numbers of available CPUs.

While it's obviously wrong in hindsight, this sentence lead me to pass the (dynamic) value of runtime.NumCPU() as the value for threads when hashing passwords, which means the hashes Argon2 would create are different (i.e. stop matching) on hosts with different numbers of CPUs.

Would it make sense to extend the comments here to point out that threads is not purely performance related, but also has an effect on the hash output (e.g. by stating it needs to be a fixed value and you can use the number of CPUs as a guideline)?

[networkimprov]

cc @FiloSottile @katiehockman

[FiloSottile]

Yeah, the docs could be better, happy to take a CL. We could go as far as saying "This is commonly left at 1."

[m90]

Cool, I can see what I can come up with in the next few days and will submit a CL over at the other repo.

[simnalamburt]

There's nothing wrong with using runtime.NumCPU() as P parameter. Users just need to store the P parameter somewhere and recover it before they verify it, just like the any other parameters and salt. Most implementations (including the reference implementation) provides "encoded hash" format to easily store these parameters preventing this kind of human error, while this package does not.

Example of encoded hash:

$argon2i$v=19$m=65536,t=2,p=4$c29tZXNhbHQ$RdescudvJCsgt3ub+b+dWRWJTmaaJObG
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ ^^^^^^^^^^^ ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
     Stored Parameters        Stored Salt            Actual hash

So consider adding a new function which returns encoded hash and verify() function just like the reference implementation.

Reference

• https://github.com/P-H-C/phc-winner-argon2/blob/60b31a3221846f2c1586c5f647aca8cf1c974d84/src/encoding.c#L338-L339

[FiloSottile]

@simnalamburt that's #16971, and you're right it will probably help with this confusion, too.

[gopherbot]

Change https://go.dev/cl/429775 mentions this issue: argon2: amend parameter documentation