Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for PKCS#11 3.0 #248

Open
wants to merge 9 commits into
base: main
Choose a base branch
from
Open

Conversation

Jakuje
Copy link

@Jakuje Jakuje commented Mar 17, 2025

The PKCS#11 3.0 introduces new APIs with new function list to reach out to new functions. The 3.2 will come with yet another function list. This requires a change in the initialization. While most of the tokens now support also the 2.4 way, using new APIs is not possible through the old function list.

This is adding the support for the new functions and tests against another PKCS#11 module, fixing some incompatibilities that I found in the testsuite here. Its a bit mixed bag and it also found some issues in the kryoptic that we need to handle, such as latchset/kryoptic#182 and latchset/kryoptic#184 so opening as a draft for now.

@hug-dev
Copy link
Member

hug-dev commented Mar 19, 2025

Thanks for the PR! Could kryoptic provide another way to test our wrapper, similarly than SoftHSM does? The more the merrier 😁!

Some of the 3.0 compatibility was introduced with #228. I guess this would also fix #209.

Shout if you need early review or anything at this point!

@Jakuje
Copy link
Author

Jakuje commented Mar 19, 2025

Some of the 3.0 compatibility was introduced with #228.

Right. This added mostly mechanisms which can work through the old API. I think the new functions added are not reachable in anyway right now.

I guess this would also fix #209.

Correct. Right now, its probably the least amount of changes to make the functions reachable from inside of rust-cryptoki. Another bunch would be implementing the accessing functions in the API (for example message based operations).

On top of that to be able to support all the versatility of the PKCS#11 3.* GetInterface API, I think we might need some different entry functions than new() and new_from_self(). Something like new_version(version) might do. OTOH its a question whether we really need to access the old API versions when the new function lists alias over the old ones.

@Jakuje
Copy link
Author

Jakuje commented Mar 19, 2025

(The previous push had broken yaml for CI. This should fix it but it still waits for approval. It will fail for the kryoptic though)

@wiktor-k
Copy link
Collaborator

(The previous push had broken yaml for CI. This should fix it but it still waits for approval. It will fail for the kryoptic though)

🤔 I think the rest of the jobs fail because of a missing cargo fmt. If you don't mind that'd be good to do now 😅

@Jakuje
Copy link
Author

Jakuje commented Mar 20, 2025

(The previous push had broken yaml for CI. This should fix it but it still waits for approval. It will fail for the kryoptic though)

🤔 I think the rest of the jobs fail because of a missing cargo fmt. If you don't mind that'd be good to do now 😅

Sure, done. I always leave this for last minute and then forget :)

@wiktor-k
Copy link
Collaborator

Some of these test failures are quite easy to fix it seems:

assertion `left == right` failed
  left: "SoftHSM project"
 right: "Kryoptic Project"

Others, such as unsupported functions, may take a while to make it compatible with both projects but it's definitely something worth pursuing. (If it takes too much time it's OK to split this PR into two as well).

Thanks for your time, excellent work! 👏

@Jakuje
Copy link
Author

Jakuje commented Mar 20, 2025

Could kryoptic provide another way to test our wrapper, similarly than SoftHSM does? The more the merrier 😁!

Do you mean something like the ci.sh? It runs a lot of other things that I did not consider relevant (formatting, clippy, building all the targets) just to run the same tests against different token, but it might be easier to have it in one place even through some duplication.

Shout if you need early review or anything at this point!

Early review would be always welcomed if there is something conceptually wrong from your point of view or something significant missing to be added.

Some of these test failures are quite easy to fix it seems: ...

Do you have any strong opinions how this should be addressed? The current tests do test token name, interface version and more that is inherently different in every token and now tied to the softhsm. They can be removed (bad) or made dynamic based on some environment variable (KRYOPTIC_CONF, SOFTHSM2_CONF or some special?), which is still quite fragile. Or did you have something else on your mind?

Others, such as unsupported functions, may take a while to make it compatible with both projects but it's definitely something worth pursuing.

These will also need some distinction of the tokens inside of the test.

(If it takes too much time it's OK to split this PR into two as well).

I just wanted to make sure the new code paths I am adding are executed so thats why I would rather keep them together. But there are more to come :)

@wiktor-k
Copy link
Collaborator

Do you have any strong opinions how this should be addressed? The current tests do test token name, interface version and more that is inherently different in every token and now tied to the softhsm. They can be removed (bad) or made dynamic based on some environment variable (KRYOPTIC_CONF, SOFTHSM2_CONF or some special?), which is still quite fragile. Or did you have something else on your mind?

No particular preference. Testing for one exact string that's vendor specific is not the right thing to do anyway IMO. I guess the test was just a sanity check that we can get the project name. I think even making it check if the returned string is not empty is good enough. If we can find a different property to test instead that'd be stable that's even better.

In general I think it'd be great if we didn't encode any vendor specific logic in test. If there are differences it'd be nice if the code checked properties at test time and took a different path. I view tests as collection of nice examples that ideally folks could reuse with their own tokens. Yeah, they have been coded against SoftHSM which was OK at that time.

Sorry for a generic response 😅 my point was to use your best judgement. Adding another token for tests is a tremendous opportunity to make the tests a lot better! :)

Jakuje added 8 commits March 24, 2025 15:07
…0 API

Signed-off-by: Jakub Jelen <jjelen@redhat.com>
Signed-off-by: Jakub Jelen <jjelen@redhat.com>
Signed-off-by: Jakub Jelen <jjelen@redhat.com>
Signed-off-by: Jakub Jelen <jjelen@redhat.com>
The DES3 is not usable for anything and modern pkcs11 modules
do not implement it. AES is much widely implemented.

Signed-off-by: Jakub Jelen <jjelen@redhat.com>
Signed-off-by: Jakub Jelen <jjelen@redhat.com>
Signed-off-by: Jakub Jelen <jjelen@redhat.com>
@Jakuje
Copy link
Author

Jakuje commented Mar 24, 2025

Thank you. Adjusted (and resolved the conflicts).

@Jakuje Jakuje marked this pull request as ready for review March 24, 2025 14:13
Signed-off-by: Jakub Jelen <jjelen@redhat.com>
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.

None yet

3 participants