Skip to content
This repository has been archived by the owner on Jun 15, 2023. It is now read-only.

refactor: its a keychain #6

Closed
wants to merge 1 commit into from
Closed

refactor: its a keychain #6

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 1 addition & 2 deletions README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,8 +88,7 @@ A private key is stored as an encrypted PKCS 8 structure in the PEM format. It i
The default options for generating the derived encryption key are in the `dek` object
```js
const defaultOptions = {
createIfNeeded: true,


//See https://cryptosense.com/parameter-choice-for-pbkdf2/
dek: {
keyLength: 512 / 8,
Expand Down
20 changes: 10 additions & 10 deletions src/cms.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,12 +5,12 @@ const forge = require('node-forge')
const util = require('./util')

class CMS {
Copy link
Member

Choose a reason for hiding this comment

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

What does CMS stand for?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

PKCS #7: Cryptographic Message Syntax; [rfc 2315](https://tools.ietf.org/html/rfc2315]

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for clarifying. Mind adding it as a comment above the class?

constructor (keystore) {
if (!keystore) {
throw new Error('keystore is required')
constructor (keychain) {
if (!keychain) {
throw new Error('keychain is required')
}
Copy link
Member

Choose a reason for hiding this comment

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

You could use assert(keychain, 'keychain is required') to make this a one liner, but a if is also fine.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

True, will do.


this.keystore = keystore
this.keychain = keychain
}

createAnonymousEncryptedData (name, plain, callback) {
Copy link
Member

Choose a reason for hiding this comment

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

What is the definition of AnonymousEncryptedData? Why does it need to exist?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will add documentation, real soon. This creates cryptographically secure data. That is data that can only be read by the holder of the key.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

See richardschneider/ipfs-encryption#7

Copy link
Member

Choose a reason for hiding this comment

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

How is that different from regular encrypted data?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

encrypted data is just a blob. CMS provides structure. It particular it describes the data encryption algorithm and parameters. Also it contains a recipientInfo that describes the key that can be used to obtain the data encryption key.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

See richardschneider/ipfs-encryption#9

Copy link
Member

Choose a reason for hiding this comment

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

I see, so this function name is more than just describing what it is it actually points to a data structure defined by CMS/PKCS#7. Please make sure to add this as a comment so that others don't get confused. (I'm always concerned on having design info on people's heads)

Copy link
Contributor Author

@richardschneider richardschneider Dec 10, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

I have written design notes in richardschneider/ipfs-encryption please start with richardschneider/ipfs-encryption#3 and richardschneider/ipfs-encryption#2. Then follow the hyper-links.

Expand All @@ -19,13 +19,13 @@ class CMS {
return callback(new Error('Data is required'))
}

self.keystore._getPrivateKey(name, (err, key) => {
self.keychain._getPrivateKey(name, (err, key) => {
if (err) {
return callback(err)
}

try {
const privateKey = forge.pki.decryptRsaPrivateKey(key, self.keystore._())
const privateKey = forge.pki.decryptRsaPrivateKey(key, self.keychain._())
Copy link
Member

Choose a reason for hiding this comment

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

Please avoid node-forge. We used it before and it is a HUGE dependency. Also last time I checked it was using UMD instead of CJS or ES6 modules which makes it a PITA to import.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

See #7

Copy link
Member

Choose a reason for hiding this comment

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

Still on #7, the crypto primitives should come from the libp2p-crypto. Let's then figure out if we can just extract these functions out of node-forge or another package on npm.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Enhancing libp2p-crypto will take time. It needs to

Crypto is a big issue.

My game plan is to initially use another package (node.forge) so that ipfs key and ipfs crypto can be implemented. Then we can gradually enhance libp2p2-crypto.

Copy link
Member

Choose a reason for hiding this comment

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

My game plan is to initially use another package (node.forge) so that ipfs key and ipfs crypto can be implemented. Then we can gradually enhance libp2p2-crypto.

Yes, we can use another package. Note, libp2p-crypto is just a bundle of other modules, we do not implement any crypto functions ourselves. However, for questions of consistency, we want libp2p crypto to expose the crypto primitives.

Can you add these funcs you need there?

util.certificateForKey(privateKey, (err, certificate) => {
if (err) return callback(err)

Expand Down Expand Up @@ -73,18 +73,18 @@ class CMS {
})
async.detect(
recipients,
(r, cb) => self.keystore.findKeyById(r.keyId, (err, info) => cb(null, !err && info)),
(r, cb) => self.keychain.findKeyById(r.keyId, (err, info) => cb(null, !err && info)),
(err, r) => {
if (err) return callback(err)
if (!r) return callback(new Error('No key found for decryption'))

async.waterfall([
(cb) => self.keystore.findKeyById(r.keyId, cb),
(key, cb) => self.keystore._getPrivateKey(key.name, cb)
(cb) => self.keychain.findKeyById(r.keyId, cb),
(key, cb) => self.keychain._getPrivateKey(key.name, cb)
], (err, pem) => {
if (err) return callback(err)

const privateKey = forge.pki.decryptRsaPrivateKey(pem, self.keystore._())
const privateKey = forge.pki.decryptRsaPrivateKey(pem, self.keychain._())
cms.decrypt(r.recipient, privateKey)
async.setImmediate(() => callback(null, Buffer.from(cms.content.getBytes(), 'binary')))
})
Expand Down