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.

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-33260 Allow a BM DFS service+dafilesrv to be secured #19422

Open
wants to merge 2 commits into
base: candidate-9.10.x
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
7 changes: 6 additions & 1 deletion dali/base/dautils.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -3675,6 +3675,11 @@ static CConfigUpdateHook directIOUpdateHook;
static CriticalSection dafileSrvNodeCS;
static Owned<INode> tlsDirectIONode, nonTlsDirectIONode;

unsigned getPreferredDaFsServerPort()
{
return getPreferredDafsClientPort(true);
}

void remapGroupsToDafilesrv(IPropertyTree *file, bool foreign, bool secure)
{
Owned<IPropertyTreeIterator> iter = file->getElements("Cluster");
Expand All @@ -3683,7 +3688,7 @@ void remapGroupsToDafilesrv(IPropertyTree *file, bool foreign, bool secure)
IPropertyTree &cluster = iter->query();
const char *planeName = cluster.queryProp("@name");
Owned<IStoragePlane> plane = getDataStoragePlane(planeName, true);
if ((0 == plane->queryHosts().size()) && isAbsolutePath(plane->queryPrefix())) // if hosts group, or url, don't touch
if (isAbsolutePath(plane->queryPrefix())) // if url (i.e. not absolute prefix path) don't touch
{
if (isContainerized())
{
Expand Down
2 changes: 1 addition & 1 deletion dali/base/dautils.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -583,7 +583,7 @@ inline unsigned calcStripeNumber(unsigned partNum, const char *lfnName, unsigned
}
interface INamedGroupStore;
extern da_decl void remapGroupsToDafilesrv(IPropertyTree *file, bool foreign, bool secure);

extern da_decl unsigned getPreferredDaFsServerPort();
#ifdef NULL_DALIUSER_STACKTRACE
extern da_decl void logNullUser(IUserDescriptor *userDesc);
#else
Expand Down
31 changes: 29 additions & 2 deletions esp/services/ws_dfsservice/ws_dfsservice.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -126,6 +126,26 @@ static void populateLFNMeta(IUserDescriptor *userDesc, const char *logicalName,
{
VStringBuffer storagePlaneXPath("storage/%s", planeXPath.str());
Owned<IPropertyTree> dataPlane = getGlobalConfigSP()->getPropTree(storagePlaneXPath);

const char *hostGroupName = dataPlane->queryProp("@hostGroup");
if (!isEmptyString(hostGroupName))
{
Owned<IPropertyTree> hostGroup = getHostGroup(hostGroupName, false);
if (hostGroup)
{
// This is only likely to be used if this service is in BM
// Cloud based storage planes are unlikely to be backed by hosts/hostGroups
dataPlane.setown(createPTreeFromIPT(dataPlane));
unsigned daFsSrvPort = getPreferredDaFsServerPort();
Owned<IPropertyTreeIterator> iter = hostGroup->getElements("hosts");
ForEach(*iter)
{
VStringBuffer endpoint("%s:%u", iter->query().queryProp(nullptr), daFsSrvPort);
dataPlane->addProp("hosts", endpoint);
}
dataPlane->removeProp("@hostGroup");
}
}
metaRoot->addPropTree("planes", dataPlane.getClear());
}
}
Expand All @@ -135,6 +155,8 @@ static void populateLFNMeta(IUserDescriptor *userDesc, const char *logicalName,
void CWsDfsEx::init(IPropertyTree *cfg, const char *process, const char *service)
{
DBGLOG("Initializing %s service [process = %s]", service, process);
VStringBuffer xpath("Software/EspProcess/EspBinding[@service=\"%s\"]/@protocol", service);
isHttps = strsame("https", cfg->queryProp(xpath));
}

bool CWsDfsEx::onGetLease(IEspContext &context, IEspLeaseRequest &req, IEspLeaseResponse &resp)
Expand Down Expand Up @@ -177,8 +199,13 @@ bool CWsDfsEx::onDFSFileLookup(IEspContext &context, IEspDFSFileLookupRequest &r
if (req.getAccessViaDafilesrv())
opts |= LfnMOptRemap;

// NB: if we ever have some services with tls, and some without in bare-metal, this may need revisiting.
if (getComponentConfigSP()->getPropBool("@tls"))
if (isContainerized())
{
// NB: if we ever have some services with tls, and some without in bare-metal, this may need revisiting.
if (getComponentConfigSP()->getPropBool("@tls"))
opts |= LfnMOptTls;
}
else if (isHttps)
opts |= LfnMOptTls;

Owned<IPropertyTree> responseTree = createPTree();
Expand Down
1 change: 1 addition & 0 deletions esp/services/ws_dfsservice/ws_dfsservice.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@

class CWsDfsEx : public CWsDfs
{
bool isHttps = false;
public:
virtual ~CWsDfsEx() {}
virtual void init(IPropertyTree *cfg, const char *process, const char *service);
Expand Down
9 changes: 9 additions & 0 deletions fs/dafilesrv/dafilesrv.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -590,6 +590,15 @@ int main(int argc, const char* argv[])
if (componentExpert)
synchronizePTree(expert, componentExpert, false, true);

// merge in bare-metal dafilesrv component cert settings into newConfig
IPropertyTree *componentCert = nullptr;
Copy link
Member

Choose a reason for hiding this comment

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

trivial: could combine with the next line (I wouldn't have commented if not only comment).

Copy link
Member Author

Choose a reason for hiding this comment

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

should be combined. Will change.

componentCert = daFileSrv->queryPropTree("cert");
if (componentCert)
{
IPropertyTree *cert = ensurePTree(newConfig, "cert");
synchronizePTree(cert, componentCert, false, true);
}

// any overrides by Instance definitions?
Owned<IPropertyTreeIterator> iter = daFileSrv->getElements("Instance");
ForEach(*iter)
Expand Down
38 changes: 24 additions & 14 deletions fs/dafsserver/dafsserver.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -132,21 +132,31 @@ static ISecureSocket *createSecureSocket(ISocket *sock, bool disableClientCertVe
CriticalBlock b(secureContextCrit);
if (!secureContextServer)
{
#ifdef _CONTAINERIZED
/* Connections are expected from 3rd parties via TLS,
* we do not expect them to provide a valid certificate for verification.
* Currently the server (this dafilesrv), will use either the "public" certificate issuer,
* unless it's visibility is "cluster" (meaning internal only)
*/
if (isContainerized())
{
/* Connections are expected from 3rd parties via TLS,
* we do not expect them to provide a valid certificate for verification.
* Currently the server (this dafilesrv), will use either the "public" certificate issuer,
* unless it's visibility is "cluster" (meaning internal only)
*/

const char *certScope = strsame("cluster", getComponentConfigSP()->queryProp("service/@visibility")) ? "local" : "public";
Owned<const ISyncedPropertyTree> info = getIssuerTlsSyncedConfig(certScope, nullptr, disableClientCertVerification);
if (!info || !info->isValid())
throw makeStringException(-1, "createSecureSocket() : missing MTLS configuration");
secureContextServer.setown(createSecureSocketContextSynced(info, ServerSocket));
#else
secureContextServer.setown(createSecureSocketContextEx2(securitySettings.getSecureConfig(), ServerSocket));
#endif
const char *certScope = strsame("cluster", getComponentConfigSP()->queryProp("service/@visibility")) ? "local" : "public";
Owned<const ISyncedPropertyTree> info = getIssuerTlsSyncedConfig(certScope, nullptr, disableClientCertVerification);
if (!info || !info->isValid())
throw makeStringException(-1, "createSecureSocket() : missing MTLS configuration");
secureContextServer.setown(createSecureSocketContextSynced(info, ServerSocket));
}
else
{
IPropertyTree *cert = getComponentConfigSP()->getPropTree("cert");
if (cert)
{
Owned<ISyncedPropertyTree> certSyncedWrapper = createSyncedPropertyTree(cert);
Copy link
Member

Choose a reason for hiding this comment

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

minor: cert will be leaked - line 151 should be an owned instance or call queryPropTree

Copy link
Member Author

Choose a reason for hiding this comment

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

whoops, should be owned, to avoid theoretical config change and invalidation of it before being used.
Will change.

secureContextServer.setown(createSecureSocketContextSynced(certSyncedWrapper, ServerSocket));
}
else
secureContextServer.setown(createSecureSocketContextEx2(securitySettings.getSecureConfig(), ServerSocket));
}
}
}
int loglevel = SSLogNormal;
Expand Down
Loading