server certificate selection callback

[gstrauss]

Description

This PR sketches out a server certificate selection callback which is run unconditionally near the end of ClientHello (and right before mbedtls internals choose and use server certificate)

Status

READY

Depends on: #5426

Requires Backporting

NO

Migrations

If there is any API change, what's the incentive and logic for it.

#5430: "Add an unconditonnal (sic) end-of-ClientHello callback"

Todos

• Tests
• Documentation
• Changelog updated

Additional comments

This PR sketches out a server certificate selection callback which is run unconditionally near the end of ClientHello (and right before mbedtls internals choose and use server certificate)

This leverages #5426: Add accessors to mbedtls_ssl_context: user data, version and is currently rebased onto that (not-yet-committed) branch.

Optional and included in this PR for interface discussion:

• reset/clear certificate list already set in ssl->handshake->sni_key_cert
  (potentially by other callbacks) by extending mbedtls_ssl_set_hs_own_cert()
• accessor to retrieve SNI during handshake (add mbedtls_ssl_get_hs_sni())

Discussion

Simple TLS extension callbacks work well when TLS extensions are assumed independent. However, that is no longer the case in common usage, even if it once was. SNI is required in TLSv1.3, and certificate selection may be influenced by:

• SNI
• supported_groups TLS extension ("elliptic_curves")
• some ALPN values (e.g. "acme-tls/1")
• (...)

mbedtls currently provides interfaces to allow support for simple callbacks to handle TLS extensions as if they were independent, and this works for many applications. This can be extended with additional callbacks, but that does not appear to be the most efficient or maintainable solution since it would add more code as new extensions are standardized.

A callback near the end of ClientHello processing -- which is called unconditionally after all TLS extensions are parsed, will scale better -- and will allow an application to react to a missing TLS extension, if one is required by the application.

Should this cert_cb callback take a (void *)p_arg? Or should new callbacks, including the cert_cb, rely on the user_data argument in #5426. This PR prefers using user_data.

The cert_cb needs access to the SNI. Should mbedtls save and provide access to the SNI value, or should consumers be required to use mbedtls_ssl_conf_sni() and save the value themselves? Having mbedtls provide an accessor means that a separate allocation just for saving SNI values can be avoided, and configuring a separate callback for SNI can be skipped. At the same time, this is a convenience, since there will already be a mechanism for the application to allocate its own structure to store SNI values and other data in ssl user_data in the SNI callback. I am leaning towards removing the accessor added in this PR since if the application does not need it, then setting the certificate in the SNI callback is sufficient, and if the application does need SNI elsewhere, then the application can use user_data.

One more change in this PR is extending mbedtls_ssl_set_hs_own_cert() to clear ssl->handshake->sni_key_cert if NULL is passed as certificate pointer. This may be useful if a certificate is configured in one of the callbacks, but then a later callback wishes to replace, rather than append to, the certificate list.

[gstrauss]

Two "no SNI callback" tests in tests/ssl-opt.sh fail due to the experimental change in this PR to always save the SNI value for later retrieval during the handshake via new function mbedtls_ssl_get_hs_sni(). The tests will need to be modified if this interface is kept.

[gstrauss]

@gstrauss I'm on holiday next week (leaving in 40 minutes), so I won't be able to react to comments or re-review, but rest assured that this will be one of my priorities when I get back.

Enjoy your holiday. I believe that I have addressed all items in your feedback, so this is PR is ready for you when you return.

[paul-elliott-arm]

Looks pretty good so far, awaiting Manuel's comments on a couple of the requested changes.

[gstrauss]

I added a commit to update doc in mbedtls/ssl.h to indicate that mbedtls_ssl_set_hs_own_cert() can be called from the certificate selection callback in addition to the SNI callback.

Please let me know if you would like me to rebase against latest tip of development, or if it is easier to re-review previous comments without the base changing. Also, please let me know if you'd like me to squash some of these commits together.

I can not see the Internal CI test results, so if there is something that needs to be adjusted in the PR, please share some hints.

[mpg]

Thanks for addressing my feedback. Looks almost good to me, just one more documentation change.

[gstrauss]

LMK if you would like me to squash any commits together, or to rebase on tip of development branch. Thx.

[paul-elliott-arm]

LGTM

[paul-elliott-arm]

LMK if you would like me to squash any commits together, or to rebase on tip of development branch. Thx.

Should be ok on rebase, given no conflicts. No need to squash, definitely.

[mpg]

LGTM. Let's wait and see if the CI is still happy. (Last time it was just pointing out that the ABI had changed, which is OK.)

[mpg]

CI passed except for the ABI-API component pointing out that the ABI has changed as a field was added to the public structure mbedtls_ssl_config, which is acceptable and expected.

[axos88]

@mpg, does this mean that one should use the new cert selection callback instead of the sni callback from now on? Is there anything the sni callback can do that the cert selection callback can not?

As far as I understand this is a superset of the sni cb, since it has access to all ssl extensions, so maybe that should be deprecated, or at least a hint added to the sni callback documentation?