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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

crypto: add key pair generation #22660

Closed
wants to merge 7 commits into
base: master
from

Conversation

@tniessen
Member

tniessen commented Sep 2, 2018

At last: asymmetric key generation for Node.js! 馃帀


I thought about the API design a lot, so if something feels strange to you, feel free to ask or make suggestions.

The new API supports RSA, DSA, EC and a variety of key encodings (both PEM and DER):

RSA (public) RSA (private) DSA (public) DSA (private) EC (public) EC (private)
PKCS#1 鉁旓笍 鉁旓笍 鉁栵笍 鉁栵笍 鉁栵笍 鉁栵笍
PKCS#1, encrypted 鉁栵笍 鉁栵笍 鉁栵笍 鉁栵笍 鉁栵笍 鉁栵笍
SPKI 鉁旓笍 鉁栵笍 鉁旓笍 鉁栵笍 鉁旓笍 鉁栵笍
PKCS#8 鉁栵笍 鉁旓笍 鉁栵笍 鉁旓笍 鉁栵笍 鉁旓笍
PKCS#8, encrypted 鉁栵笍 鉁旓笍 鉁栵笍 鉁旓笍 鉁栵笍 鉁旓笍
SEC1 鉁栵笍 鉁栵笍 鉁栵笍 鉁栵笍 鉁栵笍 鉁旓笍
SEC1, encrypted 鉁栵笍 鉁栵笍 鉁栵笍 鉁栵笍 鉁栵笍 鉁栵笍

DER is not particularly useful with other Node.js APIs, but it is the only format supported by the WebCrypto standard and thus mainly exists here for interoperability, at least for now.


I understand that this might be difficult to review, so please feel free to ask any questions you might have. I also appreciate partial reviews. There is example code in the API documentation.

As always: cc @nodejs/security-wg @nodejs/crypto

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines
Show outdated Hide outdated src/node_crypto.cc
Show outdated Hide outdated src/node_crypto.cc
Show outdated Hide outdated src/node_crypto.cc
Show outdated Hide outdated src/node_crypto.cc
Show outdated Hide outdated src/node_crypto.cc
Show outdated Hide outdated src/node_crypto.cc
Show outdated Hide outdated src/node_crypto.cc
Show outdated Hide outdated src/node_crypto.cc
Show outdated Hide outdated src/node_crypto.cc
Show outdated Hide outdated doc/api/crypto.md
const r = `-----BEGIN ${label}-----\n${body}\n-----END ${label}-----\n`;
assert(getRegExpForPEM(label).test(r));
return r;
}

This comment has been minimized.

@mcollina

mcollina Sep 2, 2018

Member

maybe this should be part of the API as well? I think Node should provide a way to read this format, if we generate it.

@mcollina

mcollina Sep 2, 2018

Member

maybe this should be part of the API as well? I think Node should provide a way to read this format, if we generate it.

@tniessen

This comment has been minimized.

Show comment
Hide comment
@tniessen

tniessen Sep 2, 2018

Member

@mcollina I understand your points and I tend to agree with you. The reason that there currently is no default pem: true and that there is no API to convert between formats is that I would like to plan an API for keys before doing that. Staying as close to WebCrypto compatibility as possible was a main concern when I wrote this PR, e.g. many of the options are compatible to WebCrypto. We might consider having key objects in the future, most other crypto APIs do, since they come with a couple of advantages. Converting between PEM and DER is trivial, that's not what users will get stuck on. Other operations, e.g. retrieving fields from an RSA key, are difficult to implement in JS (see e.g. the ursa package for an implementation), and I think we should make a plan before adding more and more functions. That's also the reason why there is no default publicKeyEncoding / privateKeyEncoding: If we decide to add key objects, we can simply return the key pair as objects if no encoding is specified without breaking compatibility with the current API.
About pem: true: This comes from WebCrypto, the standard only permits DER, not PEM, so I thought it might be good not to have a contrary default. If we improve our API to better support keys, both formats might be equally valid to use.
The other reason I didn't add any defaults is simple: We can always add defaults, but we cannot change them or take them away. These options are also not security-relevant.

(The way we handle keys is a mess, to be honest. #22553 was a first step on a long road.)

Member

tniessen commented Sep 2, 2018

@mcollina I understand your points and I tend to agree with you. The reason that there currently is no default pem: true and that there is no API to convert between formats is that I would like to plan an API for keys before doing that. Staying as close to WebCrypto compatibility as possible was a main concern when I wrote this PR, e.g. many of the options are compatible to WebCrypto. We might consider having key objects in the future, most other crypto APIs do, since they come with a couple of advantages. Converting between PEM and DER is trivial, that's not what users will get stuck on. Other operations, e.g. retrieving fields from an RSA key, are difficult to implement in JS (see e.g. the ursa package for an implementation), and I think we should make a plan before adding more and more functions. That's also the reason why there is no default publicKeyEncoding / privateKeyEncoding: If we decide to add key objects, we can simply return the key pair as objects if no encoding is specified without breaking compatibility with the current API.
About pem: true: This comes from WebCrypto, the standard only permits DER, not PEM, so I thought it might be good not to have a contrary default. If we improve our API to better support keys, both formats might be equally valid to use.
The other reason I didn't add any defaults is simple: We can always add defaults, but we cannot change them or take them away. These options are also not security-relevant.

(The way we handle keys is a mess, to be honest. #22553 was a first step on a long road.)

@tniessen

This comment has been minimized.

Show comment
Hide comment
@tniessen

tniessen Sep 9, 2018

Member

Ping. I'd be okay with adding some defaults if that helps.

Rebased, old HEAD was 54f9667.

Member

tniessen commented Sep 9, 2018

Ping. I'd be okay with adding some defaults if that helps.

Rebased, old HEAD was 54f9667.

@mcollina

This comment was marked as outdated.

Show comment
Hide comment
@mcollina

mcollina Sep 10, 2018

Member

I would prefer a format: 'pem' option rather than pem: true. The first is more extensible and it would allow more formats in the future.

Member

mcollina commented Sep 10, 2018

I would prefer a format: 'pem' option rather than pem: true. The first is more extensible and it would allow more formats in the future.

@tniessen

This comment was marked as outdated.

Show comment
Hide comment
@tniessen

tniessen Sep 10, 2018

Member

@mcollina Done, PTAL.

Member

tniessen commented Sep 10, 2018

@mcollina Done, PTAL.

@jasnell

This comment was marked as outdated.

Show comment
Hide comment
@jasnell

jasnell Sep 10, 2018

Member

@tniessen ... in the test, can you please include a quick verification that the util.promisify() wrapped version of the async function works as expected?

Member

jasnell commented Sep 10, 2018

@tniessen ... in the test, can you please include a quick verification that the util.promisify() wrapped version of the async function works as expected?

@tniessen

This comment was marked as outdated.

Show comment
Hide comment
@tniessen

tniessen Sep 10, 2018

Member

@jasnell Sure, done.

Now that format has a different meaning, maybe the error code ERR_CRYPTO_INCOMPATIBLE_KEY_FORMAT should be renamed as well?

Member

tniessen commented Sep 10, 2018

@jasnell Sure, done.

Now that format has a different meaning, maybe the error code ERR_CRYPTO_INCOMPATIBLE_KEY_FORMAT should be renamed as well?

@mcollina

This comment was marked as outdated.

Show comment
Hide comment
@mcollina

mcollina Sep 11, 2018

Member

Now that format has a different meaning, maybe the error code ERR_CRYPTO_INCOMPATIBLE_KEY_FORMAT should be renamed as well?

Maybe, but I don't have any recommendations :/.

Member

mcollina commented Sep 11, 2018

Now that format has a different meaning, maybe the error code ERR_CRYPTO_INCOMPATIBLE_KEY_FORMAT should be renamed as well?

Maybe, but I don't have any recommendations :/.

@tniessen

This comment has been minimized.

Show comment
Hide comment
@tniessen

tniessen Sep 11, 2018

Member

I changed the code to ERR_CRYPTO_INCOMPATIBLE_KEY_OPTIONS due to lack of a better idea. PTAL.

Member

tniessen commented Sep 11, 2018

I changed the code to ERR_CRYPTO_INCOMPATIBLE_KEY_OPTIONS due to lack of a better idea. PTAL.

@diasdavid

This comment has been minimized.

Show comment
Hide comment
@diasdavid

diasdavid Sep 12, 2018

thank you so much for making this happen. It will make the crypto bundle for libp2p way faster and smaller

diasdavid commented Sep 12, 2018

thank you so much for making this happen. It will make the crypto bundle for libp2p way faster and smaller

@mcollina

LGTM

@tniessen

This comment was marked as outdated.

Show comment
Hide comment

tniessen added some commits Sep 2, 2018

crypto: add API for key pair generation
This adds support for RSA, DSA and EC key pair generation with a
variety of possible output formats etc.

Fixes: #15116
@tniessen

This comment has been minimized.

Show comment
Hide comment
@tniessen

tniessen Sep 14, 2018

Member

CI passed despite GH saying otherwise. I will wait a couple more days before merging this to allow others to weigh in.

Member

tniessen commented Sep 14, 2018

CI passed despite GH saying otherwise. I will wait a couple more days before merging this to allow others to weigh in.

Show resolved Hide resolved doc/api/crypto.md
Show outdated Hide outdated doc/api/crypto.md

tniessen added some commits Sep 14, 2018

Show outdated Hide outdated src/node_crypto.cc
Show outdated Hide outdated src/node_crypto.cc
Show resolved Hide resolved src/node_crypto.cc
Show outdated Hide outdated src/node_crypto.cc
@tniessen

This comment has been minimized.

Show comment
Hide comment
@tniessen

tniessen Sep 18, 2018

Member

CI is green. Last chance for @nodejs/security-wg or @nodejs/crypto to weigh in before I land this tomorrow (decided to wait a bit longer).

Member

tniessen commented Sep 18, 2018

CI is green. Last chance for @nodejs/security-wg or @nodejs/crypto to weigh in before I land this tomorrow (decided to wait a bit longer).

@ryzokuken

馃挴 Thanks, @tniessen.

@bnoordhuis

Nice work, Tobias!

Show outdated Hide outdated lib/internal/crypto/keygen.js
Show outdated Hide outdated src/node_crypto.cc
Show outdated Hide outdated src/node_crypto.cc
// Encode SEC1 as PEM.
if (PEM_write_bio_ECPrivateKey(bio.get(), ec_key.get(),
nullptr, nullptr, 0,
nullptr, nullptr) != 1)

This comment has been minimized.

@bnoordhuis

bnoordhuis Sep 19, 2018

Member

Little known fact but it's possible to encrypt the EC private key if you pass in a DES/DES3/IDEA cipher as the third argument and the passphrase as the last argument (assuming it's a nul-terminated string, otherwise you need a custom callback.)

Maybe other ciphers work too but then you might be creating keys that other tools can't read. :-)

@bnoordhuis

bnoordhuis Sep 19, 2018

Member

Little known fact but it's possible to encrypt the EC private key if you pass in a DES/DES3/IDEA cipher as the third argument and the passphrase as the last argument (assuming it's a nul-terminated string, otherwise you need a custom callback.)

Maybe other ciphers work too but then you might be creating keys that other tools can't read. :-)

This comment has been minimized.

@tniessen

tniessen Sep 19, 2018

Member

I thought SEC1 encryption wasn't standardized (similarly for PKCS#1), so I didn't implement it. If it is, I'll gladly implement it later.

@tniessen

tniessen Sep 19, 2018

Member

I thought SEC1 encryption wasn't standardized (similarly for PKCS#1), so I didn't implement it. If it is, I'll gladly implement it later.

Show outdated Hide outdated src/node_crypto.cc
Show resolved Hide resolved src/node_crypto.cc
Show outdated Hide outdated src/node_crypto.cc
Show resolved Hide resolved src/node_crypto.cc
@tniessen

This comment has been minimized.

Show comment
Hide comment
@tniessen

This comment has been minimized.

Show comment
Hide comment
@tniessen

tniessen Sep 20, 2018

Member

Thanks for reviewing, everyone! Landed in 8c502f5, 085dcf0.

Member

tniessen commented Sep 20, 2018

Thanks for reviewing, everyone! Landed in 8c502f5, 085dcf0.

@tniessen tniessen closed this Sep 20, 2018

tniessen added a commit that referenced this pull request Sep 20, 2018

crypto: add API for key pair generation
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>

tniessen added a commit that referenced this pull request Sep 20, 2018

crypto: allow promisifying generateKeyPair
PR-URL: #22660
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>
@Trott

This comment has been minimized.

Show comment
Hide comment
@Trott

Trott Sep 20, 2018

Member

This added test-crypto-keygen test is unreliable, unfortunately:

https://ci.nodejs.org/job/node-test-commit-linuxone/5311/nodes=rhel72-s390x/console

09:59:09 not ok 2221 parallel/test-crypto-keygen
09:59:09   ---
09:59:09   duration_ms: 120.14
09:59:09   severity: fail
09:59:09   exitcode: -15
09:59:09   stack: |-
09:59:09     timeout
09:59:09     assert.js:662
09:59:09         throw actual;
09:59:09         ^
09:59:09     
09:59:09     Error: error:0D07207B:asn1 encoding routines:ASN1_get_object:header too long
09:59:09         at Sign.sign (internal/crypto/sig.js:83:27)
09:59:09         at testSignVerify (/data/iojs/build/workspace/node-test-commit-linuxone/nodes/rhel72-s390x/test/parallel/test-crypto-keygen.js:43:42)
09:59:09         at assert.throws (/data/iojs/build/workspace/node-test-commit-linuxone/nodes/rhel72-s390x/test/parallel/test-crypto-keygen.js:234:7)
09:59:09         at getActual (assert.js:574:5)
09:59:09         at Function.throws (assert.js:692:24)
09:59:09         at AsyncWrap.common.mustCall (/data/iojs/build/workspace/node-test-commit-linuxone/nodes/rhel72-s390x/test/parallel/test-crypto-keygen.js:233:12)
09:59:09         at AsyncWrap.<anonymous> (/data/iojs/build/workspace/node-test-commit-linuxone/nodes/rhel72-s390x/test/common/index.js:350:15)
09:59:09         at AsyncWrap.wrap.ondone (internal/crypto/keygen.js:42:14)
09:59:09   ...
Member

Trott commented Sep 20, 2018

This added test-crypto-keygen test is unreliable, unfortunately:

https://ci.nodejs.org/job/node-test-commit-linuxone/5311/nodes=rhel72-s390x/console

09:59:09 not ok 2221 parallel/test-crypto-keygen
09:59:09   ---
09:59:09   duration_ms: 120.14
09:59:09   severity: fail
09:59:09   exitcode: -15
09:59:09   stack: |-
09:59:09     timeout
09:59:09     assert.js:662
09:59:09         throw actual;
09:59:09         ^
09:59:09     
09:59:09     Error: error:0D07207B:asn1 encoding routines:ASN1_get_object:header too long
09:59:09         at Sign.sign (internal/crypto/sig.js:83:27)
09:59:09         at testSignVerify (/data/iojs/build/workspace/node-test-commit-linuxone/nodes/rhel72-s390x/test/parallel/test-crypto-keygen.js:43:42)
09:59:09         at assert.throws (/data/iojs/build/workspace/node-test-commit-linuxone/nodes/rhel72-s390x/test/parallel/test-crypto-keygen.js:234:7)
09:59:09         at getActual (assert.js:574:5)
09:59:09         at Function.throws (assert.js:692:24)
09:59:09         at AsyncWrap.common.mustCall (/data/iojs/build/workspace/node-test-commit-linuxone/nodes/rhel72-s390x/test/parallel/test-crypto-keygen.js:233:12)
09:59:09         at AsyncWrap.<anonymous> (/data/iojs/build/workspace/node-test-commit-linuxone/nodes/rhel72-s390x/test/common/index.js:350:15)
09:59:09         at AsyncWrap.wrap.ondone (internal/crypto/keygen.js:42:14)
09:59:09   ...
@tniessen

This comment has been minimized.

Show comment
Hide comment
@tniessen

tniessen Sep 20, 2018

Member

@Trott I fear there might be a small chance (0.392156862745098% if my calculation is correct) of OpenSSL successfully decrypting a key it is not supposed to decrypt, but leading to an invalid key and thus causing this error message. Let me try to confirm that before fixing the test.

Member

tniessen commented Sep 20, 2018

@Trott I fear there might be a small chance (0.392156862745098% if my calculation is correct) of OpenSSL successfully decrypting a key it is not supposed to decrypt, but leading to an invalid key and thus causing this error message. Let me try to confirm that before fixing the test.

@Trott

This comment has been minimized.

Show comment
Hide comment
@Trott

Trott Sep 20, 2018

Member

While stress-testing locally with -j 96 --repeat 192, I got a different error:

=== release test-crypto-keygen ===                    
Path: parallel/test-crypto-keygen
assert.js:662
    throw actual;
    ^

Error: error:0D0680A8:asn1 encoding routines:asn1_check_tlen:wrong tag
    at Sign.sign (internal/crypto/sig.js:83:27)
    at testSignVerify (/Users/trott/io.js/test/parallel/test-crypto-keygen.js:43:42)
    at assert.throws (/Users/trott/io.js/test/parallel/test-crypto-keygen.js:234:7)
    at getActual (assert.js:574:5)
    at Function.throws (assert.js:692:24)
    at AsyncWrap.common.mustCall (/Users/trott/io.js/test/parallel/test-crypto-keygen.js:233:12)
    at AsyncWrap.<anonymous> (/Users/trott/io.js/test/common/index.js:350:15)
    at AsyncWrap.wrap.ondone (internal/crypto/keygen.js:42:14)
Command: out/Release/node /Users/trott/io.js/test/parallel/test-crypto-keygen.js
[02:08|% 100|+ 191|-   1]: Done 
Member

Trott commented Sep 20, 2018

While stress-testing locally with -j 96 --repeat 192, I got a different error:

=== release test-crypto-keygen ===                    
Path: parallel/test-crypto-keygen
assert.js:662
    throw actual;
    ^

Error: error:0D0680A8:asn1 encoding routines:asn1_check_tlen:wrong tag
    at Sign.sign (internal/crypto/sig.js:83:27)
    at testSignVerify (/Users/trott/io.js/test/parallel/test-crypto-keygen.js:43:42)
    at assert.throws (/Users/trott/io.js/test/parallel/test-crypto-keygen.js:234:7)
    at getActual (assert.js:574:5)
    at Function.throws (assert.js:692:24)
    at AsyncWrap.common.mustCall (/Users/trott/io.js/test/parallel/test-crypto-keygen.js:233:12)
    at AsyncWrap.<anonymous> (/Users/trott/io.js/test/common/index.js:350:15)
    at AsyncWrap.wrap.ondone (internal/crypto/keygen.js:42:14)
Command: out/Release/node /Users/trott/io.js/test/parallel/test-crypto-keygen.js
[02:08|% 100|+ 191|-   1]: Done 
@Trott

This comment has been minimized.

Show comment
Hide comment
@Trott

Trott Sep 20, 2018

Member

I've opened #22978 so maybe we can take the conversation there...

Member

Trott commented Sep 20, 2018

I've opened #22978 so maybe we can take the conversation there...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment