Fixed EDDSA context/pre-hash support - #892
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds EdDSA context and pre-hash support through ChangesEdDSA Context and Pre-hash Support
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Application
participant SoftHSM
participant EDDSAUtil
participant Backend as BotanEDDSA or OSSLEDDSA
Application->>SoftHSM: Initialize CKM_EDDSA with parameters
SoftHSM->>EDDSAUtil: Parse and validate parameters
EDDSAUtil-->>SoftHSM: Return EDDSAMechanismParam
SoftHSM->>Backend: Start sign or verify with parameters
Backend->>Backend: Select pre-hash mode and apply context
Backend-->>SoftHSM: Return signature or verification status
SoftHSM-->>Application: Return operation result
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
569b85d to
a3cf255
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (3)
src/lib/crypto/BotanEDDSA.cpp (1)
124-131: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueRemove the unreachable
Ed448phfallbackBotanEDDSA.cpponly acceptsBotan::Ed25519_{Private,Public}Key, and bothBotanEDPrivateKey::getOrderLength()andBotanEDPublicKey::getOrderLength()return32, so this branch can never run. The same pattern exists inverify(), soEd25519phis the only pre-hash EMSA used here.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/crypto/BotanEDDSA.cpp` around lines 124 - 131, Remove the unreachable Ed448ph fallback in BotanEDDSA by keeping Ed25519ph as the only EMSA chosen from pk->getOrderLength() in both the signing and verify paths. Since BotanEDDSA only works with Botan::Ed25519_{Private,Public}Key and BotanEDPrivateKey::getOrderLength()/BotanEDPublicKey::getOrderLength() always return 32, update the logic in the relevant functions to eliminate the else branch and any duplicate Ed448ph handling.src/lib/crypto/EDDSAMechanismParam.h (1)
20-41: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valuePass
contextDataby const reference.
EDDSAMechanismParam(bool flag, ByteString contextData)takescontextDataby value, causing an extra copy on every call;const ByteString&avoids it.♻️ Proposed fix
- EDDSAMechanismParam(bool flag, ByteString contextData); + EDDSAMechanismParam(bool flag, const ByteString& contextData);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/crypto/EDDSAMechanismParam.h` around lines 20 - 41, EDDSAMechanismParam’s constructor currently copies contextData unnecessarily by taking it by value. Update the EDDSAMechanismParam(bool flag, ByteString contextData) declaration and corresponding definition to take contextData as a const ByteString& instead, and keep the member initialization in EDDSAMechanismParam consistent with that change.src/lib/SoftHSM.cpp (1)
4609-4622: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winMissing upper-bound check on EdDSA context length.
Per RFC 8032, EdDSA context strings must be at most 255 octets, yet
ulContextDataLenis accepted unbounded here. The siblingCKM_ML_DSAcase in this same file validatesulContextLen > 255; consider applying the same guard forCKM_EDDSA.🛡️ Proposed fix
CK_EDDSA_PARAMS* ckEddsaParams = (CK_EDDSA_PARAMS*) pMechanism->pParameter; eddsaParam.flag = (ckEddsaParams->phFlag != 0x00); if (ckEddsaParams->ulContextDataLen > 0) { if (ckEddsaParams->pContextData == NULL_PTR) { ERROR_MSG("Invalid parameters"); return CKR_ARGUMENTS_BAD; } + if (ckEddsaParams->ulContextDataLen > 255) + { + ERROR_MSG("Invalid parameters"); + return CKR_ARGUMENTS_BAD; + } eddsaParam.contextData = ByteString(ckEddsaParams->pContextData, ckEddsaParams->ulContextDataLen);Also applies to: 5730-5743
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/SoftHSM.cpp` around lines 4609 - 4622, The CKM_EDDSA handling in SoftHSM::signMechanism accepts an unbounded context length, so add an upper-bound check before building eddsaParam from CK_EDDSA_PARAMS. Validate that ulContextDataLen is not greater than 255, mirroring the CKM_ML_DSA guard in this file, and return CKR_ARGUMENTS_BAD with an error message when the limit is exceeded; keep the existing NULL_PTR check and ByteString assignment unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/lib/crypto/BotanEDDSA.cpp`:
- Around line 101-132: The Botan EDDSA path is ignoring caller-provided
contextData when preHash is true, which can produce signatures that do not match
context-aware verification. In BotanEDDSA::sign or the mechanism-parameter
handling block, either explicitly reject any non-empty eddsaParam->contextData
for both preHash and non-preHash cases, or wire it through only if Botan
supports it; do not silently discard it. Also remove the unused local ByteString
contextData or assign and validate it consistently alongside
EDDSAMechanismParam.
In `@src/lib/crypto/OSSLEDDSA.cpp`:
- Around line 115-138: The instance selection in OSSLEDDSA::sign is dropping
valid Ed448 context-only requests and misclassifying Ed25519 when both context
and pre-hash are requested. Update the branching on pk->getOrderLength() so
Ed448 sets the native Ed448 instance whenever contextData is present (not only
when preHash is true), and ensure the Ed25519 path explicitly handles the ph+ctx
combination instead of letting contextData.size() win first. Apply the same
selection rules in OSSLEDDSA::verify so signing and verification stay aligned.
- Around line 161-170: The OpenSSL error logging in OSSLEDDSA.cpp is fetching
the error queue twice per message, so the numeric code and text can mismatch.
Update the sign and verify error paths in the EDDSA helpers to call
ERR_get_error() once, store the result in a local variable, and use that same
value for both the formatted code and ERR_error_string() output. Apply the same
fix in the EVP_DigestSignInit, EVP_DigestSign, and corresponding verification
error logs.
- Around line 140-158: The EdDSA instance/context parameters are being applied
before EVP_DigestSignInit/EVP_DigestVerifyInit, so EVP_MD_CTX_get_pkey_ctx(ctx)
can be NULL and the params never reach OpenSSL. Update OSSLEDDSA.cpp to move the
OSSL_PARAM setup in the EdDSA sign/verify flow to the pctx obtained after init,
or switch to the _ex APIs, and ensure the same logic is used in both the signing
and verification paths so Ed25519ctx/ph and Ed448ph are actually selected.
- Around line 147-151: The `instance` parameter is being built with the wrong
OpenSSL param constructor in `OSSLEDDSA` because it is a `const char *`, not a
writable string buffer. Update the `OSSL_PARAM` construction in the signing path
and the matching verify path in `OSSLEDDSA` to use `OSSL_PARAM_utf8_ptr` for
`OSSL_SIGNATURE_PARAM_INSTANCE`, passing the pointer variable itself and the
correct string length, while leaving the context string param unchanged.
In `@src/lib/test/SignVerifyTests.cpp`:
- Around line 847-1029: The new EdDSA context tests in
testEdSignVerifyWithContext and testEdSignVerifyWithContextPreHashed only cover
successful same-context verification, so add negative assertions that a
signature created with one CK_EDDSA_PARAMS cannot be verified with a different
context or with the other phFlag mode. Reuse the existing signVerifySingle flow
or the lower-level sign/verify helpers around generateED and CK_EDDSA_PARAMS to
produce one signature and then verify it with mismatched parameters for each key
pair type, ensuring the test explicitly checks cross-context and
plain-vs-prehash failures.
---
Nitpick comments:
In `@src/lib/crypto/BotanEDDSA.cpp`:
- Around line 124-131: Remove the unreachable Ed448ph fallback in BotanEDDSA by
keeping Ed25519ph as the only EMSA chosen from pk->getOrderLength() in both the
signing and verify paths. Since BotanEDDSA only works with
Botan::Ed25519_{Private,Public}Key and
BotanEDPrivateKey::getOrderLength()/BotanEDPublicKey::getOrderLength() always
return 32, update the logic in the relevant functions to eliminate the else
branch and any duplicate Ed448ph handling.
In `@src/lib/crypto/EDDSAMechanismParam.h`:
- Around line 20-41: EDDSAMechanismParam’s constructor currently copies
contextData unnecessarily by taking it by value. Update the
EDDSAMechanismParam(bool flag, ByteString contextData) declaration and
corresponding definition to take contextData as a const ByteString& instead, and
keep the member initialization in EDDSAMechanismParam consistent with that
change.
In `@src/lib/SoftHSM.cpp`:
- Around line 4609-4622: The CKM_EDDSA handling in SoftHSM::signMechanism
accepts an unbounded context length, so add an upper-bound check before building
eddsaParam from CK_EDDSA_PARAMS. Validate that ulContextDataLen is not greater
than 255, mirroring the CKM_ML_DSA guard in this file, and return
CKR_ARGUMENTS_BAD with an error message when the limit is exceeded; keep the
existing NULL_PTR check and ByteString assignment unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c7e7c86e-e220-428d-ade4-244efe719345
📒 Files selected for processing (9)
src/lib/SoftHSM.cppsrc/lib/crypto/BotanEDDSA.cppsrc/lib/crypto/CMakeLists.txtsrc/lib/crypto/EDDSAMechanismParam.cppsrc/lib/crypto/EDDSAMechanismParam.hsrc/lib/crypto/Makefile.amsrc/lib/crypto/OSSLEDDSA.cppsrc/lib/test/SignVerifyTests.cppsrc/lib/test/SignVerifyTests.h
a3cf255 to
96365cc
Compare
82a7676 to
c4e8cca
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/lib/test/SignVerifyTests.cpp (1)
790-793: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winGate the Botan skip on the Botan version and print a skip message.
The comment states that Botan 2.X lacks context support, but the guard skips the test for every Botan version. The test then stays a silent no-op after an upgrade to a Botan version that supports context. The OpenSSL branch below prints a message; this branch does not, so the skip is invisible in test output. The stray extra indentation on line 792 also suggests a leftover condition.
♻️ Suggested change
`#ifdef` WITH_BOTAN - // Botan 2.X does not support EdDSA with context, so we skip this test for now. - return; + // Botan 2.X does not support EdDSA with context, so we skip this test for now. + fprintf(stdout, "Botan does not support EdDSA with context. Skipping testEdSignVerifyWithContext.\n"); + return; `#endif`🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/test/SignVerifyTests.cpp` around lines 790 - 793, Update the Botan guard in the test containing the EdDSA context case to skip only for Botan 2.x, using the project’s Botan version symbols or checks, and allow supported newer versions to execute the test. Before returning from the version-limited skip, print an explicit skip message consistent with the OpenSSL branch, and remove the stray indentation.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/lib/test/SignVerifyTests.cpp`:
- Around line 927-951: Update the Ed25519ph tests around eddsaParams and
eddsaParams2 to use signVerifySingleData with a 64-byte prehashed message,
passing the CK_EDDSA_PARAMS separately as signing parameters. Keep contextData
and anotherContext only as EdDSA context values, and add the different-context
case through the same signed-message path to verify cross-context mismatch
rather than using signVerifySingle.
---
Nitpick comments:
In `@src/lib/test/SignVerifyTests.cpp`:
- Around line 790-793: Update the Botan guard in the test containing the EdDSA
context case to skip only for Botan 2.x, using the project’s Botan version
symbols or checks, and allow supported newer versions to execute the test.
Before returning from the version-limited skip, print an explicit skip message
consistent with the OpenSSL branch, and remove the stray indentation.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: b4ba4aa4-4cc6-4452-b1fa-9138585667cf
📒 Files selected for processing (11)
src/bin/util/softhsm2-util-botan.cppsrc/lib/SoftHSM.cppsrc/lib/crypto/BotanEDDSA.cppsrc/lib/crypto/CMakeLists.txtsrc/lib/crypto/EDDSAMechanismParam.cppsrc/lib/crypto/EDDSAMechanismParam.hsrc/lib/crypto/Makefile.amsrc/lib/crypto/OSSLEDDSA.cppsrc/lib/crypto/OSSLEDDSA.hsrc/lib/test/SignVerifyTests.cppsrc/lib/test/SignVerifyTests.h
🚧 Files skipped from review as they are similar to previous changes (8)
- src/lib/crypto/Makefile.am
- src/lib/crypto/EDDSAMechanismParam.cpp
- src/lib/crypto/EDDSAMechanismParam.h
- src/lib/crypto/BotanEDDSA.cpp
- src/lib/SoftHSM.cpp
- src/lib/crypto/OSSLEDDSA.cpp
- src/lib/crypto/CMakeLists.txt
- src/lib/test/SignVerifyTests.h
5ba577a to
3df3213
Compare
8849a9d to
ff39f44
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/lib/test/SignVerifyTests.cpp (1)
963-1004: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
testEdSignVerifyWithContextPreHashednever sets context data.All three parameter structs set
ulContextDataLen = 0andpContextData = NULL:
- Line 965
eddsaParams- Line 983
eddsaParams2- Line 1000
eddsaParamsNoCtxThe three structs are byte-identical, and the test name promises context coverage. The combination of
phFlag = CK_TRUEwith non-emptypContextDatais therefore untested through the PKCS#11 layer. That combination is the one the Botan backend rejects inBotanEDDSA::selectEmsa, and the one the OpenSSL backend must encode as a three-entryOSSL_PARAMarray.src/lib/crypto/test/EDDSATests.cppLine 614 covers it at the crypto layer for Ed448 only.Set a real context on at least one of the parameter sets, and drop the duplicate struct.
🧪 Proposed change to exercise pre-hash together with context
CK_BYTE message[] = { 0x11, 0x79, ... }; + CK_BYTE contextData[] = "context-data"; CK_EDDSA_PARAMS eddsaParams = { CK_TRUE, // phFlag = 1 (pre-hash) - 0, // context_data_len = 0 - NULL // context_data = NULL + (CK_ULONG)(sizeof(contextData) - 1), // context_data_len + contextData // context_data };Then keep
eddsaParamsNoCtxas the pre-hash-without-context case and removeeddsaParams2.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/test/SignVerifyTests.cpp` around lines 963 - 1004, Update testEdSignVerifyWithContextPreHashed so at least one CK_EDDSA_PARAMS instance uses a non-empty context buffer and matching context length while phFlag remains CK_TRUE, exercising the PKCS#11 context path. Remove the duplicate eddsaParams2 definition and reuse eddsaParamsNoCtx for the pre-hash-without-context case, preserving coverage for both configurations.
🧹 Nitpick comments (3)
src/lib/test/SignVerifyTests.cpp (2)
866-874: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDeclare
paramsas a scalar, not a one-element array.
CK_EDDSA_PARAMS params[]holds a single element, and Lines 879 and 884 then use¶ms[0]andsizeof(params[0]). The other parameter sets in this file, for exampleeddsaParams2at Line 889, are plain scalars. Use a scalar here for consistency.♻️ Proposed change
- CK_EDDSA_PARAMS params[] = { - { - CK_FALSE, // phFlag = 0 (no pre-hash) - dataSize, // context_data_len - contextData // context_data - } - }; + CK_EDDSA_PARAMS eddsaParams = { + CK_FALSE, // phFlag = 0 (no pre-hash) + dataSize, // context_data_len + contextData // context_data + };Then replace
¶ms[0], sizeof(params[0])with&eddsaParams, sizeof(eddsaParams)on Lines 879 and 884.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/test/SignVerifyTests.cpp` around lines 866 - 874, Change the single-element CK_EDDSA_PARAMS variable params to a scalar named eddsaParams, and update both uses in the related test calls to pass &eddsaParams and sizeof(eddsaParams) instead of indexing params.
824-832: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse the runtime capability flags instead of an unconditional
return.Lines 824-828 return unconditionally in a Botan build. Every statement after Line 833 becomes unreachable, and the
curveparameter becomes unused, which can produce a compiler warning.testEdSignVerifyMismatchedParamsat Lines 1036-1054 already uses runtimeboolflags for the same capability decisions. Apply that pattern here so the code stays reachable and the intent stays in one place.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/test/SignVerifyTests.cpp` around lines 824 - 832, Update testEdSignVerifyWithContext to use runtime capability booleans, following the pattern in testEdSignVerifyMismatchedParams, instead of returning unconditionally under Botan or older OpenSSL. Gate the relevant test operations with those flags so the remaining function stays reachable and the curve parameter remains used.src/lib/crypto/EDDSAUtil.cpp (1)
22-32: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winDocument
getEddsaParamoutput requirements.The EDDSA init paths use a function-local
EDDSAMechanismParam eddsaParam, andgetEddsaParam(...)stores its address when parameters are present. Callers must keep that object alive throughC_Sign/C_Verify, and call sites must initializemechanismParamtoNULLbefore the call because the no-parameter path does not write to it. Add this contract toEDDSAUtil.h.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/crypto/EDDSAUtil.cpp` around lines 22 - 32, Document the getEddsaParam contract in EDDSAUtil.h: callers must initialize mechanismParam to NULL before calling it, and any EDDSAMechanismParam object whose address is stored must remain alive through C_Sign or C_Verify. Ensure the declaration documentation covers both parameter-present and no-parameter paths without changing the implementation.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/lib/crypto/BotanEDDSA.cpp`:
- Around line 90-94: Update the PK_Signer and PK_Verifier construction error
paths in the Botan EdDSA implementation to include the selected emsa value
returned by selectEmsa(), while preserving the existing error context and
behavior.
In `@src/lib/crypto/test/EDDSATests.cpp`:
- Around line 46-61: Guard EDDSA-specific test code in EDDSATests.cpp with
WITH_EDDSA, including the EDDSAMechanismParam.h include, EDDSATests.h include,
and related test registration/class methods. Ensure cryptotest does not compile
this source or any EDDSA symbols when WITH_EDDSA is disabled.
---
Duplicate comments:
In `@src/lib/test/SignVerifyTests.cpp`:
- Around line 963-1004: Update testEdSignVerifyWithContextPreHashed so at least
one CK_EDDSA_PARAMS instance uses a non-empty context buffer and matching
context length while phFlag remains CK_TRUE, exercising the PKCS#11 context
path. Remove the duplicate eddsaParams2 definition and reuse eddsaParamsNoCtx
for the pre-hash-without-context case, preserving coverage for both
configurations.
---
Nitpick comments:
In `@src/lib/crypto/EDDSAUtil.cpp`:
- Around line 22-32: Document the getEddsaParam contract in EDDSAUtil.h: callers
must initialize mechanismParam to NULL before calling it, and any
EDDSAMechanismParam object whose address is stored must remain alive through
C_Sign or C_Verify. Ensure the declaration documentation covers both
parameter-present and no-parameter paths without changing the implementation.
In `@src/lib/test/SignVerifyTests.cpp`:
- Around line 866-874: Change the single-element CK_EDDSA_PARAMS variable params
to a scalar named eddsaParams, and update both uses in the related test calls to
pass &eddsaParams and sizeof(eddsaParams) instead of indexing params.
- Around line 824-832: Update testEdSignVerifyWithContext to use runtime
capability booleans, following the pattern in testEdSignVerifyMismatchedParams,
instead of returning unconditionally under Botan or older OpenSSL. Gate the
relevant test operations with those flags so the remaining function stays
reachable and the curve parameter remains used.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 0a4439d9-12cc-49b3-8c0f-9ca481d11656
📒 Files selected for processing (16)
src/bin/util/softhsm2-util-botan.cppsrc/lib/SoftHSM.cppsrc/lib/crypto/BotanEDDSA.cppsrc/lib/crypto/BotanEDDSA.hsrc/lib/crypto/CMakeLists.txtsrc/lib/crypto/EDDSAMechanismParam.cppsrc/lib/crypto/EDDSAMechanismParam.hsrc/lib/crypto/EDDSAUtil.cppsrc/lib/crypto/EDDSAUtil.hsrc/lib/crypto/Makefile.amsrc/lib/crypto/OSSLEDDSA.cppsrc/lib/crypto/OSSLEDDSA.hsrc/lib/crypto/test/EDDSATests.cppsrc/lib/crypto/test/EDDSATests.hsrc/lib/test/SignVerifyTests.cppsrc/lib/test/SignVerifyTests.h
🚧 Files skipped from review as they are similar to previous changes (4)
- src/lib/crypto/Makefile.am
- src/bin/util/softhsm2-util-botan.cpp
- src/lib/SoftHSM.cpp
- src/lib/crypto/OSSLEDDSA.cpp
be3dee8 to
dd4bcb2
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/lib/test/SignVerifyTests.cpp`:
- Around line 923-1015: Extend testEdSignVerifyWithContextPreHashed and the
related context test flow to cover non-empty context data with phFlag set to
CK_TRUE. Add one successful sign/verify case using matching context parameters
and one mismatched-context case that must fail, reusing the existing
key-generation and assertion helpers.
- Around line 341-374: Update signVerifySingleData to guard the modified-input
verification block with a dataSize nonzero check, so data[0] is only read and
written when the input contains at least one byte. Preserve the existing
signature-invalid assertion and buffer restoration for non-empty messages.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: febb1e85-b117-4e81-be0e-c8064cf64b21
📒 Files selected for processing (1)
src/lib/test/SignVerifyTests.cpp
b2bd467 to
923e4f8
Compare
923e4f8 to
9898a0f
Compare
2 participants
Fix for #873
Summary by CodeRabbit
New Features
Bug Fixes
Tests