Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

HPCC-30259 Use secrets for dafilesrv client connections #17821

Merged
merged 1 commit into from
Sep 28, 2023
Merged

HPCC-30259 Use secrets for dafilesrv client connections #17821

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 32 additions & 0 deletions dali/base/dafdesc.cpp
View file
Open in desktop
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()
Copy link
Member

Choose a reason for hiding this comment

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

As discussed - it may be simpler to avoid this code and always search for a secret, but for a follow up PR.

{
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
Open in desktop
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
Open in desktop
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).
Copy link
Member Author

Choose a reason for hiding this comment

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

NB: @ghalliday @mckellyln - in a subsequent PR I will be refactoring the below so that it uses the new ISecret interface and caching the security contexts

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);
Copy link
Contributor

Choose a reason for hiding this comment

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

Should timeout be INFINITE or 0 (to use configured default) ?

Copy link
Member Author

Choose a reason for hiding this comment

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

timeout is 0.
retries is INFINITE.

    void connectSocket(SocketEndpoint &ep, unsigned connectTimeoutMs=0, unsigned connectRetries=INFINITE, bool secure=false);

Same as defaults, just explicit now.

Copy link
Contributor

Choose a reason for hiding this comment

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

ok, sorry I read the args wrong.

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
Open in desktop
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
Open in desktop
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
Loading