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

Jump to bottom

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

Closed
wants to merge 1 commit into from
Closed

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

wants to merge 1 commit into from

Conversation

aurimasniekis
Copy link

Added pkcs5_pbkdf2_hmac function to support pbkdf2 with multiple algorithm

@aurimasniekis
Copy link
Author

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 September 22, 2017 15:54
@sdogruyol
Copy link
Member

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 September 22, 2017 16:22
@aurimasniekis
Copy link
Author

aurimasniekis commented Sep 22, 2017 •
edited

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 commented Sep 22, 2017

@aurimasniekis See samr7/vanitygen#45.

@aurimasniekis
Copy link
Author

@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 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 September 22, 2017 16:43
@Sija
Copy link
Contributor

Sija commented Sep 22, 2017 •
edited

@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 September 22, 2017 17:13
@aurimasniekis
Copy link
Author

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.

Sija
Sija reviewed Sep 24, 2017
View reviewed changes
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

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

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 •
edited

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.

Sija
Sija reviewed Sep 24, 2017
View reviewed changes
spec/std/openssl/pkcs5_spec.cr Outdated
{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

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

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...

bew
bew reviewed Sep 24, 2017
View reviewed changes
spec/std/openssl/pkcs5_spec.cr Outdated
@@ -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

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

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 commented Oct 4, 2017

@aurimasniekis 🏓

@ysbaddaden
Copy link
Contributor

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

@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 October 6, 2017 11:06
@aurimasniekis
Copy link
Author

Could someone explain the CircleCI error

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

@Sija
Copy link
Contributor

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 October 6, 2017 11:35
@aurimasniekis
Copy link
Author

@Sija thanks, fixed

@Willamin
Copy link
Contributor

Willamin commented Apr 2, 2018

Is there any more movement on this PR?

@kingsleyh
Copy link

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

This was referenced Oct 21, 2018
Closed
Merged
@straight-shoota
Copy link
Member

Resolved by #6975

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Sija Sija Sija left review comments

@bew bew bew left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
8 participants
@aurimasniekis @sdogruyol @Sija @ysbaddaden @Willamin @kingsleyh @straight-shoota @bew