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
+ }
+ }
+
+ 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)
+ }
+ }
+ }
+ }
+
+ 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
