From cf7b4056ca181b36b341115e0929891df9916231 Mon Sep 17 00:00:00 2001
From: Anton Gerasimov <agerasimov@twilio.com>
Date: Mon, 5 Aug 2019 12:03:23 +0200
Subject: [PATCH] tls: add option for private keys for OpenSSL engines

Add `privateKeyIdentifier` and `privateKeyEngine` options
to get private key from an OpenSSL engine in tls.createSecureContext().

PR-URL: https://github.com/nodejs/node/pull/28973
Reviewed-By: Rod Vagg <rod@vagg.org>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
---
 doc/api/tls.md                                | 10 +++
 lib/_tls_common.js                            | 30 ++++++++
 src/node_crypto.cc                            | 56 +++++++++++++-
 src/node_crypto.h                             |  4 +
 test/addons/openssl-key-engine/binding.gyp    | 25 +++++++
 test/addons/openssl-key-engine/test.js        | 62 ++++++++++++++++
 .../openssl-key-engine/testkeyengine.cc       | 73 +++++++++++++++++++
 .../test-tls-keyengine-invalid-arg-type.js    | 23 ++++++
 .../test-tls-keyengine-unsupported.js         | 34 +++++++++
 9 files changed, 314 insertions(+), 3 deletions(-)
 create mode 100644 test/addons/openssl-key-engine/binding.gyp
 create mode 100644 test/addons/openssl-key-engine/test.js
 create mode 100644 test/addons/openssl-key-engine/testkeyengine.cc
 create mode 100644 test/parallel/test-tls-keyengine-invalid-arg-type.js
 create mode 100644 test/parallel/test-tls-keyengine-unsupported.js

diff --git a/doc/api/tls.md b/doc/api/tls.md
index 68cf5e36bc9f40..297c1e7fd460dc 100644
--- a/doc/api/tls.md
+++ b/doc/api/tls.md
@@ -1358,6 +1358,10 @@ argument.
 <!-- YAML
 added: v0.11.13
 changes:
+  - version: REPLACEME
+    pr-url: https://github.com/nodejs/node/pull/28973
+    description: Added `privateKeyIdentifier` and `privateKeyEngine` options
+                 to get private key from an OpenSSL engine.
   - version: v12.11.0
     pr-url: https://github.com/nodejs/node/pull/29598
     description: Added `sigalgs` option to override supported signature
@@ -1462,6 +1466,12 @@ changes:
     occur in an array. `object.passphrase` is optional. Encrypted keys will be
     decrypted with `object.passphrase` if provided, or `options.passphrase` if
     it is not.
+  * `privateKeyEngine` {string} Name of an OpenSSL engine to get private key
+    from. Should be used together with `privateKeyIdentifier`.
+  * `privateKeyIdentifier` {string} Identifier of a private key managed by
+    an OpenSSL engine. Should be used together with `privateKeyEngine`.
+    Should not be set together with `key`, because both options define a
+    private key in different ways.
   * `maxVersion` {string} Optionally set the maximum TLS version to allow. One
     of `TLSv1.3`, `TLSv1.2'`, `'TLSv1.1'`, or `'TLSv1'`. Cannot be specified
     along with the `secureProtocol` option, use one or the other.
diff --git a/lib/_tls_common.js b/lib/_tls_common.js
index f24cfcbca65ed3..6cd93036c273a0 100644
--- a/lib/_tls_common.js
+++ b/lib/_tls_common.js
@@ -166,6 +166,36 @@ exports.createSecureContext = function createSecureContext(options) {
     c.context.setSigalgs(sigalgs);
   }
 
+  const { privateKeyIdentifier, privateKeyEngine } = options;
+  if (privateKeyIdentifier !== undefined) {
+    if (privateKeyEngine === undefined) {
+      // Engine is required when privateKeyIdentifier is present
+      throw new ERR_INVALID_OPT_VALUE('privateKeyEngine',
+                                      privateKeyEngine);
+    }
+    if (key) {
+      // Both data key and engine key can't be set at the same time
+      throw new ERR_INVALID_OPT_VALUE('privateKeyIdentifier',
+                                      privateKeyIdentifier);
+    }
+
+    if (typeof privateKeyIdentifier === 'string' &&
+        typeof privateKeyEngine === 'string') {
+      if (c.context.setEngineKey)
+        c.context.setEngineKey(privateKeyIdentifier, privateKeyEngine);
+      else
+        throw new ERR_CRYPTO_CUSTOM_ENGINE_NOT_SUPPORTED();
+    } else if (typeof privateKeyIdentifier !== 'string') {
+      throw new ERR_INVALID_ARG_TYPE('options.privateKeyIdentifier',
+                                     ['string', 'undefined'],
+                                     privateKeyIdentifier);
+    } else {
+      throw new ERR_INVALID_ARG_TYPE('options.privateKeyEngine',
+                                     ['string', 'undefined'],
+                                     privateKeyEngine);
+    }
+  }
+
   if (options.ciphers && typeof options.ciphers !== 'string') {
     throw new ERR_INVALID_ARG_TYPE(
       'options.ciphers', 'string', options.ciphers);
diff --git a/src/node_crypto.cc b/src/node_crypto.cc
index d8bc794dcb8527..a666b0225ac625 100644
--- a/src/node_crypto.cc
+++ b/src/node_crypto.cc
@@ -471,6 +471,9 @@ void SecureContext::Initialize(Environment* env, Local<Object> target) {
 
   env->SetProtoMethod(t, "init", Init);
   env->SetProtoMethod(t, "setKey", SetKey);
+#ifndef OPENSSL_NO_ENGINE
+  env->SetProtoMethod(t, "setEngineKey", SetEngineKey);
+#endif  // !OPENSSL_NO_ENGINE
   env->SetProtoMethod(t, "setCert", SetCert);
   env->SetProtoMethod(t, "addCACert", AddCACert);
   env->SetProtoMethod(t, "addCRL", AddCRL);
@@ -764,6 +767,56 @@ void SecureContext::SetSigalgs(const FunctionCallbackInfo<Value>& args) {
   }
 }
 
+#ifndef OPENSSL_NO_ENGINE
+// Helpers for the smart pointer.
+void ENGINE_free_fn(ENGINE* engine) { ENGINE_free(engine); }
+
+void ENGINE_finish_and_free_fn(ENGINE* engine) {
+  ENGINE_finish(engine);
+  ENGINE_free(engine);
+}
+
+void SecureContext::SetEngineKey(const FunctionCallbackInfo<Value>& args) {
+  Environment* env = Environment::GetCurrent(args);
+
+  SecureContext* sc;
+  ASSIGN_OR_RETURN_UNWRAP(&sc, args.Holder());
+
+  CHECK_EQ(args.Length(), 2);
+
+  char errmsg[1024];
+  const node::Utf8Value engine_id(env->isolate(), args[1]);
+  std::unique_ptr<ENGINE, std::function<void(ENGINE*)>> e =
+                         { LoadEngineById(*engine_id, &errmsg),
+                           ENGINE_free_fn };
+  if (e.get() == nullptr) {
+    return env->ThrowError(errmsg);
+  }
+
+  if (!ENGINE_init(e.get())) {
+    return env->ThrowError("ENGINE_init");
+  }
+
+  e.get_deleter() = ENGINE_finish_and_free_fn;
+
+  const node::Utf8Value key_name(env->isolate(), args[0]);
+  EVPKeyPointer key(ENGINE_load_private_key(e.get(), *key_name,
+                                            nullptr, nullptr));
+
+  if (!key) {
+    return ThrowCryptoError(env, ERR_get_error(), "ENGINE_load_private_key");
+  }
+
+  int rv = SSL_CTX_use_PrivateKey(sc->ctx_.get(), key.get());
+
+  if (rv == 0) {
+    return ThrowCryptoError(env, ERR_get_error(), "SSL_CTX_use_PrivateKey");
+  }
+
+  sc->private_key_engine_ = std::move(e);
+}
+#endif  // !OPENSSL_NO_ENGINE
+
 int SSL_CTX_get_issuer(SSL_CTX* ctx, X509* cert, X509** issuer) {
   X509_STORE* store = SSL_CTX_get_cert_store(ctx);
   DeleteFnPtr<X509_STORE_CTX, X509_STORE_CTX_free> store_ctx(
@@ -1438,9 +1491,6 @@ void SecureContext::LoadPKCS12(const FunctionCallbackInfo<Value>& args) {
 
 
 #ifndef OPENSSL_NO_ENGINE
-// Helper for the smart pointer.
-void ENGINE_free_fn(ENGINE* engine) { ENGINE_free(engine); }
-
 void SecureContext::SetClientCertEngine(
     const FunctionCallbackInfo<Value>& args) {
   Environment* env = Environment::GetCurrent(args);
diff --git a/src/node_crypto.h b/src/node_crypto.h
index 91586563f402e1..206a19119a7321 100644
--- a/src/node_crypto.h
+++ b/src/node_crypto.h
@@ -97,6 +97,7 @@ class SecureContext : public BaseObject {
   X509Pointer issuer_;
 #ifndef OPENSSL_NO_ENGINE
   bool client_cert_engine_provided_ = false;
+  std::unique_ptr<ENGINE, std::function<void(ENGINE*)>> private_key_engine_;
 #endif  // !OPENSSL_NO_ENGINE
 
   static const int kMaxSessionSize = 10 * 1024;
@@ -119,6 +120,9 @@ class SecureContext : public BaseObject {
   static void New(const v8::FunctionCallbackInfo<v8::Value>& args);
   static void Init(const v8::FunctionCallbackInfo<v8::Value>& args);
   static void SetKey(const v8::FunctionCallbackInfo<v8::Value>& args);
+#ifndef OPENSSL_NO_ENGINE
+  static void SetEngineKey(const v8::FunctionCallbackInfo<v8::Value>& args);
+#endif  // !OPENSSL_NO_ENGINE
   static void SetCert(const v8::FunctionCallbackInfo<v8::Value>& args);
   static void AddCACert(const v8::FunctionCallbackInfo<v8::Value>& args);
   static void AddCRL(const v8::FunctionCallbackInfo<v8::Value>& args);
diff --git a/test/addons/openssl-key-engine/binding.gyp b/test/addons/openssl-key-engine/binding.gyp
new file mode 100644
index 00000000000000..6f9a8c32c14e29
--- /dev/null
+++ b/test/addons/openssl-key-engine/binding.gyp
@@ -0,0 +1,25 @@
+{
+  'targets': [
+    {
+      'target_name': 'testkeyengine',
+      'type': 'none',
+      'includes': ['../common.gypi'],
+      'conditions': [
+        ['OS=="mac" and '
+         'node_use_openssl=="true" and '
+         'node_shared=="false" and '
+         'node_shared_openssl=="false"', {
+          'type': 'shared_library',
+          'sources': [ 'testkeyengine.cc' ],
+          'product_extension': 'engine',
+          'include_dirs': ['../../../deps/openssl/openssl/include'],
+          'link_settings': {
+            'libraries': [
+              '../../../../out/<(PRODUCT_DIR)/<(openssl_product)'
+            ]
+          },
+        }],
+      ]
+    }
+  ]
+}
diff --git a/test/addons/openssl-key-engine/test.js b/test/addons/openssl-key-engine/test.js
new file mode 100644
index 00000000000000..5c93e6263626bf
--- /dev/null
+++ b/test/addons/openssl-key-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}/testkeyengine.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 serverOptions = {
+  key: agentKey,
+  cert: agentCert,
+  ca: agentCa,
+  requestCert: true,
+  rejectUnauthorized: true
+};
+
+const server = https.createServer(serverOptions, common.mustCall((req, res) => {
+  res.writeHead(200);
+  res.end('hello world');
+})).listen(0, common.localhostIPv4, common.mustCall(() => {
+  const clientOptions = {
+    method: 'GET',
+    host: common.localhostIPv4,
+    port: server.address().port,
+    path: '/test',
+    privateKeyEngine: engine,
+    privateKeyIdentifier: 'dummykey',
+    cert: agentCert,
+    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-key-engine/testkeyengine.cc b/test/addons/openssl-key-engine/testkeyengine.cc
new file mode 100644
index 00000000000000..78a4fc49a63e83
--- /dev/null
+++ b/test/addons/openssl-key-engine/testkeyengine.cc
@@ -0,0 +1,73 @@
+#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      "testkeyengine"
+#define TEST_ENGINE_NAME    "dummy test key engine"
+
+#define PRIVATE_KEY           "test/fixtures/keys/agent1-key.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>());
+}
+
+static EVP_PKEY* EngineLoadPrivkey(ENGINE* engine, const char* name,
+                                   UI_METHOD* ui_method, void* callback_data) {
+  if (strcmp(name, "dummykey") == 0) {
+    std::string key = LoadFile(PRIVATE_KEY);
+    BIO* bio = BIO_new_mem_buf(key.data(), key.size());
+    EVP_PKEY* ret = PEM_read_bio_PrivateKey(bio, nullptr, nullptr, nullptr);
+
+    BIO_vfree(bio);
+    if (ret != nullptr) {
+      return ret;
+    }
+  }
+
+  return nullptr;
+}
+
+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_privkey_function(engine, EngineLoadPrivkey);
+
+  return 1;
+}
+
+extern "C" {
+  IMPLEMENT_DYNAMIC_CHECK_FN();
+  IMPLEMENT_DYNAMIC_BIND_FN(bind_fn);
+}
+
+}  // anonymous namespace
diff --git a/test/parallel/test-tls-keyengine-invalid-arg-type.js b/test/parallel/test-tls-keyengine-invalid-arg-type.js
new file mode 100644
index 00000000000000..385841a6541f94
--- /dev/null
+++ b/test/parallel/test-tls-keyengine-invalid-arg-type.js
@@ -0,0 +1,23 @@
+'use strict';
+const common = require('../common');
+
+if (!common.hasCrypto)
+  common.skip('missing crypto');
+
+const tls = require('tls');
+
+common.expectsError(
+  () => {
+    tls.createSecureContext({ privateKeyEngine: 0,
+                              privateKeyIdentifier: 'key' });
+  },
+  { code: 'ERR_INVALID_ARG_TYPE',
+    message: / Received type number$/ });
+
+common.expectsError(
+  () => {
+    tls.createSecureContext({ privateKeyEngine: 'engine',
+                              privateKeyIdentifier: 0 });
+  },
+  { code: 'ERR_INVALID_ARG_TYPE',
+    message: / Received type number$/ });
diff --git a/test/parallel/test-tls-keyengine-unsupported.js b/test/parallel/test-tls-keyengine-unsupported.js
new file mode 100644
index 00000000000000..149fc4dc316c7c
--- /dev/null
+++ b/test/parallel/test-tls-keyengine-unsupported.js
@@ -0,0 +1,34 @@
+// Flags: --expose-internals
+'use strict';
+const common = require('../common');
+
+if (!common.hasCrypto)
+  common.skip('missing crypto');
+
+// Monkey-patch SecureContext
+const { internalBinding } = require('internal/test/binding');
+const binding = internalBinding('crypto');
+const NativeSecureContext = binding.SecureContext;
+
+binding.SecureContext = function() {
+  const rv = new NativeSecureContext();
+  rv.setEngineKey = undefined;
+  return rv;
+};
+
+const tls = require('tls');
+
+{
+  common.expectsError(
+    () => {
+      tls.createSecureContext({
+        privateKeyEngine: 'engine',
+        privateKeyIdentifier: 'key'
+      });
+    },
+    {
+      code: 'ERR_CRYPTO_CUSTOM_ENGINE_NOT_SUPPORTED',
+      message: 'Custom engines not supported by this OpenSSL'
+    }
+  );
+}