Add configure option and code to disable OpenSSL engines #781
Conversation
WalkthroughThe pull request introduces configuration options for OpenSSL engines in a cryptographic library. A new configuration argument Changes
Sequence DiagramsequenceDiagram
participant User as User
participant Configure as Configure Script
participant Compiler as Compiler
participant CryptoFactory as Crypto Factory
User->>Configure: Run with --disable-openssl-engines
Configure->>Compiler: Set compilation flags
Compiler->>CryptoFactory: Conditionally compile engine support
CryptoFactory-->>Compiler: Compile with/without engines
Poem
✨ Finishing Touches
🪧 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
🔭 Outside diff range comments (1)
src/lib/crypto/OSSLCryptoFactory.cpp (1)
Line range hint
144-165: Improve RDRAND engine error handling and cleanup.The current implementation has potential issues:
- No cleanup if ENGINE_init succeeds but ENGINE_set_default fails
- Warning messages could be more descriptive
if (rdrand_engine != NULL) { // Initialize RDRAND engine if (!ENGINE_init(rdrand_engine)) { - WARNING_MSG("ENGINE_init returned %lu\n", ERR_get_error()); + WARNING_MSG("Failed to initialize RDRAND engine: %lu\n", ERR_get_error()); + ENGINE_free(rdrand_engine); + rdrand_engine = NULL; } // Set RDRAND engine as the default for RAND_ methods else if (!ENGINE_set_default(rdrand_engine, ENGINE_METHOD_RAND)) { - WARNING_MSG("ENGINE_set_default returned %lu\n", ERR_get_error()); + WARNING_MSG("Failed to set RDRAND as default engine: %lu\n", ERR_get_error()); + ENGINE_finish(rdrand_engine); + ENGINE_free(rdrand_engine); + rdrand_engine = NULL; } }
🧹 Nitpick comments (2)
m4/acx_openssl_engines.m4 (1)
1-35: LGTM! Consider improving the error message.The macro implementation follows autoconf best practices with proper environment handling and result caching.
Consider improving the error message in the test program:
- #error "Engines are disabled" + #error "OpenSSL engines are not available (OPENSSL_NO_ENGINE is defined)"m4/acx_crypto_backend.m4 (1)
54-60: Enhance the help string for the openssl-engines option.The current help string could be more descriptive about the implications of disabling engines.
- [Disable OpenSSL engines usage] + [Disable OpenSSL engines usage (prevents usage of hardware-based random number generation and GOST)]
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
m4/acx_crypto_backend.m4(2 hunks)m4/acx_openssl_engines.m4(1 hunks)src/lib/crypto/OSSLCryptoFactory.cpp(5 hunks)src/lib/crypto/OSSLCryptoFactory.h(2 hunks)
🔇 Additional comments (4)
src/lib/crypto/OSSLCryptoFactory.h (2)
45-48: LGTM! Clean conditional compilation setup.The WITH_ENGINES macro is properly defined based on OpenSSL configuration.
110-119: LGTM! Good encapsulation of engine-related members.Engine member variables are properly wrapped in WITH_ENGINES conditional.
m4/acx_crypto_backend.m4 (1)
118-132: LGTM! Clean integration with crypto backend selection.The OpenSSL engines support check is properly integrated with the existing crypto backend selection logic.
src/lib/crypto/OSSLCryptoFactory.cpp (1)
Line range hint
233-263: Verify OpenSSL version-specific cleanup logic.The cleanup code uses version-specific paths that need verification.
✅ Verification successful
OpenSSL version-specific cleanup logic is correctly implemented
The cleanup code properly handles different OpenSSL versions and LibreSSL, with consistent version checks across the codebase and appropriate safeguards against use-after-free issues.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check OpenSSL version compatibility for cleanup logic # Test: Check OpenSSL version and LibreSSL definitions rg -A 5 "OPENSSL_VERSION|LIBRESSL" --type cppLength of output: 6812
|
LGTM |
This is to disable OpenSSL engines that are deprecated and in some setups inconvenient - especially the rdrand default usage which has priority over providers and disallows overwriting random generator using custom providers.
Summary by CodeRabbit
New Features
Bug Fixes
Refactor