Re: RFC: Net::LDAP::Simple

[submersible_toaster]

I am uncertain how much simpler you could make Net::LDAP to cope with, with perhaps the exception of the $mesg->code && die $mesg->error dance. I have some existing Net::LDAP code, from a CGI application I am prototyping, which bundles the LDAP access politely away from the main code. I have added more verbose comments to describe it, and it appears thus.

sub authenticate {
 # instance method, $self is a blessed hash holding various
 # important details like ldaphost etc.
 my $self = shift;
 # $q , a CGI->new query object
 my $q = shift;
 
  # Open an anonymous ldap session (anon reads are allowed)
 my $ldap = Net::LDAP->new( $ldaphost ) or die "LDAP Connection error"
+;
 $ldap->bind;
 my $user = $q->param('username');
 my $mesg = $ldap->search ( 
     base=>$self->{ldap}{userbase},
     filter=>"(&(cn=$user))"
 );
 # one day these dies will be calls to  pretty printed
 # html .  mymodule::error->database_error()
 $mesg->code && die $mesg->error;
 $ldap->unbind;
 
 # Dodgy, I admit : we only expect one account with the uid eq $user
 my $entry = $mesg->shift_entry;

 # Bailout if user does not exist in LDAP. 
 return undef unless ($entry);
  
 my $ldaphash = $entry->get_value('userPassword');
 my $ldapuser = $entry->get_value('uid');
 
 # hash the CGI supplied password to compare with LDAP userPassword
 my $md5 = Digest::MD5->new;
 $md5->add( $q->param('phrase') );
 my $hash = '{MD5}' . encode_base64($md5->digest, '');
 
 if ( ( $q->param('username') eq $ldapuser) and ($hash eq $ldaphash) )
  {
   my $sessionid = $self->start_session( $q );
   return $sessionid;
  }
 else {
  return undef
 }
}

I can see where you're coming from , however rewriting this to use Net::LDAP::Simple , feels more like I'm shuffling the args to different methods rather than simplifying the code. I think search paramaters belong with search methods, not in the constructor.

sub authenticate {
 my $self = shift;
 my $q = shift;
 
 # Using Net::LDAP::Simple
 my $ldap = Net::LDAP::Simple->new( 
         host=>$ldaphost ,
        base=>$self->{ldap}{userbase} ,
        searchattrs=>'uid'    
 
 ) or die "LDAP Connection error";
 
 my $user = $q->param('username');
 my $result = $ldap->simplesearch( $user );
 die $ldap->error unless $result;
 $ldap->unbind;
 
 my $entry = shift @{$result};

 # Bailout if user does not exist in LDAP. 
 return undef unless ($entry);
  
 my $ldaphash = $entry->get_value('userPassword');
 my $ldapuser = $entry->get_value('uid');
 
 my $md5 = Digest::MD5->new;
 $md5->add( $q->param('phrase') );
 my $hash = '{MD5}' . encode_base64($md5->digest, '');
 
 if ( ( $q->param('username') eq $ldapuser) and ($hash eq $ldaphash) )
  {
   my $sessionid = $self->start_session( $q );
   return $sessionid;
  }
 else {
  return undef
 }
}

Please forgive me if I have misunderstood your approach, and for goodness sake keep working on the idea. Collecting peoples ideas RE what would make LDAP simpler to use for them might be a good start. I have considered writing some meta-methods to do commonplace things like move and rename, I reckon your ideas there are spot on. I watch with interest

-toaster

I can't believe it's not psellchecked

[bronto]

First of all, thanks for your comments!

I can see where you're coming from , however rewriting this to use Net::LDAP::Simple , feels more like I'm shuffling the args to different methods rather than simplifying the code. I think search paramaters belong with search methods, not in the constructor.

The greatest benefits of this approach come when you are performing many operations on array of entries over the same connection. I'm not sure what to put on an example, since the concept of simplicity is different from person to person, but I'll try anyway.

Compare this two snippets: you are doing similar searches on the same attributes but with different search strings, then adding an objectclass to each entry and storing the entries back again.

use strict ;
use warnings ;

use Net::LDAP::Simple ;

eval {
my $ldap = Net::LDAP::Simple->new(host => 'x.it',
  bindDN => 'cn=admin,ou=People,dc=x,dc=it',
  bindpw => 'secret',
  base => 'ou=People,dc=x,dc=it',
  searchattrs => [qw(cn uid loginname)]) ;
} ;

die "Can't connect: $@" unless defined $ldap ;

my @users ;

# I won't preload all entries in production code,
# in fact this is just an example :-)
foreach my $user (qw(pinco pallino caro bellino)) {
  my $res = $ldap->simplesearch($user) ;
  die $ldap->error unless defined $res ;
  push @users,@$res ;
}

my $update = $ldap->rewrite(map($_->add(objectclass => 'posixAccount')
+)) ;

unless (@$update == @users) {
  my $entry = pop @$update ;
  warn "Cannot modify ".$entry->dn.", giving up!" ;
}

with this:

use strict ;
use warnings ;

use Net::LDAP ;

sub makefilter {
  return qq/(|(uid~=$_[0])(|(cn~=$_[0])(loginname~=$_[0])))/
}

my $ldap = Net::LDAP->new('x.it') ;
{
  my $msg = $ldap->bind('cn=admin,ou=People,dc=x,dc=it',
                        password => 'secret') ;
  die "Cannot bind: ".$msg->error if $msg->is_error ;
}

my $base = 'ou=People,dc=x,dc=it' ;

my @users ;

# I won't preload all entries in production code,
# in fact this is just an example :-)
foreach my $user (qw(pinco pallino caro bellino)) {
  my $filter = makefilter($user) ;
  my $msg = $ldap->search(base => $base,
                          filter => $filter) ;
  die $msg->error if $msg->is_error ;
  push @users,$ldap->entries ;
}

foreach my $entry (@users) {
  my $msg = $ldap->modify($entry,
                add => { objectclass => 'posixAccount' }) ;
  if ($msg->is_error) {
    warn "Cannot modify ".$entry->dn.", giving up!" ;
    last ;
  }
}

In the Net::LDAP::Simple code you don't need to define a makefilter sub, the module takes care of it; you don't need to check $msg->is_error at every call: you get an array reference or undef for every method that works on entries; you don't need to iterate over an array of @entries: the module takes care of it. Some application do exactly that, and those applications are the target of the module... er, class.

Again, thanks for your feedback!

Ciao!
--bronto

---

The very nature of Perl to be like natural language--inconsistant and full of dwim and special cases--makes it impossible to know it all without simply memorizing the documentation (which is not complete or totally correct anyway).
--John M. Dlugosz

[kennethwlangley]

This question is OT from the original post.

I've a question about the authentication method you describe. What were the reasons to retrieve the password using an anonymous bind versus trying to bind with the username/password pair given? I'm doing similar work but our dir server does not allow an anonymous bind to retrieve the userPassword attribute.

[submersible_toaster]

Still a good question!
The main reason (although it is not obvious from the code) is that there are many OUs beneath the userbase DN, for reasons too lengthy to explain here. Hence I cannot explicitly bind the given user as

$ldap->bind( "cn=$user,".$self->{ldap}{userbase} , 
        password=>$password )

Since that user may be in any of a number of sub OUs to the userbase. I admit that there was much "umm" and "err" about using an anonymous bind to find the user entry, then rebind with that DN and the supplied password. The directory in question is accessible only from 127.0.0.1 , and it is not involved in any way in storing system accounts. My concerns about userPassword hashes being stolen are largely moot, if they can only be accessed locally, if a malicious user is already local - I have more problems than them having anon read access to LDAP!.

Please post some code if you can, or in the least read/comment my meditation that more fully explains what I am stabbing in the dark at.


I can't believe it's not psellchecked

[kennethwlangley]

I'm not really sure what you asking for in the last sentence, but I did read your meditation about WIP delivery system. My taste is to leverage what already exists (in this case using an ldap bind call to authenticate users). Do you ever anticipate implementing a password policy that requires changes (e.g. once a quarter)? In my project I have to be able to support a password policy for accounts in the directory -- so I think my authentication method has to be a bind as the user. Our directory is public so I don't think allowing anonymous access to userPassword would be a good idea.
Read more... (635 Bytes)