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.

Sign up for GitHub

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

Jump to bottom

src: raise error for --enable-fips when no FIPS #38859

Closed
wants to merge 2 commits into from
Closed

src: raise error for --enable-fips when no FIPS #38859

wants to merge 2 commits into from

Conversation

danbev
Copy link
Contributor

@danbev danbev commented May 31, 2021

This commit moves the check for FIPS from the crypto module
initialization to process startup.

The motivation for this is that when OpenSSL is not FIPS enabled and the
command line options --enable-fips, or --force-fips are used, there will
only be an error raised if the crypto module is used. This can be
surprising and we have gotten feedback that users assumed that there
would be an error if these options were specified and FIPS is not
available.

Example output using OpenSSL 1.1.1:

$ ./out/Release/node -p 'crypto.getFips()'
0

$ ./out/Release/node --enable-fips  -p 'crypto.getFips()'
OpenSSL error when trying to enable FIPS:
140252042856384:error:0F06D065:common libcrypto routines:FIPS_mode_set:fips mode not supported:../deps/openssl/openssl/crypto/o_fips.c:22:

Example output using OpenSSL 3.0:

$ ./out/Release/node -p 'crypto.getFips()'
0

$ ./out/Release/node --enable-fips  -p 'crypto.getFips()'
OpenSSL error when trying to enable FIPS:
00A0462A9F7F0000:error:12800067:DSO support routines:dlfcn_load:could not load the shared library:crypto/dso/dso_dlfcn.c:118:filename(/home/danielbevenius/work/security/openssl_quic-3.0/lib/ossl-modules/fips.so): /home/danielbevenius/work/security/openssl_quic-3.0/lib/ossl-modules/fips.so: cannot open shared object file: No such file or directory
00A0462A9F7F0000:error:12800067:DSO support routines:DSO_load:could not load the shared library:crypto/dso/dso_lib.c:162:
00A0462A9F7F0000:error:078C0105:common libcrypto routines:provider_init:init fail:crypto/provider_core.c:657:name=fips

@github-actions github-actions 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. needs-ci PRs that need a full CI run. labels May 31, 2021
@nodejs-github-bot
Copy link
Collaborator

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

bl-ue
bl-ue suggested changes May 31, 2021
View reviewed changes
src/crypto/crypto_util.cc Outdated Show resolved Hide resolved
src/crypto/crypto_util.cc Outdated Show resolved Hide resolved
mhdawson
mhdawson approved these changes May 31, 2021
View reviewed changes
Copy link
Member

@mhdawson mhdawson left a comment

Choose a reason for hiding this comment

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

LGTM

richardlau
richardlau approved these changes Jun 1, 2021
View reviewed changes
Copy link
Member

@richardlau richardlau left a comment

Choose a reason for hiding this comment

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

It should be possible to write a test for this?

@danbev
Copy link
Contributor Author

danbev commented Jun 7, 2021

It should be possible to write a test for this?

That should be possible. Sorry about the late reply (I had to give other tasks priority over this one last week) and I'll take a look at adding a test this week. Thanks

@nodejs-github-bot
Copy link
Collaborator

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

@danbev
src,test: raise error for --enable-fips when no FIPS
9531296 
This commit moves the check for FIPS from the crypto module
initialization to process startup.

The motivation for this is that when OpenSSL is not FIPS enabled and the
command line options --enable-fips, or --force-fips are used, there will
only be an error raised if the crypto module is used. This can be
surprising and we have gotten feedback that users assumed that there
would be an error if these options were specified and FIPS is not
available.
@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

danbev added a commit that referenced this pull request Jun 8, 2021
@danbev
src,test: raise error for --enable-fips when no FIPS
1997aa3 
This commit moves the check for FIPS from the crypto module
initialization to process startup.

The motivation for this is that when OpenSSL is not FIPS enabled and the
command line options --enable-fips, or --force-fips are used, there will
only be an error raised if the crypto module is used. This can be
surprising and we have gotten feedback that users assumed that there
would be an error if these options were specified and FIPS is not
available.

PR-URL: #38859
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
@danbev
Copy link
Contributor Author

danbev commented Jun 8, 2021

Landed in 1997aa3.

@danbev danbev closed this Jun 8, 2021
@danbev danbev deleted the enable-fips-raise-error branch June 8, 2021 09:50
targos pushed a commit that referenced this pull request Jun 11, 2021
@danbev @targos
src,test: raise error for --enable-fips when no FIPS
08b2a4a 
This commit moves the check for FIPS from the crypto module
initialization to process startup.

The motivation for this is that when OpenSSL is not FIPS enabled and the
command line options --enable-fips, or --force-fips are used, there will
only be an error raised if the crypto module is used. This can be
surprising and we have gotten feedback that users assumed that there
would be an error if these options were specified and FIPS is not
available.

PR-URL: #38859
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
@danielleadams danielleadams mentioned this pull request Jun 14, 2021
Merged
@richardlau
Copy link
Member

Doesn't land cleanly on v14.x-staging.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bl-ue bl-ue bl-ue approved these changes

@mhdawson mhdawson mhdawson approved these changes

@richardlau richardlau richardlau approved these changes

@addaleax addaleax addaleax approved these changes

@jasnell jasnell jasnell approved these changes

Assignees
No one assigned
Labels
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. needs-ci PRs that need a full CI run.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@danbev @nodejs-github-bot @richardlau @jasnell @addaleax @mhdawson @bl-ue