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: introduce X509Certificate API #36804

Closed
wants to merge 1 commit into from

Conversation

jasnell
Copy link
Member

@jasnell jasnell commented Jan 5, 2021

Introduces the crypto.X509Certificate object.

const { X509Certificate } = require('crypto');

const x509 = new X509Certificate('{pem encoded cert}');
console.log(x509.subject);

/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)

@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 Jan 5, 2021
@jasnell jasnell 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 Jan 5, 2021
Copy link
Member

@addaleax addaleax left a 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 :shipit:

@jasnell jasnell added the request-ci Add this label to start a Jenkins CI on a PR. label Jan 5, 2021
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jan 5, 2021
@nodejs-github-bot
Copy link
Collaborator

doc/api/crypto.md Outdated Show resolved Hide resolved
Comment on lines +295 to +301
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]);
}
Copy link
Member

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?

Copy link
Member Author

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)

Copy link
Member

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

Copy link
Member Author

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>
@jasnell jasnell added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. request-ci Add this label to start a Jenkins CI on a PR. labels Jan 6, 2021
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jan 6, 2021
@nodejs-github-bot

This comment has been minimized.

@panva
Copy link
Member

panva commented Jan 6, 2021

LGTM. There are a number of edge cases with specific private keys I wish to test the checkPrivateKey method with (today/tomorrow) before this lands.

@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot

This comment has been minimized.

@jasnell

This comment has been minimized.

@rvagg

This comment has been minimized.

@jasnell

This comment has been minimized.

@rvagg

This comment has been minimized.

@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@jasnell
Copy link
Member Author

jasnell commented Jan 9, 2021

CI is good. This is just waiting for another signoff

tniessen added a commit to tniessen/node that referenced this pull request Jan 11, 2022
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
tniessen added a commit to tniessen/node that referenced this pull request Jan 11, 2022
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
tniessen added a commit that referenced this pull request Jan 14, 2022
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>
targos pushed a commit that referenced this pull request Jan 14, 2022
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>
mawaregetsuka pushed a commit to mawaregetsuka/node that referenced this pull request Jan 17, 2022
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>
tniessen added a commit to tniessen/node that referenced this pull request Jan 17, 2022
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
tniessen added a commit to tniessen/node that referenced this pull request Jan 17, 2022
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
tniessen added a commit to tniessen/node that referenced this pull request Jan 17, 2022
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
tniessen added a commit to tniessen/node that referenced this pull request Jan 18, 2022
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
thedull pushed a commit to thedull/node that referenced this pull request Jan 18, 2022
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>
tniessen added a commit that referenced this pull request Jan 19, 2022
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>
tniessen added a commit to tniessen/node that referenced this pull request Jan 19, 2022
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
tniessen added a commit to tniessen/node that referenced this pull request Jan 22, 2022
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
panva pushed a commit that referenced this pull request Jan 22, 2022
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>
BethGriggs pushed a commit that referenced this pull request Jan 25, 2022
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>
Linkgoron pushed a commit to Linkgoron/node that referenced this pull request Jan 31, 2022
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>
Linkgoron pushed a commit to Linkgoron/node that referenced this pull request Jan 31, 2022
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>
Linkgoron pushed a commit to Linkgoron/node that referenced this pull request Jan 31, 2022
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>
danielleadams pushed a commit that referenced this pull request Feb 1, 2022
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>
danielleadams pushed a commit to danielleadams/node that referenced this pull request Apr 21, 2022
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>
codebytere added a commit to electron/electron that referenced this pull request Oct 19, 2022
codebytere added a commit to electron/electron that referenced this pull request Oct 24, 2022
codebytere added a commit to electron/electron that referenced this pull request Nov 8, 2022
codebytere added a commit to electron/electron that referenced this pull request Nov 8, 2022
codebytere added a commit to electron/electron that referenced this pull request Nov 10, 2022
* 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>
khalwa pushed a commit to solarwindscloud/electron that referenced this pull request Feb 22, 2023
* 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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. 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