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

[Fabian Grünbichler]

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 
;)

> +	$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?

> +	};
> +
> +	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?

> +	    }
> +	}
> +
> +	if (!wantarray) {

accidental negation?

> +	    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
?

> +
> +	    # 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
> 
>