[pve-devel] [PATCH access-control 9/9] Domains: add sync API call
Fabian Grünbichler
f.gruenbichler at proxmox.com
Mon Mar 9 13:32:24 CET 2020
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.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
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 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