Hanno Becker | 0c3bebf | 2020-06-02 06:32:43 +0100 | [diff] [blame] | 1 | TLS 1.3 Experimental Developments |
Hanno Becker | 9338f9f | 2020-05-31 07:39:50 +0100 | [diff] [blame] | 2 | ================================= |
| 3 | |
| 4 | Overview |
| 5 | -------- |
| 6 | |
| 7 | Mbed TLS doesn't support the TLS 1.3 protocol yet, but a prototype is in development. |
| 8 | Stable parts of this prototype that can be independently tested are being successively |
| 9 | upstreamed under the guard of the following macro: |
| 10 | |
| 11 | ``` |
| 12 | MBEDTLS_SSL_PROTO_TLS1_3_EXPERIMENTAL |
| 13 | ``` |
| 14 | |
| 15 | This macro will likely be renamed to `MBEDTLS_SSL_PROTO_TLS1_3` once a minimal viable |
| 16 | implementation of the TLS 1.3 protocol is available. |
| 17 | |
Bence Szépkúti | bb0cfeb | 2021-05-28 09:42:25 +0200 | [diff] [blame] | 18 | See the [documentation of `MBEDTLS_SSL_PROTO_TLS1_3_EXPERIMENTAL`](../../include/mbedtls/mbedtls_config.h) |
Hanno Becker | 9338f9f | 2020-05-31 07:39:50 +0100 | [diff] [blame] | 19 | for more information. |
| 20 | |
| 21 | Status |
| 22 | ------ |
| 23 | |
| 24 | The following lists which parts of the TLS 1.3 prototype have already been upstreamed |
| 25 | together with their level of testing: |
| 26 | |
| 27 | * TLS 1.3 record protection mechanisms |
| 28 | |
| 29 | The record protection routines `mbedtls_ssl_{encrypt|decrypt}_buf()` have been extended |
| 30 | to support the modified TLS 1.3 record protection mechanism, including modified computation |
| 31 | of AAD, IV, and the introduction of a flexible padding. |
| 32 | |
| 33 | Those record protection routines have unit tests in `test_suite_ssl` alongside the |
| 34 | tests for the other record protection routines. |
| 35 | |
| 36 | TODO: Add some test vectors from RFC 8448. |
Hanno Becker | 5a83d29 | 2020-06-02 06:33:00 +0100 | [diff] [blame] | 37 | |
| 38 | - The HKDF key derivation function on which the TLS 1.3 key schedule is based, |
| 39 | is already present as an independent module controlled by `MBEDTLS_HKDF_C` |
| 40 | independently of the development of the TLS 1.3 prototype. |
Hanno Becker | b11c309 | 2020-08-10 17:00:19 +0100 | [diff] [blame] | 41 | |
| 42 | - The TLS 1.3-specific HKDF-based key derivation functions (see RFC 8446): |
| 43 | * HKDF-Expand-Label |
| 44 | * Derive-Secret |
| 45 | - Secret evolution |
| 46 | * The traffic {Key,IV} generation from secret |
| 47 | Those functions are implemented in `library/ssl_tls13_keys.c` and |
| 48 | tested in `test_suite_ssl` using test vectors from RFC 8448 and |
| 49 | https://tls13.ulfheim.net/. |
Hanno Becker | 7594c68 | 2021-03-05 05:17:11 +0000 | [diff] [blame] | 50 | |
| 51 | - New TLS Message Processing Stack (MPS) |
| 52 | |
| 53 | The TLS 1.3 prototype is developed alongside a rewrite of the TLS messaging layer, |
| 54 | encompassing low-level details such as record parsing, handshake reassembly, and |
| 55 | DTLS retransmission state machine. |
| 56 | |
| 57 | MPS has the following components: |
| 58 | - Layer 1 (Datagram handling) |
| 59 | - Layer 2 (Record handling) |
| 60 | - Layer 3 (Message handling) |
| 61 | - Layer 4 (Retransmission State Machine) |
| 62 | - Reader (Abstracted pointer arithmetic and reassembly logic for incoming data) |
| 63 | - Writer (Abstracted pointer arithmetic and fragmentation logic for outgoing data) |
| 64 | |
| 65 | Of those components, the following have been upstreamed |
| 66 | as part of `MBEDTLS_SSL_PROTO_TLS1_3_EXPERIMENTAL`: |
| 67 | |
| 68 | - Reader ([`library/mps_reader.h`](../../library/mps_reader.h)) |
Ronald Cron | 3785c90 | 2021-09-20 09:05:36 +0200 | [diff] [blame] | 69 | |
| 70 | |
| 71 | MVP definition |
| 72 | -------------- |
| 73 | |
Ronald Cron | f164b6a | 2021-09-27 15:36:29 +0200 | [diff] [blame] | 74 | - Overview |
| 75 | |
| 76 | - The TLS 1.3 MVP implements only the client side of the protocol. |
| 77 | |
| 78 | - The TLS 1.3 MVP supports ECDHE key establishment. |
| 79 | |
| 80 | - The TLS 1.3 MVP does not support DHE key establishment. |
| 81 | |
| 82 | - The TLS 1.3 MVP does not support pre-shared keys, including any form of |
| 83 | session resumption. This implies that it does not support sending early |
| 84 | data (0-RTT data). |
| 85 | |
| 86 | - The TLS 1.3 MVP supports the authentication of the server by the client |
| 87 | but does not support authentication of the client by the server. In terms |
| 88 | of TLS 1.3 authentication messages, this means that the TLS 1.3 MVP |
| 89 | supports the processing of the Certificate and CertificateVerify messages |
| 90 | but not of the CertificateRequest message. |
| 91 | |
| 92 | - The TLS 1.3 MVP does not support the handling of server HelloRetryRequest |
| 93 | message. In practice, this means that the handshake will fail if the MVP |
| 94 | does not provide in its ClientHello the shared secret associated to the |
| 95 | group selected by the server for key establishement. For more information, |
| 96 | see the comment associated to the `key_share` extension below. |
| 97 | |
| 98 | - If the TLS 1.3 MVP receives a HelloRetryRequest or a CertificateRequest |
| 99 | message, it aborts the handshake with an handshake_failure closure alert |
| 100 | and the `mbedtls_ssl_handshake()` returns in error with the |
| 101 | `MBEDTLS_ERR_SSL_HANDSHAKE_FAILURE` error code. |
Ronald Cron | 3785c90 | 2021-09-20 09:05:36 +0200 | [diff] [blame] | 102 | |
| 103 | - Supported cipher suites: depends on the library configuration. Potentially |
| 104 | all of them: |
| 105 | TLS_AES_128_GCM_SHA256, TLS_AES_256_GCM_SHA384, TLS_CHACHA20_POLY1305_SHA256, |
| 106 | TLS_AES_128_CCM_SHA256 and TLS_AES_128_CCM_8_SHA256. |
| 107 | |
| 108 | - Supported ClientHello extensions: |
| 109 | |
Ronald Cron | 023987f | 2021-09-27 11:59:25 +0200 | [diff] [blame] | 110 | | Extension | MVP | Prototype (1) | |
| 111 | | ---------------------------- | ------- | ------------- | |
Ronald Cron | 85e5108 | 2021-09-27 12:13:16 +0200 | [diff] [blame] | 112 | | server_name | YES | YES | |
Ronald Cron | 023987f | 2021-09-27 11:59:25 +0200 | [diff] [blame] | 113 | | max_fragment_length | no | YES | |
| 114 | | status_request | no | no | |
| 115 | | supported_groups | YES | YES | |
| 116 | | signature_algorithms | YES | YES | |
| 117 | | use_srtp | no | no | |
| 118 | | heartbeat | no | no | |
| 119 | | apln | no | YES | |
| 120 | | signed_certificate_timestamp | no | no | |
| 121 | | client_certificate_type | no | no | |
| 122 | | server_certificate_type | no | no | |
| 123 | | padding | no | no | |
Ronald Cron | 3160d70 | 2021-09-27 13:27:21 +0200 | [diff] [blame] | 124 | | key_share | YES (2) | YES | |
Ronald Cron | 023987f | 2021-09-27 11:59:25 +0200 | [diff] [blame] | 125 | | pre_shared_key | no | YES | |
| 126 | | psk_key_exchange_modes | no | YES | |
| 127 | | early_data | no | YES | |
| 128 | | cookie | no | YES | |
Ronald Cron | 3160d70 | 2021-09-27 13:27:21 +0200 | [diff] [blame] | 129 | | supported_versions | YES (3) | YES | |
Ronald Cron | 023987f | 2021-09-27 11:59:25 +0200 | [diff] [blame] | 130 | | certificate_authorities | no | no | |
| 131 | | post_handshake_auth | no | no | |
| 132 | | signature_algorithms_cert | no | no | |
Ronald Cron | 3785c90 | 2021-09-20 09:05:36 +0200 | [diff] [blame] | 133 | |
Ronald Cron | 023987f | 2021-09-27 11:59:25 +0200 | [diff] [blame] | 134 | (1) This is just for comparison. |
| 135 | |
Paul Elliott | 66491c7 | 2021-11-26 20:24:43 +0000 | [diff] [blame^] | 136 | (2) The MVP sends only one shared secret corresponding to the configured |
| 137 | preferred group. This could, however end up with connection failure if the |
| 138 | server does not support our preferred curve, as we have yet to implement |
| 139 | HelloRetryRequest. The preferred group is the group of the first curve in |
| 140 | the list of allowed curves as defined by the configuration. The list of |
| 141 | mandatory-to-implement groups (in absence of an application profile |
| 142 | standard specifying otherwise) as defined in section 9.1 of the |
| 143 | specification gives the preferred order as follows: `secp256r1`, `x25519`, |
| 144 | `secp384r1` and finally `secp521r1`. If we could therefore fix the use of |
| 145 | `secp256r1`, then we would be guaranteed that the server supported it, |
| 146 | however our current curve preference order puts `x25519` before |
| 147 | `secp256r1` and changing this for only TLS1.3 would be potentially |
| 148 | difficult (we have no desire to change TLS1.2 behaviour). The likelyhood |
| 149 | of finding a server that doesn't support `x25519` is quite low and indeed |
| 150 | the end user could themselves change the order of preference of curves |
| 151 | using the `mbedtls_ssl_conf_curves()` API if they wished to do so, so we |
| 152 | are leaving the current preference order intact. |
Ronald Cron | 3160d70 | 2021-09-27 13:27:21 +0200 | [diff] [blame] | 153 | |
Paul Elliott | 66491c7 | 2021-11-26 20:24:43 +0000 | [diff] [blame^] | 154 | (3) The MVP proposes only TLS 1.3 and does not support version negotiation. |
Ronald Cron | 3160d70 | 2021-09-27 13:27:21 +0200 | [diff] [blame] | 155 | Out-of-protocol fallback is supported though if the Mbed TLS library |
| 156 | has been built to support both TLS 1.3 and TLS 1.2: just set the |
| 157 | maximum of the minor version of the SSL configuration to |
| 158 | MBEDTLS_SSL_MINOR_VERSION_3 (`mbedtls_ssl_conf_min_version()` API) and |
| 159 | re-initiate a server handshake. |
Ronald Cron | 3785c90 | 2021-09-20 09:05:36 +0200 | [diff] [blame] | 160 | |
| 161 | - Supported groups: depends on the library configuration. |
Ronald Cron | 8ee9ed6 | 2021-09-28 14:46:43 +0200 | [diff] [blame] | 162 | Potentially all ECDHE groups but x448: |
| 163 | secp256r1, x25519, secp384r1 and secp521r1. |
Ronald Cron | c3b510f | 2021-09-27 13:36:33 +0200 | [diff] [blame] | 164 | |
| 165 | Finite field groups (DHE) are not supported. |
| 166 | |
Ronald Cron | fb87721 | 2021-09-28 15:49:39 +0200 | [diff] [blame] | 167 | - Supported signature algorithms (both for certificates and CertificateVerify): |
| 168 | depends on the library configuration. |
| 169 | Potentially: |
| 170 | rsa_pkcs1_sha256, rsa_pss_rsae_sha256, ecdsa_secp256r1_sha256, |
| 171 | ecdsa_secp384r1_sha384 and ecdsa_secp521r1_sha512. |
Ronald Cron | c3b510f | 2021-09-27 13:36:33 +0200 | [diff] [blame] | 172 | |
Ronald Cron | fb87721 | 2021-09-28 15:49:39 +0200 | [diff] [blame] | 173 | Note that in absence of an application profile standard specifying otherwise |
| 174 | the three first ones in the list above are mandatory (see section 9.1 of the |
| 175 | specification). |
Ronald Cron | c3b510f | 2021-09-27 13:36:33 +0200 | [diff] [blame] | 176 | |
| 177 | - Supported versions: only TLS 1.3, version negotiation is not supported. |
Ronald Cron | 3785c90 | 2021-09-20 09:05:36 +0200 | [diff] [blame] | 178 | |
Ronald Cron | 3e7c403 | 2021-09-27 14:22:38 +0200 | [diff] [blame] | 179 | - Compatibility with existing SSL/TLS build options: |
Ronald Cron | 3785c90 | 2021-09-20 09:05:36 +0200 | [diff] [blame] | 180 | |
| 181 | The TLS 1.3 MVP is compatible with all TLS 1.2 configuration options in the |
| 182 | sense that when enabling the TLS 1.3 MVP in the library there is no need to |
| 183 | modify the configuration for TLS 1.2. Mbed TLS SSL/TLS related features are |
| 184 | not supported or not applicable to the TLS 1.3 MVP: |
| 185 | |
Ronald Cron | 023987f | 2021-09-27 11:59:25 +0200 | [diff] [blame] | 186 | | Mbed TLS configuration option | Support | |
| 187 | | ---------------------------------------- | ------- | |
| 188 | | MBEDTLS_SSL_ALL_ALERT_MESSAGES | no | |
| 189 | | MBEDTLS_SSL_ASYNC_PRIVATE | no | |
| 190 | | MBEDTLS_SSL_CONTEXT_SERIALIZATION | no | |
| 191 | | MBEDTLS_SSL_DEBUG_ALL | no | |
| 192 | | MBEDTLS_SSL_ENCRYPT_THEN_MAC | n/a | |
| 193 | | MBEDTLS_SSL_EXTENDED_MASTER_SECRET | n/a | |
| 194 | | MBEDTLS_SSL_KEEP_PEER_CERTIFICATE | no | |
| 195 | | MBEDTLS_SSL_RENEGOTIATION | n/a | |
| 196 | | MBEDTLS_SSL_MAX_FRAGMENT_LENGTH | no | |
| 197 | | | | |
| 198 | | MBEDTLS_SSL_SESSION_TICKETS | no | |
| 199 | | MBEDTLS_SSL_EXPORT_KEYS | no (1) | |
| 200 | | MBEDTLS_SSL_SERVER_NAME_INDICATION | no | |
| 201 | | MBEDTLS_SSL_VARIABLE_BUFFER_LENGTH | no | |
| 202 | | | | |
| 203 | | MBEDTLS_ECP_RESTARTABLE | no | |
| 204 | | MBEDTLS_ECDH_VARIANT_EVEREST_ENABLED | no | |
| 205 | | | | |
| 206 | | MBEDTLS_KEY_EXCHANGE_PSK_ENABLED | n/a (2) | |
| 207 | | MBEDTLS_KEY_EXCHANGE_DHE_PSK_ENABLED | n/a | |
| 208 | | MBEDTLS_KEY_EXCHANGE_ECDHE_PSK_ENABLED | n/a | |
| 209 | | MBEDTLS_KEY_EXCHANGE_RSA_PSK_ENABLED | n/a | |
| 210 | | MBEDTLS_KEY_EXCHANGE_RSA_ENABLED | n/a | |
| 211 | | MBEDTLS_KEY_EXCHANGE_DHE_RSA_ENABLED | n/a | |
| 212 | | MBEDTLS_KEY_EXCHANGE_ECDHE_RSA_ENABLED | n/a | |
| 213 | | MBEDTLS_KEY_EXCHANGE_ECDHE_ECDSA_ENABLED | n/a | |
| 214 | | MBEDTLS_KEY_EXCHANGE_ECDH_ECDSA_ENABLED | n/a | |
| 215 | | MBEDTLS_KEY_EXCHANGE_ECDH_RSA_ENABLED | n/a | |
| 216 | | MBEDTLS_KEY_EXCHANGE_ECJPAKE_ENABLED | n/a | |
| 217 | | | | |
| 218 | | MBEDTLS_USE_PSA_CRYPTO | no | |
Ronald Cron | 3785c90 | 2021-09-20 09:05:36 +0200 | [diff] [blame] | 219 | |
Ronald Cron | 023987f | 2021-09-27 11:59:25 +0200 | [diff] [blame] | 220 | (1) Some support has already been upstreamed but it is incomplete. |
Ronald Cron | 1fa5088 | 2021-09-27 12:06:52 +0200 | [diff] [blame] | 221 | (2) Key exchange configuration options for TLS 1.3 will likely to be |
| 222 | organized around the notion of key exchange mode along the line |
| 223 | of the MBEDTLS_SSL_TLS13_KEY_EXCHANGE_MODE_NONE/PSK/PSK_EPHEMERAL/EPHEMERAL |
| 224 | runtime configuration macros. |
Ronald Cron | 3785c90 | 2021-09-20 09:05:36 +0200 | [diff] [blame] | 225 | |
Ronald Cron | 660c723 | 2021-09-27 13:40:53 +0200 | [diff] [blame] | 226 | - Quality considerations |
| 227 | - Standard Mbed TLS review bar |
| 228 | - Interoperability testing with OpenSSL and GnuTLS. Test with all the |
Ronald Cron | 7fc96c1 | 2021-09-28 15:54:57 +0200 | [diff] [blame] | 229 | cipher suites and signature algorithms supported by OpenSSL/GnuTLS server. |
Ronald Cron | 660c723 | 2021-09-27 13:40:53 +0200 | [diff] [blame] | 230 | - Negative testing against OpenSSL/GnuTLS servers with which the |
Ronald Cron | 7fc96c1 | 2021-09-28 15:54:57 +0200 | [diff] [blame] | 231 | handshake fails due to incompatibility with the capabilities of the |
Ronald Cron | 660c723 | 2021-09-27 13:40:53 +0200 | [diff] [blame] | 232 | MVP: TLS 1.2 or 1.1 server, server sending an HelloRetryRequest message in |
| 233 | response to the MVP ClientHello, server sending a CertificateRequest |
| 234 | message ... |
| 235 | |
Ronald Cron | 3785c90 | 2021-09-20 09:05:36 +0200 | [diff] [blame] | 236 | Coding rules checklist for TLS 1.3 |
| 237 | ---------------------------------- |
| 238 | |
| 239 | The following coding rules are aimed to be a checklist for TLS 1.3 upstreaming |
| 240 | work to reduce review rounds and the number of comments in each round. They |
| 241 | come along (do NOT replace) the project coding rules |
| 242 | (https://tls.mbed.org/kb/development/mbedtls-coding-standards). They have been |
| 243 | established and discussed following the review of #4882 that was the |
| 244 | PR upstreaming the first part of TLS 1.3 ClientHello writing code. |
| 245 | |
| 246 | TLS 1.3 specific coding rules: |
| 247 | |
| 248 | - TLS 1.3 specific C modules, headers, static functions names are prefixed |
Ronald Cron | b194466 | 2021-09-27 13:56:46 +0200 | [diff] [blame] | 249 | with `ssl_tls13_`. The same applies to structures and types that are |
Ronald Cron | 3785c90 | 2021-09-20 09:05:36 +0200 | [diff] [blame] | 250 | internal to C modules. |
| 251 | |
Ronald Cron | b194466 | 2021-09-27 13:56:46 +0200 | [diff] [blame] | 252 | - TLS 1.3 specific exported functions, structures and types are |
| 253 | prefixed with `mbedtls_ssl_tls13_`. |
| 254 | |
| 255 | - Use TLS1_3 in TLS 1.3 specific macros. |
Ronald Cron | 3785c90 | 2021-09-20 09:05:36 +0200 | [diff] [blame] | 256 | |
| 257 | - The names of macros and variables related to a field or structure in the |
| 258 | TLS 1.3 specification should contain as far as possible the field name as |
Ronald Cron | 72064b3 | 2021-09-27 13:54:28 +0200 | [diff] [blame] | 259 | it is in the specification. If the field name is "too long" and we prefer |
Ronald Cron | 3785c90 | 2021-09-20 09:05:36 +0200 | [diff] [blame] | 260 | to introduce some kind of abbreviation of it, use the same abbreviation |
| 261 | everywhere in the code. |
| 262 | |
| 263 | Example 1: #define CLIENT_HELLO_RANDOM_LEN 32, macro for the length of the |
| 264 | `random` field of the ClientHello message. |
| 265 | |
Dave Rodgman | c8aaac8 | 2021-10-18 12:56:53 +0100 | [diff] [blame] | 266 | Example 2 (consistent abbreviation): `mbedtls_ssl_tls13_write_sig_alg_ext()` |
Ronald Cron | 72064b3 | 2021-09-27 13:54:28 +0200 | [diff] [blame] | 267 | and `MBEDTLS_TLS_EXT_SIG_ALG`, `sig_alg` standing for |
Ronald Cron | 3785c90 | 2021-09-20 09:05:36 +0200 | [diff] [blame] | 268 | `signature_algorithms`. |
| 269 | |
| 270 | - Regarding vectors that are represented by a length followed by their value |
| 271 | in the data exchanged between servers and clients: |
| 272 | |
| 273 | - Use `<vector name>_len` for the name of a variable used to compute the |
| 274 | length in bytes of the vector, where <vector name> is the name of the |
| 275 | vector as defined in the TLS 1.3 specification. |
| 276 | |
Ronald Cron | 99733f0 | 2021-09-27 13:58:21 +0200 | [diff] [blame] | 277 | - Use `p_<vector_name>_len` for the name of a variable intended to hold |
Ronald Cron | 3785c90 | 2021-09-20 09:05:36 +0200 | [diff] [blame] | 278 | the address of the first byte of the vector length. |
| 279 | |
Ronald Cron | 99733f0 | 2021-09-27 13:58:21 +0200 | [diff] [blame] | 280 | - Use `<vector_name>` for the name of a variable intended to hold the |
Ronald Cron | 3785c90 | 2021-09-20 09:05:36 +0200 | [diff] [blame] | 281 | address of the first byte of the vector value. |
| 282 | |
Ronald Cron | 99733f0 | 2021-09-27 13:58:21 +0200 | [diff] [blame] | 283 | - Use `<vector_name>_end` for the name of a variable intended to hold |
Ronald Cron | 3785c90 | 2021-09-20 09:05:36 +0200 | [diff] [blame] | 284 | the address of the first byte past the vector value. |
| 285 | |
Ronald Cron | 99733f0 | 2021-09-27 13:58:21 +0200 | [diff] [blame] | 286 | Those idioms should lower the risk of mis-using one of the address in place |
| 287 | of another one which could potentially lead to some nasty issues. |
Ronald Cron | 3785c90 | 2021-09-20 09:05:36 +0200 | [diff] [blame] | 288 | |
| 289 | Example: `cipher_suites` vector of ClientHello in |
Dave Rodgman | c8aaac8 | 2021-10-18 12:56:53 +0100 | [diff] [blame] | 290 | `ssl_tls13_write_client_hello_cipher_suites()` |
Ronald Cron | 72064b3 | 2021-09-27 13:54:28 +0200 | [diff] [blame] | 291 | ``` |
| 292 | size_t cipher_suites_len; |
Ronald Cron | 99733f0 | 2021-09-27 13:58:21 +0200 | [diff] [blame] | 293 | unsigned char *p_cipher_suites_len; |
| 294 | unsigned char *cipher_suites; |
Ronald Cron | 72064b3 | 2021-09-27 13:54:28 +0200 | [diff] [blame] | 295 | ``` |
Ronald Cron | 3785c90 | 2021-09-20 09:05:36 +0200 | [diff] [blame] | 296 | |
Ronald Cron | fecda8d | 2021-09-27 13:59:38 +0200 | [diff] [blame] | 297 | - Where applicable, use: |
| 298 | - the macros to extract a byte from a multi-byte integer MBEDTLS_BYTE_{0-8}. |
| 299 | - the macros to write in memory in big-endian order a multi-byte integer |
| 300 | MBEDTLS_PUT_UINT{8|16|32|64}_BE. |
| 301 | - the macros to read from memory a multi-byte integer in big-endian order |
| 302 | MBEDTLS_GET_UINT{8|16|32|64}_BE. |
| 303 | - the macro to check for space when writing into an output buffer |
| 304 | `MBEDTLS_SSL_CHK_BUF_PTR`. |
| 305 | - the macro to check for data when reading from an input buffer |
| 306 | `MBEDTLS_SSL_CHK_BUF_READ_PTR`. |
Ronald Cron | 3785c90 | 2021-09-20 09:05:36 +0200 | [diff] [blame] | 307 | |
| 308 | These macros were introduced after the prototype was written thus are |
| 309 | likely not to be used in prototype where we now would use them in |
| 310 | development. |
| 311 | |
Ronald Cron | fecda8d | 2021-09-27 13:59:38 +0200 | [diff] [blame] | 312 | The three first types, MBEDTLS_BYTE_{0-8}, MBEDTLS_PUT_UINT{8|16|32|64}_BE |
| 313 | and MBEDTLS_GET_UINT{8|16|32|64}_BE improve the readability of the code and |
| 314 | reduce the risk of writing or reading bytes in the wrong order. |
Ronald Cron | 3785c90 | 2021-09-20 09:05:36 +0200 | [diff] [blame] | 315 | |
Ronald Cron | 72064b3 | 2021-09-27 13:54:28 +0200 | [diff] [blame] | 316 | The two last types, `MBEDTLS_SSL_CHK_BUF_PTR` and |
| 317 | `MBEDTLS_SSL_CHK_BUF_READ_PTR`, improve the readability of the code and |
Ronald Cron | 3785c90 | 2021-09-20 09:05:36 +0200 | [diff] [blame] | 318 | reduce the risk of error in the non-completely-trivial arithmetic to |
| 319 | check that we do not write or read past the end of a data buffer. The |
| 320 | usage of those macros combined with the following rule mitigate the risk |
| 321 | to read/write past the end of a data buffer. |
| 322 | |
Ronald Cron | 72064b3 | 2021-09-27 13:54:28 +0200 | [diff] [blame] | 323 | Examples: |
| 324 | ``` |
| 325 | hs_hdr[1] = MBEDTLS_BYTE_2( total_hs_len ); |
| 326 | MBEDTLS_PUT_UINT16_BE( MBEDTLS_TLS_EXT_SUPPORTED_VERSIONS, p, 0 ); |
| 327 | MBEDTLS_SSL_CHK_BUF_PTR( p, end, 7 ); |
| 328 | ``` |
Ronald Cron | 3785c90 | 2021-09-20 09:05:36 +0200 | [diff] [blame] | 329 | |
| 330 | - To mitigate what happened here |
| 331 | (https://github.com/ARMmbed/mbedtls/pull/4882#discussion_r701704527) from |
| 332 | happening again, use always a local variable named `p` for the reading |
| 333 | pointer in functions parsing TLS 1.3 data, and for the writing pointer in |
Ronald Cron | 3e7c403 | 2021-09-27 14:22:38 +0200 | [diff] [blame] | 334 | functions writing data into an output buffer and only that variable. The |
| 335 | name `p` has been chosen as it was already widely used in TLS code. |
Ronald Cron | 3785c90 | 2021-09-20 09:05:36 +0200 | [diff] [blame] | 336 | |
| 337 | - When an TLS 1.3 structure is written or read by a function or as part of |
| 338 | a function, provide as documentation the definition of the structure as |
| 339 | it is in the TLS 1.3 specification. |
| 340 | |
| 341 | General coding rules: |
| 342 | |
Ronald Cron | 72064b3 | 2021-09-27 13:54:28 +0200 | [diff] [blame] | 343 | - We prefer grouping "related statement lines" by not adding blank lines |
Ronald Cron | 3785c90 | 2021-09-20 09:05:36 +0200 | [diff] [blame] | 344 | between them. |
| 345 | |
| 346 | Example 1: |
Ronald Cron | 72064b3 | 2021-09-27 13:54:28 +0200 | [diff] [blame] | 347 | ``` |
Ronald Cron | 3785c90 | 2021-09-20 09:05:36 +0200 | [diff] [blame] | 348 | ret = ssl_tls13_write_client_hello_cipher_suites( ssl, buf, end, &output_len ); |
| 349 | if( ret != 0 ) |
| 350 | return( ret ); |
| 351 | buf += output_len; |
Ronald Cron | 72064b3 | 2021-09-27 13:54:28 +0200 | [diff] [blame] | 352 | ``` |
Ronald Cron | 3785c90 | 2021-09-20 09:05:36 +0200 | [diff] [blame] | 353 | |
| 354 | Example 2: |
Ronald Cron | 72064b3 | 2021-09-27 13:54:28 +0200 | [diff] [blame] | 355 | ``` |
Ronald Cron | 3785c90 | 2021-09-20 09:05:36 +0200 | [diff] [blame] | 356 | MBEDTLS_SSL_CHK_BUF_PTR( cipher_suites_iter, end, 2 ); |
| 357 | MBEDTLS_PUT_UINT16_BE( cipher_suite, cipher_suites_iter, 0 ); |
| 358 | cipher_suites_iter += 2; |
Ronald Cron | 72064b3 | 2021-09-27 13:54:28 +0200 | [diff] [blame] | 359 | ``` |
Ronald Cron | 3785c90 | 2021-09-20 09:05:36 +0200 | [diff] [blame] | 360 | |
| 361 | - Use macros for constants that are used in different functions, different |
| 362 | places in the code. When a constant is used only locally in a function |
| 363 | (like the length in bytes of the vector lengths in functions reading and |
| 364 | writing TLS handshake message) there is no need to define a macro for it. |
| 365 | |
Ronald Cron | 72064b3 | 2021-09-27 13:54:28 +0200 | [diff] [blame] | 366 | Example: `#define CLIENT_HELLO_RANDOM_LEN 32` |
Ronald Cron | 3785c90 | 2021-09-20 09:05:36 +0200 | [diff] [blame] | 367 | |
| 368 | - When declaring a pointer the dereferencing operator should be prepended to |
| 369 | the pointer name not appended to the pointer type: |
| 370 | |
Ronald Cron | 72064b3 | 2021-09-27 13:54:28 +0200 | [diff] [blame] | 371 | Example: `mbedtls_ssl_context *ssl;` |
Ronald Cron | 3785c90 | 2021-09-20 09:05:36 +0200 | [diff] [blame] | 372 | |
| 373 | - Maximum line length is 80 characters. |
| 374 | |
| 375 | Exceptions: |
| 376 | |
| 377 | - string literals can extend beyond 80 characters as we do not want to |
| 378 | split them to ease their search in the code base. |
| 379 | |
| 380 | - A line can be more than 80 characters by a few characters if just looking |
| 381 | at the 80 first characters is enough to fully understand the line. For |
| 382 | example it is generally fine if some closure characters like ";" or ")" |
| 383 | are beyond the 80 characters limit. |
| 384 | |
Ronald Cron | 847c358 | 2021-09-27 14:24:43 +0200 | [diff] [blame] | 385 | If a line becomes too long due to a refactoring (for example renaming a |
| 386 | function to a longer name, or indenting a block more), avoid rewrapping |
| 387 | lines in the same commit: it makes the review harder. Make one commit with |
| 388 | the longer lines and another commit with just the rewrapping. |
| 389 | |
Ronald Cron | 3785c90 | 2021-09-20 09:05:36 +0200 | [diff] [blame] | 390 | - When in successive lines, functions and macros parameters should be aligned |
| 391 | vertically. |
| 392 | |
| 393 | Example: |
Ronald Cron | 72064b3 | 2021-09-27 13:54:28 +0200 | [diff] [blame] | 394 | ``` |
Ronald Cron | 3785c90 | 2021-09-20 09:05:36 +0200 | [diff] [blame] | 395 | int mbedtls_ssl_tls13_start_handshake_msg( mbedtls_ssl_context *ssl, |
| 396 | unsigned hs_type, |
| 397 | unsigned char **buf, |
| 398 | size_t *buf_len ); |
Ronald Cron | 72064b3 | 2021-09-27 13:54:28 +0200 | [diff] [blame] | 399 | ``` |
Ronald Cron | 847c358 | 2021-09-27 14:24:43 +0200 | [diff] [blame] | 400 | |
| 401 | - When a function's parameters span several lines, group related parameters |
| 402 | together if possible. |
| 403 | |
| 404 | For example, prefer: |
| 405 | |
| 406 | ``` |
| 407 | mbedtls_ssl_tls13_start_handshake_msg( ssl, hs_type, |
| 408 | buf, buf_len ); |
| 409 | ``` |
| 410 | over |
| 411 | ``` |
| 412 | mbedtls_ssl_tls13_start_handshake_msg( ssl, hs_type, buf, |
| 413 | buf_len ); |
| 414 | ``` |
| 415 | even if it fits. |