HPCC-29246 Check Plane scope in DFU server when spraying/despraying #17528

wangkx · 2023-06-29T19:47:57Z

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-06-29T20:03:13Z

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

jakesmith

@wangkx - please see feedback.

I think needs reworking to explicitly handle the passed plane name in the dfu workunit., common the code if poss. that has to do very similar in the service, and avoid adding new methods to DFS + avoid use of IFileDescriptor's for scope checking, and go directly to CDfsLogicalFileName or methods that build them like queryDistributedFileDirectory().getDropZoneScopePermissions

dali/dfu/dfurun.cpp

wangkx · 2023-07-12T20:40:13Z

@jakesmith please review last 2 commits.

jakesmith

@wangkx - please see feedback.

jakesmith · 2023-07-13T09:07:57Z

dali/dfu/dfurun.cpp

@@ -595,12 +595,57 @@ class CDFUengine: public CInterface, implements IDFUengine
            auditflags |= DALI_LDAP_WRITE_WANTED;
        SecAccessFlags perm = queryDistributedFileDirectory().getFDescPermissions(fd,user,auditflags);
        IDFS_Exception *e = NULL;
-        if (!HASREADPERMISSION(perm))
+        if (!write&&!HASREADPERMISSION(perm))
            throw MakeStringException(DFSERR_LookupAccessDenied,"Lookup permission denied for physical file(s)");
        if (write&&!HASWRITEPERMISSION(perm))
            throw MakeStringException(DFSERR_CreateAccessDenied,"Create permission denied for physical file(s)");


I think the above is a little clearer if:

if (write) { if (!HASREADPERMISSION(perm)) throw MakeStringException(DFSERR_CreateAccessDenied,"Create permission denied for physical file(s)"); } else if (!HASWRITEPERMISSION(perm)) throw MakeStringException(DFSERR_LookupAccessDenied,"Lookup permission denied for physical file(s)");

also, not new, but worth changing to makeStringExceptionV and including the name of the file in the error.
I think fd->getTraceName will provide what you need

jakesmith · 2023-07-13T09:22:32Z

dali/dfu/dfurun.cpp

@@ -595,12 +595,57 @@ class CDFUengine: public CInterface, implements IDFUengine
            auditflags |= DALI_LDAP_WRITE_WANTED;
        SecAccessFlags perm = queryDistributedFileDirectory().getFDescPermissions(fd,user,auditflags);
        IDFS_Exception *e = NULL;


not new, but unused/should be removed.

jakesmith · 2023-07-13T09:26:25Z

dali/dfu/dfurun.cpp

+
+        SecAccessFlags perm;
+        bool checkLegacyPhysicalPerms = getGlobalConfigSP()->getPropBool("expert/@failOverToLegacyPhysicalPerms", !isContainerized());
+        IClusterInfo *iClusterInfo = LINK(fd->queryClusterNum(0));


why is this linking? looks like it will leak.

The code is copied from dadfs.cpp. I guess the LINK should be removed since a query function should return a pointer.

right, yes should link like this unless it's passing ownership to something else.

jakesmith · 2023-07-13T09:27:19Z

dali/dfu/dfurun.cpp

+        bool checkLegacyPhysicalPerms = getGlobalConfigSP()->getPropBool("expert/@failOverToLegacyPhysicalPerms", !isContainerized());
+        IClusterInfo *iClusterInfo = LINK(fd->queryClusterNum(0));
+        const char *planeName = iClusterInfo->queryGroupName();
+        if (isEmptyString(planeName)) {


formatting: there's inconsistency in this cpp file in general, but when modifying a method or a class, the preferred formatting is with the start block on a new line.

jakesmith · 2023-07-13T11:02:49Z

dali/dfu/dfurun.cpp

+                throw makeStringException(-1, "Empty default directory.");
+
+            StringBuffer relativePath;
+            if (!getDropZoneRelativePath(planeName,dir,relativePath))


This looks very similar to some of the functionality in the TpCommon.cpp, in : getDropZoneScopePermissions(IEspContext& context, const IPropertyTree* dropZone, const char* dropZonePath)

I think it should call a common function for relative path extraction.

see comments re. adding a jfile getRelativePath and using directly here (and in TpCommon.cpp)

jakesmith · 2023-07-13T11:09:51Z

dali/base/dautils.cpp

+    return str;
+}
+
+bool getDropZoneRelativePath(const char * dropZoneName, const char * dropZonePath, StringBuffer & relativePath)


you may want a version that takes a IPropertyTree *dropZone too,
see comment re. duplication with functionality in TpCommon.cpp's getDropZoneScopePermissions.

but .. see other comments, re. using getRelativePath

jakesmith · 2023-07-13T11:59:47Z

dali/base/dautils.cpp

+        return false;
+
+    if (strlen(dropZonePath) > lenPrefix)
+        relativePath.append(skipLeadingPathSepChars(dropZonePath + lenPrefix));


if there were multiple separators, that would be an invalid path, not sure this is the place to try to cope with invalid paths, there would be many places that would need to if they were invalid like this.

There's an existing generalized jfile function called 'splitRelativePath', which is almost suitable, but not quite.
I think you should add a genaralized function (to jfile) that returns the relative path from a path, given a leading path:

const char *getRelativePath(const char *path, const char *leadingPath) { size_t len = strlen(leadingPath); if (0 == strncmp(path, leadingPath, len)) { const char *rel = path + len; if ('\0' == *rel) return rel; if (isPathSepChar(*rel)) return rel+1; } return nullptr; }

and then I'm not sure much value in having getDropZoneRelativePath functions.
The dfurun code that uses getRelativePath becomes:

Owned<IPropertyTree> dropZonePlane = getDropZonePlane(planeName); if (!dropZonePlane) throw makeStringExceptionV(-1, "DropZone %s not found.", planeName); const char *relativePath = getRelativePath(dir, dropZonePlane->queryProp("@prefix")); if (nullptr == relPath) throw makeStringExceptionV(-1, "Invalid DropZone directory %s.", dir);

@jakesmith is it possible that the last char of the prefix is a PathSepChar and the last char of the dir is not? For example: the prefix is '/var/lib/HPCCSystems/mydropzone/' and the dir is '/var/lib/HPCCSystems/mydropzone'.

Maybe. We should ensure the helper function handles that case.
You could change the function to see if pathLen == leadingLen+1 && isPathSepChar(path[pathLen-1])) ...
then --leadingLen ...

jakesmith · 2023-07-13T12:35:33Z

dali/dfu/dfurun.cpp

+#endif
+            if (!checkLegacyPhysicalPerms)
+                throw makeStringException(-1, "Empty plane name.");
+            perm = queryDistributedFileDirectory().getFDescPermissions(fd,user,auditflags);


you can put this code in the #ifndef above, since there is no intention for it to apply to containerized setups,
if plane is blank in containerized then it's an unexpected error:

#ifndef _CONTAINERIZED Owned<IEnvironmentFactory> factory = getEnvironmentFactory(true); Owned<IConstEnvironment> env = factory->openEnvironment(); if (env->isDropZoneRestrictionEnabled() || !checkLegacyPhysicalPerms) 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

jakesmith · 2023-07-13T12:35:48Z

dali/dfu/dfurun.cpp

+                throw makeStringException(-1, "Empty plane name.");
+            perm = queryDistributedFileDirectory().getFDescPermissions(fd,user,auditflags);
+        }
+        else {


formatting: start block on newline

minor: the else case should be the normal case, eventually we should aim to delete the paths that look in the environment and check isDropZoneRestrictionEnabled..
So it is better/clearer to put the normal case 1st, and the edge case in the else blocks.

jakesmith · 2023-07-13T13:07:39Z

dali/dfu/dfurun.cpp

+        //This function checks the scope permission for one or multiple plane files.
+        //For the multiple files case, all files must be in the same directory (scope).
+        //The IFileDescriptor is used to retrieve the directory path, as well as
+        //the plane name. It also be used for the legacy check on a physical file format


Can we reword as :

This function checks the scope permissions for a file or files that reside in a single directory on a single plane.
The IFileDescriptor is used to discover the plane and directory.
If the plane is not present, it implies that it is a bare-metal system and useDropZoneRestriction is off, and there is no matching dropzone in the environment.
If this is the case, or the plane permissions are not found and @failOverToLegacyPhysicalPerms is configured, then the legacy physical file permissions will be checked.

wangkx · 2023-07-13T21:48:23Z

@jakesmith please review my changes. I also add/remove many spaces (trying to match the formatting in the same cpp files).

jakesmith

@wangkx - looks good, a couple of minor comments

jakesmith · 2023-07-14T09:17:17Z

dali/dfu/dfurun.cpp

+        {
+            traceName.setLength(255);
+            traceName.append("...");
+        }


Better to only use the above block if needed (if denied).
I'd go with:

if ((write && !HASWRITEPERMISSION(perm)) || (!write && !HASREADPERMISSION(perm))) { StringBuffer traceName; fd->getTraceName(traceName); if (traceName.length() > 255) { traceName.setLength(255); traceName.append("..."); } if (write) throw makeStringExceptionV(DFSERR_CreateAccessDenied, "Create permission denied for physical file(s): %s", traceName.str()); else throw makeStringExceptionV(DFSERR_LookupAccessDenied, "Lookup permission denied for physical file(s): %s", traceName.str()); }

This function may be used by the checkForeignFilePermissions() in #17563. I may change it to:

void ensureFilePermissions(const char * fileName, IFileDescriptor * fd, SecAccessFlags perm, bool write) { if ((write && !HASWRITEPERMISSION(perm)) || (!write && !HASREADPERMISSION(perm))) { StringBuffer traceName; if (isEmptyString(fileName)) { fd->getTraceName(traceName); elideString(traceName, 255); fileName = traceName.str(); } if (write) throw makeStringExceptionV(DFSERR_CreateAccessDenied, "Create permission denied for physical file(s): %s", fileName); else throw makeStringExceptionV(DFSERR_LookupAccessDenied, "Lookup permission denied for physical file(s): %s", fileName); } }

Also add the ensureFilePermissions() into #17563.

jakesmith · 2023-07-14T09:22:39Z

system/jlib/jfile.cpp

+    size_t leadingLen = strlen(leadingPath);
+    if ((pathLen==leadingLen-1)&&isPathSepChar(leadingPath[leadingLen-1]))
+        --leadingLen;
+    if (0 == strncmp(path,leadingPath,leadingLen)) {


trivial: block start on newline prefered formatting style

jakesmith · 2023-07-14T09:24:42Z

dali/dfu/dfurun.cpp

@@ -533,6 +533,24 @@ class CDFUengine: public CInterface, implements IDFUengine
        }
    }

+    void ensureAFilePermissions(IFileDescriptor * fd, SecAccessFlags perm, bool write)


trivial: not sure about the 'A', I think "ensureFilePermissions" would be better.

jakesmith · 2023-07-14T09:27:33Z

dali/dfu/dfurun.cpp

+    {
+        StringBuffer traceName;
+        fd->getTraceName(traceName);
+        if (traceName.length() > 255)


in fact, there's an existing function for this kind of truncation, see jstring's elideString function.

jakesmith · 2023-07-14T12:35:07Z

dali/dfu/dfurun.cpp

+        if ((write && !HASWRITEPERMISSION(perm)) || (!write && !HASREADPERMISSION(perm)))
+        {
+            StringBuffer traceName;
+            if (isEmptyString(fileName))


I'm not keen on the function passing 2 parameters one or both being able to serve up what's required.
I think better to simplify and,pass fileName only, and if seems worth it, put the code to get the tracename and truncate in a separate utility function - used in contexts that require it:

StringBuffer &getFDescName(const IFileDescriptor *fd, StringBuffer &result) { fd->getTraceName(result); elideString(result, 255); }

... .. ensureFilePermissions(getFDescName(name), perm, write);

given u can express this in a couple of lines, not sure it's worth the extra helper function though..

Should we check 'if ((write && !HASWRITEPERMISSION(perm)) || (!write && !HASREADPERMISSION(perm)))' before ensureFilePermissions(getFDescName(name), perm, write); ?

If yes, the name ensureFilePermissions should be changed to throwFilePermissionException? If no, the getFDescName() will be called even if not denied.

If yes, the name ensureFilePermissions should be changed to throwFilePermissionException? If no, the getFDescName() will be called even if not denied.

I can live with it being called unconditionally, readability is more important than 100% efficiency here.
i.e. leave it as ensureFilePermissions and call getFDescName before hand (or the 2 lines of equivalent code)

Leave this PR for now though, let's get #17563 agreed on and merged 1st.

Now, I am not sure that it is worth to change the IFileDescriptor * fd to const char * fileName in the ensureFilePermissions(). Maybe leave the checkForeignFilePermissions() as it is (not call the ensureFilePermissions())?

I suspect it is worth changing and calling from checkForeignFilePermissions, so still exercising common code.

jakesmith · 2023-07-14T14:54:50Z

Please rebase onto latest candidate-9.2.x, now that #17563 is merged.

jakesmith · 2023-07-14T16:31:17Z

esp/smc/SMCLib/TpCommon.cpp

@@ -183,21 +183,13 @@ static SecAccessFlags getDropZoneScopePermissions(IEspContext& context, const IP
        throw makeStringException(ECLWATCH_INVALID_CLUSTER_NAME, "getDropZoneScopePermissions(): DropZone path must be specified.");

    //If the dropZonePath is an absolute path, change it to a relative path.


looks the code below breaks this rule ?
Doesn't it need to conditionally call getRelativePath ? (if isAbsolute..)

Will check if isAbsolute ...

dali/dfu/dfurun.cpp

jakesmith

@wangkx - looks good, but 1 issue I think.

jakesmith

@wangkx - looks good, please squash.

Revise based on review: 1. Merge the getPlaneFilePermissions() into the checkPlaneFilePermissions(). 2. Add option failOverToLegacyPhysicalPerms. 3. If no plane name, check the DropZoneRestrictionEnabled. 4. Check read permission only for read action 5. Change the ensureFilePermissions 6. Call the ensureFilePermissions in checkForeignFilePermissions 7. Check isAbsolute before calling getRelativePath 8. Allow empty relative path in setPlaneExternal() Signed-off-by: wangkx <[email protected]>

wangkx · 2023-08-03T14:18:42Z

@jakesmith the commits are squashed.

jakesmith

@wangkx - looks good.

wangkx · 2023-08-04T13:14:12Z

@ghalliday please merge

wangkx requested a review from jakesmith June 29, 2023 22:16

jakesmith requested changes Jul 10, 2023

View reviewed changes

dali/dfu/dfurun.cpp Show resolved Hide resolved

wangkx force-pushed the h29246 branch 2 times, most recently from c355ce2 to 784616c Compare July 12, 2023 18:09

wangkx requested a review from jakesmith July 12, 2023 20:39

jakesmith reviewed Jul 13, 2023

View reviewed changes

jakesmith reviewed Jul 14, 2023

View reviewed changes

jakesmith mentioned this pull request Jul 14, 2023

HPCC-29480 Check foreign scope in DFU server when foreigncopy #17563

Merged

39 tasks

wangkx force-pushed the h29246 branch 2 times, most recently from 5b0a90e to ab4aaa7 Compare July 14, 2023 15:55

wangkx requested a review from jakesmith July 14, 2023 15:56

jakesmith reviewed Jul 14, 2023

View reviewed changes

dali/dfu/dfurun.cpp Show resolved Hide resolved

jakesmith reviewed Jul 14, 2023

View reviewed changes

wangkx requested a review from jakesmith July 14, 2023 16:57

jakesmith reviewed Aug 3, 2023

View reviewed changes

wangkx force-pushed the h29246 branch from 68cce91 to 85e1288 Compare August 3, 2023 12:23

wangkx force-pushed the h29246 branch from 85e1288 to e3e40de Compare August 3, 2023 12:25

wangkx requested a review from jakesmith August 3, 2023 14:18

jakesmith approved these changes Aug 4, 2023

View reviewed changes

wangkx mentioned this pull request Aug 8, 2023

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

Merged

39 tasks

ghalliday merged commit 37e23b6 into hpcc-systems:candidate-9.2.x Aug 11, 2023
49 checks passed

		@@ -183,21 +183,13 @@ static SecAccessFlags getDropZoneScopePermissions(IEspContext& context, const IP
		throw makeStringException(ECLWATCH_INVALID_CLUSTER_NAME, "getDropZoneScopePermissions(): DropZone path must be specified.");

		//If the dropZonePath is an absolute path, change it to a relative path.

HPCC-29246 Check Plane scope in DFU server when spraying/despraying #17528

HPCC-29246 Check Plane scope in DFU server when spraying/despraying #17528

Conversation

wangkx commented Jun 29, 2023

Type of change:

Checklist:

Smoketest:

Testing:

github-actions bot commented Jun 29, 2023

jakesmith left a comment

Choose a reason for hiding this comment

wangkx commented Jul 12, 2023

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

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

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 Jul 13, 2023

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

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 commented Jul 14, 2023

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

jakesmith left a comment

Choose a reason for hiding this comment

wangkx commented Aug 3, 2023

jakesmith left a comment

Choose a reason for hiding this comment

wangkx commented Aug 4, 2023