Nitrokey / nethsm-pkcs11

PKCS#11 module for NetHSM
Other
34 stars 10 forks source link

Investigate statics #163

Open sosthene-nitrokey opened 8 months ago

sosthene-nitrokey commented 8 months ago

Lots of LazyStatics are so because they are Arc<Mutex>, they may be replaceable by bare Mutex, getting rid of the Arc, potentially maybe also thread_locals instead so that the module can properly support multithreaded operation.

ansiwen commented 8 months ago

Some context: We need to pay attention to the different requirements of PKCS11 modules in different thread support modes. (See parameters of C_Initialize) With nginx there was (IIRC) the quirk that the initial process initialized the module already but didn't use it, then every worker fork initialized it again. So we need to handle initialization after forking of already initialized state (which can be tricky). That's one of the reasons why we switched from reqwest to ureq, because the first is using the tokio runtime, which was almost impossible to "repair" after forking. Edit "Initialize" here does not mean calling "C_Initialize" but the initialization of the module during the dlopen() call.

sosthene-nitrokey commented 8 months ago

Thanks. Is this documented somewhere?

ansiwen commented 8 months ago

No, not at a single place unfortunately. There is the PKCS11 spec, then there is this related issue https://github.com/tokio-rs/tokio/issues/1541. In general the issues with forking are: already created threads (will be missing) and file-descriptors (will be shared). So we would need to avoid both, at least before C_Initialize. I assume that's the reason why @nponsard used the lazy statics.

nponsard commented 8 months ago

No, not at a single place unfortunately. There is the PKCS11 spec, then there is this related issue tokio-rs/tokio#1541. In general the issues with forking are: already created threads (will be missing) and file-descriptors (will be shared). So we would need to avoid both, at least before C_Initialize. I assume that's the reason why @nponsard used the lazy statics.

I did not try thread_local when developing this project, the reason behind the use of lazy_static is that it was used in the https://github.com/aws/aws-nitro-enclaves-acm project (and I was not aware of thread_local at the time).

sosthene-nitrokey commented 8 months ago

Thanks for the feedback. I will try the nginx example and try to make a minimal reproducer of the forking behaviour and add it to the CI.