[pve-devel] [PATCH access-control 7/9] Auth/LDAP: add get_{users, groups} subs for syncing

[Dominik Csapak]

On 3/9/20 11:44 AM, Fabian Grünbichler wrote:
> On March 6, 2020 11:05 am, Dominik Csapak wrote:
>> this adds the subs which actually query the LDAP for users/groups
>> and returns the value in format which makes it easy to insert
>> in our parsed user.cfg
>>
>> when we find a user/groupname which cannot be in our config,
>> we warn the verification error
>>
>> for groups, we append "-$realm" to the groupname, to lower the chance of
>> accidental overwriting of existing groups (this will be documented
>> in the api call since it technically does not prevent overwriting, just
>> makes it more unlikely)
>>
>> Signed-off-by: Dominik Csapak <d.csapak at proxmox.com>
>> ---
>>   PVE/Auth/LDAP.pm | 135 +++++++++++++++++++++++++++++++++++++++++++++++
>>   1 file changed, 135 insertions(+)
>>
>> diff --git a/PVE/Auth/LDAP.pm b/PVE/Auth/LDAP.pm
>> index 6047dfb..45e4299 100755
>> --- a/PVE/Auth/LDAP.pm
>> +++ b/PVE/Auth/LDAP.pm
>> @@ -189,6 +189,141 @@ sub connect_and_bind {
>>       return $ldap;
>>   }
>>   
>> +# returns:
>> +# {
>> +#     'username at realm' => {
>> +# 	'attr1' => 'value1',
>> +# 	'attr2' => 'value2',
>> +# 	...
>> +#     },
>> +#     ...
>> +# }
>> +#
>> +# or in list context:
>> +# (
>> +#     {
>> +# 	'username at realm' => {
>> +# 	    'attr1' => 'value1',
>> +# 	    'attr2' => 'value2',
>> +# 	    ...
>> +# 	},
>> +# 	...
>> +#     },
>> +#     {
>> +# 	'uid=username,dc=....' => 'username at realm',
>> +# 	...
>> +#     }
>> +# )
>> +# the map of dn->username is needed for group membership sync
>> +sub get_users {
>> +    my ($class, $config, $realm) = @_;
>> +
>> +    my $ldap = $class->connect_and_bind($config, $realm);
>> +
>> +    my $user_attr = $config->{user_attr} // 'uid';
>> +    my $attrs = {
>> +	$user_attr => 'username',
>> +	enable => 'enable',
>> +	expire => 'expire',
>> +	firstname => 'firstname',
>> +	lastname => 'lastname',
>> +	email => 'email',
>> +	comment => 'comment',
>> +	keys => 'keys',
>> +    };
>> +
>> +    foreach my $attr (PVE::Tools::split_list($config->{sync_attributes})) {
>> +	my ($ours, $ldap) = ($attr =~ m/^\s*(\w+)=(.*)\s*$/);
> 
> IMHO, this is now exactly the other way round compared to the API schema
> description. Which likely means that we should specify it clearly there
> ;)

yes, obviously i confused myself with my wording ;)

> 
>> +	$attrs->{$ldap} = $ours;
>> +    }
>> +
>> +    my $filter = $config->{filter};
>> +    my $basedn = $config->{base_dn};
>> +
>> +    $config->{user_classes} //= 'inetorgperson, posixaccount, person, user';
>> +    my $classes = [PVE::Tools::split_list($config->{user_classes})];
>> +
>> +    my $users = PVE::LDAP::query_users($ldap, $filter, [keys %$attrs], $basedn, $classes);
>> +
>> +    my $ret = {};
>> +    my $dnmap = {};
>> +
>> +    foreach my $user (@$users) {
>> +	my $user_attrs = $user->{attributes};
>> +	my $userid = $user_attrs->{$user_attr}->[0];
>> +	my $username = "$userid\@$realm";
>> +
>> +	# we cannot sync usernames that do not meet our criteria
>> +	eval { PVE::Auth::Plugin::verify_username($username) };
>> +	if (my $err = $@) {
>> +	    warn "$err";
>> +	    next;
>> +	}
>> +
>> +	$ret->{$username} = {
>> +	    enable => 1,
>> +	    expire => 0,
> 
> expire is already defaulted to 0 by user.cfg's write_config
> enable defaults to 0 there, but to 1 here - intentionally?

ok, so i would leave expire out entirely

my intention was to set users to enabled by default,
so that the synced users can be used right away
and disabled by the admin if needed, but looking at
my actual sync code again, it seems that that only works until the
next resync (we then overwrite it with these defaults)

i would propose to remove these two entries here completely
and have the admin enable the users manually (or maybe a separate option)?

that way 'enable' and 'expire' are only set by either
* the admin manually
* a specified sync attribute in the realm config

does this make sense?

> 
>> +	};
>> +
>> +	foreach my $attr (keys %$user_attrs) {
>> +	    if (my $key = $attrs->{$attr}) {
>> +		$ret->{$username}->{$key} = $user_attrs->{$attr}->[0];
> 
> there are too many variables with 'attr' in the name here. could we at
> least rename $attrs to make it clear that it contains the mapping of LDAP
> -> user.cfg attributes/keys?

i agree, i'll try to find better names

> 
>> +	    }
>> +	}
>> +
>> +	if (!wantarray) {
> 
> accidental negation?

yes, i do not know how this slipped through...

> 
>> +	    my $dn = $user->{dn};
>> +	    $dnmap->{$dn} = $username;
>> +	}
>> +    }
>> +
>> +    return wantarray ? ($ret, $dnmap) : $ret;
>> +}
>> +
>> +# needs a map for dn -> username, we get this from the get_users call
>> +# otherwise we cannot determine the group membership
>> +sub get_groups {
>> +    my ($class, $config, $realm, $dnmap) = @_;
>> +
>> +    my $filter = $config->{group_filter};
>> +    my $basedn = $config->{group_dn} // $config->{base_dn};
>> +    my $attr = $config->{group_attr};
>> +    $config->{group_classes} //= 'groupOfNames, group, univentionGroup, ipausergroup';
>> +    my $classes = [PVE::Tools::split_list($config->{group_classes})];
>> +
>> +    my $ldap = $class->connect_and_bind($config, $realm);
>> +
>> +    my $groups = PVE::LDAP::query_groups($ldap, $basedn, $classes, $filter, $attr);
>> +
>> +    my $ret = {};
>> +
>> +    foreach my $group (@$groups) {
>> +	my $name = $group->{name};
>> +	if (!$name && $group->{dn} =~ m/^[^=]+=([^,]+),/){
>> +	    $name = PVE::Tools::trim($1);
>> +	}
>> +	if ($name) {
>> +	    $name .= "-$realm";
> 
> maybe it would be a further improvement to mark users and groups as
> 'synced' and
> - only allow overwriting previously synced entries when syncing
> - disallow editing of synced entries via the regular API
> ?

i wanted to let the admin overwrite the user info if he wants to
what would be the advantage of 'locking' the synced entries?

> 
>> +
>> +	    # we cannot sync groups that do not meet our criteria
>> +	    eval { PVE::AccessControl::verify_groupname($name) };
>> +	    if (my $err = $@) {
>> +		warn "$err";
>> +		next;
>> +	    }
>> +
>> +	    $ret->{$name} = { users => {} };
>> +	    foreach my $member (@{$group->{members}}) {
>> +		if (my $user = $dnmap->{$member}) {
>> +		    $ret->{$name}->{users}->{$user} = 1;
>> +		}
>> +	    }
>> +	}
>> +    }
>> +
>> +    return $ret;
>> +}
>> +
>>   sub authenticate_user {
>>       my ($class, $config, $realm, $username, $password) = @_;
>>   
>> -- 
>> 2.20.1
>>
>>
>> _______________________________________________
>> pve-devel mailing list
>> pve-devel at pve.proxmox.com
>> https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
>>
>>
> 
> _______________________________________________
> pve-devel mailing list
> pve-devel at pve.proxmox.com
> https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
>