-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Implement Key API #1133
Implement Key API #1133
Conversation
@diasdavid this needs both ipfs/js-ipfs-repo#156 and libp2p/js-libp2p-keychain#6 to be released to NPM |
@diasdavid I need a NPM release of |
Following up on ipfs/team-mgmt#535 (comment), the next steps to get this merge are (by order):
|
@pgte would love to have your thoughts coming from PeerPad and all the crypto libs you had to import to make it work. Ideally, after this PR, you should be able to just use js-ipfs for crypto calls directly. |
@diasdavid A very good approach to resolving these My immediate concern is that this PR changes 16 files to support Why don't I create a NoKeychain, as I did with pubsub. This will simply throw a NYI error. That way, we get these changes into |
test/cli/commands.js
Outdated
@@ -16,7 +14,7 @@ describe('commands', () => runOnAndOff((thing) => { | |||
|
|||
it('list the commands', () => { | |||
return ipfs('commands').then((out) => { | |||
expect(out.split('\n')).to.have.length(commandCount + 1) | |||
expect(out.split('\n')).to.have.length.above(50) |
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.
Please use the commandCount and not a loose assertion like above
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 strongly disagree. Commands come and go. This is a very brittle test. Why test for the number of commands? All we want to test is that the commands can be obtained.
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.
@diasdavid any comment?
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.
It is not brittle if it is testing that the output of the commands call is returning the exact number of commands.
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.
@diasdavid I just find it odd that when I add a command, there's this little test that I don't know about that then fails. Considering that js-ipfs
tests take ~30 mins to run and some tests fail indeterminately, it gets overlooked.
So I will agree to disagree with you.
Do you want me to restore the original hard coded count?
@diasdavid node-forge required primitives can be found at libp2p/js-libp2p-keychain#7 (comment) |
@diasdavid RTM. This PR provides the framework for The core uses |
@richardschneider there is no rush in merging a PR to add an API that only throws an error saying it is not implemented. Let's get the plan -- #1133 (comment) -- done first. |
Codecov Report
@@ Coverage Diff @@
## master #1133 +/- ##
========================================
Coverage ? 83.5%
========================================
Files ? 132
Lines ? 2885
Branches ? 0
========================================
Hits ? 2409
Misses ? 476
Partials ? 0
Continue to review full report at Codecov.
|
@diasdavid I guess a |
@diasdavid ready for review. Can you tell me magic to do a rebase? |
README.md
Outdated
@@ -270,6 +272,14 @@ A complete API definition is in the works. Meanwhile, you can learn how to you u | |||
- [`ipfs.object.patch.setData(multihash, data, [options, callback])`](https://github.com/ipfs/interface-ipfs-core/tree/master/SPEC/OBJECT.md#objectpatchsetdata) | |||
- [pin (not implemented, yet!)](https://github.com/ipfs/interface-ipfs-core/tree/master/SPEC/) | |||
|
|||
#### `Security` |
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.
s/Security/Crypto and Key Management
README.md
Outdated
- `ipfs.key.list([callback])` | ||
- `ipfs.key.rename(oldName, newName, [callback])` | ||
- `ipfs.key.rm(name, [callback])` | ||
|
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.
Add also the crypto
API while you are at it
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.
crypto is NYI, there's not even a spec yet.
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.
There is the same spec that there is for key https://github.com/ipfs/specs/tree/master/keystore#interface
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.
The crypt
command is not implemented in go
. Also, I have issues with the proposed spec; which I would like to raise in another place.
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.
a4da2c1
to
fc23571
Compare
@diasdavid As promised, my xmas gift to IPFS and with 5 hours to spare. Key management is now ready for review and hopefully a merge. This is dependent upon the following open PRs |
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.
Close to an LGTM. Made some comments.
README.md
Outdated
- `ipfs.key.list([callback])` | ||
- `ipfs.key.rename(oldName, newName, [callback])` | ||
- `ipfs.key.rm(name, [callback])` | ||
|
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 empty line can be removed
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.
done
- `ipfs.key.import(name, pem, password, [callback])` | ||
- `ipfs.key.list([callback])` | ||
- `ipfs.key.rename(oldName, newName, [callback])` | ||
- `ipfs.key.rm(name, [callback])` |
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.
Docs, nice!
@@ -487,6 +501,10 @@ A way to mitigate this in Chrome, is to run your IPFS node inside a Service Work | |||
| [`is-ipfs`](https://github.com/ipfs/is-ipfs) | [![npm](https://img.shields.io/npm/v/is-ipfs.svg?maxAge=86400&style=flat-square)](//github.com/ipfs/is-ipfs/releases) | [![Dep](https://david-dm.org/ipfs/is-ipfs.svg?style=flat-square)](https://david-dm.org/ipfs/is-ipfs) | [![devDep](https://david-dm.org/ipfs/is-ipfs/dev-status.svg?style=flat-square)](https://david-dm.org/ipfs/is-ipfs?type=dev) | [![Travis](https://travis-ci.org/ipfs/is-ipfs.svg?branch=master)](https://travis-ci.org/ipfs/is-ipfs) | | ![Appveyor CI](https://ci.appveyor.com/api/projects/status/github/ipfs/is-ipfs?svg=true) | [![Coverage Status](https://coveralls.io/repos/github/ipfs/is-ipfs/badge.svg?branch=master)](https://coveralls.io/github/ipfs/is-ipfs?branch=master) | | |||
| [`multihashing`](//github.com/multiformats/js-multihashing) | [![npm](https://img.shields.io/npm/v/multihashing.svg?maxAge=86400&style=flat-square)](//github.com/multiformats/js-multihashing/releases) | [![Dep](https://david-dm.org/multiformats/js-multihashing.svg?style=flat-square)](https://david-dm.org/multiformats/js-multihashing) | [![devDep](https://david-dm.org/multiformats/js-multihashing/dev-status.svg?style=flat-square)](https://david-dm.org/multiformats/js-multihashing?type=dev) | [![Travis](https://travis-ci.org/multiformats/js-multihashing.svg?branch=master)](https://travis-ci.org/multiformats/js-multihashing) | [![Circle CI](https://circleci.com/gh/multiformats/js-multihashing.svg?style=svg)](https://circleci.com/gh/jbenet/js-multihashing) | ![Appveyor CI](https://ci.appveyor.com/api/projects/status/github/multiformats/js-multihashing?svg=true) | [![Coverage Status](https://coveralls.io/repos/github/jbenet/js-multihashing/badge.svg?branch=master)](https://coveralls.io/github/jbenet/js-multihashing?branch=master) | | |||
| [`mafmt`](//github.com/whyrusleeping/js-mafmt) | [![npm](https://img.shields.io/npm/v/mafmt.svg?maxAge=86400&style=flat-square)](//github.com/whyrusleeping/js-mafmt/releases) | [![Dep](https://david-dm.org/whyrusleeping/js-mafmt.svg?style=flat-square)](https://david-dm.org/whyrusleeping/js-mafmt) | [![devDep](https://david-dm.org/whyrusleeping/js-mafmt/dev-status.svg?style=flat-square)](https://david-dm.org/whyrusleeping/js-mafmt?type=dev) | [![Travis](https://travis-ci.org/whyrusleeping/js-mafmt.svg?branch=master)](https://travis-ci.org/whyrusleeping/js-mafmt) | [![Circle CI](https://circleci.com/gh/whyrusleeping/js-mafmt.svg?style=svg)](https://circleci.com/gh/whyrusleeping/js-mafmt) | ![Appveyor CI](https://ci.appveyor.com/api/projects/status/github/whyrusleeping/js-mafmt?svg=true) | [![Coverage Status](https://coveralls.io/repos/github/whyrusleeping/js-mafmt/badge.svg?branch=master)](https://coveralls.io/github/whyrusleeping/js-mafmt?branch=master) | | |||
| **Crypto** | |||
| [`libp2p-crypto`](https://github.com/libp2p/js-libp2p-crypto) | [![npm](https://img.shields.io/npm/v/libp2p-crypto.svg?maxAge=86400&style=flat-square)](//github.com/libp2p/js-libp2p-crypto/releases) | [![Dep](https://david-dm.org/libp2p/js-libp2p-crypto.svg?style=flat-square)](https://david-dm.org/libp2p/js-libp2p-crypto) | [![devDep](https://david-dm.org/libp2p/js-libp2p-crypto/dev-status.svg?style=flat-square)](https://david-dm.org/libp2p/js-libp2p-crypto?type=dev) | [![Travis](https://travis-ci.org/libp2p/js-libp2p-crypto.svg?branch=master)](https://travis-ci.org/libp2p/js-libp2p-crypto) | [![Circle CI](https://circleci.com/gh/libp2p/js-libp2p-crypto.svg?style=svg)](https://circleci.com/gh/libp2p/js-libp2p-crypto) | ![Appveyor CI](https://ci.appveyor.com/api/projects/status/github/libp2p/js-libp2p-crypto?svg=true) | [![Coverage Status](https://coveralls.io/repos/github/libp2p/js-libp2p-crypto/badge.svg?branch=master)](https://coveralls.io/github/libp2p/js-libp2p-crypto?branch=master) | | |||
| [`libp2p-keychain`](https://github.com/libp2p/js-libp2p-keychain) | [![npm](https://img.shields.io/npm/v/libp2p-keychain.svg?maxAge=86400&style=flat-square)](//github.com/libp2p/js-libp2p-keychain/releases) | [![Dep](https://david-dm.org/libp2p/js-libp2p-keychain.svg?style=flat-square)](https://david-dm.org/libp2p/js-libp2p-keychain) | [![devDep](https://david-dm.org/libp2p/js-libp2p-keychain/dev-status.svg?style=flat-square)](https://david-dm.org/libp2p/js-libp2p-keychain?type=dev) | [![Travis](https://travis-ci.org/libp2p/js-libp2p-keychain.svg?branch=master)](https://travis-ci.org/libp2p/js-libp2p-keychain) | [![Circle CI](https://circleci.com/gh/libp2p/js-libp2p-keychain.svg?style=svg)](https://circleci.com/gh/libp2p/js-libp2p-keychain) | ![Appveyor CI](https://ci.appveyor.com/api/projects/status/github/libp2p/js-libp2p-keychain?svg=true) | [![Coverage Status](https://coveralls.io/repos/github/libp2p/js-libp2p-keychain/badge.svg?branch=master)](https://coveralls.io/github/libp2p/js-libp2p-keychain?branch=master) | |
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.
👍
package.json
Outdated
@@ -23,13 +23,11 @@ | |||
"build": "aegir build", | |||
"test": "aegir test -t node -t browser --no-cors", | |||
"test:node": "aegir test -t node", | |||
"test:browser": "aegir test -t browser -t webworker --no-cors", | |||
"test:node": "aegir test -t node", | |||
"test:browser": "aegir test -t browser --no-cors", |
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.
Why remove the webworker test?
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.
@diasdavid It came in when I did a rebase. Should I restore to original?
package.json
Outdated
"libp2p-circuit": "~0.1.4", | ||
"libp2p-floodsub": "~0.13.1", | ||
"libp2p-kad-dht": "~0.6.0", | ||
"libp2p-keychain": "github:libp2p/js-libp2p-keychain#options", |
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.
Fixing this.
get cms () { fail() } | ||
} | ||
|
||
module.exports = NoKeychain |
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 this still 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.
Yes, we need it. If the IPFS is started without --pass
, then no-keychain
is used.
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.
Isn't pass
just the key to encrypt it locally? The user should be able to run all of these methods even if locally the key is stored in plain text.
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.
No key is stored in plain text. All keys are encrypted at rest with an encryption key that is derived from the the pass phrase --pass
. See https://github.com/libp2p/js-libp2p-keychain#private-key-storage for more details.
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.
Why can't the private keys be stored as plaintext and within reach from the local user without this password-PBKDF2-whatever stuff? go-ipfs
does that.
test/cli/commands.js
Outdated
@@ -16,7 +14,7 @@ describe('commands', () => runOnAndOff((thing) => { | |||
|
|||
it('list the commands', () => { | |||
return ipfs('commands').then((out) => { | |||
expect(out.split('\n')).to.have.length(commandCount + 1) | |||
expect(out.split('\n')).to.have.length.above(50) |
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.
It is not brittle if it is testing that the output of the commands call is returning the exact number of commands.
@diasdavid @victorbjelkholm need help! Circle CI is failing
|
@diasdavid @dryajov I've spent the day rebasing this PR and changing the test code to use the new Two tests suites are still failing, which seem to indicated that It's a bit hard because of ipfs/js-ipfsd-ctl#185 and I only have a windows machine to test with. You really have to get the @diasdavid When will this get merged? It was ready in late December! |
@diasdavid I need a new NPM release of Hopefully a new release will fix the following failed tests
|
Thanks @diasdavid, the NPM release did fix those 3 tests. |
@diasdavid I'm stuck with the Both the cli and core test suites are passing. And the the core test suite uses the same test cases as HTTP API! I think its some circular reference issue between What do you think about merging/releasing this PR, then rebuilding the others and trying again? Any ideas are greatly appreciated!!!!!!!!!!! |
@dryajov see above comment. Here's a typical failure
And the code that runs the tests
|
@richardschneider ack - I'll take a look. (just linking the related issue here ipfs/js-ipfsd-ctl#194) |
@diasdavid Circle and Travis green lights! Let's merge before another breaking change. Big thanks to @dryajov for getting the HTTP API tests to work. |
👏🏽👏🏽👏🏽 awesome work! Thank you :) Will just wait for that second Travis run to the finish, it was failing before |
@diasdavid and it just passed as I was reading your comment! |
Disable building branches that aren't master - builds will happen for all PRs anyway, so this change means we don't get a branch build and a PR build on each PR.
Integrate the keychain into
js-ipfs
pre-start
so it always availableipfs key
commands from this speckey
Other tasks
interface-ipfs-core
js-ipfs-api
allow creation/reading of CMS