Skip to content

Adjust the driver to the new PSA API #7

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

Merged
merged 10 commits into from
Sep 6, 2019

Conversation

AndrzejKurek
Copy link
Contributor

@AndrzejKurek AndrzejKurek commented Aug 7, 2019

Mostly API changes, with an addition of a check-slot function.
Changed the lifetime of the driver to fit the 0x00 - 0xFF range.
This PR is based on:

This PR is a base for ARMmbed/mbed-os-example-atecc608a#10.

@Patater Patater changed the title Adjust the driver to the new PSA API. Adjust the driver to the new PSA API Aug 15, 2019
Copy link
Contributor

@k-stachowiak k-stachowiak left a comment

Choose a reason for hiding this comment

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

I found one change that may potentially cause problems in the -Werror builds, and I have left a really small cosmetic remark.

Also, it would probably be easier to review the PR if the formatting changes were put in a separate commit.

@Patater
Copy link
Contributor

Patater commented Sep 5, 2019

Could we please rebase this to apply fixups and remove the merge commits?

Andrzej Kurek and others added 2 commits September 5, 2019 06:47
Mostly API changes, with an addition of a check-slot function.
Changed the lifetime of the driver to fit the 0x00 - 0xFF range.
@AndrzejKurek
Copy link
Contributor Author

@Patater done.

Andrzej Kurek added 3 commits September 6, 2019 12:07
Cryptoauthlib will perform the checks instead and return ATCA_BAD_PARAM
The PSA core should never pass NULL to this function
@AndrzejKurek
Copy link
Contributor Author

@Patater, @k-stachowiak - I believe I addressed all of your comments.

k-stachowiak
k-stachowiak previously approved these changes Sep 6, 2019
Copy link
Contributor

@k-stachowiak k-stachowiak left a comment

Choose a reason for hiding this comment

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

Thanks for addressing the review comments. LGTM.

Patater
Patater previously approved these changes Sep 6, 2019
Copy link
Contributor

@Patater Patater left a comment

Choose a reason for hiding this comment

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

What testing was done with the top commit in this branch (6febe1f)?

Copy link

@gilles-peskine-arm gilles-peskine-arm left a comment

Choose a reason for hiding this comment

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

There's only one blocker, which is the TODO comment. I know it's only a comment, but this isn't just test code, it's example code, so this is important.

Also, again because this is example code, we should implement that TODO ASAP.

return PSA_ERROR_INVALID_ARGUMENT;
}

if (type != PSA_KEY_TYPE_ECC_PUBLIC_KEY(PSA_ECC_CURVE_SECP256R1))
{
if (type != PSA_KEY_TYPE_ECC_PUBLIC_KEY(PSA_ECC_CURVE_SECP256R1)) {

Choose a reason for hiding this comment

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

Cosmetic: it would make more sense to check the type first, then check that the data length is correct for the sole supported type.

@AndrzejKurek
Copy link
Contributor Author

@gilles-peskine-arm Done. Please re-review.

@AndrzejKurek AndrzejKurek requested a review from Patater September 6, 2019 12:42
Copy link
Contributor

@Patater Patater left a comment

Choose a reason for hiding this comment

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

LGTM

@Patater Patater merged commit 7f66686 into ARMmbed:mbed-cryptoauthlib Sep 6, 2019
@AndrzejKurek AndrzejKurek deleted the key-registration-api branch September 9, 2019 05:58
rajkan01 pushed a commit to rajkan01/mbed-os-atecc608a that referenced this pull request Nov 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants