From 4f9af75e48d3a8ee3ece8f6e836858e31910ed1a Mon Sep 17 00:00:00 2001 From: Jared Johnson Date: Fri, 23 Jan 2015 03:08:07 -0600 Subject: [PATCH 1/3] Make Redis optional Previously the greylist plugin tried to use redis and fell back to DBM. This means that if a system already had an established DBM database, but happened to have Redis running, the existing DBM db would be abandoned for a new Redis DB. This would inevitably lead to more delays for legitimate mail, and possibly lost mail. This adds a 'redis_server' argument which enables Redis and sets the location of the redis server; if it is not explicitly set, DBM is used instead. If the redis server is unavailable, rather than failing to start QP, we instead fail to register the plugin. --- plugins/greylisting | 73 ++++++++++++++++++++++++++++++-------- t/plugin_tests/greylisting | 43 ++++++++++++++++++---- 2 files changed, 95 insertions(+), 21 deletions(-) diff --git a/plugins/greylisting b/plugins/greylisting index 86d81b1..8dcf3f5 100644 --- a/plugins/greylisting +++ b/plugins/greylisting @@ -104,6 +104,13 @@ usable directory from the following list will be used: =back +=head2 redis_server + +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 + =head2 per_recipient Flag to indicate whether to use per-recipient configs. @@ -172,7 +179,7 @@ my $VERSION = '0.12'; my $DENYMSG = "This mail is temporarily denied"; my %PERMITTED_ARGS = map { $_ => 1 } qw(per_recipient remote_ip sender - recipient black_timeout white_timeout deny_late db_dir + recipient black_timeout white_timeout deny_late db_dir redis_server nfslock p0f reject loglevel geoip upgrade ); $PERMITTED_ARGS{grey_timeout} = 1; # Legacy argument now ignored @@ -204,13 +211,8 @@ sub register { $config->{reject} = $config->{mode} =~ /testonly|off/i ? 0 : 1; } $self->{_args} = $config; - unless ($config->{recipient} || $config->{per_recipient}) { - $self->register_hook('mail', 'mail_handler'); - } - else { - $self->register_hook('rcpt', 'rcpt_handler'); - } - $self->init_db(); + $self->init_db() or return; + $self->register_hooks(); $self->prune_db(); if ($self->{_args}{upgrade}) { $self->convert_db(); @@ -218,23 +220,64 @@ sub register { $self->load_exclude_files(); } +sub register_hooks { + my ($self) = @_; + $self->register_hook('data', 'data_handler'); + if ($self->{_args}{recipient} || $self->{_args}{per_recipient}) { + $self->register_hook('rcpt', 'rcpt_handler'); + } + else { + $self->register_hook('mail', 'mail_handler'); + } +} + sub init_db { - my ( $self ) = @_; - $self->db( name => 'greylist' ); - return if ! $self->db->can('path'); + my ($self) = @_; + return $self->init_redis if $self->{_args}{redis_server}; + return $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(LOGERROR, "Unable to connect to redis store: $@"); + return 0; +} + +sub parse_redis_server { + my ($self) = @_; + my $server = $self->{_args}{redis_server}; + return $server if $server =~ /:/; + return "$server:6379"; +} + +sub init_dbm { + my ($self) = @_; + $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' ); - return if $self->db->file_extension ne '.dbm'; + return 1 if $self->db->file_extension ne '.dbm'; $self->db->nfs_locking( $self->{_args}{nfslock} ); # Work around old DBM filename - return if -f "$db_dir/greylist.dbm"; + return 1 if -f "$db_dir/greylist.dbm"; my $oldname = 'denysoft_greylist'; - return if ! -f "$db_dir/$oldname.dbm"; + return 1 if ! -f "$db_dir/$oldname.dbm"; $self->db->name($oldname); + return 1; } sub load_exclude_files { @@ -319,7 +362,7 @@ sub rcpt_handler { return DECLINED; } -sub hook_data { +sub data_handler { my ($self, $transaction) = @_; return DECLINED unless $transaction->notes('greylist'); diff --git a/t/plugin_tests/greylisting b/t/plugin_tests/greylisting index 16d4af8..b75f01f 100644 --- a/t/plugin_tests/greylisting +++ b/t/plugin_tests/greylisting @@ -12,7 +12,7 @@ sub register_tests { my $self = shift; $self->register_test("test_load_exclude_files"); - $self->register_test('test_hook_data'); + $self->register_test('test_data_handler'); $self->register_test('test_get_greylist_key'); $self->register_test('test_exclude'); $self->register_test("test_greylist_geoip"); @@ -22,6 +22,9 @@ sub register_tests { $self->register_test("test_greylist_p0f_uptime"); $self->register_test('test_exclude_file_match'); $self->register_test('test_greylist'); + $self->register_test('test_init_redis'); + $self->register_test('test_init_dbm'); + $self->register_test('test_parse_redis_server'); } sub test_load_exclude_files { @@ -71,27 +74,27 @@ sub test_exclude_file_match { } } -sub test_hook_data { +sub test_data_handler { my $self = shift; my $transaction = $self->qp->transaction; - my ($code, $mess) = $self->hook_data( $transaction ); + my ($code, $mess) = $self->data_handler( $transaction ); cmp_ok( $code, '==', DECLINED, "no note" ); $transaction->notes('greylist', 1); - ($code, $mess) = $self->hook_data( $transaction ); + ($code, $mess) = $self->data_handler( $transaction ); cmp_ok( $code, '==', DECLINED, "no recipients"); my $address = Qpsmtpd::Address->new( "<$test_email>" ); $transaction->recipients( $address ); $transaction->notes('whitelistrcpt', 2); - ($code, $mess) = $self->hook_data( $transaction ); + ($code, $mess) = $self->data_handler( $transaction ); cmp_ok( $code, '==', DENYSOFT, "missing recipients"); $transaction->notes('whitelistrcpt', 1); - ($code, $mess) = $self->hook_data( $transaction ); + ($code, $mess) = $self->data_handler( $transaction ); cmp_ok( $code, '==', DECLINED, "missing recipients"); } @@ -279,3 +282,31 @@ sub rc { return return_code($r) . ": $msg"; } +sub test_init_redis { + my ($self) = @_; + delete $self->{db}; + $self->{_args}{redis_server} = 'bogusserverasdfqwerty.:6379'; + ok( ! $self->init_redis, 'init_redis() fails on bogus server' ); + eval { Qpsmtpd::DB::Redis->new }; + return if $@; + $self->{_args}{redis_server} = 'localhost'; + ok( $self->init_redis, 'init_redis() succeeds when redis is up' ); +} + +sub test_init_dbm { + my ($self) = @_; + delete $self->{db}; + delete $self->{_args}{redis_server}; + ok( $self->init_db, 'init_db() works for DBM' ); +} + +sub test_parse_redis_server { + my ($self) = @_; + $self->{_args}{redis_server} = 'asdf:1234'; + is( $self->parse_redis_server, 'asdf:1234', + 'parse_redis_server(): leave provided port alone' ); + $self->{_args}{redis_server} = 'qwerty'; + is( $self->parse_redis_server, 'qwerty:6379', + 'parse_redis_server(): add default port' ); +} + From 462a2ae3672ba56d0c506f86cff2a2eb66e154a2 Mon Sep 17 00:00:00 2001 From: Jared Johnson Date: Fri, 23 Jan 2015 15:13:56 -0600 Subject: [PATCH 2/3] Rename 'redis_server' arg to 'redis' --- plugins/greylisting | 8 ++++---- t/plugin_tests/greylisting | 10 +++++----- 2 files changed, 9 insertions(+), 9 deletions(-) diff --git a/plugins/greylisting b/plugins/greylisting index 8dcf3f5..20e6a58 100644 --- a/plugins/greylisting +++ b/plugins/greylisting @@ -104,7 +104,7 @@ usable directory from the following list will be used: =back -=head2 redis_server +=head2 redis Location of redis server where the greylisting DB will be stored. @@ -179,7 +179,7 @@ my $VERSION = '0.12'; my $DENYMSG = "This mail is temporarily denied"; my %PERMITTED_ARGS = map { $_ => 1 } qw(per_recipient remote_ip sender - recipient black_timeout white_timeout deny_late db_dir redis_server + recipient black_timeout white_timeout deny_late db_dir redis nfslock p0f reject loglevel geoip upgrade ); $PERMITTED_ARGS{grey_timeout} = 1; # Legacy argument now ignored @@ -233,7 +233,7 @@ sub register_hooks { sub init_db { my ($self) = @_; - return $self->init_redis if $self->{_args}{redis_server}; + return $self->init_redis if $self->{_args}{redis}; return $self->init_dbm; } @@ -253,7 +253,7 @@ sub init_redis { sub parse_redis_server { my ($self) = @_; - my $server = $self->{_args}{redis_server}; + my $server = $self->{_args}{redis}; return $server if $server =~ /:/; return "$server:6379"; } diff --git a/t/plugin_tests/greylisting b/t/plugin_tests/greylisting index b75f01f..7737fd3 100644 --- a/t/plugin_tests/greylisting +++ b/t/plugin_tests/greylisting @@ -285,27 +285,27 @@ sub rc { sub test_init_redis { my ($self) = @_; delete $self->{db}; - $self->{_args}{redis_server} = 'bogusserverasdfqwerty.:6379'; + $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_server} = 'localhost'; + $self->{_args}{redis} = 'localhost'; ok( $self->init_redis, 'init_redis() succeeds when redis is up' ); } sub test_init_dbm { my ($self) = @_; delete $self->{db}; - delete $self->{_args}{redis_server}; + delete $self->{_args}{redis}; ok( $self->init_db, 'init_db() works for DBM' ); } sub test_parse_redis_server { my ($self) = @_; - $self->{_args}{redis_server} = 'asdf:1234'; + $self->{_args}{redis} = 'asdf:1234'; is( $self->parse_redis_server, 'asdf:1234', 'parse_redis_server(): leave provided port alone' ); - $self->{_args}{redis_server} = 'qwerty'; + $self->{_args}{redis} = 'qwerty'; is( $self->parse_redis_server, 'qwerty:6379', 'parse_redis_server(): add default port' ); } From 5a58e02e8025ecaf1a690bfed110d8c9a5a2436a Mon Sep 17 00:00:00 2001 From: Jared Johnson Date: Fri, 23 Jan 2015 15:23:18 -0600 Subject: [PATCH 3/3] Scarier error message for redis failures --- plugins/greylisting | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/plugins/greylisting b/plugins/greylisting index 20e6a58..8ba565c 100644 --- a/plugins/greylisting +++ b/plugins/greylisting @@ -247,7 +247,7 @@ sub init_redis { ) or die 'Unknown error'; }; return 1 if ! $@; - $self->log(LOGERROR, "Unable to connect to redis store: $@"); + $self->log(LOGCRIT, "Unable to connect to redis, GREYLISTING DISABLED: $@"); return 0; }