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

feat: add exporting/importing of non rsa keys in libp2p-key format #179

Merged
merged 10 commits into from
Aug 5, 2020

Conversation

jacobheun
Copy link
Contributor

@jacobheun jacobheun commented Jul 31, 2020

This PR adds support for exporting/importing all keys in the libp2p-key format discussed in #145 (comment). This is designed to be a non breaking change. A future breaking change should update the default export of rsa to also be libp2p-key and provide an example for how to perform the migration of previously used keys.

Exported keys are encrypted via the AES-GCM algorithm using password derived PBKDF2 keys. They are exported as base64 encoded strings. I originally considered prefixing the exported bytes with a multibase prefix/multicodec prefix to make it easier to determine key type, but this would violate other formats like PEM, so we wouldn't get any meaningful reusability out of this.

This leverages Node.js and Webcrypto support only.

js-libp2p (including interop with go) and js-ipfs test suites are passing using this module. There pertinent PRs are referenced in the history below.

@jacobheun jacobheun marked this pull request as ready for review August 5, 2020 11:02
@jacobheun
Copy link
Contributor Author

FYI, I am planning to release this as 0.17.9, if there are concerns let me know.

@vasco-santos
Copy link
Member

A future non breaking change should update the default export of rsa to also be libp2p-key and provide an example for how to perform the migration of previously used keys.

You mean a future breaking change, right?

Copy link
Member

@vasco-santos vasco-santos left a comment

Choose a reason for hiding this comment

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

Overall this is looking good 💪
Just some minor details that I would like to point out

/**
*
* @param {object} param0
* @param {string} [param0.algorithm] Defaults to 'aes-128-gcm'
Copy link
Member

Choose a reason for hiding this comment

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

Can we add the defaults in the params, according to the jsdoc docs? This might be useful when we auto generate from jsdoc

/**
*
* @param {object} param0
* @param {Number} [param0.algorithmTagLength] Defaults to 16
Copy link
Member

Choose a reason for hiding this comment

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

Same as below

if (format === 'libp2p-key') {
return exporter.export(this.bytes, password)
} else {
throw errcode(new Error(`export format '${format}' is not supported`), 'ERR_INVALID_EXPORT_FORMAT')
Copy link
Member

Choose a reason for hiding this comment

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

We should add a notice for this in the README

Copy link
Member

Choose a reason for hiding this comment

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

It only has the import

src/keys/index.js Show resolved Hide resolved
const options = {
algorithm: 'aes256',
count: 10000,
saltSize: 128 / 8,
prfAlgorithm: 'sha512'
}
pem = forge.pki.encryptRsaPrivateKey(privateKey, password, options)
key = forge.pki.encryptRsaPrivateKey(privateKey, password, options)
Copy link
Member

Choose a reason for hiding this comment

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

Should we return here instead and not have the let key

* Exports the key into a password protected `format`
*
* @param {string} password - The password to encrypt the key
* @param {string} [format] - Defaults to 'libp2p-key'.
Copy link
Member

Choose a reason for hiding this comment

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

Can we put the default as jsdoc?

@jacobheun
Copy link
Contributor Author

You mean a future breaking change, right?

Yes, I fixed the description.

Copy link
Member

@vasco-santos vasco-santos left a comment

Choose a reason for hiding this comment

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

LGTM!

@jacobheun jacobheun merged commit 7273739 into master Aug 5, 2020
@jacobheun jacobheun deleted the feat/key-exporting branch August 5, 2020 15:14
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.

2 participants