Migrate OSSLDH to OpenSSL 3.0+ #817
Conversation
📝 WalkthroughWalkthroughAdds OpenSSL-version-gated DH implementations: legacy DH APIs for OpenSSL < 3.0 and EVP_PKEY/OSSL_PARAM_BLD flows for OpenSSL ≥ 3.0 across core DH logic, public/private DH key classes, PKCS#8 handling, memory management, and a test assertion update. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Caller
participant OSSLDH as OSSLDH (wrapper)
participant OpenSSL as OpenSSL (lib)
Caller->>OSSLDH: generateKeyPair(params)
OSSLDH->>OpenSSL: build params (P,G[,priv_len])
alt OpenSSL < 3.0
OSSLDH->>OpenSSL: DH_new / DH_set0_pqg / DH_generate_key()
OpenSSL-->>OSSLDH: DH keypair
else OpenSSL ≥ 3.0
OSSLDH->>OpenSSL: OSSL_PARAM_BLD / EVP_PKEY_fromdata / EVP_PKEY_keygen
OpenSSL-->>OSSLDH: EVP_PKEY keypair
end
OSSLDH-->>Caller: keypair (backend object)
Caller->>OSSLDH: deriveKey(private, peerPub)
OSSLDH->>OpenSSL: prepare peer key (BN/EVP_PKEY)
alt OpenSSL < 3.0
OSSLDH->>OpenSSL: DH_compute_key(peerPub)
OpenSSL-->>OSSLDH: shared secret
else OpenSSL ≥ 3.0
OSSLDH->>OpenSSL: EVP_PKEY_derive_init / EVP_PKEY_derive (query len, alloc, derive)
OpenSSL-->>OSSLDH: shared secret
end
OSSLDH-->>Caller: shared secret (ByteString)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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. Comment |
src/lib/crypto/test/DHTests.cpp
Outdated
| priv = (DHPrivateKey*) kp->getPrivateKey(); | ||
|
|
||
| CPPUNIT_ASSERT(priv->getX().bits() == 128); | ||
| CPPUNIT_ASSERT_EQUAL(size_t(1024), priv->getBitLength()); |
There was a problem hiding this comment.
This deserves special comment as I spent quite a lot of time trying to figure out why priv->getX().bits() no longer returns 128. This is because it is internally ignored when DH parameters are provided. This was the case for 1.1.1 too but it was saved directly to the structure so it didn't get reflected after generation. It doesn't really make sense to set it so the test is changed just to check the bit length.
|
So nice seeing devs for stopping using deprecated OSSL members. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Nitpick comments (6)
src/lib/crypto/OSSLDHPublicKey.h (2)
50-55: Make pointer-taking constructors explicit.Prevents accidental implicit conversions from DH*/EVP_PKEY* to OSSLDHPublicKey. Low-risk, local change.
-#if OPENSSL_VERSION_NUMBER < 0x30000000L - OSSLDHPublicKey(const DH* inDH); +#if OPENSSL_VERSION_NUMBER < 0x30000000L + explicit OSSLDHPublicKey(const DH* inDH); #else - OSSLDHPublicKey(const EVP_PKEY* inDH); + explicit OSSLDHPublicKey(const EVP_PKEY* inDH); #endif
86-93: Avoid double-free: delete copy/move or implement proper move semantics.This class owns a raw OpenSSL handle (DH*/EVP_PKEY*). The default copy/move will shallow-copy the pointer, risking double-free on destruction. If copying isn’t required (likely), delete copy/move for safety.
public: // Constructors OSSLDHPublicKey(); @@ virtual ~OSSLDHPublicKey(); + + // Non-copyable and non-movable to avoid double-free of the OpenSSL handle. + OSSLDHPublicKey(const OSSLDHPublicKey&) = delete; + OSSLDHPublicKey& operator=(const OSSLDHPublicKey&) = delete; + OSSLDHPublicKey(OSSLDHPublicKey&&) = delete; + OSSLDHPublicKey& operator=(OSSLDHPublicKey&&) = delete;If you need move, implement a custom move that nulls the source’s handle. I can draft that if desired.
src/lib/crypto/OSSLDHPrivateKey.h (2)
51-56: Mark the pointer-taking constructors explicit.Same rationale as the public key header.
-#if OPENSSL_VERSION_NUMBER < 0x30000000L - OSSLDHPrivateKey(const DH* inDH); +#if OPENSSL_VERSION_NUMBER < 0x30000000L + explicit OSSLDHPrivateKey(const DH* inDH); #else - OSSLDHPrivateKey(const EVP_PKEY* inDH); + explicit OSSLDHPrivateKey(const EVP_PKEY* inDH); #endif
95-103: Prevent shallow copies of the owned OpenSSL handle.Same ownership concern here; default copy/move can double-free DH*/EVP_PKEY*. If copying isn’t required in this codebase, delete copy/move.
public: // Constructors OSSLDHPrivateKey(); @@ virtual ~OSSLDHPrivateKey(); + + // Non-copyable and non-movable; avoids double-free of the OpenSSL handle. + OSSLDHPrivateKey(const OSSLDHPrivateKey&) = delete; + OSSLDHPrivateKey& operator=(const OSSLDHPrivateKey&) = delete; + OSSLDHPrivateKey(OSSLDHPrivateKey&&) = delete; + OSSLDHPrivateKey& operator=(OSSLDHPrivateKey&&) = delete;Given the maintainer’s preference for minimal scope (per #815), treating this as optional is fine; at least confirm these classes aren’t copied anywhere. I can provide a repo scan if helpful.
src/lib/crypto/test/DHTests.cpp (1)
94-101: Stale comment and hard-coded 1024; align assertion with loop key size.This block says “fixed private value length” but no fixed X bit-length is set. Also, hard-coding 1024 duplicates the previous assertion and will break if more sizes are added.
- // Generate key-pair with a fixed private value length + // Generate another key-pair; under OpenSSL 3.x the private key bit length equals the parameter size CPPUNIT_ASSERT(dh->generateKeyPair(&kp, p)); priv = (DHPrivateKey*) kp->getPrivateKey(); - CPPUNIT_ASSERT_EQUAL(size_t(1024), priv->getBitLength()); + CPPUNIT_ASSERT_EQUAL(size_t(*k), size_t(priv->getBitLength()));src/lib/crypto/OSSLDH.cpp (1)
356-365: Simplify shared secret handling in OpenSSL 3.0 pathThe OpenSSL 3.0 path creates a ByteString with
wipe(), but this is unnecessary sinceEVP_PKEY_derivewill write the entire buffer. The pattern differs from the pre-3.0 path which needs to handle leading zeros.Simplify the allocation:
ByteString secret; - secret.wipe(secretLen); + secret.resize(secretLen);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/lib/crypto/OSSLDH.cpp(8 hunks)src/lib/crypto/OSSLDHPrivateKey.cpp(7 hunks)src/lib/crypto/OSSLDHPrivateKey.h(2 hunks)src/lib/crypto/OSSLDHPublicKey.cpp(4 hunks)src/lib/crypto/OSSLDHPublicKey.h(2 hunks)src/lib/crypto/test/DHTests.cpp(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: bukka
PR: softhsm/SoftHSMv2#815
File: src/lib/SoftHSM.cpp:6543-6546
Timestamp: 2025-09-11T16:54:00.347Z
Learning: SoftHSM PR #815 (pkcs11.h 3.1.0 update, CK_RSA_AES_KEY_WRAP_PARAMS fix): Maintainer bukka prefers to keep scope minimal (header sync and param rename only); broader OAEP parameter enforcement or backend plumbing is out of scope for this PR and should be handled in a follow-up issue.
📚 Learning: 2025-09-11T16:54:00.347Z
Learnt from: bukka
PR: softhsm/SoftHSMv2#815
File: src/lib/SoftHSM.cpp:6543-6546
Timestamp: 2025-09-11T16:54:00.347Z
Learning: SoftHSM PR #815 (pkcs11.h 3.1.0 update, CK_RSA_AES_KEY_WRAP_PARAMS fix): Maintainer bukka prefers to keep scope minimal (header sync and param rename only); broader OAEP parameter enforcement or backend plumbing is out of scope for this PR and should be handled in a follow-up issue.
Applied to files:
src/lib/crypto/OSSLDH.cpp
🧬 Code graph analysis (5)
src/lib/crypto/OSSLDH.cpp (1)
src/lib/crypto/OSSLUtil.cpp (4)
byteString2bn(56-61)byteString2bn(56-56)bn2ByteString(42-53)bn2ByteString(42-42)
src/lib/crypto/OSSLDHPublicKey.h (2)
src/lib/crypto/OSSLDHPublicKey.cpp (15)
OSSLDHPublicKey(54-57)OSSLDHPublicKey(79-82)inDH(69-76)getOSSLKey(109-114)getOSSLKey(109-109)getOSSLKey(190-194)getOSSLKey(190-190)setFromOSSL(117-141)setFromOSSL(117-117)setFromOSSL(197-222)setFromOSSL(197-197)resetOSSLKey(143-150)resetOSSLKey(143-143)resetOSSLKey(224-231)resetOSSLKey(224-224)src/lib/crypto/DHPublicKey.h (1)
DHPublicKey(39-71)
src/lib/crypto/OSSLDHPrivateKey.h (1)
src/lib/crypto/OSSLDHPrivateKey.cpp (15)
OSSLDHPrivateKey(52-55)OSSLDHPrivateKey(71-74)inDH(61-68)getOSSLKey(161-166)getOSSLKey(161-161)getOSSLKey(289-293)getOSSLKey(289-289)setFromOSSL(169-193)setFromOSSL(169-169)setFromOSSL(296-321)setFromOSSL(296-296)resetOSSLKey(195-202)resetOSSLKey(195-195)resetOSSLKey(323-330)resetOSSLKey(323-323)
src/lib/crypto/OSSLDHPublicKey.cpp (4)
src/lib/crypto/OSSLDHPublicKey.h (1)
OSSLDHPublicKey(44-97)src/lib/crypto/OSSLDHPrivateKey.cpp (23)
isOfType(81-84)isOfType(81-81)inDH(61-68)resetOSSLKey(195-202)resetOSSLKey(195-195)resetOSSLKey(323-330)resetOSSLKey(323-323)setP(96-101)setP(96-96)setG(103-108)setG(103-103)getOSSLKey(161-166)getOSSLKey(161-161)getOSSLKey(289-293)getOSSLKey(289-289)createOSSLKey(205-249)createOSSLKey(205-205)createOSSLKey(333-381)createOSSLKey(333-333)setFromOSSL(169-193)setFromOSSL(169-169)setFromOSSL(296-321)setFromOSSL(296-296)src/lib/crypto/DHPublicKey.cpp (2)
setY(70-73)setY(70-70)src/lib/crypto/OSSLUtil.cpp (4)
bn2ByteString(42-53)bn2ByteString(42-42)byteString2bn(56-61)byteString2bn(56-56)
src/lib/crypto/OSSLDHPrivateKey.cpp (3)
src/lib/crypto/OSSLDHPublicKey.cpp (21)
inDH(69-76)resetOSSLKey(143-150)resetOSSLKey(143-143)resetOSSLKey(224-231)resetOSSLKey(224-224)setG(93-98)setG(93-93)setFromOSSL(117-141)setFromOSSL(117-117)setFromOSSL(197-222)setFromOSSL(197-197)setP(86-91)setP(86-86)createOSSLKey(153-186)createOSSLKey(153-153)createOSSLKey(234-282)createOSSLKey(234-234)getOSSLKey(109-114)getOSSLKey(109-109)getOSSLKey(190-194)getOSSLKey(190-190)src/lib/crypto/OSSLComp.cpp (4)
DH_get0_pqg(100-109)DH_get0_pqg(100-101)DH_get0_key(144-150)DH_get0_key(144-144)src/lib/crypto/OSSLUtil.cpp (4)
bn2ByteString(42-53)bn2ByteString(42-42)byteString2bn(56-61)byteString2bn(56-56)
⏰ 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: macOS (botan)
- GitHub Check: Windows (x64, botan)
- GitHub Check: macOS (openssl)
- GitHub Check: Linux with OpenSSL 3.0
- GitHub Check: Linux with OpenSSL 1.1.1
- GitHub Check: Linux with Botan
🔇 Additional comments (4)
src/lib/crypto/OSSLDHPublicKey.h (1)
38-42: No change required — <openssl/core_names.h> is present.
Found in src/lib/crypto/OSSLDHPublicKey.cpp:44; header is fine as-is.src/lib/crypto/OSSLDHPrivateKey.h (1)
38-42: Version-gated includes are correct — core_names already included in OSSLDHPrivateKey.cppLocated at src/lib/crypto/OSSLDHPrivateKey.cpp:45.
src/lib/crypto/OSSLDH.cpp (2)
135-146: Memory leak on error path in OpenSSL < 3.0 key generationIf
DH_new()fails,bn_pandbn_gare properly freed. However, ifDH_set0_pqg()fails on line 150, only then arebn_pandbn_gfreed. According to OpenSSL documentation,DH_set0_pqg()takes ownership of the BIGNUMs on success, so freeing them after a failed call is correct. But this pattern is error-prone and could lead to double-free if misunderstood in future modifications.
220-230: Potential double key generation in OpenSSL 3.0 pathThe code creates an initial EVP_PKEY from parameters using
EVP_PKEY_fromdata, then immediately creates a new context from this key and callsEVP_PKEY_keygenagain. This appears to generate a second key, overwriting the first one. The initial key generation viaEVP_PKEY_fromdatawithEVP_PKEY_KEYPAIRshould already create a complete key pair.Consider removing the redundant key generation:
- EVP_PKEY_CTX_free(ctx); - ctx = EVP_PKEY_CTX_new(dh, NULL); - if (EVP_PKEY_keygen_init(ctx) <= 0 || EVP_PKEY_keygen(ctx, &dh) <= 0) { - ERROR_MSG("DH key generation failed"); - BN_free(bn_p); - BN_free(bn_g); - OSSL_PARAM_free(params_built); - OSSL_PARAM_BLD_free(bld); - EVP_PKEY_CTX_free(ctx); - EVP_PKEY_free(dh); - return false; - }Likely an incorrect or invalid review comment.
bukka
force-pushed
the
openssl-3-migration-ossldh
branch
from
bbf834f to
28c33ea
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (6)
src/lib/crypto/OSSLDH.cpp (1)
476-478: Check return values ofEVP_PKEY_get_bn_param.The return values are not checked. If extraction fails,
bn_pandbn_gremain NULL, andOSSL::bn2ByteStringwill return empty ByteStrings, resulting in invalid DHParameters being stored.- EVP_PKEY_get_bn_param(dh_params, OSSL_PKEY_PARAM_FFC_P, &bn_p); - EVP_PKEY_get_bn_param(dh_params, OSSL_PKEY_PARAM_FFC_G, &bn_g); + if (!EVP_PKEY_get_bn_param(dh_params, OSSL_PKEY_PARAM_FFC_P, &bn_p) || + !EVP_PKEY_get_bn_param(dh_params, OSSL_PKEY_PARAM_FFC_G, &bn_g)) + { + ERROR_MSG("Failed to get DH parameters"); + EVP_PKEY_free(dh_params); + BN_free(bn_p); + BN_free(bn_g); + return false; + }src/lib/crypto/OSSLDHPublicKey.cpp (2)
242-268: Missing BIGNUM cleanup on early return paths increateOSSLKey.BIGNUMs allocated at lines 238-240 are not freed when
EVP_PKEY_CTX_new_id(line 243),OSSL_PARAM_BLD_new(line 250), orOSSL_PARAM_BLD_to_param(line 262) fail.if (!ctx) { ERROR_MSG("Could not create EVP_PKEY_CTX object"); + BN_free(bn_p); + BN_free(bn_g); + BN_free(bn_pub_key); return; } OSSL_PARAM_BLD *bld = OSSL_PARAM_BLD_new(); if (!bld) { ERROR_MSG("Could not create OSSL_PARAM_BLD"); EVP_PKEY_CTX_free(ctx); + BN_free(bn_p); + BN_free(bn_g); + BN_free(bn_pub_key); return; } ... if (!params) { ERROR_MSG("OSSL_PARAM_BLD_to_param failed"); OSSL_PARAM_BLD_free(bld); EVP_PKEY_CTX_free(ctx); + BN_free(bn_p); + BN_free(bn_g); + BN_free(bn_pub_key); return; }
199-202: UncheckedEVP_PKEY_get_bn_paramreturn values.While the code handles NULL BIGNUMs gracefully, silently ignoring extraction failures could mask underlying issues with malformed keys.
- EVP_PKEY_get_bn_param(inDH, OSSL_PKEY_PARAM_FFC_P, &bn_p); - EVP_PKEY_get_bn_param(inDH, OSSL_PKEY_PARAM_FFC_G, &bn_g); - EVP_PKEY_get_bn_param(inDH, OSSL_PKEY_PARAM_PUB_KEY, &bn_pub_key); + if (!EVP_PKEY_get_bn_param(inDH, OSSL_PKEY_PARAM_FFC_P, &bn_p)) + bn_p = NULL; + if (!EVP_PKEY_get_bn_param(inDH, OSSL_PKEY_PARAM_FFC_G, &bn_g)) + bn_g = NULL; + if (!EVP_PKEY_get_bn_param(inDH, OSSL_PKEY_PARAM_PUB_KEY, &bn_pub_key)) + bn_pub_key = NULL;src/lib/crypto/OSSLDHPrivateKey.cpp (3)
276-286: Missing resource cleanup inPKCS8Decode.The
PKCS8_PRIV_KEY_INFO* p8(line 281) andEVP_PKEY* pkey(line 283) are not freed before returning, causing memory leaks.EVP_PKEY* pkey = EVP_PKCS82PKEY(p8); + PKCS8_PRIV_KEY_INFO_free(p8); + if (pkey == NULL) return false; setFromOSSL(pkey); + EVP_PKEY_free(pkey); return true;
341-367: Missing BIGNUM cleanup on early return paths increateOSSLKey.BIGNUMs allocated at lines 337-339 are not freed when
EVP_PKEY_CTX_new_id,OSSL_PARAM_BLD_new, orOSSL_PARAM_BLD_to_paramfail. This is particularly concerning forbn_priv_keywhich contains sensitive key material.if (!ctx) { ERROR_MSG("Could not create EVP_PKEY_CTX object"); + BN_free(bn_p); + BN_free(bn_g); + BN_clear_free(bn_priv_key); return; } OSSL_PARAM_BLD *bld = OSSL_PARAM_BLD_new(); if (!bld) { ERROR_MSG("Could not create OSSL_PARAM_BLD"); EVP_PKEY_CTX_free(ctx); + BN_free(bn_p); + BN_free(bn_g); + BN_clear_free(bn_priv_key); return; } ... if (!params) { ERROR_MSG("OSSL_PARAM_BLD_to_param failed"); OSSL_PARAM_BLD_free(bld); EVP_PKEY_CTX_free(ctx); + BN_free(bn_p); + BN_free(bn_g); + BN_clear_free(bn_priv_key); return; }Note: Use
BN_clear_freeforbn_priv_keyto securely wipe the private key material.
298-301: UncheckedEVP_PKEY_get_bn_paramreturn values.Same issue as in OSSLDHPublicKey.cpp - extraction failures are silently ignored.
- EVP_PKEY_get_bn_param(inDH, OSSL_PKEY_PARAM_FFC_P, &bn_p); - EVP_PKEY_get_bn_param(inDH, OSSL_PKEY_PARAM_FFC_G, &bn_g); - EVP_PKEY_get_bn_param(inDH, OSSL_PKEY_PARAM_PRIV_KEY, &bn_priv_key); + if (!EVP_PKEY_get_bn_param(inDH, OSSL_PKEY_PARAM_FFC_P, &bn_p)) + bn_p = NULL; + if (!EVP_PKEY_get_bn_param(inDH, OSSL_PKEY_PARAM_FFC_G, &bn_g)) + bn_g = NULL; + if (!EVP_PKEY_get_bn_param(inDH, OSSL_PKEY_PARAM_PRIV_KEY, &bn_priv_key)) + bn_priv_key = NULL;
🧹 Nitpick comments (1)
src/lib/crypto/test/DHTests.cpp (1)
94-99: Stale comment after test modification.The comment "Generate key-pair with a fixed private value length" at line 94 is now misleading since the
setXBitLength(128)call was removed. The test no longer sets a fixed private value length; it simply generates another key-pair and verifies the overall key length.Consider updating the comment to reflect the actual test intent:
- // Generate key-pair with a fixed private value length + // Generate another key-pair and verify bit length CPPUNIT_ASSERT(dh->generateKeyPair(&kp, p));
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/lib/crypto/OSSLDH.cpp(8 hunks)src/lib/crypto/OSSLDHPrivateKey.cpp(7 hunks)src/lib/crypto/OSSLDHPrivateKey.h(2 hunks)src/lib/crypto/OSSLDHPublicKey.cpp(4 hunks)src/lib/crypto/OSSLDHPublicKey.h(2 hunks)src/lib/crypto/test/DHTests.cpp(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/lib/crypto/OSSLDHPrivateKey.h
🧰 Additional context used
🧠 Learnings (10)
📓 Common learnings
Learnt from: bukka
Repo: softhsm/SoftHSMv2 PR: 815
File: src/lib/SoftHSM.cpp:6543-6546
Timestamp: 2025-09-11T16:54:00.370Z
Learning: SoftHSM PR #815 (pkcs11.h 3.1.0 update, CK_RSA_AES_KEY_WRAP_PARAMS fix): Maintainer bukka prefers to keep scope minimal (header sync and param rename only); broader OAEP parameter enforcement or backend plumbing is out of scope for this PR and should be handled in a follow-up issue.
Learnt from: bukka
Repo: softhsm/SoftHSMv2 PR: 818
File: src/lib/SoftHSM.cpp:3238-3266
Timestamp: 2025-10-13T09:42:31.270Z
Learning: In src/lib/SoftHSM.cpp, SoftHSM::AsymDecryptInit should use MechParamCheckRSAPKCSOAEP for CKM_RSA_PKCS_OAEP to keep OAEP parameter validation identical to AsymEncryptInit, and avoid redundant manual checks for pMechanism->pParameter and MGF allowlists.
Learnt from: antoinelochet
Repo: softhsm/SoftHSMv2 PR: 809
File: m4/acx_openssl_mldsa.m4:10-35
Timestamp: 2025-08-18T17:54:37.645Z
Learning: For OpenSSL feature detection in autoconf macros, prefer functional tests (like EVP_PKEY_CTX_new_from_name) over version number checks, as the functional approach directly tests the actual capability and is more reliable across different OpenSSL builds where features may be backported or disabled.
📚 Learning: 2025-09-11T16:54:00.370Z
Learnt from: bukka
Repo: softhsm/SoftHSMv2 PR: 815
File: src/lib/SoftHSM.cpp:6543-6546
Timestamp: 2025-09-11T16:54:00.370Z
Learning: SoftHSM PR #815 (pkcs11.h 3.1.0 update, CK_RSA_AES_KEY_WRAP_PARAMS fix): Maintainer bukka prefers to keep scope minimal (header sync and param rename only); broader OAEP parameter enforcement or backend plumbing is out of scope for this PR and should be handled in a follow-up issue.
Applied to files:
src/lib/crypto/OSSLDH.cppsrc/lib/crypto/OSSLDHPublicKey.hsrc/lib/crypto/OSSLDHPrivateKey.cppsrc/lib/crypto/OSSLDHPublicKey.cpp
📚 Learning: 2025-11-11T19:42:46.886Z
Learnt from: JoaoHenrique12
Repo: softhsm/SoftHSMv2 PR: 823
File: src/lib/SoftHSM.cpp:58-58
Timestamp: 2025-11-11T19:42:46.886Z
Learning: SoftHSM include policy: Algorithm key/parameter headers (e.g., SLHParameters, SLH*Key) are included unconditionally in SoftHSM.cpp/SoftHSM.h; feature gating is handled in mechanism tables and CryptoFactory (WITH_*). Avoid wrapping such includes in WITH_* guards.
Applied to files:
src/lib/crypto/OSSLDH.cpp
📚 Learning: 2025-10-13T09:42:31.270Z
Learnt from: bukka
Repo: softhsm/SoftHSMv2 PR: 818
File: src/lib/SoftHSM.cpp:3238-3266
Timestamp: 2025-10-13T09:42:31.270Z
Learning: In src/lib/SoftHSM.cpp, SoftHSM::AsymDecryptInit should use MechParamCheckRSAPKCSOAEP for CKM_RSA_PKCS_OAEP to keep OAEP parameter validation identical to AsymEncryptInit, and avoid redundant manual checks for pMechanism->pParameter and MGF allowlists.
Applied to files:
src/lib/crypto/OSSLDH.cppsrc/lib/crypto/OSSLDHPublicKey.hsrc/lib/crypto/OSSLDHPrivateKey.cppsrc/lib/crypto/OSSLDHPublicKey.cpp
📚 Learning: 2025-10-27T16:44:57.865Z
Learnt from: antoinelochet
Repo: softhsm/SoftHSMv2 PR: 809
File: src/lib/SoftHSM.cpp:5550-5596
Timestamp: 2025-10-27T16:44:57.865Z
Learning: SoftHSM ML‑DSA additional context: avoid transient malloc in SoftHSM::{AsymSignInit, AsymVerifyInit}; let Session::setParameters deep‑copy SIGN_ADDITIONAL_CONTEXT by allocating one buffer for struct + context bytes and fixing the stored pointer; Session::resetOp frees m_pParameters so both struct and context are released.
Applied to files:
src/lib/crypto/OSSLDH.cppsrc/lib/crypto/test/DHTests.cppsrc/lib/crypto/OSSLDHPrivateKey.cppsrc/lib/crypto/OSSLDHPublicKey.cpp
📚 Learning: 2025-08-19T09:23:25.434Z
Learnt from: antoinelochet
Repo: softhsm/SoftHSMv2 PR: 809
File: src/lib/SoftHSM.cpp:4193-4196
Timestamp: 2025-08-19T09:23:25.434Z
Learning: MLDSA parameter set inference: In src/lib/crypto/MLDSAPrivateKey.{h,cpp} and MLDSAPublicKey.{h,cpp}, getParameterSet() maps key material length to CKP_ML_DSA_44/65/87, and there is no setParameterSet(). Runtime MLDSA keys used by SoftHSM (e.g., in C_SignInit/C_VerifyInit via MLDSAUtil) rely on this inference; no explicit parameter-set assignment is required.
Applied to files:
src/lib/crypto/OSSLDH.cppsrc/lib/crypto/test/DHTests.cppsrc/lib/crypto/OSSLDHPrivateKey.cppsrc/lib/crypto/OSSLDHPublicKey.cpp
📚 Learning: 2025-08-18T17:54:37.645Z
Learnt from: antoinelochet
Repo: softhsm/SoftHSMv2 PR: 809
File: m4/acx_openssl_mldsa.m4:10-35
Timestamp: 2025-08-18T17:54:37.645Z
Learning: For OpenSSL feature detection in autoconf macros, prefer functional tests (like EVP_PKEY_CTX_new_from_name) over version number checks, as the functional approach directly tests the actual capability and is more reliable across different OpenSSL builds where features may be backported or disabled.
Applied to files:
src/lib/crypto/OSSLDH.cppsrc/lib/crypto/OSSLDHPrivateKey.cppsrc/lib/crypto/OSSLDHPublicKey.cpp
📚 Learning: 2025-10-13T09:43:45.885Z
Learnt from: bukka
Repo: softhsm/SoftHSMv2 PR: 818
File: src/lib/SoftHSM.cpp:13265-13282
Timestamp: 2025-10-13T09:43:45.885Z
Learning: SoftHSM OAEP policy: In SoftHSM::MechParamCheckRSAPKCSOAEP (src/lib/SoftHSM.cpp), enforce that the MGF1 variant must correspond to the selected hashAlg (e.g., SHA-256 -> CKG_MGF1_SHA256). Mismatches should return CKR_ARGUMENTS_BAD with "Hash and MGF don't match". This mirrors the existing PSS enforcement.
Applied to files:
src/lib/crypto/OSSLDH.cppsrc/lib/crypto/OSSLDHPrivateKey.cppsrc/lib/crypto/OSSLDHPublicKey.cpp
📚 Learning: 2025-08-18T19:46:46.659Z
Learnt from: antoinelochet
Repo: softhsm/SoftHSMv2 PR: 809
File: src/lib/SoftHSM.cpp:10044-10055
Timestamp: 2025-08-18T19:46:46.659Z
Learning: In SoftHSM::generateMLDSA (src/lib/SoftHSM.cpp), the project prefers using CKP_ML_DSA_* constants for parameter set checks and logs "Unsupported parameter set: %lu". The code currently returns CKR_PARAMETER_SET_NOT_SUPPORTED for unsupported sets.
Applied to files:
src/lib/crypto/OSSLDH.cppsrc/lib/crypto/test/DHTests.cppsrc/lib/crypto/OSSLDHPrivateKey.cppsrc/lib/crypto/OSSLDHPublicKey.cpp
📚 Learning: 2025-08-11T16:42:23.012Z
Learnt from: bukka
Repo: softhsm/SoftHSMv2 PR: 806
File: src/lib/test/p11test.cpp:81-84
Timestamp: 2025-08-11T16:42:23.012Z
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/OSSLDH.cppsrc/lib/crypto/OSSLDHPrivateKey.cpp
🧬 Code graph analysis (4)
src/lib/crypto/OSSLDH.cpp (1)
src/lib/crypto/OSSLUtil.cpp (4)
byteString2bn(56-61)byteString2bn(56-56)bn2ByteString(42-53)bn2ByteString(42-42)
src/lib/crypto/OSSLDHPublicKey.h (2)
src/lib/crypto/OSSLDHPublicKey.cpp (11)
OSSLDHPublicKey(54-57)OSSLDHPublicKey(79-82)inDH(69-76)getOSSLKey(109-114)getOSSLKey(109-109)getOSSLKey(190-194)getOSSLKey(190-190)setFromOSSL(117-141)setFromOSSL(117-117)setFromOSSL(197-222)setFromOSSL(197-197)src/lib/crypto/DHPublicKey.h (1)
DHPublicKey(39-71)
src/lib/crypto/OSSLDHPrivateKey.cpp (2)
src/lib/crypto/OSSLDHPublicKey.cpp (17)
inDH(69-76)resetOSSLKey(143-150)resetOSSLKey(143-143)resetOSSLKey(224-231)resetOSSLKey(224-224)setG(93-98)setG(93-93)setFromOSSL(117-141)setFromOSSL(117-117)setFromOSSL(197-222)setFromOSSL(197-197)setP(86-91)setP(86-86)createOSSLKey(153-186)createOSSLKey(153-153)createOSSLKey(234-282)createOSSLKey(234-234)src/lib/crypto/OSSLUtil.cpp (4)
bn2ByteString(42-53)bn2ByteString(42-42)byteString2bn(56-61)byteString2bn(56-56)
src/lib/crypto/OSSLDHPublicKey.cpp (1)
src/lib/crypto/OSSLDHPrivateKey.cpp (21)
inDH(61-68)resetOSSLKey(195-202)resetOSSLKey(195-195)resetOSSLKey(323-330)resetOSSLKey(323-323)setP(96-101)setP(96-96)setG(103-108)setG(103-103)getOSSLKey(161-166)getOSSLKey(161-161)getOSSLKey(289-293)getOSSLKey(289-289)createOSSLKey(205-249)createOSSLKey(205-205)createOSSLKey(333-381)createOSSLKey(333-333)setFromOSSL(169-193)setFromOSSL(169-169)setFromOSSL(296-321)setFromOSSL(296-296)
⏰ 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 (openssl)
- GitHub Check: Linux with OpenSSL 3.0
- GitHub Check: macOS (botan)
- GitHub Check: Linux with Botan
- GitHub Check: Linux with OpenSSL 1.1.1
🔇 Additional comments (5)
src/lib/crypto/OSSLDH.cpp (1)
316-367: OpenSSL 3.x derive path looks correct.The derivation flow properly initializes the context, sets the peer, queries the secret length, allocates the buffer, performs the derivation, and cleans up the context on all paths.
src/lib/crypto/OSSLDHPublicKey.h (1)
38-97: Header correctly implements dual-backend interface.The version-gated declarations maintain type consistency between the member variable, constructor, getter, and setter signatures. The
resetOSSLKey()helper centralizes cleanup logic.src/lib/crypto/OSSLDHPublicKey.cpp (1)
50-105: Clean implementation of type checking and setter invalidation.The
isOfTypemethod and setters that callresetOSSLKey()to invalidate the cached OpenSSL representation when components change follow a clean pattern consistent with the private key class.src/lib/crypto/OSSLDHPrivateKey.cpp (2)
168-202: Legacy path implementation is clean.The pre-3.0
setFromOSSL,resetOSSLKey, and unchangedcreateOSSLKeyfollow established patterns with proper resource management.
375-380: Final cleanup increateOSSLKeyhandles the success path correctly.All allocated resources (params, bld, ctx, BNs) are properly freed after successful key creation.
bukka
force-pushed
the
openssl-3-migration-ossldh
branch
from
28c33ea to
2683431
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In @src/lib/crypto/OSSLDHPrivateKey.cpp:
- Around line 276-286: PKCS8Decode leaks both PKCS8_PRIV_KEY_INFO* p8 and
EVP_PKEY* pkey; after calling d2i_PKCS8_PRIV_KEY_INFO and EVP_PKCS82PKEY, ensure
you free p8 with PKCS8_PRIV_KEY_INFO_free(p8) and free pkey with
EVP_PKEY_free(pkey) once setFromOSSL has consumed the EVP_PKEY (and also on any
early returns/failure paths), updating OSSLDHPrivateKey::PKCS8Decode to call
those frees and to null-check before freeing.
In @src/lib/crypto/OSSLDHPublicKey.cpp:
- Around line 238-268: The BIGNUMs bn_p, bn_g and bn_pub_key allocated by
OSSL::byteString2bn are leaked on the failure branches; update createOSSLKey so
that any early return after allocating bn_p/bn_g/bn_pub_key frees them (BN_free)
— specifically free bn_p, bn_g, bn_pub_key before returning when
EVP_PKEY_CTX_new_id fails, when OSSL_PARAM_BLD_new fails, and when
OSSL_PARAM_BLD_to_param fails; preserve existing cleanup for ctx and bld
(EVP_PKEY_CTX_free, OSSL_PARAM_BLD_free) and ensure no double-free if ownership
is transferred later (only free the bn_* on error paths prior to successful
ownership transfer).
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/lib/crypto/OSSLDH.cppsrc/lib/crypto/OSSLDHPrivateKey.cppsrc/lib/crypto/OSSLDHPrivateKey.hsrc/lib/crypto/OSSLDHPublicKey.cppsrc/lib/crypto/OSSLDHPublicKey.hsrc/lib/crypto/test/DHTests.cpp
🚧 Files skipped from review as they are similar to previous changes (1)
- src/lib/crypto/test/DHTests.cpp
🧰 Additional context used
🧠 Learnings (10)
📓 Common learnings
Learnt from: bukka
Repo: softhsm/SoftHSMv2 PR: 815
File: src/lib/SoftHSM.cpp:6543-6546
Timestamp: 2025-09-11T16:54:00.370Z
Learning: SoftHSM PR #815 (pkcs11.h 3.1.0 update, CK_RSA_AES_KEY_WRAP_PARAMS fix): Maintainer bukka prefers to keep scope minimal (header sync and param rename only); broader OAEP parameter enforcement or backend plumbing is out of scope for this PR and should be handled in a follow-up issue.
Learnt from: antoinelochet
Repo: softhsm/SoftHSMv2 PR: 809
File: m4/acx_openssl_mldsa.m4:10-35
Timestamp: 2025-08-18T17:54:37.645Z
Learning: For OpenSSL feature detection in autoconf macros, prefer functional tests (like EVP_PKEY_CTX_new_from_name) over version number checks, as the functional approach directly tests the actual capability and is more reliable across different OpenSSL builds where features may be backported or disabled.
Learnt from: antoinelochet
Repo: softhsm/SoftHSMv2 PR: 809
File: src/lib/SoftHSM.cpp:4193-4196
Timestamp: 2025-08-19T09:23:25.434Z
Learning: MLDSA parameter set inference: In src/lib/crypto/MLDSAPrivateKey.{h,cpp} and MLDSAPublicKey.{h,cpp}, getParameterSet() maps key material length to CKP_ML_DSA_44/65/87, and there is no setParameterSet(). Runtime MLDSA keys used by SoftHSM (e.g., in C_SignInit/C_VerifyInit via MLDSAUtil) rely on this inference; no explicit parameter-set assignment is required.
Learnt from: antoinelochet
Repo: softhsm/SoftHSMv2 PR: 809
File: src/lib/SoftHSM.cpp:5550-5596
Timestamp: 2025-10-27T16:44:57.865Z
Learning: SoftHSM ML‑DSA additional context: avoid transient malloc in SoftHSM::{AsymSignInit, AsymVerifyInit}; let Session::setParameters deep‑copy SIGN_ADDITIONAL_CONTEXT by allocating one buffer for struct + context bytes and fixing the stored pointer; Session::resetOp frees m_pParameters so both struct and context are released.
📚 Learning: 2025-09-11T16:54:00.370Z
Learnt from: bukka
Repo: softhsm/SoftHSMv2 PR: 815
File: src/lib/SoftHSM.cpp:6543-6546
Timestamp: 2025-09-11T16:54:00.370Z
Learning: SoftHSM PR #815 (pkcs11.h 3.1.0 update, CK_RSA_AES_KEY_WRAP_PARAMS fix): Maintainer bukka prefers to keep scope minimal (header sync and param rename only); broader OAEP parameter enforcement or backend plumbing is out of scope for this PR and should be handled in a follow-up issue.
Applied to files:
src/lib/crypto/OSSLDHPublicKey.hsrc/lib/crypto/OSSLDH.cppsrc/lib/crypto/OSSLDHPrivateKey.hsrc/lib/crypto/OSSLDHPublicKey.cppsrc/lib/crypto/OSSLDHPrivateKey.cpp
📚 Learning: 2025-11-11T19:42:46.886Z
Learnt from: JoaoHenrique12
Repo: softhsm/SoftHSMv2 PR: 823
File: src/lib/SoftHSM.cpp:58-58
Timestamp: 2025-11-11T19:42:46.886Z
Learning: SoftHSM include policy: Algorithm key/parameter headers (e.g., SLHParameters, SLH*Key) are included unconditionally in SoftHSM.cpp/SoftHSM.h; feature gating is handled in mechanism tables and CryptoFactory (WITH_*). Avoid wrapping such includes in WITH_* guards.
Applied to files:
src/lib/crypto/OSSLDHPublicKey.hsrc/lib/crypto/OSSLDH.cpp
📚 Learning: 2025-10-13T09:42:31.270Z
Learnt from: bukka
Repo: softhsm/SoftHSMv2 PR: 818
File: src/lib/SoftHSM.cpp:3238-3266
Timestamp: 2025-10-13T09:42:31.270Z
Learning: In src/lib/SoftHSM.cpp, SoftHSM::AsymDecryptInit should use MechParamCheckRSAPKCSOAEP for CKM_RSA_PKCS_OAEP to keep OAEP parameter validation identical to AsymEncryptInit, and avoid redundant manual checks for pMechanism->pParameter and MGF allowlists.
Applied to files:
src/lib/crypto/OSSLDH.cppsrc/lib/crypto/OSSLDHPublicKey.cppsrc/lib/crypto/OSSLDHPrivateKey.cpp
📚 Learning: 2025-10-27T16:44:57.865Z
Learnt from: antoinelochet
Repo: softhsm/SoftHSMv2 PR: 809
File: src/lib/SoftHSM.cpp:5550-5596
Timestamp: 2025-10-27T16:44:57.865Z
Learning: SoftHSM ML‑DSA additional context: avoid transient malloc in SoftHSM::{AsymSignInit, AsymVerifyInit}; let Session::setParameters deep‑copy SIGN_ADDITIONAL_CONTEXT by allocating one buffer for struct + context bytes and fixing the stored pointer; Session::resetOp frees m_pParameters so both struct and context are released.
Applied to files:
src/lib/crypto/OSSLDH.cppsrc/lib/crypto/OSSLDHPublicKey.cppsrc/lib/crypto/OSSLDHPrivateKey.cpp
📚 Learning: 2025-08-19T09:23:25.434Z
Learnt from: antoinelochet
Repo: softhsm/SoftHSMv2 PR: 809
File: src/lib/SoftHSM.cpp:4193-4196
Timestamp: 2025-08-19T09:23:25.434Z
Learning: MLDSA parameter set inference: In src/lib/crypto/MLDSAPrivateKey.{h,cpp} and MLDSAPublicKey.{h,cpp}, getParameterSet() maps key material length to CKP_ML_DSA_44/65/87, and there is no setParameterSet(). Runtime MLDSA keys used by SoftHSM (e.g., in C_SignInit/C_VerifyInit via MLDSAUtil) rely on this inference; no explicit parameter-set assignment is required.
Applied to files:
src/lib/crypto/OSSLDH.cppsrc/lib/crypto/OSSLDHPublicKey.cppsrc/lib/crypto/OSSLDHPrivateKey.cpp
📚 Learning: 2025-08-18T19:46:46.659Z
Learnt from: antoinelochet
Repo: softhsm/SoftHSMv2 PR: 809
File: src/lib/SoftHSM.cpp:10044-10055
Timestamp: 2025-08-18T19:46:46.659Z
Learning: In SoftHSM::generateMLDSA (src/lib/SoftHSM.cpp), the project prefers using CKP_ML_DSA_* constants for parameter set checks and logs "Unsupported parameter set: %lu". The code currently returns CKR_PARAMETER_SET_NOT_SUPPORTED for unsupported sets.
Applied to files:
src/lib/crypto/OSSLDH.cppsrc/lib/crypto/OSSLDHPublicKey.cppsrc/lib/crypto/OSSLDHPrivateKey.cpp
📚 Learning: 2025-08-18T17:54:37.645Z
Learnt from: antoinelochet
Repo: softhsm/SoftHSMv2 PR: 809
File: m4/acx_openssl_mldsa.m4:10-35
Timestamp: 2025-08-18T17:54:37.645Z
Learning: For OpenSSL feature detection in autoconf macros, prefer functional tests (like EVP_PKEY_CTX_new_from_name) over version number checks, as the functional approach directly tests the actual capability and is more reliable across different OpenSSL builds where features may be backported or disabled.
Applied to files:
src/lib/crypto/OSSLDH.cppsrc/lib/crypto/OSSLDHPublicKey.cppsrc/lib/crypto/OSSLDHPrivateKey.cpp
📚 Learning: 2025-08-11T16:42:23.012Z
Learnt from: bukka
Repo: softhsm/SoftHSMv2 PR: 806
File: src/lib/test/p11test.cpp:81-84
Timestamp: 2025-08-11T16:42:23.012Z
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/OSSLDH.cppsrc/lib/crypto/OSSLDHPrivateKey.cpp
📚 Learning: 2025-08-18T17:59:30.693Z
Learnt from: antoinelochet
Repo: softhsm/SoftHSMv2 PR: 809
File: src/lib/crypto/MLDSAParameters.h:42-44
Timestamp: 2025-08-18T17:59:30.693Z
Learning: In MLDSAParameters.h, the author prefers to keep PKCS#11 constant references (CKP_ML_DSA_*) rather than explicit numeric values for better code clarity and maintainability.
Applied to files:
src/lib/crypto/OSSLDHPublicKey.cpp
🧬 Code graph analysis (5)
src/lib/crypto/OSSLDHPublicKey.h (5)
src/lib/crypto/OSSLDHPublicKey.cpp (15)
OSSLDHPublicKey(54-57)OSSLDHPublicKey(79-82)inDH(69-76)getOSSLKey(109-114)getOSSLKey(109-109)getOSSLKey(190-194)getOSSLKey(190-190)setFromOSSL(117-141)setFromOSSL(117-117)setFromOSSL(197-222)setFromOSSL(197-197)resetOSSLKey(143-150)resetOSSLKey(143-143)resetOSSLKey(224-231)resetOSSLKey(224-224)src/lib/crypto/OSSLDHPrivateKey.cpp (13)
inDH(61-68)getOSSLKey(161-166)getOSSLKey(161-161)getOSSLKey(289-293)getOSSLKey(289-289)setFromOSSL(169-193)setFromOSSL(169-169)setFromOSSL(296-321)setFromOSSL(296-296)resetOSSLKey(195-202)resetOSSLKey(195-195)resetOSSLKey(323-330)resetOSSLKey(323-323)src/lib/crypto/OSSLDSAPublicKey.cpp (4)
getOSSLKey(151-156)getOSSLKey(151-151)setFromOSSL(67-97)setFromOSSL(67-67)src/lib/crypto/OSSLDSAPrivateKey.cpp (4)
getOSSLKey(203-208)getOSSLKey(203-203)setFromOSSL(68-98)setFromOSSL(68-68)src/lib/crypto/OSSLRSAPublicKey.cpp (4)
getOSSLKey(116-121)getOSSLKey(116-116)setFromOSSL(73-90)setFromOSSL(73-73)
src/lib/crypto/OSSLDH.cpp (1)
src/lib/crypto/OSSLUtil.cpp (4)
byteString2bn(56-61)byteString2bn(56-56)bn2ByteString(42-53)bn2ByteString(42-42)
src/lib/crypto/OSSLDHPrivateKey.h (5)
src/lib/crypto/OSSLDHPrivateKey.cpp (15)
OSSLDHPrivateKey(52-55)OSSLDHPrivateKey(71-74)inDH(61-68)getOSSLKey(161-166)getOSSLKey(161-161)getOSSLKey(289-293)getOSSLKey(289-289)setFromOSSL(169-193)setFromOSSL(169-169)setFromOSSL(296-321)setFromOSSL(296-296)resetOSSLKey(195-202)resetOSSLKey(195-195)resetOSSLKey(323-330)resetOSSLKey(323-323)src/lib/crypto/OSSLDHPublicKey.cpp (13)
inDH(69-76)getOSSLKey(109-114)getOSSLKey(109-109)getOSSLKey(190-194)getOSSLKey(190-190)setFromOSSL(117-141)setFromOSSL(117-117)setFromOSSL(197-222)setFromOSSL(197-197)resetOSSLKey(143-150)resetOSSLKey(143-143)resetOSSLKey(224-231)resetOSSLKey(224-224)src/lib/crypto/OSSLDSAPublicKey.cpp (4)
getOSSLKey(151-156)getOSSLKey(151-151)setFromOSSL(67-97)setFromOSSL(67-67)src/lib/crypto/OSSLDSAPrivateKey.cpp (4)
getOSSLKey(203-208)getOSSLKey(203-203)setFromOSSL(68-98)setFromOSSL(68-68)src/lib/crypto/OSSLRSAPrivateKey.cpp (4)
getOSSLKey(272-277)getOSSLKey(272-272)setFromOSSL(68-123)setFromOSSL(68-68)
src/lib/crypto/OSSLDHPublicKey.cpp (2)
src/lib/crypto/OSSLDHPrivateKey.cpp (17)
inDH(61-68)resetOSSLKey(195-202)resetOSSLKey(195-195)resetOSSLKey(323-330)resetOSSLKey(323-323)setP(96-101)setP(96-96)setG(103-108)setG(103-103)createOSSLKey(205-249)createOSSLKey(205-205)createOSSLKey(333-381)createOSSLKey(333-333)setFromOSSL(169-193)setFromOSSL(169-169)setFromOSSL(296-321)setFromOSSL(296-296)src/lib/crypto/OSSLUtil.cpp (4)
bn2ByteString(42-53)bn2ByteString(42-42)byteString2bn(56-61)byteString2bn(56-56)
src/lib/crypto/OSSLDHPrivateKey.cpp (1)
src/lib/crypto/OSSLDHPublicKey.cpp (17)
inDH(69-76)setG(93-98)setG(93-93)setFromOSSL(117-141)setFromOSSL(117-117)setFromOSSL(197-222)setFromOSSL(197-197)setP(86-91)setP(86-86)createOSSLKey(153-186)createOSSLKey(153-153)createOSSLKey(234-282)createOSSLKey(234-234)getOSSLKey(109-114)getOSSLKey(109-109)getOSSLKey(190-194)getOSSLKey(190-190)
⏰ 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, openssl)
- GitHub Check: Linux with OpenSSL 3.0
- GitHub Check: macOS (botan)
- GitHub Check: Linux with OpenSSL 1.1.1
- GitHub Check: Linux with Botan
- GitHub Check: macOS (openssl)
- GitHub Check: Windows (x64, botan)
🔇 Additional comments (18)
src/lib/crypto/OSSLDHPrivateKey.h (3)
38-42: LGTM: Version-gated includes are correctly structured.The conditional includes correctly pull in
<openssl/dh.h>for legacy OpenSSL and<openssl/evp.h>for OpenSSL 3.0+.
51-55: LGTM: Constructor overloads are correctly version-gated.The constructor signatures properly switch between
const DH*andconst EVP_PKEY*based on OpenSSL version.
79-103: LGTM: Public API and internal representation are consistently version-gated.The
getOSSLKey(),setFromOSSL(), internaldhmember, andresetOSSLKey()helper are all properly aligned with the version-specific types.src/lib/crypto/OSSLDHPrivateKey.cpp (6)
40-48: LGTM: Version-gated includes are correctly structured.The conditional includes properly add
<openssl/core_names.h>,<openssl/param_build.h>, and<openssl/provider.h>for OpenSSL 3.0+.
57-68: LGTM: Version-gated constructor implementation.The constructor correctly uses the version-appropriate parameter type and delegates to
setFromOSSL().
71-74: LGTM: Centralized cleanup via resetOSSLKey().The destructor and all setters correctly call
resetOSSLKey()to ensure the internal OpenSSL representation is invalidated when key components change.Also applies to: 86-108
168-202: LGTM: Pre-3.0 setFromOSSL and resetOSSLKey implementations.The legacy path correctly extracts DH parameters and uses
DH_free()for cleanup.
110-158: LGTM: Pre-3.0 PKCS8Encode and PKCS8Decode implementations.Resource management is correct with proper cleanup of
PKCS8_PRIV_KEY_INFO,EVP_PKEY, andDHobjects on all paths.
253-273: LGTM: OpenSSL 3.0 PKCS8Encode implementation.The encoding path correctly handles cleanup of
PKCS8_PRIV_KEY_INFOon all paths.src/lib/crypto/OSSLDHPublicKey.h (1)
38-42: LGTM: Header declarations are correctly version-gated and consistent with private key counterpart.The public key header follows the same pattern as the private key header, with appropriate type switches for OpenSSL 3.0+.
Also applies to: 50-54, 70-93
src/lib/crypto/OSSLDH.cpp (4)
42-48: LGTM: Version-gated includes for OpenSSL 3.0+.
316-367: LGTM: OpenSSL 3.0 deriveKey implementation.The EVP_PKEY_derive flow is correctly implemented with proper error handling and cleanup on all paths.
281-315: LGTM: Pre-3.0 deriveKey implementation with leading zero compensation.The legacy path correctly handles the DH key derivation including compensation for OpenSSL removing leading zeros.
413-433: LGTM: Pre-3.0 generateParameters implementation.The legacy path correctly uses
DH_generate_parameters_exand extracts parameters viaDH_get0_pqg.src/lib/crypto/OSSLDHPublicKey.cpp (4)
39-47: LGTM: Version-gated includes.
50-105: LGTM: Common code including type definition, constructors, destructor, and setters.All setters correctly invalidate the internal OpenSSL representation by calling
resetOSSLKey().
107-186: LGTM: Pre-3.0 implementation of getOSSLKey, setFromOSSL, resetOSSLKey, and createOSSLKey.The legacy path correctly handles DH object creation and cleanup.
224-231: LGTM: OpenSSL 3.0 resetOSSLKey implementation.Correctly uses
EVP_PKEY_free()for the 3.0+ path.
bukka
force-pushed
the
openssl-3-migration-ossldh
branch
from
2683431 to
30c4746
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In @src/lib/crypto/OSSLDH.cpp:
- Around line 473-488: The code calls EVP_PKEY_CTX_free(ctx) early and then
attempts to free ctx again in the EVP_PKEY_get_bn_param error branches, causing
a double-free; fix by removing the redundant EVP_PKEY_CTX_free(ctx) calls in the
error branches (leave the single free already done) or alternatively move the
initial EVP_PKEY_CTX_free(ctx) to after the EVP_PKEY_get_bn_param checks so ctx
is freed exactly once; ensure BN_free(bn_p) is still called when appropriate and
keep ERROR_MSG and return false behavior intact for the
OSSL_PKEY_PARAM_FFC_P/FFC_G error paths.
- Around line 219-230: Replace the EVP_PKEY_fromdata call that currently uses
EVP_PKEY_KEYPAIR with EVP_PKEY_KEY_PARAMETERS since only DH domain parameters
(P, G and optional length) are being initialized (before key generation). Update
the call site EVP_PKEY_fromdata(ctx, &dh, EVP_PKEY_KEYPAIR, params_built) to use
EVP_PKEY_KEY_PARAMETERS so EVP_PKEY_fromdata, the local variable dh, and
subsequent EVP_PKEY_keygen use the parameter-only initialization path correctly;
keep the existing error handling and cleanup logic intact.
In @src/lib/crypto/OSSLDHPrivateKey.cpp:
- Around line 276-288: In OSSLDHPrivateKey::PKCS8Decode, after calling
EVP_PKCS82PKEY(p8) you must check whether the returned EVP_PKEY* pkey is NULL
before calling setFromOSSL and EVP_PKEY_free; if pkey is NULL return false
(ensuring PKCS8_PRIV_KEY_INFO_free(p8) has already been called as in the current
flow), so add a null-check for pkey immediately after the EVP_PKCS82PKEY call
and only call setFromOSSL/EVP_PKEY_free when pkey is non-NULL.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/lib/crypto/OSSLDH.cppsrc/lib/crypto/OSSLDHPrivateKey.cppsrc/lib/crypto/OSSLDHPrivateKey.hsrc/lib/crypto/OSSLDHPublicKey.cppsrc/lib/crypto/OSSLDHPublicKey.hsrc/lib/crypto/test/DHTests.cpp
🚧 Files skipped from review as they are similar to previous changes (2)
- src/lib/crypto/test/DHTests.cpp
- src/lib/crypto/OSSLDHPublicKey.cpp
🧰 Additional context used
🧠 Learnings (9)
📓 Common learnings
Learnt from: bukka
Repo: softhsm/SoftHSMv2 PR: 815
File: src/lib/SoftHSM.cpp:6543-6546
Timestamp: 2025-09-11T16:54:00.370Z
Learning: SoftHSM PR #815 (pkcs11.h 3.1.0 update, CK_RSA_AES_KEY_WRAP_PARAMS fix): Maintainer bukka prefers to keep scope minimal (header sync and param rename only); broader OAEP parameter enforcement or backend plumbing is out of scope for this PR and should be handled in a follow-up issue.
Learnt from: antoinelochet
Repo: softhsm/SoftHSMv2 PR: 809
File: m4/acx_openssl_mldsa.m4:10-35
Timestamp: 2025-08-18T17:54:37.645Z
Learning: For OpenSSL feature detection in autoconf macros, prefer functional tests (like EVP_PKEY_CTX_new_from_name) over version number checks, as the functional approach directly tests the actual capability and is more reliable across different OpenSSL builds where features may be backported or disabled.
Learnt from: antoinelochet
Repo: softhsm/SoftHSMv2 PR: 809
File: src/lib/SoftHSM.cpp:4193-4196
Timestamp: 2025-08-19T09:23:25.434Z
Learning: MLDSA parameter set inference: In src/lib/crypto/MLDSAPrivateKey.{h,cpp} and MLDSAPublicKey.{h,cpp}, getParameterSet() maps key material length to CKP_ML_DSA_44/65/87, and there is no setParameterSet(). Runtime MLDSA keys used by SoftHSM (e.g., in C_SignInit/C_VerifyInit via MLDSAUtil) rely on this inference; no explicit parameter-set assignment is required.
📚 Learning: 2025-09-11T16:54:00.370Z
Learnt from: bukka
Repo: softhsm/SoftHSMv2 PR: 815
File: src/lib/SoftHSM.cpp:6543-6546
Timestamp: 2025-09-11T16:54:00.370Z
Learning: SoftHSM PR #815 (pkcs11.h 3.1.0 update, CK_RSA_AES_KEY_WRAP_PARAMS fix): Maintainer bukka prefers to keep scope minimal (header sync and param rename only); broader OAEP parameter enforcement or backend plumbing is out of scope for this PR and should be handled in a follow-up issue.
Applied to files:
src/lib/crypto/OSSLDH.cppsrc/lib/crypto/OSSLDHPublicKey.hsrc/lib/crypto/OSSLDHPrivateKey.hsrc/lib/crypto/OSSLDHPrivateKey.cpp
📚 Learning: 2025-10-27T16:44:57.865Z
Learnt from: antoinelochet
Repo: softhsm/SoftHSMv2 PR: 809
File: src/lib/SoftHSM.cpp:5550-5596
Timestamp: 2025-10-27T16:44:57.865Z
Learning: SoftHSM ML‑DSA additional context: avoid transient malloc in SoftHSM::{AsymSignInit, AsymVerifyInit}; let Session::setParameters deep‑copy SIGN_ADDITIONAL_CONTEXT by allocating one buffer for struct + context bytes and fixing the stored pointer; Session::resetOp frees m_pParameters so both struct and context are released.
Applied to files:
src/lib/crypto/OSSLDH.cppsrc/lib/crypto/OSSLDHPrivateKey.cpp
📚 Learning: 2025-10-13T09:42:31.270Z
Learnt from: bukka
Repo: softhsm/SoftHSMv2 PR: 818
File: src/lib/SoftHSM.cpp:3238-3266
Timestamp: 2025-10-13T09:42:31.270Z
Learning: In src/lib/SoftHSM.cpp, SoftHSM::AsymDecryptInit should use MechParamCheckRSAPKCSOAEP for CKM_RSA_PKCS_OAEP to keep OAEP parameter validation identical to AsymEncryptInit, and avoid redundant manual checks for pMechanism->pParameter and MGF allowlists.
Applied to files:
src/lib/crypto/OSSLDH.cppsrc/lib/crypto/OSSLDHPrivateKey.cpp
📚 Learning: 2025-08-19T09:23:25.434Z
Learnt from: antoinelochet
Repo: softhsm/SoftHSMv2 PR: 809
File: src/lib/SoftHSM.cpp:4193-4196
Timestamp: 2025-08-19T09:23:25.434Z
Learning: MLDSA parameter set inference: In src/lib/crypto/MLDSAPrivateKey.{h,cpp} and MLDSAPublicKey.{h,cpp}, getParameterSet() maps key material length to CKP_ML_DSA_44/65/87, and there is no setParameterSet(). Runtime MLDSA keys used by SoftHSM (e.g., in C_SignInit/C_VerifyInit via MLDSAUtil) rely on this inference; no explicit parameter-set assignment is required.
Applied to files:
src/lib/crypto/OSSLDH.cppsrc/lib/crypto/OSSLDHPrivateKey.cpp
📚 Learning: 2025-11-11T19:42:46.886Z
Learnt from: JoaoHenrique12
Repo: softhsm/SoftHSMv2 PR: 823
File: src/lib/SoftHSM.cpp:58-58
Timestamp: 2025-11-11T19:42:46.886Z
Learning: SoftHSM include policy: Algorithm key/parameter headers (e.g., SLHParameters, SLH*Key) are included unconditionally in SoftHSM.cpp/SoftHSM.h; feature gating is handled in mechanism tables and CryptoFactory (WITH_*). Avoid wrapping such includes in WITH_* guards.
Applied to files:
src/lib/crypto/OSSLDH.cppsrc/lib/crypto/OSSLDHPublicKey.hsrc/lib/crypto/OSSLDHPrivateKey.cpp
📚 Learning: 2025-08-18T19:46:46.659Z
Learnt from: antoinelochet
Repo: softhsm/SoftHSMv2 PR: 809
File: src/lib/SoftHSM.cpp:10044-10055
Timestamp: 2025-08-18T19:46:46.659Z
Learning: In SoftHSM::generateMLDSA (src/lib/SoftHSM.cpp), the project prefers using CKP_ML_DSA_* constants for parameter set checks and logs "Unsupported parameter set: %lu". The code currently returns CKR_PARAMETER_SET_NOT_SUPPORTED for unsupported sets.
Applied to files:
src/lib/crypto/OSSLDH.cppsrc/lib/crypto/OSSLDHPrivateKey.cpp
📚 Learning: 2025-08-18T17:54:37.645Z
Learnt from: antoinelochet
Repo: softhsm/SoftHSMv2 PR: 809
File: m4/acx_openssl_mldsa.m4:10-35
Timestamp: 2025-08-18T17:54:37.645Z
Learning: For OpenSSL feature detection in autoconf macros, prefer functional tests (like EVP_PKEY_CTX_new_from_name) over version number checks, as the functional approach directly tests the actual capability and is more reliable across different OpenSSL builds where features may be backported or disabled.
Applied to files:
src/lib/crypto/OSSLDH.cppsrc/lib/crypto/OSSLDHPrivateKey.cpp
📚 Learning: 2025-08-11T16:42:23.012Z
Learnt from: bukka
Repo: softhsm/SoftHSMv2 PR: 806
File: src/lib/test/p11test.cpp:81-84
Timestamp: 2025-08-11T16:42:23.012Z
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/OSSLDH.cppsrc/lib/crypto/OSSLDHPrivateKey.cpp
🧬 Code graph analysis (2)
src/lib/crypto/OSSLDHPublicKey.h (4)
src/lib/crypto/OSSLDHPublicKey.cpp (15)
OSSLDHPublicKey(54-57)OSSLDHPublicKey(79-82)inDH(69-76)getOSSLKey(109-114)getOSSLKey(109-109)getOSSLKey(190-194)getOSSLKey(190-190)setFromOSSL(117-141)setFromOSSL(117-117)setFromOSSL(197-222)setFromOSSL(197-197)resetOSSLKey(143-150)resetOSSLKey(143-143)resetOSSLKey(224-231)resetOSSLKey(224-224)src/lib/crypto/OSSLDHPrivateKey.cpp (13)
inDH(61-68)getOSSLKey(161-166)getOSSLKey(161-161)getOSSLKey(291-295)getOSSLKey(291-291)setFromOSSL(169-193)setFromOSSL(169-169)setFromOSSL(298-323)setFromOSSL(298-298)resetOSSLKey(195-202)resetOSSLKey(195-195)resetOSSLKey(325-332)resetOSSLKey(325-325)src/lib/crypto/OSSLRSAPublicKey.cpp (4)
getOSSLKey(116-121)getOSSLKey(116-116)setFromOSSL(73-90)setFromOSSL(73-73)src/lib/crypto/OSSLDSAPublicKey.cpp (4)
getOSSLKey(151-156)getOSSLKey(151-151)setFromOSSL(67-97)setFromOSSL(67-67)
src/lib/crypto/OSSLDHPrivateKey.cpp (5)
src/lib/crypto/OSSLDHPublicKey.cpp (21)
inDH(69-76)resetOSSLKey(143-150)resetOSSLKey(143-143)resetOSSLKey(224-231)resetOSSLKey(224-224)setG(93-98)setG(93-93)setFromOSSL(117-141)setFromOSSL(117-117)setFromOSSL(197-222)setFromOSSL(197-197)setP(86-91)setP(86-86)createOSSLKey(153-186)createOSSLKey(153-153)createOSSLKey(234-291)createOSSLKey(234-234)getOSSLKey(109-114)getOSSLKey(109-109)getOSSLKey(190-194)getOSSLKey(190-190)src/lib/crypto/OSSLDSAPrivateKey.cpp (14)
setG(142-151)setG(142-142)setFromOSSL(68-98)setFromOSSL(68-68)setP(120-129)setP(120-120)PKCS8Encode(154-181)PKCS8Encode(154-154)createOSSLKey(211-256)createOSSLKey(211-211)PKCS8Decode(184-200)PKCS8Decode(184-184)getOSSLKey(203-208)getOSSLKey(203-203)src/lib/crypto/OSSLDSAPublicKey.cpp (10)
setG(128-137)setG(128-128)setFromOSSL(67-97)setFromOSSL(67-67)setP(106-115)setP(106-106)createOSSLKey(159-193)createOSSLKey(159-159)getOSSLKey(151-156)getOSSLKey(151-151)src/lib/crypto/OSSLRSAPrivateKey.cpp (12)
setFromOSSL(68-123)setFromOSSL(68-68)setP(132-141)setP(132-132)PKCS8Encode(223-250)PKCS8Encode(223-223)createOSSLKey(280-319)createOSSLKey(280-280)PKCS8Decode(253-269)PKCS8Decode(253-253)getOSSLKey(272-277)getOSSLKey(272-272)src/lib/crypto/OSSLRSAPublicKey.cpp (6)
setFromOSSL(73-90)setFromOSSL(73-73)createOSSLKey(124-155)createOSSLKey(124-124)getOSSLKey(116-121)getOSSLKey(116-116)
⏰ 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: Linux with OpenSSL 3.0
- GitHub Check: Linux with OpenSSL 1.1.1
- GitHub Check: Windows (x64, openssl)
- GitHub Check: Linux with Botan
- GitHub Check: macOS (botan)
- GitHub Check: macOS (openssl)
🔇 Additional comments (7)
src/lib/crypto/OSSLDHPrivateKey.h (1)
38-42: Well-structured version-gated API declarations.The header cleanly separates legacy DH* and OpenSSL 3.x EVP_PKEY* paths with consistent version guards. The introduction of
resetOSSLKey()centralizes cleanup logic. The approach of keeping the member nameddhregardless of type aligns with the PR's goal of minimal changes.Also applies to: 51-55, 79-91, 95-102
src/lib/crypto/OSSLDHPublicKey.h (1)
38-42: Consistent with private key header structure.The public key header mirrors the private key header's version-gated pattern, ensuring a uniform API surface for both key types across OpenSSL versions.
Also applies to: 50-54, 70-82, 86-93
src/lib/crypto/OSSLDH.cpp (2)
318-369: OpenSSL 3.x key derivation path looks correct.The two-phase derivation (size query, then actual derive) follows the standard OpenSSL 3.x pattern. The leading-zero compensation from the legacy path is not needed here because
EVP_PKEY_derivereturns the properly-sized secret.
42-48: Appropriate conditional includes for OpenSSL 3.x APIs.The includes for
core_names.h,param_build.h, andprovider.hare correctly gated for OpenSSL 3.x and match the APIs used in the implementation.src/lib/crypto/OSSLDHPrivateKey.cpp (3)
297-323: Proper BIGNUM extraction and cleanup insetFromOSSL.The OpenSSL 3.x version correctly extracts parameters using
EVP_PKEY_get_bn_paramand frees eachBIGNUMafter conversion toByteString. The conditional setting pattern matches the legacy implementation.
168-193: Correct legacysetFromOSSLimplementation.The extraction of P, G, and private key from the
DH*object usingDH_get0_pqgandDH_get0_keyfollows the established pattern. The returnedconst BIGNUM*pointers are internal to the DH object and correctly not freed here.
334-392: The OpenSSL 3.x implementation is missing the public key derivation—this is a critical bug, not approved code.The
createOSSLKeymethod (lines 335-392) only provides P, G, and the private key toEVP_PKEY_fromdata, but does not compute the public key component. Unlike the OpenSSL < 3.0 version which explicitly computes the public key viaBN_mod_exp(bn_pub_key, bn_g, bn_priv_key, bn_p, ctx), the OpenSSL 3.0+ path relies on a false assumption.
EVP_PKEY_fromdatadoes not automatically derive the public key from only the private key and domain parameters. This can be verified by observing:
- The
generateKeyPairmethod (lines 220-234) explicitly callsEVP_PKEY_keygenafterEVP_PKEY_fromdatato generate the complete keypairOSSLDHPublicKey::createOSSLKeyexplicitly pushesOSSL_PKEY_PARAM_PUB_KEYto the paramsOSSLDHPrivateKey::setFromOSSLonly extracts P, G, and priv_key but not the public key parameterThe fix should compute the public key before calling
EVP_PKEY_fromdata, similar to the < 3.0 implementation, or callEVP_PKEY_keygenafterward likegenerateKeyPairdoes. The current code results in an incomplete key that will fail during serialization/deserialization or operations requiring the public key component.⛔ Skipped due to learnings
Learnt from: bukka Repo: softhsm/SoftHSMv2 PR: 818 File: src/lib/SoftHSM.cpp:3238-3266 Timestamp: 2025-10-13T09:42:31.270Z Learning: In src/lib/SoftHSM.cpp, SoftHSM::AsymDecryptInit should use MechParamCheckRSAPKCSOAEP for CKM_RSA_PKCS_OAEP to keep OAEP parameter validation identical to AsymEncryptInit, and avoid redundant manual checks for pMechanism->pParameter and MGF allowlists.Learnt from: bukka Repo: softhsm/SoftHSMv2 PR: 815 File: src/lib/SoftHSM.cpp:6543-6546 Timestamp: 2025-09-11T16:54:00.370Z Learning: SoftHSM PR #815 (pkcs11.h 3.1.0 update, CK_RSA_AES_KEY_WRAP_PARAMS fix): Maintainer bukka prefers to keep scope minimal (header sync and param rename only); broader OAEP parameter enforcement or backend plumbing is out of scope for this PR and should be handled in a follow-up issue.
bukka
force-pushed
the
openssl-3-migration-ossldh
branch
from
30c4746 to
51348f0
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/lib/crypto/test/DHTests.cpp (1)
94-103: The test is missingsetXBitLength(128)based on its own comment and parallel test pattern.The test comment at line 94 states "Generate key-pair with a fixed private value length," but the call to
setXBitLength(128)is absent. ThetestSerialisation()method (line 115) shows the correct pattern:p->setXBitLength(128). Additionally, the OpenSSL 3.0 path in OSSLDH.cpp (lines 203–206) actively handles the XBitLength parameter viaOSSL_PKEY_PARAM_DH_PRIV_LENwhen set, confirming the infrastructure supports this.To align the test with its stated intent, add
p->setXBitLength(128);before thegenerateKeyPair()call at line 95, and verify the assertion validates the private value length rather than only the parameter size.
🤖 Fix all issues with AI agents
In @src/lib/crypto/OSSLDH.cpp:
- Around line 201-206: The OSSL_PARAM_BLD_push_BN and OSSL_PARAM_BLD_push_uint
calls can fail and are currently unchecked; update the code around
OSSL_PARAM_BLD_push_BN(bld, OSSL_PKEY_PARAM_FFC_P, bn_p),
OSSL_PARAM_BLD_push_BN(bld, OSSL_PKEY_PARAM_FFC_G, bn_g) and
OSSL_PARAM_BLD_push_uint(bld, OSSL_PKEY_PARAM_DH_PRIV_LEN,
params->getXBitLength()) to check each function's return (non-zero success), and
on any failure clean up allocated resources (e.g., free bld with
OSSL_PARAM_BLD_free and any BIGNUMs or other allocations), set/propagate an
error status and return or abort parameter construction before calling
EVP_PKEY_fromdata so you never pass an incomplete param set. Ensure all pushed
parameters are tested and handle the conditional push for getXBitLength()
similarly.
- Around line 318-369: The EVP_PKEY_derive path is missing the DH padding
control so OpenSSL 3.0 will strip leading zeros; after calling
EVP_PKEY_derive_set_peer(ctx, pub) (in the code that creates and uses
EVP_PKEY_CTX *ctx), call EVP_PKEY_CTX_set_dh_pad(ctx, 1) before
querying/deriving the secret so the derived secret preserves leading zeros and
matches the fixed-width behavior of the pre-3.0 branch.
In @src/lib/crypto/OSSLDHPrivateKey.cpp:
- Around line 365-367: The three calls to OSSL_PARAM_BLD_push_BN (with
OSSL_PKEY_PARAM_FFC_P/bn_p, OSSL_PKEY_PARAM_FFC_G/bn_g,
OSSL_PKEY_PARAM_PRIV_KEY/bn_priv_key) can fail and are currently unchecked;
update OSSLDHPrivateKey.cpp to check each return value and on failure clean up
(free the OSSL_PARAM_BLD, any BIGNUMs or temporary resources) and return an
error/false before calling EVP_PKEY_fromdata; ensure you perform this same
validated push pattern used in OSSLDH.cpp so incomplete parameter sets are never
passed to EVP_PKEY_fromdata.
In @src/lib/crypto/OSSLDHPublicKey.cpp:
- Around line 263-265: The OSSL_PARAM_BLD_push_BN calls can fail and their
return values are ignored; update the code around OSSL_PARAM_BLD_push_BN(bld,
OSSL_PKEY_PARAM_FFC_P, bn_p), OSSL_PARAM_BLD_push_BN(bld, OSSL_PKEY_PARAM_FFC_G,
bn_g), and OSSL_PARAM_BLD_push_BN(bld, OSSL_PKEY_PARAM_PUB_KEY, bn_pub_key) to
check each return value, handle failure by cleaning up resources (e.g., freeing
the param builder `bld` and any BIGNUMs if needed) and returning or propagating
an error indicator consistent with surrounding code (match the error path used
elsewhere in this file, e.g., returning nullptr or setting an error code), so
that any push failure is detected and handled instead of ignored.
🧹 Nitpick comments (2)
src/lib/crypto/OSSLDHPrivateKey.cpp (1)
298-324: Consider validating that critical DH parameters were retrieved.The code doesn't check
EVP_PKEY_get_bn_paramreturn values, instead relying on NULL checks. While this is defensive, it could silently skip setting critical parameters (p, g, priv_key) if the EVP_PKEY doesn't contain them, potentially resulting in an incomplete or invalid key.💡 Suggested improvement
Add a check to ensure at least the prime (p) was successfully retrieved, as a DH key without p is invalid:
EVP_PKEY_get_bn_param(inDH, OSSL_PKEY_PARAM_FFC_P, &bn_p); EVP_PKEY_get_bn_param(inDH, OSSL_PKEY_PARAM_FFC_G, &bn_g); EVP_PKEY_get_bn_param(inDH, OSSL_PKEY_PARAM_PRIV_KEY, &bn_priv_key); + if (!bn_p || !bn_g || !bn_priv_key) + { + ERROR_MSG("Failed to extract required DH parameters from EVP_PKEY"); + BN_free(bn_p); + BN_free(bn_g); + BN_free(bn_priv_key); + return; + } + if (bn_p) {src/lib/crypto/OSSLDHPublicKey.cpp (1)
197-222: Consider validating that critical DH parameters were retrieved.Similar to the private key implementation, consider adding validation that critical parameters (p, g, pub_key) were successfully retrieved from the EVP_PKEY.
💡 Suggested improvement
EVP_PKEY_get_bn_param(inDH, OSSL_PKEY_PARAM_FFC_P, &bn_p); EVP_PKEY_get_bn_param(inDH, OSSL_PKEY_PARAM_FFC_G, &bn_g); EVP_PKEY_get_bn_param(inDH, OSSL_PKEY_PARAM_PUB_KEY, &bn_pub_key); + if (!bn_p || !bn_g || !bn_pub_key) + { + ERROR_MSG("Failed to extract required DH parameters from EVP_PKEY"); + BN_free(bn_p); + BN_free(bn_g); + BN_free(bn_pub_key); + return; + } + if (bn_p) {
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/lib/crypto/OSSLDH.cppsrc/lib/crypto/OSSLDHPrivateKey.cppsrc/lib/crypto/OSSLDHPrivateKey.hsrc/lib/crypto/OSSLDHPublicKey.cppsrc/lib/crypto/OSSLDHPublicKey.hsrc/lib/crypto/test/DHTests.cpp
🚧 Files skipped from review as they are similar to previous changes (1)
- src/lib/crypto/OSSLDHPublicKey.h
🧰 Additional context used
🧠 Learnings (10)
📓 Common learnings
Learnt from: bukka
Repo: softhsm/SoftHSMv2 PR: 815
File: src/lib/SoftHSM.cpp:6543-6546
Timestamp: 2025-09-11T16:54:00.370Z
Learning: SoftHSM PR #815 (pkcs11.h 3.1.0 update, CK_RSA_AES_KEY_WRAP_PARAMS fix): Maintainer bukka prefers to keep scope minimal (header sync and param rename only); broader OAEP parameter enforcement or backend plumbing is out of scope for this PR and should be handled in a follow-up issue.
Learnt from: antoinelochet
Repo: softhsm/SoftHSMv2 PR: 809
File: m4/acx_openssl_mldsa.m4:10-35
Timestamp: 2025-08-18T17:54:37.645Z
Learning: For OpenSSL feature detection in autoconf macros, prefer functional tests (like EVP_PKEY_CTX_new_from_name) over version number checks, as the functional approach directly tests the actual capability and is more reliable across different OpenSSL builds where features may be backported or disabled.
Learnt from: antoinelochet
Repo: softhsm/SoftHSMv2 PR: 809
File: src/lib/SoftHSM.cpp:4193-4196
Timestamp: 2025-08-19T09:23:25.434Z
Learning: MLDSA parameter set inference: In src/lib/crypto/MLDSAPrivateKey.{h,cpp} and MLDSAPublicKey.{h,cpp}, getParameterSet() maps key material length to CKP_ML_DSA_44/65/87, and there is no setParameterSet(). Runtime MLDSA keys used by SoftHSM (e.g., in C_SignInit/C_VerifyInit via MLDSAUtil) rely on this inference; no explicit parameter-set assignment is required.
📚 Learning: 2025-09-11T16:54:00.370Z
Learnt from: bukka
Repo: softhsm/SoftHSMv2 PR: 815
File: src/lib/SoftHSM.cpp:6543-6546
Timestamp: 2025-09-11T16:54:00.370Z
Learning: SoftHSM PR #815 (pkcs11.h 3.1.0 update, CK_RSA_AES_KEY_WRAP_PARAMS fix): Maintainer bukka prefers to keep scope minimal (header sync and param rename only); broader OAEP parameter enforcement or backend plumbing is out of scope for this PR and should be handled in a follow-up issue.
Applied to files:
src/lib/crypto/OSSLDH.cppsrc/lib/crypto/OSSLDHPrivateKey.hsrc/lib/crypto/OSSLDHPublicKey.cppsrc/lib/crypto/OSSLDHPrivateKey.cpp
📚 Learning: 2025-10-27T16:44:57.865Z
Learnt from: antoinelochet
Repo: softhsm/SoftHSMv2 PR: 809
File: src/lib/SoftHSM.cpp:5550-5596
Timestamp: 2025-10-27T16:44:57.865Z
Learning: SoftHSM ML‑DSA additional context: avoid transient malloc in SoftHSM::{AsymSignInit, AsymVerifyInit}; let Session::setParameters deep‑copy SIGN_ADDITIONAL_CONTEXT by allocating one buffer for struct + context bytes and fixing the stored pointer; Session::resetOp frees m_pParameters so both struct and context are released.
Applied to files:
src/lib/crypto/OSSLDH.cppsrc/lib/crypto/test/DHTests.cppsrc/lib/crypto/OSSLDHPublicKey.cppsrc/lib/crypto/OSSLDHPrivateKey.cpp
📚 Learning: 2025-08-19T09:23:25.434Z
Learnt from: antoinelochet
Repo: softhsm/SoftHSMv2 PR: 809
File: src/lib/SoftHSM.cpp:4193-4196
Timestamp: 2025-08-19T09:23:25.434Z
Learning: MLDSA parameter set inference: In src/lib/crypto/MLDSAPrivateKey.{h,cpp} and MLDSAPublicKey.{h,cpp}, getParameterSet() maps key material length to CKP_ML_DSA_44/65/87, and there is no setParameterSet(). Runtime MLDSA keys used by SoftHSM (e.g., in C_SignInit/C_VerifyInit via MLDSAUtil) rely on this inference; no explicit parameter-set assignment is required.
Applied to files:
src/lib/crypto/OSSLDH.cppsrc/lib/crypto/test/DHTests.cppsrc/lib/crypto/OSSLDHPublicKey.cppsrc/lib/crypto/OSSLDHPrivateKey.cpp
📚 Learning: 2025-10-13T09:42:31.270Z
Learnt from: bukka
Repo: softhsm/SoftHSMv2 PR: 818
File: src/lib/SoftHSM.cpp:3238-3266
Timestamp: 2025-10-13T09:42:31.270Z
Learning: In src/lib/SoftHSM.cpp, SoftHSM::AsymDecryptInit should use MechParamCheckRSAPKCSOAEP for CKM_RSA_PKCS_OAEP to keep OAEP parameter validation identical to AsymEncryptInit, and avoid redundant manual checks for pMechanism->pParameter and MGF allowlists.
Applied to files:
src/lib/crypto/OSSLDH.cppsrc/lib/crypto/test/DHTests.cppsrc/lib/crypto/OSSLDHPublicKey.cppsrc/lib/crypto/OSSLDHPrivateKey.cpp
📚 Learning: 2025-11-11T19:42:46.886Z
Learnt from: JoaoHenrique12
Repo: softhsm/SoftHSMv2 PR: 823
File: src/lib/SoftHSM.cpp:58-58
Timestamp: 2025-11-11T19:42:46.886Z
Learning: SoftHSM include policy: Algorithm key/parameter headers (e.g., SLHParameters, SLH*Key) are included unconditionally in SoftHSM.cpp/SoftHSM.h; feature gating is handled in mechanism tables and CryptoFactory (WITH_*). Avoid wrapping such includes in WITH_* guards.
Applied to files:
src/lib/crypto/OSSLDH.cppsrc/lib/crypto/OSSLDHPrivateKey.cpp
📚 Learning: 2025-08-18T19:46:46.659Z
Learnt from: antoinelochet
Repo: softhsm/SoftHSMv2 PR: 809
File: src/lib/SoftHSM.cpp:10044-10055
Timestamp: 2025-08-18T19:46:46.659Z
Learning: In SoftHSM::generateMLDSA (src/lib/SoftHSM.cpp), the project prefers using CKP_ML_DSA_* constants for parameter set checks and logs "Unsupported parameter set: %lu". The code currently returns CKR_PARAMETER_SET_NOT_SUPPORTED for unsupported sets.
Applied to files:
src/lib/crypto/OSSLDH.cppsrc/lib/crypto/test/DHTests.cppsrc/lib/crypto/OSSLDHPublicKey.cppsrc/lib/crypto/OSSLDHPrivateKey.cpp
📚 Learning: 2025-08-18T17:54:37.645Z
Learnt from: antoinelochet
Repo: softhsm/SoftHSMv2 PR: 809
File: m4/acx_openssl_mldsa.m4:10-35
Timestamp: 2025-08-18T17:54:37.645Z
Learning: For OpenSSL feature detection in autoconf macros, prefer functional tests (like EVP_PKEY_CTX_new_from_name) over version number checks, as the functional approach directly tests the actual capability and is more reliable across different OpenSSL builds where features may be backported or disabled.
Applied to files:
src/lib/crypto/OSSLDH.cppsrc/lib/crypto/OSSLDHPrivateKey.cpp
📚 Learning: 2025-08-18T17:59:30.693Z
Learnt from: antoinelochet
Repo: softhsm/SoftHSMv2 PR: 809
File: src/lib/crypto/MLDSAParameters.h:42-44
Timestamp: 2025-08-18T17:59:30.693Z
Learning: In MLDSAParameters.h, the author prefers to keep PKCS#11 constant references (CKP_ML_DSA_*) rather than explicit numeric values for better code clarity and maintainability.
Applied to files:
src/lib/crypto/OSSLDH.cppsrc/lib/crypto/OSSLDHPublicKey.cpp
📚 Learning: 2025-08-11T16:42:23.012Z
Learnt from: bukka
Repo: softhsm/SoftHSMv2 PR: 806
File: src/lib/test/p11test.cpp:81-84
Timestamp: 2025-08-11T16:42:23.012Z
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/OSSLDH.cppsrc/lib/crypto/OSSLDHPrivateKey.cpp
🧬 Code graph analysis (3)
src/lib/crypto/OSSLDH.cpp (1)
src/lib/crypto/OSSLUtil.cpp (4)
byteString2bn(56-61)byteString2bn(56-56)bn2ByteString(42-53)bn2ByteString(42-42)
src/lib/crypto/OSSLDHPrivateKey.h (1)
src/lib/crypto/OSSLDHPrivateKey.cpp (15)
OSSLDHPrivateKey(52-55)OSSLDHPrivateKey(71-74)inDH(61-68)getOSSLKey(161-166)getOSSLKey(161-161)getOSSLKey(292-296)getOSSLKey(292-292)setFromOSSL(169-193)setFromOSSL(169-169)setFromOSSL(299-324)setFromOSSL(299-299)resetOSSLKey(195-202)resetOSSLKey(195-195)resetOSSLKey(326-333)resetOSSLKey(326-326)
src/lib/crypto/OSSLDHPublicKey.cpp (4)
src/lib/crypto/OSSLDHPrivateKey.cpp (17)
inDH(61-68)resetOSSLKey(195-202)resetOSSLKey(195-195)resetOSSLKey(326-333)resetOSSLKey(326-326)setP(96-101)setP(96-96)setG(103-108)setG(103-103)createOSSLKey(205-249)createOSSLKey(205-205)createOSSLKey(336-393)createOSSLKey(336-336)setFromOSSL(169-193)setFromOSSL(169-169)setFromOSSL(299-324)setFromOSSL(299-299)src/lib/crypto/DHParameters.cpp (2)
setG(48-51)setG(48-48)src/lib/crypto/DHPublicKey.cpp (2)
setY(70-73)setY(70-70)src/lib/crypto/OSSLUtil.cpp (4)
bn2ByteString(42-53)bn2ByteString(42-42)byteString2bn(56-61)byteString2bn(56-56)
⏰ 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: macOS (openssl)
- GitHub Check: Windows (x64, botan)
- GitHub Check: Windows (x64, openssl)
- GitHub Check: macOS (botan)
- GitHub Check: Linux with OpenSSL 3.0
- GitHub Check: Linux with OpenSSL 1.1.1
- GitHub Check: Linux with Botan
- GitHub Check: Windows (x86, openssl)
🔇 Additional comments (2)
src/lib/crypto/OSSLDHPrivateKey.h (1)
38-106: LGTM - Clean version-guarded API design.The header effectively separates OpenSSL < 3.0 (DH*-based) and OpenSSL ≥ 3.0 (EVP_PKEY*-based) paths with consistent conditional compilation. The addition of
resetOSSLKey()centralizes cleanup logic, improving maintainability.src/lib/crypto/OSSLDH.cpp (1)
436-504: LGTM - Proper error handling and resource cleanup.The OpenSSL 3.0 parameter generation path correctly:
- Checks return values for
EVP_PKEY_get_bn_param- Frees allocated
BIGNUMvalues (lines 502-503)- Cleans up
EVP_PKEYcontext on all paths
bukka
force-pushed
the
openssl-3-migration-ossldh
branch
from
51348f0 to
29c9d13
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/lib/crypto/OSSLDH.cpp (1)
181-258: OpenSSL 3.x key generation path is well-structured.The implementation correctly:
- Uses
OSSL_PARAM_BLDto build DH parameters- Handles
XBitLengthviaOSSL_PKEY_PARAM_DH_PRIV_LEN- Creates parameters EVP_PKEY first, then generates keypair
- Properly frees intermediate resources
One minor observation: Line 210 is missing
OSSL_PARAM_BLD_free(bld)in the error path after the push fails, but the bld is freed indirectly since we return immediately. However, for consistency with other error paths, consider adding it.♻️ Optional: Add OSSL_PARAM_BLD_free in error path at line 210
if (!OSSL_PARAM_BLD_push_BN(bld, OSSL_PKEY_PARAM_FFC_P, bn_p) || !OSSL_PARAM_BLD_push_BN(bld, OSSL_PKEY_PARAM_FFC_G, bn_g) || (params->getXBitLength() > 0 && !OSSL_PARAM_BLD_push_uint( bld, OSSL_PKEY_PARAM_DH_PRIV_LEN, params->getXBitLength()))) { ERROR_MSG("Failed to push DH params to OSSL_PARAM_BLD"); BN_free(bn_p); BN_free(bn_g); + OSSL_PARAM_BLD_free(bld); EVP_PKEY_CTX_free(ctx); return false; }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/lib/crypto/OSSLDH.cppsrc/lib/crypto/OSSLDHPrivateKey.cppsrc/lib/crypto/OSSLDHPrivateKey.hsrc/lib/crypto/OSSLDHPublicKey.cppsrc/lib/crypto/OSSLDHPublicKey.hsrc/lib/crypto/test/DHTests.cpp
🚧 Files skipped from review as they are similar to previous changes (2)
- src/lib/crypto/test/DHTests.cpp
- src/lib/crypto/OSSLDHPublicKey.h
🧰 Additional context used
🧠 Learnings (10)
📓 Common learnings
Learnt from: bukka
Repo: softhsm/SoftHSMv2 PR: 815
File: src/lib/SoftHSM.cpp:6543-6546
Timestamp: 2025-09-11T16:54:00.370Z
Learning: SoftHSM PR #815 (pkcs11.h 3.1.0 update, CK_RSA_AES_KEY_WRAP_PARAMS fix): Maintainer bukka prefers to keep scope minimal (header sync and param rename only); broader OAEP parameter enforcement or backend plumbing is out of scope for this PR and should be handled in a follow-up issue.
Learnt from: antoinelochet
Repo: softhsm/SoftHSMv2 PR: 809
File: m4/acx_openssl_mldsa.m4:10-35
Timestamp: 2025-08-18T17:54:37.645Z
Learning: For OpenSSL feature detection in autoconf macros, prefer functional tests (like EVP_PKEY_CTX_new_from_name) over version number checks, as the functional approach directly tests the actual capability and is more reliable across different OpenSSL builds where features may be backported or disabled.
Learnt from: antoinelochet
Repo: softhsm/SoftHSMv2 PR: 809
File: src/lib/SoftHSM.cpp:4193-4196
Timestamp: 2025-08-19T09:23:25.434Z
Learning: MLDSA parameter set inference: In src/lib/crypto/MLDSAPrivateKey.{h,cpp} and MLDSAPublicKey.{h,cpp}, getParameterSet() maps key material length to CKP_ML_DSA_44/65/87, and there is no setParameterSet(). Runtime MLDSA keys used by SoftHSM (e.g., in C_SignInit/C_VerifyInit via MLDSAUtil) rely on this inference; no explicit parameter-set assignment is required.
📚 Learning: 2025-09-11T16:54:00.370Z
Learnt from: bukka
Repo: softhsm/SoftHSMv2 PR: 815
File: src/lib/SoftHSM.cpp:6543-6546
Timestamp: 2025-09-11T16:54:00.370Z
Learning: SoftHSM PR #815 (pkcs11.h 3.1.0 update, CK_RSA_AES_KEY_WRAP_PARAMS fix): Maintainer bukka prefers to keep scope minimal (header sync and param rename only); broader OAEP parameter enforcement or backend plumbing is out of scope for this PR and should be handled in a follow-up issue.
Applied to files:
src/lib/crypto/OSSLDHPrivateKey.cppsrc/lib/crypto/OSSLDH.cppsrc/lib/crypto/OSSLDHPublicKey.cppsrc/lib/crypto/OSSLDHPrivateKey.h
📚 Learning: 2025-10-13T09:42:31.270Z
Learnt from: bukka
Repo: softhsm/SoftHSMv2 PR: 818
File: src/lib/SoftHSM.cpp:3238-3266
Timestamp: 2025-10-13T09:42:31.270Z
Learning: In src/lib/SoftHSM.cpp, SoftHSM::AsymDecryptInit should use MechParamCheckRSAPKCSOAEP for CKM_RSA_PKCS_OAEP to keep OAEP parameter validation identical to AsymEncryptInit, and avoid redundant manual checks for pMechanism->pParameter and MGF allowlists.
Applied to files:
src/lib/crypto/OSSLDHPrivateKey.cppsrc/lib/crypto/OSSLDH.cppsrc/lib/crypto/OSSLDHPublicKey.cpp
📚 Learning: 2025-10-27T16:44:57.865Z
Learnt from: antoinelochet
Repo: softhsm/SoftHSMv2 PR: 809
File: src/lib/SoftHSM.cpp:5550-5596
Timestamp: 2025-10-27T16:44:57.865Z
Learning: SoftHSM ML‑DSA additional context: avoid transient malloc in SoftHSM::{AsymSignInit, AsymVerifyInit}; let Session::setParameters deep‑copy SIGN_ADDITIONAL_CONTEXT by allocating one buffer for struct + context bytes and fixing the stored pointer; Session::resetOp frees m_pParameters so both struct and context are released.
Applied to files:
src/lib/crypto/OSSLDHPrivateKey.cppsrc/lib/crypto/OSSLDH.cppsrc/lib/crypto/OSSLDHPublicKey.cpp
📚 Learning: 2025-08-18T19:46:46.659Z
Learnt from: antoinelochet
Repo: softhsm/SoftHSMv2 PR: 809
File: src/lib/SoftHSM.cpp:10044-10055
Timestamp: 2025-08-18T19:46:46.659Z
Learning: In SoftHSM::generateMLDSA (src/lib/SoftHSM.cpp), the project prefers using CKP_ML_DSA_* constants for parameter set checks and logs "Unsupported parameter set: %lu". The code currently returns CKR_PARAMETER_SET_NOT_SUPPORTED for unsupported sets.
Applied to files:
src/lib/crypto/OSSLDHPrivateKey.cppsrc/lib/crypto/OSSLDH.cppsrc/lib/crypto/OSSLDHPublicKey.cpp
📚 Learning: 2025-08-18T17:54:37.645Z
Learnt from: antoinelochet
Repo: softhsm/SoftHSMv2 PR: 809
File: m4/acx_openssl_mldsa.m4:10-35
Timestamp: 2025-08-18T17:54:37.645Z
Learning: For OpenSSL feature detection in autoconf macros, prefer functional tests (like EVP_PKEY_CTX_new_from_name) over version number checks, as the functional approach directly tests the actual capability and is more reliable across different OpenSSL builds where features may be backported or disabled.
Applied to files:
src/lib/crypto/OSSLDHPrivateKey.cppsrc/lib/crypto/OSSLDH.cpp
📚 Learning: 2025-11-11T19:42:46.886Z
Learnt from: JoaoHenrique12
Repo: softhsm/SoftHSMv2 PR: 823
File: src/lib/SoftHSM.cpp:58-58
Timestamp: 2025-11-11T19:42:46.886Z
Learning: SoftHSM include policy: Algorithm key/parameter headers (e.g., SLHParameters, SLH*Key) are included unconditionally in SoftHSM.cpp/SoftHSM.h; feature gating is handled in mechanism tables and CryptoFactory (WITH_*). Avoid wrapping such includes in WITH_* guards.
Applied to files:
src/lib/crypto/OSSLDHPrivateKey.cppsrc/lib/crypto/OSSLDH.cpp
📚 Learning: 2025-08-18T17:59:30.693Z
Learnt from: antoinelochet
Repo: softhsm/SoftHSMv2 PR: 809
File: src/lib/crypto/MLDSAParameters.h:42-44
Timestamp: 2025-08-18T17:59:30.693Z
Learning: In MLDSAParameters.h, the author prefers to keep PKCS#11 constant references (CKP_ML_DSA_*) rather than explicit numeric values for better code clarity and maintainability.
Applied to files:
src/lib/crypto/OSSLDHPrivateKey.cppsrc/lib/crypto/OSSLDH.cpp
📚 Learning: 2025-08-11T16:42:23.012Z
Learnt from: bukka
Repo: softhsm/SoftHSMv2 PR: 806
File: src/lib/test/p11test.cpp:81-84
Timestamp: 2025-08-11T16:42:23.012Z
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/OSSLDHPrivateKey.cppsrc/lib/crypto/OSSLDH.cpp
📚 Learning: 2025-08-19T09:23:25.434Z
Learnt from: antoinelochet
Repo: softhsm/SoftHSMv2 PR: 809
File: src/lib/SoftHSM.cpp:4193-4196
Timestamp: 2025-08-19T09:23:25.434Z
Learning: MLDSA parameter set inference: In src/lib/crypto/MLDSAPrivateKey.{h,cpp} and MLDSAPublicKey.{h,cpp}, getParameterSet() maps key material length to CKP_ML_DSA_44/65/87, and there is no setParameterSet(). Runtime MLDSA keys used by SoftHSM (e.g., in C_SignInit/C_VerifyInit via MLDSAUtil) rely on this inference; no explicit parameter-set assignment is required.
Applied to files:
src/lib/crypto/OSSLDHPrivateKey.cppsrc/lib/crypto/OSSLDH.cppsrc/lib/crypto/OSSLDHPublicKey.cpp
🧬 Code graph analysis (4)
src/lib/crypto/OSSLDHPrivateKey.cpp (2)
src/lib/crypto/OSSLDHPublicKey.cpp (17)
inDH(69-76)resetOSSLKey(143-150)resetOSSLKey(143-143)resetOSSLKey(224-231)resetOSSLKey(224-224)setG(93-98)setG(93-93)setFromOSSL(117-141)setFromOSSL(117-117)setFromOSSL(197-222)setFromOSSL(197-197)setP(86-91)setP(86-86)createOSSLKey(153-186)createOSSLKey(153-153)createOSSLKey(234-300)createOSSLKey(234-234)src/lib/crypto/OSSLUtil.cpp (4)
bn2ByteString(42-53)bn2ByteString(42-42)byteString2bn(56-61)byteString2bn(56-56)
src/lib/crypto/OSSLDH.cpp (1)
src/lib/crypto/OSSLUtil.cpp (4)
byteString2bn(56-61)byteString2bn(56-56)bn2ByteString(42-53)bn2ByteString(42-42)
src/lib/crypto/OSSLDHPublicKey.cpp (4)
src/lib/crypto/OSSLDHPrivateKey.cpp (14)
setG(103-108)setG(103-103)getOSSLKey(161-166)getOSSLKey(161-161)getOSSLKey(292-296)getOSSLKey(292-292)createOSSLKey(205-249)createOSSLKey(205-205)createOSSLKey(336-402)createOSSLKey(336-336)setFromOSSL(169-193)setFromOSSL(169-169)setFromOSSL(299-324)setFromOSSL(299-299)src/lib/crypto/DHParameters.cpp (2)
setG(48-51)setG(48-48)src/lib/crypto/DHPublicKey.cpp (2)
setY(70-73)setY(70-70)src/lib/crypto/OSSLUtil.cpp (4)
bn2ByteString(42-53)bn2ByteString(42-42)byteString2bn(56-61)byteString2bn(56-56)
src/lib/crypto/OSSLDHPrivateKey.h (3)
src/lib/crypto/OSSLDHPrivateKey.cpp (15)
OSSLDHPrivateKey(52-55)OSSLDHPrivateKey(71-74)inDH(61-68)getOSSLKey(161-166)getOSSLKey(161-161)getOSSLKey(292-296)getOSSLKey(292-292)setFromOSSL(169-193)setFromOSSL(169-169)setFromOSSL(299-324)setFromOSSL(299-299)resetOSSLKey(195-202)resetOSSLKey(195-195)resetOSSLKey(326-333)resetOSSLKey(326-326)src/lib/crypto/DHPrivateKey.h (1)
DHPrivateKey(39-78)src/lib/crypto/OSSLDHPublicKey.cpp (13)
inDH(69-76)getOSSLKey(109-114)getOSSLKey(109-109)getOSSLKey(190-194)getOSSLKey(190-190)setFromOSSL(117-141)setFromOSSL(117-117)setFromOSSL(197-222)setFromOSSL(197-197)resetOSSLKey(143-150)resetOSSLKey(143-143)resetOSSLKey(224-231)resetOSSLKey(224-224)
⏰ 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: Linux with OpenSSL 1.1.1
- GitHub Check: Windows (x86, openssl)
- GitHub Check: Windows (x64, openssl)
- GitHub Check: macOS (botan)
- GitHub Check: Linux with Botan
- GitHub Check: macOS (openssl)
- GitHub Check: Windows (x64, botan)
- GitHub Check: Linux with OpenSSL 3.0
🔇 Additional comments (25)
src/lib/crypto/OSSLDHPrivateKey.h (4)
38-42: LGTM: Conditional includes for OpenSSL version compatibility.The version guard correctly includes
<openssl/dh.h>for legacy OpenSSL and<openssl/evp.h>for OpenSSL 3.0+, matching the internal representation change.
51-55: LGTM: Version-gated constructor signatures.Constructor overloads correctly accept the appropriate OpenSSL key type per version. This aligns with the pattern in
OSSLDHPublicKey.
79-91: LGTM: Conditional getter/setter signatures.The
getOSSLKey()return type andsetFromOSSL()parameter type are appropriately guarded by OpenSSL version, maintaining type safety for each backend.
95-102: LGTM: Internal representation and lifecycle helper.The private member
dhcorrectly changes type based on OpenSSL version. AddingresetOSSLKey()centralizes cleanup logic, improving maintainability over directDH_free/EVP_PKEY_freecalls in setters.src/lib/crypto/OSSLDH.cpp (5)
42-48: LGTM: Conditional OpenSSL 3.x includes.Correct inclusion of
core_names.h,param_build.h, andprovider.hfor the OpenSSL 3.x API.
135-137: LGTM: BN conversion moved before version branch.Moving
bn_pandbn_gcreation before the version-guarded block reduces duplication and ensures they're available for both paths.
323-381: LGTM: OpenSSL 3.x key derivation using EVP_PKEY_derive.The implementation correctly:
- Uses
EVP_PKEY_derive_set_peerfor peer public key- Sets
EVP_PKEY_CTX_set_dh_pad(ctx, 1)to handle leading-zero padding (eliminating the manual compensation needed in the legacy path)- Queries buffer size before allocation
- Properly cleans up
ctxin all error paths
448-501: LGTM: OpenSSL 3.x parameter generation.Clean implementation using
EVP_PKEY_paramgenwith proper extraction ofpandgviaEVP_PKEY_get_bn_param. Memory management is correct withBN_freefor the extracted BIGNUMs.
510-516: LGTM: Conditional cleanup for generated parameters.Correctly frees
dh_params(EVP_PKEY) and the extracted BIGNUMs for OpenSSL 3.x, while usingDH_freefor legacy.src/lib/crypto/OSSLDHPrivateKey.cpp (9)
40-48: LGTM: Conditional includes for OpenSSL 3.x.Correctly includes
core_names.h,param_build.h, andprovider.hfor OpenSSL 3.x, while retaining FIPS include for legacy.
57-68: LGTM: Version-gated constructor with clean delegation.The preprocessor-based parameter type switching is consistent with the header declaration. Both paths delegate to
setFromOSSL().
71-74: LGTM: Destructor uses centralized cleanup.Using
resetOSSLKey()instead of directDH_freeimproves maintainability and ensures consistent cleanup across both backends.
86-108: LGTM: Setters invalidate cached OpenSSL key.Calling
resetOSSLKey()after updating the base class state ensures the OpenSSL representation is recreated lazily with updated values. This pattern mirrorsOSSLDHPublicKey.
110-158: LGTM: Legacy PKCS8 encode/decode.Correctly wraps
DH*inEVP_PKEYfor PKCS#8 serialization and extracts it back on decode. Memory management is correct withEVP_PKEY_free,PKCS8_PRIV_KEY_INFO_free, andDH_free.
168-202: LGTM: Legacy setFromOSSL and resetOSSLKey.
setFromOSSL(const DH*)correctly extractsp,g, and private keyxusingDH_get0_pqg/DH_get0_key.resetOSSLKey()properly frees and nulls the internalDH*.
253-289: LGTM: OpenSSL 3.x PKCS8 encode/decode.Since
dhis already anEVP_PKEY*in the 3.x path, the PKCS8 flow is simplified—no wrapping needed.setFromOSSL(pkey)is called before freeingpkey, which is correct.
298-333: LGTM: OpenSSL 3.x setFromOSSL and resetOSSLKey.Correctly extracts
FFC_P,FFC_G, andPRIV_KEYfromEVP_PKEYusingEVP_PKEY_get_bn_param. BIGNUMs are properly freed after conversion.resetOSSLKey()usesEVP_PKEY_free.
335-402: LGTM: OpenSSL 3.x createOSSLKey implementation.The implementation:
- Uses
OSSL_PARAM_BLDto build p, g, and private key- Uses
EVP_PKEY_KEYPAIRflag (correct for private key, as it will compute the public key)- Properly cleans up all resources in error paths and success path
src/lib/crypto/OSSLDHPublicKey.cpp (7)
39-47: LGTM: Conditional includes for OpenSSL 3.x.Matches the include pattern used in
OSSLDHPrivateKey.cpp.
50-63: LGTM: Type constant and isOfType implementation.Adding
typeandisOfType()provides consistent type-checking capability.
65-76: LGTM: Version-gated constructor.Consistent with
OSSLDHPrivateKeypattern, delegating tosetFromOSSL().
78-105: LGTM: Destructor and setters use resetOSSLKey().Consistent pattern with the private key class. Setters properly invalidate the cached OpenSSL representation when key components change.
107-186: LGTM: Legacy OpenSSL path.
getOSSLKey(),setFromOSSL(),resetOSSLKey(), andcreateOSSLKey()follow the established pattern. The FIPS handling increateOSSLKey()is preserved for older OpenSSL versions.
187-231: LGTM: OpenSSL 3.x getOSSLKey, setFromOSSL, and resetOSSLKey.
setFromOSSL()correctly extractsFFC_P,FFC_G, andPUB_KEYwith proper BIGNUM cleanup.resetOSSLKey()usesEVP_PKEY_free.
233-300: LGTM: OpenSSL 3.x createOSSLKey implementation.Correctly uses
EVP_PKEY_PUBLIC_KEYflag (vsEVP_PKEY_KEYPAIRfor private key). Comprehensive error handling with proper cleanup of all allocated resources (bn_p,bn_g,bn_pub_key,ctx,bld,params).
|
@antoinelochet @jschlyter If you could quickly check this out when you have time, that would be great. I would like to move to other parts so we have the project working once OpenSSL 4.0 is released (it's the next version of OpenSSL so it won't take too long). |
Seems good to me. |
bukka
force-pushed
the
openssl-3-migration-ossldh
branch
from
29c9d13 to
35f6662
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @src/lib/crypto/test/DHTests.cpp:
- Around line 94-99: The comment above the dh->generateKeyPair(&kp, p) call
incorrectly mentions a "fixed private value length" (p->setXBitLength was
removed); update that comment in DHTests.cpp to reflect the current
behavior—e.g., "Generate a second key pair on the same parameters"—or simply
remove the reference to fixed private value length so it matches the test that
creates a second key pair and asserts priv->getBitLength() == 1024.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/lib/crypto/OSSLDH.cppsrc/lib/crypto/OSSLDHPrivateKey.cppsrc/lib/crypto/OSSLDHPrivateKey.hsrc/lib/crypto/OSSLDHPublicKey.cppsrc/lib/crypto/OSSLDHPublicKey.hsrc/lib/crypto/test/DHTests.cpp
🚧 Files skipped from review as they are similar to previous changes (1)
- src/lib/crypto/OSSLDHPublicKey.h
🧰 Additional context used
🧠 Learnings (10)
📓 Common learnings
Learnt from: bukka
Repo: softhsm/SoftHSMv2 PR: 815
File: src/lib/SoftHSM.cpp:6543-6546
Timestamp: 2025-09-11T16:54:00.370Z
Learning: SoftHSM PR #815 (pkcs11.h 3.1.0 update, CK_RSA_AES_KEY_WRAP_PARAMS fix): Maintainer bukka prefers to keep scope minimal (header sync and param rename only); broader OAEP parameter enforcement or backend plumbing is out of scope for this PR and should be handled in a follow-up issue.
Learnt from: antoinelochet
Repo: softhsm/SoftHSMv2 PR: 809
File: m4/acx_openssl_mldsa.m4:10-35
Timestamp: 2025-08-18T17:54:37.645Z
Learning: For OpenSSL feature detection in autoconf macros, prefer functional tests (like EVP_PKEY_CTX_new_from_name) over version number checks, as the functional approach directly tests the actual capability and is more reliable across different OpenSSL builds where features may be backported or disabled.
📚 Learning: 2025-09-11T16:54:00.370Z
Learnt from: bukka
Repo: softhsm/SoftHSMv2 PR: 815
File: src/lib/SoftHSM.cpp:6543-6546
Timestamp: 2025-09-11T16:54:00.370Z
Learning: SoftHSM PR #815 (pkcs11.h 3.1.0 update, CK_RSA_AES_KEY_WRAP_PARAMS fix): Maintainer bukka prefers to keep scope minimal (header sync and param rename only); broader OAEP parameter enforcement or backend plumbing is out of scope for this PR and should be handled in a follow-up issue.
Applied to files:
src/lib/crypto/OSSLDHPrivateKey.hsrc/lib/crypto/OSSLDH.cppsrc/lib/crypto/OSSLDHPrivateKey.cppsrc/lib/crypto/OSSLDHPublicKey.cpp
📚 Learning: 2025-08-19T09:23:25.434Z
Learnt from: antoinelochet
Repo: softhsm/SoftHSMv2 PR: 809
File: src/lib/SoftHSM.cpp:4193-4196
Timestamp: 2025-08-19T09:23:25.434Z
Learning: MLDSA parameter set inference: In src/lib/crypto/MLDSAPrivateKey.{h,cpp} and MLDSAPublicKey.{h,cpp}, getParameterSet() maps key material length to CKP_ML_DSA_44/65/87, and there is no setParameterSet(). Runtime MLDSA keys used by SoftHSM (e.g., in C_SignInit/C_VerifyInit via MLDSAUtil) rely on this inference; no explicit parameter-set assignment is required.
Applied to files:
src/lib/crypto/test/DHTests.cppsrc/lib/crypto/OSSLDH.cppsrc/lib/crypto/OSSLDHPrivateKey.cppsrc/lib/crypto/OSSLDHPublicKey.cpp
📚 Learning: 2025-10-27T16:44:57.865Z
Learnt from: antoinelochet
Repo: softhsm/SoftHSMv2 PR: 809
File: src/lib/SoftHSM.cpp:5550-5596
Timestamp: 2025-10-27T16:44:57.865Z
Learning: SoftHSM ML‑DSA additional context: avoid transient malloc in SoftHSM::{AsymSignInit, AsymVerifyInit}; let Session::setParameters deep‑copy SIGN_ADDITIONAL_CONTEXT by allocating one buffer for struct + context bytes and fixing the stored pointer; Session::resetOp frees m_pParameters so both struct and context are released.
Applied to files:
src/lib/crypto/test/DHTests.cppsrc/lib/crypto/OSSLDH.cppsrc/lib/crypto/OSSLDHPrivateKey.cppsrc/lib/crypto/OSSLDHPublicKey.cpp
📚 Learning: 2025-10-13T09:42:31.270Z
Learnt from: bukka
Repo: softhsm/SoftHSMv2 PR: 818
File: src/lib/SoftHSM.cpp:3238-3266
Timestamp: 2025-10-13T09:42:31.270Z
Learning: In src/lib/SoftHSM.cpp, SoftHSM::AsymDecryptInit should use MechParamCheckRSAPKCSOAEP for CKM_RSA_PKCS_OAEP to keep OAEP parameter validation identical to AsymEncryptInit, and avoid redundant manual checks for pMechanism->pParameter and MGF allowlists.
Applied to files:
src/lib/crypto/test/DHTests.cppsrc/lib/crypto/OSSLDH.cppsrc/lib/crypto/OSSLDHPrivateKey.cppsrc/lib/crypto/OSSLDHPublicKey.cpp
📚 Learning: 2025-08-18T19:46:46.659Z
Learnt from: antoinelochet
Repo: softhsm/SoftHSMv2 PR: 809
File: src/lib/SoftHSM.cpp:10044-10055
Timestamp: 2025-08-18T19:46:46.659Z
Learning: In SoftHSM::generateMLDSA (src/lib/SoftHSM.cpp), the project prefers using CKP_ML_DSA_* constants for parameter set checks and logs "Unsupported parameter set: %lu". The code currently returns CKR_PARAMETER_SET_NOT_SUPPORTED for unsupported sets.
Applied to files:
src/lib/crypto/test/DHTests.cppsrc/lib/crypto/OSSLDH.cppsrc/lib/crypto/OSSLDHPrivateKey.cppsrc/lib/crypto/OSSLDHPublicKey.cpp
📚 Learning: 2025-08-18T17:54:37.645Z
Learnt from: antoinelochet
Repo: softhsm/SoftHSMv2 PR: 809
File: m4/acx_openssl_mldsa.m4:10-35
Timestamp: 2025-08-18T17:54:37.645Z
Learning: For OpenSSL feature detection in autoconf macros, prefer functional tests (like EVP_PKEY_CTX_new_from_name) over version number checks, as the functional approach directly tests the actual capability and is more reliable across different OpenSSL builds where features may be backported or disabled.
Applied to files:
src/lib/crypto/OSSLDH.cppsrc/lib/crypto/OSSLDHPrivateKey.cpp
📚 Learning: 2025-08-18T17:59:30.693Z
Learnt from: antoinelochet
Repo: softhsm/SoftHSMv2 PR: 809
File: src/lib/crypto/MLDSAParameters.h:42-44
Timestamp: 2025-08-18T17:59:30.693Z
Learning: In MLDSAParameters.h, the author prefers to keep PKCS#11 constant references (CKP_ML_DSA_*) rather than explicit numeric values for better code clarity and maintainability.
Applied to files:
src/lib/crypto/OSSLDH.cppsrc/lib/crypto/OSSLDHPublicKey.cpp
📚 Learning: 2025-11-11T19:42:46.886Z
Learnt from: JoaoHenrique12
Repo: softhsm/SoftHSMv2 PR: 823
File: src/lib/SoftHSM.cpp:58-58
Timestamp: 2025-11-11T19:42:46.886Z
Learning: SoftHSM include policy: Algorithm key/parameter headers (e.g., SLHParameters, SLH*Key) are included unconditionally in SoftHSM.cpp/SoftHSM.h; feature gating is handled in mechanism tables and CryptoFactory (WITH_*). Avoid wrapping such includes in WITH_* guards.
Applied to files:
src/lib/crypto/OSSLDH.cpp
📚 Learning: 2025-08-11T16:42:23.012Z
Learnt from: bukka
Repo: softhsm/SoftHSMv2 PR: 806
File: src/lib/test/p11test.cpp:81-84
Timestamp: 2025-08-11T16:42:23.012Z
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/OSSLDH.cppsrc/lib/crypto/OSSLDHPrivateKey.cpp
🧬 Code graph analysis (2)
src/lib/crypto/OSSLDH.cpp (1)
src/lib/crypto/OSSLUtil.cpp (4)
byteString2bn(56-61)byteString2bn(56-56)bn2ByteString(42-53)bn2ByteString(42-42)
src/lib/crypto/OSSLDHPrivateKey.cpp (2)
src/lib/crypto/OSSLDHPublicKey.cpp (17)
inDH(69-76)resetOSSLKey(143-150)resetOSSLKey(143-143)resetOSSLKey(224-231)resetOSSLKey(224-224)setG(93-98)setG(93-93)setFromOSSL(117-141)setFromOSSL(117-117)setFromOSSL(197-222)setFromOSSL(197-197)setP(86-91)setP(86-86)createOSSLKey(153-186)createOSSLKey(153-153)createOSSLKey(234-300)createOSSLKey(234-234)src/lib/crypto/OSSLUtil.cpp (4)
bn2ByteString(42-53)bn2ByteString(42-42)byteString2bn(56-61)byteString2bn(56-56)
⏰ 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: Linux with OpenSSL 1.1.1
- GitHub Check: Windows (x64, openssl)
- GitHub Check: Windows (x64, botan)
- GitHub Check: Windows (x86, openssl)
- GitHub Check: Linux with OpenSSL 3.0
- GitHub Check: macOS (openssl)
- GitHub Check: macOS (botan)
- GitHub Check: Linux with Botan
🔇 Additional comments (7)
src/lib/crypto/OSSLDHPrivateKey.h (1)
38-102: LGTM! Clean dual-backend approach.The version-gated type system cleanly separates OpenSSL < 3.0 (DH*) from OpenSSL >= 3.0 (EVP_PKEY*) without introducing compatibility macros. The addition of
resetOSSLKey()centralizes cleanup logic, and the API surface adapts appropriately for each backend.src/lib/crypto/OSSLDH.cpp (3)
135-274: LGTM! Correct dual-backend key generation.The OpenSSL 3.0 path correctly uses
OSSL_PARAM_BLDto build DH parameters, creates a parameter-onlyEVP_PKEY, then usesEVP_PKEY_keygento generate the final keypair. Memory management is sound:
- BIGNUMs are freed in all error paths
- Intermediate parameter
EVP_PKEYis freed after successful keygen (line 256)- Version-appropriate cleanup at finalization (DH_free vs EVP_PKEY_free)
324-382: LGTM! Correct use of EVP_PKEY_derive with padding.The OpenSSL 3.0 path correctly uses
EVP_PKEY_CTX_set_dh_pad(ctx, 1)to enable automatic zero-padding, eliminating the manual compensation required in the pre-3.0 path (line 323). The two-phase derivation (query length, then derive) is the standard OpenSSL 3.0 pattern.
449-517: LGTM! Correct parameter generation and memory management.The OpenSSL 3.0 path correctly handles the different memory semantics:
- Pre-3.0:
DH_get0_pqgreturns const borrowed pointers (no free needed)- 3.0+:
EVP_PKEY_get_bn_paramallocates new BIGNUMs (freed at lines 515-516)Error paths properly clean up allocated resources before returning false.
src/lib/crypto/OSSLDHPrivateKey.cpp (1)
40-108: LGTM! Clean lifecycle management.The destructor and setters correctly use
resetOSSLKey()to invalidate the cached OpenSSL representation when key components change, preventing stale or inconsistent state. Constructor delegation tosetFromOSSLpromotes code reuse.src/lib/crypto/OSSLDHPublicKey.cpp (2)
50-105: LGTM! Consistent lifecycle and type management.The addition of the
typefield andisOfTypemethod improves runtime type safety. The setters correctly invalidate the cached OpenSSL representation viaresetOSSLKey(), matching the pattern used inOSSLDHPrivateKey.
234-300: LGTM! Correct EVP_PKEY construction for public key.The OpenSSL 3.0 path correctly builds a public-only
EVP_PKEYby pushingOSSL_PKEY_PARAM_FFC_P,OSSL_PKEY_PARAM_FFC_G, andOSSL_PKEY_PARAM_PUB_KEY, then usingEVP_PKEY_PUBLIC_KEYas the selection. Memory management is sound with proper cleanup of BIGNUMs and intermediate structures.
| // Create the OpenSSL representation of the key using OSSL_PARAM_BLD | ||
| void OSSLDHPrivateKey::createOSSLKey() | ||
| { | ||
| if (dh != NULL) return; | ||
|
|
||
| BIGNUM* bn_p = OSSL::byteString2bn(p); | ||
| BIGNUM* bn_g = OSSL::byteString2bn(g); | ||
| BIGNUM* bn_priv_key = OSSL::byteString2bn(x); | ||
|
|
||
| EVP_PKEY_CTX *ctx = EVP_PKEY_CTX_new_id(EVP_PKEY_DH, NULL); | ||
| if (!ctx) | ||
| { | ||
| ERROR_MSG("Could not create EVP_PKEY_CTX object"); | ||
| BN_free(bn_p); | ||
| BN_free(bn_g); | ||
| BN_free(bn_priv_key); | ||
| return; | ||
| } | ||
|
|
||
| OSSL_PARAM_BLD *bld = OSSL_PARAM_BLD_new(); | ||
| if (!bld) | ||
| { | ||
| ERROR_MSG("Could not create OSSL_PARAM_BLD"); | ||
| BN_free(bn_p); | ||
| BN_free(bn_g); | ||
| BN_free(bn_priv_key); | ||
| EVP_PKEY_CTX_free(ctx); | ||
| return; | ||
| } | ||
|
|
||
| if (!OSSL_PARAM_BLD_push_BN(bld, OSSL_PKEY_PARAM_FFC_P, bn_p) || | ||
| !OSSL_PARAM_BLD_push_BN(bld, OSSL_PKEY_PARAM_FFC_G, bn_g) || | ||
| !OSSL_PARAM_BLD_push_BN(bld, OSSL_PKEY_PARAM_PRIV_KEY, bn_priv_key)) | ||
| { | ||
| ERROR_MSG("Failed to push DH params to OSSL_PARAM_BLD"); | ||
| OSSL_PARAM_BLD_free(bld); | ||
| EVP_PKEY_CTX_free(ctx); | ||
| BN_free(bn_p); | ||
| BN_free(bn_g); | ||
| BN_free(bn_priv_key); | ||
| return; | ||
| } | ||
|
|
||
| OSSL_PARAM *params = OSSL_PARAM_BLD_to_param(bld); | ||
| if (!params) | ||
| { | ||
| ERROR_MSG("OSSL_PARAM_BLD_to_param failed"); | ||
| OSSL_PARAM_BLD_free(bld); | ||
| EVP_PKEY_CTX_free(ctx); | ||
| BN_free(bn_p); | ||
| BN_free(bn_g); | ||
| BN_free(bn_priv_key); | ||
| return; | ||
| } | ||
|
|
||
| if (EVP_PKEY_fromdata_init(ctx) <= 0 || EVP_PKEY_fromdata(ctx, &dh, EVP_PKEY_KEYPAIR, params) <= 0) | ||
| { | ||
| ERROR_MSG("EVP_PKEY_fromdata failed"); | ||
| dh = NULL; | ||
| } | ||
|
|
||
| OSSL_PARAM_free(params); | ||
| OSSL_PARAM_BLD_free(bld); | ||
| EVP_PKEY_CTX_free(ctx); | ||
| BN_free(bn_p); | ||
| BN_free(bn_g); | ||
| BN_free(bn_priv_key); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, check if this file exists and explore the context
fd "OSSLDHPrivateKey.cpp" --type fRepository: softhsm/SoftHSMv2
Length of output: 96
🏁 Script executed:
# Search for related DH public key computation patterns in the codebase
rg "OSSL_PKEY_PARAM_PUB_KEY" -A 3 -B 3Repository: softhsm/SoftHSMv2
Length of output: 1101
🏁 Script executed:
# Look for other uses of EVP_PKEY_fromdata with DH parameters
rg "EVP_PKEY_fromdata" -A 5 -B 5 --type cppRepository: softhsm/SoftHSMv2
Length of output: 2232
🏁 Script executed:
# Check if there are any tests or documentation about DH key handling
fd ".*DH.*" -type f | head -20Repository: softhsm/SoftHSMv2
Length of output: 231
🏁 Script executed:
# Read the full OSSLDHPrivateKey.cpp file around the createOSSLKey function
wc -l src/lib/crypto/OSSLDHPrivateKey.cppRepository: softhsm/SoftHSMv2
Length of output: 100
🏁 Script executed:
# Look at the legacy/pre-3.0 implementation in OSSLDHPrivateKey.cpp
cat -n src/lib/crypto/OSSLDHPrivateKey.cpp | head -250Repository: softhsm/SoftHSMv2
Length of output: 7626
🏁 Script executed:
# Search for BN_mod_exp usage in DH context (legacy public key computation)
rg "BN_mod_exp" -B 3 -A 3 --type cppRepository: softhsm/SoftHSMv2
Length of output: 958
🏁 Script executed:
# Read more of OSSLDHPrivateKey.cpp to see the full 3.0 createOSSLKey implementation
sed -n '250,402p' src/lib/crypto/OSSLDHPrivateKey.cppRepository: softhsm/SoftHSMv2
Length of output: 3526
🏁 Script executed:
# Check if there's any related issue or comment in the codebase about DH public key
rg "pub_key|PUB_KEY|public key" src/lib/crypto/OSSLDHPrivateKey.cpp -i -B 2 -A 2Repository: softhsm/SoftHSMv2
Length of output: 439
🌐 Web query:
OpenSSL 3.0 EVP_PKEY_fromdata DH keypair public key auto-computation
💡 Result:
Short answer: No — EVP_PKEY_fromdata will not auto-derive a classic DH public value from a supplied private value and (p,g). OpenSSL only automatically derives public keys from raw private bytes for the modern X/Ed algorithms (X25519/Ed25519/X448/Ed448); for DH you must provide the public component or use the key-generation APIs. [1][2][3]
What to do:
- Supply both private and public components in the OSSL_PARAM array to EVP_PKEY_fromdata, or
- Create domain parameters (p,g) with EVP_PKEY_fromdata then generate a keypair with EVP_PKEY_keygen/EVP_PKEY_generate, or
- Use the legacy DH APIs (DH_generate_key / DH_compute_key) if appropriate.
References:
- Raw-private-key auto-derivation (X/Ed) behavior. [1]
- EVP_PKEY_fromdata description and selection semantics. [2]
- EVP_PKEY-DH notes (validation and keygen examples). [3]
- Example/ mailing-list showing generation via param key + EVP_PKEY_generate. [4]
🏁 Script executed:
# Verify that OSSLDHPublicKey.cpp correctly pushes PUB_KEY (comparison)
sed -n '280,380p' src/lib/crypto/OSSLDHPublicKey.cppRepository: softhsm/SoftHSMv2
Length of output: 492
Add computation and inclusion of the DH public key parameter in OpenSSL 3.0 createOSSLKey.
OpenSSL 3.0's EVP_PKEY_fromdata does not automatically compute DH public keys from private keys (unlike X/Ed algorithms). The current implementation only pushes OSSL_PKEY_PARAM_FFC_P, OSSL_PKEY_PARAM_FFC_G, and OSSL_PKEY_PARAM_PRIV_KEY, but omits the required OSSL_PKEY_PARAM_PUB_KEY.
Compute the public key as g^x mod p before building parameters, matching the pre-3.0 implementation (line 244):
BIGNUM* bn_pub_key = BN_new();
BN_CTX *ctx = BN_CTX_new();
// ... error handling ...
BN_mod_exp(bn_pub_key, bn_g, bn_priv_key, bn_p, ctx);
BN_CTX_free(ctx);
// Then push to OSSL_PARAM_BLD:
if (!OSSL_PARAM_BLD_push_BN(bld, OSSL_PKEY_PARAM_FFC_P, bn_p) ||
!OSSL_PARAM_BLD_push_BN(bld, OSSL_PKEY_PARAM_FFC_G, bn_g) ||
!OSSL_PARAM_BLD_push_BN(bld, OSSL_PKEY_PARAM_PRIV_KEY, bn_priv_key) ||
!OSSL_PARAM_BLD_push_BN(bld, OSSL_PKEY_PARAM_PUB_KEY, bn_pub_key))
// ... cleanup including BN_free(bn_pub_key)| // Generate key-pair with a fixed private value length | ||
| p->setXBitLength(128); | ||
| CPPUNIT_ASSERT(dh->generateKeyPair(&kp, p)); | ||
|
|
||
| priv = (DHPrivateKey*) kp->getPrivateKey(); | ||
|
|
||
| CPPUNIT_ASSERT(priv->getX().bits() == 128); | ||
| CPPUNIT_ASSERT_EQUAL((unsigned long)(1024), priv->getBitLength()); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's find and examine the DHTests.cpp file
find . -name "DHTests.cpp" -type fRepository: softhsm/SoftHSMv2
Length of output: 94
🏁 Script executed:
# Read the specific lines mentioned in the review (94-99) plus context (84-104)
if [ -f "src/lib/crypto/test/DHTests.cpp" ]; then
sed -n '84,104p' "src/lib/crypto/test/DHTests.cpp"
fiRepository: softhsm/SoftHSMv2
Length of output: 625
🏁 Script executed:
# Check if DHPrivateKey class has setXBitLength method
find . -name "DHPrivateKey.*" -type fRepository: softhsm/SoftHSMv2
Length of output: 126
🏁 Script executed:
# Search for any references to setXBitLength in the DH-related files
rg "setXBitLength" --type cpp --type hRepository: softhsm/SoftHSMv2
Length of output: 389
🏁 Script executed:
# Check the git history to see if setXBitLength was removed
git log --oneline -20Repository: softhsm/SoftHSMv2
Length of output: 99
🏁 Script executed:
# Get the full test function to see the complete context
sed -n '67,104p' "src/lib/crypto/test/DHTests.cpp"Repository: softhsm/SoftHSMv2
Length of output: 1040
🏁 Script executed:
# Check the DHParameters.h to understand setXBitLength
head -100 "src/lib/crypto/DHParameters.h"Repository: softhsm/SoftHSMv2
Length of output: 2595
🏁 Script executed:
# Get the git diff for the commit that migrated to OpenSSL 3.0
git show --stat 35f6662Repository: softhsm/SoftHSMv2
Length of output: 39010
🏁 Script executed:
# Check the git diff for DHTests.cpp to see what changed in this section
git show 35f6662:src/lib/crypto/test/DHTests.cpp | head -120 | tail -60Repository: softhsm/SoftHSMv2
Length of output: 1507
🏁 Script executed:
# Also check if there's a previous version to compare
git log --oneline -1 35f6662Repository: softhsm/SoftHSMv2
Length of output: 99
🏁 Script executed:
# Look at the OSSLDH.cpp migration to understand how DH key length is handled
head -200 "src/lib/crypto/OSSLDH.cpp"Repository: softhsm/SoftHSMv2
Length of output: 5121
🏁 Script executed:
# Continue reading OSSLDH.cpp to see the OpenSSL 3.0 implementation
sed -n '200,350p' "src/lib/crypto/OSSLDH.cpp"Repository: softhsm/SoftHSMv2
Length of output: 3647
🏁 Script executed:
# Also search for setXBitLength usage in OSSLDH.cpp
rg "setXBitLength|getXBitLength" "src/lib/crypto/OSSLDH.cpp"Repository: softhsm/SoftHSMv2
Length of output: 270
🏁 Script executed:
# Check what happened to the setXBitLength call in DHTests by looking at git history
git log -p --all -S "setXBitLength" -- "src/lib/crypto/test/DHTests.cpp" | head -100Repository: softhsm/SoftHSMv2
Length of output: 43
🏁 Script executed:
# Check if there's a previous commit before the migration that had DHTests.cpp
git log --oneline --all -- "src/lib/crypto/test/DHTests.cpp" | head -5Repository: softhsm/SoftHSMv2
Length of output: 349
🏁 Script executed:
# Let's look at what the test actually does - check if there's any old version
git show 35f6662~1:src/lib/crypto/test/DHTests.cpp 2>/dev/null | grep -A 20 "Generate key-pair with a fixed"Repository: softhsm/SoftHSMv2
Length of output: 43
🏁 Script executed:
# Let's search the whole history for when setXBitLength was used in DHTests
git log --all -p -- "src/lib/crypto/test/DHTests.cpp" | grep -B 10 -A 10 "setXBitLength" | head -40Repository: softhsm/SoftHSMv2
Length of output: 1184
🏁 Script executed:
# Let's get the full context of how testKeyGeneration looked before the migration
git show 35f6662~1:src/lib/crypto/test/DHTests.cpp 2>/dev/null | sed -n '67,104p'Repository: softhsm/SoftHSMv2
Length of output: 43
🏁 Script executed:
# Let's check the git log more carefully
git log --oneline --all | head -20Repository: softhsm/SoftHSMv2
Length of output: 1390
🏁 Script executed:
# Get the full parent commit information
git show 35f6662~1 --stat | head -20Repository: softhsm/SoftHSMv2
Length of output: 250
🏁 Script executed:
# Try a different approach - get all revisions of DHTests.cpp
git log --all --oneline -- "src/lib/crypto/test/DHTests.cpp"Repository: softhsm/SoftHSMv2
Length of output: 616
🏁 Script executed:
# Let's check the commit before 35f6662 using the direct parent
git show 8d80107:src/lib/crypto/test/DHTests.cpp 2>/dev/null | sed -n '67,104p'Repository: softhsm/SoftHSMv2
Length of output: 1040
🏁 Script executed:
# Let's also check the OpenSSL 3.0 migration commit itself to see what changed
git show 35f6662 -- "src/lib/crypto/test/DHTests.cpp" | grep -A 30 "testKeyGeneration"Repository: softhsm/SoftHSMv2
Length of output: 941
Update the comment to reflect the actual test behavior.
The comment states "Generate key-pair with a fixed private value length", but the call to p->setXBitLength(128) was removed during the OpenSSL 3.0 migration. The test now simply generates a second key pair on the same parameters and verifies its bit length matches the parameter size (1024 bits).
Update the comment to something like: "Generate a second key pair on the same parameters" or remove the reference to "fixed private value length".
🤖 Prompt for AI Agents
In @src/lib/crypto/test/DHTests.cpp around lines 94 - 99, The comment above the
dh->generateKeyPair(&kp, p) call incorrectly mentions a "fixed private value
length" (p->setXBitLength was removed); update that comment in DHTests.cpp to
reflect the current behavior—e.g., "Generate a second key pair on the same
parameters"—or simply remove the reference to fixed private value length so it
matches the test that creates a second key pair and asserts priv->getBitLength()
== 1024.
There are still some enterprise distros that run it (and fix the security issue there) but they wouldn't probably pick up new SoftHSMv2 anyway so that's not a bit problem. The old API is also used by LibreSSL but I guess it's not probably worth keeping it for that. So I agree that we should eventually drop it. But for migration it is actually useful and safer to do it this way. Not just because it's easier for me to check it but also our AI reviews catch more mistakes as it directly compares the new version with the old one. Code Rabbit actually caught couple of issues related to differences so I can already see benefits of that. So what I'm thinking is to do the migration in this way (supporting both) and once it is done and there are no deprecations in version for 3.0+, then we can drop 1.1.1. It will take some time so it will make chance that it will cause trouble to someone still running 1.1.1 less likely. |
2 participants
This removes OpenSSL 3.0+ deprecations by using the right APIs. Specifically it uses EVP_PKEY for keys and param API for operations. It tries to create a minimal diff without changing much for OpenSSL 1.1 and keep the common code the same. The idea is that when at some point in the future the 1.1 support is dropped, it will be easier to drop it from code - that's why it doesn't try to use some special compatibility macros.
This is the first part for DH and the idea is start working on other parts (DSA, RSA, EC...) once this is accepted.
Summary by CodeRabbit
New Features
Improvements
Tests
✏️ Tip: You can customize this high-level summary in your review settings.