From 52369aec08ab0cdcbee944d55b8778b32527bdb8 Mon Sep 17 00:00:00 2001 From: Gavin Halliday Date: Fri, 27 Oct 2023 10:52:32 +0100 Subject: [PATCH] Changes following own code review Signed-off-by: Gavin Halliday --- roxie/ccd/ccdmain.cpp | 19 +++++++++--- roxie/ccd/ccdprotocol.cpp | 31 ++++++------------- roxie/ccd/hpccprotocol.hpp | 2 +- system/jlib/jptree.hpp | 2 +- system/jlib/jsecrets.cpp | 10 +----- system/jlib/jsecrets.hpp | 1 - system/security/securesocket/securesocket.cpp | 4 +-- 7 files changed, 29 insertions(+), 40 deletions(-) diff --git a/roxie/ccd/ccdmain.cpp b/roxie/ccd/ccdmain.cpp index 40a81cfcc13..fd3df306eaa 100644 --- a/roxie/ccd/ccdmain.cpp +++ b/roxie/ccd/ccdmain.cpp @@ -46,6 +46,10 @@ #include "hpccconfig.hpp" #include "udpsha.hpp" +#ifdef _USE_OPENSSL +#include "securesocket.hpp" +#endif + #if defined (__linux__) #include #include "ioprio.h" @@ -1450,7 +1454,7 @@ int CCD_API roxie_main(int argc, const char *argv[], const char * defaultYaml) else { Owned protocolPlugin = loadHpccProtocolPlugin(protocolCtx, NULL); - Owned roxieServer = protocolPlugin->createListener("runOnce", createRoxieProtocolMsgSink(myNode.getIpAddress(), 0, 1, false), 0, 0, nullptr, nullptr, nullptr, nullptr, nullptr); + Owned roxieServer = protocolPlugin->createListener("runOnce", createRoxieProtocolMsgSink(myNode.getIpAddress(), 0, 1, false), 0, 0, nullptr, nullptr); try { const char *format = topology->queryProp("@format"); @@ -1513,18 +1517,20 @@ int CCD_API roxie_main(int argc, const char *argv[], const char * defaultYaml) StringBuffer certFileName; StringBuffer keyFileName; StringBuffer passPhraseStr; - Owned tlsConfig; + Owned tlsConfig; if (serviceTLS) { protocol = "ssl"; #ifdef _USE_OPENSSL if (isContainerized()) { + //MORE: Why is this containerized only? Should it check for this in both, and fall back to the + //old config if none found and non-containerized? const char *certIssuer = roxieFarm.queryProp("@issuer"); if (isEmptyString(certIssuer)) certIssuer = roxieFarm.getPropBool("@public", true) ? "public" : "local"; - tlsConfig.setown(getIssuerTlsServerConfigWithTrustedPeers(certIssuer, roxieFarm.queryProp("trusted_peers"))); - if (!tlsConfig) + tlsConfig.setown(getIssuerTlsSyncedConfig(certIssuer, roxieFarm.queryProp("trusted_peers"), false)); + if (!tlsConfig || !tlsConfig->isValid()) throw MakeStringException(ROXIE_FILE_ERROR, "TLS secret for issuer %s not found", certIssuer); DBGLOG("Roxie service, port(%d) TLS issuer (%s)", port, certIssuer); } @@ -1555,6 +1561,9 @@ int CCD_API roxie_main(int argc, const char *argv[], const char * defaultYaml) if (!checkFileExists(keyFileName.str())) throw MakeStringException(ROXIE_FILE_ERROR, "Roxie SSL Farm Listener on port %d missing privateKeyFile (%s)", port, keyFileName.str()); + + Owned staticConfig = createSecureSocketConfig(certFileName, keyFileName, passPhraseStr); + tlsConfig.setown(createSyncedPropertyTree(staticConfig)); } #else @@ -1566,7 +1575,7 @@ int CCD_API roxie_main(int argc, const char *argv[], const char * defaultYaml) const char *config = roxieFarm.queryProp("@config"); // NB: leaks - until we fix bug in ensureProtocolPlugin() whereby some paths return a linked object and others do not IHpccProtocolPlugin *protocolPlugin = ensureProtocolPlugin(*protocolCtx, soname); - roxieServer.setown(protocolPlugin->createListener(protocol ? protocol : "native", createRoxieProtocolMsgSink(ip, port, numThreads, suspended), port, listenQueue, config, tlsConfig, certFileName, keyFileName, passPhraseStr)); + roxieServer.setown(protocolPlugin->createListener(protocol ? protocol : "native", createRoxieProtocolMsgSink(ip, port, numThreads, suspended), port, listenQueue, config, tlsConfig)); } else roxieServer.setown(createRoxieWorkUnitListener(numThreads, suspended)); diff --git a/roxie/ccd/ccdprotocol.cpp b/roxie/ccd/ccdprotocol.cpp index 2d46a4eb697..47daa5c550d 100644 --- a/roxie/ccd/ccdprotocol.cpp +++ b/roxie/ccd/ccdprotocol.cpp @@ -29,7 +29,7 @@ //================================================================================================================================ -IHpccProtocolListener *createProtocolListener(const char *protocol, IHpccProtocolMsgSink *sink, unsigned port, unsigned listenQueue, const IPropertyTree *tlsConfig, const char *certFile, const char *keyFile, const char *passPhrase); +IHpccProtocolListener *createProtocolListener(const char *protocol, IHpccProtocolMsgSink *sink, unsigned port, unsigned listenQueue, const ISyncedPropertyTree *tlsConfig); class CHpccProtocolPlugin : implements IHpccProtocolPlugin, public CInterface { @@ -60,9 +60,9 @@ class CHpccProtocolPlugin : implements IHpccProtocolPlugin, public CInterface maxHttpConnectionRequests = ctx.ctxGetPropInt("@maxHttpConnectionRequests", 0); maxHttpKeepAliveWait = ctx.ctxGetPropInt("@maxHttpKeepAliveWait", 5000); // In milliseconds } - IHpccProtocolListener *createListener(const char *protocol, IHpccProtocolMsgSink *sink, unsigned port, unsigned listenQueue, const char *config, const IPropertyTree *tlsConfig, const char *certFile, const char *keyFile, const char *passPhrase) + IHpccProtocolListener *createListener(const char *protocol, IHpccProtocolMsgSink *sink, unsigned port, unsigned listenQueue, const char *config, const ISyncedPropertyTree *tlsConfig) { - return createProtocolListener(protocol, sink, port, listenQueue, tlsConfig, certFile, keyFile, passPhrase); + return createProtocolListener(protocol, sink, port, listenQueue, tlsConfig); } public: StringArray targetNames; @@ -224,35 +224,23 @@ class ProtocolSocketListener : public ProtocolListener Owned socket; SocketEndpoint ep; StringAttr protocol; - StringAttr certFile; - StringAttr keyFile; - StringAttr passPhrase; Owned secureContext; bool isSSL = false; public: - ProtocolSocketListener(IHpccProtocolMsgSink *_sink, unsigned _port, unsigned _listenQueue, const char *_protocol, const IPropertyTree *_tlsConfig, const char *_certFile, const char *_keyFile, const char *_passPhrase) + ProtocolSocketListener(IHpccProtocolMsgSink *_sink, unsigned _port, unsigned _listenQueue, const char *_protocol, const ISyncedPropertyTree *_tlsConfig) : ProtocolListener(_sink) { port = _port; listenQueue = _listenQueue; ep.set(port, queryHostIP()); protocol.set(_protocol); - certFile.set(_certFile); - keyFile.set(_keyFile); - passPhrase.set(_passPhrase); isSSL = streq(protocol.str(), "ssl"); #ifdef _USE_OPENSSL if (isSSL) { - Owned config; - if (!_tlsConfig) - { - config.setown(createSecureSocketConfig(certFile.get(), keyFile.get(), passPhrase.get())); - _tlsConfig = config; - } - secureContext.setown(createSecureSocketContextEx2(_tlsConfig, ServerSocket)); + secureContext.setown(createSecureSocketContextSynced(_tlsConfig, ServerSocket)); } #endif } @@ -2215,16 +2203,17 @@ void ProtocolSocketListener::runOnce(const char *query) p->runOnce(query); } -IHpccProtocolListener *createProtocolListener(const char *protocol, IHpccProtocolMsgSink *sink, unsigned port, unsigned listenQueue, const IPropertyTree *tlsConfig, const char *certFile, const char *keyFile, const char *passPhrase) +IHpccProtocolListener *createProtocolListener(const char *protocol, IHpccProtocolMsgSink *sink, unsigned port, unsigned listenQueue, const ISyncedPropertyTree *tlsConfig) { if (traceLevel) { const char *certIssuer = "none"; - if (tlsConfig && tlsConfig->hasProp("@issuer")) - certIssuer = tlsConfig->queryProp("@issuer"); + Owned tlsInfo = tlsConfig->getTree(); + if (tlsInfo && tlsInfo->hasProp("@issuer")) + certIssuer = tlsInfo->queryProp("@issuer"); DBGLOG("Creating Roxie socket listener, protocol %s, issuer=%s, pool size %d, listen queue %d%s", protocol, certIssuer, sink->getPoolSize(), listenQueue, sink->getIsSuspended() ? " SUSPENDED":""); } - return new ProtocolSocketListener(sink, port, listenQueue, protocol, tlsConfig, certFile, keyFile, passPhrase); + return new ProtocolSocketListener(sink, port, listenQueue, protocol, tlsConfig); } extern IHpccProtocolPlugin *loadHpccProtocolPlugin(IHpccProtocolPluginContext *ctx, IActiveQueryLimiterFactory *_limiterFactory) diff --git a/roxie/ccd/hpccprotocol.hpp b/roxie/ccd/hpccprotocol.hpp index 21380c0a54d..0fbb19c2e0d 100644 --- a/roxie/ccd/hpccprotocol.hpp +++ b/roxie/ccd/hpccprotocol.hpp @@ -134,7 +134,7 @@ interface IActiveQueryLimiterFactory : extends IInterface interface IHpccProtocolPlugin : extends IInterface { - virtual IHpccProtocolListener *createListener(const char *protocol, IHpccProtocolMsgSink *sink, unsigned port, unsigned listenQueue, const char *config, const IPropertyTree *tlsConfig, const char *certFile, const char *keyFile, const char *passPhrase)=0; + virtual IHpccProtocolListener *createListener(const char *protocol, IHpccProtocolMsgSink *sink, unsigned port, unsigned listenQueue, const char *config, const ISyncedPropertyTree *tlsConfig)=0; }; extern IHpccProtocolPlugin *loadHpccProtocolPlugin(IHpccProtocolPluginContext *ctx, IActiveQueryLimiterFactory *limiterFactory); diff --git a/system/jlib/jptree.hpp b/system/jlib/jptree.hpp index f8d643cd805..77c4c809b21 100644 --- a/system/jlib/jptree.hpp +++ b/system/jlib/jptree.hpp @@ -443,7 +443,7 @@ interface ISyncedPropertyTree : extends IInterface //Return a sequence number which changes whenever the secret actually changes - so that a caller can determine //whether it needs to reload the certificates. virtual unsigned getVersion() const = 0; - //An indication that the property tree may be out of date because it couldn'tt be resynchronized. + //An indication that the property tree may be out of date because it couldn't be resynchronized. virtual bool isStale() const = 0; virtual bool isValid() const = 0; // Was an entry found for this secret? }; diff --git a/system/jlib/jsecrets.cpp b/system/jlib/jsecrets.cpp index 012b86bd72e..4a37ceadcf8 100644 --- a/system/jlib/jsecrets.cpp +++ b/system/jlib/jsecrets.cpp @@ -1348,7 +1348,7 @@ class CSyncedCertificateBase : public CInterfaceOf virtual unsigned getVersion() const override final { //If information that is combined with the secret (e.g. trusted peers) can also change dynamically this would - //need to be a separate hash calculated from the condig tree + //need to be a separate hash calculated from the config tree return secretHash; } @@ -1540,14 +1540,6 @@ bool hasIssuerTlsConfig(const char *issuer) return match && match->isValid(); } -const IPropertyTree *getIssuerTlsServerConfigWithTrustedPeers(const char *issuer, const char *trusted_peers) -{ - Owned match = getIssuerTlsSyncedConfig(issuer, trusted_peers, false); - if (!match) - return nullptr; - return match->getTree(); -} - enum UseMTLS { UNINIT, DISABLED, ENABLED }; static UseMTLS useMtls = UNINIT; diff --git a/system/jlib/jsecrets.hpp b/system/jlib/jsecrets.hpp index 888000cef67..364d8d9acdd 100644 --- a/system/jlib/jsecrets.hpp +++ b/system/jlib/jsecrets.hpp @@ -44,7 +44,6 @@ extern jlib_decl const ISyncedPropertyTree * getIssuerTlsSyncedConfig(const char inline const ISyncedPropertyTree * getIssuerTlsSyncedConfig(const char * issuer) { return getIssuerTlsSyncedConfig(issuer, nullptr, false); } extern jlib_decl bool hasIssuerTlsConfig(const char *issuer); -extern jlib_decl const IPropertyTree *getIssuerTlsServerConfigWithTrustedPeers(const char *issuer, const char *trusted_peers); extern jlib_decl ISyncedPropertyTree * createIssuerTlsConfig(const char * issuer, const char * optTrustedPeers, bool isClientConnection, bool acceptSelfSigned, bool addCACert, bool disableMTLS); extern jlib_decl ISyncedPropertyTree * createStorageTlsConfig(const char * secretName, bool addCACert); diff --git a/system/security/securesocket/securesocket.cpp b/system/security/securesocket/securesocket.cpp index e0558ae0c42..e1e063184bf 100644 --- a/system/security/securesocket/securesocket.cpp +++ b/system/security/securesocket/securesocket.cpp @@ -2014,7 +2014,7 @@ SECURESOCKET_API ISecureSocketContext* createSecureSocketContextSecretSrv(const throw makeStringException(-100, "TLS secure communication requested but not configured"); Owned info = getIssuerTlsSyncedConfig(issuer); - if (!!info->isValid()) + if (!info->isValid()) throw makeStringException(-101, "TLS secure communication requested but not configured (2)"); return createSecureSocketContextSynced(info, ServerSocket); @@ -2037,7 +2037,7 @@ IPropertyTree * createSecureSocketConfig(const char* certFileOrBuf, const char* SECURESOCKET_API ISecureSocketContext* createSecureSocketContextSSF(ISmartSocketFactory* ssf) { - if (ssf == nullptr || !ssf->queryTlsConfig()) + if (ssf == nullptr) return createSecureSocketContext(ClientSocket); return createSecureSocketContextSynced(ssf->queryTlsConfig(), ClientSocket);