Separate implementation-defined feature in -sys #21

ionut-arm · 2020-06-02T10:33:49Z

This commit adds a feature that masks the implementation-specific
functionality in the -sys crate, while allowing the constants and types
to be accessible regardless of a library backing.

Signed-off-by: Ionut Mihalcea ionut.mihalcea@arm.com

hug-dev

Thanks, that is going to make things simpler 👏

I have a few comments and one request as well: we are starting to have a big range of features. Could you please add in the various README.md a description of the features and what they do? Could you also add a few lines in the CI script that test compilation for the different features combination?

hug-dev · 2020-06-02T10:39:08Z

psa-crypto-sys/build.rs

-    compile_shim_library(include.clone())?;
-    generate_mbed_crypto_bindings(include)?;
-    link_to_lib(lib, statically)?;
+    generate_mbed_crypto_bindings(include.clone())?;


I think this line (144) should also be under this if below. We would only generate the implementation-defined bindings from the header file of the implementation.

Tried that, didn't work - the constants depend on the shim bindings being generated, all of them are equal to shim_<something something> - plus, the header files are supposed to be implementation independent - we're not compiling or linking a library, simply pulling in the C-defined interface and generating bindings to it.

Tried that, didn't work - the constants depend on the shim bindings being generated, all of them are equal to shim_

Ideally, we would define the constants which values are indicated in the specification ourselves, the same way as we define the types ourselves. That way we do not have to rely on the vendor folder at all when generating an implementation-independant build. I did not check the context of the change in that file, but basically I think this whole build script should not be executed when the implementation-defined feature is not on: everything is self dependant. That way, we can also remove from the shimmed constants the ones that are specified.

the header files are supposed to be implementation independent - we're not compiling or linking a library, simply pulling in the C-defined interface and generating bindings to it.

Agree for the parts of the header that are defining the specified types and constants, but not everything. We could also chose to generate bindings all the time and only re-surface from them the types and constants (fron the shim) that are specified, but that seems less clean than re-defining them ourselves and not needing bindgen at all.

I think we've drifted off from how a -sys crate should work and made it non-intuitive. The purpose of this kind of crate is generally to expose the interface and link to the original library. If we just define everything here we're not really doing any of those two. Hence, I'd probably lean towards generating all the bindings and exporting the things we think are important, but having a link feature (that is enabled by default) and which, when turned off, leaves linking to the user. The rest of the build would work the same way.

So someone (e.g. ourselves) could still use the interface, without having to link with the lib.

It might be cumbersome to do it by hand but we can just copy the ones from one bindgen generation 😃

I think we've drifted off from how a -sys crate should work and made it non-intuitive. The purpose of this kind of crate is generally to expose the interface and link to the original library.

I disagree, we do exactly the same as before but instead of letting bindgen generate the constants and types, we define them ourselves. We can do that because they are specified and stable in the Crypto API. The constants and types we define can still be used for FFI interactions with a PSA Crypto implementation and so we are filling that objective.

Maybe the link name is better than implementation-defined. When it is not set, this crate only generates what it can from the specified types and constants of the API.

hug-dev · 2020-06-02T10:51:07Z

psa-crypto-sys/src/lib.rs

-}
+#[cfg(feature = "implementation-defined")]
+pub use psa_crypto_binding::{
+    psa_algorithm_t, psa_close_key, psa_crypto_init, psa_destroy_key, psa_dh_group_t,


I think the following types are not implementation defined:

psa_algorithm_t: defined as uint32_t

psa_dh_group_t: uint8_t

psa_ecc_curve_t: uint8_t

psa_key_id_t: uint32_t

psa_key_lifetime_t: uint32_t

psa_key_type_t: uint16_t

psa_key_usage_t: uint32_t

psa_status_t: int32_t

Moreover, some of these types are used in constants.rs but that module is not under implementation-defined.

I think these types could be removed from here and only put in types.rs.

Yeah, I didn't even notice that there were types made public, I thought they were all functions, oops 🤡

hug-dev · 2020-06-02T10:54:21Z

psa-crypto-sys/src/constants.rs

@@ -130,3 +130,4 @@ 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;
+pub const PSA_DRV_SE_HAL_VERSION: u32 = 5;


I think it is better to put this one under implementation-defined as well as SE interface is not yet in the specs.

hug-dev · 2020-06-02T10:58:08Z

psa-crypto/src/types/status.rs

@@ -165,7 +164,6 @@ impl From<Error> for Status {
    }
 }

-#[cfg(feature = "with-mbed-crypto")]


Removing all of these is cool 😎

hug-dev · 2020-06-02T11:00:53Z

psa-crypto-sys/src/types.rs

+// Copyright 2020 Contributors to the Parsec project.
+// SPDX-License-Identifier: Apache-2.0
+#![allow(non_camel_case_types)]
+


I think some of those types are not defined in the spec but just in Mbed Crypto. I think one should be able to find any of those types in the PSA Crypto document. For example psa_app_key_id_t is implementation defined I think.

I think we'll need a feature to cover the SE-related functionality too, but I won't add that now

hug-dev

Thanks! The changes look good to me.
Could you please add in the README.md files a description of the crates' features?

psa-crypto-sys:
static
implementation-defined

psa-crypto:
with-mbed-crypto
no-std

And also a few cargo build with only some features selected (or none) to make sure that compilation still work for them? Like:

cargo build -p psa-crypto-sys --no-default-features
cargo build -p psa-crypto --no-default-features
cargo build -p psa-crypto --no-default-features --features no-std
cargo build -p psa-crypto --no-default-features --features with-mbed-crypto

This commit adds a feature that masks the implementation-specific functionality in the -sys crate, while allowing the constants and types to be accessible regardless of a library backing. Signed-off-by: Ionut Mihalcea <ionut.mihalcea@arm.com>

hug-dev

let's merge!

ionut-arm added the enhancement New feature or request label Jun 2, 2020

ionut-arm requested a review from hug-dev June 2, 2020 10:33

ionut-arm self-assigned this Jun 2, 2020

hug-dev reviewed Jun 2, 2020

View reviewed changes

ionut-arm force-pushed the impl-defined branch from 440be58 to e3e243b Compare June 2, 2020 15:15

hug-dev reviewed Jun 3, 2020

View reviewed changes

ionut-arm force-pushed the impl-defined branch from e3e243b to 0646ebd Compare June 3, 2020 10:29

ionut-arm force-pushed the impl-defined branch from 0646ebd to 1cb58c7 Compare June 3, 2020 10:40

hug-dev approved these changes Jun 3, 2020

View reviewed changes

ionut-arm merged commit 676e91c into parallaxsecond:master Jun 3, 2020

ionut-arm deleted the impl-defined branch June 3, 2020 10:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Separate implementation-defined feature in -sys #21

Separate implementation-defined feature in -sys #21

ionut-arm commented Jun 2, 2020

hug-dev left a comment

hug-dev Jun 2, 2020

ionut-arm Jun 2, 2020

hug-dev Jun 2, 2020

ionut-arm Jun 2, 2020 •

edited

Loading

hug-dev Jun 2, 2020

hug-dev Jun 2, 2020

hug-dev Jun 2, 2020

hug-dev Jun 2, 2020

ionut-arm Jun 2, 2020

hug-dev Jun 2, 2020

hug-dev Jun 2, 2020

hug-dev Jun 2, 2020

ionut-arm Jun 2, 2020

hug-dev left a comment

hug-dev left a comment

Separate implementation-defined feature in -sys #21

Separate implementation-defined feature in -sys #21

Conversation

ionut-arm commented Jun 2, 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

ionut-arm Jun 2, 2020 • edited Loading

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

hug-dev left a comment

Choose a reason for hiding this comment

hug-dev left a comment

Choose a reason for hiding this comment

ionut-arm Jun 2, 2020 •

edited

Loading