[pmg-devel] [PATCH pmg-api 06/12] DKIM sign outbound mail if configured

[Dominik Csapak]

some comments inline

On 10/7/19 9:28 PM, Stoiko Ivanov wrote:
> The signing is done in the Accept and BCC Actions just before the mail gets
> handed to the outbound postifx process, thus ensuring that all modifications
> done by the rule-system don't invalidate the signature
> 
> The PMG::DKIMSign/DKIM::Signer object is not cached, since subsequent calls to
> the same object lead to invalid signatures.
> 
> Signed-off-by: Stoiko Ivanov <s.ivanov at proxmox.com>
> ---
>   src/PMG/RuleDB/Accept.pm | 16 +++++++++++++++-
>   src/PMG/RuleDB/BCC.pm    | 12 ++++++++++++
>   src/bin/pmg-smtp-filter  | 14 ++++++++++++--
>   3 files changed, 39 insertions(+), 3 deletions(-)
> 
> diff --git a/src/PMG/RuleDB/Accept.pm b/src/PMG/RuleDB/Accept.pm
> index 8e76d8f..b80cb00 100644
> --- a/src/PMG/RuleDB/Accept.pm
> +++ b/src/PMG/RuleDB/Accept.pm
> @@ -89,7 +89,8 @@ sub execute {
>       my ($self, $queue, $ruledb, $mod_group, $targets,
>   	$msginfo, $vars, $marks) = @_;
>   
> -    my $subgroups = $mod_group->subgroups($targets, 1);
> +    my $ro = !$msginfo->{dkim_sign};
> +    my $subgroups = $mod_group->subgroups($targets, $ro);

what exactly is 'ro' i would prefer to have a more meaningful name,
or in this case omit it completely with:

->subgroups($targets, !$msginfo->{dkim_sign})



>   
>       my $rulename = $vars->{RULE} // 'unknown';
>   
> @@ -97,6 +98,19 @@ sub execute {
>   	my ($tg, $entity) = (@$ta[0], @$ta[1]);
>   
>   	PMG::Utils::remove_marks($entity);
> +	if ($msginfo->{dkim_sign}) {
> +	    eval {
> +		my $dkim = PMG::DKIMSign->new($msginfo->{dkim_selector},
> +		    $msginfo->{dkim_sign_all_mail});
> +		$dkim->signing_domain($msginfo->{sender});
> +		$entity->print($dkim);
> +		my $dkimsig = $dkim->create_signature();
> +		$entity->head->add('DKIM-Signature', $dkimsig, 0);
> +	    };
> +	    syslog('warning',
> +		"Could not create DKIM-Signer - disabling Signing: $@") if $@;
> +	}
> +

like we talked off-list, it probably makes more sense to put this into
PMG::DKIMSign so that we do not have to duplicate that code (for below hunk)
and also can add conditional oversigning for some headers
(like content-type)

>   
>   	if ($msginfo->{testmode}) {
>   	    my $fh = $msginfo->{test_fh};
> diff --git a/src/PMG/RuleDB/BCC.pm b/src/PMG/RuleDB/BCC.pm
> index be695f7..661de9d 100644
> --- a/src/PMG/RuleDB/BCC.pm
> +++ b/src/PMG/RuleDB/BCC.pm
> @@ -136,6 +136,18 @@ sub execute {
>   
>   	$entity = $entity->dup();
>   	PMG::Utils::remove_marks($entity);
> +	if ($msginfo->{dkim_sign}) {
> +	    eval {
> +		my $dkim = PMG::DKIMSign->new($msginfo->{dkim_selector},
> +		    $msginfo->{dkim_sign_all_mail});
> +		$dkim->signing_domain($msginfo->{sender});
> +		$entity->print($dkim);
> +		my $dkimsig = $dkim->create_signature();
> +		$entity->head->add('DKIM-Signature', $dkimsig, 0);
> +	    };
> +	    syslog('warning',
> +		"Could not create DKIM-Signer - disabling Signing: $@") if $@;
> +	}
>   
>   	if ($msginfo->{testmode}) {
>   	    my $fh = $msginfo->{test_fh};
> diff --git a/src/bin/pmg-smtp-filter b/src/bin/pmg-smtp-filter
> index 61eaf92..8b39a99 100755
> --- a/src/bin/pmg-smtp-filter
> +++ b/src/bin/pmg-smtp-filter
> @@ -41,6 +41,7 @@ use PMG::Config;
>   use PMG::MailQueue;
>   use PMG::Unpack;
>   use PMG::SMTP;
> +use PMG::DKIMSign;
>   
>   use PMG::Unpack;
>   use PMG::Statistic;
> @@ -369,7 +370,8 @@ sub load_config {
>   	$self->{ruledb}->close ();
>       }
>   
> -    $self->{pmg_cfg} = PMG::Config->new();
> +    my $pmg_cfg =  PMG::Config->new();
> +    $self->{pmg_cfg} = $pmg_cfg;

seems left over since you do not use $pmg_cfg here?

>       $self->{cinfo} = PVE::INotify::read_file("cluster.conf");
>   
>       # create spool directories
> @@ -618,7 +620,8 @@ sub handle_smtp {
>       $msginfo->{test_fh} = PMG::AtomicFile->new("testresult.out", "w")
>   	if $opt_testmode;
>   
> -    $msginfo->{trusted} = $self->{trusted};
> +    my $trusted = $self->{trusted};
> +    $msginfo->{trusted} = $trusted;

does this change between this hunk and the next in $msginfo ?

if not, i would simply check $msginfo->{trusted} below

>   
>   
>   # PHASE 1 - save incoming mail (already done)
> @@ -639,6 +642,13 @@ sub handle_smtp {
>   	$msginfo->{xforward} = $smtp->{xforward};
>   	$msginfo->{targets} = $smtp->{to};
>   
> +	my $dkim_sign = $trusted && $pmg_cfg->get('admin', 'dkim_sign');
> +	if ($dkim_sign) {
> +	    $msginfo->{dkim_sign} = $dkim_sign;
> +	    $msginfo->{dkim_sign_all_mail} = $pmg_cfg->get('admin', 'dkim_sign_all_mail');
> +	    $msginfo->{dkim_selector} = $pmg_cfg->get('admin', 'dkim_selector');
> +	}
> +
>   	$msginfo->{hostname} = PVE::INotify::nodename();
>   	my $resolv = PVE::INotify::read_file('resolvconf');
>   
>