Allow per-context SNI callback (state)

[mpg]

Currently the SNI callback, and its associated state, can only be configured per-mbedtls_ssl_config, not per-mbedtls_ssl_context. When the separation between config and context appeared in Mbed TLS 2.0, it was not always clear what would in practice be shared by several contexts (hence, should go in config), and what would more often be context-specific. Turns out there is interest in having per-ssl-context specific SNI, as originally reported in #5331

It remains to be clarified whether just having a per-ssl_context state pointer for the SNI callback would be enough, or if it's desirable to also make the function pointer itself per-ssl-context.

In terms for API, we could add a new function mbedtls_ssl_set_sni(), coming in addition to the existing mbedtls_ssl_conf_sni() which wouldn't be deprecated. The SNI callback would be taken from the ssl_context if set there, or from the ssl_config otherwise.

[mpg]

Quoting from a comment on another issue:

Another item which could use improvement in stepping handshake and which is very visible on server-side use of mbedTLS: some callbacks are only for mbedtls_ssl_config, and do not have a corresponding callback on mbedtls_ssl_context. This is not thread-safe when mbedtls_ssl_config is shared. Prime example: mbedtls_ssl_conf_sni() is for mbedtls_ssl_config, and there is no SNI callback override for specific mbedtls_ssl_context (!)

@gstrauss It's not immediately clear to me what the thread safety issue would be. The model I have in mind is that the SNI callback function has some state that it only reads (so it's not a problem for that state to be shared between threads), such as a list of available certificates; it goes over that list and picks a certificate then calls mbedtls_ssl_set_hs_own_cert() or some other ssl_set_hs function that operates on the current ssl_context (which is not shared between threads). Clearly that mental model does not cover your use case. Could you clarify what I'm missing?

Also, what do you think about this:

It remains to be clarified whether just having a per-ssl_context state pointer for the SNI callback would be enough, or if it's desirable to also make the function pointer itself per-ssl-context.

[gstrauss]

It remains to be clarified whether just having a per-ssl_context state pointer for the SNI callback would be enough, or if it's desirable to also make the function pointer itself per-ssl-context.

For lighttpd mod_mbedtls, making the pointer itself per-ssl-context would be a sufficient improvement. Right now, I overwrite the callback each and every time I enter the handshake in order to manually provide a per-ssl-context. When the ssl_config is shared among multiple connections all performing handshakes, this is not thread-safe unless access to the ssl_config is serialized.

[mpg]

Yes, I agree that modifying ssl_config once it's associated with one or more ssl_contexts is not thread-safe, and the documentation says so (actually, there are more issues than just thread-safety here, the code assumes the config to be stable over time).

I still don't think I understand why it is that you need per-context state for your SNI callback. I agree that's something that's not achievable in a safe way with the current API, but it's just not clear to me why you have this need. (Not trying to push back on adding the feature, as it's a fairly simple on, just trying to understand better how Mbed TLS is used, hopefully that will help designing future APIs better.)

[gstrauss]

I still don't think I understand why it is that you need per-context state for your SNI callback.

Let me try to describe the server-side flow in more detail. First, lighttpd is a server. For simplicity, assume a single listening socket. In this case, there is one ssl_config that lighttpd shares among all client TLS connections to lighttpd. This shared ssl_config is not modified, with the only unfortunate exception being to override the ssl_config SNI callback every time lighttpd steps the handshake. The ssl_config SNI callback is modified each time in order to provide a different lighttpd hctx as callback data to the SNI callback. lighttpd hctx is used per connection since the SNI is per connection.

While I could maintain a separate lookup table to access lighttpd hctx by file descriptor, the primary point of per-context callback data for SNI is that SNI is per-context, not per-config. This concept should be extended to all TLS extensions, since the TLS extensions apply to the connection. The reason that lighttpd does not need this for ALPN, too, is that lighttpd manually processes the entire Client Hello for TLS extensions for ALPN before mbedtls processes Client Hello. lighttpd needs to do this since mbedtls does not expect certificate selection to occur twice. Since lighttpd can not set the server certificate in both the SNI callback and the (non-existent) callback for ALPN, lighttpd parses the ALPN so that when SNI occurs, certificate selection can be made once. (FYI: for ALPN "acme-tls/1", lighttpd selects a different challenge certificate for www.example.com, rather than the already-provisioned certificate for www.example.com)

Since TLS extensions may occur in any order, there is a need (on the server-side) to collect the results of the callbacks into a per-context data structure (lighttpd mod_mbedtls hctx) and then to process them at the end of the TLS Client Hello (which is another feature request in #5331). All the hoop-jumping described above is done because mbedtls does not have a callback at the end of Client Hello, from which such a callback could query the results of all of the TLS extensions and perform such things as certificate selection.

An alternative suggestion to SNI per-context callback or per-context callback data, is to have a single pointer per-context which is passed as the data pointer to all TLS extension callbacks. Perhaps if this pointer is non-NULL, this is used for TLS extension callbacks rather than the data pointer registered with the per-config callback.

Personally, I would love to see both per-context data for TLS extension callbacks (or a single per-context data pointer, if set, used for all TLS extension callbacks) and a new callback at the end of Client Hello (and before Server Hello) (or, equivalently, a callback at the beginning of Server Hello).

[gilles-peskine-arm]

a single pointer per-context which is passed as the data pointer to all TLS extension callbacks

I think I'm missing something here: couldn't you achieve the same thing by passing the same pointer to all the callbacks?

But anyway I think the best solution to this particular would be to add a public user pointer to mbedtls_ssl_context, which the library would initialize to NULL and otherwise never modify. There should also be callbacks that run during context setup and abort, since applications might need to allocate/free a resource at this point. And then all existing callbacks that receive a mbedtls_ssl_context* as argument could read (and even modify) that pointer.

[gstrauss]

I think I'm missing something here: couldn't you achieve the same thing by passing the same pointer to all the callbacks?

Where there are callbacks, that is what I do. However, I need that data to be per-context, not per-config.

But anyway I think the best solution to this particular would be to add a public user pointer to mbedtls_ssl_context, which the library would initialize to NULL and otherwise never modify. There should also be callbacks that run during context setup and abort, since applications might need to allocate/free a resource at this point. And then all existing callbacks that receive a mbedtls_ssl_context* as argument could read (and even modify) that pointer.

Yes, that would be a useful solution, and is what some other TLS libraries do, e.g. openssl SSL_get_app_data() and gnutls gnutls_session_get_ptr(). I am not sure about the need for callbacks that run during context setup and abort since the application already needs to handle allocation and clean up of mbedtls_ssl_context, e.g. clean up with mbedtls_ssl_free() or mbedtls_ssl_session_reset().

[gilles-peskine-arm]

I am not sure about the need for callbacks that run during context setup and abort since the application already needs to handle allocation and clean up of mbedtls_ssl_context, e.g. clean up with mbedtls_ssl_free() or mbedtls_ssl_session_reset().

Good point, I'd forgotten that Mbed TLS never allocates a context internally.

[gstrauss]

But anyway I think the best solution to this particular would be to add a public user pointer to mbedtls_ssl_context, which the library would initialize to NULL and otherwise never modify.

Yes! Can this issue be reworded to this solution instead of "Allow per-context SNI callback (state)"?

[gilles-peskine-arm]

I added a user pointer in #5426. I'm not sure this resolve this issue: I get the impression we should also add a new callback. But regardless it's generally useful and extremely cheap.

[mpg]

I think adding a new callback is a different issue - #5430 - so I'd say #5426 resolves this one, just not the other.