From 8185d33fa5e564fa0cc08fc74c7fcbc96f7f616e Mon Sep 17 00:00:00 2001
From: Matt Simerson <matt@tnpi.net>
Date: Wed, 21 Jan 2015 10:09:37 -0800
Subject: [PATCH] dmarc: add error handling and tests

---
 plugins/dmarc        | 19 ++++++-------
 t/plugin_tests/dmarc | 66 ++++++++++++++++----------------------------
 2 files changed, 32 insertions(+), 53 deletions(-)

diff --git a/plugins/dmarc b/plugins/dmarc
index 196f76f..025604e 100644
--- a/plugins/dmarc
+++ b/plugins/dmarc
@@ -72,6 +72,7 @@ https://github.com/smtpd/qpsmtpd/wiki/DMARC-FAQ
 use strict;
 use warnings;
 
+use English qw/-no_match_vars/;
 use Qpsmtpd::Constants;
 
 sub register {
@@ -90,16 +91,16 @@ sub register {
     }
     else {
         $self->{_dmarc} = Mail::DMARC::PurePerl->new();
-        $self->register_hook('data_post_headers', 'data_post_handler');
+        $self->register_hook('data_post_headers', 'check_dmarc');
     };
 }
 
-sub data_post_handler {
+sub check_dmarc {
     my ($self, $transaction) = @_;
 
     if ( $self->qp->connection->relay_client() ) {
         $self->log(LOGINFO, "skip, relay client" );
-        return DECLINED;  # disable reporting to ourself
+        return DECLINED;  # don't report to ourself
     };
 
     my $dmarc = $self->{_dmarc};
@@ -117,19 +118,15 @@ sub data_post_handler {
     my @recipients = $transaction->recipients;
     eval { $dmarc->envelope_to( lc $recipients[0]->host ); }; # optional
     eval { $dmarc->envelope_from( $transaction->sender->host ); }; # may be <>
-    $dmarc->spf( $transaction->notes('dmarc_spf') );
+    eval { $dmarc->spf( $transaction->notes('dmarc_spf') ); };
     my $dkim = $self->connection->notes('dkim_verifier');
-    if ( $dkim ) {
-        eval { $dmarc->dkim( $dkim ); }
-    };
+    if ( $dkim ) { eval { $dmarc->dkim( $dkim ); }; };
     $dmarc->source_ip( $self->qp->connection->remote_ip );
     eval { $dmarc->validate(); };
-    if ( $@ ) {
+    if ( $EVAL_ERROR ) {
         $self->log(LOGERROR, $@ );
         return DECLINED if $self->is_immune;
-        $self->log(LOGINFO, "TODO: handle this validation failure");
-        return DECLINED;
-        return $self->get_reject( $@, $@ );
+        return $self->get_reject( $@ );
     };
 
 #$self->log(LOGINFO, "result: " . Dumper( $dmarc ) );
diff --git a/t/plugin_tests/dmarc b/t/plugin_tests/dmarc
index da20326..58c0909 100644
--- a/t/plugin_tests/dmarc
+++ b/t/plugin_tests/dmarc
@@ -1,64 +1,46 @@
 #!perl -w
 
 use strict;
+use English qw/-no_match_vars/;
 use POSIX qw(strftime);
 
 use Qpsmtpd::Address;
 use Qpsmtpd::Constants;
 
+my $remote_ip = '66.128.51.165';
 my $test_email = 'matt@tnpi.net';
 
 sub register_tests {
     my $self = shift;
 
-# TODO: test against newer DMARC plugin that uses Mail::DMARC
-}
-
-sub setup_test_headers {
-    my $self = shift;
-
-    my $transaction = $self->qp->transaction;
-    my $address = Qpsmtpd::Address->new( "<$test_email>" );
-    my $header  = Mail::Header->new(Modify => 0, MailFrom => "COERCE");
-    my $now    = strftime "%a %b %e %H:%M:%S %Y", localtime time;
-
-    $transaction->sender($address);
-    $transaction->header($header);
-    $transaction->header->add('From', "<$test_email>");
-    $transaction->header->add('Date', $now );
-    $transaction->body_write( "test message body " );
-
-    $self->qp->connection->relay_client(0);
-}
-
-sub test_fetch_dmarc_record {
-    my $self = shift;
-    
-    foreach ( qw/ tnpi.net nictool.com / ) {
-        my @matches = $self->fetch_dmarc_record($_);
-        cmp_ok( scalar @matches, '==', 1, 'fetch_dmarc_record');
-    }
-    foreach ( qw/ example.com / ) {
-        my @matches = $self->fetch_dmarc_record($_);
-        cmp_ok( scalar @matches, '==', 0, 'fetch_dmarc_record');
+    eval 'use Mail::DMARC';
+    if ($EVAL_ERROR) {
+        warn 'unable to load Mail::DMARC';
+        return;
     }
+
+    $self->register_test('_check_dmarc');
 }
 
-sub test_get_organizational_domain {
+sub _check_dmarc {
     my $self = shift;
 
-    $self->setup_test_headers();
-    my $transaction = $self->qp->transaction;
+    $self->qp->connection->remote_ip($remote_ip);
+    my $t = $self->qp->transaction;
+    $t->header(Mail::Header->new(Modify => 0, MailFrom => "COERCE"));
+    $t->sender(Qpsmtpd::Address->new( "<$test_email>" ));
+    $t->header->add('Date', strftime "%a %b %e %H:%M:%S %Y", localtime time);
+    $t->body_write( "test message body " );
 
-    cmp_ok( $self->get_organizational_domain('test.www.tnpi.net'), 'eq', 'tnpi.net' );
-    cmp_ok( $self->get_organizational_domain('www.example.co.uk'), 'eq', 'example.co.uk' );
-    cmp_ok( $self->get_organizational_domain('plus.google.com'),   'eq', 'google.com' );
-}
+    # no From header, reject as invalid message
+    my ($rc, $msg) = $self->check_dmarc($t);
+    cmp_ok($rc, '==', DENY, "no From header, $msg");
 
-sub test_discover_policy {
-    my $self = shift;
 
-    $self->setup_test_headers();
+    $t->header->add('From', "<$test_email>");
+    ($rc, $msg) = $self->check_dmarc($t);
+    cmp_ok($rc, '==', DENY, "$msg");
+    cmp_ok($msg, 'eq', 'failed DMARC policy', 'check_dmarc, no SPF');
 
-    ok( $self->discover_policy( 'tnpi.net' ), 'discover_policy' );
-}
+    #warn $self->qp->connection->notes('authentication_results');
+}
\ No newline at end of file