Support sha2-224, 384 and 512 #144
Conversation
|
sha2-224 depends on PR#9070 |
|
Have to look at `sha224 test failure" |
|
Fixed |
|
Can you pull to the latest wolfHSM? This added -Wextra which is going to cause you to add (void)parameter lines to your functions to avoid warnings. See PR#141 |
There was a problem hiding this comment.
Pull Request Overview
This PR adds support for SHA2-224, SHA2-384, and SHA2-512 hash algorithms to the wolfHSM project. The implementation follows the existing SHA2-256 pattern, providing both regular and DMA variants for each algorithm.
Key changes include:
- Addition of SHA224, SHA384, and SHA512 context structures to the server crypto context
- Implementation of message structures and translation functions for the new hash algorithms
- Client-side crypto callback support and API functions for all three algorithms
- Comprehensive test coverage with test vectors for each algorithm
- Benchmark module implementations for performance testing
Reviewed Changes
Copilot reviewed 16 out of 16 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
| wolfhsm/wh_server.h | Added SHA224, SHA384, and SHA512 context arrays to server crypto union |
| wolfhsm/wh_message_crypto.h | Added message structures and function declarations for all three algorithms |
| wolfhsm/wh_client_crypto.h | Added client API function declarations with documentation |
| test/wh_test_crypto.c | Added comprehensive test functions with known test vectors |
| src/wh_server_crypto.c | Implemented server-side handlers for regular and DMA operations |
| src/wh_message_crypto.c | Implemented message translation functions |
| src/wh_client_cryptocb.c | Added crypto callback support for all three algorithms |
| src/wh_client_crypto.c | Implemented client-side API functions and helper functions |
| Multiple config files | Added commented configuration options for the new algorithms |
| Multiple build files | Added SHA512 source file to build configurations |
| benchmark/ files | Added benchmark support for all three new algorithms |
bigbrett
left a comment
bigbrett
left a comment
There was a problem hiding this comment.
@miyazakh looks good, largely copy-paste as expected. Could you please fix the items that copilot found? They all appear to be real.
Side note: at some point in the future I'd like to see if we could unify all SHA2 handling code into a generic implementation that can handle the different lengths. This is a pretty absurd amount of boiler-plate and copy-paste (not your fault, just how it works right now). If there is a bug we need to fix then we would need to fix it in all 3 implementations. Something to keep in our mind going forward.
|
Hi @bigbrett
I totally agree to you. I thought the way at the beginning. But, this is almost fist time for me to add codes to As the saying goes, |
|
@miyazakh I totally understand and wouldn't expect any differently, I was just musing to myself. Lets get this merged first and then later we can consider refactoring it. |
|
Ok. |
billphipps
left a comment
billphipps
left a comment
There was a problem hiding this comment.
We need to remove the server crypto context storage of the different SHA states. This was missed during the rework of the SHA256. Everything else is typos or optional. Looks really good!
|
Looks like |
bigbrett
left a comment
bigbrett
left a comment
There was a problem hiding this comment.
This looks good, I'd like you to also unify the DMA responses in the same way, like you pointed out. You can do it in this PR or in a subsequent one, up to you.
|
Unified Sha2 DMA Request/Response. Fixed review comments for final review. |
bigbrett
left a comment
bigbrett
left a comment
There was a problem hiding this comment.
@miyazakh Looks great. Please pull the latest main, rebase this branch on top of it, then run git-clang-format main to format only your changes. Then I can merge.
I was going to do it for you and push to your branch, but since you aren't rebased on the latest, it would mean I would need to rebase for you then force push which I don't want to do.
bigbrett
left a comment
bigbrett
left a comment
There was a problem hiding this comment.
@billphipps LGTM over to you for final overview and merge
add missing Dma handling
enable sha224, sha384 and sh512 as default enable sha224, sha384 and sha512 at tcp server
addressed review comment
billphipps
left a comment
billphipps
left a comment
There was a problem hiding this comment.
Looks good! Thank you!
* add sha2-224, 384 and 512 * addressed code review add missing Dma handling * fix Sha512 tests enable sha224, sha384 and sh512 as default enable sha224, sha384 and sha512 at tcp server * addressed code review comments * unify Sha2 Dma Request/Response addressed review comment * run clang-format
* add sha2-224, 384 and 512 * addressed code review add missing Dma handling * fix Sha512 tests enable sha224, sha384 and sh512 as default enable sha224, sha384 and sha512 at tcp server * addressed code review comments * unify Sha2 Dma Request/Response addressed review comment * run clang-format
4 participants
Add sha2-224, sha2-384 and sha2-512
#137