gh-ARMmbed/mbed-crypto

Watch

Fork 0

Make PSA Crypto structs actually opaque #4

Closed

Patater wants to merge 6 commits from real-opaque-structs into development

Patater commented

2018-12-21 12:06:32 -06:00

(Migrated from github.com)

Allow users of the Mbed Crypto library (perhaps as a shared library) to not have to know details about which context structs or other implementation details to know the size of the structs.

This is done by splitting the externally facing crypto structs into opaque structures with reserved sizes and implementation structures exposed only internally for use by the library implementation.

Design principles:

The implementation wants freedom to change the PSA Crypto struct contents between versions of the library
Users of the library should be able to use static memory allocation, avoiding malloc
- This requires that the size of the PSA crypto structs be known at compile time. Run-time queries for a size to allocate dynamically aren't a workable solution.
Shrinking the size of the implementation structs should not require a recompile of a user application.
If implementation structs grow larger than the opaque structs, that's an ABI break but not an API break (requires a recompile of user applications).
The size of the structs can vary based on the configuration chosen (e.g. if only MD5 is used, and no other hashes, the hash context implementation struct should be only as big as MD5, not the max of all possible hash contexts.
- This means that the opaque structs exposed by the external struct header are an upper bound on the size. This is unfortunate, as it could waste a bit of RAM in certain configurations, but we don't yet see a way to make the opaque structs take up the minimum necessary RAM without exposing more detail about the implementation structs.

This builds on #2 for static assertions, which we use to ensure that the opaque structures have enough room to hold the implementation structs.

XXX The opaque structs are only partially opaque. We may want to make them 100% opaque, which will require reworking this PR.

Allow users of the Mbed Crypto library (perhaps as a shared library) to not have to know details about which context structs or other implementation details to know the size of the structs. This is done by splitting the externally facing crypto structs into opaque structures with reserved sizes and implementation structures exposed only internally for use by the library implementation. Design principles: - The implementation wants freedom to change the PSA Crypto struct contents between versions of the library - Users of the library should be able to use static memory allocation, avoiding malloc - This requires that the size of the PSA crypto structs be known at compile time. Run-time queries for a size to allocate dynamically aren't a workable solution. - Shrinking the size of the implementation structs should not require a recompile of a user application. - If implementation structs grow larger than the opaque structs, that's an ABI break but not an API break (requires a recompile of user applications). - The size of the structs can vary based on the configuration chosen (e.g. if only MD5 is used, and no other hashes, the hash context implementation struct should be only as big as MD5, not the max of all possible hash contexts. - This means that the opaque structs exposed by the external struct header are an upper bound on the size. This is unfortunate, as it could waste a bit of RAM in certain configurations, but we don't yet see a way to make the opaque structs take up the minimum necessary RAM without exposing more detail about the implementation structs. This builds on #2 for static assertions, which we use to ensure that the opaque structures have enough room to hold the implementation structs. XXX The opaque structs are only partially opaque. We may want to make them 100% opaque, which will require reworking this PR.

gilles-peskine-arm (Migrated from github.com) reviewed

2018-12-21 12:28:18 -06:00

include/psa/crypto_struct.h

Blocker: the fact that the RAM size depends on the configuration is a feature, not a bug. We do want to save RAM when certain algorithms are not used. For example, when people select SHA256 over SHA512, they expect to save RAM.

Blocker: the fact that the RAM size depends on the configuration is a feature, not a bug. We _do_ want to save RAM when certain algorithms are not used. For example, when people select SHA256 over SHA512, they expect to save RAM.

gilles-peskine-arm (Migrated from github.com) reviewed

2018-12-21 12:32:46 -06:00

include/psa/crypto_struct.h

Do all compilers guarantee maximum alignment for a struct that only contains byte-aligned fields in all circumstances? This is not explicitly guaranteed by the C standard. It's often indirectly implied by the ability to make a struct opaque, but a whole-program optimizer is allowed to notice that, after suitable application of as-if rules, the struct is never made opaque, and therefore it can be aligned any old way.

Patater (Migrated from github.com) reviewed

2019-01-02 08:25:02 -06:00

include/psa/crypto_struct.h

My current best idea is that this would require a more advanced configuration system, where the configuration creates a define for the minimum necessary context size based on your configuration. Creating such an advanced configuration system would be out of scope of this PR.

Yes, understood that it's desirable to save RAM with certain configurations. However, the exact mechanism of how the size changes based on the configuration should not be exposed outside the implementation. The current suggested implementation (as present in this PR) is a compromise, preferring not to leak implementation details over the ability to save RAM. Suggestions welcome as to how to allow users to allocate structures statically without leaking the exact mechanism of how the size changes based on the configuration. My current best idea is that this would require a more advanced configuration system, where the configuration creates a define for the minimum necessary context size based on your configuration. Creating such an advanced configuration system would be out of scope of this PR.

gilles-peskine-arm (Migrated from github.com) requested changes

2019-01-02 08:59:05 -06:00

gilles-peskine-arm (Migrated from github.com) left a comment

Having structures that are exposed to the compiler but documented as implementation-dependent is not a blocker for Mbed Crypto 1.0. Increasing the RAM usage unnecessarily is a blocker. Therefore I am rejecting the design of this pull request. Since the goal of this pull request is not something we particularly need at the moment, a design proposal should be held to a high standard in terms of not breaking anything else. Better wait a few months when we don't have so many other priorities.

include/psa/crypto_struct.h

The mechanism should not be exposed, but the values need to be exposed. We have a product management decision to keep RAM usage down, so increasing the RAM usage is only acceptable if there is a strong advantage. Making backward compatibility slightly more robust against misuse is not a good enough advantage for a library whose primary target is microcontrollers.

The _mechanism_ should not be exposed, but the _values_ need to be exposed. We have a product management decision to keep RAM usage down, so increasing the RAM usage is only acceptable if there is a strong advantage. Making backward compatibility slightly more robust against misuse is not a good enough advantage for a library whose primary target is microcontrollers.

Patater commented

2019-01-02 10:13:47 -06:00

(Migrated from github.com)

Rebased on latest development

Patater commented

2019-01-02 10:20:00 -06:00

(Migrated from github.com)

@gilles-peskine-arm Aside from the RAM issue, do you think we should keep with this current partially opaque structs implementation or should we go full-on opaque? Specifically, I'm curious if structs like psa_key_policy_s should go opaque and if psa_hash_operation_s should not have psa_algorithm_t alg in it, being fully opaque instead.

@gilles-peskine-arm Aside from the RAM issue, do you think we should keep with this current partially opaque structs implementation or should we go full-on opaque? Specifically, I'm curious if structs like `psa_key_policy_s` should go opaque and if `psa_hash_operation_s` should not have `psa_algorithm_t alg` in it, being fully opaque instead.

Patater (Migrated from github.com) reviewed

2019-01-02 10:25:40 -06:00

include/psa/crypto_struct.h

I'll have a think as to how else we could get the benefits of statically allocatable opaque structs without increasing RAM requirements for minimal configurations.

It's not just about being more robust against misuse, but making it easier to test for API and ABI breakage, being more clear about the API is, which headers we ship with library files (via make install), and so forth. RAM usage is a big deal, so this needs to be properly balanced. I'll have a think as to how else we could get the benefits of statically allocatable opaque structs without increasing RAM requirements for minimal configurations.

gilles-peskine-arm commented

2019-01-02 10:35:04 -06:00

(Migrated from github.com)

psa_hash_operation_s does not have a public psa_algorithm_t field. It doesn't have any public field. Our library integration has a private psa_algorithm_t field. Our service integration doesn't.

Semantically, the structures are opaque to the application programmer, but not to the compiler. Their size and alignment requirements are known to the compiler and the application programmer may declare a variable of the corresponding type and may call `sizeof` on the corresponding type. There's no way to express this semantics in C. `psa_hash_operation_s` does not have a public `psa_algorithm_t ` field. It doesn't have any public field. Our library integration has a private `psa_algorithm_t ` field. Our service integration doesn't.

Patater commented

2019-01-07 11:43:39 -06:00

(Migrated from github.com)

Closing this PR as we'll make structs opaque by documenting them as opaque, instead of making them actually opaque, for the time being. We'll need to update how our tests work in order to do automatic ABI break detection.