crypto: Add crypto.getEngines() #10865

shigeki · 2017-01-18T07:14:30Z

This adds a new api to show the list of loaded engines of OpenSSL.
It also includes the test of dynamic engine for crypto.setEngine() and fixes missed test coverages as discussed in #10786.

A test engine is built from test/fixtures/openssl_test_engine/ by adding a new target in node.gyp, but fipsld seems to have an error with linking the test engine with libcrypto.a so that a dynamic engine test is skipped in FIPS mode.

I've already made several CI tests and found that tests on Windows and arm were failed because test engine files were lost in CI environment. They can be solved by includingRelease\node_test_engine.dll in Windows tests and out/Release/libnode_test_engine.so in arm tests in binary/binary.tar.gz and binary/binary.tar.xz . I would like ask someone in @nodejs/build to help it.

In my local environments, tests in Win and arm are fine.

CC @nodejs/crypto and @jasnell

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

Affected core subsystem(s)

crypto

shigeki · 2017-01-18T07:16:46Z

CI is https://ci.nodejs.org/job/node-test-pull-request/5921/ . Tests in Win and arm would be failed.

mscdex · 2017-01-18T07:23:00Z

doc/api/crypto.md

+console.log(crypto.getEngines());
+// Prints:
+// [ { id: 'rdrand', name: 'Intel RDRAND engine' },
+// { id: 'dynamic', name: 'Dynamic engine loading support' } ]


Minor nit: this line looks better indented two more spaces to the right.

I will fix it, thanks.

mscdex · 2017-01-18T07:24:16Z

lib/crypto.js

@@ -633,6 +633,11 @@ exports.setEngine = function setEngine(id, flags) {
  return binding.setEngine(id, flags);
 };

+
+exports.getEngines = function getEngines() {


Can we get away with directly assigning to binding.getEngines ?

Do you mean we should use internalUtil.cachedResult such as getCipher() ?

I meant if we could just do exports.getEngines = binding.getEngines; and avoid the wrapper function?

That's fine with me.

mscdex · 2017-01-18T07:26:24Z

node.gyp

@@ -947,6 +947,18 @@
          ],
        }],
      ]
+    },
+    {


Is it possible to put this whole definition in a separate gyp file in test/ somewhere instead of node.gyp, since it's only used for a test?

I think having target_name and type is good to know its purpose otherwise we need to write a comment of it nearly as the same description.

mscdex · 2017-01-18T07:28:14Z

Perhaps it might be useful to also have a getEngine() that returns the currently set engine (kind of like the get/set for FIPS mode)?

shigeki · 2017-01-18T12:31:51Z

The test was unstable. Here is a new test result, https://ci.nodejs.org/job/node-test-commit/7335/

sam-github · 2017-01-18T20:05:19Z

test/parallel/test-crypto-engine.js

+'use strict';
+const common = require('../common');
+
+if (!common.hasCrypto) {


missing test description, see https://github.com/nodejs/node/blob/master/doc/guides/writing-tests.md#test-structure

sam-github · 2017-01-18T20:08:12Z

test/parallel/test-crypto-engine.js

+    found++;
+});
+
+assert.strictEqual(found, 1);


assert conflicts with comment about "at least one"

The description of comment was wrong. Fixed.

sam-github · 2017-01-18T20:09:07Z

test/parallel/test-crypto-engine.js

+let found = 0;
+
+// check at least one builtin engine exists.
+crypto.getEngines().forEach((e) => {


same as found = array.filter((e) => e.id === 'dynamic').length?

sam-github · 2017-01-18T20:10:49Z

test/parallel/test-crypto-engine.js

+
+crypto.setEngine(test_engine);
+
+crypto.getEngines().forEach((e) => {


found = crypto.getEngines.filter((e) => e.id === test_engine_id && e.name === test_engine_name)).length?

Is there any reason to use filter rather than forEach? It seems to be equivalent for me.

Its true that many of the Array methods can be implemented in terms of forEach(), so they are equivalent in that sense. Someone has to read this code, and understand that it is filtering. Better to just call filter(). And if you don't, @Trott or one of the code-and-learn people may come through and modify this to use es6 and builtin library functions - better avoid the churn and do it now, I think.

sam-github · 2017-01-18T20:14:08Z

test/parallel/test-crypto-engine.js

+  fs.accessSync(test_engine);
+}, 'node test engine ' + test_engine + ' is not found.');
+
+crypto.setEngine(test_engine);


maybe call twice to prove its possible?

It is possible. Do we need a such kind of combination tests for setEngine?

Tests should test invalid behaviours, and obscure corner conditions. If it is allowed to call multiple times, the test should do, and assert on the result, IMO. It would be possible that the second call always fails, for example, or someone might believe that both "RSA" engines get added (which isn't true). The docs don't actually say what happens when setEngine() is called multiple times with the same flag. The latest call will replace the previous default, but its not stated.

sam-github · 2017-01-18T20:17:13Z

src/node_crypto.cc

+
+  for (e = ENGINE_get_first(); e != nullptr; e = ENGINE_get_next(e)) {
+    const char* id = ENGINE_get_id(e);
+    const char* name = ENGINE_get_name(e);


too bad there is no ENGINE_get_flags() so we can know the flags

Good catch. SetEngine did not have ENGINE_set_flag so that flags was always 0 for dynamic engines. Fixed and add a new flags property.

sam-github · 2017-01-18T20:19:10Z

I thought there was not one engine, but a set of engines available at a time, and which one is used depends on the operation, so a getEngine() for current wouldn't make sense. @mscdex Am I misunderstanding? I've not worked much with engines.

mscdex · 2017-01-18T20:23:18Z

@sam-github I'm not sure I follow. If there is a setEngine(), I would think that a getEngine() would be useful to allow one to know which engine is currently set (and especially useful in tests to make sure an engine is actually set)?

sam-github · 2017-01-18T20:42:57Z

I think getEngines() may need another name. Perhaps just engines()? or listEngines()?

@mscdex I'm just looking at the code, and going off of how I would expect ossl to workcrypto.setEngine() is basically a call to https://github.com/nodejs/node/blob/master/deps/openssl/openssl/crypto/engine/eng_fat.c#L64

You can see that what it is doing is that for specific crypto algs, named by bits a field, an engine is then set as a default for that specific alg. So you can call setEngine() with X for RSA, and B for ECDH, and now you have two default engines. So, what would getEngine() return? It would have to be implemented as getEngine(flag), returning the default engine for a specific alg, and it appears such functions actually exist:

https://github.com/nodejs/node/blob/master/deps/openssl/openssl/crypto/engine/engine.h#L679-L685

Using ENGINE_METHOD_ALL or none would have to be disallowed, and it looks to me like some valid set_default types lack the corresponding get function.

shigeki · 2017-01-19T05:54:58Z

At first, I put a name of crypto.getEngineList() but there are not existing API named get*List() so I changed it.

jasnell · 2017-03-24T23:20:11Z

Ping @sam-github @shigeki ... updates on this one?

shigeki · 2017-03-25T00:15:50Z

CI was fixed to include an engine module. I will make rebase this and resolve conflicts later today.

@sam-github @mscdex We are still in a discussion of the name of this API. I do not have any strong opinion on it. I will take it if both of you can agree.

This adds a new api to show the list of loaded engines of OpenSSL. It also includes the test of dynamic engine for `crypto.setEngine()`.

gibfahn · 2017-03-27T10:53:32Z

@shigeki looks like linuxone failed with:

 g++ -shared -pthread -rdynamic -m64 -march=z196 -lopenssl -L/data/iojs/build/workspace/node-test-commit-linuxone/nodes/rhel72-s390x/out/Release/obj.target/deps/openssl  -Wl,-soname=libnode_test_engine.so -o /data/iojs/build/workspace/node-test-commit-linuxone/nodes/rhel72-s390x/out/Release/obj.target/libnode_test_engine.so -Wl,--start-group /data/iojs/build/workspace/node-test-commit-linuxone/nodes/rhel72-s390x/out/Release/obj.target/node_test_engine/test/fixtures/openssl_test_engine/node_test_engine.o -Wl,--end-group 
s390x/out/Release/obj.target/mkpeephole/deps/v8/src/interpreter/mkpeephole.o.d.raw   -c -o /data/iojs/build/workspace/node-test-commit-linuxone/nodes/rhel72-s390x/out/Release/obj.target/mkpeephole/deps/v8/src/interpreter/mkpeephole.o ../deps/v8/src/interpreter/mkpeephole.cc
cannot find -lopenssl
collect2: error: ld returned 1 exit status
make[2]: *** [/data/iojs/build/workspace/node-test-commit-linuxone/nodes/rhel72-s390x/out/Release/obj.target/libnode_test_engine.so] Error 1
make[2]: *** Waiting for unfinished jobs....

Is this something you expect, or a linuxone specific issue? If the latter, we can get someone to take a look at it.

shigeki · 2017-03-27T14:29:18Z

@gibfahn Yes, I found that a build errors were occurred on some platforms(e.g. lineone, freebsd, 64bit smartos). They seems to be caused by build dependencies for the test engine was built before libopenssl.a. This did not have this error before, I will investigate it.

gibfahn · 2017-03-27T15:22:11Z

cc/ @nodejs/platform-s390 @joransiu @jBarz @jbajwa FYI ^

fhinkel · 2017-05-26T19:01:58Z

@shigeki Looks like this has been sitting here for quite a while. Sorry. Can you rebase so we can ran the CI?

@sam-github Have your comments been addressed?

sam-github · 2017-05-26T19:22:52Z

Its stalled, we are still not sure what to name the API, I think.

bnoordhuis

LGTM modulo style nits, although the build issues probably still need to be worked out.

FWIW, crypto.getEngines() seems like a fine name to me.

bnoordhuis · 2017-05-27T10:48:55Z

test/fixtures/openssl_test_engine/node_test_engine.c

+#include <openssl/engine.h>
+
+static const char *engine_id = "node_test_engine";
+static const char *engine_name = "Node Test Engine for OpenSSL";


Tiny style nit: star leans left (const char* engine_id = ....). You could also writes this as:

static const char engine_id[] = ...;

Saves a pointer indirection.

bnoordhuis · 2017-05-27T10:51:11Z

test/fixtures/openssl_test_engine/node_test_engine.gypi

+  'sources': ['node_test_engine.c'],
+  'conditions': [
+    ['OS=="mac"', {
+      'include_dirs': ['<(PRODUCT_DIR)/../../deps/openssl/openssl/include',],


Superfluous comma, also in the other include_dirs lists.

bnoordhuis · 2017-05-27T10:51:39Z

test/parallel/test-crypto-engine.js

+  engine_lib = 'node_test_engine.dll';
+} else if (common.isAix) {
+  engine_lib = 'libnode_test_engine.a';
+} else if (process.platform === 'darwin') {


common.isOSX

bnoordhuis · 2017-05-27T10:53:07Z

test/parallel/test-crypto-engine.js

+
+const test_engine_id = 'node_test_engine';
+const test_engine_name = 'Node Test Engine for OpenSSL';
+const test_engine = path.join(path.dirname(process.execPath), engine_lib);


Tiny style nit: JS code uses camelCase, not snake_case (for the most part.)

bnoordhuis · 2017-05-27T10:53:15Z

test/parallel/test-crypto-engine.js

+
+assert.strictEqual(found, 1);
+
+// check set and get node test engine of


s/check/Check/

sam-github · 2017-01-19T18:08:27Z

doc/api/crypto.md

+
+Returns an array of objects with id, name and flags of loaded engines.
+The flags value represents an integer of one of or a mix of the crypto
+constants described above.


docs can move around, better to link to crypto.setEngine() explicitly

sam-github · 2017-01-19T18:13:13Z

src/node_crypto.cc

+
+    if (id == nullptr || name == nullptr) {
+      ENGINE_free(e);
+      return env->ThrowError("Invalid Engine: id or name is not found.");


I wonder if this is the right approach. If there is an engine with an id, but no name, then this will throw, but the user won't know what engine it is that has a name, it could be very frustrating. What about just using "" for a missing id or name, as a replacment for NULL? Or would that be terrible? I'm not sure how common it is that engines don't have this data, I would have hoped OpenSSL did not allow invalid engines!

sam-github · 2017-01-19T18:16:27Z

test/parallel/test-crypto-engine.js

+  fs.accessSync(test_engine);
+}, 'node test engine ' + test_engine + ' is not found.');
+
+crypto.setEngine(test_engine);


Tests should test invalid behaviours, and obscure corner conditions. If it is allowed to call multiple times, the test should do, and assert on the result, IMO. It would be possible that the second call always fails, for example, or someone might believe that both "RSA" engines get added (which isn't true). The docs don't actually say what happens when setEngine() is called multiple times with the same flag. The latest call will replace the previous default, but its not stated.

sam-github · 2017-01-19T18:17:46Z

test/parallel/test-crypto-engine.js

+crypto.getEngines().forEach((e) => {
+  if (e.id === 'dynamic')
+    found++;
+});


by reimplementing filter() and ()=> with forEach(), this code is 5 lines instead of 1.

You mean

found += crypto.getEngines().filter(e => e.id === 'dynamic').length

?

sam-github · 2017-01-19T18:22:02Z

test/parallel/test-crypto-engine.js

+
+crypto.setEngine(test_engine);
+
+crypto.getEngines().forEach((e) => {


Its true that many of the Array methods can be implemented in terms of forEach(), so they are equivalent in that sense. Someone has to read this code, and understand that it is filtering. Better to just call filter(). And if you don't, @Trott or one of the code-and-learn people may come through and modify this to use es6 and builtin library functions - better avoid the churn and do it now, I think.

BridgeAR · 2017-08-26T07:28:00Z

Should this be closed or is this something that is still worked on?
If the latter, it would need a rebase.

BridgeAR · 2017-09-08T01:47:31Z

Ping @shigeki

BridgeAR · 2017-09-23T01:22:34Z

Ping @shigeki again

BridgeAR · 2017-09-23T01:24:31Z

Or after looking at this again - I am actually going to close this. There was no response since March.

@shigeki if you would like to pursue this further, please reopen.

shigeki added crypto Issues and PRs related to the crypto subsystem. semver-minor PRs that contain new features and should be released in the next minor version. labels Jan 18, 2017

nodejs-github-bot added build Issues and PRs related to build files or the CI. c++ Issues and PRs that require attention from people who are familiar with C++. crypto Issues and PRs related to the crypto subsystem. lts-watch-v6.x labels Jan 18, 2017

mscdex reviewed Jan 18, 2017

View reviewed changes

shigeki mentioned this pull request Jan 18, 2017

Include shared library file in `binary/binary.tar.[xz,gz] in Win and arm tests nodejs/build#596

Closed

sam-github suggested changes Jan 18, 2017

View reviewed changes

nodejs-github-bot added the lts-watch-v6.x label Jan 19, 2017

shigeki mentioned this pull request Feb 3, 2017

test: improve crypto.setEngine coverage to check for errors #11143

Closed

3 tasks

shigeki force-pushed the get_engines branch from dc821ff to 513ef8d Compare February 7, 2017 04:10

nodejs-github-bot removed the lts-watch-v6.x label Feb 7, 2017

jasnell added the stalled Issues and PRs that are stalled. label Mar 24, 2017

crypto: Add crypto.getEngines()

946c770

This adds a new api to show the list of loaded engines of OpenSSL. It also includes the test of dynamic engine for `crypto.setEngine()`.

shigeki force-pushed the get_engines branch from 513ef8d to 946c770 Compare March 25, 2017 14:37

refack force-pushed the master branch from 16073c0 to fbe946b Compare April 14, 2017 04:11

fhinkel approved these changes May 26, 2017

View reviewed changes

bnoordhuis approved these changes May 27, 2017

View reviewed changes

sam-github approved these changes May 28, 2017

View reviewed changes

BridgeAR closed this Sep 23, 2017

@@ @@ -947,6 +947,18 @@ @@
                         ],
                       }],
                     ]
+                  },
+                  {


		crypto.setEngine(test_engine);

		crypto.getEngines().forEach((e) => {


		assert.strictEqual(found, 1);

		// check set and get node test engine of

crypto: Add crypto.getEngines() #10865

crypto: Add crypto.getEngines() #10865

Conversation

shigeki commented Jan 18, 2017

Checklist

Affected core subsystem(s)

shigeki commented Jan 18, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mscdex commented Jan 18, 2017

shigeki commented Jan 18, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sam-github Jan 19, 2017 • edited by gibfahn Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sam-github commented Jan 18, 2017

mscdex commented Jan 18, 2017

sam-github commented Jan 18, 2017

shigeki commented Jan 19, 2017

jasnell commented Mar 24, 2017

shigeki commented Mar 25, 2017

gibfahn commented Mar 27, 2017 • edited Loading

shigeki commented Mar 27, 2017

gibfahn commented Mar 27, 2017

fhinkel commented May 26, 2017 • edited Loading

sam-github commented May 26, 2017

bnoordhuis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sam-github Jan 19, 2017 • edited by gibfahn Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BridgeAR commented Aug 26, 2017

BridgeAR commented Sep 8, 2017

BridgeAR commented Sep 23, 2017

BridgeAR commented Sep 23, 2017

sam-github Jan 19, 2017 •

edited by gibfahn

Loading

gibfahn commented Mar 27, 2017 •

edited

Loading

fhinkel commented May 26, 2017 •

edited

Loading

sam-github Jan 19, 2017 •

edited by gibfahn

Loading