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,crypto: refactoring of crypto_context, SecureContext #35665

Closed
wants to merge 1 commit into from

Conversation

jasnell
Copy link
Member

@jasnell jasnell commented Oct 15, 2020

Cleaup and improvement of crypto_context and SecureContext.

This is semver-major primarily because of changes to error messages.

Signed-off-by: James M Snell [email protected]

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. tls Issues and PRs related to the tls subsystem. labels Oct 15, 2020
@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/crypto
  • @nodejs/quic

@jasnell jasnell added crypto Issues and PRs related to the crypto subsystem. errors Issues and PRs related to JavaScript errors originated in Node.js core. request-ci Add this label to start a Jenkins CI on a PR. labels Oct 15, 2020
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Oct 16, 2020
@nodejs-github-bot
Copy link
Collaborator

@jasnell jasnell added the request-ci Add this label to start a Jenkins CI on a PR. label Oct 16, 2020
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Oct 16, 2020
@nodejs-github-bot

This comment has been minimized.

lib/_tls_common.js Outdated Show resolved Hide resolved
lib/_tls_common.js Show resolved Hide resolved
@nodejs-github-bot

This comment has been minimized.

@Trott Trott added the request-ci Add this label to start a Jenkins CI on a PR. label Oct 18, 2020
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Oct 18, 2020
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@jasnell jasnell force-pushed the crypto-cleanup-securecontext branch from 30ac425 to 1f8120c Compare November 4, 2020 00:15
@jasnell jasnell added the request-ci Add this label to start a Jenkins CI on a PR. label Nov 4, 2020
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Nov 4, 2020
@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot
Copy link
Collaborator

@jasnell jasnell force-pushed the crypto-cleanup-securecontext branch from 1f8120c to 33e7a62 Compare November 6, 2020 22:04
@jasnell jasnell added the request-ci Add this label to start a Jenkins CI on a PR. label Nov 6, 2020
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Nov 6, 2020
@nodejs-github-bot

This comment has been minimized.

@jasnell jasnell added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Nov 6, 2020
@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot
Copy link
Collaborator

@aduh95 aduh95 added the commit-queue Add this label to land a pull request using GitHub Actions. label Nov 10, 2020
@github-actions github-actions bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Nov 10, 2020
@github-actions
Copy link
Contributor

Commit Queue failed
- Loading data for nodejs/node/pull/35665
✔  Done loading data for nodejs/node/pull/35665
----------------------------------- PR info ------------------------------------
Title      src,crypto: refactoring of crypto_context, SecureContext (#35665)
Author     James M Snell  (@jasnell)
Branch     jasnell:crypto-cleanup-securecontext -> nodejs:master
Labels     C++, author ready, crypto, errors, tls
Commits    1
 - src,crypto: refactoring of crypto_context, SecureContext
Committers 1
 - James M Snell 
PR-URL: https://github.com/nodejs/node/pull/35665
Reviewed-By: Alba Mendez 
Reviewed-By: Tobias Nießen 
Reviewed-By: Rich Trott 
Reviewed-By: Franziska Hinkelmann 
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/35665
Reviewed-By: Alba Mendez 
Reviewed-By: Tobias Nießen 
Reviewed-By: Rich Trott 
Reviewed-By: Franziska Hinkelmann 
--------------------------------------------------------------------------------
   ⚠  Commits were pushed since the last review:
   ⚠  - src,crypto: refactoring of crypto_context, SecureContext
   ✔  Last GitHub Actions successful
   ℹ  Last Full PR CI on 2020-11-10T09:45:35Z: https://ci.nodejs.org/job/node-test-pull-request/34275/
- Querying data for job/node-test-pull-request/34275/
✔  Build data downloaded
   ✔  Last Jenkins CI successful
   ℹ  This PR was created on Thu, 15 Oct 2020 23:54:44 GMT
   ✔  Approvals: 4
   ✔  - Alba Mendez (@mildsunrise): https://github.com/nodejs/node/pull/35665#pullrequestreview-510987295
   ✔  - Tobias Nießen (@tniessen) (TSC): https://github.com/nodejs/node/pull/35665#pullrequestreview-511000285
   ✔  - Rich Trott (@Trott) (TSC): https://github.com/nodejs/node/pull/35665#pullrequestreview-511141489
   ✔  - Franziska Hinkelmann (@fhinkel) (TSC): https://github.com/nodejs/node/pull/35665#pullrequestreview-511619185
--------------------------------------------------------------------------------
   ✔  Aborted `git node land` session in /home/runner/work/node/node/.ncu

Commit Queue action: https://github.com/nodejs/node/actions/runs/356556511

@github-actions github-actions bot added the commit-queue-failed An error occurred while landing this pull request using GitHub Actions. label Nov 10, 2020
lib/_tls_common.js Outdated Show resolved Hide resolved
lib/_tls_common.js Outdated Show resolved Hide resolved
lib/_tls_common.js Outdated Show resolved Hide resolved
lib/_tls_common.js Show resolved Hide resolved
@aduh95 aduh95 removed author ready PRs that have at least one approval, no pending requests for changes, and a CI started. commit-queue-failed An error occurred while landing this pull request using GitHub Actions. labels Nov 10, 2020
Cleaup and improvement of crypto_context and SecureContext.

Signed-off-by: James M Snell <[email protected]>
jasnell added a commit that referenced this pull request Nov 11, 2020
Cleaup and improvement of crypto_context and SecureContext.

Signed-off-by: James M Snell <[email protected]>

PR-URL: #35665
Reviewed-By: Alba Mendez <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Franziska Hinkelmann <[email protected]>
@jasnell
Copy link
Member Author

jasnell commented Nov 11, 2020

Landed in 35274cb

@jasnell jasnell closed this Nov 11, 2020
codebytere pushed a commit that referenced this pull request Nov 22, 2020
Cleaup and improvement of crypto_context and SecureContext.

Signed-off-by: James M Snell <[email protected]>

PR-URL: #35665
Reviewed-By: Alba Mendez <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Franziska Hinkelmann <[email protected]>
@codebytere codebytere mentioned this pull request Nov 22, 2020
@benjamingr
Copy link
Member

Hey, this broke socket.io which passes null as the pfx value - it has been fixed since but worth mentioning.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. crypto Issues and PRs related to the crypto subsystem. errors Issues and PRs related to JavaScript errors originated in Node.js core. tls Issues and PRs related to the tls subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants