TLS handshake fragmentation support

[rojer]

Description

Adds support for max_frag_len extension in TLS mode.

Consists of two parts:

• The change itself - fragmenting records on the way out, defragmenting on the way in
• A fix for max_frag_len negotiation on the client side: if the server ignores the extension, we must fall back to the maximum value, irrespective of what was configured.

This approach was extensively tested in production in a big fleet of devices. However, this exact code is new (i'm forward-porting patches to 2.16 which was our previous base version).

Fixes https://github.com/Mbed-TLS/mbedtls/issues/1840

PR checklist

Note: this is not a finished product. If there's agreement in principle, i will add tests, changelog, etc.

Please tick as appropriate and edit the reasons (e.g.: "backport: not needed because this is a new feature")

• [ ] changelog - TODO
• [x] 3.6 backport no: ABI break (also, new feature)
• [x] 2.28 backport no: ABI break (also, new feature)
• [ ] tests - TODO

Notes for the submitter

Please refer to the contributing guidelines, especially the checklist for PR contributors.

Help make review efficient:

• Multiple simple commits
  • please structure your PR into a series of small commits, each of which does one thing
• Avoid force-push
  • please do not force-push to update your PR - just add new commit(s)
• See our Guidelines for Contributors for more details about the review process.

[paul-elliott-arm]

CI Failed, as under some configs:

ssl_msg.c:3330:22: error: unused variable 'msg_hslen' [-Werror=unused-variable]
          const size_t msg_hslen = (hs_remain <= ssl->in_msglen ? hs_remain : ssl->in_msglen);

(You can see full test results by clicking on the Details link for the OpenCI PR Tests below)

[rojer]

yes, looks like a temporary variable was unused when building without debug. fixed.

but unused variables aside, does the overall approach look good?

[rojer]

addressed comments, rebased

[cryi]

Hi, any idea when this is going to land? This prevents us from moving to 3.6.0.

[gilles-peskine-arm]

@cryi Due to the size of the feature, I'm afraid we can't add it to the 3.6 long-time support branch. Since we are not planning any new 3.6 release, I'm afraid this won't be released before Mbed TLS 4.0, which is planned for in Q2 2025.

[cryi]

@cryi Due to the size of the feature, I'm afraid we can't add it to the 3.6 long-time support branch. Since we are not planning any new 3.6 release, I'm afraid this won't be released before Mbed TLS 4.0, which is planned for in Q2 2025.

Thank you @gilles-peskine-arm . ~~Is there any workaround without disabling tls1.3? Right now we experience failed handshakes with GitHub servers when downloading files.~~

Nvm I realized that solution is just not to use mbedtls_ssl_conf_max_frag_len at this point. We do not need it technically so we should be ok with 3.6.0. Thank you 👍🏻

[mpg]

Consists of two parts:

• The change itself - fragmenting records on the way out, defragmenting on the way in

I'd say that's already two parts, and arguably we should support defragmenting on the way in unconditionally. Could almost be two distinct PRs, except that for testing it's convenient to have both.

• A fix for max_frag_len negotiation on the client side: if the server ignores the extension, we must fall back to the maximum value, irrespective of what was configured.

I'm sorry, I'm not sure I understand here: are you talking about messages we send or messages we receive? Indeed if the server ignores the extension, then we're likely to receive messages larger than we asked for, but I don't see why that would prevent us from still sending messages in smaller fragments if we want to.

This approach was extensively tested in production in a big fleet of devices.

That's great to know! Unfortunately I don't have time to do a full review now, but this is good news, and the size of the patch is smaller than I expected for this feature, which is encouraging.

[rojer]

ok, so i take it that there is overall agreement on the approach - good to know. i will work on tests and TLS 1.3 compatibility (that i completely ignored until now, since we don't have it enabled in our build).

@mpg, i agree that defragmentation should be performed irrespective of whether fragment length is specified or not. somehow, this did not occur to me when i was working on it.

[rojer]

@brewkon ssl_tls13_process_server_hello - as mentioned, this was not tested at all with TLS 1.3, so i'm not surprised. try disabling MBEDTLS_SSL_PROTO_TLS1_3 for now. also make sure MBEDTLS_SSL_MAX_FRAGMENT_LENGTH is enabled and mbedtls_ssl_conf_max_frag_len is set.

[rojer]

yes, rebasing, making sure it works with 1.3 and adding automated tests is what needs to be done here to get this ready. this PR was kind of an rfc about the overall approach, now it needs more work to get to a mergeable state. no eta on that, as i'm pretty busy with other stuff atm.