Added support for ML-DSA in softhsm2-util#870
Open
antoinelochet wants to merge 1 commit into
Open
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 ML‑DSA import and persistence to the softhsm2-util OpenSSL backend: detects provider-backed ML‑DSA EVP_PKEYs, extracts seed/private/public components, maps parameter sets to PKCS#11 attributes, and persists key pairs via new save/alloc/free helpers (guarded by WITH_ML_DSA). ChangesML-DSA Import and Save Logic
Sequence Diagram(s)sequenceDiagram
participant Tool as softhsm2-util
participant OpenSSL as OpenSSL EVP (provider)
participant PKCS11 as PKCS#11 Token
Tool->>OpenSSL: Import request -> provider-backed EVP_PKEY
OpenSSL-->>Tool: EVP_PKEY (ML-DSA)
Tool->>OpenSSL: Query EVP_PKEY_is_a / KEYMGMT for ML-DSA
OpenSSL-->>Tool: seed, privValue, pubValue, parameterSet
Tool->>PKCS11: crypto_save_mldsa -> C_CreateObject(private, attrs: CKA_SEED, CKA_VALUE, CKA_PARAMETER_SET)
PKCS11-->>Tool: private object stored
Tool->>PKCS11: crypto_save_mldsa -> C_CreateObject(public, attrs: CKA_VALUE, CKA_PARAMETER_SET)
PKCS11-->>Tool: public object stored
Tool-->>Tool: print success message (includes label)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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 |
antoinelochet
force-pushed
the
feature/ml-dsa-softhsmutil
branch
from
422b004 to
21beaf3
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmake/modules/CompilerOptions.cmake`:
- Around line 373-379: The CMake logic for ML-DSA currently toggles WITH_ML_DSA
based only on ENABLE_MLDSA; add a functional probe like the existing EDDSA/GOST
probes by using try_compile/try_run to compile and run a small test that calls
EVP_PKEY_CTX_new_from_name(NULL, "ML-DSA-44", NULL) (or the same test snippet
used for autoconf) and set WITH_ML_DSA only if the probe succeeds; if
ENABLE_MLDSA is ON but the probe fails, emit a fatal error (message(FATAL_ERROR
...)) so the build fails fast, mirroring the behavior of the autoconf macro and
the EDDSA/GOST checks.
In `@src/bin/util/softhsm2-util-ossl.cpp`:
- Around line 440-442: The fprintf call that prints the PKCS#8 read failure is
malformed ("Error code: %")—fix it by formatting and passing the actual error
value or message: change the call to include a proper format specifier (e.g.,
"Error code: %d\n" or "Error: %s\n") and pass the appropriate error variable
(for example rc, err, or use ERR_error_string(ERR_get_error(), NULL) if using
OpenSSL). Update the fprintf invocation in the same block (the fprintf(...) that
reports "Could not read the PKCS#8 file") to use the corrected format and
argument so the error code/text is shown.
- Around line 1276-1350: The function crypto_malloc_mldsa leaks keyMat on early
returns and risks deref if mallocs fail before memcpy; ensure every early error
path calls crypto_free_mldsa(keyMat) (not plain return) after zeroing sensitive
buffers (seed, priv, pub) and only perform memcpy(keyMat->seed,...),
memcpy(keyMat->privValue,...) and memcpy(keyMat->pubValue,...) after successful
malloc checks of keyMat->seed, keyMat->privValue and keyMat->pubValue;
specifically update the error branches following EVP_PKEY_get_octet_string_param
(for seed, priv, pub) to free keyMat on failure and add immediate null-checks of
allocations right after the three malloc(...) calls and before the memcpy calls,
using crypto_free_mldsa to clean up on allocation failure.
🪄 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: 53d311e7-ea48-4feb-9f8b-598f85e8bb0e
📒 Files selected for processing (3)
cmake/modules/CompilerOptions.cmakesrc/bin/util/softhsm2-util-ossl.cppsrc/bin/util/softhsm2-util-ossl.h
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/bin/util/softhsm2-util-ossl.cpp`:
- Around line 198-201: In crypto_import_key_pair(), the error path currently
returns NULL (a pointer) inside an int function, causing a false success; change
the return from NULL to 0 to indicate failure, keeping the existing
EVP_PKEY_free(pkey) and stderr log intact so the function properly signals
failure on the OSSL_PKEY_PARAM_ML_DSA_SEED size retrieval error.
- Around line 193-202: The code currently treats any failure to get
OSSL_PKEY_PARAM_ML_DSA_SEED via EVP_PKEY_get_octet_string_param as a hard error;
instead, make the seed optional: call EVP_PKEY_get_octet_string_param(pkey,
OSSL_PKEY_PARAM_ML_DSA_SEED, NULL, 0, &seed_len) and if it returns 1 proceed to
allocate and retrieve the seed and add CKA_SEED to the object template, if it
returns 0 treat the seed as absent and continue without adding CKA_SEED, and
only treat negative return values as real errors; mirror the behavior used in
OSSLMLDSAPrivateKey::setFromOSSL so missing seed is handled gracefully rather
than failing the import.
🪄 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: 6c788348-9406-4df2-8231-03046d9e1330
📒 Files selected for processing (3)
cmake/modules/CompilerOptions.cmakesrc/bin/util/softhsm2-util-ossl.cppsrc/bin/util/softhsm2-util-ossl.h
🚧 Files skipped from review as they are similar to previous changes (2)
- cmake/modules/CompilerOptions.cmake
- src/bin/util/softhsm2-util-ossl.h
antoinelochet
force-pushed
the
feature/ml-dsa-softhsmutil
branch
from
21beaf3 to
090bbd1
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/bin/util/softhsm2-util-ossl.cpp (1)
1262-1263: 💤 Low valueRemove unused macro redefinitions.
PUBPREFIXLENandPRIVPREFIXLENare already defined in the EdDSA section (lines 1083-1084) and are not used in the ML-DSA implementation. These lines appear to be copy-paste artifacts.🧹 Suggested removal
-// Convert the OpenSSL key to binary -#define PUBPREFIXLEN 12 -#define PRIVPREFIXLEN 16 - +// Convert the OpenSSL key to binary🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/bin/util/softhsm2-util-ossl.cpp` around lines 1262 - 1263, Remove the duplicate macro redefinitions PUBPREFIXLEN and PRIVPREFIXLEN from the ML-DSA section (the lines that define "#define PUBPREFIXLEN 12" and "#define PRIVPREFIXLEN 16"); these are already defined earlier in the EdDSA section and are unused in the ML-DSA implementation, so delete those two defines and run a build to confirm no remaining references to PUBPREFIXLEN/PRIVPREFIXLEN are broken.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/bin/util/softhsm2-util-ossl.cpp`:
- Around line 1262-1263: Remove the duplicate macro redefinitions PUBPREFIXLEN
and PRIVPREFIXLEN from the ML-DSA section (the lines that define "#define
PUBPREFIXLEN 12" and "#define PRIVPREFIXLEN 16"); these are already defined
earlier in the EdDSA section and are unused in the ML-DSA implementation, so
delete those two defines and run a build to confirm no remaining references to
PUBPREFIXLEN/PRIVPREFIXLEN are broken.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 40925612-15b6-4b92-b560-4cf425323e28
📒 Files selected for processing (3)
cmake/modules/CompilerOptions.cmakesrc/bin/util/softhsm2-util-ossl.cppsrc/bin/util/softhsm2-util-ossl.h
✅ Files skipped from review due to trivial changes (1)
- cmake/modules/CompilerOptions.cmake
antoinelochet
force-pushed
the
feature/ml-dsa-softhsmutil
branch
from
090bbd1 to
79c039e
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/bin/util/softhsm2-util-ossl.cpp (2)
1350-1366: 💤 Low valueConsider adding defensive assertion for unreachable default case.
The
defaultcase in the parameter set switch (line 1364) is unreachable due to the length validation at lines 1310-1319, but silently leavingparameterSetas 0 could mask bugs if the validation logic changes. Adding an assertion or explicit error return would improve defensive coding.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/bin/util/softhsm2-util-ossl.cpp` around lines 1350 - 1366, The switch over keyMat->sizePrivValue leaves keyMat->parameterSet as 0 in the default branch which can hide future bugs; modify the default branch in this switch (the block that assigns MLDSAParameters::ML_DSA_*_PARAMETER_SET) to be defensive: either add an assertion (e.g., assert(false) or equivalent debug assertion) referencing keyMat->sizePrivValue, or return/throw an explicit error indicating an unexpected private length, so parameterSet is never silently left unset; ensure you update the containing function's control flow to propagate the error if you choose return/throw.
159-218: 💤 Low valueProvider-managed key detection relies on ML-DSA-specific parameters.
The detection for provider-managed keys (OpenSSL 3+) probes for
OSSL_PKEY_PARAM_ML_DSA_SEEDorOSSL_PKEY_PARAM_PRIV_KEY. This works for now since ML-DSA is the only provider-managed key type supported, but if future algorithms are added (e.g., ML-KEM, SLH-DSA), this detection logic would need refinement to avoid misidentifying them.Consider using
EVP_PKEY_is_a(pkey, "ML-DSA")for more robust algorithm identification when expanding provider key support.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/bin/util/softhsm2-util-ossl.cpp` around lines 159 - 218, The current provider-managed key detection in the EVP_PKEY_KEYMGMT branch relies on probing ML-DSA-specific params via EVP_PKEY_get_octet_string_param (OSSL_PKEY_PARAM_ML_DSA_SEED / OSSL_PKEY_PARAM_PRIV_KEY), which may misidentify future provider algorithms; change the detection to explicitly check the key type with EVP_PKEY_is_a(pkey, "ML-DSA") (or analogous algorithm name) before attempting ML-DSA-specific param calls, and only call EVP_PKEY_get_octet_string_param / EVP_PKEY_up_ref and assign to mldsa when EVP_PKEY_is_a returns true; update the logic in the branch guarded by keyType == EVP_PKEY_KEYMGMT to use EVP_PKEY_is_a(pkey, "ML-DSA") instead of the current param-probing heuristic so other provider algorithms won't be misdetected.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/bin/util/softhsm2-util-ossl.cpp`:
- Around line 1266-1267: Remove the redundant macro redefinitions PUBPREFIXLEN
and PRIVPREFIXLEN from the ML-DSA section: delete the two `#define` lines so the
code uses the existing definitions from the EDDSA section and avoids macro
redefinition warnings; if you prefer safety instead of deletion, wrap the
defines with `#ifndef` PUBPREFIXLEN / `#ifndef` PRIVPREFIXLEN guards to only define
them when not already defined.
---
Nitpick comments:
In `@src/bin/util/softhsm2-util-ossl.cpp`:
- Around line 1350-1366: The switch over keyMat->sizePrivValue leaves
keyMat->parameterSet as 0 in the default branch which can hide future bugs;
modify the default branch in this switch (the block that assigns
MLDSAParameters::ML_DSA_*_PARAMETER_SET) to be defensive: either add an
assertion (e.g., assert(false) or equivalent debug assertion) referencing
keyMat->sizePrivValue, or return/throw an explicit error indicating an
unexpected private length, so parameterSet is never silently left unset; ensure
you update the containing function's control flow to propagate the error if you
choose return/throw.
- Around line 159-218: The current provider-managed key detection in the
EVP_PKEY_KEYMGMT branch relies on probing ML-DSA-specific params via
EVP_PKEY_get_octet_string_param (OSSL_PKEY_PARAM_ML_DSA_SEED /
OSSL_PKEY_PARAM_PRIV_KEY), which may misidentify future provider algorithms;
change the detection to explicitly check the key type with EVP_PKEY_is_a(pkey,
"ML-DSA") (or analogous algorithm name) before attempting ML-DSA-specific param
calls, and only call EVP_PKEY_get_octet_string_param / EVP_PKEY_up_ref and
assign to mldsa when EVP_PKEY_is_a returns true; update the logic in the branch
guarded by keyType == EVP_PKEY_KEYMGMT to use EVP_PKEY_is_a(pkey, "ML-DSA")
instead of the current param-probing heuristic so other provider algorithms
won't be misdetected.
🪄 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: 5a792174-e8f1-462c-bc1e-e0622326af75
📒 Files selected for processing (2)
src/bin/util/softhsm2-util-ossl.cppsrc/bin/util/softhsm2-util-ossl.h
antoinelochet
force-pushed
the
feature/ml-dsa-softhsmutil
branch
3 times, most recently
from
8686705 to
845cda9
Compare
Contributor
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/bin/util/softhsm2-util-ossl.cpp`:
- Line 1282: The fprintf calls printing error messages to stderr are missing
trailing newlines (e.g., the call "fprintf(stderr, \"ERROR: Could not get ML-DSA
seed, rv: %d\", rv)" and the similar calls at the other sites), which can garble
console output; update each fprintf that writes errors to stderr (the ML-DSA
seed error and the two other error fprintfs around the same block) to include a
terminating "\n" in the format string so they become e.g. "fprintf(stderr,
\"ERROR: Could not get ML-DSA seed, rv: %d\\n\", rv)". Ensure all three affected
fprintf invocations are changed consistently.
🪄 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: fe4e7a23-21aa-4b18-b83a-b6de4989ee54
📒 Files selected for processing (2)
src/bin/util/softhsm2-util-ossl.cppsrc/bin/util/softhsm2-util-ossl.h
antoinelochet
force-pushed
the
feature/ml-dsa-softhsmutil
branch
from
845cda9 to
046a174
Compare
Member
|
Nice work. It would be even nicer if we could add some tests. The #864 might be something that we can build on - even if we have some shell scripts that will run, it will be a big improvement. And I suppose you have some script that you run to test it, right? |
Contributor
Author
Well... To be honest, I have tested it manually... |
antoinelochet
force-pushed
the
feature/ml-dsa-softhsmutil
branch
2 times, most recently
from
0ded500 to
33fc9a6
Compare
Contributor
Author
Added a shell basic test taking inspiration from #864 |
antoinelochet
force-pushed
the
feature/ml-dsa-softhsmutil
branch
8 times, most recently
from
c160ae6 to
b4f069e
Compare
antoinelochet
force-pushed
the
feature/ml-dsa-softhsmutil
branch
17 times, most recently
from
fd467dc to
29965b7
Compare
Contributor
Author
|
@bukka It is ready for review with the test script now. |
Contributor
Author
|
@codrerabbitai resume |
|
✅ Actions performedReviews resumed. |
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
src/lib/win32/syslog.cpp (1)
37-37: 💤 Low valueConsider the stack allocation size for
buf.The buffer
bufis stack-allocated with sizeMAX_LOG_MESSAGE_SIZE. IfMAX_LOG_MESSAGE_SIZEis large (e.g., several KB), this could risk stack overflow on threads with limited stack space. The comparable code inlog.cppusesstd::vector<char>for heap allocation, which is safer for large buffers.If
MAX_LOG_MESSAGE_SIZEis reasonably small (≤ 1-2 KB), stack allocation is fine. Otherwise, consider heap allocation or document the size constraint.🤖 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/win32/syslog.cpp` at line 37, The stack allocation `char buf[MAX_LOG_MESSAGE_SIZE];` in syslog.cpp can overflow if MAX_LOG_MESSAGE_SIZE is large; replace it with a heap-backed buffer (e.g., construct a std::vector<char> buffer(MAX_LOG_MESSAGE_SIZE) and use buffer.data() in place of `buf`) or validate/limit MAX_LOG_MESSAGE_SIZE with a runtime check/static_assert to ensure it's small (<= 1-2 KB) before keeping stack allocation; update all uses of `buf` to the new buffer variable and include a brief comment explaining the choice.src/bin/util/test/mldsa44-import-key-test.sh (1)
12-118: 💤 Low valueAddress shellcheck findings (backticks, unquoted expansions, unused vars).
Several style/robustness issues flagged by shellcheck are easy to fix in one pass:
- Legacy backticks → use
$(...): lines 12, 17, 27, 48, 50, 72, 95, 96, 118.- Unquoted glob in
for F in "$D"/*softhsm2.$S(line 60): the$Sportion should be safe but the file expansion is inside quotes; OK, but consider failing more cleanly if no match (currenttest -f "$F" || continuealready handles "no match" loop iteration when nullglob is off — the loop body will fire once with the literal pattern).- Unused
PASS_URI/KEY_URI(lines 125-126) — drop them or actually use them in the import invocation.- Unquoted
$SOFTHSM2_CONF/$TOKEN_DIRincatinvocations (lines 116, 133, 138, 146, 151, 156) — quote to be safe against paths with spaces (relevant on Windows runners withC:\Program Files\…-style temp dirs).🤖 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/bin/util/test/mldsa44-import-key-test.sh` around lines 12 - 118, Replace legacy backticks with $(...) (occurrences where variables are assigned: CWD, OPENSSL lookup, openssl_version, D, P11MODULE realpath, WINDOWS_TOKEN_DIR realpath and cygpath), quote all variable expansions passed to commands like cat, realpath, mkdir, rm, and softhsm2_tool (use "$SOFTHSM2_CONF", "$TOKEN_DIR", "$P11MODULE", "$CWD", "$WINDOWS_TOKEN_DIR"), and either remove the unused PASS_URI/KEY_URI variables or actually pass them into the import invocation; also ensure the glob loop for F in "$D"/*softhsm2.$S is left as-is but guarded (keep test -f "$F" || continue) so it behaves predictably when no match is found..github/workflows/ci.yml (1)
191-265: ⚖️ Poor tradeoffConsider consolidating the three Windows jobs.
The new
windows_ossl30andwindows_ossl35jobs duplicate most ofwindows_botan_ossl11(steps, vcpkg setup, build, and test). The differences (vcpkg overrides, arch matrix,build-options) could be expressed in a single matrix-driven job to reduce drift risk over time (e.g., if someone updates the vcpkg-action version on only one job).This is optional and non-blocking for the PR’s goal of enabling ML‑DSA.
🤖 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 @.github/workflows/ci.yml around lines 191 - 265, The CI duplicates three Windows jobs (windows_ossl30, windows_ossl35 and windows_botan_ossl11); replace them with one matrix-driven job (e.g., windows) that exposes per-matrix keys for arch, target-platform, openssl_version (used in the vcpkg.json override), and build-options, then reuse the same steps (actions/checkout, ilammy/msvc-dev-cmd, generate vcpkg.json using matrix.openssl_version, seanmiddleditch/vcpkg-action, cmake configure/build and RUN_TESTS) so differences are driven by matrix values and not duplicated across separate job blocks.
🤖 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/bin/util/test/Makefile.am`:
- Around line 9-10: The EXTRA_DIST assignment is missing a trailing
line-continuation backslash so the second file entry is not included; update the
EXTRA_DIST multi-line variable (EXTRA_DIST) to use a backslash after the
CMakeLists.txt entry so that $(srcdir)/mldsa44-import-key-test.sh is part of the
same assignment and will be packaged by make dist.
In `@src/bin/util/test/mldsa44-import-key-test.sh`:
- Around line 47-81: The Windows-specific paths in the test script (the
RUNNER_OS branch that sets D to ../../../lib/Debug/ and the softhsm2_tool
invocation using ../Debug/softhsm2-util.exe) hardcode the Debug config and will
break Release builds; update the script to detect or accept a build
configuration and/or search both Debug and Release (or use an env var like
CONFIG or CMAKE_BUILD_TYPE) when setting D and when calling softhsm2-util inside
the softhsm2_tool function, e.g., try Debug then Release (or use a provided
CONFIG) to locate the library directory and executable and set P11MODULE/command
accordingly so the script works for both Debug and Release builds.
- Around line 4-5: Remove the dangerous combination of "set -x" and the bare
"export" in the test script to avoid dumping all exported environment variables
(including secrets) to logs; either delete the standalone "export" and disable
or limit shell trace, or replace the bare export with explicit, named debug
echoes (e.g., echo "RUNNER_OS=$RUNNER_OS") for only the variables you need to
inspect so secrets are not leaked.
- Around line 7-10: The test script uses an undocumented
ACTIONS_ORCHESTRATION_ID check; update the early-skip condition to use the
documented GITHUB_JOB variable instead: replace occurrences of
ACTIONS_ORCHESTRATION_ID with GITHUB_JOB in the conditional, ensure the pattern
match still looks for "ossl35" (e.g., [[ "${GITHUB_JOB:-}" != *"ossl35"* ]]),
and keep the surrounding CI check and exit behavior intact so the later OpenSSL
version guard still handles unsupported versions.
---
Nitpick comments:
In @.github/workflows/ci.yml:
- Around line 191-265: The CI duplicates three Windows jobs (windows_ossl30,
windows_ossl35 and windows_botan_ossl11); replace them with one matrix-driven
job (e.g., windows) that exposes per-matrix keys for arch, target-platform,
openssl_version (used in the vcpkg.json override), and build-options, then reuse
the same steps (actions/checkout, ilammy/msvc-dev-cmd, generate vcpkg.json using
matrix.openssl_version, seanmiddleditch/vcpkg-action, cmake configure/build and
RUN_TESTS) so differences are driven by matrix values and not duplicated across
separate job blocks.
In `@src/bin/util/test/mldsa44-import-key-test.sh`:
- Around line 12-118: Replace legacy backticks with $(...) (occurrences where
variables are assigned: CWD, OPENSSL lookup, openssl_version, D, P11MODULE
realpath, WINDOWS_TOKEN_DIR realpath and cygpath), quote all variable expansions
passed to commands like cat, realpath, mkdir, rm, and softhsm2_tool (use
"$SOFTHSM2_CONF", "$TOKEN_DIR", "$P11MODULE", "$CWD", "$WINDOWS_TOKEN_DIR"), and
either remove the unused PASS_URI/KEY_URI variables or actually pass them into
the import invocation; also ensure the glob loop for F in "$D"/*softhsm2.$S is
left as-is but guarded (keep test -f "$F" || continue) so it behaves predictably
when no match is found.
In `@src/lib/win32/syslog.cpp`:
- Line 37: The stack allocation `char buf[MAX_LOG_MESSAGE_SIZE];` in syslog.cpp
can overflow if MAX_LOG_MESSAGE_SIZE is large; replace it with a heap-backed
buffer (e.g., construct a std::vector<char> buffer(MAX_LOG_MESSAGE_SIZE) and use
buffer.data() in place of `buf`) or validate/limit MAX_LOG_MESSAGE_SIZE with a
runtime check/static_assert to ensure it's small (<= 1-2 KB) before keeping
stack allocation; update all uses of `buf` to the new buffer variable and
include a brief comment explaining the choice.
🪄 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: a96b093f-7f9f-42a7-b226-806030aa0e12
📒 Files selected for processing (18)
.github/workflows/ci.yml.gitignorecmake/modules/CompilerOptions.cmakecmake/modules/tests/test_openssl_mldsa.cconfigure.acsrc/bin/util/CMakeLists.txtsrc/bin/util/Makefile.amsrc/bin/util/softhsm2-util-ossl.cppsrc/bin/util/softhsm2-util-ossl.hsrc/bin/util/test/CMakeLists.txtsrc/bin/util/test/Makefile.amsrc/bin/util/test/mldsa44-import-key-test.shsrc/lib/common/log.cppsrc/lib/common/log.hsrc/lib/crypto/CMakeLists.txtsrc/lib/crypto/OSSLMLDSA.cppsrc/lib/crypto/OSSLMLDSAPrivateKey.cppsrc/lib/win32/syslog.cpp
✅ Files skipped from review due to trivial changes (6)
- src/bin/util/test/CMakeLists.txt
- src/bin/util/Makefile.am
- src/lib/common/log.h
- configure.ac
- src/lib/crypto/OSSLMLDSA.cpp
- src/lib/crypto/OSSLMLDSAPrivateKey.cpp
🚧 Files skipped from review as they are similar to previous changes (2)
- src/bin/util/softhsm2-util-ossl.h
- src/bin/util/softhsm2-util-ossl.cpp
Contributor
Author
@coderabbitai Consolidate the three Windows jobs |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 9725
windows:
name: Windows (${{ matrix.arch }}, ${{ matrix.backend }}, OpenSSL ${{ matrix.ossl-version }})
runs-on: windows-2022
strategy:
fail-fast: false
matrix:
include:
# OpenSSL 1.1.1n variants (+ Botan 2.19.3)
- arch: x64
backend: openssl
target-platform: x64
ossl-version: "1.1.1n"
botan-version: ""
build-options: ""
- arch: x64
backend: botan
target-platform: x64
ossl-version: "1.1.1n"
botan-version: "2.19.3"
build-options: "-DENABLE_ECC=OFF -DENABLE_EDDSA=OFF"
- arch: x86
backend: openssl
target-platform: Win32
ossl-version: "1.1.1n"
botan-version: ""
build-options: "-DENABLE_ECC=OFF -DENABLE_EDDSA=OFF"
# OpenSSL 3.4.1
- arch: x64
backend: openssl
target-platform: x64
ossl-version: "3.4.1"
botan-version: ""
build-options: ""
- arch: x86
backend: openssl
target-platform: Win32
ossl-version: "3.4.1"
botan-version: ""
build-options: "-DENABLE_ECC=OFF -DENABLE_EDDSA=OFF"
# OpenSSL 3.5.4 + ML-DSA (x64 only — 32-bit runner limitation)
- arch: x64
backend: openssl
target-platform: x64
ossl-version: "3.5.4"
botan-version: ""
build-options: "-DENABLE_MLDSA=ON"
steps:
- uses: actions/checkout@v4
- uses: ilammy/msvc-dev-cmd@v1
with:
arch: ${{ matrix.arch }}
- name: Create vcpkg.json
shell: pwsh
run: |
$deps = @("openssl", "cppunit")
$overrides = @(@{ name = "openssl"; "version-string" = "${{ matrix.ossl-version }}" })
if ("${{ matrix.backend }}" -eq "botan") {
$deps += "botan"
$overrides += @{ name = "botan"; "version-string" = "${{ matrix.botan-version }}" }
}
[ordered]@{
dependencies = $deps
overrides = $overrides
"builtin-baseline" = "38d1652f152d36481f2f4e8a85c0f1e14f3769f7"
} | ConvertTo-Json -Depth 5 | Out-File vcpkg.json -Encoding utf8
- uses: seanmiddleditch/vcpkg-action@master
id: vcpkg
with:
manifest-dir: ${{ github.workspace }}
triplet: ${{ matrix.arch }}-windows
token: ${{ github.token }}
- name: Build
run: |
mkdir build
cmake -B build ${{ steps.vcpkg.outputs.vcpkg-cmake-config }} -A ${{ matrix.target-platform }} -DWITH_CRYPTO_BACKEND=${{ matrix.backend }} ${{ matrix.build-options }} -DDISABLE_NON_PAGED_MEMORY=ON -DBUILD_TESTS=ON
cmake --build build
- name: Test
env:
CTEST_OUTPUT_ON_FAILURE: 1
run: |
cmake --build build --target RUN_TESTSWhat changed vs. the three separate jobs:
The only behavioural difference is the job display name now includes the OpenSSL version (e.g. |
antoinelochet
force-pushed
the
feature/ml-dsa-softhsmutil
branch
2 times, most recently
from
d60a1e4 to
f0323ea
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/bin/util/test/mldsa44-import-key-test.sh (1)
4-7:⚠️ Potential issue | 🟠 Major | ⚡ Quick winEarly CI gate is skipping the ML-DSA test in intended jobs
Line 4 ties execution to
GITHUB_JOBcontainingossl35, but current job ids don’t reliably match that token (e.g.,windows,linux_ossl_35). This causes the test to be skipped even when OpenSSL is 3.5+. The version check later in the script already handles eligibility, so this gate should be removed (or replaced with an explicit CI flag).Suggested minimal fix
-if [[ "$CI" == "true" ]] && [[ "$GITHUB_JOB" != *"ossl35"* ]] ; then - echo "This test is intended to be run with OpenSSL >= 3.5, skipping." >&2 - exit 0 -fi +# Let the OpenSSL version check below decide whether to skip.🤖 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/bin/util/test/mldsa44-import-key-test.sh` around lines 4 - 7, Remove the early CI gate that checks CI and GITHUB_JOB (the if block testing [[ "$CI" == "true" ]] && [[ "$GITHUB_JOB" != *"ossl35"* ]] ... fi) from the mldsa44-import-key-test.sh script so the test isn't prematurely skipped; rely on the existing OpenSSL version check later in the script (or, if desired, replace the gate with an explicit CI-only flag check) so jobs with non-matching GITHUB_JOB names like "linux_ossl_35" no longer get incorrectly excluded.
🤖 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.
Duplicate comments:
In `@src/bin/util/test/mldsa44-import-key-test.sh`:
- Around line 4-7: Remove the early CI gate that checks CI and GITHUB_JOB (the
if block testing [[ "$CI" == "true" ]] && [[ "$GITHUB_JOB" != *"ossl35"* ]] ...
fi) from the mldsa44-import-key-test.sh script so the test isn't prematurely
skipped; rely on the existing OpenSSL version check later in the script (or, if
desired, replace the gate with an explicit CI-only flag check) so jobs with
non-matching GITHUB_JOB names like "linux_ossl_35" no longer get incorrectly
excluded.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4a1a8c2d-4c11-48ec-8a3a-d61fbf6ae81c
📒 Files selected for processing (18)
.github/workflows/ci.yml.gitignorecmake/modules/CompilerOptions.cmakecmake/modules/tests/test_openssl_mldsa.cconfigure.acsrc/bin/util/CMakeLists.txtsrc/bin/util/Makefile.amsrc/bin/util/softhsm2-util-ossl.cppsrc/bin/util/softhsm2-util-ossl.hsrc/bin/util/test/CMakeLists.txtsrc/bin/util/test/Makefile.amsrc/bin/util/test/mldsa44-import-key-test.shsrc/lib/common/log.cppsrc/lib/common/log.hsrc/lib/crypto/CMakeLists.txtsrc/lib/crypto/OSSLMLDSA.cppsrc/lib/crypto/OSSLMLDSAPrivateKey.cppsrc/lib/win32/syslog.cpp
✅ Files skipped from review due to trivial changes (6)
- src/bin/util/test/Makefile.am
- .gitignore
- src/lib/common/log.h
- src/lib/crypto/OSSLMLDSA.cpp
- src/lib/common/log.cpp
- src/lib/crypto/CMakeLists.txt
🚧 Files skipped from review as they are similar to previous changes (9)
- src/bin/util/Makefile.am
- configure.ac
- cmake/modules/CompilerOptions.cmake
- src/bin/util/CMakeLists.txt
- src/bin/util/test/CMakeLists.txt
- src/bin/util/softhsm2-util-ossl.h
- src/lib/win32/syslog.cpp
- src/lib/crypto/OSSLMLDSAPrivateKey.cpp
- src/bin/util/softhsm2-util-ossl.cpp
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
src/bin/util/test/mldsa44-import-key-test.sh (3)
127-127: ⚡ Quick winQuote variable expansions to prevent word splitting.
Multiple
catcommands use unquoted variable expansions (e.g.,cat $SOFTHSM2_CONF,cat $TOKEN_DIR/token.log). While unlikely to cause issues in practice since the paths are constructed within the script, quoting is best practice and prevents potential issues if paths contain spaces.♻️ Proposed fix
-cat $SOFTHSM2_CONF +cat "$SOFTHSM2_CONF"- cat $TOKEN_DIR/token.log + cat "$TOKEN_DIR"/token.logApply the same pattern to lines 149, 157, 162, and 167.
Also applies to: 144-144, 149-149, 157-157, 162-162, 167-167
🤖 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/bin/util/test/mldsa44-import-key-test.sh` at line 127, Replace unquoted variable expansions in the cat commands with quoted expansions to prevent word-splitting: e.g., change cat $SOFTHSM2_CONF to cat "$SOFTHSM2_CONF" and cat $TOKEN_DIR/token.log to cat "$TOKEN_DIR/token.log"; apply the same quoting pattern for the other occurrences mentioned (the other cat invocations using SOFTHSM2_CONF and TOKEN_DIR) so all variable usages are wrapped in double quotes.
9-9: 💤 Low valueConsider modernizing command substitution syntax.
The script uses legacy backtick syntax
`...`in multiple places (lines 9, 14, 24, 106, 107, 129). The modern$(...)syntax is preferred as it's more readable, nests better, and is recommended by ShellCheck.♻️ Example conversions
-CWD=`pwd` +CWD=$(pwd)-OPENSSL=`command -v "$OPENSSL"` +OPENSSL=$(command -v "$OPENSSL")-openssl_version=`openssl version` || exit $? +openssl_version=$(openssl version) || exit $?Apply similar changes to lines 106, 107, and 129.
Also applies to: 14-14, 24-24, 106-107, 129-129
🤖 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/bin/util/test/mldsa44-import-key-test.sh` at line 9, Replace legacy backtick command substitutions with modern $(...) form: change occurrences like the CWD assignment (CWD=`pwd`) and other backtick usages around lines where variables are set or commands are captured (the instances at lines 14, 24, 106, 107, 129) to use $(pwd) and equivalent $(...) forms; update each variable assignment and any nested substitutions in the script so they use the $(...) syntax to improve readability and nesting safety while preserving the exact command semantics.
136-137: ⚡ Quick winRemove unused variables.
PASS_URIandKEY_URIare defined but never used in the test. Remove dead code to improve clarity.♻️ Proposed fix
-PASS_URI=pass:$TOKEN_PIN -KEY_URI=pkcs11:id=%$TOKEN_ID KEY_FILE="$TOKEN_DIR"/openssl_test_key🤖 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/bin/util/test/mldsa44-import-key-test.sh` around lines 136 - 137, The script defines unused variables PASS_URI and KEY_URI which should be removed; delete the lines that set PASS_URI=pass:$TOKEN_PIN and KEY_URI=pkcs11:id=%$TOKEN_ID in the test script (and run a quick search for PASS_URI / KEY_URI to ensure no remaining references), leaving TOKEN_PIN and TOKEN_ID intact only if they're used elsewhere. Ensure the script still passes shellcheck and test runs after removing these dead-variable definitions.
🤖 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 `@cmake/modules/tests/test_openssl_mldsa.c`:
- Around line 5-10: The EVP_PKEY_CTX returned by EVP_PKEY_CTX_new_from_name is
not freed; call EVP_PKEY_CTX_free(ctx) after successful allocation (before
returning 0) so the allocated EVP_PKEY_CTX is released; keep the existing NULL
check (return 1 if ctx == NULL) and only free when ctx is non-NULL (i.e., free
the ctx returned by EVP_PKEY_CTX_new_from_name before the successful return).
---
Nitpick comments:
In `@src/bin/util/test/mldsa44-import-key-test.sh`:
- Line 127: Replace unquoted variable expansions in the cat commands with quoted
expansions to prevent word-splitting: e.g., change cat $SOFTHSM2_CONF to cat
"$SOFTHSM2_CONF" and cat $TOKEN_DIR/token.log to cat "$TOKEN_DIR/token.log";
apply the same quoting pattern for the other occurrences mentioned (the other
cat invocations using SOFTHSM2_CONF and TOKEN_DIR) so all variable usages are
wrapped in double quotes.
- Line 9: Replace legacy backtick command substitutions with modern $(...) form:
change occurrences like the CWD assignment (CWD=`pwd`) and other backtick usages
around lines where variables are set or commands are captured (the instances at
lines 14, 24, 106, 107, 129) to use $(pwd) and equivalent $(...) forms; update
each variable assignment and any nested substitutions in the script so they use
the $(...) syntax to improve readability and nesting safety while preserving the
exact command semantics.
- Around line 136-137: The script defines unused variables PASS_URI and KEY_URI
which should be removed; delete the lines that set PASS_URI=pass:$TOKEN_PIN and
KEY_URI=pkcs11:id=%$TOKEN_ID in the test script (and run a quick search for
PASS_URI / KEY_URI to ensure no remaining references), leaving TOKEN_PIN and
TOKEN_ID intact only if they're used elsewhere. Ensure the script still passes
shellcheck and test runs after removing these dead-variable definitions.
🪄 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: c822863a-09e3-4799-a6b7-487a38d4fe59
📒 Files selected for processing (18)
.github/workflows/ci.yml.gitignorecmake/modules/CompilerOptions.cmakecmake/modules/tests/test_openssl_mldsa.cconfigure.acsrc/bin/util/CMakeLists.txtsrc/bin/util/Makefile.amsrc/bin/util/softhsm2-util-ossl.cppsrc/bin/util/softhsm2-util-ossl.hsrc/bin/util/test/CMakeLists.txtsrc/bin/util/test/Makefile.amsrc/bin/util/test/mldsa44-import-key-test.shsrc/lib/common/log.cppsrc/lib/common/log.hsrc/lib/crypto/CMakeLists.txtsrc/lib/crypto/OSSLMLDSA.cppsrc/lib/crypto/OSSLMLDSAPrivateKey.cppsrc/lib/win32/syslog.cpp
✅ Files skipped from review due to trivial changes (5)
- .gitignore
- src/lib/common/log.h
- src/bin/util/test/Makefile.am
- src/lib/crypto/CMakeLists.txt
- src/lib/common/log.cpp
🚧 Files skipped from review as they are similar to previous changes (11)
- src/bin/util/Makefile.am
- configure.ac
- src/bin/util/CMakeLists.txt
- src/lib/win32/syslog.cpp
- src/lib/crypto/OSSLMLDSA.cpp
- src/bin/util/test/CMakeLists.txt
- cmake/modules/CompilerOptions.cmake
- src/bin/util/softhsm2-util-ossl.h
- src/lib/crypto/OSSLMLDSAPrivateKey.cpp
- .github/workflows/ci.yml
- src/bin/util/softhsm2-util-ossl.cpp
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
cmake/modules/tests/test_openssl_mldsa.c (1)
5-10:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFree
EVP_PKEY_CTXon the success path.Line 6 allocates
ctx, but Line 10 returns without releasing it. This is still a leak (even in a smoke test).🛠️ Proposed fix
if (ctx == NULL) return 1; + EVP_PKEY_CTX_free(ctx); return 0;#!/bin/bash rg -n 'EVP_PKEY_CTX_new_from_name|EVP_PKEY_CTX_free' cmake/modules/tests/test_openssl_mldsa.c🤖 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 `@cmake/modules/tests/test_openssl_mldsa.c` around lines 5 - 10, The test allocates an EVP_PKEY_CTX via EVP_PKEY_CTX_new_from_name into the variable ctx but returns 0 on success without freeing it; modify the success path to call EVP_PKEY_CTX_free(ctx) before returning (i.e., after confirming ctx != NULL and before the final return 0) so the EVP_PKEY_CTX is released; ensure you only call EVP_PKEY_CTX_free when ctx is non-NULL to avoid NULL-free issues.
🤖 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/bin/util/test/mldsa44-import-key-test.sh`:
- Line 127: Unquoted variable expansions in the script cause word-splitting for
paths with spaces; update every cat invocation that uses the SOFTHSM2_CONF and
TOKEN_DIR variables so they are quoted (e.g., change usages of cat
$SOFTHSM2_CONF to cat "$SOFTHSM2_CONF" and cat $TOKEN_DIR/token.log to cat
"$TOKEN_DIR/token.log"). Locate the occurrences referencing the SOFTHSM2_CONF
variable and the TOKEN_DIR/token.log string in the test script and wrap the
variable expansions in double quotes to prevent splitting and globbing.
---
Duplicate comments:
In `@cmake/modules/tests/test_openssl_mldsa.c`:
- Around line 5-10: The test allocates an EVP_PKEY_CTX via
EVP_PKEY_CTX_new_from_name into the variable ctx but returns 0 on success
without freeing it; modify the success path to call EVP_PKEY_CTX_free(ctx)
before returning (i.e., after confirming ctx != NULL and before the final return
0) so the EVP_PKEY_CTX is released; ensure you only call EVP_PKEY_CTX_free when
ctx is non-NULL to avoid NULL-free issues.
🪄 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: dfe6c029-65c1-40d8-ac55-e58f52815d7a
📒 Files selected for processing (18)
.github/workflows/ci.yml.gitignorecmake/modules/CompilerOptions.cmakecmake/modules/tests/test_openssl_mldsa.cconfigure.acsrc/bin/util/CMakeLists.txtsrc/bin/util/Makefile.amsrc/bin/util/softhsm2-util-ossl.cppsrc/bin/util/softhsm2-util-ossl.hsrc/bin/util/test/CMakeLists.txtsrc/bin/util/test/Makefile.amsrc/bin/util/test/mldsa44-import-key-test.shsrc/lib/common/log.cppsrc/lib/common/log.hsrc/lib/crypto/CMakeLists.txtsrc/lib/crypto/OSSLMLDSA.cppsrc/lib/crypto/OSSLMLDSAPrivateKey.cppsrc/lib/win32/syslog.cpp
✅ Files skipped from review due to trivial changes (8)
- src/bin/util/CMakeLists.txt
- .gitignore
- src/lib/crypto/OSSLMLDSA.cpp
- src/bin/util/test/CMakeLists.txt
- src/bin/util/test/Makefile.am
- configure.ac
- src/bin/util/Makefile.am
- src/lib/crypto/CMakeLists.txt
🚧 Files skipped from review as they are similar to previous changes (8)
- src/lib/common/log.h
- cmake/modules/CompilerOptions.cmake
- src/lib/crypto/OSSLMLDSAPrivateKey.cpp
- src/lib/win32/syslog.cpp
- src/bin/util/softhsm2-util-ossl.h
- src/lib/common/log.cpp
- .github/workflows/ci.yml
- src/bin/util/softhsm2-util-ossl.cpp
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/bin/util/test/mldsa44-import-key-test.sh (1)
106-107: ⚡ Quick winModernize legacy backticks to
$(...)syntax.Lines 106, 107, and 127 still use backticks for command substitution. Modern shell style prefers
$(...)for better readability and nesting support. ShellCheck SC2006 flags these as style issues.♻️ Proposed fix
- WINDOWS_TOKEN_DIR=`realpath "$TOKEN_DIR"` - WINDOWS_TOKEN_DIR=`cygpath -w "$WINDOWS_TOKEN_DIR"` + WINDOWS_TOKEN_DIR=$(realpath "$TOKEN_DIR") + WINDOWS_TOKEN_DIR=$(cygpath -w "$WINDOWS_TOKEN_DIR")-SOFTHSM2_CONF=`realpath "$SOFTHSM2_CONF"` +SOFTHSM2_CONF=$(realpath "$SOFTHSM2_CONF")Also applies to: 127-127
🤖 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/bin/util/test/mldsa44-import-key-test.sh` around lines 106 - 107, Replace legacy backtick command substitutions with modern $(...) form for the assignments to WINDOWS_TOKEN_DIR that use TOKEN_DIR (i.e., change WINDOWS_TOKEN_DIR=`realpath "$TOKEN_DIR"` and WINDOWS_TOKEN_DIR=`cygpath -w "$WINDOWS_TOKEN_DIR"` to use $(...) syntax) and also update the other backtick substitution noted later in the script; preserve existing quoting around variables and command arguments when converting so behavior remains identical.
🤖 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/bin/util/test/mldsa44-import-key-test.sh`:
- Line 153: The command uses an unquoted variable expansion: change the cat
invocation that references TOKEN_DIR so the path is quoted (i.e., use "
$TOKEN_DIR/token.log " with proper quoting of the whole expansion) to prevent
word-splitting when TOKEN_DIR contains spaces; update the occurrence that
matches the pattern used on the other lines (the cat command referencing
TOKEN_DIR/token.log) to be consistent with the quoted uses on lines 140, 148,
and 158.
---
Nitpick comments:
In `@src/bin/util/test/mldsa44-import-key-test.sh`:
- Around line 106-107: Replace legacy backtick command substitutions with modern
$(...) form for the assignments to WINDOWS_TOKEN_DIR that use TOKEN_DIR (i.e.,
change WINDOWS_TOKEN_DIR=`realpath "$TOKEN_DIR"` and WINDOWS_TOKEN_DIR=`cygpath
-w "$WINDOWS_TOKEN_DIR"` to use $(...) syntax) and also update the other
backtick substitution noted later in the script; preserve existing quoting
around variables and command arguments when converting so behavior remains
identical.
🪄 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: 85d1cd55-6791-49fb-9949-5f35040895cc
📒 Files selected for processing (18)
.github/workflows/ci.yml.gitignorecmake/modules/CompilerOptions.cmakecmake/modules/tests/test_openssl_mldsa.cconfigure.acsrc/bin/util/CMakeLists.txtsrc/bin/util/Makefile.amsrc/bin/util/softhsm2-util-ossl.cppsrc/bin/util/softhsm2-util-ossl.hsrc/bin/util/test/CMakeLists.txtsrc/bin/util/test/Makefile.amsrc/bin/util/test/mldsa44-import-key-test.shsrc/lib/common/log.cppsrc/lib/common/log.hsrc/lib/crypto/CMakeLists.txtsrc/lib/crypto/OSSLMLDSA.cppsrc/lib/crypto/OSSLMLDSAPrivateKey.cppsrc/lib/win32/syslog.cpp
✅ Files skipped from review due to trivial changes (9)
- src/bin/util/Makefile.am
- src/bin/util/CMakeLists.txt
- src/lib/crypto/OSSLMLDSA.cpp
- src/lib/common/log.h
- .gitignore
- src/bin/util/test/CMakeLists.txt
- src/bin/util/test/Makefile.am
- src/lib/common/log.cpp
- configure.ac
🚧 Files skipped from review as they are similar to previous changes (7)
- src/lib/crypto/OSSLMLDSAPrivateKey.cpp
- src/lib/win32/syslog.cpp
- cmake/modules/CompilerOptions.cmake
- src/bin/util/softhsm2-util-ossl.h
- src/lib/crypto/CMakeLists.txt
- .github/workflows/ci.yml
- src/bin/util/softhsm2-util-ossl.cpp
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Added missing ML-DSA cmake compiler option
Fixes #853
Summary by CodeRabbit
New Features
Bug Fixes / UX
Tests
CI / Build
Chores