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-30037 Check legacy DZ physical permission in ESP services #17658

Merged
merged 1 commit into from
Oct 4, 2023
Merged

HPCC-30037 Check legacy DZ physical permission in ESP services #17658

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
6 changes: 6 additions & 0 deletions dali/base/dadfs.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1181,6 +1181,7 @@ protected: friend class CDistributedFile;
IDistributedSuperFile *lookupSuperFile(const char *logicalname, IUserDescriptor *user, AccessMode accessMode, IDistributedFileTransaction *transaction, unsigned timeout=INFINITE);

SecAccessFlags getFilePermissions(const char *lname,IUserDescriptor *user,unsigned auditflags);
SecAccessFlags getFScopePermissions(const char *scope,IUserDescriptor *user,unsigned auditflags);
SecAccessFlags getNodePermissions(const IpAddress &ip,IUserDescriptor *user,unsigned auditflags);
SecAccessFlags getFDescPermissions(IFileDescriptor *,IUserDescriptor *user,unsigned auditflags=0);
SecAccessFlags getDLFNPermissions(CDfsLogicalFileName &dlfn,IUserDescriptor *user,unsigned auditflags=0);
Expand Down Expand Up @@ -11875,6 +11876,11 @@ SecAccessFlags CDistributedFileDirectory::getDropZoneScopePermissions(const char
return getScopePermissions(dlfn.get(),user,auditflags);
}

SecAccessFlags CDistributedFileDirectory::getFScopePermissions(const char *scope,IUserDescriptor *user,unsigned auditflags)
{
return getScopePermissions(scope,user,auditflags);
}

void CDistributedFileDirectory::setDefaultUser(IUserDescriptor *user)
{
if (user)
Expand Down
1 change: 1 addition & 0 deletions dali/base/dadfs.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -642,6 +642,7 @@ interface IDistributedFileDirectory: extends IInterface
virtual void removeSuperFile(const char *_logicalname, bool delSubs=false, IUserDescriptor *user=NULL, IDistributedFileTransaction *transaction=NULL)=0;

virtual SecAccessFlags getFilePermissions(const char *lname,IUserDescriptor *user,unsigned auditflags=0)=0; // see dasess for auditflags values
virtual SecAccessFlags getFScopePermissions(const char *scope,IUserDescriptor *user,unsigned auditflags=0)=0; // see dasess for auditflags values
virtual void setDefaultUser(IUserDescriptor *user)=0;
virtual IUserDescriptor* queryDefaultUser()=0;
virtual SecAccessFlags getNodePermissions(const IpAddress &ip,IUserDescriptor *user,unsigned auditflags=0)=0;
Expand Down
11 changes: 6 additions & 5 deletions dali/dfu/dfurun.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -668,7 +668,6 @@ class CDFUengine: public CInterface, implements IDFUengine
auditflags |= DALI_LDAP_WRITE_WANTED;

SecAccessFlags perm;
bool checkLegacyPhysicalPerms = getGlobalConfigSP()->getPropBool("expert/@failOverToLegacyPhysicalPerms",!isContainerized());
IClusterInfo *iClusterInfo = fd->queryClusterNum(0);
const char *planeName = iClusterInfo->queryGroupName();
if (!isEmptyString(planeName))
Expand All @@ -685,17 +684,19 @@ class CDFUengine: public CInterface, implements IDFUengine
throw makeStringExceptionV(-1,"Invalid DropZone directory %s.",dir);

perm = queryDistributedFileDirectory().getDropZoneScopePermissions(planeName,relativePath,user,auditflags);
if (((!write&&!HASREADPERMISSION(perm))||(write&&!HASWRITEPERMISSION(perm)))&&checkLegacyPhysicalPerms)
perm = queryDistributedFileDirectory().getFDescPermissions(fd,user,auditflags);
if (((!write&&!HASREADPERMISSION(perm))||(write&&!HASWRITEPERMISSION(perm))))
{
if (getGlobalConfigSP()->getPropBool("expert/@failOverToLegacyPhysicalPerms",!isContainerized()))
perm = queryDistributedFileDirectory().getFDescPermissions(fd,user,auditflags);
}
}
else
{
#ifndef _CONTAINERIZED
Owned<IEnvironmentFactory> factory = getEnvironmentFactory(true);
Owned<IConstEnvironment> env = factory->openEnvironment();
if (env->isDropZoneRestrictionEnabled()||!checkLegacyPhysicalPerms)
if (env->isDropZoneRestrictionEnabled())
throw makeStringException(-1,"Empty plane name.");
perm = queryDistributedFileDirectory().getFDescPermissions(fd,user,auditflags);
#else
throw makeStringException(-1,"Unexpected empty plane name."); // should never be the case in containerized setups
#endif
Expand Down
17 changes: 3 additions & 14 deletions esp/services/ws_fs/ws_fsBinding.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -410,16 +410,11 @@ int CFileSpraySoapBindingEx::downloadFile(IEspContext &context, CHttpRequest* re
if (!osStr.isEmpty() && (atoi(osStr.str())== OS_WINDOWS))
pathSep = '\\';
pathStr.replace(pathSep=='\\'?'/':'\\', pathSep);
addPathSepChar(pathStr);

if (!validateDropZoneHostAndPath(dropZoneName, netAddressStr, pathStr)) //The pathStr should be the absolute path for the dropzone.
throw makeStringException(ECLWATCH_INVALID_INPUT, "Invalid DropZoneName, NetAddress or Path.");
SecAccessFlags permission = getDZPathScopePermissions(context, dropZoneName, pathStr, netAddressStr);
if (permission < SecAccess_Read)
throw makeStringExceptionV(ECLWATCH_INVALID_INPUT, "Access DropZone Scope %s %s %s not allowed for user %s (permission:%s). Read Access Required.",
dropZoneName.str(), netAddressStr.str(), pathStr.str(), context.queryUserId(), getSecAccessFlagName(permission));
validateDropZoneReq(context, dropZoneName, netAddressStr, pathStr, SecAccess_Read);
Copy link
Member

Choose a reason for hiding this comment

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

what happens if isDropZoneRestrictionEnabled is disabled?
Can validateDropZoneReq called in this context still throw an error?

Need to make sure all routes are consistent.

It may not be true universally now (?) , but if a dropzone doesn't exist and drop zone restrictions are off, it makes little sense to go on to check any LDAP permissions.

Copy link
Member Author

@wangkx wangkx Sep 8, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

The validateDropZoneReq() calls the getDZPathScopePermissions() which handles the isDropZoneRestrictionEnabled if a dropzone cannot be found based on host and path (a bug to be fixed inside the error message). All other DZ permission checks in ESP should go through the getDZPathScopePermissions().
Inside the getDZPathScopePermissions(), if a dropzone name is given, but, a dropzone cannot be found based on the name, an exception is thrown. Should the isDropZoneRestrictionEnabled be checked before throwing the exception (see the dfurun for the similar situation)?

Copy link
Member

Choose a reason for hiding this comment

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

See comment in validateDropZoneReq(), this method is only called for upload/download and maybe 'special' if there is zero expectation for it to be used without a dropzone (since it is in response to the UI presenting a list of files in dropzones).

If there are other situations this could be called - and legacy systems have used upload/download without a corresponding dropzone and with useDropZoneRestriction=false , then you will need to careful you don't break backward compatibility by enforcing it now even with useDropZoneRestriction=false

I think it's probably okay, because used in conjunction with UI only - but the semantics and reasoning needs to be clear (with comments)


StringBuffer fullName;
addPathSepChar(pathStr);
fullName.appendf("%s%s", pathStr.str(), nameStr.str());

StringBuffer headerStr("attachment;");
Expand Down Expand Up @@ -463,13 +458,7 @@ int CFileSpraySoapBindingEx::onStartUpload(IEspContext& ctx, CHttpRequest* reque
request->getParameter("NetAddress", netAddress);
request->getParameter("Path", path);
request->getParameter("DropZoneName", dropZoneName);
if (!validateDropZoneHostAndPath(dropZoneName, netAddress, path)) //The path should be the absolute path for the dropzone.
throw makeStringException(ECLWATCH_INVALID_INPUT, "Invalid DropZoneName, NetAddress or Path.");
SecAccessFlags permission = getDZPathScopePermissions(ctx, dropZoneName, path, netAddress);
if (permission < SecAccess_Full)
throw makeStringExceptionV(ECLWATCH_INVALID_INPUT, "Access DropZone Scope %s %s %s not allowed for user %s (permission:%s). Full Access Required.",
dropZoneName.str(), netAddress.str(), path.str(), ctx.queryUserId(), getSecAccessFlagName(permission));

validateDropZoneReq(ctx, dropZoneName, netAddress, path, SecAccess_Full);
Copy link
Member

Choose a reason for hiding this comment

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

same q. as above

return EspHttpBinding::onStartUpload(ctx, request, response, serv, method);
}

Expand Down
49 changes: 16 additions & 33 deletions esp/services/ws_fs/ws_fsService.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2036,10 +2036,7 @@ void CFileSprayEx::readAndCheckSpraySourceReq(IEspContext& context, MemoryBuffer
{
//Based on the tests, the dfuserver only supports the wildcard inside the file name, like '/path/f*'.
//The dfuserver throws an error if the wildcard is inside the path, like /p*ath/file.
SecAccessFlags permission = getDZFileScopePermissions(context, sourcePlaneReq, path, nullptr);
if (permission < SecAccess_Read)
throw makeStringExceptionV(ECLWATCH_INVALID_INPUT, "Access DropZone Scope %s %s not allowed for user %s (permission:%s). Read Access Required.",
sourcePlaneReq.str(), path, context.queryUserId(), getSecAccessFlagName(permission));
validateDZFileScopePermsAndLegacyPhysicalPerms(context, sourcePlaneReq, path, sourceIPReq, SecAccess_Read);
}

if (!sourcePathReq.isEmpty())
Expand Down Expand Up @@ -2585,10 +2582,7 @@ bool CFileSprayEx::onDespray(IEspContext &context, IEspDespray &req, IEspDespray
if (!isEmptyString(destPlane)) // must be true, unless bare-metal and isDropZoneRestrictionEnabled()==false
{
getDropZoneInfoByDestPlane(version, destPlane, destfile, destfileWithPath, umask, destip);
SecAccessFlags permission = getDZFileScopePermissions(context, destPlane, destfileWithPath, destip);
if (permission < SecAccess_Write)
throw makeStringExceptionV(ECLWATCH_INVALID_INPUT, "Access DropZone Scope %s %s not allowed for user %s (permission:%s). Write Access Required.",
destPlane, destfileWithPath.str(), context.queryUserId(), getSecAccessFlagName(permission));
validateDZFileScopePermsAndLegacyPhysicalPerms(context, destPlane, destfileWithPath, destip, SecAccess_Write);
}
else
destfileWithPath.append(destfile).trim();
Expand Down Expand Up @@ -3029,23 +3023,18 @@ bool CFileSprayEx::onFileList(IEspContext &context, IEspFileListRequest &req, IE
}
else
{
StringBuffer dzName;
if (isEmptyString(dropZoneName))
dropZoneName = findDropZonePlaneName(netaddr, sPath, dzName);
if (!isEmptyString(dropZoneName))
Owned<IPropertyTree> dropZone = getDropZoneAndValidateHostAndPath(dropZoneName, netaddr, sPath);
if (dropZone) // In bare-metal and isDropZoneRestrictionEnabled()==false, the dropZone may be nullptr.
{
SecAccessFlags permission = getDZPathScopePermissions(context, dropZoneName, sPath, nullptr);
if (permission < SecAccess_Read)
throw makeStringExceptionV(ECLWATCH_INVALID_INPUT, "Access DropZone Scope %s %s not allowed for user %s (permission:%s). Read Access Required.",
dropZoneName, sPath.str(), context.queryUserId(), getSecAccessFlagName(permission));
if (isEmptyString(dropZoneName))
dropZoneName = dropZone->queryProp("@name");
validateDZPathScopePermsAndLegacyPhysicalPerms(context, dropZoneName, sPath, netaddr, SecAccess_Read);
}

StringArray hosts;
if (isEmptyString(netaddr))
{
Owned<IPropertyTree> dropZone = getDropZonePlane(dropZoneName);
if (!dropZone)
throw makeStringExceptionV(ECLWATCH_INVALID_INPUT, "Unknown landing zone: %s", dropZoneName);
{ // If the netaddr is empty, the dropZoneName will not be empty and the dropZone should be
// set by the getDropZoneAndValidateHostAndPath().
getPlaneHosts(hosts, dropZone);
if (!hosts.ordinality())
hosts.append("localhost");
Expand All @@ -3055,9 +3044,7 @@ bool CFileSprayEx::onFileList(IEspContext &context, IEspFileListRequest &req, IE

ForEachItemIn(i, hosts)
{
const char* host = hosts.item(i);
if (validateDropZoneHostAndPath(dropZoneName, host, sPath))
getPhysicalFiles(context, dropZoneName, host, sPath, fileNameMask, directoryOnly, files);
getPhysicalFiles(context, dropZoneName, hosts.item(i), sPath, fileNameMask, directoryOnly, files);
}
}

Expand Down Expand Up @@ -3145,7 +3132,7 @@ void CFileSprayEx::addPhysicalFile(IEspContext& context, IDirectoryIterator* di,
bool CFileSprayEx::searchDropZoneFiles(IEspContext& context, const char* dropZoneName, const char* server,
const char* dir, const char* relDir, const char* nameFilter, IArrayOf<IConstPhysicalFileStruct>& files, unsigned& filesFound)
{
if (getDZPathScopePermissions(context, dropZoneName, dir, server) < SecAccess_Read)
if (getDZPathScopePermsAndLegacyPhysicalPerms(context, dropZoneName, dir, server, SecAccess_Read) < SecAccess_Read)
return false;

RemoteFilename rfn;
Expand Down Expand Up @@ -3366,7 +3353,7 @@ void CFileSprayEx::getPhysicalFiles(IEspContext &context, const char *dropZoneNa
if (dropZoneName && di->isDir())
{
VStringBuffer fullPath("%s%s", path, fileName.str());
if (getDZPathScopePermissions(context, dropZoneName, fullPath, nullptr) < SecAccess_Read)
if (getDZPathScopePermsAndLegacyPhysicalPerms(context, dropZoneName, fullPath, host, SecAccess_Read) < SecAccess_Read)
continue;
}

Expand Down Expand Up @@ -3477,7 +3464,7 @@ bool CFileSprayEx::onDropZoneFiles(IEspContext &context, IEspDropZoneFilesReques
StringBuffer planeName;
if (isEmptyString(dzName))
dzName = findDropZonePlaneName(netAddress, directoryStr, planeName);
if (!isEmptyString(dzName) && getDZPathScopePermissions(context, dzName, directoryStr, nullptr) < SecAccess_Read)
if (!isEmptyString(dzName) && (getDZPathScopePermsAndLegacyPhysicalPerms(context, dzName, directoryStr, netAddress, SecAccess_Read) < SecAccess_Read))
return false;

bool directoryOnly = req.getDirectoryOnly();
Expand Down Expand Up @@ -3627,12 +3614,7 @@ void CFileSprayEx::checkDropZoneFileScopeAccess(IEspContext &context, const char
if (isEmptyString(dropZoneName))
dropZoneName = findDropZonePlaneName(netAddress, dropZonePath, dzName);
if (!isEmptyString(dropZoneName))
{
SecAccessFlags permission = getDZPathScopePermissions(context, dropZoneName, dropZonePath, nullptr);
if (permission < accessReq)
throw makeStringExceptionV(ECLWATCH_INVALID_INPUT, "Access DropZone Scope %s %s not allowed for user %s (permission:%s). %s Permission Required.",
dropZoneName, dropZonePath, context.queryUserId(), getSecAccessFlagName(permission), accessReqName);
}
validateDZPathScopePermsAndLegacyPhysicalPerms(context, dropZoneName, dropZonePath, netAddress, accessReq);

RemoteFilename rfn;
SocketEndpoint ep(netAddress);
Expand Down Expand Up @@ -3674,7 +3656,8 @@ void CFileSprayEx::checkDropZoneFileScopeAccess(IEspContext &context, const char
StringBuffer fullPath(dropZonePath);
addPathSepChar(fullPath).append(pathToCheck);
//If the dropzone name is not found, the DZPathScopePermissions cannot be validated.
SecAccessFlags permission = isEmptyString(dropZoneName) ? accessReq : getDZPathScopePermissions(context, dropZoneName, fullPath, nullptr);
SecAccessFlags permission = isEmptyString(dropZoneName) ? accessReq
: getDZPathScopePermsAndLegacyPhysicalPerms(context, dropZoneName, fullPath, netAddress, accessReq);
if (permission < accessReq)
{
uniquePath.setValue(pathToCheck.str(), false); //add a path denied
Expand Down
Loading