HPCC-33018 Expose chosen OpenSSL cryptography capabilities via a plugin #19343

jackdelv · 2024-12-05T18:50:54Z

Type of change:

This change is a bug fix (non-breaking change which fixes an issue).
This change is a new feature (non-breaking change which adds functionality).
This change improves the code (refactor or other change that does not change the functionality)
This change fixes warnings (the fix does not alter the functionality or the generated code)
This change is a breaking change (fix or feature that will cause existing behavior to change).
This change alters the query API (existing queries will have to be recompiled)

Checklist:

Smoketest:

Send notifications about my Pull Request position in Smoketest queue.
Test my draft Pull Request.

Testing:

github-actions · 2024-12-05T19:34:44Z

Jira Issue: https://hpccsystems.atlassian.net//browse/HPCC-33018

Jirabot Action Result:
Workflow Transition To: Merge Pending
Updated PR

dcamper

I stopped reviewing after I realized that there may be an issue with supporting passphrases for private keys.

Your RSA.Sign() test uses both the private key and a passphrase, and you indicate that it works. That implies that the key requires that passphrase. But, that key is used elsewhere without the passphrase, and it also appears to work. That isn't right; something should be failing.

Take a look at that, please.

ecllibrary/std/OpenSSL.ecl

dcamper · 2024-12-05T19:25:56Z

ecllibrary/std/OpenSSL.ecl

+     *
+     * @see     AvailableAlgorithms()
+     */
+    EXPORT DATA Hash(DATA _indata, VARSTRING _hash_name) := lib_openssl.OpenSSL.digesthash(_indata, _hash_name);


Make the ECL version of the arguments friendly: indata andhash_name (no leading underscores).

Thought: Would it be worthwhile to settle on an argument name of "algorithm_name" (or something, as long as it is consistent) everywhere that argument is used? Thinking of matching the "AvailableAlgorithms()" naming....

I think it is worthwhile to make the argument name consistent everywhere. I have set it to "algorithm_name."

dcamper · 2024-12-05T19:28:55Z

ecllibrary/std/OpenSSL.ecl

+     *                      it must be of the expected size for the given
+     *                      algorithm; OPTIONAL, defaults to creating a
+     *                      random value
+     * @param   salt        TCURRENT_OPENSSL_VERSIONencryption; if not set


It looks like a 'replace all' event went wrong here.

ecllibrary/std/OpenSSL.ecl

dcamper · 2024-12-05T19:35:46Z

ecllibrary/std/OpenSSL.ecl

+     *
+     * @see     Encrypt()
+     */
+    EXPORT DATA Decrypt(DATA ciphertext, STRING pem_private_key) := lib_openssl.OpenSSL.rsaDecrypt(ciphertext, pem_private_key);


This function should permit passing a passphrase for the private key, like RSA.Sign().

Added passphrase parameter.

dcamper · 2024-12-05T19:35:59Z

ecllibrary/std/OpenSSL.ecl

+     * @see     Seal()
+     *          Ciphers.AvailableAlgorithms()
+     */
+    EXPORT DATA Unseal(DATA ciphertext, STRING pem_private_key, VARSTRING symmetric_algorithm = 'aes-256-cbc') := lib_openssl.OpenSSL.rsaUnseal(ciphertext, pem_private_key, symmetric_algorithm);


This function should permit passing a passphrase for the private key, like RSA.Sign().

Added passphrase parameter.

dcamper · 2024-12-05T19:37:38Z

ecllibrary/std/OpenSSL.ecl

+     * @see     Digest.AvailableAlgorithms()
+     *          Sign()
+     */
+    EXPORT BOOLEAN VerifySignature(DATA signature, DATA signedData, DATA passphrase, STRING pem_public_key, VARSTRING hash_name) := lib_openssl.OpenSSL.rsaVerifySignature(signature, signedData, passphrase, pem_public_key, hash_name);


Public keys don't have passphrases.

Removed passphrase parameter from VerifySignature.

dcamper · 2024-12-05T19:41:05Z

ecllibrary/teststd/OpenSSL/TestOpenSSL.ecl

+    EXPORT encrypt_default_iv_default_salt := Std.OpenSSL.Ciphers.Encrypt((DATA)PLAINTEXT, CIPHERS_CIPHER, (DATA)PASSPHRASE);
+    EXPORT encrypt_rsa := Std.OpenSSL.RSA.Encrypt((DATA)PLAINTEXT, RSA_PUBLIC_1);
+    EXPORT seal_rsa := Std.OpenSSL.RSA.Seal((DATA)PLAINTEXT, [RSA_PUBLIC_1, RSA_PUBLIC_2]);
+    EXPORT signed_rsa_sha256 := Std.OpenSSL.RSA.Sign((DATA)PLAINTEXT, (DATA)PASSPHRASE, RSA_PRIVATE_1, 'sha256');


Was RSA_PRIVATE_1 created with a non-empty passphrase? If not then I don't see how this could work.

It was accepting the passphrase parameter, but not doing anything with it. The passphrase is now used and I have added RSA_PRIVATE_2 that was created with a non-empty passphrase to test.

dcamper

Looking pretty good! Just a few changes for this round.

ecllibrary/std/OpenSSL.ecl

dcamper · 2024-12-06T14:04:39Z

ecllibrary/std/OpenSSL.ecl

+     * @see     Digest.AvailableAlgorithms()
+     *          VerifySignature()
+     */
+    EXPORT DATA Sign(DATA plaintext, DATA passphrase, STRING pem_private_key, VARSTRING algorithm_name) := lib_openssl.OpenSSL.rsaSign(plaintext, passphrase, pem_private_key, algorithm_name);


Let's make algorithm_name default to 'sha256'. That is the most common hash algorithm for signing. Remember to adjust the @param doc accordingly.

Changed default to value to sha256.

dcamper · 2024-12-06T14:05:49Z

ecllibrary/std/OpenSSL.ecl

+     * @see     Digest.AvailableAlgorithms()
+     *          Sign()
+     */
+    EXPORT BOOLEAN VerifySignature(DATA signature, DATA signedData, STRING pem_public_key, VARSTRING algorithm_name) := lib_openssl.OpenSSL.rsaVerifySignature(signature, signedData, pem_public_key, algorithm_name);


Same as with Sign(): Set the default value of algorithm_name to 'sha256' and adjust the @param doc.

dcamper · 2024-12-06T14:06:55Z

ecllibrary/teststd/OpenSSL/TestOpenSSL.ecl

+    EXPORT STRING CIPHERS_CIPHER := 'aes-256-cbc';
+    EXPORT STRING PASSPHRASE := 'mypassphrase';
+
+    EXPORT STRING RSA_PUBLIC_1 :=


Question: Why not use the triple-apostrophe multi-line quote scheme for these big PEM keys? It's fine the way it is, just asking.

When using the triple-apostrophe syntax, the regression test would fail inside PEM_read_bio_PrivateKey with "Error: -1: Error within loading a pkey: error:1E08010C:DECODER routines::unsupported", but worked just fine using the other syntax.

dcamper · 2024-12-06T14:08:24Z

ecllibrary/teststd/OpenSSL/TestOpenSSL.ecl

+    EXPORT seal_rsa := Std.OpenSSL.RSA.Seal((DATA)PLAINTEXT, [RSA_PUBLIC_1, RSA_PUBLIC_2]);
+    EXPORT signed_rsa_sha256 := Std.OpenSSL.RSA.Sign((DATA)PLAINTEXT, (DATA)'', RSA_PRIVATE_1, 'sha256');
+    EXPORT signed_rsa_sha256_passphrase := Std.OpenSSL.RSA.Sign((DATA)PLAINTEXT, (DATA)PASSPHRASE, RSA_PRIVATE_2, 'SHA256');
+    // EXPORT signed_rsa_sha256_wrong_passphrase := Std.OpenSSL.RSA.Sign((DATA)PLAINTEXT, (DATA)'notmypassphrase', RSA_PRIVATE_2, 'SHA256'); Fails with Error: -1: Error within loading a pkey: error:1C800064:Provider routines::bad decrypt


I've asked Gavin for recommendations on how to test this. Trapping rtlFail() can be difficult, if not impossible.

It appears that it isn't possible to cleanly capture a failure that throws an exception. My advice is to leave the failure tests in the file, commented-out, along with a comment describing why they are commented-out.

Added an explanation to the test.

dcamper · 2024-12-06T15:47:36Z

ecllibrary/std/OpenSSL.ecl

+    EXPORT DATA Decrypt(DATA ciphertext, VARSTRING algorithm_name, DATA passphrase, DATA iv = (DATA)'', DATA salt = (DATA)'') := lib_openssl.OpenSSL.cipherDecrypt(ciphertext, algorithm_name, passphrase, iv, salt);
+END; // Ciphers
+
+EXPORT RSA := MODULE


I just realized that the MODULEs I defined need the best. Existing structure is:

OpenSSL Digest AvailableAlgorithms() Hash() Ciphers AvailableAlgorithms() IVSize() SaltSize() Encrypt() Decrypt() RSA Seal() Unseal() Encrypt() Decrypt() Sign() VerifySignature()

New structure should be:

OpenSSL Digest AvailableAlgorithms() Hash() Cipher * AvailableAlgorithms() IVSize() SaltSize() Encrypt() Decrypt() PublicKey * RSASeal() * RSAUnseal() * Encrypt() Decrypt() Sign() VerifySignature()

Asterisks indicate changes.

dcamper · 2024-12-06T16:17:54Z

plugins/openssl/openssl.cpp

+            failOpenSSLError("creating buffer for EVP_PKEY");
+
+        EVP_PKEY * pkey;
+        if (startsWith(key, "-----BEGIN RSA PUBLIC KEY-----"))


This public key may not be RSA; it could be ECC. I think you should check for 'contains "PUBLIC KEY-----"' (not sure about the dashes).

Added a function isPublicKey to check the string for "PUBLIC KEY-----".

dcamper · 2024-12-06T16:28:53Z

plugins/openssl/openssl.cpp

+        // Create and initialize the message digest context
+        mdCtx = EVP_MD_CTX_new();
+        if (!mdCtx)
+            failOpenSSLError("EVP_MD_CTX_new (rsaSign)");


The 'rsaSign' is a misnomer, because you can sign with other public key types. This may need alteration elsewhere in the code as well.

Technically, there is a subset of public key types that can be used for signing. We can let OpenSSL generate an error message if necessary; no need to explicitly check. For reference:

Only EVP_PKEY types that support signing can be used with these functions. This includes MAC algorithms where the MAC generation is considered as a form of "signing". Built-in EVP_PKEY types supported by these functions are CMAC, Poly1305, DSA, ECDSA, HMAC, RSA, SipHash, Ed25519 and Ed448.

Removed RSA naming from Sign and VerifySignature and changed the modulename to PublicKey

dcamper · 2024-12-06T16:33:01Z

plugins/openssl/openssl.cpp

+    }
+}
+
+OPENSSL_API void OPENSSL_CALL rsaSign(ICodeContext *ctx, size32_t & __lenResult, void * & __result, size32_t len_plaintext, const void * _plaintext, size32_t len_passphrase, const void * _passphrase, size32_t len_pem_private_key, const char * _pem_private_key, const char * _algorithm_name)


Suggest renaming this function because it is not RSA-specific.

Changed to pkSign as part of the Public Key module.

dcamper · 2024-12-06T16:33:12Z

plugins/openssl/openssl.cpp

+    }
+}
+
+OPENSSL_API bool OPENSSL_CALL rsaVerifySignature(ICodeContext *ctx, size32_t len_signature, const void * _signature, size32_t len_signedData, const void * _signedData, size32_t len_pem_public_key, const char * _pem_public_key, const char * _algorithm_name)


Suggest renaming this function because it is not RSA-specific.

Changed to pkVerifySignature

dcamper

Looks good. Approving, but I would ask that you make the trivial changes I noted.

dcamper · 2024-12-10T20:27:53Z

ecllibrary/std/OpenSSL.ecl

+     *
+     * @return  The encrypted ciphertext.
+     *
+     * @see     Unseal()


We did some renaming, so this Unseal() was changed to RSAUnseal(). Check all other @see functions as well.

Fixed the @see functions in the comments.

dcamper · 2024-12-10T20:32:01Z

plugins/openssl/CMakeLists.txt

+        OpenSSL::SSL
+        OpenSSL::Crypto
+    )
+endif()


Looks like you need a final linefeed in the file.

Added a linefeed.

plugins/openssl/openssl.cpp

ghalliday

Generally looks good. A comment about the name of the plugin, an object leak and a few scattered comments about style.

ghalliday · 2024-12-11T15:14:37Z

plugins/openssl/CMakeLists.txt

+# Cmake Input File for openssl
+#############################################################
+
+project(openssl)


I suspect a different name would be better, otherwise it is confusing whether it means the real openssl library. sslservices?

Changed to sslservices. The toplevel std/OpenSSL.ecl file is still named OpenSSl. Should that be renamed as well e.g. SSL.ecl?

ghalliday · 2024-12-11T15:16:01Z

plugins/openssl/openssl.hpp

+#define _OPENSSL_INCL
+
+#ifdef _WIN32
+#define OPENSSL_CALL _cdecl


Is this needed? Do the regression tests work on windows?

I'm not sure if it is required. I added it to stay consistent with cryptolib and fileservices.

I also don't think it works on windows because it expects 'ecl' to be installed. I haven't tried doing a windows build though.

ghalliday · 2024-12-11T15:21:37Z

plugins/openssl/openssl.cpp

+            }
+        }
+        misses++;
+        const T * newObj = getObjectByName(algorithm_name);


I think these will never be freed. Should this be using a unique_ptr in the tuple?

The algorithms and digests are allocated by OPENSSL_init_crypto, and generally, they are freed when the application exits. They can be explicitly freed by a call to OPENSSL_cleanup, but that is discouraged in the documentation. I think it is currently correct.

ghalliday · 2024-12-11T15:23:24Z

plugins/openssl/openssl.cpp

+            failOpenSSLError("adding new object to cache");
+
+        return newObj;
+    };


minor: ; not needed after a function definition (and in other places)

ghalliday · 2024-12-11T15:30:58Z

plugins/openssl/openssl.cpp

+    void clear()
+    {
+        for (auto& c : cache)
+            EVP_PKEY_free(std::get<1>(c));


You can use unique_ptr to avoid having to explicitly delete the associated items. (Or create a wrapper class.)

Changed to use unique_ptr.

ghalliday · 2024-12-11T15:38:51Z

ecllibrary/teststd/OpenSSL/TestOpenSSL.ecl

+
+    EXPORT STRING RSA_PUBLIC_1 :=
+    '-----BEGIN RSA PUBLIC KEY-----' + '\n' +
+    'MIICCgKCAgEAuLYYTmPMzp13999s7bUAaJZVON+9k/2PDBF5AfHtQ40FmxwRdG3d' + '\n' +


This may trigger warnings from scanning rules about private keys being exported in public repositories.... I'm not quite sure what the best solution idea is.

The TestCrypto_PKE.ecl Includes a private key the same way for testing. I'm not sure if there is a better way either.

ghalliday · 2024-12-11T15:44:39Z

plugins/openssl/openssl.cpp

+            {
+                for (size_t x = 0; x < publicKeys.size(); x++)
+                {
+                    if (encryptedKeys[x])


minor: A general point, for free, delete and rtlFree it is legal to pass a null ptr, and it is defined to do nothing - so no need to check first.

Removed checks around rtlFree.

ghalliday · 2024-12-11T15:46:34Z

plugins/openssl/openssl.cpp

+
+            // Allocate memory for encrypted keys
+            size_t keyCount = publicKeys.size();
+            encryptedKeys = static_cast<byte **>(rtlMalloc(sizeof(byte *) * keyCount));


minor: You could use
enryptedKeys = new byte * [keyCount];
and
delete [] encryptedKeys;

rtlMalloc and rtlFree need to be used for allocated data that is returned from the function. The reason is to avoid conflicts between the debug and release versions of the c++ library.

Changed to use new/delete syntax instead of rtlMalloc/rtlFree.

ghalliday · 2024-12-11T15:48:28Z

plugins/openssl/openssl.cpp

+                failOpenSSLError("EVP_SealFinal");
+            ciphertextLen += len;
+
+            int totalKeyLen = 0;


general: Use size32_t for the sizes of objects that are never going to be ridiculously large, size_t for full compatibility. Same in various other places.

Changed to use size32_t instead of int in most places.

ghalliday · 2024-12-11T15:50:29Z

plugins/openssl/openssl.cpp

+            // Keys; each is (size_t) + <content>
+            for (size_t x = 0; x < keyCount; x++)
+            {
+                size_t keyLen = keyLens[x];


Should this be size32_t rather than size_t? All ecl structures are 32 bit lengths. Another alternative if it is not read from ecl, you could use appendPacked()

I think this should be size32_t.

ghalliday

Thanks jack, a few more comments.

ghalliday · 2024-12-20T12:19:56Z

plugins/sslservices/sslservices.cpp

 {

 void failOpenSSLError(const std::string& context)
 {
-    unsigned long   errCode = 0;
-    char            buffer[120];
+    size_t errCode = 0;


I don't think this is used

Removed unused variable.

ghalliday · 2024-12-20T12:20:45Z

plugins/sslservices/sslservices.cpp

@@ -77,7 +77,7 @@ void failOpenSSLError(const std::string& context)
 int passphraseCB(char *passPhraseBuf, int passPhraseBufSize, int rwflag, void *pPassPhraseMB)
 {
    size32_t len = ((MemoryBuffer*)pPassPhraseMB)->length();
-    if (passPhraseBufSize >= (int)len)
+    if (passPhraseBufSize >= len)


may need to cast passPhraseBufSize to size32_t to avoid complains about mixing signed and unsigned in a comparison

Added cast to size32_t for passPhraseBufSize.

ghalliday · 2024-12-20T12:21:53Z

plugins/sslservices/sslservices.cpp


 private:
-    int hits;
-    int misses;
+    size32_t hits;


this should be unsigned. size32_t is only used for numbers of bytes - not arbitrary counts.

Changed to unsigned.

ghalliday · 2024-12-20T12:22:40Z

plugins/sslservices/sslservices.cpp

-    int hits;
-    int misses;
-    std::list<std::tuple<unsigned, EVP_PKEY *>> cache;
+    size32_t hits;


same comment about unsigned here

Fixed. Also reverted the hash to an unsigned since it is not a number of bytes.

ghalliday · 2024-12-20T12:24:16Z

plugins/sslservices/sslservices.cpp

-    std::list<std::tuple<unsigned, EVP_PKEY *>> cache;
+    size32_t hits;
+    size32_t misses;
+    std::list<std::tuple<size32_t, std::unique_ptr<EVP_PKEY, decltype(&EVP_PKEY_free)>>> cache;


cleanup: you can use a typedef/using to declare a type for a unique Pkey, which simplifies the code when adding to the cache

Added typedef.

ghalliday · 2024-12-20T12:25:39Z

plugins/sslservices/sslservices.cpp

                cache.pop_back();
-            }
        }
        else
            failOpenSSLError("loading a pkey");

        return pkey;


multi-threading: What happens if there are lots of calls to resolve items in the cache from other threads - so this item is evicted (and deleted) as soon as the function returns. May require a shared pointer.

I don't think that is an issue since the caches are initialized with a thread_local storage specifier. Should the caches be shared among multiple threads?

I think this needs to be clearly documented as a comment on the class. The cached results cannot be relied on if it is called from multiple threads.

Added note to caches.

ghalliday · 2024-12-20T12:26:06Z

plugins/sslservices/sslservices.cpp


 //--------------------------------------------------------------------------
-// Advertised Entry Point Functions
+// Advertised Entry Posize32_t Functions


Slightly too enthusiastic replacement of int!

ghalliday · 2024-12-20T12:27:21Z

plugins/sslservices/sslservices.cpp

@@ -407,17 +398,17 @@ OPENSSL_API void OPENSSL_CALL cipherEncrypt(ICodeContext *ctx, size32_t & __lenR

        try
        {
-            int len = 0;
-            int ciphertextLen = 0;
+            size32_t len = 0;


This is a case where using int is probably correct - because that is the type required by the api you are calling. (technically the reinterpret cast is undefined behaviour.)

Changed cases where the type is required by the api to use that type.

ghalliday · 2024-12-20T12:28:49Z

plugins/sslservices/sslservices.cpp

-            for (size_t x = 0; x < publicKeys.size(); x++)
-                rtlFree(encryptedKeys[x]);
-            rtlFree(encryptedKeys);
+            delete [] encryptedKeys;


You still need to iterate and delete the encryptedKeys

Only fixed in the catch{} block - it needs similar code here.

Fixed in try block.

ghalliday · 2024-12-20T12:30:18Z

plugins/sslservices/sslservices.cpp

            plaintext.ensureCapacity(plaintextLen);

-            int len = 0;
-            if (EVP_OpenUpdate(decryptCtx, static_cast<byte *>(plaintext.bufferBase()), &len, newCipherText, newCipherTextLen) != 1)
+            size32_t len = 0;


same comment about int being correct for this call/instance

jackdelv

@ghalliday Fixed those issues and had one question about the storage class of the caches.

jackdelv · 2024-12-20T16:06:18Z

plugins/sslservices/sslservices.cpp

 {

 void failOpenSSLError(const std::string& context)
 {
-    unsigned long   errCode = 0;
-    char            buffer[120];
+    size_t errCode = 0;


Removed unused variable.

jackdelv · 2024-12-20T16:06:46Z

plugins/sslservices/sslservices.cpp

@@ -77,7 +77,7 @@ void failOpenSSLError(const std::string& context)
 int passphraseCB(char *passPhraseBuf, int passPhraseBufSize, int rwflag, void *pPassPhraseMB)
 {
    size32_t len = ((MemoryBuffer*)pPassPhraseMB)->length();
-    if (passPhraseBufSize >= (int)len)
+    if (passPhraseBufSize >= len)


Added cast to size32_t for passPhraseBufSize.

jackdelv · 2024-12-20T16:07:55Z

plugins/sslservices/sslservices.cpp


 private:
-    int hits;
-    int misses;
+    size32_t hits;


Changed to unsigned.

jackdelv · 2024-12-20T16:08:32Z

plugins/sslservices/sslservices.cpp

-    int hits;
-    int misses;
-    std::list<std::tuple<unsigned, EVP_PKEY *>> cache;
+    size32_t hits;


Fixed. Also reverted the hash to an unsigned since it is not a number of bytes.

jackdelv · 2024-12-20T16:08:46Z

plugins/sslservices/sslservices.cpp

-    std::list<std::tuple<unsigned, EVP_PKEY *>> cache;
+    size32_t hits;
+    size32_t misses;
+    std::list<std::tuple<size32_t, std::unique_ptr<EVP_PKEY, decltype(&EVP_PKEY_free)>>> cache;


Added typedef.

jackdelv · 2024-12-20T16:08:57Z

plugins/sslservices/sslservices.cpp


 //--------------------------------------------------------------------------
-// Advertised Entry Point Functions
+// Advertised Entry Posize32_t Functions


jackdelv · 2024-12-20T16:09:32Z

plugins/sslservices/sslservices.cpp

@@ -407,17 +398,17 @@ OPENSSL_API void OPENSSL_CALL cipherEncrypt(ICodeContext *ctx, size32_t & __lenR

        try
        {
-            int len = 0;
-            int ciphertextLen = 0;
+            size32_t len = 0;


Changed cases where the type is required by the api to use that type.

jackdelv · 2024-12-20T16:09:45Z

plugins/sslservices/sslservices.cpp

-            for (size_t x = 0; x < publicKeys.size(); x++)
-                rtlFree(encryptedKeys[x]);
-            rtlFree(encryptedKeys);
+            delete [] encryptedKeys;


jackdelv · 2024-12-20T16:09:55Z

plugins/sslservices/sslservices.cpp

            plaintext.ensureCapacity(plaintextLen);

-            int len = 0;
-            if (EVP_OpenUpdate(decryptCtx, static_cast<byte *>(plaintext.bufferBase()), &len, newCipherText, newCipherTextLen) != 1)
+            size32_t len = 0;


jackdelv · 2024-12-20T16:13:45Z

plugins/sslservices/sslservices.cpp

                cache.pop_back();
-            }
        }
        else
            failOpenSSLError("loading a pkey");

        return pkey;


I don't think that is an issue since the caches are initialized with a thread_local storage specifier. Should the caches be shared among multiple threads?

ghalliday

A few comments about the thread safety of the caches. Something to discuss with Dan whether it is best to have per thread caches, or one shared cache.
If they are per thread, then the initialisation all needs to happen in the constructor, and clean up in the destructor. If one shared, then the returned object's lifetime needs more thought.

ghalliday · 2025-01-02T11:14:04Z

plugins/sslservices/sslservices.cpp

+void AlgorithmCache<EVP_MD>::setCacheName() {cacheName = "DIGEST";}
+
+template <>
+const EVP_CIPHER * AlgorithmCache<EVP_CIPHER>::getObjectByName(const char * name) { return EVP_get_cipherbyname(name); }


Do these objects need cleaning up? I suspect not, but please confirm.

No, the lists of ciphers/digests are populated at initialization and cleaned up automatically by OpenSSL.

ghalliday · 2025-01-02T11:15:37Z

plugins/sslservices/sslservices.cpp

+    void init()
+    {
+        setCacheName();
+        hits = 0;


minor: Safer/cleaner to have default initialisers on the member variables. They should be initialised in the (implicit or explicit) constructor

Added default initializer to cache member variables.

ghalliday · 2025-01-02T11:16:28Z

plugins/sslservices/sslservices.cpp

+
+MODULE_INIT(INIT_PRIORITY_STANDARD)
+{
+    pkeyCache.init();


If these are thread-local variables you cannot rely on calling init.

Removed init functions and moved logic to constructor and added default initializers.

ghalliday · 2025-01-02T11:23:17Z

plugins/sslservices/sslservices.cpp

+        return newObj;
+    }
+
+    void printStatistics() {DBGLOG("SSLSERVICES %s CACHE STATS: HITS = %d, MISSES = %d", cacheName.c_str(), hits, misses);}


This would be better logged as a metric, with text that is machine readable. Something along the lines of ...:

LOG(MCmonitorMetric, "{ \"type\": \"metric\", \"name\": \"sslServiceCache%s\", \"hits\": \"%u\", \"misses\": \"%u\" }", cacheName.c_str(), hits, misses);

There is only one other place that currently does this, but it would be good to switch all existing equivalents to a similar pattern. It would be worth rationalising the format.

Removed printStatistics functions and change to LOG call in the destructor.

ghalliday · 2025-01-02T11:23:51Z

plugins/sslservices/sslservices.cpp

+    const T * getObjectByName(const char * name);
+};
+
+template <>


Interesting, I didn't know you could do this...

ghalliday · 2025-01-02T11:59:31Z

plugins/sslservices/sslservices.cpp

-            for (size_t x = 0; x < publicKeys.size(); x++)
-                rtlFree(encryptedKeys[x]);
-            rtlFree(encryptedKeys);
+            delete [] encryptedKeys;


Only fixed in the catch{} block - it needs similar code here.

ghalliday · 2025-01-02T12:00:37Z

plugins/sslservices/sslservices.cpp

+                size32_t keySize = 0;
+                memcpy(&keySize, inPtr, sizeof(keySize));
+                inPtr += sizeof(keySize);
+                encryptedKeys.emplace_back(reinterpret_cast<const char *>(inPtr), keySize);


same potential problem with large numbers of keys, and cache size.

I don't think this is the same issue because there is only a single key checked out from the cache to compare to the encrypted public keys stored in the _ciphertext.

ghalliday · 2025-01-02T12:03:59Z

plugins/sslservices/sslservices.cpp

+            __lenResult = plaintextLen;
+            MemoryBuffer resultBuffer(__lenResult);
+            resultBuffer.append(__lenResult, plaintext.bufferBase());
+            __result = resultBuffer.detachOwn();


Can you directly return from plaintext.detachOwn() and avoid a clone?

Fixed and removed clone MemoryBuffer.

ghalliday · 2025-01-02T12:09:58Z

plugins/sslservices/sslservices.cpp

                cache.pop_back();
-            }
        }
        else
            failOpenSSLError("loading a pkey");

        return pkey;


I think this needs to be clearly documented as a comment on the class. The cached results cannot be relied on if it is called from multiple threads.

ghalliday · 2025-01-02T12:16:53Z

plugins/sslservices/sslservices.cpp

+{
+    if(PRINT_STATS)
+    {
+        pkeyCache.printStatistics();


This is only going to work for hthor - because the caches are threaded local. Even then there are situations where it will display nothing.

dcamper · 2025-01-02T12:57:13Z

A few comments about the thread safety of the caches. Something to discuss with Dan whether it is best to have per thread caches, or one shared cache. If they are per thread, then the initialisation all needs to happen in the constructor, and clean up in the destructor. If one shared, then the returned object's lifetime needs more thought.

I had originally suggested that Jack implement thread-local caches. This is one of the (few) cases where I think the overhead of thread-safety via locks outweighs the savings of globally caching constructed objects. Considerations included: cached objects are usually quite small; there will typically be very few objects in any particular run (there won't be much switching between algorithms or keys during a job, for instance); and crypto calls will generally be applied en masse to large data in a TRANSFORM (as opposed to one-off calls at the top level of a BWR).

jackdelv · 2025-01-03T17:28:41Z

@ghalliday I had a couple questions that I left for you in the comments. Back to you.

ghalliday

@jackdelv looks good. Please can you squash ready for merging.

ghalliday · 2025-01-08T09:56:50Z

plugins/sslservices/sslservices.cpp

+    }
+}
+
+MODULE_INIT(INIT_PRIORITY_STANDARD)


trivial: could now be deleted.

github-actions · 2025-01-08T09:58:56Z

Jira Issue: https://hpccsystems.atlassian.net//browse/HPCC-33018

Jirabot Action Result:
Workflow Transition To: Merge Pending

Signed-off-by: Jack Del Vecchio <[email protected]>

jackdelv · 2025-01-08T13:23:48Z

@ghalliday Squashed.

github-actions · 2025-01-10T10:29:15Z

Jirabot Action Result:
Added fix version: 9.10.0
Workflow Transition: 'Resolve issue'

jackdelv requested a review from dcamper December 5, 2024 18:51

dcamper requested changes Dec 5, 2024

View reviewed changes

jackdelv requested a review from dcamper December 6, 2024 13:56

dcamper requested changes Dec 6, 2024

View reviewed changes

jackdelv requested a review from dcamper December 10, 2024 19:48

jackdelv marked this pull request as ready for review December 10, 2024 19:58

jackdelv requested a review from ghalliday December 10, 2024 19:59

dcamper approved these changes Dec 10, 2024

View reviewed changes

ghalliday reviewed Dec 11, 2024

View reviewed changes

jackdelv requested a review from ghalliday December 12, 2024 21:11

ghalliday reviewed Dec 20, 2024

View reviewed changes

jackdelv commented Dec 20, 2024

View reviewed changes

jackdelv requested a review from ghalliday December 20, 2024 16:16

ghalliday reviewed Jan 2, 2025

View reviewed changes

jackdelv requested a review from ghalliday January 3, 2025 17:28

ghalliday approved these changes Jan 8, 2025

View reviewed changes

plugins/sslservices/sslservices.cpp Outdated

}

}

MODULE_INIT(INIT_PRIORITY_STANDARD)

Copy link

Member

ghalliday Jan 8, 2025

Choose a reason for hiding this comment

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

trivial: could now be deleted.

ghalliday closed this Jan 8, 2025

ghalliday reopened this Jan 8, 2025

HPCC-33018 Expose chosen OpenSSL cryptography capabilities via a plugin

a45ed25

Signed-off-by: Jack Del Vecchio <[email protected]>

jackdelv force-pushed the openssl branch from 3e8e840 to a45ed25 Compare January 8, 2025 13:23

ghalliday merged commit 908fd65 into hpcc-systems:master Jan 10, 2025
50 of 53 checks passed

HPCC-33018 Expose chosen OpenSSL cryptography capabilities via a plugin #19343

HPCC-33018 Expose chosen OpenSSL cryptography capabilities via a plugin #19343

Conversation

jackdelv commented Dec 5, 2024 • edited Loading

Type of change:

Checklist:

Smoketest:

Testing:

github-actions bot commented Dec 5, 2024

dcamper left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dcamper left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dcamper left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghalliday left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghalliday left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jackdelv commented Dec 5, 2024 •

edited

Loading