[pmg-devel] [PATCH pmg-api v2 2/5] use postgres inet functions for greylist matching

Stoiko Ivanov s.ivanov at proxmox.com
Mon Apr 20 13:22:40 CEST 2020


In preparation for adding support for a configurable greylist netmask [0]
and greylisting for ipv6 hosts the width of the IPNet column of the cgreylist
table needs to be extended to 49 [1].

Instead of comparing the first 3 octets of a ipv4 address we store the
complete network definition (i.e. for 192.0.2.127/24 - 192.0.2.0/24)
The last octet is not saved, but written as 0 (the information is not
needed, and not used currently). The generation of the network is done
with postgresql's functions for the inet and cidr datatypes [2,3].

The change of the column width instead of using the inet datatype prevents
errors while syncing or downgrading, although older nodes in a cluster (or
downgraded nodes) will not match new records.

When syncing from a node with old-style data the rows are inserted in the
new format.

Upon upgrade (`pmgdb init` in the postinst script) the data is changed to
the new format and matched for duplicates (in case one node in the cluster
got upgraded and it's contents were synced we should not edit the data
again). This process does cause the Cgreylist table to be scanned, which
takes time linear in the number of rows (e.g. with a test-dataset of
~ 1 million rows the upgrade is blocked for ~50 seconds on an
average testinstallation).
Changing only the column datatype does not lock the table and is almost
instantenous [4].

[0] defining from which neighbors a mail is accepted on the second attempt
[1] INET6_ADDRSTRLEN is 46 + 4 for the netmask ('/128) - \0
[2] https://www.postgresql.org/docs/11/datatype-net-types.html
[3] https://www.postgresql.org/docs/11/functions-net.html
[4] Notes section of https://www.postgresql.org/docs/11/sql-altertable.html

Signed-off-by: Stoiko Ivanov <s.ivanov at proxmox.com>
---
 src/PMG/Cluster.pm |  7 +++++--
 src/PMG/DBTools.pm | 43 +++++++++++++++++++++++++++++++++++++++---
 src/bin/pmgpolicy  | 47 ++++++++++++++++++++++++++--------------------
 3 files changed, 72 insertions(+), 25 deletions(-)

diff --git a/src/PMG/Cluster.pm b/src/PMG/Cluster.pm
index e890924..f99232d 100644
--- a/src/PMG/Cluster.pm
+++ b/src/PMG/Cluster.pm
@@ -763,12 +763,15 @@ sub sync_greylist_db {
 	    "mtime >= $lastmt AND CID != 0";
     };
 
-    my $merge_sth = $dbh->prepare($PMG::DBTools::cgreylist_merge_sql);
+    # FIXME: drop Host column with PMG 7.0
+    my $merge_sth = $dbh->prepare(PMG::DBTools::cgreylist_merge_sql());
     my $mergefunc = sub {
 	my ($ref) = @_;
 
+	my $ipnet = $ref->{ipnet};
+	$ipnet .= '.0/24' if $ipnet !~ /\/\d+$/;
 	$merge_sth->execute(
-	    $ref->{ipnet}, $ref->{host}, $ref->{sender}, $ref->{receiver},
+	    $ipnet, 0, $ref->{sender}, $ref->{receiver},
 	    $ref->{instance}, $ref->{rctime}, $ref->{extime}, $ref->{delay},
 	    $ref->{blocked}, $ref->{passed}, 0, $ref->{cid});
     };
diff --git a/src/PMG/DBTools.pm b/src/PMG/DBTools.pm
index a7a18a4..269ff3b 100644
--- a/src/PMG/DBTools.pm
+++ b/src/PMG/DBTools.pm
@@ -18,10 +18,16 @@ use PMG::Utils qw(postgres_admin_cmd);
 
 our $default_db_name = "Proxmox_ruledb";
 
-our $cgreylist_merge_sql =
+# FIXME: drop Host column with PMG 7.0
+sub cgreylist_merge_sql {
+    my ($with_mask) = @_;
+
+    my $network = $with_mask ? 'network(set_masklen(?, ?))' : '?';
+
+    my $sql =
     'INSERT INTO CGREYLIST (IPNet,Host,Sender,Receiver,Instance,RCTime,' .
     'ExTime,Delay,Blocked,Passed,MTime,CID) ' .
-    'VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?) ' .
+    "VALUES ($network, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?) " .
     '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),' .
@@ -30,6 +36,9 @@ our $cgreylist_merge_sql =
     'Blocked = GREATEST(CGREYLIST.Blocked, excluded.Blocked),' .
     'Passed = GREATEST(CGREYLIST.Passed, excluded.Passed)';
 
+    return $sql;
+}
+
 sub open_ruledb {
     my ($database, $host, $port) = @_;
 
@@ -102,7 +111,7 @@ sub database_list {
 
 my $cgreylist_ctablecmd =  <<__EOD;
     CREATE TABLE CGreylist
-    (IPNet VARCHAR(16) NOT NULL,
+    (IPNet VARCHAR(49) NOT NULL,
      Host INTEGER NOT NULL,
      Sender VARCHAR(255) NOT NULL,
      Receiver VARCHAR(255) NOT NULL,
@@ -522,6 +531,34 @@ sub upgradedb {
 		 "AND value = 'content-type:application/x-java-vm';");
     };
 
+    # FIXME: drop Host column with PMG 7.0
+    # increase column size of cgreylist.ipnet for ipv6 support and transfer data
+    eval {
+	my $sth = $dbh->prepare("SELECT character_maximum_length ".
+	    "FROM information_schema.columns ".
+	    "WHERE table_name = 'cgreylist' AND column_name = 'ipnet'");
+	$sth->execute();
+	my $res = $sth->fetchrow_hashref();
+	if ($res->{character_maximum_length} == 16) {
+	    $dbh->begin_work;
+	    $dbh->do("ALTER TABLE CGreylist ALTER COLUMN " .
+		     "IPNet TYPE varchar(49)");
+	    eval {
+		$dbh->do("UPDATE CGreylist cg1 SET IPNet = IPNet || '.0/24' ".
+			"WHERE position('/' in IPNet) = 0 AND ".
+			"NOT EXISTS (SELECT 1 FROM CGreylist cg2 WHERE ".
+			"cg2.IPNet = cg1.IPNet || '.0/24' AND ".
+			"cg1.Receiver = cg2.Receiver AND cg1.Sender = cg2.Sender)");
+	    };
+	    #ignore errors here - legacy rows will eventually expire
+	    $dbh->commit;
+	}
+    };
+    if (my $err = $@) {
+	$dbh->rollback;
+	die $err;
+    }
+
     foreach my $table (keys %$tables) {
 	eval { $dbh->do("ANALYZE $table"); };
 	warn $@ if $@;
diff --git a/src/bin/pmgpolicy b/src/bin/pmgpolicy
index ebc24ce..b371525 100755
--- a/src/bin/pmgpolicy
+++ b/src/bin/pmgpolicy
@@ -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;
 
@@ -605,9 +606,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 = ?");
+			"WHERE IPNet::cidr = network(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 +619,9 @@ sub greylist_value {
 
 		    if (!defined($ref->{rctime})) {
 
-			$dbh->do($PMG::DBTools::cgreylist_merge_sql, undef,
-				 $net, $host, $sender, $rcpt, $instance,
+			# FIXME: drop Host column with PMG 7.0
+			$dbh->do(PMG::DBTools::cgreylist_merge_sql(1), undef,
+				 $ip, $masklen, 0, $sender, $rcpt, $instance,
 				 $ctime, $ctime + 10, 0, 100000, 0, $ctime, $self->{lcid});
 		    }
 
@@ -659,9 +662,10 @@ sub greylist_value {
 
 	my $sth = $dbh->prepare(
 	    "SELECT * FROM CGreylist " .
-	    "where IPNet = ? AND Sender = ? AND Receiver = ?");
+	    "WHERE IPNet::cidr = network(set_masklen(?, ?)) AND ".
+	    "Sender = ? AND Receiver = ?");
 
-	$sth->execute($net, $sender, $rcpt);
+	$sth->execute($ip, $masklen, $sender, $rcpt);
 
 	my $ref = $sth->fetchrow_hashref();
 
@@ -669,9 +673,9 @@ sub greylist_value {
 
 	if (!defined($ref->{rctime})) {
 
-	    $dbh->do($PMG::DBTools::cgreylist_merge_sql, undef,
-		     $net, $host, $sender, $rcpt, $instance,
-		     $ctime, $ctime + $greylist_lifetime, 0, 1, 0, $ctime, $self->{lcid});
+	    $dbh->do(PMG::DBTools::cgreylist_merge_sql(1), undef,
+		    $ip, $masklen, 0, $sender, $rcpt, $instance,
+		    $ctime, $ctime + $greylist_lifetime, 0, 1, 0, $ctime, $self->{lcid});
 
 	    $res = $defer_res;
 	    $self->log(3, "defer greylisted mail");
@@ -683,27 +687,30 @@ sub greylist_value {
 		$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);
+			"SET Blocked = Blocked + 1, MTime = ? " .
+			"WHERE IPNet::cidr = network(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);
+			    "SET Passed = Passed + 1, $delay ExTime = ?, MTime = ? " .
+			    "WHERE IPNet::cidr = network(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 = ?, " .
-			     "Blocked = 1, Passed = 0 " .
-			     "WHERE IPNet =  ? AND Sender = ? AND Receiver = ?", undef,
-			     $host, $ctime, $ctime + $greylist_lifetime, $ctime, $instance,
-			     $net, $sender, $rcpt);
+			    "SET RCTime = ?, ExTime = ?, MTime = ?, Instance = ?, " .
+			    "Blocked = 1, Passed = 0 " .
+			    "WHERE IPNet::cidr = network(set_masklen(?, ?)) AND ".
+			    "Sender = ? AND Receiver = ?", undef,
+			    $ctime, $ctime + $greylist_lifetime, $ctime, $instance,
+			    $ip, $masklen, $sender, $rcpt);
 		}
 	    }
 	}
-- 
2.20.1




More information about the pmg-devel mailing list