Add TLS 1.3 ciphersuite and key exchange identifiers and API

[hanno-becker]

Summary: This PR fixes #4328 on the introduction of TLS 1.3 ciphersuite and key exchange identifiers.

API design considerations: The concept of TLS 1.2 and TLS 1.3 ciphersuites are different. TLS 1.2 ciphersuites combine symmetric algorithms and key exchange, while TLS 1.3 ciphersuites only fix symmetric algorithms. TLS 1.3 key exchanges are configured differently. It has been considered whether we want to unify the 1.2+1.3 APIs by allowing to always configure symmetric algorithms and key exchanges separately, and have Mbed TLS infer what "TLS 1.2 ciphersuites" are suitable for that -- however, this is likely resulting in an API break and requires significant design discussion. I therefore propose to go with the simpler solution implemented in this PR, which keeps mbedtls_ssl_conf_ciphersuites() for TLS 1.2 + 1.3 ciphersuites, and adds mbedtls_ssl_conf_tls13_key_exchange_modes() for TLS 1.3 key exchange modes. We can still consider unification of the API in Mbed TLS 4.0, when we've had a bit of experience with the TLS 1.3 implementation.

TLS 1.3 Ciphersuites:

The PR adds IANA identifiers for TLS 1.3 cipher suites:

#define MBEDTLS_TLS1_3_AES_128_GCM_SHA256                     0x1301 /**< TLS 1.3 */
#define MBEDTLS_TLS1_3_AES_256_GCM_SHA384                     0x1302 /**< TLS 1.3 */
#define MBEDTLS_TLS1_3_CHACHA20_POLY1305_SHA256               0x1303 /**< TLS 1.3 */
#define MBEDTLS_TLS1_3_AES_128_CCM_SHA256                     0x1304 /**< TLS 1.3 */
#define MBEDTLS_TLS1_3_AES_128_CCM_8_SHA256                   0x1305 /**< TLS 1.3 */

It also adds corresponding internal ciphersuite definitions to ssl_ciphersuites.c. Since the concept of TLS 1.3 ciphersuites does not imply a key exchange, the corresponding field in mbedtls_ssl_ciphersuite_t is defined as MBEDTLS_KEY_EXCHANGE_NONE.

TLS 1.3 ciphersuites have been added at the top of the default ciphersuite list if MBEDTLS_TLS1_3_EXPERIMENTAL is enabled. Note that this does not break TLS 1.2 compatibility because unknown ciphersuites MUST be ignored.

The PR also expands the documentation of mbedtls_ssl_conf_ciphersuites() and explains that the function will not need to be modified to accommodate TLS 1.3, incl. downgradable TLS 1.3 / TLS 1.2 dual-builds, despite the different ciphersuite concepts. The only thing that will change for TLS 1.3 is that there needs to be a separate key exchange configuration API, which is not necessary in TLS 1.2. This will be added in the future.

TLS 1.3 key exchanges:

The PR adds identifiers MBEDTLS_SSL_TLS13_KEY_EXCHANGE_MODE_XXX for TLS 1.3 key exchange modes and a new API mbedtls_ssl_conf_tls13_key_exchange_modes() to configure them. It allows all key exchanges by default. ssl_client2 and ssl_server2 receive a new command line argument tls13_kex_modes to configure TLS 1.3 key exchange modes. Tests to ssl-opt.sh are added which exercise that those parameters are recognized, but nothing more yet.

[hanno-becker]

Note for reviewers: Instead of introducing new identifiers, we could try to embed the TLS 1.3 key exchanges into the TLS 1.2 key exchanges, e.g. mapping pure-PSK to MBEDTLS_KEY_EXCHANGE_PSK, pure-ephemeral to MBEDTLS_KEY_EXCHANGE_ECDHE_ECDSA, and PSK-ephemeral to MBEDTLS_KEY_EXCHANGE_ECDHE_PSK.

However, this would require re-defining mbedtls_ssl_key_exchange_t values in a way that's suitable for use as bitfields - at least, I find this simpler and shorter in code than requiring a 0-terminated list of key exchange identifiers in mbedtls_ssl_conf_tls13_key_exchange_modes(). We would likely need to remove the enum and define the values directly as macros, as is done here.

As pointed out by @gilles-peskine-arm, API issues can be avoided here: Either (a) We view mbedtls_ssl_key_exchange_t as outside the API because it's not featuring in any public structure or function anyway. Or, we just leave it as garbage in ssl_ciphersuites.h and just change the internal code to no longer use it, but the newly introduced bitfield of key exchanges.

I'd very much welcome feedback here. cc @mpg

[mpg]

Generally speaking, I'm not in favour of semantic multiplexing, so I think it's cleaner (in terms of maintainability but also documentation) to have separate names for TLS 1.3 key exchanges and TLS 1.2 key exchanges. What would be the upside of shoehorning the 1.3 identifiers into the 1.2 thing?

[hanno-becker]

Long-term I would like to move towards a single 1.2 and 1.3 API which configures key exchange and symmetric suite separately. This is trivial for 1.3, but for 1.2 it would mean auto-inferring which "TLS 1.2 ciphersuites" match the choice of "key exchange + symmetric suite".

When we want to consider such an API, either in 4.0 or in 3.X while deprecating mbedtls_ssl_conf_ciphersuites(), we should have a single namespace we can refer users to for the choice of key exchanges and ciphersuites they can pick from. With macro-defined TLS 1.3 key exchanges the enum-defines TLS 1.2 key exchanges, this is not the case.

That's why I would prefer to have either a single enum with both 1.2 and 1.3 key exchanges, or a similarly-named block of macros for both.

[hanno-becker]

@mpg There're actually two different things under discussion here, which I forgot in my last reply:

1. Can/Should we have a single enum / macro-list for TLS 1.2 and TLS 1.3 key exchanges.
2. Can/Should we mergi e the TLS 1.3 identifiers into the TLS 1.2 identifiers.

In my last reply, I argued why I think 1. is useful, but originally I also asked about 2. I now agree that 2. is error prone. Despite having its own identifiers, the TLS 1.3 prototype also uses the 1.2 identifiers internally, I guess to mirror the logic from the 1.2 code -- but I come to think that this should be changed, and opened #4831.

What do you think about 1.?

[hanno-becker]

One remark on enum vs. macros: I prefer to use macros that can be used as bitfields because it allows to simplify checks like https://github.com/ARMmbed/mbedtls/blob/development/include/mbedtls/ssl_ciphersuites.h#L442:

static inline int mbedtls_ssl_ciphersuite_cert_req_allowed( const mbedtls_ssl_ciphersuite_t *info )
{
    switch( info->MBEDTLS_PRIVATE(key_exchange) )
    {
        case MBEDTLS_KEY_EXCHANGE_RSA:
        case MBEDTLS_KEY_EXCHANGE_DHE_RSA:
        case MBEDTLS_KEY_EXCHANGE_ECDH_RSA:
        case MBEDTLS_KEY_EXCHANGE_ECDHE_RSA:
        case MBEDTLS_KEY_EXCHANGE_ECDH_ECDSA:
        case MBEDTLS_KEY_EXCHANGE_ECDHE_ECDSA:
            return( 1 );

        default:
            return( 0 );
    }
}

of which we have quite a few: With a bitfield, the check takes the form bitfield & MASK != 0, with MASK being known at compile-time.

[hanno-becker]

I would like to hear your opinions on all of this, but given that the structures in question are private, we might want to defer any changes to the way TLS 1.2 handles key exchanges to a separate PR.

[mpg]

Excellent point, I didn't realise there was two separate questions here. Good that we have agreement on 2. Now that 1 is isolated, indeed it makes much more sense, especially considering your previous message. I think I agree that it would be good to be able to mix 1.2 and 1.3 identifiers in a single API, which in practice means probably means either them being values of a single enum, or macros "from a single block", ie with compatible values.

Considering we already have an enum that's technically public, I'm slightly inclined to add values to that enum. Unfortunately, that would mean mbedtls_ssl_conf_tls13_key_exchange_modes() would probably have to take a 0-terminated list instead of a bitfield, which is less efficient but OTOH that would be more consistent with existing APIs. (Then in 4.0 or later we can revisit that if we think a bitfield would really be better.)

I don't feel entirely comfortable with options (a) and (b) mentioned above: for (a), we made a lot of changes in 3.0 to make it clearer what wasn't part of the public API, and now we would again decide that's something that's technically public and not documented as private actually was private anyway? For (b), I'm afraid having duplicated identifiers may be error-prone. However, I could also live with any of those options if you and Gilles feel confident that the possible issues I'm seeing are actually not a problem.

[mpg]

Note: I was replying to this message and hadn't seen the last two when I wrote mine.

I agree that the discussion about 1.2 identifiers can be postponed, so I'll approve this PR as it is.

[hanno-becker]

Thank you for your feedback, @mpg!

I agree that duplication in (b) is not ideal, but I think it's a minor point, because we can add documentation clearly stating that the old enum should not be used anymore (incl. by us), and perhaps even add a deprecation attribute to help the compiler flag any use.

[gilles-peskine-arm]

I prefer enums when the full set of possible values is known in advance and we don't need any control on the width of the type.

Note that enums are compatible with bitmasks (I think you meant bitmasks, not bitfields):

enum {
    ZERO = 1 << 0,
    ONE = 1 << 1,
    TWO = 1 << 2,
    THREE = 1 << 3,
} foo_t;
foo_t value = TWO;
unsigned mask = ONE | TWO;

However I would only do that when masks need to be stored in data structures. When the set of values is known at compile time, I prefer a switch-case: it's more readable than having to go and look up a compound constant and figuring out what the bitwise operations are doing.

[mpg]

Looks pretty good to me, only found a couple of minor issues, but I'm happy with the general design.

[hanno-becker]

Thank you @mpg for your review! I believe to have addressed your comments. Could you please re-review?

[mpg]

Thanks for addressing my feedback! Looks all good to me now, we just need to resolve the conversation about key exchange identifiers.

[mpg]

LGTM

[hanno-becker]

Rebased to resolve trivial merge conflicts. @mpg Can you please re-review?

[mpg]

The rebase looks correct to me.

[mpg]

@bensze01 are you planning to review this soon? Otherwise, please remove yourself from the list and label "needs: reviewer". Thanks.

[ronald-cron-arm]

This looks almost good to me. I have only a few minor comments.

[ronald-cron-arm]

Thanks for the changes. LGTM.

[ronald-cron-arm]

I missed that in my previous review but you have unnecessary information in commit messages for Mbed TLS project. Please remove the "Change-Id" and
"CustomizedGitHooks: yes" lines.

[ronald-cron-arm]

Thanks for fixing the commit messages.

[mpg]

LGTM.