Remove MBEDTLS_SSL_RECORD_CHECKING

[chris-jones-arm]

Context

The configuration option MBEDTLS_SSL_RECORD_CHECKING only controls compilation of one function (mbedtls_ssl_check_record) used in DTLS to check a buffer's validity and authenticity. This option/function is enabled in the default configuration and so is often used in practice as it serves a helpful purpose.

In terms of code size the function is quite small and can most likely be optimised out by the linker so the code size is negligible.

---

Rationale

Every compilation option has a maintenance cost behind it. The more options there are the fewer combinations of options we can test and so by reducing the amount of options we can increase our testing coverage. By keeping this option it would also increase the chance of it being misconfigured or broken in the future.

As the option provides a useful purpose but does not control a significant performance or code size trade-off the option should be removed and treated as if it were always enabled.

---

Work items for 3.0

• Remove the MBEDTLS_SSL_RECORD_CHECKING option from config.h.
• Remove all references to MBEDTLS_SSL_RECORD_CHECKING while ensuring that mbedtls_ssl_check_record is now always enabled in the library.

[hanno-becker]

This option/function is enabled in the default configuration and so is often used in practice as it serves a helpful purpose.

This is a quite new feature added for a specific customer and usecase. I have strong doubts it is in widespread use, and I'm therefore not convinced that it's a good idea to remove this option and enable it by default.

I would suggest to keep it and disable it by default in Mbed TLS 3.0.

Ping @mpg who was part of the development of this feature if I remember correctly.

[mpg]

If we try to evaluate this against the guidelines I try to sum up by e-mail recently, I think if fall in the category "trade-off between feature and code-size" for which the suggested decision tree looked like this:

• is the code size difference small enough?
  • yes -> remove (enabled)
  • no: is the feature useful? (may require asking on list)
    • yes: keep the option
    • no: remove (disabled), aka drop the feature entirely

Here I think the code size difference is zero as this is a single function so the linker can just garbage-collect it if it's not called. (And everyone who cares about code size uses link-time garbage collection.) So I think "remove (enabled)" is indeed the right thing to do.

Actually now that I think of it, I don't think that was ever a reason to make this a compile-time option, we probably just did it out of habit: new feature -> new compile-time option, which most of the time is a good rule of thumb as we don't want people who don't need the feature to pay the price in code size, but in this instance the price is zero which is definitely small enough :)

(But for the record, I agree with Hanno's point that this function is actually only useful for very specific cases. The point of this function is to check the validity of a record without consuming it which is something most people never need to do: applications usually just want to consume the record (ie get its content), and since the library is well-behaved it will automatically check the validity of the record before passing its content to the application.)

[hanno-becker]

@mpg I agree with of what you say: Link time GC will remove the function if it's not used.

However, this invalidates the canonical way in which (potential?) users would determine whether Mbed TLS is fit for a their use case: Build a custom config, compile, and look at the code-size.

If we rely on link time GC, then this no longer works, and users don't know what code-size they can expect until they've actually linked the library against their final application.

Now here the difference isn't large, but (a) things can accumulate, and (b) we've seen applications with extreme code-size budget, and there is risk Mbed TLS isn't seen as viable for those if the initial code-size exploration as described leads to an larger footprint.

Overall, I remain skeptical of the removal of customizability (which I've heard being called out by partners as something that makes Mbed TLS stand out) to ease the testing complexity. I don't want to re-open a discussion though that you might have had in the team already -- has this been considered @mpg @daverodgman?

[mpg]

However, this invalidates the canonical way in which (potential?) users would determine whether Mbed TLS is fit for a their use case: Build a custom config, compile, and look at the code-size.

I don't think that needs to work, and in fact it already doesn't. For example if you use an ECC-only build, currently you have no way of excluding mbedtls_mpi_exp_mod() even though it's not used for ECC and a pretty large function. So I think building a looking at the code size of the library just provides an upper bound. I think that's the case for most libraries and (RT)OSes.

What you describe is the way we (the Mbed TLS team, as opposed to the integration team or the customer) measured our progress. It was a very useful tool for us, but I think we shouldn't give it more importance that it deserves.

Regarding the importance of customizability, I agree that's something we need to be careful about. But there's a trade-off between that and complexity*, and we need to find the right balance. In this specific instance, I think the gain of having this particular option is simply not worth the cost of having yet another config.h option. (The "it adds up" argument goes both ways.)

*Also, I think there's a trade-off with usability, though that's not relevant in this specific instance. I think we've both seen custom configs by some users that just didn't make a lot of sense. (Especially when it comes to controlling trade-offs between speed, ram, and code size in crypto algs, we have too many low-level options that are hard for users to discover and understand, we should have a better scheme for that, but that's outside the scope of 3.0 (unfortunately).)

[hanno-becker]

@mpg Thank you for your detailed reply. I remain wary about giving up customizability, but your points are very good and valid, so I'm ok moving forward with this issue (not that this would be a requirement, but just for the record).

[mpg]

Thanks for agreeing to move forward with this issue. I think your point that customizability is something our users appreciate and that we shouldn't remove too much of it is indeed something to keep in mind.

Another thing I forgot to mention: if we find out later that we went too far and that in a few instances we should have kept the option, it's always possible to re-introduce it in any 3.x release. By contrast, we have very few opportunities to remove options.

[hanno-becker]

Good point again.