Crypto Subsystem API Improvements

dpkristensen commented 1 year ago

Introduction

The current design of the crypto API has some issues that can lead to usability problems, especially given that not all of the shims provided are fully implemented.

The incomplete design of the Crypto API means that users are likely going to just skip it altogether and instead work directly with specific libraries. This makes it difficult to switch out the implementation later.

Indeed the lack of sample code illustrating the API lends itself to poor discoverability and code coverage. There is one sample using Mbed TLS, which would not work with TinyCrypt because it invokes the hash functions.

Problem description

Here is a list of problems I've encountered while implementing cryptographic operations using Zephyr as part of the Nordic nRF Connect SDK v2.2.0 (Zephyr tag v3.2.99-ncs1-rc2):

zephyr/crypto/hash.h and zephyr/crypto/cipher.h do not have functions.

If one was looking for ciphers or hash functions, this would be a natural starting point; but as-written there is nothing directly callable in there.

Crypto drivers do not specify a way to indicate what algorithms are supported

I realize TinyCrypt is deprecated and the shim is marked as experimental, but let's look at it from a holistic point of view. The hash functions are not implemented, so calling hash_begin_session() on this device results in a crash. The default configuration in the nRF Connect SDK enables TinyCrypt, so it's naturally advertising itself as something that could/should be used.

Proposed change

I propose the following conceptual changes:

Name/structure the headers to make it clear where the reader should start looking.
Allow the code to query which functions/algorithms each device supports.
Allow the code to look up a supported driver based on what is supported.

Detailed RFC

In this section of the document the target audience is the dev team. Upon reading this section each engineer should have a rather clear picture of what needs to be done in order to implement the described feature.

Proposed change (Detailed)

1 Header Improvements

Rename hash.h -> crypto_hash.h
Rename cipher.h -> crypto_cipher.h

This will make it more clear that these headers are subordinate to crypto.h.

2 Driver Query

Implement methods to support runtime algorithm query.

/**
 * @brief Query hash algorithm support
 *
 * Determine whether the device supports a given hash algorithm.
 *
 * @param  dev      Pointer to the device structure for the driver instance.
 * @param  algo     The hash algorithm to be used in this session. e.g sha256
 *
 * @return 1 if supported, 0 if not supported, or negative errno code on failure.
 */
int crypto_hash_supported(const struct device* dev, enum hash_algo algo);

/**
 ... similar to above
 */
int crypto_cipher_supported(const struct device* dev, enum cipher_algo algo, enum cipher_mode mode);

3 Device Lookup

Device lookup would be nice, but it depends heavily on how the drivers are structured. Hardware drivers could implement this in the DTS, while software drivers would require conditional compilation in include/zephyr/devicetree/crypto.h

The nice thing about this approach is that drivers not yet migrated to the device tree methods will just be "invisible" to these methods.

/* Example usage: */
struct hash_ctx hash;
const struct device* dev =
    DEVICE_DT_GET(DT_COMPAT_GET_ANY_STATUS_OKAY(crypto_hash_sha256));
int err = hash_begin_session(dev, &hash, CRYPTO_HASH_ALGO_SHA256);
/* err == -ENODEV if none */

Or an alternative similar to the current device_get_binding(...) approach:

const struct device* dev = device_get_compat("crypto,hash_sha256");
/* dev == NULL if not found */

4 Robustness

For robustness, I also propose the following low-risk / low-effort changes:

hash_begin_session() - Check for API function before calling backend, return -ENOSYS if not supported.
cipher_begin_session() - Check for API function before calling backend, return -ENOSYS if not supported.

Dependencies

If done right, no broad migration required; can be done on per-component basis.

Concerns and Unresolved Questions

For code size concerns, I believe the compiler should be able to optimize much of this out if not used.

Alternatives

Continue status quo of bypassing crypto subsystem and using libraries directly.

dpkristensen commented 4 months ago

I'd like to point out that this proposal was made before I was aware of the PSA Certified organization created by ARM and its partners. This mechanism allows supporting multiple crypto subsystems at once, and so is more flexible; but PSA as an API is likely only to gain in popularity.

I think PSA as an API surface should exist as a first-class API in Zephyr, making this subsystem improvement more relevant to "advanced" usages/configurations. PSA could be implemented to take the first device available through this subsystem and use it.

bobwolff-cpi commented 2 months ago

As a note of refinement here, I'd like to suggest that the current system has a granularity of "on or off" but often times there is a call for only needing HASH routines and not encryption/decryption or vice-versa. It would be lovely to specify this level of granularity. As a simplistic example of what I'm trying to get across:

struct crypto_api {
#if defined(CONFIG_HASHING)
  .hashing_begin = this_hasing_begin,
  .hasing_end = this_hasing_end,
#endif
#if defined(CONFIG_ENCRYPT_DECRYPT)
  .encrypt = this_encrypt,
  .decrypt = this_decrypt,
#endif
};

dpkristensen commented 2 months ago

As a note of refinement here, I'd like to suggest that the current system has a granularity of "on or off" but often times there is a call for only needing HASH routines and not encryption/decryption or vice-versa. It would be lovely to specify this level of granularity.

The better way to achieve such an API separation is not to have it as on or off within the same API structure, but instead be two separate API structures per implementation. It is difficult to say whether hashing or encryption is supported without knowing what algorithms are targeted. The approach I listed with the device tree nodes where each algorithm is a separate entry would achieve such granularity.

One of the main goals of my original proposal is not to break existing API calls; but it would be up to the Zephyr maintainers to decide if they are worth keeping around.

dpkristensen commented 1 month ago

Closing this RFC per discussion on #43712. PSA API should be the go-to abstraction for generic crypto operations; specialized crypto operations or hardware may require a custom implementation and a custom driver anyway, so this just creates extra maintenance and clutters the API surface.

zephyrproject-rtos / zephyr