tls: Use system's openssl.cnf for OpenSSL configuration

[temaivanoff]

Pull Request check-list

Please make sure to review and check all of these items:

• Does make -j8 test (UNIX) or vcbuild test nosign (Windows) pass with
  this change (including linting)?
• Is the commit message formatted according to [CONTRIBUTING.md][0]?
• If this change fixes a bug (or a performance problem), is a regression
  test (or a benchmark) included?
• Is a documentation update included (if this change modifies
  existing APIs, or introduces new ones)?

---

No LSB modules are available.
Distributor ID: Ubuntu
Description: Ubuntu 14.04.3 LTS
Release: 14.04
Codename: trusty

make -j8 test
Total errors found: 0

[temaivanoff]

This allows you to load the standard file system openssl.cnf . Where I connect libgost.so, after which the node -> tls.getCiphers (); and others see the encryption algorithms GOST2001-GOST89-GOST89
GOST94-GOST89-GOST89

---

Problem:
crypto.setEngine('/usr/lib64/openssl/engines/libgost.so')
Works only crypto.getCiphers(); and has no effect on tls.getCiphers ();

[temaivanoff]

tls.getCiphers();
[ 'aes128-gcm-sha256',
.........
'gost2001-gost89-gost89',
'gost94-gost89-gost89',
.........
'srp-rsa-aes-256-cbc-sha' ]

[Fishrock123]

cc @nodejs/crypto, @ChALkeR

[Fishrock123]

@fast0490f Could you please fill out the checklist at the top of this issue? Notably, confirm make -j4 test works, and clean up the commit formatting? Thanks!

[temaivanoff]

@Fishrock123 I added tests and have more information . Please tell me do you need something else ?

[temaivanoff]

Also, I know that , when adding OPENSSL_config(NULL); solves the problem of connecting the modules to node fips OpenSSL

[Trott]

What's up with renamed the test directory?

[temaivanoff]

@Trott It tests directory run command 'make -j8' . Sorry, I thought that it is necessary to create a new directory for the test so that you can make sure that my change does not cause errors and conflicts .

[temaivanoff]

@Trott I corrected my mistake to change the folder test

[temaivanoff]

@Trott @ChALkeR @Fishrock123 I've added a new change that allows you to enable this optional. This reduces the possibility of conflicts and errors for others. I think this is a good solution .

[indutny]

Perhaps, openssl-system-conf? Sounds a bit more logical to me. (Though, I'm not a native speaker myself, so I could be wrong here too).

[indutny]

This should be guarded by HAVE_OPENSSL too.

[indutny]

May I ask you to change the description to something like this:

use system's openssl.cnf for OpenSSL configuration

[indutny]

Some minor nits, otherwise looking quite good! Please let me know if you have any questions, or when it will be ready for next series of review!

[temaivanoff]

@indutny Hi, I am very glad that you paid attention to my pull request. Thank you for your comments! I made what you adviced.

Should we also consider adding --openssl-conf=/path CLI argument?

I have considered the function OPENSSL_config() responsible of it and found out that the function receive the argument but don't use it. Maybe OpenSSL developers reserved this variable for further using, because path is formed by different algorithms to openssl.cnf file.
I am always ready to reply your comments to make nodejs better =).

[indutny]

Please do # if NODE_FIPS_MODE

[indutny]

@fast0490f PR looks much better now! Thank you.

One more thing, may I ask you to amend https://github.com/nodejs/node/blob/master/doc/node.1 too? Feel free to ask me, if anything will look unfamiliar or hard to you. I'll be glad to help!

I have considered the function OPENSSL_config() responsible of it and found out that the function receive the argument but don't use it. Maybe OpenSSL developers reserved this variable for further using, because path is formed by different algorithms to openssl.cnf file.
I am always ready to reply your comments to make nodejs better =).

It really looks like it is using it on this line. Am I wrong?

[thefourtheye]

@fast0490f Your git configuration is not reflecting your identity. You might want to check this https://help.github.com/articles/setting-your-username-in-git/

[thefourtheye]

Also, the file permissions are messed up. They got changed from 644 to 755.

[temaivanoff]

@indutny Hi, I made what you adviced =)

It really looks like it is using it on this line. Am I wrong?

I found the following information:
OPENSSL_config configures OpenSSL using the standard openssl.cnf configuration file name using config_name. If config_name is NULL then the file specified in the environment variable OPENSSL_CONF will be used, and if that is not set then a system default location is used. Errors are silently ignored. Multiple calls have no effect

[temaivanoff]

@thefourtheye Thank you very much for the help, I fixed the comments.

[thefourtheye]

This comment seems redundant now, doesn't it?

[indutny]

Yep.

[indutny]

@fast0490f I think node.1 still needs an update.

I have considered the function OPENSSL_config() responsible of it and found out that the function receive the argument but don't use it. Maybe OpenSSL developers reserved this variable for further using, because path is formed by different algorithms to openssl.cnf file.

I don't get it... it really looks like the path you pass to OPENSSL_config is used to load configuration if non-NULL argument is passed to it. What I'm trying to suggest is a separate command-line option for providing specific openssl path.

[burmisov]

@indutny I really don't know why, but for whatever reason OPENSSL_config() does NOT use the argument as config file path. I haven't done much C/C++ since university, but as far as I can tell it is really so in the source, if you could take a minute to follow: 1, 2, 3, 4

I don't know what the appname thing stands for, but it is definetly not the path.

Maybe I should file an issue with OpenSSL (docs?), but the actual behaviour definetely contradicts the description on the site.

[indutny]

Gosh, sorry about the delay. Looking into it now.

[indutny]

@burmisov you are right, for some reason it is used as an appname. Guess it is an OpenSSL bug, I'll see what OpenSSL team thinks about it.

Meanwhile except one nit (see comments) - this PR LGTM.

@nodejs/crypto may I have one more LGTM on this, please?

[indutny]

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

[bnoordhuis]

Honestly, I think it's a really clunky interface. You pass --openssl-system-conf to node, then and only then does it start respecting the OPENSSL_CONF environment variable, but when it's unset it defaults to the OPENSSLDIR build-time option, which in our builds is /usr/local/ssl and probably not what most users want or expect (and quite possibly conflicts if it's for a different openssl version.)

I suggest we wait until the openssl team releases a fix for the filename/appname thing, cherry-pick that and make --openssl-system-conf take a filename as its argument (and possibly rename it to --openssl-config or something like that.) Until then, -1 from me.

[indutny]

@bnoordhuis reasonable, let's talk to OpenSSL team first.

[indutny]

@bnoordhuis seems like it is fixed in master in some sense. They have deprecated OPENSSL_config, and the new API OPENSSL_init_crypto accepts settings structure, which could be initialized with OPENSSL_INIT_set_config_filename, which works as expected.

...actually, I just realized that doc is correct:

OPENSSL_config() configures OpenSSL using the standard openssl.cnf configuration file name using config_name.

It is just a really strange wording there, but it doesn't say that config_name is used as a file name.

[stefanmb]

@indutny @fast0490f

I'm late to comment but I'm a bit confused by this pull request. It seems to reintroduce the idea of controlling the OpenSSL's loading of a configuration file via command line argument, which we explored (and chose not to do) in #5181.

However, the call to OpenSSL_config(NULL) is already present at https://github.com/geoworks/node/blob/master/src/node_crypto.cc#L5657, and internally OpenSSL will load the default configuration file, or the one from OPENSSL_CONF (if present in the environment) as per https://www.openssl.org/docs/manmaster/crypto/OPENSSL_config.html.

Why do we need another call to OpenSSL_Config(NULL) at https://github.com/geoworks/node/blob/master/src/node_crypto.cc#L5650?

[bnoordhuis]

That was introduced with the FIPS support in commit 7c48cb5? Can we remove that?

[burmisov]

Now that there is some attention drawn to this matter, I'd like to point out that this PR (specifically calling OpenSSL_Config(NULL) before other cipher config etc) fixes #5101, and I don't know if there is some other way around that.

[stefanmb]

I had a chance to look at this issue further and here are my thoughts, sorry for the wall of text.

@bnoordhuis The call to OpenSSL_Config(NULL) already exists in the codebase and works irrespective of any command line options. It was added as part of 7c48cb5 because the OpenSSL configuration has a specific directive for configuring FIPS on or off:

[ evp_sect ]
# Set to "yes" to enter FIPS mode if supported
fips_mode = no

Users experienced with FIPS reported that the config file is the expected way to control the FIPS mode. After the change proposed in this PR the code would look like this:

void InitCryptoOnce() {
#ifdef HAVE_OPENSSL
  // Use system's openssl.cnf for OpenSSL configuration
  if (openssl_system_conf) {
   OPENSSL_config(nullptr);
  }
#endif  // HAVE_OPENSSL

  SSL_library_init();
  OpenSSL_add_all_algorithms();
  SSL_load_error_strings();
  OPENSSL_config(NULL);

I'm -1 on this change if we are calling OPENSSL_config twice. Removing the second call will break the convention we already established for FIPS mode, which was that it can be turned on by simply specifying OPENSSL_CONF to point to a file with the correct fips_mode directive. Changing this behaviour may be a semver-major change in my opinion.

Now, coming back to the problem reported by @fast0490f and @burmisov: GOST support is provided by a separate engine. This engine is actually bundled with OpenSSL 1.0.x and we have it in the deps/openssl/openssl/engines/ccgost folder. However, by default we seem to compile without it, partly because of these lines (there are also some linking issues that would have to be solved).

Therefore, the only way to use GOST is to provide an external library, I'm assuming this is what @fast0490f and @burmisov did using the config file, it looks like this:

openssl_conf = openssl_def

[openssl_def]
engines = engine_section

[engine_section]
gost = gost_section

[gost_section]
engine_id = gost
dynamic_path = /usr/lib/x86_64-linux-gnu/openssl-1.0.0/engines/libgost.so
default_algorithms = ALL
CRYPT_PARAMS = id-Gost28147-89-CryptoPro-A-ParamSet

This file is already being read without the changes in this PR, but I think the problem is that it's read after SSL_library_init() and friends - meaning that it's probably too late to specify the custom engine and the default one is used instead, without GOST support.

tl;dr
I'm -1 on introducing "--openssl-system-conf", it's misleading (the config file is already read without it) and partly redundant. Instead, I suggest simply moving the existing OpenSSL_config(NULL) call to the top of InitCryptoOnce in node_crypto.cc and making sure the FIPS tests still pass, note that this means compiling Node.js with and without FIPS mode and running the tests, as they behave differently depending on how Node was compiled.

[burmisov]

@stefanmb Thanks for the explanations, it is now quite clear to me what is happening.
+1 on your proposal. Is it viable for me to put some resource to it and do a PR?

[stefanmb]

@burmisov I've made the minimal PR here: #6374

[temaivanoff]

thanks guys! :)

[stefanmb]

I'm closing this PR since the solution we agreed on was landed as part of #6374.

Thanks to everyone for pointing this out to us!