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

Exposing OpenSSL RSA KeyGen #15116

Closed
daviddias opened this issue Aug 31, 2017 · 31 comments
Closed

Exposing OpenSSL RSA KeyGen #15116

daviddias opened this issue Aug 31, 2017 · 31 comments
Labels
crypto Issues and PRs related to the crypto subsystem. feature request Issues that request new features to be added to Node.js.

Comments

@daviddias
Copy link

I'm trying to understand if there is a reason why RSA KeyGen was never exposed as an API in the crypto module. I'm familiar with the pure js solutions such as keypair but they are really slow compared to using OpenSSL.

Was there any other thread about this where a decision was made?

@benjamingr
Copy link
Member

@nodejs/crypto

@benjamingr benjamingr added the crypto Issues and PRs related to the crypto subsystem. label Aug 31, 2017
@mscdex mscdex added the question Issues that look for answers. label Aug 31, 2017
@bnoordhuis
Copy link
Member

There hasn't been much demand. It has been discussed a few times but since most people generate their keys ahead of time and since you can shell out to (for example) openssl genrsa, there doesn't seem to be a pressing need.

@bnoordhuis
Copy link
Member

I'll close this out. Feel free to reopen if needed.

@daviddias
Copy link
Author

We use it extensively in js-ipfs as we spawn multiple nodes and use RSA keys as Peer Identities. Is this something that can still be considered or should we look for another route?

@tniessen
Copy link
Member

Is this something that can still be considered or should we look for another route?

I would like to wait until we have a final decision about createCipher (see e.g. #13941) before making any further changes to the crypto API. Afterwards, I might consider implementing this.

@bnoordhuis
Copy link
Member

I'll reopen this in the mean time so it's not forgotten about.

@bnoordhuis bnoordhuis reopened this Sep 11, 2017
@bnoordhuis bnoordhuis added feature request Issues that request new features to be added to Node.js. and removed question Issues that look for answers. labels Sep 11, 2017
@bsZeroFive
Copy link

bsZeroFive commented Sep 19, 2017

I would also love to see this as a feature in the default crypto module. Always wondered why this wrapper wasn't integrated. Are there any updates on this request?

@tniessen
Copy link
Member

@bsZeroFive Not yet, I will comment here once a decision has been made. Please use the button on the right to subscribe to this issue if you wish to be notified.

Implementation heavily relies on asynchronous crypto, I believe @jasnell is working on that.

@croraf
Copy link

croraf commented Oct 26, 2017

I would like to have both RSA and EC keys generation functionality (in PEM format), to be used with crypto's signing functionality that supports both RSASSA-PKCS1-v1_5 and ECDSA digital signing algorithms.

@tniessen
Copy link
Member

tniessen commented Apr 6, 2018

The main problem with RSA key generation is that it takes time, time that would be spent synchronously with our current threading model, thus blocking the event loop. We could hand it off to libuv as an async task, but even then it would block other IO / async operations, and very few concurrent key generations could easily block the entire process. I believe someone suggested to use CPU-bound threads (maybe @jasnell?) in addition to existing IO threads, which might be our only option at this point (apart from spawning a separate process). We could also spawn threads ad hoc. Thoughts?

@bnoordhuis
Copy link
Member

You're not wrong but that's already the status quo with crypto.randomBytes() and crypto.pbkdf2(). I'd start out simple with uv_queue_work() and only change to something more complicated if it becomes a bottleneck.

@tniessen
Copy link
Member

@bnoordhuis What should the API be like? I implemented a first version supporting simple RSA key generation today, but I am unsure about the API design. Currently, it looks like

// API:
crypto.generateKeyPair(type, [options], callback);
let key = crypto.generateKeyPairSync(type, [options]);

// Example for RSA (using the default exponent):
crypto.generateKeyPair('rsa', { bits: 4096 }, (err, key) => {
  // key is an unencrypted PKCS#8 string
});

This has the advantage that it can be extended easily for other asymmetric algorithms with options that are specific to those. The alternative is to create an API that is specific to RSA, the choice mostly depends on how likely it is that we will add more generators in the future.

Additionally, there should probably be options about the output format (e.g. encryption, PKCS#8 / PKCS#1 etc.), these would fit into the options object. What should these look like for RSA?

@jasnell
Copy link
Member

jasnell commented Apr 10, 2018

Do we absolutely need the sync version at all?

@tniessen
Copy link
Member

@jasnell I don't think so, I just tried to align it with randomBytes and pbkdf2.

@bnoordhuis
Copy link
Member

Just so we're on the same page, the basic idea is to provide openssl genpkey functionality programmatically?

I'd probably stick with EVP_PKEY_CTX_ctrl_str() so we don't have to duplicate all the per-algorithm configuration options. It's less work, should help with online searches and we're less likely to screw up something.

Strawman proposal:

const params = [
  'rsa_keygen_bits:2048',
  'rsa_padding_mode:none',  // 'key:value' to mimic `openssl genpkey -pkeyopt key:value`
];
crypto.generateKeyPair('rsa', { params }, (err, key) => {
  // should probably also take a pubkey arg
});

Using EVP_PKEY_keygen() gives DSA, DH and EC (including x25519) for free.

@jasnell
Copy link
Member

jasnell commented Apr 11, 2018

@tniessen ... yeah, I get that. I'm thinking for the start, however, let's just do the async version. If someone comes up with a good case for a sync version later, then it can be added as a semver-minor. Less new API surface area. But that's just my opinion, I'm certainly not going to -1 it if the sync version is there :-)

@tniessen
Copy link
Member

@bnoordhuis I understand the reasoning towards EVP_PKEY_CTX_ctrl_str and it should work, but using it just doesn't look very JS-ish to me, this looks highly implementation-specific (which it is). What do you think @jasnell?

// should probably also take a pubkey arg

You are right, most users will need access to both. There should probably be a separate API to convert between key formats (PKCS#1 / PKCS#8, PEM / DER) -- including public key extraction -- as well.

@bnoordhuis
Copy link
Member

If you want something that's more idiomatic JS:

const [key, pubkey] = await crypto.createKeyPair('rsa')
  .setOption('rsa_keygen_bits', 2048)
  .setOption('rsa_padding_mode', 'none')
  .generate();

Or with magic proxies:

const [key, pubkey] = await crypto.createKeyPair('rsa')
  .rsaKeygenBits(2048)  // translates to .setOption('rsa_keygen_bits', 2048)
  .rsaPaddingMode('none')
  .generate();

@canterberry
Copy link

crypto.Sign#sign() requires a PEM-encoded private key, but neither crypto.DiffieHellman#generateKeys() nor crypto.ECDH#generateKeys() produces a private key that can be PEM-encoded using any of the built-in crypto APIs.

@tniessen
Copy link
Member

@canterberry What exactly are you trying to do? DH/ECDH are usually used for key exchanges.

@canterberry
Copy link

canterberry commented Apr 26, 2018

As far as I can tell, the only way to generate a key-pair (i.e: for any operation w/ asymmetric cryptography) natively is via the crypto module. While negotiating a shared secret via a DH key exchange is one use case, the one I'm solving for is creating and validating signatures (specifically, for JWT tokens used for authentication).

For example, jsonwebtoken jwt.sign(), much like crypto.Sign#sign(), expects a PEM-encoded private key when using an asymmetric algorithm like ES256.

@tniessen
Copy link
Member

@canterberry Then you are in the right place, this issue is about the missing key generation API of the crypto module. The existing DH / ECDH APIs are for key exchanges.

@canterberry
Copy link

I've finally put together an interim solution for my use case. It may be a starting point for incorporating PEM encoding into crypto.ECDH#getPublicKey() et al.

A gist that prints the PEM-encoded private key and public key from an ECDH key-pair using the secp256k1 curve is here. The solution here is a little dirty, but is, at least, correct.

I may spend some time trying to extend this to support other algorithms as well, since there seems to be some demand for it.

@hbksagar
Copy link

@croraf @canterberry Found a library https://www.npmjs.com/package/ec-pem

@derknorton
Copy link

A BIG thanks to @hbksagar for the ec-pem module link! It works great and I was able to put together a basic set of EC based examples that rely solely on 'crypto' and 'ec-pem'. Here they are if anyone needs a starting point. NOTE: that I did need to call the deprecated curve.setPublicKey(), method to make it work, so additional input on that discussion:

var crypto = require('crypto');
var ec_pem = require('ec-pem');

var algorithm = 'secp521r1';

function digest(message) {
    var hasher = crypto.createHash('sha512');
    hasher.update(message);
    return hasher.digest('hex');
}

function generate() {
    var curve = crypto.createECDH(algorithm);
    curve.generateKeys();
    return {
        privateKey: curve.getPrivateKey('hex'),
        publicKey: curve.getPublicKey('hex')
    };
}

function recreate(privateKey) {
    var curve = crypto.createECDH(algorithm);
    curve.setPrivateKey(privateKey, 'hex');
    return {
        privateKey: curve.getPrivateKey('hex'),
        publicKey: curve.getPublicKey('hex')
    };
}

function sign(privateKey, message) {
    var curve = crypto.createECDH(algorithm);
    curve.setPrivateKey(privateKey, 'hex');
    var pem = ec_pem(curve, algorithm);
    var signer = crypto.createSign('ecdsa-with-SHA1');
    signer.update(message);
    return signer.sign(pem.encodePrivateKey(), 'base64');
}

function verify(publicKey, message, signature) {
    var curve = crypto.createECDH(algorithm);
    curve.setPublicKey(publicKey, 'hex');
    var pem = ec_pem(curve, algorithm);
    var verifier = crypto.createVerify('ecdsa-with-SHA1');
    verifier.update(message);
    return verifier.verify(pem.encodePublicKey(), signature, 'base64');
}

function encrypt(publicKey, plaintext) {
    // generate and encrypt a 32-byte symmetric key
    var curve = crypto.createECDH(algorithm);
    curve.generateKeys();
    var seed = curve.getPublicKey('hex');  // use the new public key as the seed
    var key = curve.computeSecret(publicKey, 'hex').slice(0, 32);  // take only first 32 bytes

    // encrypt the message using the symmetric key
    var iv = crypto.randomBytes(12);
    var cipher = crypto.createCipheriv('aes-256-gcm', key, iv);
    var ciphertext = cipher.update(plaintext, 'utf8', 'base64');
    ciphertext += cipher.final('base64');
    var tag = cipher.getAuthTag();
    return {
        iv: iv,
        tag: tag,
        seed: seed,
        ciphertext: ciphertext
    };
}

function decrypt(privateKey, message) {
    // decrypt the 32-byte symmetric key
    var seed = message.seed;
    var curve = crypto.createECDH(algorithm);
    curve.setPrivateKey(privateKey, 'hex');
    var key = curve.computeSecret(seed, 'hex').slice(0, 32);  // take only first 32 bytes

    // decrypt the message using the symmetric key
    var iv = message.iv;
    var tag = message.tag;
    var ciphertext = message.ciphertext;
    var decipher = crypto.createDecipheriv('aes-256-gcm', key, iv);
    decipher.setAuthTag(tag);
    var plaintext = decipher.update(ciphertext, 'base64', 'utf8');
    plaintext += decipher.final('utf8');
    return plaintext;
}

@coolaj86
Copy link
Contributor

coolaj86 commented Jul 13, 2018

uRSA has been the main go-to for RSA key generation, but over the years its changed maintainers a number of times and it took forever to get node v10.x support merged in.

openssl is not usually available on Windows and not always available on IoT devices, where something like forge.js is unbearably slow (between 10 and 20 minutes to generate a 2048-bit key).

I think that pulling it into core would be really great - especially considering the eslint fiasco. I'd much rather see crypto packages in core rather than scattered abroad in the community and have to wait so long to upgrade to newer versions of node.

It seems like node is mature enough now to start blessing or replacing common modules that have proven the most common needs.

@tniessen
Copy link
Member

@coolaj86 I implemented most of it, just the API design is still a WIP. I'll see whether I can put something together soon.

@coolaj86
Copy link
Contributor

coolaj86 commented Jul 13, 2018

@tniessen Awesome. I'm really excited to hear that.

I'm not sure if you're familiar with the ecosystem, but currently there are about 4 HUGE (or compiled) libraries that must be used in various combinations in tandem to do just a few simple tasks.

  • ursa
  • forge.js
  • asn1.js
  • pki.js (both JavaScript at HipsterScript/BabelScript/ECMAScript-over-9000 versions)
  • and a few more if you want ECDSA support (which I do)

Tasks that need to be done, generally speaking (https://git.coolaj86.com/coolaj86/keypairs.js#api):

  • generate key
  • JWK <--> PEM conversion (MEGABYTES of JavaScript required for this)
  • sign JWS (for generic standards-based web authentication, including Let's Encrypt, JOSE, OIDC)
  • generate CSR (for Let's Encrypt/ACME and again, MEGABYTES)

All of those except for generate CSR can be done with WebCrypto in the browser, but node still requires a few megabytes of JavaScript to get them done, depending on the operation. My vote would be to make the APIs as similar to WebCrypto as reasonable (even though the WebCrypto APIs aren't very reasonable). It seems a shame to me that node and the web community are so often at odds and (I'm assuming for some sort of political reason) create contrasting APIs that are difficult to reason about.

All I want for Christmas this year is to be able to reduce dependencies in my code. Please help my Christmas wish come true.

@tniessen
Copy link
Member

node and the web community are so often at odds and (I'm assuming for some sort of political reason) create contrasting APIs that are difficult to reason about.

Could you expand on this with a focus on the crypto API? Implementing WebCrypto has been discussed before and I personally don't think WebCrypto is a good fit for Node.js.

@coolaj86
Copy link
Contributor

coolaj86 commented Jul 18, 2018

@tniessen

Preamble

If you've solved the user experience, you've solved everything. If you haven't solved the user experience, you've solved nothing.
Any technical advance, no matter how great, is a waste of effort if it doesn't solve the user's problem. However, every user's problem will eventually require technical advance.

I think that WebCrypto API is a remarkable example of how bureaucratic process results in unintelligible and barely implementable APIs that create almost as many problems as they solve.

HOWEVER, I want to see the Peer Web advance and take hold. The networking problems of the existing internet evolved from dial-up cannot easily be solved (and DHTs are not a user-friendly solution), but we can use encryption to create practically peer-to-peer connections even when network connections don't allow physical peer-to-peer (or unbrokered) connections.

In this way WebCrypto solves problems that could not reasonably be solved with any existing effort, so as cumbersome and terrible as they are, they're a +1 for the web overall.

Security === Convenience

Security and convenience are mutually inclusive. You can only have security with convenience. You cannot have security without convenience.

When some "secure" thing is even slightly inconvenient, users will do something else which is simpler and that will break the security - i.e. centralizing 2FA on multiple devices with Authy or, more commonly, passwords on sticky notes on monitors and whiteboards (I've seen this even in a "secure" military building that required a clearance escort).

Having two APIs for crypto is cumbersome and slows development and adoption. It's more code, more tests, more confusion, more headaches - and therefore less security.

However, the more node adopts and adheres to "Web Standards" APIs - even stupid ones like ArrayBuffer that define endianness in a guaranteed non-deterministic, non-portable way which makes all of its subtypes, except DataView, useless for all practical purposes - the easier it is for the community to create polyfills that work in multiple environments, and the fewer competing standards we will have.

WebCrypto vs Crypto

There's no value to the community to have dozens of incompatible libraries that all do different 70%s of the same thing - forge.js, pki.js asn1.js, elliptic.js, etc, etc, etc. Node already has the problem of fragmented crypto support (which this issues was raised long ago to address). It provides more partial solutions than "whole solutions" (i.e. methods that can operate on RSA keys, but can't generate them, so "the whole solution" is missing).

The WebCrypto people did the wrong thing. They made it difficult. They didn't consider node (which was here first). They made it impractical to do feature detection. They didn't actually define a standard to guarantee any sort of baseline support. In short they had an impractical mathematically-based view on security and completely ignored the human factor.

So even though WebCrypto was probably designed by people who don't even use JavaScript, it's a standard that is more widely adopted than node will ever be (i.e. it's implemented on everything with a browser - phones, computers, even some TVs and gaming consoles). To that end, if node will conform to that standard or provide wrappers for it, it will make it much easier in all JavaScript environments to use and develop code that accomplishes 100% of common tasks ("whole solutions") and doesn't require external libraries (and enables even more convenient libraries to be built on the same base).

It will benefit node and the web to have greater adoption of secure standards.

It kinda sucks that node does all the innovation and then the web standards go in a completely different direction, but the node community has the opportunity to "be the bigger person" and "play nice" for the greater good and for the benefit of the users who generally aren't going to adopt secure practices if it's difficult and takes loads of extra research.

Give people a single path and make it easy to do the secure thing - even easier than doing the insecure thing - and they'll do it out of convenience if nothing else.

There will be more innovation in sum total because more people will have access to the technology.

@tniessen
Copy link
Member

tniessen commented Sep 2, 2018

Thanks for the input, everyone. I opened #22660 with an API proposal.

tniessen added a commit to tniessen/node that referenced this issue Sep 14, 2018
This adds support for RSA, DSA and EC key pair generation with a
variety of possible output formats etc.

Fixes: nodejs#15116
targos pushed a commit that referenced this issue Sep 21, 2018
This adds support for RSA, DSA and EC key pair generation with a
variety of possible output formats etc.

PR-URL: #22660
Fixes: #15116
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ujjwal Sharma <usharma1998@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
crypto Issues and PRs related to the crypto subsystem. feature request Issues that request new features to be added to Node.js.
Projects
None yet
Development

No branches or pull requests