This is an automated email from the ASF dual-hosted git repository.
wilfred-s pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/yunikorn-core.git
The following commit(s) were added to refs/heads/master by this push:
new 2cb0c245 [YUNIKORN-3432] LDAP resolver cleanup (#1139)
2cb0c245 is described below
commit 2cb0c245adf1be4014fa1dc0e93278def35ee7cb
Author: Wilfred Spiegelenburg <[email protected]>
AuthorDate: Fri Aug 28 02:27:14 2026 +1000
[YUNIKORN-3432] LDAP resolver cleanup (#1139)
The ConvertUGI call does a group resolution before checking the user is
acceptable based on the user regular expression. This means that the
resolver gets called for users that will get rejected later wasting
resources.
Fix a panic processing the memberOf attribute.
Fix locking calls to not use the instance var, and fix the Stop
implementation to remove small race condition around stopped flag which
could allow stopping twice.
Fix logging and comments.
Closes: #1139
Signed-off-by: Wilfred Spiegelenburg <[email protected]>
---
pkg/common/security/usergroup.go | 49 +++++++++++----------
pkg/common/security/usergroup_ldap_resolver.go | 44 +++++++++++--------
.../security/usergroup_ldap_resolver_test.go | 51 ++++++++++++++++++----
3 files changed, 95 insertions(+), 49 deletions(-)
diff --git a/pkg/common/security/usergroup.go b/pkg/common/security/usergroup.go
index f05b6766..e03ffecf 100644
--- a/pkg/common/security/usergroup.go
+++ b/pkg/common/security/usergroup.go
@@ -117,16 +117,17 @@ func (c *UserGroupCache) GetResolverType() string {
// run the cleanup in a separate routine
func (c *UserGroupCache) run() {
- log.Log(log.Security).Info("Starting user/group cache cleaner")
+ log.Log(log.Security).Info("Starting UserGroupCache cleaner")
for {
select {
case <-c.stop:
+ log.Log(log.Security).Info("UserGroupCache cleaner
exiting")
return
case <-time.After(c.interval):
runStart := time.Now()
c.cleanUpCache()
log.Log(log.Security).Debug("time consumed cleaning the
UserGroupCache",
- zap.Stringer("duration", time.Since(runStart)))
+ zap.Duration("duration", time.Since(runStart)))
}
}
}
@@ -136,8 +137,8 @@ func (c *UserGroupCache) cleanUpCache() {
oldest := time.Now().Unix() - poscache
oldestFailed := time.Now().Unix() - negcache
// clean up the cache so we do not grow out of bounds
- instance.lock.Lock()
- defer instance.lock.Unlock()
+ c.lock.Lock()
+ defer c.lock.Unlock()
// walk over the entries in the map and delete the expired ones,
cleanup based on the resolved time.
// Negative cached entries will expire quicker
for key, val := range c.ugs {
@@ -150,8 +151,8 @@ func (c *UserGroupCache) cleanUpCache() {
// resetCache clears the cached content, test use only
func (c *UserGroupCache) resetCache() {
log.Log(log.Security).Debug("UserGroupCache reset")
- instance.lock.Lock()
- defer instance.lock.Unlock()
+ c.lock.Lock()
+ defer c.lock.Unlock()
c.ugs = make(map[string]*UserGroup)
}
@@ -168,6 +169,13 @@ func (c *UserGroupCache) ConvertUGI(ugi
*si.UserGroupInformation, force bool) (U
return UserGroup{}, fmt.Errorf("empty user cannot
resolve")
}
}
+
+ // make sure the user is acceptable before doing any more work
+ // if this fails the app is rejected anyway, loading groups etc is
wasting cycles
+ if !configs.UserRegExp.MatchString(ugi.User) {
+ return UserGroup{}, fmt.Errorf("invalid username, it contains
invalid characters")
+ }
+
// try to resolve the user if group info is empty otherwise we just
convert
if len(ugi.Groups) == 0 {
ug, err := c.GetUserGroup(ugi.User)
@@ -178,10 +186,6 @@ func (c *UserGroupCache) ConvertUGI(ugi
*si.UserGroupInformation, force bool) (U
}
}
- if !configs.UserRegExp.MatchString(ugi.User) {
- return UserGroup{}, fmt.Errorf("invalid username, it contains
invalid characters")
- }
-
// If groups are already present we should just convert
newUG := UserGroup{User: ugi.User}
newUG.Groups = append(newUG.Groups, ugi.Groups...)
@@ -192,7 +196,7 @@ func (c *UserGroupCache) ConvertUGI(ugi
*si.UserGroupInformation, force bool) (U
return newUG, nil
}
-// GetUserGroup get the user group information for a singe user. An error will
still return a UserGroup.
+// GetUserGroup get the user group information for a single user. An error
will still return a UserGroup.
// The Failed flag in the object will be set to true for any failures.
// The information is cached, negatively and positively.
func (c *UserGroupCache) GetUserGroup(userName string) (UserGroup, error) {
@@ -244,8 +248,8 @@ func (c *UserGroupCache) GetUserGroup(userName string)
(UserGroup, error) {
// add it to the cache, even if we fail negative cache is also good to
know
c.lock.Lock()
- defer c.lock.Unlock()
c.ugs[userName] = ug
+ c.lock.Unlock()
return *ug, err
}
@@ -253,19 +257,18 @@ func (c *UserGroupCache) GetUserGroup(userName string)
(UserGroup, error) {
func (c *UserGroupCache) Stop() {
// make sure that in case of multiple partitions, we call Stop() only
once (the instance is shared)
// see ClusterContext.Stop()
- if !stopped.Load() {
- log.Log(log.Security).Info("Stopping UserGroupCache background
cleanup")
- close(c.stop)
- // Clear the cache before resetting the instance
- c.lock.Lock()
- c.ugs = make(map[string]*UserGroup)
- c.lock.Unlock()
- once = &sync.Once{} // re-init so that GetUserGroupCache() can
create a new instance again
- instance = nil
- stopped.Store(true)
+ if !stopped.CompareAndSwap(false, true) {
+ log.Log(log.Security).Info("UserGroupCache already stopped")
return
}
- log.Log(log.Security).Info("UserGroupCache already stopped")
+ log.Log(log.Security).Info("Stopping UserGroupCache background cleanup")
+ close(c.stop)
+ // Clear the cache before resetting the instance
+ c.lock.Lock()
+ c.ugs = make(map[string]*UserGroup)
+ once = &sync.Once{} // re-init so that GetUserGroupCache() can create a
new instance again
+ instance = nil // should not be needed as any Get from now on will
overwrite the instance
+ c.lock.Unlock()
}
// resolveGroups resolves the groups for the user if the user exists and
updates the cache.
diff --git a/pkg/common/security/usergroup_ldap_resolver.go
b/pkg/common/security/usergroup_ldap_resolver.go
index eea68b44..cf2d692c 100644
--- a/pkg/common/security/usergroup_ldap_resolver.go
+++ b/pkg/common/security/usergroup_ldap_resolver.go
@@ -232,7 +232,7 @@ func GetLdapAccess() LdapAccess {
return ldapAccessImpl{}
}
-// LDAPResolverConfig holds the configuration for the LDAP resolver
+// LdapConfig holds the configuration for the LDAP resolver
type LdapConfig struct {
Host string
Port int
@@ -249,14 +249,10 @@ type LdapConfig struct {
func GetUserGroupCacheLdap(reader ConfigReader, access LdapAccess)
*UserGroupCache {
config, err := reader.ReadLdapConfig()
if err != nil {
- // Log a FATAL level message - this is very prominent and will
typically cause the application to exit
+ // Log a FATAL level message - this is very prominent and will
cause the application to exit
log.Log(log.Security).Fatal("LDAP configuration not found or
invalid. No secrets were loaded from the secrets directory.",
zap.String("secretsPath", common.LdapMountPath),
zap.String("resolution", "Ensure LDAP secrets are
properly mounted and accessible"))
-
- // If the Fatal log doesn't cause an exit (depends on logger
configuration),
- // we could also panic here to ensure the application stops
- panic("LDAP configuration not found or invalid")
}
ldapLookup := &LdapLookup{
@@ -274,7 +270,7 @@ func GetUserGroupCacheLdap(reader ConfigReader, access
LdapAccess) *UserGroupCac
}
}
-// Default linux behaviour: a user is member of the primary group with the
same name
+// LdapLookupUser mimics default linux behaviour: a user is member of the
primary group with the same name
func (LdapLookup) LdapLookupUser(userName string) (*user.User, error) {
log.Log(log.Security).Debug("Performing LDAP user lookup",
zap.String("username", userName),
@@ -286,14 +282,17 @@ func (LdapLookup) LdapLookupUser(userName string)
(*user.User, error) {
}, nil
}
+// LdapLookupGroupID mimics a linux group with a name and ID that have the
same value
func (LdapLookup) LdapLookupGroupID(gid string) (*user.Group, error) {
log.Log(log.Security).Debug("Looking up LDAP group ID",
zap.String("groupID", gid))
- group := user.Group{Gid: gid}
- group.Name = gid
- return &group, nil
+ return &user.Group{
+ Gid: gid,
+ Name: gid,
+ }, nil
}
+// LDAPLookupGroupIds load the group memberships for the user from the LDAP
server
func (lu LdapLookup) LDAPLookupGroupIds(osUser *user.User) ([]string, error) {
sr, err := ldapSearch(lu.access, lu.config, osUser.Username)
if err != nil {
@@ -309,10 +308,18 @@ func (lu LdapLookup) LDAPLookupGroupIds(osUser
*user.User) ([]string, error) {
log.Log(log.Security).Debug("LDAP 'memberOf' attributes for
user",
zap.String("user", osUser.Username),
zap.Strings("attributes", attr))
- for i := range attr {
- s := strings.Split(attr[i], ",")
- newgroup := strings.Split(s[0], "CN=")
- groups = append(groups, newgroup[1])
+ // range of the values in the memberOf attribute handles 0 or
more entries
+ for _, v := range attr {
+ // split the DN into parts, first part is the group name
+ // even if value has only one part s will be valid and
have at least 1 entry
+ s := strings.Split(v, ",")
+ // split on the equal sign that defines this part of
the DN, do not really care what the attribute name is
+ newgroup := strings.Split(s[0], "=")
+ if len(newgroup) != 2 {
+ // some illegal construct in the DN part as it
did not have an equal sign
+ continue
+ }
+ groups = append(groups, strings.TrimSpace(newgroup[1]))
}
}
return groups, nil
@@ -321,11 +328,9 @@ func (lu LdapLookup) LDAPLookupGroupIds(osUser *user.User)
([]string, error) {
// ldapSearch performs an LDAP search for the specified username
// This replaces the old LDAPConn_Bind function with a more testable approach
func ldapSearch(ldapAccess LdapAccess, ldapConf LdapConfig, userName string)
(*ldap.SearchResult, error) {
- var ldapUri string
+ ldapUri := "ldap"
if ldapConf.useSsl {
ldapUri = "ldaps"
- } else {
- ldapUri = "ldap"
}
ldapaddr := fmt.Sprintf("%s://%s:%d", ldapUri, ldapConf.Host,
ldapConf.Port)
@@ -354,7 +359,8 @@ func ldapSearch(ldapAccess LdapAccess, ldapConf LdapConfig,
userName string) (*l
return nil, err
}
- filter := fmt.Sprintf(ldapConf.Filter, userName)
+ // defence in depth: username is limited in what it may contain via the
regexp, still escape the filter to be safe.
+ filter := fmt.Sprintf(ldapConf.Filter, ldap.EscapeFilter(userName))
log.Log(log.Security).Debug("Executing LDAP search",
zap.String("baseDN", ldapConf.BaseDN),
zap.String("filter", filter),
@@ -376,6 +382,8 @@ func ldapSearch(ldapAccess LdapAccess, ldapConf LdapConfig,
userName string) (*l
return nil, err
}
+ // a search that does not find the entry returns no error but a search
result with no entries
+ // not found is treated the same as no group memberships during
processing.
log.Log(log.Security).Debug("LDAP search completed successfully",
zap.String("username", userName),
zap.Int("entriesFound", len(sr.Entries)))
diff --git a/pkg/common/security/usergroup_ldap_resolver_test.go
b/pkg/common/security/usergroup_ldap_resolver_test.go
index 17f28cf8..a9995ad8 100644
--- a/pkg/common/security/usergroup_ldap_resolver_test.go
+++ b/pkg/common/security/usergroup_ldap_resolver_test.go
@@ -30,6 +30,7 @@ import (
"time"
"github.com/go-ldap/ldap/v3"
+ "golang.org/x/exp/slices"
"gotest.tools/v3/assert"
"github.com/apache/yunikorn-core/pkg/common"
@@ -321,24 +322,58 @@ func TestLDAPLookupGroupIds(t *testing.T) {
{
Attributes: []*ldap.EntryAttribute{
{
- Name: "memberOf",
- Values:
[]string{"CN=group1,OU=groups,DC=example,DC=com",
"CN=group2,OU=groups,DC=example,DC=com"},
+ Name: "memberOf",
},
},
},
},
}
-
u := &user.User{Username: "testuser"}
lu := &LdapLookup{
- access: newMockLdapAccess(mockResult, nil),
config: LdapConfig{},
}
- groups, err := lu.LDAPLookupGroupIds(u)
- assert.NilError(t, err)
- assert.Assert(t, strings.Contains(strings.Join(groups, ","), "group1"))
- assert.Assert(t, strings.Contains(strings.Join(groups, ","), "group2"))
+ tests := []struct {
+ name string
+ memberOf []string
+ groups []string
+ }{
+ {"empty member", []string{}, []string{}},
+ {"nil member", nil, []string{}},
+ {"single", []string{"CN=group1,OU=groups,DC=example,DC=com"},
[]string{"group1"}},
+ {"non CN", []string{"uid=1234,OU=groups,DC=example,DC=com"},
[]string{"1234"}},
+ {"lowercase CN",
[]string{"cn=group1,OU=groups,DC=example,DC=com"}, []string{"group1"}},
+ {"multival", []string{"CN=group1,OU=groups,DC=example,DC=com",
"CN=group2,OU=groups,DC=example,DC=com"}, []string{"group1", "group2"}},
+ {"mixed", []string{"CN=group1,OU=groups,DC=example,DC=com",
"cn=group2,OU=groups,DC=example,DC=com"}, []string{"group1", "group2"}},
+ }
+ for _, tt := range tests {
+ t.Run(tt.name, func(t *testing.T) {
+ mockResult.Entries[0].Attributes[0].Values = tt.memberOf
+ lu.access = newMockLdapAccess(mockResult, nil)
+ groups, err := lu.LDAPLookupGroupIds(u)
+ assert.NilError(t, err)
+ assert.Assert(t, slices.Equal(groups, tt.groups),
"group slices are not equal")
+ })
+ }
+
+ // no attributes returned on a successful search
+ t.Run("no attributes",
+ func(t *testing.T) {
+ mockResult.Entries[0].Attributes = nil
+ lu.access = newMockLdapAccess(mockResult, nil)
+ groups, err := lu.LDAPLookupGroupIds(u)
+ assert.NilError(t, err)
+ assert.Assert(t, len(groups) == 0)
+ })
+
+ // no entries found: Entries is always initialised, never nil
+ t.Run("no entries",
+ func(t *testing.T) {
+ lu.access =
newMockLdapAccess(&ldap.SearchResult{Entries: make([]*ldap.Entry, 0)}, nil)
+ groups, err := lu.LDAPLookupGroupIds(u)
+ assert.NilError(t, err)
+ assert.Assert(t, len(groups) == 0)
+ })
}
func TestLDAPLookupGroupIdsError(t *testing.T) {
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
