-
-
Notifications
You must be signed in to change notification settings - Fork 3k
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
Key import and export cli commands #7546
Conversation
Thank you for submitting this PR!
Getting other community members to do a review would be great help too on complex PRs (you can ask in the chats/forums). If you are unsure about something, just leave us a comment.
We currently aim to provide initial feedback/triaging within two business days. Please keep an eye on any labelling actions, as these will indicate priorities and status of your contribution. |
Are the tests private? I'm getting redirected to login when I try to look at sharness. |
Thanks @rendaw, I'll try and take a look at this PR next week. It looks like the CI results are publicly available via the green/red marks but that the runs themselves are not publicly available. |
Awesome, thanks! Also FWIW I included the kitchen sink here, I figured it's easier to add stuff early and remove it later than add stuff later. |
Thanks @rendaw for the work. A few of the kitchen sink things to remove here are:
I can try and do some pushes to this branch and see if we can push it over the finish line. |
Great, thanks! Removed the export option from gen. |
return fmt.Errorf("key with name '%s' doesn't exist", name) | ||
} | ||
|
||
encoded, err := crypto.MarshalPrivateKey(sk) |
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.
@rendaw I switched us from using base58btc encoded strings to just sending the raw bytes over the wire and working with files.
While in the future it's pretty easy for us to add new base encodings to import/export (e.g. --type=raw/multibase) it's actually pretty painful for us to allow for imports to be strings OR files.
I went with files over strings here as it seems likely to me that people will want to ultimately store these blobs in files. Additionally, the keystore currently utilizes files so people are likely already used to backing up keys this way.
name := req.Arguments[0] | ||
|
||
if name == "self" { | ||
return fmt.Errorf("cannot export key with name 'self'") |
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.
Hm. Is there any reason not to allow this?
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 guess not, but we have to the keys live in different places and it's a little messier. Let's add it in a followup 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.
@rendaw this looks pretty good so I'm going to merge it. Thanks for the hard work. We're trying to move along a release candidate for 0.7.0 soon and I wanted to get this in.
If you have any questions/thoughts about design decisions I made in the last week feel free to comment here or in a new issue and we can always file more PRs 😄
Awesome, thanks for helping me out here! The sounds good. The actual form of this functionality isn't so important to me anyway. |
Fixes #4240
This modifies
ipfs key generate
to add an export flag, and addsipfs key export
ipfs key import
ipfs key identify
. The latter prints the public key fingerprint for an exported key.I ran some tests, but I had issues with running the tests locally so I'm hoping that some CI will run the tests for me if I make a PR.
I mentioned some concerns in the linked issue. Also not sure how to handle the private key in the sharness tests -- I could move that into a variable, but I didn't see precedent for that so I kept it inline for the first revision.