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

feat: keychain #631

Closed
wants to merge 112 commits into from
Closed

feat: keychain #631

wants to merge 112 commits into from

Conversation

vasco-santos
Copy link
Member

@vasco-santos vasco-santos commented May 7, 2020

This PR is part of the PeerStore improvements EPIC and tackles part of the milestone 4 - KeyChain.

Points to note:

Note on Merging:

Right now the plan (obtained from libp2p-keychain PR) is to:

  • squash the merge to minimize the commit pollution
  • update the readme to point to the original repo
  • append the Co-Author list to the squashed commit to maintain author credit
    • This co author list was generated from the libp2p-keychain package.json which is auto generated from the commit history there.
Co-authored-by: Alan Shaw <[email protected]>
Co-authored-by: Alberto Elias <[email protected]>
Co-authored-by: Alex Potsides <[email protected]>
Co-authored-by: David Dias <[email protected]>
Co-authored-by: Hugo Dias <[email protected]>
Co-authored-by: Jacob Heun <[email protected]>
Co-authored-by: Maciej Krüger <[email protected]>
Co-authored-by: Masahiro Saito <[email protected]>
Co-authored-by: Richard Schneider <[email protected]>
Co-authored-by: Vasco Santos <[email protected]>
Co-authored-by: Vasco Santos <[email protected]>
Co-authored-by: Victor Bjelkholm <[email protected]>

This replaces #525 because the other PR was super outdated and complex to rebase at this point.

daviddias and others added 30 commits December 6, 2017 08:24
Add syntax highlighting to README
This commit updates all CI scripts to the latest version
Persist the key info in the store
test: key name comparision
* test: openssl interop is now the responsibility of libp2p-crypto

* feat: use libp2p-crypto, not node-forge, for key management

* fix: use libp2p-crypto.pbkdf, not node-forge

* fix: do not ship CMS

This removes all depencies on node-forge

* test: update dependencies

* test: remove dead code
vasco-santos and others added 16 commits April 24, 2020 14:58
BREAKING CHANGE: all API methods with peer-info parameters or return values were changed. You can check the API.md document, in order to check the new values to use
Bumps [datastore-fs](https://github.com/ipfs/js-datastore-fs) from 0.9.1 to 1.0.0.
- [Release notes](https://github.com/ipfs/js-datastore-fs/releases)
- [Changelog](https://github.com/ipfs/js-datastore-fs/blob/master/CHANGELOG.md)
- [Commits](ipfs/js-datastore-fs@v0.9.1...v1.0.0)

Signed-off-by: dependabot-preview[bot] <[email protected]>

Co-authored-by: dependabot-preview[bot] <27856297+dependabot-preview[bot]@users.noreply.github.com>
@vasco-santos vasco-santos force-pushed the feat/keychain branch 4 times, most recently from 1df9134 to b1a2b8a Compare May 7, 2020 15:45
Copy link
Contributor

@jacobheun jacobheun left a comment

Choose a reason for hiding this comment

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

Just a general note in addition to the other comments, I think this PR would be a bit easier to review and would maintain an easier historical squash reference if it was split into 2 PRs.

  • PR1: Just include libp2p-keychain in libp2p. This would let us run tests and just use keychain via require('libp2p/src/keychain'). This also ensure the squash commit has the "as is" keychain implementation from existing contributors.
  • PR2: Integrate directly into libp2p. This just makes it easier to see if any keychain code needs to be touched during integration

// ...
const listenMa = libp2p.transportManager.getAddrs()
// [ <Multiaddr 047f00000106f9ba - /ip4/127.0.0.1/tcp/63930> ]

Copy link
Contributor

Choose a reason for hiding this comment

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

This whole added block already exists, this is adding duplicate data.

Copy link
Contributor

Choose a reason for hiding this comment

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

Removing this should also fix the formatting below as it isn't properly closing the code blocks


```js
const keyInfo = await libp2p.keychain.createKey('keyTest', 'rsa', 4096)
const enc = await libp2p.keychain.cms.encrypt('keyTest', Buffer.from('data'))
Copy link
Contributor

Choose a reason for hiding this comment

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

This is an encrypt example, but this section is decrypt

this.datastore = this._options.datastore
this.keychain = this._options.keychain
Copy link
Contributor

Choose a reason for hiding this comment

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

_options.keychain is an object, not a KeyChain, this will break

@@ -168,6 +172,9 @@ class Libp2p extends EventEmitter {
this.peerRouting = peerRouting(this)
this.contentRouting = contentRouting(this)

// Keychain
this.keychain = this._options._keychain || new NoKeychain()
Copy link
Contributor

Choose a reason for hiding this comment

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

Why NoKeychain? It's just throwing errors whenever you try to use it. Just leave it undefined and let type errors happen.

await keychain.importPeer('self', peerId)
}

options._keychain = keychain
Copy link
Contributor

Choose a reason for hiding this comment

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

Having keychain and _keychain on options is confusing. Why not move the keychain creation in to the libp2p constructor? We can create libp2p then do the keychain await stuff before returning the libp2p instance.

@vasco-santos
Copy link
Member Author

Closed in favour of #633 and #634

@achingbrain achingbrain deleted the feat/keychain branch May 18, 2023 13:39
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.

10 participants