Skip to content

Commit

Permalink
HPCC-30259 Use secrets for dafilesrv client connections
Browse files Browse the repository at this point in the history 
Signed-off-by: Jake Smith <[email protected]>
  • Loading branch information
@jakesmith
jakesmith committed Sep 27, 2023
1 parent 2b038c1 commit 637050c
Show file tree
Hide file tree
Showing 6 changed files with 181 additions and 42 deletions.
32 changes: 32 additions & 0 deletions dali/base/dafdesc.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1040,6 +1040,7 @@ class CFileDescriptor: public CFileDescriptorBase, implements ISuperFileDescrip

SocketEndpointArray *pending; // for constructing cluster group
Owned<IStoragePlane> remoteStoragePlane;
std::vector<std::string> dafileSrvEndpoints;
bool setupdone;
byte version;

Expand Down Expand Up @@ -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<INodeIterator> 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;

Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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));
}
}

Expand Down Expand Up @@ -1707,6 +1738,7 @@ class CFileDescriptor: public CFileDescriptorBase, implements ISuperFileDescrip

virtual ~CFileDescriptor()
{
removeMappedDafileSrvSecrets();
closePending(); // not sure strictly needed
ForEachItemInRev(p, parts)
delpart(p);
Expand Down
15 changes: 12 additions & 3 deletions esp/clients/ws_dfsclient/ws_dfsclient.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -321,9 +321,9 @@ class CServiceDistributedFile : public CServiceDistributedFileBase<IDistributedF
IPropertyTree *file = dfsFile->queryFileMeta()->queryPropTree("File");

const char *remoteName = dfsFile->queryRemoteName(); // NB: null if local
IPropertyTree *dafileSrvRemoteFilePlane = nullptr;
if (!isEmptyString(remoteName))
{
IPropertyTree *dafileSrvRemoteFilePlane = nullptr;
Owned<IPropertyTree> remoteStorage = getRemoteStorage(remoteName);
if (!remoteStorage)
throw makeStringExceptionV(0, "Remote storage '%s' not found", remoteName);
Expand Down Expand Up @@ -394,10 +394,19 @@ class CServiceDistributedFile : public CServiceDistributedFileBase<IDistributedF
file->setProp("@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<AccessMode>(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
Expand Down
94 changes: 61 additions & 33 deletions fs/dafsclient/rmtclient.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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;
Expand All @@ -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<ISecureSocket> 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<IPropertyTree> 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<ISecureSocketContext> 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");
Expand Down Expand Up @@ -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 ":"");
}
}

Expand Down Expand Up @@ -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);
}
}

Expand Down Expand Up @@ -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);
Expand Down
7 changes: 5 additions & 2 deletions fs/dafsclient/rmtclient_impl.hpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -153,22 +153,25 @@ class CRemoteBase : public CSimpleInterfaceOf<IDaFsConnection>
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;

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()
{
Expand Down
70 changes: 66 additions & 4 deletions fs/dafsclient/rmtfile.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -222,10 +222,13 @@ CDaliServixFilter *createDaliServixFilter(IPropertyTree &filterProps)
return filter;
}


class CDaliServixIntercept: public CInterface, implements IDaFileSrvHook
{
CIArrayOf<CDaliServixFilter> filters;
StringAttr forceRemotePattern;
CriticalSection secretCrit;
std::unordered_map<std::string, unsigned> urls;

void addFilter(CDaliServixFilter *filter)
{
Expand All @@ -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
Expand Down Expand Up @@ -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;


Expand Down Expand Up @@ -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;
Expand All @@ -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()
{
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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();
Expand Down
Loading

0 comments on commit 637050c

Please sign in to comment.