Skip to content
This repository has been archived by the owner on Feb 12, 2024. It is now read-only.

Implement Key API #1133

Merged
merged 42 commits into from
Jan 30, 2018
Merged

Implement Key API #1133

merged 42 commits into from
Jan 30, 2018

Conversation

richardschneider
Copy link
Contributor

@richardschneider richardschneider commented Dec 7, 2017

Integrate the keychain into js-ipfs

  • the keychain is constructed in pre-start so it always available
  • add the ipfs key commands from this spec
  • add the HTTP API routes and resources for key

Other tasks

  • add spec/tests to interface-ipfs-core
  • add "key" to js-ipfs-api
  • allow creation/reading of CMS

@ghost ghost assigned richardschneider Dec 7, 2017
@ghost ghost added the status/in-progress In progress label Dec 7, 2017
@richardschneider
Copy link
Contributor Author

@diasdavid this needs both ipfs/js-ipfs-repo#156 and libp2p/js-libp2p-keychain#6 to be released to NPM

@richardschneider
Copy link
Contributor Author

@diasdavid I need a NPM release of libp2p-keychain. Currently the package.json is getting it from github

@richardschneider richardschneider mentioned this pull request Dec 9, 2017
5 tasks
@richardschneider richardschneider changed the title [WIP] Use the keychain Implement Key API using the keychain Dec 10, 2017
@daviddias
Copy link
Member

daviddias commented Dec 13, 2017

Following up on ipfs/team-mgmt#535 (comment), the next steps to get this merge are (by order):

  • Get the node-forge primitives that are being used in libp2p-keychain exposed by libp2p-crypto
    • Explore if we can code pick the functions that we want or if there is another module that doesn't require us to bring node-forge.
  • Release libp2p-crypto
  • Release libp2p-keychain
  • Expose the .crypto and .key APIs in js-libp2p
  • Update this PR

@daviddias
Copy link
Member

@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.

@richardschneider
Copy link
Contributor Author

richardschneider commented Dec 13, 2017

@diasdavid A very good approach to resolving these crypto issues!

My immediate concern is that this PR changes 16 files to support ipfs key management. These changes will become harder to merge as time goes on.

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 master without requiring node.forge. And it partially implements #1135

@@ -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)
Copy link
Member

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

Copy link
Contributor Author

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@diasdavid any comment?

Copy link
Member

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.

Copy link
Contributor Author

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?

@richardschneider
Copy link
Contributor Author

@diasdavid node-forge required primitives can be found at libp2p/js-libp2p-keychain#7 (comment)

@richardschneider richardschneider changed the title Implement Key API using the keychain Key API framework Dec 17, 2017
@richardschneider
Copy link
Contributor Author

richardschneider commented Dec 17, 2017

@diasdavid RTM. This PR provides the framework for ipfs key, both commands, routes and core.

The core uses NoKeychain that just generates a Not Yet Implemented error. This DOES NOT use node-forge.

@daviddias daviddias changed the title Key API framework Implement Key API Dec 17, 2017
@daviddias
Copy link
Member

@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
Copy link

codecov bot commented Dec 20, 2017

Codecov Report

❗ No coverage uploaded for pull request base (master@cfa38ca). Click here to learn what that means.
The diff coverage is 80.57%.

Impacted file tree graph

@@           Coverage Diff            @@
##             master   #1133   +/-   ##
========================================
  Coverage          ?   83.5%           
========================================
  Files             ?     132           
  Lines             ?    2885           
  Branches          ?       0           
========================================
  Hits              ?    2409           
  Misses            ?     476           
  Partials          ?       0
Impacted Files Coverage Δ
src/cli/commands/init.js 76.92% <ø> (ø)
src/http/index.js 82.71% <ø> (ø)
src/http/api/routes/index.js 100% <100%> (ø)
src/cli/utils.js 93.22% <100%> (ø)
src/http/api/routes/key.js 100% <100%> (ø)
src/core/components/index.js 100% <100%> (ø)
src/core/components/init.js 88.52% <100%> (ø)
src/http/api/resources/index.js 100% <100%> (ø)
src/cli/commands/key.js 100% <100%> (ø)
src/core/boot.js 94.33% <100%> (ø)
... and 11 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update cfa38ca...bab526e. Read the comment docs.

@richardschneider
Copy link
Contributor Author

@diasdavid I guess a rebase is required. Can you explain the commands I need. Last time I tried, the PR got munted.

@richardschneider
Copy link
Contributor Author

@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`
Copy link
Member

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])`

Copy link
Member

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

Copy link
Contributor Author

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.

Copy link
Member

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

Copy link
Contributor Author

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@richardschneider
Copy link
Contributor Author

@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

@ghost ghost assigned daviddias Dec 28, 2017
Copy link
Member

@daviddias daviddias left a 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])`

Copy link
Member

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

Copy link
Contributor Author

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])`
Copy link
Member

@daviddias daviddias Dec 28, 2017

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) |
Copy link
Member

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",
Copy link
Member

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?

Copy link
Contributor Author

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",
Copy link
Member

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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this still needed ??

Copy link
Contributor Author

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.

Copy link
Member

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.

Copy link
Contributor Author

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.

Copy link

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.

@@ -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)
Copy link
Member

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.

@richardschneider
Copy link
Contributor Author

richardschneider commented Dec 28, 2017

@diasdavid @victorbjelkholm need help! Circle CI is failing

sudo dpkg -i libnss3*.deb
(Reading database ... 447846 files and directories currently installed.)
Preparing to unpack libnss3-1d_3.28.4-0ubuntu0.14.04.3_amd64.deb ...
Unpacking libnss3-1d:amd64 (2:3.28.4-0ubuntu0.14.04.3) over (2:3.28.4-0ubuntu0.14.04.3) ...
dpkg: error processing archive libnss3-1d_3.28.4-0ubuntu0.14.04.3_i386.deb (--install):
 package architecture (i386) does not match system (amd64)

@richardschneider
Copy link
Contributor Author

@diasdavid @dryajov I've spent the day rebasing this PR and changing the test code to use the new ipfsd-ctl code.

Two tests suites are still failing, which seem to indicated that -pass ... argument is not being passed to the deamon. I'll start investigating in more detail.

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 go boys to release for win32 so that appveyor works. Why isn't Jenkins be used for this project?

@diasdavid When will this get merged? It was ready in late December!

@richardschneider
Copy link
Contributor Author

@diasdavid I need a new NPM release of js-libp2p-keychain with updated deps on js-libp2p-crypto.

Hopefully a new release will fix the following failed tests

  9) interface-ipfs-core tests .key exchange exports:
     Uncaught AssertionError: expected [Error: this only supports TripleDES] to not exist
      at Timeout.ipfs.key.export [as _onTimeout] (node_modules/interface-ipfs-core/src/key.js:156:30)
  10) interface-ipfs-core tests .key exchange imports:
     Uncaught AssertionError: expected [Error: PEM encoded key is required] to not exist
      at Timeout.ipfs.key.import [as _onTimeout] (node_modules/interface-ipfs-core/src/key.js:169:30)
  11) interface-ipfs-core tests .key exchange removes:
     Uncaught AssertionError: expected [Error: Key 'clone' does not exist. ENOENT: no such file or directory, open '/tmp/ipfs-test-554d23dceba5050855569143a605eeb0/keys/info/clone.data'] to not exist
      at Timeout.ipfs.key.rm [as _onTimeout] (node_modules/interface-ipfs-core/src/key.js:183:30)```

@richardschneider
Copy link
Contributor Author

Thanks @diasdavid, the NPM release did fix those 3 tests.

@richardschneider
Copy link
Contributor Author

@diasdavid I'm stuck with the HTTP API ## interface-ipfs-core over ipfs-api .key test suite.

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 ipfsd-ctl, ipfs-api and ipfs; but I cannot prove it. Because of ipfs/js-ipfsd-ctl#193, I cannot even look at debug logs.

What do you think about merging/releasing this PR, then rebuilding the others and trying again?

Any ideas are greatly appreciated!!!!!!!!!!!

@richardschneider
Copy link
Contributor Author

@dryajov see above comment.

Here's a typical failure

 HTTP API ## interface-ipfs-core over ipfs-api .key .gen
Debug: internal, implementation, error 
    AssertionError: expected [Error: Key management requires '--pass ...' option] to not exist
    at ipfs.key.gen (/home/ubuntu/js-ipfs/node_modules/interface-ipfs-core/src/key.js:48:32)
    at send (/home/ubuntu/js-ipfs/node_modules/ipfs-api/src/utils/send-request.js:206:16)
    at f (/home/ubuntu/js-ipfs/node_modules/once/once.js:25:25)

And the code that runs the tests

const DaemonFactory = require('ipfsd-ctl')
const df = DaemonFactory.create({ exec: 'src/cli/bin.js' })
const options = {
  args: ['--pass ipfs-is-awesome-software']
}

const nodes = []
const common = {
  setup: function (callback) {
    callback(null, {
      spawnNode: (cb) => {
        df.spawn(options, (err, _ipfsd) => {
          if (err) {
            return cb(err)
          }

          nodes.push(_ipfsd)
          cb(null, _ipfsd.api)
        })
      }
    })
  },
  teardown: function (callback) {
    parallel(nodes.map((node) => (cb) => node.stop(cb)), callback)
  }
}

test.key(common)

@dryajov
Copy link
Member

dryajov commented Jan 29, 2018

@richardschneider ack - I'll take a look.

(just linking the related issue here ipfs/js-ipfsd-ctl#194)

@richardschneider
Copy link
Contributor Author

@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.

@daviddias
Copy link
Member

👏🏽👏🏽👏🏽 awesome work! Thank you :)

Will just wait for that second Travis run to the finish, it was failing before

@richardschneider
Copy link
Contributor Author

@diasdavid and it just passed as I was reading your comment!

@daviddias daviddias merged commit d945fce into master Jan 30, 2018
@ghost ghost removed the status/in-progress In progress label Jan 30, 2018
@daviddias daviddias deleted the keychain branch January 30, 2018 18:48
MicrowaveDev pushed a commit to galtproject/js-ipfs that referenced this pull request May 22, 2020
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.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants