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

Support OTP >=24 #253

Open
wants to merge 25 commits into
base: master
Choose a base branch
from
Open

Support OTP >=24 #253

wants to merge 25 commits into from

Conversation

shuieryin
Copy link

No description provided.

@shuieryin shuieryin closed this Aug 25, 2022
@shuieryin shuieryin reopened this Aug 25, 2022
@@ -125,8 +125,7 @@ generate_sig(SaltedPassword, AuthMessage) ->

%% @private
hi(Password, Salt, Iterations) ->
{ok, Key} = pbkdf2:pbkdf2(sha, Password, Salt, Iterations, 20),
Key.
crypto:pbkdf2_hmac(sha, Password, Salt, Iterations, 20).
Copy link
Contributor

@dmsnell dmsnell Sep 10, 2022

Choose a reason for hiding this comment

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

We probably want to follow the pattern in #232 to avoid breaking the library on older releases of OTP. That is, crypto:pbkdf2_hmac is new in OTP 24 and missing from older versions, so if we run this patch on OTP 23 we'll crash for the same reason this fix exists.

Checking for OTP version

-define(OLD_CRYPTO_API, true).
-ifdef(OTP_RELEASE).
-if(?OTP_RELEASE >= 23).
-undef(OLD_CRYPTO_API).
-endif.
-endif.

Conditionally using newer functions

-ifdef(OLD_CRYPTO_API).
hmac(One, Two) -> crypto:hmac(sha, One, Two).
-else.
hmac(One, Two) -> crypto:mac(hmac, sha, One, Two).
-endif.

Copy link
Author

Choose a reason for hiding this comment

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

updated, didn't know crypto in OTP < 24 doesn't have pbkdf2_hmac, thanks for the info

Copy link
Contributor

Choose a reason for hiding this comment

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

My read on the docs is that it came in with OTP 24.2, with crypto 5.0.5

Come to think of it, do we need to check for 24.2 even? I guess OTP 24.0 and OTP 24.1 won't have it either. I'm not sure off-hand how to check for the minor version and I hope we can do it in the macro so this doesn't have to be a runtime check.

Copy link
Author

Choose a reason for hiding this comment

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

I couldn't find a predefined macro returns the minor/patch version

Copy link
Author

Choose a reason for hiding this comment

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

I couldn't find a predefined macro returns the minor/patch version

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.

2 participants