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

crypto: add support for RSA-PSS keys #26960

Closed

Conversation

tniessen
Copy link
Member

This commit adds support for RSA-PSS keys, including

  • KeyObjects of type 'rsa-pss',
  • key pair generation for RSA-PSS, and
  • signing and verification using RSA-PSS keys.

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

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. labels Mar 28, 2019
@tniessen tniessen added crypto Issues and PRs related to the crypto subsystem. semver-minor PRs that contain new features and should be released in the next minor version. labels Mar 28, 2019
Copy link
Contributor

@sam-github sam-github left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the use case for this? rsa-pss keys can only come from X.509 SubjectPublicKeyInfo, AFAICT, so its not clear to me where node could get them from until we support extraction of public keys from certs, anyhow, so I guess this is prep for that? I don't think jamming "sign" parameters into a "key" is great from an API usage point of view, but since this is mirroring openssl, I guess we have to go along with it. The docs might recommend against using these key types unless required for interop. I'm puzzled by EVP_PKEY_RSA2, that's a pretty opaque name for an OpenSSL key type :-(. I'm not sure any of these concerns are addressable, given OpenSSL is our crypto lib. I left a few small change requests/reminders, but basically LGTM. I'll approve once its updated.

doc/api/crypto.md Outdated Show resolved Hide resolved
doc/api/crypto.md Outdated Show resolved Hide resolved
doc/api/crypto.md Outdated Show resolved Hide resolved
doc/api/crypto.md Show resolved Hide resolved
doc/api/crypto.md Outdated Show resolved Hide resolved
lib/internal/crypto/keygen.js Show resolved Hide resolved
src/node_crypto.cc Show resolved Hide resolved
doc/api/crypto.md Outdated Show resolved Hide resolved
test/parallel/test-crypto-key-objects.js Show resolved Hide resolved
test/parallel/test-crypto-key-objects.js Show resolved Hide resolved
@tniessen
Copy link
Member Author

What's the use case for this?

Let me ask a different question: What is the alternative to this? OpenSSL supports RSA-PSS since 1.1.1, and so does node (kind of). One can already load RSA-PSS keys, except that node will crash on asymmetricKeyType and that signing and verification don't work as expected.

rsa-pss keys can only come from X.509 SubjectPublicKeyInfo, AFAICT, so its not clear to me where node could get them from until we support extraction of public keys from certs, anyhow, so I guess this is prep for that?

You can also load RSA-PSS keys just like any other RSA key from a file, see the test scripts.

I don't think jamming "sign" parameters into a "key" is great from an API usage point of view,

I assume the motivation behind this is to prevent people and applications from using the key for anything other than the algorithm it was meant for, and, if required, with the exact parameters it was meant for. Using the same key for different algorithms is often considered bad practice. WebCrypto restricts all keys to specific parameterized algorithms IIRC.

but since this is mirroring openssl, I guess we have to go along with it.

I don't know about OpenSSL and their motivation, but I don't see many alternatives for them either. I think OpenSSL does exactly what it should do when given an RSA-PSS key.

I'm puzzled by EVP_PKEY_RSA2, that's a pretty opaque name for an OpenSSL key type :-(.

I did not add anything related to EVP_PKEY_RSA2, the code already existed, it's just an alternative OID for "normal" RSA, see #26960 (comment). The key type for RSA-PSS is EVP_PKEY_RSA_PSS.

I don't understand why OpenSSL calls this a "key type".

I think this is similar to ed25519 and X25519. They are essentially the same thing (except encoded differently), but they are used for different algorithms. We could use the same key type as we do for EVP_PKEY_RSA ('rsa'), but:

  1. Encryption and decryption would fail depending on whether the key was encoded as RSA-PSS or as rsaEncryption.
  2. It would be impossible to export the key as PKCS#1, whereas normal RSA keys allow that. Well, technically, we could make that work, but then the behavior after exporting and then importing the key would differ from the original behavior.
  3. Signing would only work with PSS padding, and the default padding would differ between RSA keys.

I think these downsides justify a separate key type, but feel free to disagree. Maybe I'm wrong and these implications aren't true!

@panva
Copy link
Member

panva commented Mar 28, 2019

I think these downsides justify a separate key type, but feel free to disagree. Maybe I'm wrong and these implications aren't true!

+1 for keeping the two separate key types.

@sam-github
Copy link
Contributor

OK, that background helps. Particularly your point about the non-x.509 transport of these kinds of keys in PEM (PKCS8 I assume). So, I've no objections. I'm also, a little, heartened that the number of EVP_PKEY types isn't endless, so the list won't grow too much larger.

Still a couple nits with the docs, but otherwise LGTM.

@tniessen
Copy link
Member Author

I'm also, a little, heartened that the number of EVP_PKEY types isn't endless, so the list won't grow too much larger.

I think it's getting too long too quickly, but I don't think there is anything we can do about it. On the brighter side, OpenSSL doesn't support much more right now, and as long as the rest of our APIs do not depend on the exact key type, we should be fine.

Still a couple nits with the docs, but otherwise LGTM.

Thank you for reviewing so quickly, I'll try to get to it ASAP.

@nodejs-github-bot
Copy link
Collaborator

doc/api/crypto.md Show resolved Hide resolved
@tniessen tniessen mentioned this pull request Mar 29, 2019
16 tasks
@BridgeAR
Copy link
Member

@tniessen this requires a rebase.

@tniessen
Copy link
Member Author

Thanks @BridgeAR, I'll rebase after #26786 lands.

This commit adds support for RSA-PSS keys, including
- KeyObjects of type rsa-pss,
- key pair generation for RSA-PSS, and
- signing and verification using RSA-PSS keys.
@tniessen tniessen force-pushed the crypto-add-support-for-rsapss-keys branch from b79c831 to 70a2145 Compare April 1, 2019 11:33
@nodejs-github-bot
Copy link
Collaborator

@tniessen
Copy link
Member Author

tniessen commented Apr 4, 2019

Ping @nodejs/crypto

@BridgeAR BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Apr 4, 2019
@danbev
Copy link
Contributor

danbev commented Apr 8, 2019

Landed in 969bd1e.

@danbev danbev closed this Apr 8, 2019
danbev pushed a commit that referenced this pull request Apr 8, 2019
This commit adds support for RSA-PSS keys, including
- KeyObjects of type rsa-pss,
- key pair generation for RSA-PSS, and
- signing and verification using RSA-PSS keys.

PR-URL: #26960
Reviewed-By: Sam Roberts <[email protected]>
Reviewed-By: Daniel Bevenius <[email protected]>
@tniessen
Copy link
Member Author

tniessen commented Apr 8, 2019

Thank you, Daniel!

agl pushed a commit to google/boringssl that referenced this pull request Oct 23, 2019
These functions are used by Node.js in
nodejs/node#26960. BoringSSL does not
support EVP_PKEY_RSA_PSS keys, so they always fail.

This simplifies building Node with BoringSSL.

Change-Id: I81c4cdba8791a60d965bc176d09e5c818153860c
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/38524
Reviewed-by: David Benjamin <[email protected]>
Commit-Queue: David Benjamin <[email protected]>
@tniessen tniessen removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jan 14, 2021
tniessen added a commit to tniessen/node that referenced this pull request Aug 31, 2021
panva pushed a commit that referenced this pull request Sep 1, 2021
Refs: #26774
Refs: #26960

PR-URL: #39963
Reviewed-By: Filip Skokan <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
targos pushed a commit that referenced this pull request Sep 6, 2021
Refs: #26774
Refs: #26960

PR-URL: #39963
Reviewed-By: Filip Skokan <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
targos pushed a commit that referenced this pull request Sep 6, 2021
Refs: #26774
Refs: #26960

PR-URL: #39963
Reviewed-By: Filip Skokan <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. crypto Issues and PRs related to the crypto subsystem. lib / src Issues and PRs related to general changes in the lib or src directory. semver-minor PRs that contain new features and should be released in the next minor version.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants