Remove MBEDTLS_SSL_EXPORT_KEYS

[gilles-peskine-arm]

Remove the configuration option MBEDTLS_SSL_EXPORT_KEYS, making it always on.

We can do this in a minor version since its absence is not an observable aspect of the API.

Rationale

The option MBEDTLS_SSL_EXPORT_KEYS only gates the ability to set a callback. It does not change the behavior of the code.

Its impact on security is negligible: in order to introduce a vulnerability or facilitate exploitation, the application has to actively use the functionality. (The added function pointer in the SSL context is just one of many, so this does not facilitate exploitation to a non-negligible extent.)

The impact on code size is small: about 80B on an M0+ build, including the configuration function that doesn't get called unless the callback is used.

While we do test with the option disabled, this only happens in test-ref-configs.pl. Our testing missed at least one bug that happened in production: https://github.com/ARMmbed/mbedtls/issues/3998.

[hanno-becker]

I strongly object to this issue, because of code-size.

"Because of 80B? Are you joking?"

No, I'm not: Removing options and saying that it doesn't matter because each one of them saves only - say - 80B, is like stepping into the rain saying "A single raindrop doesn't get me wet!"

Put 10 of those changes together and you have a code-size impact which does matter for highly constrained devices.

I agree that there's a challenge with testing many options, and we will never get around combinatorial explosion, but removing options is the wrong way. We should instead put more thought in the choice of configurations that we test.

This is really a choice of what Mbed TLS is as a product (ping @daverodgman): If we want to be serious embedded TLS library, we can't afford having those changes creep in - which should much rather go the other way, just as we did for the baremetal branch, were we challenged every byte of code. That's in fact what we should strive for: Mbed TLS should be configurable so that if you do it correctly, the final binary should have nothing in it that you don't need.

[gilles-peskine-arm]

@hanno-arm You have unrealistic expectations about Mbed TLS. Manuel compiled a list of improvements from the baremetal branch that we could take. Making export optional would be about 5th from the bottom, i.e. far below the threshold that we'll ever get around to doing. And that's accounting for the whole cost of the option, not taking into account the code that the compiler can eliminate.

Combinatorial explosion of testing is a problem that you can't just solve by “putting more thought” into it. We have over a hundred boolean options. The total number of combinations is in the cryptographic range. It's literally infeasible to test them all. The fewer obscure combinations there are, the more of a chance we have to be able to reason about them all, let alone cover by testing.

We've put more than 80B of code in redundant checks that we want to have for security or maintainability. Oh, this function can't fail now, but it returns int in case a failure mode was added later — well, we need to put if( ret != 0 ) goto cleanup after calling it. This variable on the stack in a low-level internal function is realistically never going to remain live until it can be observed — but we still zeroize it. Mbed TLS is a security product, and it's portable. This limits what can be done to reduce the code size. You could write the functionality in assembly and fit it into far fewer kilobytes, but you'd have to say goodbye to maintainability and to security reviews.

[hanno-becker]

Combinatorial explosion of testing is a problem that you can't just solve by “putting more thought” into it. We have over a hundred boolean options. The total number of combinations is in the cryptographic range. It's literally infeasible to test them all. The fewer obscure combinations there are, the more of a chance we have to be able to reason about them all, let alone cover by testing.

That's what I said: You will never get around the combinatorial explosion, there must be thought into which configurations are worth testing. That being the case anyway, I don't see the grounds for removing worthwhile configuration options ("worthwhile" here being := guarding something that is not normally used, and which cannot be removed by the compiler or LTO if it's not used).

Mbed TLS must remain highly configurable, or, in the words of a contributor, maintain its "stellar customizability". Yes, testing is hard, and yes, perhaps some people are overwhelmed by the number of options. But the solution to neither of them is the removal of configuration options, in my view.

[AndrzejKurek]

While I agree with both sides of this discussion, I don't see a general trend in removing configurable options, so (hopefully) a rain of small codesize increases should not happen. This option, in combination with others, resulted in an untested behaviour (reported by me here: https://github.com/ARMmbed/mbedtls/issues/3998), so I'll prepare a quick preview PR for removing it, and we can discuss further from that point.