[WIP]fix compile error for re-introducing exists options. #293

yuhaoth · 2021-07-16T10:09:39Z

Description

To fix #15 , This patch is to fix sub-task 1.

There are many compile error fixed in this pr.

Now, the compile passed but test fail.
I am fix test fail and try to breakdown patch for fixing compile error.

Status

IN DEVELOPMENT

Requires Backporting

NO

Migrations

NO

hanno-becker · 2021-07-20T04:54:24Z

tests/suites/test_suite_mps.function

@@ -527,7 +527,7 @@ static void build_mock_transforms( mbedtls_mps_transform_wrap_t *ptr0,
    ptr1->mock.enabled = enabled;
    ptr1->mock.pad     = pad;
 }
-
+#if 0 //dirty fix compile warning


Which compile-time guard do we want here?

hanno-becker · 2021-07-20T04:54:44Z

include/mbedtls/config.h

@@ -1453,7 +1453,7 @@
 *
 * TODO: Document
 */
-#define MBEDTLS_SSL_USE_MPS
+//#define MBEDTLS_SSL_USE_MPS


Please keep this on by default.

hanno-becker · 2021-07-20T04:56:05Z

include/mbedtls/config.h

@@ -1761,7 +1761,7 @@
 * functionality specific to TLS 1.3.
 */
 #define MBEDTLS_SSL_PROTO_TLS1_3_EXPERIMENTAL
-
+#define MBEDTLS_SSL_PROTO_TLS1_2_OR_EARLIER


This is a derived-macro, it should not be defined by hand.

hanno-becker · 2021-07-20T04:59:18Z

library/ssl_tls.c

@@ -4416,7 +4416,18 @@ static void ssl_mps_free( mbedtls_ssl_context *ssl )
    mps_alloc_free( &ssl->mps.alloc );
 }
 #endif /* MEDTLS_SSL_USE_MPS */
-
+#if defined(MBEDTLS_SSL_PROTO_TLS1_3_EXPERIMENTAL)
+static inline int check_version_config(const mbedtls_ssl_config *conf)


Same comment as in #290 - style adherence and more descriptive name needed.

hanno-becker · 2021-07-20T04:59:31Z

library/ssl_tls.c

@@ -6854,12 +6870,37 @@ int mbedtls_ssl_handshake_step( mbedtls_ssl_context *ssl )

 #if defined(MBEDTLS_SSL_CLI_C)
    if( ssl->conf->endpoint == MBEDTLS_SSL_IS_CLIENT )
+    {
+#if defined(MBEDTLS_SSL_PROTO_TLS1_3_EXPERIMENTAL)


hanno-becker · 2021-07-20T04:59:47Z

programs/ssl/ssl_client2.c

@@ -3020,10 +3020,12 @@ int main( int argc, char *argv[] )
 #endif /* MBEDTLS_KEY_EXCHANGE_SOME_PSK_ENABLED */

    if( opt.min_version != DFL_MIN_VERSION )
+        // TAG for Jerry Yu, This is important for TLS1.3 now


Can this comment be removed?

hanno-becker · 2021-07-20T05:03:52Z

@yuhaoth This is a very important PR for the integration of TLS 1.3.

Can you expand on the description a bit, and elaborate what works, and what doesn't? We don't have to -- and should not -- use big monolithic PRs, so if you see a good intermediate step that's merge-worthy, we should take it. For example, we could merge this PR if (a) TLS 1.3 continues to compile and pass tests, and (b) TLS 1.2 compiles, even if not yet passing tests. And then have another PR to address the tests for TLS 1.2.

hanno-becker · 2021-07-20T05:05:10Z

library/ssl_cli.c

@@ -1717,8 +1717,12 @@ static int ssl_parse_supported_point_formats_ext( mbedtls_ssl_context *ssl,
            p[0] == MBEDTLS_ECP_PF_COMPRESSED )
        {
 #if defined(MBEDTLS_ECDH_C) || defined(MBEDTLS_ECDSA_C)
+#if defined(MBEDTLS_SSL_PROTO_TLS1_3_EXPERIMENTAL)
+            ssl->handshake->ecdh_ctx[0].point_format = p[0];


We should improve readability here and have a getter which returns the ECDH context. Can you modify the commit accordingly?

hanno-becker · 2021-07-20T05:06:38Z

include/mbedtls/ssl_internal.h

 #endif /* MBEDTLS_SSL_PROTO_TLS1_3_EXPERIMENTAL */
+    size_t pmslen;                                          /*!<  premaster length        */


Can you explain why this change was made?

hanno-becker · 2021-07-20T05:06:56Z

include/mbedtls/ssl_internal.h

@@ -684,8 +684,8 @@ struct mbedtls_ssl_handshake_params
 #endif
 #endif
 #endif /* MBEDTLS_SSL_PROTO_TLS1_2 || MBEDTLS_SSL_PROTO_TLS1_3_EXPERIMENTAL */
-
-#if defined(MBEDTLS_SSL_PROTO_TLS1_3_EXPERIMENTAL)
+#if 0


Which guard is needed here?

hanno-becker · 2021-07-20T05:09:16Z

include/mbedtls/ssl_internal.h

@@ -1280,25 +1280,24 @@ int mbedtls_ssl_write_record( mbedtls_ssl_context *ssl, uint8_t force_flush );
 #if defined(MBEDTLS_SSL_PROTO_TLS1_3_EXPERIMENTAL)
 int mbedtls_ssl_read_certificate_process(mbedtls_ssl_context* ssl);
 int mbedtls_ssl_write_certificate_process(mbedtls_ssl_context* ssl);
-#else
+#endif /* MBEDTLS_SSL_PROTO_TLS1_3_EXPERIMENTAL */


The pattern here should be different:

#if defined(MBEDTLS_SSL_PROTO_TLS1_3_EXPERIMENTAL) tls 1.3 specific things #elif defined(MBEDTLS_SSL_PROTO_TLS1_2_OR_EARLIER) tls 1.2 specific things #endif common things

From what I understand mbedtls_ssl_parse/write_certificate() are specific to the TLS 1.2 code?

hanno-becker · 2021-07-20T05:09:32Z

include/mbedtls/ssl_internal.h


 #if defined(MBEDTLS_SSL_PROTO_TLS1_3_EXPERIMENTAL)
 int mbedtls_ssl_finished_in_process( mbedtls_ssl_context* ssl );
 int mbedtls_ssl_finished_out_process( mbedtls_ssl_context* ssl );
-#else
+#endif /* MBEDTLS_SSL_PROTO_TLS1_3_EXPERIMENTAL */


same commen here

hanno-becker · 2021-07-20T05:11:55Z

library/ssl_msg.c

@@ -499,6 +499,82 @@ static void ssl_extract_add_data_from_record( unsigned char* add_data,

    *add_data_len = cur - add_data;
 }
+#endif /* MBEDTLS_SSL_PROTO_TLS1_3_EXPERIMENTAL */
+static void ssl_extract_add_data_from_record( unsigned char* add_data,


Why was this change made? The functions are mostly the same across TLS 1.2 and TLS 1.3, so IMO it makes sense to share the code and add a guard for the section that differs across 1.2 and 1.3?

hanno-becker · 2021-07-20T05:12:56Z

library/ssl_msg.c

@@ -2877,6 +2877,12 @@ void mbedtls_ssl_recv_flight_completed( mbedtls_ssl_context *ssl )
        ssl->handshake->retransmit_state = MBEDTLS_SSL_RETRANS_PREPARING;
 }

+static void ssl_reset_retransmit_timeout( mbedtls_ssl_context *ssl )


I reckon this was moved from somewhere? Where from?

According to hannestschofenig#15, different options report fail. To fix the issue we should not break passed options. The script is to make sure current status. If all relative issues are resolved, This patch should be removed or re-considered. issues: hannestschofenig#15, hannestschofenig#297,hannestschofenig#238,hannestschofenig#298,hannestschofenig#301 Change-Id: Iaebbdaa5861802f2a48e6bca238a94672ddfaf70 CustomizedGitHooks: yes Signed-off-by: Jerry Yu <[email protected]>

`write_version` and `read_version` is used under mps case. If mps is disabled, the functions should be disabled also. Change-Id: Ib55054eb1e6a0cd0a337ae64105817f025717150 Signed-off-by: Jerry Yu <[email protected]>

Remove MBED_SSL_SESSION_TICKETS condition to fix `start` not defined. And add TLS1_2_OR_EARLIER to avoid unused variable error Add MBEDTLS_SSL_RENEGOTIATION condition to fix `ret` not defined error. Change-Id: I76608d16c61a5135d78d2ad878b09bd6ac5a4899 Signed-off-by: Jerry Yu <[email protected]>

`mbedtls_ssl_check_cert_usage` is redefined in TLS1.3. That's due to different of `key_exchange` field. The function only use `key_exchange` field of `ciphersuite_info`. To keep consistency, we change the prototype of it. Change-Id: I1905866e3e5dbfbdbff760896fce8b8eb40502c4 Signed-off-by: Jerry Yu <[email protected]>

With TLS1.2 enabled, those functions report undeclared warning Change-Id: Ie20e9e9e9cee3fe8561c368c24042096b0b36320 Signed-off-by: Jerry Yu <[email protected]>

In config default function, sig_hashes is configed to diferent value according to TLS vesrion. To support TLS1.2 and 1.3, set different value according to version config. Change-Id: Ibef8e05b1453d2f0fc522eaf5b9ec370a84829e2 Signed-off-by: Jerry Yu <[email protected]>

mbedtls_ssl_conf_dtls_cookies depends on MBEDTLS_SSL_DTLS_HELLO_VERIFY. Not all place is wrapped with it. CustomizedGitHooks: yes Change-Id: I603cbaeabccf969c2785198409c0d59f3afa889f Signed-off-by: Jerry Yu <[email protected]>

…origin/pr/fix-warnings', 'origin/pr/fix-local-variable-undefine-error', 'origin/pr/fix-mbedtls_ssl_check_cert_usage-error', 'origin/pr/fix-functions-undeclared-warnings' and 'origin/pr/fix-sig_hashes-errors' into tls13-prototype Change-Id: Ic6ee391e3df9133c8c35ad3554b9571d558c1048

Change-Id: I63e218070e96637a15242fec3a66b5e448986287 CustomizedGitHooks: yes

First step to enable other protocol. Compile status: Fail Change-Id: I9fbb2e7994abe556db3954e69807f45b0f99d63e CustomizedGitHooks: yes Signed-off-by: Jerry Yu <[email protected]>

yuhaoth marked this pull request as draft July 16, 2021 10:10

yuhaoth force-pushed the pr/fix-compile-error branch from b02ad28 to 035a343 Compare July 19, 2021 07:46

hanno-becker changed the base branch from tls13-prototype to tls13-prototype-backup180520 July 20, 2021 04:37

hanno-becker changed the base branch from tls13-prototype-backup180520 to tls13-prototype July 20, 2021 04:37

hanno-becker changed the base branch from tls13-prototype to tls13-prototype-backup180520 July 20, 2021 04:53

hanno-becker changed the base branch from tls13-prototype-backup180520 to tls13-prototype July 20, 2021 04:53

hanno-becker reviewed Jul 20, 2021

View reviewed changes

hanno-becker mentioned this pull request Jul 20, 2021

[WIP]Re-introduce support of TLS 1.3 #290

Closed

hanno-becker reviewed Jul 20, 2021

View reviewed changes

yuhaoth force-pushed the pr/fix-compile-error branch 2 times, most recently from 1eb3318 to ce467cd Compare July 25, 2021 14:46

yuhaoth added 2 commits July 26, 2021 15:15

fix unused-function warning

a11a793

`write_version` and `read_version` is used under mps case. If mps is disabled, the functions should be disabled also. Change-Id: Ib55054eb1e6a0cd0a337ae64105817f025717150 Signed-off-by: Jerry Yu <[email protected]>

yuhaoth force-pushed the pr/fix-compile-error branch from ce467cd to f760a4e Compare July 26, 2021 13:35

yuhaoth force-pushed the pr/fix-compile-error branch from f760a4e to 1ec1800 Compare July 27, 2021 07:28

fix functions undeclared warnings

81c88ff

With TLS1.2 enabled, those functions report undeclared warning Change-Id: Ie20e9e9e9cee3fe8561c368c24042096b0b36320 Signed-off-by: Jerry Yu <[email protected]>

yuhaoth added 5 commits July 27, 2021 18:18

disable EXPORT_KEYS

8917997

Change-Id: I63e218070e96637a15242fec3a66b5e448986287 CustomizedGitHooks: yes

Enable MBEDTLS_SSL_PROTO_*

2306e50

First step to enable other protocol. Compile status: Fail Change-Id: I9fbb2e7994abe556db3954e69807f45b0f99d63e CustomizedGitHooks: yes Signed-off-by: Jerry Yu <[email protected]>

yuhaoth force-pushed the pr/fix-compile-error branch from 1ec1800 to 2306e50 Compare July 28, 2021 06:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP]fix compile error for re-introducing exists options. #293

[WIP]fix compile error for re-introducing exists options. #293

yuhaoth commented Jul 16, 2021

hanno-becker Jul 20, 2021

hanno-becker Jul 20, 2021

hanno-becker Jul 20, 2021

hanno-becker Jul 20, 2021

hanno-becker Jul 20, 2021

hanno-becker Jul 20, 2021

hanno-becker commented Jul 20, 2021

hanno-becker Jul 20, 2021

hanno-becker Jul 20, 2021

hanno-becker Jul 20, 2021

hanno-becker Jul 20, 2021

hanno-becker Jul 20, 2021

hanno-becker Jul 20, 2021

hanno-becker Jul 20, 2021

		#endif /* MBEDTLS_SSL_PROTO_TLS1_3_EXPERIMENTAL */
		size_t pmslen; /!< premaster length /

[WIP]fix compile error for re-introducing exists options. #293

Are you sure you want to change the base?

[WIP]fix compile error for re-introducing exists options. #293

Conversation

yuhaoth commented Jul 16, 2021

Description

Status

Requires Backporting

Migrations

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hanno-becker commented Jul 20, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment