[pve-devel] [PATCH access-control 9/9] Domains: add sync API call

[Fabian Grünbichler]

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.csapak at 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?

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

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?

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

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

> +			}

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

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

> +
> +	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 at pve.proxmox.com
> https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
> 
>