src: eliminate ManagedEVPPkey

Prior to this change, the ManagedEVPPkey class added an additional layer of abstraction to the EVP_PKEY class that wasn't strictly necessary. Previously we had: KeyObjectHandle -> std::shared_ptr<KeyObjectData> -> ManagedEVPPkey -> EVPKeyPointer After this change we have: KeyObjectHandle -> KeyObjectData -> EVPKeyPointer The `KeyObjectData` class no longer needs to be wrapped in std::shared_ptr but it will hold the underlying EVPKeyPointer in a std::shared_ptr. This greatly simplifies the abstraction and provides an overall reduction in code and complexity, although the changeset in this PR is fairly extensive to get there. This refactor is being done to simplify the codebase as part of the process of extracting crypto functionality to the separate ncrypto dep. PR-URL: nodejs#54751 Reviewed-By: Yagiz Nizipli <[email protected]>
tpoisseau · Nov 21, 2024 · 7e0d76c · 7e0d76c
1 parent 020fa3e
commit 7e0d76c
Show file tree

Hide file tree

Showing 29 changed files with 603 additions and 712 deletions.
diff --git a/src/crypto/README.md b/src/crypto/README.md
@@ -149,15 +149,10 @@ threadpool).
 Refer to `crypto_keys.h` and `crypto_keys.cc` for all code relating to the
 core key objects.
 
-#### `ManagedEVPPKey`
-
-The `ManagedEVPPKey` class is a smart pointer for OpenSSL `EVP_PKEY`
-structures. These manage the lifecycle of Public and Private key pairs.
-
 #### `KeyObjectData`
 
 `KeyObjectData` is an internal thread-safe structure used to wrap either
-a `ManagedEVPPKey` (for Public or Private keys) or a `ByteSource` containing
+a `EVPKeyPointer` (for Public or Private keys) or a `ByteSource` containing
 a Secret key.
 
 #### `KeyObjectHandle`

diff --git a/src/crypto/crypto_aes.cc b/src/crypto/crypto_aes.cc
@@ -31,15 +31,13 @@ namespace {
 // The key_data must be a secret key.
 // On success, this function sets out to a new ByteSource
 // instance containing the results and returns WebCryptoCipherStatus::OK.
-WebCryptoCipherStatus AES_Cipher(
-    Environment* env,
-    KeyObjectData* key_data,
-    WebCryptoCipherMode cipher_mode,
-    const AESCipherConfig& params,
-    const ByteSource& in,
-    ByteSource* out) {
-  CHECK_NOT_NULL(key_data);
-  CHECK_EQ(key_data->GetKeyType(), kKeyTypeSecret);
+WebCryptoCipherStatus AES_Cipher(Environment* env,
+                                 const KeyObjectData& key_data,
+                                 WebCryptoCipherMode cipher_mode,
+                                 const AESCipherConfig& params,
+                                 const ByteSource& in,
+                                 ByteSource* out) {
+  CHECK_EQ(key_data.GetKeyType(), kKeyTypeSecret);
 
   const int mode = EVP_CIPHER_mode(params.cipher);
 
@@ -69,14 +67,13 @@ WebCryptoCipherStatus AES_Cipher(
     return WebCryptoCipherStatus::FAILED;
   }
 
-  if (!EVP_CIPHER_CTX_set_key_length(
-          ctx.get(),
-          key_data->GetSymmetricKeySize()) ||
+  if (!EVP_CIPHER_CTX_set_key_length(ctx.get(),
+                                     key_data.GetSymmetricKeySize()) ||
       !EVP_CipherInit_ex(
           ctx.get(),
           nullptr,
           nullptr,
-          reinterpret_cast<const unsigned char*>(key_data->GetSymmetricKey()),
+          reinterpret_cast<const unsigned char*>(key_data.GetSymmetricKey()),
           params.iv.data<unsigned char>(),
           encrypt)) {
     return WebCryptoCipherStatus::FAILED;
@@ -217,21 +214,20 @@ std::vector<unsigned char> BlockWithZeroedCounter(
   return new_counter_block;
 }
 
-WebCryptoCipherStatus AES_CTR_Cipher2(
-    KeyObjectData* key_data,
-    WebCryptoCipherMode cipher_mode,
-    const AESCipherConfig& params,
-    const ByteSource& in,
-    unsigned const char* counter,
-    unsigned char* out) {
+WebCryptoCipherStatus AES_CTR_Cipher2(const KeyObjectData& key_data,
+                                      WebCryptoCipherMode cipher_mode,
+                                      const AESCipherConfig& params,
+                                      const ByteSource& in,
+                                      unsigned const char* counter,
+                                      unsigned char* out) {
   CipherCtxPointer ctx(EVP_CIPHER_CTX_new());
   const bool encrypt = cipher_mode == kWebCryptoCipherEncrypt;
 
   if (!EVP_CipherInit_ex(
           ctx.get(),
           params.cipher,
           nullptr,
-          reinterpret_cast<const unsigned char*>(key_data->GetSymmetricKey()),
+          reinterpret_cast<const unsigned char*>(key_data.GetSymmetricKey()),
           counter,
           encrypt)) {
     // Cipher init failed
@@ -259,13 +255,12 @@ WebCryptoCipherStatus AES_CTR_Cipher2(
   return WebCryptoCipherStatus::OK;
 }
 
-WebCryptoCipherStatus AES_CTR_Cipher(
-    Environment* env,
-    KeyObjectData* key_data,
-    WebCryptoCipherMode cipher_mode,
-    const AESCipherConfig& params,
-    const ByteSource& in,
-    ByteSource* out) {
+WebCryptoCipherStatus AES_CTR_Cipher(Environment* env,
+                                     const KeyObjectData& key_data,
+                                     WebCryptoCipherMode cipher_mode,
+                                     const AESCipherConfig& params,
+                                     const ByteSource& in,
+                                     ByteSource* out) {
   auto num_counters = BignumPointer::New();
   if (!BN_lshift(num_counters.get(), BignumPointer::One(), params.length))
     return WebCryptoCipherStatus::FAILED;
@@ -518,16 +513,15 @@ Maybe<bool> AESCipherTraits::AdditionalConfig(
   return Just(true);
 }
 
-WebCryptoCipherStatus AESCipherTraits::DoCipher(
-    Environment* env,
-    std::shared_ptr<KeyObjectData> key_data,
-    WebCryptoCipherMode cipher_mode,
-    const AESCipherConfig& params,
-    const ByteSource& in,
-    ByteSource* out) {
+WebCryptoCipherStatus AESCipherTraits::DoCipher(Environment* env,
+                                                const KeyObjectData& key_data,
+                                                WebCryptoCipherMode cipher_mode,
+                                                const AESCipherConfig& params,
+                                                const ByteSource& in,
+                                                ByteSource* out) {
 #define V(name, fn, _)                                                         \
   case kKeyVariantAES_##name:                                                  \
-    return fn(env, key_data.get(), cipher_mode, params, in, out);
+    return fn(env, key_data, cipher_mode, params, in, out);
   switch (params.variant) {
     VARIANTS(V)
     default:

diff --git a/src/crypto/crypto_aes.h b/src/crypto/crypto_aes.h
@@ -67,13 +67,12 @@ struct AESCipherTraits final {
       WebCryptoCipherMode cipher_mode,
       AESCipherConfig* config);
 
-  static WebCryptoCipherStatus DoCipher(
-      Environment* env,
-      std::shared_ptr<KeyObjectData> key_data,
-      WebCryptoCipherMode cipher_mode,
-      const AESCipherConfig& params,
-      const ByteSource& in,
-      ByteSource* out);
+  static WebCryptoCipherStatus DoCipher(Environment* env,
+                                        const KeyObjectData& key_data,
+                                        WebCryptoCipherMode cipher_mode,
+                                        const AESCipherConfig& params,
+                                        const ByteSource& in,
+                                        ByteSource* out);
 };
 
 using AESCryptoJob = CipherJob<AESCipherTraits>;

diff --git a/src/crypto/crypto_cipher.cc b/src/crypto/crypto_cipher.cc
@@ -989,7 +989,7 @@ template <PublicKeyCipher::Operation operation,
           PublicKeyCipher::EVP_PKEY_cipher_t EVP_PKEY_cipher>
 bool PublicKeyCipher::Cipher(
     Environment* env,
-    const ManagedEVPPKey& pkey,
+    const EVPKeyPointer& pkey,
     int padding,
     const EVP_MD* digest,
     const ArrayBufferOrViewContents<unsigned char>& oaep_label,
@@ -1056,8 +1056,9 @@ void PublicKeyCipher::Cipher(const FunctionCallbackInfo<Value>& args) {
   Environment* env = Environment::GetCurrent(args);
 
   unsigned int offset = 0;
-  ManagedEVPPKey pkey =
-      ManagedEVPPKey::GetPublicOrPrivateKeyFromJs(args, &offset);
+  auto data = KeyObjectData::GetPublicOrPrivateKeyFromJs(args, &offset);
+  if (!data) return;
+  const auto& pkey = data.GetAsymmetricKey();
   if (!pkey)
     return;
 

diff --git a/src/crypto/crypto_cipher.h b/src/crypto/crypto_cipher.h
@@ -110,7 +110,7 @@ class PublicKeyCipher {
             EVP_PKEY_cipher_init_t EVP_PKEY_cipher_init,
             EVP_PKEY_cipher_t EVP_PKEY_cipher>
   static bool Cipher(Environment* env,
-                     const ManagedEVPPKey& pkey,
+                     const EVPKeyPointer& pkey,
                      int padding,
                      const EVP_MD* digest,
                      const ArrayBufferOrViewContents<unsigned char>& oaep_label,
@@ -195,27 +195,23 @@ class CipherJob final : public CryptoJob<CipherTraits> {
     CryptoJob<CipherTraits>::RegisterExternalReferences(New, registry);
   }
 
-  CipherJob(
-      Environment* env,
-      v8::Local<v8::Object> object,
-      CryptoJobMode mode,
-      KeyObjectHandle* key,
-      WebCryptoCipherMode cipher_mode,
-      const ArrayBufferOrViewContents<char>& data,
-      AdditionalParams&& params)
-      : CryptoJob<CipherTraits>(
-            env,
-            object,
-            AsyncWrap::PROVIDER_CIPHERREQUEST,
-            mode,
-            std::move(params)),
-        key_(key->Data()),
+  CipherJob(Environment* env,
+            v8::Local<v8::Object> object,
+            CryptoJobMode mode,
+            KeyObjectHandle* key,
+            WebCryptoCipherMode cipher_mode,
+            const ArrayBufferOrViewContents<char>& data,
+            AdditionalParams&& params)
+      : CryptoJob<CipherTraits>(env,
+                                object,
+                                AsyncWrap::PROVIDER_CIPHERREQUEST,
+                                mode,
+                                std::move(params)),
+        key_(key->Data().addRef()),
         cipher_mode_(cipher_mode),
-        in_(mode == kCryptoJobAsync
-            ? data.ToCopy()
-            : data.ToByteSource()) {}
+        in_(mode == kCryptoJobAsync ? data.ToCopy() : data.ToByteSource()) {}
 
-  std::shared_ptr<KeyObjectData> key() const { return key_; }
+  const KeyObjectData& key() const { return key_; }
 
   WebCryptoCipherMode cipher_mode() const { return cipher_mode_; }
 
@@ -278,7 +274,7 @@ class CipherJob final : public CryptoJob<CipherTraits> {
   }
 
  private:
-  std::shared_ptr<KeyObjectData> key_;
+  KeyObjectData key_;
   WebCryptoCipherMode cipher_mode_;
   ByteSource in_;
   ByteSource out_;

diff --git a/src/crypto/crypto_context.cc b/src/crypto/crypto_context.cc
@@ -613,15 +613,14 @@ void SecureContext::SetKeylogCallback(KeylogCb cb) {
   SSL_CTX_set_keylog_callback(ctx_.get(), cb);
 }
 
-Maybe<void> SecureContext::UseKey(Environment* env,
-                                  std::shared_ptr<KeyObjectData> key) {
-  if (key->GetKeyType() != KeyType::kKeyTypePrivate) {
+Maybe<void> SecureContext::UseKey(Environment* env, const KeyObjectData& key) {
+  if (key.GetKeyType() != KeyType::kKeyTypePrivate) {
     THROW_ERR_CRYPTO_INVALID_KEYTYPE(env);
     return Nothing<void>();
   }
 
   ClearErrorOnReturn clear_error_on_return;
-  if (!SSL_CTX_use_PrivateKey(ctx_.get(), key->GetAsymmetricKey().get())) {
+  if (!SSL_CTX_use_PrivateKey(ctx_.get(), key.GetAsymmetricKey().get())) {
     ThrowCryptoError(env, ERR_get_error(), "SSL_CTX_use_PrivateKey");
     return Nothing<void>();
   }

diff --git a/src/crypto/crypto_context.h b/src/crypto/crypto_context.h
@@ -59,7 +59,7 @@ class SecureContext final : public BaseObject {
 
   v8::Maybe<void> AddCert(Environment* env, BIOPointer&& bio);
   v8::Maybe<void> SetCRL(Environment* env, const BIOPointer& bio);
-  v8::Maybe<void> UseKey(Environment* env, std::shared_ptr<KeyObjectData> key);
+  v8::Maybe<void> UseKey(Environment* env, const KeyObjectData& key);
 
   void SetCACert(const BIOPointer& bio);
   void SetRootCerts();

diff --git a/src/crypto/crypto_dh.cc b/src/crypto/crypto_dh.cc
@@ -432,30 +432,30 @@ Maybe<bool> DHKeyExportTraits::AdditionalConfig(
 }
 
 WebCryptoKeyExportStatus DHKeyExportTraits::DoExport(
-    std::shared_ptr<KeyObjectData> key_data,
+    const KeyObjectData& key_data,
     WebCryptoKeyFormat format,
     const DHKeyExportConfig& params,
     ByteSource* out) {
-  CHECK_NE(key_data->GetKeyType(), kKeyTypeSecret);
+  CHECK_NE(key_data.GetKeyType(), kKeyTypeSecret);
 
   switch (format) {
     case kWebCryptoKeyFormatPKCS8:
-      if (key_data->GetKeyType() != kKeyTypePrivate)
+      if (key_data.GetKeyType() != kKeyTypePrivate)
         return WebCryptoKeyExportStatus::INVALID_KEY_TYPE;
-      return PKEY_PKCS8_Export(key_data.get(), out);
+      return PKEY_PKCS8_Export(key_data, out);
     case kWebCryptoKeyFormatSPKI:
-      if (key_data->GetKeyType() != kKeyTypePublic)
+      if (key_data.GetKeyType() != kKeyTypePublic)
         return WebCryptoKeyExportStatus::INVALID_KEY_TYPE;
-      return PKEY_SPKI_Export(key_data.get(), out);
+      return PKEY_SPKI_Export(key_data, out);
     default:
       UNREACHABLE();
   }
 }
 
 namespace {
-ByteSource StatelessDiffieHellmanThreadsafe(const ManagedEVPPKey& our_key,
-                                            const ManagedEVPPKey& their_key) {
-  auto dp = DHPointer::stateless(our_key.pkey(), their_key.pkey());
+ByteSource StatelessDiffieHellmanThreadsafe(const EVPKeyPointer& our_key,
+                                            const EVPKeyPointer& their_key) {
+  auto dp = DHPointer::stateless(our_key, their_key);
   if (!dp) return {};
 
   return ByteSource::Allocated(dp.release());
@@ -467,13 +467,13 @@ void Stateless(const FunctionCallbackInfo<Value>& args) {
   CHECK(args[0]->IsObject() && args[1]->IsObject());
   KeyObjectHandle* our_key_object;
   ASSIGN_OR_RETURN_UNWRAP(&our_key_object, args[0].As<Object>());
-  CHECK_EQ(our_key_object->Data()->GetKeyType(), kKeyTypePrivate);
+  CHECK_EQ(our_key_object->Data().GetKeyType(), kKeyTypePrivate);
   KeyObjectHandle* their_key_object;
   ASSIGN_OR_RETURN_UNWRAP(&their_key_object, args[1].As<Object>());
-  CHECK_NE(their_key_object->Data()->GetKeyType(), kKeyTypeSecret);
+  CHECK_NE(their_key_object->Data().GetKeyType(), kKeyTypeSecret);
 
-  ManagedEVPPKey our_key = our_key_object->Data()->GetAsymmetricKey();
-  ManagedEVPPKey their_key = their_key_object->Data()->GetAsymmetricKey();
+  const auto& our_key = our_key_object->Data().GetAsymmetricKey();
+  const auto& their_key = their_key_object->Data().GetAsymmetricKey();
 
   Local<Value> out;
   if (!StatelessDiffieHellmanThreadsafe(our_key, their_key)
@@ -503,14 +503,14 @@ Maybe<bool> DHBitsTraits::AdditionalConfig(
   ASSIGN_OR_RETURN_UNWRAP(&public_key, args[offset], Nothing<bool>());
   ASSIGN_OR_RETURN_UNWRAP(&private_key, args[offset + 1], Nothing<bool>());
 
-  if (private_key->Data()->GetKeyType() != kKeyTypePrivate ||
-      public_key->Data()->GetKeyType() != kKeyTypePublic) {
+  if (private_key->Data().GetKeyType() != kKeyTypePrivate ||
+      public_key->Data().GetKeyType() != kKeyTypePublic) {
     THROW_ERR_CRYPTO_INVALID_KEYTYPE(env);
     return Nothing<bool>();
   }
 
-  params->public_key = public_key->Data();
-  params->private_key = private_key->Data();
+  params->public_key = public_key->Data().addRef();
+  params->private_key = private_key->Data().addRef();
 
   return Just(true);
 }
@@ -528,18 +528,15 @@ bool DHBitsTraits::DeriveBits(
     Environment* env,
     const DHBitsConfig& params,
     ByteSource* out) {
-  *out = StatelessDiffieHellmanThreadsafe(
-      params.private_key->GetAsymmetricKey(),
-      params.public_key->GetAsymmetricKey());
+  *out = StatelessDiffieHellmanThreadsafe(params.private_key.GetAsymmetricKey(),
+                                          params.public_key.GetAsymmetricKey());
   return true;
 }
 
-Maybe<bool> GetDhKeyDetail(
-    Environment* env,
-    std::shared_ptr<KeyObjectData> key,
-    Local<Object> target) {
-  ManagedEVPPKey pkey = key->GetAsymmetricKey();
-  CHECK_EQ(EVP_PKEY_id(pkey.get()), EVP_PKEY_DH);
+Maybe<bool> GetDhKeyDetail(Environment* env,
+                           const KeyObjectData& key,
+                           Local<Object> target) {
+  CHECK_EQ(EVP_PKEY_id(key.GetAsymmetricKey().get()), EVP_PKEY_DH);
   return Just(true);
 }