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

Support openssl 3.0.0 #633

Draft
wants to merge 5 commits into
base: develop
Choose a base branch
from
Draft

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.

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