[pmg-devel] [PATCH pmg-api 2/5] Change greylisting table to use 'inet' for ips

Stoiko Ivanov s.ivanov at proxmox.com
Wed Apr 15 12:53:15 CEST 2020


In preparation for adding support for a configurable greylist netmask [0]
and for adding support for greylisting of ipv6 hosts the definition of the
greylist table needs to be adapted.

Instead of saving the 'ipnet' (first 3 octets of the ipv4 address as text)
and the 'host' (as integer) in 2 separate columns, the complete ip is stored
as inet type (which supports both ipv4 and ipv6 addresses - see [1]).
Comparison for the greylist decision is done with the ip with an added
netmask of 24 and the inet contains operator (see [2]).

In order to maintain backward compatibility within a cluster (the sync of
the greylist table goes from all nodes to all other nodes) - a new table
'greylist' is created, and the data is appropriately copied upon package
upgrade (`pmgdb init` in the postinst script). The original 'cgreylist'
table is dropped and instead a view is created backed the new table.

which database layout is found on a remote node is decided based on wheter
'cgreylist' is a regular table instead of a view (see [3]).

The change of semantics makes it possible that 2 ips from the same network
are stored (when the 2 ips send a mail to 2 different clusternodes, within
one syncintervall) - however this does not change the outcome of a lookup.
The potential for a denial of service is limited by the number of nodes in
the cluster.

[0] defining from which neighbors a mail is accepted on the second attempt
[1] https://www.postgresql.org/docs/11/datatype-net-types.html
[2] https://www.postgresql.org/docs/11/functions-net.html
[3] https://www.postgresql.org/docs/11/infoschema-tables.html

Signed-off-by: Stoiko Ivanov <s.ivanov at proxmox.com>
---
 src/PMG/Cluster.pm         | 30 +++++++++----
 src/PMG/DBTools.pm         | 86 ++++++++++++++++++++++++++++----------
 src/bin/pmgpolicy          | 56 +++++++++++++------------
 src/tests/test_greylist.pl |  2 +-
 4 files changed, 116 insertions(+), 58 deletions(-)

diff --git a/src/PMG/Cluster.pm b/src/PMG/Cluster.pm
index e890924..9f39516 100644
--- a/src/PMG/Cluster.pm
+++ b/src/PMG/Cluster.pm
@@ -757,23 +757,37 @@ sub sync_localstat_db {
 sub sync_greylist_db {
     my ($dbh, $rdb, $ni) = @_;
 
-    my $selectfunc = sub {
-	my ($ctime, $lastmt) = @_;
-	return "SELECT * from CGreylist WHERE extime >= $ctime AND " .
-	    "mtime >= $lastmt AND CID != 0";
-    };
 
-    my $merge_sth = $dbh->prepare($PMG::DBTools::cgreylist_merge_sql);
+    my $selectfunc;
+    if (PMG::DBTools::database_table_exists($rdb, 'CGreylist')) {
+	$selectfunc = sub {
+	    my ($ctime, $lastmt) = @_;
+	    return "SELECT (IPNet || '.' || Host)::inet as IPAddr, " .
+		    "Sender, Receiver, Instance, RCTime, ExTime, Delay, ".
+		    "Blocked, Passed, CID, MTime FROM CGreylist ".
+		    "WHERE extime >= $ctime AND mtime >= $lastmt AND CID != 0";
+	};
+	syslog ('info', "Detected legacy database layout on remote node - please upgrade");
+    } else {
+
+	$selectfunc = sub {
+	    my ($ctime, $lastmt) = @_;
+	    return "SELECT * from Greylist WHERE extime >= $ctime AND " .
+		"mtime >= $lastmt AND CID != 0";
+	};
+    }
+    my $merge_sth = $dbh->prepare($PMG::DBTools::greylist_merge_sql);
     my $mergefunc = sub {
 	my ($ref) = @_;
 
+	my $ipaddr = defined($ref->{ipaddr}) ? $ref->{ipaddr} : "$ref->{ipnet}.$ref->{host}";
 	$merge_sth->execute(
-	    $ref->{ipnet}, $ref->{host}, $ref->{sender}, $ref->{receiver},
+	    $ipaddr, $ref->{sender}, $ref->{receiver},
 	    $ref->{instance}, $ref->{rctime}, $ref->{extime}, $ref->{delay},
 	    $ref->{blocked}, $ref->{passed}, 0, $ref->{cid});
     };
 
-    return $sync_generic_mtime_db->($dbh, $rdb, $ni, 'CGreylist', $selectfunc, $mergefunc);
+    return $sync_generic_mtime_db->($dbh, $rdb, $ni, 'Greylist', $selectfunc, $mergefunc);
 }
 
 sub sync_userprefs_db {
diff --git a/src/PMG/DBTools.pm b/src/PMG/DBTools.pm
index a7a18a4..0e89e2c 100644
--- a/src/PMG/DBTools.pm
+++ b/src/PMG/DBTools.pm
@@ -18,17 +18,16 @@ use PMG::Utils qw(postgres_admin_cmd);
 
 our $default_db_name = "Proxmox_ruledb";
 
-our $cgreylist_merge_sql =
-    'INSERT INTO CGREYLIST (IPNet,Host,Sender,Receiver,Instance,RCTime,' .
+our $greylist_merge_sql =
+    'INSERT INTO Greylist (IPAddr,Sender,Receiver,Instance,RCTime,' .
     'ExTime,Delay,Blocked,Passed,MTime,CID) ' .
-    'VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?) ' .
-    'ON CONFLICT (IPNet,Sender,Receiver) DO UPDATE SET ' .
-    'Host = CASE WHEN CGREYLIST.MTime >= excluded.MTime THEN CGREYLIST.Host ELSE excluded.Host END,' .
-    'CID = GREATEST(CGREYLIST.CID, excluded.CID), RCTime = LEAST(CGREYLIST.RCTime, excluded.RCTime),' .
-    'ExTime = GREATEST(CGREYLIST.ExTime, excluded.ExTime),' .
-    'Delay = GREATEST(CGREYLIST.Delay, excluded.Delay),' .
-    'Blocked = GREATEST(CGREYLIST.Blocked, excluded.Blocked),' .
-    'Passed = GREATEST(CGREYLIST.Passed, excluded.Passed)';
+    'VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?) ' .
+    'ON CONFLICT (IPAddr,Sender,Receiver) DO UPDATE SET ' .
+    'CID = GREATEST(Greylist.CID, excluded.CID), RCTime = LEAST(Greylist.RCTime, excluded.RCTime),' .
+    'ExTime = GREATEST(Greylist.ExTime, excluded.ExTime),' .
+    'Delay = GREATEST(Greylist.Delay, excluded.Delay),' .
+    'Blocked = GREATEST(Greylist.Blocked, excluded.Blocked),' .
+    'Passed = GREATEST(Greylist.Passed, excluded.Passed)';
 
 sub open_ruledb {
     my ($database, $host, $port) = @_;
@@ -100,10 +99,9 @@ sub database_list {
     return $database_list;
 }
 
-my $cgreylist_ctablecmd =  <<__EOD;
-    CREATE TABLE CGreylist
-    (IPNet VARCHAR(16) NOT NULL,
-     Host INTEGER NOT NULL,
+my $greylist_ctablecmd =  <<__EOD;
+    CREATE TABLE Greylist
+    (IPAddr INET NOT NULL,
      Sender VARCHAR(255) NOT NULL,
      Receiver VARCHAR(255) NOT NULL,
      Instance VARCHAR(255),
@@ -114,15 +112,25 @@ my $cgreylist_ctablecmd =  <<__EOD;
      Passed INTEGER NOT NULL,
      CID INTEGER NOT NULL,
      MTime INTEGER NOT NULL,
-     PRIMARY KEY (IPNet, Sender, Receiver));
+     PRIMARY KEY (IPAddr, Sender, Receiver));
 
-    CREATE INDEX CGreylist_Instance_Sender_Index ON CGreylist (Instance, Sender);
+    CREATE INDEX Greylist_Instance_Sender_Index ON Greylist (Instance, Sender);
 
-    CREATE INDEX CGreylist_ExTime_Index ON CGreylist (ExTime);
+    CREATE INDEX Greylist_ExTime_Index ON Greylist (ExTime);
 
-    CREATE INDEX CGreylist_MTime_Index ON CGreylist (MTime);
+    CREATE INDEX Greylist_MTime_Index ON Greylist (MTime);
 __EOD
 
+my $cgreylist_cviewcmd =
+    q{CREATE OR REPLACE VIEW CGreylist AS
+    SELECT split_ip[1] as IPNet, split_ip[2]::integer as Host, Sender,
+	Receiver, Instance, RCTime, ExTime, Delay, Blocked, Passed, Cid, Mtime
+    FROM
+	(SELECT regexp_matches(host(ipaddr), '(\d+\.\d+\.\d+)\.(\d+)$')
+	    as split_ip, * FROM Greylist
+	    WHERE (family(Greylist.IPAddr) = 4)
+	) cglsplit;};
+
 my $clusterinfo_ctablecmd =  <<__EOD;
     CREATE TABLE ClusterInfo
     (CID INTEGER NOT NULL,
@@ -318,6 +326,17 @@ sub cond_create_dbtable {
     }
 }
 
+sub database_table_exists {
+    my ($dbh, $table) = @_;
+
+    my $sth = $dbh->prepare(
+	"SELECT table_name FROM information_schema.tables " .
+	"WHERE table_name = ? and table_type = 'BASE TABLE'");
+    $sth->execute(lc($table));
+    my $res = $sth->fetchrow_hashref();
+    return defined($res);
+}
+
 sub database_column_exists {
     my ($dbh, $table, $column) = @_;
 
@@ -402,7 +421,9 @@ sub create_ruledb {
 	       Grouptype INTEGER NOT NULL,
 	       PRIMARY KEY (Objectgroup_ID, Rule_ID, Grouptype));
 
-	      $cgreylist_ctablecmd;
+	      $greylist_ctablecmd;
+
+	      $cgreylist_cviewcmd;
 
 	      $clusterinfo_ctablecmd;
 
@@ -473,7 +494,7 @@ sub upgradedb {
 	'StatInfo', $statinfo_ctablecmd,
 	'CMailStore', $cmailstore_ctablecmd,
 	'UserPrefs', $userprefs_ctablecmd,
-	'CGreylist', $cgreylist_ctablecmd,
+	'Greylist', $greylist_ctablecmd,
 	'CStatistic', $cstatistic_ctablecmd,
 	'ClusterInfo', $clusterinfo_ctablecmd,
 	'VirusInfo', $virusinfo_stat_ctablecmd,
@@ -522,6 +543,25 @@ sub upgradedb {
 		 "AND value = 'content-type:application/x-java-vm';");
     };
 
+    # move data from legacy CGreylist table and create compatibility view
+    if (database_table_exists($dbh, 'CGreylist')) {
+	eval {
+	    $dbh->begin_work;
+	    $dbh->do("INSERT INTO Greylist ".
+		    "SELECT (IPNet || '.' || Host)::inet as IPAddr, " .
+		    "Sender, Receiver, Instance, RCTime, ExTime, Delay, ".
+		    "Blocked, Passed, CID, MTime ".
+		    "FROM CGreylist ");
+	    $dbh->do("DROP TABLE CGreylist");
+	    $dbh->do($cgreylist_cviewcmd);
+	    $dbh->commit;
+	};
+	if (my $err = $@) {
+	    $dbh->rollback;
+	    die $err;
+	}
+    }
+
     foreach my $table (keys %$tables) {
 	eval { $dbh->do("ANALYZE $table"); };
 	warn $@ if $@;
@@ -885,7 +925,7 @@ sub init_masterdb {
 		  "UPDATE CReceivers SET CStatistic_CID = $lcid WHERE CStatistic_CID = 0;");
 
 	print STDERR "update greylist database\n";
-	$dbh->do ("UPDATE CGreylist SET CID = $lcid WHERE CID = 0;");
+	$dbh->do ("UPDATE Greylist SET CID = $lcid WHERE CID = 0;");
 
 	print STDERR "update localstat database\n";
 	$dbh->do ("UPDATE LocalStat SET CID = $lcid WHERE CID = 0;");
@@ -1059,7 +1099,7 @@ sub update_master_clusterinfo {
 
     $dbh->do("DELETE FROM ClusterInfo WHERE CID = $clientcid");
 
-    my @mt = ('CMSReceivers', 'CGreylist', 'UserPrefs', 'DomainStat', 'DailyStat', 'LocalStat', 'VirusInfo');
+    my @mt = ('CMSReceivers', 'Greylist', 'UserPrefs', 'DomainStat', 'DailyStat', 'LocalStat', 'VirusInfo');
 
     foreach my $table (@mt) {
 	$dbh->do ("INSERT INTO ClusterInfo (cid, name, ivalue) select $clientcid, 'lastmt_$table', " .
@@ -1082,7 +1122,7 @@ sub update_client_clusterinfo {
     $dbh->do ("INSERT INTO ClusterInfo (cid, name, ivalue) select $mastercid, 'lastid_CStatistic', " .
 	      "COALESCE (max (rid), -1) FROM CStatistic WHERE cid = $mastercid");
 
-    my @mt = ('CMSReceivers', 'CGreylist', 'UserPrefs', 'DomainStat', 'DailyStat', 'LocalStat', 'VirusInfo');
+    my @mt = ('CMSReceivers', 'Greylist', 'UserPrefs', 'DomainStat', 'DailyStat', 'LocalStat', 'VirusInfo');
 
     foreach my $table (@mt) {
 	$dbh->do ("INSERT INTO ClusterInfo (cid, name, ivalue) select $mastercid, 'lastmt_$table', " .
diff --git a/src/bin/pmgpolicy b/src/bin/pmgpolicy
index ebc24ce..e49317e 100755
--- a/src/bin/pmgpolicy
+++ b/src/bin/pmgpolicy
@@ -195,7 +195,7 @@ sub run_dequeue {
 	my $cmds = '';
 
 	my $sth = $dbh->prepare(
-	    "SELECT distinct instance, sender FROM CGreylist " .
+	    "SELECT distinct instance, sender FROM Greylist " .
 	    "WHERE passed = 0 AND extime < ? $rntxt");
 
 	$sth->execute ($now);
@@ -203,7 +203,7 @@ sub run_dequeue {
 
 	while (my $ref = $sth->fetchrow_hashref()) {
 	    my $sth2 = $dbh->prepare(
-		"SELECT * FROM CGreylist WHERE instance = ? AND sender = ?");
+		"SELECT * FROM Greylist WHERE instance = ? AND sender = ?");
 	    $sth2->execute ($ref->{instance}, $ref->{sender});
 	    my $rctime;
 	    my @rcvrs;
@@ -260,7 +260,7 @@ sub run_dequeue {
 	    $self->log (0, $msg);
 	}
 
-	$dbh->do ("DELETE FROM CGreylist WHERE extime < $now");
+	$dbh->do ("DELETE FROM Greylist WHERE extime < $now");
 
 	$dbh->commit;
     };
@@ -553,6 +553,7 @@ sub greylist_value {
 
     my ($net, $host) = $ip =~ m/(\d+\.\d+\.\d+)\.(\d+)/; # IPv4 for now
     return 'dunno' if !defined($net);
+    my $masklen = 24;
 
     my $spf_header;
 
@@ -604,10 +605,10 @@ sub greylist_value {
 
 		    # check if there is already a record in the GL database
 		    my $sth = $dbh->prepare(
-			"SELECT * FROM CGreylist " .
-			"where IPNet = ? AND Sender = ? AND Receiver = ?");
+			"SELECT * FROM Greylist " .
+			"where IPAddr <<= set_masklen(?, ?) AND Sender = ? AND Receiver = ?");
 
-		    $sth->execute($net, $sender, $rcpt);
+		    $sth->execute($ip, $masklen, $sender, $rcpt);
 		    my $ref = $sth->fetchrow_hashref();
 		    $sth->finish();
 
@@ -617,8 +618,8 @@ sub greylist_value {
 
 		    if (!defined($ref->{rctime})) {
 
-			$dbh->do($PMG::DBTools::cgreylist_merge_sql, undef,
-				 $net, $host, $sender, $rcpt, $instance,
+			$dbh->do($PMG::DBTools::greylist_merge_sql, undef,
+				 $ip, $sender, $rcpt, $instance,
 				 $ctime, $ctime + 10, 0, 100000, 0, $ctime, $self->{lcid});
 		    }
 
@@ -658,10 +659,10 @@ sub greylist_value {
 	#$dbh->do ("LOCK TABLE CGreylist IN ROW EXCLUSIVE MODE");
 
 	my $sth = $dbh->prepare(
-	    "SELECT * FROM CGreylist " .
-	    "where IPNet = ? AND Sender = ? AND Receiver = ?");
+	    "SELECT * FROM Greylist " .
+	    "where IPAddr <<= set_masklen(?,?) AND Sender = ? AND Receiver = ?");
 
-	$sth->execute($net, $sender, $rcpt);
+	$sth->execute($ip, $masklen, $sender, $rcpt);
 
 	my $ref = $sth->fetchrow_hashref();
 
@@ -669,8 +670,8 @@ sub greylist_value {
 
 	if (!defined($ref->{rctime})) {
 
-	    $dbh->do($PMG::DBTools::cgreylist_merge_sql, undef,
-		     $net, $host, $sender, $rcpt, $instance,
+	    $dbh->do($PMG::DBTools::greylist_merge_sql, undef,
+		     $ip, $sender, $rcpt, $instance,
 		     $ctime, $ctime + $greylist_lifetime, 0, 1, 0, $ctime, $self->{lcid});
 
 	    $res = $defer_res;
@@ -682,28 +683,31 @@ sub greylist_value {
 		# defer (resent within greylist_delay window)
 		$res = $defer_res;
 		$self->log(3, "defer greylisted mail");
-		$dbh->do("UPDATE CGreylist " .
-			 "SET  Blocked = Blocked + 1, Host = ?, MTime = ? " .
-			 "WHERE IPNet = ? AND Sender = ? AND Receiver = ?", undef,
-			 $host, $ctime, $net, $sender, $rcpt);
+		$dbh->do("UPDATE Greylist " .
+			 "SET Blocked = Blocked + 1, MTime = ? " .
+			 "WHERE IPAddr <<= set_masklen(?,?) AND " .
+			 "Sender = ? AND Receiver = ?", undef,
+			 $ctime, $ip, $masklen, $sender, $rcpt);
 	    } else {
 		if ($ctime < $ref->{extime}) {
 		    # accept (not expired)
 		    my $lifetime = $sender eq "" ? 0 : $greylist_awlifetime;
 		    my $delay = $ref->{passed} ? "" : "Delay = $age, ";
-		    $dbh->do("UPDATE CGreylist " .
-			     "SET  Passed =  Passed + 1, $delay Host = ?, ExTime = ?, MTime = ? " .
-			     "WHERE IPNet =  ? AND Sender = ? AND Receiver = ?", undef,
-			     $host, $ctime + $lifetime, $ctime, $net, $sender, $rcpt);
+		    $dbh->do("UPDATE Greylist " .
+			     "SET Passed = Passed + 1, $delay ExTime = ?, MTime = ? " .
+			     "WHERE IPADDR <<= set_masklen(?, ?) AND " .
+			     "Sender = ? AND Receiver = ?", undef,
+			     $ctime + $lifetime, $ctime, $ip, $masklen, $sender, $rcpt);
 		} else {
 		    # defer (record is expired)
 		    $res = $defer_res;
-		    $dbh->do("UPDATE CGreylist " .
-			     "SET  Host = ?, RCTime = ?, ExTime = ?, MTime = ?, Instance = ?, " .
+		    $dbh->do("UPDATE Greylist " .
+			     "SET RCTime = ?, ExTime = ?, MTime = ?, Instance = ?, " .
 			     "Blocked = 1, Passed = 0 " .
-			     "WHERE IPNet =  ? AND Sender = ? AND Receiver = ?", undef,
-			     $host, $ctime, $ctime + $greylist_lifetime, $ctime, $instance,
-			     $net, $sender, $rcpt);
+			     "WHERE IPAddr <<= set_masklen(?,?) AND " .
+			     "Sender = ? AND Receiver = ?", undef,
+			     $ctime, $ctime + $greylist_lifetime, $ctime, $instance,
+			     $ip, $masklen, $sender, $rcpt);
 		}
 	    }
 	}
diff --git a/src/tests/test_greylist.pl b/src/tests/test_greylist.pl
index 387cf72..6b0c676 100755
--- a/src/tests/test_greylist.pl
+++ b/src/tests/test_greylist.pl
@@ -27,7 +27,7 @@ system ("perl -I.. ../bin/pmgpolicy -d $testdb -t --port $testport --pidfile '$t
 
 sub reset_gldb {
     my $dbh = PMG::DBTools::open_ruledb($testdb);
-    $dbh->do ("DELETE FROM CGreylist");
+    $dbh->do ("DELETE FROM Greylist");
     $dbh->disconnect();
 }
 
-- 
2.20.1




More information about the pmg-devel mailing list