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

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

[Stoiko Ivanov]

v1->v2:
* addressed Fabian's feedback (where appropriate more details are added to the
individual patches)

original cover-letter:

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             | 58 +++++++++++++++++++++++
 src/bin/pmg-daily            |  9 +++-
 3 files changed, 118 insertions(+), 38 deletions(-)

-- 
2.20.1

[pmg-devel] [PATCH pmg-api v2 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>
---
v1->v2:
* added anchors to all regex-matches

 src/PMG/Utils.pm | 27 +++++++++++++++++++++++++++
 1 file changed, 27 insertions(+)

diff --git a/src/PMG/Utils.pm b/src/PMG/Utils.pm
index d3fae9e..ba6e839 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 =~ /(?:^|\n)(-----BEGIN PGP PUBLIC KEY BLOCK-----.+-----END PGP PUBLIC KEY BLOCK-----)(?:\n|$)/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

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

[Stoiko Ivanov]

Signed-off-by: Stoiko Ivanov <s.ivanov@proxmox.com>
---
v1->v2:
* changed the transliteration (tr///) to an equivalent more common s///

 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..6b9f8f9 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) = @_;
+
+	    # see sa-update source:
+	    my $channel_file_base = $channel;
+	    $channel_file_base =~ s/[^A-Za-z0-9-]+/_/g;
+	    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] [PATCH pmg-api v2 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, 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.

Importing gpg-keys is also done with individual sa-update invocations,
because sa-update only imports the last present --import argument
(wrong use of Getopt::Long)

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

diff --git a/src/PMG/API2/SpamAssassin.pm b/src/PMG/API2/SpamAssassin.pm
index 6b9f8f9..fa638c4 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 ba6e839..9992c64 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,32 @@ 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 $localchannels = PMG::Utils::local_spamassassin_channels();
+    for my $channel (@$localchannels) {
+	my $importcmd = [$SAUPDATE, '--import', $channel->{filename}];
+	push @$importcmd, '-v' if $verbose;
+
+	print "Importing gpg key from $channel->{filename}\n" if $verbose;
+	PVE::Tools::run_command($importcmd);
+    }
+
+    my $fresh_updates = 0;
+
+    for my $channel (@$localchannels) {
+	my $cmd = [$SAUPDATE, '--channel', $channel->{channelurl}, '--gpgkey', $channel->{keyid}];
+	push @$cmd, '-v' if $verbose;
+
+	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

[pmg-devel] [PATCH pmg-api v2 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 | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/src/bin/pmg-daily b/src/bin/pmg-daily
index 32ccb95..8865c94 100755
--- a/src/bin/pmg-daily
+++ b/src/bin/pmg-daily
@@ -73,12 +73,19 @@ 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);
+};
+syslog('err', "$@") 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

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

[Thomas Lamprecht]

On 30.12.20 18:15, 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.

I'd like to have a, at least minimal, test for new parsers. Ideally a OK one and
problematic one per success or error condition, respectively.

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

[Thomas Lamprecht]

On 30.12.20 18:15, 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, 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.
> 
> Importing gpg-keys is also done with individual sa-update invocations,
> because sa-update only imports the last present --import argument
> (wrong use of Getopt::Long)
> 
> Signed-off-by: Stoiko Ivanov <s.ivanov@proxmox.com>
> ---
>  src/PMG/API2/SpamAssassin.pm |  6 +++---
>  src/PMG/Utils.pm             | 31 +++++++++++++++++++++++++++++++
>  2 files changed, 34 insertions(+), 3 deletions(-)
> 
> diff --git a/src/PMG/API2/SpamAssassin.pm b/src/PMG/API2/SpamAssassin.pm
> index 6b9f8f9..fa638c4 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);

can we please avoid the $SAUPDATE variable completely, either just directly use
"sa-updates" with no path prefixed as this is much cleaner and safer to do, we
have a sensible PATH env after all and having fixed paths made problems in the
past (especially with usr-merge in built systems).

If you must, add a sa_update helper method doing the actual run_command and some
possible other common things, maybe even having a clean, not overly general,
parameter signature.

>  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 ba6e839..9992c64 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,32 @@ 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 $localchannels = PMG::Utils::local_spamassassin_channels();
> +    for my $channel (@$localchannels) {
> +	my $importcmd = [$SAUPDATE, '--import', $channel->{filename}];
> +	push @$importcmd, '-v' if $verbose;
> +
> +	print "Importing gpg key from $channel->{filename}\n" if $verbose;
> +	PVE::Tools::run_command($importcmd);
> +    }
> +
> +    my $fresh_updates = 0;
> +
> +    for my $channel (@$localchannels) {
> +	my $cmd = [$SAUPDATE, '--channel', $channel->{channelurl}, '--gpgkey', $channel->{keyid}];
> +	push @$cmd, '-v' if $verbose;
> +
> +	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;
> 

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

[Thomas Lamprecht]

On 30.12.20 18:15, Stoiko Ivanov wrote:
> Signed-off-by: Stoiko Ivanov <s.ivanov@proxmox.com>
> ---
> v1->v2:
> * changed the transliteration (tr///) to an equivalent more common s///
> 
>  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..6b9f8f9 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) = @_;
> +
> +	    # see sa-update source:
> +	    my $channel_file_base = $channel;
> +	    $channel_file_base =~ s/[^A-Za-z0-9-]+/_/g;
> +	    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";

I know it was already there, but this  mayresults in a undef comparison
when checking `if ($newversion > $version) {` below.
I'd do a separate followup with that being fixed, e.g., by either prepending the
if check with a `!defined($version) || ...` check or setting it to -1000 or so.

> +		}
> +	    }
> +	    # 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];

this reads a bit weird, but can be OK as $check_channel does seem to catch
all actual possible errors.

>      }});
>  
>  __PACKAGE__->register_method({
> 

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

[Stoiko Ivanov]

On Fri, 15 Jan 2021 11:02:42 +0100
Thomas Lamprecht <t.lamprecht@proxmox.com> wrote:

> On 30.12.20 18:15, Stoiko Ivanov wrote:
> > Signed-off-by: Stoiko Ivanov <s.ivanov@proxmox.com>
> > ---
> > v1->v2:
> > * changed the transliteration (tr///) to an equivalent more common s///
> > 
> >  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..6b9f8f9 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) = @_;
> > +
> > +	    # see sa-update source:
> > +	    my $channel_file_base = $channel;
> > +	    $channel_file_base =~ s/[^A-Za-z0-9-]+/_/g;
> > +	    my $channelfile = "${sa_update_dir}${channel_file_base}.cf";
> > +
> > +	    my $mtime = -1;
> > +	    my $version = -1;
$version gets initialized to -1 here...
> > +	    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";  
> 
> I know it was already there, but this  mayresults in a undef comparison
> when checking `if ($newversion > $version) {` below.
> I'd do a separate followup with that being fixed, e.g., by either prepending the
> if check with a `!defined($version) || ...` check or setting it to -1000 or so.
so it should not be undefined if the metadata is not matching?
(I quickly tested in on my system - and if I drop the -1 I get a undef
comparison in the logs - but it works as is)
or am I missing something?

> 
> > +		}
> > +	    }
> > +	    # 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];  
> 
> this reads a bit weird, but can be OK as $check_channel does seem to catch
> all actual possible errors.
> 
> >      }});
> >  
> >  __PACKAGE__->register_method({
> >   
> 
> 

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

[Thomas Lamprecht]

On 18.01.21 20:47, Stoiko Ivanov wrote:
>>> +	my $check_channel = sub {
>>> +	    my ($channel) = @_;
>>> +
>>> +	    # see sa-update source:
>>> +	    my $channel_file_base = $channel;
>>> +	    $channel_file_base =~ s/[^A-Za-z0-9-]+/_/g;
>>> +	    my $channelfile = "${sa_update_dir}${channel_file_base}.cf";
>>> +
>>> +	    my $mtime = -1;
>>> +	    my $version = -1;
> $version gets initialized to -1 here...
>>> +	    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";  
>> I know it was already there, but this  mayresults in a undef comparison
>> when checking `if ($newversion > $version) {` below.
>> I'd do a separate followup with that being fixed, e.g., by either prepending the
>> if check with a `!defined($version) || ...` check or setting it to -1000 or so.
> so it should not be undefined if the metadata is not matching?
> (I quickly tested in on my system - and if I drop the -1 I get a undef
> comparison in the logs - but it works as is)
> or am I missing something?
> 

Ok, my bad then - sorry!