GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — crypto: introduce X509Certificate API by jasnell · Pull Request #36804 · nodejs/node

Conversation

@jasnell

@jasnell jasnell commented Jan 5, 2021 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

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)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

bricss, styfle, szmarczak, ffflorian, and bolmaster2 reacted with hooray emoji bricss, styfle, and szmarczak reacted with rocket emoji
All reactions
  • 🎉 5 reactions
  • 🚀 3 reactions
@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 involving general changes in the lib/ or src/ directories. 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
addaleax
addaleax approved these changes Jan 5, 2021
View reviewed changes

@addaleax addaleax left a comment

Copy link
Copy Markdown
Member

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 :shipit:

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

bricss reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
@jasnell jasnell added the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Jan 5, 2021
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Jan 5, 2021
@nodejs-github-bot

nodejs-github-bot commented Jan 5, 2021

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/35308/

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

panva
panva reviewed Jan 5, 2021
View reviewed changes
Comment thread doc/api/crypto.md Resolved

Uh oh!

There was an error while loading. Please reload this page.

panva
panva reviewed Jan 5, 2021
View reviewed changes
Comment thread lib/internal/crypto/x509.js Resolved

Uh oh!

There was an error while loading. Please reload this page.

panva
panva reviewed Jan 5, 2021
View reviewed changes
Comment thread lib/internal/crypto/x509.js Outdated
Comment on lines 295 to 301

@panva panva Jan 5, 2021

Copy link
Copy Markdown
Member

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?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@jasnell jasnell Jan 6, 2021

Copy link
Copy Markdown
Member Author

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)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@panva panva Jan 6, 2021

Copy link
Copy Markdown
Member

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@jasnell jasnell Jan 6, 2021

Copy link
Copy Markdown
Member Author

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@jasnell
crypto: introduce X509Certificate API
63fc892
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 <[email protected]>
@jasnell
jasnell force-pushed the x509certificate branch from f2f2da8 to 63fc892 Compare January 6, 2021 06:24
@jasnell jasnell added author ready PRs with CI started, the required approvals, and no outstanding review comments. request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. labels Jan 6, 2021
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Jan 6, 2021
@nodejs-github-bot

This comment has been minimized.

Sign in to view
@panva

panva commented Jan 6, 2021

Copy link
Copy Markdown
Member

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.

jasnell reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@nodejs-github-bot

This comment has been minimized.

Sign in to view
@nodejs-github-bot

This comment has been minimized.

Sign in to view
@jasnell

This comment has been minimized.

Sign in to view
@rvagg

This comment has been minimized.

Sign in to view
@jasnell

This comment has been minimized.

Sign in to view
@rvagg

This comment has been minimized.

Sign in to view
@Trott Trott mentioned this pull request Jan 8, 2021
privileged port issue on Raspberry Pi devices in CI #36847
Closed
@nodejs-github-bot

This comment has been minimized.

Sign in to view
@nodejs-github-bot

nodejs-github-bot commented Jan 9, 2021

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/35388/

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@nodejs-github-bot

nodejs-github-bot commented Jan 9, 2021

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/35397/

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@jasnell

jasnell commented Jan 9, 2021

Copy link
Copy Markdown
Member Author

CI is good. This is just waiting for another signoff

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

panva
panva approved these changes Jan 9, 2021
View reviewed changes
@targos targos added the dont-land-on-v14.x label Aug 8, 2021
tniessen added a commit to tniessen/node that referenced this pull request Jan 11, 2022
@tniessen
doc: correct checkHost behavior with wildcards etc
df5f073
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 tniessen mentioned this pull request Jan 11, 2022
doc: correct checkHost behavior with wildcards etc #41468
Closed
tniessen added a commit that referenced this pull request Jan 14, 2022
@tniessen
doc: correct checkHost behavior with wildcards etc
df50775
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 <[email protected]>
Reviewed-By: Filip Skokan <[email protected]>
targos pushed a commit that referenced this pull request Jan 14, 2022
@tniessen @targos
doc: correct checkHost behavior with wildcards etc
5c0c459
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 <[email protected]>
Reviewed-By: Filip Skokan <[email protected]>
@tniessen tniessen mentioned this pull request Jan 17, 2022
crypto: support RFC 2818 compatible checkHost #41569
Closed
tniessen added a commit to tniessen/node that referenced this pull request Jan 18, 2022
@tniessen
crypto: support RFC 2818 compatible checkHost
959817a
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 that referenced this pull request Jan 19, 2022
@tniessen
crypto: support RFC 2818 compatible checkHost
da1b59f
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 <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
@tniessen tniessen mentioned this pull request Jan 19, 2022
crypto: change default check(Host|Email) behavior #41600
Closed
tniessen added a commit to tniessen/node that referenced this pull request Jan 22, 2022
@tniessen
crypto: change default check(Host|Email) behavior
992e7d4
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
@tniessen @panva
crypto: change default check(Host|Email) behavior
18365d8
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 <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Filip Skokan <[email protected]>
BethGriggs pushed a commit that referenced this pull request Jan 25, 2022
@tniessen @BethGriggs
crypto: support RFC 2818 compatible checkHost
076ec47
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 <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
danielleadams pushed a commit that referenced this pull request Feb 1, 2022
@tniessen @danielleadams
doc: correct checkHost behavior with wildcards etc
d445816
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 <[email protected]>
Reviewed-By: Filip Skokan <[email protected]>
danielleadams pushed a commit to danielleadams/node that referenced this pull request Apr 21, 2022
@tniessen @danielleadams
crypto: support RFC 2818 compatible checkHost
adb88fc
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 <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
codebytere added a commit to electron/electron that referenced this pull request Oct 19, 2022
@codebytere
crypto: introduce X509Certificate API
b1d4069
nodejs/node#36804
codebytere added a commit to electron/electron that referenced this pull request Oct 24, 2022
@codebytere
crypto: introduce X509Certificate API
360bb0f
nodejs/node#36804
codebytere added a commit to electron/electron that referenced this pull request Nov 8, 2022
@codebytere
crypto: introduce X509Certificate API
9706bd6
nodejs/node#36804
codebytere added a commit to electron/electron that referenced this pull request Nov 8, 2022
@codebytere
crypto: introduce X509Certificate API
f9634a6
nodejs/node#36804
codebytere added a commit to electron/electron that referenced this pull request Nov 10, 2022
@codebytere @patchup
chore: upgrade to Node.js v18 (#35999)
75d2caf
* 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
@codebytere @patchup @khalwa
chore: upgrade to Node.js v18 (electron#35999)
b19d67a
* 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

Reviewers

@panva panva panva approved these changes
@addaleax addaleax addaleax approved these changes

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

author ready PRs with CI started, the required approvals, and no outstanding review comments. 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 involving general changes in the lib/ or src/ directories. semver-minor PRs that contain new features and should be released in the next minor version.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

6 participants

@jasnell @nodejs-github-bot @panva @rvagg @addaleax @targos
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.