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 in-house MD5 with OpenSSL #8272

Closed
wants to merge 1 commit into from

Conversation

jeffro256
Copy link
Contributor

@jeffro256 jeffro256 commented Apr 20, 2022

Pros:

  • ~4x speed up for MD5 hashes
  • ~800 less lines of code
  • Marginally smaller build sizes

Cons:

  • Less control over implementation
  • According to @hyc, there is a chance that some OpenSSL implementations will not support MD5

Benchmarks:

Specs:

Ubuntu 20.04
Intel(R) Core(TM) i7-8750H CPU @ 2.20GHz
31 GB RAM

Hashing 1M strings 1000 characters long:

Hashing using OpenSSL Implementation:  2651850801 ns
Hashing using EPEE Implementation:    11596632481 ns
Speedup: 4.37x

Hashing 1M strings 100 characters long:

Hashing using OpenSSL Implementation:  550373610 ns
Hashing using EPEE Implementation:    2146028452 ns
Speedup: 3.90x

Conclusions:

This could help boost wallet sync times where many RPC requests are made and authenticated over a short period of time.

Pros:
* ~4x speed up for MD5 hashes
* ~800 less lines of code
* Marginally smaller build sizes when using shared libs

Cons:
* Less control over implementation

Benchmarks:

Specs:

    Ubuntu 20.04
    Intel(R) Core(TM) i7-8750H CPU @ 2.20GHz
    31 GB RAM

Hashing 1M strings 1000 characters long:

    Hashing using OpenSSL Implementation:  2651850801 ns
    Hashing using EPEE Implementation:    11596632481 ns
    Speedup: 4.37x

Hashing 1M strings 100 characters long:

    Hashing using OpenSSL Implementation:  550373610 ns
    Hashing using EPEE Implementation:    2146028452 ns
    Speedup: 3.90x

Conclusions:
This could help boost wallet sync times where many RPC requests are made and authenticated over a short period of time.
@jeffro256
Copy link
Contributor Author

jeffro256 commented Apr 20, 2022

Here is my benchmark code. To compile you need the following files:

  • memwipe.h and memwipe.c
  • md5_l.h and md5_l.inl
  • md5global.h
  • hmac-md5.h
  • And obviously the OpenSSL libraries

I compiled with: g++ main.cpp memwipe.c -o benchmark -l crypto

@hyc
Copy link
Collaborator

hyc commented Apr 20, 2022

Introducing this kind of dependency is a bit dicy, since MD5 support is optional in OpenSSL, and since MD5 has been deprecated in favor of SHA for a few years already, it's not uncommon for sites to omit it from their OpenSSL builds.

Authentication traffic shouldn't be a large percentage of a wallet RPC session, so how much does this really save in practice?

@jeffro256
Copy link
Contributor Author

it's not uncommon for sites to omit it from their OpenSSL builds

Not trying to be contrarian, but do you have any examples of builds of OpenSSL for platforms we support which omit MD5?

since MD5 has been deprecated in favor of SHA for a few years already, it's not uncommon for sites to omit it from their OpenSSL builds.

For this reason, shouldn't we be moving away from MD5 digest authentication anyways? There are a ton of more secure authentication protocols with nearly as much, if not more, support than MD5 Digest Authentication. I'm hoping that this is not permanent, it would make happier the quicker we ditch MD5 authentication, or at least require digest authentication to happen over SSL.

Authentication traffic shouldn't be a large percentage of a wallet RPC session, so how much does this really save in practice?

I haven't bench-marked that yet, but it's probably not too much. What do you think would be the most realistic way to stress test this?

@hyc
Copy link
Collaborator

hyc commented Apr 21, 2022

Not trying to be contrarian, but do you have any examples of builds of OpenSSL for platforms we support which omit MD5?

No, because as I said it's about sites, not platforms.

I haven't bench-marked that yet, but it's probably not too much. What do you think would be the most realistic way to stress test this?

Run a private node, sync'd but disconnected from mainnet, and aim a wallet at it. If you use monero-wallet-rpc you ought to be able to automate the entire sequence to eliminate run variations.

@jeffro256
Copy link
Contributor Author

Closing this because MD5 is deprecated since OpenSSL 3.0, and like @hyc said, some people don't build their sites with MD5.

@jeffro256 jeffro256 closed this Apr 22, 2022
@jeffro256 jeffro256 deleted the openssl_md5 branch April 22, 2022 20:02
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