-
Notifications
You must be signed in to change notification settings - Fork 72
Allow software operations in PKCS11 provider #241
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
Conversation
8bd617e
to
1f736f4
Compare
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 looks pretty good to me overall!!
@@ -22,5 +22,6 @@ provider_type = "Pkcs11" | |||
key_info_manager = "on-disk-manager" | |||
library_path = "/usr/local/lib/softhsm/libsofthsm2.so" | |||
user_pin = "123456" | |||
software_public_operations = true |
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.
Do you think we should keep it that way forever? It would be nice to check that public key operations executed on hardware are working as well if that is the default.
I think it's fine to have the software_public_operations
only checked manually for now.
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.
Yeah, sorry, I'll change that back and add some config
tests that check the software implementation
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.
I've added a couple of tests for the software verification. Do you think we should run all tests against both? As in, do something in ci.sh
instead of just config tests?
if crate::utils::GlobalConfig::log_error_details() { | ||
error!( | ||
"Failed to remove public part of key \"{}\" from PSA Crypto.", | ||
key_triple.key_name() |
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.
I would personally say that if key_triple
is passed as a parameter just for logging, we can just skip that and log in all cases: Failed to remove public key from PSA Crypto.
But feel free to keep it if you think this info is definitely needed.
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.
Yeah, I was a bit 😒 changing that just to print the key triple, but could change it back
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.
Changed!
5438e23
to
91ff3f6
Compare
This commit adds functionality to the PKCS11 provider to allow it to handle public key operations in software, using a PSA Crypto-compatible library. The functionality is enabled via a config flag. Signed-off-by: Ionut Mihalcea <ionut.mihalcea@arm.com>
91ff3f6
to
61d207b
Compare
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.
I think the testing is good at this is now!
@@ -137,7 +137,7 @@ pgrep -f target/debug/parsec >/dev/null | |||
if [ "$PROVIDER_NAME" = "all" ]; then | |||
echo "Execute all-providers tests" | |||
RUST_BACKTRACE=1 cargo test $TEST_FEATURES --manifest-path ./e2e_tests/Cargo.toml all_providers | |||
RUST_BACKTRACE=1 cargo test $TEST_FEATURES --manifest-path ./e2e_tests/Cargo.toml config | |||
RUST_BACKTRACE=1 cargo test $TEST_FEATURES --manifest-path ./e2e_tests/Cargo.toml config -- --test-threads=1 |
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.
Is testing in single-threaded mode needed even with the mutex we have now in PKCS11 provider?
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.
That was a mistake in the first place, not putting it like that. If you think about it, each of those tests changes the configuration of the service and expects it to be in a certain configuration while it's running. Multiple tests at the same time make no sense.
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.
oh right those are the config tests! Agree, then
This commit adds functionality to the PKCS11 provider to allow it to
handle public key operations in software, using a PSA Crypto-compatible
library. The functionality is enabled via a config flag.
Resolves #223
Signed-off-by: Ionut Mihalcea ionut.mihalcea@arm.com