Update tests to use OpenSSL legacy provider if OpenSSL 3.0 used #783
Conversation
WalkthroughThe pull request introduces enhancements to the continuous integration (CI) workflow and cryptographic test configurations. The changes primarily focus on updating the Linux job definitions in the GitHub Actions workflow, modifying preprocessor directives in cryptographic test files, and adjusting include paths in the Makefile. The modifications aim to improve compatibility with newer versions of OpenSSL, specifically version 3.0.0 and above, by adding provider loading functionality and updating compilation flags. Changes
Possibly related PRs
Suggested reviewers
Poem
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
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: 0
🧹 Nitpick comments (2)
src/lib/test/p11test.cpp (1)
81-84: Consider refactoring duplicated provider loading code.This provider loading code is identical to the one in cryptotest.cpp. Consider extracting it to a shared utility function.
Create a new header file (e.g.,
test_utils.h):#pragma once #ifdef WITH_OPENSSL #include <openssl/opensslv.h> #if OPENSSL_VERSION_NUMBER >= 0x3000000 #include <openssl/provider.h> inline bool load_openssl_providers() { if (!OSSL_PROVIDER_load(NULL, "legacy")) { fprintf(stderr, "Failed to load legacy provider\n"); return false; } if (!OSSL_PROVIDER_load(NULL, "default")) { fprintf(stderr, "Failed to load default provider\n"); return false; } return true; } #endif #endifThen use it in both test files:
#include "test_utils.h" ... #if defined(WITH_OPENSSL) && OPENSSL_VERSION_NUMBER >= 0x3000000 - OSSL_PROVIDER_load(NULL, "legacy"); - OSSL_PROVIDER_load(NULL, "default"); + if (!load_openssl_providers()) { + return 1; + } #endif.github/workflows/ci.yml (1)
Line range hint
37-57: Consider adding OpenSSL version verification.The Ubuntu 24.04 job looks good but should verify it's using OpenSSL 3.0.
Add version check after the prepare step:
- name: Prepare run: | sudo apt update -qq sudo apt install libcppunit-dev libbotan-2-dev p11-kit + - name: Verify OpenSSL version + if: matrix.backend == 'openssl' + run: | + openssl version | grep -q "OpenSSL 3"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
.github/workflows/ci.yml(1 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 (5)
src/lib/crypto/test/cryptotest.cpp (2)
47-51: LGTM! Clean preprocessor directive changes.The simplified
#ifdef WITH_OPENSSLachieves the same result as#if defined(WITH_OPENSSL)with cleaner syntax.
81-84: Verify provider loading success.The provider loading looks correct, but we should verify that both providers load successfully.
Add error handling:
#if defined(WITH_OPENSSL) && OPENSSL_VERSION_NUMBER >= 0x3000000 - OSSL_PROVIDER_load(NULL, "legacy"); - OSSL_PROVIDER_load(NULL, "default"); + if (!OSSL_PROVIDER_load(NULL, "legacy")) { + fprintf(stderr, "Failed to load legacy provider\n"); + return 1; + } + if (!OSSL_PROVIDER_load(NULL, "default")) { + fprintf(stderr, "Failed to load default provider\n"); + return 1; + } #endifsrc/lib/test/p11test.cpp (1)
48-57: LGTM! Necessary includes for OpenSSL support.The addition of config.h and OpenSSL headers is required for version checks and provider loading.
src/lib/test/Makefile.am (1)
3-8: LGTM! Necessary include path and crypto flag additions.The changes correctly add:
- Access to higher-level includes via
-I$(srcdir)/../..- Crypto-specific includes via
@CRYPTO_INCLUDES@.github/workflows/ci.yml (1)
11-36: LGTM! Well-structured Ubuntu 20.04 job configuration.The job is correctly configured to test with OpenSSL 1.1.1 and includes proper error handling by outputting test logs on failure.
|
@bukka thanks for this PR. You may want to add this patch that further constraints engine support to only openssl 1.x. It is from our Fedora patchset, I adjusted it to this PR. |
|
#781 is now merged, please rebase and ensure tests are not failing. |
bukka
force-pushed
the
openssl-3-testing-legacy-prov
branch
from
5041923 to
533ca3e
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
.github/workflows/ci.yml (1)
54-54: Consider standardizing error handling between Ubuntu versions.The CXXFLAGS configuration differs between Ubuntu versions:
- Ubuntu 20.04:
-Werror -DBOTAN_NO_DEPRECATED_WARNINGS- Ubuntu 24.04:
-DBOTAN_NO_DEPRECATED_WARNINGSConsider standardizing the
-Werrorflag usage across both configurations to maintain consistent error handling.Also applies to: 28-28
📜 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/crypto/test/cryptotest.cpp
- src/lib/test/Makefile.am
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: macOS (botan)
- GitHub Check: Linux (botan)
- GitHub Check: Windows (x86, openssl)
- GitHub Check: Linux (botan)
- GitHub Check: Windows (x64, botan)
- GitHub Check: Linux (openssl)
- GitHub Check: Windows (x64, openssl)
- GitHub Check: Linux (openssl)
- GitHub Check: macOS (openssl)
🔇 Additional comments (2)
.github/workflows/ci.yml (2)
11-13: LGTM! Clear job naming and version documentation.The job renaming from 'linux' to 'linux_ubuntu_20' with explicit version comment improves clarity by indicating the exact Ubuntu version and its associated OpenSSL/Botan versions.
37-62: Verify OpenSSL 3.0 compatibility in the new Ubuntu 24.04 job.The new job configuration aligns with the PR objectives to support OpenSSL 3.0. However, there are a few considerations:
- The
-Werrorflag is omitted in the Ubuntu 24.04 job. This might allow warnings that would be errors in the 20.04 job to pass silently.- Based on the PR objectives mentioning "legacy provider", we should verify if any additional environment variables are needed for OpenSSL 3.0 compatibility.
Let's verify the OpenSSL version and available providers in Ubuntu 24.04:
✅ Verification successful
The Ubuntu 24.04 job configuration is correct for OpenSSL 3.0 compatibility
The omission of
-Werrorin the Ubuntu 24.04 job is intentional and appropriate during the transition to OpenSSL 3.0. The codebase already includes proper version checks and test cases for OpenSSL 3.0 compatibility. No additional environment variables for legacy provider support are needed as the code handles version differences appropriately.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check OpenSSL version and providers in Ubuntu 24.04 # Note: This is for information only as the actual environment will be in the CI # Search for OpenSSL version checks or provider loading in the codebase rg -n "OPENSSL_VERSION|OPENSSL.*_provider" --type cpp # Search for any OpenSSL environment variables in test scripts rg -n "OPENSSL.*=" --type shLength of output: 2418
Script:
#!/bin/bash # Check configure scripts and build system for OpenSSL settings rg -n "openssl|OPENSSL|Werror" configure.ac rg -n "openssl|OPENSSL|Werror" Makefile.am fd configure --type f --exec grep -l "openssl|OPENSSL|Werror" {} \;Length of output: 163
Script:
#!/bin/bash # Check other CI jobs and build configurations rg -n "ubuntu-20|ubuntu-22" .github/workflows/ rg -n "CXXFLAGS|CFLAGS" .github/workflows/ rg -n "Werror|Wall" .github/workflows/Length of output: 650
|
Ok I rebased it and fixed the 1.1.1 build / tests but Ubuntu 24.04 still has got some failure (on both Botan and OpenSSL). The tests work for me with custom OpenSSL builds (tested with latest 3.0 and 3.4) so it might be something Ubuntu 24.04 specific. I will look into it next week. |
This enables legacy provider when running tests with OpenSSL 3.0 as some old algorithms are used and it would be good to keep the tested. It also makes tests green when running with OpenSSL 3.0 (except maybe when the crypto RHEL policy is selected).
Summary by CodeRabbit
CI/CD Updates
Cryptography Improvements
Build System