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.csa...@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@realm' => {
+# 'attr1' => 'value1',
+# 'attr2' => 'value2',
+# ...
+# },
+# ...
+# }
+#
+# or in list context:
+# (
+# {
+# 'username@realm' => {
+# 'attr1' => 'value1',
+# 'attr2' => 'value2',
+# ...
+# },
+# ...
+# },
+# {
+# 'uid=username,dc=....' => 'username@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@pve.proxmox.com
https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
_______________________________________________
pve-devel mailing list
pve-devel@pve.proxmox.com
https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
_______________________________________________
pve-devel mailing list
pve-devel@pve.proxmox.com
https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
