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

Crypto modern api #18

Merged
merged 9 commits into from
Feb 20, 2025
Merged

Crypto modern api #18

merged 9 commits into from
Feb 20, 2025

Conversation

NelsonVides
Copy link
Collaborator

@NelsonVides NelsonVides commented Feb 17, 2025

Solves #14, and also implements the SHA3 family of algorithms, which turned out to be a low hanging fruit after the API migration.

@codecov-commenter
Copy link

codecov-commenter commented Feb 17, 2025

Codecov Report

Attention: Patch coverage is 63.69427% with 57 lines in your changes missing coverage. Please review.

Project coverage is 63.31%. Comparing base (9055d60) to head (5e1e46d).

Files with missing lines Patch % Lines
c_src/fast_pbkdf2.c 63.63% 56 Missing ⚠️
src/fast_pbkdf2.erl 66.66% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##             main      #18       +/-   ##
===========================================
- Coverage   89.94%   63.31%   -26.63%     
===========================================
  Files           2        2               
  Lines         179      199       +20     
  Branches       13       44       +31     
===========================================
- Hits          161      126       -35     
- Misses         18       73       +55     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@NelsonVides NelsonVides linked an issue Feb 18, 2025 that may be closed by this pull request
@NelsonVides NelsonVides marked this pull request as ready for review February 18, 2025 10:46
@NelsonVides NelsonVides force-pushed the crypto_modern_api branch 2 times, most recently from 71c37f3 to a144c64 Compare February 19, 2025 07:28
@NelsonVides
Copy link
Collaborator Author

There's some progress, now the failure is well handled and doesn't segfault nor aborts. But the failure's still there 🥲

Copy link
Member

@chrzaszcz chrzaszcz left a comment

Choose a reason for hiding this comment

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

I reviewed partially, because the code might change. Everything looks really good so far (I checked more than half of the C code).

This is the main rework. I initially wrote the standard functions for
SHA1, and only later refactored to extract all SHA1 specifics into macro
parameters, so that all other algorithms could be generated by the
macro.

It follows the same idea that the previous code, however it is not a
step-by-step refactoring but a rewrite from scratch.
There was an issue in MacOS where in 15.x the dynamic linker would give
preference to the system's cached `libboringssl.dylib`, instead of the
OpenSSL's one put in the path. For the dynamic linker to resolve the
right library correctly, the static linker would need to add strict
resolution rules by being provided more strict flags.
Copy link
Contributor

@DenysGonchar DenysGonchar left a comment

Choose a reason for hiding this comment

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

looks good to me

Copy link
Member

@chrzaszcz chrzaszcz left a comment

Choose a reason for hiding this comment

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

Looks good 👌 I didn't analyse the algorithm itself that much, but the transformation from the previous code looks good to me.

@DenysGonchar DenysGonchar merged commit 9040257 into main Feb 20, 2025
7 checks passed
@DenysGonchar DenysGonchar deleted the crypto_modern_api branch February 20, 2025 17:40
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.

OpenSSL3 deprecation warnings
4 participants