Skip to content

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

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
Closed
danbev wants to merge 2 commits into from
Closed

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

danbev wants to merge 2 commits into from

Conversation

@danbev
Copy link
Contributor

@danbevdanbev 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 directory00A0462A9F7F0000: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-actionsgithub-actionsbot 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

nodejs-github-bot commented May 31, 2021

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

bl-ue
bl-ue suggested changes May 31, 2021
View reviewed changes
mhdawson
mhdawson approved these changes May 31, 2021
View reviewed changes
Copy link
Member

@mhdawsonmhdawson left a comment

Choose a reason for hiding this comment

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

LGTM

@danbevdanbevforce-pushed the enable-fips-raise-error branch from bcfa6df to c94d2acCompareJune 1, 2021 04:11
richardlau
richardlau approved these changes Jun 1, 2021
View reviewed changes
Copy link
Member

@richardlaurichardlau 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
ContributorAuthor

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

@danbevdanbevforce-pushed the enable-fips-raise-error branch from c94d2ac to bd9c8caCompareJune 7, 2021 07:17
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Jun 7, 2021

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

richardlau
richardlau reviewed Jun 7, 2021
View reviewed changes
@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.
@danbevdanbevforce-pushed the enable-fips-raise-error branch from 1d4d918 to 9531296CompareJune 7, 2021 11:44
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Jun 7, 2021

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

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Jun 8, 2021

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
ContributorAuthor

danbev commented Jun 8, 2021

Landed in 1997aa3.

@danbevdanbev closed this Jun 8, 2021
@danbevdanbev 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]>
@danielleadamsdanielleadams mentioned this pull request Jun 14, 2021
Merged
@richardlau
Copy link
Member

richardlau commented Jul 16, 2021

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

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

Reviewers

@jasnelljasnelljasnell approved these changes

@addaleaxaddaleaxaddaleax approved these changes

@richardlaurichardlaurichardlau approved these changes

@mhdawsonmhdawsonmhdawson approved these changes

+1 more reviewer

@bl-uebl-uebl-ue approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

c++Issues and PRs that require attention from people who are familiar with C++.lib / srcIssues and PRs related to general changes in the lib or src directory.needs-ciPRs 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