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

Replace openssl by crates num-bigint, sha2 #317

Merged
merged 2 commits into from
Aug 26, 2023
Merged

Conversation

Patrik-Stas
Copy link
Contributor

@Patrik-Stas Patrik-Stas commented Jul 12, 2021

OpenSSL in libvcx is increasing build complexity and has occasionally produced build issues on some platforms.
However there's only 2 use cases for OpenSSL - big numbers and sha256. These problems has been implemented in pure Rust already.
Big numbers and sha hashing is used for encoding credential attributes and is being tested by following tests:

    aries::handlers::issuance::issuer::utils::tests::test_encode_empty_field
    aries::handlers::issuance::issuer::utils::tests::test_encode_with_aries_format_several_attributes_success
    aries::handlers::issuance::issuer::utils::tests::test_encode_with_mixed_format_several_attributes_success
    aries::handlers::issuance::issuer::utils::tests::test_encode_with_new_format_one_attribute_success
    aries::handlers::issuance::issuer::utils::tests::test_encode_with_new_format_several_attributes_success
    aries::handlers::issuance::issuer::utils::tests::test_encode_with_one_attribute_success
    aries::handlers::issuance::issuer::utils::tests::test_encode_with_several_attributes_success
    utils::openssl::test::test_encoding

Signed-off-by: Patrik Stas patrik.stas@absa.africa

@Patrik-Stas Patrik-Stas requested a review from a team as a code owner July 12, 2021 12:21
@Patrik-Stas Patrik-Stas changed the title Replace openssl by crates 'num-bigint', 'sha2' Replace openssl by crates num-bigint, sha2 Jul 12, 2021
@Patrik-Stas Patrik-Stas force-pushed the dependencies/remove-openssl branch 2 times, most recently from ed28943 to d51da45 Compare July 12, 2021 12:26
@codecov-commenter
Copy link

codecov-commenter commented Jul 12, 2021

Codecov Report

Merging #317 (1638231) into main (cfea9f7) will increase coverage by 0.01%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #317      +/-   ##
==========================================
+ Coverage   44.45%   44.46%   +0.01%     
==========================================
  Files         417      417              
  Lines       28975    28964      -11     
  Branches     6176     6178       +2     
==========================================
  Hits        12880    12880              
+ Misses      12296    12285      -11     
  Partials     3799     3799              
Flag Coverage Δ
unittests-aries-vcx 44.46% <100.00%> (+0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Changed Coverage Δ
aries_vcx/src/utils/validation.rs 0.00% <ø> (ø)
aries_vcx/src/utils/openssl.rs 39.28% <100.00%> (+14.28%) ⬆️

... and 3 files with indirect coverage changes

Copy link
Contributor

@mirgee mirgee left a comment

Choose a reason for hiding this comment

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

LGTM! 👍 However, there are many newly redundant references to openssl in Android and iOS build scripts, dockerfiles, build.rs, etc. now. I would prefer to remove these as part of this PR to avoid letting the dead code rot indefinitely.

@Patrik-Stas
Copy link
Contributor Author

LGTM! 👍 However, there are many newly redundant references to openssl in Android and iOS build scripts, dockerfiles, build.rs, etc. now. I would prefer to remove these as part of this PR to avoid letting the dead code rot indefinitely.

agree, will do

@gmulhearn
Copy link
Contributor

@Patrik-Stas just looking at old PRs. is this PR still relevant? libvcx doesn't depend on openssl anymore. but aries-vcx does, should it be migrated?

@bobozaur
Copy link
Contributor

Reviving this. We should get back to it at some point in the near future, after the refactors are done.

gmulhearn
gmulhearn previously approved these changes Aug 4, 2023
mirgee
mirgee previously approved these changes Aug 25, 2023
Comment on lines 47 to 48
sha2 = "0.9.5"
num-bigint = "0.4.0"
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we use the latest releases?

Signed-off-by: Patrik Stas <patrik.stas@absa.africa>
Signed-off-by: Patrik Stas <patrik.stas@absa.africa>
@Patrik-Stas Patrik-Stas dismissed stale reviews from mirgee and gmulhearn via 1638231 August 26, 2023 16:05
@mirgee mirgee merged commit 091529f into main Aug 26, 2023
30 checks passed
@mirgee mirgee deleted the dependencies/remove-openssl branch August 26, 2023 18:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants