Add option to dynamically load libcrypto.so.* #1603

ueno · 2023-11-06T05:48:38Z

As discussed on #1599, this is a strawman proposal to make the common cryptographic algorithms (AES, SHA2, SHA3, and RNG) switchable to avoid OpenSSL dependency at package installation time. The applications can replace the implementation by providing custom callback functions for with OQS_AES_set_callbacks, OQS_SHA2_set_callbacks,
OQS_SHA3_set_callbacks, and OQS_SHA3_x4_callbacks.

This also adds OQS_DLOPEN_OPENSSL build option to use OpenSSL through dynamically loaded libcrypto.so.* with dlopen, instead of linking to the library at build time.

Fixes #1599

Does this PR change the input/output behaviour of a cryptographic algorithm (i.e., does it change known answer test values)? (If so, a version bump will be required from x.y.z to x.(y+1).0.)
Does this PR change the list of algorithms available -- either adding, removing, or renaming? Does this PR otherwise change an API? (If so, PRs in fully supported downstream projects dependent on these, i.e., oqs-provider and OQS-OpenSSH will also need to be ready for review and merge by the time this is merged.)

baentsch · 2023-12-10T07:01:27Z

Thanks for the proposal. Conceptually, I (think I) get it, but wonder why all tests (using it, i.e., not Windows :) fail on this code: It seems this PR basically delays loading OpenSSL symbols -- but then, why does the hash (and AES) self-test tests/test_hash.py fail all AES and SHA2 tests? SHA3 tests pass. Also, all (PQ) algorithm tests fail. Some further explanations (or updates) would be appreciated as I don't find the time right now to debug into this.

SWilson4 · 2024-01-26T20:14:31Z

@ueno Are you interested in continuing to work on this? Just wondering whether to leave this PR open for further development or to close it. Either way, it will remain linked to the issue and can be used as a proof-of-concept / starting point for future work.

ueno · 2024-01-27T00:29:07Z

@SWilson4 sorry for the inactivity; yes, I'm still interested in working on this, targeting the end of this quarter.

ueno · 2024-03-07T05:06:21Z

@baentsch @SWilson4 sorry for the delay; I've managed to make this PR pass the GitHub CI on Linux and macOS. Could you take a look when you have time?

.CMake/alg_support.cmake

CMakeLists.txt

src/common/aes/aes.h

src/oqsconfig.h.cmake

baentsch

Thanks for this contribution, @ueno ! I still think I understand how it works, let all our CI "loose" on it and have a few initial comments from a first review pass through. Just to set expectations: Final review and merge may not happen very quickly as we're trying to get a new liboqs release out the door and this isn't on the feature list for the release.

CONFIGURE.md

baentsch · 2024-03-08T09:58:50Z

Thanks again, @ueno -- LGTM now. Now one "logical" question pertaining downstream use: Will this feature work without problems if someone activates the new option "OQS_DLOPEN_OPENSSL" while using liboqs in oqsprovider (running within openssl) or any other integration (say, nginx)? Conceptually, I'd say so, but do you see any chance (well, risk, really) that this "picks up" a library different from the one operating the provider at the same time, creating any form of havoc or "strange behaviours"? Shall we add tests for this in the downstreams?

ueno · 2024-03-11T03:54:36Z

I guess we could add such downstream integration tests through packit. So far, I've created a copr repository which hosts modified liboqs package with this changeset. With the package installed on Fedora 39, I confirm nginx is working with hybrid key exchange (x25519_kyber768) following the test scenario provided by @beldmit.

ueno · 2024-03-26T13:54:19Z

@baentsch now that 0.10.0 is out, is there anything specific we can do to move this forward? As mentioned, my local testing with oqsprovider and nginx/curl didn't show any problems.

beldmit · 2024-03-27T16:44:54Z

I confirm I see no problems with oqsprovider. The only suspicion I have is related to multithreading use cases

baentsch

@baentsch now that 0.10.0 is out, is there anything specific we can do to move this forward? As mentioned, my local testing with oqsprovider and nginx/curl didn't show any problems.

As stated earlier, this LGTM. With 0.10.0 out let's move this towards merge with the second review: @SWilson4 would you have time? Thanks @beldmit for your test/feedback.

According to the manual page, EVP_MD_CTX_destroy has been renamed to EVP_MD_CTX_free in OpenSSL 1.1.0 and only provided as a compatibility macro in later OpenSSL releases: https://www.openssl.org/docs/man1.1.1/man3/EVP_MD_CTX_free.html Signed-off-by: Daiki Ueno <dueno@redhat.com>

Throughout the code base, liboqs uses pthread_once for one-shot initialization and falls back to thread-unsafe code if it is not supported nor enabled on the system. For consistency and to remove additional dependency on OpenSSL, this switches the use of CRYPTO_THREAD_run_once with that. Signed-off-by: Daiki Ueno <dueno@redhat.com>

This allows applications to replace the implementation of common cryptographic algorithms at runtime, by setting callback functions for each operations with OQS_AES_set_callbacks, OQS_SHA2_set_callbacks, OQS_SHA3_set_callbacks, and OQS_SHA3_x4_callbacks. Those functions may be called once before OQS_init; otherwise the default implementation will be used. Signed-off-by: Daiki Ueno <dueno@redhat.com>

This adds OQS_DLOPEN_OPENSSL build option to use OpenSSL through dynamically loaded libcrypto.so.* with dlopen, instead of linking to the library at build time. That way the applications could use their favorite implementation of common cryptographic primitives without pulling in the OpenSSL as a hard dependency. Signed-off-by: Daiki Ueno <dueno@redhat.com>

SWilson4 · 2024-04-03T15:44:59Z

For some reason I'm not able to view logs for the failing CI jobs: only a message that says "this job failed". I'm going to rerun them and see if that helps.

SWilson4 · 2024-04-03T16:44:55Z

Functionality-wise, this looks OK to me. Thanks, @ueno!

It would be nice to test the OQS_*_set_callbacks API in CI somehow. Is this something that's feasible to do here, or is better done in another setting?

ueno · 2024-04-03T23:25:50Z

@SWilson4 That is a good point. My plan was to add a Nettle backend and run CI there, once their SHAKE interface has settled. Additionally/alternatively I could add a non-exhaustive test here, wrapping the existing callbacks.

SWilson4 · 2024-04-04T00:00:53Z

@SWilson4 That is a good point. My plan was to add a Nettle backend and run CI there, once their SHAKE interface has settled. Additionally/alternatively I could add a non-exhaustive test here, wrapping the existing callbacks.

Cool! It would be great to see another liboqs integration. I think it would be a good idea to also have some sort of testing in liboqs CI so that we don't have to monitor an external repo. Perhaps we can set up a Docker image containing nettle and run some tests using it as a common code provider. Or maybe this would be better suited to oqs-demos---although it's sort of the reverse order of our current demos (i.e. using another project in liboqs as opposed to vice versa). Thoughts on this, @baentsch?

At any rate, those options are both (imo) out of scope for this PR, so for now please add the non-exhaustive test you mentioned to get it landed.

This adds tests that exercise OQS_*_set_callbacks by overriding one of the function of each and ensuring the wrapper function is called. Signed-off-by: Daiki Ueno <dueno@redhat.com>

ueno · 2024-04-04T02:08:51Z

Sure, b60ba69 is an attempt to add tests for OQS_*_set_callbacks.

SWilson4

Looks good to me. Thank you very much for this contribution!

ueno force-pushed the wip/crypto-backend branch from 76d86e1 to f892a4f Compare November 6, 2023 05:55

ueno force-pushed the wip/crypto-backend branch 11 times, most recently from 6ea299d to 9a26324 Compare March 7, 2024 04:44

ueno changed the title ~~Make common algorithms implementation pluggable~~ Add option to dynamically load libcrypto.so.* Mar 7, 2024

ueno marked this pull request as ready for review March 7, 2024 05:06

ueno requested a review from dstebila as a code owner March 7, 2024 05:06