Skip to content

Add pkcs11-module-default-slot-id cfg for default slot ID#536

Draft
bukka wants to merge 2 commits intoopenssl-projects:mainfrom
bukka:pkcs11-module-default-slot-id
Draft

Add pkcs11-module-default-slot-id cfg for default slot ID#536
bukka wants to merge 2 commits intoopenssl-projects:mainfrom
bukka:pkcs11-module-default-slot-id

Conversation

@bukka
Copy link
Contributor

@bukka bukka commented Mar 10, 2025

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

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

simo5 commented Mar 10, 2025

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).

Copy link
Contributor

@Jakuje Jakuje left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 bukka force-pushed the pkcs11-module-default-slot-id branch from a564597 to 631c354 Compare April 8, 2025 15:36
@bukka
Copy link
Contributor Author

bukka commented Apr 8, 2025

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
Copy link
Collaborator

simo5 commented Apr 9, 2025

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
Copy link
Contributor

Jakuje commented Apr 10, 2025

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 bukka force-pushed the pkcs11-module-default-slot-id branch from 631c354 to 9e06cd7 Compare July 24, 2025 19:03
@bukka bukka force-pushed the pkcs11-module-default-slot-id branch from 9e06cd7 to 82e94bf Compare August 10, 2025 18:22
@bukka
Copy link
Contributor Author

bukka commented Aug 10, 2025

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 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
Copy link
Contributor Author

bukka commented Sep 13, 2025

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
Copy link
Collaborator

simo5 commented Sep 15, 2025

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 bukka force-pushed the pkcs11-module-default-slot-id branch from 4508607 to 8bbefa6 Compare October 14, 2025 14:13
@bukka
Copy link
Contributor Author

bukka commented Oct 14, 2025

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 bukka force-pushed the pkcs11-module-default-slot-id branch from 8bbefa6 to 493fe16 Compare January 25, 2026 17:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants