Properly support smaller fragment / record size

[rojer]

Suggested enhancement

After implementing incoming message defragmentation, effort should be made to properly support smaller record sizes by implementing support for max_fragment_length (RFC6066) as well as record_size_limit (RFC8449) extensions.

Justification

Mbed TLS needs this because it is popular on embedded platforms for which allocating 16K buffers is not always possible.

[rojer]

For a long time i have maintained a custom set of patches implementing defragmentation, fragmentation and fixing some issues around max fragment length extension (for mongoose-os and now internally for Shelly) - submitted some time ago in #8981.
Arguably the biggest part of that - incominghandshake message defragmentation - has already been merged, what;s left are smaller bits, but before resbmitting I'd like to get an agreement on the approach and potential API changes, since we now have two extensions .

• Mbed TLS already has a setting for max fragment size - mbedtls_ssl_conf_max_frag_len that the documentation acnowledges doesn't work properly on the client side but actually doesn't work on the server size either, since afetr successful negotiation the clients expect us to fragment the messages and error out if we don't.

• Then there is mbedtls_ssl_set_mtu that is kind of the same but not really.

• We have the new record_size_limit extension that is not in the picture at all but should be, somehow.

As I mentioned, we have been using max_fragment_length extension patches successfully with TLS 1.2 in production for a long time now. Support of it exists and is relatively widespread (notably OpenSSL supports it since 1.1.0) with equally notable exceptions (Google, for example).

My quick survey a couple months ago showed that nobody supports record_size_limit on the server yet, but some servers support max_fragment_length for 1.3.

Based on the above, my suggestion is to:

• Include record_size_limit and always include it with both 1.2 and 1.3 client hello message if set explicitly via mbedtls_ssl_set_mtu() or the size of the input buffer is less than 16K.
• If MTU or buffer size happen to match one of the allowed values for max_fragment_length, include that too (so that values for both extensions are always consistent).
• Deprecate mbedtls_ssl_conf_max_frag_len() as it's just another way of setting maximum desired input message size.

Tagging @mpg for discussion.

[rojer]

@minosgalanakis i will provide the wanted help but i'd like to agree on the approach first so as not to waste effort

[minosgalanakis]

@rojer

First off, thank you for your contribution and for actively engaging with this—it's much appreciated.

As you may have noticed, we’ve been going through a lengthy process of splitting the Mbed TLS and TF-PSA Crypto functionality into separate repositories. The upcoming release will be heavily focused on the remaining work related to that split.

The good news is that this presents an opportunity to revise the public-facing API in the next release, assuming we can agree on a design.

The less good news is that we likely won’t have the bandwidth to drive those changes ourselves. That said, we’d be happy to accept small, well-scoped patches—especially those that come with accompanying tests. In fact, during the recent set of PRs (#9989, #10007, #10021, #10045), we extended our test suite to better support the kind of defragmentation work you’re contributing. Thanks to that, it should now be easier to test similar functionality in new PRs.

From what I understand, you're aiming to extend functionality on the TLS and X.509 side of the code, which resides in the Mbed TLS repository under development?

My suggestion would be to focus on TLS 1.3 first, starting with changes that are tightly scoped and low-impact and moving toward more involved updates as needed. We can then revisit TLS 1.2 support if there’s a need.

Out of curiosity, which configuration or use case is most important for your work?

[rojer]

thanks @minosgalanakis

yes, point about testing is taken, i will make sure our future changes are accompanied by tests.

From what I understand, you're aiming to extend functionality on the TLS and X.509 side of the code, which resides in the Mbed TLS repository under development?

Yes, in particular extending support for smaller TLS record size - negotiation via extensions, and fragmentation on the way out as required (reassembly has already been dealt with).

My suggestion would be to focus on TLS 1.3 first, starting with changes that are tightly scoped and low-impact and moving toward more involved updates as needed. We can then revisit TLS 1.2 support if there’s a need.

ack. though as with reassembly, this is layer is common to both, only extension sets are potentially different (but my argument is that they too should be the same).

Out of curiosity, which configuration or use case is most important for your work?

we operate on embedded devices with very small amount of usable memory, support for smaller fragment length is vital to be able to maintain more than one concurrent TLS connection (1/2 persistent cloud connections and transient HTTPS fetches). buffer bloat during OTA updates with server sending a stream of 16K records used to be a big problem for us, until we implemented max_fragment_length on the client (mbedtls) and server (openssl).

so, as step 1, and before i send my patches, i would like to discuss the potential public API change: should mbedtls_ssl_set_mtu() become the only way of setting the desired maximum record size for the connection, both TLS and DTLS? for DTLS it will continue to work as before, for TLS, if set to < 16K, it will produce ClientHello extensions trying to negotiate the desired record_size_limit and max_fragment_length, if applicable.

[minosgalanakis]

Hi @rojer , apologies for the delay in getting back to you — the team has been quite focused on the recent repository split and the Mbed TLS 4.0.0 and TF-PSA-Crypto 1.0.0-beta releases.

I recently had a chat with @davidhorstmann-arm about how we might help incorporate your proposed changes, if you’re still interested in contributing to this feature.

I’ll let David follow up with more details.

[rojer]

@minosgalanakis yes, we are still interested and i (or one of my colleagues) can contribute to it.

[davidhorstmann-arm]

Hi @rojer,

Firstly, apologies for the delay in replying.

I think this is a good proposal in general but I have a few thoughts:

• Since the 4.0 release has happened, we can't remove or change the existing API. However, we could certainly add a new function that takes a maximum record size and figures out which extensions to use to request it (either max_frag_len or record_size_limit depending on the version of TLS).

• I think it would anyway make sense to retain the individual 'low-level' functions for people who know whether they are using MFL or RSL on both ends and want to be able to set them directly.

• Including both RSL and MFL could be a tricky area. If I understand the standards right then we are allowed to include different values for each (and RSL will be used if supported, otherwise MFL) - from RFC 8449:

  A server that supports the "record_size_limit extension MUST ignore a "max_fragment_length" that appears in a ClientHello if both extensions appear.

  If we only send MFL when the configured size happens to match, we can have a problem in the following case:

  1. The user sets the maximum record size to 2048 which is a valid MFL
  2. They happily connect to a server that supports MFL but not RSL
  3. They discover some spare memory down the back of the sofa and decide to allocate a bigger buffer - 3072 bytes
  4. Mbed TLS no longer sends MFL because the size no longer matches one of the prescribed MFL sizes
  5. Suddenly they cannot connect even though they have increased the size of the record buffers

  So I think it would make sense for us to always send both extensions and have the MFL extension indicate the largest size possible less than the configured size but the RSL indicate the size itself. That way, the client never gets larger records than it can handle.

• I don't think it would make sense to both have a new function and figure out what size to request based on the size of the input buffer. This creates multiple ways to do the same thing, which I'm not a big fan of. I think we should either:

  • Make the user request a maximum size explicitly, or
  • Set the maximum size automatically based on the buffer size

What do you think?

I will also ask @ronald-cron-arm to take a look and comment if he has some time, as he has much more in-depth knowledge in this area than I do.