Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Create a PSA Crypto Rust wrapper crate #62

Closed
ionut-arm opened this issue Oct 30, 2019 · 12 comments
Closed

Create a PSA Crypto Rust wrapper crate #62

ionut-arm opened this issue Oct 30, 2019 · 12 comments
Labels
enhancement New feature or request

Comments

@ionut-arm
Copy link
Member

Our Mbed Provider is closely tied to the FFI layer to Mbed Crypto, which lead to questions being raised about the safety of the code, especially around unsafe blocks and how such blocks should be handled in the larger context.

A sensible solution is to split out the code directly interfacing with Mbed Crypto into its own module/crate, keeping the unsafe calls contained.

@hug-dev
Copy link
Member

hug-dev commented Dec 6, 2019

As part of that, we should also make the Mbed Crypto crate dynamically load the C shared library to remove the need of building it ourselves.

@hug-dev
Copy link
Member

hug-dev commented Jan 6, 2020

I propose the following design/ideas for the Mbed Crypto crate.
It will have three goals:

  1. Link the available Mbed Crypto library on the system (in the default installation location) to the crate. Thtat means that we will no longer compile it ourselves.
  2. Produce bindgen Rust FFI to call the C library from Rust + write the needed preprocessor constants in a separate file (the equivalent of our current constants.rs). That would be the low-level, unsafe, layer.
  3. On top of that layer produce a safe Rust wrapper that uses idomatic Rust features and abstract C types.

There seems to be a lot of duplication between things we use in the Parsec Rust interface and things we would abstract in this crate. For example, the KeyAttributes structure and the way we convert it to the psa_key_attributes_t C structure. Should we re-use the same types?
As in, should we have a parsec-interface-rs dependency in the Mbed Crypto crate? Should those structures be only defined once in the Mbed Crypto crate?

@ionut-arm
Copy link
Member Author

should we have a parsec-interface-rs dependency in the Mbed Crypto crate?

I'm not sure that adds much value - we can have a dependency on the Mbed crypto crate in the interface crate and implement the conversions there. The Mbed-specific structures and some abstractions for the usage patterns should be defined in the Mbed crate, but nothing more

@hug-dev hug-dev changed the title Create a safe wrapper around Mbed Crypto Create a Mbed Crypto Rust wrapper crate Jan 29, 2020
@egrimley-arm
Copy link
Collaborator

If I understand correctly, bindgen generates Rust code that lets you directly access structures that are supposed to be opaque (such as psa_key_attributes_t), while it doesn't give you access to parts of the official API (such as psa_set_key_lifetime) which are defined as "static" functions or perhaps even as macros. So you might still have to write and compile a static C library consisting of shims/stubs. Or is there a better way?

@hug-dev
Copy link
Member

hug-dev commented Apr 20, 2020

You understanding is correct and regarding the psa_set_key_*** and psa_get_key_*** accessors particularly, as they are all defined in Mbed Crypto as static inline, bindgen can not generate Rust code for those.
The solutions we have:

  1. rely on the Mbed Crypto implementation of psa_key_attributes_t as we do now. Our code will break over different versions of Mbed Crypto that modify the opaque structure. It won't be portable if we want to target another PSA Crypto API implementation.
  2. find a way to compile Mbed Crypto forcing it to not inline the functions. There is a -fno-inline switch on GCC but Im wondering if it works even on functions marked static inline in a header file. Probably not.
  3. As you said, in the build.rs file compile with Mbed Crypto a library of C functions calling all the static inline functions so that bindgen can see them. We could actually do the same for macro values part of the API to not have to define a constants.rs file.
  4. Wait that bindgen supports that itself

I think 3 is not so bad and could be implemented relatively easily.

@hug-dev
Copy link
Member

hug-dev commented Apr 20, 2020

That also made me think that this crate we are going to create, it could/should be a psa-crypto-rust crate and not a mbed-crypto-rust one (Mbed Crypto being the default).

@ionut-arm
Copy link
Member Author

I think 3 is not so bad and could be implemented relatively easily.

It's true that it won't be a huge effort now, but it might end up requiring a lot of maintenance as new versions are released. It's true, though, that the same effort would be required for maintaining handwritten Rust code as well.

@hug-dev hug-dev changed the title Create a Mbed Crypto Rust wrapper crate Create a PSA Crypto Rust wrapper crate Apr 21, 2020
@hug-dev
Copy link
Member

hug-dev commented Apr 21, 2020

Renamed the issue as creating a crate that interfaces with PSA Crypto and not only Mbed Crypto is a goal of this.
This should link with Mbed Crypto by default though.

@gilles-peskine-arm
Copy link

rely on the Mbed Crypto implementation of psa_key_attributes_t as we do now. Our code will break over different versions of Mbed Crypto that modify the opaque structure.

Yes, your code will break. Don't do that.

find a way to compile Mbed Crypto forcing it to not inline the functions.

That does sound reasonable.

@egrimley-arm
Copy link
Collaborator

egrimley-arm commented Apr 22, 2020

I'm currently experimenting with this by splitting off part of Parsec into a sub-crate currently called mbed-crypto, though I'm trying to use just the official PSA Crypto API so that it's not Mbed-specific.

@hug-dev
Copy link
Member

hug-dev commented Apr 23, 2020

The repo has been created at https://github.com/parallaxsecond/rust-psa-crypto
Feel free to make a pull request with your changes!

@hug-dev
Copy link
Member

hug-dev commented Jun 2, 2020

The wrapper is now in a good shape! The next step is import psa-crypto to be used in the Mbed Crypto Provider. That will be done as part of #177

@hug-dev hug-dev closed this as completed Jun 2, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

4 participants