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.

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

PSA Multipart AEAD CCM Internal implementation and tests. #5047

Merged

Conversation

paul-elliott-arm
Copy link
Member

@paul-elliott-arm paul-elliott-arm commented Oct 7, 2021

Description

This PR adds CCM to the algorithms supported by PSA multipart AEAD. It adds an internal implementation, and a suite of tests for it.

This completes one of the tasks of #3721, however there may be follow up work with regards to Nonce sizes.

Status

READY

Requires Backporting

No. The changes to the underlying cipher API to support multipart are only on this branch. Multipart AEAD was also not backported.

Migrations

No.

Todos

  • Tests
  • Changelog updated

Steps to test

test_suite_psa_crypto should run clean, and all PSA tests should pass.

Signed-off-by: Paul Elliott <paul.elliott@arm.com>
Encrypt / Decrypt with expected result.

Signed-off-by: Paul Elliott <paul.elliott@arm.com>
Known failures, concentrating on verify (bad signature etc.)

Signed-off-by: Paul Elliott <paul.elliott@arm.com>
Signed-off-by: Paul Elliott <paul.elliott@arm.com>
Signed-off-by: Paul Elliott <paul.elliott@arm.com>
Signed-off-by: Paul Elliott <paul.elliott@arm.com>
Signed-off-by: Paul Elliott <paul.elliott@arm.com>
Signed-off-by: Paul Elliott <paul.elliott@arm.com>
Signed-off-by: Paul Elliott <paul.elliott@arm.com>
@paul-elliott-arm paul-elliott-arm self-assigned this Oct 7, 2021
@paul-elliott-arm paul-elliott-arm added Arm Contribution component-psa PSA keystore/dispatch layer (storage, drivers, …) needs-ci Needs to pass CI tests needs-review Every commit must be reviewed by at least two team members, needs-reviewer This PR needs someone to pick it up for review size-m Estimated task size: medium (~1w) labels Oct 7, 2021
@gilles-peskine-arm gilles-peskine-arm removed needs-ci Needs to pass CI tests needs: changelog labels Oct 8, 2021
@mprse mprse self-requested a review October 11, 2021 10:22
library/psa_crypto.c Show resolved Hide resolved
@@ -446,11 +444,22 @@ psa_status_t mbedtls_psa_aead_set_lengths(
size_t ad_length,
size_t plaintext_length )
{
/* Nothing here yet, work is currently done in PSA Core, however support
* for CCM will require this function. */

Copy link
Contributor

Choose a reason for hiding this comment

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

Redundant empty line

library/psa_crypto_aead.c Show resolved Hide resolved
library/psa_crypto_aead.c Show resolved Hide resolved
@mstarzyk-mobica mstarzyk-mobica removed the needs-reviewer This PR needs someone to pick it up for review label Oct 22, 2021
Signed-off-by: Paul Elliott <paul.elliott@arm.com>
@paul-elliott-arm
Copy link
Member Author

CI Fail is the previous (and now fixed) ssl issue, unrelated to this.

Copy link
Contributor

@mprse mprse left a comment

Choose a reason for hiding this comment

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

Code changes looks good, but CI is still failing.
Will approve when CI is green.

@paul-elliott-arm
Copy link
Member Author

Code changes looks good, but CI is still failing. Will approve when CI is green.

The CI failures are due to unrelated known CI issues (timeout issues in the SSL Tests) - hence the HEAD failing, but the Merge tests generally not. The only way I could potentially resolve this would be to force update the branch, as some fixes have been merged since this PR was made.

Copy link
Contributor

@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.

I'm only requesting some minor documentation changes because the issues I found are largely preexisting. I've filed #5285, #5286 and #5287 as follow-ups to be done ASAP (but it's ok to release 3.1 without those follow-ups).

library/psa_crypto.c Show resolved Hide resolved
tests/suites/test_suite_psa_crypto.function Show resolved Hide resolved
tests/suites/test_suite_psa_crypto.data Show resolved Hide resolved
tests/suites/test_suite_psa_crypto.data Outdated Show resolved Hide resolved
@gilles-peskine-arm gilles-peskine-arm added needs-work and removed needs-review Every commit must be reviewed by at least two team members, Arm Contribution labels Dec 8, 2021
Signed-off-by: Paul Elliott <paul.elliott@arm.com>
Signed-off-by: Paul Elliott <paul.elliott@arm.com>
@gilles-peskine-arm gilles-peskine-arm dismissed mstarzyk-mobica’s stale review December 9, 2021 13:48

The requested changes have been addressed and approved by @mprse.

@gilles-peskine-arm gilles-peskine-arm added approved Design and code approved - may be waiting for CI or backports and removed needs-work labels Dec 9, 2021
@gilles-peskine-arm
Copy link
Contributor

The CI failures in pr-head are a known unrelated issue in TLS tests, now resolved. pr-merge passed. So CI is good.

@gilles-peskine-arm gilles-peskine-arm merged commit d5b2a59 into Mbed-TLS:development Dec 9, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Design and code approved - may be waiting for CI or backports component-psa PSA keystore/dispatch layer (storage, drivers, …) size-m Estimated task size: medium (~1w)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement multipart CCM for PSA
4 participants