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
pkcs11-tool: add support for importing Ed25518/448 keys #2985
pkcs11-tool: add support for importing Ed25518/448 keys #2985
Conversation
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.
just one nit inline.
Does this require some modified softhsm or could this be intergrated in the tests in tests/ directory?
I'll be honest, I haven't looked at adding any tests so far. I develop against a QNX custom key management solution at the moment with my own custom software HSM (using openssl 1.1.1 or 3.0). I could look into adding ED key generation and EDDSA tests as it seems softhsm2 does support them. It would take me some time to figure out the test setup and modifications to do so. |
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.
lgtm.
Tests would be great though. I think something simple as modifying the following test (that now tries to generate and import RSA and EC keys) to do also ED25519 and ED448 should do:
https://github.com/OpenSC/OpenSC/blob/master/tests/test-pkcs11-tool-import.sh
Yes that's what I was looking at. Feel free to hold this up until I add the test or I can push another PR for the test only separately once I can get things working. I can also do the same for EDDSA at the same time if done in a separate PR. |
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.
gost_info_free()
uses OPENSSL_free()
to release the structure's members. I'm not sure if we are relying on some OpenSSL internal initialization somewhere, but if so, we should use OPENSSL_malloc()
to create the members by hand. If we don't use some internal OpenSSL allocation, then we can stick to malloc()
, but should switch to free()
when releasing the memory.
I think we are good. Can you squash the fixup commits so we can cleanly merge this? |
eda4c3c
to
fe9ccaa
Compare
Related #2952
NOTE: I do not have this patch in my version of the tool so the EC_POINT bit value is off for 448: 5493770
OIDs are printed properly regardless.
Tested this with pkcs11-tool compiled with both openssl 1.1.1 and 3.0.
Checklist