Add bindings for multi-part operations

[jacobprudhomme]

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

Description

This PR adds direct bindings for the multi-part operations C_{Encrypt,Decrypt,Sign,Verify,Digest}{Init,Update,Final}. Closes #250

Summary of Changes

• Added encrypt_{initialize,update,finalize}() to session/encryption.rs
• Added decrypt_{initialize,update,finalize}() to session/decryption.rs
• Added {sign,verify}_{initialize,update,finalize}() to session/signing_macing.rs
• Added digest_{initialize,update,finalize}() to session/digesting.rs
• Added tests for all of the above to tests/basic.rs

Questions/Feedback Wanted

• Are there any strong feelings about the naming (e.g. would initialize_encrypt() be preferred over encrypt_initialize()?)? I did things this way so that it was more discoverable, in that a user could start typing encrypt and would see all of the encrypt operations grouped together
• I added tests for the sad-path cases of calling an update/finalize operation before it was initialized and initializing an operation that's already in progress. Should I delete these, given that this is an implementation detail of the underlying PKCS#11 device and not this library itself?
• Do any version number changes need to be made manually?
• Should I add myself to CONTRIBUTORS.md as mentioned here? Or is that only for long-term/consistent contributors? (Sorry if that's a silly question! This is my first time contributing to an open-source project in a more substantial way)

[hug-dev]

This looks really good, good job ⭐ ! Very nice with all the tests that you added too! Definitely a good step to have in the code and then we can decide later if we want to add more typing to check the multi-part sequence at compile time.

Are there any strong feelings about the naming

I like your thinking about discoverability! My only opinion would be to match with the PKCS11 shorter names and have for example with decrypt: decrypt_init and decrypt_final?

Should I delete these, given that this is an implementation detail of the underlying PKCS#11 device and not this library itself?

I don't think so! Those are really good and are actually not implementation details since they are written in the spec:

The message-digesting operation MUST have been initialized with C_DigestInit.

so perfectly sensible to check that :)

Do any version number changes need to be made manually?

We will do that ourselves when we decide to make a new release!

Should I add myself to CONTRIBUTORS.md as mentioned here?

No requirement on our side! That is only if you want it :) I think the original reason is to have people behind what the copyright notice says.

[jacobprudhomme]

@hug-dev thanks for the feedback! I changed the function names as you suggested and applied the lint fixes that were preventing CI from passing :)

[hug-dev]

Thanks!

[jacobprudhomme]

Sorry, it seems there was another CI error to fix. Should all be good now!

[wiktor-k]

This looks very clean and nice, thank you! 👍

Would you mind rebasing this on top of main? We recently landed a big PR and I wonder if it'll all work together nicely :)

(if @hug-dev want to merge it and potentially collect the pieces of the merge I'm fine with that too 😅 )

[jacobprudhomme]

Just rebased off of main!

[wiktor-k]

Ugh... it seems I screwed it up this time by pressing "Enable auto-merge" but Run tests against Kryoptic is not Required for merging... ugh... maybe I'll just... 😶‍🌫️

[jacobprudhomme]

Ouff... I know what's causing the issue too. All the keys I created during my tests had the bare minimum template attributes necessary to not trigger an error from SoftHSM (for example, not setting Attribute::{Encrypt,Decrypt,Sign,Verify}(true)). It seems either Kryoptic is more strict about requiring these attributes, or it doesn't set them to their defaults?

Should I open another PR to fix this?

[wiktor-k]

Should I open another PR to fix this?

If you could that'd be really appreciated 🙏

[hug-dev]

Added the missing CI checks as required now!