Skip to content

Commit

Permalink
tls: implement clientCertEngine option
Browse files Browse the repository at this point in the history
Add an option 'clientCertEngine' to `tls.createSecureContext()` which gets
wired up to OpenSSL function `SSL_CTX_set_client_cert_engine`. The option
is passed through from `https.request()` as well. This allows using a custom
OpenSSL engine to provide the client certificate.
  • Loading branch information
joelostrowski authored and MylesBorins committed Dec 11, 2017
1 parent 3b1db7f commit 33c1e8b
Show file tree
Hide file tree
Showing 11 changed files with 305 additions and 15 deletions.
6 changes: 6 additions & 0 deletions doc/api/errors.md
Original file line number Diff line number Diff line change
Expand Up @@ -631,6 +631,12 @@ Used when `Console` is instantiated without `stdout` stream or when `stdout` or

Used when the native call from `process.cpuUsage` cannot be processed properly.

<a id="ERR_CRYPTO_CUSTOM_ENGINE_NOT_SUPPORTED"></a>
### ERR_CRYPTO_CUSTOM_ENGINE_NOT_SUPPORTED

Used when a client certificate engine is requested that is not supported by the
version of OpenSSL being used.

<a id="ERR_CRYPTO_ECDH_INVALID_FORMAT"></a>
### ERR_CRYPTO_ECDH_INVALID_FORMAT

Expand Down
12 changes: 12 additions & 0 deletions lib/_tls_common.js
Original file line number Diff line number Diff line change
Expand Up @@ -208,6 +208,18 @@ exports.createSecureContext = function createSecureContext(options, context) {
c.context.setFreeListLength(0);
}

if (typeof options.clientCertEngine === 'string') {
if (c.context.setClientCertEngine)
c.context.setClientCertEngine(options.clientCertEngine);
else
throw new errors.Error('ERR_CRYPTO_CUSTOM_ENGINE_NOT_SUPPORTED');
} else if (options.clientCertEngine != null) {
throw new errors.TypeError('ERR_INVALID_ARG_TYPE',
'options.clientCertEngine',
['string', 'null', 'undefined'],
options.clientCertEngine);
}

return c;
};

Expand Down
4 changes: 4 additions & 0 deletions lib/_tls_wrap.js
Original file line number Diff line number Diff line change
Expand Up @@ -816,6 +816,7 @@ function tlsConnectionListener(rawSocket) {
// - rejectUnauthorized. Boolean, default to true.
// - key. string.
// - cert: string.
// - clientCertEngine: string.
// - ca: string or array of strings.
// - sessionTimeout: integer.
//
Expand Down Expand Up @@ -859,6 +860,7 @@ function Server(options, listener) {
key: this.key,
passphrase: this.passphrase,
cert: this.cert,
clientCertEngine: this.clientCertEngine,
ca: this.ca,
ciphers: this.ciphers,
ecdhCurve: this.ecdhCurve,
Expand Down Expand Up @@ -931,6 +933,8 @@ Server.prototype.setOptions = function(options) {
if (options.key) this.key = options.key;
if (options.passphrase) this.passphrase = options.passphrase;
if (options.cert) this.cert = options.cert;
if (options.clientCertEngine)
this.clientCertEngine = options.clientCertEngine;
if (options.ca) this.ca = options.ca;
if (options.secureProtocol) this.secureProtocol = options.secureProtocol;
if (options.crl) this.crl = options.crl;
Expand Down
4 changes: 4 additions & 0 deletions lib/https.js
Original file line number Diff line number Diff line change
Expand Up @@ -160,6 +160,10 @@ Agent.prototype.getName = function getName(options) {
if (options.cert)
name += options.cert;

name += ':';
if (options.clientCertEngine)
name += options.clientCertEngine;

name += ':';
if (options.ciphers)
name += options.ciphers;
Expand Down
2 changes: 2 additions & 0 deletions lib/internal/errors.js
Original file line number Diff line number Diff line change
Expand Up @@ -155,6 +155,8 @@ E('ERR_CHILD_CLOSED_BEFORE_REPLY', 'Child closed before reply received');
E('ERR_CONSOLE_WRITABLE_STREAM',
'Console expects a writable stream instance for %s');
E('ERR_CPU_USAGE', 'Unable to obtain cpu usage %s');
E('ERR_CRYPTO_CUSTOM_ENGINE_NOT_SUPPORTED',
'Custom engines not supported by this OpenSSL');
E('ERR_CRYPTO_ECDH_INVALID_FORMAT', 'Invalid ECDH format: %s');
E('ERR_CRYPTO_ENGINE_UNKNOWN', 'Engine "%s" was not found');
E('ERR_CRYPTO_FIPS_FORCED',
Expand Down
95 changes: 82 additions & 13 deletions src/node_crypto.cc
Original file line number Diff line number Diff line change
Expand Up @@ -361,6 +361,41 @@ static int PasswordCallback(char *buf, int size, int rwflag, void *u) {
return 0;
}

// Loads OpenSSL engine by engine id and returns it. The loaded engine
// gets a reference so remember the corresponding call to ENGINE_free.
// In case of error the appropriate js exception is scheduled
// and nullptr is returned.
#ifndef OPENSSL_NO_ENGINE
static ENGINE* LoadEngineById(const char* engine_id, char (*errmsg)[1024]) {
MarkPopErrorOnReturn mark_pop_error_on_return;

ENGINE* engine = ENGINE_by_id(engine_id);

if (engine == nullptr) {
// Engine not found, try loading dynamically.
engine = ENGINE_by_id("dynamic");
if (engine != nullptr) {
if (!ENGINE_ctrl_cmd_string(engine, "SO_PATH", engine_id, 0) ||
!ENGINE_ctrl_cmd_string(engine, "LOAD", nullptr, 0)) {
ENGINE_free(engine);
engine = nullptr;
}
}
}

if (engine == nullptr) {
int err = ERR_get_error();
if (err != 0) {
ERR_error_string_n(err, *errmsg, sizeof(*errmsg));
} else {
snprintf(*errmsg, sizeof(*errmsg),
"Engine \"%s\" was not found", engine_id);
}
}

return engine;
}
#endif // !OPENSSL_NO_ENGINE

// This callback is used to avoid the default passphrase callback in OpenSSL
// which will typically prompt for the passphrase. The prompting is designed
Expand Down Expand Up @@ -505,6 +540,10 @@ void SecureContext::Initialize(Environment* env, Local<Object> target) {
SecureContext::SetSessionTimeout);
env->SetProtoMethod(t, "close", SecureContext::Close);
env->SetProtoMethod(t, "loadPKCS12", SecureContext::LoadPKCS12);
#ifndef OPENSSL_NO_ENGINE
env->SetProtoMethod(t, "setClientCertEngine",
SecureContext::SetClientCertEngine);
#endif // !OPENSSL_NO_ENGINE
env->SetProtoMethod(t, "getTicketKeys", SecureContext::GetTicketKeys);
env->SetProtoMethod(t, "setTicketKeys", SecureContext::SetTicketKeys);
env->SetProtoMethod(t, "setFreeListLength", SecureContext::SetFreeListLength);
Expand Down Expand Up @@ -1302,6 +1341,46 @@ void SecureContext::LoadPKCS12(const FunctionCallbackInfo<Value>& args) {
}


#ifndef OPENSSL_NO_ENGINE
void SecureContext::SetClientCertEngine(
const FunctionCallbackInfo<Value>& args) {
Environment* env = Environment::GetCurrent(args);
CHECK_EQ(args.Length(), 1);
CHECK(args[0]->IsString());

SecureContext* sc = Unwrap<SecureContext>(args.This());

MarkPopErrorOnReturn mark_pop_error_on_return;

// SSL_CTX_set_client_cert_engine does not itself support multiple
// calls by cleaning up before overwriting the client_cert_engine
// internal context variable.
// Instead of trying to fix up this problem we in turn also do not
// support multiple calls to SetClientCertEngine.
if (sc->client_cert_engine_provided_) {
return env->ThrowError(
"Multiple calls to SetClientCertEngine are not allowed");
}

const node::Utf8Value engine_id(env->isolate(), args[0]);
char errmsg[1024];
ENGINE* engine = LoadEngineById(*engine_id, &errmsg);

if (engine == nullptr) {
return env->ThrowError(errmsg);
}

int r = SSL_CTX_set_client_cert_engine(sc->ctx_, engine);
// Free reference (SSL_CTX_set_client_cert_engine took it via ENGINE_init).
ENGINE_free(engine);
if (r == 0) {
return ThrowCryptoError(env, ERR_get_error());
}
sc->client_cert_engine_provided_ = true;
}
#endif // !OPENSSL_NO_ENGINE


void SecureContext::GetTicketKeys(const FunctionCallbackInfo<Value>& args) {
#if !defined(OPENSSL_NO_TLSEXT) && defined(SSL_CTX_get_tlsext_ticket_keys)

Expand Down Expand Up @@ -6124,20 +6203,10 @@ void SetEngine(const FunctionCallbackInfo<Value>& args) {

ClearErrorOnReturn clear_error_on_return;

// Load engine.
const node::Utf8Value engine_id(env->isolate(), args[0]);
ENGINE* engine = ENGINE_by_id(*engine_id);

// Engine not found, try loading dynamically
if (engine == nullptr) {
engine = ENGINE_by_id("dynamic");
if (engine != nullptr) {
if (!ENGINE_ctrl_cmd_string(engine, "SO_PATH", *engine_id, 0) ||
!ENGINE_ctrl_cmd_string(engine, "LOAD", nullptr, 0)) {
ENGINE_free(engine);
engine = nullptr;
}
}
}
char errmsg[1024];
ENGINE* engine = LoadEngineById(*engine_id, &errmsg);

if (engine == nullptr) {
int err = ERR_get_error();
Expand Down
7 changes: 7 additions & 0 deletions src/node_crypto.h
Original file line number Diff line number Diff line change
Expand Up @@ -93,6 +93,9 @@ class SecureContext : public BaseObject {
SSL_CTX* ctx_;
X509* cert_;
X509* issuer_;
#ifndef OPENSSL_NO_ENGINE
bool client_cert_engine_provided_ = false;
#endif // !OPENSSL_NO_ENGINE

static const int kMaxSessionSize = 10 * 1024;

Expand Down Expand Up @@ -135,6 +138,10 @@ class SecureContext : public BaseObject {
const v8::FunctionCallbackInfo<v8::Value>& args);
static void Close(const v8::FunctionCallbackInfo<v8::Value>& args);
static void LoadPKCS12(const v8::FunctionCallbackInfo<v8::Value>& args);
#ifndef OPENSSL_NO_ENGINE
static void SetClientCertEngine(
const v8::FunctionCallbackInfo<v8::Value>& args);
#endif // !OPENSSL_NO_ENGINE
static void GetTicketKeys(const v8::FunctionCallbackInfo<v8::Value>& args);
static void SetTicketKeys(const v8::FunctionCallbackInfo<v8::Value>& args);
static void SetFreeListLength(
Expand Down
24 changes: 24 additions & 0 deletions test/addons/openssl-client-cert-engine/binding.gyp
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
{
'targets': [
{
'target_name': 'testengine',
'type': 'none',
'conditions': [
['OS=="mac" and '
'node_use_openssl=="true" and '
'node_shared=="false" and '
'node_shared_openssl=="false"', {
'type': 'shared_library',
'sources': [ 'testengine.cc' ],
'product_extension': 'engine',
'include_dirs': ['../../../deps/openssl/openssl/include'],
'link_settings': {
'libraries': [
'../../../../out/<(PRODUCT_DIR)/<(OPENSSL_PRODUCT)'
]
},
}]
]
}
]
}
62 changes: 62 additions & 0 deletions test/addons/openssl-client-cert-engine/test.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,62 @@
'use strict';
const common = require('../../common');
const fixture = require('../../common/fixtures');

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

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

const engine = path.join(__dirname,
`/build/${common.buildType}/testengine.engine`);

if (!fs.existsSync(engine))
common.skip('no client cert engine');

const assert = require('assert');
const https = require('https');

const agentKey = fs.readFileSync(fixture.path('/keys/agent1-key.pem'));
const agentCert = fs.readFileSync(fixture.path('/keys/agent1-cert.pem'));
const agentCa = fs.readFileSync(fixture.path('/keys/ca1-cert.pem'));

const port = common.PORT;

const serverOptions = {
key: agentKey,
cert: agentCert,
ca: agentCa,
requestCert: true,
rejectUnauthorized: true
};

const server = https.createServer(serverOptions, (req, res) => {
res.writeHead(200);
res.end('hello world');
}).listen(port, common.localhostIPv4, () => {
const clientOptions = {
method: 'GET',
host: common.localhostIPv4,
port: port,
path: '/test',
clientCertEngine: engine, // engine will provide key+cert
rejectUnauthorized: false, // prevent failing on self-signed certificates
headers: {}
};

const req = https.request(clientOptions, common.mustCall(function(response) {
let body = '';
response.setEncoding('utf8');
response.on('data', function(chunk) {
body += chunk;
});

response.on('end', common.mustCall(function() {
assert.strictEqual(body, 'hello world');
server.close();
}));
}));

req.end();
});
Loading

1 comment on commit 33c1e8b

@gibfahn
Copy link
Member

@gibfahn gibfahn commented on 33c1e8b Dec 19, 2017

Choose a reason for hiding this comment

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

SEMVER-MINOR

PR-URL: #14903
Reviewed-By: Daniel Bevenius [email protected]
Reviewed-By: Fedor Indutny [email protected]
Reviewed-By: Anna Henningsen [email protected]
Reviewed-By: Ben Noordhuis [email protected]

Please sign in to comment.