[pmg-devel] [PATCH pmg-api 0/4] add support for locally configured SA channels

[pmg-devel] [PATCH pmg-api 0/4] add support for locally configured SA channels

[Stoiko Ivanov]

With the recent announcement by KAM about the availability of a signed
update channel for the KAM.cf (and other) ruleset [0], I thought that this
might be a better suited way to provide regular updates to KAM.cf compared
to the initial patch I sent some time ago [1].

The upside of using sa-update is that it does lint the ruleset before trying
to pull the updates, and afterwards as well. GPG signed updates by KAM can
also be considered production worthy.

I tried to align the implementation to the channel.d mechanism mentioned in
[0] since it seems:
* somewhat sensible (these days my first choice would not be shell-code
  snippets)
* one mechanism of potentially having a distributable way of providing
  the necessary configuration for an external SA ruleset - and the only one
  I'm aware of, which is not a self-tailored script

The patchset was tested with the file provided at [0] on my test
installation

Should this be accepted we could ship
/etc/mail/spamassassin/channel.d/KAM_channel.conf in proxmox-spamassassin


[0] https://mcgrail.com/template/kam.cf_channel
[1] https://lists.proxmox.com/pipermail/pmg-devel/2020-November/001397.html

Stoiko Ivanov (4):
  add helper for parsing SA channel.d files
  api: spamassassin: read local channels
  api: spamassassin: update local channels
  pmg-daily: run sa-update for local channels

 src/PMG/API2/SpamAssassin.pm | 89 +++++++++++++++++++++---------------
 src/PMG/Utils.pm             | 56 +++++++++++++++++++++++
 src/bin/pmg-daily            | 11 ++++-
 3 files changed, 118 insertions(+), 38 deletions(-)

-- 
2.20.1

[pmg-devel] [PATCH pmg-api 1/4] add helper for parsing SA channel.d files

[Stoiko Ivanov]

RHEL/CentOS based SpamAssassin implementations ship an update script,
which reads shell snippets from
/etc/mail/spamassassin/channel.d/*.conf and uses the information there
to update SA rules from the configured channels [0].

Noticed the existence of this directory/mechanism while reading the
announcement of the updatechannel for the KAM ruleset [1].

Parsing the file as text, instead of sourcing it in a shell, since I
hope that the channel files distributed don't rely on running commands
to get the ruleset url and gpg key.

[0] https://src.fedoraproject.org/rpms/spamassassin/blob/master/f/sa-update.cronscript
[1] https://mcgrail.com/template/kam.cf_channel

Signed-off-by: Stoiko Ivanov <s.ivanov@proxmox.com>
---
 src/PMG/Utils.pm | 27 +++++++++++++++++++++++++++
 1 file changed, 27 insertions(+)

diff --git a/src/PMG/Utils.pm b/src/PMG/Utils.pm
index d3fae9e..3f5b045 100644
--- a/src/PMG/Utils.pm
+++ b/src/PMG/Utils.pm
@@ -1442,5 +1442,32 @@ sub domain_regex {
     return $regex;
 }
 
+sub local_spamassassin_channels {
+
+    my $res = [];
+
+    my $local_channel_dir = '/etc/mail/spamassassin/channel.d/';
+    my $read_sa_channel = sub {
+	my ($filename) = @_;
+
+	my $channel_file = $local_channel_dir.$filename;
+	my $content = PVE::Tools::file_get_contents($channel_file);
+	my $channel = {
+	    filename => $channel_file,
+	};
+
+	($channel->{keyid}) = ($content =~ /^KEYID=([a-fA-F0-9]+)$/m);
+	die "no KEYID in $filename!\n" if !defined($channel->{keyid});
+	($channel->{channelurl}) = ($content =~ /CHANNELURL=(.+)$/m);
+	die "no CHANNELURL in $filename!\n" if !defined($channel->{channelurl});
+	($channel->{gpgkey}) = ($content =~ /(-----BEGIN PGP PUBLIC KEY BLOCK-----.+-----END PGP PUBLIC KEY BLOCK-----)/s);
+	die "no GPG public key in $filename!\n" if !defined($channel->{gpgkey});
+
+	push(@$res, $channel);
+    };
+
+    PVE::Tools::dir_glob_foreach($local_channel_dir, '.*\.conf', $read_sa_channel);
+    return $res;
+}
 
 1;
-- 
2.20.1

Re: [pmg-devel] [PATCH pmg-api 1/4] add helper for parsing SA channel.d files

[Fabian Grünbichler]

On December 16, 2020 6:18 pm, Stoiko Ivanov wrote:
> RHEL/CentOS based SpamAssassin implementations ship an update script,
> which reads shell snippets from
> /etc/mail/spamassassin/channel.d/*.conf and uses the information there
> to update SA rules from the configured channels [0].
> 
> Noticed the existence of this directory/mechanism while reading the
> announcement of the updatechannel for the KAM ruleset [1].
> 
> Parsing the file as text, instead of sourcing it in a shell, since I
> hope that the channel files distributed don't rely on running commands
> to get the ruleset url and gpg key.
> 
> [0] https://src.fedoraproject.org/rpms/spamassassin/blob/master/f/sa-update.cronscript
> [1] https://mcgrail.com/template/kam.cf_channel
> 
> Signed-off-by: Stoiko Ivanov <s.ivanov@proxmox.com>
> ---
>  src/PMG/Utils.pm | 27 +++++++++++++++++++++++++++
>  1 file changed, 27 insertions(+)
> 
> diff --git a/src/PMG/Utils.pm b/src/PMG/Utils.pm
> index d3fae9e..3f5b045 100644
> --- a/src/PMG/Utils.pm
> +++ b/src/PMG/Utils.pm
> @@ -1442,5 +1442,32 @@ sub domain_regex {
>      return $regex;
>  }
>  
> +sub local_spamassassin_channels {
> +
> +    my $res = [];
> +
> +    my $local_channel_dir = '/etc/mail/spamassassin/channel.d/';
> +    my $read_sa_channel = sub {
> +	my ($filename) = @_;
> +
> +	my $channel_file = $local_channel_dir.$filename;
> +	my $content = PVE::Tools::file_get_contents($channel_file);
> +	my $channel = {
> +	    filename => $channel_file,
> +	};
> +
> +	($channel->{keyid}) = ($content =~ /^KEYID=([a-fA-F0-9]+)$/m);
> +	die "no KEYID in $filename!\n" if !defined($channel->{keyid});
> +	($channel->{channelurl}) = ($content =~ /CHANNELURL=(.+)$/m);

should maybe be anchored as well?

> +	die "no CHANNELURL in $filename!\n" if !defined($channel->{channelurl});
> +	($channel->{gpgkey}) = ($content =~ /(-----BEGIN PGP PUBLIC KEY BLOCK-----.+-----END PGP PUBLIC KEY BLOCK-----)/s);

could also be anchored:

(^|\n)

(\n|$)

or something similar

> +	die "no GPG public key in $filename!\n" if !defined($channel->{gpgkey});
> +
> +	push(@$res, $channel);
> +    };
> +
> +    PVE::Tools::dir_glob_foreach($local_channel_dir, '.*\.conf', $read_sa_channel);
> +    return $res;
> +}
>  
>  1;
> -- 
> 2.20.1
> 
> 
> 
> _______________________________________________
> pmg-devel mailing list
> pmg-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pmg-devel
> 
> 
> 

[pmg-devel] [PATCH pmg-api 2/4] api: spamassassin: read local channels

[Stoiko Ivanov]

Signed-off-by: Stoiko Ivanov <s.ivanov@proxmox.com>
---
the patch of src/PMG/API2/SpamAssassin mostly indents code and I found it
helpful to view it with `git diff -w`

 src/PMG/API2/SpamAssassin.pm | 83 +++++++++++++++++++++---------------
 1 file changed, 49 insertions(+), 34 deletions(-)

diff --git a/src/PMG/API2/SpamAssassin.pm b/src/PMG/API2/SpamAssassin.pm
index eab02d9..5f9c3a5 100644
--- a/src/PMG/API2/SpamAssassin.pm
+++ b/src/PMG/API2/SpamAssassin.pm
@@ -80,50 +80,65 @@ __PACKAGE__->register_method({
 	my ($param) = @_;
 
 	my $saversion = $Mail::SpamAssassin::VERSION;
-	my $channelfile = "/var/lib/spamassassin/$saversion/updates_spamassassin_org.cf";
+	my $sa_update_dir = "/var/lib/spamassassin/$saversion/";
+
+	my $check_channel = sub {
+	    my ($channel) = @_;
+
+	    # taken from sa-update:
+	    my $channel_file_base = $channel;
+	    $channel_file_base =~ tr/A-Za-z0-9-/_/cs;
+	    my $channelfile = "${sa_update_dir}${channel_file_base}.cf";
+
+	    my $mtime = -1;
+	    my $version = -1;
+	    my $newversion = -1;
+
+	    if (-f $channelfile) {
+		# stat metadata cf file
+		$mtime = (stat($channelfile))[9]; # 9 is mtime
+
+		# parse version from metadata cf file
+		my $metadata = PVE::Tools::file_read_firstline($channelfile);
+		if ($metadata =~ m/\s([0-9]+)$/) {
+		    $version = $1;
+		} else {
+		    warn "invalid metadata in '$channelfile'\n";
+		}
+	    }
+	    # call sa-update to see if updates are available
 
-	my $mtime = -1;
-	my $version = -1;
-	my $newversion = -1;
+	    my $cmd = "$SAUPDATE -v --checkonly --channel $channel";
+	    PVE::Tools::run_command($cmd, noerr => 1, logfunc => sub {
+		my ($line) = @_;
 
-	if (-f $channelfile) {
-	    # stat metadata cf file
-	    $mtime = (stat($channelfile))[9]; # 9 is mtime
+		if ($line =~ m/Update available for channel \S+: -?[0-9]+ -> ([0-9]+)/) {
+		    $newversion = $1;
+		}
+	    });
 
-	    # parse version from metadata cf file
-	    my $metadata = PVE::Tools::file_read_firstline($channelfile);
-	    if ($metadata =~ m/\s([0-9]+)$/) {
-		$version = $1;
-	    } else {
-		warn "invalid metadata in '$channelfile'\n";
-	    }
-	}
-	# call sa-update to see if updates are available
+	    my $result = {
+		channel => $channel,
+	    };
 
-	my $cmd = "$SAUPDATE -v --checkonly";
-	PVE::Tools::run_command($cmd, noerr => 1, logfunc => sub {
-	    my ($line) = @_;
+	    $result->{version} = $version if $version > -1;
+	    $result->{update_version} = $newversion if $newversion > -1;
+	    $result->{last_updated} = $mtime if $mtime > -1;
 
-	    if ($line =~ m/Update available for channel \S+: -?[0-9]+ -> ([0-9]+)/) {
-		$newversion = $1;
+	    if ($newversion > $version) {
+		$result->{update_avail} = 1;
+	    } else {
+		$result->{update_avail} = 0;
 	    }
-	});
-
-	my $result = {
-	    channel => 'updates.spamassassin.org',
+	    return $result;
 	};
 
-	$result->{version} = $version if $version > -1;
-	$result->{update_version} = $newversion if $newversion > -1;
-	$result->{last_updated} = $mtime if $mtime > -1;
+	my @channels = ('updates.spamassassin.org');
 
-	if ($newversion > $version) {
-	    $result->{update_avail} = 1;
-	} else {
-	    $result->{update_avail} = 0;
-	}
+	my $localchannels = PMG::Utils::local_spamassassin_channels();
+	push(@channels, map { $_->{channelurl} } @$localchannels);
 
-	return [$result];
+	return [ map { $check_channel->($_) } @channels];
     }});
 
 __PACKAGE__->register_method({
-- 
2.20.1

Re: [pmg-devel] [PATCH pmg-api 2/4] api: spamassassin: read local channels

[Fabian Grünbichler]

On December 16, 2020 6:18 pm, Stoiko Ivanov wrote:
> Signed-off-by: Stoiko Ivanov <s.ivanov@proxmox.com>
> ---
> the patch of src/PMG/API2/SpamAssassin mostly indents code and I found it
> helpful to view it with `git diff -w`
> 
>  src/PMG/API2/SpamAssassin.pm | 83 +++++++++++++++++++++---------------
>  1 file changed, 49 insertions(+), 34 deletions(-)
> 
> diff --git a/src/PMG/API2/SpamAssassin.pm b/src/PMG/API2/SpamAssassin.pm
> index eab02d9..5f9c3a5 100644
> --- a/src/PMG/API2/SpamAssassin.pm
> +++ b/src/PMG/API2/SpamAssassin.pm
> @@ -80,50 +80,65 @@ __PACKAGE__->register_method({
>  	my ($param) = @_;
>  
>  	my $saversion = $Mail::SpamAssassin::VERSION;
> -	my $channelfile = "/var/lib/spamassassin/$saversion/updates_spamassassin_org.cf";
> +	my $sa_update_dir = "/var/lib/spamassassin/$saversion/";
> +
> +	my $check_channel = sub {
> +	    my ($channel) = @_;
> +
> +	    # taken from sa-update:
> +	    my $channel_file_base = $channel;
> +	    $channel_file_base =~ tr/A-Za-z0-9-/_/cs;

this is syntax we don't use anywhere else, so maybe add a comment or 
rewrite it using a regular replacement?

> +	    my $channelfile = "${sa_update_dir}${channel_file_base}.cf";
> +
> +	    my $mtime = -1;
> +	    my $version = -1;
> +	    my $newversion = -1;
> +
> +	    if (-f $channelfile) {
> +		# stat metadata cf file
> +		$mtime = (stat($channelfile))[9]; # 9 is mtime
> +
> +		# parse version from metadata cf file
> +		my $metadata = PVE::Tools::file_read_firstline($channelfile);
> +		if ($metadata =~ m/\s([0-9]+)$/) {
> +		    $version = $1;
> +		} else {
> +		    warn "invalid metadata in '$channelfile'\n";
> +		}
> +	    }
> +	    # call sa-update to see if updates are available
>  
> -	my $mtime = -1;
> -	my $version = -1;
> -	my $newversion = -1;
> +	    my $cmd = "$SAUPDATE -v --checkonly --channel $channel";
> +	    PVE::Tools::run_command($cmd, noerr => 1, logfunc => sub {
> +		my ($line) = @_;
>  
> -	if (-f $channelfile) {
> -	    # stat metadata cf file
> -	    $mtime = (stat($channelfile))[9]; # 9 is mtime
> +		if ($line =~ m/Update available for channel \S+: -?[0-9]+ -> ([0-9]+)/) {
> +		    $newversion = $1;
> +		}
> +	    });
>  
> -	    # parse version from metadata cf file
> -	    my $metadata = PVE::Tools::file_read_firstline($channelfile);
> -	    if ($metadata =~ m/\s([0-9]+)$/) {
> -		$version = $1;
> -	    } else {
> -		warn "invalid metadata in '$channelfile'\n";
> -	    }
> -	}
> -	# call sa-update to see if updates are available
> +	    my $result = {
> +		channel => $channel,
> +	    };
>  
> -	my $cmd = "$SAUPDATE -v --checkonly";
> -	PVE::Tools::run_command($cmd, noerr => 1, logfunc => sub {
> -	    my ($line) = @_;
> +	    $result->{version} = $version if $version > -1;
> +	    $result->{update_version} = $newversion if $newversion > -1;
> +	    $result->{last_updated} = $mtime if $mtime > -1;
>  
> -	    if ($line =~ m/Update available for channel \S+: -?[0-9]+ -> ([0-9]+)/) {
> -		$newversion = $1;
> +	    if ($newversion > $version) {
> +		$result->{update_avail} = 1;
> +	    } else {
> +		$result->{update_avail} = 0;
>  	    }
> -	});
> -
> -	my $result = {
> -	    channel => 'updates.spamassassin.org',
> +	    return $result;
>  	};
>  
> -	$result->{version} = $version if $version > -1;
> -	$result->{update_version} = $newversion if $newversion > -1;
> -	$result->{last_updated} = $mtime if $mtime > -1;
> +	my @channels = ('updates.spamassassin.org');
>  
> -	if ($newversion > $version) {
> -	    $result->{update_avail} = 1;
> -	} else {
> -	    $result->{update_avail} = 0;
> -	}
> +	my $localchannels = PMG::Utils::local_spamassassin_channels();
> +	push(@channels, map { $_->{channelurl} } @$localchannels);
>  
> -	return [$result];
> +	return [ map { $check_channel->($_) } @channels];
>      }});
>  
>  __PACKAGE__->register_method({
> -- 
> 2.20.1
> 
> 
> 
> _______________________________________________
> pmg-devel mailing list
> pmg-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pmg-devel
> 
> 
> 

[pmg-devel] [PATCH pmg-api 3/4] api: spamassassin: update local channels

[Stoiko Ivanov]

This patch adds a helper to loop over all present Spamassassin
channels files in /etc/mail/spamassassin/channel.d and:
* import the included gpg key into sa-update's keyring
* run sa-update for each channel separately

the verbose argument of the helper is for reusing the code in
pmg-daily (where we only want to log errors and be less informative)

In order to only hardcode the path of sa-update once the definition
was moved to PMG::Utils.

The choice of invoking sa-update for each channel separately is based,
instead of providing multiple '--channel' and '--gpgkey' options to
a single command was made to prevent downloading signatures, which
were signed by a key not configured for the channel.

Signed-off-by: Stoiko Ivanov <s.ivanov@proxmox.com>
---
 src/PMG/API2/SpamAssassin.pm |  6 +++---
 src/PMG/Utils.pm             | 29 +++++++++++++++++++++++++++++
 2 files changed, 32 insertions(+), 3 deletions(-)

diff --git a/src/PMG/API2/SpamAssassin.pm b/src/PMG/API2/SpamAssassin.pm
index 5f9c3a5..df46c64 100644
--- a/src/PMG/API2/SpamAssassin.pm
+++ b/src/PMG/API2/SpamAssassin.pm
@@ -11,15 +11,13 @@ use PVE::RESTHandler;
 use PMG::RESTEnvironment;
 use PVE::JSONSchema qw(get_standard_option);
 
-use PMG::Utils;
+use PMG::Utils qw($SAUPDATE);
 use PMG::Config;
 
 use Mail::SpamAssassin;
 
 use base qw(PVE::RESTHandler);
 
-my $SAUPDATE = '/usr/bin/sa-update';
-
 __PACKAGE__->register_method ({
     name => 'index',
     path => '',
@@ -174,6 +172,8 @@ __PACKAGE__->register_method({
 	    my $cmd = "$SAUPDATE -v";
 
 	    PVE::Tools::run_command($cmd, noerr => 1);
+
+	    PMG::Utils::update_local_spamassassin_channels(1);
 	};
 
 	return $rpcenv->fork_worker('saupdate', undef, $authuser, $realcmd);
diff --git a/src/PMG/Utils.pm b/src/PMG/Utils.pm
index 3f5b045..de74d4e 100644
--- a/src/PMG/Utils.pm
+++ b/src/PMG/Utils.pm
@@ -44,6 +44,7 @@ use base 'Exporter';
 
 our @EXPORT_OK = qw(
 postgres_admin_cmd
+$SAUPDATE
 );
 
 my $valid_pmg_realms = ['pam', 'pmg', 'quarantine'];
@@ -1442,6 +1443,8 @@ sub domain_regex {
     return $regex;
 }
 
+our $SAUPDATE = '/usr/bin/sa-update';
+
 sub local_spamassassin_channels {
 
     my $res = [];
@@ -1470,4 +1473,30 @@ sub local_spamassassin_channels {
     return $res;
 }
 
+sub update_local_spamassassin_channels {
+    my ($verbose) = @_;
+    # import all configured channel's gpg-keys to sa-update's keyring
+    my $importcmd = "$SAUPDATE";
+    $importcmd .= '-v' if $verbose;
+    my $localchannels = PMG::Utils::local_spamassassin_channels();
+    for my $channel (@$localchannels) {
+	$importcmd .= " --import $channel->{filename}";
+    }
+    print "Importing gpg files from locally available channel definitions\n" if $verbose;
+    PVE::Tools::run_command($importcmd, noerr => 1);
+
+    my $fresh_updates = 0;
+
+    for my $channel (@$localchannels) {
+	my $cmd = "$SAUPDATE -v --channel $channel->{channelurl} --gpgkey $channel->{keyid}";
+	print "Updating $channel->{channelurl}\n" if $verbose;
+	my $ret = PVE::Tools::run_command($cmd, noerr => 1);
+	die "updating $channel->{channelurl} failed - sa-update exited with $ret\n" if $ret >= 2;
+
+	$fresh_updates = 1 if $ret == 0;
+    }
+
+    return $fresh_updates
+}
+
 1;
-- 
2.20.1

Re: [pmg-devel] [PATCH pmg-api 3/4] api: spamassassin: update local channels

[Fabian Grünbichler]

On December 16, 2020 6:18 pm, Stoiko Ivanov wrote:
> This patch adds a helper to loop over all present Spamassassin
> channels files in /etc/mail/spamassassin/channel.d and:
> * import the included gpg key into sa-update's keyring
> * run sa-update for each channel separately
> 
> the verbose argument of the helper is for reusing the code in
> pmg-daily (where we only want to log errors and be less informative)
> 
> In order to only hardcode the path of sa-update once the definition
> was moved to PMG::Utils.
> 
> The choice of invoking sa-update for each channel separately is based,
> instead of providing multiple '--channel' and '--gpgkey' options to
> a single command was made to prevent downloading signatures, which
> were signed by a key not configured for the channel.
> 
> Signed-off-by: Stoiko Ivanov <s.ivanov@proxmox.com>
> ---
>  src/PMG/API2/SpamAssassin.pm |  6 +++---
>  src/PMG/Utils.pm             | 29 +++++++++++++++++++++++++++++
>  2 files changed, 32 insertions(+), 3 deletions(-)
> 
> diff --git a/src/PMG/API2/SpamAssassin.pm b/src/PMG/API2/SpamAssassin.pm
> index 5f9c3a5..df46c64 100644
> --- a/src/PMG/API2/SpamAssassin.pm
> +++ b/src/PMG/API2/SpamAssassin.pm
> @@ -11,15 +11,13 @@ use PVE::RESTHandler;
>  use PMG::RESTEnvironment;
>  use PVE::JSONSchema qw(get_standard_option);
>  
> -use PMG::Utils;
> +use PMG::Utils qw($SAUPDATE);
>  use PMG::Config;
>  
>  use Mail::SpamAssassin;
>  
>  use base qw(PVE::RESTHandler);
>  
> -my $SAUPDATE = '/usr/bin/sa-update';
> -
>  __PACKAGE__->register_method ({
>      name => 'index',
>      path => '',
> @@ -174,6 +172,8 @@ __PACKAGE__->register_method({
>  	    my $cmd = "$SAUPDATE -v";
>  
>  	    PVE::Tools::run_command($cmd, noerr => 1);
> +
> +	    PMG::Utils::update_local_spamassassin_channels(1);
>  	};
>  
>  	return $rpcenv->fork_worker('saupdate', undef, $authuser, $realcmd);
> diff --git a/src/PMG/Utils.pm b/src/PMG/Utils.pm
> index 3f5b045..de74d4e 100644
> --- a/src/PMG/Utils.pm
> +++ b/src/PMG/Utils.pm
> @@ -44,6 +44,7 @@ use base 'Exporter';
>  
>  our @EXPORT_OK = qw(
>  postgres_admin_cmd
> +$SAUPDATE
>  );
>  
>  my $valid_pmg_realms = ['pam', 'pmg', 'quarantine'];
> @@ -1442,6 +1443,8 @@ sub domain_regex {
>      return $regex;
>  }
>  
> +our $SAUPDATE = '/usr/bin/sa-update';
> +
>  sub local_spamassassin_channels {
>  
>      my $res = [];
> @@ -1470,4 +1473,30 @@ sub local_spamassassin_channels {
>      return $res;
>  }
>  
> +sub update_local_spamassassin_channels {
> +    my ($verbose) = @_;
> +    # import all configured channel's gpg-keys to sa-update's keyring
> +    my $importcmd = "$SAUPDATE";
> +    $importcmd .= '-v' if $verbose;
> +    my $localchannels = PMG::Utils::local_spamassassin_channels();
> +    for my $channel (@$localchannels) {
> +	$importcmd .= " --import $channel->{filename}";
> +    }
> +    print "Importing gpg files from locally available channel definitions\n" if $verbose;
> +    PVE::Tools::run_command($importcmd, noerr => 1);

why is $importcmd not an array/list? how does the import behave if one 
of X channel files is bad/corrupt/malformed/...? wouldn't it make sense 
to import + update each channel on their own?

is the noerr needed?

> +    my $fresh_updates = 0;
> +
> +    for my $channel (@$localchannels) {
> +	my $cmd = "$SAUPDATE -v --channel $channel->{channelurl} --gpgkey $channel->{keyid}";
> +	print "Updating $channel->{channelurl}\n" if $verbose;
> +	my $ret = PVE::Tools::run_command($cmd, noerr => 1);

$cmd should also be a list..

> +	die "updating $channel->{channelurl} failed - sa-update exited with $ret\n" if $ret >= 2;
> +
> +	$fresh_updates = 1 if $ret == 0;
> +    }
> +
> +    return $fresh_updates
> +}
> +
>  1;
> -- 
> 2.20.1
> 
> 
> 
> _______________________________________________
> pmg-devel mailing list
> pmg-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pmg-devel
> 
> 
> 

Re: [pmg-devel] [PATCH pmg-api 3/4] api: spamassassin: update local channels

[Stoiko Ivanov]

On Wed, 30 Dec 2020 14:07:22 +0100
Fabian Grünbichler <f.gruenbichler@proxmox.com> wrote:

> On December 16, 2020 6:18 pm, Stoiko Ivanov wrote:
> > This patch adds a helper to loop over all present Spamassassin
> > channels files in /etc/mail/spamassassin/channel.d and:
> > * import the included gpg key into sa-update's keyring
> > * run sa-update for each channel separately
> > 
> > the verbose argument of the helper is for reusing the code in
> > pmg-daily (where we only want to log errors and be less informative)
> > 
> > In order to only hardcode the path of sa-update once the definition
> > was moved to PMG::Utils.
> > 
> > The choice of invoking sa-update for each channel separately is based,
> > instead of providing multiple '--channel' and '--gpgkey' options to
> > a single command was made to prevent downloading signatures, which
> > were signed by a key not configured for the channel.
> > 
> > Signed-off-by: Stoiko Ivanov <s.ivanov@proxmox.com>
> > ---
> >  src/PMG/API2/SpamAssassin.pm |  6 +++---
> >  src/PMG/Utils.pm             | 29 +++++++++++++++++++++++++++++
> >  2 files changed, 32 insertions(+), 3 deletions(-)
> > 
> > diff --git a/src/PMG/API2/SpamAssassin.pm b/src/PMG/API2/SpamAssassin.pm
> > index 5f9c3a5..df46c64 100644
> > --- a/src/PMG/API2/SpamAssassin.pm
> > +++ b/src/PMG/API2/SpamAssassin.pm
> > @@ -11,15 +11,13 @@ use PVE::RESTHandler;
> >  use PMG::RESTEnvironment;
> >  use PVE::JSONSchema qw(get_standard_option);
> >  
> > -use PMG::Utils;
> > +use PMG::Utils qw($SAUPDATE);
> >  use PMG::Config;
> >  
> >  use Mail::SpamAssassin;
> >  
> >  use base qw(PVE::RESTHandler);
> >  
> > -my $SAUPDATE = '/usr/bin/sa-update';
> > -
> >  __PACKAGE__->register_method ({
> >      name => 'index',
> >      path => '',
> > @@ -174,6 +172,8 @@ __PACKAGE__->register_method({
> >  	    my $cmd = "$SAUPDATE -v";
> >  
> >  	    PVE::Tools::run_command($cmd, noerr => 1);
> > +
> > +	    PMG::Utils::update_local_spamassassin_channels(1);
> >  	};
> >  
> >  	return $rpcenv->fork_worker('saupdate', undef, $authuser, $realcmd);
> > diff --git a/src/PMG/Utils.pm b/src/PMG/Utils.pm
> > index 3f5b045..de74d4e 100644
> > --- a/src/PMG/Utils.pm
> > +++ b/src/PMG/Utils.pm
> > @@ -44,6 +44,7 @@ use base 'Exporter';
> >  
> >  our @EXPORT_OK = qw(
> >  postgres_admin_cmd
> > +$SAUPDATE
> >  );
> >  
> >  my $valid_pmg_realms = ['pam', 'pmg', 'quarantine'];
> > @@ -1442,6 +1443,8 @@ sub domain_regex {
> >      return $regex;
> >  }
> >  
> > +our $SAUPDATE = '/usr/bin/sa-update';
> > +
> >  sub local_spamassassin_channels {
> >  
> >      my $res = [];
> > @@ -1470,4 +1473,30 @@ sub local_spamassassin_channels {
> >      return $res;
> >  }
> >  
> > +sub update_local_spamassassin_channels {
> > +    my ($verbose) = @_;
> > +    # import all configured channel's gpg-keys to sa-update's keyring
> > +    my $importcmd = "$SAUPDATE";
> > +    $importcmd .= '-v' if $verbose;
> > +    my $localchannels = PMG::Utils::local_spamassassin_channels();
> > +    for my $channel (@$localchannels) {
> > +	$importcmd .= " --import $channel->{filename}";
> > +    }
> > +    print "Importing gpg files from locally available channel definitions\n" if $verbose;
> > +    PVE::Tools::run_command($importcmd, noerr => 1);  
> 
> why is $importcmd not an array/list? how does the import behave if one 
> of X channel files is bad/corrupt/malformed/...? wouldn't it make sense 
> to import + update each channel on their own?
Great catch - Thanks!
While the man-page of sa-update(1p) explictly states that it is supported
to import multiple keys at once with multiple '--import' options - the
Getopt::Long usage in sa-update does not - it simply imports the last
provided on the command line.
if the file is malformed - sa-updates exits with 2 (and gpg's error output)

-> will change it to run sa-update --import for each channel separately
(and provide the arguments as list).

> 
> is the noerr needed?
in this case it's wrong (I copied from the sa-update invocation below,
where it's needed) - sa-update exits quite directly after calling `gpg
--import` - with gpg's exit status.

> 
> > +    my $fresh_updates = 0;
> > +
> > +    for my $channel (@$localchannels) {
> > +	my $cmd = "$SAUPDATE -v --channel $channel->{channelurl} --gpgkey $channel->{keyid}";
> > +	print "Updating $channel->{channelurl}\n" if $verbose;
> > +	my $ret = PVE::Tools::run_command($cmd, noerr => 1);  
> 
> $cmd should also be a list..
> 
> > +	die "updating $channel->{channelurl} failed - sa-update exited with $ret\n" if $ret >= 2;
> > +
> > +	$fresh_updates = 1 if $ret == 0;
> > +    }
> > +
> > +    return $fresh_updates
> > +}
> > +
> >  1;
> > -- 
> > 2.20.1
> > 
> > 
> > 
> > _______________________________________________
> > pmg-devel mailing list
> > pmg-devel@lists.proxmox.com
> > https://lists.proxmox.com/cgi-bin/mailman/listinfo/pmg-devel
> > 
> > 
> >   

[pmg-devel] [PATCH pmg-api 4/4] pmg-daily: run sa-update for local channels

[Stoiko Ivanov]

Signed-off-by: Stoiko Ivanov <s.ivanov@proxmox.com>
---
 src/bin/pmg-daily | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/src/bin/pmg-daily b/src/bin/pmg-daily
index 32ccb95..f5b0650 100755
--- a/src/bin/pmg-daily
+++ b/src/bin/pmg-daily
@@ -73,12 +73,21 @@ if (my $http_proxy = $cfg->get('admin', 'http_proxy')) {
 }
 
 # update spamassassin rules
+my $restart_filter = 0;
 if (system('sa-update') == 0) {
     # if the exit code is 0, new updates were downloaded
     # then restart the pmg-smtp-filter to load the new rules
-    PMG::Utils::service_cmd('pmg-smtp-filter', 'restart');
+    $restart_filter = 1;
 }
 
+eval {
+    $restart_filter ||= PMG::Utils::update_local_spamassassin_channels(0);
+};
+if (my $err = $@) {
+    syslog('err', $err);
+}
+
+PMG::Utils::service_cmd('pmg-smtp-filter', 'restart') if $restart_filter;
 # run bayes database maintainance
 system('sa-learn --force-expire >/dev/null 2>&1');
 
-- 
2.20.1

Re: [pmg-devel] [PATCH pmg-api 4/4] pmg-daily: run sa-update for local channels

[Fabian Grünbichler]

On December 16, 2020 6:18 pm, Stoiko Ivanov wrote:
> Signed-off-by: Stoiko Ivanov <s.ivanov@proxmox.com>
> ---
>  src/bin/pmg-daily | 11 ++++++++++-
>  1 file changed, 10 insertions(+), 1 deletion(-)
> 
> diff --git a/src/bin/pmg-daily b/src/bin/pmg-daily
> index 32ccb95..f5b0650 100755
> --- a/src/bin/pmg-daily
> +++ b/src/bin/pmg-daily
> @@ -73,12 +73,21 @@ if (my $http_proxy = $cfg->get('admin', 'http_proxy')) {
>  }
>  
>  # update spamassassin rules
> +my $restart_filter = 0;
>  if (system('sa-update') == 0) {
>      # if the exit code is 0, new updates were downloaded
>      # then restart the pmg-smtp-filter to load the new rules
> -    PMG::Utils::service_cmd('pmg-smtp-filter', 'restart');
> +    $restart_filter = 1;
>  }
>  
> +eval {
> +    $restart_filter ||= PMG::Utils::update_local_spamassassin_channels(0);
> +};
> +if (my $err = $@) {
> +    syslog('err', $err);
> +}

could just be

syslog(..) if $@

> +
> +PMG::Utils::service_cmd('pmg-smtp-filter', 'restart') if $restart_filter;
>  # run bayes database maintainance
>  system('sa-learn --force-expire >/dev/null 2>&1');
>  
> -- 
> 2.20.1
> 
> 
> 
> _______________________________________________
> pmg-devel mailing list
> pmg-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pmg-devel
> 
> 
>