Re: [pve-devel] [access-contorl] fix #2947 login name for the LDAP/AD realm can be case-insensitive

[Thomas Lamprecht <t.lamprecht@proxmox.com>]

On 8/28/20 2:39 PM, Thomas Lamprecht wrote:
> off-topic: Please switch over your mails "To" address from the old
> <pve-devel@pve.proxmox.com> to the new <pve-devel@lists.proxmox.com>, thank you!
> 
> On 8/28/20 1:19 PM, Wolfgang Link wrote:
>> This is an optional for LDAP and AD realm.
>> The default behavior is case-sensitive.
>>
> 
> looks good in general, thanks!
> two nits and and additional place where we may want to use the new helper
> below.
> 
>> Signed-off-by: Wolfgang Link <w.link@proxmox.com>
>> ---
>>  PVE/API2/AccessControl.pm | 20 ++++++++++++++++++++
>>  PVE/Auth/LDAP.pm          |  7 +++++++
>>  2 files changed, 27 insertions(+)
>>
>> diff --git a/PVE/API2/AccessControl.pm b/PVE/API2/AccessControl.pm
>> index fd27786..4b4d0be 100644
>> --- a/PVE/API2/AccessControl.pm
>> +++ b/PVE/API2/AccessControl.pm
>> @@ -226,6 +226,25 @@ __PACKAGE__->register_method ({
>>      returns => { type => "null" },
>>      code => sub { return undef; }});
>>  
>> +sub lookup_username {
>> +    my ($username) = @_;
>> +
>> +    $username =~ /@(.+)/;
>> +
>> +    my $realm = $1;
>> +    my $domain_cfg = cfs_read_file("domains.cfg");
>> +    my $casesensitive = $domain_cfg->{ids}->{$realm}->{casesensitive} // 1;
>> +    my $usercfg = cfs_read_file('user.cfg');
>> +
>> +    if ($casesensitive == 0) {
> 
> nit: we normally rather use `if (!$casesensitive)`
> 
>> +	foreach my $user_name (keys %{$usercfg->{users}}) {
>> +	    return $user_name if lc $username eq lc $user_name;
>> +	}
> 
> above is fine, but if we'd use the built-in grep we could also easily die
> on multiple matches, for example, something below (untested):
> 
> my @matches = grep { lc $username eq lc $_ } @{keys %{$usercfg->{users}}};
> 
> die "ambiguous case insensitive match of username '$username', cannot safely grant access!\n"
>     if scalar @matches > 1;
> 
> return $matches[1];
> 
> 
> And we then actually want to use this method also in the API call for adding
> new users, to ensure an admin do not accidentally adds the case-sensitive same

argh, meant the same user when compared case *in*sensitively

> user multiple times.
> 
>> +    }
>> +
>> +    return $username;
>> +};
>> +
>>  __PACKAGE__->register_method ({
>>      name => 'create_ticket',
>>      path => 'ticket',
>> @@ -292,6 +311,7 @@ __PACKAGE__->register_method ({
>>  	my $username = $param->{username};
>>  	$username .= "\@$param->{realm}" if $param->{realm};
>>  
>> +	$username = lookup_username($username);
>>  	my $rpcenv = PVE::RPCEnvironment::get();
>>  
>>  	my $res;
>> diff --git a/PVE/Auth/LDAP.pm b/PVE/Auth/LDAP.pm
>> index 09b2202..75cce0f 100755
>> --- a/PVE/Auth/LDAP.pm
>> +++ b/PVE/Auth/LDAP.pm
>> @@ -129,6 +129,12 @@ sub properties {
>>  	    optional => 1,
>>  	    default => 'ldap',
>>  	},
>> +        casesensitive => {
> 
> 
> can we please use 'case-sensitive' here, makes it way easier to read.
> (and we (Wolfgang B and me) want to go away from using underscore as
> separator for API/ABI properties or parameters :))
> 
>> +	    description => "username is case-sensitive",
>> +	    type => 'boolean',
>> +	    optional => 1,
>> +	    default => 1,
>> +	}
>>      };
>>  }
>>  
>> @@ -159,6 +165,7 @@ sub options {
>>  	group_classes => { optional => 1 },
>>  	'sync-defaults-options' => { optional => 1 },
>>  	mode => { optional => 1 },
>> +	casesensitive => { optional => 1 },
>>      };
>>  }
>>  
>>