Skip to content

Conversation

@abbra
Copy link
Contributor

@abbra abbra commented May 27, 2021

This is a first step to make SoftHSM compiled and tests running with OpenSSL 3.0.0 under CentOS 9 Stream (similar to Fedora 34). We cannot use DES anymore there without loading a legacy provider but even if it is loaded, system-wide crypto policies on Fedora/CentOS Stream/RHEL would forbid its use. Same with RSA 1024 or lower key sizes.

The test changes simply make it so that the tests are only run if we are able to initialize encoders or generate keys to work on. Sadly, CPPUNIT cannot produce warnings-only output, they have to be either failures or success, so I have to skip tests that cannot be run.

@abbra
Copy link
Contributor Author

abbra commented May 27, 2021

Test failures seem to be unrelated to my changes

@abbra
Copy link
Contributor Author

abbra commented May 27, 2021

Found few more tests that fail due to DES key use..

@abbra abbra force-pushed the support-openssl-3.0.0 branch from 496fe2c to ca037b3 Compare June 2, 2021 13:28
loqs added a commit to loqs/PACKAGES-OSSL3 that referenced this pull request Feb 25, 2022
loqs added a commit to loqs/PACKAGES-OSSL3 that referenced this pull request Mar 22, 2022
loqs added a commit to loqs/PACKAGES-OSSL3 that referenced this pull request Aug 29, 2022
@mcepl
Copy link

mcepl commented Aug 30, 2024

Hmm, I get:

[   55s] make[4]: Entering directory '/home/abuild/rpmbuild/BUILD/softhsm-2.6.1/src/bin/migrate'
[   55s] g++ -DHAVE_CONFIG_H -I. -I../../..  -I./../../lib/pkcs11 -I./../common    -O2 -Wall -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=3 -fstack-protector-strong -funwind-tables -fasynchronous-unwind-tables -fstack-clash-protection -Werror=return-type  -g -Wall -Wextra -fvisibility=hidden -c -o softhsm2-migrate.o softhsm2-migrate.cpp
[   55s] make[4]: Leaving directory '/home/abuild/rpmbuild/BUILD/softhsm-2.6.1/src/bin/migrate'
[   55s] make[4]: Entering directory '/home/abuild/rpmbuild/BUILD/softhsm-2.6.1/src/bin/migrate'
[   55s] /usr/bin/bash ../../../libtool  --tag=CXX   --mode=link g++  -O2 -Wall -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=3 -fstack-protector-strong -funwind-tables -fasynchronous-unwind-tables -fstack-clash-protection -Werror=return-type  -g -Wall -Wextra -fvisibility=hidden   -o softhsm2-migrate softhsm2-migrate.o ../common/findslot.o ../common/getpw.o ../common/library.o -lsqlite3 -lrt 
[   55s] libtool: link: g++ -O2 -Wall -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=3 -fstack-protector-strong -funwind-tables -fasynchronous-unwind-tables -fstack-clash-protection -Werror=return-type -g -Wall -Wextra -fvisibility=hidden -o softhsm2-migrate softhsm2-migrate.o ../common/findslot.o ../common/getpw.o ../common/library.o  -lsqlite3 -lrt
[   55s] make[4]: Leaving directory '/home/abuild/rpmbuild/BUILD/softhsm-2.6.1/src/bin/migrate'
[   55s] make[4]: Nothing to be done for 'all-am'.
[   55s] make[3]: Nothing to be done for 'all-am'.
[   55s] + cp /home/abuild/rpmbuild/SOURCES/softhsm2-pk11install.c .
[   55s] ++ pkg-config --cflags nss
[   55s] + gcc -I/usr/include/nss3 -I/usr/include/nspr4 -O2 -Wall -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=3 -fstack-protector-strong -funwind-tables -fasynchronous-unwind-tables -fstack-clash-protection -Werror=return-type -g -c softhsm2-pk11install.c
[   55s] softhsm2-pk11install.c: In function ‘installPKCS11’:
[   55s] softhsm2-pk11install.c:201:20: error: implicit declaration of function ‘NSC_ModuleDBFunc’ [-Wimplicit-function-declaration]
[   55s]   201 |     rc = (char **) NSC_ModuleDBFunc(type == Install ?
[   55s]       |                    ^~~~~~~~~~~~~~~~
[   55s] softhsm2-pk11install.c:201:10: warning: cast to pointer from integer of different size [-Wint-to-pointer-cast]
[   55s]   201 |     rc = (char **) NSC_ModuleDBFunc(type == Install ?
[   55s]       |          ^
[   55s] softhsm2-pk11install.c: In function ‘installAllPKCS11’:
[   55s] softhsm2-pk11install.c:220:9: warning: variable ‘len’ set but not used [-Wunused-but-set-variable]
[   55s]   220 |     int len;
[   55s]       |         ^~~

Complete build log with all versions of packages used and steps taken to reproduce.

@jschlyter
Copy link
Contributor

Please rebase for retesting

@jschlyter jschlyter added the enhancement New or improved functionality label Nov 29, 2024
@jschlyter jschlyter self-assigned this Nov 29, 2024
@abbra abbra force-pushed the support-openssl-3.0.0 branch from ca037b3 to bdd1cd7 Compare November 29, 2024 13:38
@abbra abbra requested a review from a team as a code owner November 29, 2024 13:38
@mcepl
Copy link

mcepl commented Nov 29, 2024

When trying this patch on the top of the current develop branch (commit f7883c2), I get this compilation bug:

[   32s] make[5]: Entering directory '/home/abuild/rpmbuild/BUILD/SoftHSMv2-2.6.1+git.1732869438.f7883c2/src/lib/crypto'
[   32s] /usr/bin/bash ../../../libtool  --tag=CXX   --mode=compile g++ -DHAVE_CONFIG_H -I. -I../../..  -I./.. -I./../common -I./../data_mgr -I./../pkcs11 -I/usr/include   -O2 -Wall -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=3 -fstack-protector-strong -funwind-tables -fasynchronous-unwind-tables -fstack-clash-protection -Werror=return-type  -g -Wall -Wextra -fvisibility=hidden -c -o OSSLCryptoFactory.lo OSSLCryptoFactory.cpp
[   32s] libtool: compile:  g++ -DHAVE_CONFIG_H -I. -I../../.. -I./.. -I./../common -I./../data_mgr -I./../pkcs11 -I/usr/include -O2 -Wall -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=3 -fstack-protector-strong -funwind-tables -fasynchronous-unwind-tables -fstack-clash-protection -Werror=return-type -g -Wall -Wextra -fvisibility=hidden -c OSSLCryptoFactory.cpp  -fPIC -DPIC -o .libs/OSSLCryptoFactory.o
[   32s] OSSLCryptoFactory.cpp: In constructor 'OSSLCryptoFactory::OSSLCryptoFactory()':
[   32s] OSSLCryptoFactory.cpp:151:9: error: 'ENGINE_load_rdrand' was not declared in this scope
[   32s]   151 |         ENGINE_load_rdrand();
[   32s]       |         ^~~~~~~~~~~~~~~~~~
[   32s] OSSLCryptoFactory.cpp:154:25: error: 'ENGINE_by_id' was not declared in this scope
[   32s]   154 |         rdrand_engine = ENGINE_by_id("rdrand");
[   32s]       |                         ^~~~~~~~~~~~
[   32s] OSSLCryptoFactory.cpp:159:22: error: 'ENGINE_init' was not declared in this scope
[   32s]   159 |                 if (!ENGINE_init(rdrand_engine))
[   32s]       |                      ^~~~~~~~~~~
[   32s] OSSLCryptoFactory.cpp:164:61: error: 'ENGINE_METHOD_RAND' was not declared in this scope
[   32s]   164 |                 else if (!ENGINE_set_default(rdrand_engine, ENGINE_METHOD_RAND))
[   32s]       |                                                             ^~~~~~~~~~~~~~~~~~
[   32s] OSSLCryptoFactory.cpp:164:27: error: 'ENGINE_set_default' was not declared in this scope
[   32s]   164 |                 else if (!ENGINE_set_default(rdrand_engine, ENGINE_METHOD_RAND))
[   32s]       |                           ^~~~~~~~~~~~~~~~~~
[   32s] OSSLCryptoFactory.cpp: In destructor 'virtual OSSLCryptoFactory::~OSSLCryptoFactory()':
[   32s] OSSLCryptoFactory.cpp:263:17: error: 'ENGINE_finish' was not declared in this scope
[   32s]   263 |                 ENGINE_finish(rdrand_engine);
[   32s]       |                 ^~~~~~~~~~~~~
[   32s] OSSLCryptoFactory.cpp:264:17: error: 'ENGINE_free' was not declared in this scope
[   32s]   264 |                 ENGINE_free(rdrand_engine);
[   32s]       |                 ^~~~~~~~~~~
[   32s] OSSLCryptoFactory.cpp: In member function 'virtual SymmetricAlgorithm* OSSLCryptoFactory::getSymmetricAlgorithm(SymAlgo::Type)':
[   32s] OSSLCryptoFactory.cpp:312:16: warning: enumeration value 'Unknown' not handled in switch [-Wswitch]
[   32s]   312 |         switch (algorithm)
[   32s]       |                ^
[   32s] OSSLCryptoFactory.cpp: In member function 'virtual AsymmetricAlgorithm* OSSLCryptoFactory::getAsymmetricAlgorithm(AsymAlgo::Type)':
[   32s] OSSLCryptoFactory.cpp:329:16: warning: enumeration value 'Unknown' not handled in switch [-Wswitch]
[   32s]   329 |         switch (algorithm)
[   32s]       |                ^
[   32s] OSSLCryptoFactory.cpp:329:16: warning: enumeration value 'GOST' not handled in switch [-Wswitch]
[   32s] OSSLCryptoFactory.cpp: In member function 'virtual HashAlgorithm* OSSLCryptoFactory::getHashAlgorithm(HashAlgo::Type)':
[   32s] OSSLCryptoFactory.cpp:361:16: warning: enumeration value 'Unknown' not handled in switch [-Wswitch]
[   32s]   361 |         switch (algorithm)
[   32s]       |                ^
[   32s] OSSLCryptoFactory.cpp:361:16: warning: enumeration value 'GOST' not handled in switch [-Wswitch]
[   32s] OSSLCryptoFactory.cpp: In member function 'virtual MacAlgorithm* OSSLCryptoFactory::getMacAlgorithm(MacAlgo::Type)':
[   32s] OSSLCryptoFactory.cpp:389:16: warning: enumeration value 'Unknown' not handled in switch [-Wswitch]
[   32s]   389 |         switch (algorithm)
[   32s]       |                ^
[   32s] OSSLCryptoFactory.cpp:389:16: warning: enumeration value 'HMAC_GOST' not handled in switch [-Wswitch]
[   32s] make[5]: *** [Makefile:868: OSSLCryptoFactory.lo] Error 1

Complete build log

@abbra abbra force-pushed the support-openssl-3.0.0 branch from bdd1cd7 to a8f611d Compare November 29, 2024 14:08
@jschlyter
Copy link
Contributor

@abbra, can you add a CI target for "{linux,macos,windows}-openssl3" or similar so we can get test coverage with OpenSSL 3 and 1.1.x at the same time?

@abbra abbra force-pushed the support-openssl-3.0.0 branch from 46f0ca4 to b69cd17 Compare November 29, 2024 14:22
@abbra
Copy link
Contributor Author

abbra commented Nov 29, 2024

I have to push few more updates, sorry. There is an issue with rebasing my older patches around tests, so some of them aren't correct.

Signed-off-by: Alexander Bokovoy <[email protected]>
OpenSSL 3.0 moves DES into a legacy provider which has to be loaded
explicitly. By default, it will not be loaded and DES methods in tests
will fail. Nest test blocks under successful initialization.

Signed-off-by: Alexander Bokovoy <[email protected]>
OpenSSL 3.0 on systems with systemd-wide crypto policy (Fedora, RHEL,
CentOS 9 Stream) might block certain key sizes which causes the tests to
fail. Skip these tests because we are not going to get the results
anyway.

There is no way with CPPUNIT to produce a warning only, so we have to
skip the whole test result.

Signed-off-by: Alexander Bokovoy <[email protected]>
Signed-off-by: Alexander Bokovoy <[email protected]>
@abbra abbra force-pushed the support-openssl-3.0.0 branch from b69cd17 to 8d8b727 Compare November 29, 2024 14:23
@abbra
Copy link
Contributor Author

abbra commented Nov 29, 2024

Also, Fedora does not have engine API enabled anymore, so I cannot build locally anymore, need to pull a patch that disables engine's support but that one will break on other openssl versions.

@jschlyter
Copy link
Contributor

CI now updated, please merge develop and we should be in better shape. In theory.

@mcepl
Copy link

mcepl commented Nov 29, 2024

Almost there, now only few tests are failing:

[  104s] !!!FAILURES!!!
[  104s] Test Results:
[  104s] Run:  78   Failures: 1   Errors: 0
[  104s] 
[  104s] 
[  104s] 1) test: SymmetricAlgorithmTests::testAesEncryptDecrypt (F) line: 902 SymmetricAlgorithmTests.cpp
[  104s] equality assertion failed
[  104s] - Expected: 0
[  104s] - Actual  : 145
[  104s] 
[  104s] 
[  104s]  : OK
[  104s] 
[  104s] !!!FAILURES!!!
[  104s] Test Results:
[  104s] Run:  78   Failures: 1   Errors: 0
[  104s] 
[  104s] 
[  104s] 1) test: SymmetricAlgorithmTests::testAesEncryptDecrypt (F) line: 902 SymmetricAlgorithmTests.cpp
[  104s] equality assertion failed
[  104s] - Expected: 0
[  104s] - Actual  : 145
[  104s] 
[  104s] 
[  104s]  : assertion
[  104s] 
[  104s] !!!FAILURES!!!
[  104s] Test Results:
[  104s] Run:  78   Failures: 2   Errors: 0
[  104s] 
[  104s] 
[  104s] 1) test: SymmetricAlgorithmTests::testAesEncryptDecrypt (F) line: 902 SymmetricAlgorithmTests.cpp
[  104s] equality assertion failed
[  104s] - Expected: 0
[  104s] - Actual  : 145
[  104s] 
[  104s] 
[  104s] 2) test: ForkTests::testResetOnFork (F) line: 130 ForkTests.cpp
[  104s] assertion failed
[  104s] - Expression: rv == CKR_OK
[  104s] 
[  104s] 
[  104s]  : assertion
[  104s] 
[  104s] !!!FAILURES!!!
[  104s] Test Results:
[  104s] Run:  78   Failures: 2   Errors: 0
[  104s] 
[  104s] 
[  104s] 1) test: SymmetricAlgorithmTests::testAesEncryptDecrypt (F) line: 902 SymmetricAlgorithmTests.cpp
[  104s] equality assertion failed
[  104s] - Expected: 0
[  104s] - Actual  : 145
[  104s] 
[  104s] 
[  104s] 2) test: ForkTests::testResetOnFork (F) line: 130 ForkTests.cpp
[  104s] assertion failed
[  104s] - Expression: rv == CKR_OK

Complete build log

THANK YOU!

@jschlyter
Copy link
Contributor

Setting as draft until tests passes.

@jschlyter jschlyter marked this pull request as draft December 14, 2024 16:45
@abbra
Copy link
Contributor Author

abbra commented Dec 14, 2024

Yeah, sorry, had no time to look at that...

@bukka
Copy link
Contributor

bukka commented Jan 28, 2025

I just created #783 which will enable legacy provider so it will still keep testing the legacy things. I also added a new job for ubuntu 24.04 but think it makes sense to also run it with Botan.

In terms of this PR, I think the main issue is that it can actually hide problems in the implementation (e.g. ignore real failures in legacy algs implementation). After my change it's really just about dealing with non upstream RHEL changes which I'm not sure need to be address here. I think that anyone who wants to run tests there should just compile their own version of OpenSSL which is pretty easy. But if there was a need to really make it work nicely there, I think better solution would be to identify it during the configuration (m4 macro checking) and then just compile out parts that are not supported.

@bukka
Copy link
Contributor

bukka commented Jan 28, 2025

Maybe if it's too tricky to identify that the algs are disabled without the policy, it would be ok to introduce some special config options for that. I might actually look into it as I have a similar issue in PHP, where I maintain openssl extension, and we have a similar request there.

@mcepl
Copy link

mcepl commented Jan 29, 2025

I think that anyone who wants to run tests there should just compile their own version of OpenSSL which is pretty easy.

This is absolutely unacceptable idea for the Linux distributions. Of course, we have pre-existing build of OpenSSL and yes, we would like to have tests run to gain at least some hope for the package being functional.

@bukka
Copy link
Contributor

bukka commented Jan 29, 2025

This is absolutely unacceptable idea for the Linux distributions. Of course, we have pre-existing build of OpenSSL and yes, we would like to have tests run to gain at least some hope for the package being functional.

Well distributions can patch the tests as well. I think this is what is being done for PHP packages - some tests are modified only in the distribution version to work there. In our case we also use some EC group functionality that is patched out there. I'm not saying that's ideal but it is something that can be done instead.

Anyway it probably makes more sense to have those changes upstream and I will be actually looking into the better solution for PHP so might try to apply it then here as well which would be probably ideal. I think the current solution of ignoring failures is not ideal and it might be better to look into a bit more robust way.

@bukka
Copy link
Contributor

bukka commented Mar 19, 2025

Just an update that I was looking into the similar thing in PHP and best solution for that are build and then compile time checks.

But honestly I'm not sure if it makes much sense to do that before the things get updated to the new API (using EVP and parameters). I started looking into what it would take here as we were doing a similar thing in the PHP openssl extension. It's kind of similar here although it will require a bit more tweaking because there is that extra abstraction separating keys and the actual operations. It's not small amount of work but it's doable. Unfortunately and I currently have to prioritise open source work that I have some funding for. So this is kind of my free time looking which is very limited time wise so it will probably take ages to get anywhere... But will see maybe things will change and I will find some time or there will be someone else to implement this. In any case, it's something that should be done otherwise it will soon stop working with OpenSSL completely at some point.

@bukka
Copy link
Contributor

bukka commented Mar 20, 2025

Ok so I just decided to give it a try and started with DH migration: bukka#1 . It's the first shot and needs some minor fixes but think it's a good start. It still requires much more work to migrate everything and get rid of all deprecations but with some persistence, it should be doable.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New or improved functionality

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants