Add pkcs11-module-default-slot-id cfg for default slot ID by bukka · Pull Request #536 · openssl-projects/pkcs11-provider

bukka · 2025-03-10T14:48:38Z

Description

There are use cases when one wants to select the default slot id (especially when the first slot ID is not convenient for this purpose) which is used for example for storing public keys.

Checklist

Code modified for feature
Test suite updated with functionality tests
Test suite updated with negative tests
Documentation updated

Reviewer's checklist:

Any issues marked for closing are addressed
There is a test suite reasonably covering new functionality or modifications
This feature/change has adequate documentation added
Code conform to coding style that today cannot yet be enforced via the check style test
Commits have short titles and sensible commit messages
Coverity Scan has run if needed (code PR) and no new defects were found

simo5 · 2025-03-10T17:22:03Z

Looks reasonable but also requires changes to provider-pkcs11.7.md as well as at least one test I think (software tokens can expose many slots, shouldn't be too hard).

Jakuje

Do you plan to get back to this PR any time soon to finalize it? I found one copy paste error, but otherwise I think it is going in the right direction!

bukka · 2025-04-08T15:42:50Z

Do you plan to get back to this PR any time soon to finalize it?

I just fixed that typo and added docs but tests will take me a bit more time. I had a look and it seems like it has currently only single slot setup so might need some extending to allow adding setup for multiple slots with different pins and then somehow pass through the slot id so it can be used (e.g. SoftHSMv2 will re-assign the value so it needs to be fetched - possibly from the output message). Or maybe there is an easier way how to test it?

simo5 · 2025-04-09T12:13:22Z

Do you plan to get back to this PR any time soon to finalize it?

I just fixed that typo and added docs but tests will take me a bit more time. I had a look and it seems like it has currently only single slot setup so might need some extending to allow adding setup for multiple slots with different pins and then somehow pass through the slot id so it can be used (e.g. SoftHSMv2 will re-assign the value so it needs to be fetched - possibly from the output message). Or maybe there is an easier way how to test it?

Kryoptic can easily be configured to have as many slots as you want (each one with a different token), you just need to provide a config file that specifies them and initialize them.

Jakuje · 2025-04-10T09:26:52Z

Here is a draft commit that adds multiple slots to the kryoptic setup (and initializes just one):

Jakuje@5527293

Feel free to reuse it or adjust it to crate a test case. I do not exactly know how the problem you are solving would demonstrate so if you will need some help to put things together, please let me know.

bukka · 2025-08-10T19:04:07Z

Just a quick update here that I put some initial setup for multiple sot with an initial C test for refreshing (after fork check), which is where I needed this setting of the default slot ID. This is still incomplete as the load key currently loads the key without calling verify (something that I need to figure out later). I just realised that I have got some other fixes to address this particular issue in a different way so this test could no longer test the actual functionality for default slot. So I plan to change the test as it might be done in an easier way through the import. Basically I could import new pub key and then just check that it's in the default slot. I will still need the current fork test for other changes so will just add it as part of other (future) PR.

It means that it's still a draft and there will be some further clean up as well so don't bother with review yet as it will change. This note is also for me so I remember it when I'm back on this in the next few weeks. :)

bukka · 2025-09-13T14:16:56Z

I spent another chunk of time on this and getting this tested is actually a bit harder.

First I tried the new approach with just importing the pubkey. This can be done in a couple of ways and the simplest seemed to be doing that during the match. It means using EVP_PKEY_eq. However this didn't really get me too far because such imported key is just temporarily imported but no actual operation that requires login will run on it. It also doesn't get stored because storing happens only in p11prov_obj_get_handle.

I thought about it and what I really need first is to somehow get EVP_DigestVerifyInit executed on public key that will have keymgmt pointing to pkcs11 provider. That imports the key (through evp_keymgmt_export -> ec_export / rsa_export -> evp_keymgmt_util_try_import) and then does the verify (that should call p11prov_sig_operate_init which calls p11prov_obj_store_public_key). This is actually the path where I saw some other issues so it would be really good to get it tested as it would help for some other tests.

The part that seems a bit harder is to get the public key keymgmt pointing to pkcs11 provider. At least it doesn't seem to work in test when using store load as used by testing load_key even for pkcs11 URI (e.g. tried with pkcs11:type=public;id=%02%01) or pubkey PEM for pubkey present in hsm. The reason is that it just calls export from the provider and than cache the pubkey and uses default provider for further operations. I guess I need to find a different way how to load it because I can actually see it used when debugging nginx which happens during handshake. I will need to look more to the TLS code to see how it's loaded there for client cert pub key.

simo5 · 2025-09-15T13:36:59Z

Simply set propquery to "provider=pkcs11" in your tests and it should try to force operations to happen on the token.
However because the provider delays initialization you may need to change the token configuration to init early in order for openssl to find all the functionality the provider can offer.

bukka · 2025-10-14T14:28:58Z

Thanks. Using OSSL_STORE_open_ex and EVP_DigestVerifyInit_ex with props set to provider=pkcs11 did the job (I didn't want to set it globally in openssl.cnf so just went with this).

It actually doesn't store pub key after refresh so it's hitting slightly different flow - I will need to figure out the flow when the pub key gets stored though. I feel I'm quite close to it but don't have more time today so will look into it next month when I have got my next time slot on this.

bukka · 2026-04-03T16:20:03Z

So I finally managed to get this to the working state. I decided to test just the actual setting of default slot. The default slot is used only in store_key which is called when storing pub key or private key. This actually does not happen very often and the most reliable / re-creatable case is using EVP_PKEY_fromdata which took me some time to realise.

So the test needed extending the framework for multislot variant that needs to be separate because bunch of tests depend on the fact that there is just one slot. With this setup, the test just set the slot to the second one (be default provider picks the first one as default) and then imports pubkey using EVP_PKEY_fromdata which calls store_key and stores to the default slot. This is then verified using pkcs11-tool.

I'm not sure why coverage test is failing. I tried locally with -Db_coverage=true but it passes for me.

And that TLS test (other failure) might be a bit flaky and seems unrelated to this.

Jakuje · 2026-04-07T16:32:31Z

I'm not sure why coverage test is failing. I tried locally with -Db_coverage=true but it passes for me.

The CI job will likely need to be updated to upload test artifacts on failure (which is not done now).

The tls test in kryoptic sounds flaky. The faulure from the artifacts is:

spawn openssl s_server -propquery ?provider=pkcs11 -accept 23456 -groups ?x25519_mlkem768:?x25519:?x448_mlkem1024:?x448:secp256r1:secp521r1:secp384r1:ffdhe2048:ffdhe3072:ffdhe4096:ffdhe6144:ffdhe8192 -naccept 1 -key pkcs11:type=private;id=%00%01 -cert pkcs11:type=cert;object=testCert
Using default temp DH parameters
ACCEPT
ERROR
000315E6527F0000:error:0A00006C:SSL routines:ssl_derive:bad key share:ssl/s3_lib.c:5436:

This is the test ## Run test with TLS 1.2. The message is not much informative so it will need some more debugging.

Jakuje

just a nit for documentation, otherwise looks good.

Not sure whats up with the CI though

simo5 · 2026-04-27T15:16:32Z

@bukka can you squash all commits that deal with tests? I see a lot of back and forth there and would rather collapse all dead-ends away

bukka · 2026-05-03T11:41:27Z

@simo5 I squashed to a single commit which probably makes most sense as that multi slot setup should be introduced with some test that uses it. It's also rebased.

simo5

Overall we are close, just a few nitpicks from me on top of those from @Jakuje

simo5 · 2026-05-05T13:10:25Z

         * and the following query excludes it */
        if ((sctx->default_slot == CK_UNAVAILABLE_INFORMATION)
+            && (prov_default_slotid == CK_UNAVAILABLE_INFORMATION
+                || prov_default_slotid == slot->id)


Why did you clamp this check down here?
This code is meant to avoid using softokn first slot whch is usually a read-only slot and it also usually not what you'd mark as default.

This just adds an extra condition to make sure that the specified provider default slot is set only if it's a valid slot id (meaning if there is any slot with such ID). The softokn conditions still applies here as it's AND. What am I missing?

I would think that if a slot is set in configuration as the default it should be used regardless of other considerations ?

simo5 · 2026-05-05T13:13:38Z


 setup_script=find_program('setup.sh')
-all_suites=['softokn', 'softhsm', 'kryoptic', 'kryoptic.nss']
+multi_suites=['kryoptic.multislot']


Doesn't look like you really need to change around these definitions, given kryoptic.multislot is used only in one test you can simply use it here, and leave "all_suites" as it is. It will make more clear that kryoptic.multislot is special and not one of the generic suites to run.
This will cause less churn as well.

Yeah sure I can change it if you prefer.

Just to be clear, all will not really mean all then. Also it might be useful to also add softhsm.multislot in the future so there could be more multislot ones.

But as I said I don't really mind to change it. This was just my thinking why I did it this way. Let me know if you still want me to change and I will do that.

yes I prefer to keep the change minimal for now, and only add multislot to "all" if it really becomes more widely used.

There are use cases when one wants to select the default slot id (especially when the first slot ID is not convenient for this purpose) which is used for example for storing public keys. As part of this change, the test framework was extended to allow testing of multi slot scenarios. Signed-off-by: Jakub Zelenka <jakub.openssl@gmail.com>

bukka

Oh I replied 2 weeks ago but forgot to submit so here are my replies - sorry for the delay :)

bukka · 2026-05-10T14:52:04Z

         * and the following query excludes it */
        if ((sctx->default_slot == CK_UNAVAILABLE_INFORMATION)
+            && (prov_default_slotid == CK_UNAVAILABLE_INFORMATION
+                || prov_default_slotid == slot->id)


This just adds an extra condition to make sure that the specified provider default slot is set only if it's a valid slot id (meaning if there is any slot with such ID). The softokn conditions still applies here as it's AND. What am I missing?

bukka · 2026-05-10T14:59:51Z


 setup_script=find_program('setup.sh')
-all_suites=['softokn', 'softhsm', 'kryoptic', 'kryoptic.nss']
+multi_suites=['kryoptic.multislot']


Yeah sure I can change it if you prefer.

Just to be clear, all will not really mean all then. Also it might be useful to also add softhsm.multislot in the future so there could be more multislot ones.

But as I said I don't really mind to change it. This was just my thinking why I did it this way. Let me know if you still want me to change and I will do that.

Jakuje · 2026-05-24T09:53:52Z

+cases, it allows you to ensure that pkcs11-module-token-pin points to the
+expected slot.
+
+If no value is specified, the first listed slot is used.


nit: The logic for the selection of the slot is more complicated -- the slot needs to be initialized, it needs to require a login and it can not be locked:

&& (slot->token.flags & CKF_LOGIN_REQUIRED) && (slot->token.flags & CKF_TOKEN_INITIALIZED) && (!(slot->token.flags & CKF_USER_PIN_LOCKED))) {

I think it would be better to be a bit more specific here, for example:

Suggested change

If no value is specified, the first listed slot is used.

If no value is specified, the first slot listed by the token, that is initialized and does not have a PIN locked is used.

(I do not insist of this wording -- but I would prefer to have the documentation more specific)

bukka force-pushed the pkcs11-module-default-slot-id branch from 3dd2936 to a564597 Compare March 10, 2025 15:48

Jakuje reviewed Apr 8, 2025

View reviewed changes

Comment thread src/provider.c Outdated

bukka force-pushed the pkcs11-module-default-slot-id branch from a564597 to 631c354 Compare April 8, 2025 15:36

bukka mentioned this pull request Apr 12, 2025

Incorrect slot for new associated object #557

Open

bukka force-pushed the pkcs11-module-default-slot-id branch from 631c354 to 9e06cd7 Compare July 24, 2025 19:03

bukka force-pushed the pkcs11-module-default-slot-id branch from 9e06cd7 to 82e94bf Compare August 10, 2025 18:22

bukka mentioned this pull request Aug 10, 2025

Associated private key search needs login #610

Open

bukka force-pushed the pkcs11-module-default-slot-id branch 2 times, most recently from 887ff0a to 4508607 Compare September 13, 2025 13:21

bukka force-pushed the pkcs11-module-default-slot-id branch from 4508607 to 8bbefa6 Compare October 14, 2025 14:13

bukka force-pushed the pkcs11-module-default-slot-id branch from 8bbefa6 to 493fe16 Compare January 25, 2026 17:05

bukka force-pushed the pkcs11-module-default-slot-id branch 2 times, most recently from b31dd82 to 8c96dab Compare March 14, 2026 19:36

bukka force-pushed the pkcs11-module-default-slot-id branch from 8c96dab to 08fbf3e Compare April 3, 2026 15:12

bukka marked this pull request as ready for review April 3, 2026 16:09

bukka mentioned this pull request Apr 19, 2026

Add failure test logs for coverage job #724

Merged

bukka force-pushed the pkcs11-module-default-slot-id branch from 700b4bc to cd56f76 Compare April 25, 2026 13:07

Jakuje previously approved these changes Apr 27, 2026

View reviewed changes

Comment thread docs/provider-pkcs11.7.md Outdated

bukka dismissed Jakuje’s stale review via 3a7b6a0 May 3, 2026 11:39

bukka force-pushed the pkcs11-module-default-slot-id branch from cd56f76 to 3a7b6a0 Compare May 3, 2026 11:39

simo5 requested changes May 5, 2026

View reviewed changes

bukka force-pushed the pkcs11-module-default-slot-id branch from 3a7b6a0 to fb3cd8a Compare May 10, 2026 14:48

bukka commented May 23, 2026

View reviewed changes

Jakuje reviewed May 24, 2026

View reviewed changes

	If no value is specified, the first listed slot is used.
	If no value is specified, the first slot listed by the token, that is initialized and does not have a PIN locked is used.

Conversation

bukka commented Mar 10, 2025

Description

Checklist

Reviewer's checklist:

Uh oh!

simo5 commented Mar 10, 2025

Uh oh!

Jakuje left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

simo5 commented Apr 9, 2025

Uh oh!

Jakuje commented Apr 10, 2025

Uh oh!

bukka commented Aug 10, 2025

Uh oh!

bukka commented Sep 13, 2025

Uh oh!

simo5 commented Sep 15, 2025

Uh oh!

bukka commented Oct 14, 2025

Uh oh!

bukka commented Apr 3, 2026

Uh oh!

Jakuje commented Apr 7, 2026

Uh oh!

Jakuje left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

simo5 commented Apr 27, 2026

Uh oh!

bukka commented May 3, 2026

Uh oh!

simo5 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bukka left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

bukka commented Apr 8, 2025 •

edited

Loading