Make use of FFDH keys in TLS 1.3 v.2 #7627

mprse · 2023-05-22T07:39:54Z

Description

Resolves: #5979
Continuation of: #6102
Needs: #7577

This PR adds support for FFDH keys and uses structures and adapts code used for ECDH computation.
Provides only mbedtls tests in ssl-opt.sh

PR checklist

Please tick as appropriate and edit the reasons (e.g.: "backport: not needed because this is a new feature")

changelog provided
backport not required (new feature)
tests provided (partially, missing openssl, gnutls tests)

mpg

Looks pretty good overall, most points are fairly minor, except the reliance on dhm.h which I think is structurally no what we want at this point.

library/ssl_tls.c

library/ssl_client.c

include/mbedtls/psa_util.h

include/mbedtls/dhm.h

tests/ssl-opt.sh

library/ssl_tls12_client.c

library/ssl_tls12_server.c

ChangeLog.d/ffdh-tls-1-3.txt

library/ssl_misc.h

mpg

Thanks for addressing my feedback! I like what you've done with the functions that handle human-readable names, I think it's really neat now.

I also like the unification of ECDH and FFDH when writing our key share, but I have a number of suggestions to improve it further.

Edit: see also: #7627 (comment) and #7627 (comment)

programs/ssl/ssl_test_lib.c

library/ssl_tls13_generic.c

mpg

Looks good to me, but while the iron's hot, let's try to improve as discussed here (new helper macro + trying to de-duplicate code unless it turns out to be harder than I think).

library/ssl_tls13_generic.c

mpg

LGTM, thanks!

mprse · 2023-06-05T15:34:15Z

Force-pushed the following fix:

@@ -528,10 +528,10 @@ int parse_curves(const char *curves, uint16_t *group_list, size_t group_list_len
             }
         }
 
-        mbedtls_printf("Number of curves: %d\n", i);
+        mbedtls_printf("Number of curves: %lu\n", i);
 
         if (i == group_list_len - 1 && *p != '\0') {
-            mbedtls_printf("curves list too long, maximum %d",
+            mbedtls_printf("curves list too long, maximum %lu",
                            group_list_len - 1);
             return -1;
         }

Edit:
zu -> lu as mingw had problem with zu.

mpg · 2023-06-06T08:47:29Z

@mprse I think we also need to cast the argument: printf("... %lu ...", (unsigned long) foo);. It's quite annoying that in 2023 we can't seem to use %zu yet.

mprse · 2023-06-27T12:50:41Z

CI was green and I added only the last commit to fix comments (shouldn't cause any harm).
As discussed earlier, remaining comments (adapting function names, parameters names, structures, etc) will be fixed in the follow-up PR as this one becomes big already.
Setting to ready for review.

Signed-off-by: Przemek Stekiel <przemyslaw.stekiel@mobica.com>

mprse · 2023-06-28T07:16:49Z

Force pushed last commit to restart CI as it seems it was aborted (not sure now if my happiness from green CI wasn't too early) .

Also it seems that there is still problem with ffdh8192 on CI (even with one retry):


TLS 1.3 m->m: HRR x25519 -> ffdhe8192 .................................. RETRY(client-timeout) RETRY(client-timeout) FAIL
  ! bad client exit code (expected 0, got 143)
  ! outputs saved to o-XXX-1682.log

TLS 1.3 m->m: HRR x448 -> ffdhe8192 .................................... RETRY(client-timeout) RETRY(client-timeout) FAIL
  ! bad client exit code (expected 0, got 143)
  ! outputs saved to o-XXX-1688.log

TLS 1.3 m->m: HRR ffdhe2048 -> ffdhe8192 ............................... RETRY(client-timeout) PASS

mpg · 2023-06-28T08:28:35Z

Do we really need to use the largest ffdh group in tests? Or could we only test with ffdhe2048 and ffdhe3072 in all the compat tests in order to avoid the performance issues?

Perhaps keep only one test with ffdh8072 in order to make sure buffer sizes are correct, but that one would be guarded by client_needs_more_time 4 and not_with_valgrind to reduce the probability of timeouts. (Btw, this is not about adding retries, it's about enlarging the delay before timeout.)

Wdyt?

mprse · 2023-06-28T08:36:09Z

Do we really need to use the largest ffdh group in tests? Or could we only test with ffdhe2048 and ffdhe3072 in all the compat tests in order to avoid the performance issues?

First we had all groups tested but this was too much, so we decided to have only corner cases (smallest and biggest group). This makes sense, but since we have such problems on CI maybe using FFDHE6144 instead FFDHE8192 would be enough?

mpg · 2023-06-28T08:52:33Z

I think we need to differentiate our strategy based on what we're testing. It looks to me like the numerous "TLS 1.3 X->Y: HRR zzz -> ttt" are mostly about negotiation by means of HelloRetryRequest: here I think we don't care about using large groups, and just using any two distinct groups would be enough, so I'd pick the two smallest groups.

Then I think we need one test using the largest group with ourselves to validate buffer sizes. Then one interop test with each group with GnuTLS and/or OpenSSL to make sure we don't have group-specific errors (such as a typo in the group parameters or their IDs).

(Note: this might apply to curves as well. I'm not sure we need to test absolutely all combinations of curves either in the HelloRetryRequest tests.)

@ronald-cron-arm wdyt?

mprse · 2023-06-28T10:07:03Z

Then I think we need one test using the largest group with ourselves to validate buffer sizes. Then one interop test with each group with GnuTLS and/or OpenSSL to make sure we don't have group-specific errors (such as a typo in the group parameters or their IDs).

Should these tests be manually added to ssl-opt.sh?

So I will leave only ffdhe2048 to be generated by script and add single G->m and m->G (GNU tls will be used as openssl doesn't support FFDH) test for each group other than ffdhe2048 in ssl-opt.sh:

Test cases added to ssl-opt.sh for groups other than ffdhe2048:

requires_config_enabled MBEDTLS_SSL_SRV_C
requires_config_enabled MBEDTLS_DEBUG_C
requires_config_enabled MBEDTLS_SSL_TLS1_3_KEY_EXCHANGE_MODE_EPHEMERAL_ENABLED
requires_config_enabled MBEDTLS_SSL_TLS1_3_COMPATIBILITY_MODE
requires_config_enabled MBEDTLS_X509_RSASSA_PSS_SUPPORT
requires_config_enabled PSA_WANT_ALG_FFDH
requires_gnutls_tls1_3
requires_gnutls_next_no_ticket
requires_gnutls_next_disable_tls13_compat
run_test "TLS 1.3 G->m: AES_128_GCM_SHA256,ffdhe2048,rsa_pss_rsae_sha256" \
         "$P_SRV crt_file=data_files/server2-sha256.crt key_file=data_files/server2.key debug_level=4 force_ciphersuite=TLS1-3-AES-128-GCM-SHA256 sig_algs=rsa_pss_rsae_sha256 curves=ffdhe2048 tls13_kex_modes=ephemeral cookies=0 tickets=0" \
         "$G_NEXT_CLI_NO_CERT --debug=4 --single-key-share --x509cafile data_files/test-ca_cat12.crt --priority=NONE:+AES-128-GCM:+SHA256:+AEAD:+SIGN-RSA-PSS-RSAE-SHA256:+GROUP-FFDHE2048:+VERS-TLS1.3:%NO_TICKETS" \
         0 \
         -s "Protocol is TLSv1.3" \
         -s "server hello, chosen ciphersuite: TLS1-3-AES-128-GCM-SHA256 ( id=4865 )" \
         -s "received signature algorithm: 0x804" \
         -s "got named group: ffdhe2048(0100)" \
         -s "Certificate verification was skipped" \
         -C "received HelloRetryRequest message"
		 
		 
requires_gnutls_tls1_3
requires_gnutls_next_no_ticket
requires_gnutls_next_disable_tls13_compat
requires_config_enabled MBEDTLS_SSL_CLI_C
requires_config_enabled MBEDTLS_DEBUG_C
requires_config_enabled MBEDTLS_SSL_TLS1_3_KEY_EXCHANGE_MODE_EPHEMERAL_ENABLED
requires_config_enabled MBEDTLS_SSL_TLS1_3_COMPATIBILITY_MODE
requires_config_enabled MBEDTLS_X509_RSASSA_PSS_SUPPORT
requires_config_enabled PSA_WANT_ALG_FFDH
run_test "TLS 1.3 m->G: AES_128_GCM_SHA256,ffdhe2048,rsa_pss_rsae_sha256" \
         "$G_NEXT_SRV_NO_CERT --http --disable-client-cert --debug=4 --x509certfile data_files/server2-sha256.crt --x509keyfile data_files/server2.key --priority=NONE:+AES-128-GCM:+SHA256:+AEAD:+SIGN-RSA-PSS-RSAE-SHA256:+GROUP-FFDHE2048:+VERS-TLS1.3:%NO_TICKETS" \
         "$P_CLI ca_file=data_files/test-ca_cat12.crt debug_level=4 force_ciphersuite=TLS1-3-AES-128-GCM-SHA256 sig_algs=rsa_pss_rsae_sha256 curves=ffdhe2048" \
         0 \
         -c "HTTP/1.0 200 OK" \
         -c "Protocol is TLSv1.3" \
         -c "server hello, chosen ciphersuite: ( 1301 ) - TLS1-3-AES-128-GCM-SHA256" \
         -c "Certificate Verify: Signature algorithm ( 0804 )" \
         -c "NamedGroup: ffdhe2048 ( 100 )" \
         -c "Verifying peer X.509 certificate... ok" \
         -C "received HelloRetryRequest message"

mpg · 2023-06-28T10:41:26Z

Should these tests be manually added to ssl-opt.sh?

That was my thinking, yes.

ronald-cron-arm · 2023-06-28T11:13:09Z

Do we really need to use the largest ffdh group in tests? Or could we only test with ffdhe2048 and ffdhe3072 in all the compat tests in order to avoid the performance issues?

Perhaps keep only one test with ffdh8072 in order to make sure buffer sizes are correct, but that one would be guarded by client_needs_more_time 4 and not_with_valgrind to reduce the probability of timeouts. (Btw, this is not about adding retries, it's about enlarging the delay before timeout.)

Wdyt?

Yes that looks reasonable to me.

ronald-cron-arm · 2023-06-28T11:16:49Z

I think we need to differentiate our strategy based on what we're testing. It looks to me like the numerous "TLS 1.3 X->Y: HRR zzz -> ttt" are mostly about negotiation by means of HelloRetryRequest: here I think we don't care about using large groups, and just using any two distinct groups would be enough, so I'd pick the two smallest groups.

Then I think we need one test using the largest group with ourselves to validate buffer sizes. Then one interop test with each group with GnuTLS and/or OpenSSL to make sure we don't have group-specific errors (such as a typo in the group parameters or their IDs).

(Note: this might apply to curves as well. I'm not sure we need to test absolutely all combinations of curves either in the HelloRetryRequest tests.)

@ronald-cron-arm wdyt?

That looks good to me.

ronald-cron-arm · 2023-06-28T11:17:12Z

Then I think we need one test using the largest group with ourselves to validate buffer sizes. Then one interop test with each group with GnuTLS and/or OpenSSL to make sure we don't have group-specific errors (such as a typo in the group parameters or their IDs).

Should these tests be manually added to ssl-opt.sh?

So I will leave only ffdhe2048 to be generated by script and add single G->m and m->G (GNU tls will be used as openssl doesn't support FFDH) test for each group other than ffdhe2048 in ssl-opt.sh:

Test cases added to ssl-opt.sh for groups other than ffdhe2048:

requires_config_enabled MBEDTLS_SSL_SRV_C
requires_config_enabled MBEDTLS_DEBUG_C
requires_config_enabled MBEDTLS_SSL_TLS1_3_KEY_EXCHANGE_MODE_EPHEMERAL_ENABLED
requires_config_enabled MBEDTLS_SSL_TLS1_3_COMPATIBILITY_MODE
requires_config_enabled MBEDTLS_X509_RSASSA_PSS_SUPPORT
requires_config_enabled PSA_WANT_ALG_FFDH
requires_gnutls_tls1_3
requires_gnutls_next_no_ticket
requires_gnutls_next_disable_tls13_compat
run_test "TLS 1.3 G->m: AES_128_GCM_SHA256,ffdhe2048,rsa_pss_rsae_sha256" \
         "$P_SRV crt_file=data_files/server2-sha256.crt key_file=data_files/server2.key debug_level=4 force_ciphersuite=TLS1-3-AES-128-GCM-SHA256 sig_algs=rsa_pss_rsae_sha256 curves=ffdhe2048 tls13_kex_modes=ephemeral cookies=0 tickets=0" \
         "$G_NEXT_CLI_NO_CERT --debug=4 --single-key-share --x509cafile data_files/test-ca_cat12.crt --priority=NONE:+AES-128-GCM:+SHA256:+AEAD:+SIGN-RSA-PSS-RSAE-SHA256:+GROUP-FFDHE2048:+VERS-TLS1.3:%NO_TICKETS" \
         0 \
         -s "Protocol is TLSv1.3" \
         -s "server hello, chosen ciphersuite: TLS1-3-AES-128-GCM-SHA256 ( id=4865 )" \
         -s "received signature algorithm: 0x804" \
         -s "got named group: ffdhe2048(0100)" \
         -s "Certificate verification was skipped" \
         -C "received HelloRetryRequest message"
		 
		 
requires_gnutls_tls1_3
requires_gnutls_next_no_ticket
requires_gnutls_next_disable_tls13_compat
requires_config_enabled MBEDTLS_SSL_CLI_C
requires_config_enabled MBEDTLS_DEBUG_C
requires_config_enabled MBEDTLS_SSL_TLS1_3_KEY_EXCHANGE_MODE_EPHEMERAL_ENABLED
requires_config_enabled MBEDTLS_SSL_TLS1_3_COMPATIBILITY_MODE
requires_config_enabled MBEDTLS_X509_RSASSA_PSS_SUPPORT
requires_config_enabled PSA_WANT_ALG_FFDH
run_test "TLS 1.3 m->G: AES_128_GCM_SHA256,ffdhe2048,rsa_pss_rsae_sha256" \
         "$G_NEXT_SRV_NO_CERT --http --disable-client-cert --debug=4 --x509certfile data_files/server2-sha256.crt --x509keyfile data_files/server2.key --priority=NONE:+AES-128-GCM:+SHA256:+AEAD:+SIGN-RSA-PSS-RSAE-SHA256:+GROUP-FFDHE2048:+VERS-TLS1.3:%NO_TICKETS" \
         "$P_CLI ca_file=data_files/test-ca_cat12.crt debug_level=4 force_ciphersuite=TLS1-3-AES-128-GCM-SHA256 sig_algs=rsa_pss_rsae_sha256 curves=ffdhe2048" \
         0 \
         -c "HTTP/1.0 200 OK" \
         -c "Protocol is TLSv1.3" \
         -c "server hello, chosen ciphersuite: ( 1301 ) - TLS1-3-AES-128-GCM-SHA256" \
         -c "Certificate Verify: Signature algorithm ( 0804 )" \
         -c "NamedGroup: ffdhe2048 ( 100 )" \
         -c "Verifying peer X.509 certificate... ok" \
         -C "received HelloRetryRequest message"

I am ok with that.

- Full tests generated by script only for ffdhe2048 group - Single G->m and m->G exchange test for each other group Signed-off-by: Przemek Stekiel <przemyslaw.stekiel@mobica.com>

mpg

LGTM. Good job!

mpg · 2023-06-29T07:22:59Z

I had a look at the API-ABI report, and the changes are acceptable: two internal functions have been renamed.

mprse · 2023-06-29T12:22:12Z

Created follow-up PR that address remaining comments.

ronald-cron-arm

LGTM given that we've agreed to address remaining comments in a follow-up PR. I have also created a follow-up PR to try and improve one part of the code (#7862).

library/ssl_client.c

ronald-cron-arm · 2023-06-30T13:15:17Z

tests/scripts/generate_tls13_compat_tests.py

@@ -67,6 +67,7 @@
    'secp521r1': 0x19,
    'x25519': 0x1d,
    'x448': 0x1e,
+    'ffdhe2048': 0x100,


Suggested change

'ffdhe2048': 0x100,

# Only one finite field group to keep testing time within reasonable bounds.

'ffdhe2048': 0x100,

I am happy for this to be done in the follow-up PR.

mpg · 2023-07-03T08:12:24Z

I'm merging this before conflicts appear, and work will continue in #7858 and #7862.

mprse added needs-preceding-pr Requires another PR to be merged first needs-ci Needs to pass CI tests priority-high High priority - will be reviewed soon labels May 22, 2023

mprse added this to To Do in Roadmap Board for Mbed TLS via automation May 22, 2023

mprse mentioned this pull request May 22, 2023

Make use of FFDH keys in TLS 1.3 #6102

Closed

2 tasks

mprse force-pushed the ffdh_tls13_v2 branch 3 times, most recently from e6d6df0 to 211a676 Compare May 23, 2023 11:56

mprse removed the needs-ci Needs to pass CI tests label May 24, 2023

mprse force-pushed the ffdh_tls13_v2 branch from 5063d40 to 4b7a9d8 Compare May 26, 2023 08:25

mprse mentioned this pull request May 26, 2023

FFDH 4: driver-only parity testing - with TLS 1.3 #7656

Merged

3 tasks

mpg self-requested a review May 30, 2023 08:43

ronald-cron-arm self-requested a review May 30, 2023 10:14

mpg requested changes May 30, 2023

View reviewed changes

Roadmap Board for Mbed TLS automation moved this from To Do to In Development May 30, 2023

mpg mentioned this pull request May 30, 2023

TLS: Clean up (EC)DH dependencies #7612

Closed

mprse force-pushed the ffdh_tls13_v2 branch from 0fee37c to a877e2b Compare June 1, 2023 10:42

mprse added needs-review Every commit must be reviewed by at least two team members, needs-ci Needs to pass CI tests and removed needs-preceding-pr Requires another PR to be merged first labels Jun 1, 2023

mprse requested a review from mpg June 2, 2023 05:21

mprse removed the needs-ci Needs to pass CI tests label Jun 2, 2023

mpg reviewed Jun 2, 2023

View reviewed changes

mpg requested changes Jun 5, 2023

View reviewed changes

library/ssl_tls13_generic.c Outdated Show resolved Hide resolved

mpg previously approved these changes Jun 5, 2023

View reviewed changes

mprse dismissed mpg’s stale review via 117107e June 5, 2023 15:33

mprse force-pushed the ffdh_tls13_v2 branch from be49551 to 117107e Compare June 5, 2023 15:33

mprse force-pushed the ffdh_tls13_v2 branch from 117107e to 7fda8ec Compare June 6, 2023 07:51

mprse requested review from mpg and ronald-cron-arm June 27, 2023 12:51

Fix description of functions

7dda271

Signed-off-by: Przemek Stekiel <przemyslaw.stekiel@mobica.com>

mprse force-pushed the ffdh_tls13_v2 branch from c3aaaf6 to 7dda271 Compare June 28, 2023 07:16

Change ffdh testing strategy

3484db4

- Full tests generated by script only for ffdhe2048 group - Single G->m and m->G exchange test for each other group Signed-off-by: Przemek Stekiel <przemyslaw.stekiel@mobica.com>

mprse removed the needs-ci Needs to pass CI tests label Jun 28, 2023

mpg approved these changes Jun 29, 2023

View reviewed changes

mprse mentioned this pull request Jun 29, 2023

Make use of FFDH keys in TLS 1.3 - follow-up #7858

Merged

3 tasks

ronald-cron-arm mentioned this pull request Jun 30, 2023

Improve write supported group ext #7862

Merged

3 tasks

ronald-cron-arm approved these changes Jun 30, 2023

View reviewed changes

Roadmap Board for Mbed TLS automation moved this from In Development to Has Approval Jun 30, 2023

mpg added approved Design and code approved - may be waiting for CI or backports and removed needs-review Every commit must be reviewed by at least two team members, labels Jul 3, 2023

mpg merged commit 56b159a into Mbed-TLS:development Jul 3, 2023
14 of 17 checks passed

Roadmap Board for Mbed TLS automation moved this from Has Approval to Done Jul 3, 2023

yuhaoth mentioned this pull request Jul 4, 2023

Bug:fix test_tls13_only_ephemeral_ffdh fail #7871

Closed

3 tasks

ronald-cron-arm mentioned this pull request Jul 6, 2023

tls13: server: Fix spurious HRR #7887

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make use of FFDH keys in TLS 1.3 v.2 #7627

Make use of FFDH keys in TLS 1.3 v.2 #7627

mprse commented May 22, 2023

mpg left a comment

mpg left a comment •

edited

mpg left a comment

mpg left a comment

mprse commented Jun 5, 2023 •

edited

mpg commented Jun 6, 2023

mprse commented Jun 27, 2023

mprse commented Jun 28, 2023 •

edited

mpg commented Jun 28, 2023

mprse commented Jun 28, 2023

mpg commented Jun 28, 2023 •

edited

mprse commented Jun 28, 2023 •

edited

mpg commented Jun 28, 2023

ronald-cron-arm commented Jun 28, 2023

ronald-cron-arm commented Jun 28, 2023

ronald-cron-arm commented Jun 28, 2023

mpg left a comment

mpg commented Jun 29, 2023

mprse commented Jun 29, 2023

ronald-cron-arm left a comment •

edited

ronald-cron-arm Jun 30, 2023

mpg commented Jul 3, 2023

	'ffdhe2048': 0x100,
	# Only one finite field group to keep testing time within reasonable bounds.
	'ffdhe2048': 0x100,

Make use of FFDH keys in TLS 1.3 v.2 #7627

Make use of FFDH keys in TLS 1.3 v.2 #7627

Conversation

mprse commented May 22, 2023

Description

PR checklist

mpg left a comment

Choose a reason for hiding this comment

mpg left a comment • edited

Choose a reason for hiding this comment

mpg left a comment

Choose a reason for hiding this comment

mpg left a comment

Choose a reason for hiding this comment

mprse commented Jun 5, 2023 • edited

mpg commented Jun 6, 2023

mprse commented Jun 27, 2023

mprse commented Jun 28, 2023 • edited

mpg commented Jun 28, 2023

mprse commented Jun 28, 2023

mpg commented Jun 28, 2023 • edited

mprse commented Jun 28, 2023 • edited

mpg commented Jun 28, 2023

ronald-cron-arm commented Jun 28, 2023

ronald-cron-arm commented Jun 28, 2023

ronald-cron-arm commented Jun 28, 2023

mpg left a comment

Choose a reason for hiding this comment

mpg commented Jun 29, 2023

mprse commented Jun 29, 2023

ronald-cron-arm left a comment • edited

Choose a reason for hiding this comment

ronald-cron-arm Jun 30, 2023

Choose a reason for hiding this comment

mpg commented Jul 3, 2023

mpg left a comment •

edited

mprse commented Jun 5, 2023 •

edited

mprse commented Jun 28, 2023 •

edited

mpg commented Jun 28, 2023 •

edited

mprse commented Jun 28, 2023 •

edited

ronald-cron-arm left a comment •

edited