Add a new constructor that does not call C_Finalize when dropped

[Geobert]

trafficstars

Hi,

I’m in a mixed env with Rust code used in a C++ program. C++ side initialize the pkcs11 lib so Rust side can’t. If Rust side calls C_Finalize, I get a random crash, which, I think, is because of a double free?

[hug-dev]

Hello! Sorry for the long time without answer!

C_Finalize is called when then Pkcs11 instance is dropped. We could potentially add a new constructor which builds an instance that does not auto-finalize :)

[wiktor-k]

Hi @Geobert, I've made a slight adjustment to the API in #290 and would be super-happy if you could take a look if this solves your issue. The new function would be Pkcs11Impl::new_unchecked where you can pass the raw object and it would not be finalized.

Thanks in advance!

[simo5]

To be honest C_Finalize should probably never be implicitly called, as it can seriously disrupt concurrent token access (or force applications to keep the Pkcs11 structure in a global indefinitely simply to avoid issues). There should also definitely be a way to initialize Pkcs11 without calling C_Initialize (for the same reason).

[hug-dev]

as it can seriously disrupt concurrent token access (or force applications to keep the Pkcs11 structure in a global indefinitely simply to avoid issues)

What do you mean? By using the Arc encapsulation (which is now done on our side but maybe soon should be done by the caller), one can pass a Pkcs11 to different threads and be sure that it will on be finalized once, when all threads are done using it. That's what we do in the login_feast test.

There should also definitely be a way to initialize Pkcs11 without calling C_Initialize (for the same reason).

That is a good point!

[wiktor-k]

There should also definitely be a way to initialize Pkcs11 without calling C_Initialize (for the same reason).

Frankly, that has been my idea in https://github.com/parallaxsecond/rust-cryptoki/pull/290: to have a Pkcs11 object that just allows calling PKCS#11 functions in a safe way but never does any lifecycle ops (that's even in the documentation in that PR). And then another one, which does lifecycle (for quite a lot of use-cases it's sufficient and straightforward) which wraps the first one and also exposes PKCS#11 functions.

[simo5]

What do you mean? By using the Arc encapsulation (which is now done on our side but maybe soon should be done by the caller), one can pass a Pkcs11 to different threads and be sure that it will on be finalized once, when all threads are done using it. That's what we do in the login_feast test.

This works fine and well when a single crate depends on Cryptoki, but as soon as a large project have two dependencies that independently use Cryptoki (or just independent implementations of PKCS11, like via C bindings) you may have conflicts.

For the most part you can use two separate Pkcs11 structures w/o much interference as long as they do not do untoward things like: uncontrollably C_Initialize, uncontrollably C_Finalize and uncontrollably C_CloseAllSessions on the same slot.

Multiple C_Initialize are generally survivable as most well-behaved tokens simply return CKR_CRYPTOKI_ALREADY_INITIALIZED after the first, but C_Finalize is not survivable by "the other code path", similarly C_CloseAllSessions if only one slot is in use by both code paths.

Note that according to my reading currently Cryptoki fails to operate if C_Initialize returns CKR_CRYPTOKI_ALREADY_INITIALIZED, which is ... not great.

For example in Kryoptic you can create new slots on the fly by calling C_Initialize at any time with appropriate additional configuration (yes it is out of spec and mostly used for testing, but it is a thing :)

But also if you have an application using rust-tls compiled against native OpenSSL you may find that OpenSSL already initialized a token before Cryptoki is involved, and Cryptoki will blow up (one use case may be that the application creates keys on the token using cryptoki and then let rust-tls -> OpenSSL -> token deal with TLS authentication).

[hug-dev]

Thanks for showing some use cases where this would be a problem! Now I understand better that our "finalize-on-drop" approach might not be ideal by default.

I suggest then that we remove finalizing on drop but modify finalize to actually call C_Finalize. Then it would be the responsibility of the caller to finalize if they need it. We should modify our tests/examples to make sure it is finalizing as a last step. We could also add a new constructor finalizing on Drop but not sure if it is actually wanted in real-world scenarios as you described above.

Regarding initializing, one does not actually need to call initialize in order to use the other functions in this crate. One can still call it and control if the error is AlreadyInitialised to continue or not... If it's correct or not to expose that as an error or make it pass silently is a philosophical question... I think it's ok to say that everything which is not CK_OK is an error.

[simo5]

It is definitely correct to expose the error, an application may behave differently (for example avoid calling C_Finalize) based on that.

The challenge is making sure applications know how to handle that error, I guess that is mostly a documentation issue unless you want to change the return type to be a triplet <Ok, Warnings, Errors>. In some ways something like that is appealing, OTOH it would be quite surprising and perhaps too disruptive to application that expect standard error conventions.

An option would be to provide a mapping function that can properly interpret if an error is basically just a warning and "pass", or it is an actual fatal error the application really has to handle.

Something that would be invoked like this: .map(continue_on_warnings)? perhaps ?

Or maybe something like that is simply shown as a coding example, I am not totally sure, so perhaps do nothing and just document that some function can return errors that are fundamentally just warnings :-)

[hug-dev]

Or maybe something like that is simply shown as a coding example, I am not totally sure, so perhaps do nothing and just document that some function can return errors that are fundamentally just warnings :-)

That sounds like a good first step at least! And we could definitely also add an example on how that "ignoring" capability would look like.

[hug-dev]

All in all I suggest the following changes to be implemented:

• remove finalizing on Drop for the Pkcs11 structure
• fill the finalize method with the actual call
• remove the initialized flag on Pkcs11 and the getter method
• apply first commit of #290 (removing Arc and linking sessions to clients)
• adapt all tests and examples to call finalize when they are done and show a way to continue execution and not quitting early if the AlreadyInitialized error comes up

If you agree on it, then all of the above could be done in the same PR!