Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for trusted CA callbacks #2532

Merged
merged 29 commits into from Apr 16, 2019

Conversation

@hanno-arm
Copy link
Contributor

commented Mar 28, 2019

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.

hanno-arm and others added some commits Mar 27, 2019

Improve documentation of old X.509 CRT verification functions
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 prototype for CRT verification with static and dynamic CA list
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.

@hanno-arm hanno-arm requested a review from mpg Mar 28, 2019

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

@sbutcher-arm sbutcher-arm requested review from jarlamsa and sbutcher-arm Mar 28, 2019

@sbutcher-arm

This comment has been minimized.

Copy link
Collaborator

commented Mar 29, 2019

Unfortunately the CI for both the pr-head and pr-merge jobs are failing at the same point:


  CC    test_suite_pkcs1_v21.c
make[1]: *** [ssl/ssl_client2] Error 1
make[1]: *** Waiting for unfinished jobs....
Makefile:223: recipe for target 'ssl/ssl_client2' failed
  CC    test_suite_cipher.gcm.c
Makefile:15: recipe for target 'programs' failed
make: *** [programs] Error 2
make: *** Waiting for unfinished jobs....
^^^^test_check_params_without_platform: build+test: MBEDTLS_CHECK_PARAMS without MBEDTLS_PLATFORM_C: command make CC=gcc CFLAGS=-Werror -O1 all test -> 2^^^^

******************************************************************
* test_check_params_without_platform: Done, cleaning up 
* Fri Mar 29 12:17:52 UTC 2019
******************************************************************

Marking as needs: Work

Fix ssl_client2 and ssl_server2 if !PLATFORM_C
The CA callback changes introduce mbedtls_calloc() and
mbedtls_free() to ssl_client2 and ssl_server2, which
wasn't defined unless MBEDTLS_PLATFORM_C was set.
@hanno-arm

This comment has been minimized.

Copy link
Contributor Author

commented Mar 30, 2019

@sbutcher-arm Thanks. Pushed a fix to address this.

@hanno-arm

This comment has been minimized.

Copy link
Contributor Author

commented Mar 30, 2019

@jarlamsa Could you please re-review?

@mpg
Copy link
Contributor

left a comment

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.

*
* \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.

This comment has been minimized.

Copy link
@mpg

mpg Apr 1, 2019

Contributor

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.

Copy link
@jarlamsa

jarlamsa Apr 1, 2019

Contributor

Changed

This comment has been minimized.

Copy link
@hanno-arm

hanno-arm Apr 1, 2019

Author Contributor

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

Copy link
@jarlamsa

jarlamsa Apr 1, 2019

Contributor

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.

Copy link
@mpg

mpg Apr 2, 2019

Contributor

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

Copy link
@hanno-arm

hanno-arm Apr 2, 2019

Author Contributor

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

Show resolved Hide resolved include/mbedtls/x509_crt.h Outdated
Show resolved Hide resolved tests/suites/test_suite_x509parse.data Outdated
#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;

This comment has been minimized.

Copy link
@mpg

mpg Apr 1, 2019

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

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

This comment has been minimized.

Copy link
@mpg

mpg Apr 1, 2019

Contributor

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.

Show resolved Hide resolved tests/suites/test_suite_x509parse.function
Show resolved Hide resolved tests/suites/test_suite_x509parse.function Outdated
Show resolved Hide resolved tests/suites/test_suite_x509parse.function Outdated
Show resolved Hide resolved tests/ssl-opt.sh Outdated
@@ -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

This comment has been minimized.

Copy link
@mpg

mpg Apr 1, 2019

Contributor

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.

Copy link
@jarlamsa

jarlamsa Apr 1, 2019

Contributor

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

Show resolved Hide resolved include/mbedtls/config.h Outdated
* \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.

This comment has been minimized.

Copy link
@hanno-arm

hanno-arm Apr 1, 2019

Author Contributor

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

@hanno-arm
Copy link
Contributor Author

left a comment

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.

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

This comment has been minimized.

Copy link
@mpg

mpg Apr 2, 2019

Contributor

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
Copy link
Contributor

left a comment

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

@Patater
Copy link
Collaborator

left a comment

Mostly minor style comments, plus a question or two.

Please add any necessary ChangeLog entries.

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

This comment has been minimized.

Copy link
@Patater

Patater Apr 4, 2019

Collaborator

Style

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

This comment has been minimized.

Copy link
@Patater

Patater Apr 4, 2019

Collaborator

Style:

-             -c "use CA callback for X.509 CRT verification"\
+             -c "use CA callback for X.509 CRT verification" \
"$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"\

This comment has been minimized.

Copy link
@Patater

Patater Apr 4, 2019

Collaborator

Style:

-             -c "use CA callback for X.509 CRT verification"\
+             -c "use CA callback for X.509 CRT verification" \
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"\

This comment has been minimized.

Copy link
@Patater

Patater Apr 4, 2019

Collaborator

Style:

-             -c "use CA callback for X.509 CRT verification"\
+             -c "use CA callback for X.509 CRT verification" \
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"\

This comment has been minimized.

Copy link
@Patater

Patater Apr 4, 2019

Collaborator

Style:

-             -c "use CA callback for X.509 CRT verification"\
+             -c "use CA callback for X.509 CRT verification" \
Show resolved Hide resolved tests/suites/test_suite_x509parse.function Outdated
Show resolved Hide resolved programs/ssl/ssl_server2.c
Show resolved Hide resolved tests/suites/test_suite_x509parse.function
Show resolved Hide resolved tests/suites/test_suite_x509parse.function Outdated
Show resolved Hide resolved include/mbedtls/x509_crt.h
@Patater

Patater approved these changes Apr 9, 2019

Copy link
Collaborator

left a comment

LGTM

@yanesca

This comment has been minimized.

Copy link
Collaborator

commented Apr 10, 2019

@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

mpg approved these changes Apr 10, 2019

Copy link
Contributor

left a comment

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

@Patater Patater merged commit 846ae7a into ARMmbed:development Apr 16, 2019

3 checks passed

continuous-integration/jenkins/pr-head This commit looks good
Details
continuous-integration/jenkins/pr-merge This commit looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.