nodejs / node

Node.js JavaScript runtime ✨🐢🚀✨
https://nodejs.org
Other
106.58k stars 29.06k forks source link

Exposing OpenSSL RSA KeyGen #15116

Closed daviddias closed 6 years ago

daviddias commented 7 years ago

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 commented 7 years ago

@nodejs/crypto

bnoordhuis commented 7 years ago

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 commented 7 years ago

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

daviddias commented 7 years ago

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 commented 7 years ago

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. https://github.com/nodejs/node/pull/13941) before making any further changes to the crypto API. Afterwards, I might consider implementing this.

bnoordhuis commented 7 years ago

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

bsZeroFive commented 7 years ago

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 commented 7 years ago

@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 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

@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 commented 6 years ago

Do we absolutely need the sync version at all?

tniessen commented 6 years ago

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

bnoordhuis commented 6 years ago

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 commented 6 years ago

@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 commented 6 years ago

@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 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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

canterberry commented 6 years ago

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 commented 6 years ago

@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 commented 6 years ago

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 commented 6 years ago

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

derknorton commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

@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 commented 6 years ago

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

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

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 commented 6 years ago

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 commented 6 years ago

@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 commented 6 years ago

Thanks for the input, everyone. I opened https://github.com/nodejs/node/pull/22660 with an API proposal.