HPCC-31761 LDAP only uses first cached default file scope permission #18651

kenrowland · 2024-05-14T17:43:07Z

Added a per user default file scope permission cache

Signed-Off-By: Kenneth Rowland [email protected]

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 · 2024-05-14T17:43:27Z

Jira Issue: https://hpccsystems.atlassian.net//browse/HPCC-31761

Jirabot Action Result:
Workflow Transition: Merge Pending
Updated PR

kenrowland · 2024-05-14T17:48:11Z

@ghalliday @jakesmith preliminary review. Will add a flag to operate as it did. Also need to add read and write locks for the default file scope permission cache. Both will be in the next commit.

jakesmith

@kenrowland - looks good in principle.

But it needs to be made thread safe, afaics as it stands, 1 thread could be clearing or altering m_userDefaultFileScopePermissions, whilst another read from it, which would lead to a crash.

I think if the clear() goes in removeAllManagedFileScopes (with it's WriteLockBlock lock) , and the query is protected by a ReadLockBlock on m_scopesRWLock, it should make it thread safe.

It looks like m_managedFileScopesMap can change whilst it is being processed by another thread

jakesmith · 2024-05-15T10:30:13Z

system/security/shared/caching.cpp

-            m_defaultPermission = SecAccess_None;
+        defaultPermission = m_secMgr->queryDefaultPermission(user);
+        std::string userName(username);
+        m_userDefaultFileScopePermissions.insert(std::pair<std::string, SecAccessFlags>(userName, defaultPermission));


trivial: more concisely expressed (and more efficient) as:

m_userDefaultFileScopePermissions.emplace(username, defaultPermission);

NB: no need for explicit std::string conversion.

kenrowland · 2024-05-15T17:45:30Z

@jakesmith Don't know if you saw my previous comment, but locks were something I am adding.

jakesmith · 2024-05-16T10:25:32Z

@jakesmith Don't know if you saw my previous comment, but locks were something I am adding.

No, sorry, I missed it.
Okay, initial commit looks okay.

jakesmith

@kenrowland - please see comments.

jakesmith · 2024-05-22T09:55:44Z

system/security/LdapSecurity/ldapconnection.cpp

@@ -664,6 +664,7 @@ class CLdapConfig : implements ILdapConfig, public CInterface
            m_sdfieldname.append("aci");
        else if(m_serverType == OPEN_LDAP)
            m_sdfieldname.append("aci");
+


trivial: unintended new newline (?) - nice to not alter this file for git history if so.

Hmm, not sure why, but removed it.

system/security/shared/caching.cpp

jakesmith · 2024-05-22T10:31:02Z

system/security/shared/caching.cpp

+            defaultScopesReadLock.clear();
+            WriteLockBlock defaultScopesWriteLock(m_defaultScopesRWLock);
+            defaultPermission = m_secMgr->queryDefaultPermission(user);
+            std::string userName(username);


at least in my setup, this line is not necessary, i.e. you can pass const char * to the emplace and it will implicitly construct a std::string

The find, in my experience, has to construct an object of the key type. So, I left the username std::string variable, but cleaned it up a bit.

jakesmith · 2024-05-22T10:33:59Z

system/security/shared/caching.cpp

@@ -711,8 +741,16 @@ void CPermissionsCache::flush()
            delete (*ui).second;
        m_userCache.clear();
    }
+    if (m_useLegacyDefaultFileScopePermissionCache)
+    {
+        m_defaultPermission = SecAccess_None;


should this be :
m_defaultPermission = SecAccess_Unknown

.. to trigger the next call to queryDefaultPermission to get new permissions?

Yes, good catch.

kenrowland · 2024-05-22T13:40:45Z

system/security/LdapSecurity/ldapconnection.cpp

@@ -664,6 +664,7 @@ class CLdapConfig : implements ILdapConfig, public CInterface
            m_sdfieldname.append("aci");
        else if(m_serverType == OPEN_LDAP)
            m_sdfieldname.append("aci");
+


Hmm, not sure why, but removed it.

system/security/shared/caching.cpp

kenrowland · 2024-05-22T15:20:25Z

system/security/shared/caching.cpp

+            defaultScopesReadLock.clear();
+            WriteLockBlock defaultScopesWriteLock(m_defaultScopesRWLock);
+            defaultPermission = m_secMgr->queryDefaultPermission(user);
+            std::string userName(username);


The find, in my experience, has to construct an object of the key type. So, I left the username std::string variable, but cleaned it up a bit.

kenrowland · 2024-05-22T15:22:26Z

system/security/shared/caching.cpp

@@ -711,8 +741,16 @@ void CPermissionsCache::flush()
            delete (*ui).second;
        m_userCache.clear();
    }
+    if (m_useLegacyDefaultFileScopePermissionCache)
+    {
+        m_defaultPermission = SecAccess_None;


Yes, good catch.

kenrowland · 2024-05-22T15:27:44Z

system/security/shared/caching.cpp

+            if (m_secMgr)
+                m_defaultPermission = m_secMgr->queryDefaultPermission(user);
+            else
+                m_defaultPermission = SecAccess_None;


I changed this to SecAccess_Full. If no security manager then full access. I'm pretty sure the code essentially prevents this from being called if there is no security manager, so it may be moot, but correct none the less.

jakesmith

@kenrowland - looks good. Please squash for final scan.

kenrowland · 2024-05-24T13:37:41Z

@jakesmith squashed as requested. You mentioned final scan, so I re-requested review.

jakesmith

@kenrowland Looks good.

@ghalliday - should this go into 9.6 ?

ghalliday

@kenrowland a couple of questions/comments. No need to change to a critical section since tested as-is.

ghalliday · 2024-05-28T12:49:29Z

system/security/shared/caching.cpp

+    if (m_secMgr)
+    {
+        const std::string username(user.getName());
+        ReadLockBlock defaultScopesReadLock(m_defaultScopesRWLock);


I would have used a simpler critical section.

The R/W lock complicates this code (relock and potential for deadlock you have avoided).

This is unlikely to be heavily contended.

R/W lock is less efficient than a a critical section.

If it contended and needs a write then the other threads will need to wait anyway.
The R/W blocks are good for situations where parallel reads can continue and not have the write block them. (Even then it is only likely to be helpful when slow and heavily contended.)

From what I have seen, it appears that default file scopes are used a lot. Making this code as efficient as possible is probably the right thing to do. I agree that lock for read, release, lock for write and check again, is not optimal. Since you mentioned a critical section, I think the time to change it is now. I will make that change in the interest of keeping default scope checking efficient.

ghalliday · 2024-05-28T12:51:24Z

system/security/shared/caching.cpp

+            if (m_secMgr)
+                m_defaultPermission = m_secMgr->queryDefaultPermission(user);
+            else
+                m_defaultPermission = SecAccess_Full;


This is different from the code in 9.2.x/9.4.x. Is this deliberate? I suspect it is, but could do with an indication why.

A previous outdated comment indicates why it is set to Full. If there is no security manager, access should be full for all users and all scopes. However, technically as currently implemented, this code will never be called if there is no security manager. However, must still handle the case. I will add short comment on that line.

The case where the legacy cache is not used still returns SecAccess_None. It be clearer to test for m_secMgr at the start of the function and return SecAccess_Full.

ghalliday · 2024-05-28T12:52:40Z

system/security/shared/caching.cpp

+            else
+                m_defaultPermission = SecAccess_Full;
+
+            DBGLOG("Legacy default file scope permission set to %s(%d) for all users, based on User '%s'", getSecAccessFlagName(m_defaultPermission),


This looks like this tracing could be excessive.

That line will appear once whenever the cache is reset and when using the legacy code that has the error. I think it is important to leave in if the legacy option is chosen, then you can search back in the logs, if needed, to see what default scope permission is in effect.

ghalliday · 2024-05-28T12:54:09Z

system/security/shared/caching.cpp

+            {
+                defaultPermission = m_secMgr->queryDefaultPermission(user);
+                m_userDefaultFileScopePermissions.emplace(username, defaultPermission);
+                DBGLOG("Added user '%s' to default file scope permissions with access %s(%d)", username.c_str(), getSecAccessFlagName(defaultPermission),


Should this logging be protected with a flag e.g. traceLDAP?

As we move towards more secure permissions and removal of Authenticated users from default file scope permissions, I think this information is useful in helping lock down a cluster. In fact, I debated making it a PROGLOG so that the information on default scopes assigned to users is available. If you think a flag is more appropriate, I can add one. I think now is the time if we are going to do it.

This will be logged for the first lookup for each user every time the cache is cleared. Given the potential cost of logging that might add up. Keep as is for the moment, but review how much logging it is generating once this is being used on a live system.
(Simple support for trace flags is not yet available in esp, when it is it will be much simpler to add a guard.)

kenrowland · 2024-05-31T13:25:30Z

@jakesmith @ghalliday I converted the synchronization protection for the default scope permission map to use a mutex. This was based on the complexity of having to release the read lock, acquire the write lock, and recheck for a user's permission. A critical section was suggested, but the map needed to be protected when cleared after a cache timeout and when flushed.

jakesmith · 2024-05-31T15:43:28Z

A critical section was suggested, but the map needed to be protected when cleared after a cache timeout and when flushed.

curious, can you elaborate, why did this preclude using a CriticalSection ?

kenrowland · 2024-05-31T17:11:01Z

The semantics of a critical section is to protect a section of code from being reentered. The scope permission map needed to be protected in three different places, so a critical section did not seem appropriate. Instead a mutex protecting access to the data structure seemed appropriate.

ghalliday · 2024-06-03T09:49:11Z

The semantics of a critical section is to protect a section of code from being reentered. The scope permission map needed to be protected in three different places, so a critical section did not seem appropriate. Instead a mutex protecting access to the data structure seemed appropriate.

That is probably the original use for critical sections, but they provide very similar functionality to mutxes. I previously thought that the only difference was that Mutexes supported named inter-process mutexs. However it was a great question - because it made me look at the current implementation of mutexes.

They appear to be using condition variables in addition to the mutex. The code is ancient, but I am wondering why they are implemented like that.
So for interest I added them to the AtomicTimingStressTest. Here are the comparisons:

10:01:06.590708 1593187 UNK UNK CriticalSection,unsigned __int64@1/1 threads(1) 18ns/iteration lost(0)
10:01:06.691740 1593187 UNK UNK CriticalSection,unsigned __int64@1/1 threads(2) 50ns/iteration lost(0)
10:01:08.514772 1593187 UNK UNK CriticalSection,unsigned __int64@1/1 threads(32) 56ns/iteration lost(0)
10:01:12.619658 1593187 UNK UNK CriticalSection,unsigned __int64@1/1 threads(64) 64ns/iteration lost(0)
10:01:16.767319 1593187 UNK UNK CriticalSection,unsigned __int64@1/1 threads(65) 63ns/iteration lost(0)
10:01:24.923653 1593187 UNK UNK CriticalSection,unsigned __int64@1/1 threads(128) 63ns/iteration lost(0)

and

10:02:37.931778 1593187 UNK UNK Mutex,unsigned __int64@1/1 threads(1) 22ns/iteration lost(0)
10:02:39.980819 1593187 UNK UNK Mutex,unsigned __int64@1/1 threads(2) 1024ns/iteration lost(0)
10:04:20.179637 1593187 UNK UNK Mutex,unsigned __int64@1/1 threads(32) 3130ns/iteration lost(0)
10:07:42.497517 1593187 UNK UNK Mutex,unsigned __int64@1/1 threads(64) 3160ns/iteration lost(0)
10:11:06.593294 1593187 UNK UNK Mutex,unsigned __int64@1/1 threads(65) 3138ns/iteration lost(0)
10:17:39.614748 1593187 UNK UNK Mutex,unsigned __int64@1/1 threads(128) 3069ns/iteration lost(0)

i.e. they perform terribly. They perform even worse as amount of work performed in the mutex increases.

I will open a jira to switch Mutexes to use the critical section code!

ghalliday · 2024-06-03T10:57:12Z

@kenrowland having taken a closer look at the mutex implementation/performance this code should be switched to use a CriticalSection instead of a Mutex. (I agree we should really use Mutex in many places we use CriticalSection.)

Longer term we will rewrite the Mutex class so it is efficient.

kenrowland · 2024-06-03T20:16:21Z

@ghalliday Switched to use a critical section

jakesmith

@kenrowland - 1 question, but looks fine.

jakesmith · 2024-06-04T08:35:15Z

system/security/shared/caching.cpp

@@ -549,7 +549,8 @@ inline void CPermissionsCache::removeAllManagedFileScopes()

    etc. Until full scope path checked, or no read permissions hit on ancestor scope.
 */
-static CriticalSection msCacheSyncCS;//for managed scopes cache syncronization
+static CriticalSection msCacheSyncCS;//for managed scopes cache synchronization
+static CriticalSection syncDefaultScopePermissions;//for cached default file scope permissions


curios why this (and msCacheSyncCS) aren't members of CPermissionCache? (

The protection for managed scopes was implemented that way, so I kept the same paradigm. I'd have to look deeper, but perhaps it is related to the option to share the cache across multiple instances of the security manager (as is the case with an ESP with multiple services loaded).

kenrowland · 2024-06-04T13:16:00Z

@ghalliday Any final comments?

kenrowland · 2024-06-05T15:39:00Z

@ghalliday please merge if no further comments

ghalliday

@kenrowland I'm not sure. I think best to merge as is, and have a follow on PR that moves the code out of the CriticalSection

ghalliday · 2024-06-07T11:35:11Z

system/security/shared/caching.cpp


+    SecAccessFlags defaultPermission = SecAccess_None;
+    CriticalBlock defaultScopePermissionBlock(syncDefaultScopePermissions);
+    const std::string username(user.getName());


minor: This code doesn't need to be inside the critical section. Minimising the code with a mutex helps reduce contention. I will merge as-is.

ghalliday · 2024-06-07T11:36:54Z

system/security/shared/caching.cpp

+    {
+        defaultPermission = m_secMgr->queryDefaultPermission(user);
+        m_userDefaultFileScopePermissions.emplace(username, defaultPermission);
+        DBGLOG("Added user '%s' to default file scope permissions with access %s(%d)", username.c_str(), getSecAccessFlagName(defaultPermission),


It is best to avoid any logging inside a critical section - because it could be blocked on io. Better to set a flag and trace once the critical section is left.

ghalliday · 2024-06-11T13:32:43Z

@kenrowland I have created a Jira to further clean up the code.
Should this target an earlier version (with the option disabled by default) and the default on in 9.8.x?

kenrowland · 2024-06-11T18:36:33Z

@ghalliday

First, do you want the logging code moved as part of a group of PRs to be merged at once?

Second, IMO, this fix should target both 9.6 and 9.8, both with the option to use the old code disabled. It fixes security holes. Plus any dependence on the older "way" is not predictable.

I sent a separate email with information about how we can handle the fix.

kenrowland · 2024-06-12T12:57:34Z

@ghalliday request for merging may have gotten lost in the recent comments. Once merged, making the changes for the follow up Jira will be much easier.

ghalliday · 2024-06-13T08:29:55Z

ok. @kenrowland please can you retarget this to 9.6.x with the option defaulting to legacy, and then when that is merged create a separate pr enabling it targetting master.

kenrowland · 2024-06-13T13:05:48Z

@ghalliday Retargeted as requested

… uses it for future requests Added a per user default file scope permission cache Signed-Off-By: Kenneth Rowland [email protected]

kenrowland changed the title ~~HPCC-31761 LDAP caches first user's default file scope permission and…~~ HPCC-31761 LDAP only uses first cached default file scope permission May 14, 2024

kenrowland requested review from ghalliday and jakesmith May 14, 2024 17:45

jakesmith requested changes May 15, 2024

View reviewed changes

kenrowland requested a review from jakesmith May 21, 2024 15:06

jakesmith reviewed May 22, 2024

View reviewed changes

kenrowland commented May 22, 2024

View reviewed changes

kenrowland requested a review from jakesmith May 22, 2024 15:29

jakesmith reviewed May 24, 2024

View reviewed changes

kenrowland force-pushed the HPCC-31761 branch from 88deb76 to 4c1cd70 Compare May 24, 2024 13:35

kenrowland requested a review from jakesmith May 24, 2024 13:37

jakesmith approved these changes May 24, 2024

View reviewed changes

ghalliday reviewed May 28, 2024

View reviewed changes

kenrowland requested review from ghalliday and jakesmith May 31, 2024 13:25

jakesmith approved these changes Jun 4, 2024

View reviewed changes

kenrowland force-pushed the HPCC-31761 branch from f07f87e to 40cf38a Compare June 5, 2024 15:37

ghalliday approved these changes Jun 7, 2024

View reviewed changes

kenrowland changed the base branch from master to candidate-9.6.x June 13, 2024 12:54

kenrowland changed the base branch from candidate-9.6.x to master June 13, 2024 12:55

kenrowland force-pushed the HPCC-31761 branch from e65b89c to 15e00a2 Compare June 13, 2024 12:58

kenrowland changed the base branch from master to candidate-9.6.x June 13, 2024 12:59

HPCC-31761 LDAP caches first user's default file scope permission and…

c41e168

… uses it for future requests Added a per user default file scope permission cache Signed-Off-By: Kenneth Rowland [email protected]

kenrowland force-pushed the HPCC-31761 branch from ff0ad96 to c41e168 Compare June 18, 2024 13:56

ghalliday merged commit de2efba into hpcc-systems:candidate-9.6.x Jun 19, 2024
49 checks passed

HPCC-31761 LDAP only uses first cached default file scope permission #18651

HPCC-31761 LDAP only uses first cached default file scope permission #18651

Conversation

kenrowland commented May 14, 2024 • edited Loading

Type of change:

Checklist:

Smoketest:

Testing:

github-actions bot commented May 14, 2024

kenrowland commented May 14, 2024

jakesmith left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kenrowland commented May 15, 2024

jakesmith commented May 16, 2024

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

jakesmith left a comment

Choose a reason for hiding this comment

kenrowland commented May 24, 2024

jakesmith left a comment

Choose a reason for hiding this comment

ghalliday 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

kenrowland May 29, 2024 • 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

kenrowland commented May 31, 2024

jakesmith commented May 31, 2024

kenrowland commented May 31, 2024

ghalliday commented Jun 3, 2024

ghalliday commented Jun 3, 2024

kenrowland commented Jun 3, 2024

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

kenrowland commented Jun 4, 2024

kenrowland commented Jun 5, 2024

ghalliday left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghalliday commented Jun 11, 2024

kenrowland commented Jun 11, 2024 • edited Loading

kenrowland commented Jun 12, 2024

ghalliday commented Jun 13, 2024

kenrowland commented Jun 13, 2024

kenrowland commented May 14, 2024 •

edited

Loading

kenrowland May 29, 2024 •

edited

Loading

kenrowland commented Jun 11, 2024 •

edited

Loading