Modify CKR_ATTRIBUTE_READ_ONLY Error when generating ML-DSA key pair using pkcs11-tool by bgkim-ictk · Pull Request #850 · softhsm/SoftHSMv2

bgkim-ictk · 2026-03-19T05:43:35Z

Modify CKR_ATTRIBUTE_READ_ONLY Error when generating ML-DSA key pair using following pkcs11-tool command

pkcs11-tool --module /usr/local/lib/softhsm/libsofthsm2.so --slot 1944964809 --login --pin 1234 --keypairgen --key-type ML-DSA-44 --id 0 --label "ML-DSA-44"

please review

Summary by CodeRabbit

New Features
- ML-DSA support is now configurable across supported cryptographic backends.
Bug Fixes
- Refined key generation to properly handle parameter set attributes during object creation operations.

…using pkcs11-tool * pkcs11-tool --module /usr/local/lib/softhsm/libsofthsm2.so --slot 1944964809 --login --pin 1234 --keypairgen --key-type ML-DSA-44 --id 0 --label "ML-DSA-44"

coderabbitai · 2026-03-19T05:43:53Z

📝 Walkthrough

Walkthrough

These changes extend build system include paths, add ML-DSA feature flag propagation for both crypto backends, and refine P11 attribute validation to exempt CKA_PARAMETER_SET from prohibited restrictions during key generation.

Changes

Cohort / File(s)	Summary
Build Include Paths `CMakeLists.txt`	Added two include search paths (`${CMAKE_SOURCE_DIR}/src/lib/slot_mgr` and `${CMAKE_SOURCE_DIR}/src/lib/object_store`) to global compiler include directories.
Crypto Backend Configuration `cmake/modules/CompilerOptions.cmake`	Added conditional logic to set `WITH_ML_DSA=1` when `ENABLE_MLDSA` is enabled, applied consistently to both Botan and OpenSSL backend paths.
P11 Attribute Validation `src/lib/P11Attributes.cpp`	Refined prohibited attribute enforcement during `OBJECT_OP_GENERATE` to exempt `CKA_PARAMETER_SET` type; added trailing newline to file.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

Poem

🐰 Bouncing through configs with glee,
Include paths growing like carrots in spring,
ML-DSA flags propagate free,
P11 attributes now more lenient they be—
A nimble refactor, quick and clean! 🌱✨

🚥 Pre-merge checks | ✅ 3

✅ Passed checks (3 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately describes the main change: modifying the CKR_ATTRIBUTE_READ_ONLY error behavior when generating ML-DSA key pairs via pkcs11-tool, which aligns with the PR objectives and the code changes (particularly the P11Attributes.cpp modification exempting CKA_PARAMETER_SET).
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

Create PR with unit tests

📝 Coding Plan

Generate coding plan for human review comments

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.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

Tip

CodeRabbit can approve the review once all CodeRabbit's comments are resolved.

Enable the reviews.request_changes_workflow setting to automatically approve the review once all CodeRabbit's comments are resolved.

coderabbitai

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 `@cmake/modules/CompilerOptions.cmake`:
- Around line 222-224: When ENABLE_MLDSA is set, don't unconditionally set
WITH_ML_DSA; instead add backend capability probes like the ECC/EDDSA/GOST
sections: create test_openssl_mldsa.c and test_botan_mldsa.cpp that attempt to
compile/link a minimal ML-DSA usage, and replace the unconditional
set(WITH_ML_DSA 1) with try_run checks that set WITH_ML_DSA only on success and
call FATAL_ERROR if the probe fails when the backend is selected; use the same
try_run/ALIAS pattern and variable names as the existing ECC/EDDSA/GOST probes
so ENABLE_MLDSA triggers a proper compile/link test for both OpenSSL and Botan.

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: a8d40ef9-fcfc-4ec6-a4e4-57da973bb753

📥 Commits

Reviewing files that changed from the base of the PR and between 62c20ed and 308e8ee.

📒 Files selected for processing (3)

CMakeLists.txt
cmake/modules/CompilerOptions.cmake
src/lib/P11Attributes.cpp

coderabbitai · 2026-03-19T05:49:00Z

cmake/modules/CompilerOptions.cmake

+    if(ENABLE_MLDSA)
+        set(WITH_ML_DSA 1)
+    endif(ENABLE_MLDSA)


⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash set -euo pipefail echo "== ML-DSA capability tests in cmake/modules/tests ==" fd -HI 'test_.*mldsa\.(c|cpp)$' cmake/modules/tests || true echo echo "== WITH_ML_DSA / ENABLE_MLDSA references ==" rg -n --type=cmake 'WITH_ML_DSA|ENABLE_MLDSA|mldsa' cmake/modules/CompilerOptions.cmake cmake/modules/tests config.h.in.cmake

Repository: softhsm/SoftHSMv2

Length of output: 603

🏁 Script executed:

# Show context around MLDSA blocks and identify backend probes for other algorithms head -n 430 cmake/modules/CompilerOptions.cmake | tail -n 230

Repository: softhsm/SoftHSMv2

Length of output: 8909

🏁 Script executed:

# Search for ECC/EDDSA probes and check if there are example probe patterns rg -n -A 5 'ECC|EDDSA|try_compile|try_run' cmake/modules/CompilerOptions.cmake | head -n 100

Repository: softhsm/SoftHSMv2

Length of output: 4197

🏁 Script executed:

# List all files in cmake/modules/tests to see what probes exist ls -la cmake/modules/tests/

Repository: softhsm/SoftHSMv2

Length of output: 1088

🏁 Script executed:

# Verify the OpenSSL MLDSA section around line 377 sed -n '370,385p' cmake/modules/CompilerOptions.cmake

Repository: softhsm/SoftHSMv2

Length of output: 595

Add ML-DSA backend capability probes for both OpenSSL and Botan, following the pattern used for ECC, EDDSA, and GOST.

Lines 222–224 (Botan) and 377–379 (OpenSSL) set WITH_ML_DSA unconditionally when ENABLE_MLDSA is ON. If the selected backend lacks ML-DSA support, configuration succeeds but compilation or linking fails later. Create test files (test_openssl_mldsa.c and test_botan_mldsa.cpp) and use try_run probes with FATAL_ERROR on failure, matching the existing pattern for ECC, EDDSA, and GOST.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed. In `@cmake/modules/CompilerOptions.cmake` around lines 222 - 224, When ENABLE_MLDSA is set, don't unconditionally set WITH_ML_DSA; instead add backend capability probes like the ECC/EDDSA/GOST sections: create test_openssl_mldsa.c and test_botan_mldsa.cpp that attempt to compile/link a minimal ML-DSA usage, and replace the unconditional set(WITH_ML_DSA 1) with try_run checks that set WITH_ML_DSA only on success and call FATAL_ERROR if the probe fails when the backend is selected; use the same try_run/ALIAS pattern and variable names as the existing ECC/EDDSA/GOST probes so ENABLE_MLDSA triggers a proper compile/link test for both OpenSSL and Botan.

bukka

CC @antoinelochet for review

bukka · 2026-03-20T20:44:11Z

cmake/modules/CompilerOptions.cmake

+    if(ENABLE_MLDSA)
+        set(WITH_ML_DSA 1)
+    endif(ENABLE_MLDSA)


MLDSA currently does not work with Botan so this should be removed

We would need Botan3 for that which is not currently supported. It will fail with Botan2.

bukka · 2026-03-20T21:03:20Z

src/lib/P11Attributes.cpp

 		{
-			ERROR_MSG("Prohibited attribute was passed to key generation function");
-			return CKR_ATTRIBUTE_READ_ONLY;
+            if (type != CKA_PARAMETER_SET) 


This seems a bit hacky. I might need to debug it to see what the actual reason for this.

antoinelochet · 2026-03-20T22:19:52Z

CC @antoinelochet for review

I did not try to generate ML-DSA keys with anything but calling the library directly.
I do not know what pkcs11-tool does in this case.
Like you, I will have to checkout the branch and see what is happening.

Modify CKR_ATTRIBUTE_READ_ONLY Error when generating ML-DSA key pair …

308e8ee

…using pkcs11-tool * pkcs11-tool --module /usr/local/lib/softhsm/libsofthsm2.so --slot 1944964809 --login --pin 1234 --keypairgen --key-type ML-DSA-44 --id 0 --label "ML-DSA-44"

bgkim-ictk requested a review from a team as a code owner March 19, 2026 05:43

coderabbitai bot reviewed Mar 19, 2026

View reviewed changes

bukka reviewed Mar 20, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Modify CKR_ATTRIBUTE_READ_ONLY Error when generating ML-DSA key pair using pkcs11-tool#850

Modify CKR_ATTRIBUTE_READ_ONLY Error when generating ML-DSA key pair using pkcs11-tool#850
bgkim-ictk wants to merge 1 commit intosofthsm:mainfrom
bgkim-ictk:mldsa_modify

bgkim-ictk commented Mar 19, 2026 •

edited by coderabbitai bot

Loading

Uh oh!

coderabbitai bot commented Mar 19, 2026 •

edited

Loading

Walkthrough

Changes

Estimated code review effort

Poem

Uh oh!

coderabbitai bot left a comment

Uh oh!

coderabbitai bot Mar 19, 2026

Uh oh!

bukka left a comment •

edited

Loading

Uh oh!

bukka Mar 20, 2026

Uh oh!

antoinelochet Mar 20, 2026

Uh oh!

bukka Mar 20, 2026

Uh oh!

antoinelochet commented Mar 20, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

bgkim-ictk commented Mar 19, 2026 • edited by coderabbitai bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary by CodeRabbit

Uh oh!

coderabbitai bot commented Mar 19, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Estimated code review effort

Poem

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

coderabbitai bot Mar 19, 2026

Choose a reason for hiding this comment

Uh oh!

bukka left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bukka Mar 20, 2026

Choose a reason for hiding this comment

Uh oh!

antoinelochet Mar 20, 2026

Choose a reason for hiding this comment

Uh oh!

bukka Mar 20, 2026

Choose a reason for hiding this comment

Uh oh!

antoinelochet commented Mar 20, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

bgkim-ictk commented Mar 19, 2026 •

edited by coderabbitai bot

Loading

coderabbitai bot commented Mar 19, 2026 •

edited

Loading

bukka left a comment •

edited

Loading