crypto: add crypto:kdf/4#11301
Conversation
CT Test Results 2 files 14 suites 4m 31s ⏱️ Results for commit fd899ca. ♻️ This comment has been updated with latest results. To speed up review, make sure that you have read Contributing to Erlang/OTP and that all checks pass. See the TESTING and DEVELOPMENT HowTo guides for details about how to run test locally. Artifacts// Erlang/OTP Github Action Bot |
Mikaka27
left a comment
Mikaka27
left a comment
There was a problem hiding this comment.
Would it be possible for you to add mentions of this to the documentation files as well?
| #ifndef HAVE_ARGON2 | ||
| if (kdf->kind == ARGON2_kdf && kdf->kdf != NULL) { | ||
| EVP_KDF_free(kdf->kdf); | ||
| kdf->kdf = NULL; | ||
| } | ||
| #endif |
There was a problem hiding this comment.
Is that just a precaution, or does it actually happen on some openssl versions?
There was a problem hiding this comment.
OpenSSL < 3.2 doesn't support argon2 but does support other KDFs. This showed up in the CT output, for example, because it appears to run against OpenSSL 3.0.
There was a problem hiding this comment.
But did you see it when you locally compiled it with openssl 3.0, or in github or somewhere else? When I compiled it with openssl 3.0 EVP_KDF_fetch returned NULL for argon2.
There was a problem hiding this comment.
I see the pattern that is used in cipher.c and adopted that in a986cfa
There was a problem hiding this comment.
Ok I see, it was harmless anyway, but I was wondering if I missed something, thanks.
Mikaka27
added
testing
I've added docs to lib/crypto/doc/guides/algorithm_details.md in 83a045f. Let me know if I missed it somewhere else. |
Mikaka27
added
testing
Mikaka27
left a comment
Mikaka27
left a comment
There was a problem hiding this comment.
I'm sorry, I haven't caught everything at once, I added comments.
It would be good if you could squash the commits somehow so we don't have commits fixing other commits.
Otherwise this looks good to me.
Happy to squash, just was trying to keep it incremental for review. |
|
Squashed to d395559 |
|
I did a push, didn't want to bother you with it, I moved |
Mikaka27
added
testing
|
One nit with that change, other initialisms (MAC, RSA, etc) are uppercased in the docs, but this is Kdf. I'd expect it to be KDF. |
I can fix that later, don't have my computer with me now, thanks :) |
|
@jtdowney I've pushed what you pointed out, can you take a look if you see any other problems in the PR? |
2 participants
Add a
crypto:kdf/4NIF backed by OpenSSL's EVP_KDF, supporting argon2, hkdf, scrypt, sskdf, and pbkdf2 with option maps.This was something I had proposed on Erlang Forums and also discovered is the same as #10179.
Closes #10179