Revert to use full DN instead of CN for autorization.

I do not fully trust our users to do the right thing and avoid
to have the same group names in different parts of the organisation.

If I rely on CN, it means a duplicate name could grant access
incorrectly. This is a problem, and I do not want this refactor
to be causing a security impact.

Instead of fully rewriting the code OR ensuring that the org
have no duplicate CN names, I make sure the regexp is matching
on the full DN.

I expect this should be cleaned in the future, as Kubernetes itself
does not have this notion.

internal/ldap/client.go

@@ -64,29 +64,30 @@ func (c *LDAPClient) AuthZ(user *types.User) (*types.User, error) {
 		return &types.User{}, fmt.Errorf("cannot get memberships for user %s in LDAP", user.Username)
 	}
 
-	user.Groups = ldapMemberships.toGroupNames()
-
-	// To be removed in final stage
-	user.IsAdmin = len(ldapMemberships.AdminAccess) > 0
-	user.IsAppOps = (len(ldapMemberships.AppOpsAccess) > 0) || (len(ldapMemberships.CustomerOpsAccess) > 0)
-	user.IsCloudOps = len(ldapMemberships.CloudOpsAccess) > 0
-	user.IsViewer = len(ldapMemberships.ViewerAccess) > 0
-	user.IsService = len(ldapMemberships.ServiceAccess) > 0
-	user.ProjectAccesses = ldapMemberships.toProjectNames()
-
 	// We now have all the user details (including special groups).
 	// we can check if the user has the basic right to get a token.
 	// If they do, it means we trust the user, and we'll rely on the authorization db of each asset
 	// (dex+kubi plugin+argocm for argcd, kubernetes+kubiwebhook+rolebindings for kube api, promote...)
 
-	allowedInCluster, err := isAllowed(user, c.AllowedGroupRegexps)
+	allowedInCluster, err := ldapMemberships.isUserAllowedOnCluster(c.AllowedGroupRegexps)
 	if err != nil {
 		return nil, fmt.Errorf("user is not autorised in this cluster due to an regex error %v, %v", user.UserDN, err)
 	}
 	if !allowedInCluster {
 		return nil, fmt.Errorf("user is not allowed in this cluster %v", user.UserDN)
 	}
 
+	// now create the user data accordingly.
+	user.Groups = ldapMemberships.toGroupNames()
+
+	// To be removed in final stage
+	user.IsAdmin = len(ldapMemberships.AdminAccess) > 0
+	user.IsAppOps = (len(ldapMemberships.AppOpsAccess) > 0) || (len(ldapMemberships.CustomerOpsAccess) > 0)
+	user.IsCloudOps = len(ldapMemberships.CloudOpsAccess) > 0
+	user.IsViewer = len(ldapMemberships.ViewerAccess) > 0
+	user.IsService = len(ldapMemberships.ServiceAccess) > 0
+	user.ProjectAccesses = ldapMemberships.toProjectNames()
+
 	return user, nil
 }
 

internal/ldap/membership.go

@@ -2,6 +2,7 @@ package ldap
 
 import (
 	"fmt"
+	"regexp"
 	"strings"
 
 	"gopkg.in/ldap.v2"
@@ -105,3 +106,28 @@ func (m *LDAPMemberships) toProjectNames() []string {
 	}
 	return groups
 }
+
+func (m *LDAPMemberships) isUserAllowedOnCluster(regexpPatterns []string) (bool, error) {
+	var allowedInCluster bool
+	// To get access, the user needs at least one of the following:
+	// - Be granted access to at least one project
+	// - Have special rights
+	// - Have their group listed in an extra group allowlist
+	if len(m.AdminAccess) > 0 || len(m.AppOpsAccess) > 0 || len(m.CustomerOpsAccess) > 0 || len(m.ViewerAccess) > 0 || len(m.ServiceAccess) > 0 || len(m.CloudOpsAccess) > 0 || len(m.ClusterGroupsAccess) > 0 {
+		allowedInCluster = true
+	} else { // else is not mandatory it's just an optimisation: don't browse all groups if we already know the user has the rights to the cluster
+		for _, groupName := range m.NonSpecificGroups {
+			for _, pattern := range regexpPatterns {
+				matched, err := regexp.MatchString(pattern, strings.ToUpper(groupName.DN)) // we match on full DN rather than CN because nobody prevents the ppl in the different entities to create a CN identical as the one used for adminGroup. This is purely out of precaution. In the future, we might want to change the regexp to match only the cn of the groups if we have the guarantee the users will not create groups that are duplicate.
+				if err != nil {
+					return false, fmt.Errorf("error matching pattern %v: %v", pattern, err)
+				}
+				if matched {
+					allowedInCluster = true
+					break
+				}
+			}
+		}
+	}
+	return allowedInCluster, nil
+}

internal/ldap/membership_test.go

@@ -91,3 +91,95 @@ func TestToGroupNames(t *testing.T) {
 		})
 	}
 }
+func TestIsUserAllowedOnCluster(t *testing.T) {
+	tests := []struct {
+		name           string
+		members        LDAPMemberships
+		regexpPatterns []string
+		expected       bool
+		expectError    bool
+	}{
+		{
+			name: "User with admin access",
+			members: LDAPMemberships{
+				AdminAccess: []*ldap.Entry{
+					{DN: "cn=admin1", Attributes: []*ldap.EntryAttribute{{Name: "cn", Values: []string{"admin1"}}}},
+				},
+			},
+			regexpPatterns: []string{},
+			expected:       true,
+			expectError:    false,
+		},
+		{
+			name: "User with no special access but matching group pattern",
+			members: LDAPMemberships{
+				NonSpecificGroups: []*ldap.Entry{
+					{DN: "cn=group1,OU=CAGIP,O=CA", Attributes: []*ldap.EntryAttribute{{Name: "cn", Values: []string{"group1"}}}},
+				},
+			},
+			regexpPatterns: []string{"CN=GROUP1,OU=CAGIP,O=CA"},
+			expected:       true,
+			expectError:    false,
+		},
+		{
+			name: "User with no special access but matching generic group pattern",
+			members: LDAPMemberships{
+				NonSpecificGroups: []*ldap.Entry{
+					{DN: "cn=group1,OU=CAGIP,O=CA", Attributes: []*ldap.EntryAttribute{{Name: "cn", Values: []string{"group1"}}}},
+				},
+			},
+			regexpPatterns: []string{"CN=.*,OU=CAGIP,O=CA"},
+			expected:       true,
+			expectError:    false,
+		},
+		{
+			name: "User with no access and no matching group pattern",
+			members: LDAPMemberships{
+				NonSpecificGroups: []*ldap.Entry{
+					{DN: "cn=group1", Attributes: []*ldap.EntryAttribute{{Name: "cn", Values: []string{"group1"}}}},
+				},
+			},
+			regexpPatterns: []string{"CN=group2"},
+			expected:       false,
+			expectError:    false,
+		},
+		{
+			name: "User with no access and invalid regex pattern",
+			members: LDAPMemberships{
+				NonSpecificGroups: []*ldap.Entry{
+					{DN: "cn=group1", Attributes: []*ldap.EntryAttribute{{Name: "cn", Values: []string{"group1"}}}},
+				},
+			},
+			regexpPatterns: []string{"["},
+			expected:       false,
+			expectError:    true,
+		},
+		{
+			name: "User with multiple access types",
+			members: LDAPMemberships{
+				AdminAccess: []*ldap.Entry{
+					{DN: "cn=admin1", Attributes: []*ldap.EntryAttribute{{Name: "cn", Values: []string{"admin1"}}}},
+				},
+				AppOpsAccess: []*ldap.Entry{
+					{DN: "cn=appops1", Attributes: []*ldap.EntryAttribute{{Name: "cn", Values: []string{"appops1"}}}},
+				},
+			},
+			regexpPatterns: []string{},
+			expected:       true,
+			expectError:    false,
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			got, err := tt.members.isUserAllowedOnCluster(tt.regexpPatterns)
+			if (err != nil) != tt.expectError {
+				t.Errorf("isUserAllowedOnCluster() error = %v, expectError %v", err, tt.expectError)
+				return
+			}
+			if got != tt.expected {
+				t.Errorf("isUserAllowedOnCluster() = %v, want %v", got, tt.expected)
+			}
+		})
+	}
+}