Introduce SSL getter API for handshake state

[hanno-becker]

Context: This was raised by Jeremy Audiger on the Mbed TLS mailing list.

Issue: When we make mbedtls_ssl_context internal, there is no supported way of extracting the handshake state. Given that we do expose the fact that the handshake happens in steps via mbedtls_ssl_handshake_step(), there should arguably be a public getter function that allows to retrieve the handshake state.

[neoxic]

@mpg

Regarding exposing the state with a "no guarantee" warning - Gilles made his point again while I was writing and I think it's a really strong point.

So, the outcome of this discussion (plus some internal discussions) is that we favour approaches based on callbacks.

Fair enough. The path MbedTLS is taking is now clear. Unfortunately, it looks like the problem with a session-based DTLS server that I tried to shed some light upon above will become buried even deeper with callbacks. Moreover, since there is no easy transition path between handshake_step() and callbacks, supporting both 2.x and 3.x without resorting to "unstable APIs" becomes a real pain (unless the new approach is going to be back-ported, of course).

[mpg]

@gstrauss

@mpg I have not traced through the code to understand the DTLS flows, so this is a question: ssl->handshake->verify_cookie_len is set in ssl_parse_client_hello() (if ssl->renego_status == MBEDTLS_SSL_INITIAL_HANDSHAKE) before ssl->conf->f_cert_cb(). However, later in the handshake in ssl_write_server_hello(), verify_cookie_len is checked and ssl_write_hello_verify_request() is sent. That looks like it should stay in ssl_write_server_hello(), but can it also be called in ssl_parse_client_hello(), short-circuiting ssl_parse_client_hello(), when ssl->handshake->verify_cookie_len is set in ssl_parse_client_hello()?

I think that's correct, but I was thinking of a smaller change, like:

diff --git a/library/ssl_srv.c b/library/ssl_srv.c
index 094fca8932d7..7379b6df18ca 100644
--- a/library/ssl_srv.c
+++ b/library/ssl_srv.c
@@ -1875,7 +1875,10 @@ read_record_header:
     /*
      * Server certification selection (after processing TLS extensions)
      */
-    if( ssl->conf->f_cert_cb && ( ret = ssl->conf->f_cert_cb( ssl ) ) != 0 )
+    if( ssl->conf->f_cert_cb &&
+        !( ssl->conf->transport == MBEDTLS_SSL_TRANSPORT_DATAGRAM &&
+        ssl->handshake->verify_cookie_len != 0 ) &&
+        ( ret = ssl->conf->f_cert_cb( ssl ) ) != 0 )
     {
         MBEDTLS_SSL_DEBUG_RET( 1, "f_cert_cb", ret );
         return( ret );

(or something equivalent with better formatting and a comment).

Musing: Were there to be a new accessor available to check the cookie status, then the new accessor could be called from f_cert_cb before bind() and connect() the DTLS socket (and skip further certificate selection logic if ssl_write_hello_verify_request() is to be called later from ssl_write_server_hello()

I agree, I think that was (2) in the first paragraph of this comment. I think both ways would work, though right now I have a slight preference for (1) because I think it's just simpler.

[mpg]

@neoxic

Unfortunately, it looks like the problem with a session-based DTLS server that I tried to shed some light upon above will become buried even deeper with callbacks.

I agree that's not ideal. OTOH, like someone said over in the OpenSSL discussion, I think the "right" way to solve the underlying issue is neither callbacks nor stepping the handshake, but a dedicated BIO implementation that would probably use a single unconnected socket and handle the dispatch. There's a mismatch between what the DTLS layer expects and what the sockets API provides, and bridging that gap should be the BIO callback's job. And ideally it should be provided by the DTLS library (or at least a working example of how to go about it), rather that written over and over again by each application developer. Unfortunately, considering all the other things on our plate, that's unlikely to happen soon.

If you ever have the right combination of time and motivation and feel like submitting a PR for this, please get in touch on the mailing list first to discuss the general design and the planning (our review bandwidth tends to be a bottleneck, so for large/complex contributions it's better if the review effort is budgeted in advance).

[mpg]

@neoxic

supporting both 2.x and 3.x without resorting to "unstable APIs" becomes a real pain (unless the new approach is going to be back-ported, of course).

As a matter of principle, I think we could consider back-porting selected features in order to make supporting both 2.x and 3.x easier, but unfortunately in this case the changes affect the ABI, which we "try very hard" not to break in LTS branches - meaning we'd only break the ABI in an LTS if that was the only way we found to fix a security vulnerability.

Sorry about that, I hear your pain, but I don't think we can do anything to relieve it now. (Perhaps in the future we release 4.0 we can wait a bit before turning the latest 3.x to LTS status, to leave time for similar issues to surface and find solutions that make it less of a pain to support both 3.x and 4.x.)

[neoxic]

@mpg

I agree that's not ideal. OTOH, like someone said over in the OpenSSL discussion, I think the "right" way to solve the underlying issue is neither callbacks nor stepping the handshake, but a dedicated BIO implementation that would probably use a single unconnected socket and handle the dispatch.

Personally, I think complicating the implementation of BIO is a dead-end approach. And the exact reason I abandoned my attempts to incorporate OpenSSL in favour of MbedTLS was that:

1. MbedTLS has (at least for now) a way to give control to the user (me) over how I want my BIO to deal with the difficulties and subtleties that stem from OS-based inconsistencies;
2. OpenSSL developers are stuck in babysitting users, i.e. solving unrelated tasks, thus this particular problem is unlikely to be ever solved. My hopes MbedTLS is not going to follow suit...

If you ever have the right combination of time and motivation and feel like submitting a PR for this, please get in touch on the mailing list first to discuss the general design and the planning (our review bandwidth tends to be a bottleneck, so for large/complex contributions it's better if the review effort is budgeted in advance).

Yes, that was on my TODO list. In particular, I planned to develop and donate a correctly working example as part of the MbedTLS example suite. But... Since we hit a major snag with MbedTLS 3.0 in regards to handshake_step(), these plans are now on hold until some feasible approach is on the horizon for I don't believe you will favour using "unstable APIs" in your examples.

EDIT:

There's a mismatch between what the DTLS layer expects and what the sockets API provides, and bridging that gap should be the BIO callback's job. And ideally it should be provided by the DTLS library (or at least a working example of how to go about it), rather that written over and over again by each application developer. Unfortunately, considering all the other things on our plate, that's unlikely to happen soon.

Indeed, there is much more sense in what you are saying than modifying a totally unrelated server certificate selection callback. Since there are already things like mbedtls_ssl_conf_dtls_cookies() and mbedtls_ssl_set_client_transport_id() that work only in case of a DTLS server, it might be natural to just add something like:

typedef int mbedtls_ssl_greeter_t(void *ctx);

void mbedtls_ssl_set_greeter(mbedtls_ssl_context *ssl, void *ctx, mbedtls_ssl_greeter_t *func);

... which again makes sense only for DTLS servers and triggers func on a successful ClientHello. This way, it will also be backward-compatible and easily back-portable. Furthermore, a dedicated callback like this will very much expose the bind/connect problem rather than suppress it with the unrelated server certificate callback.

[gstrauss]

I think that's correct, but I was thinking of a smaller change, like: [snip]

Musing: Were there to be a new accessor available to check the cookie status, then the new accessor could be called from f_cert_cb before bind() and connect() the DTLS socket (and skip further certificate selection logic if ssl_write_hello_verify_request() is to be called later from ssl_write_server_hello()

I agree, I think that was (2) in the first paragraph of this comment. I think both ways would work, though right now I have a slight preference for (1) because I think it's just simpler.

f_cert_cb is intended to be called unconditionally, unless there is a fatal error in processing the client hello before that point. Your simple patch would add an exception, which I do not think is the best design choice here. If the handshake proceeds (it does), then f_cert_cb should be called to be able to perform certificate selection and any other application-level policy that should occur after the ClientHello has been processed. @mpg, I think your suggestion ((2) in the first paragraph of this comment) is a better solution, though other alternatives might also be proposed by those who know DTLS better than I. A place for DTLS bind() and connect() by the application might be created elsewhere, before the ServerHello or any other response is sent to the client.

[paul-elliott-arm]

Added https://github.com/ARMmbed/mbedtls/pull/5653 to implement mbedtls_ssl_is_handshake_over(), as discussed above. Not setting it to close this issue, as there is still conversation going on here about other problems aroundt this area.

[minosgalanakis]

As discussed in issue and subsequent design review issue https://github.com/Mbed-TLS/mbedtls/issues/8529, the handshake and handshake state should not be directly exposed.

As mentioned above exposing the internal state does not guarantee consistent behaviour across different versions.

We have added callbacks at #5454 and #8456 and stepping the handshake is possbile. We are not considering adding a dedicated BIO implementation at this stage.

I will be closing this issue, but please feel free to re-open or write a new-one if there is still a gap that we need to consider moving forward.