First draft of rust-psa-crypto, Rust wrapper for (some of) mbed-crypto. #5

egrimley-arm · 2020-04-28T11:13:11Z

Currently this depends on an old version of mbed-crypto.
These files are adapted from Parsec:

build-conf.toml, build.rs, setup_mbed_crypto.sh

hug-dev · 2020-04-28T11:21:43Z

You can execute the CI tests with ./tests/ci.sh to check what failed.
In that case cargo fmt might help to format all of your files!

egrimley-arm · 2020-04-28T11:25:07Z

You can execute the CI tests with ./tests/ci.sh to check what failed.
In that case cargo fmt might help to format all of your files!

Can I just do "git commit", "git push" to update this PR?

hug-dev · 2020-04-28T11:28:21Z

Can I just do "git commit", "git push" to update this PR?

Yes absolutely!
You can also do:

git commit --amend
git push -f your_fork_remote_name first

if you want to keep the history clean but we do not mind at all

hug-dev · 2020-04-28T11:56:49Z

Seems like it failed on some clippy checks. You can see what's wrong with ./tests/ci.sh.

Currently this depends on an old version of mbed-crypto. These files are adapted from Parsec: build-conf.toml, build.rs, setup_mbed_crypto.sh Signed-off-by: Edmund Grimley Evans <[email protected]>

egrimley-arm · 2020-04-28T13:00:28Z

I can't reproduce the remaining failure: error[E0013]: constants cannot refer to statics

Does it require a different version of Rust?

hug-dev · 2020-04-28T13:20:35Z

I tried locally with the same version of Rust Travis CI uses (1.43.0) and I do not get any error...
The difference between GitHub Actions and Travis CI is that Travis builds natively on a Aarch64 machine.
The error might happen if bindgen for Aarch64 generates the shim constants to be static instead of const.
Locally I have:

pub const shim_PSA_SUCCESS: psa_status_t = 0;

in shim_bindings.rs but maybe it is something like:

pub static shim_PSA_SUCCESS: psa_status_t = 0;

on Aarch64?

hug-dev · 2020-04-28T13:28:37Z

See this issue. In the prelude of the job it says:

clang version 3.8.0-2ubuntu4 (tags/RELEASE_380/final)

That might be the problem.

hug-dev · 2020-04-28T13:34:05Z

Ok try this to switch to a more recent Ubuntu version.
Try to apply the following patch and send your changes again:

diff --git a/.travis.yml b/.travis.yml
index 4f5fdb6..85d84e3 100644
--- a/.travis.yml
+++ b/.travis.yml
@@ -1,5 +1,6 @@
 # Executing our tests on Arm64 with Travis CI
 arch: arm64
+dist: bionic
 language: rust
 script:
   - ./tests/ci.sh

Signed-off-by: Edmund Grimley Evans <[email protected]>

egrimley-arm · 2020-04-28T14:08:19Z

That seemed to fix it.

hug-dev

Thanks for the first PR!!
I made some comments. Some I think should be addressed in this PR but the ones marked "ignore" are general comments and can be ignored in this PR. I am happy to have this PR merged without the ignore comments but I think they need to be discussed and probably implemented in other PRs.

hug-dev · 2020-04-29T10:28:26Z

src/constants.rs

+pub const PSA_KEY_TYPE_ECC_PUBLIC_KEY_BASE: psa_key_type_t = shim_PSA_KEY_TYPE_ECC_PUBLIC_KEY_BASE;
+pub const PSA_KEY_TYPE_ECC_KEY_PAIR_BASE: psa_key_type_t = shim_PSA_KEY_TYPE_ECC_KEY_PAIR_BASE;
+pub const PSA_KEY_TYPE_ECC_CURVE_MASK: psa_key_type_t = shim_PSA_KEY_TYPE_ECC_CURVE_MASK;
+pub const PSA_ECC_CURVE_SECT163K1: psa_ecc_curve_t = shim_PSA_ECC_CURVE_SECT163K1;
+pub const PSA_ECC_CURVE_SECT163R1: psa_ecc_curve_t = shim_PSA_ECC_CURVE_SECT163R1;
+pub const PSA_ECC_CURVE_SECT163R2: psa_ecc_curve_t = shim_PSA_ECC_CURVE_SECT163R2;
+pub const PSA_ECC_CURVE_SECT193R1: psa_ecc_curve_t = shim_PSA_ECC_CURVE_SECT193R1;
+pub const PSA_ECC_CURVE_SECT193R2: psa_ecc_curve_t = shim_PSA_ECC_CURVE_SECT193R2;
+pub const PSA_ECC_CURVE_SECT233K1: psa_ecc_curve_t = shim_PSA_ECC_CURVE_SECT233K1;
+pub const PSA_ECC_CURVE_SECT233R1: psa_ecc_curve_t = shim_PSA_ECC_CURVE_SECT233R1;
+pub const PSA_ECC_CURVE_SECT239K1: psa_ecc_curve_t = shim_PSA_ECC_CURVE_SECT239K1;
+pub const PSA_ECC_CURVE_SECT283K1: psa_ecc_curve_t = shim_PSA_ECC_CURVE_SECT283K1;
+pub const PSA_ECC_CURVE_SECT283R1: psa_ecc_curve_t = shim_PSA_ECC_CURVE_SECT283R1;
+pub const PSA_ECC_CURVE_SECT409K1: psa_ecc_curve_t = shim_PSA_ECC_CURVE_SECT409K1;
+pub const PSA_ECC_CURVE_SECT409R1: psa_ecc_curve_t = shim_PSA_ECC_CURVE_SECT409R1;
+pub const PSA_ECC_CURVE_SECT571K1: psa_ecc_curve_t = shim_PSA_ECC_CURVE_SECT571K1;
+pub const PSA_ECC_CURVE_SECT571R1: psa_ecc_curve_t = shim_PSA_ECC_CURVE_SECT571R1;
+pub const PSA_ECC_CURVE_SECP160K1: psa_ecc_curve_t = shim_PSA_ECC_CURVE_SECP160K1;
+pub const PSA_ECC_CURVE_SECP160R1: psa_ecc_curve_t = shim_PSA_ECC_CURVE_SECP160R1;
+pub const PSA_ECC_CURVE_SECP160R2: psa_ecc_curve_t = shim_PSA_ECC_CURVE_SECP160R2;
+pub const PSA_ECC_CURVE_SECP192K1: psa_ecc_curve_t = shim_PSA_ECC_CURVE_SECP192K1;
+pub const PSA_ECC_CURVE_SECP192R1: psa_ecc_curve_t = shim_PSA_ECC_CURVE_SECP192R1;
+pub const PSA_ECC_CURVE_SECP224K1: psa_ecc_curve_t = shim_PSA_ECC_CURVE_SECP224K1;
+pub const PSA_ECC_CURVE_SECP224R1: psa_ecc_curve_t = shim_PSA_ECC_CURVE_SECP224R1;
+pub const PSA_ECC_CURVE_SECP256K1: psa_ecc_curve_t = shim_PSA_ECC_CURVE_SECP256K1;
+pub const PSA_ECC_CURVE_SECP256R1: psa_ecc_curve_t = shim_PSA_ECC_CURVE_SECP256R1;
+pub const PSA_ECC_CURVE_SECP384R1: psa_ecc_curve_t = shim_PSA_ECC_CURVE_SECP384R1;
+pub const PSA_ECC_CURVE_SECP521R1: psa_ecc_curve_t = shim_PSA_ECC_CURVE_SECP521R1;
+pub const PSA_ECC_CURVE_BRAINPOOL_P256R1: psa_ecc_curve_t = shim_PSA_ECC_CURVE_BRAINPOOL_P256R1;
+pub const PSA_ECC_CURVE_BRAINPOOL_P384R1: psa_ecc_curve_t = shim_PSA_ECC_CURVE_BRAINPOOL_P384R1;
+pub const PSA_ECC_CURVE_BRAINPOOL_P512R1: psa_ecc_curve_t = shim_PSA_ECC_CURVE_BRAINPOOL_P512R1;
+pub const PSA_ECC_CURVE_CURVE25519: psa_ecc_curve_t = shim_PSA_ECC_CURVE_CURVE25519;
+pub const PSA_ECC_CURVE_CURVE448: psa_ecc_curve_t = shim_PSA_ECC_CURVE_CURVE448;
+pub const PSA_ALG_VENDOR_FLAG: psa_algorithm_t = shim_PSA_ALG_VENDOR_FLAG;
+pub const PSA_ALG_CATEGORY_MASK: psa_algorithm_t = shim_PSA_ALG_CATEGORY_MASK;
+pub const PSA_ALG_CATEGORY_HASH: psa_algorithm_t = shim_PSA_ALG_CATEGORY_HASH;
+pub const PSA_ALG_CATEGORY_MAC: psa_algorithm_t = shim_PSA_ALG_CATEGORY_MAC;
+pub const PSA_ALG_CATEGORY_CIPHER: psa_algorithm_t = shim_PSA_ALG_CATEGORY_CIPHER;
+pub const PSA_ALG_CATEGORY_AEAD: psa_algorithm_t = shim_PSA_ALG_CATEGORY_AEAD;
+pub const PSA_ALG_CATEGORY_SIGN: psa_algorithm_t = shim_PSA_ALG_CATEGORY_SIGN;
+pub const PSA_ALG_CATEGORY_ASYMMETRIC_ENCRYPTION: psa_algorithm_t =
+    shim_PSA_ALG_CATEGORY_ASYMMETRIC_ENCRYPTION;
+pub const PSA_ALG_CATEGORY_KEY_AGREEMENT: psa_algorithm_t = shim_PSA_ALG_CATEGORY_KEY_AGREEMENT;
+pub const PSA_ALG_CATEGORY_KEY_DERIVATION: psa_algorithm_t = shim_PSA_ALG_CATEGORY_KEY_DERIVATION;
+pub const PSA_ALG_HASH_MASK: psa_algorithm_t = shim_PSA_ALG_HASH_MASK;
+pub const PSA_ALG_MD2: psa_algorithm_t = shim_PSA_ALG_MD2;
+pub const PSA_ALG_MD4: psa_algorithm_t = shim_PSA_ALG_MD4;
+pub const PSA_ALG_MD5: psa_algorithm_t = shim_PSA_ALG_MD5;
+pub const PSA_ALG_RIPEMD160: psa_algorithm_t = shim_PSA_ALG_RIPEMD160;
+pub const PSA_ALG_SHA_1: psa_algorithm_t = shim_PSA_ALG_SHA_1;
+pub const PSA_ALG_SHA_224: psa_algorithm_t = shim_PSA_ALG_SHA_224;
+pub const PSA_ALG_SHA_256: psa_algorithm_t = shim_PSA_ALG_SHA_256;
+pub const PSA_ALG_SHA_384: psa_algorithm_t = shim_PSA_ALG_SHA_384;
+pub const PSA_ALG_SHA_512: psa_algorithm_t = shim_PSA_ALG_SHA_512;
+pub const PSA_ALG_SHA_512_224: psa_algorithm_t = shim_PSA_ALG_SHA_512_224;
+pub const PSA_ALG_SHA_512_256: psa_algorithm_t = shim_PSA_ALG_SHA_512_256;
+pub const PSA_ALG_SHA3_224: psa_algorithm_t = shim_PSA_ALG_SHA3_224;
+pub const PSA_ALG_SHA3_256: psa_algorithm_t = shim_PSA_ALG_SHA3_256;
+pub const PSA_ALG_SHA3_384: psa_algorithm_t = shim_PSA_ALG_SHA3_384;
+pub const PSA_ALG_SHA3_512: psa_algorithm_t = shim_PSA_ALG_SHA3_512;
+pub const PSA_ALG_ANY_HASH: psa_algorithm_t = shim_PSA_ALG_ANY_HASH;
+pub const PSA_ALG_MAC_SUBCATEGORY_MASK: psa_algorithm_t = shim_PSA_ALG_MAC_SUBCATEGORY_MASK;
+pub const PSA_ALG_HMAC_BASE: psa_algorithm_t = shim_PSA_ALG_HMAC_BASE;
+pub const PSA_ALG_MAC_TRUNCATION_MASK: psa_algorithm_t = shim_PSA_ALG_MAC_TRUNCATION_MASK;
+pub const PSA_ALG_CIPHER_MAC_BASE: psa_algorithm_t = shim_PSA_ALG_CIPHER_MAC_BASE;
+pub const PSA_ALG_CBC_MAC: psa_algorithm_t = shim_PSA_ALG_CBC_MAC;
+pub const PSA_ALG_CMAC: psa_algorithm_t = shim_PSA_ALG_CMAC;
+pub const PSA_ALG_CIPHER_STREAM_FLAG: psa_algorithm_t = shim_PSA_ALG_CIPHER_STREAM_FLAG;
+pub const PSA_ALG_CIPHER_FROM_BLOCK_FLAG: psa_algorithm_t = shim_PSA_ALG_CIPHER_FROM_BLOCK_FLAG;
+pub const PSA_ALG_ARC4: psa_algorithm_t = shim_PSA_ALG_ARC4;
+pub const PSA_ALG_CTR: psa_algorithm_t = shim_PSA_ALG_CTR;
+pub const PSA_ALG_CFB: psa_algorithm_t = shim_PSA_ALG_CFB;
+pub const PSA_ALG_OFB: psa_algorithm_t = shim_PSA_ALG_OFB;
+pub const PSA_ALG_XTS: psa_algorithm_t = shim_PSA_ALG_XTS;
+pub const PSA_ALG_CBC_NO_PADDING: psa_algorithm_t = shim_PSA_ALG_CBC_NO_PADDING;
+pub const PSA_ALG_CBC_PKCS7: psa_algorithm_t = shim_PSA_ALG_CBC_PKCS7;
+pub const PSA_ALG_CCM: psa_algorithm_t = shim_PSA_ALG_CCM;
+pub const PSA_ALG_GCM: psa_algorithm_t = shim_PSA_ALG_GCM;
+pub const PSA_ALG_AEAD_TAG_LENGTH_MASK: psa_algorithm_t = shim_PSA_ALG_AEAD_TAG_LENGTH_MASK;
+pub const PSA_ALG_RSA_PKCS1V15_SIGN_BASE: psa_algorithm_t = shim_PSA_ALG_RSA_PKCS1V15_SIGN_BASE;
+pub const PSA_ALG_RSA_PSS_BASE: psa_algorithm_t = shim_PSA_ALG_RSA_PSS_BASE;
+pub const PSA_ALG_DSA_BASE: psa_algorithm_t = shim_PSA_ALG_DSA_BASE;
+pub const PSA_ALG_DETERMINISTIC_DSA_BASE: psa_algorithm_t = shim_PSA_ALG_DETERMINISTIC_DSA_BASE;
+pub const PSA_ALG_DSA_DETERMINISTIC_FLAG: psa_algorithm_t = shim_PSA_ALG_DSA_DETERMINISTIC_FLAG;
+pub const PSA_ALG_ECDSA_BASE: psa_algorithm_t = shim_PSA_ALG_ECDSA_BASE;
+pub const PSA_ALG_DETERMINISTIC_ECDSA_BASE: psa_algorithm_t = shim_PSA_ALG_DETERMINISTIC_ECDSA_BASE;
+pub const PSA_ALG_RSA_PKCS1V15_CRYPT: psa_algorithm_t = shim_PSA_ALG_RSA_PKCS1V15_CRYPT;
+pub const PSA_ALG_RSA_OAEP_BASE: psa_algorithm_t = shim_PSA_ALG_RSA_OAEP_BASE;
+pub const PSA_ALG_HKDF_BASE: psa_algorithm_t = shim_PSA_ALG_HKDF_BASE;
+pub const PSA_ALG_TLS12_PRF_BASE: psa_algorithm_t = shim_PSA_ALG_TLS12_PRF_BASE;
+pub const PSA_ALG_TLS12_PSK_TO_MS_BASE: psa_algorithm_t = shim_PSA_ALG_TLS12_PSK_TO_MS_BASE;
+pub const PSA_ALG_KEY_DERIVATION_MASK: psa_algorithm_t = shim_PSA_ALG_KEY_DERIVATION_MASK;
+pub const PSA_KEY_LIFETIME_VOLATILE: psa_key_lifetime_t = shim_PSA_KEY_LIFETIME_VOLATILE;
+pub const PSA_KEY_LIFETIME_PERSISTENT: psa_key_lifetime_t = shim_PSA_KEY_LIFETIME_PERSISTENT;
+pub const PSA_KEY_USAGE_EXPORT: psa_key_usage_t = shim_PSA_KEY_USAGE_EXPORT;
+pub const PSA_KEY_USAGE_ENCRYPT: psa_key_usage_t = shim_PSA_KEY_USAGE_ENCRYPT;
+pub const PSA_KEY_USAGE_DECRYPT: psa_key_usage_t = shim_PSA_KEY_USAGE_DECRYPT;
+pub const PSA_KEY_USAGE_SIGN: psa_key_usage_t = shim_PSA_KEY_USAGE_SIGN;
+pub const PSA_KEY_USAGE_VERIFY: psa_key_usage_t = shim_PSA_KEY_USAGE_VERIFY;
+pub const PSA_KEY_USAGE_DERIVE: psa_key_usage_t = shim_PSA_KEY_USAGE_DERIVE;


Maybe we can directly access the const generated by bindgen?

If so and as we are not using PSA_KEY_SLOT_COUNT and PSA_MAX_PERSISTENT_KEY_IDENTIFIER, we could even delete this file.

I can't immediately see how to do that. Processing the psa/crypto.h header file with bindgen gives you nothing for PSA_SUCCESS and many other symbols (rust-lang/rust-bindgen#316). So you need to write some C code that uses the preprocessor symbols. That C code includes psa/crypto.h, so processing it with bindgen gives you a load of stuff that you don't want to make public (a limitation of the C preprocessor). So you need a list of the symbols that you do want to make public. Moreover, the symbol in your C code can't be the same as the psa/crypto.h symbol (another limitation of the C preprocessor). So if you want the Rust library to export a symbol with the same name as in psa/crypto.h you end up with something similar to what I've done.

Or is there a totally different approach? Or a bindgen option that saves us? (There are various --whitelist-... options, for example, which might solve one of the problems mentioned above.)

I can't immediately see how to do that. Processing the psa/crypto.h header file with bindgen gives you nothing for PSA_SUCCESS and many other symbols (rust-lang/rust-bindgen#316). So you need to write some C code that uses the preprocessor symbols.

I agree with that.

So you need to write some C code that uses the preprocessor symbols. That C code includes psa/crypto.h, so processing it with bindgen gives you a load of stuff that you don't want to make public (a limitation of the C preprocessor).

Oh I see what you mean now: we only want to make public the useful symbols of bindgen. Doing that also ensure us that those symbols are stable across crate versions.

I think my point came from similar ideas expressed in other comments, if we want this crate to be more idiomatic (in my idea of what is idiomatic), we would replace those value with our own constructs and use the bindgen values there in conversion functions.
For example we might create a Hash enumeration which contains all the hash algorithm values. Having a defined type for that help restrict some functions to only take a Hash as a parameter. Similarly with other values found there.
We did a lot of this work in Parsec interface and I am sure we can reuse a lot of it.

hug-dev · 2020-04-29T10:33:37Z

src/c/shim.c

+// Copyright 2020 Contributors to the Parsec project.
+// SPDX-License-Identifier: Apache-2.0
+
+#include "shim.h"


A comment would be nice here to explain why do we need this file, maybe link to this issue as well.

hug-dev · 2020-04-29T11:50:51Z

src/lib.rs

+impl Global {
+    fn new() -> Global {
+        Global {
+            init_succeeded: false,
+            key_mutex: Mutex::new(()),
+        }
+    }
+}
+
+lazy_static! {
+    static ref GLOBAL: Mutex<Global> = Mutex::new(Global::new());
+}
+
+fn init() -> bool {
+    if GLOBAL.lock().unwrap().init_succeeded {
+        return true;
+    }
+    let status = unsafe { psa_crypto_binding::psa_crypto_init() };
+    if status != PSA_SUCCESS {
+        return false;
+    }
+    GLOBAL.lock().unwrap().init_succeeded = true;
+    true
+}
+
+macro_rules! wrap_any {
+    ($x:expr) => {
+        if !init() {
+            panic!("Error when initialising PSA Crypto")
+        } else {
+            #[allow(unused_unsafe)]
+            unsafe {
+                $x
+            }
+        }
+    };
+}
+
+macro_rules! wrap_status {
+    ($x:expr) => {
+        if !init() {
+            PSA_ERROR_BAD_STATE
+        } else {
+            unsafe { $x }
+        }
+    };
+}
+
+macro_rules! key_lock {
+    ($x:expr) => {{
+        let mutex = &GLOBAL.lock().unwrap().key_mutex;
+        let _guard = mutex.lock().unwrap();
+        $x
+    }};
+}


ignore

I am not a Rust expert but I think the class + methods paradigm would be more idiomatic (and efficient) in this case.
You could have:

struct PsaCrypto { // Calls to psa_open_key, psa_generate_key and psa_destroy_key // are not thread safe. We work around this bug with the key_mutex. // Mbed issue: https://github.com/ARMmbed/mbed-crypto/issues/266 key_mutex: Mutex<()>, }

A new static method could initialise the library and returns a PsaCrypto instance and the other PSA Crypto operations would be implemented as methods on this struct.

That way you don't need to check and to lock a mutex everytime you want to call an operation as this is checked statically.

This is worth discussing, but what you've suggested could be misleading if there can only be one instance of this structure. What I've done is quite likely not idiomatic Rust, but perhaps a wrapper for a C library should not be expected to look like idiomatic Rust.

(Turning PSA Crypto's single global space of key IDs into a separate space for each application is Parsec's job, right?)

(The mutex is only there to work around a bug in some versions of Mbed Crypto. It would seem wrong to optimise the Rust wrapper's API for working around a bug that will eventually disappear. I don't know what the plans are for the initialisation requirement. If that were to be removed then there would be no need for the mutex or any static data in the Rust wrapper, I think.)

This is worth discussing, but what you've suggested could be misleading if there can only be one instance of this structure.

Good point! We can explore options how to create singletons. Having one instance of PsaCrypto would allow you to leverage the Rust borrow-checker to have thread-safety without a mutex.

perhaps a wrapper for a C library should not be expected to look like idiomatic Rust

I tend to disagree with that point. I think Rust developpers consuming this crate will expect it to follow the usual Rust patterns. In the end "idiomatic" just means easy to use in a Rust context: safe and convenient abstractions. I do not think we should force the C style on Rust.

(Turning PSA Crypto's single global space of key IDs into a separate space for each application is Parsec's job, right?)

That is correct! By "abstraction" I do not mean to achieve that. I do think that we should provide the exact same features/functionalities than PSA Crypto with the same semantics. Still use Key ID. By abstraction I mean using the high-level features of Rust: stricter types with methods being the most important one for a Rust to C wrapper in my opinion.

(The mutex is only there to work around a bug in some versions of Mbed Crypto. It would seem wrong to optimise the Rust wrapper's API for working around a bug that will eventually disappear. I don't know what the plans are for the initialisation requirement. If that were to be removed then there would be no need for the mutex or any static data in the Rust wrapper, I think.)

In this message I meant the mutex that is needed around the GLOBAL structure.

Seems this hasn't been mentioned yet: std::sync::Once; Not necessarily saying we should use it here, it just looks useful/aligned with the task at hand

Also, while I'm not a fan of the "panic if used twice" pattern, this page has some good points about having the compiler do some of the sanity checks for you.

hug-dev · 2020-04-29T11:52:24Z

src/lib.rs

+pub fn psa_get_key_bits(attributes: &psa_key_attributes_t) -> usize {
+    wrap_any!((*attributes).x.core.bits as usize)
+}
+
+pub fn psa_get_key_type(attributes: &psa_key_attributes_t) -> psa_key_type_t {
+    wrap_any!((*attributes).x.core.type_)
+}


Could those two getters be defined in the shim as well?

Yes, they should be. Done.

hug-dev · 2020-04-29T12:01:08Z

src/lib.rs

+pub fn psa_import_key(
+    attributes: *const psa_key_attributes_t,
+    data: *const u8,
+    data_length: usize,
+    handle: *mut psa_key_handle_t,
+) -> psa_status_t {


ignore

I think we ought to aim to not have any unsafe/non-idiomatic types in our public API. In particular:

a Rust abstraction for psa_key_attributes_t and psa_algorithm_t. The setters/getters on the key attributes could even be implemented as methods on the new type we create. See Create abstractions of key attributes and algorithms #2

in our APIs, instead of having for buffer a raw pointer (*const u8) and a size, have a Vec<u8> and use methods on it to convert it to what the C API expects. See Create the Rust representation of PSA Crypto operations #3

have all our operations return a Result, which contains the data that the operation returns (or () if nothing) or an error (a psa_status_t, abstracted) if there was an error

All worth discussing. Currently I have wrapped psa_key_attributes_t, so its internals are hidden, I think, but I haven't wrapped psa_algorithm_t, which is therefore visible to the user of the Rust wrapper as a u32 (with the current Mbed implementation).

ionut-arm

I think this is good, we can continue making changes on top of it!

ionut-arm · 2020-04-29T12:25:44Z

src/lib.rs

+#[allow(non_camel_case_types)]
+pub struct psa_key_attributes_t {
+    x: psa_crypto_binding::psa_key_attributes_t,
+}


This is ok as is for now, but we'll have to decide if we want to keep the naming convention from the C spec or if we want to make this truly Rust-native and use the camel case naming convention.

Also, if you're not interested in having a proper name for the internal value, you can simply do

pub struct psa_key_attributes_t(psa_crypto_binding::psa_key_attributes_t);

and access it through self.0

Converting names to the Rust naming convention seems like a good idea to me. Anyone else in favour or opposed?

I think it is a good idea too but maybe in a later PR where we "rustify" other things as well?

Signed-off-by: Edmund Grimley Evans <[email protected]>

hug-dev · 2020-04-30T09:53:14Z

I think this is good to be merged as it is. We can continue discussing the different points and add more PRs on top of that if we agree.

hug-dev · 2020-05-04T10:05:52Z

Hi @egrimley-arm
Would you be ok if we merge this?

I think I am going to work on this crate as well, as part of parallaxsecond/parsec#128, as I need to convert key attributes between the FFI types and the Rust native types we use in Parsec.

What I think the next steps could be:

remove compilation of Mbed Crypto in the build phase and the static linking to instead dynamically link with libmbedcrypto.so. I think we can expect the library and header files to be at the default locations like /usr/local/lib. This is what the install step of Mbed TLS is currently doing
split the code in layers:
- a FFI layer (src/ffi/mod.rs) containing all the APIs with all bindgen types, not abstracted. The shim functions as well. Most of the functions should be unsafe. It would also contain the constants not generated by bindgen. This layer is not supposed to be idiomatic or safe but it will be stable with the API version that we target.
- a core layer (src/core/mod.rs) which contains the abstraction: high-level idiomatic types, thread safe methods, initialization handling. This will be expected to be safe and idiomatic Rust. It would also contain conversion methods for the high-level structures from the FFI ones and the other way around.
add simple integration tests for both levels

egrimley-arm · 2020-05-04T10:29:43Z

I'd be happy to see this merged.

Is there any point in keeping static linking as an alternative? Or too much work to maintain?

Having two layers sounds sensible. There might be some choices to make about what belongs in which layer. For example, making the methods thread-safe is a bug workaround, as I understand it, which could arguably belong in the lower layer, closer to where the bug is.

Is the intention to make the abstraction layer independent of the PSA Crypto API version? I haven't looked at the PSA (draft) APIs hard enough to know whether that's feasible.

hug-dev · 2020-05-04T13:44:54Z

Merged it as a base to work on.

Is there any point in keeping static linking as an alternative? Or too much work to maintain?

I think you are right and that should be the case. I actually started reading what was the common way of providing Rust wrappers around C libraries. This and this are very good read and I think we should align to what they describe. There are good example of this in the Mbed TLS crate and this one.
That would mean splitting this repository into two crates:

psa-crypto-sys containing the bindings, constants and shims
psa-crypto containing the abstraction

For example, making the methods thread-safe is a bug workaround, as I understand it, which could arguably belong in the lower layer, closer to where the bug is.

We will have to check if the bug is still there with the version 1.0 of the API. Also, I do not know if we should fix a bug that is inside one implementation of the API and not the specification. I would say that as long as we respect the conditions described by the API which make its use memory and thread safe, then we are fine.

Is the intention to make the abstraction layer independent of the PSA Crypto API version? I haven't looked at the PSA (draft) APIs hard enough to know whether that's feasible.

I think it will still be targeted at a specific API version. The abstraction I have in mind is just a systematic translation of the specification into Rust idiomatic types so it should pretty straightforward to do. Maybe there could be layers on top of that which give even more abstractions and would be independant of PSA Crypto API version but I am not sure it would be the goal of this crate.

egrimley-arm force-pushed the first branch from e6550b8 to 218063b Compare April 28, 2020 11:15

hug-dev requested review from hug-dev and ionut-arm April 28, 2020 11:18

hug-dev assigned egrimley-arm Apr 28, 2020

hug-dev added the enhancement New feature or request label Apr 28, 2020

egrimley-arm force-pushed the first branch from 218063b to 5f48609 Compare April 28, 2020 11:32

First draft of rust-psa-crypto, Rust wrapper for (some of) mbed-crypto.

1e63cf6

Currently this depends on an old version of mbed-crypto. These files are adapted from Parsec: build-conf.toml, build.rs, setup_mbed_crypto.sh Signed-off-by: Edmund Grimley Evans <[email protected]>

egrimley-arm force-pushed the first branch from 5f48609 to 1e63cf6 Compare April 28, 2020 12:23

.travis.yml: Add "dist: bionic".

9e1579e

Signed-off-by: Edmund Grimley Evans <[email protected]>

egrimley-arm force-pushed the first branch from 33115d4 to 9e1579e Compare April 28, 2020 14:02

hug-dev reviewed Apr 29, 2020

View reviewed changes

ionut-arm reviewed Apr 29, 2020

View reviewed changes

Use shims for two getters, and add a comment.

7fabe3d

Signed-off-by: Edmund Grimley Evans <[email protected]>

egrimley-arm force-pushed the first branch from 70d4017 to 7fabe3d Compare April 29, 2020 17:17

hug-dev merged commit ec25c97 into parallaxsecond:master May 4, 2020

egrimley-arm deleted the first branch May 6, 2020 15:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

First draft of rust-psa-crypto, Rust wrapper for (some of) mbed-crypto. #5

First draft of rust-psa-crypto, Rust wrapper for (some of) mbed-crypto. #5

egrimley-arm commented Apr 28, 2020

hug-dev commented Apr 28, 2020

egrimley-arm commented Apr 28, 2020

hug-dev commented Apr 28, 2020

hug-dev commented Apr 28, 2020

egrimley-arm commented Apr 28, 2020

hug-dev commented Apr 28, 2020

hug-dev commented Apr 28, 2020

hug-dev commented Apr 28, 2020

egrimley-arm commented Apr 28, 2020

hug-dev left a comment

hug-dev Apr 29, 2020

hug-dev Apr 29, 2020

egrimley-arm Apr 29, 2020

hug-dev Apr 30, 2020

hug-dev Apr 29, 2020

egrimley-arm Apr 29, 2020

hug-dev Apr 29, 2020

egrimley-arm Apr 29, 2020

hug-dev Apr 30, 2020

ionut-arm Apr 30, 2020

hug-dev Apr 29, 2020

egrimley-arm Apr 29, 2020

hug-dev Apr 29, 2020

egrimley-arm Apr 29, 2020

ionut-arm left a comment

ionut-arm Apr 29, 2020

egrimley-arm Apr 29, 2020

hug-dev Apr 30, 2020

hug-dev commented Apr 30, 2020

hug-dev commented May 4, 2020

egrimley-arm commented May 4, 2020

hug-dev commented May 4, 2020

First draft of rust-psa-crypto, Rust wrapper for (some of) mbed-crypto. #5

First draft of rust-psa-crypto, Rust wrapper for (some of) mbed-crypto. #5

Conversation

egrimley-arm commented Apr 28, 2020

hug-dev commented Apr 28, 2020

egrimley-arm commented Apr 28, 2020

hug-dev commented Apr 28, 2020

hug-dev commented Apr 28, 2020

egrimley-arm commented Apr 28, 2020

hug-dev commented Apr 28, 2020

hug-dev commented Apr 28, 2020

hug-dev commented Apr 28, 2020

egrimley-arm commented Apr 28, 2020

hug-dev left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ionut-arm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hug-dev commented Apr 30, 2020

hug-dev commented May 4, 2020

egrimley-arm commented May 4, 2020

hug-dev commented May 4, 2020