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

src: move some X509Certificate stuff to ncrypto #54241

Closed
wants to merge 10 commits into from

Conversation

jasnell
Copy link
Member

@jasnell jasnell commented Aug 7, 2024

Starting working on moving the core bits of X509Certificate to ncrypto... this is incomplete but it's a big set of changes so I'm splitting them up in to multiple chunks.

@jasnell jasnell requested review from anonrig and tniessen August 7, 2024 04:58
@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/crypto
  • @nodejs/security-wg

@jasnell jasnell added the request-ci Add this label to start a Jenkins CI on a PR. label Aug 7, 2024
@nodejs-github-bot nodejs-github-bot added lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. labels Aug 7, 2024
@jasnell jasnell added crypto Issues and PRs related to the crypto subsystem. c++ Issues and PRs that require attention from people who are familiar with C++. dependencies Pull requests that update a dependency file. labels Aug 7, 2024
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Aug 7, 2024
@nodejs-github-bot

This comment was marked as outdated.

This comment was marked as outdated.

@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot

This comment was marked as outdated.

deps/ncrypto/engine.cc Show resolved Hide resolved
deps/ncrypto/ncrypto.cc Show resolved Hide resolved
src/crypto/crypto_x509.cc Outdated Show resolved Hide resolved
src/crypto/crypto_x509.cc Show resolved Hide resolved
src/crypto/crypto_x509.cc Show resolved Hide resolved
src/crypto/crypto_x509.h Outdated Show resolved Hide resolved
@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Aug 7, 2024

@jasnell jasnell added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. and removed needs-ci PRs that need a full CI run. labels Aug 8, 2024
@anonrig anonrig requested review from mcollina and benjamingr August 12, 2024 15:13
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

RSLGTM as long as CI passes

@jasnell jasnell added the commit-queue Add this label to land a pull request using GitHub Actions. label Aug 12, 2024
@nodejs-github-bot nodejs-github-bot added commit-queue-failed An error occurred while landing this pull request using GitHub Actions. and removed commit-queue Add this label to land a pull request using GitHub Actions. labels Aug 12, 2024
@nodejs-github-bot
Copy link
Collaborator

Commit Queue failed
- Loading data for nodejs/node/pull/54241
✔  Done loading data for nodejs/node/pull/54241
----------------------------------- PR info ------------------------------------
Title      src: move some X509Certificate stuff to ncrypto (#54241)
Author     James M Snell <jasnell@gmail.com> (@jasnell)
Branch     jasnell:use-ncrypto-moar -> nodejs:main
Labels     crypto, c++, lib / src, author ready, dependencies
Commits    10
 - src: move some X509Certificate stuff to ncrypto
 - src: shift more crypto X509 to ncrypto
 - src: shift more x509 to ncrypto
 - src: shift more x509 to ncrypto
 - src: shift even more x509 to ncrypto
 - src: shift more x509 to ncrypto
 - src: fixup crypto linting after x509 changes
 - src: fixup includes after x509 changes
 - src: fixups after code review
 - src: address nits
Committers 1
 - James M Snell <jasnell@gmail.com>
PR-URL: https://github.com/nodejs/node/pull/54241
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/54241
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
--------------------------------------------------------------------------------
   ℹ  This PR was created on Wed, 07 Aug 2024 04:58:43 GMT
   ✔  Approvals: 2
   ✔  - Yagiz Nizipli (@anonrig) (TSC): https://github.com/nodejs/node/pull/54241#pullrequestreview-2225624836
   ✔  - Matteo Collina (@mcollina) (TSC): https://github.com/nodejs/node/pull/54241#pullrequestreview-2233450678
   ✔  Last GitHub CI successful
   ℹ  Last Full PR CI on 2024-08-07T22:13:52Z: https://ci.nodejs.org/job/node-test-pull-request/60953/
- Querying data for job/node-test-pull-request/60953/
   ✔  Last Jenkins CI successful
--------------------------------------------------------------------------------
   ✔  No git cherry-pick in progress
   ✔  No git am in progress
   ✔  No git rebase in progress
--------------------------------------------------------------------------------
- Bringing origin/main up to date...
From https://github.com/nodejs/node
 * branch                  main       -> FETCH_HEAD
✔  origin/main is now up-to-date
- Downloading patch for 54241
From https://github.com/nodejs/node
 * branch                  refs/pull/54241/merge -> FETCH_HEAD
✔  Fetched commits as 624db509520a..083bad70e707
--------------------------------------------------------------------------------
[main 4aa5f71b56] src: move some X509Certificate stuff to ncrypto
 Author: James M Snell <jasnell@gmail.com>
 Date: Mon Aug 5 16:24:51 2024 -0700
 2 files changed, 43 insertions(+), 1 deletion(-)
[main 8f81e293dd] src: shift more crypto X509 to ncrypto
 Author: James M Snell <jasnell@gmail.com>
 Date: Tue Aug 6 19:28:20 2024 -0700
 4 files changed, 103 insertions(+), 20 deletions(-)
[main 9b78f096ba] src: shift more x509 to ncrypto
 Author: James M Snell <jasnell@gmail.com>
 Date: Tue Aug 6 20:43:10 2024 -0700
 4 files changed, 210 insertions(+), 69 deletions(-)
[main 0e14dbdf2d] src: shift more x509 to ncrypto
 Author: James M Snell <jasnell@gmail.com>
 Date: Tue Aug 6 21:12:59 2024 -0700
 6 files changed, 125 insertions(+), 85 deletions(-)
[main df19fced42] src: shift even more x509 to ncrypto
 Author: James M Snell <jasnell@gmail.com>
 Date: Tue Aug 6 21:39:11 2024 -0700
 4 files changed, 138 insertions(+), 96 deletions(-)
[main 27eb278981] src: shift more x509 to ncrypto
 Author: James M Snell <jasnell@gmail.com>
 Date: Tue Aug 6 21:54:26 2024 -0700
 4 files changed, 48 insertions(+), 49 deletions(-)
[main 6d166ebcb3] src: fixup crypto linting after x509 changes
 Author: James M Snell <jasnell@gmail.com>
 Date: Tue Aug 6 21:55:02 2024 -0700
 2 files changed, 45 insertions(+), 45 deletions(-)
[main b268532cfd] src: fixup includes after x509 changes
 Author: James M Snell <jasnell@gmail.com>
 Date: Wed Aug 7 06:59:26 2024 -0700
 4 files changed, 9 insertions(+), 14 deletions(-)
[main 947f98a9f2] src: fixups after code review
 Author: James M Snell <jasnell@gmail.com>
 Date: Wed Aug 7 07:52:31 2024 -0700
 1 file changed, 1 insertion(+), 3 deletions(-)
[main 8272776189] src: address nits
 Author: James M Snell <jasnell@gmail.com>
 Date: Wed Aug 7 08:43:42 2024 -0700
 2 files changed, 4 insertions(+), 4 deletions(-)
   ✔  Patches applied
There are 10 commits in the PR. Attempting autorebase.
Rebasing (2/20)

Executing: git node land --amend --yes
--------------------------------- New Message ----------------------------------
src: move some X509Certificate stuff to ncrypto

PR-URL: #54241
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

[detached HEAD 138db0c245] src: move some X509Certificate stuff to ncrypto
Author: James M Snell <jasnell@gmail.com>
Date: Mon Aug 5 16:24:51 2024 -0700
2 files changed, 43 insertions(+), 1 deletion(-)
Rebasing (3/20)
Rebasing (4/20)

Executing: git node land --amend --yes
--------------------------------- New Message ----------------------------------
src: shift more crypto X509 to ncrypto

PR-URL: #54241
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

[detached HEAD d866d08687] src: shift more crypto X509 to ncrypto
Author: James M Snell <jasnell@gmail.com>
Date: Tue Aug 6 19:28:20 2024 -0700
4 files changed, 103 insertions(+), 20 deletions(-)
Rebasing (5/20)
Rebasing (6/20)

Executing: git node land --amend --yes
--------------------------------- New Message ----------------------------------
src: shift more x509 to ncrypto

PR-URL: #54241
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

[detached HEAD b95758585b] src: shift more x509 to ncrypto
Author: James M Snell <jasnell@gmail.com>
Date: Tue Aug 6 20:43:10 2024 -0700
4 files changed, 210 insertions(+), 69 deletions(-)
Rebasing (7/20)
Rebasing (8/20)

Executing: git node land --amend --yes
--------------------------------- New Message ----------------------------------
src: shift more x509 to ncrypto

PR-URL: #54241
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

[detached HEAD 31206a134f] src: shift more x509 to ncrypto
Author: James M Snell <jasnell@gmail.com>
Date: Tue Aug 6 21:12:59 2024 -0700
6 files changed, 125 insertions(+), 85 deletions(-)
Rebasing (9/20)
Rebasing (10/20)

Executing: git node land --amend --yes
--------------------------------- New Message ----------------------------------
src: shift even more x509 to ncrypto

PR-URL: #54241
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

[detached HEAD 6ea876450c] src: shift even more x509 to ncrypto
Author: James M Snell <jasnell@gmail.com>
Date: Tue Aug 6 21:39:11 2024 -0700
4 files changed, 138 insertions(+), 96 deletions(-)
Rebasing (11/20)
Rebasing (12/20)

Executing: git node land --amend --yes
--------------------------------- New Message ----------------------------------
src: shift more x509 to ncrypto

PR-URL: #54241
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

[detached HEAD 2cabbd704b] src: shift more x509 to ncrypto
Author: James M Snell <jasnell@gmail.com>
Date: Tue Aug 6 21:54:26 2024 -0700
4 files changed, 48 insertions(+), 49 deletions(-)
Rebasing (13/20)
Rebasing (14/20)

Executing: git node land --amend --yes
--------------------------------- New Message ----------------------------------
src: fixup crypto linting after x509 changes

PR-URL: #54241
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

[detached HEAD 5eecf9d324] src: fixup crypto linting after x509 changes
Author: James M Snell <jasnell@gmail.com>
Date: Tue Aug 6 21:55:02 2024 -0700
2 files changed, 45 insertions(+), 45 deletions(-)
Rebasing (15/20)
Rebasing (16/20)

Executing: git node land --amend --yes
--------------------------------- New Message ----------------------------------
src: fixup includes after x509 changes

PR-URL: #54241
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

[detached HEAD 1e73f13ae5] src: fixup includes after x509 changes
Author: James M Snell <jasnell@gmail.com>
Date: Wed Aug 7 06:59:26 2024 -0700
4 files changed, 9 insertions(+), 14 deletions(-)
Rebasing (17/20)
Rebasing (18/20)

Executing: git node land --amend --yes
--------------------------------- New Message ----------------------------------
src: fixups after code review

PR-URL: #54241
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

[detached HEAD d5c10d06ca] src: fixups after code review
Author: James M Snell <jasnell@gmail.com>
Date: Wed Aug 7 07:52:31 2024 -0700
1 file changed, 1 insertion(+), 3 deletions(-)
Rebasing (19/20)
Rebasing (20/20)

Executing: git node land --amend --yes
--------------------------------- New Message ----------------------------------
src: address nits

PR-URL: #54241
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

[detached HEAD eed729e2b2] src: address nits
Author: James M Snell <jasnell@gmail.com>
Date: Wed Aug 7 08:43:42 2024 -0700
2 files changed, 4 insertions(+), 4 deletions(-)

Successfully rebased and updated refs/heads/main.

ℹ Add commit-queue-squash label to land the PR as one commit, or commit-queue-rebase to land as separate commits.

https://github.com/nodejs/node/actions/runs/10357382875

jasnell added a commit that referenced this pull request Aug 12, 2024
PR-URL: #54241
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
@jasnell
Copy link
Member Author

jasnell commented Aug 12, 2024

Landed in 5f230d2

@jasnell jasnell closed this Aug 12, 2024
@jasnell jasnell removed the commit-queue-failed An error occurred while landing this pull request using GitHub Actions. label Aug 12, 2024
targos pushed a commit that referenced this pull request Aug 14, 2024
PR-URL: #54241
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
targos pushed a commit that referenced this pull request Aug 14, 2024
PR-URL: #54241
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
@RafaelGSS RafaelGSS mentioned this pull request Aug 19, 2024
@targos targos added the dont-land-on-v20.x PRs that should not land on the v20.x-staging branch and should not be released in v20.x. label Sep 21, 2024
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. dependencies Pull requests that update a dependency file. dont-land-on-v20.x PRs that should not land on the v20.x-staging branch and should not be released in v20.x. lib / src Issues and PRs related to general changes in the lib or src directory.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants