Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

driver-only ECDSA: enable ECDSA-based TLS 1.2 key exchanges #7117

Merged
merged 5 commits into from
Mar 9, 2023
+44 −38
Merged

driver-only ECDSA: enable ECDSA-based TLS 1.2 key exchanges #7117

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
f84b7d5
test: enable ECDSA based key exchanges in driver coverage tests
valeriosetti Feb 23, 2023
30c4618
Add new PSA_HAS_FULL_ECDSA macro for easily signal that PSA has full …
valeriosetti Feb 23, 2023
75fba32
ssl: use new macros for ECDSA capabilities
valeriosetti Feb 23, 2023
2f08147
test: fix disparities in test_suite_ssl
valeriosetti Feb 23, 2023
1470ce3
fix typos
valeriosetti Mar 8, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 4 additions & 2 deletions include/mbedtls/check_config.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -280,7 +280,8 @@
#endif

#if defined(MBEDTLS_KEY_EXCHANGE_ECDH_ECDSA_ENABLED) && \
( !defined(MBEDTLS_ECDH_C) || !defined(MBEDTLS_ECDSA_C) || \
( !defined(MBEDTLS_ECDH_C) || \
!(defined(MBEDTLS_ECDSA_C) || defined(PSA_HAVE_FULL_ECDSA)) || \
!defined(MBEDTLS_X509_CRT_PARSE_C) )
#error "MBEDTLS_KEY_EXCHANGE_ECDH_ECDSA_ENABLED defined, but not all prerequisites"
Copy link
Contributor

@AndrzejKurek AndrzejKurek Mar 9, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, I think I know what's the issue:
if in component_test_psa_crypto_config_accel_ecdsa I remove

    scripts/config.py unset MBEDTLS_KEY_EXCHANGE_ECDHE_ECDSA_ENABLED
    scripts/config.py unset MBEDTLS_KEY_EXCHANGE_ECDH_ECDSA_ENABLED

everything builds and tests fine (as the test suites using ECDHE_ECDSA are skipped, there's no MBEDTLS_PK_CAN_ECDSA_SOME), but ssl-server2 and ssl-client2 report that they can use ECDHE-ECDSA ciphersuites, and the handshake fails.
Is it a gap in the dependencies here?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, good catch! Indeed now that you mention it the ECDSA part of the dependency should be:

((!defined(MBEDTLS_USE_PSA_CRYPTO) && defined(MBEDTLS_ECDSA_C)) || \
 (defined(MBEDTLS_USE_PSA_CRYPTO) && defined(PSA_HAVE_FULL_ECDSA)))

Wow, sorry for missing that and thanks for trying things out and catching it!

I wanted to do a final ECDSA PR to add a ChangeLog entry, I'll fix that there too.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, that's the second time that that tricky pair of MBEDTLS_USE_PSA_CRYPTO vs !MBEDTLS_USE_PSA_CRYPTO catches us :) Thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, it took me a while to understand the point here, but I agree with you now :)
@AndrzejKurek thanks for the hint!

@mpg you can assign the new issue to me so I will complete this part

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or can I simply create a pull request to fix this part without an associated issue?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, sorry, I had already started working on it before I was your comment.

But in general yes, it's OK to create a PR without an issue - we just need to add a size label to the PR and add it to the EPICs board in the correct EPIC. (PRs linked to an issue don't need that as they show on the board attached to their issue.)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, sorry, I had already started working on it before I was your comment.

No problem! Thanks for taking care of it ;)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#7245

#endif
Expand Down Expand Up @@ -313,7 +314,8 @@
#endif

#if defined(MBEDTLS_KEY_EXCHANGE_ECDHE_ECDSA_ENABLED) && \
( !defined(MBEDTLS_ECDH_C) || !defined(MBEDTLS_ECDSA_C) || \
( !defined(MBEDTLS_ECDH_C) || \
!(defined(MBEDTLS_ECDSA_C) || defined(PSA_HAVE_FULL_ECDSA)) || \
!defined(MBEDTLS_X509_CRT_PARSE_C) )
#error "MBEDTLS_KEY_EXCHANGE_ECDHE_ECDSA_ENABLED defined, but not all prerequisites"
#endif
Expand Down
5 changes: 5 additions & 0 deletions include/mbedtls/config_psa.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -308,6 +308,11 @@ extern "C" {
#define PSA_HAVE_SOFT_BLOCK_AEAD 1
#endif

#if defined(PSA_WANT_ALG_ECDSA) && defined(PSA_WANT_KEY_TYPE_ECC_KEY_PAIR) && \
defined(PSA_WANT_KEY_TYPE_ECC_PUBLIC_KEY)
#define PSA_HAVE_FULL_ECDSA 1
#endif

#if defined(PSA_WANT_KEY_TYPE_AES)
#if !defined(MBEDTLS_PSA_ACCEL_KEY_TYPE_AES)
#define PSA_HAVE_SOFT_KEY_TYPE_AES 1
Expand Down
25 changes: 14 additions & 11 deletions library/ssl_tls.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1206,7 +1206,7 @@ static int ssl_handshake_init(mbedtls_ssl_context *ssl)
if (mbedtls_ssl_hash_from_md_alg(*md) == MBEDTLS_SSL_HASH_NONE) {
continue;
}
#if defined(MBEDTLS_ECDSA_C)
#if defined(MBEDTLS_PK_CAN_ECDSA_SOME)
sig_algs_len += sizeof(uint16_t);
#endif

Expand Down Expand Up @@ -1234,7 +1234,7 @@ static int ssl_handshake_init(mbedtls_ssl_context *ssl)
if (hash == MBEDTLS_SSL_HASH_NONE) {
continue;
}
#if defined(MBEDTLS_ECDSA_C)
#if defined(MBEDTLS_PK_CAN_ECDSA_SOME)
*p = ((hash << 8) | MBEDTLS_SSL_SIG_ECDSA);
p++;
#endif
Expand Down Expand Up @@ -4979,22 +4979,25 @@ static int ssl_preset_suiteb_ciphersuites[] = {
*/
static uint16_t ssl_preset_default_sig_algs[] = {

#if defined(MBEDTLS_ECDSA_C) && defined(MBEDTLS_HAS_ALG_SHA_256_VIA_MD_OR_PSA_BASED_ON_USE_PSA) && \
#if defined(MBEDTLS_PK_CAN_ECDSA_SOME) && \
defined(MBEDTLS_HAS_ALG_SHA_256_VIA_MD_OR_PSA_BASED_ON_USE_PSA) && \
defined(MBEDTLS_ECP_DP_SECP256R1_ENABLED)
MBEDTLS_TLS1_3_SIG_ECDSA_SECP256R1_SHA256,
#endif /* MBEDTLS_ECDSA_C && MBEDTLS_HAS_ALG_SHA_256_VIA_MD_OR_PSA_BASED_ON_USE_PSA &&
#endif /* MBEDTLS_PK_CAN_ECDSA_SOME && MBEDTLS_HAS_ALG_SHA_256_VIA_MD_OR_PSA_BASED_ON_USE_PSA &&
MBEDTLS_ECP_DP_SECP256R1_ENABLED */

#if defined(MBEDTLS_ECDSA_C) && defined(MBEDTLS_HAS_ALG_SHA_384_VIA_MD_OR_PSA_BASED_ON_USE_PSA) && \
#if defined(MBEDTLS_PK_CAN_ECDSA_SOME) && \
defined(MBEDTLS_HAS_ALG_SHA_384_VIA_MD_OR_PSA_BASED_ON_USE_PSA) && \
defined(MBEDTLS_ECP_DP_SECP384R1_ENABLED)
MBEDTLS_TLS1_3_SIG_ECDSA_SECP384R1_SHA384,
#endif /* MBEDTLS_ECDSA_C && MBEDTLS_HAS_ALG_SHA_384_VIA_MD_OR_PSA_BASED_ON_USE_PSA&&
#endif /* MBEDTLS_PK_CAN_ECDSA_SOME && MBEDTLS_HAS_ALG_SHA_384_VIA_MD_OR_PSA_BASED_ON_USE_PSA&&
MBEDTLS_ECP_DP_SECP384R1_ENABLED */

#if defined(MBEDTLS_ECDSA_C) && defined(MBEDTLS_HAS_ALG_SHA_512_VIA_MD_OR_PSA_BASED_ON_USE_PSA) && \
#if defined(MBEDTLS_PK_CAN_ECDSA_SOME) && \
defined(MBEDTLS_HAS_ALG_SHA_512_VIA_MD_OR_PSA_BASED_ON_USE_PSA) && \
defined(MBEDTLS_ECP_DP_SECP521R1_ENABLED)
MBEDTLS_TLS1_3_SIG_ECDSA_SECP521R1_SHA512,
#endif /* MBEDTLS_ECDSA_C && MBEDTLS_HAS_ALG_SHA_384_VIA_MD_OR_PSA_BASED_ON_USE_PSA&&
#endif /* MBEDTLS_PK_CAN_ECDSA_SOME && MBEDTLS_HAS_ALG_SHA_384_VIA_MD_OR_PSA_BASED_ON_USE_PSA&&
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
#endif /* MBEDTLS_PK_CAN_ECDSA_SOME && MBEDTLS_HAS_ALG_SHA_384_VIA_MD_OR_PSA_BASED_ON_USE_PSA&&
#endif /* MBEDTLS_PK_CAN_ECDSA_SOME && MBEDTLS_HAS_ALG_SHA_512_VIA_MD_OR_PSA_BASED_ON_USE_PSA &&

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(This is minor, not worth pushing if there's no other issue)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Noted! In case no other change is required for this PR, I will solve it in a following one (ex: the one that I will create for addressing #7148)

MBEDTLS_ECP_DP_SECP521R1_ENABLED */

#if defined(MBEDTLS_X509_RSASSA_PSS_SUPPORT) && \
Expand Down Expand Up @@ -5034,7 +5037,7 @@ static uint16_t ssl_preset_default_sig_algs[] = {
#if defined(MBEDTLS_SSL_PROTO_TLS1_2)
static uint16_t ssl_tls12_preset_default_sig_algs[] = {
#if defined(MBEDTLS_HAS_ALG_SHA_512_VIA_MD_OR_PSA_BASED_ON_USE_PSA)
#if defined(MBEDTLS_ECDSA_C)
#if defined(MBEDTLS_PK_CAN_ECDSA_SOME)
MBEDTLS_SSL_TLS12_SIG_AND_HASH_ALG(MBEDTLS_SSL_SIG_ECDSA, MBEDTLS_SSL_HASH_SHA512),
#endif
#if defined(MBEDTLS_X509_RSASSA_PSS_SUPPORT)
Expand All @@ -5045,7 +5048,7 @@ static uint16_t ssl_tls12_preset_default_sig_algs[] = {
#endif
#endif /* MBEDTLS_HAS_ALG_SHA_512_VIA_MD_OR_PSA_BASED_ON_USE_PSA*/
#if defined(MBEDTLS_HAS_ALG_SHA_384_VIA_MD_OR_PSA_BASED_ON_USE_PSA)
#if defined(MBEDTLS_ECDSA_C)
#if defined(MBEDTLS_PK_CAN_ECDSA_SOME)
MBEDTLS_SSL_TLS12_SIG_AND_HASH_ALG(MBEDTLS_SSL_SIG_ECDSA, MBEDTLS_SSL_HASH_SHA384),
#endif
#if defined(MBEDTLS_X509_RSASSA_PSS_SUPPORT)
Expand All @@ -5056,7 +5059,7 @@ static uint16_t ssl_tls12_preset_default_sig_algs[] = {
#endif
#endif /* MBEDTLS_HAS_ALG_SHA_384_VIA_MD_OR_PSA_BASED_ON_USE_PSA*/
#if defined(MBEDTLS_HAS_ALG_SHA_256_VIA_MD_OR_PSA_BASED_ON_USE_PSA)
#if defined(MBEDTLS_ECDSA_C)
#if defined(MBEDTLS_PK_CAN_ECDSA_SOME)
MBEDTLS_SSL_TLS12_SIG_AND_HASH_ALG(MBEDTLS_SSL_SIG_ECDSA, MBEDTLS_SSL_HASH_SHA256),
#endif
#if defined(MBEDTLS_X509_RSASSA_PSS_SUPPORT)
Expand Down
4 changes: 0 additions & 4 deletions tests/scripts/all.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2111,10 +2111,6 @@ config_psa_crypto_config_ecdsa_use_psa () {
# Disable the module that's accelerated
scripts/config.py unset MBEDTLS_ECDSA_C
fi
# Disable things that depend on it
# TODO: make these work - #6862
scripts/config.py unset MBEDTLS_KEY_EXCHANGE_ECDHE_ECDSA_ENABLED
scripts/config.py unset MBEDTLS_KEY_EXCHANGE_ECDH_ECDSA_ENABLED
# Restartable feature is not yet supported by PSA. Once it will in
# the future, the following line could be removed (see issues
# 6061, 6332 and following ones)
Expand Down
2 changes: 1 addition & 1 deletion tests/ssl-opt.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1450,7 +1450,7 @@ do_run_test_once() {
# detect_required_features() function), it does NOT guarantee that the
# result is accurate. It does not check other conditions, such as:
# - MBEDTLS_SSL_PROTO_TLS1_x can be disabled to selectively remove
# TLS 1.2/1.3 suppport
# TLS 1.2/1.3 support
# - we can force a ciphersuite which contains "WITH" in its name, meaning
# that we are going to use TLS 1.2
# - etc etc
Expand Down
Loading