From 33c1e8b3d56fc4546a45489e05817725a95b53b9 Mon Sep 17 00:00:00 2001
From: joelostrowski <jo@craftwork.dk>
Date: Fri, 15 Apr 2016 16:49:36 +0200
Subject: [PATCH] tls: implement clientCertEngine option

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.
---
 doc/api/errors.md                             |   6 ++
 lib/_tls_common.js                            |  12 +++
 lib/_tls_wrap.js                              |   4 +
 lib/https.js                                  |   4 +
 lib/internal/errors.js                        |   2 +
 src/node_crypto.cc                            |  95 ++++++++++++++---
 src/node_crypto.h                             |   7 ++
 .../openssl-client-cert-engine/binding.gyp    |  24 +++++
 .../addons/openssl-client-cert-engine/test.js |  62 +++++++++++
 .../openssl-client-cert-engine/testengine.cc  | 100 ++++++++++++++++++
 test/parallel/test-https-agent-getname.js     |   4 +-
 11 files changed, 305 insertions(+), 15 deletions(-)
 create mode 100644 test/addons/openssl-client-cert-engine/binding.gyp
 create mode 100644 test/addons/openssl-client-cert-engine/test.js
 create mode 100644 test/addons/openssl-client-cert-engine/testengine.cc

diff --git a/doc/api/errors.md b/doc/api/errors.md
index 0475cfb1e0954a..3455d03e68793e 100644
--- a/doc/api/errors.md
+++ b/doc/api/errors.md
@@ -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
 
diff --git a/lib/_tls_common.js b/lib/_tls_common.js
index 75eb6a2ec53449..fcb6de81669e23 100644
--- a/lib/_tls_common.js
+++ b/lib/_tls_common.js
@@ -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;
 };
 
diff --git a/lib/_tls_wrap.js b/lib/_tls_wrap.js
index 7a7371ae9eb8f5..3e6f2b06423f44 100644
--- a/lib/_tls_wrap.js
+++ b/lib/_tls_wrap.js
@@ -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.
 //
@@ -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,
@@ -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;
diff --git a/lib/https.js b/lib/https.js
index f6e5a533b95084..5013791fe2de25 100644
--- a/lib/https.js
+++ b/lib/https.js
@@ -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;
diff --git a/lib/internal/errors.js b/lib/internal/errors.js
index b898c9fdce82d8..8da4e39f19dce6 100644
--- a/lib/internal/errors.js
+++ b/lib/internal/errors.js
@@ -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',
diff --git a/src/node_crypto.cc b/src/node_crypto.cc
index 2a641ad38017fb..ed2782e78978a1 100644
--- a/src/node_crypto.cc
+++ b/src/node_crypto.cc
@@ -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
@@ -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);
@@ -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)
 
@@ -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();
diff --git a/src/node_crypto.h b/src/node_crypto.h
index c3bc5d24c36dbd..a9719dec257d26 100644
--- a/src/node_crypto.h
+++ b/src/node_crypto.h
@@ -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;
 
@@ -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(
diff --git a/test/addons/openssl-client-cert-engine/binding.gyp b/test/addons/openssl-client-cert-engine/binding.gyp
new file mode 100644
index 00000000000000..b069e43429c12b
--- /dev/null
+++ b/test/addons/openssl-client-cert-engine/binding.gyp
@@ -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)'
+            ]
+          },
+        }]
+      ]
+    }
+  ]
+}
diff --git a/test/addons/openssl-client-cert-engine/test.js b/test/addons/openssl-client-cert-engine/test.js
new file mode 100644
index 00000000000000..d07b9c6a1c2c81
--- /dev/null
+++ b/test/addons/openssl-client-cert-engine/test.js
@@ -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();
+});
diff --git a/test/addons/openssl-client-cert-engine/testengine.cc b/test/addons/openssl-client-cert-engine/testengine.cc
new file mode 100644
index 00000000000000..078695a0566691
--- /dev/null
+++ b/test/addons/openssl-client-cert-engine/testengine.cc
@@ -0,0 +1,100 @@
+#include <assert.h>
+#include <string.h>
+#include <stdlib.h>
+
+#include <openssl/engine.h>
+#include <openssl/pem.h>
+
+#include <fstream>
+#include <iterator>
+#include <string>
+
+#ifndef ENGINE_CMD_BASE
+# error did not get engine.h
+#endif
+
+#define TEST_ENGINE_ID      "testengine"
+#define TEST_ENGINE_NAME    "dummy test engine"
+
+#define AGENT_KEY           "test/fixtures/keys/agent1-key.pem"
+#define AGENT_CERT          "test/fixtures/keys/agent1-cert.pem"
+
+namespace {
+
+int EngineInit(ENGINE* engine) {
+  return 1;
+}
+
+int EngineFinish(ENGINE* engine) {
+  return 1;
+}
+
+int EngineDestroy(ENGINE* engine) {
+  return 1;
+}
+
+std::string LoadFile(const char* filename) {
+  std::ifstream file(filename);
+  return std::string(std::istreambuf_iterator<char>(file),
+                     std::istreambuf_iterator<char>());
+}
+
+
+int EngineLoadSSLClientCert(ENGINE* engine,
+                            SSL* ssl,
+                            STACK_OF(X509_NAME)* ca_dn,
+                            X509** ppcert,
+                            EVP_PKEY** ppkey,
+                            STACK_OF(X509)** pother,
+                            UI_METHOD* ui_method,
+                            void* callback_data) {
+  if (ppcert != nullptr) {
+    std::string cert = LoadFile(AGENT_CERT);
+    if (cert.empty()) {
+      return 0;
+    }
+
+    BIO* bio = BIO_new_mem_buf(cert.data(), cert.size());
+    *ppcert = PEM_read_bio_X509(bio, nullptr, nullptr, nullptr);
+     BIO_vfree(bio);
+     if (*ppcert == nullptr) {
+       printf("Could not read certificate\n");
+       return 0;
+     }
+  }
+
+  if (ppkey != nullptr) {
+    std::string key = LoadFile(AGENT_KEY);
+    if (key.empty()) {
+      return 0;
+    }
+
+    BIO* bio = BIO_new_mem_buf(key.data(), key.size());
+    *ppkey = PEM_read_bio_PrivateKey(bio, nullptr, nullptr, nullptr);
+    BIO_vfree(bio);
+    if (*ppkey == nullptr) {
+      printf("Could not read private key\n");
+      return 0;
+    }
+  }
+
+  return 1;
+}
+
+int bind_fn(ENGINE* engine, const char* id) {
+  ENGINE_set_id(engine, TEST_ENGINE_ID);
+  ENGINE_set_name(engine, TEST_ENGINE_NAME);
+  ENGINE_set_init_function(engine, EngineInit);
+  ENGINE_set_finish_function(engine, EngineFinish);
+  ENGINE_set_destroy_function(engine, EngineDestroy);
+  ENGINE_set_load_ssl_client_cert_function(engine, EngineLoadSSLClientCert);
+
+  return 1;
+}
+
+extern "C" {
+  IMPLEMENT_DYNAMIC_CHECK_FN();
+  IMPLEMENT_DYNAMIC_BIND_FN(bind_fn);
+}
+
+}  // anonymous namespace
diff --git a/test/parallel/test-https-agent-getname.js b/test/parallel/test-https-agent-getname.js
index 0986f8472de871..0cdc9568d84470 100644
--- a/test/parallel/test-https-agent-getname.js
+++ b/test/parallel/test-https-agent-getname.js
@@ -12,7 +12,7 @@ const agent = new https.Agent();
 // empty options
 assert.strictEqual(
   agent.getName({}),
-  'localhost::::::::::'
+  'localhost:::::::::::'
 );
 
 // pass all options arguments
@@ -31,5 +31,5 @@ const options = {
 
 assert.strictEqual(
   agent.getName(options),
-  '0.0.0.0:443:192.168.1.1:ca:cert:ciphers:key:pfx:false:localhost:'
+  '0.0.0.0:443:192.168.1.1:ca:cert::ciphers:key:pfx:false:localhost:'
 );