build,src,test,doc: enable FIPS for OpenSSL 3.0#38633
Closed
Uh oh!
There was an error while loading. Please reload this page.
Conversation
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
github-actionsbot added lib / src 
jasnell approved these changes May 11, 2021
Collaborator
nodejs-github-bot commented May 11, 2021
Collaborator
nodejs-github-bot commented May 12, 2021
This commit enables FIPS when Node.js is dynamically linking against quictls/openssl-3.0. BUILDING.md has been updated with instructions to configure and build quictls/openssl 3.0.0-alpha-15 and includes a couple of work-arounds which I believe are fixed in alpha-16 and can be removed when alpha-16 is available. The information might be a little too detailed/verbose but I thought it would be helpful to at least initially include all the steps.
Add macro guard to OpenSSL 3.0 include.
Update to 3.0.0-alpha-16
Trott reviewed May 12, 2021
Uh oh!
There was an error while loading. Please reload this page.
Trott approved these changes May 12, 2021
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
| libpthread.so.0 => /usr/lib64/libpthread.so.0 (0x00007fd910eb5000) | ||
| libc.so.6 => /usr/lib64/libc.so.6 (0x00007fd910cec000) | ||
| /lib64/ld-linux-x86-64.so.2 (0x00007fd9117f2000) | ||
| ``` |
Member
There was a problem hiding this comment.
Can also run process.report.getReport() in Node.js and look at the sharedObjects section to see loaded libraries.
ContributorAuthor
There was a problem hiding this comment.
I'll add a note about using it. I think it might be useful to keep the ldd command just as a way to make sure the libraries can be found or node will note be able to run.
Member
There was a problem hiding this comment.
+1 for keeping the ldd command. I'm pointing out an alternative... we could possibly also use the report in tests in the future as additional verification.
Uh oh!
There was an error while loading. Please reload this page.
Collaborator
nodejs-github-bot commented May 12, 2021
richardlau approved these changes May 13, 2021
Member
richardlau left a comment
richardlau left a comment There was a problem hiding this comment.
Fantastic work @danbev! I've followed the instructions and they work, so approving. There's some edge case weirdness¹ that we can iterate on afterwards -- OpenSSL 3 support is going to be experimental until a proper release.
¹ For example:
- setting
OPENSSL_CONFto point to the config file works but using Node.js'--openssl-configcommand line option does not appear to enable FIPS for the same config file. - misconfiguring
.includein the config to a wrong path/non-existent file appears to makenodehang during start-up. May be an upstream OpenSSL issue?
mhdawson pushed a commit that referenced this pull request May 14, 2021
This commit enables FIPS when Node.js is dynamically linking against quictls/openssl-3.0. BUILDING.md has been updated with instructions to configure and build quictls/openssl 3.0.0-alpha-15 and includes a couple of work-arounds which I believe are fixed in alpha-16 and can be removed when alpha-16 is available. The information might be a little too detailed/verbose but I thought it would be helpful to at least initially include all the steps. PR-URL: #38633 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Michael Dawson <[email protected]>
Member
mhdawson commented May 14, 2021
Landed in 0d7644f |
ContributorAuthor
danbev commented May 15, 2021
I'll take a closer look at these issues next week. Thanks! |
targos pushed a commit that referenced this pull request May 17, 2021
This commit enables FIPS when Node.js is dynamically linking against quictls/openssl-3.0. BUILDING.md has been updated with instructions to configure and build quictls/openssl 3.0.0-alpha-15 and includes a couple of work-arounds which I believe are fixed in alpha-16 and can be removed when alpha-16 is available. The information might be a little too detailed/verbose but I thought it would be helpful to at least initially include all the steps. PR-URL: #38633 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Michael Dawson <[email protected]>
danbev added a commit to danbev/node that referenced this pull request May 19, 2021
This commit adds a call to OPENSSL_init_crypto to initialize OPENSSL_INIT_LOAD_CONFIG to avoid the default behavior where errors raised during the parsing of the OpenSSL configuration file are not propagated and cannot be detected. The motivation for this is that if FIPS is configured the OpenSSL configuration file will have an .include pointing to the fipsmodule.cnf file generated by the openssl fipsinstall command. If the path to this file is incorrect no error will be reported. For Node.js this will mean that EntropySource will be called by V8 as part of its initalization process, and EntropySource will in turn call CheckEntropy. CheckEntropy will call RAND_status which will now always return 0 leading to an endless loop and the node process will appear to hang/freeze. I'll continue investigating the cause of this and see if this is expected behavior or not, but in the mean time it would be good to be able to workaround this issue with this commit. Refs: nodejs#38633 (review)
danbev added a commit that referenced this pull request May 24, 2021
This commit adds a call to OPENSSL_init_crypto to initialize OPENSSL_INIT_LOAD_CONFIG to avoid the default behavior where errors raised during the parsing of the OpenSSL configuration file are not propagated and cannot be detected. The motivation for this is that if FIPS is configured the OpenSSL configuration file will have an .include pointing to the fipsmodule.cnf file generated by the openssl fipsinstall command. If the path to this file is incorrect no error will be reported. For Node.js this will mean that EntropySource will be called by V8 as part of its initalization process, and EntropySource will in turn call CheckEntropy. CheckEntropy will call RAND_status which will now always return 0 leading to an endless loop and the node process will appear to hang/freeze. I'll continue investigating the cause of this and see if this is expected behavior or not, but in the mean time it would be good to be able to workaround this issue with this commit. PR-URL: #38732 Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Minwoo Jung <[email protected]> Refs: #38633 (review)
danielleadams pushed a commit that referenced this pull request May 31, 2021
This commit adds a call to OPENSSL_init_crypto to initialize OPENSSL_INIT_LOAD_CONFIG to avoid the default behavior where errors raised during the parsing of the OpenSSL configuration file are not propagated and cannot be detected. The motivation for this is that if FIPS is configured the OpenSSL configuration file will have an .include pointing to the fipsmodule.cnf file generated by the openssl fipsinstall command. If the path to this file is incorrect no error will be reported. For Node.js this will mean that EntropySource will be called by V8 as part of its initalization process, and EntropySource will in turn call CheckEntropy. CheckEntropy will call RAND_status which will now always return 0 leading to an endless loop and the node process will appear to hang/freeze. I'll continue investigating the cause of this and see if this is expected behavior or not, but in the mean time it would be good to be able to workaround this issue with this commit. PR-URL: #38732 Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Minwoo Jung <[email protected]> Refs: #38633 (review)
richardlau added a commit to richardlau/node-1 that referenced this pull request Nov 23, 2022
Both paths for the condition being removed result in the same value being assigned to `openssl_product`. This condition was also problematic as it was testing a variable in the same scope which gyp/gyp-next currently does not support. Refs: https://gyp.gsrc.io/docs/InputFormatReference.md#user_defined-variables PR-URL: nodejs#45076 Refs: nodejs/node-gyp#2750 Refs: nodejs#38633 Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Michaël Zasso <[email protected]>
guangwong pushed a commit to noslate-project/node that referenced this pull request Jan 3, 2023
Both paths for the condition being removed result in the same value being assigned to `openssl_product`. This condition was also problematic as it was testing a variable in the same scope which gyp/gyp-next currently does not support. Refs: https://gyp.gsrc.io/docs/InputFormatReference.md#user_defined-variables PR-URL: nodejs/node#45076 Refs: nodejs/node-gyp#2750 Refs: nodejs/node#38633 Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Michaël Zasso <[email protected]>
guangwong pushed a commit to noslate-project/node that referenced this pull request Jan 3, 2023
Both paths for the condition being removed result in the same value being assigned to `openssl_product`. This condition was also problematic as it was testing a variable in the same scope which gyp/gyp-next currently does not support. Refs: https://gyp.gsrc.io/docs/InputFormatReference.md#user_defined-variables PR-URL: nodejs/node#45076 Refs: nodejs/node-gyp#2750 Refs: nodejs/node#38633 Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Michaël Zasso <[email protected]>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment
7 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This commit enables FIPS when Node.js is dynamically linking against
quictls/openssl-3.0.BUILDING.mdhas been updated with instructions to configure and buildquictls/openssl 3.0.0-alpha-15 and includes a couple of work-arounds
which I believe are fixed in alpha-16 and can be removed when it is
available. The information might be a little too detailed/verbose but I
thought it would be helpful to at least initially include all the steps.