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

[Thomas Lamprecht]

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


> -    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';
>      }
>  
>