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

Sha256 support #62

Merged
merged 4 commits into from
Aug 10, 2024
Merged

Sha256 support #62

merged 4 commits into from
Aug 10, 2024

Conversation

bigbrett
Copy link
Contributor

@bigbrett bigbrett commented Aug 9, 2024

Adds non-DMA sha256 support to wolfHSM crypto. Intermediate SHA state is held by the client and passed back and forth on each SHA256 API call. Passes tests and wolfCrypt tests

Note: The message in wh_packet.h are fixed size (with no padding needed the way they are implemented), and so I didn't use the "trailing implicit buffer" scheme that the other messages use, as it adds some more complicated array indexing logic since there are two optional buffers that would need to be passed back and forth. This results in a few extra unused bytes being sent on the first update message (unused resume hash state), and potentially on the finalize message (remainder of the SHA block not used for data).

I can go back and modify this to use the implicit buffers and variable length data if needed, but the code is a little simpler this way so figured I would see what you though first.

@bigbrett bigbrett requested review from billphipps and jpbland1 August 9, 2024 17:24
billphipps
billphipps previously approved these changes Aug 9, 2024
Copy link
Contributor

@billphipps billphipps 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! I had a question on the need for the resume flag (and sha256->flags then). If you think its as simple as it can be, then I'll merge.

src/wh_server_crypto.c Outdated Show resolved Hide resolved
src/wh_client_cryptocb.c Outdated Show resolved Hide resolved
/* Length of the last input block of data. Only valid if isLastBlock=1 */
uint32_t lastBlockLen;
/* Full sha256 input block to hash */
uint8_t inBlock[64]; /* TODO (BRN) WC_SHA256_BLOCK_SIZE */
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not use the WC_SHA256_BLOCK_SIZE? Same of other todos

Copy link
Contributor Author

Choose a reason for hiding this comment

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

because then we need to pull wolfCrypt headers into this file, which will mean we need to macro gate a ton of stuff AND it will break our padding test, since it will check padding of all the wolfCrypt structs in the headers we pull in :( I can just use a local macro for now until we figure out a better way to do it

Copy link
Contributor

Choose a reason for hiding this comment

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

Understood. Recommend add additional #define's in here if you want to keep these sizes statically defined.

src/wh_server_crypto.c Show resolved Hide resolved
src/wh_client_cryptocb.c Outdated Show resolved Hide resolved
Copy link
Contributor

@billphipps billphipps left a comment

Choose a reason for hiding this comment

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

Excellent simplification. We can clean up the fixed sizes later.

/* Length of the last input block of data. Only valid if isLastBlock=1 */
uint32_t lastBlockLen;
/* Full sha256 input block to hash */
uint8_t inBlock[64]; /* TODO (BRN) WC_SHA256_BLOCK_SIZE */
Copy link
Contributor

Choose a reason for hiding this comment

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

Understood. Recommend add additional #define's in here if you want to keep these sizes statically defined.

@billphipps billphipps merged commit 7b3b4ec into wolfSSL:main Aug 10, 2024
2 checks passed
jefferyq2 pushed a commit to jefferyq2/wolfHSM that referenced this pull request Nov 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants