[pmg-devel] [PATCH pmg-api] fix #2661: reintroduce LDAPCache->ldap_connect

[Dominik Csapak]

On 4/3/20 1:58 PM, Stoiko Ivanov wrote:
> Thanks for finding and fixing this so quickly!
> 
> The patch works and makes sense to me.
> one (unrelated) stylistic nit below, which is only a matter of taste
> 
> looked quickly through the code and use and tested it in my setup:
> 
> Reviewed-By: Stoiko Ivanov <s.ivanov at proxmox.com>
> Tested-By: Stoiko Ivanov <s.ivanov at proxmox.com>
> 
> On Fri, Apr 03, 2020 at 09:16:27AM +0200, Dominik Csapak wrote:
>> this was removed and integrated into ldap_connect_and_bind, but
>> we used it outside in LDAPSet.pm
>>
>> so reintroduce it again
>>
>> Signed-off-by: Dominik Csapak <d.csapak at proxmox.com>
>> ---
>>   src/PMG/LDAPCache.pm | 54 ++++++++++++++++++++++++--------------------
>>   1 file changed, 30 insertions(+), 24 deletions(-)
>>
>> diff --git a/src/PMG/LDAPCache.pm b/src/PMG/LDAPCache.pm
>> index 987f8bc..3045c27 100755
>> --- a/src/PMG/LDAPCache.pm
>> +++ b/src/PMG/LDAPCache.pm
>> @@ -248,33 +248,39 @@ sub querygroups {
>>       }
>>   }
>>   
>> +sub ldap_connect {
>> +    my ($self) = @_;
>> +
>> +    my $hosts = [ $self->{server1} ];
>> +    push @$hosts, $self->{server2} if $self->{server2};
>> +
>> +    my $opts = {};
>> +    my $scheme = $self->{mode};
>> +
>> +    if ($scheme eq 'ldaps') {
>> +	$opts->{verify} = 'require' if $self->{verify};
>> +	if ($self->{cafile}) {
>> +	    $opts->{cafile} = $self->{cafile};
>> +	} else {
>> +	    $opts->{capath} = '/etc/ssl/certs/';
>> +	}
>> +    } elsif ($self->{mode} eq 'ldap+starttls') {
>> +	$opts->{verify} = $self->{verify} ? 'require' : 'none';
>> +
>> +	if ($self->{cafile}) {
>> +	    $opts->{cafile} = $self->{cafile};
>> +	} else {
>> +	    $opts->{capath} = '/etc/ssl/certs/';
>> +	}
>> +    }
> not introduced by your patch - but why not make the condition
> ($scheme eq 'ldaps' || $scheme eq 'ldap+starttls')
> set the common parameters and only set the verify individually?
> (more a question of curiosity than a suggestion for changing it)

good question, also why did i use $scheme one time and $self-{mode} the 
other?
i guess i did rewrite the code a few times and at the end
the branches were this similar, but i did not notice ^^

@Thomas, should i send a v2 or do you prefer it to have a follow up?
(so that the 'fix' commit is separate)

> 
>> +
>> +    return PVE::LDAP::ldap_connect($hosts, $scheme, $self->{port}, $opts);
>> +}
>> +
>>   sub ldap_connect_and_bind {
>>        my ($self) = @_;
>>   
>> -     my $hosts = [ $self->{server1} ];
>> -     push @$hosts, $self->{server2} if $self->{server2};
>> -
>> -     my $opts = {};
>> -     my $scheme = $self->{mode};
>> -
>> -     if ($scheme eq 'ldaps') {
>> -	 $opts->{verify} = 'require' if $self->{verify};
>> -	 if ($self->{cafile}) {
>> -	     $opts->{cafile} = $self->{cafile};
>> -	 } else {
>> -	     $opts->{capath} = '/etc/ssl/certs/';
>> -	 }
>> -     } elsif ($self->{mode} eq 'ldap+starttls') {
>> -	 $opts->{verify} = $self->{verify} ? 'require' : 'none';
>> -
>> -	 if ($self->{cafile}) {
>> -	     $opts->{cafile} = $self->{cafile};
>> -	 } else {
>> -	     $opts->{capath} = '/etc/ssl/certs/';
>> -	 }
>> -     }
>> -
>> -     my $ldap =  eval { PVE::LDAP::ldap_connect($hosts, $scheme, $self->{port}, $opts) };
>> +     my $ldap =  eval { $self->ldap_connect() };
>>        die "Can't bind to ldap server '$self->{id}': " . ($@) . "\n" if $@;
>>   
>>        my $dn;
>> -- 
>> 2.20.1
>>
>>
>> _______________________________________________
>> pmg-devel mailing list
>> pmg-devel at pve.proxmox.com
>> https://pve.proxmox.com/cgi-bin/mailman/listinfo/pmg-devel
>>