Skip to content

Commit

Permalink
ldapccl: support partial ldap groups mapping on authz
Browse files Browse the repository at this point in the history
fixes cockroachdb#133779
Epic CRDB-33829

Currently, an LDAP user may be configured to be a member of multiple LDAP
groups, and when we retrieve the group roles for the user during LDAP authZ we
try to find corresponding roles for all the groups being synced and fail the
roles grant operation if any of the group roles do not exist on CRDB. This is a
problem as not all groups are desired to have corresponding CRDB roles and in
such a case a partial roles grant should take place.

Release note(security):  This fix will add support for partial roles from ldap
synced group to be mapped to crdb roles and ensure appropriate erroring for
undesired behavior.
  • Loading branch information
souravcrl committed Nov 18, 2024
1 parent 3397bc5 commit 35f723e
Show file tree
Hide file tree
Showing 3 changed files with 58 additions and 5 deletions.
4 changes: 2 additions & 2 deletions pkg/ccl/ldapccl/authorization_ldap_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -237,8 +237,8 @@ func TestLDAPRolesAreGranted(t *testing.T) {
require.True(t, foundSession)

// Add a group that does not have a corresponding CRDB role, and verify that
// the user cannot login.
// the user can still login via partial groups mapping.
mockLDAP.SetGroups("cn=foo", []string{"cn=foo_parent_2", "cn=nonexistent_role"})
_, err = fooDB.Conn(ctx)
require.ErrorContains(t, err, "LDAP authorization: error assigning roles to user foo: EnsureUserOnlyBelongsToRoles-grant: role/user \"nonexistent_role\" does not exist")
require.NoError(t, err)
}
51 changes: 51 additions & 0 deletions pkg/ccl/testccl/authccl/testdata/ldap
Original file line number Diff line number Diff line change
Expand Up @@ -329,3 +329,54 @@ SELECT pg_has_role('ldap_user', 'ldap.user.parent.2', 'MEMBER')
false

subtest end

subtest partial_ldap_groups_map

set_hba
host all ldap_user 127.0.0.1/32 ldap ldapserver=localhost ldapport=636 ldapbasedn="O=security org,DC=localhost" ldapbinddn="CN=service_account,O=security org,DC=localhost" ldapbindpasswd=ldap_pwd ldapsearchattribute=sAMAccountName ldapsearchfilter="(memberOf=*)" "ldapgrouplistfilter=(cn=*)"
----
# Active authentication configuration on this node:
# Original configuration:
# loopback all all all trust # built-in CockroachDB default
# host all root all cert-password # CockroachDB mandatory rule
# host all ldap_user 127.0.0.1/32 ldap ldapserver=localhost ldapport=636 ldapbasedn="O=security org,DC=localhost" ldapbinddn="CN=service_account,O=security org,DC=localhost" ldapbindpasswd=ldap_pwd ldapsearchattribute=sAMAccountName ldapsearchfilter="(memberOf=*)" "ldapgrouplistfilter=(cn=*)"
#
# Interpreted configuration:
# TYPE DATABASE USER ADDRESS METHOD OPTIONS
loopback all all all trust
host all root all cert-password
host all ldap_user 127.0.0.1/32 ldap ldapserver=localhost ldapport=636 "ldapbasedn=O=security org,DC=localhost" "ldapbinddn=CN=service_account,O=security org,DC=localhost" ldapbindpasswd=ldap_pwd ldapsearchattribute=sAMAccountName "ldapsearchfilter=(memberOf=*)" "ldapgrouplistfilter=(cn=*)"

sql
CREATE ROLE "ldap-parent-synced";
----
ok

ldap_mock set_groups=(ldap_user,cn=ldap-parent-synced,cn=ldap-parent-unsynced)
----

connect user=ldap_user password="ldap_pwd"
----
ok defaultdb

query_row
SELECT pg_has_role('ldap_user', 'ldap-parent-synced', 'MEMBER')
----
true

query_row
SELECT 1 FROM pg_roles WHERE rolname='ldap-parent-synced'
----
1

query_row
SELECT 1 FROM pg_roles WHERE rolname='ldap-parent-unsynced'
----
ERROR: no rows in result set

query_row
SELECT pg_has_role('ldap_user', 'ldap-parent-unsynced', 'MEMBER')
----
ERROR: pg_has_role(): role 'ldap-parent-unsynced' does not exist (SQLSTATE 42704)

subtest end
8 changes: 5 additions & 3 deletions pkg/sql/authorization.go
Original file line number Diff line number Diff line change
Expand Up @@ -758,10 +758,12 @@ func EnsureUserOnlyBelongsToRoles(
grantStmt := strings.Builder{}
grantStmt.WriteString("GRANT ")
for i, role := range rolesToGrant {
if i > 0 {
grantStmt.WriteString(", ")
if roleExists, _ := RoleExists(ctx, txn, role); roleExists {
if i > 0 {
grantStmt.WriteString(", ")
}
grantStmt.WriteString(role.SQLIdentifier())
}
grantStmt.WriteString(role.SQLIdentifier())
}
grantStmt.WriteString(" TO ")
grantStmt.WriteString(user.SQLIdentifier())
Expand Down

0 comments on commit 35f723e

Please sign in to comment.