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

wangkx · 2023-08-08T12:52:05Z

Type of change:

This change is a bug fix (non-breaking change which fixes an issue).
This change is a new feature (non-breaking change which adds functionality).
This change improves the code (refactor or other change that does not change the functionality)
This change fixes warnings (the fix does not alter the functionality or the generated code)
This change is a breaking change (fix or feature that will cause existing behavior to change).
This change alters the query API (existing queries will have to be recompiled)

Checklist:

Smoketest:

Send notifications about my Pull Request position in Smoketest queue.
Test my draft Pull Request.

Testing:

github-actions · 2023-08-08T12:54:21Z

https://track.hpccsystems.com/browse/HPCC-30037
Jira updated

wangkx · 2023-08-08T14:23:51Z

@jakesmith please review the second commit which is based on the commit in #17528.

jakesmith · 2023-08-15T14:35:25Z

@wangkx - before I review, now that https://track.hpccsystems.com/browse/HPCC-29246 is merged, can you rebase and change from draft status?

jakesmith

@wangkx - please rebase

wangkx · 2023-08-15T17:48:59Z

Rebased

jakesmith

@wangkx - see comment.

jakesmith · 2023-08-17T10:49:40Z

esp/services/ws_fs/ws_fsService.cpp

-            }
+            //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, sourceIPReq, SecAccess_Read);


Removing the check on if (dropZone) line 2016 (and elsewhere throughout these changes), doesn't look right.

It is conflating 2 different issues:

The existence of a valid dropzone definition in the environment (or no match if restrictions are off)

LDAP checking of that dropzone based on PLANE::::... or failing over to legacy in bare-metal if failover is configured.

If the dropzone in 1) doesn't exist (and was allowed to continue because restrictions off), then it should not continue to scope check.
If the dropzone does exist, it should scope check and that scope checking should failover to legacy physical form if failOverToLegacyPhysicalPerms is on.

NB: it is also wrong in dfurun.cpp, it too should not go on to check if an environment dropzone couldn't be found (and was allowed to proceed because restrictions were off).

If the dropzone in 1) doesn't exist (and was allowed to continue because restrictions off), then it should not continue to scope check.

@jakesmith We still need to check legacy physical form if the failOverToLegacyPhysicalPerms is on, right?

only if dropzone exists. And as noted above, dfurun.cpp should do same.

How about the Foreign File in dfurun.cpp? Should I check legacy physical form there if the failOverToLegacyPhysicalPerms is on?

yes, that is not dependent on whether there's a dropzone or not.
The comments above are only related to what to do when there's no dropzone found (which implies restrictions off too).

wangkx · 2023-09-05T20:43:41Z

@jakesmith please review (I do not keep the previous commit since it is fully changed).

jakesmith

@wangkx - please see comments

jakesmith · 2023-09-08T09:14:37Z

esp/services/ws_fs/ws_fsBinding.cpp

-        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);


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.

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)?

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)

jakesmith · 2023-09-08T09:15:04Z

esp/services/ws_fs/ws_fsBinding.cpp

-        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);


same q. as above

jakesmith · 2023-09-08T09:19:58Z

esp/services/ws_fs/ws_fsService.cpp

@@ -1920,6 +1920,16 @@ static bool parseUNCPath(const char* sprayPath, StringBuffer& localPath, StringB
    return true;
 }

+static void validateDZFileScopePermission(IEspContext& context, const char* dropZoneName, const char* path, const char* host, SecAccessFlags permissionReq)


this new function is very similar and/or overlapping with the new function you've introduced in ws_fsBinding.cpp.
Which makes it hard to see at a glance why there are multiple functions performing very similar functions and easy to introduce differences and inconsistencies.

At a glance, it looks like the one in ws_fsBinding.cpp, could be calling/reusing this one,
They could do with renaming to clarify their difference too.

This function calls getDZFileScopePermissions() and the new function in ws_fsBinding.cpp calls getDZPathScopePermissions(). I am going to move (possible rename and merge) them (and the getDZPathScopePermissionsEx()) into TpCommon.cpp.

jakesmith · 2023-09-08T09:26:31Z

esp/services/ws_fs/ws_fsService.cpp

-                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));
+                validateDZFileScopePermission(context, sourcePlaneReq, path, sourceIPReq, SecAccess_Read);


I think validateDZFileScopePermission will allow access if dropzone restrictions are disabled, whether or not the dropzone exists .... it shouldn't be checking permissions.

But it's not clear that is the case looking at validateDZFileScopePermission.
In these common helper functions, it would be good if it clearly commented the semantics, including what happens when dropzone restrictions are disabled.
Another reason to ensure as much is common as possible (see other comments)

jakesmith · 2023-09-08T09:30:49Z

esp/services/ws_fs/ws_fsService.cpp

@@ -2975,6 +2979,15 @@ bool CFileSprayEx::onDFUWUFile(IEspContext &context, IEspDFUWUFileRequest &req,
    return true;
 }

+static SecAccessFlags getDZPathScopePermissionsEx(IEspContext &context, const char *dropZoneName, const char *path,


this also looks like a very similar function to other new helper functions introduced (a subset).
All these similar functions should be in one place and utilize each other and be clearer how they behave and how they differ.

jakesmith · 2023-09-08T12:10:13Z

esp/smc/SMCLib/TpCommon.cpp

+
+    //The setExternal() requests the path with a file name. The file name is not used by the getDLFNPermissions().
+    //If the dropZoneFile is a path only, the dropZoneScope + a faked file name is passed to the setExternal().
+    StringBuffer pathWithFileName(dropZoneFile);


it would help if there were 2 functions one for Path and one for File (getLegacyDZPhysicalFilePerms, getLegacyDZPhysicalPathPerms)
And, rather than this workaround for the lack of support from IDistributedFileDirectory for validating a path that isn't a file, it would be better to add support.
What's missing I think is a simple:

IDistributedFileDirectory::getScopePermissions(const char *scopes, ....)

method.

If that is added, then the common code for that getLegacyDZPhysicalFilePerms and getLegacyDZPhysicalPathPerms calls , can use a CDfsLogicalFileName and conditionally do:

if (isPath) scopesToCheck = dlfn.get(); else { dlfn.getScopes(scopes); scopesToCheck = scopes; } return queryDistributedFileDirectory().getScopePermissions(scopesToCheck, userDesc);

jakesmith

@wangkx - looks better, a couple of questions surrounding useDropZoneRestriction in the case of upload/download.
Could still do with more clarification (comments) on the different but very similar functions in TpCommon.cpp.

And, I think there's more scope to rationalize them further, but that can wait for another day.

jakesmith · 2023-09-15T12:00:04Z

esp/services/ws_fs/ws_fsBinding.cpp

@@ -412,12 +412,7 @@ int CFileSpraySoapBindingEx::downloadFile(IEspContext &context, CHttpRequest* re
        pathStr.replace(pathSep=='\\'?'/':'\\', pathSep);
        addPathSepChar(pathStr);


would be slightly clearer to add this after the call to validateDropZoneReq
being here, makes it look like it is required for the call.

jakesmith · 2023-09-15T12:13:19Z

esp/smc/SMCLib/TpCommon.cpp

+    return permission;
+}
+
+//Validate dropzone host and file path. Also validate dropzone file permission when the file path does not contain a file name.


this is used in 2 contexts, download and upload.

Is it correct that it does not check isDropZoneRestrictionEnabled() ?

Maybe it is ok, because it is expected to be serviced by UI interactions that have listed dropzones - so it is wholly unexpected for their to be a request here without a valid dropzone.

If it is okay, it needs a clear comment as to what this is for and why it does not check isDropZoneRestrictionEnabled() in this case.

when the file path does not contain a file name.

When rewriting the comment I would avoid using 'when' it makes it sound like it could be either a path to a file or a path without a file.
Consistently calling naming the parameter 'path' or 'filePath' would be clearer.

The problem is inside the validateDropZoneHostAndPath() which is called by validateDropZoneReq() and not check isDropZoneRestrictionEnabled(). It should be fixed because the validateDropZoneHostAndPath() is also called by the FileSpray.FileList. I am working on it now.

jakesmith · 2023-09-15T12:17:59Z

esp/services/ws_fs/ws_fsBinding.cpp

-        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);


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)

jakesmith · 2023-09-15T13:47:44Z

esp/services/ws_fs/ws_fsService.cpp

-        if (!isEmptyString(dzName) && getDZPathScopePermissions(context, dzName, directoryStr, nullptr) < SecAccess_Read)
-            return false;
+        if (!isEmptyString(dzName) && (getDZPathScopePermsAndLegacyPhysicalPerms(context, dzName, directoryStr, netAddress, SecAccess_Read) < SecAccess_Read))
+                return false;


odd indentation

Will fit it

wangkx · 2023-09-19T21:34:53Z

@jakesmith please review my changes.

jakesmith

@wangkx - looks clearer, and ready. 2 trivial spacing issues.
Please address those and squash

jakesmith · 2023-09-22T12:08:25Z

esp/services/ws_fs/ws_fsService.cpp

-                    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");


trivial/formattnig: spacing around '='

jakesmith · 2023-09-22T12:32:45Z

esp/smc/SMCLib/TpCommon.cpp

+
+    Owned<IPropertyTree> dropZone = getDropZoneAndValidateHostAndPath(dropZoneName, host, filePath);
+    if (dropZone)
+        dropZoneName= dropZone->queryProp("@name");


trivial/formatting: spacing around '='

1. In checkPlaneFilePermissions() (dfurun.cpp), check legacy physical permission only if the plane name is set. 2. Add getLegacyDZPhysicalPerms() in ESP SMCLib. 3. Call getLegacyDZPhysicalPerms() in validateDropZoneAccess() which is used by ESP WsFileIO service. 4. In ESP FileSpray service, check legacy physical permission if plane exists. Revise based on review 1. Add queryDistributedFileDirectory().getFScopePermissions() and call it in the getLegacyDZPhysicalPerms(). 2. Move validateDropZoneReq(), validateDZFileScopePermission() and getDZPathScopePermissionsEx()) into TpCommon.cpp. Rename the getDZPathScopePermissionsEx() to getDZPathScopePermsAndLegacyPhysicalPerms(). Rename the validateDZFileScopePermission() to validateDZFileScopePermissions(). Also revise them. 3. The following 3 files are not called from outside. Change them from extern to static: getDZPathScopePermissions(), getDZFileScopePermissions() and getLegacyDZPhysicalPerms(). 4. Add comments. 5. Fix a bug in the message for the throwOrLogDropZoneLookUpError(). 6. 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. 7. Rename the validateDZFileScopePermissions() to validateDZFileScopePermsAndLegacyPhysicalPerms() 8. Consoliate the code in FileSprayEx::onFileList() by using the getDropZoneAndValidateHostAndPath(). 9. Add validateDZPathScopePermsAndLegacyPhysicalPerms() which contains the common code for the validateDropZoneReq() and WsFileSprayEx. 10. Revise the code in validateDropZoneReq(). 11. Move the addPathSepChar() line, fix an intent bug, and fix a message bug. 12. Add more comments. Signed-off-by: wangkx <[email protected]>

wangkx · 2023-09-22T19:14:04Z

@ghalliday please merge this PR.

wangkx requested a review from jakesmith August 8, 2023 14:21

jakesmith reviewed Aug 15, 2023

View reviewed changes

wangkx force-pushed the h30037 branch from 8f42dbd to e696731 Compare August 15, 2023 17:57

wangkx marked this pull request as ready for review August 15, 2023 18:12

wangkx requested a review from jakesmith August 15, 2023 18:12

jakesmith requested changes Aug 17, 2023

View reviewed changes

wangkx force-pushed the h30037 branch from e696731 to bdb9f0b Compare August 18, 2023 17:05

wangkx force-pushed the h30037 branch from bdb9f0b to cf5c6e1 Compare September 5, 2023 19:07

wangkx requested a review from jakesmith September 5, 2023 20:43

jakesmith requested changes Sep 8, 2023

View reviewed changes

wangkx requested a review from jakesmith September 13, 2023 12:40

jakesmith reviewed Sep 15, 2023

View reviewed changes

wangkx force-pushed the h30037 branch from f64c34c to 00bebf5 Compare September 19, 2023 18:07

wangkx requested a review from jakesmith September 19, 2023 21:29

jakesmith approved these changes Sep 22, 2023

View reviewed changes

wangkx force-pushed the h30037 branch from 00bebf5 to f2e53c0 Compare September 22, 2023 13:32

ghalliday merged commit bf1ce49 into hpcc-systems:candidate-9.2.x Oct 4, 2023
49 checks passed

		@@ -412,12 +412,7 @@ int CFileSpraySoapBindingEx::downloadFile(IEspContext &context, CHttpRequest* re
		pathStr.replace(pathSep=='\\'?'/':'\\', pathSep);
		addPathSepChar(pathStr);

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

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

Conversation

wangkx commented Aug 8, 2023

Type of change:

Checklist:

Smoketest:

Testing:

github-actions bot commented Aug 8, 2023

wangkx commented Aug 8, 2023

jakesmith commented Aug 15, 2023

jakesmith left a comment

Choose a reason for hiding this comment

wangkx commented Aug 15, 2023 • edited Loading

jakesmith left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wangkx commented Sep 5, 2023

jakesmith left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wangkx Sep 8, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jakesmith left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wangkx commented Sep 19, 2023 • edited Loading

jakesmith left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wangkx commented Sep 22, 2023

wangkx commented Aug 15, 2023 •

edited

Loading

wangkx Sep 8, 2023 •

edited

Loading

wangkx commented Sep 19, 2023 •

edited

Loading