Add support for trusted CA callbacks #2532

hanno-becker · 2019-03-28T16:43:04Z

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:

void mbedtls_ssl_conf_ca_chain( mbedtls_ssl_config *conf,
                                mbedtls_x509_crt *ca_chain,
                                mbedtls_x509_crl *ca_crl );

This has the following disadvantages:

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.

typedef
   int (*mbedtls_x509_ca_cb_t)( void* ctx,
                  mbedtls_x509_crt *child,
                  mbedtls_x509_crt **candidates );

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's
Subject 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:

int mbedtls_x509_crt_verify_with_cb( mbedtls_x509_crt *crt,
                     mbedtls_x509_crt_ca_cb_t f_ca_cb,
                     void *p_ca_cb,
                     const mbedtls_x509_crt_profile *profile,
                     const char *cn, uint32_t *flags,
                     int (*f_vrfy)(void *, mbedtls_x509_crt *, int, uint32_t *),
                     void *p_vrfy );

This is almost equivalent to the existing API

int mbedtls_x509_crt_verify_with_profile( mbedtls_x509_crt *crt,
                     mbedtls_x509_crt *trust_ca,
                     mbedtls_x509_crl *ca_crl,
                     const mbedtls_x509_crt_profile *profile,
                     const char *cn, uint32_t *flags,
                     int (*f_vrfy)(void *, mbedtls_x509_crt *, int, uint32_t *),
                     void *p_vrfy );

int mbedtls_x509_crt_verify( mbedtls_x509_crt *crt,
                     mbedtls_x509_crt *trust_ca,
                     mbedtls_x509_crl *ca_crl,
                     const char *cn, uint32_t *flags,
                     int (*f_vrfy)(void *, mbedtls_x509_crt *, int, uint32_t *),
                     void *p_vrfy );

but replaces the linked list trust_ca of trusted certificates by pointers to the CA callback and its context (CRLs are not supported with CA callbacks -- see the "Limitations" section below).

New SSL API for handshakes using CA callbacks

The following new API allows to make use of CA callbacks in TLS handshakes:

void mbedtls_ssl_conf_ca_cb( mbedtls_ssl_config *conf,
                             mbedtls_x509_crt_ca_cb_t f_ca_cb,
                             void *p_ca_cb );

It should be used in place of the existing API mbedtls_ssl_conf_ca_chain().

Limitations

The new SSL API mbedtls_ssl_conf_ca_cb() is incompatible with a number of existing APIs and features:

mbedtls_ssl_conf_ca_chain()
CRLs
CA indication in server-side CertificateRequest messages.
CA chains set via SNI callbacks
Restartable ECC for X.509 signature verification

Please see the documentation for more information.

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.

mpg

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.

mpg · 2019-04-01T09:13:32Z

include/mbedtls/x509_crt.h

+ *
+ * \param p_ctx         An opaque context passed to the callback.
+ * \param child         The certificate for which to search a potential signer.
+ *                      This must point to a readable certificate.


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.)

@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.

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.

@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.

@mpg @jarlamsa Yes, that makes sense - thank you for elaborating, and sorry for missing that point in the first place!

include/mbedtls/x509_crt.h

tests/suites/test_suite_x509parse.data

mpg · 2019-04-01T09:54:59Z

library/ssl_tls.c

+#if defined(MBEDTLS_X509_TRUSTED_CERTIFICATE_CALLBACK)
+    /* mbedtls_ssl_conf_ca_chain() and mbedtls_ssl_conf_ca_cb()
+     * cannot be used together. */
+    conf->f_ca_cb = NULL;


(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.)

mpg · 2019-04-01T10:00:19Z

library/x509_crt.c

 */
-int mbedtls_x509_crt_verify_restartable( mbedtls_x509_crt *crt,
+static int mbedtls_x509_crt_verify_restartable_cb( mbedtls_x509_crt *crt,


Name nitpicking: static function usually don't start with mbedtls_ (also, the name here is not consistent with the commit message). But my real issue here is what @jarlamsa said.

tests/suites/test_suite_x509parse.function

tests/ssl-opt.sh

mpg · 2019-04-01T10:20:32Z

tests/ssl-opt.sh

@@ -2954,6 +2981,207 @@ run_test    "Authentication: send CA list in CertificateRequest, client self sig
            -c "! mbedtls_ssl_handshake returned" \
            -s "X509 - Certificate verification failed"

+# Tests for auth_mode, using CA callback


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.

I think that the need to duplicate the tests shows that the test system should be reworked to avoid that.

include/mbedtls/config.h

Change the naming to reflect that the function uses a new ca callback feature to distinguish different callbacks.

hanno-becker · 2019-04-01T13:35:28Z

include/mbedtls/x509_crt.h

 * \param candidate_cas The address at which to store the address of the first
 *                      entry in the generated linked list of candidate signers.
- *                      This must not be \c NULL.
+ *                      This will not be \c NULL.


I disagree with this change, as indicated in the response to @mpg's comment.

hanno-becker

Thanks @jarlamsa for making the requested changes! It looks fine to me, apart from one change in the documentation wording, as indicated in the comments.

mpg · 2019-04-02T07:38:25Z

library/ssl_tls.c

@@ -6050,7 +6050,7 @@ static int ssl_parse_certificate_verify( mbedtls_ssl_context *ssl,
        have_ca_chain = 1;

        MBEDTLS_SSL_DEBUG_MSG( 3, ( "use CA callback for X.509 CRT verification" ) );
-        ret = mbedtls_x509_crt_verify_with_cb(
+        ret = mbedtls_x509_crt_verify_with_ca_cb(


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.)

mpg

Thanks for making the requested changes. I'm happy with the result.

Patater

Mostly minor style comments, plus a question or two.

Please add any necessary ChangeLog entries.

Patater · 2019-04-04T11:52:02Z

programs/ssl/ssl_client2.c

@@ -439,6 +455,62 @@ static void my_debug( void *ctx, int level,
    fflush(  (FILE *) ctx  );
 }

+#if defined(MBEDTLS_X509_TRUSTED_CERTIFICATE_CALLBACK)
+int ca_callback( void *data, mbedtls_x509_crt const *child,
+                 mbedtls_x509_crt **candidates)


Style

- mbedtls_x509_crt **candidates) + mbedtls_x509_crt **candidates )

Patater · 2019-04-04T12:09:11Z

tests/ssl-opt.sh

+            "$P_SRV debug_level=3" \
+            "$P_CLI ca_callback=1 debug_level=3 " \
+            0 \
+            -c "use CA callback for X.509 CRT verification"\


Style:

- -c "use CA callback for X.509 CRT verification"\ + -c "use CA callback for X.509 CRT verification" \

Patater · 2019-04-04T12:09:17Z

tests/ssl-opt.sh

+            "$P_CLI ca_callback=1 debug_level=3 crt_file=data_files/server5.crt \
+             key_file=data_files/server5.key" \
+            0 \
+            -c "use CA callback for X.509 CRT verification"\


Style:

- -c "use CA callback for X.509 CRT verification"\ + -c "use CA callback for X.509 CRT verification" \

Patater · 2019-04-04T12:11:27Z

tests/ssl-opt.sh

+             key_file=data_files/server5.key" \
+            "$P_CLI ca_callback=1 debug_level=3 auth_mode=required" \
+            1 \
+            -c "use CA callback for X.509 CRT verification"\


Style:

- -c "use CA callback for X.509 CRT verification"\ + -c "use CA callback for X.509 CRT verification" \

Patater · 2019-04-04T12:11:38Z

tests/ssl-opt.sh

+             key_file=data_files/server5.key" \
+            "$P_CLI ca_callback=1 debug_level=3 auth_mode=optional" \
+            0 \
+            -c "use CA callback for X.509 CRT verification"\


Style:

- -c "use CA callback for X.509 CRT verification"\ + -c "use CA callback for X.509 CRT verification" \

tests/suites/test_suite_x509parse.function

programs/ssl/ssl_server2.c

tests/suites/test_suite_x509parse.function

include/mbedtls/x509_crt.h

Patater

LGTM

tests/suites/test_suite_x509parse.function

yanesca · 2019-04-10T08:57:20Z

@mpg Can you have a quick look at the last two commits and say if you are happy with them? (34 lines of mostly style fixes.)

mpg

I've reviewed the last two commits and they look OK to me!

sg0993 · 2019-11-26T09:41:27Z

How to download all the commits ?Can offer a url to download?

Patater · 2019-11-26T09:59:56Z

How to download all the commits ?Can offer a url to download?

Get the patch set from https://patch-diff.githubusercontent.com/raw/ARMmbed/mbedtls/pull/2532.patch
Navigate to the base of this branch, so the patch can apply cleanly. git checkout mbedtls-2.17.0
Use git am 2532.patch to apply the patch set

sg0993 · 2019-11-27T07:14:18Z

Hi Patater,
Thanks ,I have downloaded the patch ,but I found that the patch is based the 2.17.0,but I works in branch master.The code between them is different,such as the file:ssl_tls.c,the master do not has the fuction ssl_parse_certificate_verify,but patch should modify this .
So,as master branch,how to apply this patch?

Hanno Becker and others added 16 commits March 28, 2019 15:39

Add compile-time option to enable X.509 CA callbacks

288dedc

Add X.509 CRT verification API using trusted CA callbacks

5c8df78

Improve documentation of old X.509 CRT verification functions

902451d

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.

Add SSL configuration API for trusted CA callbacks

8bf74f3

Test for ca list callback

03cd120

Change callback name to ca_callback

912ed33

Add a failure testcase for ca callback

557426a

Add possibility to use ca_callbacks in ssl programs

1b4a2ba

Add X.509 CA callback to SSL configuration and implement setter API

5adaad9

Make use of CA callback if present when verifying peer CRT chain

afd0b0a

Implement X.509 CRT verification using CA callback

f53893b

Declare CA callback type even if feature is disabled

e15dae7

Minor fixes to CA callback tests

cbb5903

Only run X.509 CRT verification tests with CA callback tests if !CRL

0350d56

Add ssl-opt.sh tests for trusted CA callbacks

746aaf3

hanno-becker added enhancement mbed TLS team needs-review Every commit must be reviewed by at least two team members, component-tls component-x509 labels Mar 28, 2019

hanno-becker requested a review from mpg March 28, 2019 16:43

hanno-becker changed the title ~~Add support for trusted CA callbacks~~ [DRAFT] Add support for trusted CA callbacks Mar 28, 2019

hanno-becker added needs-design-approval and removed needs-design-approval labels Mar 28, 2019

Hanno Becker added 3 commits March 28, 2019 17:06

Update query_config.c

fa738d1

Remove trailing whitespace in test_suite_x509parse.function

3f932bb

Update version_features.c

fed5d9d

simonbutcher requested review from jarlamsa and simonbutcher March 28, 2019 17:18

jarlamsa previously approved these changes Apr 1, 2019

View reviewed changes

mpg requested changes Apr 1, 2019

View reviewed changes

mpg reviewed Apr 1, 2019

View reviewed changes

include/mbedtls/config.h Outdated Show resolved Hide resolved

Jarno Lamsa added 5 commits April 1, 2019 14:33

Change the verify function naming

31d9db6

Change the naming to reflect that the function uses a new ca callback feature to distinguish different callbacks.

Change docs according to review comments

f49fedc

Remove mbedtls_ from the static function name

2ee67a6

Address review comments regarding ssl_client2 and ssl tests

f7a7f9e

Address comments for x509 tests

dfd22c4

jarlamsa dismissed their stale review via dfd22c4 April 1, 2019 13:30

hanno-becker commented Apr 1, 2019

View reviewed changes

Fix name to function call

9822c0d

mpg reviewed Apr 2, 2019

View reviewed changes

mpg previously approved these changes Apr 2, 2019

View reviewed changes

Patater suggested changes Apr 4, 2019

View reviewed changes

yanesca added 2 commits April 5, 2019 16:44

Fix style issues and a typo

d7ecbd6

Document and test flags in x509_verify

846ae7a

yanesca dismissed mpg’s stale review via 846ae7a April 5, 2019 15:45

Patater approved these changes Apr 9, 2019

View reviewed changes

tests/suites/test_suite_x509parse.function Show resolved Hide resolved

mpg approved these changes Apr 10, 2019

View reviewed changes

yanesca added approved Design and code approved - may be waiting for CI or backports and removed needs-review Every commit must be reviewed by at least two team members, labels Apr 10, 2019

Patater merged commit 846ae7a into Mbed-TLS:development Apr 16, 2019

mpg mentioned this pull request Apr 18, 2019

add support for MBEDTLS_SSL_VERIFY_EXTERNAL authmode #2091

Closed

mpg mentioned this pull request Dec 29, 2020

The problem of cross certificate verification #3975

Closed

gilles-peskine-arm mentioned this pull request May 16, 2024

TLS 1.3: Add support for trusted certificate callback #9002

Open

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for trusted CA callbacks #2532

Add support for trusted CA callbacks #2532

hanno-becker commented Mar 28, 2019

mpg left a comment

mpg Apr 1, 2019

jarlamsa Apr 1, 2019

hanno-becker Apr 1, 2019

jarlamsa Apr 1, 2019

mpg Apr 2, 2019

hanno-becker Apr 2, 2019

mpg Apr 1, 2019 •

edited

mpg Apr 1, 2019

mpg Apr 1, 2019

jarlamsa Apr 1, 2019

hanno-becker Apr 1, 2019

hanno-becker left a comment

mpg Apr 2, 2019

mpg left a comment

Patater left a comment

Patater Apr 4, 2019

Patater Apr 4, 2019

Patater Apr 4, 2019

Patater Apr 4, 2019

Patater Apr 4, 2019

Patater left a comment

yanesca commented Apr 10, 2019

mpg left a comment

sg0993 commented Nov 26, 2019

Patater commented Nov 26, 2019

sg0993 commented Nov 27, 2019

Add support for trusted CA callbacks #2532

Add support for trusted CA callbacks #2532

Conversation

hanno-becker commented Mar 28, 2019

Summary

Background

Approach: CA callbacks

New X.509 API for CRT verification using CA callbacks

New SSL API for handshakes using CA callbacks

Limitations

mpg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mpg Apr 1, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hanno-becker left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mpg left a comment

Choose a reason for hiding this comment

Patater left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Patater left a comment

Choose a reason for hiding this comment

yanesca commented Apr 10, 2019

mpg left a comment

Choose a reason for hiding this comment

sg0993 commented Nov 26, 2019

Patater commented Nov 26, 2019

sg0993 commented Nov 27, 2019

mpg Apr 1, 2019 •

edited