[pmg-devel] [PATCH pmg-api v5 04/11] create new users for the rule db

[Stoiko Ivanov]

On Fri,  4 Apr 2025 15:14:31 +0200
Maximiliano Sandoval <m.sandoval at proxmox.com> wrote:

> These users will be used by the pmg-smtp-filter and pmgpolicy. We add a
> helper function to open the rule_db as a given user.
> 
> Signed-off-by: Maximiliano Sandoval <m.sandoval at proxmox.com>
> ---
>  debian/postinst         |  8 ++++++++
>  src/PMG/DBTools.pm      | 26 ++++++++++++++++++++++++--
>  src/bin/pmg-smtp-filter |  4 ++--
>  src/bin/pmgpolicy       |  6 +++---
>  4 files changed, 37 insertions(+), 7 deletions(-)
> 
> diff --git a/debian/postinst b/debian/postinst
> index 98444d22..708350ec 100644
> --- a/debian/postinst
> +++ b/debian/postinst
> @@ -48,6 +48,10 @@ migrate_apt_auth_conf() {
>      fi
>  }
>  
> +migrate_pmg_smtp_filter() {
> +    pmgdb update >/dev/null 2>&1 &
> +}
> +
a) `pmgdb update` gets called below upon upgrade anyways
b) the creation of the users happens in upgradedb, which gets called on
`pmgdb init` (which also gets called upon upgrade below) - but I see that
the naming does invite misunderstandings!


>  case "$1" in
>      triggered)
>  
> @@ -67,6 +71,10 @@ case "$1" in
>  
>          if test ! -e /proxmox_install_mode ; then
>  
> +            if test -n "$2" && dpkg --compare-versions "$2" 'lt' '9.0.0'; then
> +                migrate_pmg_smtp_filter
> +            fi
> +
>              pmgconf="/etc/pmg/pmg.conf"
>              if test -n "$2" && dpkg --compare-versions "$2" 'lt' '8.0.2'; then
>                  # on upgrade add pre 8.0 default values for advfilter, use_awl and use_bayes
> diff --git a/src/PMG/DBTools.pm b/src/PMG/DBTools.pm
> index 1acc0cb6..7b5181ab 100644
> --- a/src/PMG/DBTools.pm
> +++ b/src/PMG/DBTools.pm
> @@ -38,7 +38,7 @@ sub cgreylist_merge_sql {
>  }
>  
>  sub open_ruledb {
> -    my ($database, $host, $port) = @_;
> +    my ($database, $host, $port, $user) = @_;
>  
>      $port //= 5432;
>  
> @@ -74,13 +74,19 @@ sub open_ruledb {
>  	return $rdb;
>      } else {
>  	my $dsn = "DBI:Pg:dbname=$database;host=/var/run/postgresql;port=$port";
> -	my $user = $> == 0 ? 'root' : 'www-data';
> +	$user //= $> == 0 ? 'root' : 'www-data';
>  	my $dbh = DBI->connect($dsn, $user, undef, { PrintError => 0, RaiseError => 1 });
>  
>  	return $dbh;
>      }
>  }
>  
> +sub open_ruledb_as {
> +    my ($database, $user) = @_;
> +
> +    open_ruledb($database, undef, undef, $user);
could we infer the user from `$</$UID` (see `man perlvar` and
https://perldoc.perl.org/functions/getpwuid)?


> +}
> +
>  sub delete_ruledb {
>      my ($dbname) = @_;
>  
> @@ -609,6 +615,22 @@ sub upgradedb {
>  	}
>      }
>  
> +    foreach my $user ('pmgpolicy', 'pmg-smtp-filter') {
> +	eval {
> +	    my $silent_opts = { outfunc => sub {}, errfunc => sub {} };
> +	    postgres_admin_cmd('createuser',  $silent_opts, '-D', $user);
> +
> +	    $dbh->begin_work;
> +	    $dbh->do("GRANT USAGE, SELECT ON ALL SEQUENCES IN SCHEMA public TO \"$user\"");
> +	    $dbh->do("GRANT SELECT, INSERT, UPDATE, DELETE ON ALL TABLES IN SCHEMA public TO \"$user\"");
this grants quite many rights to the users - which is probably ok (as the
services currently have them as well and both need to write in some tables
(statistics for both, greylisting for pmgpolicy at least (from memory))
but please do mention why you think it's ok in the commit-message/a
comment - else it's hard to reason why this was ok a few years from now.

> +	    $dbh->commit;
this line here - or the use of `do`, or the `createuser` call failing after
the user is created once  and this line causes the following
warning in the journal upon booting:
```
Aug 14 00:41:15 pmg-on-trixietest systemd[1]: Starting pmgsync.service - Sync Proxmox Configuration...
Aug 14 00:41:15 pmg-on-trixietest pmgdb[246]: Analyzing/Upgrading existing Databases...rollback ineffective with AutoCommit enabled at /usr/share/perl5/PMG/DBTools.pm line 666.
Aug 14 00:41:15 pmg-on-trixietest pmgdb[246]: rollback ineffective with AutoCommit enabled at /usr/share/perl5/PMG/DBTools.pm line 666.
Aug 14 00:41:15 pmg-on-trixietest pmgdb[246]: done
```



> +
> +	};
> +	if (my $err = $@) {
> +	    $dbh->rollback;
> +	}
> +    }
> +
>      foreach my $table (keys %$tables) {
>  	eval { $dbh->do("ANALYZE $table"); };
>  	warn $@ if $@;
> diff --git a/src/bin/pmg-smtp-filter b/src/bin/pmg-smtp-filter
> index fcaaffc5..e95e6458 100755
> --- a/src/bin/pmg-smtp-filter
> +++ b/src/bin/pmg-smtp-filter
> @@ -387,7 +387,7 @@ sub load_config {
>      PMG::MailQueue::create_spooldirs($self->{cinfo}->{local}->{cid});
>  
>      eval {
> -	my $dbh = PMG::DBTools::open_ruledb ($database);
> +	my $dbh = PMG::DBTools::open_ruledb_as($database, 'pmg-smtp-filter');
>  	$self->{ruledb} = PMG::RuleDB->new ($dbh);
>  
>  	# load rulecache
> @@ -538,7 +538,7 @@ sub run_dequeue {
>  
>      my $cinfo = PVE::INotify::read_file("cluster.conf");
>  
> -    my $dbh = eval { PMG::DBTools::open_ruledb($database) };
> +    my $dbh = eval { PMG::DBTools::open_ruledb_as($database, 'pmg-smtp-filter') };
>      if ($err = $@) {
>  	$self->log (0, "ERROR: $err");
>  	return;
> diff --git a/src/bin/pmgpolicy b/src/bin/pmgpolicy
> index 3f976ff7..92fb6f89 100755
> --- a/src/bin/pmgpolicy
> +++ b/src/bin/pmgpolicy
> @@ -142,7 +142,7 @@ sub run_dequeue {
>      my $dbh;
>  
>      eval {
> -	$dbh = PMG::DBTools::open_ruledb($database);
> +	$dbh = PMG::DBTools::open_ruledb_as($database, 'pmgpolicy');
>      };
>      my $err = $@;
>  
> @@ -343,7 +343,7 @@ sub load_config {
>      my $dbh;
>  
>      eval {
> -	$dbh = PMG::DBTools::open_ruledb($database);
> +	$dbh = PMG::DBTools::open_ruledb_as($database, 'pmgpolicy');
>  	$self->{ruledb} = PMG::RuleDB->new($dbh);
>  	$self->{rulecache} = PMG::RuleCache->new($self->{ruledb});
>      };
> @@ -523,7 +523,7 @@ sub greylist_value {
>  	$self->log(0, 'Database connection broken - trying to reconnect');
>  	my $dbh;
>  	eval {
> -	    $dbh = PMG::DBTools::open_ruledb($database);
> +	    $dbh = PMG::DBTools::open_ruledb_as($database, 'pmgpolicy');
>  	};
>  	my $err = $@;
>  	if ($err) {