[pve-devel] [PATCH access-control 3/4] auth ldap/ad: introduce connection 'mode'

[Dominik Csapak]

On 4/6/20 1:41 PM, Thomas Lamprecht wrote:
> On 4/6/20 1:31 PM, Dominik Csapak wrote:
>> instead of having only a 'secure' flag which switches between
>> ldap/ldaps we now have a mode which also contains 'ldap+starttls'
>>
>> our connection code in PVE::LDAP can handle this already (used in pmg)
>> so that is no problem
>>
>> if we want to really remove the 'secure' flag, e.g. in 7.0
>> we'd either have to rewrite the config or have it as an error
>> in a pve6to7 script
>>
>> Signed-off-by: Dominik Csapak <d.csapak at proxmox.com>
>> ---
>> we could of course rewrite the config now already, but
>> i am not sure if that is wise, since e.g. config management
>> tools do not expect it now? is that a thing we should care about?
>>
>>   PVE/Auth/AD.pm   | 10 ++++++----
>>   PVE/Auth/LDAP.pm | 15 ++++++++++++---
>>   2 files changed, 18 insertions(+), 7 deletions(-)
>>
>> diff --git a/PVE/Auth/AD.pm b/PVE/Auth/AD.pm
>> index 4f40be3..395d78b 100755
>> --- a/PVE/Auth/AD.pm
>> +++ b/PVE/Auth/AD.pm
>> @@ -27,7 +27,7 @@ sub properties {
>>   	    maxLength => 256,
>>   	},
>>   	secure => {
>> -	    description => "Use secure LDAPS protocol.",
>> +	    description => "Use secure LDAPS protocol. DEPRECATED: use 'mode' instead.",
>>   	    type => 'boolean',
>>   	    optional => 1,
>>   	},
>> @@ -92,6 +92,7 @@ sub options {
>>   	group_filter => { optional => 1 },
>>   	group_classes => { optional => 1 },
>>   	'sync-defaults-options' => { optional => 1 },
>> +	mode => { optional => 1 },
>>       };
>>   }
>>   
>> @@ -109,9 +110,10 @@ sub authenticate_user {
>>       my $servers = [$config->{server1}];
>>       push @$servers, $config->{server2} if $config->{server2};
>>   
>> -    my $default_port = $config->{secure} ? 636: 389;
>> +    my $scheme = $config->{mode} // ($config->{secure} ? 'ldaps' : 'ldap');
>> +
>> +    my $default_port = $scheme eq 'ldaps' ? 636 : 389;
>>       my $port = $config->{port} // $default_port;
>> -    my $scheme = $config->{secure} ? 'ldaps' : 'ldap';
>>   
>>       my %ad_args;
>>       if ($config->{verify}) {
>> @@ -129,7 +131,7 @@ sub authenticate_user {
>>   	$ad_args{verify} = 'none';
>>       }
>>   
>> -    if ($config->{secure}) {
>> +    if ($scheme ne 'ldap') {
>>   	$ad_args{sslversion} = $config->{sslversion} // 'tlsv1_2';
>>       }
>>   
>> diff --git a/PVE/Auth/LDAP.pm b/PVE/Auth/LDAP.pm
>> index 905cc47..9b9199d 100755
>> --- a/PVE/Auth/LDAP.pm
>> +++ b/PVE/Auth/LDAP.pm
>> @@ -117,6 +117,13 @@ sub properties {
>>   	    format => 'realm-sync-options',
>>   	    optional => 1,
>>   	},
>> +	mode => {
>> +	    description => "LDAP protocol mode.",
>> +	    type => 'string',
>> +	    enum => [ 'ldap', 'ldaps', 'ldap+starttls'],
>> +	    optional => 1,
>> +	    default => 'ldap',
>> +	},
>>       };
>>   }
>>   
>> @@ -145,6 +152,7 @@ sub options {
>>   	group_filter => { optional => 1 },
>>   	group_classes => { optional => 1 },
>>   	'sync-defaults-options' => { optional => 1 },
>> +	mode => { optional => 1 },
>>       };
>>   }
>>   
>> @@ -154,9 +162,10 @@ sub connect_and_bind {
>>       my $servers = [$config->{server1}];
>>       push @$servers, $config->{server2} if $config->{server2};
>>   
>> -    my $default_port = $config->{secure} ? 636: 389;
>> +    my $scheme = $config->{mode} // ($config->{secure} ? 'ldaps' : 'ldap');
>> +
>> +    my $default_port = $scheme eq 'ldaps' ? 636 : 389;
>>       my $port = $config->{port} // $default_port;
> 
> can we factor the get port and scheme with default fallbacks out to it's
> own little helper? After all we use it twice and it crowds things here
> 
> OK, besides that
> 

ah yes of course

> 
>> -    my $scheme = $config->{secure} ? 'ldaps' : 'ldap';
>>   
>>       my %ldap_args;
>>       if ($config->{verify}) {
>> @@ -174,7 +183,7 @@ sub connect_and_bind {
>>   	$ldap_args{verify} = 'none';
>>       }
>>   
>> -    if ($config->{secure}) {
>> +    if ($scheme ne 'ldap') {
>>   	$ldap_args{sslversion} = $config->{sslversion} || 'tlsv1_2';
>>       }
>>   
>>
>