ICP encryption tests #17089
Closed
ICP encryption tests #17089
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
13 tasks
|
for what it's worth, I'd love to help with the refactor of the ICP code. I (now) at least have some component level understanding, and having some tests there would allow us to refactor a bit more fearlessly. This takes us in the right direction, methinks |
tonyhutter
reviewed
Feb 24, 2025
|
Looks like it's passing on the runners 👍 |
This commit adds tests that ensure that the ICP crypto_encrypt() and crypto_decrypt() produce the correct results for all implementations available on this platform. The actual ZTS scripts are simple drivers for the crypto_test program in it's "correctness" mode. This mode takes a file full of test vectors (inputs and expected outputs), runs them, and checks that the results are expected. It will run the tests for each implementation of the algorithm provided by the ICP. The test vectors are taken from Project Wycheproof, which provides a huge number of tests, including exercising many edge cases and common implementation mistakes. These tests are provided are JSON files, so a program is included here to convert them into a simpler line-based format for crypto_test to consume. crypto_test also has a "performance" mode, which will run simple benchmarks against all implementations provded by the ICP and output them for comparison. This is not used by ZTS, but is available to assist with development of new implementations of the underlying primitives. Thanks-to: Joel Low <joel@joelsplace.sg> Sponsored-by: https://despairlabs.com/sponsor/ Signed-off-by: Rob Norris <robn@despairlabs.com>
IVs != 96 bits get hashed with GHASH to bring them to 96 bits. Any call to GHASH will mix the ghash state in gcm_ghash. This is expected to be zero at first use in an encrypt or decrypt operation, so it needs to be zeroed after using GHASH in setup. gcm_init() does this, but gcm_avx_init() zeroed it before setup, not after, resulting in incorrect encrypt/decrypt results when using AVX GCM with an IV != 96 bits. OpenZFS _always_ uses a 96 bit IV (ZIO_DATA_IV_LEN) so this will never have been hit in any real-world use, which is extremely fortunate, as we would have incorrectly-encrypted data on-disk. Still, as long as we have this code here we should make sure it's correct. Thanks-to: Joel Low <joel@joelsplace.sg> Sponsored-by: https://despairlabs.com/sponsor/ Signed-off-by: Rob Norris <robn@despairlabs.com>
robn
force-pushed
the
icp-crypto-test
branch
from
4a86451 to
550a246
Compare
tonyhutter
approved these changes
Feb 25, 2025
AttilaFueloep
approved these changes
Feb 25, 2025
|
Thanks everyone! I hope this will help make it much easier to improve our encryption code in the future. <3 |
13 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Motivation and Context
I wanted to do a good review of #17058, but really had no idea how to without some way to compare the output with what we already have.
And then I went mad and ended up writing a whole correctness and performance test driver for the ICP. And then it found a real (though inconsequential) bug. Good times all round!
Thanks @lowjoel for motivation and rubber-ducking 👍
Description
The main commit adds tests that ensure that the ICP
crypto_encrypt()andcrypto_decrypt()produce the correct results for all implementations available on this platform.The actual ZTS scripts are simple drivers for the
crypto_testprogram in it's "correctness" mode. This mode takes a file full of test vectors (inputs and expected outputs), runs them, and checks that the results are expected. It will run the tests for each implementation of the algorithm provided by the ICP.The test vectors are taken from Project Wycheproof, which provides a huge number of tests, including exercising many edge cases and common implementation mistakes. These tests are provided are JSON files, so a program is included here to convert them into a simpler line-based format for
crypto_testto consume.crypto_testalso has a "performance" mode, which will run simple benchmarks against all implementations provded by the ICP and output them for comparison. This is not used by ZTS, but is available to assist with development of new implementations of the underlying primitives.AVX GCM bug fix
While get it working we found that the AVX GCM was producting the wrong results for IVs != 96 bits (12 bytes). In this case AES-GCM does a couple of GHASH rounds on the provided IV to bring it to 96 bits. The AVX setup did this, but didn't clear the GHASH state before finishing init, so it had leftover state from the IV setup.
Fortunately, OpenZFS always uses 96-bit IVs, so we've avoided this. Just points out more code that we carry but don't use, but that's for another PR.
How Has This Been Tested?
It's all tests!
The ZTS additions are simple wrappers around
crypto_test -c <testfile>. Those have been run successfully on Linux and FreeBSD. (note that FreeBSD is still important here, as the ICP is used for userspace crypto).In "correctness" mode, it runs all the test vectors for each available implementation:
Because there's currently no way to find out what implementations the ICP has available, and I didn't want to add more to this than necessary, we just try them all. If the platform doesn't support them, it's at least nice about it:
If any of the test runs fail, the program will return non-zero, so it wires up to ZTS nicely.
For the performance mode, it'll do the usual thing of run all implementations on various input sizes and show a table:
More benchmarks with the hardware I available here: https://gist.github.com/robn/994f1618b4994b2f90dc8c7838fa5654. Some nice opportunities there!
Types of changes
Checklist:
Signed-off-by.