Allow the user to build oqs-provider as a static library.

[ghost]

This commit removes the SHARED argument of the add_library. By doing so, we let the user choose the build type of library.

By default, CMake will build a static library. Thus, BUILD_SHARED_LIBS must be used to switch to a shared library.

oqs-provider as a static library allows us to use the provider without having to store its shared library somewhere. In addition, it happens that some operating systems prohibit the use of dlopen/dlsym.

To load oqs-provider when it is embedded into a library of a binary, one can use the OSSL_PROVIDER_add_builtin API from OpenSSL 3.

[baentsch]

Thanks for the contribution! Yes, having it be one DLL/SO was the intention (by default) -- I'm not sure whether this works seamlessly on all platforms, though. Let's see what CI says... Please also check out the two nits in separate comments.

[ghost]

I've fixed the CI (I forgot the GitHub Actions part).

[baentsch]

The latest discussion above on the use of the static/shared/module moniker had me review also your rationale for the PR:

it happens that some operating systems prohibit the use of dlopen/dlsym

Which OS are you referring-to? Wouldn't such OS feature completely disable the purpose of openssl providers (as a dynamically loadable library)? I can understand this argument

one can use the OSSL_PROVIDER_add_builtin API from OpenSSL 3.

for wanting to build oqsprovider as a static lib: That way its should be possible to completely embed it into an application without (wanting) it being dynamically linked in: That definitely eases distribution of binaries with PQ capabilities inside.

But if this is the reason for this PR (?), the default behaviour of cmake (building static) clashes with the default purpose of a provider (being dynamically loadable).

What about this proposal then: We'd add a specific build option, say "OQS_PROVIDER_BUILD_STATIC" that --when set-- changes "MODULE" to "STATIC" in

oqs-provider/oqsprov/CMakeLists.txt

Line 34 in 6b34839

add_library(oqsprovider SHARED ${PROVIDER_SOURCE_FILES})

(and probably also resets/warns of set "BUILD_SHARED_LIBS" option)?

[ghost]

Which OS are you referring-to? Wouldn't such OS feature completely disable the purpose of openssl providers (as a dynamically loadable library)? I can understand this argument

I was referring to iOS on PAC-enabled devices (basically all the aarch64 mac and iPhones since the XS).

But if this is the reason for this PR (?), the default behaviour of cmake (building static) clashes with the default purpose of a provider (being dynamically loadable).

What about this proposal then: We'd add a specific build option, say "OQS_PROVIDER_BUILD_STATIC" that --when set-- changes "MODULE" to "STATIC" in

oqs-provider/oqsprov/CMakeLists.txt

Line 34 in 6b34839

add_library(oqsprovider SHARED ${PROVIDER_SOURCE_FILES})

(and probably also resets/warns of set "BUILD_SHARED_LIBS" option)?

I totally agree with you on that point. Changing the default behavior is indeed not a good idea.

So I implemented OQS_PROVIDER_BUILD_STATIC in e74d9da, and I removed the BUILD_SHARED_LIBS=ON flags I had previously set.

About the MODULE type, I can do it in another PR (I tried it here, and it failed to compile, so I guess it needs additional investigation).

[baentsch]

About the MODULE type, I can do it in another PR (I tried it here, and it failed to compile, so I guess it needs additional investigation).

ACK. Makes sense. Pushed your PR to the OQS CI test realm to see that everything's OK (apart from the CI nit above). Thanks again for the addition. Please consider adding yourself to the Contributors section at the end of the README.md.

[ghost]

About the MODULE type, I can do it in another PR (I tried it here, and it failed to compile, so I guess it needs additional investigation).

ACK. Makes sense. Pushed your PR to the OQS CI test realm to see that everything's OK (apart from the CI nit above). Thanks again for the addition. Please consider adding yourself to the Contributors section at the end of the README.md.

Ok!

I added two additional CI targets to test the OQS_PROVIDER_BUILD_STATIC=ON on Linux and macOS.

(and added myself in the README.md in 9eaa075).

[ghost]

Added a SKIP_TESTS parameter in the CircleCI configuration.
Since the two targets I added build oqs-provider statically, tests cannot be run.

Done in https://github.com/open-quantum-safe/oqs-provider/compare/9eaa075bdc2d033e9eb32d9fe6f40d1d5efa55e6..c26e1bf90c8a5a7f0d6f6666c2d41b915ebd249f.

[baentsch]

Added a SKIP_TESTS parameter in the CircleCI configuration.
Since the two targets I added build oqs-provider statically, tests cannot be run.

Agreed, tests as-is cannot be run with a statically built oqsprovider as they currently still rely on the dynamic-module-by-config load mechanism (OSSL_LIB_CTX_load_config) to activate oqsprovider. But I'd argue the above/the latest commits don't seem an ideal solution: That way the tests for the new configuration only check whether it can build but not whether oqsprovider can run in this config.

A more complete solution (not requiring any changes to any CI logic, i.e., performing functional testing) seems to be possible by using the new config variable as a #define in the test cases to call OSSL_PROVIDER_add_builtin and in the tests/CMakeLists.txt to suitably link the (then static) lib in to deliver the required init symbol. The code for the legacy provider seems to utilize the define "STATIC_LEGACY" for exactly this purpose and could serve as an example how to do this...

[baentsch]

@thb-sb When re-basing your PR, please be sure to update the new CONFIGURE.md with documentation for "OQS_PROVIDER_BUILD_STATIC".

[ghost]

@thb-sb When re-basing your PR, please be sure to update the new CONFIGURE.md with documentation for "OQS_PROVIDER_BUILD_STATIC".

Thanks for the reminder! Done :)

But I'd argue the above/the latest commits don't seem an ideal solution: That way the tests for the new configuration only check whether it can build but not whether oqsprovider can run in this config.

You're totally right, that SKIP_TESTS thing wasn't a good idea.

A more complete solution (not requiring any changes to any CI logic, i.e., performing functional testing) seems to be possible by using the new config variable as a #define in the test cases to call OSSL_PROVIDER_add_builtin and in the tests/CMakeLists.txt to suitably link the (then static) lib in to deliver the required init symbol. The code for the legacy provider seems to utilize the define "STATIC_LEGACY" for exactly this purpose and could serve as an example how to do this...

This is a far more better idea! So I did it: in test_common.h, I declared the following method:

void load_oqs_provider(OSSL_LIB_CTX *libctx, const char *modulename, const char *configfile);

Then, in test/CMakeLists.txt I implemented the following function:

function(targets_set_static_provider)                                            
  foreach(target ${ARGN})                                                        
    target_compile_definitions(${target} PRIVATE "OQS_PROVIDER_STATIC")          
    target_link_libraries(${target} PRIVATE oqsprovider)                         
  endforeach()                                                                   
endfunction() 

This function is called if OQS_PROVIDER_BUILD_STATIC is set.

Finally, in test/test_common.c, the implementation of load_oqs_provider depends on the definition of OQS_PROVIDER_STATIC: if not set, then oqs-provider is loaded the same way as before, using the shared module. If set, OSSL_PROVIDER_add_builtin is instead used.

Note that OSSL_PROVIDER_add_builtin is called with the OSSL_provider_init symbol name, defined in oqsprov.
This only works if no other provider is statically linked, otherwise we may end up with a clash.
So, I was thinking that after this PR and the one about replacing SHARED with MODULE, I could submit a PR to let the user specify the init function name with a CMake option. What do you think? :)

[baentsch]

Thanks for these updates -- exactly as I'd hoped it should work.

I could submit a PR to let the user specify the init function name with a CMake option. What do you think? :)

With tongue-in-cheek, I'd say "Yes, let's hang them by the options" :), but more seriously, I'd rather suggest (best already in this PR) setting (and using) a fixed name "oqs_provider_init" if "OQS_PROVIDER_STATIC" is set exactly to avoid

if no other provider is statically linked, otherwise we may end up with a clash.

[ghost]

Thanks for these updates -- exactly as I'd hoped it should work.

I could submit a PR to let the user specify the init function name with a CMake option. What do you think? :)

With tongue-in-cheek, I'd say "Yes, let's hang them by the options" :), but more seriously, I'd rather suggest (best already in this PR) setting (and using) a fixed name "oqs_provider_init" if "OQS_PROVIDER_STATIC" is set exactly to avoid

if no other provider is statically linked, otherwise we may end up with a clash.

Ok, and done in c2251e3!

I also updated the file CONFIGURE.md.

[baentsch]

What has been done to trigger all these (end-of-line) code changes? Problem is that in various places (e.g., oqsprov.c) they clash with our code-generating logic (see e.g. this CI run: If anything gets changed between "OQS_TEMPLATE_FRAGMENT...START" and "...END" brackets, the corresponding changes (also) must be present in the corresponding generator code in oqs-template (to survive a next code generation round, i.e., when integrating a new iteration of liboqs). Please see the first step in https://github.com/open-quantum-safe/oqs-provider/wiki/Release-process. Thus, can I ask you to also update the template files with these end-of-line changes (eliminating spaces?), run python3 oqs-template/generate.py and re-build/test? An update is anyway necessary to address the merge conflict introduced by your previous contribution... :) Thanks in advance.

[baentsch]

@thb-sb Do you have time to complete the PR as per the above or shall I do this? I might have to do another PR though as I probably don't have push rights to your repo, right?

[ghost]

@thb-sb Do you have time to complete the PR as per the above or shall I do this? I might have to do another PR though as I probably don't have push rights to your repo, right?

Yes, I'll have time! I'm on vacation this week, but next week I'll finish this PR

[ghost]

I'm back!

The conflicting changes with the generated code were produced by my vim (I configured it so that it removes leading and trailing whitespaces on lines), so I used nano to add my changes to oqsprov/oqsprov.c `:).

Please see the first step in https://github.com/open-quantum-safe/oqs-provider/wiki/Release-process. Thus, can I ask you to also update the template files with these end-of-line changes (eliminating spaces?), run python3 oqs-template/generate.py and re-build/test?

I'll do that as soon as possible, in another PR! (edit: done in #215)

An update is anyway necessary to address the merge conflict introduced by your previous contribution... :) Thanks in advance.

I rebased this commit on top of main. Now I'm waiting for the CI to be green ✅

[baentsch]

I now follow(ed) the logic to "load" the provider when statically linked -- but at least for oqs_test_endecode there's something missing: We load the provider for two contexts in that test -- and the new function load_oqs_provider does it only for one... so I'm afraid that initialization has to be done differently (or twice with different contexts).

[ghost]

We load the provider for two contexts in that test -- and the new function load_oqs_provider does it only for one... so I'm afraid that initialization has to be done differently (or twice with different contexts).

I think I just missed the other LIB_CTX… we could simply call load_oqs_provider on both contexts, couldn't we?

[baentsch]

we could simply call load_oqs_provider on both contexts, couldn't we?

Yes, that would allow ctest to pass. The various openssl-based tests in scripts/runtests.sh that won't be able to get to pass, though. Agreed, they're pointless for a statically built oqsprovider -- but then there should be a mechanism to disable them when "OQS_PROVIDER_BUILD_STATIC=ON".

[ghost]

Hum, I think runtests.sh should exit at the first error. Right now it's not the case:

scripts/runtests.sh: 152: [: -eq: unexpected operator
/bin/bash: line 2: [: too many argument

(see https://app.circleci.com/pipelines/github/open-quantum-safe/oqs-provider/683/workflows/29cf80aa-76a0-4174-ad37-c5bbaaf1ae75/jobs/1457?invite=true#step-106-11)

Perhaps we should add a set -e at the top of runtests.sh

I'm still working on it!

[baentsch]

I'm still working on it!

ACK. Please let me know when Ready for Review. While you're at it, please consider adding a line to the RELEASE.md highlighting this new feature.

Pending further changes

[ghost]

Okay, so I tried to leave runtests.sh untouched, but I ran into the following issue (CircleCI job):

Internal ctest changing into directory: /Users/distiller/project/_build
Test project /Users/distiller/project/_build
    Start 1: oqs_signatures
1/5 Test #1: oqs_signatures ...................Subprocess aborted***Exception:   0.04 sec
error registering dilithium2 with no hash
/Users/distiller/project/test/test_common.c:47: OpenSSL internal error: OSSL_PROVIDER_available(libctx, modulename)

From my understanding, I fell into the following case:

oqs-provider/scripts/runtests.sh

Lines 139 to 143 in 5250576

	# Disable testing for version 3.0.1: Buggy as hell:
	if "${OPENSSL_APP}" version | grep -q "OpenSSL 3.0.1"; then
	echo "Skipping testing of buggy OpenSSL 3.0.1"
	exit 0
	fi

Thus I'm wondering if having this check in both CircleCI's config.yml and runtests.sh is actually a good idea…

What do you think @baentsch ?

[baentsch]

From my understanding, I fell into the following case:

Well, Yes, I by now have no real trust in proper operation of OpenSSL 3.0.[0-8] (pertaining to providers :).

Thus I'm wondering if having this check in both CircleCI's config.yml and runtests.sh is actually a good idea…

I'm afraid that's then necessary, indeed.

But what irks me also is that the CI task (still) uses OpenSSL1.1.1 to build liboqs. Such error (not finding OpenSSL111 symbols "baked into" liboqs (and thus oqsprovider) may also be a reason behind the failure to load the provider. This was the reason for #212. Looking back at that PR, the fix only applies to the "courtesy build script" and not CI, though :-( So maybe same thing: CI needs to be upgraded, too (for liboqs to always use the same OpenSSL version as is used to run oqsprovider).

[ghost]

may also be a reason behind the failure to load the provider.

Okay, so I investigated this issue. To do so, I cloned liboqs, oqs-provider and various versions of OpenSSL. Here is what I found:

In this CircleCI job, we see that CMake uses the OpenSSL 1.1.1 include directory to build liboqs:

Found OpenSSL: /usr/local/opt/[email protected]/lib/libcrypto.dylib (found suitable version "1.1.1m", minimum required is "1.1.1")

while OpenSSL 3.0.1 is used for oqs-provider:

-- Found OpenSSL: /usr/local/opt/openssl@3/lib/libcrypto.dylib (found suitable version "3.0.1", minimum required is "3.0")

Thus, inside a sandbox, I compiled liboqs using OpenSSL 1.1.1m, and oqs-provider using OpenSSL 3.0.1. And I got the very same errors:

CTest output on oqs-provider

$ ctest --test-dir build --output-on-failure                                     
Internal ctest changing into directory: /Users/pawn/tests/openssl-3.0.8-oqs-provider/oqs-provider/build
Test project /Users/pawn/tests/openssl-3.0.8-oqs-provider/oqs-provider/build     
    Start 1: oqs_signatures                                                      
1/5 Test #1: oqs_signatures ...................Subprocess aborted***Exception:   0.01 sec
error registering dilithium2 with no hash                                        
/Users/pawn/tests/openssl-3.0.8-oqs-provider/oqs-provider/test/oqs_test_signatures.c:106: OpenSSL internal error: OSSL_PROVIDER_available(libctx, modulename)
                                                                                 
    Start 2: oqs_kems                                                            
2/5 Test #2: oqs_kems .........................Subprocess aborted***Exception:   0.00 sec
error registering dilithium2 with no hash                                        
/Users/pawn/tests/openssl-3.0.8-oqs-provider/oqs-provider/test/oqs_test_kems.c:85: OpenSSL internal error: OSSL_PROVIDER_available(libctx, modulename)
                                                                                 
    Start 3: oqs_groups                                                          
3/5 Test #3: oqs_groups .......................Subprocess aborted***Exception:   0.01 sec
error registering dilithium2 with no hash                                        
/Users/pawn/tests/openssl-3.0.8-oqs-provider/oqs-provider/test/oqs_test_groups.c:144: OpenSSL internal error: OSSL_PROVIDER_available(libctx, modulename)
                                                                                 
    Start 4: oqs_tlssig                                                          
4/5 Test #4: oqs_tlssig .......................Subprocess aborted***Exception:   0.00 sec
error registering dilithium2 with no hash                                        
/Users/pawn/tests/openssl-3.0.8-oqs-provider/oqs-provider/test/oqs_test_tlssig.c:158: OpenSSL internal error: OSSL_PROVIDER_available(libctx, modulename)
                                                                                 
    Start 5: oqs_endecode                                                        
5/5 Test #5: oqs_endecode .....................Subprocess aborted***Exception:   0.00 sec
error registering dilithium2 with no hash                                        
/Users/pawn/tests/openssl-3.0.8-oqs-provider/oqs-provider/test/oqs_test_endecode.c:217: OpenSSL internal error: OSSL_PROVIDER_available(libctx, modulename)
                                                                                 
                                                                                 
0% tests passed, 5 tests failed out of 5                                         
                                                                                 
Total Test time (real) =   0.03 sec                                              
                                                                                 
The following tests FAILED:                                                      
    1 - oqs_signatures (Subprocess aborted)                                      
    2 - oqs_kems (Subprocess aborted)                                            
    3 - oqs_groups (Subprocess aborted)                                          
    4 - oqs_tlssig (Subprocess aborted)                                          
    5 - oqs_endecode (Subprocess aborted)                                        
Errors while running CTest

Then, I tried to compile liboqs with OpenSSL 3.0.1 (instead of 1.1.1), and I also got those errors (error registering dilithium2 with no hash).

Finally, I tested with several versions of OpenSSL 3.0:

• OpenSSL 3.0.1: error while registering the oqsprovider
• OpenSSL 3.0.2: failed to compile [1]
• OpenSSL 3.0.3 to 3.0.8: works

For each version V, I tried with liboqs+OpenSSL 1.1.1 and liboqs+OpenSSL V, and nothing changed.

Consequently, I think that using the OpenSSL 1.1.1 headers to compile liboqs isn't an issue (yet?), however OpenSSL 3.0.1 definitely appears to be broken.

I think we may fix that issue by upgrading the OpenSSL 3 version from homebrew:

env HOMEBREW_NO_AUTO_UPDATE=1 brew install cmake ninja [email protected]

I'll give it a shot.

[1]: OpenSSL 3.0.2 errors while compiling:

Output of make on OpenSSL 3.0.2

ld: Undefined symbols:
  _ossl_der_oid_id_aes128_wrap, referenced from:
      _kek_algs in libdefault.a[89](libdefault-lib-x942kdf.o)
  _ossl_der_oid_id_aes192_wrap, referenced from:
      _kek_algs in libdefault.a[89](libdefault-lib-x942kdf.o)
  _ossl_der_oid_id_aes256_wrap, referenced from:
      _kek_algs in libdefault.a[89](libdefault-lib-x942kdf.o)
  _ossl_der_oid_id_alg_CMS3DESwrap, referenced from:
      _kek_algs in libdefault.a[89](libdefault-lib-x942kdf.o)

[ghost]

Now that #217 is merged, I rebased this PR onto main, it should work.

CircleCI is having an outage on their GitHub service, so we'll have to wait and re-trigger the CircleCI pipelines…

[baentsch]

Tests with the external encodings library also cannot run as-is (in CI). See https://app.circleci.com/pipelines/github/open-quantum-safe/oqs-provider/710/workflows/e31c536a-beb9-4f93-ac9d-4cef35d2ec59/jobs/1589. Suggest to skip those in case of static build.

@bhess What's your take as to the need for testing of this library after IETF has rejected the encoding it implements? Should we keep it in CI? If not, the test failing in the trace above could simply be culled completely.

[ghost]

Suggest to skip those in case of static build.

It's done. Actually I did it for macOS but I forgot about linux...

[baentsch]

Thanks again for this contribution, @thb-sb!