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.

antoinelochet · 2026-04-03T11:42:05Z

I have checked with the PKCS#11 3.2 spec (https://docs.oasis-open.org/pkcs11/pkcs11-spec/v3.2/csd01/pkcs11-spec-v3.2-csd01.html#_Toc195693264) and the OpenSC/pkcs11-tool 0.27.1 implementation.
If my interpretation is correct, here are my findings.

While the spec says that ML-DSA private key should not set the CKA_PARAMETER_SET attribute on the C_GenerateKeyPair operation (Chapter 6.67.3 with Table 13, note 4), pkcs11-tool does it: https://github.com/OpenSC/OpenSC/blob/19868984dc4dc697af6a86d65ab32a1f19a43ea4/src/tools/pkcs11-tool.c#L3730
The SoftHSM implementation of CKA_PARAMETER_SET is respectful of the PKCS#11 3.2 specs and forbids setting CKA_PARAMETER_SET on the private key template through the use of P11Attribute::ck4.
So it seems an issue on pkcs11-tool side. Maybe it has been done because other PKCS#11 implementations do not care. I don't know.
I see several options in my order of preference:

We can stay as-is because we want to stick as close as possible to the spec.
We can check the presence of the attribute on the private template on the C_GenerateKeyPair call and remove it (while logging this problem in DEBUG level) so that we do not change the current implementation.
We can remove the P11Attribute::ck4 constraint and add a coherence check between the public and private templates.

@bgkim-ictk @bukka what do you think ?

bukka · 2026-04-03T11:50:22Z

It would be great to know what was the reasoning for it in pkcs11-tool. Maybe @Jakuje knows it.

Jakuje · 2026-04-03T15:06:32Z

I think the SHOULD is a recommendation. Hard rejecting the key generation when private key template contains the parameter set does not sound like a good idea (as long as it is matching the parameter set on public key).

I do not think this was an intention, but when I was writing that, there was not many pkcs11 tokens with this in the wild. I think it was more likely an overlook which was not noticed by anyone before. Please open a PR/issue on OpenSC to fix that.

antoinelochet · 2026-04-03T15:20:30Z

On the Table 13, the notes are 'MUST': "4 MUST not be specified when object is generated with C_GenerateKey or C_GenerateKeyPair."

Jakuje · 2026-04-03T15:31:26Z

On the Table 13, the notes are 'MUST': "4 MUST not be specified when object is generated with C_GenerateKey or C_GenerateKeyPair."

Ok. My bad then. Its then clearly a bug in pkcs11-tool to be fixed. Though accepting it should not hurt on the sofhsm side, unless you aim to be bug-bug-compatible with pkcs11 specs

antoinelochet · 2026-04-03T16:17:33Z

I agree. I did not test setting CKA_PARAMETER_SET on privat ekey template any other tokens that I have on hand.
I may do that next week.
In the meantime, what do you think of a solution where first we check that CKA_PARAMETER_SET value is the same on the public and private template. Second, remove it from the private template and trace this removal through a log message.

Jakuje · 2026-04-03T16:24:36Z

I agree. I did not test setting CKA_PARAMETER_SET on privat ekey template any other tokens that I have on hand.

I was testing this mostly against kryoptic, which was the only token I was aware having these mechanism at that time. And we are clearly not that strict in this regard.

In the meantime, what do you think of a solution where first we check that CKA_PARAMETER_SET value is the same on the public and private template. Second, remove it from the private template and trace this removal through a log message.

I am not familiar with the softhsm internals of handling of the attributes, but I think this should work.

antoinelochet · 2026-04-07T09:29:31Z

I did some testing on Thales Luna7 and Bull Proteccio. Luna7 does not care the CKA_PARAMETER_SET is set on private key template, Proteccio does.
I suggest that we stick to my latest proposed solution.
@bgkim-ictk @bukka is that ok for you ?

…key generation The attribute is defined in the list with the footnote 4 (from Table 13) with the description: > MUST not be specified when object is generated with C_GenerateKey This matches what EC_PARAMS have and how they are (correctly) handled. softhsm/SoftHSMv2#850 Signed-off-by: Jakub Jelen <jjelen@redhat.com>

Jakuje · 2026-04-07T16:14:25Z

For the record, I opened OpenSC/OpenSC#3640 to fix this. But it will get some time before it will be in a release so I think implementing a workaround in softhsm would still make sense.

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

Jakuje mentioned this pull request Apr 7, 2026

Do not set PARAMETER_SET in pkcs11-tool key generation OpenSC/OpenSC#3640

Open

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!

antoinelochet commented Apr 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bukka commented Apr 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Jakuje commented Apr 3, 2026

Uh oh!

antoinelochet commented Apr 3, 2026

Uh oh!

Jakuje commented Apr 3, 2026

Uh oh!

antoinelochet commented Apr 3, 2026

Uh oh!

Jakuje commented Apr 3, 2026

Uh oh!

antoinelochet commented Apr 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Jakuje commented Apr 7, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 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

antoinelochet commented Apr 3, 2026 •

edited

Loading

bukka commented Apr 3, 2026 •

edited

Loading

antoinelochet commented Apr 7, 2026 •

edited

Loading