Re: [PATCH 3/4] consolidate auth logic into Qpsmtpd::Auth

Comments?

Matt


On May 8, 2012, at 10:03 PM, Matt Simerson wrote:

> These 3 auth plugins all have a data store they fetch the reference
> password or hash from. They then match the attempted password or hash
> against the reference. This consolidates the latter portion (validating
> the password/hash) into Auth.pm.
> 
> * less duplicated code in the plugins.
> * Pass validation consistently handled for these 3 plugins.
> * less work to create similar auth plugins
> 
> Qpsmtpd::Auth also caches the CRAM-MD5 ticket. It could also cache
> user/pass info if this was desirable.
> ---
> lib/Qpsmtpd/Auth.pm                   |   59 +++++++++++++++++++++++++++++++--
> plugins/auth/auth_flat_file           |   34 ++++++++++---------
> plugins/auth/auth_vpopmail            |   49 ++++++++-------------------
> plugins/auth/auth_vpopmail_sql        |   54 ++++++++++++------------------
> t/Test/Qpsmtpd.pm                     |    1 +
> t/plugin_tests/auth/auth_vpopmail_sql |    5 +++
> 6 files changed, 118 insertions(+), 84 deletions(-)
> 
> diff --git a/lib/Qpsmtpd/Auth.pm b/lib/Qpsmtpd/Auth.pm
> index ec885b4..52e441d 100644
> --- a/lib/Qpsmtpd/Auth.pm
> +++ b/lib/Qpsmtpd/Auth.pm
> @@ -4,9 +4,11 @@ package Qpsmtpd::Auth;
> use strict;
> use warnings;
> 
> -use MIME::Base64;
> use Qpsmtpd::Constants;
> 
> +use Digest::HMAC_MD5 qw(hmac_md5_hex);
> +use MIME::Base64;
> +
> sub e64 {
>   my ($arg) = @_;
>   my $res = encode_base64($arg);
> @@ -144,6 +146,7 @@ sub get_auth_details_cram_md5 {
>         return;
>     }
> 
> +    $session->{auth}{ticket} = $ticket;
>     return ($ticket, $user, $passHash);
> };
> 
> @@ -159,6 +162,58 @@ sub get_base64_response {
>     return $answer;
> };
> 
> -# tag: qpsmtpd plugin that sets RELAYCLIENT when the user authentifies
> +sub validate_password {
> +    my ( $self, %a ) = @_;
> +
> +    my ($pkg, $file, $line) = caller();
> +    $file = (split '/', $file)[-1];     # strip off the path
> +
> +    my $src_clear     = $a{src_clear};
> +    my $src_crypt     = $a{src_crypt};
> +    my $attempt_clear = $a{attempt_clear};
> +    my $attempt_hash  = $a{attempt_hash};
> +    my $method        = $a{method} or die "missing method";
> +    my $ticket        = $a{ticket} || $self->{auth}{ticket};
> +    my $deny          = $a{deny} || DENY;
> +
> +    if ( ! $src_crypt && ! $src_clear ) {
> +        $self->log(LOGINFO, "fail: missing password");
> +        return ( $deny, "$file - no such user" );
> +    };
> +
> +    if ( ! $src_clear && $method =~ /CRAM-MD5/i ) {
> +        $self->log(LOGINFO, "skip: cram-md5 not supported w/o clear pass");
> +        return ( DECLINED, $file );
> +    }
> +
> +    if ( defined $attempt_clear ) {
> +        if ( $src_clear && $src_clear eq $attempt_clear ) {
> +            $self->log(LOGINFO, "pass: clear match");
> +            return ( OK, $file );
> +        };
> +
> +        if ( $src_crypt && $src_crypt eq crypt( $attempt_clear, $src_crypt ) ) {
> +            $self->log(LOGINFO, "pass: crypt match");
> +            return ( OK, $file );
> +        }
> +    };
> +
> +    if ( defined $attempt_hash && $src_clear ) {
> +        if ( ! $ticket ) {
> +            $self->log(LOGERROR, "skip: missing ticket");
> +            return ( DECLINED, $file );
> +        };
> +
> +        if ( $attempt_hash eq hmac_md5_hex( $ticket, $src_clear ) ) {
> +            $self->log(LOGINFO, "pass: hash match");
> +            return ( OK, $file );
> +        };
> +    };
> +
> +    $self->log(LOGINFO, "fail: wrong password");
> +    return ( $deny, "$file - wrong password" );
> +};
> +
> +# tag: qpsmtpd plugin that sets RELAYCLIENT when the user authenticates
> 
> 1;
> diff --git a/plugins/auth/auth_flat_file b/plugins/auth/auth_flat_file
> index 4d0abbc..a17d051 100644
> --- a/plugins/auth/auth_flat_file
> +++ b/plugins/auth/auth_flat_file
> @@ -30,7 +30,11 @@ algorithm so no password is transfered in the clear.
> 
> =cut
> 
> -use Digest::HMAC_MD5 qw(hmac_md5_hex);
> +use strict;
> +use warnings;
> +
> +use Qpsmtpd::Auth;
> +use Qpsmtpd::Constants;
> 
> sub register {
>     my ( $self, $qp ) = @_;
> @@ -45,35 +49,35 @@ sub auth_flat_file {
>       @_;
> 
>     if ( ! defined $passClear && ! defined $passHash ) {
> +        $self->log(LOGINFO, "fail: missing password");
>         return ( DENY, "authflat - missing password" );
>     }
> 
>     my ( $pw_name, $pw_domain ) = split '@', lc($user);
> 
>     unless ( defined $pw_domain ) {
> +        $self->log(LOGINFO, "fail: missing domain");
>         return DECLINED;
>     }
> 
>     my ($auth_line) = grep {/^$pw_name\@$pw_domain:/} $self->qp->config('flat_auth_pw');
> -    
> +
>     if ( ! defined $auth_line) {
> -        $self->log(LOGINFO, "User not found: $pw_name\@$pw_domain");
> +        $self->log(LOGINFO, "fail: no such user: $user");
>         return DECLINED;
>     }
> -    
> -    $self->log(LOGINFO, "Authentication for: $pw_name\@$pw_domain");
> 
>     my ($auth_user, $auth_pass) = split(/:/, $auth_line, 2);
> -    
> -    # at this point we can assume the user name matched
> -    if ( defined $passClear && $auth_pass eq $passClear ) {
> -        return ( OK, "authflat" );
> -    };
> 
> -    if ( defined $passHash && $passHash eq hmac_md5_hex($ticket, $auth_pass) ) {
> -        return ( OK, "authflat" );
> -    };
> -
> -    return ( DENY, "authflat - wrong password" );
> +    # at this point we can assume the user name matched
> +    return Qpsmtpd::Auth::validate_password( $self,
> +            src_clear     => $auth_pass,
> +            src_crypt     => undef,
> +            attempt_clear => $passClear,
> +            attempt_hash  => $passHash,
> +            method        => $method,
> +            ticket        => $ticket,
> +            deny          => DENY,
> +        );
> }
> 
> diff --git a/plugins/auth/auth_vpopmail b/plugins/auth/auth_vpopmail
> index ab05698..91a5ac6 100644
> --- a/plugins/auth/auth_vpopmail
> +++ b/plugins/auth/auth_vpopmail
> @@ -42,10 +42,10 @@ Please see the LICENSE file included with qpsmtpd for details.
> use strict;
> use warnings;
> 
> +use Qpsmtpd::Auth;
> use Qpsmtpd::Constants;
> 
> -use Digest::HMAC_MD5 qw(hmac_md5_hex);
> -#use vpopmail;    # we eval this in $test_vpopmail
> +#use vpopmail;    # we eval this in $test_vpopmail_module
> 
> sub register {
>     my ($self, $qp) = @_;
> @@ -60,54 +60,33 @@ sub register {
> sub auth_vpopmail {
>     my ($self, $transaction, $method, $user, $passClear, $passHash, $ticket) =
>       @_;
> -    my ($pw_name, $pw_domain) = split "@", lc($user);
> 
> -    $self->log(LOGINFO, "Authenticating against vpopmail: $user");
> -
> -    my $pw              = vauth_getpw($pw_name, $pw_domain);
> +    my $pw              = vauth_getpw( split '@', lc($user) );
>     my $pw_clear_passwd = $pw->{pw_clear_passwd};
>     my $pw_passwd       = $pw->{pw_passwd};
> 
> -    # make sure the user exists
>     if (!$pw || (!$pw_clear_passwd && !$pw_passwd)) {
> +        $self->log(LOGINFO, "fail: invalid user $user");
>         return (DENY, "auth_vpopmail - invalid user");
> -
>         # change DENY to DECLINED to support multiple auth plugins
>     }
> 
> -    return (OK, "auth_vpopmail")
> -      if $pw_passwd eq crypt($passClear, $pw_passwd);
> -
> -    # simplest case: clear text passwords
> -    if (defined $passClear && defined $pw_clear_passwd) {
> -        return (DENY, "auth_vpopmail - incorrect password")
> -          if $passClear ne $pw_clear_passwd;
> -        return (OK, "auth_vpopmail");
> -    }
> -
> -    if ($method =~ /CRAM-MD5/i) {
> -
> -        # clear_passwd isn't defined so we cannot support CRAM-MD5
> -        return (DECLINED, "auth_vpopmail") if !defined $pw_clear_passwd;
> -
> -        if (defined $passHash
> -            and $passHash eq hmac_md5_hex($ticket, $pw_clear_passwd))
> -        {
> -        }
> -    }
> -
> -    return (OK, "auth_vpopmail")
> -      if (defined $passHash
> -          && $passHash eq hmac_md5_hex($ticket, $pw_clear_passwd));
> -
> -    return (DENY, "auth_vpopmail - unknown error");
> +    return Qpsmtpd::Auth::validate_password( $self,
> +        src_clear     => $pw->{pw_clear_passwd},
> +        src_crypt     => $pw->{pw_passwd},
> +        attempt_clear => $passClear,
> +        attempt_hash  => $passHash,
> +        method        => $method,
> +        ticket        => $ticket,
> +        deny          => DENY,
> +    );
> }
> 
> sub test_vpopmail_module {
>     my $self = shift;
> # vpopmail will not allow vauth_getpw to succeed unless the requesting user is vpopmail or root.
> # by default, qpsmtpd runs as the user 'qpsmtpd' and does not have permission.
> -    eval "use vpopmail";
> +    eval 'use vpopmail';
>     if ( $@ ) {
>         $self->log(LOGERROR, "skip: is vpopmail perl module installed?");
>         return;
> diff --git a/plugins/auth/auth_vpopmail_sql b/plugins/auth/auth_vpopmail_sql
> index b68cec2..dd9b3cb 100644
> --- a/plugins/auth/auth_vpopmail_sql
> +++ b/plugins/auth/auth_vpopmail_sql
> @@ -66,13 +66,21 @@ Please see the LICENSE file included with qpsmtpd for details.
> use strict;
> use warnings;
> 
> -use DBI;
> +use Qpsmtpd::Auth;
> use Qpsmtpd::Constants;
> -use Digest::HMAC_MD5 qw(hmac_md5_hex);
> +
> +#use DBI;  # done in ->register
> 
> sub register {
>     my ( $self, $qp ) = @_;
> 
> +    eval 'use DBI';
> +    if ( $@ ) {
> +        warn "plugin disabled. is DBI installed?\n";
> +        $self->log(LOGERROR, "skip: plugin disabled. is DBI installed?\n");
> +        return;
> +    };
> +
>     $self->register_hook('auth-plain',    'auth_vmysql');
>     $self->register_hook('auth-login',    'auth_vmysql');
>     $self->register_hook('auth-cram-md5', 'auth_vmysql');
> @@ -122,45 +130,27 @@ sub auth_vmysql {
>     my ( $self, $transaction, $method, $user, $passClear, $passHash, $ticket ) = @_;
> 
>     my $dbh = $self->get_db_handle() or return DECLINED;
> -    my $db_user = $self->get_vpopmail_user($dbh, $user) or return DECLINED;
> +    my $u = $self->get_vpopmail_user($dbh, $user) or return DECLINED;
> 
>     # if vpopmail was not built with '--enable-clear-passwd=y'
>     # then pw_clear_passwd may not even exist
> -    my $pw_clear_passwd = $db_user->{'pw_clear_passwd'};
> -    my $pw_passwd = $db_user->{'pw_passwd'}; # always present
> +    # my $pw_clear_passwd = $db_user->{'pw_clear_passwd'};
> 
> -    if ( ! $pw_passwd && ! $pw_clear_passwd ) {
> +    if ( ! $u->{pw_passwd} && ! $u->{pw_clear_passwd} ) {
>         $self->log(LOGINFO, "fail: no such user");
>         return ( DENY, "auth_vmysql - no such user" );
>     };
> 
>     # at this point, the user name has matched
> 
> -    if ( ! $pw_clear_passwd && $method =~ /CRAM-MD5/i ) {
> -        $self->log(LOGINFO, "skip: cram-md5 not supported w/o clear pass");
> -        return ( DECLINED, "auth_vmysql" );
> -    }
> -
> -    if ( defined $passClear ) {
> -        if ( $pw_clear_passwd && $pw_clear_passwd eq $passClear ) {
> -            $self->log(LOGINFO, "pass: clear match");
> -            return ( OK, "auth_vmysql" );
> -        };
> -
> -        if ( $pw_passwd eq crypt( $passClear, $pw_passwd ) ) {
> -            $self->log(LOGINFO, "pass: crypt match");
> -            return ( OK, "auth_vmysql" );
> -        }
> -    };
> -
> -    if ( defined $passHash && $pw_clear_passwd &&
> -         $passHash eq hmac_md5_hex( $ticket, $pw_clear_passwd )
> -         ) {
> -        $self->log(LOGINFO, "pass: hash match");
> -        return ( OK, "auth_vmysql" );
> -    };
> -
> -    $self->log(LOGINFO, "fail: wrong password");
> -    return ( DENY, "auth_vmysql - wrong password" );
> +    return Qpsmtpd::Auth::validate_password( $self,
> +            src_clear     => $u->{pw_clear_passwd},
> +            src_crypt     => $u->{pw_passwd},
> +            attempt_clear => $passClear,
> +            attempt_hash  => $passHash,
> +            method        => $method,
> +            ticket        => $ticket,
> +            deny          => DENY,
> +        );
> }
> 
> diff --git a/t/Test/Qpsmtpd.pm b/t/Test/Qpsmtpd.pm
> index 0d830e0..e2c39c5 100644
> --- a/t/Test/Qpsmtpd.pm
> +++ b/t/Test/Qpsmtpd.pm
> @@ -1,6 +1,7 @@
> package Test::Qpsmtpd;
> use strict;
> use lib 't';
> +use lib 'lib';
> use Carp qw(croak);
> use base qw(Qpsmtpd::SMTP);
> use Test::More;
> diff --git a/t/plugin_tests/auth/auth_vpopmail_sql b/t/plugin_tests/auth/auth_vpopmail_sql
> index 0e6c84e..e61cad0 100644
> --- a/t/plugin_tests/auth/auth_vpopmail_sql
> +++ b/t/plugin_tests/auth/auth_vpopmail_sql
> @@ -6,6 +6,11 @@ use warnings;
> sub register_tests {
>     my $self = shift;
> 
> +    eval 'use DBI';
> +    if ( $@ ) {
> +        warn "skipping auth_vpopmail_sql tests, is DBI installed?\n";
> +        return;
> +    };
>     $self->register_test("auth_vpopmail_sql", 3);
> }
> 
> -- 
> 1.7.9.6
>