Split credential checking into a separate method from generic binding

They really are separate things. This reduces the risk of security
holes like <https://rt.cpan.org/Public/Bug/Display.html?id=81908>.

diff --git a/Changes b/Changes
index 92b11c8..d69ffa5 100644 (file)
--- a/Changes
+++ b/Changes
@@ -1,5 +1,6 @@
   - Escape special characters in user/role names
   - Use the stored user credentials to look up roles
+  - Split credential checking into a separate method from generic binding
 
 1.014 26 April 2013
   - Don't fall back to unauthenticated bind when authenticating

diff --git a/lib/Catalyst/Authentication/Store/LDAP/Backend.pm b/lib/Catalyst/Authentication/Store/LDAP/Backend.pm
index 403b63c..a6c46ab 100644 (file)
--- a/lib/Catalyst/Authentication/Store/LDAP/Backend.pm
+++ b/lib/Catalyst/Authentication/Store/LDAP/Backend.pm
@@ -208,8 +208,7 @@ If $binddn is "anonymous", an anonymous bind will be performed.
 =cut
 
 sub ldap_bind {
-    my ( $self, $ldap, $binddn, $bindpw, $forauth ) = @_;
-    $forauth ||= 0;
+    my ( $self, $ldap, $binddn, $bindpw ) = @_;
     $ldap ||= $self->ldap_connect;
     if ( !defined($ldap) ) {
         Catalyst::Exception->throw("LDAP Server undefined!");
@@ -226,20 +225,11 @@ sub ldap_bind {
         $self->_ldap_bind_anon($ldap);
     }
     else {
-        # Don't fall back to unauthenticated bind when authenticating
-        if ($bindpw or $forauth eq 'forauth') {
+        if ($bindpw) {
             my $mesg = $ldap->bind( $binddn, 'password' => $bindpw );
             if ( $mesg->is_error ) {
-
-                # If we're not checking this bind for authentication purposes
-                # Go ahead an blow up if we fail.
-                if ( $forauth ne 'forauth' ) {
-                    Catalyst::Exception->throw(
-                        "Error on Initial Bind: " . $mesg->error );
-                }
-                else {
-                    return undef;
-                }
+                Catalyst::Exception->throw(
+                    "Error on Initial Bind: " . $mesg->error );
             }
         }
         else {
@@ -257,6 +247,24 @@ sub _ldap_bind_anon {
     }
 }
 
+=head2 ldap_auth( $binddn, $bindpw )
+
+Connect to the LDAP server and do an authenticated bind against the
+directory. Throws an exception if connecting to the LDAP server fails.
+Returns 1 if binding succeeds, 0 if it fails.
+
+=cut
+
+sub ldap_auth {
+    my ( $self, $binddn, $bindpw ) = @_;
+    my $ldap = $self->ldap_connect;
+    if ( !defined $ldap ) {
+        Catalyst::Exception->throw("LDAP server undefined!");
+    }
+    my $mesg = $ldap->bind( $binddn, password => $bindpw );
+    return $mesg->is_error ? 0 : 1;
+}
+
 =head2 lookup_user($id)
 
 Given a User ID, this method will:

diff --git a/lib/Catalyst/Authentication/Store/LDAP/User.pm b/lib/Catalyst/Authentication/Store/LDAP/User.pm
index cafeab4..8089331 100644 (file)
--- a/lib/Catalyst/Authentication/Store/LDAP/User.pm
+++ b/lib/Catalyst/Authentication/Store/LDAP/User.pm
@@ -140,10 +140,7 @@ bind, 0 on failure.
 
 sub check_password {
     my ( $self, $password ) = @_;
-    my $ldap
-        = $self->store->ldap_bind( undef, $self->ldap_entry->dn, $password,
-        'forauth' );
-    if ( defined($ldap) ) {
+    if ( $self->store->ldap_auth($self->ldap_entry->dn, $password) ) {
         # Stash a closure which can be used to retrieve the connection in the users context later.
         $_ldap_connection_passwords{refaddr($self)} = $password;
         return 1;