Provide incremental by-subsystem initialization for the crypto library #202
Conversation
athoelke
added
enhancement
athoelke
force-pushed
the
crypto-partial-init
branch
from
314871a to
a4b7323
Compare
There was a problem hiding this comment.
This design was prototyped in Mbed-TLS/mbedtls#6636 and I consider that prototype to be sufficient validation for the API design.
The specification looks good to me except for a few places where some Mbed TLS specific history remains.
doc/crypto/api/library/library.rst
Outdated
| .. summary:: | ||
| Crypto subsystem identifier for accelerator drivers. | ||
|
|
||
| Initializing this subsystem results in initialization of all registered accelerator drivers. |
There was a problem hiding this comment.
It's awkward that the PSA specification now discusses drivers, but it doesn't say anything anywhere about the existence of drivers. We should at least give an overview of what “accelerator drivers”, “secure element drivers” and “entropy drivers” are, conceptually, even if we leave the concrete aspects as something that's implementation-defined.
There was a problem hiding this comment.
I think that is worth a small PR of its own to discuss such things somewhere between the design goals/sample architectures/implementation considerations?
| It is guaranteed that the following operations do not to require this subsystem: | ||
|
|
||
| * Hash operations. | ||
| * Signature verification operations. |
There was a problem hiding this comment.
I wonder if we should qualify this further. A use case that I'm aware of, but didn't account for in my original documentation, is the following. A secure element has an encrypted-authenticated channel with the main processor. Setting up this channel needs local symmetric encryption capabilities, but also local randomness to establish a session key. As a consequence, accessing any key in the secure element will need the local RNG to be initialized. Now that means that PSA_CRYPTO_SUBSYSTEM_SECURE_ELEMENTS will implicitly initialize PSA_CRYPTO_SUBSYSTEM_RANDOM, so you don't actually need to initialize PSA_CRYPTO_SUBSYSTEM_RANDOM explicitly to verify a signature with a key from the secure element. So the statement here is technically true, but perhaps misleading for this unusual but existing scenario.
There was a problem hiding this comment.
The scenario that a subsystem might be initialized implicitly as another subsystem depends on it is described in the description of psa_crypto_init_subsystem(), using the example of communications in a client/server implementation. We could add this example to that description, or we could add a note here (in PSA_CRYPTO_SUBSYSTEM_RANDOM) that RANDOM may be required by other subsystems, for example to initialize communication with a secure element.
| It is guaranteed that the following operations do not to require this subsystem: | ||
|
|
||
| * Hash operations. | ||
| * Signature verification operations. |
There was a problem hiding this comment.
I wonder if we should also mention get-attributes and some exports? Exporting a public key should never require an RNG, but exporting the public key of a key pair might involve blinding.
There was a problem hiding this comment.
I've added key encapsulation and PAKE to the list of operations that require the RANDOM subsystem; and modified the 'some operations using the private or secret key' statement to 'some operations using key pairs' to more readily include export of public keys (where this is calculated on demand).
I can add attribute retrieval, and also export of a symmetric key type or a public key type, to the list of not-needing randomness.
doc/crypto/api/library/library.rst
Outdated
| :definition: /* implementation-defined value */ | ||
|
|
||
| .. summary:: | ||
| Crypto subsystem identifier for access to built-in keys. |
There was a problem hiding this comment.
Similar to drivers, in the PSA API, we never define “built-in key”. The concept is straightforward, but there should be a sentence somewhere saying that implementations might define built-in keys.
There was a problem hiding this comment.
This might be worth adding to a separate PR, alongside the coverage of accelerators and secure elements
| :definition: /* implementation-defined value */ | ||
|
|
||
| .. summary:: | ||
| Crypto subsystem identifier for all available subsystems. |
There was a problem hiding this comment.
Is this guaranteed to include all the PSA_CRYPTO_SUBSYSTEM_xxx values defined in this specification? For example, if an implementation doesn't support secure elements, should PSA_CRYPTO_SUBSYSTEM_SECURE_ELEMENTS be included?
I think it should, because it's trivial to implement and makes it less fiddly for application developers (even though applications that use partial initialization are rarely going to be very portable). In particular it would reduce the chances that an application breaks if the next version of the implementation adds new capabilities.
There was a problem hiding this comment.
I think this is effectively implied by the rule that using this identifier in a call to psa_crypto_init_subsystem() is equivalent to a call to psa_crypto_init(). But maybe we should spell that out, to avoid an implementation short cut that doesn't consider this kind of time evolution of the implementation?
athoelke
force-pushed
the
crypto-partial-init
branch
from
a4b7323 to
5360369
Compare
|
Rebased and updated this PR |
There was a problem hiding this comment.
I've reviewed up to 5360369. Ok so far. Still to be addressed:
- Provide incremental by-subsystem initialization for the crypto library #202 (comment)
- Provide incremental by-subsystem initialization for the crypto library #202 (comment)
- Provide incremental by-subsystem initialization for the crypto library #202 (comment)
- Provide incremental by-subsystem initialization for the crypto library #202 (comment)
- Provide incremental by-subsystem initialization for the crypto library #202 (comment)
I think I've covered all except the need for some discussion of accelerators, secure elements, and built-in keys within the document. I plan to deal with that in a separate PR, unless you would prefer to keep it coupled with this one? |
|
Ah, I realise that this one still needs a change to the PR. |
|
Ok for a separate PR. But I don't think considerations around “drivers” and “built-in keys” should leak outside the specification of subsystem initialization for now. |
I think I could reword the text for the subsystem definitions to avoid talking about 'drivers' - which suggests a specific way of a portable implementation providing support for varied hardware. I just need to review the spec for a mention of 'secure element' and 'accelerator', and add/amend if necessary. |
|
I've added a couple more commits.
I think this addresses all of the outstanding comments. |
… key' * Xref the terms in suitable places * Remove 'registration of drivers' from the description of crypto subsystem initialization.
athoelke
force-pushed
the
crypto-partial-init
branch
from
74df62f to
0ad77a5
Compare
|
This is will not be in the next release of mbed TLS (4.0), and we prefer to only include new API when there is an implementation of the interface, |
See #16 for discussion of this API, which enables partial initialization of the library. This is useful in constrained contexts, for example during early boot, when not all library functionality is required to support the application use case.
Fixes #16