-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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.
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
Fix the build with MBEDTLS_USE_PSA_CRYPTO without ECDSA #3585
Fix the build with MBEDTLS_USE_PSA_CRYPTO without ECDSA #3585
Conversation
ChangeLog.d/bugfix_PR3294.txt
Outdated
@@ -0,0 +1,8 @@ | |||
Bugfix | |||
* Add guards in pk_wrap.c to ensure if ECDSA is not defined, errors are |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggestion: Focus on the Why? instead of the What?: E.g. "Fix build failure in configurations where MBEDTLS_USE_PSA_CRYPTO is enabled but ECDSA is disabled. Contributed by XXX. Fixes #3294."
library/pk_wrap.c
Outdated
@@ -994,11 +996,24 @@ static int pk_ecdsa_sig_asn1_from_psa( unsigned char *sig, size_t *sig_len, | |||
return( 0 ); | |||
} | |||
|
|||
#endif |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor: We usually add a comment indicating which guard is being closed: #endif /* MBEDTLS_ECDSA_C */
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems to remain open.
library/pk_wrap.c
Outdated
@@ -1029,6 +1044,7 @@ static int pk_opaque_sign_wrap( void *ctx, mbedtls_md_type_t md_alg, | |||
|
|||
/* transcode it to ASN.1 sequence */ | |||
return( pk_ecdsa_sig_asn1_from_psa( sig, sig_len, buf_len ) ); | |||
#endif |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here.
library/pk_wrap.c
Outdated
((void) sig_len); | ||
((void) f_rng); | ||
((void) p_rng); | ||
return( PSA_ERROR_NOT_SUPPORTED ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This function should return an error from the Mbed TLS PK error space; MBEDTLS_ERR_PK_FEATURE_UNAVAILABLE
seem like a good fit.
library/pk_wrap.c
Outdated
@@ -34,7 +34,7 @@ | |||
#include "mbedtls/ecp.h" | |||
#endif | |||
|
|||
#if defined(MBEDTLS_ECDSA_C) | |||
#if defined(MBEDTLS_ECDSA_C) || defined(MBEDTLS_USE_PSA_CRYPTO) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change doesn't seem to be necessary anymore after the addition of the guards below. Am I missing something?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot @jdurkop for your work, I think the PR fixes the bug in the right way. As indicated in the comments, there are a few minor points that need changing, mostly about style and the ChangeLog, but otherwise the PR looks good.
@@ -912,6 +912,8 @@ static int pk_opaque_can_do( mbedtls_pk_type_t type ) | |||
type == MBEDTLS_PK_ECDSA ); | |||
} | |||
|
|||
#if defined(MBEDTLS_ECDSA_C) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Style: Trailing whitespace.
ChangeLog.d/bugfix_PR3294.txt
Outdated
Bugfix | ||
* Add guards in pk_wrap.c to ensure if ECDSA is not defined, errors are | ||
returned. Remove warnings in pk.c for unused variables. Add new test | ||
(test_depends_pkalgs_psa) to all.sh to confirm when USE_PSA_CRYPTO |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Style: Trailing whitespace.
Add guards in pk_wrap.c to ensure if ECDSA is not defined, errors are returned. Remove warnings in pk.c for unused variables. Add new test (test_depends_pkalgs_psa) to all.sh to confirm when USE_PSA_CRYPTO is defined that features are working properly. Fix Mbed-TLS#3294 Signed-off-by: John Durkop <john.durkop@fermatsoftware.com>
Signed-off-by: John Durkop <john.durkop@fermatsoftware.com>
Add new test (test_depends_curves_psa) to all.sh to confirm that test is passing when MBEDTLS_USE_PSA_CRYPTO is defined. Fix Mbed-TLS#3294 Signed-off-by: John Durkop <john.durkop@fermatsoftware.com>
20a277f
to
c14be90
Compare
Minor updates to changelog for more concise wording and fixed styling in other files as needed. Signed-off-by: John Durkop <john.durkop@fermatsoftware.com>
Thanks for the review comments. I have incorporated the changes and updated the files. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @jdurkop for the rework. There's one trivial issue remaining, see https://github.com/ARMmbed/mbedtls/pull/3585/files#r475713073. Other than that, it looks good to me - let's wait if the CI picks anything up.
Fixes Mbed-TLS#3294 Signed-off-by: John Durkop <john.durkop@fermatsoftware.com>
Good catch, I thought I had all those missing guards the first time. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
During testing and development of this issue, I uncovered a related issue. There are two versions of the ecdsa_verify_wrap() function, one with MBEDTLS_USE_PSA_CRYPTO and one when it is not enabled. The non PSA version is not using the md_alg parameter since it is nt really requied, The PSA version of the function was using that parameter to derive a different value it needed for PSA_ALG_ECDSA. The argument of PSA_ALG_ECDSA is ignored for psa_sign_hash and psa_verify_hash. It's present because it's used and must be a valid hash, not zero, for psa_sign_hash (but not psa_verify_hash) with PSA_ALG_DETERMINISTIC_ECDSA, and it's needed for psa_{sign,verify}_message which we don't implement yet. So basically the code really just needs to define the local parameter to PSA_ALG_ECDSA_ANY for the verify function and avoid using the md_alg and returning incorrect error codes. The previous error the main fix addresses was masking this issue because tests would not get this far. I created a different issue ticket that I was planning on applying this issue to but when I noticed the CI test failed due to the depends-hashes test I knew it was related to this change. The test it ran ends up running depends-hashes with USE_PSA_CRYPTO enabled which will show a failure unless this patch is used. I don't have any insights why the Mbed OS Testing is failing both builds. |
The Mbed OS failures are preexisting, known issues. Please ignore Mbed OS tests for now. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @jdurkop,
Apologies for the late reply. Thank you for the explanations on 2989826 - ideally, those should have been part of the commit message, making the commit self-contained. Also, the commit does not fix #3294 but rather another issue related to the PSA-PK wrapper, which could have been tracked and resolved independently.
But all that nitpicking aside, thanks for spotting the issue in the first place and for fixing it! (Again in the right way, in my opinion) To clarify that the verification tests don't depend on any hash, would you mind changing pk_ec_test_vec()
to use MBEDTLS_MD_NONE
again instead of MBEDTLS_MD_SHA1
, including a comment on the (ir)relevance of this value? Otherwise, one might wonder why the tests (a) appear to use SHA-1 in the first place, and (b) why they aren't guarded by MBEDTLS_SHA1_C
.
Thanks again for your work and patience!
@hanno-arm, thanks for the excellent clarification. Just to confirm your points, my plan is to correct the commit comment to include more details and also to do a minor fix in test_suite_pk.function for pk_ec_test_vec() by using MBEDTLS_MD_NONE instead of the MBEDTLS_MD_SHA1 with an updated comment. I'll push those commits later today if that sounds correct. |
@jdurkop That sounds good! |
Needed to make additional fixes so that when MBEDTLS_USE_PSA_CRYPTO is defined, the depends-hashes test will succeed. There are two versions of the ecdsa_verify_wrap() function, one with MBEDTLS_USE_PSA_CRYPTO and when when it is not enabled. The non PSA version is not using the md_alg parameter since it is not required. The PSA version was using that parameter to derive a different value it needed for PSA_ALG_ECDSA. The arguement of PSA_ALG_ECDSA is ignored for psa_sign_hash and psa_verify_hash. It is present because it is used and must be a valid hash, not zero, for psa_sign_hash (but not psa_verify_hash) with PSA_ALG_DETERMINISTIC_ECDSA, and it is needed for psa_sign_message and psa_verify_message which are not implemented yet. The local parameter now uses PSA_ALG_ECDSA_ANY for the verify function to avoid using the md_alg parameter and avoids returning incorrect error codes. Fixes Mbed-TLS#3587 Signed-off-by: John Durkop <john.durkop@fermatsoftware.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me apart from two things:
- We must fix the Travis issue.
- The history contains a merge commit. This isn't forbidden, but we try to only have merge commits in pull requests for complex cases, like when a major change happens in the code base during the review of a pull request, or when a complex feature gets merged after some time on a separate branch. This one doesn't warrant it. Please rewrite the history so that the two commits “Fix test issues with depends-hashes” don't happen in parallel.
@@ -1179,11 +1179,29 @@ component_test_depends_curves () { | |||
record_status tests/scripts/curves.pl | |||
} | |||
|
|||
component_test_depends_curves_psa () { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A consequence of this change that I hadn't thought of is that the Travis build now takes too long. Seen in https://api.travis-ci.org/v3/job/725229070/log.txt :
The job exceeded the maximum time limit for jobs, and has been terminated.
It only got as far as midway through component_test_depends_pkalgs_psa
.
I don't think all the “depends” tests are useful to have on Travis. They all run on Jenkins as well which can have a heavier load (we use a free service from Travis, but we pay for system that runs Jenkins, so the rate limits on Jenkins is whatever we're prepared to pay, and a few more test runs won't make much difference). The reason we also run them on Travis is that the Jenkins logs are not public (a problem which we'll hopefully solve soon), so when something fails on Jenkins on a pull request by someone who isn't an Arm employee, an Arm employee needs to come along and post the results. It's not that big a deal for these tests since they don't fail very often.
@mpg IIRC you were the main proponent of having the depends tests on Travis. In hindsight, is this still useful? Should we restrict to the previously existing ones, or remove them altogether? Now that Travis has some non-x86 archictectures, I'd like to run some tests on at least arm64, and to keep the load roughly constant, something should go and the depends tests are the most obvious candidate.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree, considering we're restricted on Travis, I think testing on other architectures is more important than running the depends test on Travis. And hopefully soonish we'll get Jenkins to report results publicly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jdurkop So I think you should remove the following block from .travis.yml
:
- name: check compilation guards
script:
- tests/scripts/all.sh -k 'test_depends_*' 'build_key_exchanges'
with a commit message explaining that these have become too large for the free Travis instance, so they'll now only be run on Jenkins (possibly with a link Gilles's comment above).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mpg Thanks for the suggestion.
The pk_ec_test_vec() was incorrectly using MBEDTLS_MD_SHA1 for the parameter to mbedtls_pk_verify(). It should use MBEDTLS_MD_NONE since that parameter is ignored for this test case. Signed-off-by: John Durkop <john.durkop@fermatsoftware.com>
b82f9b5
to
d4efa8d
Compare
Moved the new component_test_depends_pkalgs_psa to after the component_test_depends_pkalgs test to be more consistent. Signed-off-by: John Durkop <john.durkop@fermatsoftware.com>
@gilles-peskine-arm Thanks for the additional review comments. I updated the history and fixed the test order in all.sh. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM apart from the Travis issue. Thanks @jdurkop!
With the increase in depends testing for PSA changes introduced here the Travis builds are now taking too long. The check for compilation guards will only be run on Jenkins now. See this comment for further details. Mbed-TLS#3585 (comment) Signed-off-by: John Durkop <john.durkop@fermatsoftware.com>
Description
This commit is fixing an issue when an undefined reference occurs when using MBEDTLS_USE_PSA_CRYPTO and removing ECDSA support. New guards added to pk_wrap.c and updates to fix unused function parameter warnings in pk_wrap.c and pk.c. Also created two new tests in all.sh to verify basic features are working when MBEDTLS_USE_PSA_CRYPTO is enabled.
Fixes #3294
Status
READY
Requires Backporting
NO
Migrations
NO
Steps to test or reproduce
Reproduce:
Enable MBEDTLS_USE_PSA_CRYPTO and remove ECDSA_C and build setup. It will show the undefined reference warning without these changes.
Test:
After these changes are applied the build will work properly.
Added new tests to all.sh to easily confirm basics with USE_PSA_CRYPTO are working properly: test_depends_pkalgs_psa and test_depends_curves_psa