HPCC-27255 TLS cert/key as buffers #17719

mckellyln · 2023-08-29T02:57:18Z

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 · 2023-08-29T02:57:51Z

https://track.hpccsystems.com/browse/HPCC-27255
Jira updated

mckellyln · 2023-08-29T12:37:15Z

@afishbeck is this what you are sort of looking for ?
It is a draft PR - let me know if this is on the right track.

mckellyln · 2023-08-29T12:37:57Z

@jakesmith this is a draft PR - let me know your thoughts.

afishbeck · 2023-08-31T14:53:21Z

@mckellyln I haven't reviewed the details, but this functionality looks great.

The one thing that would make this just sort of work magically for much of the kubernetes code is if you updated this constructor:

CSecureSocketContext(const IPropertyTree* config, SecureSocketType sockettype)

So that it detects if the following are file paths like they are now, or memory buffers.

If the ptree could support either then it would become almost seemless.

    const char* certfile = config->queryProp("certificate");
    if (isPEMBuffer(certfile)
       -- loadfrommemory
    else
       --current behavior

    const char* privkeyfile = config->queryProp("privatekey");
    if (isPEMBuffer(privkeyfile)
       -- loadfrommemory
    else
       --current behavior

    if(m_verify)
    {
        const char* cabuffer = config->queryProp("verify/ca_certificates/pem");
    if (!isEmptyString(cabuffer)
       --loadfrommemory
   else
   {
        const char* capath = config->queryProp("verify/ca_certificates/@path");
    if (!isEmptyString(capath)
       --current behavior
  }

Can verify the path to the cacert pem buffer later, but that's the idea.

mckellyln · 2023-08-31T18:40:43Z

ok, I've added code for this, thanks.

esp/bindings/SOAP/Platform/soapbind.cpp

system/security/securesocket/securesocket.cpp

afishbeck · 2023-08-31T21:47:28Z

@mckellyln I made a minor suggestion above. Other than that, I haven't reviewed the fine points, but the design looks great. I will be able to make use of this to allow more certificates to come from vault rather than local secrets.

mckellyln · 2023-09-01T12:42:10Z

ok, thanks, I will remove the '@' from the '@pem'

jakesmith

@mckellyln - looks good, a few minor points and questions, main one may be - is const char * the best choice for these certificate buffers that ultimately call through to the methods that take a void * + len ?

esp/clients/ws_dfsclient/ws_dfsclient.cpp

esp/platform/esp.hpp

system/security/securesocket/securesocket.cpp

jakesmith · 2023-09-04T14:20:28Z

system/security/securesocket/securesocket.cpp

+    STACK_OF(X509_INFO) *infostk = PEM_X509_INFO_read_bio(cbio, NULL, NULL, NULL);
+    if (!infostk)
+    {
+        BIO_free(cbio);


minor: it would be safer to 'own' the BIO pointer, would avoid the manual free's and guarantee (e.g. if exception) that it would be freed, e.g. with:

cryptohelper::OwnedEVPBio cbio = BIO_new_mem_buf((void *)certbuf, -1);

worth considering for similar cleanup code, e.g. when SSL_CTX_free called

jakesmith · 2023-09-04T14:21:29Z

system/security/securesocket/securesocket.cpp

+
+    return false;
+}
+
 class CSecureSocketContext : implements ISecureSocketContext, public CInterface


not new: slightly more efficient (avoids VMT) to do : public CInterfaceOf

ok, changed

system/security/securesocket/securesocket.cpp

jakesmith · 2023-09-04T14:22:59Z

system/security/securesocket/securesocket.cpp

+            char errbuf[512];
+            ERR_error_string_n(ERR_get_error(), errbuf, 512);
+            SSL_CTX_free(m_ctx);
+            throw MakeStringException(-1, "error loading certificate chain %s", errbuf);


trivial: MakeStringException is deprecated in favour of makeStringException or makeStringExceptionV

system/security/securesocket/securesocket.hpp

ghalliday

@mckellyln a few comments. I didn't look in detail at the functions.

esp/clients/ws_dfsclient/ws_dfsclient.cpp

ghalliday · 2023-09-06T09:02:46Z

esp/bindings/SOAP/Platform/soapbind.cpp

+
+    if (!isEmptyString(clientCertBuf))
+    {
+        info->setProp("certificatebuf", clientCertBuf);


@afishbeck there is a mixture of attributes (enable, accept) and values (trusted_peers, pem, certificatebuf etc.) is there a reason for each?

@ghalliday There is, this is basically config for a SecureSocket to be used for either client or server. It's been around quite a while, and used multiple places. I create some from secrets which is where this PR comes from. We could clean it up at some point (along with much of the SecureSocket interface), but it would be a separate effort.

ghalliday · 2023-09-06T09:05:19Z

esp/bindings/SOAP/Platform/soapbind.cpp

-    if (client_cert_.length() || ca_certs_.length() || accept_self_signed_)
+
+    if (client_cert_buf_.length() || ca_certs_buf_.length())
+        soapclient.setSecureSocketConfig(createSecClientConfigBuf(client_cert_buf_, client_priv_key_buf_, ca_certs_buf_, accept_self_signed_));


may be simpler as a single function that also takes the buffer parameters - it would remove some duplicate code and would also work with a mixuture of buffers and files (although I can't see that being used)

system/security/securesocket/securesocket.cpp

ghalliday · 2023-09-06T09:11:19Z

system/security/securesocket/securesocket.cpp

+    RSA *rsa = PEM_read_bio_RSAPrivateKey(cbio, NULL, 0, NULL);
+    if (rsa == NULL)
+    {
+        BIO_free(cbio);


Worth using a wrapper class for a BIO * which calls BIO_free on exit. It would clean up the code in this and other functions.
... I see Jake has suggested the same below. I think the following works (it compiles!), and might be a good pattern to adopt elsewhere:

struct BIO_deleter { void operator() ( BIO * bio ) { BIO_free( bio ); } }; using unique_bioptr = std::unique_ptr<BIO, BIO_deleter>; ... unique_bioptr cbio(BIO_new_mem_buf((void *)privkeybuf, -1));

@jakesmith thoughts?

std::unique_ptr vs cryptohelper::OwnedEVPBio would be okay too, and more standard.
Only -ve is that std::unique_ptr doesn't implicitly cast to the underlying pointer the way we are used to with our Owned* varieties [i.e. u need to call uptr.get() ]

Either would make the code cleaner/safer.
It would be nice if we consistently use the same pattern though.

ok, added for BIO.
Perhaps could also do same for stack of x509 ...

yes, I think in almost all cases, it would be cleaner/safer and make the code more robust in general (the pke/cryptocommon code does so for all SSL objects).

mckellyln · 2023-09-12T22:21:51Z

Have made many updates in commit 4 to address your comments.
Please re-review when you have time.

mckellyln · 2023-09-12T22:23:36Z

system/security/securesocket/securesocket.cpp

+    if (!cbio)
+        return false;
+
+    STACK_OF(X509_INFO) *infostk = PEM_X509_INFO_read_bio(cbio.get(), NULL, NULL, NULL);


perhaps a unique_ptr could be used for infostk as well ?

yes, I think it would help the code be more robust in general if we contained these objects, such as they were auto deleted when out of scope.

mckellyln · 2023-09-12T22:25:26Z

build / checks failed because of something not related to code changes -

Error: buildx failed with: ERROR: failed to solve: failed to register layer: write /opt/HPCCSystems/lib/libws_topology.so: no space left on device

jakesmith

@mckellyln - please see comments

jakesmith · 2023-09-13T09:50:36Z

esp/clients/ws_dfsclient/ws_dfsclient.cpp

-    setRpcSSLOptions(rpc, true, clientCertFilename, clientPrivateKeyFilename, caCertFilename, false);
+    Owned<IPropertyTree> secretPTree = getSecret("storage", secretName);
+    if (!secretPTree)
+        throw MakeStringException(-1, "secret %s.%s not found", "storage", secretName);


trivial: this should really be makeStringExceptionV, also in some other places new and old.

jakesmith · 2023-09-13T09:54:00Z

esp/bindings/SOAP/Platform/soapbind.hpp

    virtual void setClientCertificate(const char *certPath, const char *privateKeyPath) override
    {
        client_cert_.set(certPath);
        client_priv_key_.set(privateKeyPath);
    }
+
+    void setCACertificatesBuf(const char *caCertBuf) override {ca_certs_buf_.set(caCertBuf);}


personal: not keen on formatting like this, clearer if spaced and on newlines IMO.

jakesmith · 2023-09-13T09:57:05Z

esp/bindings/SOAP/Platform/soapbind.hpp

+
+    void setCACertificatesBuf(const char *caCertBuf) override {ca_certs_buf_.set(caCertBuf);}
+
+    virtual void setClientCertificateBuf(const char *certBuf, const char *privKeyBuf)


minor: missing 'override'

jakesmith · 2023-09-13T10:50:13Z

system/security/securesocket/securesocket.cpp


        SSL_CTX_set_mode(m_ctx, SSL_CTX_get_mode(m_ctx) | SSL_MODE_AUTO_RETRY);
    }

-    CSecureSocketContext(const char* certfile, const char* privkeyfile, const char* passphrase, SecureSocketType sockettype)
+    CSecureSocketContext(StringBuffer& certBuf, StringBuffer& privkeyBuf, const char* passphrase, SecureSocketType sockettype)


currently, this new constructor isn't used, but also it's confusing that there's 1 ctor taking 2 const char *'s and another taking 2 StringBuffer's.

I think as with the property config ctor, it would be better to consolidate into 1, which differentiated whether a file or content, via isPEMBuffer().

There's quite a bit of common code here too (was to some extent before as well), it would be cleaner/clearer to read if there were member helper functions, e.g. setCertificateFromBuf, setCertificateFromFile, and same for private key.
The calling code in ctor's could then reuse, and with the above change, both can call a common setCertificate and setPrivateKey private member methods, i.e.

void setCertificate(const char *certificate) { if (isPEMBuffer(certificate)) setCertificateFromBuf(certificate); else setCertificateFromFile(certificate); } void setPrivateKey(const char *privateKey) { if (isPEMBuffer(privateKey)) setPrivateKeyFromBuf(privateKey); else setPrivateKeyFromFile(privateKey); }

jakesmith · 2023-09-13T10:53:51Z

system/security/securesocket/securesocket.cpp

-        const char* privkeyfile = config->queryProp("privatekey");
-        if(privkeyfile && *privkeyfile)
+        bool hasPrivKey = false;
+        const char* privkeybuf = config->queryProp("privatekeybuf");


trivial: quite a lot of pre-existing mixed conventions in use already, but we should try to use camelCase for new/changed code.

jakesmith · 2023-09-13T10:56:38Z

system/security/securesocket/securesocket.cpp

            if(!SSL_CTX_check_private_key(m_ctx))
            {
+                SSL_CTX_free(m_ctx);


agree, management of these types of objects would be cleaner/safer if owned (e.g. with unique_ptr).
and if this code is moved into the helper methods that handle private key setting, then it will be more relevant, as it will be owned by the class - the free would not need to happen at this point.

jakesmith · 2023-09-13T11:16:00Z

system/security/securesocket/securesocket.cpp

+    if (!cbio)
+        return false;
+
+    RSA *rsa = PEM_read_bio_RSAPrivateKey(cbio.get(), NULL, 0, NULL);


I think this is being leaked (similar code in pke.cpp, ensures it is owned)

jakesmith · 2023-09-13T11:17:27Z

system/security/securesocket/securesocket.cpp

+    if (!cbio)
+        return false;
+
+    STACK_OF(X509_INFO) *infostk = PEM_X509_INFO_read_bio(cbio.get(), NULL, NULL, NULL);


yes, I think it would help the code be more robust in general if we contained these objects, such as they were auto deleted when out of scope.

system/security/securesocket/securesocket.cpp

jakesmith · 2023-09-13T11:21:08Z

system/security/securesocket/securesocket.cpp

+    return false;
+}
+
+class CSecureSocketContext : public CInterfaceOf<ISecureSocketContext>
 {
 private:
    SSL_CTX*    m_ctx = nullptr;


it would help clearup other code if this was owned.

ghalliday · 2023-09-14T08:55:49Z

system/security/securesocket/securesocket.cpp

@@ -1155,7 +1155,147 @@ const char* strtok__(const char* s, const char* d, StringBuffer& tok)
    return s;
 }

-class CSecureSocketContext : implements ISecureSocketContext, public CInterface
+using BIO_ptr = std::unique_ptr<BIO, decltype(&::BIO_free)>;


The code I suggested:

struct BIO_deleter { void operator() ( BIO * bio ) { BIO_free( bio ); } }; using unique_bioptr = std::unique_ptr<BIO, BIO_deleter>;

avoids the need to pass BIO_free to the constructor of each BIO_ptr.

ok, that's a nice approach.

We should keep these helper objects and functions together.
We already have existing helpers in system/security/cryptohelper/cryptocommon.hpp,
both for owned semantics of these ssl objects, and for openssl exception creation.

I'd be happy for those to be switched to using std::unique_ptr in this PR, or it might be best to use them as is for now, and switch their implementation to std::unique_ptr at a later date.

mckellyln · 2023-09-14T17:28:50Z

Code changes in commits 5 and 6.
Only items left (except for new comments) are:
1). X509_STORE *store = SSL_CTX_get_cert_store(ctx); - do we need to explicitly free this ?
2). no unique_ptr / owned wrapper for STACK_OF(X509_INFO) *infoStk to free up automatically
Please re-review when you have time, thx.

jakesmith

@mckellyln - looks pretty good, some minor comments, 1 question, and replies to your q's

jakesmith · 2023-09-14T17:37:01Z

esp/bindings/SOAP/Platform/soapbind.cpp

@@ -271,24 +271,37 @@ int CHttpSoapBinding::HandleSoapRequest(CHttpRequest* request, CHttpResponse* re
    return 0;
 }

-
-static IPropertyTree *createSecClientConfig(const char *clientCertPath, const char *clientPrivateKey, const char *caCertsPath, bool acceptSelfSigned)
+static IPropertyTree *createSecClientConfig(const char *clientCertFileorBuf, const char *clientPrivKeyFileorBuf, const char *caCertsPathorBuf, bool acceptSelfSigned)


trivial/pedantic/camelCase: ...Fileor... > ...FileOr..

(and similar elsewhere)

ok, changed

jakesmith · 2023-09-15T08:17:09Z

esp/bindings/SOAP/Platform/soapbind.hpp

                throw makeStringExceptionV(-1,"Client private key not found %s.", clientPrivateKey);
+
            rpc.setClientCertificate(clientCert, clientPrivateKey);


question: if this code (and similar at other entry points) spotted it was a file, could it load it into a buffer and then unify much of the code it calls to dealing with PEM buffer's only (not files), and therefore simply the code?

In our current kubernetes installations that would work. But in general I'm not sure whether our file handling supports more formats than just PEM. This buffer handling is for PEM content only.

jakesmith · 2023-09-15T08:20:15Z

system/security/securesocket/securesocket.cpp

-        {
-            throw MakeStringException(-1, "ctx can't be created");
-        }
+            throw makeStringExceptionV(-1, "ctx can't be created");


minor: should be makeStringException

ok, changed

jakesmith · 2023-09-15T08:22:47Z

system/security/securesocket/securesocket.cpp

        {
            char errbuf[512];
            ERR_error_string_n(ERR_get_error(), errbuf, 512);
-            throw MakeStringException(-1, "error loading certificate chain file %s - %s", certfile, errbuf);
+            throw makeStringExceptionV(-1, "error loading certificate chain file %s - %s", certFileorBuf, errbuf);


could use cryptohelper::makeEVPExceptionVA instead of these 3 lines,
and similar in other places below.

ok, changed

jakesmith · 2023-09-15T08:22:49Z

system/security/securesocket/securesocket.cpp

+            {
+                char errbuf[512];
+                ERR_error_string_n(ERR_get_error(), errbuf, 512);
+                throw makeStringExceptionV(-1, "error loading certificate chain %s", errbuf);


could use cryptohelper::makeEVPExceptionVA instead of these 3 lines

ok, changed

@mckellyln - although squashed, I have scanned this and other changes made in the last commit and looks good.

jakesmith · 2023-09-15T08:24:10Z

system/security/securesocket/securesocket.cpp

        }

-        if(!SSL_CTX_check_private_key(m_ctx))
+        if (!SSL_CTX_check_private_key(m_ctx))
+            throw makeStringExceptionV(-1, "Private key does not match the certificate public key");


minor: should be makeStringException

ok, changed

jakesmith · 2023-09-15T08:24:16Z

system/security/securesocket/securesocket.cpp

-            throw MakeStringException(-1, "Private key does not match the certificate public key");
+            // can have multiple certs in buffer
+            if (!loadVerifyLocationsPEMBuffer(m_ctx, caCertsPathorBuf))
+                throw makeStringExceptionV(-1, "Error loading CA certificates");


minor: should be makeStringException

ok, changed

jakesmith · 2023-09-15T08:27:27Z

system/security/securesocket/securesocket.cpp

-    {
-        SSL_CTX_free(m_ctx);
-    }
+    ~CSecureSocketContext() { }


no need, better to remove it.

jakesmith · 2023-09-15T08:30:46Z

system/security/securesocket/securesocket.cpp

-            m_meth = SSLv23_client_method();
-        else
-            m_meth = SSLv23_server_method();
+        // MCK TODO: should we set a default cipherList, as is done in other ctor (below) ?


looks like we should provide a way for an opional cipherList to be provided via this ctor route too,
and default like the other ctor if not.
and/or could it a method of ISecureSocketContext and (optionally) called to set after construction?

ghalliday

@mckellyln that looks good. A few comments that you may want to address, but I think it can be merged soon/now and other issues tackled later.

ghalliday · 2023-09-15T13:05:04Z

esp/bindings/SOAP/Platform/soapbind.cpp

-        info->setProp("certificate", clientCertPath);
-        if (!isEmptyString(clientPrivateKey))
-            info->setProp("privatekey", clientPrivateKey);
+        if (isPEMBuffer(clientCertFileorBuf))


Can this always set "certificate" and then have the PEMbuffer test later? A later clean up PR would be fine.

Yes, but there is also the pem / @path difference, do we leave that in or also remove ?

ghalliday · 2023-09-15T13:09:22Z

system/jlib/jsecrets.cpp

@@ -1066,6 +1066,25 @@ const MemoryAttr &getSecretUdpKey(bool required)
    return udpKey;
 }

+jlib_decl bool isPEMBuffer(const char *certificate)


Might be worth coming up with a more intuitive function name (I didn't know what PEM stood for). Something like containsEmbeddedKey()?

ok, renamed function.

mckellyln · 2023-09-15T14:51:15Z

Squashed commits

afishbeck

@mckellyln some comments.

afishbeck · 2023-09-01T21:57:44Z

esp/bindings/SOAP/Platform/soapbind.cpp

@@ -271,6 +271,29 @@ int CHttpSoapBinding::HandleSoapRequest(CHttpRequest* request, CHttpResponse* re
    return 0;
 }

+static IPropertyTree *createSecClientConfigBuf(const char *clientCertBuf, const char *clientPrivKeyBuf, const char *caCertsBuf, bool acceptSelfSigned)


This is picky and might just be me, but I'd prefer the method name be slightly different.
Like "createInMemorySecClientConfig" or "createBufferedSecClientConfig".. the ending of "ConfigBuf" just implies something specific to me.

afishbeck · 2023-09-01T22:07:59Z

system/security/securesocket/securesocket.cpp

+    if (!cbio)
+        return false;
+
+    RSA *rsa = PEM_read_bio_RSAPrivateKey(cbio, NULL, 0, NULL);


Part of this is probably copy and paste, but new code being added should use nullptr rather than NULL.

afishbeck · 2023-09-01T22:21:24Z

system/security/securesocket/securesocket.cpp


        SSL_CTX_set_mode(m_ctx, SSL_CTX_get_mode(m_ctx) | SSL_MODE_AUTO_RETRY);
    }

-    CSecureSocketContext(const char* certfile, const char* privkeyfile, const char* passphrase, SecureSocketType sockettype)
+    CSecureSocketContext(StringBuffer& certBuf, StringBuffer& privkeyBuf, const char* passphrase, SecureSocketType sockettype)


I think the parameters should still just be char pointers and you should find another way of making the constructor signature unique. Like passing flags. That way if the caller is using some other type of string class they can still function.

If you did keep them as StringBuffer, I think the parameters could be const.

afishbeck · 2023-09-01T22:56:15Z

system/security/securesocket/securesocket.cpp

+                certfile = config->queryProp("certificate");
+            if (certfile && *certfile)
+            {
+                if (isPEMBuffer(certfile))


Looking at this now, I think I can just do what you did when I create these config ptrees. When I have a buffer I can call it certificatebuf, privatekeybuf, pem, etc. You don't need to autodetect the type. It just gets a bit convoluted.

afishbeck · 2023-09-01T22:57:56Z

system/security/securesocket/securesocket.cpp

@@ -1871,6 +2122,11 @@ SECURESOCKET_API ISecureSocketContext* createSecureSocketContextEx2(const IPrope
    return new securesocket::CSecureSocketContext(config, sockettype);
 }       

+SECURESOCKET_API ISecureSocketContext* createSecureSocketContextEx3(StringBuffer& certBuf, StringBuffer& privkeyBuf, const char* passphrase, SecureSocketType sockettype)


I don't think we should force these to be passed as StringBuffer. It takes away the flexibility const char * gives is someone is using a different primitive for the strings, or reads them directly into some location.

afishbeck · 2023-09-15T14:33:58Z

esp/bindings/SOAP/Platform/soapbind.hpp

                throw makeStringExceptionV(-1,"Client private key not found %s.", clientPrivateKey);
+
            rpc.setClientCertificate(clientCert, clientPrivateKey);


In our current kubernetes installations that would work. But in general I'm not sure whether our file handling supports more formats than just PEM. This buffer handling is for PEM content only.

afishbeck · 2023-09-15T14:42:07Z

esp/bindings/SOAP/Platform/soapbind.cpp

-        if (!isEmptyString(clientPrivateKey))
-            info->setProp("privatekey", clientPrivateKey);
+        if (isPEMBuffer(clientCertFileOrBuf))
+            info->setProp("certificatebuf", clientCertFileOrBuf);


Having given it more thought, to support other formats in the future should we call this "certificate_pem" or something similar?

ok, renamed. Yes the check for if its embedded could return a type ...

afishbeck · 2023-09-15T14:56:50Z

system/security/securesocket/securesocket.cpp

+    return true;
+}
+
+static bool loadVerifyLocationsPEMBuffer(SSL_CTX *ctx, const char *caCertBuf, int caCertLen=-1)


I think "verify locations" usually refers to locations on disk, can we change the name to something like "setVerifyCertPEMBuffer" or "setCACertPEMBuffer"?

ok, renamed to setVerifyCertPEMBuffer().

afishbeck · 2023-09-15T15:12:25Z

system/security/securesocket/securesocket.cpp

-                throw MakeStringException(-1, "error loading certificate chain file %s - %s", certfile, errbuf);
-            }
-        }
+        const char *certFileOrBuf = config->queryProp("certificatebuf");


As mentioned above, I think we should rename these "buf" properties to "certificate_pem" or similiar so we know exactly what they are. For any new formats in the future we can use other names.

ok, renamed.

afishbeck · 2023-09-15T15:14:55Z

system/security/securesocket/securesocket.cpp

-        password.set(passphrase);
-        SSL_CTX_set_default_passwd_cb_userdata(m_ctx, (void*)password.str());
-        SSL_CTX_set_default_passwd_cb(m_ctx, pem_passwd_cb);
+    void setVerifyLocations(const char *caCertsPathOrBuf)


Perhaps rename this to "setCACerts" or "setVerifyCerts". I think locations is meant to convey files and/or directories.

ok, renamed to setVerifyCerts

mckellyln · 2023-09-15T16:23:49Z

@afishbeck made changes based on your comments in commit 2a.
Please re-review when you have time.

afishbeck

@mckellyln clarified my 1 remaining question.

afishbeck · 2023-09-15T19:10:44Z

system/security/securesocket/securesocket.cpp

+        return false;
+    }
+
+    X509_STORE *store = SSL_CTX_get_cert_store(ctx);


@mckellyln what I'm wondering is, because we want only our cacerts, and don't want any default cacerts to remain...

Instead of doing:

X509_STORE *store = SSL_CTX_get_cert_store(ctx); loop{ X509_STORE_add_cert(store, infoVal->x509); }

we should do?:

X509_STORE *store = X509_STORE_new(); loop{ X509_STORE_add_cert(store, infoVal->x509); } SSL_CTX_set_cert_store(ctx, store); //this will clear (and free) old and add new

mckellyln · 2023-09-16T00:22:50Z

ok, good point, I agree we should reset/clear store or check if cert already there, etc.
I was thinking of it being same as call to SSL_CTX_load_verify_locations(), but its not quite the same.
I'll look at it next week.
Thanks.

mckellyln · 2023-09-18T14:37:21Z

@afishbeck like this ?

afishbeck · 2023-09-18T15:05:59Z

system/security/securesocket/securesocket.cpp

+        }
+    }
+
+    SSL_CTX_set_cert_store(ctx, store);


What happens when the OwnedX509Store above goes out of scope but the pointer has been passed to SSL_CTX_set_cert_store?

Should you do:
SSL_CTX_set_cert_store(ctx, store.getClear());

instead?

Yes, thank you. It needs to survive.

But then does it get freed when ref count goes to zero ? Or it is already zero at this point ?

When would it ever get freed ? I am thinking we need to free it when we free ctx ?

Maybe a store should be added to CSecureSocketContext ?

I've added a m_store member, if you think this is an ok way to go.

@mckellyln I think this is the difference between SSL_CTX_set_cert_store and SSL_CTX_set1_cert_store. One takes ownership and one increments and decrements the reference count. Either case needs testing to make sure there is no leak or extra free.

I think the only reason to add an m_store is if the store is shared by multiple contexts otherwise it's extra complexity with nothing to gain.

afishbeck

@mckellyln one comment.

mckellyln · 2023-09-18T16:19:26Z

Having some trouble deciding / understanding the lifetime of a store within a context. Please let me know what you think of adding an m_store member like the last commit ??

mckellyln · 2023-09-18T16:53:49Z

ok, removed commit 4a with m_store, so back to original design with the getClear() so it is not freed just after here.

ghalliday

@mckellyln looks good, and code looks clean. Please squash once Tony has approved and I will merge.

afishbeck

@mckellyln looks good.

Signed-off-by: Mark Kelly <[email protected]>

mckellyln · 2023-09-19T15:02:00Z

Squashed and rebased onto candidate-9.4.x

mckellyln force-pushed the tls_buffer branch from 75bae22 to 90f299e Compare August 29, 2023 12:35

mckellyln requested a review from afishbeck August 29, 2023 12:36

mckellyln requested a review from jakesmith August 29, 2023 12:37

mckellyln force-pushed the tls_buffer branch from 262c16d to 9fa8701 Compare August 31, 2023 20:12

mckellyln marked this pull request as ready for review August 31, 2023 20:13

afishbeck reviewed Aug 31, 2023

View reviewed changes

esp/bindings/SOAP/Platform/soapbind.cpp Outdated Show resolved Hide resolved

afishbeck reviewed Aug 31, 2023

View reviewed changes

system/security/securesocket/securesocket.cpp Outdated Show resolved Hide resolved

mckellyln requested a review from afishbeck September 1, 2023 12:58

jakesmith reviewed Sep 4, 2023

View reviewed changes

ghalliday self-requested a review September 5, 2023 14:44

ghalliday reviewed Sep 6, 2023

View reviewed changes

mckellyln force-pushed the tls_buffer branch 2 times, most recently from 9ab52fe to 6ba3969 Compare September 11, 2023 18:11

mckellyln requested review from jakesmith and ghalliday September 12, 2023 22:21

mckellyln commented Sep 12, 2023

View reviewed changes

jakesmith reviewed Sep 13, 2023

View reviewed changes

mckellyln force-pushed the tls_buffer branch from f89fd21 to 7c1049f Compare September 13, 2023 14:51

ghalliday reviewed Sep 14, 2023

View reviewed changes

mckellyln requested a review from jakesmith September 14, 2023 17:29

jakesmith reviewed Sep 15, 2023

View reviewed changes

ghalliday approved these changes Sep 15, 2023

View reviewed changes

mckellyln force-pushed the tls_buffer branch from ea74cd2 to 3565cac Compare September 15, 2023 14:47

jakesmith approved these changes Sep 15, 2023

View reviewed changes

afishbeck reviewed Sep 15, 2023

View reviewed changes

mckellyln requested a review from afishbeck September 15, 2023 16:23

afishbeck reviewed Sep 15, 2023

View reviewed changes

mckellyln requested a review from afishbeck September 18, 2023 14:36

afishbeck reviewed Sep 18, 2023

View reviewed changes

mckellyln force-pushed the tls_buffer branch from a8d5097 to 40fd720 Compare September 18, 2023 15:43

mckellyln requested review from afishbeck and ghalliday September 18, 2023 16:18

mckellyln force-pushed the tls_buffer branch 2 times, most recently from d51854a to 40fd720 Compare September 18, 2023 16:52

ghalliday approved these changes Sep 19, 2023

View reviewed changes

afishbeck approved these changes Sep 19, 2023

View reviewed changes

mckellyln force-pushed the tls_buffer branch from 40fd720 to 3ff56db Compare September 19, 2023 13:57

ghalliday changed the base branch from candidate-9.2.x to candidate-9.4.x September 19, 2023 14:37

mckellyln force-pushed the tls_buffer branch from 3ff56db to 8ceec8c Compare September 19, 2023 14:51

HPCC-27255 TLS cert/key as buffers

6425bed

Signed-off-by: Mark Kelly <[email protected]>

mckellyln force-pushed the tls_buffer branch from 8ceec8c to 6425bed Compare September 19, 2023 14:58

ghalliday merged commit 5f388e5 into hpcc-systems:candidate-9.4.x Sep 19, 2023


		void setCACertificatesBuf(const char *caCertBuf) override {ca_certs_buf_.set(caCertBuf);}

		virtual void setClientCertificateBuf(const char certBuf, const char privKeyBuf)

		throw makeStringExceptionV(-1,"Client private key not found %s.", clientPrivateKey);

		rpc.setClientCertificate(clientCert, clientPrivateKey);

HPCC-27255 TLS cert/key as buffers #17719

HPCC-27255 TLS cert/key as buffers #17719

Conversation

mckellyln commented Aug 29, 2023

Type of change:

Checklist:

Smoketest:

Testing:

github-actions bot commented Aug 29, 2023

mckellyln commented Aug 29, 2023

mckellyln commented Aug 29, 2023

afishbeck commented Aug 31, 2023 • edited Loading

mckellyln commented Aug 31, 2023

afishbeck commented Aug 31, 2023 • edited Loading

mckellyln commented Sep 1, 2023

jakesmith 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

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

jakesmith Sep 13, 2023 • edited Loading

Choose a reason for hiding this comment

mckellyln commented Sep 12, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mckellyln commented Sep 12, 2023

jakesmith 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

mckellyln commented Sep 14, 2023

jakesmith 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

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

mckellyln commented Sep 15, 2023

afishbeck left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

afishbeck commented Aug 31, 2023 •

edited

Loading

afishbeck commented Aug 31, 2023 •

edited

Loading

jakesmith Sep 13, 2023 •

edited

Loading