-
Notifications
You must be signed in to change notification settings - Fork 984
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
feat(identity): allow importing and exporting ECDSA keys #3863
Conversation
Unreleased patch version 0.43.1 bumped. Added entry for PR 3606: Deriving 'Clone' for 'mdns::Event'
New unreleased minor version 0.44.0.
Co-authored-by: Thomas Eizinger <thomas@eizinger.io>
Unsynced co-authered change.
You can work around this by adding an explicit dependency for |
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.
Awesome! Thank you for splitting this out.
I think we should integrate this into the Keypair::to_protobuf_encoding
function.
Also, can you add a changelog entry?
Can be fixed by using |
Another way would be to simply branch off a different commit:
Where <COMMIT_HASH> is a commit before all your changes. Find one with |
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.
Implementation looks good to me, CI failures need to be addressed :)
ecdsa::SecretKey
pkcs#8 en/decode supportCo-authored-by: Thomas Eizinger <thomas@eizinger.io>
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.
Awesome, thank you!
Waiting with merge on the otherlibp2p implementations to check that we are doing the right thing:
See libp2p/specs#537.
Um what's wrong with the hex value? |
We don't seem to be doing the correct encoding, can you look into that? |
The key used for testing is generated using openssl. It was in pem so I told openssl to convert it to pkcs#8. The first integer is 0 by looking at the key on my machine. However, the key generated by our implementation also comes with the first integer being 0. |
Problem solved. However I don't see a convenient way to encode the key. Seems we need to implement it ourselves. |
I haven't dug into the details but you mentioned RFC5915 in the specs repository. A crates.io search revealed the following: https://docs.rs/sec1/0.7.2/sec1/ Perhaps that is all we need? |
God we just wanted to drop that dependency, hilarious. |
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.
Awesome, thanks for updating!
Description
Implement encoding to/decoding from DER-encoded secret key document for
ecdsa::SecretKey
.Implement encoding to/decoding from protobuf format for ECDSA keys.
Bump dependency
p256
from 0.12 to 0.13.Bump dependency
sec1
from 0.3.0 to 0.7Related: #3681.
Notes & open questions
Attempt for decoding support failed because the returned error typepkcs8::error::Error
does not implementstd::error::Error
(requiresstd
feature onpkcs8
, despitestd
is enabled onp256
), which is required forDecodingError::failed_to_parse
.Change checklist