From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by lists.proxmox.com (Postfix) with ESMTPS id 33123938BB for ; Tue, 20 Feb 2024 14:03:51 +0100 (CET) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 0AAF97221 for ; Tue, 20 Feb 2024 14:03:51 +0100 (CET) Received: from proxmox-new.maurer-it.com (proxmox-new.maurer-it.com [94.136.29.106]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by firstgate.proxmox.com (Proxmox) with ESMTPS for ; Tue, 20 Feb 2024 14:03:50 +0100 (CET) Received: from proxmox-new.maurer-it.com (localhost.localdomain [127.0.0.1]) by proxmox-new.maurer-it.com (Proxmox) with ESMTP id BA5B243F47 for ; Tue, 20 Feb 2024 14:03:49 +0100 (CET) Date: Tue, 20 Feb 2024 14:03:47 +0100 From: Stoiko Ivanov To: Dominik Csapak Cc: pmg-devel@lists.proxmox.com Message-ID: <20240220140347.1b6aba0e@rosa.proxmox.com> In-Reply-To: <20240209125440.2572239-7-d.csapak@proxmox.com> References: <20240209125440.2572239-1-d.csapak@proxmox.com> <20240209125440.2572239-7-d.csapak@proxmox.com> X-Mailer: Claws Mail 4.1.1 (GTK 3.24.38; x86_64-pc-linux-gnu) MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit X-SPAM-LEVEL: Spam detection results: 0 AWL 0.085 Adjusted score from AWL reputation of From: address BAYES_00 -1.9 Bayes spam probability is 0 to 1% DMARC_MISSING 0.1 Missing DMARC policy KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record T_SCC_BODY_TEXT_LINE -0.01 - URIBL_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to URIBL was blocked. See http://wiki.apache.org/spamassassin/DnsBlocklists#dnsbl-block for more information. [rule.id, ruledb.pm, dbtools.pm, objectgrouphelpers.pm, rules.pm] Subject: Re: [pmg-devel] [PATCH pmg-api 06/12] add rule attributes and/invert (for each relevant type) X-BeenThere: pmg-devel@lists.proxmox.com X-Mailman-Version: 2.1.29 Precedence: list List-Id: Proxmox Mail Gateway development discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 20 Feb 2024 13:03:51 -0000 similarly here deleting the rule-attributes upon rule-deletion seems missing (also most suggestions from the last patch also apply, but I try to mention them explicitly inline as well) comments inline: On Fri, 9 Feb 2024 13:54:30 +0100 Dominik Csapak wrote: > like with the objectgroups, add an attributes table for groups, and an > 'and'/'invert' attribute for each relevant object type > (what/when/from/to). > > This is intended to modify the behaviour for the matching regarding > object groups, so that one has more choice in the logical matching. > > Signed-off-by: Dominik Csapak > --- > src/PMG/API2/ObjectGroupHelpers.pm | 8 ++ > src/PMG/API2/Rules.pm | 53 ++++++++- > src/PMG/DBTools.pm | 15 +++ > src/PMG/RuleDB.pm | 169 +++++++++++++++++++++++------ > 4 files changed, 209 insertions(+), 36 deletions(-) > > diff --git a/src/PMG/API2/ObjectGroupHelpers.pm b/src/PMG/API2/ObjectGroupHelpers.pm > index a08a6a3..3060157 100644 > --- a/src/PMG/API2/ObjectGroupHelpers.pm > +++ b/src/PMG/API2/ObjectGroupHelpers.pm > @@ -31,6 +31,14 @@ sub format_rule { > active => $rule->{active}, > direction => $rule->{direction}, > }; > + my $types = [qw(what when from to)]; > + my $attributes = [qw(and invert)]; > + for my $type ($types->@*) { > + for my $attribute ($attributes->@*) { > + my $opt = "${type}-${attribute}"; > + $data->{$opt} = $rule->{$opt} if defined($rule->{$opt}); > + } > + } > > $cond_create_group->($data, 'from', $from); > $cond_create_group->($data, 'to', $to); > diff --git a/src/PMG/API2/Rules.pm b/src/PMG/API2/Rules.pm > index c48370f..1ebadc2 100644 > --- a/src/PMG/API2/Rules.pm > +++ b/src/PMG/API2/Rules.pm > @@ -149,6 +149,54 @@ my $rule_params = { > type => 'boolean', > optional => 1, > }, > + 'what-and' => { > + description => "Flag to 'and' combine WHAT group matches.", > + type => 'boolean', > + default => 0, > + optional => 1, > + }, > + 'what-invert' => { > + description => "Flag to invert WHAT group matches.", > + type => 'boolean', > + default => 0, > + optional => 1, > + }, > + 'when-and' => { > + description => "Flag to 'and' combine WHEN group matches.", > + type => 'boolean', > + default => 0, > + optional => 1, > + }, > + 'when-invert' => { > + description => "Flag to invert WHEN group matches.", > + type => 'boolean', > + default => 0, > + optional => 1, > + }, > + 'from-and' => { > + description => "Flag to 'and' combine FROM group matches.", > + type => 'boolean', > + default => 0, > + optional => 1, > + }, > + 'from-invert' => { > + description => "Flag to invert FROM group matches.", > + type => 'boolean', > + default => 0, > + optional => 1, > + }, > + 'to-and' => { > + description => "Flag to 'and' combine TO group matches.", > + type => 'boolean', > + default => 0, > + optional => 1, > + }, > + 'to-invert' => { > + description => "Flag to invert TO group matches.", > + type => 'boolean', > + default => 0, > + optional => 1, > + }, > }; > > sub get_rule_params { > @@ -203,7 +251,10 @@ __PACKAGE__->register_method ({ > > my $rule = $rdb->load_rule($id); > > - for my $key (qw(name active direction priority)) { > + my $keys = ["name", "priority"]; > + push $keys->@*, keys get_rule_params()->%*; > + > + for my $key ($keys->@*) { > $rule->{$key} = $param->{$key} if defined($param->{$key}); > } > > diff --git a/src/PMG/DBTools.pm b/src/PMG/DBTools.pm > index 0d3d9c3..605eb71 100644 > --- a/src/PMG/DBTools.pm > +++ b/src/PMG/DBTools.pm > @@ -295,6 +295,18 @@ my $userprefs_ctablecmd = <<__EOD; > > __EOD > > +my $rule_attributes_cmd = <<__EOD; > + CREATE TABLE Rule_Attributes ( > + Rule_ID INTEGER NOT NULL, we could consider adding a foreign key constraint on Rule.id here > + Name VARCHAR(20) NOT NULL, > + Value BYTEA NULL, if we're only storing booleans, we could use the proper column type for this > + PRIMARY KEY (Rule_ID, Name) > + ); > + > + CREATE INDEX Rule_Attributes_Rule_ID_Index ON Rule_Attributes(Rule_ID); > + > +__EOD > + > my $object_group_attributes_cmd = <<__EOD; > CREATE TABLE Objectgroup_Attributes ( > Objectgroup_ID INTEGER NOT NULL, > @@ -452,6 +464,8 @@ sub create_ruledb { > > $virusinfo_stat_ctablecmd; > > + $rule_attributes_cmd; > + > $object_group_attributes_cmd; > EOD > ); > @@ -508,6 +522,7 @@ sub upgradedb { > 'CStatistic', $cstatistic_ctablecmd, > 'ClusterInfo', $clusterinfo_ctablecmd, > 'VirusInfo', $virusinfo_stat_ctablecmd, > + 'Rule_Attributes', $rule_attributes_cmd, > 'Objectgroup_Attributes', $object_group_attributes_cmd, > }; > > diff --git a/src/PMG/RuleDB.pm b/src/PMG/RuleDB.pm > index df9e526..70770a8 100644 > --- a/src/PMG/RuleDB.pm > +++ b/src/PMG/RuleDB.pm > @@ -665,6 +665,35 @@ sub delete_object { > return 1; > } > > +sub update_rule_attributes { > + my ($self, $rule) = @_; > + > + my $types = [qw(what when from to)]; > + my $attributes = [qw(and invert)]; > + > + for my $type ($types->@*) { > + for my $attribute ($attributes->@*) { > + my $prop = "$type-$attribute"; > + > + # only save the values if they're set to 1 > + if ($rule->{$prop}) { > + $self->{dbh}->do( > + "INSERT INTO Rule_Attributes (Rule_ID, Name, Value) " . > + "VALUES (?, ?, ?) ". > + "ON CONFLICT (Rule_ID, Name) DO UPDATE SET Value = ?", undef, > + $rule->{id}, $prop, $rule->{$prop}, $rule->{$prop}, > + ); > + } else { > + $self->{dbh}->do( > + "DELETE FROM Rule_Attributes " . > + "WHERE Rule_ID = ? AND Name = ?", undef, > + $rule->{id}, $prop, > + ); > + } > + } > + } > +} > + > sub save_rule { > my ($self, $rule) = @_; > > @@ -679,28 +708,53 @@ sub save_rule { > > my $rulename = encode('UTF-8', $rule->{name}); > if (defined($rule->{id})) { > + $self->{dbh}->begin_work; > > - $self->{dbh}->do( > - "UPDATE Rule " . > - "SET Name = ?, Priority = ?, Active = ?, Direction = ? " . > - "WHERE ID = ?", undef, > - $rulename, $rule->{priority}, $rule->{active}, > - $rule->{direction}, $rule->{id}); > + eval { > + $self->{dbh}->do( > + "UPDATE Rule " . > + "SET Name = ?, Priority = ?, Active = ?, Direction = ? " . > + "WHERE ID = ?", undef, > + $rulename, $rule->{priority}, $rule->{active}, > + $rule->{direction}, $rule->{id}); > + > + $self->update_rule_attributes($rule); > > - return $rule->{id}; > + $self->{dbh}->commit; > + }; > > + if (my $err = $@) { > + $self->{dbh}->rollback; > + syslog('err', $err); > + return undef; > + } > } else { > - my $sth = $self->{dbh}->prepare( > - "INSERT INTO Rule (Name, Priority, Active, Direction) " . > - "VALUES (?, ?, ?, ?);"); > + $self->{dbh}->begin_work; > > - $sth->execute($rulename, $rule->priority, $rule->active, > - $rule->direction); > + eval { > + my $sth = $self->{dbh}->prepare( > + "INSERT INTO Rule (Name, Priority, Active, Direction) " . > + "VALUES (?, ?, ?, ?);"); > + > + $sth->execute($rulename, $rule->priority, $rule->active, > + $rule->direction); > + > + > + $rule->{id} = PMG::Utils::lastid($self->{dbh}, 'rule_id_seq'); > + > + $self->update_rule_attributes($rule); > > - return $rule->{id} = PMG::Utils::lastid($self->{dbh}, 'rule_id_seq'); > + $self->{dbh}->commit; > + }; > + > + if (my $err = $@) { > + $self->{dbh}->rollback; > + syslog('err', $err); > + return undef; > + } > } > > - return undef; > + return $rule->{id}; > } > > sub delete_rule { > @@ -826,24 +880,58 @@ sub rule_remove_group { > return 1; > } > > +sub load_rule_attributes { > + my ($self, $rule) = @_; > + > + my $types = [qw(what when from to)]; > + my $attributes = [qw(and invert)]; > + > + my $attribute_sth = $self->{dbh}->prepare("SELECT * FROM Rule_Attributes WHERE Rule_ID = ?"); > + $attribute_sth->execute($rule->{id}); > + > + ATTRIBUTES_LOOP: > + while (my $ref = $attribute_sth->fetchrow_hashref()) { > + for my $type ($types->@*) { > + for my $attribute ($attributes->@*) { > + my $prop = "$type-$attribute"; > + if ($ref->{name} eq $prop) { > + $rule->{$prop} = $ref->{value}; > + next ATTRIBUTES_LOOP; would a simple regex match for the attribute here not work equally well, and prevent the GOTO/next LABEL? if ($ref->{name} =~ /(?:what|when|from|to)-(?:and|invert)/) (also we might consider a die if it does not match the expected pattern?) > + } > + } > + } > + } > +} > + > sub load_rule { > my ($self, $id) = @_; > > defined($id) || die "undefined id: ERROR"; > > - my $sth = $self->{dbh}->prepare( > - "SELECT * FROM Rule where id = ? ORDER BY Priority DESC"); > + $self->{dbh}->begin_work; transaction probably not needed for selects > > - my $rules = (); > + my $rule; > > - $sth->execute($id); > + eval { > + my $sth = $self->{dbh}->prepare( > + "SELECT * FROM Rule where id = ? ORDER BY Priority DESC"); > > - my $ref = $sth->fetchrow_hashref(); > - die "rule '$id' does not exist\n" if !defined($ref); > + $sth->execute($id); > + > + my $ref = $sth->fetchrow_hashref(); > + die "rule '$id' does not exist\n" if !defined($ref); > > - my $rule = PMG::RuleDB::Rule->new($ref->{name}, $ref->{priority}, > - $ref->{active}, $ref->{direction}); > - $rule->{id} = $ref->{id}; > + $rule = PMG::RuleDB::Rule->new($ref->{name}, $ref->{priority}, > + $ref->{active}, $ref->{direction}); > + $rule->{id} = $ref->{id}; > + > + $self->load_rule_attributes($rule); > + }; > + my $err = $@; > + > + $self->{dbh}->rollback; unconditionally rollback > + > + die $err if $err; not needed if no eval/transaction > > return $rule; > } > @@ -851,22 +939,33 @@ sub load_rule { > sub load_rules { > my ($self) = @_; > > - my $sth = $self->{dbh}->prepare( > - "SELECT * FROM Rule ORDER BY Priority DESC"); > - > my $rules = (); > > - $sth->execute(); > + $self->{dbh}->begin_work; transaction probably not needed for selects > > - while (my $ref = $sth->fetchrow_hashref()) { > - my $rulename = PMG::Utils::try_decode_utf8($ref->{name}); > - my $rule = PMG::RuleDB::Rule->new($rulename, $ref->{priority}, > - $ref->{active}, $ref->{direction}); > - $rule->{id} = $ref->{id}; > - push @$rules, $rule; > - } > + eval { > + my $sth = $self->{dbh}->prepare( > + "SELECT * FROM Rule ORDER BY Priority DESC"); > > - $sth->finish(); > + $sth->execute(); > + > + while (my $ref = $sth->fetchrow_hashref()) { > + my $rulename = PMG::Utils::try_decode_utf8($ref->{name}); > + my $rule = PMG::RuleDB::Rule->new($rulename, $ref->{priority}, > + $ref->{active}, $ref->{direction}); > + $rule->{id} = $ref->{id}; > + #$self->load_rule_attributes($rule); this line is commented out? > + > + push @$rules, $rule; > + } > + > + $sth->finish(); > + }; > + my $err = $@; > + > + $self->{dbh}->rollback; unconditionally rollback > + > + die $err if $err; not needed if no eval/transaction > > return $rules; > }