From 4c9bcc0ee4233575f3276368845b3f055bafa641 Mon Sep 17 00:00:00 2001
From: Jared Johnson <jjohnson@efolder.net>
Date: Wed, 18 Feb 2015 10:25:22 -0600
Subject: [PATCH 1/4] Configure but don't connect to DB in init_db()

This allows us to start up QP even if e.g. Redis is down
---
 lib/Qpsmtpd/DB/File/DBM.pm | 16 ++++++++++++----
 lib/Qpsmtpd/Plugin.pm      | 10 ++++++++--
 plugins/greylisting        | 35 ++++++++++++++++-------------------
 t/plugin_tests/greylisting | 21 ++++++++++++++-------
 t/qpsmtpd-db-file-dbm.t    | 10 ++++++++++
 t/qpsmtpd-plugin.t         | 22 ++++++++++++++++++++++
 6 files changed, 82 insertions(+), 32 deletions(-)

diff --git a/lib/Qpsmtpd/DB/File/DBM.pm b/lib/Qpsmtpd/DB/File/DBM.pm
index abedb83..c038618 100644
--- a/lib/Qpsmtpd/DB/File/DBM.pm
+++ b/lib/Qpsmtpd/DB/File/DBM.pm
@@ -180,15 +180,23 @@ sub flush {
 }
 
 sub dir {
-    my ( $self, @candidate_dirs ) = @_;
-    return $self->{dir} if $self->{dir} and ! @candidate_dirs;
-    push @candidate_dirs, ( $self->qphome . '/var/db', $self->qphome . '/config' );
-    for my $d ( @candidate_dirs ) {
+    my ( $self, @arg ) = @_;
+    return $self->{dir} if $self->{dir} and ! @arg;
+    for my $d ( $self->candidate_dirs(@arg) ) {
         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;
diff --git a/lib/Qpsmtpd/Plugin.pm b/lib/Qpsmtpd/Plugin.pm
index a92ca78..dd8ccf4 100644
--- a/lib/Qpsmtpd/Plugin.pm
+++ b/lib/Qpsmtpd/Plugin.pm
@@ -347,10 +347,16 @@ sub _register_standard_hooks {
     }
 }
 
+sub db_args {
+    my ( $self, %arg ) = @_;
+    $self->{db_args} = \%arg if %arg;
+    $self->{db_args}{name} ||= $self->plugin_name;
+    return %{ $self->{db_args} };
+}
+
 sub db {
     my ( $self, %arg ) = @_;
-    $arg{name} ||= $self->plugin_name;
-    return $self->{db} ||= Qpsmtpd::DB->new(%arg);
+    return $self->{db} ||= Qpsmtpd::DB->new( $self->db_args(%arg) );
 }
 
 1;
diff --git a/plugins/greylisting b/plugins/greylisting
index e727c8c..76b25bb 100644
--- a/plugins/greylisting
+++ b/plugins/greylisting
@@ -211,7 +211,7 @@ 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();
     if ($self->{_args}{upgrade}) {
@@ -233,22 +233,20 @@ sub register_hooks {
 
 sub init_db {
     my ($self) = @_;
-    return $self->init_redis if $self->{_args}{redis};
-    return $self->init_dbm;
+    if ( $self->{_args}{redis} ) {
+        $self->init_redis;
+    } else {
+        $self->init_dbm;
+    }
 }
 
 sub init_redis {
     my ($self) = @_;
-    eval {
-        $self->db(
-            name   => 'greylist',
-            class  => 'Qpsmtpd::DB::Redis',
-            server => $self->parse_redis_server,
-        ) or die 'Unknown error';
-    };
-    return 1 if ! $@;
-    $self->log(LOGCRIT, "Unable to connect to redis, GREYLISTING DISABLED: $@");
-    return 0;
+    $self->db_args(
+        name   => 'greylist',
+        class  => 'Qpsmtpd::DB::Redis',
+        server => $self->parse_redis_server,
+    );
 }
 
 sub parse_redis_server {
@@ -263,19 +261,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";
-    $self->db->name($oldname);
-    return 1;
+    if ( ! -f "$db_dir/greylist.dbm" && -f "$db_dir/$oldname.dbm" ) {
+        $self->db->name($oldname);
+    }
 }
 
 sub load_exclude_files {
diff --git a/t/plugin_tests/greylisting b/t/plugin_tests/greylisting
index 7fd2a84..db96f73 100644
--- a/t/plugin_tests/greylisting
+++ b/t/plugin_tests/greylisting
@@ -333,19 +333,26 @@ sub rc {
 sub test_init_redis {
     my ($self) = @_;
     delete $self->{db};
-    $self->{_args}{redis} = 'bogusserverasdfqwerty.:6379';
-    ok( ! $self->init_redis, 'init_redis() fails on bogus server' );
-    eval { Qpsmtpd::DB::Redis->new };
-    return if $@;
-    $self->{_args}{redis} = 'localhost';
-    ok( $self->init_redis, 'init_redis() succeeds when redis is up' );
+    $self->{_args}{redis} = 'testredis';
+    $self->init_db;
+    is( keyvals($self->db_args),
+      'class=Qpsmtpd::DB::Redis;name=greylist;server=testredis:6379',
+      '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 {
diff --git a/t/qpsmtpd-db-file-dbm.t b/t/qpsmtpd-db-file-dbm.t
index 38f052d..58a94a7 100644
--- a/t/qpsmtpd-db-file-dbm.t
+++ b/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' );
diff --git a/t/qpsmtpd-plugin.t b/t/qpsmtpd-plugin.t
index 7d9c605..a6596ca 100644
--- a/t/qpsmtpd-plugin.t
+++ b/t/qpsmtpd-plugin.t
@@ -9,11 +9,33 @@ use Test::Qpsmtpd;
 
 use_ok('Qpsmtpd::Plugin');
 
+__db_args();
 __db();
 __register_hook();
 
 done_testing();
 
+sub __db_args {
+    my $plugin = FakePlugin->new;
+    is( keyvals($plugin->db_args),
+      'name=___MockHook___',
+      'default db args populated' );
+    is( keyvals($plugin->db_args( arg1 => 1 )),
+      'arg1=1;name=___MockHook___',
+      'passed args in addition to defaults' );
+    is( keyvals($plugin->db_args( name => 'bob', arg2 => 2 )),
+      'arg2=2;name=bob',
+      'passed args override defaults' );
+    is( keyvals($plugin->db_args),
+      'arg2=2;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' );

From 51ca3fcda42d13471dbaccba612bf08e29b60e4d Mon Sep 17 00:00:00 2001
From: Jared Johnson <jjohnson@efolder.net>
Date: Thu, 19 Feb 2015 13:20:14 -0600
Subject: [PATCH 2/4] Skip greylisting when we can't talk to greylist DB

---
 plugins/greylisting        | 37 +++++++++++++++++++++++++------------
 t/plugin_tests/greylisting | 23 +++++++++++++++++++++++
 2 files changed, 48 insertions(+), 12 deletions(-)

diff --git a/plugins/greylisting b/plugins/greylisting
index 76b25bb..deaa044 100644
--- a/plugins/greylisting
+++ b/plugins/greylisting
@@ -213,7 +213,8 @@ sub register {
     $self->{_args} = $config;
     $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();
     }
@@ -383,16 +384,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;
@@ -404,7 +414,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)");
@@ -416,26 +426,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 {
-    my ($self, $return_val) = @_;
-
-    $self->db->unlock;
-    return $return_val if defined $return_val;    # explicit override
+sub failcode {
+    my ($self) = @_;
     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;
diff --git a/t/plugin_tests/greylisting b/t/plugin_tests/greylisting
index db96f73..e5b93a9 100644
--- a/t/plugin_tests/greylisting
+++ b/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 {
@@ -365,3 +367,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' );
+}
+

From 42c551944e7dcb146f73ca2462ccfe04e15b2a84 Mon Sep 17 00:00:00 2001
From: Jared Johnson <jjohnson@efolder.net>
Date: Mon, 23 Feb 2015 14:13:39 -0600
Subject: [PATCH 3/4] Add some validation for passed db args

---
 lib/Qpsmtpd/Plugin.pm | 7 +++++++
 t/qpsmtpd-plugin.t    | 9 +++++++++
 2 files changed, 16 insertions(+)

diff --git a/lib/Qpsmtpd/Plugin.pm b/lib/Qpsmtpd/Plugin.pm
index dd8ccf4..379031b 100644
--- a/lib/Qpsmtpd/Plugin.pm
+++ b/lib/Qpsmtpd/Plugin.pm
@@ -349,6 +349,7 @@ 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;
     return %{ $self->{db_args} };
@@ -356,7 +357,13 @@ sub db_args {
 
 sub db {
     my ( $self, %arg ) = @_;
+    $self->validate_db_args(@_);
     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;
diff --git a/t/qpsmtpd-plugin.t b/t/qpsmtpd-plugin.t
index a6596ca..2373c0e 100644
--- a/t/qpsmtpd-plugin.t
+++ b/t/qpsmtpd-plugin.t
@@ -9,12 +9,21 @@ 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),

From 15a297372dab411df3de4f302f890a77791ca0c2 Mon Sep 17 00:00:00 2001
From: Jared Johnson <jjohnson@efolder.net>
Date: Mon, 23 Feb 2015 14:32:42 -0600
Subject: [PATCH 4/4] Default to one-second connect timeout for Redis

---
 lib/Qpsmtpd/Plugin.pm      | 1 +
 plugins/greylisting        | 3 +++
 t/plugin_tests/greylisting | 3 ++-
 t/qpsmtpd-plugin.t         | 8 ++++----
 4 files changed, 10 insertions(+), 5 deletions(-)

diff --git a/lib/Qpsmtpd/Plugin.pm b/lib/Qpsmtpd/Plugin.pm
index 379031b..d339696 100644
--- a/lib/Qpsmtpd/Plugin.pm
+++ b/lib/Qpsmtpd/Plugin.pm
@@ -352,6 +352,7 @@ sub db_args {
     $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} };
 }
 
diff --git a/plugins/greylisting b/plugins/greylisting
index deaa044..12926a2 100644
--- a/plugins/greylisting
+++ b/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.
diff --git a/t/plugin_tests/greylisting b/t/plugin_tests/greylisting
index e5b93a9..d4b32b5 100644
--- a/t/plugin_tests/greylisting
+++ b/t/plugin_tests/greylisting
@@ -338,7 +338,8 @@ sub test_init_redis {
     $self->{_args}{redis} = 'testredis';
     $self->init_db;
     is( keyvals($self->db_args),
-      'class=Qpsmtpd::DB::Redis;name=greylist;server=testredis:6379',
+      'class=Qpsmtpd::DB::Redis;cnx_timeout=1;'
+      . 'name=greylist;server=testredis:6379',
       'init_redis() sets redis args' );
 }
 
diff --git a/t/qpsmtpd-plugin.t b/t/qpsmtpd-plugin.t
index 2373c0e..dd08d6f 100644
--- a/t/qpsmtpd-plugin.t
+++ b/t/qpsmtpd-plugin.t
@@ -27,16 +27,16 @@ sub __validate_db_args {
 sub __db_args {
     my $plugin = FakePlugin->new;
     is( keyvals($plugin->db_args),
-      'name=___MockHook___',
+      'cnx_timeout=1;name=___MockHook___',
       'default db args populated' );
     is( keyvals($plugin->db_args( arg1 => 1 )),
-      'arg1=1;name=___MockHook___',
+      'arg1=1;cnx_timeout=1;name=___MockHook___',
       'passed args in addition to defaults' );
     is( keyvals($plugin->db_args( name => 'bob', arg2 => 2 )),
-      'arg2=2;name=bob',
+      'arg2=2;cnx_timeout=1;name=bob',
       'passed args override defaults' );
     is( keyvals($plugin->db_args),
-      'arg2=2;name=bob',
+      'arg2=2;cnx_timeout=1;name=bob',
       'get previous args' );
 }