Fix panics on failed initialization

[sosthene-nitrokey]

This patch

• adds a Result version of the Device static so that any call can first check that the initialization was successful
• improves the deserialization of certificate fingerprints to make it fail properly rather than panic
• Use the CertStore add_parseable_certificates to discard unparseable certificates rather than panic. If no certificate is parsed, the initialization returns an error.

fix #161

[codecov[bot]]

Codecov Report

Attention: 34 lines in your changes are missing coverage. Please review.

Comparison is base (43da14b) 90.74% compared to head (4157673) 90.73%.

:exclamation: Current head 4157673 differs from pull request most recent head ff07b5e. Consider uploading reports for the commit ff07b5e to get more accurate results

Files	Patch %	Lines
pkcs11/src/config/config_file.rs	84.21%	9 Missing :warning:
pkcs11/src/api/mod.rs	78.26%	5 Missing :warning:
pkcs11/src/api/object.rs	81.81%	4 Missing :warning:
pkcs11/src/api/token.rs	88.88%	4 Missing :warning:
pkcs11/src/config/initialization.rs	94.73%	4 Missing :warning:
pkcs11/src/backend/events.rs	66.66%	2 Missing :warning:
pkcs11/src/backend/key.rs	50.00%	2 Missing :warning:
pkcs11/src/backend/mod.rs	0.00%	2 Missing :warning:
pkcs11/src/backend/slot.rs	86.66%	2 Missing :warning:

Additional details and impacted files

```diff @@ Coverage Diff @@ ## main #165 +/- ## ========================================== - Coverage 90.74% 90.73% -0.01% ========================================== Files 31 31 Lines 6158 6436 +278 ========================================== + Hits 5588 5840 +252 - Misses 570 596 +26 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

[sosthene-nitrokey]

I would like to change the approach so that the statics are only used in the top level calls, and every thing else takes a &<relevant static> but that probably makes sense in another PR IMHO.

But so that I correctly understand your approach, you would make the Device static a Mutex<Option<...>> that is Some between an Initialize call and a Finialize call.

That could work but I'm not sure how much it would work in a multithreaded context. I'd need to learn how PKCS11 deals with that. Given the weird nginx workarournds and #157 I think this can be approached in another PR. Maybe in this one I can remove the ensure_init! for functions that are expected to be infallible.

[robin-nitrokey]

An alternative approach would be to use OnceLock and only initialize the device on the first C_Initialize call. I don’t think we are required to free all resources on finalize, or to re-initialize everything on each initialize call.

I’m fine with splitting this up into a separate PR, but I’d still like to change the error codes and the direct DEVICE access in this PR. Specifically:

• In C_Initialize, return FUNCTION_FAILED in the Err case.
• In all other functions (i. e. the ensure_init macro), return NOT_INITIALIZED in the Err case.
• Use DEVICE_RESULT instead of DEVICE (or just replace DEVICE with DEVICE_RESULT). In the Err case, return NOT_INITIALIZED.

That would mean that calls after C_Finalize could still work, but it would handle the other cases without introducing big changes.

[sosthene-nitrokey]

Ok, I will do that then, but I think some uses of DEVICE are in infallible functions.

[robin-nitrokey]

Thanks! In that case, having an explicit panic is IMHO still preferable to an indirect panic through accessing a lazy static.

[sosthene-nitrokey]

Done, the non-tested lines are all error handling that should never happen.

[ansiwen]

Can you confirm that this still works with forks, that is, if the process that dlopened this module forks and then calls C_Initialize? (for example the tokio runtime can't handle that, see https://github.com/tokio-rs/tokio/issues/1541, so there are dragons luring). One of the issues is: you can't cleanly fork if there are threads created already.

[sosthene-nitrokey]

Do you have an example test for that from the previous errors you and Nils encountered? It would be nice to have it in CI because that would be broken easily on accident.

This will be especially relevant as I plan on tackling https://github.com/Nitrokey/nethsm-pkcs11/issues/157 next.

[sosthene-nitrokey]

I don't think anything I've done in this PR should change how it interacts with fork(). After a fork() the static should keep the same values, which means that the initialization should be the same.

[ansiwen]

Ok, maybe this is not relevant because I mix it up with the LazyStatic that is mentioned in issue #163. I just wanted to throw it in, in case it's related. The only example I have is running an actual nginx, that's where we saw the issues. I'm not aware of a test, that @nponsard wrote for that, but would certainly be useful. It would require loading the module with dlopen(), then doing a fork, and then testing the functionality of the module in the fork.

[nponsard]

I did not write any test for that, when I was investigatig the forking issue I was running the nginx example.

[sosthene-nitrokey]

Just tested on top of the images in #169 , it works with nginx. Now testing with apache.

[sosthene-nitrokey]

Both work, with a slight need for an update of the apache image which is incorrect in #169