From 4f9af75e48d3a8ee3ece8f6e836858e31910ed1a Mon Sep 17 00:00:00 2001 From: Jared Johnson Date: Fri, 23 Jan 2015 03:08:07 -0600 Subject: [PATCH] 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' ); +} +