[pve-devel] [PATCH access-control 9/9] Domains: add sync API call
Dominik Csapak
d.csapak at proxmox.com
Mon Mar 9 13:22:20 CET 2020
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.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?
>
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 at pve.proxmox.com
>> https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
>>
>>
>
> _______________________________________________
> pve-devel mailing list
> pve-devel at pve.proxmox.com
> https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
>
More information about the pve-devel
mailing list