Authen::Passphrase::BlowfishCrypt - Create user.

[packetstormer]

Hi Monks,

Firstly, I just want to apologise for what might seems like a pointless post. I am self teaching myself Perl (from a non coding background) and I have nobody to "bounce" ideas, syntax and coding methods off. I see most posts on this site are accompanied by large amounts of code whereas I find myself having the most difficulty with concepts.

Anyhoo, I do have some code I would like critiqued, if possible. It is a small function to hash a password and encrypt it using BlowFishCrypt.
I am unsure if it is actually doing what I think it is (I think its good but not certain). I am trying to take a password, add a random salt and insert into a users table. The dbh function is elsewhere but just connects to the database. Does this look good? Any comments or advice would be great.

sub add_user {
    my $dbh = new_dbh();
    my $username = $_[0];
    my $password = $_[1];
    my $email = "$username\@email.com"; #change when live.
    my $ppr = Authen::Passphrase::BlowfishCrypt->new(
        cost => 12, salt_random => 1,
        passphrase => "$password");
    my $hash = $ppr->hash_base64;
    my $salt = $ppr->salt_base64;
    my $sth = $dbh->prepare('INSERT users (users_id,user_name,password
+,email,lib_id,department_id,
                        permissions, session_id,salt)
                        VALUES
                        ("",?,?,?,"1","1","1","",?)
                        ')
                or die "Couldn't prepare statement: " . $dbh->errstr;
    $sth->execute($username,$hash,$email,$salt) or die "Couldn't execu
+te statement: " . $sth->errstr;
    $sth->finish;
    $dbh->disconnect;

}

[kielstirling]

Hi,

Yes it does appear that it is doing what you state

I would however look at caching the db handle. Your code connects and disconnects every time you create a user account.

[oko1]

Ahh, conceptual questions. A great way to learn something, assuming you have the patience for it. :) My favorite approach, anyway.

From that angle - are you sure that you want to use BlowFish to do this? Frankly, you're not going to get any more security out of it than the basic 'crypt' method gives you - and using a non-standard method for it might give you some headaches down the road, especially if you have to do anything with other people's code related to dealing with passwords.

Next, separate the DB call out of this routine. Consider the situation where you want to add 200 users all at once: do you _really_ want to open and close the DB 200 times? Or would it make more sense to loop 'add_user' and return a statement for each one, then open the DB, fire off all the queries, and close it? Although maybe 'build_user' would be a more appropriate name for the sub at that point. :)

Avoid "magic values". Specifically, don't set that hard value for the email address in there. Long after that sub has become lost in the 10k lines of code around it, and long after you've forgotten what you wrote, you'll be scratching your head and trying to figure out where in *heck* that value came from. Instead, make it another argument that's passed to your sub:

sub add_user {
    my ($username, $password, $email) = @_;
    ...

Just some stuff to consider. Others may have more ideas.

[Anonymous Monk]

Thanks for the reply. I see your point about the database connections - thanks for the suggestion, it makes better sense!

The only reason I chose bcrypt was for the longer key setup and I was hoping that would slow down and/or effect any attempt at a brute force attack
Do you think this isn't worth it?