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

[Stoiko Ivanov]

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)

> +
> +    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
>