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

LibCrypto: Replace hashes implementation with OpenSSL #2989

Merged
merged 3 commits into from
Dec 22, 2024

Conversation

devgianlu
Copy link
Contributor

@devgianlu devgianlu commented Dec 20, 2024

This PR aims to bring OpenSSL into the project as briefly discussed on Discord by replacing all hashes implementation with OpenSSL.

Additional discussion here.

It required multiple changes:

  • Make hashes non-copiable because they contain a heap allocated pointer (allocating EVP_MD_CTX manually has been deprecated)
  • Reference hash classes via NonnullOwnPtr only (because they are non-copiable)
  • Drop all existing hashes implementations
  • Use the OpenSSLHashFunction base class to implement the same hashes

I was not able to come up with a way to divide this PR into more commits without increasing the amount of changes.

Overall this doesn't break anything and drastically reduces the amout of code. I suspect it also increases performance (that will probably be more obvious with stuff like RSA).

@devgianlu devgianlu requested a review from alimpfard as a code owner December 20, 2024 08:28
Copy link
Contributor

@alimpfard alimpfard left a comment

Choose a reason for hiding this comment

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

Assuming everyone's on board with using openssl, this looks very nice.

A couple stylistic/nitpicky things:

Libraries/LibCrypto/Hash/OpenSSLHashFunction.h Outdated Show resolved Hide resolved
Libraries/LibCrypto/Hash/OpenSSLHashFunction.h Outdated Show resolved Hide resolved
Libraries/LibCrypto/Hash/OpenSSLHashFunction.h Outdated Show resolved Hide resolved
@rmg-x
Copy link
Contributor

rmg-x commented Dec 20, 2024

Related issue: #1834 (for linking)

Libraries/LibCrypto/Hash/OpenSSLHashFunction.h Outdated Show resolved Hide resolved
Libraries/LibCrypto/Hash/BLAKE2b.h Outdated Show resolved Hide resolved
Libraries/LibCrypto/Hash/HashManager.h Show resolved Hide resolved
Libraries/LibTLS/TLSv12.cpp Show resolved Hide resolved
Add OpenSSL with vcpkg and link with LibCrypto using CMake.

Also added a placeholder GN setup.
This abstract class allows implementing hashes backed by OpenSSL with
very few lines of code, see next commit.
This required multiple changes:
- Make hashes non-copiable because they contain a heap allocated pointer
- Reference classes via `NonnullOwnPtr` only (they are non-copiable)
- Drop all existing hashes implementations
- Use the `OpenSSLHashFunction` base class to implement the same hashes

I was not able to come up with a way to divide this commit into multiple
without increasing the amount of changes.

Nothing breaks with this commit!
@devgianlu
Copy link
Contributor Author

Unrelated CI failure :(

@devgianlu devgianlu closed this Dec 21, 2024
@devgianlu devgianlu deleted the openssl branch December 21, 2024 11:06
@devgianlu devgianlu restored the openssl branch December 21, 2024 11:06
@devgianlu devgianlu reopened this Dec 21, 2024
@alimpfard alimpfard merged commit 89061dd into LadybirdBrowser:master Dec 22, 2024
15 of 16 checks passed
@alimpfard
Copy link
Contributor

Let's dew it

@devgianlu devgianlu deleted the openssl branch December 22, 2024 17:54
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.

4 participants