Make PSA Crypto structs actually opaque #4

Patater · 2018-12-21T18:06:32Z

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 · 2018-12-21T18:28:18Z

include/psa/crypto_struct.h

-        mbedtls_sha512_context sha512;
-#endif
-    } ctx;
+    uint8_t reserved[217];


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.

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.

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.

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 · 2018-12-21T18:32:45Z

include/psa/crypto_struct.h

-        mbedtls_sha512_context sha512;
-#endif
-    } ctx;
+    uint8_t reserved[217];


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.

gilles-peskine-arm

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.

gilles-peskine-arm · 2019-01-02T14:56:15Z

include/psa/crypto_struct.h

-        mbedtls_sha512_context sha512;
-#endif
-    } ctx;
+    uint8_t reserved[217];


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.

Use target-specific include directories instead. This allows us finer control over which sets of headers the library, programs, and tests can use.

Ensure separation between .c and .h files by moving .h files to a new internal include folder. It's nice to have internal headers in their own folder, so that one needn't tab so much to get the .c file they want to edit in the library folder, tabbing over .h files, or vice-versa when wanting to edit a .h file: use the "library/include" folder for internal .h files, and "library/" for .c files.

Static asserts are so useful we want to use them not only in our tests. Move STATIC_ASSERT_EXPR to utils.h, an internal header, to make it available to tests and the library implementation.

Add a static assert macro that evaluates to a struct definition, so we can assert things statically. This is useful vs STATIC_ASSERT_EXPR, which evaluates to an expression, where an expression is not needed.

Move crypto_struct.h, which doen't specify the API or ABI of the library, to the internal include folder. These headers should not get distrubuted with our shared library, for example, as they contain implementation details that may change between version of the shared library.

It's important to be clear that the contents of the crypto structs are not part of the API, and not guaranteed to be the same between versions. The only guarantee is that the sizes of the structs will not shrink. If the structs grow, this is an ABI break, but not an API break, as the user-allocated object may not be big enough for the new larger struct. A recompile of the application with the new header will fix this. XXX This makes crypto structs partially opaque. I've left in a few things that we could promise will always be present in the structs. If we choose not to always promise these, we can change what's in the crypto_structs.h file.

Patater · 2019-01-02T16:13:47Z

Rebased on latest development

Patater · 2019-01-02T16:20:00Z

@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 · 2019-01-02T16:35:04Z

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 · 2019-01-07T17:43:39Z

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.

Patater added enhancement New feature or request needs: preceding PR Requires another PR to be merged first needs: design review labels Dec 21, 2018

Patater force-pushed the real-opaque-structs branch from 0ad3904 to 3852a56 Compare December 21, 2018 18:13

gilles-peskine-arm reviewed Dec 21, 2018

View reviewed changes

gilles-peskine-arm requested changes Jan 2, 2019

View reviewed changes

Patater added 6 commits January 2, 2019 16:12

Don't use global includes for library

4efc888

Use target-specific include directories instead. This allows us finer control over which sets of headers the library, programs, and tests can use.

Move STATIC_ASSERT_EXPR to utils.h

28284c8

Static asserts are so useful we want to use them not only in our tests. Move STATIC_ASSERT_EXPR to utils.h, an internal header, to make it available to tests and the library implementation.

Add a static assert macro

3c09f55

Add a static assert macro that evaluates to a struct definition, so we can assert things statically. This is useful vs STATIC_ASSERT_EXPR, which evaluates to an expression, where an expression is not needed.

Patater force-pushed the real-opaque-structs branch from 3852a56 to e1fbc05 Compare January 2, 2019 16:13

Patater closed this Jan 7, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Make PSA Crypto structs actually opaque #4

Make PSA Crypto structs actually opaque #4

Uh oh!

Patater commented Dec 21, 2018 •

edited

Loading

Uh oh!

gilles-peskine-arm Dec 21, 2018

Uh oh!

Patater Jan 2, 2019

Uh oh!

gilles-peskine-arm Jan 2, 2019

Uh oh!

Patater Jan 2, 2019

Uh oh!

gilles-peskine-arm Dec 21, 2018

Uh oh!

gilles-peskine-arm left a comment

Uh oh!

gilles-peskine-arm Jan 2, 2019

Uh oh!

Patater commented Jan 2, 2019

Uh oh!

Patater commented Jan 2, 2019

Uh oh!

gilles-peskine-arm commented Jan 2, 2019

Uh oh!

Patater commented Jan 7, 2019

Uh oh!

Uh oh!

Make PSA Crypto structs actually opaque #4

Make PSA Crypto structs actually opaque #4

Uh oh!

Conversation

Patater commented Dec 21, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gilles-peskine-arm Dec 21, 2018

Choose a reason for hiding this comment

Uh oh!

Patater Jan 2, 2019

Choose a reason for hiding this comment

Uh oh!

gilles-peskine-arm Jan 2, 2019

Choose a reason for hiding this comment

Uh oh!

Patater Jan 2, 2019

Choose a reason for hiding this comment

Uh oh!

gilles-peskine-arm Dec 21, 2018

Choose a reason for hiding this comment

Uh oh!

gilles-peskine-arm left a comment

Choose a reason for hiding this comment

Uh oh!

gilles-peskine-arm Jan 2, 2019

Choose a reason for hiding this comment

Uh oh!

Patater commented Jan 2, 2019

Uh oh!

Patater commented Jan 2, 2019

Uh oh!

gilles-peskine-arm commented Jan 2, 2019

Uh oh!

Patater commented Jan 7, 2019

Uh oh!

Uh oh!

Patater commented Dec 21, 2018 •

edited

Loading