From 00bebf5f5b65a71e7df9553278c512c0aeaa7a8a Mon Sep 17 00:00:00 2001 From: wangkx Date: Fri, 15 Sep 2023 10:36:45 -0400 Subject: [PATCH] HPCC-30037 Revise based on review 1. Rename the validateDropZoneHostAndPath() to getDropZoneAndValidateHostAndPath() and revise it. It is called when uploading/downloading dropzone file and in WsFileSprayEx::onFileList(). There are 2 issues in the existing code: a. some code are duplicated with the code in FileSprayEx::onFileList(). b. the isDropZoneRestrictionEnabled is not used. 2. Rename the validateDZFileScopePermissions() to validateDZFileScopePermsAndLegacyPhysicalPerms() 3. Consoliate the code in FileSprayEx::onFileList() by using the getDropZoneAndValidateHostAndPath(). 4. Add validateDZPathScopePermsAndLegacyPhysicalPerms() which contains the common code for the validateDropZoneReq() and WsFileSprayEx. 5. Revise the code in validateDropZoneReq(). 6. Move the addPathSepChar() line, fix an intent bug, and fix a message bug. 7. Add more comments. Signed-off-by: wangkx --- esp/services/ws_fs/ws_fsBinding.cpp | 2 +- esp/services/ws_fs/ws_fsService.cpp | 36 ++++------ esp/smc/SMCLib/TpCommon.cpp | 104 ++++++++++++++++++---------- esp/smc/SMCLib/TpWrapper.hpp | 6 +- 4 files changed, 85 insertions(+), 63 deletions(-) diff --git a/esp/services/ws_fs/ws_fsBinding.cpp b/esp/services/ws_fs/ws_fsBinding.cpp index a404e875e93..f64b89286d0 100644 --- a/esp/services/ws_fs/ws_fsBinding.cpp +++ b/esp/services/ws_fs/ws_fsBinding.cpp @@ -410,11 +410,11 @@ int CFileSpraySoapBindingEx::downloadFile(IEspContext &context, CHttpRequest* re if (!osStr.isEmpty() && (atoi(osStr.str())== OS_WINDOWS)) pathSep = '\\'; pathStr.replace(pathSep=='\\'?'/':'\\', pathSep); - addPathSepChar(pathStr); validateDropZoneReq(context, dropZoneName, netAddressStr, pathStr, SecAccess_Read); StringBuffer fullName; + addPathSepChar(pathStr); fullName.appendf("%s%s", pathStr.str(), nameStr.str()); StringBuffer headerStr("attachment;"); diff --git a/esp/services/ws_fs/ws_fsService.cpp b/esp/services/ws_fs/ws_fsService.cpp index f01f5b03153..10abeb77f01 100644 --- a/esp/services/ws_fs/ws_fsService.cpp +++ b/esp/services/ws_fs/ws_fsService.cpp @@ -2036,7 +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. - validateDZFileScopePermissions(context, sourcePlaneReq, path, sourceIPReq, SecAccess_Read); + validateDZFileScopePermsAndLegacyPhysicalPerms(context, sourcePlaneReq, path, sourceIPReq, SecAccess_Read); } if (!sourcePathReq.isEmpty()) @@ -2582,7 +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); - validateDZFileScopePermissions(context, destPlane, destfileWithPath, destip, SecAccess_Write); + validateDZFileScopePermsAndLegacyPhysicalPerms(context, destPlane, destfileWithPath, destip, SecAccess_Write); } else destfileWithPath.append(destfile).trim(); @@ -3023,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 dropZone = getDropZoneAndValidateHostAndPath(dropZoneName, netaddr, sPath); + if (dropZone) // In bare-metal and isDropZoneRestrictionEnabled()==false, the dropZone may be nullptr. { - SecAccessFlags permission = getDZPathScopePermsAndLegacyPhysicalPerms(context, dropZoneName, sPath, netaddr, SecAccess_Read); - if (permission < SecAccess_Read) - throw makeStringExceptionV(ECLWATCH_INVALID_INPUT, "Access DropZone %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 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"); @@ -3049,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); } } @@ -3472,7 +3465,7 @@ bool CFileSprayEx::onDropZoneFiles(IEspContext &context, IEspDropZoneFilesReques if (isEmptyString(dzName)) dzName = findDropZonePlaneName(netAddress, directoryStr, planeName); if (!isEmptyString(dzName) && (getDZPathScopePermsAndLegacyPhysicalPerms(context, dzName, directoryStr, netAddress, SecAccess_Read) < SecAccess_Read)) - return false; + return false; bool directoryOnly = req.getDirectoryOnly(); IArrayOf &files = resp.getFiles(); @@ -3621,12 +3614,7 @@ void CFileSprayEx::checkDropZoneFileScopeAccess(IEspContext &context, const char if (isEmptyString(dropZoneName)) dropZoneName = findDropZonePlaneName(netAddress, dropZonePath, dzName); if (!isEmptyString(dropZoneName)) - { - SecAccessFlags permission = getDZPathScopePermsAndLegacyPhysicalPerms(context, dropZoneName, dropZonePath, netAddress, accessReq); - 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); diff --git a/esp/smc/SMCLib/TpCommon.cpp b/esp/smc/SMCLib/TpCommon.cpp index 74790ba4a2f..b235c4f2a6f 100644 --- a/esp/smc/SMCLib/TpCommon.cpp +++ b/esp/smc/SMCLib/TpCommon.cpp @@ -148,33 +148,38 @@ StringBuffer &findDropZonePlaneName(const char *host, const char *path, StringBu return planeName; } -extern TPWRAPPER_API bool validateDropZoneHostAndPath(const char* dropZoneName, const char* hostToCheck, const char* pathToCheck) +//If the dropZoneName is empty, find out the dropzone based on the host and path. In bare-metal and +//isDropZoneRestrictionEnabled()==false, the dropzone may not be found. +//If the dropZoneName is not empty, find out the dropzone based on the dropZoneName and validate the host and path. +extern TPWRAPPER_API IPropertyTree* getDropZoneAndValidateHostAndPath(const char* dropZoneName, const char* host, const char* path) { - //Both hostToCheck and pathToCheck should not be empty. For backward compatibility, the dropZoneName may be empty. - if (isEmptyString(hostToCheck)) - throw makeStringException(ECLWATCH_INVALID_INPUT, "Host not defined."); - if (isEmptyString(pathToCheck)) - throw makeStringException(ECLWATCH_INVALID_INPUT, "Path not defined."); - if (isContainerized() && streq("localhost", hostToCheck)) - hostToCheck = nullptr; // "localhost" is a placeholder for mounted dropzones that have no hosts. - if (isEmptyString(hostToCheck) && isEmptyString(dropZoneName)) - throw makeStringException(ECLWATCH_INVALID_INPUT, "No dropzone or host provided."); + StringBuffer pathToCheck(path); + addPathSepChar(pathToCheck); - if (containsRelPaths(pathToCheck)) //Detect a path like: /home/lexis/runtime/var/lib/HPCCSystems/mydropzone/../../../ - throw makeStringExceptionV(ECLWATCH_INVALID_INPUT, "Invalid path %s", pathToCheck); + const char* hostToCheck = nullptr; + if (isContainerized() && strsame(host, "localhost")) + hostToCheck = nullptr; // "localhost" is a placeholder for mounted dropzones that have no hosts. + else + hostToCheck = host; - StringBuffer path(pathToCheck); - addPathSepChar(path); + Owned dropZone; if (isEmptyString(dropZoneName)) { - Owned plane = findDropZonePlane(path, hostToCheck, isIPAddress(hostToCheck), false); - return nullptr != plane; + dropZone.setown(findDropZonePlane(pathToCheck, hostToCheck, isIPAddress(hostToCheck), false)); + if (!dropZone) + throwOrLogDropZoneLookUpError(ECLWATCH_INVALID_INPUT, "DropZone not found for host '%s' path '%s'.", + isEmptyString(host) ? "unspecified" : host, isEmptyString(path) ? "unspecified" : path); } - - Owned plane = getDropZonePlane(dropZoneName); - if (nullptr == plane) - return false; - return validateDropZone(plane, path, hostToCheck, isIPAddress(hostToCheck)); + else + { + dropZone.setown(getDropZonePlane(dropZoneName)); + if (!dropZone) + throw makeStringExceptionV(ECLWATCH_INVALID_INPUT, "DropZone '%s' not found.", dropZoneName); + if (!validateDropZone(dropZone, pathToCheck, hostToCheck, isIPAddress(hostToCheck))) + throw makeStringExceptionV(ECLWATCH_INVALID_INPUT, "The host '%s' or path '%s' is not valid for dropzone %s.", + isEmptyString(host) ? "unspecified" : host, isEmptyString(path) ? "unspecified" : path, dropZoneName); + } + return dropZone.getClear(); } static SecAccessFlags getDropZoneScopePermissions(IEspContext& context, const IPropertyTree* dropZone, const char* dropZonePath) @@ -207,7 +212,7 @@ static SecAccessFlags getDZPathScopePermissions(IEspContext& context, const char dropZone.setown(findDropZonePlane(dropZonePath, dropZoneHost, true, false)); if (!dropZone) { - throwOrLogDropZoneLookUpError(ECLWATCH_INVALID_INPUT, "getDZPathScopePermissions(): DropZone %s not found for host '%s' path '%s'.", + throwOrLogDropZoneLookUpError(ECLWATCH_INVALID_INPUT, "getDZPathScopePermissions(): DropZone not found for host '%s' path '%s'.", isEmptyString(dropZoneHost) ? "unspecified" : dropZoneHost, isEmptyString(dropZonePath) ? "unspecified" : dropZonePath); return SecAccess_Full; } @@ -266,9 +271,10 @@ static SecAccessFlags getLegacyDZPhysicalPerms(IEspContext& context, const char* return queryDistributedFileDirectory().getFScopePermissions(scopesToCheck, userDesc); } -//Get dropzone file permission when the file path does not contain a file name. To get the permission, -//this function (and the validate functions below) calls the getDZPathScopePermissions() which handles -//isDropZoneRestrictionEnabled if a dropzone cannot be found based on host and path. +//Get dropzone file permission for a dropzone file path (the path does NOT contain a file name). +//This function (and the validate functions below) calls the getDZPathScopePermissions(). In the +//getDZPathScopePermissions(), the isDropZoneRestrictionEnabled() will be used for bare-metal if +//a dropzone cannot be found based on host and path. extern TPWRAPPER_API SecAccessFlags getDZPathScopePermsAndLegacyPhysicalPerms(IEspContext &context, const char *dropZoneName, const char *path, const char *host, SecAccessFlags permissionReq) { @@ -278,20 +284,19 @@ extern TPWRAPPER_API SecAccessFlags getDZPathScopePermsAndLegacyPhysicalPerms(IE return permission; } -//Validate dropzone host and file path. Also validate dropzone file permission when the file path does not contain a file name. -extern TPWRAPPER_API void validateDropZoneReq(IEspContext& ctx, const char* dropZoneName, const char* host, const char* path, SecAccessFlags permissionReq) +//Validate dropzone file permission for a dropzone file path (the path does NOT contain a file name) +extern TPWRAPPER_API void validateDZPathScopePermsAndLegacyPhysicalPerms(IEspContext &context, const char *dropZoneName, const char *path, + const char *host, SecAccessFlags permissionReq) { - if (!validateDropZoneHostAndPath(dropZoneName, host, path)) //The pathStr should be the absolute path for the dropzone. - throw makeStringException(ECLWATCH_INVALID_INPUT, "Invalid DropZoneName, NetAddress or Path."); - - SecAccessFlags permission = getDZPathScopePermsAndLegacyPhysicalPerms(ctx, dropZoneName, path, host, permissionReq); + SecAccessFlags permission = getDZPathScopePermsAndLegacyPhysicalPerms(context, dropZoneName, path, host, permissionReq); if (permission < permissionReq) - throw makeStringExceptionV(ECLWATCH_INVALID_INPUT, "Access DropZone %s %s %s not allowed for user %s (permission:%s). %s permission required.", - dropZoneName, host, path, ctx.queryUserId(), getSecAccessFlagName(permission), getSecAccessFlagName(permissionReq)); + throw makeStringExceptionV(ECLWATCH_INVALID_INPUT, "Access DropZone %s %s not allowed for user %s (permission:%s). Read Access Required.", + dropZoneName, path, context.queryUserId(), getSecAccessFlagName(permission)); } -//Validate dropzone file permission when the file path contains a file name. -extern TPWRAPPER_API void validateDZFileScopePermissions(IEspContext& context, const char* dropZoneName, const char* path, const char* host, SecAccessFlags permissionReq) +//Validate dropzone file permission for a dropzone file path (the path DOES contain a file name) +extern TPWRAPPER_API void validateDZFileScopePermsAndLegacyPhysicalPerms(IEspContext& context, const char* dropZoneName, const char* path, + const char* host, SecAccessFlags permissionReq) { SecAccessFlags permission = getDZFileScopePermissions(context, dropZoneName, path, host); if ((permission < permissionReq) && getGlobalConfigSP()->getPropBool("expert/@failOverToLegacyPhysicalPerms", !isContainerized())) @@ -301,7 +306,34 @@ extern TPWRAPPER_API void validateDZFileScopePermissions(IEspContext& context, c dropZoneName, path, context.queryUserId(), getSecAccessFlagName(permission), getSecAccessFlagName(permissionReq)); } -//Validate dropzone file path and permission, Also set the dlfn. +//This function is called when uploading a file to a dropzone or downloading a file to a dropzone. +//1. both host and filePath should not be empty. +//2. the filePath should not contain bad file path. +//3. call the getDropZoneAndValidateHostAndPath(). +//4. if the dropzone name is available, validate dropzone file permission. Should be available unless bare-metal and isDropZoneRestrictionEnabled()==false. +extern TPWRAPPER_API void validateDropZoneReq(IEspContext& ctx, const char* dropZoneName, const char* host, const char* filePath, SecAccessFlags permissionReq) +{ + if (isEmptyString(host) || isEmptyString(filePath)) //The host and filePath must be specified for accessing a DropZone file. + throw makeStringException(ECLWATCH_INVALID_INPUT, "Host or file path not defined."); + + if (containsRelPaths(filePath)) //Detect a path like: /home/lexis/runtime/var/lib/HPCCSystems/mydropzone/../../../ + throw makeStringExceptionV(ECLWATCH_INVALID_INPUT, "Invalid path %s", filePath); + + Owned dropZone = getDropZoneAndValidateHostAndPath(dropZoneName, host, filePath); + if (dropZone) + dropZoneName= dropZone->queryProp("@name"); + + if (!isEmptyString(dropZoneName)) + validateDZPathScopePermsAndLegacyPhysicalPerms(ctx, dropZoneName, filePath, host, permissionReq); +} + +//This function is called when accessing a dropzone file from the WsFileIO service (the fileNameWithRelPath is a relative path). +//1. the targetDZNameOrHost could be a dropzone name or dropzone host. Find out the dropzone. +// If the targetDZNameOrHost is a dropzone name and the hostReq is not empty, validate the hostReq inside the dropzone. +// If the dropzone cannot be found based on the targetDZNameOrHost, throw an exception. +//2. the fileNameWithRelPath should not contain bad file path. +//3. validate dropzone file permission. +//4. set the dlfn using dropzone name and fileNameWithRelPath. extern TPWRAPPER_API void validateDropZoneAccess(IEspContext& context, const char* targetDZNameOrHost, const char* hostReq, SecAccessFlags permissionReq, const char* fileNameWithRelPath, CDfsLogicalFileName& dlfn) { diff --git a/esp/smc/SMCLib/TpWrapper.hpp b/esp/smc/SMCLib/TpWrapper.hpp index c8a882a09ed..3c68dd133c7 100644 --- a/esp/smc/SMCLib/TpWrapper.hpp +++ b/esp/smc/SMCLib/TpWrapper.hpp @@ -240,12 +240,14 @@ extern TPWRAPPER_API bool validateDataPlaneName(const char *remoteDali, const ch extern TPWRAPPER_API bool matchNetAddressRequest(const char* netAddressReg, bool ipReq, IConstTpMachine& tpMachine); extern TPWRAPPER_API StringBuffer &findDropZonePlaneName(const char* host, const char* path, StringBuffer& planeName); -extern TPWRAPPER_API bool validateDropZoneHostAndPath(const char* dropZoneName, const char* hostToCheck, const char* pathToCheck); +extern TPWRAPPER_API IPropertyTree* getDropZoneAndValidateHostAndPath(const char* dropZoneName, const char* host, const char* path); extern TPWRAPPER_API void validateDropZoneAccess(IEspContext& context, const char* targetDZNameOrHost, const char* hostReq, SecAccessFlags permissionReq, const char* fileNameWithRelPath, CDfsLogicalFileName& dlfn); extern TPWRAPPER_API SecAccessFlags getDZPathScopePermsAndLegacyPhysicalPerms(IEspContext &context, const char *dropZoneName, const char *path, const char *host, SecAccessFlags permissionReq); +extern TPWRAPPER_API void validateDZPathScopePermsAndLegacyPhysicalPerms(IEspContext &context, const char *dropZoneName, const char *path, + const char *host, SecAccessFlags permissionReq); extern TPWRAPPER_API void validateDropZoneReq(IEspContext& ctx, const char* dropZoneName, const char* host, const char* path, SecAccessFlags permissionReq); -extern TPWRAPPER_API void validateDZFileScopePermissions(IEspContext& context, const char* dropZoneName, const char* path, const char* host, SecAccessFlags permissionReq); +extern TPWRAPPER_API void validateDZFileScopePermsAndLegacyPhysicalPerms(IEspContext& context, const char* dropZoneName, const char* path, const char* host, SecAccessFlags permissionReq); #endif //_ESPWIZ_TpWrapper_HPP__