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

Use luaossl instead of luacrypto. #5

Merged
merged 7 commits into from Mar 15, 2016
Merged

Use luaossl instead of luacrypto. #5

merged 7 commits into from Mar 15, 2016

Conversation

saucisson
Copy link
Contributor

The luacrypto module does not seem to be maintained anymore (mkottman/luacrypto#39). Its current version on luarocks does not accept Lua 5.3, which is annoying.
This pull request is a proposal to use luaossl instead, as it is compatible with newer version of Lua.

Warning: i am a totally newbie in openssl, so the proposed fixes might be totally wrong. I just checked that all tests still pass. Moreover, the luaossl module can throw errors. I did not add code to catch them.

@DorianGray
Copy link
Member

We need to catch the errors at least on decode and return them as nil, message to match the rest of the behavior(to avoid users from having to add pcalls everywhere). More tests on this behavior would be ideal.

…ed keys. Note that jwt will still throw an error as it cannot generate the signature.
@saucisson
Copy link
Contributor Author

From luaossl documentation, neither hmac.* functions nor digest.* ones throw errors.
But the pkey.* functions can throw. They are now wrapped within pcalls, and two tests have been added to check this behavior. Note that there is still an error thrown when the signature cannot be generated.

@DorianGray
Copy link
Member

Awesome, I'm going to test this with a few of my projects to make sure it doesn't fail in weird ways and then merge. This looks great, thanks!

@saucisson
Copy link
Contributor Author

It can cause problems to you and other people using pkeys instead of text keys, as they are expected to be instances of openssl.key in this branch, and crypto.pkey in master.
An easy to fix this is to use crypto to dump keys and give them back to openssl. It is implemented in the latest commits, and tests using luacrypto keys have been added back.

@DorianGray
Copy link
Member

Hmm, if we're going to break compatibility anyways, we should make the interface accept string keys instead of pkey objects. Hide the implementation details from the user. I'll do that unless you get there first.

@saucisson
Copy link
Contributor Author

I have updated the code and tests to only accept strings for keys. An assertion throws an error in case a pkey (from luacrypto or luaossl) is used, as it is an invalid use of the library.

DorianGray added a commit that referenced this pull request Mar 15, 2016
Use luaossl instead of luacrypto.
@DorianGray DorianGray merged commit c735a33 into Olivine-Labs:master Mar 15, 2016
@DorianGray
Copy link
Member

Thanks! That looks really good.

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

3 participants