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

Jump to bottom

Added support for AES-ECB to the PSA Crypto implementation #3480

Merged
merged 5 commits into from
Sep 14, 2020
Merged

Added support for AES-ECB to the PSA Crypto implementation #3480

merged 5 commits into from
Sep 14, 2020

Conversation

stevew817
Copy link
Contributor

@stevew817 stevew817 commented Jul 6, 2020 •
edited
Loading

Description

This PR implements support for PSA_ALG_ECB_NO_PADDING as defined in the PSA Crypto API spec v1.0.0. Feature was requested in #3277.

Status

READY

Requires Backporting

NO

Migrations

NO

Todos

  • Tests
  • Documentation
  • Changelog updated

Steps to test or reproduce

Tested through the PSA Crypto test suite.

@stevew817
Added support for AES-ECB to the PSA Crypto implementation
ed3c9ec 
PSA_ALG_ECB_NO_PADDING came in to the PSA Crypto API spec v1.0.0, but
was not implemented yet in the mbed TLS implementation.

Signed-off-by: Steven Cooreman <[email protected]>
@stevew817
Copy link
Contributor Author

Does this need a changelog entry?

@gilles-peskine-arm
Copy link
Contributor

I think yes, this counts as a new feature so it should have a changelog entry.

@gilles-peskine-arm gilles-peskine-arm added component-crypto Crypto primitives and low-level interfaces enhancement PSA compliance labels Jul 6, 2020
@yanesca yanesca requested a review from bensze01 July 9, 2020 09:20
bensze01
bensze01 previously approved these changes Jul 27, 2020
View reviewed changes
Copy link
Contributor

@bensze01 bensze01 left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Only The MbedOS Tests are failing, LGTM

hanno-becker
hanno-becker reviewed Aug 14, 2020
View reviewed changes
library/psa_crypto.c Outdated Show resolved Hide resolved
hanno-becker
hanno-becker reviewed Aug 14, 2020
View reviewed changes
library/psa_crypto.c Outdated Show resolved Hide resolved
hanno-becker
hanno-becker reviewed Aug 14, 2020
View reviewed changes
library/psa_crypto.c Outdated Show resolved Hide resolved
hanno-becker
hanno-becker reviewed Aug 14, 2020
View reviewed changes
library/psa_crypto.c Outdated Show resolved Hide resolved
hanno-becker
hanno-becker reviewed Aug 14, 2020
View reviewed changes
tests/suites/test_suite_psa_crypto.function Outdated
Comment on lines 3418 to 3419
{
PSA_ASSERT( psa_cipher_set_iv( &operation, iv->x, iv->len ) );
Copy link

Choose a reason for hiding this comment

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

Suggested change
{
PSA_ASSERT( psa_cipher_set_iv( &operation, iv->x, iv->len ) );
{
PSA_ASSERT( psa_cipher_set_iv( &operation, iv->x, iv->len ) );

hanno-becker
hanno-becker reviewed Aug 14, 2020
View reviewed changes
tests/suites/test_suite_psa_crypto.function Outdated
Comment on lines 3490 to 3491
if( iv->len > 0 ) {
PSA_ASSERT( psa_cipher_set_iv( &operation, iv->x, iv->len ) );
Copy link

Choose a reason for hiding this comment

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

Suggested change
if( iv->len > 0 ) {
PSA_ASSERT( psa_cipher_set_iv( &operation, iv->x, iv->len ) );
if( iv->len > 0 )
{
PSA_ASSERT( psa_cipher_set_iv( &operation, iv->x, iv->len ) );

Copy link

Choose a reason for hiding this comment

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

Minor: The brace { is still misplaced here and at other occasions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done now, sorry for the oversight.

hanno-becker
hanno-becker reviewed Aug 14, 2020
View reviewed changes
tests/suites/test_suite_psa_crypto.function Outdated
Comment on lines 3559 to 3560
if( iv->len > 0 ) {
PSA_ASSERT( psa_cipher_set_iv( &operation, iv->x, iv->len ) );
Copy link

Choose a reason for hiding this comment

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

Suggested change
if( iv->len > 0 ) {
PSA_ASSERT( psa_cipher_set_iv( &operation, iv->x, iv->len ) );
if( iv->len > 0 )
{
PSA_ASSERT( psa_cipher_set_iv( &operation, iv->x, iv->len ) );

Copy link

Choose a reason for hiding this comment

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

Same here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done now, sorry for the oversight.

hanno-becker
hanno-becker reviewed Aug 14, 2020
View reviewed changes
tests/suites/test_suite_psa_crypto.function Outdated
Comment on lines 3628 to 3629
if( alg != PSA_ALG_ECB_NO_PADDING ) {
PSA_ASSERT( psa_cipher_generate_iv( &operation1,
Copy link

Choose a reason for hiding this comment

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

Suggested change
if( alg != PSA_ALG_ECB_NO_PADDING ) {
PSA_ASSERT( psa_cipher_generate_iv( &operation1,
if( alg != PSA_ALG_ECB_NO_PADDING )
{
PSA_ASSERT( psa_cipher_generate_iv( &operation1,

Copy link

Choose a reason for hiding this comment

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

Same here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done now, sorry for the oversight.

hanno-becker
hanno-becker reviewed Aug 14, 2020
View reviewed changes
tests/suites/test_suite_psa_crypto.function Outdated
Comment on lines 3652 to 3653
if( iv_length > 0 ) {
PSA_ASSERT( psa_cipher_set_iv( &operation2,
Copy link

Choose a reason for hiding this comment

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

Suggested change
if( iv_length > 0 ) {
PSA_ASSERT( psa_cipher_set_iv( &operation2,
if( iv_length > 0 )
{
PSA_ASSERT( psa_cipher_set_iv( &operation2,

Copy link

Choose a reason for hiding this comment

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

Same here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done now, sorry for the oversight.

hanno-becker
hanno-becker reviewed Aug 14, 2020
View reviewed changes
tests/suites/test_suite_psa_crypto.function Outdated
Comment on lines 3718 to 3719
if( alg != PSA_ALG_ECB_NO_PADDING ) {
PSA_ASSERT( psa_cipher_generate_iv( &operation1,
Copy link

Choose a reason for hiding this comment

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

Suggested change
if( alg != PSA_ALG_ECB_NO_PADDING ) {
PSA_ASSERT( psa_cipher_generate_iv( &operation1,
if( alg != PSA_ALG_ECB_NO_PADDING )
{
PSA_ASSERT( psa_cipher_generate_iv( &operation1,

Copy link

Choose a reason for hiding this comment

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

Same here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done now, sorry for the oversight.

hanno-becker
hanno-becker reviewed Aug 14, 2020
View reviewed changes
tests/suites/test_suite_psa_crypto.function Outdated
Comment on lines 3753 to 3754
if( iv_length > 0 ) {
PSA_ASSERT( psa_cipher_set_iv( &operation2,
Copy link

Choose a reason for hiding this comment

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

Suggested change
if( iv_length > 0 ) {
PSA_ASSERT( psa_cipher_set_iv( &operation2,
if( iv_length > 0 )
{
PSA_ASSERT( psa_cipher_set_iv( &operation2,

Copy link

Choose a reason for hiding this comment

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

Same here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done now, sorry for the oversight.

hanno-becker
hanno-becker suggested changes Aug 14, 2020
View reviewed changes
Copy link

@hanno-becker hanno-becker left a 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, thanks @stevew817 for your work!

There's just a bunch of trivial style issues remaining as indicated in the comments.

@ronald-cron-arm ronald-cron-arm added the needs-review Every commit must be reviewed by at least two team members, label Sep 2, 2020
@daverodgman daverodgman linked an issue Sep 2, 2020 that may be closed by this pull request
Expose ECB through the PSA API #3277
Closed
@daverodgman daverodgman added this to the Backlog milestone Sep 2, 2020
gilles-peskine-arm
gilles-peskine-arm requested changes Sep 7, 2020
View reviewed changes
library/psa_crypto.c Outdated Show resolved Hide resolved
library/psa_crypto.c Show resolved Hide resolved
tests/suites/test_suite_psa_crypto.data Outdated Show resolved Hide resolved
tests/suites/test_suite_psa_crypto.data Outdated Show resolved Hide resolved
gilles-peskine-arm
gilles-peskine-arm approved these changes Sep 8, 2020
View reviewed changes
library/psa_crypto.c

status = PSA_SUCCESS;

exit:
Copy link
Contributor

Choose a reason for hiding this comment

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

Since there's no cleanup, it would be more readable to use return( status ) instead of goto exit (and return( whatever ); instead of status = whatever; goto exit;). I don't consider it a blocker.

@stevew817
Copy link
Contributor Author

@hanno-arm I'd appreciate it if you could give your thumbs up now that all your requested changes have been incorporated. This PR has been open for over two months already.

hanno-becker
hanno-becker approved these changes Sep 14, 2020
View reviewed changes
Copy link

@hanno-becker hanno-becker left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Apologies for the delay, @stevew817, and thank you for making the requested changes. LGTM

@hanno-becker hanno-becker removed the needs-review Every commit must be reviewed by at least two team members, label Sep 14, 2020
@bensze01
Copy link
Contributor

bensze01 commented Sep 14, 2020 •
edited
Loading

All the mbedTLS tests have passed (at the time of writing, only the MbedOS tests are still running).

@bensze01 bensze01 added the approved Design and code approved - may be waiting for CI or backports label Sep 14, 2020
@gilles-peskine-arm gilles-peskine-arm merged commit b1d3f27 into Mbed-TLS:development Sep 14, 2020
@stevew817 stevew817 mentioned this pull request Sep 14, 2020
Merged
3 tasks
@bensze01 bensze01 modified the milestones: September 2020 Sprint, PSA Crypto: Q2-Q3 Implement missing v1.0 spec functionality Jul 28, 2021
@gilles-peskine-arm gilles-peskine-arm mentioned this pull request Sep 23, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gilles-peskine-arm gilles-peskine-arm gilles-peskine-arm approved these changes

@hanno-becker hanno-becker hanno-becker approved these changes

@bensze01 bensze01 bensze01 left review comments

Assignees

@bensze01 bensze01

Labels
approved Design and code approved - may be waiting for CI or backports component-crypto Crypto primitives and low-level interfaces enhancement
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Expose ECB through the PSA API
7 participants
@stevew817 @gilles-peskine-arm @bensze01 @hanno-becker @daverodgman @DarshpreetSabharwal @ronald-cron-arm