[GitHub] [nifi-minifi-cpp] szaszm commented on a diff in pull request #1656: MINIFICPP-2191 Replace deprecated OpenSSL API calls

["szaszm (via GitHub)" <gi...@apache.org>]

szaszm commented on code in PR #1656:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1656#discussion_r1324448063


##########
libminifi/src/utils/tls/CertificateUtils.cpp:
##########
@@ -87,6 +92,73 @@ X509_unique_ptr convertWindowsCertificate(const PCCERT_CONTEXT certificate) {
   return X509_unique_ptr{d2i_X509(nullptr, &certificate_binary, certificate_length)};
 }
 
+struct OSSL_PARAM_BLD_deleter {
+  void operator()(OSSL_PARAM_BLD* param_builder) const { OSSL_PARAM_BLD_free(param_builder); }
+};
+using OSSL_PARAM_BLD_unique_ptr = std::unique_ptr<OSSL_PARAM_BLD, OSSL_PARAM_BLD_deleter>;
+
+struct OSSL_PARAM_deleter {
+  void operator()(OSSL_PARAM* params) const { OSSL_PARAM_free(params); }
+};
+using OSSL_PARAM_unique_ptr = std::unique_ptr<OSSL_PARAM, OSSL_PARAM_deleter>;
+
+struct EVP_PKEY_CTX_deleter {
+  void operator()(EVP_PKEY_CTX* pkey_context) const { EVP_PKEY_CTX_free(pkey_context); }
+};
+using EVP_PKEY_CTX_unique_ptr = std::unique_ptr<EVP_PKEY_CTX, EVP_PKEY_CTX_deleter>;
+
+EVP_PKEY_unique_ptr convertWindowsRsaKeyPair(std::span<BYTE> data) {
+  // https://docs.microsoft.com/en-us/windows/win32/api/bcrypt/ns-bcrypt-bcrypt_rsakey_blob
+  auto const blob = reinterpret_cast<BCRYPT_RSAKEY_BLOB *>(data.data());
+
+  if (blob->Magic == BCRYPT_RSAFULLPRIVATE_MAGIC) {
+    OSSL_PARAM_BLD_unique_ptr param_builder{OSSL_PARAM_BLD_new()};
+
+    // n is the modulus common to both public and private key
+    auto const n = BN_bin2bn(data.data() + sizeof(BCRYPT_RSAKEY_BLOB) + blob->cbPublicExp, blob->cbModulus, nullptr);
+    // e is the public exponent
+    auto const e = BN_bin2bn(data.data() + sizeof(BCRYPT_RSAKEY_BLOB), blob->cbPublicExp, nullptr);
+    // d is the private exponent
+    auto const d = BN_bin2bn(data.data() + sizeof(BCRYPT_RSAKEY_BLOB) + blob->cbPublicExp + blob->cbModulus + blob->cbPrime1
+                                 + blob->cbPrime2 + blob->cbPrime1 + blob->cbPrime2 + blob->cbPrime1, blob->cbModulus, nullptr);

Review Comment:
   I'd change the declaration to
   1. make it clear that these are pointers. I'm neutral on whether the pointed-to type is auto-deduced or written out.
   2. make the pointed-to type const, because we don't need to modify it later
   ```suggestion
       // n is the modulus common to both public and private key
       const auto* const n = BN_bin2bn(data.data() + sizeof(BCRYPT_RSAKEY_BLOB) + blob->cbPublicExp, blob->cbModulus, nullptr);
       // e is the public exponent
       const auto* const e = BN_bin2bn(data.data() + sizeof(BCRYPT_RSAKEY_BLOB), blob->cbPublicExp, nullptr);
       // d is the private exponent
       const auto* const d = BN_bin2bn(data.data() + sizeof(BCRYPT_RSAKEY_BLOB) + blob->cbPublicExp + blob->cbModulus + blob->cbPrime1
                                    + blob->cbPrime2 + blob->cbPrime1 + blob->cbPrime2 + blob->cbPrime1, blob->cbModulus, nullptr);
   ```
   
   I would also consider rewriting the complex pointer arithmetic to use a span of the full blob, and divide it into a set of subspans for each component, before converting it to openssl BN. Here's a solution that just needs to be converted from `std::expected` to expected-lite: https://godbolt.org/z/KGjnG8f1a
   
   



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org