Summary

This PR adds a new compile-time option MBEDTLS_X509_TRUSTED_CERTIFICATE_CALLBACK, disabled by default, which enables new APIs mbedtls_x509_crt_verify_with_cb() and mbedtls_ssl_conf_ca_cb() which allow users to provide the set of trusted certificates through a callback instead of a static linked list.

Background

So far, users configure trusted CAs and CRLs statically through the following API:

The configuration is not allowed to change while at least one SSL context is bound to it. In particular, changes to the list of trusted CAs is not possible unless all open connections are closed beforehand, which might not be acceptable.

The API forces the set of trusted CAs to be passed as a linked list. This doesn't allow efficient traversal in case of a large number of CAs.

Prior to the recent X.509 optimizations - especially the addition of parsing X.509 certificates without copying the input buffer - setting up an mbedtls_x509_crt structure inevitably leads to a copy of the raw data underlying a certificate being created. For a large number of CA certificates, this results in a high memory overhead which might not be tolerable.

Approach: CA callbacks

This PR suggests adding an API to allow the user to register a callback which takes a child certificate and returns a list of potential trusted signers.

The intended semantics is that cb searches some external storage of trusted CAs for potential signers for the child certificate child (for example, by matching the child's Issuer with the trusted certificate'sSubject field) and returns a linked list of those. Mbed TLS will then traverse this linked list and look for an actual signer, in the same way as it previously traversed the statically configured CA list.

No CRT verification checks are offloaded to the callback, and no assumptions are made on the list of certificates returned. For example, it is functionally correct to always return list of all trusted certificates, but the intended use of the callback is that it searches through an efficient presentation of the database
of trusted CAs, and returns a small linked-list of potential signers only.

The candidates list of candidate signers returned by the callback must be heap allocated, and ownership is passed to Mbed TLS when the callback returns, which frees it once it's no longer used. This leads to a slight memory overhead, because it requires the duplication of the CAs returned by the callback; however, this inefficiency seems preferable compared to a more complex callback mechanism passing ownership back and forth between the callback and Mbed TLS, considering that the candidate list will usually contain either no or exactly one element, and that the callback feature will not be used on constrained devices.

New X.509 API for CRT verification using CA callbacks

The callback type mbedtls_x509_crt_ca_cb_t is used in the following new X.509 CRT verification API:

This commit applies the documentation improvements noticed and applied
while adding the documentation for the new X.509 CRT verification API
mbedtls_x509_crt_verify_with_cb() to the existing verification APIs.

So far, there were the following CRT verification functions:
- `mbedtls_x509_crt_verify()` -- no profile, no restartable ECC
- `mbedtls_x509_crt_verify_with_profile()` -- profile, no restartable ECC
- `mbedtls_x509_crt_verify_restartable()` -- profile, restartable ECC
all publicly declared and offering increasing functionality.
On the implementation-side,
- `mbedtls_x509_crt_verify()` resolves to
a call to `mbedtls_x509_crt_verify_with_profile()` setting
the profile to `NULL`, and
- `mbedtls_x509_crt_verify_with_profile()`
resolves to a call to ``mbedtls_x509_crt_verify_restartable()`
setting the ECC restart context to NULL.
This commit adds two more functions to this zoo:
- `mbedtls_x509_crt_verify_with_cb()`
- `x509_crt_verify_restartable_cb()`
Here, `mbedtls_x509_crt_verify_with_cb()` is similar to
`mbedtls_x509_crt_verify_with_profile()` but uses a CA callback
instead of a static CA list, and no restart context.
`x509_crt_verify_restartable_cb()` is similar to
`mbedtls_x509_crt_verify_restartable()` but allows to either use
a static list of trusted CAs _or_ a trusted CA callback.
On the implementation-side,
- the body of `mbedtls_x509_crt_verify_restartable()` is moved to
`x509_crt_verify_restartable_cb()`, and the new version of
`mbedtls_x509_crt_verify_restartable()` just resolves to
`x509_crt_verify_restartable_cb()` with the trusted CA callback
set to NULL.
- The new function `mbedtls_x509_crt_verify_with_cb()`
forward to `x509_crt_verify_restartable_cb()` with the restart
context set to `NULL`.
There's no change to the implementation yet, and in particular,
`mbedtls_x509_crt_verify_with_cb()` isn't yet usable.

The PR looks good to me overall, except that I agree with Jarno that the name of the new public function X.509 should be more explicit as there are now 2 callbacks, and in the future there might be more of them (CRL callback? OCSP callback?). As names of public APIs are long-term and can't easily be fixed after the first release, I'm going to consider this one a blocker and request the public name to be fixed before I approve the PR.

The rest of my feedback is minor and doesn't block merging or this PR. Feel free to fix some low-hanging fruits (eg typos) depending on your available time and ignore the rest for now - we can still fix it later if we really care.

This comment has been minimized.

Minor: I find this wording slightly confusing. We are making a promise to the user that this will point to a readable certificate, not requesting them to pass a pointer to a readable certificate. Can we change "must" to "will" here?

(Same for next parameter. Contrast with the note below where we are actually requesting something from the user.)

This comment has been minimized.

This comment has been minimized.

@mpg@jarlamsa I think there is a misunderstanding here: The child pointer must point to a readable CRT, and the candidate_cas ptr must not be \c NULL. This is to be distinguished from the value of *candidate_cas on success - but even the latter may be NULL if the functions returns successfully but didn't find any candidates.

This comment has been minimized.

Those are "must" from the APIs point of view and "will" from the users point of view. Both are in sense valid, but which are we using? In the following note the must is referring that what the callback must do.

This comment has been minimized.

@hanno-arm I think we're talking about different things here, I'm really not talking about returned values or the distinction between canditates_cas and *candidate_cas, but rather about the difference between documenting a normal API function and the type of a callback.

For API functions, we (Mbed TLS) are providing a function to users and telling them what they must do when using this function. For callbacks, users are providing a function to us, are we're telling them what we promise we'll do when calling that function, so that they now what assumptions they can rely on when writing the function.

For me, the point is who's talking to whom. Since we're writing the documentation, it feels weird to me to write "must" for documenting requirements on ourselves. We should be talking to the user, not to ourselves.

This comment has been minimized.

edited

Contributor

(Edit: in reply to Jarno's comment.) Unfortunately this function returns void, so I don't think there is an easy way to do that. Hopefully people will read the documentation and are unlikely to try that anyway. (I know this is not very satisfying but I don't see a better way without larger changes.)

This comment has been minimized.

I think this section is a near-duplicate of the previous one, which is fine but should probably be documented here and in the previous section, so that in the future when we update one we remember to update the other accordingly.

This comment has been minimized.

Note for the future: it would be cleaner if this had been done in the same commit that changed the function name in its definition. We should try to make sure each commit at least builds on its own (and if possible, pass tests) unless there's a good reason not to. For example, writing tests for functions before they're implemented is a good reason for the tests not to build for a few commits. But other than this kind of choice, most commits should build cleanly on their own.

(No need to rewrite history for that, we tend to avoid rewriting history while review is in progress - just mentioning that for future PRs.)

Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.