Skip to content
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

Merged
merged 7 commits into from
Aug 3, 2020
Merged

Key import and export cli commands #7546

merged 7 commits into from
Aug 3, 2020

Conversation

rendaw
Copy link

@rendaw rendaw commented Jul 16, 2020

Fixes #4240

This modifies ipfs key generate to add an export flag, and adds ipfs 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.

@welcome
Copy link

welcome bot commented Jul 16, 2020

Thank you for submitting this PR!
A maintainer will be here shortly to review it.
We are super grateful, but we are also overloaded! Help us by making sure that:

  • The context for this PR is clear, with relevant discussion, decisions
    and stakeholders linked/mentioned.

  • Your contribution itself is clear (code comments, self-review for the
    rest) and in its best form. Follow the code contribution
    guidelines

    if they apply.

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.
Next steps:

  • A maintainer will triage and assign priority to this PR, commenting on
    any missing things and potentially assigning a reviewer for high
    priority items.

  • The PR gets reviews, discussed and approvals as needed.

  • The PR is merged by maintainers when it has been approved and comments addressed.

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.
We are very grateful for your contribution!

@rendaw rendaw changed the title Fix #4240: Key import and export cli commands Fix #4240 Key import and export cli commands Jul 16, 2020
@rendaw rendaw changed the title Fix #4240 Key import and export cli commands Key import and export cli commands Jul 16, 2020
@rendaw
Copy link
Author

rendaw commented Jul 16, 2020

Are the tests private? I'm getting redirected to login when I try to look at sharness.

@aschmahmann
Copy link
Contributor

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.

@rendaw
Copy link
Author

rendaw commented Jul 25, 2020

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.

@aschmahmann
Copy link
Contributor

Thanks @rendaw for the work. A few of the kitchen sink things to remove here are:

  • ipfs key gen should not allow exporting keys
  • ipfs key identify doesn't seem super useful right now

I can try and do some pushes to this branch and see if we can push it over the finish line.

@rendaw
Copy link
Author

rendaw commented Aug 1, 2020

Great, thanks! Removed the export option from gen.

return fmt.Errorf("key with name '%s' doesn't exist", name)
}

encoded, err := crypto.MarshalPrivateKey(sk)
Copy link
Contributor

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.

core/commands/keystore.go Outdated Show resolved Hide resolved
name := req.Arguments[0]

if name == "self" {
return fmt.Errorf("cannot export key with name 'self'")
Copy link
Member

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?

Copy link
Contributor

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

Copy link
Contributor

@aschmahmann aschmahmann left a 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 😄

@aschmahmann aschmahmann merged commit dfb8141 into ipfs:master Aug 3, 2020
@rendaw
Copy link
Author

rendaw commented Aug 4, 2020

Awesome, thanks for helping me out here! The sounds good. The actual form of this functionality isn't so important to me anyway.

@aschmahmann aschmahmann mentioned this pull request Sep 22, 2020
72 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

import and export ipfs private keys
3 participants