mbedtls icon indicating copy to clipboard operation
mbedtls copied to clipboard

Published 20 hours ago • verified publisher icon Mbed-TLS

Driver hashes rsa v21

Open mpg opened this issue 3 years ago • 4 comments

Implement #6098

Gatekeeping notes: no ChangeLog entry (part of a series, see #6146), no backport (new feature).

This PR also constitutes an example of how to adapt crypto modules (not governed by MBEDTLS_USE_PSA_CRYPTO) so that they can work in builds with driver-only hashes and no MD, by using PSA as a fall-back when MD is not present, to ensure backwards compatibility.

mpg avatar Jul 27 '22 08:07 mpg

I finished the first round of my review and it seems that things look good :)

AndrzejKurek avatar Jul 28 '22 10:07 AndrzejKurek

I'm unassigning myself and labeling the PR as "needs: adoption" as I'll be away from now until next end of August.

mpg avatar Jul 28 '22 11:07 mpg

Addressed review comments from @AndrzejKurek .

mprse avatar Jul 29 '22 10:07 mprse

This PR still needs additional reviewer as I adopted the PR, so I think in this case I shouldn't review it. We need it for further work on https://github.com/orgs/Mbed-TLS/projects/1#column-18883163 tasks.

mprse avatar Aug 01 '22 10:08 mprse

I couldn't rebase and force push, because this PR comes from @mpg fork repository. I was able to resolve conflicts via github. Let's see if CI passes and review again.

mprse avatar Aug 10 '22 09:08 mprse

I'm not sure if we're fine with such merge commits; Wouldn't it be cleaner to create a new PR with cherry-picked commits and fixed history? Apart from that - looks good to me.

AndrzejKurek avatar Aug 10 '22 13:08 AndrzejKurek

We have here entire PR history, so this merge commit maybe is not so bad. Let's wait for the CI.

mprse avatar Aug 10 '22 13:08 mprse

We generally try to avoid merge commits in pull requests, but they aren't forbidden. If the PR already has had significant review activity, and rebasing is not easy, a merge can be the better solution. At a glance, in this case, a merge seems like a good solution.

I do have a request however: please document the merge conflict resolution in the commit message of the merge.

gilles-peskine-arm avatar Aug 10 '22 15:08 gilles-peskine-arm

I rebased the PR on top of development as we normally do. PR was already reviewed and accepted by both reviewers, so this should not be a problem. Previously I wasn't able to do a force push probably because of network connection problem.

EDIT: Resolved conflicts: In file tests/suites/test_suite_pk.data removed radix test arguments as they were removed form pk_rsa_verify_ext_test_vec test function.

mprse avatar Aug 11 '22 10:08 mprse