-
Notifications
You must be signed in to change notification settings - Fork 29.9k
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
build,src,test,doc: enable FIPS for OpenSSL 3.0 #38633
Conversation
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
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) | ||
``` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can also run process.report.getReport()
in Node.js and look at the sharedObjects
section to see loaded libraries.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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_CONF
to point to the config file works but using Node.js'--openssl-config
command line option does not appear to enable FIPS for the same config file. - misconfiguring
.include
in the config to a wrong path/non-existent file appears to makenode
hang during start-up. May be an upstream OpenSSL issue?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
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 <jasnell@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Richard Lau <rlau@redhat.com> Reviewed-By: Michael Dawson <midawson@redhat.com>
Landed in 0d7644f |
I'll take a closer look at these issues next week. Thanks! |
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 <jasnell@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Richard Lau <rlau@redhat.com> Reviewed-By: Michael Dawson <midawson@redhat.com>
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)
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 <rlau@redhat.com> Reviewed-By: Minwoo Jung <nodecorelab@gmail.com> Refs: #38633 (review)
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 <rlau@redhat.com> Reviewed-By: Minwoo Jung <nodecorelab@gmail.com> Refs: #38633 (review)
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 <midawson@redhat.com> Reviewed-By: Michaël Zasso <targos@protonmail.com>
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 <midawson@redhat.com> Reviewed-By: Michaël Zasso <targos@protonmail.com>
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 <midawson@redhat.com> Reviewed-By: Michaël Zasso <targos@protonmail.com>
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 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.