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

crypto: add key object API #24234

Open
wants to merge 8 commits into
base: master
from

Conversation

Projects
None yet
8 participants
@tniessen
Member

tniessen commented Nov 7, 2018

Preamble: This is a huge change and I will do my best to help with reviewing it. There might still be dozens of places that need some work, but so far, everything seems to be working nicely. I summarized the motivation behind this change in #15113 (comment). There are also lots of possible discussions around this, e.g. whether key derivation should consume / produce key objects etc.


This commit makes multiple important changes:

  1. A new key object API is introduced. The KeyObject class itself is
    not exposed to users, instead, several new APIs can be used to
    construct key objects: createSecretKey, createPrivateKey and
    createPublicKey. The new API also allows to convert between
    different key formats, and even though the API itself is not
    compatible to the WebCrypto standard in any way, it makes
    interoperability much simpler.

  2. Key objects can be used instead of the raw key material in all
    relevant crypto APIs.

  3. The handling of asymmetric keys has been unified and greatly
    improved. Node.js now fully supports both PEM-encoded and
    DER-encoded public and private keys.

  4. Conversions between buffers and strings have been moved to native
    code for sensitive data such as symmetric keys due to security
    considerations such as zeroing temporary buffers.

  5. For compatibility with older versions of the crypto API, this
    change allows to specify Buffers and strings as the "passphrase"
    option when reading or writing an encoded key. Note that this
    can result in unexpected behavior if the password contains a
    null byte.


cc @nodejs/crypto @nodejs/security-wg @nodejs/security

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

First pass, looks good.
Some guideline nits.

Show resolved Hide resolved lib/internal/crypto/sig.js Outdated
Show resolved Hide resolved src/node_crypto.cc Outdated
Show resolved Hide resolved src/node_crypto.cc Outdated
Show resolved Hide resolved src/node_crypto.cc Outdated
Show resolved Hide resolved src/node_crypto.cc
Show resolved Hide resolved src/node_crypto.cc
Show resolved Hide resolved src/node_crypto.cc
Show resolved Hide resolved src/node_crypto.cc Outdated
Show resolved Hide resolved src/node_crypto.cc
Show resolved Hide resolved src/node_crypto.cc
* `cipher`: {string} If specified, the private key will be encrypted with
the given `cipher` and `passphrase` using PKCS#5 v2.0 password based
encryption.
* `passphrase`: {string | Buffer} The passphrase to use for encryption, see

This comment has been minimized.

@sam-github

sam-github Nov 8, 2018

Member

if passphrase is provided without a cipher, can we default to a good strong cipher?

This comment has been minimized.

@sam-github

sam-github Nov 14, 2018

Member

^--- I don't think we should force people to pick a cipher in order to use the API. If we do force them, we should at least recommend one in the docs. They shouldn't have googling around trying to figure out a respected cipher to use, I think that qualifies as a foot gun.

Show resolved Hide resolved doc/api/crypto.md Outdated
Most applications should consider using the new `KeyObject` API instead of
passing keys as strings or `Buffer`s due to improved security features.
### keyObject.getType()

This comment has been minimized.

@addaleax

addaleax Nov 8, 2018

Member

Might be nice to make some of these getters? :)

This comment has been minimized.

@tniessen

tniessen Nov 9, 2018

Member

I considered that, and it would certainly have some advantages (e.g. retrieving symmetricKeySize would not throw an error on asymmetric keys (or should it?)), but most other crypto APIs use functions instead of getters. What do you think?

This comment has been minimized.

@sam-github

sam-github Nov 14, 2018

Member

If making it a getter means changing this to keyObject.type, I'm +1

This comment has been minimized.

@jasnell

jasnell Nov 19, 2018

Member

I'd be +1 on using getters. It's a nicer API model, in my opinion. We shouldn't need to feel too constrained by the existing API pattern here.

Show resolved Hide resolved src/node_crypto.cc Outdated
Show resolved Hide resolved src/node_crypto.cc Outdated
Show resolved Hide resolved src/node_crypto.cc Outdated
Show resolved Hide resolved src/node_crypto.cc Outdated
Show resolved Hide resolved src/node_crypto.cc Outdated
@@ -1046,6 +1046,76 @@ encoding of `'utf8'` is enforced. If `data` is a [`Buffer`][], `TypedArray`, or
This can be called many times with new data as it is streamed.
## Class: KeyObject

This comment has been minimized.

@mcollina

mcollina Nov 9, 2018

Member

Would we want to add this as experimental first?

This comment has been minimized.

@tniessen

tniessen Nov 9, 2018

Member

That might be a good idea. Would that require us to put it behind a flag or could it be "documentation-only" experimental?

This comment has been minimized.

@sam-github

sam-github Nov 19, 2018

Member

The API is straightforward enough, I'd be ok with it not being experimental.

This comment has been minimized.

@jasnell

jasnell Nov 19, 2018

Member

If it did land as experimental, it would not need a flag as the behavior is entirely opt in. A runtime warning would be sufficient. That said, we did not land the new key generation APIs as experimental and I don't think we should really have to land this one that way either.

@TimothyGu

This comment has been minimized.

Member

TimothyGu commented Nov 10, 2018

The new API also allows to convert between different key formats, and even though the API itself is not compatible to the WebCrypto standard in any way, it makes interoperability much simpler.

What are the differences between this new API and the API exposed through web crypto? Could we realistically change this API to be compatible?

@tniessen

This comment has been minimized.

Member

tniessen commented Nov 10, 2018

What are the differences between this new API and the API exposed through web crypto? Could we realistically change this API to be compatible?

WebCrypto has a very different design. I originally called the new API CryptoKey but changed it to KeyObject to avoid confusion with the WebCrypto CryptoKey class. Some of the key differences:

  • CryptoKey objects can be exported as raw/SPKI/PKCS#8 only and that is done using a separate function, exportKey, not using an instance method. Our options are much more complex, e.g. to support encrypted keys, and I think it makes sense for this to be an instance method.

  • The type property is actually compatible to the getType() function of the KeyObject class, and if we decide to implement @addaleax suggestion and turn it into a property, it will match the WebCrypto spec. However, CryptoKey instances have three additional properties that would have a huge impact on the way people can use key objects:

    readonly attribute boolean extractable;
    readonly attribute object algorithm;
    readonly attribute object usages;
    

    We could implement them, but they don't make sense unless we adopt the whole WebCrypto API in my opinion, and as I said before, I don't think we should do that.

  • Symmetric CryptoKey objects don't have a property describing the size of the underlying key, and the only way to get the type of an asymmetric key is to parse the algorithm identifier associated with the key.


Rebased, old HEAD was 6c92496.

tniessen added some commits Sep 20, 2018

crypto: add key object API
This commit makes multiple important changes:

1. A new key object API is introduced. The KeyObject class itself is
   not exposed to users, instead, several new APIs can be used to
   construct key objects: createSecretKey, createPrivateKey and
   createPublicKey. The new API also allows to convert between
   different key formats, and even though the API itself is not
   compatible to the WebCrypto standard in any way, it makes
   interoperability much simpler.

2. Key objects can be used instead of the raw key material in all
   relevant crypto APIs.

3. The handling of asymmetric keys has been unified and greatly
   improved. Node.js now fully supports both PEM-encoded and
   DER-encoded public and private keys.

4. Conversions between buffers and strings have been moved to native
   code for sensitive data such as symmetric keys due to security
   considerations such as zeroing temporary buffers.

5. For compatibility with older versions of the crypto API, this
   change allows to specify Buffers and strings as the "passphrase"
   option when reading or writing an encoded key. Note that this
   can result in unexpected behavior if the password contains a
   null byte.
Show resolved Hide resolved src/node_crypto.cc Outdated
Show resolved Hide resolved src/node_crypto.cc Outdated
Show resolved Hide resolved src/node_crypto.h
allocated_data_(allocated_data),
size_(size) {}
ByteSource ByteSource::Allocated(char* data, size_t size) {

This comment has been minimized.

@refack

refack Nov 12, 2018

Member

IMHO this is a leaky abstraction. I would suggest you make two classes OwningByteSource and ForignByteSource, that inherit from an abstract ByteSource

This comment has been minimized.

@refack

refack Nov 12, 2018

Member

If this is supposed to be a by-value only class, then you could implement this polymorphism with the Pimpl pattern

tniessen added some commits Nov 12, 2018

@sam-github

I really like the direction, I think its a good API. Left some comments, mostly on the API. The C++ is a lot to read through, sorry, I ran out of time. I would prefer this not be experimental. I assume it doesn't intentionally break current APIs, and it also isn't particularly complex in terms of its API, so its hard to see it generating much comment or controversy or need to change. Would be great if it can go into 11.x, get some mileage there, and be stable in 12.xx. I doubt it can make it into 10.x, its a lot of churn.

Most applications should consider using the new `KeyObject` API instead of
passing keys as strings or `Buffer`s due to improved security features.
### keyObject.getType()

This comment has been minimized.

@sam-github

sam-github Nov 14, 2018

Member

If making it a getter means changing this to keyObject.type, I'm +1

-->
* Returns: {number}
For symmetric keys, this function returns the size of the key in bytes. This

This comment has been minimized.

@sam-github

sam-github Nov 14, 2018

Member

crypto sizes are usually measured in bits, not bytes. AES-256, for example, would have a symetric size of 32 using this API, which strikes me as surprising. Perhaps it depends on the intended use of the number... if it is intended to go back as an argument to other APIs that expect sizes in bytes, this would make more sense, is that the case?

This comment has been minimized.

@tniessen

tniessen Nov 17, 2018

Member

It is true that most sizes are measured in bits, but most crypto APIs I have come across have used bytes since more or less all practically relevant applications use a multiple of 8 bits anyway, and it is much more convenient to allocate, copy, transfer and free a number of bytes. I think all of our APIs express sizes as bytes, e.g. key derivation.

This comment has been minimized.

@sam-github

sam-github Nov 19, 2018

Member

OK, agree on returning bytes. Don't understand why there is only a getSymmetricSize() though. Once we have getAsymmetricSize(), people are going to be scratching their head wondering why there are two functions.

* Returns: {number}
For symmetric keys, this function returns the size of the key in bytes. This
function is undefined for asymmetric keys.

This comment has been minimized.

@sam-github

sam-github Nov 14, 2018

Member

ec keys have a size, as do RSA. Perhaps it should be defined for asym?

This comment has been minimized.

@tniessen

tniessen Nov 17, 2018

Member

It would be a completely different measure though. This seems to be related to our discussion around exposing properties of the key (such as the RSA modulus etc) via this API, maybe we should move forward in that direction?

This comment has been minimized.

@sam-github

sam-github Nov 19, 2018

Member

Why is it a completely different measure?

* Returns: {string}
For asymmetric keys, this function returns the type of the embedded key (e.g.,
`'rsa'` or `'dsa'`). This function is undefined for symmetric keys.

This comment has been minimized.

@sam-github

sam-github Nov 14, 2018

Member

There are only three types, might as well enumerate them all:

type of the key, one of 'rsa', 'dsa', or 'ec'.

Note I removed "embedded". Not sure what it refers to. Is there a non-embedded key, that you want to contrast this with? Don't think so. Its just the key .

* `cipher`: {string} If specified, the private key will be encrypted with
the given `cipher` and `passphrase` using PKCS#5 v2.0 password based
encryption.
* `passphrase`: {string | Buffer} The passphrase to use for encryption, see

This comment has been minimized.

@sam-github

sam-github Nov 14, 2018

Member

^--- I don't think we should force people to pick a cipher in order to use the API. If we do force them, we should at least recommend one in the docs. They shouldn't have googling around trying to figure out a respected cipher to use, I think that qualifies as a foot gun.

} else {
CHECK_EQ(config.format_, kKeyFormatDER);
if (config.type_.ToChecked() == kKeyEncodingPKCS1) {

This comment has been minimized.

@sam-github

sam-github Nov 14, 2018

Member

maybe just stylistic, but I'd use a switch() statement for this, and the default would be an assert(), which is basically what you did for the last else

}
}
// OpenSSL can fail to parse the key but still return a non-null pointer.

This comment has been minimized.

@sam-github
KeyObject* key = Unwrap<KeyObject>(handle.As<Object>());
CHECK(key);
// Note that this is only safe as long as the key object cannot be accessed
// by other threads.

This comment has been minimized.

@sam-github

sam-github Nov 14, 2018

Member

Is this a worthwhile comment? Doesn't all the code in node_crypto.cc fall apart if multiple threads start mucking with the ossl contexts simultaneously, or is there something specific about this object/operation?

return result;
}
static ManagedEVPPKey GetPublicKeyFromJS(

This comment has been minimized.

@sam-github

sam-github Nov 14, 2018

Member

I'm a big fan of treating acronyms as words for the purposes of CamelCase: ManagedEvppKey GetPublicKeyFromJs()... PKCS1OAEPPSSAES ... argh. Ignore if that's not the node c++ way, or you don't agree.

Show resolved Hide resolved src/node_crypto.cc
* Returns: {string}
For asymmetric keys, this function returns the type of the embedded key (e.g.,
`'rsa'` or `'dsa'`). This function is undefined for symmetric keys.

This comment has been minimized.

@jasnell

jasnell Nov 19, 2018

Member
Suggested change Beta
`'rsa'` or `'dsa'`). This function is undefined for symmetric keys.
`'rsa'` or `'dsa'`). This function is `undefined` for symmetric keys.
@jasnell

This comment has been minimized.

Member

jasnell commented Nov 19, 2018

Definite +1 on this moving forward. Code generally looking good but won't sign off until it's further along.

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