crypto: add RSA encryption #7561
Conversation
|
Thank you for contributing this pull request! Here are a few pointers to make sure your submission will be considered for inclusion. The following commiters were not found in the CLA:
You can fix all these things without opening another issue. Please see CONTRIBUTING.md for more information |
|
There is no control over encryption algorithm? It doesn't even support the various RSA PKCS#1 algorithms, much less any DH/ECDH. |
|
@sam-github not sure what you mean by "encryption algorithm". If you mean padding, which is the only configurable thing in RSA encryption, then yes, it's currently hard-coded to |
lib/crypto.js
Outdated
There was a problem hiding this comment.
In fact it works only with RSA, right? Why not just name it explicitly?
There was a problem hiding this comment.
Is there anything other than RSA that you can public-encrypt with? If yes, then this function should support that as well. If not, then there is no ambiguity.
There was a problem hiding this comment.
Ok, you have a point. Please continue and add tests.
|
Seems to be looking good otherwise! |
|
Added privateDecrypt and tests. |
|
By the way, I'm open to suggestions regarding the API, since now would be the best time to discuss these things. If there aren't any, I'll add docs. |
|
@seishun, you are correct, I forgot DH crypto-systems don't support encryption as directly as RSA. And yes, I mean padding, or schemes as PKCS#1 calls them. There are two RSA encryption schemes (RSAES-OAEP and RSAES-PKCS1-v1_5) defined by PKCS#1, and an API that doesn't support both is a problem waiting to happen, IMO. And while OAEP should probably be default, if there is a default, the v1.5 support should be there, too. Putting RSA in the function name, since it will only ever support RSA, might help prevent bug reports about the API not working with DH/ECDH keys. I'd also suggest adding a negative test to 05b2cf4 with a DH key... it should return some useful message (and not abort). |
|
A negative test with input larger than allowed might be useful, too. Again, I'm anticipating users not knowing that naive use of RSA encryption is probably not what they want to be using, and trying to encrypt with data larger than the padding/key size allows. |
Do you have an idea for the API to set the padding?
"RSAPublicEncrypt"? We could just document that
Currently it throws with an OpenSSL error in both cases. I'm not sure if we need tests for that though, I don't see tests for trying to sign with a public key for example. |
|
I don't really enjoy two things about it:
I understand that key parsing has probably neglectable performance impact, considering the heaviness of the encryption/decryption phases, but still wasting time on every call is not something that I would wish |
Well, Sign and Verify do the same – if you want to sign or verify multiple chunks of data, it will have to parse the same key each time. If we do deviate from this for RSA encryption, do you have an idea for the API? We could have a pair of classes like PublicEncrypt/PrivateDecrypt, but that again would be inconsistent with Sign/Verify since the latter are not reusable. Maybe we should change the API of Sign/Verify to allow specifying the key at the construction, and allow reusing them? |
|
One step at the time, let's see what we could do with RSA thing. Perhaps the classes should be named PublicKey and PrivateKey instead. |
|
I think if we call them PublicKey and PrivateKey, people might erroneously assume you can pass their instances to sign.sign and verifier.verify instead of PEM strings. |
|
Yeah... @tjfontaine and @trevnorris I really need to know your opinion on it guys. Perhaps it would be fine if crypto will accept |
I don't like this either. I feel like it would make things inconsistent and confusing: to verify something, you have to pass a PEM string or a |
|
Ok, let's just make it accept strings for now, you are right now. |
|
I guess it is LGTM then, @tjfontaine LGTY? |
|
@indutny will this become merged? v13 or earlier? |
|
@seishun could you please add some docs? |
|
@indutny done! |
src/node_crypto.cc
Outdated
There was a problem hiding this comment.
Could these functions be generic templates? I think we could use 2 functions instead of 4 here.
There was a problem hiding this comment.
@indutny I'm confused, which functions do you propose to combine?
There was a problem hiding this comment.
PublicEncrypt and PublicDecrypt both forms.
|
@seishun does it makes sense? Also, feel free to ask me anything :) I'd like to get this landed before v0.12! |
|
@indutny yes, it makes sense, and I have a question. Does publicEncrypt need to support keys encoded using a RSAPublicKey structure (the PEM header is "BEGIN RSA PUBLIC KEY"), or is it enough to just support keys encoded using a SubjectPublicKeyInfo structure (the PEM header is "BEGIN PUBLIC KEY")? Currently it supports both, but it makes it harder to combine the PublicEncrypt and PrivateDecrypt functions. OpenSSL generates SubjectPublicKeyInfo keys by default, and it's trivial to convert between the two formats. |
|
It'd be better to support both keys. |
|
Done. I couldn't think of a way to combine the rest of PublicEncrypt and PrivateDecrypt due to the aforementioned difference in PEM handling. Also let me know if you have a better idea for the name of the combined function. Another idea was to read both public and private keys in PKEYCipher. However, it turned out that OpenSSL segfaults when attempting to use EVP_PKEY_decrypt with a public key, rather than return an error code. |
src/node_crypto.cc
Outdated
There was a problem hiding this comment.
Could you please declare this type in a typedef right above this line?
There was a problem hiding this comment.
Which type, of EVP_PKEY_cipher_init or EVP_PKEY_cipher?
src/node_crypto.cc
Outdated
There was a problem hiding this comment.
Now, to make it even better, let's create a class in node_crypto.h called PublicKeyCipher and move these types and functions to it ;) You could remove PKEY prefix after that.
src/node_crypto.cc
Outdated
There was a problem hiding this comment.
Why not move it to a class declaration?
There was a problem hiding this comment.
Move what, the template definition?
There was a problem hiding this comment.
No, type declaration. But if you'll merge two functions above, this won't be needed.
|
I like it, thank you very much! :) It is going to be even more awesome if you'll fix the rest of the issues ;) |
|
Done, hope I didn't miss anything. |
src/node_crypto.cc
Outdated
There was a problem hiding this comment.
Please move the strncmp to the next line.
|
@seishun thank you! :) This is totally becoming a very nice thing |
|
Done! |
|
LGTM, thank you! |
Continuing from #7359.
Just public encryption for now, since this is my personal use case that I can test in real life (and it works fine). I would like to receive feedback on this part before I continue.
The API is simple:
crypto.publicEncrypt(key, buf)returns an encrypted buffer, wherebufis the buffer to encrypt andkeyis the public key in PEM format.