FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

src: add proper mutexes for accessing FIPS state by addaleax · Pull Request #42278 · nodejs/node · GitHub

/ node Public

src: add proper mutexes for accessing FIPS state - #42278

Merged
nodejs-github-bot merged 2 commits into
nodejs:masterfrom
addaleax:fips-mutexes
Apr 3, 2022
ConversationCommits (2)ChecksFiles changed
Merged

src: add proper mutexes for accessing FIPS state#42278
nodejs-github-bot merged 2 commits into
nodejs:masterfrom
addaleax:fips-mutexes

Conversation

addaleax commented Mar 10, 2022

Copy link
Copy Markdown
Member

The FIPS state handling and OpenSSL initialization code makes
accesses to global OpenSSL state without any protection against
parallel modifications from multiple threads.

This commit adds such protections.

src: add proper mutexes for accessing FIPS state
095c3e3 
The FIPS state handling and OpenSSL initialization code makes
accesses to global OpenSSL state without any protection against
parallel modifications from multiple threads.

This commit adds such protections.

nodejs-github-bot commented Mar 10, 2022

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/crypto

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. crypto Issues and PRs related to the crypto subsystem. needs-ci PRs that need a full CI run. labels Mar 10, 2022
addaleax commented Mar 10, 2022
View reviewed changes
Comment thread src/crypto/crypto_util.cc Outdated
Comment on lines +222 to +223
// TODO: This should not be possible to set from worker threads.
// CHECK(env->owns_process_state());

addaleax Mar 10, 2022

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Left out because this could be seen as a semver-major change. If we don’t think it’s semver-major, I’m happy to add this as well.

richardlau Apr 3, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Is it worth doing this for Node.js 18 as a semver-major anyway?

addaleax Apr 4, 2022

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Yeah, sure. It’s just not quite as important and I didn’t want to make this PR one that wouldn’t be backported.

addaleax added request-ci Add this label to start a Jenkins CI on a PR. and removed needs-ci PRs that need a full CI run. labels Mar 10, 2022
github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Mar 10, 2022

This comment was marked as outdated.

Sign in to view
tniessen reviewed Mar 10, 2022
View reviewed changes
Comment thread src/crypto/crypto_util.cc

// Protect accesses to FIPS state with a mutex. This should potentially
// be part of a larger mutex for global OpenSSL state.
static Mutex fips_mutex;

tniessen Mar 10, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

This makes sense for semantic reasons, but is my understanding correct that this is technically not required since per_process::cli_options_mutex will already guarantee mutually exclusive access whenever fips_mutex is used?

addaleax Mar 10, 2022

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Yes, that is correct as far as the current state of this PR is concerned. 👍

tniessen Mar 10, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Thanks for clarifying! 😃

tniessen approved these changes Mar 10, 2022
View reviewed changes

tniessen left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

LGTM with or without fips_mutex.

tniessen added the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label Mar 10, 2022

This comment was marked as outdated.

Sign in to view
tniessen added the openssl Issues and PRs related to the OpenSSL dependency. label Mar 10, 2022
github-actions Bot mentioned this pull request Mar 11, 2022
Open
16 tasks

This comment was marked as outdated.

Sign in to view

This comment was marked as outdated.

Sign in to view

This comment was marked as outdated.

Sign in to view

This comment was marked as outdated.

Sign in to view
github-actions Bot mentioned this pull request Mar 12, 2022
Open
15 tasks

This comment was marked as outdated.

Sign in to view

tniessen commented Mar 12, 2022

Copy link
Copy Markdown
Member

I have no idea what's going on with arm CI...

github-actions Bot mentioned this pull request Mar 13, 2022
Open
17 tasks

This comment was marked as outdated.

Sign in to view
github-actions Bot mentioned this pull request Mar 14, 2022
Open
28 tasks
github-actions Bot mentioned this pull request Apr 3, 2022
Open
8 tasks
richardlau added the request-ci Add this label to start a Jenkins CI on a PR. label Apr 3, 2022
github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Apr 3, 2022

nodejs-github-bot commented Apr 3, 2022

Copy link
Copy Markdown
Collaborator

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

nodejs-github-bot commented Apr 3, 2022

Copy link
Copy Markdown
Collaborator

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

nodejs-github-bot commented Apr 3, 2022

Copy link
Copy Markdown
Collaborator

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

richardlau added the commit-queue Add this label to land a pull request using GitHub Actions. label Apr 3, 2022
nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Apr 3, 2022
nodejs-github-bot merged commit 1c69dfe into nodejs:master Apr 3, 2022

nodejs-github-bot commented Apr 3, 2022

Copy link
Copy Markdown
Collaborator

Landed in 1c69dfe

github-actions Bot mentioned this pull request Apr 4, 2022
Open
9 tasks
addaleax deleted the fips-mutexes branch April 4, 2022 09:18

richardlau commented Apr 4, 2022

Copy link
Copy Markdown
Member

src/crypto/crypto_util.cc was created as part of a semver-major refactor #35093 so this will need a manual backport to v14.x-staging if it is required there. It would also need a backport for 12.x but since we're not planning any further 12.x releases after tomorrow's release it's probably a bit late to land there.

github-actions Bot mentioned this pull request Apr 5, 2022
Open
13 tasks
juanarbol pushed a commit to juanarbol/node that referenced this pull request Apr 5, 2022
src: add proper mutexes for accessing FIPS state
3084af3 
The FIPS state handling and OpenSSL initialization code makes
accesses to global OpenSSL state without any protection against
parallel modifications from multiple threads.

This commit adds such protections.

PR-URL: nodejs#42278
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
This was referenced Apr 5, 2022
Closed
Merged
github-actions Bot mentioned this pull request Apr 6, 2022
Open
11 tasks
juanarbol pushed a commit that referenced this pull request Apr 6, 2022
src: add proper mutexes for accessing FIPS state
e798e26 
The FIPS state handling and OpenSSL initialization code makes
accesses to global OpenSSL state without any protection against
parallel modifications from multiple threads.

This commit adds such protections.

PR-URL: #42278
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
github-actions Bot mentioned this pull request Apr 7, 2022
Open
17 tasks
xtx1130 pushed a commit to xtx1130/node that referenced this pull request Apr 25, 2022
src: add proper mutexes for accessing FIPS state
b51a7f7 
The FIPS state handling and OpenSSL initialization code makes
accesses to global OpenSSL state without any protection against
parallel modifications from multiple threads.

This commit adds such protections.

PR-URL: nodejs#42278
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
juanarbol pushed a commit that referenced this pull request May 31, 2022
src: add proper mutexes for accessing FIPS state
b3016c7 
The FIPS state handling and OpenSSL initialization code makes
accesses to global OpenSSL state without any protection against
parallel modifications from multiple threads.

This commit adds such protections.

PR-URL: #42278
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
juanarbol mentioned this pull request May 31, 2022
Closed
danielleadams pushed a commit that referenced this pull request Jun 27, 2022
src: add proper mutexes for accessing FIPS state
456c186 
The FIPS state handling and OpenSSL initialization code makes
accesses to global OpenSSL state without any protection against
parallel modifications from multiple threads.

This commit adds such protections.

PR-URL: #42278
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
targos pushed a commit that referenced this pull request Jul 11, 2022
src: add proper mutexes for accessing FIPS state
4ffc088 
The FIPS state handling and OpenSSL initialization code makes
accesses to global OpenSSL state without any protection against
parallel modifications from multiple threads.

This commit adds such protections.

PR-URL: #42278
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
targos mentioned this pull request Aug 3, 2022
Merged
guangwong pushed a commit to noslate-project/node that referenced this pull request Oct 10, 2022
src: add proper mutexes for accessing FIPS state
758ab3f 
The FIPS state handling and OpenSSL initialization code makes
accesses to global OpenSSL state without any protection against
parallel modifications from multiple threads.

This commit adds such protections.

PR-URL: nodejs/node#42278
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

tniessen tniessen approved these changes
richardlau richardlau approved these changes
jasnell jasnell approved these changes
RaisinTen RaisinTen approved these changes

Assignees

No one assigned

Labels

author ready PRs that have at least one approval, no outstanding review comments, 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. openssl Issues and PRs related to the OpenSSL dependency.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

Back | FazBrowse Home | New Git URL