Merge pull request #233 from jaredj/greylist-error-handling

Skip greylisting when we can't talk to greylist DB

lib/Qpsmtpd/DB/File/DBM.pm

@@ -180,15 +180,23 @@ sub flush {
 }
 
 sub dir {
-    my ( $self, @candidate_dirs ) = @_;
+    my ( $self, @arg ) = @_;
-    return $self->{dir} if $self->{dir} and ! @candidate_dirs;
+    return $self->{dir} if $self->{dir} and ! @arg;
-    push @candidate_dirs, ( $self->qphome . '/var/db', $self->qphome . '/config' );
+    for my $d ( $self->candidate_dirs(@arg) ) {
-    for my $d ( @candidate_dirs ) {
         next if ! $self->validate_dir($d);
         return $self->{dir} = $d; # first match wins
     }
 }
 
+sub candidate_dirs {
+    my ( $self, @arg ) = @_;
+    return @{ $self->{candidate_dirs} } if $self->{candidate_dirs} && ! @arg;
+    $self->{candidate_dirs} = \@arg if @arg;
+    push @{ $self->{candidate_dirs} },
+      $self->qphome . '/var/db', $self->qphome . '/config';
+    return @{ $self->{candidate_dirs} };
+}
+
 sub validate_dir {
     my ( $self, $d ) = @_;
     return 0 if ! $d;

lib/Qpsmtpd/Plugin.pm

@@ -347,10 +347,24 @@ sub _register_standard_hooks {
     }
 }
 
+sub db_args {
+    my ( $self, %arg ) = @_;
+    $self->validate_db_args(@_);
+    $self->{db_args} = \%arg if %arg;
+    $self->{db_args}{name} ||= $self->plugin_name;
+    $self->{db_args}{cnx_timeout} ||= 1;
+    return %{ $self->{db_args} };
+}
+
 sub db {
     my ( $self, %arg ) = @_;
-    $arg{name} ||= $self->plugin_name;
+    $self->validate_db_args(@_);
-    return $self->{db} ||= Qpsmtpd::DB->new(%arg);
+    return $self->{db} ||= Qpsmtpd::DB->new( $self->db_args(%arg) );
+}
+
+sub validate_db_args {
+    (my $self, undef, my @args) = @_;
+    die "Invalid db arguments\n" if @args % 2;
 }
 
 1;

plugins/greylisting

@@ -111,6 +111,9 @@ Location of redis server where the greylisting DB will be stored.
 Redis can be used as a scalable and clusterable alternative
 to a simple DBM file.  For more information, see http://redis.io
 
+When Redis is in use, this plugin will wait up to 1 second to connect;
+when Redis is unavailable, clients will not be greylisted.
+
 =head2 per_recipient <bool>
 
 Flag to indicate whether to use per-recipient configs.
@@ -211,9 +214,10 @@ sub register {
         $config->{reject} = $config->{mode} =~ /testonly|off/i ? 0 : 1;
     }
     $self->{_args} = $config;
-    $self->init_db() or return;
+    $self->init_db();
     $self->register_hooks();
-    $self->prune_db();
+    eval { $self->prune_db(); };
+    $self->log(LOGERROR, "Unable to prune greylist DB: $@") if $@;
     if ($self->{_args}{upgrade}) {
         $self->convert_db();
     }
@@ -233,22 +237,20 @@ sub register_hooks {
 
 sub init_db {
     my ($self) = @_;
-    return $self->init_redis if $self->{_args}{redis};
+    if ( $self->{_args}{redis} ) {
-    return $self->init_dbm;
+        $self->init_redis;
+    } else {
+        $self->init_dbm;
+    }
 }
 
 sub init_redis {
     my ($self) = @_;
-    eval {
+    $self->db_args(
-        $self->db(
+        name   => 'greylist',
-            name   => 'greylist',
+        class  => 'Qpsmtpd::DB::Redis',
-            class  => 'Qpsmtpd::DB::Redis',
+        server => $self->parse_redis_server,
-            server => $self->parse_redis_server,
+    );
-        ) or die 'Unknown error';
-    };
-    return 1 if ! $@;
-    $self->log(LOGCRIT, "Unable to connect to redis, GREYLISTING DISABLED: $@");
-    return 0;
 }
 
 sub parse_redis_server {
@@ -263,19 +265,18 @@ sub init_dbm {
     $self->db(
         name  => 'greylist',
         class => 'Qpsmtpd::DB::File::DBM'
-    ) or return 0;
+    );
     my $cdir = $self->{_args}{db_dir};
     $cdir = $1 if $cdir and $cdir =~ m{^([-a-zA-Z0-9./_]+)$};
     # greylisting-specific hints for where to store the greylist DB
     my $db_dir = $self->db->dir( $cdir, '/var/lib/qpsmtpd/greylisting' );
     $self->db->nfs_locking( $self->{_args}{nfslock} );
-    
+
     # Work around old DBM filename
-    return 1 if -f "$db_dir/greylist.dbm";
     my $oldname = 'denysoft_greylist';
-    return 1 if ! -f "$db_dir/$oldname.dbm";
+    if ( ! -f "$db_dir/greylist.dbm" && -f "$db_dir/$oldname.dbm" ) {
-    $self->db->name($oldname);
+        $self->db->name($oldname);
-    return 1;
+    }
 }
 
 sub load_exclude_files {
@@ -386,16 +387,25 @@ sub greylist {
 
 
     my $key  = $self->get_greylist_key($sender, $rcpt) or return DECLINED;
+    my @result;
+    eval {
+        $self->db->lock;
+        @result = $self->greylist_result($key, $config);
+        $self->db->unlock;
+    };
+    return $self->errcode($@) if $@;
+    return @result;
+}
 
+sub greylist_result {
+    my ($self, $key, $config) = @_;
     my $fmt = "%s:%d:%d:%d";
-
-    $self->db->lock or return DECLINED;
     my $value = $self->db->get($key);
     if ( ! $value ) {
         # new IP or entry timed out - record new
         $self->db->set( $key, sprintf $fmt, $self->now, 1, 0, 0 );
         $self->log(LOGWARN, "fail: initial DENYSOFT, unknown");
-        return $self->cleanup_and_return();
+        return $self->failcode();
     }
 
     my ( $ts, $new, $black, $white ) = split /:/, $value;
@@ -407,7 +417,7 @@ sub greylist {
         if ( $self->now - $ts < $config->{white_timeout} ) {
             $self->db->set( $key, sprintf $fmt, $self->now, $new, $black, ++$white );
             $self->log(LOGINFO, "pass: white, $white deliveries");
-            return $self->cleanup_and_return(DECLINED);
+            return DECLINED;
         }
         else {
             $self->log(LOGINFO, "key $key has timed out (white)");
@@ -419,26 +429,29 @@ sub greylist {
         $self->db->set( $key, sprintf $fmt, $ts, $new, ++$black, 0 );
         $self->log(LOGWARN,
                    "fail: black DENYSOFT - $black deferred connections");
-        return $self->cleanup_and_return();
+        return $self->failcode();
     }
 
     $self->log(LOGWARN, "pass: timed out (grey)");
-    return $self->cleanup_and_return(DECLINED);
+    return DECLINED;
 }
 
 # This exists purely to be overridden for testing
 sub now { time() }
 
-sub cleanup_and_return {
+sub failcode {
-    my ($self, $return_val) = @_;
+    my ($self) = @_;
-
-    $self->db->unlock;
-    return $return_val if defined $return_val;    # explicit override
     return DECLINED
       if defined $self->{_args}{reject} && !$self->{_args}{reject};
     return DENYSOFT, $DENYMSG;
 }
 
+sub errcode {
+    my ($self, $err) = @_;
+    $self->log(LOGERROR, "UNABLE TO OBTAIN GREYLIST RESULT:$err");
+    return DECLINED;
+}
+
 sub get_greylist_key {
     my $self   = shift;
     my $sender = shift || $self->qp->transaction->sender;

t/plugin_tests/greylisting

@@ -26,6 +26,8 @@ sub register_tests {
     $self->register_test('test_init_redis');
     $self->register_test('test_init_dbm');
     $self->register_test('test_parse_redis_server');
+    $self->register_test('test_failcode');
+    $self->register_test('test_errcode');
 }
 
 sub test_load_exclude_files {
@@ -333,19 +335,27 @@ sub rc {
 sub test_init_redis {
     my ($self) = @_;
     delete $self->{db};
-    $self->{_args}{redis} = 'bogusserverasdfqwerty.:6379';
+    $self->{_args}{redis} = 'testredis';
-    ok( ! $self->init_redis, 'init_redis() fails on bogus server' );
+    $self->init_db;
-    eval { Qpsmtpd::DB::Redis->new };
+    is( keyvals($self->db_args),
-    return if $@;
+      'class=Qpsmtpd::DB::Redis;cnx_timeout=1;'
-    $self->{_args}{redis} = 'localhost';
+      . 'name=greylist;server=testredis:6379',
-    ok( $self->init_redis, 'init_redis() succeeds when redis is up' );
+      'init_redis() sets redis args' );
+}
+
+sub keyvals {
+    my ( %h ) = @_;
+    return join ";", map { "$_=$h{$_}" } sort keys %h;
 }
 
 sub test_init_dbm {
     my ($self) = @_;
     delete $self->{db};
     delete $self->{_args}{redis};
-    ok( $self->init_db, 'init_db() works for DBM' );
+    $self->init_db;
+    is( $self->db->name, 'greylist', 'init_dbm() sets correct db name' );
+    is( $self->db->path, 't/config/greylist.dbm', 'init_dbm() sets correct path' );
+    is( ref $self->db, 'Qpsmtpd::DB::File::DBM', 'init_dbm() gives DBM object' );
 }
 
 sub test_parse_redis_server {
@@ -358,3 +368,24 @@ sub test_parse_redis_server {
         'parse_redis_server(): add default port' );
 }
 
+sub test_failcode {
+    my ($self) = @_;
+    $self->{_args}{reject} = 0;
+    is( $self->rc( $self->failcode() ), 'DECLINED',
+      'failcode(): reject disabled' );
+    delete $self->{_args}{reject};
+    is( $self->rc( $self->failcode() ), 'DENYSOFT: This mail is temporarily denied',
+      'failcode(): reject enabled' );
+}
+
+sub test_errcode {
+    my ($self) = @_;
+    delete $self->qp->{_logged};
+    is( $self->rc( $self->errcode('test error') ),
+      'DECLINED',
+      'errcode(): proper return code' );
+    is( join("\n", @{ $self->qp->{_logged} || [] }),
+      'LOGERROR:greylistingUNABLE TO OBTAIN GREYLIST RESULT:test error',
+      'errocode(): logged error' );
+}
+

t/qpsmtpd-db-file-dbm.t

@@ -19,6 +19,7 @@ __get_keys();
 __size();
 __flush();
 __qphome();
+__candidate_dirs();
 __validate_dir();
 __dir();
 __untie_gotcha();
@@ -108,6 +109,15 @@ sub __qphome {
     is( $db->qphome, 't', 'qphome()' );
 }
 
+sub __candidate_dirs {
+    is( join('|', $db->candidate_dirs), 't/var/db|t/config',
+      'candidate_dirs() default ' );
+    is( join('|', $db->candidate_dirs('foo')), 'foo|t/var/db|t/config',
+      'candidate_dirs() passed args plus defaults' );
+    is( join('|', $db->candidate_dirs), 'foo|t/var/db|t/config',
+      'candidate_dirs() cached values' );
+}
+
 sub __validate_dir {
     is( $db->validate_dir(),      0, 'validate_dir(): false on no input' );
     is( $db->validate_dir(undef), 0, 'validate_dir(): false on undef' );

t/qpsmtpd-plugin.t

@@ -9,11 +9,42 @@ use Test::Qpsmtpd;
 
 use_ok('Qpsmtpd::Plugin');
 
+__validate_db_args();
+__db_args();
 __db();
 __register_hook();
 
 done_testing();
 
+sub __validate_db_args {
+    my $plugin = FakePlugin->new;
+    eval { $plugin->validate_db_args($plugin, testkey => 1) };
+    is( $@, '', 'validate_db_args() does not die on valid data' );
+    eval { $plugin->validate_db_args($plugin, 'bogus') };
+    is( $@, "Invalid db arguments\n", 'validate_db_args() dies on invalid data' );
+}
+
+sub __db_args {
+    my $plugin = FakePlugin->new;
+    is( keyvals($plugin->db_args),
+      'cnx_timeout=1;name=___MockHook___',
+      'default db args populated' );
+    is( keyvals($plugin->db_args( arg1 => 1 )),
+      'arg1=1;cnx_timeout=1;name=___MockHook___',
+      'passed args in addition to defaults' );
+    is( keyvals($plugin->db_args( name => 'bob', arg2 => 2 )),
+      'arg2=2;cnx_timeout=1;name=bob',
+      'passed args override defaults' );
+    is( keyvals($plugin->db_args),
+      'arg2=2;cnx_timeout=1;name=bob',
+      'get previous args' );
+}
+
+sub keyvals {
+    my ( %h ) = @_;
+    return join ";", map { "$_=$h{$_}" } sort keys %h;
+}
+
 sub __db {
     my $plugin = FakePlugin->new;
     my $db = $plugin->db( class => 'FakeDB', name => 'testfoo' );