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

Psa cipher chacha20 multipart operation implementation #20788

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

Psa cipher chacha20 multipart operation implementation #20788

wants to merge 1 commit into from

Conversation

Wunderbaeumchen99817
Copy link
Contributor

Contribution description

Adds CHACHA20 multipart glue code

Testing procedure

TBD

Issues/PRs references

N/A

@github-actions github-actions bot added Platform: ARM Platform: This PR/issue effects ARM-based platforms Area: tests Area: tests and testing framework Area: build system Area: Build system Area: pkg Area: External package ports Area: cpu Area: CPU/MCU ports Area: sys Area: System labels Jul 16, 2024
mguetschow
mguetschow requested changes Jul 17, 2024
View reviewed changes
Copy link
Contributor

@mguetschow mguetschow left a comment

Choose a reason for hiding this comment

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

Thanks, looks promising already!

Mostly minor nits below, but one thing in particular surprised me: You allow the update function to be called exactly once, I don't think that's a restriction intended by PSA Crypto. To fix this, you might have to enlarge your context object to hold up to 63B from the preceding unfinished block. Not sure if there is a better way than that.

pkg/driver_cryptocell_310/include/psa_periph_chacha20_ctx.h Outdated Show resolved Hide resolved
pkg/driver_cryptocell_310/include/psa_periph_chacha20_ctx.h Outdated Show resolved Hide resolved
pkg/driver_cryptocell_310/include/psa_periph_chacha20_ctx.h Outdated Show resolved Hide resolved
pkg/driver_cryptocell_310/include/psa_periph_chacha20_ctx.h Outdated Show resolved Hide resolved
pkg/driver_cryptocell_310/psa_cryptocell_310/cipher_chacha20.c Outdated Show resolved Hide resolved
sys/psa_crypto/psa_crypto_algorithm_dispatch.c Outdated Show resolved Hide resolved
sys/psa_crypto/psa_crypto_location_dispatch.c Outdated Show resolved Hide resolved
tests/sys/crypto/tests-crypto-chacha20poly1305.c Outdated Show resolved Hide resolved
tests/sys/psa_crypto_cipher/example_cipher_chacha20.c Outdated Show resolved Hide resolved
tests/sys/psa_crypto_cipher/main.c Outdated Show resolved Hide resolved
mguetschow
mguetschow requested changes Jul 24, 2024
View reviewed changes
Copy link
Contributor

@mguetschow mguetschow left a comment

Choose a reason for hiding this comment

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

had a quick look at the tests, some suggestions to follow the coding convention

tests/sys/psa_crypto_cipher/example_cipher_chacha20.c Outdated Show resolved Hide resolved
tests/sys/psa_crypto_cipher/example_cipher_chacha20.c Outdated Show resolved Hide resolved
tests/sys/psa_crypto_cipher/example_cipher_chacha20.c Outdated Show resolved Hide resolved
tests/sys/psa_crypto_cipher/example_cipher_chacha20.c Outdated Show resolved Hide resolved
tests/sys/psa_crypto_cipher/example_cipher_chacha20.c Outdated Show resolved Hide resolved
tests/sys/psa_crypto_cipher/example_cipher_chacha20.c Outdated Show resolved Hide resolved
@mguetschow mguetschow added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Jul 24, 2024
@riot-ci
Copy link

riot-ci commented Jul 24, 2024 •
edited
Loading

Murdock results

✔️ PASSED

a061d2f sys/psa_crypto: chacha20 multipart glue code

Success Failures Total Runtime
10195 0 10196 17m:45s

Artifacts

mguetschow
mguetschow requested changes Jul 24, 2024
View reviewed changes
Copy link
Contributor

@mguetschow mguetschow left a comment

Choose a reason for hiding this comment

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

Could you double-check the whole PR again to follow the coding convention, especially regarding if-statements and comments?

pkg/driver_cryptocell_310/psa_cryptocell_310/cipher_chacha20.c Outdated Show resolved Hide resolved
pkg/driver_cryptocell_310/psa_cryptocell_310/cipher_chacha20.c Outdated Show resolved Hide resolved
pkg/driver_cryptocell_310/psa_cryptocell_310/cipher_chacha20.c Outdated Show resolved Hide resolved
pkg/driver_cryptocell_310/psa_cryptocell_310/cipher_chacha20.c Show resolved Hide resolved
pkg/driver_cryptocell_310/psa_cryptocell_310/cipher_chacha20.c Outdated Show resolved Hide resolved
sys/crypto/psa_riot_cipher/chacha20.c Outdated Show resolved Hide resolved
sys/include/crypto/psa/riot_ciphers.h Outdated Show resolved Hide resolved
mguetschow
mguetschow requested changes Jul 29, 2024
View reviewed changes
pkg/driver_cryptocell_310/include/psa_periph_chacha20_ctx.h Outdated Show resolved Hide resolved
sys/crypto/psa_riot_cipher/chacha20.c Show resolved Hide resolved
sys/crypto/psa_riot_cipher/chacha20.c Outdated Show resolved Hide resolved
sys/crypto/psa_riot_cipher/chacha20.c Outdated Show resolved Hide resolved
sys/psa_crypto/include/psa_ciphers.h Show resolved Hide resolved
@mguetschow
Copy link
Contributor

When you've handled the last outstanding comment, please go ahead and squash directly for a final round of review.

@Wunderbaeumchen99817 Wunderbaeumchen99817 force-pushed the psa-cipher-chacha20-multipart-operation-implementation branch from b5f330f to a061d2f Compare July 30, 2024 16:54
@darthdrannel
Copy link

After merge of PR 20720, the sys/psa_crypto/doc.txt should be updated to include the added context in this PR

@mguetschow mguetschow requested a review from Einhornhool August 20, 2024 09:02
@Lukas-Luger
Copy link

Tests on native and feather-nrf52840-sense checked out OK.

@Lukas-Luger
Copy link

After merge of PR 20720, the sys/psa_crypto/doc.txt should be updated to include the added context in this PR

Can you please elaborate what context should be added to the doc?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mguetschow mguetschow mguetschow requested changes

@darthdrannel darthdrannel darthdrannel left review comments

@Einhornhool Einhornhool Awaiting requested review from Einhornhool

@leandrolanzieri leandrolanzieri Awaiting requested review from leandrolanzieri leandrolanzieri will be requested when the pull request is marked ready for review leandrolanzieri is a code owner

@aabadie aabadie Awaiting requested review from aabadie aabadie will be requested when the pull request is marked ready for review aabadie is a code owner

@MichelRottleuthner MichelRottleuthner Awaiting requested review from MichelRottleuthner MichelRottleuthner will be requested when the pull request is marked ready for review MichelRottleuthner is a code owner

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
Area: build system Area: Build system Area: cpu Area: CPU/MCU ports Area: pkg Area: External package ports Area: sys Area: System Area: tests Area: tests and testing framework CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Platform: ARM Platform: This PR/issue effects ARM-based platforms
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@Wunderbaeumchen99817 @riot-ci @mguetschow @darthdrannel @Lukas-Luger