Update tests to use OpenSSL legacy provider if OpenSSL 3.0 used #806
Conversation
WalkthroughReplaces the Linux matrix CI with three dedicated Linux jobs (Botan, OpenSSL 1.1.1, OpenSSL 3.0), adds OpenSSL 3.x provider initialization in two test executables, and adjusts test include/CPPFLAGS. macOS and Windows CI remain unchanged. Changes
Sequence Diagram(s)sequenceDiagram
participant TestExe as Test executable
participant OpenSSL as OpenSSL runtime
TestExe->>OpenSSL: check OPENSSL_VERSION_NUMBER
alt WITH_OPENSSL and version >= 3.0
TestExe->>OpenSSL: OSSL_PROVIDER_load(NULL, "legacy")
OpenSSL-->>TestExe: legacy provider handle
TestExe->>OpenSSL: OSSL_PROVIDER_load(NULL, "default")
OpenSSL-->>TestExe: default provider handle
end
TestExe->>TestExe: continue initialization and run tests
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
.github/workflows/ci.yml (1)
68-70: Consider tracking the deprecation warning fixes.The commented -Werror suggests known OpenSSL 3.0 deprecation warnings that should be addressed in a future PR.
Consider opening an issue to track the deprecation warning fixes so -Werror can be re-enabled for the OpenSSL 3.0 build.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
.github/workflows/ci.yml(2 hunks)src/lib/crypto/test/cryptotest.cpp(2 hunks)src/lib/test/Makefile.am(1 hunks)src/lib/test/p11test.cpp(2 hunks)
🔇 Additional comments (8)
src/lib/test/p11test.cpp (2)
48-48: LGTM! Config.h inclusion supports build configuration.The unconditional inclusion of config.h enables proper macro definitions for the OpenSSL version checks below.
52-57: LGTM! Proper OpenSSL 3.0+ conditional compilation.The version guard correctly targets OpenSSL 3.0 and later versions using the standard version check
0x3000000fL.src/lib/test/Makefile.am (1)
3-8: LGTM! Proper build configuration for OpenSSL integration.The additional include path
$(srcdir)/../..enables access to config.h, and@CRYPTO_INCLUDES@ensures crypto headers are properly located during compilation. This supports the OpenSSL conditional compilation in the test files.src/lib/crypto/test/cryptotest.cpp (2)
47-47: LGTM! Consistent preprocessor directive style.The change from
#if defined(WITH_OPENSSL)to#ifdef WITH_OPENSSLimproves readability while maintaining the same functionality.
49-51: LGTM! Proper conditional inclusion of OpenSSL 3.0+ headers.The version guard correctly includes the provider header only for OpenSSL 3.0 and later versions.
.github/workflows/ci.yml (3)
11-29: LGTM! Clear Botan-specific job configuration.The dedicated Botan job with appropriate compiler flags and dependencies provides clear separation from OpenSSL builds.
31-56: LGTM! Proper OpenSSL 1.1.1 compatibility testing.The downgrade to OpenSSL 1.1.1 ensures backward compatibility testing. The manual package installation approach is appropriate for this specific version requirement.
58-77: LGTM! OpenSSL 3.0 job will test the new provider initialization.This job will exercise the OpenSSL 3.0 provider initialization code added in the test executables. The commented -Werror suggests awareness of deprecation warnings that need to be addressed separately.
bukka
force-pushed
the
openssl-3-testing-legacy-prov-v2
branch
from
b73b627 to
1f4924c
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/lib/crypto/test/cryptotest.cpp (1)
47-54: Unify WITH_OPENSSL guard style for consistency.Line 47 uses
#ifdef WITH_OPENSSLwhile later blocks use#if defined(WITH_OPENSSL)(Lines 61, 71). Prefer one style across the file; suggest switching Line 47 to match the rest.-#ifdef WITH_OPENSSL +#if defined(WITH_OPENSSL)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
.github/workflows/ci.yml(2 hunks)src/lib/crypto/test/cryptotest.cpp(2 hunks)src/lib/test/Makefile.am(1 hunks)src/lib/test/p11test.cpp(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- src/lib/test/Makefile.am
- src/lib/test/p11test.cpp
- .github/workflows/ci.yml
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-11T16:42:22.968Z
Learnt from: bukka
PR: softhsm/SoftHSMv2#806
File: src/lib/test/p11test.cpp:81-84
Timestamp: 2025-08-11T16:42:22.968Z
Learning: In SoftHSM test files (like p11test.cpp and cryptotest.cpp), error handling for OSSL_PROVIDER_load() calls for "legacy" and "default" providers is intentionally omitted because these providers won't fail in practice and the test code is kept deliberately simple.
Applied to files:
src/lib/crypto/test/cryptotest.cpp
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: Windows (x86, openssl)
- GitHub Check: Windows (x64, botan)
- GitHub Check: Windows (x64, openssl)
- GitHub Check: macOS (openssl)
- GitHub Check: macOS (botan)
- GitHub Check: Linux with Botan
- GitHub Check: Linux (openssl 3.0)
- GitHub Check: Linux with OpenSSL 1.1.1
🔇 Additional comments (2)
src/lib/crypto/test/cryptotest.cpp (2)
49-51: Correct include guard for OpenSSL 3 provider header — LGTM.
OPENSSL_VERSION_NUMBER >= 0x30000000Lis the right threshold for OpenSSL 3.x. This matches upstream guidance.
81-84: Provider loading for OpenSSL 3 — LGTM.Guard and order are correct and align with p11test.cpp and the CI job targeting OpenSSL 3.0. Intentional omission of error handling in tests acknowledged.
bukka
force-pushed
the
openssl-3-testing-legacy-prov-v2
branch
from
1f4924c to
5273e57
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/lib/test/p11test.cpp (2)
48-48: Consider including config.h first (optional)Autotools projects typically include config.h before other headers so feature/test macros affect subsequent includes. Not required here, but moving it to the top improves consistency.
64-67: Typo: rename 'solurceLine' to 'sourceLine'Minor readability fix; no behavioral change.
Apply this diff:
- const CPPUNIT_NS::SourceLine solurceLine( failure.sourceLine() ); + const CPPUNIT_NS::SourceLine sourceLine( failure.sourceLine() ); - std::cout << solurceLine.fileName() << ' ' << solurceLine.lineNumber() << ' ' << message.shortDescription() << std::endl; + std::cout << sourceLine.fileName() << ' ' << sourceLine.lineNumber() << ' ' << message.shortDescription() << std::endl;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
.github/workflows/ci.yml(2 hunks)src/lib/crypto/test/cryptotest.cpp(2 hunks)src/lib/test/Makefile.am(1 hunks)src/lib/test/p11test.cpp(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- src/lib/crypto/test/cryptotest.cpp
- src/lib/test/Makefile.am
- .github/workflows/ci.yml
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-11T16:42:22.968Z
Learnt from: bukka
PR: softhsm/SoftHSMv2#806
File: src/lib/test/p11test.cpp:81-84
Timestamp: 2025-08-11T16:42:22.968Z
Learning: In SoftHSM test files (like p11test.cpp and cryptotest.cpp), error handling for OSSL_PROVIDER_load() calls for "legacy" and "default" providers is intentionally omitted because these providers won't fail in practice and the test code is kept deliberately simple.
Applied to files:
src/lib/test/p11test.cpp
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: Windows (x86, openssl)
- GitHub Check: macOS (openssl)
- GitHub Check: Windows (x64, openssl)
- GitHub Check: Windows (x64, botan)
- GitHub Check: macOS (botan)
- GitHub Check: Linux with OpenSSL 3.0
- GitHub Check: Linux with Botan
- GitHub Check: Linux with OpenSSL 1.1.1
🔇 Additional comments (2)
src/lib/test/p11test.cpp (2)
52-57: OpenSSL header gating is correct for 3.x buildsIncluding openssl/opensslv.h under WITH_OPENSSL and conditionally including openssl/provider.h only for 3.0+ is accurate and avoids compile issues on 1.1.1.
81-84: Provider initialization for OpenSSL 3.x: LGTMInitializing the "legacy" and "default" providers at test startup is aligned with the PR goal and matches the approach in other test files. Omitting error handling here is consistent with prior team decision for test simplicity.
bukka
force-pushed
the
openssl-3-testing-legacy-prov-v2
branch
from
5273e57 to
5043deb
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (2)
.github/workflows/ci.yml (2)
41-43: Download OpenSSL debs over HTTPS and hard-fail on HTTP errors.Using HTTP for security packages is risky and curl won’t fail on HTTP errors unless -f is used. Switch to HTTPS and add -fsSLO so failures surface immediately.
- curl -O http://security.ubuntu.com/ubuntu/pool/main/o/openssl/libssl1.1_1.1.1f-1ubuntu2.24_amd64.deb - curl -O http://security.ubuntu.com/ubuntu/pool/main/o/openssl/libssl-dev_1.1.1f-1ubuntu2.24_amd64.deb - curl -O http://security.ubuntu.com/ubuntu/pool/main/o/openssl/openssl_1.1.1f-1ubuntu2.24_amd64.deb + curl -fsSLO https://security.ubuntu.com/ubuntu/pool/main/o/openssl/libssl1.1_1.1.1f-1ubuntu2.24_amd64.deb + curl -fsSLO https://security.ubuntu.com/ubuntu/pool/main/o/openssl/libssl-dev_1.1.1f-1ubuntu2.24_amd64.deb + curl -fsSLO https://security.ubuntu.com/ubuntu/pool/main/o/openssl/openssl_1.1.1f-1ubuntu2.24_amd64.deb
44-46: Ensure dpkg dependency resolution on ubuntu-24.04 after installing OpenSSL 1.1File: .github/workflows/ci.yml
Lines: 44–46Installing Ubuntu 20.04 (focal) OpenSSL 1.1 packages on the ubuntu-24.04 runner can leave the package manager in a broken state. Add an explicit update + fix step:
sudo dpkg -i --force-confnew openssl_1.1.1f-1ubuntu2.24_amd64.deb \ libssl1.1_1.1.1f-1ubuntu2.24_amd64.deb \ libssl-dev_1.1.1f-1ubuntu2.24_amd64.deb + sudo apt-get update -y + sudo apt-get -f install -yPlease verify that these commands complete without unresolved dependencies on the ubuntu-24.04 runner. If issues persist, consider adding apt pinning for libssl1.1 or switching this job to ubuntu-22.04.
🧹 Nitpick comments (1)
.github/workflows/ci.yml (1)
47-55: Add a quick verification step to assert OpenSSL 1.1.1 is in use before building.This helps catch packaging drift early and makes failures more actionable.
- - name: Build + - name: Verify OpenSSL version + run: | + openssl version -a + pkg-config --modversion openssl || true + - name: Build env: CXXFLAGS: -Werror run: | ./autogen.sh ./configure --with-crypto-backend=openssl make
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
.github/workflows/ci.yml(2 hunks)src/lib/crypto/test/cryptotest.cpp(2 hunks)src/lib/test/Makefile.am(1 hunks)src/lib/test/p11test.cpp(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- src/lib/test/Makefile.am
- src/lib/crypto/test/cryptotest.cpp
- src/lib/test/p11test.cpp
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: Windows (x64, botan)
- GitHub Check: Windows (x64, openssl)
- GitHub Check: Windows (x86, openssl)
- GitHub Check: macOS (botan)
- GitHub Check: macOS (openssl)
- GitHub Check: Linux with OpenSSL 3.0
- GitHub Check: Linux with Botan
- GitHub Check: Linux with OpenSSL 1.1.1
🔇 Additional comments (1)
.github/workflows/ci.yml (1)
22-26: Botan warnings policy looks good.Enforcing -Werror and suppressing Botan deprecations is a sensible balance for CI.
Also add pipeline to test OpenSSL 3.0
bukka
force-pushed
the
openssl-3-testing-legacy-prov-v2
branch
from
5043deb to
87424fb
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
.github/workflows/ci.yml (1)
41-46: Use HTTPS and fail-fast flags for package downloads (security hardening).Current curl uses plain HTTP; susceptible to MITM. Also add fail/redirect/silent flags to ensure early failure.
Apply:
- curl -O http://security.ubuntu.com/ubuntu/pool/main/o/openssl/libssl1.1_1.1.1f-1ubuntu2.24_amd64.deb - curl -O http://security.ubuntu.com/ubuntu/pool/main/o/openssl/libssl-dev_1.1.1f-1ubuntu2.24_amd64.deb - curl -O http://security.ubuntu.com/ubuntu/pool/main/o/openssl/openssl_1.1.1f-1ubuntu2.24_amd64.deb + curl -fSLO https://security.ubuntu.com/ubuntu/pool/main/o/openssl/libssl1.1_1.1.1f-1ubuntu2.24_amd64.deb + curl -fSLO https://security.ubuntu.com/ubuntu/pool/main/o/openssl/libssl-dev_1.1.1f-1ubuntu2.24_amd64.deb + curl -fSLO https://security.ubuntu.com/ubuntu/pool/main/o/openssl/openssl_1.1.1f-1ubuntu2.24_amd64.debOptional follow-up: verify checksums before dpkg -i if you have pinned SHAs available.
🧹 Nitpick comments (2)
.github/workflows/ci.yml (2)
18-19: Nit: Install without recommends to speed up CI and reduce churn.Avoid pulling optional packages.
Apply:
- sudo apt-get install -y libcppunit-dev libbotan-2-dev p11-kit + sudo apt-get install -y --no-install-recommends libcppunit-dev libbotan-2-dev p11-kit
65-66: libssl-dev likely unnecessary on ubuntu-24.04 runners; consider removing to speed up CI.Per retrieved learnings, ubuntu-24.04 GitHub runners already include OpenSSL 3.0 and development headers. Dropping this saves apt time and avoids potential needrestart churn.
Apply:
- sudo apt-get install -y libcppunit-dev libssl-dev p11-kit + sudo apt-get install -y --no-install-recommends libcppunit-dev p11-kitIf you keep it for belt-and-suspenders, consider adding --no-install-recommends.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
.github/workflows/ci.yml(2 hunks)src/lib/crypto/test/cryptotest.cpp(2 hunks)src/lib/test/Makefile.am(1 hunks)src/lib/test/p11test.cpp(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- src/lib/test/p11test.cpp
- src/lib/test/Makefile.am
- src/lib/crypto/test/cryptotest.cpp
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-08-12T13:25:41.686Z
Learnt from: bukka
PR: softhsm/SoftHSMv2#806
File: .github/workflows/ci.yml:66-66
Timestamp: 2025-08-12T13:25:41.686Z
Learning: GitHub Actions ubuntu-24.04 runner image comes with OpenSSL 3.0 and development headers pre-installed by default, so explicit installation of libssl-dev is not required for OpenSSL builds in workflows using this runner image.
Applied to files:
.github/workflows/ci.yml
📚 Learning: 2025-08-12T13:25:41.686Z
Learnt from: bukka
PR: softhsm/SoftHSMv2#806
File: .github/workflows/ci.yml:66-66
Timestamp: 2025-08-12T13:25:41.686Z
Learning: Ubuntu 24.04 (ubuntu-24.04 GitHub Actions runner image) comes with OpenSSL 3.0 and development headers (libssl-dev equivalent) pre-installed, so explicit installation of libssl-dev is not required for OpenSSL builds in CI workflows using this image.
Applied to files:
.github/workflows/ci.yml
📚 Learning: 2025-08-12T13:25:41.686Z
Learnt from: bukka
PR: softhsm/SoftHSMv2#806
File: .github/workflows/ci.yml:66-66
Timestamp: 2025-08-12T13:25:41.686Z
Learning: The GitHub Actions ubuntu-24.04 runner image includes OpenSSL and libssl-dev pre-installed by default, as documented in the official actions/runner-images repository. Explicit installation of libssl-dev is not required for OpenSSL builds using this runner image.
Applied to files:
.github/workflows/ci.yml
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: Windows (x64, openssl)
- GitHub Check: Windows (x86, openssl)
- GitHub Check: Windows (x64, botan)
- GitHub Check: macOS (openssl)
- GitHub Check: Linux with OpenSSL 1.1.1
- GitHub Check: Linux with OpenSSL 3.0
- GitHub Check: macOS (botan)
- GitHub Check: Linux with Botan
🔇 Additional comments (2)
.github/workflows/ci.yml (2)
11-13: Good split of Linux jobs; clearer intent per backend.Dedicated jobs for Botan, OpenSSL 1.1.1, and OpenSSL 3.0 improve readability and triage.
51-53: Add quick sanity check of OpenSSL toolchain version before configure.Helps ensure the job actually uses 1.1.1 after dpkg downgrade and eases debugging.
Apply:
./autogen.sh - ./configure --with-crypto-backend=openssl + openssl version -a || true + pkg-config --modversion openssl || true + ./configure --with-crypto-backend=openssl
4 participants
Also added pipeline to test OpenSSL 3.0 and removed matrix which didn't really make much sense and was just adding unnecessary deps for each workflow.
Summary by CodeRabbit
Chores
Tests
No user-facing behavior changes.