-
Notifications
You must be signed in to change notification settings - Fork 29.7k
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: extend RSA-OAEP support with oaepHash #28335
crypto: extend RSA-OAEP support with oaepHash #28335
Conversation
This adds an oaepHash option to asymmetric encryption which allows users to specify a hash function when using OAEP padding. This feature is required for interoperability with WebCrypto applications.
I'll give the branch it a try! Quick note, it would be great if the "support" for this was detectable, other than semver checking the process node version or trying and failing of course. If it would even fail with an unrecognized option. |
We don't have a unified approach for this yet.
We cannot patch this into existing Node.js releases, and I don't think we would if we could. It would most likely be a breaking change. |
I think we should somehow mark node as supporting this new API. Backporting the throwing of errors is not possible, as you say, but some constant could be exported for crypto that could be checked for existence, that would be helpful. As-is, I think that it can only be detected by a fragile comparison against node version (fragile because as the feature is backported the comparison needs changing), or a speculative RSA key gen and encrypt then decrypt operation (using decrypt failure to indicate a mismatch of digest algs). Btw, did you consider allowing the OAEP digest to be set as a property of the private key, with |
Maybe something to the effect of |
Binding this to a key object would definitely be an obstacle for re-using the same key for different decryption schemes. I prefer an encrypt/decrypt option like this, very useful, very flexible. |
I agree, but I don't think we should use ad hoc solutions for something as important as feature detection. The same problem exists for many, many other features (CCM, OCB, RSA-PSS, PEM-level encryption, GCM validation, ...), and that's only counting crypto.
I did, but then we'd have to add the
Side note: Not being able to do that is considered good design by many cryptographers. It is often considered bad practice to use the same key for different algorithms. |
I know. In this case the same exposed public RSA JWK is used to encrypt messages with JWA RSA1_5 or RSA-OAEP(-256) and the recipient decrypts those. Were it vice versa i wouldn’t mention it. For instance: An OAuth 2 exposes the public key and accepts confidental signed and encrypted authorization requests from many relying parties. These may use different JWAs. |
Using crypto constants for feature detection is pretty common, we do it lots of places. Its how TLS1.3 is detected, and I assume the existence of crypto.constants.RSA_PKCS1_OAEP_PADDING can also be used to detect support for OAEP, but not of support for this new I think it would be consistent to add a constant, for example |
@sam-github Sorry for the late reply, I appreciate your concerns.
All hash functions returned by
True, it would help, but it would be inconsistent with other APIs that accept hash functions, and since all hash functions are accepted by OpenSSL, we would either have to manually restrict that or add constants for all hash functions.
That would make feature detection more difficult in ESM since undefined "constants" cannot be imported, yet this alone would only be a minor inconvenience and could be "solved" by adding the constant to |
Neither is this and it doesn't account for backports or custom builds. const [major, minor] = process.version.substr(1).split('.').map(x => parseInt(x, 10))
if (major > 12 || (major === 12 && minor >= xx) {
// yay support!
}
|
@panva That does not contradict what I said. As I wrote before, I can add such a constant, but that does not change the fact that I don't think it is an elegant solution. I would like to hear Sam's opinion on this. If he is in favor of such a constant, I'll add it and proceed to land this. |
@tniessen not trying to contradict ;) just saying we already have a kind of established way in crypto.constants |
The Are you sure none of CCM, OCB, RSA-PSS, PEM-level encryption, GCM validation are detectable using contants? I think RSA_PKCS1_PSS_PADDING exists for PSS, and If @panva and others find it impossible to detect the existence of the feature, the feature might not be used, which would be a shame. Note that a hard coded dependency on node > 12.x.y (where x.y is where it was released) would fail to detect backports, so would assume it was unsupported even if it was. But at this time, 10.x is so far behind that maybe this will never backport, so that's OK. I will do some thinking of a more general purpose feature detection system for Node.js, but that won't happen fast, if ever. If you can't think of an OpenSSL (or OpenSSL-like) constant you'd like to expose to indicate the feature's existence (or the XOF variable length feature, which is in the same category), lets get this landed. |
Let's do it, i agree that v10 is so far behind with backports that checking major/minor is likely okay for checking the support. |
This adds an oaepHash option to asymmetric encryption which allows users to specify a hash function when using OAEP padding. This feature is required for interoperability with WebCrypto applications. PR-URL: #28335 Fixes: #25756 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Notable changes: * **crypto**: * Added an oaepHash option to asymmetric encryption which allows users to specify a hash function when using OAEP padding (Tobias Nießen) [#28335](#28335). * **deps**: * Updated V8 to 7.6.303.29 (Michaël Zasso) [#28955](#28955). * Improves the performance of various APIs such as `JSON.parse` and methods called on frozen arrays. * Adds the [`Promise.allSettled`](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Promise/allSettled) method. * Improves support of `BigInt` in `Intl` methods. * For more information: https://v8.dev/blog/v8-release-76 * Updated libuv to 1.31.0 (cjihrig) [#29070](#29070). * `UV_FS_O_FILEMAP` has been added for faster access to memory mapped files on Windows. * `uv_fs_mkdir()` now returns `UV_EINVAL` for invalid filenames on Windows. It previously returned `UV_ENOENT`. * The `uv_fs_statfs()` API has been added. * The `uv_os_environ()` and `uv_os_free_environ()` APIs have been added. * **fs**: * Added `fs.writev` and `fs.writevSync` methods. They allow to write an array of `ArrayBufferView`s to a file descriptor (Anas Aboureada) [#25925](#25925). * **http**: * Added three properties to `OutgoingMessage.prototype`: `writableObjectMode`, `writableLength` and `writableHighWaterMark` [#29018](#29018). * **stream**: * Added an new property `writableEnded` to writable streams. Its value is set to `true` after `writable.end()` has been called. (Robert Nagy) [#28934](#28934). PR-URL: #29210
Notable changes: * crypto: * Added an oaepHash option to asymmetric encryption which allows users to specify a hash function when using OAEP padding. #28335 * deps: * Updated V8 to 7.6.303.29. #28955 * Improves the performance of various APIs such as `JSON.parse` and methods called on frozen arrays. * Adds the Promise.allSettled method. * Improves support of `BigInt` in `Intl` methods. * For more information: https://v8.dev/blog/v8-release-76 * Updated libuv to 1.31.0. #29070 * `UV_FS_O_FILEMAP` has been added for faster access to memory mapped files on Windows. * `uv_fs_mkdir()` now returns `UV_EINVAL` for invalid filenames on Windows. It previously returned `UV_ENOENT`. * The `uv_fs_statfs()` API has been added. * The `uv_os_environ()` and `uv_os_free_environ()` APIs have been added. * fs: * Added `fs.writev`, `fs.writevSync` and `filehandle.writev` (promise version) methods. They allow to write an array of `ArrayBufferView`s to a file descriptor. #25925 https://github.com/nodejs/node/pull/29186/files * http: * Added three properties to `OutgoingMessage.prototype`: `writableObjectMode`, `writableLength` and `writableHighWaterMark` #29018 * stream: * Added an new property `readableEnded` to readable streams. Its value is set to `true` when the `'end'` event is emitted. #28814 * Added an new property `writableEnded` to writable streams. Its value is set to `true` after `writable.end()` has been called. #28934 PR-URL: #29210
Notable changes: * crypto: * Added an oaepHash option to asymmetric encryption which allows users to specify a hash function when using OAEP padding. nodejs#28335 * deps: * Updated V8 to 7.6.303.29. nodejs#28955 * Improves the performance of various APIs such as `JSON.parse` and methods called on frozen arrays. * Adds the Promise.allSettled method. * Improves support of `BigInt` in `Intl` methods. * For more information: https://v8.dev/blog/v8-release-76 * Updated libuv to 1.31.0. nodejs#29070 * `UV_FS_O_FILEMAP` has been added for faster access to memory mapped files on Windows. * `uv_fs_mkdir()` now returns `UV_EINVAL` for invalid filenames on Windows. It previously returned `UV_ENOENT`. * The `uv_fs_statfs()` API has been added. * The `uv_os_environ()` and `uv_os_free_environ()` APIs have been added. * fs: * Added `fs.writev`, `fs.writevSync` and `filehandle.writev` (promise version) methods. They allow to write an array of `ArrayBufferView`s to a file descriptor. nodejs#25925 https://github.com/nodejs/node/pull/29186/files * http: * Added three properties to `OutgoingMessage.prototype`: `writableObjectMode`, `writableLength` and `writableHighWaterMark` nodejs#29018 * stream: * Added an new property `readableEnded` to readable streams. Its value is set to `true` when the `'end'` event is emitted. nodejs#28814 * Added an new property `writableEnded` to writable streams. Its value is set to `true` after `writable.end()` has been called. nodejs#28934 PR-URL: nodejs#29210
Notable changes: * crypto: * Added an oaepHash option to asymmetric encryption which allows users to specify a hash function when using OAEP padding. #28335 * deps: * Updated V8 to 7.6.303.29. #28955 * Improves the performance of various APIs such as `JSON.parse` and methods called on frozen arrays. * Adds the Promise.allSettled method. * Improves support of `BigInt` in `Intl` methods. * For more information: https://v8.dev/blog/v8-release-76 * Updated libuv to 1.31.0. #29070 * `UV_FS_O_FILEMAP` has been added for faster access to memory mapped files on Windows. * `uv_fs_mkdir()` now returns `UV_EINVAL` for invalid filenames on Windows. It previously returned `UV_ENOENT`. * The `uv_fs_statfs()` API has been added. * The `uv_os_environ()` and `uv_os_free_environ()` APIs have been added. * fs: * Added `fs.writev`, `fs.writevSync` and `filehandle.writev` (promise version) methods. They allow to write an array of `ArrayBufferView`s to a file descriptor. #25925 #29186 * http: * Added three properties to `OutgoingMessage.prototype`: `writableObjectMode`, `writableLength` and `writableHighWaterMark` #29018 * stream: * Added an new property `readableEnded` to readable streams. Its value is set to `true` when the `'end'` event is emitted. #28814 * Added an new property `writableEnded` to writable streams. Its value is set to `true` after `writable.end()` has been called. #28934 PR-URL: #29210
Notable changes: * crypto: * Added an oaepHash option to asymmetric encryption which allows users to specify a hash function when using OAEP padding. #28335 * deps: * Updated V8 to 7.6.303.29. #28955 * Improves the performance of various APIs such as `JSON.parse` and methods called on frozen arrays. * Adds the Promise.allSettled method. * Improves support of `BigInt` in `Intl` methods. * For more information: https://v8.dev/blog/v8-release-76 * Updated libuv to 1.31.0. #29070 * `UV_FS_O_FILEMAP` has been added for faster access to memory mapped files on Windows. * `uv_fs_mkdir()` now returns `UV_EINVAL` for invalid filenames on Windows. It previously returned `UV_ENOENT`. * The `uv_fs_statfs()` API has been added. * The `uv_os_environ()` and `uv_os_free_environ()` APIs have been added. * fs: * Added `fs.writev`, `fs.writevSync` and `filehandle.writev` (promise version) methods. They allow to write an array of `ArrayBufferView`s to a file descriptor. #25925 #29186 * http: * Added three properties to `OutgoingMessage.prototype`: `writableObjectMode`, `writableLength` and `writableHighWaterMark` #29018 * stream: * Added an new property `readableEnded` to readable streams. Its value is set to `true` when the `'end'` event is emitted. #28814 * Added an new property `writableEnded` to writable streams. Its value is set to `true` after `writable.end()` has been called. #28934 PR-URL: #29210
This is particularly useful because CloudFront's Field Level Encryption uses RSA_OAEP_SHA256_MGF1, which this library doesn't support yet. Support for oaepHash was added in node 12.9 (nodejs/node#28335), so this won't work for older node versions. It's still a backwards compatible change because by default `oaepHash` will be undefined, as before. I've updated the tests to cover use of the new parameter, but they're not very strict because they both encrypt and decrypt using the same parameter. This means if node silently ignores the oaepHash parameter (as it will in versions < 12.9) the tests will still pass, which isn't great. On the other hand, I think this project may still be being tested on an older version of node, so perhaps the fact the tests won't break is an unexpected blessing. I've also tested this manually against AWS CloudFront's Field Level Encryption and it seems to work. Resolves aws#198
Resolves #198 This is particularly useful because CloudFront's Field Level Encryption uses RSA_OAEP_SHA256_MGF1, which this library doesn't support yet. Support for oaepHash was added in node 12.9 (nodejs/node#28335), so this won't work for older node versions. It's still a backwards compatible change because by default `oaepHash` will be undefined, as before. Added oaepHash feature detection. It is important to be prescriptive in what options will work. Node.js versions that do not support `oaepHash` will silently encrypt data. This means that the encrypted data key would not have the security properties requested. So, `oaep_hash_supported.ts` will attempt to encrypt and report the success. This will happen only once, on initialization. The integration tests have also been updated to verify OAEP test vectors based on OAEP hash support in Node.js.
This adds an
oaepHash
option to asymmetric encryption which allows users to specify a hash function when using OAEP padding. This feature is required for interoperability with WebCrypto applications.Fixes: #25756
cc @nodejs/crypto @sam-github @panva
Checklist
make -j4 test
(UNIX), orvcbuild test
(Windows) passes