New safe and idiomatic interface #43
Conversation
Transforms the root folder into a Cargo workspace with one crate containing the existing pkcs11 crate. This is to allow having multiple crates living in this repository and in particular the pkcs11-sys crate which is going to be added in the next commit. Co-authored-by: Joe Ellis <joe.ellis@arm.com> Co-authored-by: Ionut Mihalcea <ionut.mihalcea@arm.com> Co-authored-by: Hugues de Valon <hugues.devalon@arm.com> Signed-off-by: Hugues de Valon <hugues.devalon@arm.com>
The pkcs11-sys crate directly uses the new bindgen feature available in 0.56.0 to generate bindings and minimal library code to dynamically load the PKCS11 library. The crate commits in tree the bindings for the x86_64-unknown-linux-gnu target with the possibility of generating them during build time through a feature. The goal is to make compilation easier and faster for supported targets. This crate exposes all values and types coming from the interface. Co-authored-by: Joe Ellis <joe.ellis@arm.com> Co-authored-by: Ionut Mihalcea <ionut.mihalcea@arm.com> Co-authored-by: Hugues de Valon <hugues.devalon@arm.com> Signed-off-by: Hugues de Valon <hugues.devalon@arm.com>
This commit adds the `new` module in the pkcs11 library. This new module is an abstraction directly over the pkcs11-sys crate. It currently only abstracts a subset of the PKCS11 interface but all the types and functions abstracted are idiomatic and safe. The functions only work with abstracted types and do not use raw bindgen types. This commit also adds a few tests for this new module and also conversions from PSA Crypto Algorithm type. Co-authored-by: Joe Ellis <joe.ellis@arm.com> Co-authored-by: Ionut Mihalcea <ionut.mihalcea@arm.com> Co-authored-by: Hugues de Valon <hugues.devalon@arm.com> Signed-off-by: Hugues de Valon <hugues.devalon@arm.com>
|
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. |
|
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 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! |
|
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 馃槂 |
|
On the Parsec Community Meeting of the 23rd February 2021, we will be discussing the future of our usage of this |
|
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. |
As was discussed, this add the contents of the `new` module and the `pkcs11-sys` crate, renaming everything to `cryptoki`. See [this PR](mheese/rust-pkcs11#43) for all the history. Signed-off-by: Hugues de Valon <hugues.devalon@arm.com>
|
The repo now exists at https://github.com/parallaxsecond/rust-cryptoki @mheese thanks again for your work and I hope we will be able to work again in the future. |
|
We continued development of the PKCS11 wrapper here. |
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:
pkcs11-syscrate 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.pkcs11crate 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
newmodule. Only thisnewmodule depends onpkcs11-sys.Here is what we've done:
2ac5f49: move all the existing code into thepkcs11folder and create a Cargo workspace at the top level76f3f0b: add thepkcs11-syscrate 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 onpkcs11-sysNow to go a bit more in details about that
newmodule:#[deny(missing_doc)]Opinions
Sessioncontains a reference toPkcs11. This allows not to be able to destroy thePkcs11context while there are live sessions._initand normal call)Pkcs11finalizeon dropAttributeandMechanismown 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 havingAttributeorMechanismstructures which contain raw pointers themselves and which can often be invalid if the data ponited at moved.get_attributemethod 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.Sessionclose and logout on droploginandlogoutdo 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.Pkcs11structure and zeroized on drop. It must be set before trying to log in with any session.Sessiondoes not implementSyncto prevent a reference ofSessionto 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.Sendis still there though because it is possible and safe with the current implementation to give to a thread ownership of aSession.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