Plans for TF-PSA-Crypto legacy headers #145
Conversation
Covers high-level plans for header privatization. Signed-off-by: Gilles Peskine <[email protected]>
Signed-off-by: Gilles Peskine <[email protected]>
It's public specifically for the sake of X.509, not for application code. Signed-off-by: Gilles Peskine <[email protected]>
Signed-off-by: Gilles Peskine <[email protected]>
Signed-off-by: Gilles Peskine <[email protected]>
Signed-off-by: Gilles Peskine <[email protected]>
Signed-off-by: Gilles Peskine <[email protected]>
|
Thanks for the review. I've addressed your comments, but not yet added the missing sections that I'm still working on. |
docs/architecture/0e-plans.md
Outdated
| The following header files define cryptography-adjacent interfaces which we have no plans to replace. | ||
|
|
||
| * `asn1.`, `asn1write.h`: ASN.1, needed for key parsing/writing as well as for X.509. | ||
| * `base64.h`, `pem.h`: Encoding help, needed for key parsing/writing as well as for X.509. Arguably Base64 could be made private, for the use of PEM only, but it is currently used for non-PEM purposes in Mbed TLS. Thus I propose to keep it officially public in TF-PSA-Crypto 1.x. |
There was a problem hiding this comment.
Indeed. I'm not sure what the current status is, but I remember being deeply unsatisfied with our base64 API last time I looked at it (to the point of starting trying to fix it locally, which I evidently never got time to finish). The main problem being we're pretty inconsistent in where the reported sizes include the terminating nul byte or not, and things are not clearly documented. I don't remember if PEM suffer from similar issues these days.
Taking a step back, it could be argued that neither PEM nor base64 should rely on nul-termination, but rather only on size arguments.
I'm not sure if that's enough of a reason to not include them in 1.0 and re-include a better version later. (Obviously in an ideal world we'd have time to improve their API before the 1.0 and things would be easier, but we don't live in that world.)
Signed-off-by: Gilles Peskine <[email protected]>
Signed-off-by: Gilles Peskine <[email protected]>
|
|
||
| `oid.h` and `MBEDTLS_OID_C` are in charge of defining all the OIDs used internally or by application code. This is a known problem which can result in applications wasting code size on many OIDs that they don't care about. We are planning a redesign: https://github.com/Mbed-TLS/mbedtls/issues/9380 . | ||
|
|
||
| At this point, we do not consider this critical for TF-PSA-Crypto 1.0, and we are likely to work on it during the lifetime of TF-PSA-Crypto 1.x. This may change if we reevaluate the priority based on concrete use cases. In the meantime, `oid.h` will become private, but will remain used in Mbed TLS. |
There was a problem hiding this comment.
In the meantime,
oid.hwill become private, but will remain used in Mbed TLS.
What does this mean? In the categories of header described above, 'private' means that it can't be used by Mbed TLS, so this seems like a contradiction.
There was a problem hiding this comment.
I've tried to consistently use “private” for unofficial interfaces that may still be exposed, and “internal” for interfaces where there is some technical measure limiting outside use (e.g. function only declared in a header in core or **/src). So there's no contradiction here: oid.h is not an official interface, but it's exposed to Mbed TLS. Did I get the terminology wrong somewhere above?
docs/architecture/0e-plans.md
Outdated
| | `hkdf.h` | `mbedtls_hkdf_` | Delete | https://github.com/Mbed-TLS/mbedtls/issues/9150 | | ||
| | `hmac_drbg.h` | `mbedtls_hmac_drbg_` | Private | [can be made fully private](#headers-that-can-be-made-fully-private) with a little work for [RNG header privatization](#rng-header-privatization) | | ||
| | `lms.h` | `mbedtls_lms_` | Public | [no PSA equivalent](#cryptographic-mechanisms-with-no-PSA-equivalent) | | ||
| | `md.h` | `mbedtls_md_` | Expose | [context types](#headers-with-context-types), but likely [Public hash-only `md.h`](#public-hash-only-md.h) | |
There was a problem hiding this comment.
Nit: The link doesn't work, maybe:
| | `md.h` | `mbedtls_md_` | Expose | [context types](#headers-with-context-types), but likely [Public hash-only `md.h`](#public-hash-only-md.h) | | |
| | `md.h` | `mbedtls_md_` | Expose | [context types](#headers-with-context-types), but likely [Public hash-only `md.h`](#changes-to-public-crypto-headers) | |
gilles-peskine-arm
added
needs-review
Signed-off-by: Gilles Peskine <[email protected]>
I had reorganized some sections without updating their links. Also fix some incorrect title-to-fragment conversions (trying to follow GitHub markdown rules). Signed-off-by: Gilles Peskine <[email protected]>
Signed-off-by: Gilles Peskine <[email protected]>
docs/architecture/0e-plans.md
Outdated
| * Inside the Mbed TLS library, to parse and write X.509 objects. | ||
| * In application code, very occasionally. (Examples: [Fire-evacuation-guidance-system-IoT](https://github.com/2nd-Chance/Fire-evacuation-guidance-system-IoT/blob/33031a8255fe1ae516ddd58f1baa808801cd3abf/iotivity/resource/csdk/security/src/credresource.c#L3185) (dead project), [SiLabs Bluetooth attestation server](https://github.com/SiliconLabs/bluetooth_applications/blob/3eb0f3c9e234ada1f10714fb9376fcbc8e95807f/bluetooth_secure_attestation/bt_secure_attestation_server/src/ecdh_util3.c#L375)) | ||
|
|
||
| The use of PEM inside the Mbed TLS is intrinsic. It doesn't leak through the API of Mbed TLS, but Mbed TLS cannot be implemented without PEM. This is different from other private modules that Mbed TLS currently calls internally, but will no longer need to call once Mbed TLS has fully migrated to PSA. Thus, in the long term, TF-PSA-Crypto needs to expose its PEM API to Mbed TLS. (We reject the hypotheses of independent PEM implementations, or of making PEM its own library, as too much maintenance work.) |
|
|
||
| * Inside the crypto library, to implement PEM. | ||
| * In the `pem2der` sample program. | ||
| * In Mbed TLS SSL test programs, for context serialization. (It's not clear to me why we encode in Base64 rather than binary.) |
There was a problem hiding this comment.
Not clear to me either, seems completely arbitrary to me. (And if we want something printable, we already have utility functions for hex in the test programs and could use that instead.)
Signed-off-by: Gilles Peskine <[email protected]>
After discussing this with Manuel, we believe that spending the time to unify the error code space will save us precious design and execution time for header privatization. Signed-off-by: Gilles Peskine <[email protected]>
Signed-off-by: Gilles Peskine <[email protected]>
Signed-off-by: Gilles Peskine <[email protected]>
Signed-off-by: Gilles Peskine <[email protected]>
docs/architecture/0e-plans.md
Outdated
|
|
||
| * Mbed TLS code checks for specific error codes from crypto functions. These error codes must remain exposed. | ||
| * Specific error codes appear in TF-PSA-Crypto or Mbed TLS documentation. These error codes are thus effectively public, and the declaration of the `MBEDTLS_ERR_xxx` constant should remain public. | ||
| * Public functions return error codes that are now private, thus undocumented. This is a quality problem for users. It is not critical to resolve this problem before the 1.0 release, because if the specific error code is not documented, a future minor release either start documenting the error code or change the function's behavior to make it return a documented error code instead. |
There was a problem hiding this comment.
Missing word: "a future minor release can either"
docs/architecture/0e-plans.md
Outdated
|
|
||
| Having to convert between the two error code spaces costs significant code size. It also adds complexity and we occasionally have bugs where we forget to convert. Thus it would be desirable not to have this distinction. | ||
|
|
||
| It is already the case in Mbed TLS 3.x that a PSA error value cannot be equal to a legacy error, because we avoid assigning values in a range that would overlap. |
There was a problem hiding this comment.
(Thinking out loud.) OK, so looking at error_common.h and crypto_values.h (and always taking the absolute value):
- PSA crypto errors are [132, 153] or 248, that is 0x84 to 0x99 and 0xf8.
- Mbed TLS low level errors are 1 to 127 or 0x01 to 0x7f
- Mbed TLS high level errors are multiples of 128, so 0xhh00 or 0xhh80
So, a PSA error value can't be equal to a legacy low level error (because it's >= 128) or to a legacy high level error (because it's not a multiple 128, being in the range [132, 248]).
Was going to say it could conceivably be equal to a legacy "combined" (high + low) error code, but that can't happen either: since the "high-level module number" (ie the top nibble of the error code) can't be 0, all combined error codes are >= 0x1000.
Ok, so indeed a PSA error code can't be equal to a legacy error.
| | `mbedtls_ecdsa_raw_to_der` | crypto internal | Keep public | | ||
| | `mbedtls_ecdsa_der_to_raw` | crypto internal | Keep public | | ||
|
|
||
| The RNG wrapper function `mbedtls_psa_get_random` (and the associated constant `MBEDTLS_PSA_RANDOM_STATE`) has a negligible cost for us and can help users transition their code. Keep it, but mark it as deprecated, possibly inlined, and change it (code and documentation) to just return the return value of `psa_get_random`. |
There was a problem hiding this comment.
can help users transition their code
What do you have in mind here? My thinking is this function's primary purpose was it could be used as the f_rng argument of functions that have such a parameter. But we'll be removing all such parameters, so once that's done how will this function still be helpful to users?
There was a problem hiding this comment.
Anything that happens in X.509 or TLS as a result of API changes in crypto, is also something that's likely to happen in code that implements other protocols. Maintainers of third-party code bases that implement other protocols may want to start using TF-PSA-Crypto 1.0 before they have fully converted their code. They may even want to support both Mbed TLS 3.6 and TF-PSA-Crypto in the same code branch, if they deploy the same application to devices which are not all upgraded to the latest crypto at the same time. So they may want to pass f_rng arguments around in their own code, even if they end up being unused when building against TF-PSA-Crypto.
docs/architecture/0e-plans.md
Outdated
|
|
||
| The ECC group conversion functions are now purely private since the type `mbedtls_ecp_group_id` is private. They aren't used by Mbed TLS, so move their declaration to an internal header. | ||
|
|
||
| The digest algorithm conversion functions are heavily used in TLS code (mostly if not exclusively TLS 1.3). They perform a very simple conversion, so it should be easy to get rid of them. However, as long as `mbedtls_md_type_t` still exists (see [Public hash-only `md.h`](public-hash-only-mdh)), it could make sense to have them as public functions in `md.h`. |
There was a problem hiding this comment.
Yes, I think it makes sense to keep them in md.h as long as it exists (that is, until 5.0).
Signed-off-by: Gilles Peskine <[email protected]>
Signed-off-by: Gilles Peskine <[email protected]>
Signed-off-by: Gilles Peskine <[email protected]>
Signed-off-by: Gilles Peskine <[email protected]>
Signed-off-by: Gilles Peskine <[email protected]>
Signed-off-by: Gilles Peskine <[email protected]>
Signed-off-by: Gilles Peskine <[email protected]>
Work in progress on Mbed-TLS/mbedtls#9882.
Status: the architectural overview is complete. Some parts need further refinement and many parts need task breakdown. In many places, the task breakdown needs input from prioritization, which needs input from the architectural overview, and thus is beyond the scope of this issue. Thus this document fulfills #9882.
PR checklist