Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Added support for AES-ECB to the PSA Crypto implementation #3480
Added support for AES-ECB to the PSA Crypto implementation #3480
Changes from 1 commit
ed3c9ec
2a48b53
a6033e9
ffecb7b
177deba
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add a few more test cases (for both encrypt and decrypt): 0 bytes, 32 bytes, a non-multiple of 16.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add at least a 1-block 3DES test case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
3DES isn't in scope for this PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR makes Mbed TLS recognize
PSA_ALG_ECB_NO_PADDING
, so it should test the algorithm with at least a selection of block ciphers. 3DES and other block ciphers should either work correctly or returnNOT_SUPPORTED
. I requested a test for 3DES and not for e.g. CAMELLIA because CAMELLIA is very likely to work if AES does, whereas DES and 3DES could be incorrect due to a misuse of a hard-coded 16 instead of the block size somewhere.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add a test case where the input isn't split on a block boundary (e.g. 1+15). And please add a bad test case where the total input isn't a whole number of blocks. All for both encrypt and decrypt.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There weren't any negative test cases being done with _multipart tests. Is it really up to me to introduce that here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A prior test gap is not a justification for expanding the test gap in general. But ok, let's defer negative testing for multipart operations since it's a preexisting lack of test code. Do please add test with input that isn't split on a block boundary, which was already done for CBC.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure (push incoming for the non-block-multiple operations).