-
-
Notifications
You must be signed in to change notification settings - Fork 31.7k
crypto: expose process.features.openssl_is_boringssl
#58387
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
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #58387 +/- ##
=======================================
Coverage 90.21% 90.22%
=======================================
Files 635 635
Lines 187148 187150 +2
Branches 36740 36751 +11
=======================================
+ Hits 168842 168849 +7
+ Misses 11081 11073 -8
- Partials 7225 7228 +3
🚀 New features to boost your workflow:
|
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.
Should we expose this bit on process.features
instead? crypto.constants
object does not seem to be a good place for feature detection.
b3dafce
to
6a80917
Compare
@legendecas i don't feel super strongly either way but via |
a2f1d52
to
978897f
Compare
src/node_constants.cc
Outdated
#undef OPENSSL_IS_BORINGSSL | ||
#define OPENSSL_IS_BORINGSSL 1 | ||
NODE_DEFINE_CONSTANT(target, OPENSSL_IS_BORINGSSL); | ||
#endif |
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'm not a big fan of redefining the existing define this way. Perhaps this could just be crypto.constants.BORINGSSL
and the definition here could be:
#ifdef OPENSSL_IS_BORINGSSL
constexpr auto BORINGSSL = 1;
#else
constexpr auto BORINGSSL = 0;
#endif
NODE_DEFINE_CONSTANT(target, BORINGSSL);
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.
Alternatively, I do wonder if this is better exposed via process.config
or process.features
. We do have the existing process.config.openssl_is_fips
and process.config.openssl_quic
flags in process.config
.
ha... I missed the previous comment about this ;-)
978897f
to
c80b6bd
Compare
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.
Change generally LGTM but there's one additional test that I think needs to be updated (test-process-features.js
)
crypto.constants.OPENSSL_IS_BORINGSSL
process.features.openssl_is_boringssl
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 but test/parallel/test-process-features.js
has to be updated
c80b6bd
to
1422921
Compare
Landed in 2b42534 |
This PR exposes
process.features.openssl_is_boringssl
. This allows knowing which crypto library is in use at the JS level - previously the only way to know was to check the version string, which would be0.0.0
when built with BoringSSL.This also sets the stage for adapting some of Node.js' crypto tests to run and pass with both BoringSSL and OpenSSL.