On March 9, 2020 1:22 pm, Dominik Csapak wrote: > On 3/9/20 11:45 AM, Fabian Grünbichler wrote: >> On March 6, 2020 11:05 am, Dominik Csapak wrote: >>> this api call syncs the users and groups from LDAP/AD to the >>> user.cfg, by default only users, but can be configured >>> >>> it also implements a 'prune' mode where we first delete all >>> users/groups from the config and sync them again >>> >>> also add this command to pveum >>> >>> Signed-off-by: Dominik Csapak <d.csa...@proxmox.com> >>> --- >>> PVE/API2/Domains.pm | 122 ++++++++++++++++++++++++++++++++++++++++++++ >>> PVE/CLI/pveum.pm | 1 + >>> 2 files changed, 123 insertions(+) >>> >>> diff --git a/PVE/API2/Domains.pm b/PVE/API2/Domains.pm >>> index 7f98e71..fea97dd 100644 >>> --- a/PVE/API2/Domains.pm >>> +++ b/PVE/API2/Domains.pm >>> @@ -2,6 +2,7 @@ package PVE::API2::Domains; >>> >>> use strict; >>> use warnings; >>> +use PVE::Exception qw(raise_param_exc); >>> use PVE::Tools qw(extract_param); >>> use PVE::Cluster qw (cfs_read_file cfs_write_file); >>> use PVE::AccessControl; >>> @@ -244,4 +245,125 @@ __PACKAGE__->register_method ({ >>> return undef; >>> }}); >>> >>> +__PACKAGE__->register_method ({ >>> + name => 'sync', >>> + path => '{realm}/sync', >>> + method => 'POST', >>> + permissions => { >>> + description => "You need 'Realm.AllocateUser' on >>> '/access/realm/<realm>' on the realm and 'User.Modify' permissions to >>> '/access/groups/'.", >>> + check => [ 'and', >>> + [ 'userid-param', 'Realm.AllocateUser'], >>> + [ 'userid-group', ['User.Modify']], >>> + ], >>> + }, >>> + description => "Syncs users and/or groups from LDAP to user.cfg. ". >>> + "NOTE: Synced groups will have the name 'name-\$realm', so ". >>> + "make sure those groups do not exist to prevent >>> overwriting.", >> >> see comment on patch #7 >> >>> + protected => 1, >>> + parameters => { >>> + additionalProperties => 0, >>> + properties => { >>> + realm => get_standard_option('realm'), >>> + scope => { >>> + description => "Select what to sync.", >>> + type => 'string', >>> + enum => [qw(users groups both)], >>> + optional => 1, >>> + default => 'users', >>> + }, >>> + prune => { >>> + description => "Remove users/groups from realm which do not >>> exist in LDAP.", >>> + type => 'boolean', >>> + optional => 1, >>> + default => 0, >>> + }, >>> + } >>> + }, >>> + returns => { type => 'null' }, >>> + code => sub { >>> + my ($param) = @_; >>> + >>> + my $rpcenv = PVE::RPCEnvironment::get(); >>> + my $authuser = $rpcenv->get_user(); >>> + >>> + >>> + my $realm = $param->{realm}; >>> + my $cfg = cfs_read_file($domainconfigfile); >>> + my $ids = $cfg->{ids}; >>> + >>> + raise_param_exc({ 'realm' => 'Realm does not exist.' }) if >>> !defined($ids->{$realm}); >>> + my $type = $ids->{$realm}->{type}; >>> + >>> + if ($type ne 'ldap' && $type ne 'ad') { >>> + die "Only LDAP/AD realms can be synced.\n"; >>> + } >>> + >>> + my $scope = $param->{scope} // 'users'; >>> + my $sync_users = $scope eq 'users' || $scope eq 'both'; >>> + my $sync_groups = $scope eq 'groups' || $scope eq 'both'; >>> + >>> + my $plugin = PVE::Auth::Plugin->lookup($ids->{$realm}->{type}); >>> + >>> + my $realmdata = $ids->{$realm}; >>> + my $users = {}; >>> + my $groups = {}; >>> + >>> + if ($sync_groups) { >>> + my $dnmap = {}; >>> + ($users, $dnmap) = $plugin->get_users($realmdata, $realm); >>> + $groups = $plugin->get_groups($realmdata, $realm, $dnmap); >>> + } else { >>> + $users = $plugin->get_users($realmdata, $realm); >>> + } >> >> could this (+ parsing/writing user.cfg + cluster lock acquisition) not >> take longer than 30s on bigger LDAP/AD instances? IMHO syncing LDAP to >> user.cfg is task-worthy in any case (e.g., to log some of the stuff >> mentioned below). any cons to forking a worker here? >> > > no we could fork a worker here, but at least on pmg > (where we also much work during a sync, namely writing the data into a > berkeley db) > we also do not use a worker and i have not heard of any case where > the sync would take longer than the 30 seconds > >>> + >>> + PVE::AccessControl::lock_user_config( >>> + sub { >>> + my $usercfg = cfs_read_file("user.cfg"); >>> + >>> + if ($sync_users) { >>> + my $oldusers = $usercfg->{users}; >>> + >>> + if ($param->{prune}) { >>> + foreach my $userid (keys %$oldusers) { >>> + next if $userid !~ m/\@$realm$/; >>> + next if $users->{$userid}; >>> + delete $oldusers->{$userid}; >> >> 1.) this old user might still be referenced in groups or ACLs, or have >> tokens + ACLs referencing them. would it make sense to check and warn >> about those here? they get cleaned up on the next RW-cycle.. should we >> force one? > > actually my intention was to leave the configured acls in the config, but > yeah if they get deleted anyway it would make sense to clean them up > anyway.... no need to write to user.cfg twice > >> >> 2.) maybe 'prune' could be renamed to 'full', and we could just delete >> the whole user here and drop the second 'next', and just remember >> 'tokens' to add them back below if necessary? > > this way we would lose all overwritten fields for the user
yes, that was my question - is the source of truth LDAP, or is the source of truth our local config ;) >> >>> + } >>> + } >>> + >>> + foreach my $userid (keys %$users) { >>> + my $user = $users->{$userid}; >>> + if (!defined($oldusers->{$userid})) { >>> + $oldusers->{$userid} = $user; >>> + } else { >>> + # TODO only 'new' attributes as option? >> >> IMHO a bit confusing, but there might be a use case if e.g. LDAP is out >> of our control, and we want to override some attributes locally in PVE.. > > yes that was my intention, the admin should be able to override > the attributes locally (at least until the next sync) > > my comment refers to an option that would only sync fields that > are not set at all in the old config > >> >>> + my $olduser = $oldusers->{$userid}; >>> + foreach my $attr (keys %$user) { >>> + $olduser->{$attr} = $user->{$attr}; >> >> this does not clean up attributes that have been deleted from LDAP - is >> this intentional? if yes, it should be mentioned somewhere.. if not, see >> 2.) above > > ok this is a bit tricky, how would we detect removed fields from ldap, > vs never set at all? we also do not really want to remove overwritten > fields from the admin (that were never set from ldap) we can't unless we explicitly want to track that on a per-attribute/property level. the current behaviour is a bit unexpected, especially when thinking about 2FA keys.. we could avoid the isuse by making LDAP the source of truth and not allowing any modification of synced users/groups (forcing changes through LDAP), but I am not sure whether that is what users of this feature want. > >> >>> + } >> >>> + } >>> + } >>> + } >>> + >>> + if ($sync_groups) { >> >> it just $sync_groups is passed, the groups might refer to non-existing >> users.. should this be checked and caught/warned about? > > right, we only want group members which are actually in the config > >> >>> + my $oldgroups = $usercfg->{groups}; >>> + >>> + if ($param->{prune}) { >>> + foreach my $groupid (keys %$oldgroups) { >>> + next if $groupid !~ m/\-$realm$/; >>> + next if $groups->{$groupid}; >>> + delete $oldgroups->{$groupid}; >> >> same comment regarding ACLs referring to deleted groups applies here as >> well >> >>> + } >>> + } >>> + >>> + foreach my $groupid (keys %$groups) { >>> + $oldgroups->{$groupid} = $groups->{$groupid}; >>> + } >>> + } >>> + cfs_write_file("user.cfg", $usercfg); >>> + }, "syncing users failed"); >> >> nit: adapt message based on $scope ? > > ok, thanks for the review :) > >> >>> + >>> + return undef; >>> + }}); >>> + >>> 1; >>> diff --git a/PVE/CLI/pveum.pm b/PVE/CLI/pveum.pm >>> index d3721b6..cffd4da 100755 >>> --- a/PVE/CLI/pveum.pm >>> +++ b/PVE/CLI/pveum.pm >>> @@ -148,6 +148,7 @@ our $cmddef = { >>> modify => [ 'PVE::API2::Domains', 'update', ['realm'] ], >>> delete => [ 'PVE::API2::Domains', 'delete', ['realm'] ], >>> list => [ 'PVE::API2::Domains', 'index', [], {}, $print_api_result, >>> $PVE::RESTHandler::standard_output_options], >>> + sync => [ 'PVE::API2::Domains', 'sync', ['realm'], ], >>> }, >>> >>> ticket => [ 'PVE::API2::AccessControl', 'create_ticket', >>> ['username'], undef, >>> -- >>> 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