Skip to content

crypto: Add crypto.getEngines() #10865

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.

Sign up for GitHub

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

Closed
shigeki wants to merge 1 commit into from
Closed

crypto: Add crypto.getEngines() #10865

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 19 additions & 0 deletions doc/api/crypto.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1698,6 +1698,25 @@ is a bit field taking one of or a mix of the following flags (defined in
* `crypto.constants.ENGINE_METHOD_ALL`
* `crypto.constants.ENGINE_METHOD_NONE`

### crypto.getEngines()
<!-- YAML
added: REPLACEME
-->

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.

```js
const crypto = require('crypto');
console.log(crypto.getEngines());
// Prints:
// [ { id: 'rdrand', name: 'Intel RDRAND engine', flags: 8 },
// { id: 'dynamic',
// name: 'Dynamic engine loading support',
// flags: 4 } ]
```

### crypto.timingSafeEqual(a, b)
<!-- YAML
added: v6.6.0
Expand Down
3 changes: 3 additions & 0 deletions lib/crypto.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -650,6 +650,9 @@ exports.setEngine = function setEngine(id, flags) {
return binding.setEngine(id, flags);
};


exports.getEngines = binding.getEngines;

exports.randomBytes = exports.pseudoRandomBytes = randomBytes;

exports.rng = exports.prng = randomBytes;
Expand Down
12 changes: 12 additions & 0 deletions node.gyp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -691,6 +691,18 @@
'ldflags': [ '-I<(SHARED_INTERMEDIATE_DIR)' ]
}],
]
},
{
# node test engine used in test/parallel/test-crypto-engine.js
'target_name': 'node_test_engine',
'type': 'loadable_module',
'conditions': [
['node_use_openssl=="true" and '
'node_shared_openssl=="false" and '
'openssl_fips==""', { # fipsld fails to link libcrypto.a
'includes': ['test/fixtures/openssl_test_engine/node_test_engine.gypi'],
}],
],
}
], # end targets

Expand Down
1 change: 1 addition & 0 deletions src/env.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -104,6 +104,7 @@ namespace node {
V(emitting_top_level_domain_error_string, "_emittingTopLevelDomainError") \
V(exchange_string, "exchange") \
V(enumerable_string, "enumerable") \
V(id_string, "id") \
V(idle_string, "idle") \
V(irq_string, "irq") \
V(encoding_string, "encoding") \
Expand Down
31 changes: 31 additions & 0 deletions src/node_crypto.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5988,11 +5988,41 @@ void SetEngine(const FunctionCallbackInfo<Value>& args) {
}
}

ENGINE_set_flags(engine, flags);
int r = ENGINE_set_default(engine, flags);
ENGINE_free(engine);
if (r == 0)
return ThrowCryptoError(env, ERR_get_error());
}


void GetEngines(const FunctionCallbackInfo<Value>& args) {
Environment* env = Environment::GetCurrent(args);
ENGINE* e;
int i = 0;
Local<Array> arr = Array::New(env->isolate());

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);
Copy link
Copy Markdown
Contributor

@sam-github sam-github Jan 18, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Contributor Author

@shigeki shigeki Jan 19, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

int flags = ENGINE_get_flags(e);

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

@sam-github sam-github Jan 19, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

}

Local<Object> obj = Object::New(env->isolate());
obj->Set(env->id_string(), OneByteString(env->isolate(), id));
obj->Set(env->name_string(), OneByteString(env->isolate(), name));
obj->Set(env->flags_string(), Integer::New(env->isolate(), flags));
arr->Set(i++, obj);
}

ENGINE_free(e);

args.GetReturnValue().Set(arr);
}
#endif // !OPENSSL_NO_ENGINE

void GetFipsCrypto(const FunctionCallbackInfo<Value>& args) {
Expand Down Expand Up @@ -6042,6 +6072,7 @@ void InitCrypto(Local<Object> target,
env->SetMethod(target, "certExportChallenge", ExportChallenge);
#ifndef OPENSSL_NO_ENGINE
env->SetMethod(target, "setEngine", SetEngine);
env->SetMethod(target, "getEngines", GetEngines);
#endif // !OPENSSL_NO_ENGINE
env->SetMethod(target, "getFipsCrypto", GetFipsCrypto);
env->SetMethod(target, "setFipsCrypto", SetFipsCrypto);
Expand Down
1 change: 1 addition & 0 deletions src/node_crypto.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -744,6 +744,7 @@ class ECDH : public BaseObject {
bool EntropySource(unsigned char* buffer, size_t length);
#ifndef OPENSSL_NO_ENGINE
void SetEngine(const v8::FunctionCallbackInfo<v8::Value>& args);
void GetEngines(const v8::FunctionCallbackInfo<v8::Value>& args);
#endif // !OPENSSL_NO_ENGINE
void InitCrypto(v8::Local<v8::Object> target);

Expand Down
14 changes: 14 additions & 0 deletions test/fixtures/openssl_test_engine/node_test_engine.c
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
#include <openssl/engine.h>

static const char *engine_id = "node_test_engine";
static const char *engine_name = "Node Test Engine for OpenSSL";
Copy link
Copy Markdown
Member

@bnoordhuis bnoordhuis May 27, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


static int bind(ENGINE *e, const char *id)
{
ENGINE_set_id(e, engine_id);
ENGINE_set_name(e, engine_name);
return 1;
}

IMPLEMENT_DYNAMIC_BIND_FN(bind)
IMPLEMENT_DYNAMIC_CHECK_FN()
39 changes: 39 additions & 0 deletions test/fixtures/openssl_test_engine/node_test_engine.gypi
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
{
'sources': ['node_test_engine.c'],
'conditions': [
['OS=="mac"', {
'include_dirs': ['<(PRODUCT_DIR)/../../deps/openssl/openssl/include',],
Copy link
Copy Markdown
Member

@bnoordhuis bnoordhuis May 27, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Superfluous comma, also in the other include_dirs lists.

'library_dirs': ['<(LIB_DIR)'],
'libraries': ['-lopenssl'],
}, 'OS=="win"', {
'dependencies': [
'./deps/openssl/openssl.gyp:openssl',
],
'include_dirs': ['<(PRODUCT_DIR)/../deps/openssl/openssl/include',],
'library_dirs': ['<(LIB_DIR)'],
'libraries': [
'-lkernel32.lib',
'-luser32.lib',
'-lgdi32.lib',
'-lwinspool.lib',
'-lcomdlg32.lib',
'-ladvapi32.lib',
'-lshell32.lib',
'-lole32.lib',
'-loleaut32.lib',
'-luuid.lib',
'-lodbc32.lib',
'-lDelayImp.lib',
'-lopenssl.lib',
],
}, {
'library_dirs': ['<(LIB_DIR)/deps/openssl'],
'ldflags': ['-lopenssl'],
'include_dirs': ['<(PRODUCT_DIR)/../../deps/openssl/openssl/include',],
}],
[ 'OS in "freebsd openbsd netbsd solaris" or \
(OS=="linux" and target_arch!="ia32")', {
'cflags': ['-fPIC'],
}],
],
}
62 changes: 62 additions & 0 deletions test/parallel/test-crypto-engine.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,14 +1,76 @@
'use strict';
const common = require('../common');

// This test ensures that a dynamic engine can be registered with
// crypto.setEngine() and crypto.getEngines() obtains the list for
// both builtin and dynamic engines.

if (!common.hasCrypto) {
common.skip('missing crypto');
return;
}

const assert = require('assert');
const crypto = require('crypto');
const fs = require('fs');
const path = require('path');

let found = 0;

// check builtin engine exists.
// A dynamic engine is loaded by ENGINE_load_builtin_engines()
// in InitCryptoOnce().
crypto.getEngines().forEach((e) => {
if (e.id === 'dynamic')
found++;
});
Copy link
Copy Markdown
Contributor

@sam-github sam-github Jan 19, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Member

@gibfahn gibfahn May 28, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You mean

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

?


assert.strictEqual(found, 1);
Copy link
Copy Markdown
Contributor

@sam-github sam-github Jan 18, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

assert conflicts with comment about "at least one"

Copy link
Copy Markdown
Contributor Author

@shigeki shigeki Jan 19, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The description of comment was wrong. Fixed.


// check set and get node test engine of
Copy link
Copy Markdown
Member

@bnoordhuis bnoordhuis May 27, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/check/Check/

// test/fixtures/openssl_test_engine/node_test_engine.c

if (process.config.variables.node_shared_openssl) {
common.skip('node test engine cannot be built in shared openssl');
return;
}

if (process.config.variables.openssl_fips) {
common.skip('node test engine cannot be built in FIPS mode.');
return;
}

let engine_lib;

if (common.isWindows) {
engine_lib = 'node_test_engine.dll';
} else if (common.isAix) {
engine_lib = 'libnode_test_engine.a';
} else if (process.platform === 'darwin') {
Copy link
Copy Markdown
Member

@bnoordhuis bnoordhuis May 27, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

common.isOSX

engine_lib = 'node_test_engine.so';
} else {
engine_lib = 'libnode_test_engine.so';
}

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);
Copy link
Copy Markdown
Member

@bnoordhuis bnoordhuis May 27, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


assert.doesNotThrow(function() {
fs.accessSync(test_engine);
}, 'node test engine ' + test_engine + ' is not found.');

crypto.setEngine(test_engine);
Copy link
Copy Markdown
Contributor

@sam-github sam-github Jan 18, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe call twice to prove its possible?

Copy link
Copy Markdown
Contributor Author

@shigeki shigeki Jan 19, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Contributor

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


crypto.getEngines().forEach((e) => {
Copy link
Copy Markdown
Contributor

@sam-github sam-github Jan 18, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Contributor Author

@shigeki shigeki Jan 19, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Contributor

@sam-github sam-github Jan 19, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

if (e.id === test_engine_id && e.name === test_engine_name &&
e.flags === crypto.constants.ENGINE_METHOD_ALL)
found++;
});

assert.strictEqual(found, 2);

// Error Tests for setEngine
assert.throws(function() {
crypto.setEngine(true);
}, /^TypeError: "id" argument should be a string$/);
Expand Down