Skip to content

Conversation

@jacobprudhomme
Copy link
Contributor

@jacobprudhomme jacobprudhomme commented Mar 31, 2025

Author: Jacob Prud'homme
Email: [email protected]

Description

In tests relating to multi-part operations, added missing attributes to key templates, since Kryoptic was throwing errors about said keys not having the correct attributes set on them to perform certain operations (see here)

Summary of Changes

  • Tweaked tests for multipart encrypt/decrypt/verify/sign/digest functionality
    • Added missing attributes to templates for keys
    • Asserted that only a generic PKCS#11 error happens for the *_not_initialized tests, since the exact error returned by various backends is different
    • Similarly, changed the *_already_initialized tests to account for differing behaviour between backends
    • Ignored sha256_digest_key test, since it seems to be impossible to test the desired functionality on all backends for the moment (in particular, extracting the value from a secret key marked Extractable and not Sensitive is not always supported)

@jacobprudhomme
Copy link
Contributor Author

It seems a host of new errors has popped up now. I'll keep exploring

@jacobprudhomme jacobprudhomme marked this pull request as draft March 31, 2025 09:21
@wiktor-k
Copy link
Collaborator

I'll CC @Jakuje just in case they have some thoughts on the Kryoptic issues. kthxbai 🙇

@Jakuje
Copy link
Collaborator

Jakuje commented Mar 31, 2025

I think we had some issues with the multipart operations recently. They were fixed in main, but not in any release version yet: latchset/kryoptic#179

I will see if we will get a new release out (and update in Fedora) or I can backport the changes to Fedora later today. Sorry for inconvenience! I think the issue is not on your side!

@wiktor-k
Copy link
Collaborator

I think we had some issues with the multipart operations recently. They were fixed in main, but not in any release version yet: latchset/kryoptic#179

I will see if we will get a new release out (and update in Fedora) or I can backport the changes to Fedora later today. Sorry for inconvenience! I think the issue is not on your side!

Thank you. 🙏 It's great to hear your remarks directly.

@jacobprudhomme jacobprudhomme marked this pull request as ready for review March 31, 2025 13:25
@jacobprudhomme
Copy link
Contributor Author

Fixed the tests! I had to ignore one to get all of them to pass. I'm open to suggestions on how to rewrite that test, though. I already tried manually creating a key with a manually set value and using that to compare against, but it seems Kryoptic doesn't allow using a key created this way to be used in C_DigestKey

Copy link
Collaborator

@wiktor-k wiktor-k left a comment

Choose a reason for hiding this comment

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

Okay, I think it looks good. I guess the ignored test could check for softhsm the same way as some other tests (it's hard to check on mobile for me now).

Let's leave it open for a while as @Jakuje may have an idea on how to better adjust it.

Signed-off-by: Jacob Prud'homme <[email protected]>
@Jakuje

This comment was marked as outdated.

@Jakuje

This comment was marked as outdated.

@Jakuje

This comment was marked as outdated.

@Jakuje
Copy link
Collaborator

Jakuje commented Mar 31, 2025

(but still odd that the rust-cryptoki tries to call it without any template)

Ok, I see the bug. With the optimizations, we do not pull all attributes from db for get_attributes and the cka_sensitive defaults to true. Filled in latchset/kryoptic#193 -- I will try to have a look into that.

For the time being, feel free to skip this test for kryoptic, similarly as I skipped some others (with the link to the above issue). Once I will have a fix and new release, we can remove the exclusion.

@wiktor-k
Copy link
Collaborator

wiktor-k commented Apr 1, 2025

For the time being, feel free to skip this test for kryoptic, similarly as I skipped some others (with the link to the above issue). Once I will have a fix and new release, we can remove the exclusion.

Hah, it seems including Kryoptic makes for a nice cross-testing experience. Great that we have you here to help debug the issues.

@jacobprudhomme Would you be so kind to add a simple if !softhsm { return } code branch there? I think otherwise this is good to go (unless @hug-dev spots something 😅 ).

@jacobprudhomme
Copy link
Contributor Author

Alright, done!

@wiktor-k wiktor-k enabled auto-merge April 1, 2025 08:45
Copy link
Collaborator

@wiktor-k wiktor-k left a comment

Choose a reason for hiding this comment

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

Much appreciated 🙏

Copy link
Member

@hug-dev hug-dev left a comment

Choose a reason for hiding this comment

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

Thank you!

@wiktor-k wiktor-k merged commit 3869b37 into parallaxsecond:main Apr 1, 2025
8 checks passed
@jacobprudhomme jacobprudhomme deleted the fix-multipart-operations-tests branch April 2, 2025 08:40
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.

4 participants