New safe and idiomatic interface

[hug-dev]

Hello 👋

Although we were gone silent for a while, @ionut-arm, @joechrisellis and myself were working hard on this for the past few weeks! The PR is gigantic and it is quite frightening, we all agree. Not so much of an early Christmas Present 🎁

Context

Let me summarise here the context and goal of this work, for anyone looking at this. In our project, Parsec we are interfacing in Rust with the PKCS11 interface. We are using this crate and discovered several memory safety issues. As @mheese and us agreed, we needed higher level abstractions that were safer than the raw bindgen types.
Also, there was this PR #40 showing that the bindings generated did not work for all architecture because the spec is based on native C types which do not have a defined size (int, long, etc).
Finally, @joechrisellis added libloading support to bindgen through this PR.

With all these three things together, it sounded like this crate was a perfect target for the following idiom, often used for Rust crate abstracting over C APIs:

• having a low level pkcs11-sys crate which uses bindgen and only exposes the raw bindgen types (unsafe and non idiomatic). In our case this would directly use the new feature in bindgen to provide dynamic loading.
• having on top of that a high level pkcs11 crate which only exposes idiomatic and safe Rust types and functions.

What we did

We did exactly that with the three commits in this PR! In order to not change the current interface, we did not modify anything from the current code but rather but all the new abstractions in the new module. Only this new module depends on pkcs11-sys.

Here is what we've done:

• 2ac5f49: move all the existing code into the pkcs11 folder and create a Cargo workspace at the top level
• 76f3f0b: add the pkcs11-sys crate to the Cargo workspace. This crate contains the bindings for Linux on x86 and can generate them for other architectures.
• 4cd81d1: add the new module, which depends on pkcs11-sys

Now to go a bit more in details about that new module:

• it does not implement everything. Only a subset of the PKCS11 API is currently abstracted but the goal is that everything which is currently abstracted is safe and idiomatic. Our initial target was to abstract everything that we need in Parsec.
• it is opinionated in order to be both idiomatic and safe (see more on this below)
• it is to be used independantly of the already existing structures
• all public items are documented (#[deny(missing_doc)]
• there are a few tests but we integrated in Parsec through this PR and it passed fine the intensive stress testing.

Opinions

• Session contains a reference to Pkcs11. This allows not to be able to destroy the Pkcs11 context while there are live sessions.
• All single-part operations are done in one call (instead of _init and normal call)
• Pkcs11 finalize on drop
• The Attribute and Mechanism own all of the data they contain. That helps keeping the memory safety by passing the raw pointer of that data to PKCS11 only at the last moment. That is instead of having Attribute or Mechanism structures which contain raw pointers themselves and which can often be invalid if the data ponited at moved.
• The get_attribute method returns a new, copied, vector of attributes, for simplicity. The API is then different from the PKCS11 but the ease of use is much better.
• Session close and logout on drop
• login and logout do not fail if another of the same slot is already logged in/logged out. This helps handling the fact that all sessions on the same slot share the log in/out status.
• The PIN is stored per slot on the Pkcs11 structure and zeroized on drop. It must be set before trying to log in with any session.
• Session does not implement Sync to prevent a reference of Session to be shared amoung multiple threads. To allow that safely, the module would have to implement more locking around some operations which makes it more complex. Usually, a new session would be created in its own thread. Send is still there though because it is possible and safe with the current implementation to give to a thread ownership of a Session.

Reviewing

Please feel free to tell us what you think about the general idea. We thought that it is better for the Rust ecosystem to have one crate for PKCS11 abstraction rather than us going and creating our own fork.

We agree that it is a big job to review as it is. Feel free to tell us if you would like us to split it in more PRs, in the way you want.

Fix #38
Fix #42

[hug-dev]

Hi @mheese !

Hope you were able to take some holidays during the Christmas break :)

I wanted to annoyingly ping you again about this PR, did you have a change to look at it? I am happy to help in any way I can for the review: trying to split it in shorter PRs, explaining some parts better, etc. I am also open having a chat over Zoom or something to go through it/discuss together if that's easier.

I think the best would be to have one single safe and idiomatic PKCS11 Rust crate in Rust instead of us going in our own corner creating a new one. Plus review always make things better.

[nickray]

For what it's worth (representing SoloKeys), I would also be very interested in a higher-level/idiomatic/safe/easier API to use PKCS#11 with. I'm currently using this library through my own pkcs11-uri (which I think would be great to upstream), to inject HSM-backed goodness in various activities.

Also (not that it quite fits in this PR, but in line with it being preferable to have one shared, maintained Rust API), I'd very much like and need a bump to v3, specifically for Ed25519 support.

@mheese, if you are out of time or interest, it would be good to know if you'd prefer sharing stewardship with other parties, or prefer other parties starting an independent implementation/fork? Thank you in any case for what you've done, it has been helpful to use this library!

[hug-dev]

We (the Parsec team) would be more than happy to help (depending of our time/capacity) in the maintenance of this repo.

Providing an abstraction over PKCS11, we are pretty much very interested in the good development of this crate as well 😃
We are doing the same with TPMs and our tss-esapi crate.

[hug-dev]

On the Parsec Community Meeting of the 23rd February 2021, we will be discussing the future of our usage of this pkcs11 crate and what actions should we take.
As we said before, we would love to help in any way we can to build one single safe and idiomatic Rust crate for pkcs11 but now we might have to think about solutions such as the sad news of creating our own PKCS11 abstraction.
Anyone interested in kindly inviting to this meeting!

[hug-dev]

Here are the meeting minutes.

As a summary, we decided to create our own Rust PKCS11 wrapper with the content of the new module of that PR to start with. We are still thinking about the name before going ahead. cryptoki was proposed.

[hug-dev]

The repo now exists at https://github.com/parallaxsecond/rust-cryptoki
Anyone is welcome to make contributions!

@mheese thanks again for your work and I hope we will be able to work again in the future.
Feel free to close this PR and all the related issues :)

[hug-dev]

We continued development of the PKCS11 wrapper here.