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

Expose LibCrypt's PKCS5_PBKDF2_HMAC #7264

Merged
merged 21 commits into from
Apr 8, 2019
Merged

Expose LibCrypt's PKCS5_PBKDF2_HMAC #7264

merged 21 commits into from
Apr 8, 2019

Conversation

mniak
Copy link
Contributor

@mniak mniak commented Jan 4, 2019

Fixes #6973
Replaces pull request #6975

@RX14 RX14 added this to the 0.28.0 milestone Jan 4, 2019
@RX14 RX14 changed the title Add pkcs5 pbkdf2 hmac Expose LibCrypt's PKCS5_PBKDF2_HMAC Jan 4, 2019
@RX14
Copy link
Contributor

RX14 commented Jan 4, 2019

This is perfect, thanks and sorry for the confusion!

Copy link
Member

@straight-shoota straight-shoota left a comment

Choose a reason for hiding this comment

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

The OpenSSL bindings are missing API docs everywhere, but it would be great to include at least some minimal documentation for the newly added features.

src/openssl/algorithm.cr Show resolved Hide resolved
src/openssl/hmac.cr Show resolved Hide resolved
src/openssl/algorithm.cr Outdated Show resolved Hide resolved
SHA512

def to_evp
# The internal bindings to the *LibCrypto* digest operations sometimes require a hash algorithm
Copy link
Contributor

@wooster0 wooster0 Mar 6, 2019

Choose a reason for hiding this comment

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

Ditto. Needs to be put in front of to_evp.
But I would only use the bottom text:

Returns the appropriate equivalent hash algorithm that corresponds to the
current enum value.

Copy link
Member

Choose a reason for hiding this comment

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

"equivalent hash algorithm" should be more precise. This enum already represents hash algorithms. This method returns the corresponding identifier in libcrypto.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The @r00ster91's suggestion seems very concise to me. Thanks for the suggestion.
But really have to agree that this phrasing adds some ambiguity like stated by @straight-shoota.

Copy link
Contributor Author

@mniak mniak Mar 8, 2019

Choose a reason for hiding this comment

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

The OpenSSL documentation mentions the expression "message digest function" when referring to this type.

Also, the documentation for the SSLeay library (from where OpenSSL was forked) makes it clear that the acronym EVP stands for Envelope.

Copy link
Contributor Author

@mniak mniak Mar 8, 2019

Choose a reason for hiding this comment

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

The type of the equivalent hash algorithms in the LibCrypto API is EVP_MD.

I found that this MD stands for Message Digest, and in some point the ancient documentation mentions the expressions and "digest method" and "message digest algorithm" related to this type.

That pretty much confuses me. I thought that the algorithm was called hash and the ouput was called digest. (source).

It seems that back then digest algorithm meant what now we call hash algorithm.

Well... I think this is no help at all. I just wanted to add some value, as I can't by means of language, since I do not write very well in english.

Copy link
Contributor Author

@mniak mniak Mar 11, 2019

Choose a reason for hiding this comment

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

About this review, can I mark as resolved?
I'm not sure what to do

src/openssl/algorithm.cr Outdated Show resolved Hide resolved
src/openssl/algorithm.cr Show resolved Hide resolved
src/openssl/algorithm.cr Outdated Show resolved Hide resolved
src/openssl/algorithm.cr Outdated Show resolved Hide resolved
src/openssl/algorithm.cr Outdated Show resolved Hide resolved
@Sija
Copy link
Contributor

Sija commented Mar 10, 2019

Hmm, why the darwin CI builds are constantly failing...?

@straight-shoota
Copy link
Member

@Sija That's an unrelated issue. See #7535

@Sija
Copy link
Contributor

Sija commented Mar 10, 2019

@straight-shoota I reckon that, I was just wondering... Thanks for creating an issue for that!

src/openssl/algorithm.cr Outdated Show resolved Hide resolved
@RX14
Copy link
Contributor

RX14 commented Mar 12, 2019

needs a format with bin/crystal tool format

@straight-shoota
Copy link
Member

Please rebase on master. This will allow test_darwin job to run (see #7535).

@bcardiff
Copy link
Member

bcardiff commented Apr 5, 2019

@mniak I would need to ask you for another rebase on master. I'm sorry this is coming 1 month after the last request. Thanks!

@@ -236,6 +236,7 @@ lib LibCrypto
fun md5 = MD5(data : UInt8*, lengh : LibC::SizeT, md : UInt8*) : UInt8*

fun pkcs5_pbkdf2_hmac_sha1 = PKCS5_PBKDF2_HMAC_SHA1(pass : LibC::Char*, passlen : LibC::Int, salt : UInt8*, saltlen : LibC::Int, iter : LibC::Int, keylen : LibC::Int, out : UInt8*) : LibC::Int
fun pkcs5_pbkdf2_hmac = PKCS5_PBKDF2_HMAC(pass : LibC::Char*, passlen : LibC::Int, salt : UInt8*, saltlen : LibC::Int, iter : LibC::Int, digest : EVP_MD, keylen : LibC::Int, out : UInt8*) : LibC::Int
Copy link
Member

Choose a reason for hiding this comment

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

Here a {% if compare_versions(LibSSL::OPENSSL_VERSION, "1.0.0") >= 0 %} is missing to avoid declaring the fun if it is not available.

@bcardiff
Copy link
Member

bcardiff commented Apr 8, 2019

@mniak I will handle the rebase and minor changes I asked and push the changes. Thanks for the patience with this change.

@bcardiff bcardiff merged commit a00c092 into crystal-lang:master Apr 8, 2019
@bcardiff
Copy link
Member

bcardiff commented Apr 8, 2019

Thanks @mniak and all reviewers

@mniak
Copy link
Contributor Author

mniak commented Apr 8, 2019

I'm sorry for the delay. Theses days I'm not having mucha spare time as I started in a new position.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants