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

Added pkcs5_pbkdf2_hmac to lib_crypto.cr and pkcs5.cr #5021

Conversation

aurimasniekis
Copy link

@aurimasniekis aurimasniekis commented Sep 22, 2017

Added pkcs5_pbkdf2_hmac function to support pbkdf2 with multiple algorithm

@aurimasniekis
Copy link
Author

@aurimasniekis aurimasniekis commented Sep 22, 2017

libcrypto was not found if I understood correctly the CircleCI error...

@aurimasniekis aurimasniekis force-pushed the feature/opensll_pbkdf2_implementation branch from a141b97 to dc525e2 Compare Sep 22, 2017
@sdogruyol
Copy link
Member

@sdogruyol sdogruyol commented Sep 22, 2017

Hey @aurimasniekis, welcome and congrats for your first contribution to Crystal 🎉

@aurimasniekis aurimasniekis force-pushed the feature/opensll_pbkdf2_implementation branch from a377535 to b2a2921 Compare Sep 22, 2017
@aurimasniekis
Copy link
Author

@aurimasniekis aurimasniekis commented Sep 22, 2017

Hi, @sdogruyol thanks your welcoming! I can't figure out why build doesn't want to work. I tried the same solution which helped to build on my local machine but it doesn't work. The most interesting thing that original PKCS5_PBKDF2_HMAC_SHA1 is found but additional PKCS5_PBKDF2_HMAC method is not found then it's in the same lib. Even then in OpenSSL PKCS5_PBKDF2_HMAC_SHA1 wraps a call PKCS5_PBKDF2_HMAC source

@Sija
Copy link
Contributor

@Sija Sija commented Sep 22, 2017

@aurimasniekis
Copy link
Author

@aurimasniekis aurimasniekis commented Sep 22, 2017

@Sija thanks for the suggestion, I will try it out. But why then previous PKCS5_PBKDF2_HMAC_SHA1 works? :D

@Sija
Copy link
Contributor

@Sija Sija commented Sep 22, 2017

@aurimasniekis I guess because it has been changed between OpenSSL versions 0.9 and 1.0, see source for 0.9.8 to see what I mean.

@aurimasniekis aurimasniekis force-pushed the feature/opensll_pbkdf2_implementation branch from 1cb97a4 to 30348ef Compare Sep 22, 2017
@Sija
Copy link
Contributor

@Sija Sija commented Sep 22, 2017

@aurimasniekis FYI, PKCS5_PBKDF2_HMAC was added in openssl/openssl@856640b. Header declaration though was added later in openssl/openssl@777c47a.

@aurimasniekis aurimasniekis force-pushed the feature/opensll_pbkdf2_implementation branch 2 times, most recently from 37f58c5 to cf7835c Compare Sep 22, 2017
@aurimasniekis
Copy link
Author

@aurimasniekis aurimasniekis commented Sep 22, 2017

I modified Makefile to accept custom ENV variable CRYSTAL_FLAGS to pass it to the crystal build so that I can pass in bin/ci flag --link-flags -L/usr/local/opt/openssl/lib and then everything works.

Makefile Outdated
@@ -27,7 +27,7 @@ static ?= ## Enable static linking
O := .build
SOURCES := $(shell find src -name '*.cr')
SPEC_SOURCES := $(shell find spec -name '*.cr')
FLAGS := $(if $(release),--release )$(if $(stats),--stats )$(if $(progress),--progress )$(if $(threads),--threads $(threads) )$(if $(debug),-d )$(if $(static),--static )
FLAGS := $(if $(release),--release )$(if $(stats),--stats )$(if $(progress),--progress )$(if $(threads),--threads $(threads) )$(if $(debug),-d )$(if $(static),--static )$(if $(CRYSTAL_FLAGS),$(CRYSTAL_FLAGS) )
Copy link
Contributor

@Sija Sija Sep 24, 2017

Choose a reason for hiding this comment

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

Splitting $(CRYSTAL_FLAGS) changes into a separate line for sake of clarity might be a good thing here, WDYT @aurimasniekis?

Copy link
Author

@aurimasniekis aurimasniekis Sep 24, 2017

Choose a reason for hiding this comment

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

If you think it's better than yes I could do it. But personally, I do not think many people go into Makefile

Copy link
Contributor

@Sija Sija Sep 24, 2017

Choose a reason for hiding this comment

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

I think so, yes. ATM it's the longest line in the whole file and this change would make it even longer, not nice.

{2**16, 16, "1b345dd55f62a35aecdb9229bc7ae95b"},
{2**16, 32, "1b345dd55f62a35aecdb9229bc7ae95b305a8d538940134627e46f82d3a41e5e"},
].each do |(iterations, key_size, expected)|
OpenSSL::PKCS5.pbkdf2_hmac(:sha1, "password", "salt", iterations, key_size).hexstring.should eq expected
Copy link
Contributor

@Sija Sija Sep 24, 2017

Choose a reason for hiding this comment

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

Could you add samples for other algos as well?

Copy link
Author

@aurimasniekis aurimasniekis Sep 24, 2017

Choose a reason for hiding this comment

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

Yes, I should do it... I was bit lazy...

@@ -12,4 +12,15 @@ describe OpenSSL::PKCS5 do
OpenSSL::PKCS5.pbkdf2_hmac_sha1("password", "salt", iterations, key_size).hexstring.should eq expected
end
end

it "computes pbkdf2_hmac" do
[
Copy link
Contributor

@bew bew Sep 24, 2017

Choose a reason for hiding this comment

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

You could use a tuple here instead of an array

Copy link
Author

@aurimasniekis aurimasniekis Oct 6, 2017

Choose a reason for hiding this comment

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

I will try because I just copied from original test case

@Sija
Copy link
Contributor

@Sija Sija commented Oct 4, 2017

@ysbaddaden
Copy link
Contributor

@ysbaddaden ysbaddaden commented Oct 5, 2017

We don't have a strong policy about supported OpenSSL versions in Crystal, but v0.9.8 is still used and deployed a lot.

If an OpenSSL feature is only available on "recent" OpenSSL release, there should be an OpenSSL version check to enable the feature, just like we do for other OpenSSL features that are only available in v1.0 or v1.1.

@aurimasniekis
Copy link
Author

@aurimasniekis aurimasniekis commented Oct 6, 2017

@Sija sorry, I forgot about this PR after holidays
@ysbaddaden Do you by wrapping with version check would be sufficient?

@aurimasniekis aurimasniekis force-pushed the feature/opensll_pbkdf2_implementation branch from cf7835c to 8b6a2c4 Compare Oct 6, 2017
@aurimasniekis
Copy link
Author

@aurimasniekis aurimasniekis commented Oct 6, 2017

Could someone explain the CircleCI error

Error: formatting 'spec/std/openssl/pkcs5_spec.cr' produced changes

@Sija
Copy link
Contributor

@Sija Sija commented Oct 6, 2017

@aurimasniekis it's a check done by crystal tool format --check. always run crystal tool format before pushing to avoid failing CI because of it.

@aurimasniekis aurimasniekis force-pushed the feature/opensll_pbkdf2_implementation branch from 8b6a2c4 to 7438838 Compare Oct 6, 2017
@aurimasniekis
Copy link
Author

@aurimasniekis aurimasniekis commented Oct 6, 2017

@Sija thanks, fixed

@Willamin
Copy link
Contributor

@Willamin Willamin commented Apr 2, 2018

Is there any more movement on this PR?

@kingsleyh
Copy link

@kingsleyh kingsleyh commented Jun 3, 2018

Hi - I really need this - I've taken the code and patched my library to get it to work - but it would be really awesome if would be integrated into Crystal so I could avoid the patching

@straight-shoota
Copy link
Member

@straight-shoota straight-shoota commented Jan 23, 2021

Resolved by #6975

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.

None yet

8 participants