From 637050c8e08ab592d70df6348ec123a903f2530a Mon Sep 17 00:00:00 2001 From: Jake Smith Date: Wed, 20 Sep 2023 12:28:27 +0100 Subject: [PATCH] HPCC-30259 Use secrets for dafilesrv client connections Signed-off-by: Jake Smith --- dali/base/dafdesc.cpp | 32 ++++++++ esp/clients/ws_dfsclient/ws_dfsclient.cpp | 15 +++- fs/dafsclient/rmtclient.cpp | 94 +++++++++++++++-------- fs/dafsclient/rmtclient_impl.hpp | 7 +- fs/dafsclient/rmtfile.cpp | 70 ++++++++++++++++- fs/dafsclient/rmtfile.hpp | 5 ++ 6 files changed, 181 insertions(+), 42 deletions(-) diff --git a/dali/base/dafdesc.cpp b/dali/base/dafdesc.cpp index 42f77372327..19e23434d1f 100644 --- a/dali/base/dafdesc.cpp +++ b/dali/base/dafdesc.cpp @@ -1040,6 +1040,7 @@ class CFileDescriptor: public CFileDescriptorBase, implements ISuperFileDescrip SocketEndpointArray *pending; // for constructing cluster group Owned remoteStoragePlane; + std::vector dafileSrvEndpoints; bool setupdone; byte version; @@ -1398,6 +1399,32 @@ class CFileDescriptor: public CFileDescriptorBase, implements ISuperFileDescrip } } + // mapDafileSrvSecrets is a CFileDescriptor is created if it is associated with a remoteStoragePlane. + // Identify the target dafilesrv location urls a secret based connections in the dafilesrv hook + // NB: the expectation is that they'll only be 1 target service dafilesrv URL + // These will remain associated in the hook, until this CFileDescriptor object is destroyed, and removeMappedDafileSrvSecrets is called. + void mapDafileSrvSecrets(IClusterInfo &cluster) + { + Owned groupIter = cluster.queryGroup()->getIterator(); + + ForEach(*groupIter) + { + INode &node = groupIter->query(); + StringBuffer endpointString; + node.endpoint().getEndpointHostText(endpointString); + auto it = std::find(dafileSrvEndpoints.begin(), dafileSrvEndpoints.end(), endpointString.str()); + if (it == dafileSrvEndpoints.end()) + dafileSrvEndpoints.push_back(endpointString.str()); + } + for (auto &dafileSrvEp: dafileSrvEndpoints) + queryDaFileSrvHook()->addSecretUrl(dafileSrvEp.c_str()); + } + void removeMappedDafileSrvSecrets() + { + for (auto &dafileSrvEp: dafileSrvEndpoints) + queryDaFileSrvHook()->removeSecretUrl(dafileSrvEp.c_str()); + } + public: IMPLEMENT_IINTERFACE; @@ -1482,6 +1509,8 @@ class CFileDescriptor: public CFileDescriptorBase, implements ISuperFileDescrip { assertex(1 == clusters.ordinality()); // only one cluster per logical remote file supported/will have resolved to 1 remoteStoragePlane.setown(createStoragePlane(remoteStoragePlaneMeta)); + if (attr->getPropBool("@_remoteSecure")) + mapDafileSrvSecrets(clusters.item(0)); } } else @@ -1613,6 +1642,8 @@ class CFileDescriptor: public CFileDescriptorBase, implements ISuperFileDescrip assertex(1 == clusters.ordinality()); // only one cluster per logical remote file supported/will have resolved to 1 remoteStoragePlane.setown(createStoragePlane(remoteStoragePlaneMeta)); clusters.item(0).applyPlane(remoteStoragePlane); + if (attr->getPropBool("@_remoteSecure")) + mapDafileSrvSecrets(clusters.item(0)); } } @@ -1707,6 +1738,7 @@ class CFileDescriptor: public CFileDescriptorBase, implements ISuperFileDescrip virtual ~CFileDescriptor() { + removeMappedDafileSrvSecrets(); closePending(); // not sure strictly needed ForEachItemInRev(p, parts) delpart(p); diff --git a/esp/clients/ws_dfsclient/ws_dfsclient.cpp b/esp/clients/ws_dfsclient/ws_dfsclient.cpp index d622a790210..3f60c8764af 100644 --- a/esp/clients/ws_dfsclient/ws_dfsclient.cpp +++ b/esp/clients/ws_dfsclient/ws_dfsclient.cpp @@ -321,9 +321,9 @@ class CServiceDistributedFile : public CServiceDistributedFileBasequeryFileMeta()->queryPropTree("File"); const char *remoteName = dfsFile->queryRemoteName(); // NB: null if local - IPropertyTree *dafileSrvRemoteFilePlane = nullptr; if (!isEmptyString(remoteName)) { + IPropertyTree *dafileSrvRemoteFilePlane = nullptr; Owned remoteStorage = getRemoteStorage(remoteName); if (!remoteStorage) throw makeStringExceptionV(0, "Remote storage '%s' not found", remoteName); @@ -394,10 +394,19 @@ class CServiceDistributedFile : public CServiceDistributedFileBasesetProp("@directory", newPath.str()); } } + if (dafileSrvRemoteFilePlane) + { + file->setPropTree("Attr/_remoteStoragePlane", createPTreeFromIPT(dafileSrvRemoteFilePlane)); + if (remoteStorage->hasProp("@secret")) + { + // if remote storage service is secure, dafilesrv connections must be also. + // this flag is used by consumers of this IFleDescriptor to tell whether they need to make + // secure secret based connections to the dafilesrv's + file->setPropBool("Attr/@_remoteSecure", true); + } + } } AccessMode accessMode = static_cast(dfsFile->queryCommonMeta()->getPropInt("@accessMode")); - if (dafileSrvRemoteFilePlane) - file->setPropTree("Attr/_remoteStoragePlane", createPTreeFromIPT(dafileSrvRemoteFilePlane)); fileDesc.setown(deserializeFileDescriptorTree(file)); fileDesc->setTraceName(logicalName); // NB: the accessMode is being defined by the client call, and has been stored in the IDFSFile common meta diff --git a/fs/dafsclient/rmtclient.cpp b/fs/dafsclient/rmtclient.cpp index 7f69a7f0131..49dfc0657d0 100644 --- a/fs/dafsclient/rmtclient.cpp +++ b/fs/dafsclient/rmtclient.cpp @@ -667,7 +667,7 @@ void clientAddSocketToCache(SocketEndpoint &ep,ISocket *socket) //--------------------------------------------------------------------------- -void CRemoteBase::connectSocket(SocketEndpoint &ep, unsigned connectTimeoutMs, unsigned connectRetries) +void CRemoteBase::connectSocket(SocketEndpoint &ep, unsigned connectTimeoutMs, unsigned connectRetries, bool secure) { if (!connectTimeoutMs) connectTimeoutMs = dafsConnectTimeoutMs; @@ -696,10 +696,7 @@ void CRemoteBase::connectSocket(SocketEndpoint &ep, unsigned connectTimeoutMs, u if (TF_TRACE_CLIENT_CONN) { ep.getEndpointHostText(eps); - if (ep.port == securitySettings.queryDaFileSrvSSLPort()) - PROGLOG("Connecting SECURE to %s", eps.str()); - else - PROGLOG("Connecting to %s", eps.str()); + PROGLOG("Connecting %sto %s", secure?"SECURE ":"", eps.str()); //PrintStackReport(); } bool ok = true; @@ -714,13 +711,36 @@ void CRemoteBase::connectSocket(SocketEndpoint &ep, unsigned connectTimeoutMs, u } else socket.setown(ISocket::connect(ep)); - if (ep.port == securitySettings.queryDaFileSrvSSLPort()) + if (secure) { #ifdef _USE_OPENSSL Owned ssock; try { - ssock.setown(createSecureSocket(socket.getClear(), nullptr)); + // instead of creating CRemoteFile/CRemoteBase with a secret, it could instead lookup, and create the ISecureSocketContext and pass that + // into the ctor. That would avoid having to look the secret up, extract the certs and recreate the context on each connection. + // The context would be 'owned' by the hook, and would expire when the mappings are removed (when removeMappedDafileSrvSecrets is called). + if (storageSecret) + { + Owned secretPTree = getSecret("storage", storageSecret); + if (!secretPTree) + throw makeStringExceptionV(-1, "secret %s.%s not found", "storage", storageSecret.str()); + + StringBuffer certSecretBuf; + getSecretKeyValue(certSecretBuf, secretPTree, "tls.crt"); + + StringBuffer privKeySecretBuf; + getSecretKeyValue(privKeySecretBuf, secretPTree, "tls.key"); + + Owned secureContext = createSecureSocketContextEx(certSecretBuf, privKeySecretBuf, nullptr, ClientSocket); + int loglevel = SSLogNormal; +#ifdef _DEBUG + loglevel = SSLogMax; +#endif + ssock.setown(secureContext->createSecureSocket(socket.getClear(), loglevel)); + } + else + ssock.setown(createSecureSocket(socket.getClear(), nullptr)); int status = ssock->secure_connect(); if (status < 0) throw createDafsException(DAFSERR_connection_failed, "Failure to establish secure connection"); @@ -787,10 +807,7 @@ void CRemoteBase::connectSocket(SocketEndpoint &ep, unsigned connectTimeoutMs, u if (!timeExpired) { Sleep(sleeptime); // prevent multiple retries beating - if (ep.port == securitySettings.queryDaFileSrvSSLPort()) - PROGLOG("Retrying SECURE connect"); - else - PROGLOG("Retrying connect"); + PROGLOG("Retrying %sconnect", secure?"SECURE ":""); } } @@ -879,35 +896,41 @@ void CRemoteBase::sendRemoteCommand(MemoryBuffer & src, MemoryBuffer & reply, bo if (!socket) { - bool doConnect = true; - if (connectMethod == SSLFirst || connectMethod == UnsecureFirst) + if (storageSecret) + connectSocket(tep, 0, INFINITE, true); + else { - // MCK - could maintain a list of 100 or so previous endpoints and if connection failed - // then mark port down for a delay (like 15 min above) to avoid having to try every time ... - try - { - connectSocket(tep, 5000, 0); - doConnect = false; - } - catch (IDAFS_Exception *e) + bool doConnect = true; + if (connectMethod == SSLFirst || connectMethod == UnsecureFirst) { - if (e->errorCode() == DAFSERR_connection_failed) + bool secure = tep.port == securitySettings.queryDaFileSrvSSLPort(); + // MCK - could maintain a list of 100 or so previous endpoints and if connection failed + // then mark port down for a delay (like 15 min above) to avoid having to try every time ... + try { - unsigned prevPort = tep.port; - if (prevPort == securitySettings.queryDaFileSrvSSLPort()) - tep.port = securitySettings.queryDaFileSrvPort(); + connectSocket(tep, 5000, 0); + doConnect = false; + } + catch (IDAFS_Exception *e) + { + if (e->errorCode() == DAFSERR_connection_failed) + { + unsigned prevPort = tep.port; + if (secure) + tep.port = securitySettings.queryDaFileSrvPort(); + else + tep.port = securitySettings.queryDaFileSrvSSLPort(); + WARNLOG("Connect failed on port %d, retrying on port %d", prevPort, tep.port); + doConnect = true; + e->Release(); + } else - tep.port = securitySettings.queryDaFileSrvSSLPort(); - WARNLOG("Connect failed on port %d, retrying on port %d", prevPort, tep.port); - doConnect = true; - e->Release(); + throw e; } - else - throw e; } + if (doConnect) + connectSocket(tep, 0, INFINITE, tep.port == securitySettings.queryDaFileSrvSSLPort()); } - if (doConnect) - connectSocket(tep); } } @@ -979,6 +1002,11 @@ CRemoteBase::CRemoteBase(const SocketEndpoint &_ep, DAFSConnectCfg _connectMetho connectMethod = _connectMethod; } +CRemoteBase::CRemoteBase(const SocketEndpoint &_ep, const char *_storageSecret, const char * _filename) + : ep(_ep), storageSecret(_storageSecret), filename(_filename) +{ +} + void CRemoteBase::disconnect() { CriticalBlock block(crit); diff --git a/fs/dafsclient/rmtclient_impl.hpp b/fs/dafsclient/rmtclient_impl.hpp index 9a36dea14af..96481a7f5f3 100644 --- a/fs/dafsclient/rmtclient_impl.hpp +++ b/fs/dafsclient/rmtclient_impl.hpp @@ -153,9 +153,9 @@ class CRemoteBase : public CSimpleInterfaceOf static SocketEndpoint lastfailep; static unsigned lastfailtime; static CriticalSection lastFailEpCrit; - DAFSConnectCfg connectMethod; + DAFSConnectCfg connectMethod = SSLNone; - void connectSocket(SocketEndpoint &ep, unsigned connectTimeoutMs=0, unsigned connectRetries=INFINITE); + void connectSocket(SocketEndpoint &ep, unsigned connectTimeoutMs=0, unsigned connectRetries=INFINITE, bool secure=false); void killSocket(SocketEndpoint &tep); protected: friend class CRemoteFileIO; @@ -163,12 +163,15 @@ protected: friend class CRemoteFileIO; StringAttr filename; CriticalSection crit; SocketEndpoint ep; + StringBuffer storageSecret; void sendRemoteCommand(MemoryBuffer & src, MemoryBuffer & reply, bool retry=true, bool lengthy=false, bool handleErrCode=true); void sendRemoteCommand(MemoryBuffer & src, bool retry); public: CRemoteBase(const SocketEndpoint &_ep, const char * _filename); CRemoteBase(const SocketEndpoint &_ep, DAFSConnectCfg _connectMethod, const char * _filename); + CRemoteBase(const SocketEndpoint &_ep, const char *_storageSecret, const char * _filename); + void disconnect(); const char *queryLocalName() { diff --git a/fs/dafsclient/rmtfile.cpp b/fs/dafsclient/rmtfile.cpp index 5f61fabdc2e..92f3cbe16cc 100644 --- a/fs/dafsclient/rmtfile.cpp +++ b/fs/dafsclient/rmtfile.cpp @@ -222,10 +222,13 @@ CDaliServixFilter *createDaliServixFilter(IPropertyTree &filterProps) return filter; } + class CDaliServixIntercept: public CInterface, implements IDaFileSrvHook { CIArrayOf filters; StringAttr forceRemotePattern; + CriticalSection secretCrit; + std::unordered_map urls; void addFilter(CDaliServixFilter *filter) { @@ -244,6 +247,13 @@ class CDaliServixIntercept: public CInterface, implements IDaFileSrvHook virtual IFile * createIFile(const RemoteFilename & filename) { SocketEndpoint ep = filename.queryEndpoint(); + + // check 1st if this is a secret based url + StringBuffer storageSecret; + getSecretBased(storageSecret, filename); + if (storageSecret.length()) + return createDaliServixFile(filename, storageSecret); + bool noport = (ep.port==0); setDafsEndpointPort(ep); if (!filename.isLocal()||(ep.port!=DAFILESRV_PORT && ep.port!=SECURE_DAFILESRV_PORT)) // assume standard port is running on local machine @@ -340,6 +350,35 @@ class CDaliServixIntercept: public CInterface, implements IDaFileSrvHook { filters.kill(); } + virtual StringBuffer &getSecretBased(StringBuffer &storageSecret, const RemoteFilename & filename) override + { + const SocketEndpoint &ep = filename.queryEndpoint(); + + StringBuffer endpointStr; + ep.getEndpointHostText(endpointStr); + + CriticalBlock b(secretCrit); + auto it = urls.find(endpointStr.str()); + if (it != urls.end()) + { + VStringBuffer secureUrl("https://%s", endpointStr.str()); + generateDynamicUrlSecretName(storageSecret, secureUrl, nullptr); + } + return storageSecret; + } + virtual void addSecretUrl(const char *url) override + { + CriticalBlock b(secretCrit); + urls[url]++; // NB: if doesn't exist std::unordered_map will insert with default values, i.e. with an initial unsigned value of 0 + } + virtual void removeSecretUrl(const char *url) override + { + CriticalBlock b(secretCrit); + auto it = urls.find(url); + assertex(it != urls.end()); + if (--it->second == 0) + urls.erase(it); + } } *DaliServixIntercept = NULL; @@ -663,11 +702,8 @@ class CRemoteFile : public CRemoteBase, implements IFile StringAttr remotefilename; unsigned flags; bool isShareSet; -public: - IMPLEMENT_IINTERFACE_O_USING(CRemoteBase); - CRemoteFile(const SocketEndpoint &_ep, const char * _filename) - : CRemoteBase(_ep, _filename) + void commonInit() { flags = ((unsigned)IFSHread)|((S_IRUSR|S_IWUSR|S_IRGRP|S_IWGRP|S_IROTH|S_IWOTH)<<16); isShareSet = false; @@ -677,6 +713,20 @@ class CRemoteFile : public CRemoteBase, implements IFile filename.set(winDriveFilename); } } +public: + IMPLEMENT_IINTERFACE_O_USING(CRemoteBase); + + CRemoteFile(const SocketEndpoint &_ep, const char * _filename) + : CRemoteBase(_ep, _filename) + { + commonInit(); + } + + CRemoteFile(const SocketEndpoint &ep, const char *filename, const char *storageSecret) + : CRemoteBase(ep, storageSecret, filename) + { + commonInit(); + } bool exists() { @@ -1751,6 +1801,13 @@ IFile *createDaliServixFile(const RemoteFilename & file) return createRemoteFile(ep, path.str()); } +IFile *createDaliServixFile(const RemoteFilename & file, const char *storageSecret) +{ + StringBuffer path; + file.getLocalPath(path); + return new CRemoteFile(file.queryEndpoint(), path.str(), storageSecret); +} + void clientDisconnectRemoteIoOnExit(IFileIO *fileio, bool set) { CRemoteFileIO *cfileio = QUERYINTERFACE(fileio,CRemoteFileIO); @@ -2169,6 +2226,11 @@ class CRemoteFilteredFileIOBase : public CRemoteBase, implements IRemoteFileIO CRemoteFilteredFileIOBase(SocketEndpoint &ep, const char *filename, IOutputMetaData *actual, IOutputMetaData *projected, const RowFilter &fieldFilters, unsigned __int64 chooseN) : CRemoteBase(ep, filename) { + // populate secret if there is one + RemoteFilename rfn; + rfn.setPath(ep, filename); + queryDaFileSrvHook()->getSecretBased(storageSecret, rfn); + // NB: inputGrouped == outputGrouped for now, but may want output to be ungrouped openRequest(); diff --git a/fs/dafsclient/rmtfile.hpp b/fs/dafsclient/rmtfile.hpp index 0515876bed3..e27c04ee7e8 100644 --- a/fs/dafsclient/rmtfile.hpp +++ b/fs/dafsclient/rmtfile.hpp @@ -40,12 +40,17 @@ interface IDaFileSrvHook : extends IRemoteFileCreateHook virtual IPropertyTree *addFilters(IPropertyTree *filters, const SocketEndpoint *ipAddress) = 0; virtual IPropertyTree *addMyFilters(IPropertyTree *filters, SocketEndpoint *myEp=NULL) = 0; virtual void clearFilters() = 0; + virtual StringBuffer &getSecretBased(StringBuffer &storageSecret, const RemoteFilename & filename) = 0; + virtual void addSecretUrl(const char *url) = 0; + virtual void removeSecretUrl(const char *url) = 0; }; extern DAFSCLIENT_API IDaFileSrvHook *queryDaFileSrvHook(); extern DAFSCLIENT_API void setDaliServixSocketCaching(bool set); extern DAFSCLIENT_API bool canAccessDirectly(const RemoteFilename & file); extern DAFSCLIENT_API IFile *createDaliServixFile(const RemoteFilename & file); +extern DAFSCLIENT_API IFile *createDaliServixFile(const RemoteFilename & file, const char *storageSecret); + extern DAFSCLIENT_API void enableForceRemoteReads(); // forces file reads to be remote reads if they match environment setting 'forceRemotePattern' pattern. extern DAFSCLIENT_API bool testForceRemote(const char *path); // return true if forceRemote setup/pattern will make this path a remote read.