* [pve-devel] [access-contorl] fix #2947 login name for the LDAP/AD realm can be case-insensitive
@ 2020-08-28 11:19 Wolfgang Link
2020-08-28 12:39 ` Thomas Lamprecht
2020-08-31 13:05 ` Dominik Csapak
0 siblings, 2 replies; 4+ messages in thread
From: Wolfgang Link @ 2020-08-28 11:19 UTC (permalink / raw)
To: pve-devel
This is an optional for LDAP and AD realm.
The default behavior is case-sensitive.
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) {
+ foreach my $user_name (keys %{$usercfg->{users}}) {
+ return $user_name if lc $username eq lc $user_name;
+ }
+ }
+
+ 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 => {
+ 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 },
};
}
--
2.20.1
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [pve-devel] [access-contorl] fix #2947 login name for the LDAP/AD realm can be case-insensitive
2020-08-28 11:19 [pve-devel] [access-contorl] fix #2947 login name for the LDAP/AD realm can be case-insensitive Wolfgang Link
@ 2020-08-28 12:39 ` Thomas Lamprecht
2020-08-28 12:41 ` Thomas Lamprecht
2020-08-31 13:05 ` Dominik Csapak
1 sibling, 1 reply; 4+ messages in thread
From: Thomas Lamprecht @ 2020-08-28 12:39 UTC (permalink / raw)
To: Proxmox VE development discussion, Wolfgang Link, pve-devel
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
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 },
> };
> }
>
>
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [pve-devel] [access-contorl] fix #2947 login name for the LDAP/AD realm can be case-insensitive
2020-08-28 12:39 ` Thomas Lamprecht
@ 2020-08-28 12:41 ` Thomas Lamprecht
0 siblings, 0 replies; 4+ messages in thread
From: Thomas Lamprecht @ 2020-08-28 12:41 UTC (permalink / raw)
To: Proxmox VE development discussion, Wolfgang Link
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 },
>> };
>> }
>>
>>
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [pve-devel] [access-contorl] fix #2947 login name for the LDAP/AD realm can be case-insensitive
2020-08-28 11:19 [pve-devel] [access-contorl] fix #2947 login name for the LDAP/AD realm can be case-insensitive Wolfgang Link
2020-08-28 12:39 ` Thomas Lamprecht
@ 2020-08-31 13:05 ` Dominik Csapak
1 sibling, 0 replies; 4+ messages in thread
From: Dominik Csapak @ 2020-08-31 13:05 UTC (permalink / raw)
To: pve-devel
just found something small that thomas did not mention (inline)
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.
>
> 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 =~ /@(.+)/;
the user part of the user name can contain an @
(e.g. foo@bar@pve) so we have to correctly match after the last @
/@([^@]+)$/
> +
> + 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) {
> + foreach my $user_name (keys %{$usercfg->{users}}) {
> + return $user_name if lc $username eq lc $user_name;
> + }
> + }
> +
> + 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 => {
> + 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 },
> };
> }
>
>
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2020-08-31 13:05 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-08-28 11:19 [pve-devel] [access-contorl] fix #2947 login name for the LDAP/AD realm can be case-insensitive Wolfgang Link
2020-08-28 12:39 ` Thomas Lamprecht
2020-08-28 12:41 ` Thomas Lamprecht
2020-08-31 13:05 ` Dominik Csapak
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox