crypto: expose OPENSSL_IS_BORINGSSL constant

[codebytere]

This PR exposes a constant OPENSSL_IS_BORINGSSL, which could then be used from JS like:

const crypto = require('crypto');
const usingBoringSSL = crypto.constants.OPENSSL_IS_BORINGSSL;

and will then allow Electron to control algorithm test support more granularly and also open the door to better adapting Node.js' own crypto tests to fail less for us.

[aduh95]

Should this be documented in https://nodejs.org/api/crypto.html#crypto_crypto_constants_1?

[addaleax]

Should this be documented in https://nodejs.org/api/crypto.html#crypto_crypto_constants_1?

Yes, but to be fair, OPENSSL_VERSION_NUMBER is also not documented. I’m not sure if that was just something that was missed, or intentionally kept undocumented.

[tniessen]

This appears to be the only definition of OPENSSL_IS_BORINGSSL, at least on the master branch:

#define OPENSSL_IS_BORINGSSL
#define OPENSSL_VERSION_NUMBER 0x1010107f
#define SSLEAY_VERSION_NUMBER OPENSSL_VERSION_NUMBER

If that's the definition that would be picked up here, OPENSSL_IS_BORINGSSL is not a constant but rather a feature test macro, and this change would lead to compilation errors when using BoringSSL. (Or is there a different definition of OPENSSL_IS_BORINGSSL somewhere?)

It might make sense to simply assign true to the constant if the macro is defined, and to leave it undefined otherwise. (But then it doesn't really reflect the macro, so we might as well use a different name.)

[codebytere]

@tniessen what do you think you'd prefer?

[tniessen]

what do you think you'd prefer?

@codebytere Maybe I am missing something and this does work in Electron? I don't have an Electron setup locally so I can't test that. But if I am right and this doesn't work, the simplest solution is probably to just assign the value true to the constant in Node.js, and only if the feature test macro is defined.

[richardlau]

and will then allow Electron to control algorithm test support more granularly and also open the door to better adapting Node.js' own crypto tests to fail less for us.

FWIW #27862 and the changes we've had to make to tests re. key sizes and algorithms to account for FIPS/OpenSSL 3.0 leads me to question if there's a better way to decide algorithm test support in our tests.

[codebytere]

@tniessen yes this works for Electron as-is!

[tniessen]

@codebytere What value does OPENSSL_IS_BORINGSSL have in Electron, and where is it #defined?

This definition assigns no value to OPENSSL_IS_BORINGSSL, so I don't see how that could work, but maybe I'm missing something.

[codebytere]

@tniessen sorry for late reply but your link above is correct - it's not assigned a value. The compiler will look for the #define existing and make a decision based on that - so #if defined(OPENSSL_IS_BORINGSSL) would be true in Electron since that define simply exists.

[tniessen]

your link above is correct - it's not assigned a value. The compiler will look for the #define existing and make a decision based on that - so #if defined(OPENSSL_IS_BORINGSSL) would be true in Electron since that define simply exists.

@codebytere That's my understanding as well. But if OPENSSL_IS_BORINGSSL is not assigned a value, and this patch is passing OPENSSL_IS_BORINGSSL as an argument to NODE_DEFINE_CONSTANT, then how can this work?

---

Say we pass this snippet to g++ -E:

// Definition in BoringSSL:
#define OPENSSL_IS_BORINGSSL

// This PR:
#ifdef OPENSSL_IS_BORINGSSL
    NODE_DEFINE_CONSTANT(target, OPENSSL_IS_BORINGSSL);
#endif

After preprocessing, because OPENSSL_IS_BORINGSSL is defined but not assigned a value, g++ -E outputs:

NODE_DEFINE_CONSTANT(target, );

---

NODE_DEFINE_CONSTANT is defined with two arguments, but the compiler doesn't care much.

node/src/node.h

Lines 607 to 623 in 13b569c

	#define NODE_DEFINE_CONSTANT(target, constant) \
	do { \
	v8::Isolate* isolate = target->GetIsolate(); \
	v8::Local<v8::Context> context = isolate->GetCurrentContext(); \
	v8::Local<v8::String> constant_name = \
	v8::String::NewFromUtf8(isolate, #constant, \
	v8::NewStringType::kInternalized).ToLocalChecked(); \
	v8::Local<v8::Number> constant_value = \
	v8::Number::New(isolate, static_cast<double>(constant)); \
	v8::PropertyAttribute constant_attributes = \
	static_cast<v8::PropertyAttribute>(v8::ReadOnly | v8::DontDelete); \
	(target)->DefineOwnProperty(context, \
	constant_name, \
	constant_value, \
	constant_attributes).Check(); \
	} \
	while (0)

Still, within the macro, static_cast<double>(constant) appears, where constant is OPENSSL_IS_BORINGSSL, which does not represent any value.

---

I tried compiling node with this patch and an additional #define OPENSSL_IS_BORINGSSL and got this, which is what I expected since the preprocessor replaces constant with an empty sequence:

In file included from ../src/node_constants.h:27,
                 from ../src/node_options.h:10,
                 from ../src/inspector_agent.h:9,
                 from ../src/env.h:29,
                 from ../src/env-inl.h:29,
                 from ../src/node_constants.cc:22:
../src/node_constants.cc: In function ‘void node::{anonymous}::DefineCryptoConstants(v8::Local<v8::Object>)’:
../src/node.h:615:62: error: expected primary-expression before ‘)’ token
  615 |         v8::Number::New(isolate, static_cast<double>(constant));              \
      |                                                              ^
../src/node_constants.cc:808:5: note: in expansion of macro ‘NODE_DEFINE_CONSTANT’
  808 |     NODE_DEFINE_CONSTANT(target, OPENSSL_IS_BORINGSSL);
      |     ^~~~~~~~~~~~~~~~~~~~
make[1]: *** [libnode.target.mk:386: /home/tniessen/dev/node/out/Release/obj.target/libnode/src/node_constants.o] Error 1
make: *** [Makefile:110: node] Error 2

[tniessen]

This PR has multiple approvals and you said it works in Electron as is, so I really don't want to block it. I'd just like to understand how this works since we don't currently test building against BoringSSL within Node.js :)