Refactor initialing/finalizing Pkcs11 and Session structures #326
Conversation
Signed-off-by: Wiktor Kwapisiewicz <wiktor@metacode.biz>
As explained in parallaxsecond#208, this might not be wanted in contexts where the Pkcs11 structure is initialised/finalised outside of the Rust program. Signed-off-by: Hugues de Valon <hugues.de-valon@einride.tech>
Callers can directly use the initialize function and check the return code to see if the initialization already happened. Signed-off-by: Hugues de Valon <hugues.de-valon@einride.tech>
Currently a session can only be closed on drop. Signed-off-by: Hugues de Valon <hugues.de-valon@einride.tech>
Signed-off-by: Hugues de Valon <hugues.de-valon@einride.tech>
| session.close()?; | ||
| pkcs11.finalize()?; |
There was a problem hiding this comment.
Correct me if I'm wrong but this change means that if an error happens before these lines are hit then no close and finalize will happen. Of course that's only outside of the happy path and maybe not so important (since tests are assumed to always complete successfully) 🤔
There was a problem hiding this comment.
True 😅 I was too lazy to add finalize on all the paths just for the tests
There was a problem hiding this comment.
Naah, I'm just thinking other folks may also be too lazy or just use the code here as an inspiration for their code... I'm not sure how big of a deal would be if there was no finalization on error cases.
(My original PR idea was to have something that finalizes, just like the old code, to have all options available 🤔 )
There was a problem hiding this comment.
I think as per the discussion in #208, seems like initialization/finalization of Pkcs11 mostly happen outside of the application that uses it... So not sure if it's something we should care about that much. But we can also add a new constructor which could finalize on Drop. Maybe on a new PR 🤓 ?
There was a problem hiding this comment.
While it is necessary to call Initialize, calling Finalize is not such a big deal; when the process terminates, generally, all is gone anyway.
Any pkcs11 driver that needs to keep state has to face the chance the process crashes anyway, so any state saved, or client/server architecture within a pkcs11 driver needs to be able to handle the case where finalize is not called and unclean shutdowns happened.
|
@Jakuje Not sure why Kryoptic allows double initialization just for this PR 🤔 |
Hmm. I thought we had this test case covered. Looking at the code, we have a check for the https://github.com/latchset/kryoptic/blob/fbc31d2731c459d1211b73c0f25c2d4d92818c44/src/lib.rs#L556 I guess it might be partially because the way how the tests are executed, but sounds to me like a bug to fix. Do you want to open an issue or should I do that? |
I did with latchset/kryoptic#375! |
|
I guess the kryoptic version containing the fix is not published yet and not available via |
It depends how long you want to wait. :) But given that waiting would basically prevent merging any code now, I would propose to resurrect the #311 and try to run against main (or fixed commit, even though the build times might get longer -- but most of the delays can be solved by caching). I can try to propose something. Should be quite easy to do based on the current FIPS pipeline. To have some rough idea how long it will take to get the fixes to fedora, we have kryoptic version 1.4.0 due to be released likely next week and then it will take some more days to get it updated in Fedora so my guesstimate would be at least one week from now. |
|
Good point! I think it should be ok to run against |
|
Opened #328 for testing against kryoptic main. |
4 participants
As discussed in #208 and related PRs, the lifecycle management of the Pkcs11 structure might probably happen outside of the Rust program and such automatic finalizing on Drop is not wanted.
This PR removes that and also allows manually closing a session, which is needed as sessions take reference of the Pkcs11 structure.
Also removes the double initialization check and adapt the tests to work.
Fix #208