From 8eed0466381435a0426fe9c2ea75fa295d906533 Mon Sep 17 00:00:00 2001 From: Nick Clemens Date: Wed, 13 Dec 2017 13:27:36 +0000 Subject: [PATCH] Bug 18947: LDAP - do not assume anonymous bind if no user or password To test: Ideally tested on a working ldap server with bind by auth and no anonymous bind 1 - Define an LDAP config with bind by auth 2 - Don't define user/pass 3 - Define anonymous_bind = 0 4 - Attempt bind by auth 5 - Error is something like: LDAP search failed to return object : XXXXXXXXX: LdapErr: XXXX-XXXXXX, comment: In order to perform this operation a successful bind must be completed on the connection., data 0, v2580 at /usr/share/koha/lib/C4/Auth_with_ldap.pm line 102. 6 - Define user/pass 7 - Now bind by auth should work 8 - remove user/pass 9 - Apply patch 10 - Attempt again 11 - Bind by auth shoudl succeed prove -v t/db_dependent/Auth_with_ldap.t Signed-off-by: Martin Renvoize Signed-off-by: Brendan A Gallagher Signed-off-by: Nick Clemens --- C4/Auth_with_ldap.pm | 9 ++++----- t/db_dependent/Auth_with_ldap.t | 25 +++++++++++++++++++++---- 2 files changed, 25 insertions(+), 9 deletions(-) diff --git a/C4/Auth_with_ldap.pm b/C4/Auth_with_ldap.pm index 53f0eb65ff..b5876a0d8d 100644 --- a/C4/Auth_with_ldap.pm +++ b/C4/Auth_with_ldap.pm @@ -59,7 +59,6 @@ my $prefhost = $ldap->{hostname} or die ldapserver_error('hostname'); my $base = $ldap->{base} or die ldapserver_error('base'); $ldapname = $ldap->{user} ; $ldappassword = $ldap->{pass} ; -$ldap->{anonymous_bind} = 1 unless $ldapname && $ldappassword; our %mapping = %{$ldap->{mapping}}; # FIXME dpavlin -- don't die because of || (); from 6eaf8511c70eb82d797c941ef528f4310a15e9f9 my @mapkeys = keys %mapping; $debug and print STDERR "Got ", scalar(@mapkeys), " ldap mapkeys ( total ): ", join ' ', @mapkeys, "\n"; @@ -76,7 +75,7 @@ if(defined $ldap->{categorycode_mapping}) { } my %config = ( - anonymous => ($ldapname and $ldappassword) ? 0 : 1, + anonymous => defined ($ldap->{anonymous_bind}) ? $ldap->{anonymous_bind} : 1, replicate => defined($ldap->{replicate}) ? $ldap->{replicate} : 1, # add from LDAP to Koha database for new user update => defined($ldap->{update} ) ? $ldap->{update} : 1, # update from LDAP to Koha database for existing user ); @@ -127,7 +126,7 @@ sub checkpw_ldap { # first, LDAP authentication if ( $ldap->{auth_by_bind} ) { my $principal_name; - if ( $ldap->{anonymous_bind} ) { + if ( $config{anonymous} ) { # Perform an anonymous bind my $res = $db->bind; @@ -155,7 +154,7 @@ sub checkpw_ldap { # Perform a LDAP bind for the given username using the matched DN my $res = $db->bind( $principal_name, password => $password ); if ( $res->code ) { - if ( $ldap->{anonymous_bind} ) { + if ( $config{anonymous} ) { # With anonymous_bind approach we can be sure we have found the correct user # and that any 'code' response indicates a 'bad' user (be that blocked, banned # or password changed). We should not fall back to local accounts in this case. @@ -176,7 +175,7 @@ sub checkpw_ldap { $userldapentry = $search->shift_entry; } } else { - my $res = ($ldap->{anonymous_bind}) ? $db->bind : $db->bind($ldapname, password=>$ldappassword); + my $res = ($config{anonymous}) ? $db->bind : $db->bind($ldapname, password=>$ldappassword); if ($res->code) { # connection refused warn "LDAP bind failed as ldapuser " . ($ldapname || '[ANONYMOUS]') . ": " . description($res); return 0; diff --git a/t/db_dependent/Auth_with_ldap.t b/t/db_dependent/Auth_with_ldap.t index 1a1c3d048f..e0c3c4532c 100755 --- a/t/db_dependent/Auth_with_ldap.t +++ b/t/db_dependent/Auth_with_ldap.t @@ -41,6 +41,8 @@ my $update = 0; my $replicate = 0; my $auth_by_bind = 1; my $anonymous_bind = 1; +my $user = 'cn=Manager,dc=metavore,dc=com'; +my $pass = 'metavore'; # Variables controlling LDAP behaviour my $desired_authentication_result = 'success'; @@ -143,7 +145,7 @@ subtest 'checkpw_ldap tests' => sub { subtest 'auth_by_bind = 1 tests' => sub { - plan tests => 9; + plan tests => 11; $auth_by_bind = 1; @@ -161,6 +163,19 @@ subtest 'checkpw_ldap tests' => sub { 'checkpw_ldap prints correct warning if LDAP anonymous bind fails'; is( $ret, 0, 'checkpw_ldap returns 0 if LDAP anonymous bind fails' ); + $anonymous_bind = 0; + $user = undef; + $pass = undef; + reload_ldap_module(); + + warning_like { + $ret = C4::Auth_with_ldap::checkpw_ldap( $dbh, 'hola', + password => 'hey' ); + } + qr/LDAP bind failed as kohauser hola: LDAP error #1: error_name/, + 'checkpw_ldap prints correct warning if LDAP bind_by_auth fails'; + is( $ret, 0, 'checkpw_ldap returns 0 if LDAP bind_by_auth fails' ); + $desired_authentication_result = 'success'; $anonymous_bind = 1; $desired_admin_bind_result = 'success'; @@ -251,8 +266,10 @@ subtest 'checkpw_ldap tests' => sub { # Anonymous bind $anonymous_bind = 1; + $user = 'cn=Manager,dc=metavore,dc=com'; + $pass = 'metavore'; $desired_admin_bind_result = 'error'; - $desired_bind_result = 'error'; + $desired_bind_result = 'error'; reload_ldap_module(); warning_like { @@ -361,11 +378,11 @@ sub mockedC4Config { base => 'dc=metavore,dc=com', hostname => 'localhost', mapping => \%ldap_mapping, - pass => 'metavore', + pass => $pass, principal_name => '%s@my_domain.com', replicate => $replicate, update => $update, - user => 'cn=Manager,dc=metavore,dc=com', + user => $user, ); return \%ldap_config; } -- 2.39.5