-
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: introduce X509Certificate API #36804
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not a crypto expert but the code LGTM
checkPrivateKey(pkey) { | ||
if (!isKeyObject(pkey)) | ||
throw new ERR_INVALID_ARG_TYPE('pkey', 'KeyObject', pkey); | ||
if (pkey.type !== 'private') | ||
throw new ERR_INVALID_ARG_VALUE('pkey', pkey); | ||
return this[kHandle].checkPrivateKey(pkey[kHandle]); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I assume this compares the public key contained in the cert with the one derived from the provided private key?
There is a use-case in my mind coming from the JWK specification for checking that a certificate's public key is consistent with a public key obtained through different syntax. Would the underlying implementation accommodate if pkey.type
was anything but secret
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, this checks that the given private key matches the certs public key.
For the public-to-public key check use case, we should likely add methods directly to KeyObject
for checking that (in a separate PR)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So isn't this method redundant with a future PR extending the KeyObject API in place? Given that one can get a public KeyObject using crypto.createPublicKey with privateKey as an argument
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given that the other method doesn't exist yet, no ;-) ... if/when we get a method on KeyObject
to check consistency we can update this to use that if necessary.
Introduces the `crypto.X509Certificate` object. ```js const { X509Certificate } = require('crypto'); const x509 = new X509Certificate('{pem encoded cert}'); console.log(x509.subject); ``` Fixes: nodejs#29181 Signed-off-by: James M Snell <jasnell@gmail.com>
f2f2da8
to
63fc892
Compare
This comment has been minimized.
This comment has been minimized.
LGTM. There are a number of edge cases with specific private keys I wish to test the |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
CI is good. This is just waiting for another signoff |
The current documentation is inaccurate in that checkHost does not necessarily return the given host name, but instead returns the subject name that matched the given host name. Refs: nodejs#36804
The current documentation is inaccurate in that checkHost does not necessarily return the given host name, but instead returns the subject name that matched the given host name. Refs: nodejs#36804
The current documentation is inaccurate in that checkHost does not necessarily return the given host name, but instead returns the subject name that matched the given host name. Refs: #36804 PR-URL: #41468 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Filip Skokan <panva.ip@gmail.com>
The current documentation is inaccurate in that checkHost does not necessarily return the given host name, but instead returns the subject name that matched the given host name. Refs: #36804 PR-URL: #41468 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Filip Skokan <panva.ip@gmail.com>
The current documentation is inaccurate in that checkHost does not necessarily return the given host name, but instead returns the subject name that matched the given host name. Refs: nodejs#36804 PR-URL: nodejs#41468 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Filip Skokan <panva.ip@gmail.com>
The 'subject' option should not only accept the values 'always' and 'never' because neither is compatible with RFC 2818, i.e., HTTPS. This change adds a third value 'default', which implies the behavior that HTTPS mandates. The new 'default' case matches the default behavior of OpenSSL for both DNS names and email addresses. Future Node.js versions should change the default option value from 'always' to 'default'. Refs: nodejs#36804
The 'subject' option should not only accept the values 'always' and 'never' because neither is compatible with RFC 2818, i.e., HTTPS. This change adds a third value 'default', which implies the behavior that HTTPS mandates. The new 'default' case matches the default behavior of OpenSSL for both DNS names and email addresses. Future Node.js versions should change the default option value from 'always' to 'default'. Refs: nodejs#36804
The 'subject' option should not only accept the values 'always' and 'never' because neither is compatible with RFC 2818, i.e., HTTPS. This change adds a third value 'default', which implies the behavior that HTTPS mandates. The new 'default' case matches the default behavior of OpenSSL for both DNS names and email addresses. Future Node.js versions should change the default option value from 'always' to 'default'. Refs: nodejs#36804
The 'subject' option should not only accept the values 'always' and 'never' because neither is compatible with RFC 2818, i.e., HTTPS. This change adds a third value 'default', which implies the behavior that HTTPS mandates. The new 'default' case matches the default behavior of OpenSSL for both DNS names and email addresses. Future Node.js versions should change the default option value from 'always' to 'default'. Refs: nodejs#36804
The current documentation is inaccurate in that checkHost does not necessarily return the given host name, but instead returns the subject name that matched the given host name. Refs: nodejs#36804 PR-URL: nodejs#41468 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Filip Skokan <panva.ip@gmail.com>
The 'subject' option should not only accept the values 'always' and 'never' because neither is compatible with RFC 2818, i.e., HTTPS. This change adds a third value 'default', which implies the behavior that HTTPS mandates. The new 'default' case matches the default behavior of OpenSSL for both DNS names and email addresses. Future Node.js versions should change the default option value from 'always' to 'default'. Refs: #36804 PR-URL: #41569 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
This changes the default behavior of the X509Certificate functions checkHost and checkEmail to match the default behavior of OpenSSL's X509_check_host and X509_check_email functions, respectively, which is also what RFC 2818 mandates for HTTPS. Refs: nodejs#36804 Refs: nodejs#41569
This changes the default behavior of the X509Certificate functions checkHost and checkEmail to match the default behavior of OpenSSL's X509_check_host and X509_check_email functions, respectively, which is also what RFC 2818 mandates for HTTPS. Refs: nodejs#36804 Refs: nodejs#41569
This changes the default behavior of the X509Certificate functions checkHost and checkEmail to match the default behavior of OpenSSL's X509_check_host and X509_check_email functions, respectively, which is also what RFC 2818 mandates for HTTPS. Refs: #36804 Refs: #41569 PR-URL: #41600 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Filip Skokan <panva.ip@gmail.com>
The 'subject' option should not only accept the values 'always' and 'never' because neither is compatible with RFC 2818, i.e., HTTPS. This change adds a third value 'default', which implies the behavior that HTTPS mandates. The new 'default' case matches the default behavior of OpenSSL for both DNS names and email addresses. Future Node.js versions should change the default option value from 'always' to 'default'. Refs: #36804 PR-URL: #41569 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
The current documentation is inaccurate in that checkHost does not necessarily return the given host name, but instead returns the subject name that matched the given host name. Refs: nodejs#36804 PR-URL: nodejs#41468 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Filip Skokan <panva.ip@gmail.com>
The 'subject' option should not only accept the values 'always' and 'never' because neither is compatible with RFC 2818, i.e., HTTPS. This change adds a third value 'default', which implies the behavior that HTTPS mandates. The new 'default' case matches the default behavior of OpenSSL for both DNS names and email addresses. Future Node.js versions should change the default option value from 'always' to 'default'. Refs: nodejs#36804 PR-URL: nodejs#41569 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
This changes the default behavior of the X509Certificate functions checkHost and checkEmail to match the default behavior of OpenSSL's X509_check_host and X509_check_email functions, respectively, which is also what RFC 2818 mandates for HTTPS. Refs: nodejs#36804 Refs: nodejs#41569 PR-URL: nodejs#41600 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Filip Skokan <panva.ip@gmail.com>
The current documentation is inaccurate in that checkHost does not necessarily return the given host name, but instead returns the subject name that matched the given host name. Refs: #36804 PR-URL: #41468 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Filip Skokan <panva.ip@gmail.com>
The 'subject' option should not only accept the values 'always' and 'never' because neither is compatible with RFC 2818, i.e., HTTPS. This change adds a third value 'default', which implies the behavior that HTTPS mandates. The new 'default' case matches the default behavior of OpenSSL for both DNS names and email addresses. Future Node.js versions should change the default option value from 'always' to 'default'. Refs: nodejs#36804 PR-URL: nodejs#41569 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
* chore: update to Node.js v18 * child_process: improve argument validation nodejs/node#41305 * bootstrap: support configure-time user-land snapshot nodejs/node#42466 * chore: update GN patch * src: disambiguate terms used to refer to builtins and addons nodejs/node#44135 * src: use a typed array internally for process._exiting nodejs/node#43883 * chore: lib/internal/bootstrap -> lib/internal/process * src: disambiguate terms used to refer to builtins and addons nodejs/node#44135 * chore: remove redudant browserGlobals patch * chore: update BoringSSL patch * src: allow embedder-provided PageAllocator in NodePlatform nodejs/node#38362 * chore: fixup Node.js crypto tests - nodejs/node#44171 - nodejs/node#41600 * lib: add Promise methods to avoid-prototype-pollution lint rule nodejs/node#43849 * deps: update V8 to 10.1 nodejs/node#42657 * src: add kNoBrowserGlobals flag for Environment nodejs/node#40532 * chore: consolidate asar initialization patches * deps: update V8 to 10.1 nodejs/node#42657 * deps: update V8 to 9.8 nodejs/node#41610 * src,crypto: remove AllocatedBuffers from crypto_spkac nodejs/node#40752 * build: enable V8's shared read-only heap nodejs/node#42809 * src: fix ssize_t error from nghttp2.h nodejs/node#44393 * chore: fixup ESM patch * chore: fixup patch indices * src: merge NativeModuleEnv into NativeModuleLoader nodejs/node#43824 * [API] Pass OOMDetails to OOMErrorCallback https://chromium-review.googlesource.com/c/v8/v8/+/3647827 * src: iwyu in cleanup_queue.cc * src: return Maybe from a couple of functions nodejs/node#39603 * src: clean up embedder API nodejs/node#35897 * src: refactor DH groups to delete crypto_groups.h nodejs/node#43896 * deps,src: use SIMD for normal base64 encoding nodejs/node#39775 * chore: remove deleted source file * chore: update patches * chore: remove deleted source file * lib: add fetch nodejs/node#41749 * chore: remove nonexistent node specs * test: split report OOM tests nodejs/node#44389 * src: trace fs async api nodejs/node#44057 * http: trace http request / response nodejs/node#44102 * test: split test-crypto-dh.js nodejs/node#40451 * crypto: introduce X509Certificate API nodejs/node#36804 * src: split property helpers from node::Environment nodejs/node#44056 * nodejs/node#38905 bootstrap: implement run-time user-land snapshots via --build-snapshot and --snapshot-blob * lib,src: implement WebAssembly Web API nodejs/node#42701 * fixup! deps,src: use SIMD for normal base64 encoding * fixup! src: refactor DH groups to delete crypto_groups.h * chore: fixup base64 GN file * fix: check that node::InitializeContext() returns true * chore: delete _noBrowserGlobals usage * chore: disable fetch in renderer procceses * dns: default to verbatim=true in dns.lookup() nodejs/node#39987 Co-authored-by: PatchUp <73610968+patchup[bot]@users.noreply.github.com>
* chore: update to Node.js v18 * child_process: improve argument validation nodejs/node#41305 * bootstrap: support configure-time user-land snapshot nodejs/node#42466 * chore: update GN patch * src: disambiguate terms used to refer to builtins and addons nodejs/node#44135 * src: use a typed array internally for process._exiting nodejs/node#43883 * chore: lib/internal/bootstrap -> lib/internal/process * src: disambiguate terms used to refer to builtins and addons nodejs/node#44135 * chore: remove redudant browserGlobals patch * chore: update BoringSSL patch * src: allow embedder-provided PageAllocator in NodePlatform nodejs/node#38362 * chore: fixup Node.js crypto tests - nodejs/node#44171 - nodejs/node#41600 * lib: add Promise methods to avoid-prototype-pollution lint rule nodejs/node#43849 * deps: update V8 to 10.1 nodejs/node#42657 * src: add kNoBrowserGlobals flag for Environment nodejs/node#40532 * chore: consolidate asar initialization patches * deps: update V8 to 10.1 nodejs/node#42657 * deps: update V8 to 9.8 nodejs/node#41610 * src,crypto: remove AllocatedBuffers from crypto_spkac nodejs/node#40752 * build: enable V8's shared read-only heap nodejs/node#42809 * src: fix ssize_t error from nghttp2.h nodejs/node#44393 * chore: fixup ESM patch * chore: fixup patch indices * src: merge NativeModuleEnv into NativeModuleLoader nodejs/node#43824 * [API] Pass OOMDetails to OOMErrorCallback https://chromium-review.googlesource.com/c/v8/v8/+/3647827 * src: iwyu in cleanup_queue.cc * src: return Maybe from a couple of functions nodejs/node#39603 * src: clean up embedder API nodejs/node#35897 * src: refactor DH groups to delete crypto_groups.h nodejs/node#43896 * deps,src: use SIMD for normal base64 encoding nodejs/node#39775 * chore: remove deleted source file * chore: update patches * chore: remove deleted source file * lib: add fetch nodejs/node#41749 * chore: remove nonexistent node specs * test: split report OOM tests nodejs/node#44389 * src: trace fs async api nodejs/node#44057 * http: trace http request / response nodejs/node#44102 * test: split test-crypto-dh.js nodejs/node#40451 * crypto: introduce X509Certificate API nodejs/node#36804 * src: split property helpers from node::Environment nodejs/node#44056 * nodejs/node#38905 bootstrap: implement run-time user-land snapshots via --build-snapshot and --snapshot-blob * lib,src: implement WebAssembly Web API nodejs/node#42701 * fixup! deps,src: use SIMD for normal base64 encoding * fixup! src: refactor DH groups to delete crypto_groups.h * chore: fixup base64 GN file * fix: check that node::InitializeContext() returns true * chore: delete _noBrowserGlobals usage * chore: disable fetch in renderer procceses * dns: default to verbatim=true in dns.lookup() nodejs/node#39987 Co-authored-by: PatchUp <73610968+patchup[bot]@users.noreply.github.com>
Introduces the
crypto.X509Certificate
object./cc @addaleax @panva @nodejs/crypto
(note.. this PR does not do it yet, but an eventual goal is to update the existing APIs on TLSSocket for getting the cert and peercert so that they return
X509Certificate
instances instead of the legacy objects)