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

Introduce SHA256/SHA512 interleave testing, HAVE_DSA; revised ERROR_OUT #7262

Merged
merged 1 commit into from
Mar 5, 2024

Conversation

gojimmypi
Copy link
Contributor

@gojimmypi gojimmypi commented Feb 20, 2024

Description

Note this description has been modified since originally submitted.

Background

While working on wolfSSL/wolfssh-examples#4 an error was encountered calculating the SSH signature with the SHA256 HW Acceleration enabled on the Espressif ESP32.

The root cause of the problem is the random.c has several functions such as HashGen() which may initiate their own local SHA256 hash even when SINGLE_THREADED is enabled. The current Espressif ESP32 hardware "in use" detection is currently a simple int InUse variable. This does not allow multiple, interleaved hashes to be computed separately with single-use hardware, such as that in an SSH signature and the contained hash within.

This is why the wolfSSH example was able to generate a SHA384 hardware-accelerated signature successfully.

Interleaving tests are going to be needed for future expansion of our Espressif library as newer chips actually do support interleaving in hardware.

Details

This PR adds interleaving calculations to the wolfCrypt test.c.

Code is enabled by default unless turned off with NO_WOLFSSL_SHA256_INTERLEAVE and/or NO_WOLFSSL_SHA512_INTERLEAVE

I've also added what I found was an interesting test: an exactly BLOCKSIZE long input test and a known output for SHA256 testing.

For exhaustive testing, I could not find a way to enable DSA testing, so I added HAVE_DSA to settings.h to be able to force it on.

Other minor changes such as adjusting the order of ERROR_OUT output to print an error first, then the system config, then a warning that the app has been halted. The prior steps looked nearly like the embedded app had otherwise rebooted. (the system config detail has grown fairly long). For the Espressif environment, a failed test is a bit more obvious now:

image

I originally had a breadcrumb macro for the ERROR_OUT, but changed it to WOLFSSL_ENTER("error_test"); (thanks @bandi13 for the excellent suggestions!)

While enabling more items for testing, there was a compile error in wolfSSL_DH_compute_key regarding a missing initialization. A fix for that is included here, too.

There was a minor problem with the SHA metrics with NO_WOLFSSL_ESP32_CRYPT_HASH, so that fix is also included.

To make am even more interesting and aggressive SHA test, I also suggest calling all of the other SHA tests during each respective test. For example: Call the SHA512 test between each init and final step of the SHA256 test.

Fixes zd# n/a

Testing

How did you test?

Tested only with Espressif hardware acceleration. This test is only moderately interesting for software calculations, but may be of high interest on other platforms with SHA256 accleration.

Checklist

  • added tests
  • updated/added doxygen
  • updated appropriate READMEs
  • Updated manual and documentation

@ejohnstown
Copy link
Contributor

retest this please

testVector test_sha[3];
testVector a, b, c, d;
testVector test_sha[4];
#ifndef NO_WOLFSSL_SHA256_INTERLEAVE
Copy link
Contributor

Choose a reason for hiding this comment

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

This NO_WOLFSSL_SHA256_INTERLEAVE macro is new and only applies to test.c correct? If so the name should include "TEST" in it. Also needs documented.
I believe the interleave avoids the copy, update, final scenario?

Copy link
Contributor Author

@gojimmypi gojimmypi Feb 21, 2024

Choose a reason for hiding this comment

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

This NO_WOLFSSL_SHA256_INTERLEAVE macro is new and only applies to test.c correct?

Yes it is new; No, not intended for only test.c. Although it is only included in test.c now, future versions of the Espressif library may support SHA interleaving (on SoC devices that support it). I envision this as being an optional feature. It will likely be valuable when testing performance between systems that have hardware-accelerated SHA interleaving enabled (a bit of overhead expected) vs no interleaving needed.

An example of needing the interleave is the SSH signature. Other applications may not need nor want the interleaving overhead needed for hardware interleaving.

I believe the interleave avoids the copy, update, final scenario?

Interesting question. No, not avoided. The interleave does perform the initialize, update, and final (then copy to output). The interesting thing is that when for example one SHA256 hash is in progress, what happens to a concurrent SHA256 in progress at the same time in a different object.

There's another type of copy that might be interesting to more exhaustively test as well: the copy of the wc_Sha256 object to another instance. I could address that in a separate PR (or here?) if desired.

Also needs documented.

Thanks for reminder. Added wolfSSL/documentation#120

int times = sizeof(test_sha) / sizeof(struct testVector), i;
active_wolfssl_test = "sha256_test";
Copy link
Contributor

Choose a reason for hiding this comment

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

If you are adding active_wolfssl_test please expand or provide a way to eliminate for small targets.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've removed the active_wolfssl_test after discussing with @bandi13. Same functionality, but much better.

See revised PR description.

@dgarske dgarske removed their assignment Feb 21, 2024
@gojimmypi gojimmypi changed the title Introduce NO_WOLFSSL_SHA256_INTERLEAVE Introduce SHA256/SHA512 interleave testing, HAVE_DSA; revised ERROR_OUT Feb 23, 2024
@gojimmypi gojimmypi force-pushed the PR-SHA-Interleave branch 4 times, most recently from 7b14c81 to 318814e Compare February 24, 2024 00:53
@gojimmypi
Copy link
Contributor Author

Jenkins retest this please

Copy link
Contributor

@dgarske dgarske 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 good/tests out. I like the extra "entering". Over to @douzzer

@dgarske dgarske assigned wolfSSL-Bot and unassigned dgarske Feb 28, 2024
@SparkiDev SparkiDev assigned SparkiDev and unassigned gojimmypi Mar 5, 2024
@SparkiDev SparkiDev merged commit ee39a8f into wolfSSL:master Mar 5, 2024
112 checks passed
This was referenced Jun 21, 2024
@gojimmypi gojimmypi deleted the PR-SHA-Interleave branch October 9, 2024 18:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants